All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Skip context_barrier emission for unused contexts
@ 2019-06-04 15:24 Chris Wilson
  2019-06-04 18:26 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Chris Wilson @ 2019-06-04 15:24 UTC (permalink / raw)
  To: intel-gfx

The intent was to skip unused HW contexts by checking ce->state.
However, this only works for execlists where the ppGTT pointers is
stored inside the HW context. For gen7, the ppGTT is alongside the
logical state and must be updated on all active engines but, crucially,
only on active engines. As we need different checks, and to keep
context_barrier_task() agnostic, pass in the predicate.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
 .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 08721ef62e4e..6819b598d226 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
 I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
 static int context_barrier_task(struct i915_gem_context *ctx,
 				intel_engine_mask_t engines,
+				bool (*skip)(struct intel_context *ce, void *data),
 				int (*emit)(struct i915_request *rq, void *data),
 				void (*task)(void *data),
 				void *data)
@@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
 			break;
 		}
 
-		if (!(ce->engine->mask & engines) || !ce->state)
+		if (!(ce->engine->mask & engines))
+			continue;
+
+		if (skip && skip(ce, data))
 			continue;
 
 		rq = intel_context_create_request(ce);
@@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
 	return 0;
 }
 
+static bool skip_ppgtt_update(struct intel_context *ce, void *data)
+{
+	if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
+		return !ce->state;
+	else
+		return !atomic_read(&ce->pin_count);
+}
+
 static int set_ppgtt(struct drm_i915_file_private *file_priv,
 		     struct i915_gem_context *ctx,
 		     struct drm_i915_gem_context_param *args)
@@ -1103,6 +1115,7 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
 	 * only indirectly through the context.
 	 */
 	err = context_barrier_task(ctx, ALL_ENGINES,
+				   skip_ppgtt_update,
 				   emit_ppgtt_update,
 				   set_ppgtt_barrier,
 				   old);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
index 1bc3b8026400..41105f6ed206 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
@@ -1619,6 +1619,11 @@ __engine_name(struct drm_i915_private *i915, intel_engine_mask_t engines)
 	return "none";
 }
 
+static bool skip_unused_engines(struct intel_context *ce, void *data)
+{
+	return !ce->state;
+}
+
 static void mock_barrier_task(void *data)
 {
 	unsigned int *counter = data;
@@ -1651,7 +1656,7 @@ static int mock_context_barrier(void *arg)
 
 	counter = 0;
 	err = context_barrier_task(ctx, 0,
-				   NULL, mock_barrier_task, &counter);
+				   NULL, NULL, mock_barrier_task, &counter);
 	if (err) {
 		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
 		goto out;
@@ -1664,7 +1669,10 @@ static int mock_context_barrier(void *arg)
 
 	counter = 0;
 	err = context_barrier_task(ctx, ALL_ENGINES,
-				   NULL, mock_barrier_task, &counter);
+				   skip_unused_engines,
+				   NULL,
+				   mock_barrier_task,
+				   &counter);
 	if (err) {
 		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
 		goto out;
@@ -1685,7 +1693,7 @@ static int mock_context_barrier(void *arg)
 	counter = 0;
 	context_barrier_inject_fault = BIT(RCS0);
 	err = context_barrier_task(ctx, ALL_ENGINES,
-				   NULL, mock_barrier_task, &counter);
+				   NULL, NULL, mock_barrier_task, &counter);
 	context_barrier_inject_fault = 0;
 	if (err == -ENXIO)
 		err = 0;
@@ -1700,7 +1708,10 @@ static int mock_context_barrier(void *arg)
 
 	counter = 0;
 	err = context_barrier_task(ctx, ALL_ENGINES,
-				   NULL, mock_barrier_task, &counter);
+				   skip_unused_engines,
+				   NULL,
+				   mock_barrier_task,
+				   &counter);
 	if (err) {
 		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
 		goto out;
-- 
2.20.1

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Skip context_barrier emission for unused contexts
  2019-06-04 15:24 [PATCH] drm/i915: Skip context_barrier emission for unused contexts Chris Wilson
@ 2019-06-04 18:26 ` Patchwork
  2019-06-05 10:40 ` [PATCH] " Tvrtko Ursulin
  2019-06-05 17:40 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-06-04 18:26 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Skip context_barrier emission for unused contexts
URL   : https://patchwork.freedesktop.org/series/61595/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6187 -> Patchwork_13171
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/

Known issues
------------

  Here are the changes found in Patchwork_13171 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_basic@basic-all:
    - fi-cml-u:           [PASS][1] -> [INCOMPLETE][2] ([fdo#110566])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/fi-cml-u/igt@gem_exec_basic@basic-all.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/fi-cml-u/igt@gem_exec_basic@basic-all.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-icl-u3:          [PASS][3] -> [DMESG-WARN][4] ([fdo#107724]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html

  
#### Possible fixes ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size:
    - fi-icl-u3:          [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6] +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-varying-size.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u2:          [FAIL][7] ([fdo#103167]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/fi-icl-u2/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566
  [fdo#110829]: https://bugs.freedesktop.org/show_bug.cgi?id=110829


Participating hosts (54 -> 44)
------------------------------

  Missing    (10): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510 fi-kbl-7560u fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6187 -> Patchwork_13171

  CI_DRM_6187: 201dda6b2f7138214cdba69211c7504ce7b8b96e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5037: a98c9cd50aa48933217ca41055279ccb1680d25b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13171: d666683ac70c9093030a9c5183bc48f18fe38b4a @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d666683ac70c drm/i915: Skip context_barrier emission for unused contexts

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-04 15:24 [PATCH] drm/i915: Skip context_barrier emission for unused contexts Chris Wilson
  2019-06-04 18:26 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2019-06-05 10:40 ` Tvrtko Ursulin
  2019-06-06  9:09   ` Chris Wilson
  2019-06-05 17:40 ` ✓ Fi.CI.IGT: success for " Patchwork
  2 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-06-05 10:40 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 04/06/2019 16:24, Chris Wilson wrote:
> The intent was to skip unused HW contexts by checking ce->state.
> However, this only works for execlists where the ppGTT pointers is
> stored inside the HW context. For gen7, the ppGTT is alongside the
> logical state and must be updated on all active engines but, crucially,
> only on active engines. As we need different checks, and to keep
> context_barrier_task() agnostic, pass in the predicate.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
>   .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
>   2 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 08721ef62e4e..6819b598d226 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
>   I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
>   static int context_barrier_task(struct i915_gem_context *ctx,
>   				intel_engine_mask_t engines,
> +				bool (*skip)(struct intel_context *ce, void *data),
>   				int (*emit)(struct i915_request *rq, void *data),
>   				void (*task)(void *data),
>   				void *data)
> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>   			break;
>   		}
>   
> -		if (!(ce->engine->mask & engines) || !ce->state)
> +		if (!(ce->engine->mask & engines))
> +			continue;
> +
> +		if (skip && skip(ce, data))
>   			continue;
>   
>   		rq = intel_context_create_request(ce);
> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>   	return 0;
>   }
>   
> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> +{
> +	if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> +		return !ce->state;
> +	else
> +		return !atomic_read(&ce->pin_count);

Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and 
avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?

Regards,

Tvrtko

> +}
> +
>   static int set_ppgtt(struct drm_i915_file_private *file_priv,
>   		     struct i915_gem_context *ctx,
>   		     struct drm_i915_gem_context_param *args)
> @@ -1103,6 +1115,7 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
>   	 * only indirectly through the context.
>   	 */
>   	err = context_barrier_task(ctx, ALL_ENGINES,
> +				   skip_ppgtt_update,
>   				   emit_ppgtt_update,
>   				   set_ppgtt_barrier,
>   				   old);
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index 1bc3b8026400..41105f6ed206 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -1619,6 +1619,11 @@ __engine_name(struct drm_i915_private *i915, intel_engine_mask_t engines)
>   	return "none";
>   }
>   
> +static bool skip_unused_engines(struct intel_context *ce, void *data)
> +{
> +	return !ce->state;
> +}
> +
>   static void mock_barrier_task(void *data)
>   {
>   	unsigned int *counter = data;
> @@ -1651,7 +1656,7 @@ static int mock_context_barrier(void *arg)
>   
>   	counter = 0;
>   	err = context_barrier_task(ctx, 0,
> -				   NULL, mock_barrier_task, &counter);
> +				   NULL, NULL, mock_barrier_task, &counter);
>   	if (err) {
>   		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>   		goto out;
> @@ -1664,7 +1669,10 @@ static int mock_context_barrier(void *arg)
>   
>   	counter = 0;
>   	err = context_barrier_task(ctx, ALL_ENGINES,
> -				   NULL, mock_barrier_task, &counter);
> +				   skip_unused_engines,
> +				   NULL,
> +				   mock_barrier_task,
> +				   &counter);
>   	if (err) {
>   		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>   		goto out;
> @@ -1685,7 +1693,7 @@ static int mock_context_barrier(void *arg)
>   	counter = 0;
>   	context_barrier_inject_fault = BIT(RCS0);
>   	err = context_barrier_task(ctx, ALL_ENGINES,
> -				   NULL, mock_barrier_task, &counter);
> +				   NULL, NULL, mock_barrier_task, &counter);
>   	context_barrier_inject_fault = 0;
>   	if (err == -ENXIO)
>   		err = 0;
> @@ -1700,7 +1708,10 @@ static int mock_context_barrier(void *arg)
>   
>   	counter = 0;
>   	err = context_barrier_task(ctx, ALL_ENGINES,
> -				   NULL, mock_barrier_task, &counter);
> +				   skip_unused_engines,
> +				   NULL,
> +				   mock_barrier_task,
> +				   &counter);
>   	if (err) {
>   		pr_err("Failed at line %d, err=%d\n", __LINE__, err);
>   		goto out;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: Skip context_barrier emission for unused contexts
  2019-06-04 15:24 [PATCH] drm/i915: Skip context_barrier emission for unused contexts Chris Wilson
  2019-06-04 18:26 ` ✓ Fi.CI.BAT: success for " Patchwork
  2019-06-05 10:40 ` [PATCH] " Tvrtko Ursulin
@ 2019-06-05 17:40 ` Patchwork
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-06-05 17:40 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Skip context_barrier emission for unused contexts
URL   : https://patchwork.freedesktop.org/series/61595/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6187_full -> Patchwork_13171_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Known issues
------------

  Here are the changes found in Patchwork_13171_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-apl2/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-apl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#109349])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb3/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move:
    - shard-hsw:          [PASS][5] -> [SKIP][6] ([fdo#109271]) +28 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-hsw2/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-move.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-iclb:         [PASS][7] -> [FAIL][8] ([fdo#103167]) +4 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@psr-suspend:
    - shard-skl:          [PASS][9] -> [INCOMPLETE][10] ([fdo#104108] / [fdo#106978])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl3/igt@kms_frontbuffer_tracking@psr-suspend.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl9/igt@kms_frontbuffer_tracking@psr-suspend.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-iclb:         [PASS][11] -> [INCOMPLETE][12] ([fdo#107713] / [fdo#110042])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-skl:          [PASS][13] -> [INCOMPLETE][14] ([fdo#104108]) +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145] / [fdo#110403])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@psr2_primary_mmap_cpu:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#109441]) +2 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb2/igt@kms_psr@psr2_primary_mmap_cpu.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb6/igt@kms_psr@psr2_primary_mmap_cpu.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][19] -> [FAIL][20] ([fdo#99912])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-kbl1/igt@kms_setmode@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-kbl7/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * {igt@gem_ctx_param@vm}:
    - shard-hsw:          [DMESG-WARN][21] ([fdo#110836]) -> [PASS][22] +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-hsw2/igt@gem_ctx_param@vm.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-hsw6/igt@gem_ctx_param@vm.html

  * igt@gem_softpin@noreloc-s3:
    - shard-skl:          [INCOMPLETE][23] ([fdo#104108]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl5/igt@gem_softpin@noreloc-s3.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl3/igt@gem_softpin@noreloc-s3.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled:
    - shard-snb:          [SKIP][25] ([fdo#109271]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-snb7/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-snb6/igt@kms_draw_crc@draw-method-xrgb2101010-blt-untiled.html

  * igt@kms_flip@2x-flip-vs-wf_vblank-interruptible:
    - shard-hsw:          [SKIP][27] ([fdo#109271]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-hsw1/igt@kms_flip@2x-flip-vs-wf_vblank-interruptible.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-hsw2/igt@kms_flip@2x-flip-vs-wf_vblank-interruptible.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][29] ([fdo#105363]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - shard-iclb:         [FAIL][31] ([fdo#103167]) -> [PASS][32] +8 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][33] ([fdo#108566]) -> [PASS][34] +3 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-apl1/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-apl2/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-suspend:
    - shard-skl:          [INCOMPLETE][35] ([fdo#104108] / [fdo#106978]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl6/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl9/igt@kms_frontbuffer_tracking@fbcpsr-suspend.html

  * igt@kms_psr@psr2_no_drrs:
    - shard-iclb:         [SKIP][37] ([fdo#109441]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb8/igt@kms_psr@psr2_no_drrs.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb2/igt@kms_psr@psr2_no_drrs.html

  * igt@kms_setmode@basic:
    - shard-hsw:          [FAIL][39] ([fdo#99912]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-hsw7/igt@kms_setmode@basic.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-hsw1/igt@kms_setmode@basic.html

  * igt@perf@blocking:
    - shard-skl:          [FAIL][41] ([fdo#110728]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-skl8/igt@perf@blocking.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-skl2/igt@perf@blocking.html

  * igt@prime_self_import@export-vs-gem_close-race:
    - shard-hsw:          [INCOMPLETE][43] ([fdo#103540]) -> [PASS][44] +1 similar issue
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-hsw2/igt@prime_self_import@export-vs-gem_close-race.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-hsw1/igt@prime_self_import@export-vs-gem_close-race.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         [TIMEOUT][45] ([fdo#109673]) -> [INCOMPLETE][46] ([fdo#107713] / [fdo#109100])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6187/shard-iclb6/igt@gem_mmap_gtt@forked-big-copy-xy.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/shard-iclb5/igt@gem_mmap_gtt@forked-big-copy-xy.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#110042]: https://bugs.freedesktop.org/show_bug.cgi?id=110042
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110836]: https://bugs.freedesktop.org/show_bug.cgi?id=110836
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6187 -> Patchwork_13171

  CI_DRM_6187: 201dda6b2f7138214cdba69211c7504ce7b8b96e @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5037: a98c9cd50aa48933217ca41055279ccb1680d25b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13171: d666683ac70c9093030a9c5183bc48f18fe38b4a @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13171/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-05 10:40 ` [PATCH] " Tvrtko Ursulin
@ 2019-06-06  9:09   ` Chris Wilson
  2019-06-06  9:19     ` Tvrtko Ursulin
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2019-06-06  9:09 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
> 
> On 04/06/2019 16:24, Chris Wilson wrote:
> > The intent was to skip unused HW contexts by checking ce->state.
> > However, this only works for execlists where the ppGTT pointers is
> > stored inside the HW context. For gen7, the ppGTT is alongside the
> > logical state and must be updated on all active engines but, crucially,
> > only on active engines. As we need different checks, and to keep
> > context_barrier_task() agnostic, pass in the predicate.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
> > Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
> >   .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
> >   2 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 08721ef62e4e..6819b598d226 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
> >   I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
> >   static int context_barrier_task(struct i915_gem_context *ctx,
> >                               intel_engine_mask_t engines,
> > +                             bool (*skip)(struct intel_context *ce, void *data),
> >                               int (*emit)(struct i915_request *rq, void *data),
> >                               void (*task)(void *data),
> >                               void *data)
> > @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >                       break;
> >               }
> >   
> > -             if (!(ce->engine->mask & engines) || !ce->state)
> > +             if (!(ce->engine->mask & engines))
> > +                     continue;
> > +
> > +             if (skip && skip(ce, data))
> >                       continue;
> >   
> >               rq = intel_context_create_request(ce);
> > @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >       return 0;
> >   }
> >   
> > +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> > +{
> > +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> > +             return !ce->state;
> > +     else
> > +             return !atomic_read(&ce->pin_count);
> 
> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and 
> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?

No, because we need the barrier on gen7 !rcs which doesn't have
ce->state (but does need to switch mm).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-06  9:09   ` Chris Wilson
@ 2019-06-06  9:19     ` Tvrtko Ursulin
  2019-06-06 11:06       ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-06-06  9:19 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 06/06/2019 10:09, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
>>
>> On 04/06/2019 16:24, Chris Wilson wrote:
>>> The intent was to skip unused HW contexts by checking ce->state.
>>> However, this only works for execlists where the ppGTT pointers is
>>> stored inside the HW context. For gen7, the ppGTT is alongside the
>>> logical state and must be updated on all active engines but, crucially,
>>> only on active engines. As we need different checks, and to keep
>>> context_barrier_task() agnostic, pass in the predicate.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
>>> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
>>>    .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
>>>    2 files changed, 29 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> index 08721ef62e4e..6819b598d226 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
>>>    I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
>>>    static int context_barrier_task(struct i915_gem_context *ctx,
>>>                                intel_engine_mask_t engines,
>>> +                             bool (*skip)(struct intel_context *ce, void *data),
>>>                                int (*emit)(struct i915_request *rq, void *data),
>>>                                void (*task)(void *data),
>>>                                void *data)
>>> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>>>                        break;
>>>                }
>>>    
>>> -             if (!(ce->engine->mask & engines) || !ce->state)
>>> +             if (!(ce->engine->mask & engines))
>>> +                     continue;
>>> +
>>> +             if (skip && skip(ce, data))
>>>                        continue;
>>>    
>>>                rq = intel_context_create_request(ce);
>>> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>>>        return 0;
>>>    }
>>>    
>>> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
>>> +{
>>> +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
>>> +             return !ce->state;
>>> +     else
>>> +             return !atomic_read(&ce->pin_count);
>>
>> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and
>> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?
> 
> No, because we need the barrier on gen7 !rcs which doesn't have
> ce->state (but does need to switch mm).

That's not the path which would be covered by !atomic_read(&ce->pin_count) ?

Regards,

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-06  9:19     ` Tvrtko Ursulin
@ 2019-06-06 11:06       ` Chris Wilson
  2019-06-06 11:28         ` Tvrtko Ursulin
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2019-06-06 11:06 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2019-06-06 10:19:10)
> 
> On 06/06/2019 10:09, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
> >>
> >> On 04/06/2019 16:24, Chris Wilson wrote:
> >>> The intent was to skip unused HW contexts by checking ce->state.
> >>> However, this only works for execlists where the ppGTT pointers is
> >>> stored inside the HW context. For gen7, the ppGTT is alongside the
> >>> logical state and must be updated on all active engines but, crucially,
> >>> only on active engines. As we need different checks, and to keep
> >>> context_barrier_task() agnostic, pass in the predicate.
> >>>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
> >>> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
> >>>    .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
> >>>    2 files changed, 29 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> index 08721ef62e4e..6819b598d226 100644
> >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
> >>>    I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
> >>>    static int context_barrier_task(struct i915_gem_context *ctx,
> >>>                                intel_engine_mask_t engines,
> >>> +                             bool (*skip)(struct intel_context *ce, void *data),
> >>>                                int (*emit)(struct i915_request *rq, void *data),
> >>>                                void (*task)(void *data),
> >>>                                void *data)
> >>> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >>>                        break;
> >>>                }
> >>>    
> >>> -             if (!(ce->engine->mask & engines) || !ce->state)
> >>> +             if (!(ce->engine->mask & engines))
> >>> +                     continue;
> >>> +
> >>> +             if (skip && skip(ce, data))
> >>>                        continue;
> >>>    
> >>>                rq = intel_context_create_request(ce);
> >>> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >>>        return 0;
> >>>    }
> >>>    
> >>> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> >>> +{
> >>> +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> >>> +             return !ce->state;
> >>> +     else
> >>> +             return !atomic_read(&ce->pin_count);
> >>
> >> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and
> >> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?
> > 
> > No, because we need the barrier on gen7 !rcs which doesn't have
> > ce->state (but does need to switch mm).
> 
> That's not the path which would be covered by !atomic_read(&ce->pin_count) ?

And when pin_count > 0 it would then skip due to !ce->state, leading us
back to the current problem.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-06 11:06       ` Chris Wilson
@ 2019-06-06 11:28         ` Tvrtko Ursulin
  2019-06-06 11:37           ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-06-06 11:28 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 06/06/2019 12:06, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-06 10:19:10)
>>
>> On 06/06/2019 10:09, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
>>>>
>>>> On 04/06/2019 16:24, Chris Wilson wrote:
>>>>> The intent was to skip unused HW contexts by checking ce->state.
>>>>> However, this only works for execlists where the ppGTT pointers is
>>>>> stored inside the HW context. For gen7, the ppGTT is alongside the
>>>>> logical state and must be updated on all active engines but, crucially,
>>>>> only on active engines. As we need different checks, and to keep
>>>>> context_barrier_task() agnostic, pass in the predicate.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
>>>>> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
>>>>>     .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
>>>>>     2 files changed, 29 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>> index 08721ef62e4e..6819b598d226 100644
>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
>>>>>     I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
>>>>>     static int context_barrier_task(struct i915_gem_context *ctx,
>>>>>                                 intel_engine_mask_t engines,
>>>>> +                             bool (*skip)(struct intel_context *ce, void *data),
>>>>>                                 int (*emit)(struct i915_request *rq, void *data),
>>>>>                                 void (*task)(void *data),
>>>>>                                 void *data)
>>>>> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>>>>>                         break;
>>>>>                 }
>>>>>     
>>>>> -             if (!(ce->engine->mask & engines) || !ce->state)
>>>>> +             if (!(ce->engine->mask & engines))
>>>>> +                     continue;
>>>>> +
>>>>> +             if (skip && skip(ce, data))
>>>>>                         continue;
>>>>>     
>>>>>                 rq = intel_context_create_request(ce);
>>>>> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>>>>>         return 0;
>>>>>     }
>>>>>     
>>>>> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
>>>>> +{
>>>>> +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
>>>>> +             return !ce->state;
>>>>> +     else
>>>>> +             return !atomic_read(&ce->pin_count);
>>>>
>>>> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and
>>>> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?
>>>
>>> No, because we need the barrier on gen7 !rcs which doesn't have
>>> ce->state (but does need to switch mm).
>>
>> That's not the path which would be covered by !atomic_read(&ce->pin_count) ?
> 
> And when pin_count > 0 it would then skip due to !ce->state, leading us
> back to the current problem.

Brain fart.. Okay.. but pin_count check itself is not sufficient for 
both platforms? Can't we skip pin_count == 0 && ce->state != NULL on 
execlists?

Regards,

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-06 11:28         ` Tvrtko Ursulin
@ 2019-06-06 11:37           ` Chris Wilson
  2019-06-06 11:52             ` Tvrtko Ursulin
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2019-06-06 11:37 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2019-06-06 12:28:23)
> 
> On 06/06/2019 12:06, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-06-06 10:19:10)
> >>
> >> On 06/06/2019 10:09, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
> >>>>
> >>>> On 04/06/2019 16:24, Chris Wilson wrote:
> >>>>> The intent was to skip unused HW contexts by checking ce->state.
> >>>>> However, this only works for execlists where the ppGTT pointers is
> >>>>> stored inside the HW context. For gen7, the ppGTT is alongside the
> >>>>> logical state and must be updated on all active engines but, crucially,
> >>>>> only on active engines. As we need different checks, and to keep
> >>>>> context_barrier_task() agnostic, pass in the predicate.
> >>>>>
> >>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
> >>>>> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
> >>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>> ---
> >>>>>     drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
> >>>>>     .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
> >>>>>     2 files changed, 29 insertions(+), 5 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>>> index 08721ef62e4e..6819b598d226 100644
> >>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>>> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
> >>>>>     I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
> >>>>>     static int context_barrier_task(struct i915_gem_context *ctx,
> >>>>>                                 intel_engine_mask_t engines,
> >>>>> +                             bool (*skip)(struct intel_context *ce, void *data),
> >>>>>                                 int (*emit)(struct i915_request *rq, void *data),
> >>>>>                                 void (*task)(void *data),
> >>>>>                                 void *data)
> >>>>> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
> >>>>>                         break;
> >>>>>                 }
> >>>>>     
> >>>>> -             if (!(ce->engine->mask & engines) || !ce->state)
> >>>>> +             if (!(ce->engine->mask & engines))
> >>>>> +                     continue;
> >>>>> +
> >>>>> +             if (skip && skip(ce, data))
> >>>>>                         continue;
> >>>>>     
> >>>>>                 rq = intel_context_create_request(ce);
> >>>>> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >>>>>         return 0;
> >>>>>     }
> >>>>>     
> >>>>> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> >>>>> +{
> >>>>> +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> >>>>> +             return !ce->state;
> >>>>> +     else
> >>>>> +             return !atomic_read(&ce->pin_count);
> >>>>
> >>>> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and
> >>>> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?
> >>>
> >>> No, because we need the barrier on gen7 !rcs which doesn't have
> >>> ce->state (but does need to switch mm).
> >>
> >> That's not the path which would be covered by !atomic_read(&ce->pin_count) ?
> > 
> > And when pin_count > 0 it would then skip due to !ce->state, leading us
> > back to the current problem.
> 
> Brain fart.. Okay.. but pin_count check itself is not sufficient for 
> both platforms? Can't we skip pin_count == 0 && ce->state != NULL on 
> execlists?

Alas not. It is really is a divergence in HW behaviour.

For execlists, we need to modifying an existing context image, but may
skip if there is no image at all (as it will be constructed with the
right registers).

For gen7, we need to take care of the supplementary pinning of
ctx->ppgtt. That is done for us on the first context_pin, but when
pin_count > 0, we must fiddle. Fiddly fiddling. There's probably a way
to resolve it without having to pull so much lower level detail into
i915_gem_context.c, I am not seeing it atm (and honestly not looking too
hard ;).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: Skip context_barrier emission for unused contexts
  2019-06-06 11:37           ` Chris Wilson
@ 2019-06-06 11:52             ` Tvrtko Ursulin
  0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2019-06-06 11:52 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 06/06/2019 12:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-06-06 12:28:23)
>>
>> On 06/06/2019 12:06, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2019-06-06 10:19:10)
>>>>
>>>> On 06/06/2019 10:09, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2019-06-05 11:40:27)
>>>>>>
>>>>>> On 04/06/2019 16:24, Chris Wilson wrote:
>>>>>>> The intent was to skip unused HW contexts by checking ce->state.
>>>>>>> However, this only works for execlists where the ppGTT pointers is
>>>>>>> stored inside the HW context. For gen7, the ppGTT is alongside the
>>>>>>> logical state and must be updated on all active engines but, crucially,
>>>>>>> only on active engines. As we need different checks, and to keep
>>>>>>> context_barrier_task() agnostic, pass in the predicate.
>>>>>>>
>>>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110836
>>>>>>> Fixes: 62c8e423450d ("drm/i915: Skip unused contexts for context_barrier_task()")
>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/i915/gem/i915_gem_context.c   | 15 ++++++++++++++-
>>>>>>>      .../drm/i915/gem/selftests/i915_gem_context.c | 19 +++++++++++++++----
>>>>>>>      2 files changed, 29 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>>>> index 08721ef62e4e..6819b598d226 100644
>>>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>>>>>>> @@ -902,6 +902,7 @@ static void cb_retire(struct i915_active *base)
>>>>>>>      I915_SELFTEST_DECLARE(static intel_engine_mask_t context_barrier_inject_fault);
>>>>>>>      static int context_barrier_task(struct i915_gem_context *ctx,
>>>>>>>                                  intel_engine_mask_t engines,
>>>>>>> +                             bool (*skip)(struct intel_context *ce, void *data),
>>>>>>>                                  int (*emit)(struct i915_request *rq, void *data),
>>>>>>>                                  void (*task)(void *data),
>>>>>>>                                  void *data)
>>>>>>> @@ -931,7 +932,10 @@ static int context_barrier_task(struct i915_gem_context *ctx,
>>>>>>>                          break;
>>>>>>>                  }
>>>>>>>      
>>>>>>> -             if (!(ce->engine->mask & engines) || !ce->state)
>>>>>>> +             if (!(ce->engine->mask & engines))
>>>>>>> +                     continue;
>>>>>>> +
>>>>>>> +             if (skip && skip(ce, data))
>>>>>>>                          continue;
>>>>>>>      
>>>>>>>                  rq = intel_context_create_request(ce);
>>>>>>> @@ -1058,6 +1062,14 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>>>>>>>          return 0;
>>>>>>>      }
>>>>>>>      
>>>>>>> +static bool skip_ppgtt_update(struct intel_context *ce, void *data)
>>>>>>> +{
>>>>>>> +     if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
>>>>>>> +             return !ce->state;
>>>>>>> +     else
>>>>>>> +             return !atomic_read(&ce->pin_count);
>>>>>>
>>>>>> Would "return !atomic_read(&ce->pin_count) || !ce->state;" work and
>>>>>> avoid the somewhat irky HAS_LOGICAL_RING_CONTEXTS check?
>>>>>
>>>>> No, because we need the barrier on gen7 !rcs which doesn't have
>>>>> ce->state (but does need to switch mm).
>>>>
>>>> That's not the path which would be covered by !atomic_read(&ce->pin_count) ?
>>>
>>> And when pin_count > 0 it would then skip due to !ce->state, leading us
>>> back to the current problem.
>>
>> Brain fart.. Okay.. but pin_count check itself is not sufficient for
>> both platforms? Can't we skip pin_count == 0 && ce->state != NULL on
>> execlists?
> 
> Alas not. It is really is a divergence in HW behaviour.
> 
> For execlists, we need to modifying an existing context image, but may
> skip if there is no image at all (as it will be constructed with the
> right registers).
> 
> For gen7, we need to take care of the supplementary pinning of
> ctx->ppgtt. That is done for us on the first context_pin, but when
> pin_count > 0, we must fiddle. Fiddly fiddling. There's probably a way
> to resolve it without having to pull so much lower level detail into
> i915_gem_context.c, I am not seeing it atm (and honestly not looking too
> hard ;).

Okay, I don't fancy looking too hard either. It was only a discussion 
about a potential optimisation to begin with. So without much further ado:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-06-06 11:53 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-04 15:24 [PATCH] drm/i915: Skip context_barrier emission for unused contexts Chris Wilson
2019-06-04 18:26 ` ✓ Fi.CI.BAT: success for " Patchwork
2019-06-05 10:40 ` [PATCH] " Tvrtko Ursulin
2019-06-06  9:09   ` Chris Wilson
2019-06-06  9:19     ` Tvrtko Ursulin
2019-06-06 11:06       ` Chris Wilson
2019-06-06 11:28         ` Tvrtko Ursulin
2019-06-06 11:37           ` Chris Wilson
2019-06-06 11:52             ` Tvrtko Ursulin
2019-06-05 17:40 ` ✓ Fi.CI.IGT: success for " Patchwork

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.