Per context dynamic (sub)slice power-gating

From: Tvrtko Ursulin <[email protected]>

Configuring RPCS in context image just before pin is sufficient and will come extra handy in one of the following patches.

v2:
* Split image setup a bit differently. (Chris Wilson)

Signed-off-by Tvrtko Ursulin <[email protected]>

Suggested-by Chris Wilson <[email protected]>

Cc Chris Wilson <[email protected]>

From: Chris Wilson <[email protected]>

We want to expose the ability to reconfigure the slices, subslice and eu per context and per engine. To facilitate that, store the current configuration on the context for each engine, which is initially set to the device default upon creation.

v2: record sseu configuration per context & engine (Chris)

v3: introduce the i915_gem_context_sseu to store powergating
programming, sseu_dev_info has grown quite a bit (Lionel)

v4: rename i915_gem_sseu into intel_sseu (Chris)
use to_intel_context() (Chris)

v5: More to_intel_context() (Tvrtko)
Switch intel_sseu from union to struct (Tvrtko) Move context default sseu in existing loop (Chris)

v6: s/intel_sseu_from_device_sseu/intel_device_default_sseu/ (Tvrtko)

Tvrtko Ursulin:

v7:
* Pass intel_sseu by pointer instead of value to make_rpcs.
* Rebase for make_rpcs changes.

v8:
* Rebase for RPCS edit on pin.

v9:
* Rebase for context image setup changes.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

Quoting Tvrtko Ursulin (2018-09-05 15:22:18)

Quoted 30 lines

From: Chris Wilson <[email protected]>

We want to expose the ability to reconfigure the slices, subslice and eu per context and per engine. To facilitate that, store the current configuration on the context for each engine, which is initially set to the device default upon creation.

v2: record sseu configuration per context & engine (Chris)

v3: introduce the i915_gem_context_sseu to store powergating
programming, sseu_dev_info has grown quite a bit (Lionel)

v4: rename i915_gem_sseu into intel_sseu (Chris)
use to_intel_context() (Chris)

v5: More to_intel_context() (Tvrtko)
Switch intel_sseu from union to struct (Tvrtko) Move context default sseu in existing loop (Chris)

v6: s/intel_sseu_from_device_sseu/intel_device_default_sseu/ (Tvrtko)

Tvrtko Ursulin:

v7:
* Pass intel_sseu by pointer instead of value to make_rpcs.
* Rebase for make_rpcs changes.

v8:
* Rebase for RPCS edit on pin.

Quoted 6 lines

v9:
* Rebase for context image setup changes.

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

I feel this is substantially different (since I just outlined a v1!) to merit a

Reviewed-by Chris Wilson <[email protected]>

and probably deserves a different author. I think Lionel is still the principle author here, but Tvrtko has done a lot of refactoring and integrating in the new scheme.

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -1

-static u32 make_rpcs(struct drm_i915_private *dev_priv);
+static u32 make_rpcs(struct drm_i915_private *dev_priv,
+                    struct intel_sseu *ctx_sseu);

drivers/gpu/drm/i915/intel_lrc.c __execlists_context_pin(struct intel_engine_cs *engine, · line 1353

Quoted 8 lines

static struct intel_context * __execlists_context_pin(struct intel_engine_cs *engine,

drivers/gpu/drm/i915/intel_lrc.c +1 -1

@@ -1349,7 +1350,7 @@ __execlists_context_pin(struct intel_engine_cs *engine,
         /* RPCS */
         if (engine->class == RENDER_CLASS) {
                 ce->lrc_reg_state[CTX_R_PWR_CLK_STATE + 1] =
-                                               make_rpcs(engine->i915);
+                                       make_rpcs(engine->i915, &ce->sseu);

We have different habits here; my vim config just gives this a single tab indent beyond the incomplete line. (Was going to say it earlier ;)

 {
        const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
        bool subslice_pg = sseu->has_subslice_pg;

drivers/gpu/drm/i915/intel_lrc.c +2 -2

-       u8 slices = hweight8(ctx_sseu->slice_mask);
-       u8 subslices = hweight8(ctx_sseu->subslice_mask);
+       struct intel_sseu ctx_sseu;
+       u8 slices, subslices;
        u32 rpcs = 0;

Quoting Tvrtko Ursulin (2018-09-05 15:22:19)

drivers/gpu/drm/i915/intel_lrc.c int logical_xcs_ring_init(struct intel_engine_cs *engine) · line 2495

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +2 -2

-static u32 make_rpcs(struct drm_i915_private *dev_priv,
-                    struct intel_sseu *ctx_sseu)
+u32 gen8_make_rpcs(struct drm_i915_private *dev_priv,
+                  struct intel_sseu *req_sseu)

Should we retrospectively make this const?

(And anychance for a s/dev_priv/i915?)

Quoted 8 lines

  {
         const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
         bool subslice_pg = sseu->has_subslice_pg;

drivers/gpu/drm/i915/intel_lrc.c +2 -2

-       u8 slices = hweight8(ctx_sseu->slice_mask);
-       u8 subslices = hweight8(ctx_sseu->subslice_mask);
+       struct intel_sseu ctx_sseu;
+       u8 slices, subslices;
         u32 rpcs = 0;

drivers/gpu/drm/i915/intel_lrc.c int logical_xcs_ring_init(struct intel_engine_cs *engine) · line 2511

Quoted 10 lines

drivers/gpu/drm/i915/intel_lrc.c +9 -0

 
+       /*
+        * If i915/perf is active, we want a stable powergating configuration
+        * on the system. The most natural configuration to take in that case
+        * is the default (i.e maximum the hardware can do).
+        */
+       if (unlikely(dev_priv->perf.oa.exclusive_stream))
+               ctx_sseu = intel_device_default_sseu(dev_priv);
+       else
+               ctx_sseu = *req_sseu;

:(

I'm not sure if I can suggest anything better, but this does feel like a layering violation.

It makes sense which makes it only feel worse.

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
* Rebase for make_rpcs changes.

v5:
* Apply OA restriction from make_rpcs directly.

v6:
* Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_perf.c |  5 +++++
  drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
  drivers/gpu/drm/i915/intel_lrc.h |  3 +++
  3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c +0 -0

  }
 
  /*

drivers/gpu/drm/i915/intel_lrc.c +20 -10

index 8a477e43dbca..9709c1fbe836 100644
@@ -1305,9 +1305,6 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
  	return i915_vma_pin(vma, 0, 0, flags);
  }
 
-static u32 make_rpcs(struct drm_i915_private *dev_priv,
-		     struct intel_sseu *ctx_sseu);
-
  static struct intel_context *
  __execlists_context_pin(struct intel_engine_cs *engine,
  			struct i915_gem_context *ctx,

@@ -1350,7 +1347,7 @@ __execlists_context_pin(struct intel_engine_cs *engine,
  	/* RPCS */
  	if (engine->class == RENDER_CLASS) {
  		ce->lrc_reg_state[CTX_R_PWR_CLK_STATE + 1] =
-					make_rpcs(engine->i915, &ce->sseu);
+					gen8_make_rpcs(engine->i915, &ce->sseu);
  	}
 
  	ce->state->obj->pin_global++;

@@ -2494,15 +2491,28 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine)
  	return logical_ring_init(engine);
  }
 
-static u32 make_rpcs(struct drm_i915_private *dev_priv,
-		     struct intel_sseu *ctx_sseu)
+u32 gen8_make_rpcs(struct drm_i915_private *dev_priv,
+		   struct intel_sseu *req_sseu)
  {
  	const struct sseu_dev_info *sseu = &INTEL_INFO(dev_priv)->sseu;
  	bool subslice_pg = sseu->has_subslice_pg;
-	u8 slices = hweight8(ctx_sseu->slice_mask);
-	u8 subslices = hweight8(ctx_sseu->subslice_mask);
+	struct intel_sseu ctx_sseu;
+	u8 slices, subslices;
  	u32 rpcs = 0;
 
+	/*
+	 * If i915/perf is active, we want a stable powergating configuration
+	 * on the system. The most natural configuration to take in that case
+	 * is the default (i.e maximum the hardware can do).
+	 */
+	if (unlikely(dev_priv->perf.oa.exclusive_stream))
+		ctx_sseu = intel_device_default_sseu(dev_priv);
+	else
+		ctx_sseu = *req_sseu;
+
+	slices = hweight8(ctx_sseu.slice_mask);
+	subslices = hweight8(ctx_sseu.subslice_mask);
+
  	/*
  	 * Since the SScount bitfield in GEN8_R_PWR_CLK_STATE is only three bits
  	 * wide and Icelake has up to eight subslices, specfial programming is

@@ -2572,13 +2582,13 @@ static u32 make_rpcs(struct drm_i915_private *dev_priv,
  	if (sseu->has_eu_pg) {
  		u32 val;
 
-		val = ctx_sseu->min_eus_per_subslice << GEN8_RPCS_EU_MIN_SHIFT;
+		val = ctx_sseu.min_eus_per_subslice << GEN8_RPCS_EU_MIN_SHIFT;
  		GEM_BUG_ON(val & ~GEN8_RPCS_EU_MIN_MASK);
  		val &= GEN8_RPCS_EU_MIN_MASK;
 
  		rpcs |= val;
 
-		val = ctx_sseu->max_eus_per_subslice << GEN8_RPCS_EU_MAX_SHIFT;
+		val = ctx_sseu.max_eus_per_subslice << GEN8_RPCS_EU_MAX_SHIFT;
  		GEM_BUG_ON(val & ~GEN8_RPCS_EU_MAX_MASK);
  		val &= GEN8_RPCS_EU_MAX_MASK;
 

drivers/gpu/drm/i915/intel_lrc.h +0 -0

index f5a5502ecf70..11da6fc0002d 100644

Quoted 61 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
* Rebase for make_rpcs changes.

v5:
* Apply OA restriction from make_rpcs directly.

v6:
* Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_perf.c |  5 +++++
  drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
  drivers/gpu/drm/i915/intel_lrc.h |  3 +++
  3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx, · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
 
              CTX_REG(reg_state, state_offset, flex_regs[i], value);
      }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Context save might happen after we edited the image and that would override the values we just put in there.

The OA config is emitted through context image edition in this function but also through the ring buffer in gen8_switch_to_updated_kernel_context() for the kernel context.

Since we can't have a context modify its own RCPS value, we'll have to resort to yet another context to do that for the kernel context.

I remember having a patch that created yet another kernel context (let's call it rpcs edition context), which is used to reconfigure rpcs for every context but itself and then have the kernel context reconfigure this rpcs edition context.

Or alternatively not do anything to it, because it's only going to run to edit other contexts at a time when we don't care about power configuration stability.

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 74 lines · Tvrtko Ursulin

Quoted 61 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
* Rebase for make_rpcs changes.

v5:
* Apply OA restriction from make_rpcs directly.

v6:
* Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
   drivers/gpu/drm/i915/i915_perf.c |  5 +++++
   drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
   drivers/gpu/drm/i915/intel_lrc.h |  3 +++
   3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx, · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
 
               CTX_REG(reg_state, state_offset, flex_regs[i], value);
       }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

Quoted 5 lines

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

Quoted 18 lines

Context save might happen after we edited the image and that would override the values we just put in there.

The OA config is emitted through context image edition in this function but also through the ring buffer in gen8_switch_to_updated_kernel_context() for the kernel context.

Since we can't have a context modify its own RCPS value, we'll have to resort to yet another context to do that for the kernel context.

I remember having a patch that created yet another kernel context (let's call it rpcs edition context), which is used to reconfigure rpcs for every context but itself and then have the kernel context reconfigure this rpcs edition context.

Quoted 3 lines

Or alternatively not do anything to it, because it's only going to run to edit other contexts at a time when we don't care about power configuration stability.

Exactly.
-Chris

Quoted 75 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 74 lines · Tvrtko Ursulin

Quoted 61 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
* Rebase for make_rpcs changes.

v5:
* Apply OA restriction from make_rpcs directly.

v6:
* Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
   drivers/gpu/drm/i915/i915_perf.c |  5 +++++
   drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
   drivers/gpu/drm/i915/intel_lrc.h |  3 +++
   3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx, · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
 
               CTX_REG(reg_state, state_offset, flex_regs[i], value);
       }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

Quoted 7 lines

Quoted 5 lines

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 80 lines · Chris Wilson

Quoted 79 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 78 lines · Tvrtko Ursulin

Quoted 61 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel)
More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
* Rebase for make_rpcs changes.

v5:
* Apply OA restriction from make_rpcs directly.

v6:
* Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30 ++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++
    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx, · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
 
                CTX_REG(reg_state, state_offset, flex_regs[i], value);
        }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 91 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 82 lines · Chris Wilson

Quoted 81 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 80 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1685

Quoted 9 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              
 dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Quoted 8 lines

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Quoted 106 lines · Chris Wilson

Quoted 96 lines

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 87 lines · Chris Wilson

Quoted 86 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 85 lines · Tvrtko Ursulin

Quoted 66 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard
the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard
the user requested configuration for the period of time when i915/perf
is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1686

Quoted 10 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], 
 value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+                            &to_intel_context(ctx,
+                                              
 dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Quoted 8 lines

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 4 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value.
Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Quoted 100 lines · Lionel Landwerlin

Quoted 98 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 81 lines · Chris Wilson

Quoted 80 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 79 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Regards,

Tvrtko

Quoted 102 lines · Tvrtko Ursulin

Quoted 100 lines · Lionel Landwerlin

Quoted 98 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 81 lines · Chris Wilson

Quoted 80 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 79 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1684

Quoted 8 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 8 lines

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Let's say you have contextA with sseu(slice,subslice)=(0x1/0xff) for ICL.

You then enable OA which locks the configuration at (0x1,0xf).

The kernel context has retained its (0x1/0xff) configuration.

And after you change the config of contextA to (0x1,0x7).

This would lead to the kernel context scheduled with (0x1,0xff) while OA is active.

Quoted 8 lines

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

Quoted 1 line

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Documentation says : "This register must not be programmed directly through CPU MMIO cycle."

Sorry :(

Quoted 121 lines · Lionel Landwerlin

Quoted 103 lines · Tvrtko Ursulin

Quoted 101 lines · Lionel Landwerlin

Quoted 99 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 82 lines · Chris Wilson

Quoted 81 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 80 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1685

Quoted 9 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, 
 GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 8 lines

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Let's say you have contextA with sseu(slice,subslice)=(0x1/0xff) for ICL.

You then enable OA which locks the configuration at (0x1,0xf).

The kernel context has retained its (0x1/0xff) configuration.

Quoted 4 lines

And after you change the config of contextA to (0x1,0x7).

This would lead to the kernel context scheduled with (0x1,0xff) while OA is active.

Okay that's a problem discussed in the paragraph below - that the kernel context is not updated at all. But is it a problem for OA? Will it mess up some counters even if kernel context isn't executing anything interacting with them? Or is it?

Quoted 12 lines

Quoted 10 lines

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Quoted 4 lines

Documentation says : "This register must not be programmed directly through CPU MMIO cycle."

Sorry :(

Ugh.. okay, help me understand if kernel context absolutely needs to follow the "lock" for OA to work and then we'll see what to do.

Regards,

Tvrtko

Quoted 123 lines · Tvrtko Ursulin

Quoted 121 lines · Lionel Landwerlin

Quoted 103 lines · Tvrtko Ursulin

Quoted 101 lines · Lionel Landwerlin

Quoted 99 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 82 lines · Chris Wilson

Quoted 81 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 80 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1685

Quoted 9 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, 
 GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 8 lines

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Let's say you have contextA with sseu(slice,subslice)=(0x1/0xff) for ICL.

You then enable OA which locks the configuration at (0x1,0xf).

The kernel context has retained its (0x1/0xff) configuration.

Quoted 6 lines

Quoted 4 lines

And after you change the config of contextA to (0x1,0x7).

This would lead to the kernel context scheduled with (0x1,0xff) while OA is active.

Okay that's a problem discussed in the paragraph below - that the kernel context is not updated at all. But is it a problem for OA? Will it mess up some counters even if kernel context isn't executing anything interacting with them? Or is it?

What the HW is going to do to the NOA logic in power configuration changes is not really documented.
Experimentally on SKL GT4, it seems a change in power configuration will trigger a power off of everything before applying the power at the new configuration. So that would imply loosing the NOA programming when we switch to the kernel context which means invalid values in the counters.

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Documentation says : "This register must not be programmed directly through CPU MMIO cycle."

Quoted 3 lines

Quoted 1 line

Sorry :(

Ugh.. okay, help me understand if kernel context absolutely needs to follow the "lock" for OA to work and then we'll see what to do.

I think so.

Your idea of a throw away context to reprogramming every seems sound.

Quoted 152 lines · Lionel Landwerlin

Quoted 123 lines · Tvrtko Ursulin

Quoted 121 lines · Lionel Landwerlin

Quoted 103 lines · Tvrtko Ursulin

Quoted 101 lines · Lionel Landwerlin

Quoted 99 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 82 lines · Chris Wilson

Quoted 81 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 80 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1685

Quoted 9 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, 
 GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 8 lines

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Let's say you have contextA with sseu(slice,subslice)=(0x1/0xff) for ICL.

You then enable OA which locks the configuration at (0x1,0xf).

The kernel context has retained its (0x1/0xff) configuration.

Quoted 6 lines

Quoted 4 lines

And after you change the config of contextA to (0x1,0x7).

This would lead to the kernel context scheduled with (0x1,0xff) while OA is active.

Okay that's a problem discussed in the paragraph below - that the kernel context is not updated at all. But is it a problem for OA? Will it mess up some counters even if kernel context isn't executing anything interacting with them? Or is it?

What the HW is going to do to the NOA logic in power configuration changes is not really documented.
Experimentally on SKL GT4, it seems a change in power configuration will trigger a power off of everything before applying the power at the new configuration. So that would imply loosing the NOA programming when we switch to the kernel context which means invalid values in the counters.

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Documentation says : "This register must not be programmed directly through CPU MMIO cycle."

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Sorry :(

Ugh.. okay, help me understand if kernel context absolutely needs to follow the "lock" for OA to work and then we'll see what to do.

I think so.

Your idea of a throw away context to reprogramming every seems sound.

I was in the middle of refactoring the series to implement this when I started suspecting the need for it.

Premise of the problem statement was the kernel context doesn't get updated by the OA code, but, there is a loop in gen8_configure_all_contexts which goes through exactly all of them after it has idled the GPU.

AFAICS that means it is able to map the kernel context state and edit it so everything seems fine from this angle. (Kernel context is perma-pinned in software, but after idling the GPU we know it is not actually on the GPU so it safe to edit it's image.)

Am I missing some hole here, and if not, why does the code needs to have a primary update via a request in gen8_switch_to_updated_kernel_context? Context image after idling seems again sufficient.

Would it be possible to exercise the hypothetical loss of NOA configuration from an IGT, if kernel context wasn't correctly updated?

Regards,

Tvrtko

Quoted 166 lines · Tvrtko Ursulin

Quoted 152 lines · Lionel Landwerlin

Quoted 123 lines · Tvrtko Ursulin

Quoted 121 lines · Lionel Landwerlin

Quoted 103 lines · Tvrtko Ursulin

Quoted 101 lines · Lionel Landwerlin

Quoted 99 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-09-06 11:18:01)

Quoted 82 lines · Chris Wilson

Quoted 81 lines

Quoting Lionel Landwerlin (2018-09-06 10:57:47)

Quoted 80 lines · Tvrtko Ursulin

Quoted 62 lines

From: Lionel Landwerlin <[email protected]>

If some of the contexts submitting workloads to the GPU have been configured to shutdown slices/subslices, we might loose the NOA configurations written in the NOA muxes.

One possible solution to this problem is to reprogram the NOA muxes when we switch to a new context. We initially tried this in the workaround batchbuffer but some concerns where raised about the cost of reprogramming at every context switch. This solution is also not without consequences from the userspace point of view. Reprogramming of the muxes can only happen once the powergating configuration has changed (which happens after context switch). This means for a window of time during the recording, counters recorded by the OA unit might be invalid. This requires userspace dealing with OA reports to discard the invalid values.

Minimizing the reprogramming could be implemented by tracking of the last programmed configuration somewhere in GGTT and use MI_PREDICATE to discard some of the programming commands, but the command streamer would still have to parse all the MI_LRI instructions in the workaround batchbuffer.

Another solution, which this change implements, is to simply disregard the user requested configuration for the period of time when i915/perf is active. There is no known issue with this apart from a performance penality for some media workloads that benefit from running on a partially powergated GPU. We already prevent RC6 from affecting the programming so it doesn't sound completely unreasonable to hold on powergating for the same reason.

v2: Leave RPCS programming in intel_lrc.c (Lionel)

v3: Update for s/union intel_sseu/struct intel_sseu/ (Lionel) More to_intel_context() (Tvrtko) s/dev_priv/i915/ (Tvrtko)

Tvrtko Ursulin:

v4:
    * Rebase for make_rpcs changes.

v5:
    * Apply OA restriction from make_rpcs directly.

v6:
    * Rebase for context image setup changes.

Signed-off-by Lionel Landwerlin <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
    drivers/gpu/drm/i915/i915_perf.c |  5 +++++
    drivers/gpu/drm/i915/intel_lrc.c | 30
++++++++++++++++++++----------
    drivers/gpu/drm/i915/intel_lrc.h |  3 +++

    3 files changed, 28 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/i915_perf.c +0 -0

index ccb20230df2c..dd65b72bddd4 100644

drivers/gpu/drm/i915/i915_perf.c static void · line 1685

Quoted 9 lines

drivers/gpu/drm/i915/i915_perf.c +5 -0

@@ -1677,6 +1677,11 @@ static void 
 gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                 CTX_REG(reg_state, state_offset, flex_regs[i], value);
         }
+
+     CTX_REG(reg_state, CTX_R_PWR_CLK_STATE, 
 GEN8_R_PWR_CLK_STATE,
+             gen8_make_rpcs(dev_priv,
+ &to_intel_context(ctx,
+ dev_priv->engine[RCS])->sseu));

I think there is one issue I missed on the previous iterations of this patch.

This gen8_update_reg_state_unlocked() is called when the GPU is parked on the kernel context.

It's supposed to update all contexts, but I think we might not be able to update the kernel context image while the GPU is using it.

Quoted 8 lines

Quoted 2 lines

The kernel context is only ever taken in extremis (you are either parking or stalling userspace) so I don't care.

The patch exposing the RPCS configuration to userspace will make use of the kernel context while OA/perf is enabled. Even if it reprograms the locked value that will break the power configuration stability on Gen11 (because the locked configuration will be different from the kernel context configuration).

Sure, but as you point out that's only on changing configuration.

What's missing in the patch is that we only bail early if the new sseu matches the ce->sseu, but that doesn't necessarily match whats in the context due to OA. (Or maybe I missed the conversion to rpcs value and checking.)
-Chris

Quoted 8 lines

Quoted 2 lines

Yep, because the gen8_make_rpcs() post processes the values store at the gem context level, we risk rerunning the kernel context to write the exiting value. Sorry this is all so messy :(

Lets see if I managed to follow here.

The current code indeed bails out at the set ctx param level if the requested state matches the ce->state. My thinking was that ce->state is the master state and whatever happens in "post processing" via gen8_make_rpcs should be hidden from it since the design is that the i915_perf.c will re-configure all contexts when the OA active status changes (to either direction).

So I don't see a problem in those two interactions.

Let's say you have contextA with sseu(slice,subslice)=(0x1/0xff) for ICL.

You then enable OA which locks the configuration at (0x1,0xf).

The kernel context has retained its (0x1/0xff) configuration.

Quoted 6 lines

Quoted 4 lines

And after you change the config of contextA to (0x1,0x7).

This would lead to the kernel context scheduled with (0x1,0xff) while OA is active.

Okay that's a problem discussed in the paragraph below - that the kernel context is not updated at all. But is it a problem for OA? Will it mess up some counters even if kernel context isn't executing anything interacting with them? Or is it?

What the HW is going to do to the NOA logic in power configuration changes is not really documented.
Experimentally on SKL GT4, it seems a change in power configuration will trigger a power off of everything before applying the power at the new configuration. So that would imply loosing the NOA programming when we switch to the kernel context which means invalid values in the counters.

Quoted 15 lines

Quoted 15 lines

Quoted 10 lines

Apart from one, get_param_sseu will lie a bit - we can discuss about this one more. At one point I suggested we have two sets of masks in the uAPI, requested and active in a way. So userspace could query what it set and what is actually active.

Now second issue is if i915_perf.c is able to reprogram the kernel config.

Here its true, it will write to the context image and that will get overwritten by context save.

If that is a problem for OA, I was initially if a throw-away second "kernel" context could be use to re-program the real one, but perhaps even simpler - what about a mmio write to program the RPCS while kernel context is active?

Documentation says : "This register must not be programmed directly through CPU MMIO cycle."

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

Sorry :(

Ugh.. okay, help me understand if kernel context absolutely needs to follow the "lock" for OA to work and then we'll see what to do.

I think so.

Your idea of a throw away context to reprogramming every seems sound.

I was in the middle of refactoring the series to implement this when I started suspecting the need for it.

Premise of the problem statement was the kernel context doesn't get updated by the OA code, but, there is a loop in gen8_configure_all_contexts which goes through exactly all of them after it has idled the GPU.

Quoted 3 lines

AFAICS that means it is able to map the kernel context state and edit it so everything seems fine from this angle. (Kernel context is perma-pinned in software, but after idling the GPU we know it is not actually on the GPU so it safe to edit it's image.)

Am I missing some hole here, and if not, why does the code needs to have a primary update via a request in gen8_switch_to_updated_kernel_context? Context image after idling seems again sufficient.

My understanding is that the current code puts the GPU idle on the kernel context.
When idling stops, the GPU will save the last values from the HW register into the kernel context image.
We currently don't see any issue because we also run a set of commands on the kernel context prior to idle that mirror the contexts image edition.

That won't be the case for the RPCS register because we can't load it from the command streamer.

Quoted 2 lines

Would it be possible to exercise the hypothetical loss of NOA configuration from an IGT, if kernel context wasn't correctly updated?

The test configs saved in the kernel use NOA and their counters are progressing at different ratios from the "core clock" (RING_TIMESTAMP register). In IGT tests/perf.c, the gen8_sanity_check_test_oa_reports() verify that the counters progress as expected.

But I can't tell from what part of the GT the test configs are sourcing signals from. If they do from unslice, then that won't work :(

From: Tvrtko Ursulin <[email protected]>

Timeline barrier allows serialization between different timelines.

After calling i915_timeline_set_barrier with a request, all following submissions on this timeline will be set up as depending on this request, or barrier. Once the barrier has been completed it automatically gets cleared and things continue as normal.

This facility will be used by the upcoming context SSEU code.

v2:
* Assert barrier has been retired on timeline_fini. (Chris Wilson)
* Fix mock_timeline.

Signed-off-by Tvrtko Ursulin <[email protected]>

Suggested-by Chris Wilson <[email protected]>

Cc Chris Wilson <[email protected]>

From: Chris Wilson <[email protected]>

We want to allow userspace to reconfigure the subslice configuration for its own use case. To do so, we expose a context parameter to allow adjustment of the RPCS register stored within the context image (and currently not accessible via LRI). If the context is adjusted before first use, the adjustment is for "free"; otherwise if the context is active we flush the context off the GPU (stalling all users) and forcing the GPU to save the context to memory where we can modify it and so ensure that the register is reloaded on next execution.

The overhead of managing additional EU subslices can be significant, especially in multi-context workloads. Non-GPGPU contexts should preferably disable the subslices it is not using, and others should fine-tune the number to match their workload.

We expose complete control over the RPCS register, allowing configuration of slice/subslice, via masks packed into a u64 for simplicity. For example,

        struct drm_i915_gem_context_param arg;
        struct drm_i915_gem_context_param_sseu sseu = { .class = 0,
                                                        .instance = 0, };

        memset(&arg, 0, sizeof(arg));
        arg.ctx_id = ctx;
        arg.param = I915_CONTEXT_PARAM_SSEU;
        arg.value = (uintptr_t) &sseu;
        if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &arg) == 0) {
                sseu.packed.subslice_mask = 0;

                drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &arg);
        }

could be used to disable all subslices where supported.

v2: Fix offset of CTX_R_PWR_CLK_STATE in intel_lr_context_set_sseu() (Lionel)

v3: Add ability to program this per engine (Chris)

v4: Move most get_sseu() into i915_gem_context.c (Lionel)

v5: Validate sseu configuration against the device's capabilities (Lionel)

v6: Change context powergating settings through MI_SDM on kernel context (Chris)

v7: Synchronize the requests following a powergating setting change using a global
dependency (Chris)
Iterate timelines through dev_priv.gt.active_rings (Tvrtko) Disable RPCS configuration setting for non capable users (Lionel/Tvrtko)

v8: s/union intel_sseu/struct intel_sseu/ (Lionel)
s/dev_priv/i915/ (Tvrtko)
Change uapi class/instance fields to u16 (Tvrtko) Bump mask fields to 64bits (Lionel)
Don't return EPERM when dynamic sseu is disabled (Tvrtko)

v9: Import context image into kernel context's ppgtt only when
reconfiguring powergated slice/subslices (Chris) Use aliasing ppgtt when needed (Michel)

Tvrtko Ursulin:

v10:
* Update for upstream changes.
* Request submit needs a RPM reference.
* Reject on !FULL_PPGTT for simplicity.
* Pull out get/set param to helpers for readability and less indent.
* Use i915_request_await_dma_fence in add_global_barrier to skip waits on the same timeline and avoid GEM_BUG_ON.
* No need to explicitly assign a NULL pointer to engine in legacy mode.
* No need to move gen8_make_rpcs up.
* Factored out global barrier as prep patch.
* Allow to only CAP_SYS_ADMIN if !Gen11.

v11:
* Remove engine vfunc in favour of local helper. (Chris Wilson)
* Stop retiring requests before updates since it is not needed (Chris Wilson)
* Implement direct CPU update path for idle contexts. (Chris Wilson)
* Left side dependency needs only be on the same context timeline. (Chris Wilson)
* It is sufficient to order the timeline. (Chris Wilson)
* Reject !RCS configuration attempts with -ENODEV for now.

v12:
* Rebase for make_rpcs.

v13:
* Centralize SSEU normalization to make_rpcs.
* Type width checking (uAPI <-> implementation).
* Gen11 restrictions uAPI checks.
* Gen11 subslice count differences handling.
Chris Wilson:
* args->size handling fixes.
* Update context image from GGTT.
* Postpone context image update to pinning.
* Use i915_gem_active_raw instead of last_request_on_engine.

v14:
* Add activity tracker on intel_context to fix the lifetime issues and simplify the code. (Chris Wilson)

v15:
* Fix context pin leak if no space in ring by simplifying the context pinning sequence.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100899
Issue: https://github.com/intel/media-driver/issues/267

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Lionel Landwerlin <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Zhipeng Gong <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
 drivers/gpu/drm/i915/i915_gem_context.c | 303 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_gem_context.h |   6 +
 drivers/gpu/drm/i915/intel_lrc.c        |   4 +-
 include/uapi/drm/i915_drm.h             |  43 ++++
 4 files changed, 353 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_gem_context.c +53 -0

index ca2c8fcd1090..aa1f34e63080 100644
@@ -90,6 +90,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "i915_trace.h"
+#include "intel_lrc_reg.h"
 #include "intel_workarounds.h"
 
 #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1

@@ -322,6 +323,14 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
        return desc;
 }
 
+static void intel_context_retire(struct i915_gem_active *active,
+                                struct i915_request *rq)
+{
+       struct intel_context *ce = container_of(active, typeof(*ce), active);
+
+       intel_context_unpin(ce);
+}
+
 static struct i915_gem_context *
 __create_hw_context(struct drm_i915_private *dev_priv,
                    struct drm_i915_file_private *file_priv)

@@ -345,6 +354,8 @@ __create_hw_context(struct drm_i915_private *dev_priv,
                ce->gem_context = ctx;
                /* Use the whole device by default */
                ce->sseu = intel_device_default_sseu(dev_priv);
+
+               init_request_active(&ce->active, intel_context_retire);
        }
 
        INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);

@@ -846,6 +857,48 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
        return 0;
 }
 
+static int get_sseu(struct i915_gem_context *ctx,
+                   struct drm_i915_gem_context_param *args)
+{
+       struct drm_i915_gem_context_param_sseu user_sseu;
+       struct intel_engine_cs *engine;
+       struct intel_context *ce;
+
+       if (args->size == 0)
+               goto out;
+       else if (args->size < sizeof(user_sseu))
+               return -EINVAL;
+
+       if (copy_from_user(&user_sseu, u64_to_user_ptr(args->value),
+                          sizeof(user_sseu)))
+               return -EFAULT;
+
+       if (user_sseu.rsvd1 || user_sseu.rsvd2)
+               return -EINVAL;
+
+       engine = intel_engine_lookup_user(ctx->i915,
+                                         user_sseu.class,
+                                         user_sseu.instance);
+       if (!engine)
+               return -EINVAL;
+
+       ce = to_intel_context(ctx, engine);
+
+       user_sseu.slice_mask = ce->sseu.slice_mask;
+       user_sseu.subslice_mask = ce->sseu.subslice_mask;
+       user_sseu.min_eus_per_subslice = ce->sseu.min_eus_per_subslice;
+       user_sseu.max_eus_per_subslice = ce->sseu.max_eus_per_subslice;
+
+       if (copy_to_user(u64_to_user_ptr(args->value), &user_sseu,
+                        sizeof(user_sseu)))
+               return -EFAULT;
+
+out:
+       args->size = sizeof(user_sseu);
+
+       return 0;
+}
+
 int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                                    struct drm_file *file)
 {

drivers/gpu/drm/i915/i915_gem_context.c +3 -0

        switch (args->param) {
        case I915_CONTEXT_PARAM_BAN_PERIOD:
                ret = -EINVAL;
                break;
        case I915_CONTEXT_PARAM_NO_ZEROMAP:
+               args->size = 0;
                args->value = ctx->flags & CONTEXT_NO_ZEROMAP;
                break;
        case I915_CONTEXT_PARAM_GTT_SIZE:
+               args->size = 0;
+
                if (ctx->ppgtt)
                        args->value = ctx->ppgtt->vm.total;
                else if (to_i915(dev)->mm.aliasing_ppgtt)

int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{

drivers/gpu/drm/i915/i915_gem_context.c +3 -1

@@ -957,7 +1254,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                                ctx->sched.priority = priority;
                }
                break;
-
+       case I915_CONTEXT_PARAM_SSEU:
+               ret = set_sseu(ctx, args);
+               break;
        default:
                ret = -EINVAL;
                break;

drivers/gpu/drm/i915/i915_gem_context.h +6 -0

index 79d2e8f62ad1..968e1d47d944 100644
@@ -165,6 +165,12 @@ struct i915_gem_context {
                u64 lrc_desc;
                int pin_count;
 
+               /**
+                * active: Active tracker for the external rq activity on this
+                * intel_context object.
+                */
+               struct i915_gem_active active;
+
                const struct intel_context_ops *ops;
 
                /** sseu: Control eu/slice partitioning */

drivers/gpu/drm/i915/intel_lrc.c +0 -0

index 9709c1fbe836..3c85392a3109 100644

Quoting Tvrtko Ursulin (2018-09-05 15:22:21)

Quoted 1 line

From: Chris Wilson <[email protected]>

Now this looks nothing like my first suggestion!

I think Tvrtko should stand ad the author of the final mechanism, I think it is substantially different from the submission method first done by Lionel.

Quoted 210 lines

We want to allow userspace to reconfigure the subslice configuration for its own use case. To do so, we expose a context parameter to allow adjustment of the RPCS register stored within the context image (and currently not accessible via LRI). If the context is adjusted before first use, the adjustment is for "free"; otherwise if the context is active we flush the context off the GPU (stalling all users) and forcing the GPU to save the context to memory where we can modify it and so ensure that the register is reloaded on next execution.

The overhead of managing additional EU subslices can be significant, especially in multi-context workloads. Non-GPGPU contexts should preferably disable the subslices it is not using, and others should fine-tune the number to match their workload.

We expose complete control over the RPCS register, allowing configuration of slice/subslice, via masks packed into a u64 for simplicity. For example,

         struct drm_i915_gem_context_param arg;
         struct drm_i915_gem_context_param_sseu sseu = { .class = 0,
                                                         .instance = 0, };

         memset(&arg, 0, sizeof(arg));
         arg.ctx_id = ctx;
         arg.param = I915_CONTEXT_PARAM_SSEU;
         arg.value = (uintptr_t) &sseu;
         if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, &arg) == 0) {
                 sseu.packed.subslice_mask = 0;

                 drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &arg);
         }

could be used to disable all subslices where supported.

v2: Fix offset of CTX_R_PWR_CLK_STATE in intel_lr_context_set_sseu() (Lionel)

v3: Add ability to program this per engine (Chris)

v4: Move most get_sseu() into i915_gem_context.c (Lionel)

v5: Validate sseu configuration against the device's capabilities (Lionel)

v6: Change context powergating settings through MI_SDM on kernel context (Chris)

v7: Synchronize the requests following a powergating setting change using a global
dependency (Chris)
Iterate timelines through dev_priv.gt.active_rings (Tvrtko) Disable RPCS configuration setting for non capable users (Lionel/Tvrtko)

v8: s/union intel_sseu/struct intel_sseu/ (Lionel)
s/dev_priv/i915/ (Tvrtko)
Change uapi class/instance fields to u16 (Tvrtko) Bump mask fields to 64bits (Lionel)
Don't return EPERM when dynamic sseu is disabled (Tvrtko)

v9: Import context image into kernel context's ppgtt only when
reconfiguring powergated slice/subslices (Chris) Use aliasing ppgtt when needed (Michel)

Tvrtko Ursulin:

v10:
* Update for upstream changes.
* Request submit needs a RPM reference.
* Reject on !FULL_PPGTT for simplicity.
* Pull out get/set param to helpers for readability and less indent.
* Use i915_request_await_dma_fence in add_global_barrier to skip waits on the same timeline and avoid GEM_BUG_ON.
* No need to explicitly assign a NULL pointer to engine in legacy mode.
* No need to move gen8_make_rpcs up.
* Factored out global barrier as prep patch.
* Allow to only CAP_SYS_ADMIN if !Gen11.

v11:
* Remove engine vfunc in favour of local helper. (Chris Wilson)
* Stop retiring requests before updates since it is not needed (Chris Wilson)
* Implement direct CPU update path for idle contexts. (Chris Wilson)
* Left side dependency needs only be on the same context timeline. (Chris Wilson)
* It is sufficient to order the timeline. (Chris Wilson)
* Reject !RCS configuration attempts with -ENODEV for now.

v12:
* Rebase for make_rpcs.

v13:
* Centralize SSEU normalization to make_rpcs.
* Type width checking (uAPI <-> implementation).
* Gen11 restrictions uAPI checks.
* Gen11 subslice count differences handling.
Chris Wilson:
* args->size handling fixes.
* Update context image from GGTT.
* Postpone context image update to pinning.
* Use i915_gem_active_raw instead of last_request_on_engine.

v14:
* Add activity tracker on intel_context to fix the lifetime issues and simplify the code. (Chris Wilson)

v15:
* Fix context pin leak if no space in ring by simplifying the context pinning sequence.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100899
Issue: https://github.com/intel/media-driver/issues/267

Signed-off-by Chris Wilson <[email protected]>

Signed-off-by Lionel Landwerlin <[email protected]>

Cc Dmitry Rogozhkin <[email protected]>

Cc Tvrtko Ursulin <[email protected]>

Cc Zhipeng Gong <[email protected]>

Cc Joonas Lahtinen <[email protected]>

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_gem_context.c | 303 +++++++++++++++++++++++-
  drivers/gpu/drm/i915/i915_gem_context.h |   6 +
  drivers/gpu/drm/i915/intel_lrc.c        |   4 +-
  include/uapi/drm/i915_drm.h             |  43 ++++
  4 files changed, 353 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/i915_gem_context.c +53 -0

index ca2c8fcd1090..aa1f34e63080 100644
@@ -90,6 +90,7 @@
  #include <drm/i915_drm.h>
  #include "i915_drv.h"
  #include "i915_trace.h"
+#include "intel_lrc_reg.h"
  #include "intel_workarounds.h"
 
  #define ALL_L3_SLICES(dev) (1 << NUM_L3_SLICES(dev)) - 1

@@ -322,6 +323,14 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
         return desc;
  }
 
+static void intel_context_retire(struct i915_gem_active *active,
+                                struct i915_request *rq)
+{
+       struct intel_context *ce = container_of(active, typeof(*ce), active);
+
+       intel_context_unpin(ce);
+}
+
  static struct i915_gem_context *
  __create_hw_context(struct drm_i915_private *dev_priv,
                     struct drm_i915_file_private *file_priv)

@@ -345,6 +354,8 @@ __create_hw_context(struct drm_i915_private *dev_priv,
                 ce->gem_context = ctx;
                 /* Use the whole device by default */
                 ce->sseu = intel_device_default_sseu(dev_priv);
+
+               init_request_active(&ce->active, intel_context_retire);
         }
 
         INIT_RADIX_TREE(&ctx->handles_vma, GFP_KERNEL);

@@ -846,6 +857,48 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
         return 0;
  }
 
+static int get_sseu(struct i915_gem_context *ctx,
+                   struct drm_i915_gem_context_param *args)
+{
+       struct drm_i915_gem_context_param_sseu user_sseu;
+       struct intel_engine_cs *engine;
+       struct intel_context *ce;
+
+       if (args->size == 0)
+               goto out;
+       else if (args->size < sizeof(user_sseu))
+               return -EINVAL;
+
+       if (copy_from_user(&user_sseu, u64_to_user_ptr(args->value),
+                          sizeof(user_sseu)))
+               return -EFAULT;
+
+       if (user_sseu.rsvd1 || user_sseu.rsvd2)
+               return -EINVAL;
+
+       engine = intel_engine_lookup_user(ctx->i915,
+                                         user_sseu.class,
+                                         user_sseu.instance);
+       if (!engine)
+               return -EINVAL;
+
+       ce = to_intel_context(ctx, engine);
+
+       user_sseu.slice_mask = ce->sseu.slice_mask;
+       user_sseu.subslice_mask = ce->sseu.subslice_mask;
+       user_sseu.min_eus_per_subslice = ce->sseu.min_eus_per_subslice;
+       user_sseu.max_eus_per_subslice = ce->sseu.max_eus_per_subslice;
+
+       if (copy_to_user(u64_to_user_ptr(args->value), &user_sseu,
+                        sizeof(user_sseu)))
+               return -EFAULT;
+
+out:
+       args->size = sizeof(user_sseu);
+
+       return 0;
+}
+
  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                                     struct drm_file *file)
  {

@@ -858,15 +911,17 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
         if (!ctx)
                 return -ENOENT;
 

drivers/gpu/drm/i915/i915_gem_context.c int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data, · line 861

Quoted 1 line

drivers/gpu/drm/i915/i915_gem_context.c +0 -1

-       args->size = 0;

I've a slight preference to setting to 0 then overwriting it afterwards.

Quoted 45 lines

drivers/gpu/drm/i915/i915_gem_context.c +3 -0

         switch (args->param) {
         case I915_CONTEXT_PARAM_BAN_PERIOD:
                 ret = -EINVAL;
                 break;
         case I915_CONTEXT_PARAM_NO_ZEROMAP:
+               args->size = 0;
                 args->value = ctx->flags & CONTEXT_NO_ZEROMAP;
                 break;
         case I915_CONTEXT_PARAM_GTT_SIZE:
+               args->size = 0;
+
                 if (ctx->ppgtt)
                         args->value = ctx->ppgtt->vm.total;
                 else if (to_i915(dev)->mm.aliasing_ppgtt)

int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{

drivers/gpu/drm/i915/i915_gem_context.c +3 -1

@@ -957,7 +1254,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                                 ctx->sched.priority = priority;
                 }
                 break;
-
+       case I915_CONTEXT_PARAM_SSEU:
+               ret = set_sseu(ctx, args);
+               break;
         default:
                 ret = -EINVAL;
                 break;

drivers/gpu/drm/i915/i915_gem_context.h +6 -0

index 79d2e8f62ad1..968e1d47d944 100644
@@ -165,6 +165,12 @@ struct i915_gem_context {
                 u64 lrc_desc;
                 int pin_count;
 
+               /**
+                * active: Active tracker for the external rq activity on this
+                * intel_context object.
+                */
+               struct i915_gem_active active;
+
                 const struct intel_context_ops *ops;
 

drivers/gpu/drm/i915/intel_lrc.c u32 gen8_make_rpcs(struct drm_i915_private *dev_priv, · line 2543

Quoted 13 lines

drivers/gpu/drm/i915/i915_gem_context.h +0 -0

                 /** sseu: Control eu/slice partitioning */

drivers/gpu/drm/i915/intel_lrc.c +3 -1

index 9709c1fbe836..3c85392a3109 100644
@@ -2538,7 +2538,9 @@ u32 gen8_make_rpcs(struct drm_i915_private *dev_priv,
          * subslices are enabled, or a count between one and four on the first
          * slice.
          */
-       if (IS_GEN11(dev_priv) && slices == 1 && subslices >= 4) {
+       if (IS_GEN11(dev_priv) &&
+           slices == 1 &&
+           subslices > min_t(u8, 4, hweight8(sseu->subslice_mask[0]) / 2)) {

Sneaky. Is this a direct consequence of exposing sseu to the user, or should argue the protection required irrespective of who fill sseu?

drivers/gpu/drm/i915/intel_lrc.c u32 gen8_make_rpcs(struct drm_i915_private *dev_priv, · line 2546

Quoted 4 lines

drivers/gpu/drm/i915/intel_lrc.c +0 -0

                 GEM_BUG_ON(subslices & 1);
 
                 subslice_pg = false;

For the rq mechanics after all the hassle I gave Tvrtko,

Reviewed-by Chris Wilson <[email protected]>

I didn't look closely at the validation layer.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 1 line

I've a slight preference to setting to 0 then overwriting it afterwards.

We can't use/validate it then. Alternative to just not clear it for ABI where it is not used? In other words would go away from the case branches completely. Does the ABI depend on it being zeroed on return from get_param? It would be strange..

Quoted 3 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-09-05 15:22:21)

Quoted 1 line

From: Chris Wilson <[email protected]>

Quoted 5 lines

Now this looks nothing like my first suggestion!

I think Tvrtko should stand ad the author of the final mechanism, I think it is substantially different from the submission method first done by Lionel.

Okay I'll relieve you from authorship on this one. Not sure who between Lionel and me with, but I'll think of something.

From: Tvrtko Ursulin <[email protected]>

Updated series after continuing Lionel's work.

Userspace for the feature is the media-driver project on GitHub. Please see
https://github.com/intel/media-driver/pull/271/commits.

No headline changes this time.

Some review feedback, some refactoring, some patches got merged and two new appeared to help with the simplified implementation and also lock SSEU config to a workable set on Icelake.

Quoting Tvrtko Ursulin (2018-09-05 15:22:15)

Quoted 4 lines

From: Tvrtko Ursulin <[email protected]>

Updated series after continuing Lionel's work.

Quoted 8 lines

Userspace for the feature is the media-driver project on GitHub. Please see
https://github.com/intel/media-driver/pull/271/commits.

No headline changes this time.

Some review feedback, some refactoring, some patches got merged and two new appeared to help with the simplified implementation and also lock SSEU config to a workable set on Icelake.

Scanning through the buglist, caught this little gem
https://bugs.freedesktop.org/show_bug.cgi?id=103484
in which Lionel mentioned that he wanted to fix up as part of this series. The conclusion is that we can remove the i915_sseu_status debugfs if we are happy with the testing and runtime adjustment.