[PATCH 1/2] seqno

[PATCH 1/2] seqno

[Chris Wilson]

---
 drivers/gpu/drm/i915/i915_gem.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1fadb02..4f270e8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3599,6 +3599,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	 */
 	args->busy = obj->active;
 	if (args->busy) {
+		u32 seqno;
+
 		/* Unconditionally flush objects, even when the gpu still uses this
 		 * object. Userspace calling this function indicates that it wants to
 		 * use this buffer rather sooner than later, so issuing the required
@@ -3617,7 +3619,10 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 		 * timer or when irqs are actually unmasked, and our
 		 * working set ends up being larger than required.
 		 */
-		i915_gem_retire_requests_ring(obj->ring, 0);
+		seqno = obj->last_rendering_seqno;
+		if (!i915_seqno_passed(obj->ring->last_seqno, seqno))
+			seqno = 0;
+		i915_gem_retire_requests_ring(obj->ring, seqno);
 		args->busy = obj->active;
 	}
 
-- 
1.7.10

[PATCH 2/2] drm/i915: Avoid concurrent access when marking the device as idle/busy

[Chris Wilson]

Whilst marking the device as active is protect by struct_mutex, we would
mark the individual components as not-busy without any locking, leading
to potential confusion and missed powersaving (or even performance
loss). Move the softirq accesses to an atomic and defer the actual
update until we acquire the struct_mutex in the worker.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c      |    2 +-
 drivers/gpu/drm/i915/i915_drv.h      |    3 +-
 drivers/gpu/drm/i915/intel_display.c |   86 +++++++++++++++-------------------
 drivers/gpu/drm/i915/intel_drv.h     |    1 -
 4 files changed, 42 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ee25584..ee402c5 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1701,7 +1701,7 @@ bool i915_gpu_busy(void)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	ret = dev_priv->busy;
+	ret = !!(dev_priv->busy & (1 << 31));
 
 out_unlock:
 	spin_unlock(&mchdev_lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3815d9a..47e5722 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -778,7 +778,8 @@ typedef struct drm_i915_private {
 	int lvds_downclock;
 	struct work_struct idle_work;
 	struct timer_list idle_timer;
-	bool busy;
+	unsigned busy;
+	atomic_t idle;
 	u16 orig_clock;
 	int child_dev_num;
 	struct child_device_config *child_dev;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d5aa2d2..1428e6e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5519,14 +5519,7 @@ 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;
+	atomic_set_mask(1 << 31, &dev_priv->idle);
 	queue_work(dev_priv->wq, &dev_priv->idle_work);
 }
 
@@ -5535,19 +5528,9 @@ static void intel_gpu_idle_timer(unsigned long arg)
 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;
+	drm_i915_private_t *dev_priv = intel_crtc->base.dev->dev_private;
 
-	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;
+	atomic_set_mask(1 << intel_crtc->pipe, &dev_priv->idle);
 	queue_work(dev_priv->wq, &dev_priv->idle_work);
 }
 
@@ -5651,26 +5634,32 @@ static void intel_idle_update(struct work_struct *work)
 						    idle_work);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_crtc *crtc;
-	struct intel_crtc *intel_crtc;
-
-	if (!i915_powersave)
-		return;
+	unsigned idle;
 
 	mutex_lock(&dev->struct_mutex);
 
-	i915_update_gfx_val(dev_priv);
+	idle = atomic_xchg(&dev_priv->idle, 0);
+	if (idle & (1 << 31))
+		intel_sanitize_pm(dev);
 
+	if (!i915_powersave)
+		goto unlock;
+
+	i915_update_gfx_val(dev_priv);
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc;
+
 		/* Skip inactive CRTCs */
 		if (!crtc->fb)
 			continue;
 
 		intel_crtc = to_intel_crtc(crtc);
-		if (!intel_crtc->busy)
+		if (idle & (1 << intel_crtc->pipe))
 			intel_decrease_pllclock(crtc);
 	}
 
-
+unlock:
+	dev_priv->busy &= ~idle;
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -5687,38 +5676,42 @@ static void intel_idle_update(struct work_struct *work)
 void intel_mark_busy(struct drm_device *dev, 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;
+	struct drm_crtc *crtc;
+	unsigned busy = 0;
 
-	if (!dev_priv->busy) {
+	if ((dev_priv->busy & (1 << 31)) == 0) {
 		intel_sanitize_pm(dev);
-		dev_priv->busy = true;
-	} else
-		mod_timer(&dev_priv->idle_timer, jiffies +
-			  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+		busy |= 1 << 31;
+	}
+
+	if (!i915_powersave)
+		goto out;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc;
+
 		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) {
+		if (to_intel_framebuffer(crtc->fb)->obj == obj) {
+			if ((dev_priv->busy & (1 << intel_crtc->pipe)) == 0) {
 				/* 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));
+				busy |= 1 << intel_crtc->pipe;
 			}
+
+			mod_timer(&intel_crtc->idle_timer, jiffies +
+				  msecs_to_jiffies(CRTC_IDLE_TIMEOUT));
 		}
 	}
+
+out:
+	mod_timer(&dev_priv->idle_timer, jiffies +
+		  msecs_to_jiffies(GPU_IDLE_TIMEOUT));
+
+	atomic_clear_mask(busy, &dev_priv->idle);
+	dev_priv->busy |= busy;
 }
 
 static void intel_crtc_destroy(struct drm_crtc *crtc)
@@ -6311,7 +6304,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);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0bd6d8a..64c2e7d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -165,7 +165,6 @@ struct intel_crtc {
 	u8 lut_r[256], lut_g[256], lut_b[256];
 	int dpms_mode;
 	bool active; /* is the crtc on? independent of the dpms mode */
-	bool busy; /* is scanout buffer being updated frequently? */
 	struct timer_list idle_timer;
 	bool lowfreq_avail;
 	struct intel_overlay *overlay;
-- 
1.7.10

Re: [PATCH 1/2] seqno

[Chris Wilson]

Ignore these, wrong pair of patches.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre