[CI 1/2] drm/i915: Tidy reporting busy status during i915_gem_retire_requests()

[CI 1/2] drm/i915: Tidy reporting busy status during i915_gem_retire_requests()

[Chris Wilson]

As we know by inspection whether any engine is still busy as we retire
all the requests, we can pass that information back via return value
rather than check again afterwards.

v2: A little more polish missed in patch splitting

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 1a215320cefb..5e55270bb2de 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -749,16 +749,18 @@ complete:
 	return ret;
 }
 
-static void engine_retire_requests(struct intel_engine_cs *engine)
+static bool engine_retire_requests(struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_request *request, *next;
 
 	list_for_each_entry_safe(request, next, &engine->request_list, link) {
 		if (!i915_gem_request_completed(request))
-			break;
+			return false;
 
 		i915_gem_request_retire(request);
 	}
+
+	return true;
 }
 
 void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
@@ -772,11 +774,9 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!dev_priv->gt.awake);
 
-	for_each_engine(engine, dev_priv) {
-		engine_retire_requests(engine);
-		if (!intel_engine_is_active(engine))
+	for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines)
+		if (engine_retire_requests(engine))
 			dev_priv->gt.active_engines &= ~intel_engine_flag(engine);
-	}
 
 	if (dev_priv->gt.active_engines == 0)
 		queue_delayed_work(dev_priv->wq,
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[CI 2/2] drm/i915: Make for_each_engine_masked() more compact and quicker

[Chris Wilson]

Rather than walk the full array of engines checking whether each is in
the mask in turn, we can use the mask to jump to the right engines. This
should quicker for a sparse array of engines or mask, whilst generating
smaller code:

   text	   data	    bss	    dec	    hex	filename
1251010	   4579	    800	1256389	 132bc5	drivers/gpu/drm/i915/i915.ko
1250530	   4579	    800	1255909	 1329e5	drivers/gpu/drm/i915/i915.ko

The downside is that we have to pass in a temporary, alas no C99
iterators yet.

[P.S. Joonas doesn't like having to pass extra temporaries into the
macro, and even less that I called them tmp. As yet, we haven't found a
macro that avoids passing in a temporary that is smaller. We probably
will get C99 iterators first!]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 15 +++++++++------
 drivers/gpu/drm/i915/i915_gem_request.c    |  3 ++-
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 ++-
 drivers/gpu/drm/i915/i915_irq.c            |  3 ++-
 drivers/gpu/drm/i915/intel_uncore.c        |  9 ++++++---
 5 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fdaf23336df..77a5478043e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2093,13 +2093,16 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 		for_each_if (((id__) = (engine__)->id, \
 			      intel_engine_initialized(engine__)))
 
+#define __mask_next_bit(mask) ({					\
+	int __idx = ffs(mask) - 1;					\
+	mask &= ~BIT(__idx);						\
+	__idx;								\
+})
+
 /* Iterator over subset of engines selected by mask */
-#define for_each_engine_masked(engine__, dev_priv__, mask__) \
-	for ((engine__) = &(dev_priv__)->engine[0]; \
-	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
-	     (engine__)++) \
-		for_each_if (((mask__) & intel_engine_flag(engine__)) && \
-			     intel_engine_initialized(engine__))
+#define for_each_engine_masked(engine__, dev_priv__, mask__, tmp__) \
+	for (tmp__ = mask__ & INTEL_INFO(dev_priv__)->ring_mask;	\
+	     tmp__ ? (engine__ = &(dev_priv__)->engine[__mask_next_bit(tmp__)]), 1 : 0; )
 
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 5e55270bb2de..8e97abaf7999 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -766,6 +766,7 @@ static bool engine_retire_requests(struct intel_engine_cs *engine)
 void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
+	unsigned tmp;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
@@ -774,7 +775,7 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!dev_priv->gt.awake);
 
-	for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines)
+	for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines, tmp)
 		if (engine_retire_requests(engine))
 			dev_priv->gt.active_engines &= ~intel_engine_flag(engine);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e4369411f07d..6437a0b7e0cf 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -330,6 +330,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	struct i915_gem_context *ctx = client->owner;
 	struct guc_context_desc desc;
 	struct sg_table *sg;
+	unsigned tmp;
 	u32 gfx_addr;
 
 	memset(&desc, 0, sizeof(desc));
@@ -339,7 +340,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	desc.priority = client->priority;
 	desc.db_id = client->doorbell_id;
 
-	for_each_engine_masked(engine, dev_priv, client->engines) {
+	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
 		struct intel_context *ce = &ctx->engine[engine->id];
 		uint32_t guc_engine_id = engine->guc_id;
 		struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 7610eca4f687..5de311cb27c5 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3169,6 +3169,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 
 	if (hung) {
 		char msg[80];
+		unsigned tmp;
 		int len;
 
 		/* If some rings hung but others were still busy, only
@@ -3178,7 +3179,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 			hung &= ~stuck;
 		len = scnprintf(msg, sizeof(msg),
 				"%s on ", stuck == hung ? "No progress" : "Hang");
-		for_each_engine_masked(engine, dev_priv, hung)
+		for_each_engine_masked(engine, dev_priv, hung, tmp)
 			len += scnprintf(msg + len, sizeof(msg) - len,
 					 "%s, ", engine->name);
 		msg[len-2] = '\0';
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 43f833901b8e..c53d50e66ae1 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1597,8 +1597,10 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 	if (engine_mask == ALL_ENGINES) {
 		hw_mask = GEN6_GRDOM_FULL;
 	} else {
+		unsigned tmp;
+
 		hw_mask = 0;
-		for_each_engine_masked(engine, dev_priv, engine_mask)
+		for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
 			hw_mask |= hw_engine_mask[engine->id];
 	}
 
@@ -1714,15 +1716,16 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 			      unsigned engine_mask)
 {
 	struct intel_engine_cs *engine;
+	unsigned tmp;
 
-	for_each_engine_masked(engine, dev_priv, engine_mask)
+	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
 		if (gen8_request_engine_reset(engine))
 			goto not_ready;
 
 	return gen6_reset_engines(dev_priv, engine_mask);
 
 not_ready:
-	for_each_engine_masked(engine, dev_priv, engine_mask)
+	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
 		gen8_unrequest_engine_reset(engine);
 
 	return -EIO;
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [CI,1/2] drm/i915: Tidy reporting busy status during i915_gem_retire_requests()

[Patchwork]

== Series Details ==

Series: series starting with [CI,1/2] drm/i915: Tidy reporting busy status during i915_gem_retire_requests()
URL   : https://patchwork.freedesktop.org/series/11664/
State : warning

== Summary ==

Series 11664v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11664/revisions/1/mbox/

Test drv_module_reload_basic:
                skip       -> PASS       (fi-skl-6260u)
Test gem_exec_gttfill:
        Subgroup basic:
                pass       -> SKIP       (fi-snb-2600)
Test gem_exec_suspend:
        Subgroup basic-s3:
                incomplete -> PASS       (fi-hsw-4770k)

fi-bdw-5557u     total:252  pass:235  dwarn:0   dfail:0   fail:2   skip:15 
fi-bsw-n3050     total:252  pass:205  dwarn:0   dfail:0   fail:1   skip:46 
fi-byt-n2820     total:252  pass:207  dwarn:0   dfail:0   fail:3   skip:42 
fi-hsw-4770k     total:252  pass:228  dwarn:0   dfail:0   fail:2   skip:22 
fi-hsw-4770r     total:252  pass:224  dwarn:0   dfail:0   fail:2   skip:26 
fi-ivb-3520m     total:252  pass:220  dwarn:0   dfail:0   fail:1   skip:31 
fi-skl-6260u     total:252  pass:236  dwarn:0   dfail:0   fail:2   skip:14 
fi-skl-6700k     total:252  pass:222  dwarn:0   dfail:0   fail:2   skip:28 
fi-snb-2520m     total:252  pass:207  dwarn:0   dfail:0   fail:2   skip:43 
fi-snb-2600      total:252  pass:206  dwarn:0   dfail:0   fail:2   skip:44 

Results at /archive/results/CI_IGT_test/Patchwork_2440/

6068022442b9e3760829afb40fc87c3407141461 drm-intel-nightly: 2016y-08m-26d-19h-31m-08s UTC integration manifest
8d5b2d6 drm/i915: Make for_each_engine_masked() more compact and quicker
2bbf576 drm/i915: Tidy reporting busy status during i915_gem_retire_requests()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] Another flavour of for_each_engine_masked()

[Dave Gordon]

This macro was recently updated to skip testing for non-existent or
uninteresting engines by using ffs() to directly find the next engine of
interest. However, it required the introduction of a caller-provided
temporary variable, which some people regard as inelegant. So, this
patch provides another variant, which still uses the fast-skip mechanism
but without requiring the temporary, for the cost of a slight increase
in code size (~20 bytes per callsite).

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 19 ++++++++++---------
 drivers/gpu/drm/i915/i915_gem_request.c    |  3 +--
 drivers/gpu/drm/i915/i915_guc_submission.c |  3 +--
 drivers/gpu/drm/i915/i915_irq.c            |  3 +--
 drivers/gpu/drm/i915/intel_uncore.c        |  9 +++------
 5 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c413587..6c59876 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2093,16 +2093,17 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 		for_each_if (((id__) = (engine__)->id, \
 			      intel_engine_initialized(engine__)))
 
-#define __mask_next_bit(mask) ({					\
-	int __idx = ffs(mask) - 1;					\
-	mask &= ~BIT(__idx);						\
-	__idx;								\
-})
-
 /* Iterator over subset of engines selected by mask */
-#define for_each_engine_masked(engine__, dev_priv__, mask__, tmp__) \
-	for (tmp__ = mask__ & INTEL_INFO(dev_priv__)->ring_mask;	\
-	     tmp__ ? (engine__ = &(dev_priv__)->engine[__mask_next_bit(tmp__)]), 1 : 0; )
+#define for_each_engine_masked(engine__, dev_priv__, mask__)		\
+	for ((engine__) = NULL;						\
+	     ({								\
+		u32 next__ = INTEL_INFO(dev_priv__)->ring_mask;		\
+		if (likely(engine__))					\
+			next__ &= ~1u << (engine__)->id;		\
+		next__ = ffs(mask__ & next__);				\
+		(engine__) = (dev_priv__)->engine + next__ - 1;		\
+		next__;							\
+	     }); )
 
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 9cc08a1..5e55270 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -766,7 +766,6 @@ static bool engine_retire_requests(struct intel_engine_cs *engine)
 void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	unsigned int tmp;
 
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 
@@ -775,7 +774,7 @@ void i915_gem_retire_requests(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!dev_priv->gt.awake);
 
-	for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines, tmp)
+	for_each_engine_masked(engine, dev_priv, dev_priv->gt.active_engines)
 		if (engine_retire_requests(engine))
 			dev_priv->gt.active_engines &= ~intel_engine_flag(engine);
 
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 32699a7..e436941 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -330,7 +330,6 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	struct i915_gem_context *ctx = client->owner;
 	struct guc_context_desc desc;
 	struct sg_table *sg;
-	unsigned int tmp;
 	u32 gfx_addr;
 
 	memset(&desc, 0, sizeof(desc));
@@ -340,7 +339,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	desc.priority = client->priority;
 	desc.db_id = client->doorbell_id;
 
-	for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
+	for_each_engine_masked(engine, dev_priv, client->engines) {
 		struct intel_context *ce = &ctx->engine[engine->id];
 		uint32_t guc_engine_id = engine->guc_id;
 		struct guc_execlist_context *lrc = &desc.lrc[guc_engine_id];
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 82358d4..7610eca 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3169,7 +3169,6 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 
 	if (hung) {
 		char msg[80];
-		unsigned int tmp;
 		int len;
 
 		/* If some rings hung but others were still busy, only
@@ -3179,7 +3178,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 			hung &= ~stuck;
 		len = scnprintf(msg, sizeof(msg),
 				"%s on ", stuck == hung ? "No progress" : "Hang");
-		for_each_engine_masked(engine, dev_priv, hung, tmp)
+		for_each_engine_masked(engine, dev_priv, hung)
 			len += scnprintf(msg + len, sizeof(msg) - len,
 					 "%s, ", engine->name);
 		msg[len-2] = '\0';
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e9f68cd..43f8339 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1597,10 +1597,8 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 	if (engine_mask == ALL_ENGINES) {
 		hw_mask = GEN6_GRDOM_FULL;
 	} else {
-		unsigned int tmp;
-
 		hw_mask = 0;
-		for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
+		for_each_engine_masked(engine, dev_priv, engine_mask)
 			hw_mask |= hw_engine_mask[engine->id];
 	}
 
@@ -1716,16 +1714,15 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
 			      unsigned engine_mask)
 {
 	struct intel_engine_cs *engine;
-	unsigned int tmp;
 
-	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
+	for_each_engine_masked(engine, dev_priv, engine_mask)
 		if (gen8_request_engine_reset(engine))
 			goto not_ready;
 
 	return gen6_reset_engines(dev_priv, engine_mask);
 
 not_ready:
-	for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
+	for_each_engine_masked(engine, dev_priv, engine_mask)
 		gen8_unrequest_engine_reset(engine);
 
 	return -EIO;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Another flavour of for_each_engine_masked()

[Joonas Lahtinen]

On to, 2016-09-01 at 15:17 +0100, Dave Gordon wrote:
> This macro was recently updated to skip testing for non-existent or
> uninteresting engines by using ffs() to directly find the next engine of
> interest. However, it required the introduction of a caller-provided
> temporary variable, which some people regard as inelegant. So, this
> patch provides another variant, which still uses the fast-skip mechanism
> but without requiring the temporary, for the cost of a slight increase
> in code size (~20 bytes per callsite).

I did the same kind of modification, it got Nacked.

I'd myself vote for loosing the temporary for 20 bytes too.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Another flavour of for_each_engine_masked()

[Chris Wilson]

On Thu, Sep 01, 2016 at 03:17:44PM +0100, Dave Gordon wrote:
> This macro was recently updated to skip testing for non-existent or
> uninteresting engines by using ffs() to directly find the next engine of
> interest. However, it required the introduction of a caller-provided
> temporary variable, which some people regard as inelegant. So, this
> patch provides another variant, which still uses the fast-skip mechanism
> but without requiring the temporary, for the cost of a slight increase
> in code size (~20 bytes per callsite).

Slight? Next.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Another flavour of for_each_engine_masked()

[Dave Gordon]

On 01/09/16 15:48, Chris Wilson wrote:
> On Thu, Sep 01, 2016 at 03:17:44PM +0100, Dave Gordon wrote:
>> This macro was recently updated to skip testing for non-existent or
>> uninteresting engines by using ffs() to directly find the next engine of
>> interest. However, it required the introduction of a caller-provided
>> temporary variable, which some people regard as inelegant. So, this
>> patch provides another variant, which still uses the fast-skip mechanism
>> but without requiring the temporary, for the cost of a slight increase
>> in code size (~20 bytes per callsite).
>
> Slight? Next.
> -Chris

106 bytes out of 1166488; that's 0.009% or 91ppm.
The adjective "slight" definitely applies :)

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx