drm/i915: Avoid concurrent access when marking the device as idle/busy
authorChris Wilson <chris@chris-wilson.co.uk>
Sat, 21 Jul 2012 11:31:41 +0000 (12:31 +0100)
committerDaniel Vetter <daniel.vetter@ffwll.ch>
Wed, 25 Jul 2012 16:23:56 +0000 (18:23 +0200)
As suggested by Daniel, rip out the independent timers for device and
crtc busyness and integrate the manual powermanagement of the display
engine into the GEM core and its request tracking. The benefits are that
the code is a lot smaller, fewer moving parts and should fit more neatly
into the overall activity tracking of the driver.

v2: Complete overhaul and removal of the racy timers and workers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
drivers/gpu/drm/i915/i915_drv.h
drivers/gpu/drm/i915/i915_gem.c
drivers/gpu/drm/i915/i915_gem_execbuffer.c
drivers/gpu/drm/i915/intel_display.c
drivers/gpu/drm/i915/intel_drv.h
drivers/gpu/drm/i915/intel_pm.c

index 700dc838c57fe70bd3b5f05904fa614618e643f4..f1765893da6065fc7b61bf16b42c7dafa913a2a4 100644 (file)
@@ -785,9 +785,6 @@ typedef struct drm_i915_private {
        bool lvds_downclock_avail;
        /* indicates the reduced downclock for LVDS*/
        int lvds_downclock;
-       struct work_struct idle_work;
-       struct timer_list idle_timer;
-       bool busy;
        u16 orig_clock;
        int child_dev_num;
        struct child_device_config *child_dev;
index f26e2b201badbd43d950660774bba985142337f6..b274810eaeabcad2d259c837116d4ca917fb0ded 100644 (file)
@@ -1462,11 +1462,14 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
        struct drm_device *dev = obj->base.dev;
        struct drm_i915_private *dev_priv = dev->dev_private;
 
-       list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
-
        BUG_ON(obj->base.write_domain & ~I915_GEM_GPU_DOMAINS);
        BUG_ON(!obj->active);
 
+       if (obj->pin_count) /* are we a framebuffer? */
+               intel_mark_fb_idle(obj);
+
+       list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
+
        list_del_init(&obj->ring_list);
        obj->ring = NULL;
 
@@ -1602,9 +1605,11 @@ i915_add_request(struct intel_ring_buffer *ring,
                                  jiffies +
                                  msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
                }
-               if (was_empty)
+               if (was_empty) {
                        queue_delayed_work(dev_priv->wq,
                                           &dev_priv->mm.retire_work, HZ);
+                       intel_mark_busy(dev_priv->dev);
+               }
        }
 
        return 0;
@@ -1810,6 +1815,8 @@ i915_gem_retire_work_handler(struct work_struct *work)
 
        if (!dev_priv->mm.suspended && !idle)
                queue_delayed_work(dev_priv->wq, &dev_priv->mm.retire_work, HZ);
+       if (idle)
+               intel_mark_idle(dev);
 
        mutex_unlock(&dev->struct_mutex);
 }
index 6be1a8920a8493dac24678217b20e9750483b399..25b2c54e1261e4736de842768330ae837a4a2301 100644 (file)
@@ -767,13 +767,11 @@ i915_gem_execbuffer_move_to_active(struct list_head *objects,
                        obj->dirty = 1;
                        obj->last_write_seqno = seqno;
                        if (obj->pin_count) /* check for potential scanout */
-                               intel_mark_busy(ring->dev, obj);
+                               intel_mark_fb_busy(obj);
                }
 
                trace_i915_gem_object_change_domain(obj, old_read, old_write);
        }
-
-       intel_mark_busy(ring->dev, NULL);
 }
 
 static void
index 93d9934ba328747f9466355e61fede9c459e53ba..b463829b92eb8a872ff39989f043033011eb0517 100644 (file)
@@ -5860,46 +5860,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
        return mode;
 }
 
-#define GPU_IDLE_TIMEOUT 500 /* ms */
-
-/* When this timer fires, we've been idle for awhile */
-static void intel_gpu_idle_timer(unsigned long arg)
-{
-       struct drm_device *dev = (struct drm_device *)arg;
-       drm_i915_private_t *dev_priv = dev->dev_private;
-
-       if (!list_empty(&dev_priv->mm.active_list)) {
-               /* Still processing requests, so just re-arm the timer. */
-               mod_timer(&dev_priv->idle_timer, jiffies +
-                         msecs_to_jiffies(GPU_IDLE_TIMEOUT));
-               return;
-       }
-
-       dev_priv->busy = false;
-       queue_work(dev_priv->wq, &dev_priv->idle_work);
-}
-
-#define CRTC_IDLE_TIMEOUT 1000 /* ms */
-
-static void intel_crtc_idle_timer(unsigned long arg)
-{
-       struct intel_crtc *intel_crtc = (struct intel_crtc *)arg;
-       struct drm_crtc *crtc = &intel_crtc->base;
-       drm_i915_private_t *dev_priv = crtc->dev->dev_private;
-       struct intel_framebuffer *intel_fb;
-
-       intel_fb = to_intel_framebuffer(crtc->fb);
-       if (intel_fb && intel_fb->obj->active) {
-               /* The framebuffer is still being accessed by the GPU. */
-               mod_timer(&intel_crtc->idle_timer, jiffies +
-                         msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
-               return;
-       }
-
-       intel_crtc->busy = false;
-       queue_work(dev_priv->wq, &dev_priv->idle_work);
-}
-
 static void intel_increase_pllclock(struct drm_crtc *crtc)
 {
        struct drm_device *dev = crtc->dev;
@@ -5929,10 +5889,6 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
                if (dpll & DISPLAY_RATE_SELECT_FPA1)
                        DRM_DEBUG_DRIVER("failed to upclock LVDS!\n");
        }
-
-       /* Schedule downclock */
-       mod_timer(&intel_crtc->idle_timer, jiffies +
-                 msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
 }
 
 static void intel_decrease_pllclock(struct drm_crtc *crtc)
@@ -5971,89 +5927,48 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
 
 }
 
-/**
- * intel_idle_update - adjust clocks for idleness
- * @work: work struct
- *
- * Either the GPU or display (or both) went idle.  Check the busy status
- * here and adjust the CRTC and GPU clocks as necessary.
- */
-static void intel_idle_update(struct work_struct *work)
+void intel_mark_busy(struct drm_device *dev)
+{
+       intel_sanitize_pm(dev);
+       i915_update_gfx_val(dev->dev_private);
+}
+
+void intel_mark_idle(struct drm_device *dev)
 {
-       drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
-                                                   idle_work);
-       struct drm_device *dev = dev_priv->dev;
+       intel_sanitize_pm(dev);
+}
+
+void intel_mark_fb_busy(struct drm_i915_gem_object *obj)
+{
+       struct drm_device *dev = obj->base.dev;
        struct drm_crtc *crtc;
-       struct intel_crtc *intel_crtc;
 
        if (!i915_powersave)
                return;
 
-       mutex_lock(&dev->struct_mutex);
-
-       i915_update_gfx_val(dev_priv);
-
        list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-               /* Skip inactive CRTCs */
                if (!crtc->fb)
                        continue;
 
-               intel_crtc = to_intel_crtc(crtc);
-               if (!intel_crtc->busy)
-                       intel_decrease_pllclock(crtc);
+               if (to_intel_framebuffer(crtc->fb)->obj == obj)
+                       intel_increase_pllclock(crtc);
        }
-
-
-       mutex_unlock(&dev->struct_mutex);
 }
 
-/**
- * intel_mark_busy - mark the GPU and possibly the display busy
- * @dev: drm device
- * @obj: object we're operating on
- *
- * Callers can use this function to indicate that the GPU is busy processing
- * commands.  If @obj matches one of the CRTC objects (i.e. it's a scanout
- * buffer), we'll also mark the display as busy, so we know to increase its
- * clock frequency.
- */
-void intel_mark_busy(struct drm_device *dev, struct drm_i915_gem_object *obj)
+void intel_mark_fb_idle(struct drm_i915_gem_object *obj)
 {
-       drm_i915_private_t *dev_priv = dev->dev_private;
-       struct drm_crtc *crtc = NULL;
-       struct intel_framebuffer *intel_fb;
-       struct intel_crtc *intel_crtc;
-
-       if (!drm_core_check_feature(dev, DRIVER_MODESET))
-               return;
-
-       if (!dev_priv->busy) {
-               intel_sanitize_pm(dev);
-               dev_priv->busy = true;
-       } else
-               mod_timer(&dev_priv->idle_timer, jiffies +
-                         msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+       struct drm_device *dev = obj->base.dev;
+       struct drm_crtc *crtc;
 
-       if (obj == NULL)
+       if (!i915_powersave)
                return;
 
        list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
                if (!crtc->fb)
                        continue;
 
-               intel_crtc = to_intel_crtc(crtc);
-               intel_fb = to_intel_framebuffer(crtc->fb);
-               if (intel_fb->obj == obj) {
-                       if (!intel_crtc->busy) {
-                               /* Non-busy -> busy, upclock */
-                               intel_increase_pllclock(crtc);
-                               intel_crtc->busy = true;
-                       } else {
-                               /* Busy -> busy, put off timer */
-                               mod_timer(&intel_crtc->idle_timer, jiffies +
-                                         msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
-                       }
-               }
+               if (to_intel_framebuffer(crtc->fb)->obj == obj)
+                       intel_decrease_pllclock(crtc);
        }
 }
 
@@ -6512,7 +6427,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
                goto cleanup_pending;
 
        intel_disable_fbc(dev);
-       intel_mark_busy(dev, obj);
+       intel_mark_fb_busy(obj);
        mutex_unlock(&dev->struct_mutex);
 
        trace_i915_flip_request(intel_crtc->plane, obj);
@@ -6678,11 +6593,6 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
        }
 
        drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
-
-       intel_crtc->busy = false;
-
-       setup_timer(&intel_crtc->idle_timer, intel_crtc_idle_timer,
-                   (unsigned long)intel_crtc);
 }
 
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
@@ -7265,10 +7175,6 @@ void intel_modeset_init(struct drm_device *dev)
        /* Just disable it once at startup */
        i915_disable_vga(dev);
        intel_setup_outputs(dev);
-
-       INIT_WORK(&dev_priv->idle_work, intel_idle_update);
-       setup_timer(&dev_priv->idle_timer, intel_gpu_idle_timer,
-                   (unsigned long)dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
@@ -7319,14 +7225,6 @@ void intel_modeset_cleanup(struct drm_device *dev)
        /* flush any delayed tasks or pending work */
        flush_scheduled_work();
 
-       /* Shut off idle work before the crtcs get freed. */
-       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
-               intel_crtc = to_intel_crtc(crtc);
-               del_timer_sync(&intel_crtc->idle_timer);
-       }
-       del_timer_sync(&dev_priv->idle_timer);
-       cancel_work_sync(&dev_priv->idle_work);
-
        drm_mode_config_cleanup(dev);
 }
 
index 2846f5e8cca35c7e459e4016927bf77a938cdfef..8c7f48310842a52c69ff88401ad8f85b12ff5a23 100644 (file)
@@ -156,8 +156,6 @@ struct intel_crtc {
        int dpms_mode;
        bool active; /* is the crtc on? independent of the dpms mode */
        bool primary_disabled; /* is the crtc obscured by a plane? */
-       bool busy; /* is scanout buffer being updated frequently? */
-       struct timer_list idle_timer;
        bool lowfreq_avail;
        struct intel_overlay *overlay;
        struct intel_unpin_work *unpin_work;
@@ -373,8 +371,10 @@ extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
                            bool is_sdvob);
 extern void intel_dvo_init(struct drm_device *dev);
 extern void intel_tv_init(struct drm_device *dev);
-extern void intel_mark_busy(struct drm_device *dev,
-                           struct drm_i915_gem_object *obj);
+extern void intel_mark_busy(struct drm_device *dev);
+extern void intel_mark_idle(struct drm_device *dev);
+extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
+extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
 extern bool intel_lvds_init(struct drm_device *dev);
 extern void intel_dp_init(struct drm_device *dev, int output_reg,
                          enum port port);
index 94aabcaa3a673a46aa0e9361133aa3331ed8e453..85d1b1c57df27a9553bfdae3592287b8b1c5f84e 100644 (file)
@@ -3075,14 +3075,17 @@ EXPORT_SYMBOL_GPL(i915_gpu_lower);
 bool i915_gpu_busy(void)
 {
        struct drm_i915_private *dev_priv;
+       struct intel_ring_buffer *ring;
        bool ret = false;
+       int i;
 
        spin_lock(&mchdev_lock);
        if (!i915_mch_dev)
                goto out_unlock;
        dev_priv = i915_mch_dev;
 
-       ret = dev_priv->busy;
+       for_each_ring(ring, dev_priv, i)
+               ret |= !list_empty(&ring->request_list);
 
 out_unlock:
        spin_unlock(&mchdev_lock);