All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Introduce for_each_ring() macro
@ 2012-05-09 22:27 Chris Wilson
  2012-05-10 21:21 ` Chris Wilson
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2012-05-09 22:27 UTC (permalink / raw)
  To: intel-gfx

In many places we wish to iterate over the rings associated with the
GPU, so refactor them to use a common macro.

Along the way, there are a few code removals that should be side-effect
free and some rearrangement which should only have a cosmetic impact,
such as error-state.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |    9 ++---
 drivers/gpu/drm/i915/i915_drv.c       |   10 ++---
 drivers/gpu/drm/i915/i915_drv.h       |    9 +++--
 drivers/gpu/drm/i915/i915_gem.c       |   33 +++++++---------
 drivers/gpu/drm/i915/i915_gem_evict.c |   14 +++----
 drivers/gpu/drm/i915/i915_irq.c       |   69 +++++++++++++--------------------
 drivers/gpu/drm/i915/intel_pm.c       |   13 ++++---
 7 files changed, 69 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 950f72a..eb2b3c2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -699,6 +699,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	struct drm_device *dev = error_priv->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_error_state *error = error_priv->error;
+	struct intel_ring_buffer *ring;
 	int i, j, page, offset, elt;
 
 	if (!error) {
@@ -706,7 +707,6 @@ static int i915_error_state(struct seq_file *m, void *unused)
 		return 0;
 	}
 
-
 	seq_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
 		   error->time.tv_usec);
 	seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
@@ -722,11 +722,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
 		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
 	}
 
-	i915_ring_error_state(m, dev, error, RCS);
-	if (HAS_BLT(dev))
-		i915_ring_error_state(m, dev, error, BCS);
-	if (HAS_BSD(dev))
-		i915_ring_error_state(m, dev, error, VCS);
+	for_each_ring(ring, dev_priv, i)
+		i915_ring_error_state(m, dev, error, i);
 
 	if (error->active_bo)
 		print_error_buffers(m, "Active",
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 981f3a1..084ef29 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -884,15 +884,15 @@ int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->mm.suspended) {
+		struct intel_ring_buffer *ring;
+		int i;
+
 		dev_priv->mm.suspended = 0;
 
 		i915_gem_init_swizzling(dev);
 
-		dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
-		if (HAS_BSD(dev))
-		    dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
-		if (HAS_BLT(dev))
-		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
+		for_each_ring(ring, dev_priv, i)
+			ring->init(ring, ring->obj);
 
 		i915_gem_init_ppgtt(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9bbaf5c..3c07e43 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -408,9 +408,7 @@ typedef struct drm_i915_private {
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
-	uint32_t last_acthd;
-	uint32_t last_acthd_bsd;
-	uint32_t last_acthd_blt;
+	uint32_t last_acthd[I915_NUM_RINGS];
 	uint32_t last_instdone;
 	uint32_t last_instdone1;
 
@@ -815,6 +813,11 @@ typedef struct drm_i915_private {
 	struct drm_property *force_audio_property;
 } drm_i915_private_t;
 
+/* Iterate over initialised rings */
+#define for_each_ring(ring__, dev_priv__, i__) \
+	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
+		if (((ring__) = &(dev_priv__)->ring[(i__)])->obj)
+
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
 	HDMI_AUDIO_OFF,			/* force turn off HDMI audio */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d972dbf..158c8d0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1660,10 +1660,11 @@ void i915_gem_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		i915_gem_reset_ring_lists(dev_priv, &dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		i915_gem_reset_ring_lists(dev_priv, ring);
 
 	/* Remove anything from the flushing lists. The GPU cache is likely
 	 * to be lost on reset along with the data, so simply move the
@@ -1768,10 +1769,11 @@ void
 i915_gem_retire_requests(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		i915_gem_retire_requests_ring(&dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		i915_gem_retire_requests_ring(ring);
 }
 
 static void
@@ -1779,6 +1781,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv;
 	struct drm_device *dev;
+	struct intel_ring_buffer *ring;
 	bool idle;
 	int i;
 
@@ -1798,9 +1801,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	 * objects indefinitely.
 	 */
 	idle = true;
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_ring_buffer *ring = &dev_priv->ring[i];
-
+	for_each_ring(ring, dev_priv, i) {
 		if (!list_empty(&ring->gpu_write_list)) {
 			struct drm_i915_gem_request *request;
 			int ret;
@@ -2144,10 +2145,11 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
 int i915_gpu_idle(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int ret, i;
 
 	/* Flush everything onto the inactive list. */
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for_each_ring(ring, dev_priv, i) {
 		ret = i915_ring_idle(&dev_priv->ring[i]);
 		if (ret)
 			return ret;
@@ -3470,9 +3472,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
 		/* GFX_MODE is per-ring on gen7+ */
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		ring = &dev_priv->ring[i];
-
+	for_each_ring(ring, dev_priv, i) {
 		if (INTEL_INFO(dev)->gen >= 7)
 			I915_WRITE(RING_MODE_GEN7(ring),
 				   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
@@ -3588,10 +3588,11 @@ void
 i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		intel_cleanup_ring_buffer(&dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		intel_cleanup_ring_buffer(ring);
 }
 
 int
@@ -3599,7 +3600,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret, i;
+	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return 0;
@@ -3621,10 +3622,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	BUG_ON(!list_empty(&dev_priv->mm.active_list));
 	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
 	BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		BUG_ON(!list_empty(&dev_priv->ring[i].active_list));
-		BUG_ON(!list_empty(&dev_priv->ring[i].request_list));
-	}
 	mutex_unlock(&dev->struct_mutex);
 
 	ret = drm_irq_install(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 3bcf045..ae7c24e 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj, *next;
 	bool lists_empty;
-	int ret,i;
+	int ret;
 
 	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
 		       list_empty(&dev_priv->mm.flushing_list) &&
@@ -178,17 +178,13 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 
 	trace_i915_gem_evict_everything(dev, purgeable_only);
 
-	ret = i915_gpu_idle(dev);
-	if (ret)
-		return ret;
-
 	/* The gpu_idle will flush everything in the write domain to the
 	 * active list. Then we must move everything off the active list
 	 * with retire requests.
 	 */
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
-			return -EBUSY;
+	ret = i915_gpu_idle(dev);
+	if (ret)
+		return ret;
 
 	i915_gem_retire_requests(dev);
 
@@ -203,5 +199,5 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 		}
 	}
 
-	return ret;
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 324431e..bcf34d3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1022,15 +1022,11 @@ static void i915_gem_record_rings(struct drm_device *dev,
 				  struct drm_i915_error_state *error)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	struct drm_i915_gem_request *request;
 	int i, count;
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_ring_buffer *ring = &dev_priv->ring[i];
-
-		if (ring->obj == NULL)
-			continue;
-
+	for_each_ring(ring, dev_priv, i) {
 		i915_record_ring_state(dev, error, ring);
 
 		error->ring[i].batchbuffer =
@@ -1295,6 +1291,8 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 void i915_handle_error(struct drm_device *dev, bool wedged)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
+	int i;
 
 	i915_capture_error_state(dev);
 	i915_report_and_clear_eir(dev);
@@ -1306,11 +1304,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
 		/*
 		 * Wakeup waiting processes so they don't hang
 		 */
-		wake_up_all(&dev_priv->ring[RCS].irq_queue);
-		if (HAS_BSD(dev))
-			wake_up_all(&dev_priv->ring[VCS].irq_queue);
-		if (HAS_BLT(dev))
-			wake_up_all(&dev_priv->ring[BCS].irq_queue);
+		for_each_ring(ring, dev_priv, i)
+			wake_up_all(&ring->irq_queue);
 	}
 
 	queue_work(dev_priv->wq, &dev_priv->error_work);
@@ -1515,11 +1510,6 @@ ring_last_seqno(struct intel_ring_buffer *ring)
 
 static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
 {
-	/* We don't check whether the ring even exists before calling this
-	 * function. Hence check whether it's initialized. */
-	if (ring->obj == NULL)
-		return true;
-
 	if (list_empty(&ring->request_list) ||
 	    i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
 		/* Issue a wake-up to catch stuck h/w. */
@@ -1553,26 +1543,25 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	if (dev_priv->hangcheck_count++ > 1) {
+		bool hung = true;
+
 		DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
 		i915_handle_error(dev, true);
 
 		if (!IS_GEN2(dev)) {
+			struct intel_ring_buffer *ring;
+			int i;
+
 			/* Is the chip hanging on a WAIT_FOR_EVENT?
 			 * If so we can simply poke the RB_WAIT bit
 			 * and break the hang. This should work on
 			 * all but the second generation chipsets.
 			 */
-			if (kick_ring(&dev_priv->ring[RCS]))
-				return false;
-
-			if (HAS_BSD(dev) && kick_ring(&dev_priv->ring[VCS]))
-				return false;
-
-			if (HAS_BLT(dev) && kick_ring(&dev_priv->ring[BCS]))
-				return false;
+			for_each_ring(ring, dev_priv, i)
+				hung &= !kick_ring(ring);
 		}
 
-		return true;
+		return hung;
 	}
 
 	return false;
@@ -1588,16 +1577,23 @@ void i915_hangcheck_elapsed(unsigned long data)
 {
 	struct drm_device *dev = (struct drm_device *)data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint32_t acthd, instdone, instdone1, acthd_bsd, acthd_blt;
-	bool err = false;
+	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
+	struct intel_ring_buffer *ring;
+	bool err = false, idle;
+	int i;
 
 	if (!i915_enable_hangcheck)
 		return;
 
+	memset(acthd, 0, sizeof(acthd));
+	idle = true;
+	for_each_ring(ring, dev_priv, i) {
+	    idle &= i915_hangcheck_ring_idle(ring, &err);
+	    acthd[i] = intel_ring_get_active_head(ring);
+	}
+
 	/* If all work is done then ACTHD clearly hasn't advanced. */
-	if (i915_hangcheck_ring_idle(&dev_priv->ring[RCS], &err) &&
-	    i915_hangcheck_ring_idle(&dev_priv->ring[VCS], &err) &&
-	    i915_hangcheck_ring_idle(&dev_priv->ring[BCS], &err)) {
+	if (idle) {
 		if (err) {
 			if (i915_hangcheck_hung(dev))
 				return;
@@ -1616,15 +1612,8 @@ void i915_hangcheck_elapsed(unsigned long data)
 		instdone = I915_READ(INSTDONE_I965);
 		instdone1 = I915_READ(INSTDONE1);
 	}
-	acthd = intel_ring_get_active_head(&dev_priv->ring[RCS]);
-	acthd_bsd = HAS_BSD(dev) ?
-		intel_ring_get_active_head(&dev_priv->ring[VCS]) : 0;
-	acthd_blt = HAS_BLT(dev) ?
-		intel_ring_get_active_head(&dev_priv->ring[BCS]) : 0;
 
-	if (dev_priv->last_acthd == acthd &&
-	    dev_priv->last_acthd_bsd == acthd_bsd &&
-	    dev_priv->last_acthd_blt == acthd_blt &&
+	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
 	    dev_priv->last_instdone == instdone &&
 	    dev_priv->last_instdone1 == instdone1) {
 		if (i915_hangcheck_hung(dev))
@@ -1632,9 +1621,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 	} else {
 		dev_priv->hangcheck_count = 0;
 
-		dev_priv->last_acthd = acthd;
-		dev_priv->last_acthd_bsd = acthd_bsd;
-		dev_priv->last_acthd_blt = acthd_blt;
+		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
 		dev_priv->last_instdone = instdone;
 		dev_priv->last_instdone1 = instdone1;
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8bbfb75..bb7eb09 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2291,6 +2291,7 @@ int intel_enable_rc6(const struct drm_device *dev)
 
 void gen6_enable_rps(struct drm_i915_private *dev_priv)
 {
+	struct intel_ring_buffer *ring;
 	u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
 	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 	u32 pcu_mbox, rc6_mask = 0;
@@ -2325,8 +2326,8 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		I915_WRITE(RING_MAX_IDLE(dev_priv->ring[i].mmio_base), 10);
+	for_each_ring(ring, dev_priv, i)
+		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
@@ -3013,17 +3014,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 n;
+	int i;
 
 	spin_lock(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	for (n = 0; n < I915_NUM_RINGS; n++)
-		if (dev_priv->ring[n].obj)
-			ret |= !list_empty(&dev_priv->ring[n].request_list);
+	for_each_ring(ring, dev_priv, i)
+		ret |= !list_empty(&ring->request_list);
 
 out_unlock:
 	spin_unlock(&mchdev_lock);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] drm/i915: Introduce for_each_ring() macro
  2012-05-09 22:27 [PATCH] drm/i915: Introduce for_each_ring() macro Chris Wilson
@ 2012-05-10 21:21 ` Chris Wilson
  2012-05-10 22:39   ` Ben Widawsky
  0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2012-05-10 21:21 UTC (permalink / raw)
  To: intel-gfx

In many places we wish to iterate over the rings associated with the
GPU, so refactor them to use a common macro.

Along the way, there are a few code removals that should be side-effect
free and some rearrangement which should only have a cosmetic impact,
such as error-state.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |    9 ++---
 drivers/gpu/drm/i915/i915_drv.c       |   10 ++---
 drivers/gpu/drm/i915/i915_drv.h       |    9 +++--
 drivers/gpu/drm/i915/i915_gem.c       |   33 +++++++---------
 drivers/gpu/drm/i915/i915_gem_evict.c |   14 +++----
 drivers/gpu/drm/i915/i915_irq.c       |   69 +++++++++++++--------------------
 drivers/gpu/drm/i915/intel_pm.c       |    6 ++-
 7 files changed, 66 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 950f72a..eb2b3c2 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -699,6 +699,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	struct drm_device *dev = error_priv->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_error_state *error = error_priv->error;
+	struct intel_ring_buffer *ring;
 	int i, j, page, offset, elt;
 
 	if (!error) {
@@ -706,7 +707,6 @@ static int i915_error_state(struct seq_file *m, void *unused)
 		return 0;
 	}
 
-
 	seq_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
 		   error->time.tv_usec);
 	seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
@@ -722,11 +722,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
 		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
 	}
 
-	i915_ring_error_state(m, dev, error, RCS);
-	if (HAS_BLT(dev))
-		i915_ring_error_state(m, dev, error, BCS);
-	if (HAS_BSD(dev))
-		i915_ring_error_state(m, dev, error, VCS);
+	for_each_ring(ring, dev_priv, i)
+		i915_ring_error_state(m, dev, error, i);
 
 	if (error->active_bo)
 		print_error_buffers(m, "Active",
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5898be9..2b7967d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -893,15 +893,15 @@ int i915_reset(struct drm_device *dev)
 	 */
 	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
 			!dev_priv->mm.suspended) {
+		struct intel_ring_buffer *ring;
+		int i;
+
 		dev_priv->mm.suspended = 0;
 
 		i915_gem_init_swizzling(dev);
 
-		dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
-		if (HAS_BSD(dev))
-		    dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
-		if (HAS_BLT(dev))
-		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
+		for_each_ring(ring, dev_priv, i)
+			ring->init(ring, ring->obj);
 
 		i915_gem_init_ppgtt(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 83a557c..c2e0732 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -410,9 +410,7 @@ typedef struct drm_i915_private {
 #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
 	struct timer_list hangcheck_timer;
 	int hangcheck_count;
-	uint32_t last_acthd;
-	uint32_t last_acthd_bsd;
-	uint32_t last_acthd_blt;
+	uint32_t last_acthd[I915_NUM_RINGS];
 	uint32_t last_instdone;
 	uint32_t last_instdone1;
 
@@ -820,6 +818,11 @@ typedef struct drm_i915_private {
 	struct drm_property *force_audio_property;
 } drm_i915_private_t;
 
+/* Iterate over initialised rings */
+#define for_each_ring(ring__, dev_priv__, i__) \
+	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
+		if (((ring__) = &(dev_priv__)->ring[(i__)])->obj)
+
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
 	HDMI_AUDIO_OFF,			/* force turn off HDMI audio */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 44a5f24..99961ce 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1655,10 +1655,11 @@ void i915_gem_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		i915_gem_reset_ring_lists(dev_priv, &dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		i915_gem_reset_ring_lists(dev_priv, ring);
 
 	/* Remove anything from the flushing lists. The GPU cache is likely
 	 * to be lost on reset along with the data, so simply move the
@@ -1763,10 +1764,11 @@ void
 i915_gem_retire_requests(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		i915_gem_retire_requests_ring(&dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		i915_gem_retire_requests_ring(ring);
 }
 
 static void
@@ -1774,6 +1776,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 {
 	drm_i915_private_t *dev_priv;
 	struct drm_device *dev;
+	struct intel_ring_buffer *ring;
 	bool idle;
 	int i;
 
@@ -1793,9 +1796,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
 	 * objects indefinitely.
 	 */
 	idle = true;
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_ring_buffer *ring = &dev_priv->ring[i];
-
+	for_each_ring(ring, dev_priv, i) {
 		if (!list_empty(&ring->gpu_write_list)) {
 			struct drm_i915_gem_request *request;
 			int ret;
@@ -2137,10 +2138,11 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
 int i915_gpu_idle(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int ret, i;
 
 	/* Flush everything onto the inactive list. */
-	for (i = 0; i < I915_NUM_RINGS; i++) {
+	for_each_ring(ring, dev_priv, i) {
 		ret = i915_ring_idle(&dev_priv->ring[i]);
 		if (ret)
 			return ret;
@@ -3463,9 +3465,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
 		/* GFX_MODE is per-ring on gen7+ */
 	}
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		ring = &dev_priv->ring[i];
-
+	for_each_ring(ring, dev_priv, i) {
 		if (INTEL_INFO(dev)->gen >= 7)
 			I915_WRITE(RING_MODE_GEN7(ring),
 				   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
@@ -3581,10 +3581,11 @@ void
 i915_gem_cleanup_ringbuffer(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	int i;
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		intel_cleanup_ring_buffer(&dev_priv->ring[i]);
+	for_each_ring(ring, dev_priv, i)
+		intel_cleanup_ring_buffer(ring);
 }
 
 int
@@ -3592,7 +3593,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 		       struct drm_file *file_priv)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret, i;
+	int ret;
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return 0;
@@ -3614,10 +3615,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
 	BUG_ON(!list_empty(&dev_priv->mm.active_list));
 	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
 	BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		BUG_ON(!list_empty(&dev_priv->ring[i].active_list));
-		BUG_ON(!list_empty(&dev_priv->ring[i].request_list));
-	}
 	mutex_unlock(&dev->struct_mutex);
 
 	ret = drm_irq_install(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 3bcf045..ae7c24e 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj, *next;
 	bool lists_empty;
-	int ret,i;
+	int ret;
 
 	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
 		       list_empty(&dev_priv->mm.flushing_list) &&
@@ -178,17 +178,13 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 
 	trace_i915_gem_evict_everything(dev, purgeable_only);
 
-	ret = i915_gpu_idle(dev);
-	if (ret)
-		return ret;
-
 	/* The gpu_idle will flush everything in the write domain to the
 	 * active list. Then we must move everything off the active list
 	 * with retire requests.
 	 */
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
-			return -EBUSY;
+	ret = i915_gpu_idle(dev);
+	if (ret)
+		return ret;
 
 	i915_gem_retire_requests(dev);
 
@@ -203,5 +199,5 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 		}
 	}
 
-	return ret;
+	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2a062d7..cc4a633 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1022,15 +1022,11 @@ static void i915_gem_record_rings(struct drm_device *dev,
 				  struct drm_i915_error_state *error)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
 	struct drm_i915_gem_request *request;
 	int i, count;
 
-	for (i = 0; i < I915_NUM_RINGS; i++) {
-		struct intel_ring_buffer *ring = &dev_priv->ring[i];
-
-		if (ring->obj == NULL)
-			continue;
-
+	for_each_ring(ring, dev_priv, i) {
 		i915_record_ring_state(dev, error, ring);
 
 		error->ring[i].batchbuffer =
@@ -1295,6 +1291,8 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
 void i915_handle_error(struct drm_device *dev, bool wedged)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_ring_buffer *ring;
+	int i;
 
 	i915_capture_error_state(dev);
 	i915_report_and_clear_eir(dev);
@@ -1306,11 +1304,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
 		/*
 		 * Wakeup waiting processes so they don't hang
 		 */
-		wake_up_all(&dev_priv->ring[RCS].irq_queue);
-		if (HAS_BSD(dev))
-			wake_up_all(&dev_priv->ring[VCS].irq_queue);
-		if (HAS_BLT(dev))
-			wake_up_all(&dev_priv->ring[BCS].irq_queue);
+		for_each_ring(ring, dev_priv, i)
+			wake_up_all(&ring->irq_queue);
 	}
 
 	queue_work(dev_priv->wq, &dev_priv->error_work);
@@ -1515,11 +1510,6 @@ ring_last_seqno(struct intel_ring_buffer *ring)
 
 static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
 {
-	/* We don't check whether the ring even exists before calling this
-	 * function. Hence check whether it's initialized. */
-	if (ring->obj == NULL)
-		return true;
-
 	if (list_empty(&ring->request_list) ||
 	    i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
 		/* Issue a wake-up to catch stuck h/w. */
@@ -1553,26 +1543,25 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	if (dev_priv->hangcheck_count++ > 1) {
+		bool hung = true;
+
 		DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
 		i915_handle_error(dev, true);
 
 		if (!IS_GEN2(dev)) {
+			struct intel_ring_buffer *ring;
+			int i;
+
 			/* Is the chip hanging on a WAIT_FOR_EVENT?
 			 * If so we can simply poke the RB_WAIT bit
 			 * and break the hang. This should work on
 			 * all but the second generation chipsets.
 			 */
-			if (kick_ring(&dev_priv->ring[RCS]))
-				return false;
-
-			if (HAS_BSD(dev) && kick_ring(&dev_priv->ring[VCS]))
-				return false;
-
-			if (HAS_BLT(dev) && kick_ring(&dev_priv->ring[BCS]))
-				return false;
+			for_each_ring(ring, dev_priv, i)
+				hung &= !kick_ring(ring);
 		}
 
-		return true;
+		return hung;
 	}
 
 	return false;
@@ -1588,16 +1577,23 @@ void i915_hangcheck_elapsed(unsigned long data)
 {
 	struct drm_device *dev = (struct drm_device *)data;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	uint32_t acthd, instdone, instdone1, acthd_bsd, acthd_blt;
-	bool err = false;
+	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
+	struct intel_ring_buffer *ring;
+	bool err = false, idle;
+	int i;
 
 	if (!i915_enable_hangcheck)
 		return;
 
+	memset(acthd, 0, sizeof(acthd));
+	idle = true;
+	for_each_ring(ring, dev_priv, i) {
+	    idle &= i915_hangcheck_ring_idle(ring, &err);
+	    acthd[i] = intel_ring_get_active_head(ring);
+	}
+
 	/* If all work is done then ACTHD clearly hasn't advanced. */
-	if (i915_hangcheck_ring_idle(&dev_priv->ring[RCS], &err) &&
-	    i915_hangcheck_ring_idle(&dev_priv->ring[VCS], &err) &&
-	    i915_hangcheck_ring_idle(&dev_priv->ring[BCS], &err)) {
+	if (idle) {
 		if (err) {
 			if (i915_hangcheck_hung(dev))
 				return;
@@ -1616,15 +1612,8 @@ void i915_hangcheck_elapsed(unsigned long data)
 		instdone = I915_READ(INSTDONE_I965);
 		instdone1 = I915_READ(INSTDONE1);
 	}
-	acthd = intel_ring_get_active_head(&dev_priv->ring[RCS]);
-	acthd_bsd = HAS_BSD(dev) ?
-		intel_ring_get_active_head(&dev_priv->ring[VCS]) : 0;
-	acthd_blt = HAS_BLT(dev) ?
-		intel_ring_get_active_head(&dev_priv->ring[BCS]) : 0;
 
-	if (dev_priv->last_acthd == acthd &&
-	    dev_priv->last_acthd_bsd == acthd_bsd &&
-	    dev_priv->last_acthd_blt == acthd_blt &&
+	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
 	    dev_priv->last_instdone == instdone &&
 	    dev_priv->last_instdone1 == instdone1) {
 		if (i915_hangcheck_hung(dev))
@@ -1632,9 +1621,7 @@ void i915_hangcheck_elapsed(unsigned long data)
 	} else {
 		dev_priv->hangcheck_count = 0;
 
-		dev_priv->last_acthd = acthd;
-		dev_priv->last_acthd_bsd = acthd_bsd;
-		dev_priv->last_acthd_blt = acthd_blt;
+		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
 		dev_priv->last_instdone = instdone;
 		dev_priv->last_instdone1 = instdone1;
 	}
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8f8d1da..438ff29 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2326,6 +2326,7 @@ int intel_enable_rc6(const struct drm_device *dev)
 
 void gen6_enable_rps(struct drm_i915_private *dev_priv)
 {
+	struct intel_ring_buffer *ring;
 	u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
 	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 	u32 pcu_mbox, rc6_mask = 0;
@@ -2360,8 +2361,8 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for (i = 0; i < I915_NUM_RINGS; i++)
-		I915_WRITE(RING_MAX_IDLE(dev_priv->ring[i].mmio_base), 10);
+	for_each_ring(ring, dev_priv, i)
+		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
@@ -3048,6 +3049,7 @@ 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;
 
 	spin_lock(&mchdev_lock);
-- 
1.7.10

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Introduce for_each_ring() macro
  2012-05-10 21:21 ` Chris Wilson
@ 2012-05-10 22:39   ` Ben Widawsky
  2012-05-10 23:18     ` Chris Wilson
  2012-05-11  9:00     ` Daniel Vetter
  0 siblings, 2 replies; 6+ messages in thread
From: Ben Widawsky @ 2012-05-10 22:39 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Thu, 10 May 2012 22:21:50 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> In many places we wish to iterate over the rings associated with the
> GPU, so refactor them to use a common macro.
> 
> Along the way, there are a few code removals that should be side-effect
> free and some rearrangement which should only have a cosmetic impact,
> such as error-state.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (if last hunk is removed
and commit messages is amended)
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c   |    9 ++---
>  drivers/gpu/drm/i915/i915_drv.c       |   10 ++---
>  drivers/gpu/drm/i915/i915_drv.h       |    9 +++--
>  drivers/gpu/drm/i915/i915_gem.c       |   33 +++++++---------
>  drivers/gpu/drm/i915/i915_gem_evict.c |   14 +++----
>  drivers/gpu/drm/i915/i915_irq.c       |   69 +++++++++++++--------------------
>  drivers/gpu/drm/i915/intel_pm.c       |    6 ++-
>  7 files changed, 66 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 950f72a..eb2b3c2 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -699,6 +699,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
>  	struct drm_device *dev = error_priv->dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct drm_i915_error_state *error = error_priv->error;
> +	struct intel_ring_buffer *ring;
>  	int i, j, page, offset, elt;
>  
>  	if (!error) {
> @@ -706,7 +707,6 @@ static int i915_error_state(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> -
>  	seq_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
>  		   error->time.tv_usec);
>  	seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
> @@ -722,11 +722,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
>  		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
>  	}
>  
> -	i915_ring_error_state(m, dev, error, RCS);
> -	if (HAS_BLT(dev))
> -		i915_ring_error_state(m, dev, error, BCS);
> -	if (HAS_BSD(dev))
> -		i915_ring_error_state(m, dev, error, VCS);
> +	for_each_ring(ring, dev_priv, i)
> +		i915_ring_error_state(m, dev, error, i);
>  
>  	if (error->active_bo)
>  		print_error_buffers(m, "Active",
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5898be9..2b7967d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -893,15 +893,15 @@ int i915_reset(struct drm_device *dev)
>  	 */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
>  			!dev_priv->mm.suspended) {
> +		struct intel_ring_buffer *ring;
> +		int i;
> +
>  		dev_priv->mm.suspended = 0;
>  
>  		i915_gem_init_swizzling(dev);
>  
> -		dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
> -		if (HAS_BSD(dev))
> -		    dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
> -		if (HAS_BLT(dev))
> -		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
> +		for_each_ring(ring, dev_priv, i)
> +			ring->init(ring, ring->obj);
>  
>  		i915_gem_init_ppgtt(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 83a557c..c2e0732 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -410,9 +410,7 @@ typedef struct drm_i915_private {
>  #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
>  	struct timer_list hangcheck_timer;
>  	int hangcheck_count;
> -	uint32_t last_acthd;
> -	uint32_t last_acthd_bsd;
> -	uint32_t last_acthd_blt;
> +	uint32_t last_acthd[I915_NUM_RINGS];
>  	uint32_t last_instdone;
>  	uint32_t last_instdone1;
>  
> @@ -820,6 +818,11 @@ typedef struct drm_i915_private {
>  	struct drm_property *force_audio_property;
>  } drm_i915_private_t;
>  
> +/* Iterate over initialised rings */
> +#define for_each_ring(ring__, dev_priv__, i__) \
> +	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> +		if (((ring__) = &(dev_priv__)->ring[(i__)])->obj)
> +
>  enum hdmi_force_audio {
>  	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
>  	HDMI_AUDIO_OFF,			/* force turn off HDMI audio */

I'm somewhat disappointed with using ring->obj here. But I do not have a
better suggestion.

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 44a5f24..99961ce 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1655,10 +1655,11 @@ void i915_gem_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
> +	struct intel_ring_buffer *ring;
>  	int i;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		i915_gem_reset_ring_lists(dev_priv, &dev_priv->ring[i]);
> +	for_each_ring(ring, dev_priv, i)
> +		i915_gem_reset_ring_lists(dev_priv, ring);
>  
>  	/* Remove anything from the flushing lists. The GPU cache is likely
>  	 * to be lost on reset along with the data, so simply move the
> @@ -1763,10 +1764,11 @@ void
>  i915_gem_retire_requests(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
>  	int i;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		i915_gem_retire_requests_ring(&dev_priv->ring[i]);
> +	for_each_ring(ring, dev_priv, i)
> +		i915_gem_retire_requests_ring(ring);
>  }
>  
>  static void
> @@ -1774,6 +1776,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  {
>  	drm_i915_private_t *dev_priv;
>  	struct drm_device *dev;
> +	struct intel_ring_buffer *ring;
>  	bool idle;
>  	int i;
>  
> @@ -1793,9 +1796,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
>  	 * objects indefinitely.
>  	 */
>  	idle = true;
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		struct intel_ring_buffer *ring = &dev_priv->ring[i];
> -
> +	for_each_ring(ring, dev_priv, i) {
>  		if (!list_empty(&ring->gpu_write_list)) {
>  			struct drm_i915_gem_request *request;
>  			int ret;
> @@ -2137,10 +2138,11 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
>  int i915_gpu_idle(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
>  	int ret, i;
>  
>  	/* Flush everything onto the inactive list. */
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> +	for_each_ring(ring, dev_priv, i) {
>  		ret = i915_ring_idle(&dev_priv->ring[i]);
>  		if (ret)
>  			return ret;
> @@ -3463,9 +3465,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
>  		/* GFX_MODE is per-ring on gen7+ */
>  	}
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		ring = &dev_priv->ring[i];
> -
> +	for_each_ring(ring, dev_priv, i) {
>  		if (INTEL_INFO(dev)->gen >= 7)
>  			I915_WRITE(RING_MODE_GEN7(ring),
>  				   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> @@ -3581,10 +3581,11 @@ void
>  i915_gem_cleanup_ringbuffer(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
>  	int i;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		intel_cleanup_ring_buffer(&dev_priv->ring[i]);
> +	for_each_ring(ring, dev_priv, i)
> +		intel_cleanup_ring_buffer(ring);
>  }
>  
>  int
> @@ -3592,7 +3593,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
>  		       struct drm_file *file_priv)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	int ret, i;
> +	int ret;
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		return 0;
> @@ -3614,10 +3615,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
>  	BUG_ON(!list_empty(&dev_priv->mm.active_list));
>  	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
>  	BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		BUG_ON(!list_empty(&dev_priv->ring[i].active_list));
> -		BUG_ON(!list_empty(&dev_priv->ring[i].request_list));
> -	}
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	ret = drm_irq_install(dev);

I'm not opposed to this change, but it would have probably been better
to not remove the BUG_ONs.

> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 3bcf045..ae7c24e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj, *next;
>  	bool lists_empty;
> -	int ret,i;
> +	int ret;
>  
>  	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
>  		       list_empty(&dev_priv->mm.flushing_list) &&
> @@ -178,17 +178,13 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  
>  	trace_i915_gem_evict_everything(dev, purgeable_only);
>  
> -	ret = i915_gpu_idle(dev);
> -	if (ret)
> -		return ret;
> -
>  	/* The gpu_idle will flush everything in the write domain to the
>  	 * active list. Then we must move everything off the active list
>  	 * with retire requests.
>  	 */
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
> -			return -EBUSY;
> +	ret = i915_gpu_idle(dev);
> +	if (ret)
> +		return ret;
>  
>  	i915_gem_retire_requests(dev);
>  

Sneaky way to get rid of the warning you never liked... I won't forget
this.

> @@ -203,5 +199,5 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  		}
>  	}
>  
> -	return ret;
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2a062d7..cc4a633 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1022,15 +1022,11 @@ static void i915_gem_record_rings(struct drm_device *dev,
>  				  struct drm_i915_error_state *error)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
>  	struct drm_i915_gem_request *request;
>  	int i, count;
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		struct intel_ring_buffer *ring = &dev_priv->ring[i];
> -
> -		if (ring->obj == NULL)
> -			continue;
> -
> +	for_each_ring(ring, dev_priv, i) {
>  		i915_record_ring_state(dev, error, ring);
>  
>  		error->ring[i].batchbuffer =
> @@ -1295,6 +1291,8 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
>  void i915_handle_error(struct drm_device *dev, bool wedged)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_ring_buffer *ring;
> +	int i;
>  
>  	i915_capture_error_state(dev);
>  	i915_report_and_clear_eir(dev);
> @@ -1306,11 +1304,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
>  		/*
>  		 * Wakeup waiting processes so they don't hang
>  		 */
> -		wake_up_all(&dev_priv->ring[RCS].irq_queue);
> -		if (HAS_BSD(dev))
> -			wake_up_all(&dev_priv->ring[VCS].irq_queue);
> -		if (HAS_BLT(dev))
> -			wake_up_all(&dev_priv->ring[BCS].irq_queue);
> +		for_each_ring(ring, dev_priv, i)
> +			wake_up_all(&ring->irq_queue);
>  	}
>  
>  	queue_work(dev_priv->wq, &dev_priv->error_work);
> @@ -1515,11 +1510,6 @@ ring_last_seqno(struct intel_ring_buffer *ring)
>  
>  static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
>  {
> -	/* We don't check whether the ring even exists before calling this
> -	 * function. Hence check whether it's initialized. */
> -	if (ring->obj == NULL)
> -		return true;
> -
>  	if (list_empty(&ring->request_list) ||
>  	    i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
>  		/* Issue a wake-up to catch stuck h/w. */
> @@ -1553,26 +1543,25 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  
>  	if (dev_priv->hangcheck_count++ > 1) {
> +		bool hung = true;
> +
>  		DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
>  		i915_handle_error(dev, true);
>  
>  		if (!IS_GEN2(dev)) {
> +			struct intel_ring_buffer *ring;
> +			int i;
> +
>  			/* Is the chip hanging on a WAIT_FOR_EVENT?
>  			 * If so we can simply poke the RB_WAIT bit
>  			 * and break the hang. This should work on
>  			 * all but the second generation chipsets.
>  			 */
> -			if (kick_ring(&dev_priv->ring[RCS]))
> -				return false;
> -
> -			if (HAS_BSD(dev) && kick_ring(&dev_priv->ring[VCS]))
> -				return false;
> -
> -			if (HAS_BLT(dev) && kick_ring(&dev_priv->ring[BCS]))
> -				return false;
> +			for_each_ring(ring, dev_priv, i)
> +				hung &= !kick_ring(ring);
>  		}
>  
> -		return true;
> +		return hung;
>  	}
>  
>  	return false;

It's worth noting in the commit message that there is a functional
change here. Before we'd exit out on the first ring kicked, now we
enumerate over all rings regardless.

I'm still a fan of ripping out the kicks completely.

> @@ -1588,16 +1577,23 @@ void i915_hangcheck_elapsed(unsigned long data)
>  {
>  	struct drm_device *dev = (struct drm_device *)data;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	uint32_t acthd, instdone, instdone1, acthd_bsd, acthd_blt;
> -	bool err = false;
> +	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
> +	struct intel_ring_buffer *ring;
> +	bool err = false, idle;
> +	int i;
>  
>  	if (!i915_enable_hangcheck)
>  		return;
>  
> +	memset(acthd, 0, sizeof(acthd));
> +	idle = true;
> +	for_each_ring(ring, dev_priv, i) {
> +	    idle &= i915_hangcheck_ring_idle(ring, &err);
> +	    acthd[i] = intel_ring_get_active_head(ring);
> +	}
> +
>  	/* If all work is done then ACTHD clearly hasn't advanced. */
> -	if (i915_hangcheck_ring_idle(&dev_priv->ring[RCS], &err) &&
> -	    i915_hangcheck_ring_idle(&dev_priv->ring[VCS], &err) &&
> -	    i915_hangcheck_ring_idle(&dev_priv->ring[BCS], &err)) {
> +	if (idle) {
>  		if (err) {
>  			if (i915_hangcheck_hung(dev))
>  				return;

Similar functional change as previous: we do all rings no matter what.

> @@ -1616,15 +1612,8 @@ void i915_hangcheck_elapsed(unsigned long data)
>  		instdone = I915_READ(INSTDONE_I965);
>  		instdone1 = I915_READ(INSTDONE1);
>  	}
> -	acthd = intel_ring_get_active_head(&dev_priv->ring[RCS]);
> -	acthd_bsd = HAS_BSD(dev) ?
> -		intel_ring_get_active_head(&dev_priv->ring[VCS]) : 0;
> -	acthd_blt = HAS_BLT(dev) ?
> -		intel_ring_get_active_head(&dev_priv->ring[BCS]) : 0;
>  
> -	if (dev_priv->last_acthd == acthd &&
> -	    dev_priv->last_acthd_bsd == acthd_bsd &&
> -	    dev_priv->last_acthd_blt == acthd_blt &&
> +	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
>  	    dev_priv->last_instdone == instdone &&
>  	    dev_priv->last_instdone1 == instdone1) {
>  		if (i915_hangcheck_hung(dev))
> @@ -1632,9 +1621,7 @@ void i915_hangcheck_elapsed(unsigned long data)
>  	} else {
>  		dev_priv->hangcheck_count = 0;
>  
> -		dev_priv->last_acthd = acthd;
> -		dev_priv->last_acthd_bsd = acthd_bsd;
> -		dev_priv->last_acthd_blt = acthd_blt;
> +		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
>  		dev_priv->last_instdone = instdone;
>  		dev_priv->last_instdone1 = instdone1;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 8f8d1da..438ff29 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2326,6 +2326,7 @@ int intel_enable_rc6(const struct drm_device *dev)
>  
>  void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  {
> +	struct intel_ring_buffer *ring;
>  	u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
>  	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
>  	u32 pcu_mbox, rc6_mask = 0;
> @@ -2360,8 +2361,8 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
>  	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
>  	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
>  
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		I915_WRITE(RING_MAX_IDLE(dev_priv->ring[i].mmio_base), 10);
> +	for_each_ring(ring, dev_priv, i)
> +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
>  
>  	I915_WRITE(GEN6_RC_SLEEP, 0);
>  	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> @@ -3048,6 +3049,7 @@ 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;
>  
>  	spin_lock(&mchdev_lock);


There is still a hunk fail here.

-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Introduce for_each_ring() macro
  2012-05-10 22:39   ` Ben Widawsky
@ 2012-05-10 23:18     ` Chris Wilson
  2012-05-11  9:00     ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2012-05-10 23:18 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: intel-gfx

On Thu, 10 May 2012 15:39:32 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> I'm not opposed to this change, but it would have probably been better
> to not remove the BUG_ONs.

I actually removed them because in order to satisfy them we have to
initialise rings we never use (and have another patch so that we only
intialise rings once inside intel_ringbuffer.c). We were causing extra
work for ourselves for no good reason.

And we need to cull the BUG_ONs for which a WARN and recovery would
serve better.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Introduce for_each_ring() macro
  2012-05-10 22:39   ` Ben Widawsky
  2012-05-10 23:18     ` Chris Wilson
@ 2012-05-11  9:00     ` Daniel Vetter
  2012-05-11  9:26       ` Chris Wilson
  1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2012-05-11  9:00 UTC (permalink / raw)
  To: Ben Widawsky; +Cc: intel-gfx

On Thu, May 10, 2012 at 03:39:32PM -0700, Ben Widawsky wrote:
> On Thu, 10 May 2012 22:21:50 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > In many places we wish to iterate over the rings associated with the
> > GPU, so refactor them to use a common macro.
> > 
> > Along the way, there are a few code removals that should be side-effect
> > free and some rearrangement which should only have a cosmetic impact,
> > such as error-state.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net> (if last hunk is removed
> and commit messages is amended)
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c   |    9 ++---
> >  drivers/gpu/drm/i915/i915_drv.c       |   10 ++---
> >  drivers/gpu/drm/i915/i915_drv.h       |    9 +++--
> >  drivers/gpu/drm/i915/i915_gem.c       |   33 +++++++---------
> >  drivers/gpu/drm/i915/i915_gem_evict.c |   14 +++----
> >  drivers/gpu/drm/i915/i915_irq.c       |   69 +++++++++++++--------------------
> >  drivers/gpu/drm/i915/intel_pm.c       |    6 ++-
> >  7 files changed, 66 insertions(+), 84 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 950f72a..eb2b3c2 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -699,6 +699,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
> >  	struct drm_device *dev = error_priv->dev;
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	struct drm_i915_error_state *error = error_priv->error;
> > +	struct intel_ring_buffer *ring;
> >  	int i, j, page, offset, elt;
> >  
> >  	if (!error) {
> > @@ -706,7 +707,6 @@ static int i915_error_state(struct seq_file *m, void *unused)
> >  		return 0;
> >  	}
> >  
> > -
> >  	seq_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
> >  		   error->time.tv_usec);
> >  	seq_printf(m, "PCI ID: 0x%04x\n", dev->pci_device);
> > @@ -722,11 +722,8 @@ static int i915_error_state(struct seq_file *m, void *unused)
> >  		seq_printf(m, "DONE_REG: 0x%08x\n", error->done_reg);
> >  	}
> >  
> > -	i915_ring_error_state(m, dev, error, RCS);
> > -	if (HAS_BLT(dev))
> > -		i915_ring_error_state(m, dev, error, BCS);
> > -	if (HAS_BSD(dev))
> > -		i915_ring_error_state(m, dev, error, VCS);
> > +	for_each_ring(ring, dev_priv, i)
> > +		i915_ring_error_state(m, dev, error, i);
> >  
> >  	if (error->active_bo)
> >  		print_error_buffers(m, "Active",
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 5898be9..2b7967d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -893,15 +893,15 @@ int i915_reset(struct drm_device *dev)
> >  	 */
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET) ||
> >  			!dev_priv->mm.suspended) {
> > +		struct intel_ring_buffer *ring;
> > +		int i;
> > +
> >  		dev_priv->mm.suspended = 0;
> >  
> >  		i915_gem_init_swizzling(dev);
> >  
> > -		dev_priv->ring[RCS].init(&dev_priv->ring[RCS]);
> > -		if (HAS_BSD(dev))
> > -		    dev_priv->ring[VCS].init(&dev_priv->ring[VCS]);
> > -		if (HAS_BLT(dev))
> > -		    dev_priv->ring[BCS].init(&dev_priv->ring[BCS]);
> > +		for_each_ring(ring, dev_priv, i)
> > +			ring->init(ring, ring->obj);
> >  
> >  		i915_gem_init_ppgtt(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 83a557c..c2e0732 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -410,9 +410,7 @@ typedef struct drm_i915_private {
> >  #define DRM_I915_HANGCHECK_PERIOD 1500 /* in ms */
> >  	struct timer_list hangcheck_timer;
> >  	int hangcheck_count;
> > -	uint32_t last_acthd;
> > -	uint32_t last_acthd_bsd;
> > -	uint32_t last_acthd_blt;
> > +	uint32_t last_acthd[I915_NUM_RINGS];
> >  	uint32_t last_instdone;
> >  	uint32_t last_instdone1;
> >  
> > @@ -820,6 +818,11 @@ typedef struct drm_i915_private {
> >  	struct drm_property *force_audio_property;
> >  } drm_i915_private_t;
> >  
> > +/* Iterate over initialised rings */
> > +#define for_each_ring(ring__, dev_priv__, i__) \
> > +	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> > +		if (((ring__) = &(dev_priv__)->ring[(i__)])->obj)
> > +
> >  enum hdmi_force_audio {
> >  	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
> >  	HDMI_AUDIO_OFF,			/* force turn off HDMI audio */
> 
> I'm somewhat disappointed with using ring->obj here. But I do not have a
> better suggestion.

I think we could wrap the ring->obj checking into intel_ring_initialized
and adjust the if checke like

	if (((ring_) = &(dev_priv__)->ring[(i__)]) && intel_ring_initialized (ring__))

The follow-on patch to use the ring->obj check more would then also be
self-documenting.

> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 44a5f24..99961ce 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1655,10 +1655,11 @@ void i915_gem_reset(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj;
> > +	struct intel_ring_buffer *ring;
> >  	int i;
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++)
> > -		i915_gem_reset_ring_lists(dev_priv, &dev_priv->ring[i]);
> > +	for_each_ring(ring, dev_priv, i)
> > +		i915_gem_reset_ring_lists(dev_priv, ring);
> >  
> >  	/* Remove anything from the flushing lists. The GPU cache is likely
> >  	 * to be lost on reset along with the data, so simply move the
> > @@ -1763,10 +1764,11 @@ void
> >  i915_gem_retire_requests(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> >  	int i;
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++)
> > -		i915_gem_retire_requests_ring(&dev_priv->ring[i]);
> > +	for_each_ring(ring, dev_priv, i)
> > +		i915_gem_retire_requests_ring(ring);
> >  }
> >  
> >  static void
> > @@ -1774,6 +1776,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
> >  {
> >  	drm_i915_private_t *dev_priv;
> >  	struct drm_device *dev;
> > +	struct intel_ring_buffer *ring;
> >  	bool idle;
> >  	int i;
> >  
> > @@ -1793,9 +1796,7 @@ i915_gem_retire_work_handler(struct work_struct *work)
> >  	 * objects indefinitely.
> >  	 */
> >  	idle = true;
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > -		struct intel_ring_buffer *ring = &dev_priv->ring[i];
> > -
> > +	for_each_ring(ring, dev_priv, i) {
> >  		if (!list_empty(&ring->gpu_write_list)) {
> >  			struct drm_i915_gem_request *request;
> >  			int ret;
> > @@ -2137,10 +2138,11 @@ static int i915_ring_idle(struct intel_ring_buffer *ring)
> >  int i915_gpu_idle(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> >  	int ret, i;
> >  
> >  	/* Flush everything onto the inactive list. */
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > +	for_each_ring(ring, dev_priv, i) {
> >  		ret = i915_ring_idle(&dev_priv->ring[i]);
> >  		if (ret)
> >  			return ret;
> > @@ -3463,9 +3465,7 @@ void i915_gem_init_ppgtt(struct drm_device *dev)
> >  		/* GFX_MODE is per-ring on gen7+ */
> >  	}
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > -		ring = &dev_priv->ring[i];
> > -
> > +	for_each_ring(ring, dev_priv, i) {
> >  		if (INTEL_INFO(dev)->gen >= 7)
> >  			I915_WRITE(RING_MODE_GEN7(ring),
> >  				   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> > @@ -3581,10 +3581,11 @@ void
> >  i915_gem_cleanup_ringbuffer(struct drm_device *dev)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> >  	int i;
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++)
> > -		intel_cleanup_ring_buffer(&dev_priv->ring[i]);
> > +	for_each_ring(ring, dev_priv, i)
> > +		intel_cleanup_ring_buffer(ring);
> >  }
> >  
> >  int
> > @@ -3592,7 +3593,7 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  		       struct drm_file *file_priv)
> >  {
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > -	int ret, i;
> > +	int ret;
> >  
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET))
> >  		return 0;
> > @@ -3614,10 +3615,6 @@ i915_gem_entervt_ioctl(struct drm_device *dev, void *data,
> >  	BUG_ON(!list_empty(&dev_priv->mm.active_list));
> >  	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
> >  	BUG_ON(!list_empty(&dev_priv->mm.inactive_list));
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > -		BUG_ON(!list_empty(&dev_priv->ring[i].active_list));
> > -		BUG_ON(!list_empty(&dev_priv->ring[i].request_list));
> > -	}
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	ret = drm_irq_install(dev);
> 
> I'm not opposed to this change, but it would have probably been better
> to not remove the BUG_ONs.

Shouldn't the ring->obj check dtrt here and we could just convert that
BUG_ON loop to for_each_ring?

> > index 3bcf045..ae7c24e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> > @@ -168,7 +168,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj, *next;
> >  	bool lists_empty;
> > -	int ret,i;
> > +	int ret;
> >  
> >  	lists_empty = (list_empty(&dev_priv->mm.inactive_list) &&
> >  		       list_empty(&dev_priv->mm.flushing_list) &&
> > @@ -178,17 +178,13 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> >  
> >  	trace_i915_gem_evict_everything(dev, purgeable_only);
> >  
> > -	ret = i915_gpu_idle(dev);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/* The gpu_idle will flush everything in the write domain to the
> >  	 * active list. Then we must move everything off the active list
> >  	 * with retire requests.
> >  	 */
> > -	for (i = 0; i < I915_NUM_RINGS; i++)
> > -		if (WARN_ON(!list_empty(&dev_priv->ring[i].gpu_write_list)))
> > -			return -EBUSY;
> > +	ret = i915_gpu_idle(dev);
> > +	if (ret)
> > +		return ret;
> >  
> >  	i915_gem_retire_requests(dev);
> >  
> 
> Sneaky way to get rid of the warning you never liked... I won't forget
> this.

I concur that killing that warnings is sneaky ;-) Can't we just keep them
- the ring->obj check should do the right thing? Or what exactly do I
miss?

> > @@ -203,5 +199,5 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
> >  		}
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2a062d7..cc4a633 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1022,15 +1022,11 @@ static void i915_gem_record_rings(struct drm_device *dev,
> >  				  struct drm_i915_error_state *error)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> >  	struct drm_i915_gem_request *request;
> >  	int i, count;
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++) {
> > -		struct intel_ring_buffer *ring = &dev_priv->ring[i];
> > -
> > -		if (ring->obj == NULL)
> > -			continue;
> > -
> > +	for_each_ring(ring, dev_priv, i) {
> >  		i915_record_ring_state(dev, error, ring);
> >  
> >  		error->ring[i].batchbuffer =
> > @@ -1295,6 +1291,8 @@ static void i915_report_and_clear_eir(struct drm_device *dev)
> >  void i915_handle_error(struct drm_device *dev, bool wedged)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_ring_buffer *ring;
> > +	int i;
> >  
> >  	i915_capture_error_state(dev);
> >  	i915_report_and_clear_eir(dev);
> > @@ -1306,11 +1304,8 @@ void i915_handle_error(struct drm_device *dev, bool wedged)
> >  		/*
> >  		 * Wakeup waiting processes so they don't hang
> >  		 */
> > -		wake_up_all(&dev_priv->ring[RCS].irq_queue);
> > -		if (HAS_BSD(dev))
> > -			wake_up_all(&dev_priv->ring[VCS].irq_queue);
> > -		if (HAS_BLT(dev))
> > -			wake_up_all(&dev_priv->ring[BCS].irq_queue);
> > +		for_each_ring(ring, dev_priv, i)
> > +			wake_up_all(&ring->irq_queue);
> >  	}
> >  
> >  	queue_work(dev_priv->wq, &dev_priv->error_work);
> > @@ -1515,11 +1510,6 @@ ring_last_seqno(struct intel_ring_buffer *ring)
> >  
> >  static bool i915_hangcheck_ring_idle(struct intel_ring_buffer *ring, bool *err)
> >  {
> > -	/* We don't check whether the ring even exists before calling this
> > -	 * function. Hence check whether it's initialized. */
> > -	if (ring->obj == NULL)
> > -		return true;
> > -
> >  	if (list_empty(&ring->request_list) ||
> >  	    i915_seqno_passed(ring->get_seqno(ring), ring_last_seqno(ring))) {
> >  		/* Issue a wake-up to catch stuck h/w. */
> > @@ -1553,26 +1543,25 @@ static bool i915_hangcheck_hung(struct drm_device *dev)
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> >  
> >  	if (dev_priv->hangcheck_count++ > 1) {
> > +		bool hung = true;
> > +
> >  		DRM_ERROR("Hangcheck timer elapsed... GPU hung\n");
> >  		i915_handle_error(dev, true);
> >  
> >  		if (!IS_GEN2(dev)) {
> > +			struct intel_ring_buffer *ring;
> > +			int i;
> > +
> >  			/* Is the chip hanging on a WAIT_FOR_EVENT?
> >  			 * If so we can simply poke the RB_WAIT bit
> >  			 * and break the hang. This should work on
> >  			 * all but the second generation chipsets.
> >  			 */
> > -			if (kick_ring(&dev_priv->ring[RCS]))
> > -				return false;
> > -
> > -			if (HAS_BSD(dev) && kick_ring(&dev_priv->ring[VCS]))
> > -				return false;
> > -
> > -			if (HAS_BLT(dev) && kick_ring(&dev_priv->ring[BCS]))
> > -				return false;
> > +			for_each_ring(ring, dev_priv, i)
> > +				hung &= !kick_ring(ring);
> >  		}
> >  
> > -		return true;
> > +		return hung;
> >  	}
> >  
> >  	return false;
> 
> It's worth noting in the commit message that there is a functional
> change here. Before we'd exit out on the first ring kicked, now we
> enumerate over all rings regardless.

I agree that's noteworthy (and poking all rings makes more sense anyway).

> I'm still a fan of ripping out the kicks completely.

We can't, it helps to get us out of broken userspace on gen3 (where we
lack gpu reset support).

> > @@ -1588,16 +1577,23 @@ void i915_hangcheck_elapsed(unsigned long data)
> >  {
> >  	struct drm_device *dev = (struct drm_device *)data;
> >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > -	uint32_t acthd, instdone, instdone1, acthd_bsd, acthd_blt;
> > -	bool err = false;
> > +	uint32_t acthd[I915_NUM_RINGS], instdone, instdone1;
> > +	struct intel_ring_buffer *ring;
> > +	bool err = false, idle;
> > +	int i;
> >  
> >  	if (!i915_enable_hangcheck)
> >  		return;
> >  
> > +	memset(acthd, 0, sizeof(acthd));
> > +	idle = true;
> > +	for_each_ring(ring, dev_priv, i) {
> > +	    idle &= i915_hangcheck_ring_idle(ring, &err);
> > +	    acthd[i] = intel_ring_get_active_head(ring);
> > +	}
> > +
> >  	/* If all work is done then ACTHD clearly hasn't advanced. */
> > -	if (i915_hangcheck_ring_idle(&dev_priv->ring[RCS], &err) &&
> > -	    i915_hangcheck_ring_idle(&dev_priv->ring[VCS], &err) &&
> > -	    i915_hangcheck_ring_idle(&dev_priv->ring[BCS], &err)) {
> > +	if (idle) {
> >  		if (err) {
> >  			if (i915_hangcheck_hung(dev))
> >  				return;
> 
> Similar functional change as previous: we do all rings no matter what.
> 
> > @@ -1616,15 +1612,8 @@ void i915_hangcheck_elapsed(unsigned long data)
> >  		instdone = I915_READ(INSTDONE_I965);
> >  		instdone1 = I915_READ(INSTDONE1);
> >  	}
> > -	acthd = intel_ring_get_active_head(&dev_priv->ring[RCS]);
> > -	acthd_bsd = HAS_BSD(dev) ?
> > -		intel_ring_get_active_head(&dev_priv->ring[VCS]) : 0;
> > -	acthd_blt = HAS_BLT(dev) ?
> > -		intel_ring_get_active_head(&dev_priv->ring[BCS]) : 0;
> >  
> > -	if (dev_priv->last_acthd == acthd &&
> > -	    dev_priv->last_acthd_bsd == acthd_bsd &&
> > -	    dev_priv->last_acthd_blt == acthd_blt &&
> > +	if (memcmp(dev_priv->last_acthd, acthd, sizeof(acthd)) == 0 &&
> >  	    dev_priv->last_instdone == instdone &&
> >  	    dev_priv->last_instdone1 == instdone1) {
> >  		if (i915_hangcheck_hung(dev))
> > @@ -1632,9 +1621,7 @@ void i915_hangcheck_elapsed(unsigned long data)
> >  	} else {
> >  		dev_priv->hangcheck_count = 0;
> >  
> > -		dev_priv->last_acthd = acthd;
> > -		dev_priv->last_acthd_bsd = acthd_bsd;
> > -		dev_priv->last_acthd_blt = acthd_blt;
> > +		memcpy(dev_priv->last_acthd, acthd, sizeof(acthd));
> >  		dev_priv->last_instdone = instdone;
> >  		dev_priv->last_instdone1 = instdone1;
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 8f8d1da..438ff29 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2326,6 +2326,7 @@ int intel_enable_rc6(const struct drm_device *dev)
> >  
> >  void gen6_enable_rps(struct drm_i915_private *dev_priv)
> >  {
> > +	struct intel_ring_buffer *ring;
> >  	u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> >  	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
> >  	u32 pcu_mbox, rc6_mask = 0;
> > @@ -2360,8 +2361,8 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
> >  	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
> >  
> > -	for (i = 0; i < I915_NUM_RINGS; i++)
> > -		I915_WRITE(RING_MAX_IDLE(dev_priv->ring[i].mmio_base), 10);
> > +	for_each_ring(ring, dev_priv, i)
> > +		I915_WRITE(RING_MAX_IDLE(ring->mmio_base), 10);
> >  
> >  	I915_WRITE(GEN6_RC_SLEEP, 0);
> >  	I915_WRITE(GEN6_RC1e_THRESHOLD, 1000);
> > @@ -3048,6 +3049,7 @@ 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;
> >  
> >  	spin_lock(&mchdev_lock);
> 
> 
> There is still a hunk fail here.
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Introduce for_each_ring() macro
  2012-05-11  9:00     ` Daniel Vetter
@ 2012-05-11  9:26       ` Chris Wilson
  0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2012-05-11  9:26 UTC (permalink / raw)
  To: Daniel Vetter, Ben Widawsky; +Cc: intel-gfx

On Fri, 11 May 2012 11:00:12 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 10, 2012 at 03:39:32PM -0700, Ben Widawsky wrote:
> > Sneaky way to get rid of the warning you never liked... I won't forget
> > this.
> 
> I concur that killing that warnings is sneaky ;-) Can't we just keep them
> - the ring->obj check should do the right thing? Or what exactly do I
> miss?

Other than I'm not a fan of those warnings; completely in the wrong
spot. If you want a warning that i915_gpu_idle is not doing its job
even in the face of a wedged GPU (so in fact i915_gem_reset failing),
it would be better to place those warnings inside i915_gpu_idle.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-11  9:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-09 22:27 [PATCH] drm/i915: Introduce for_each_ring() macro Chris Wilson
2012-05-10 21:21 ` Chris Wilson
2012-05-10 22:39   ` Ben Widawsky
2012-05-10 23:18     ` Chris Wilson
2012-05-11  9:00     ` Daniel Vetter
2012-05-11  9:26       ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.