Per context dynamic (sub)slice power-gating

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)

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

---
  drivers/gpu/drm/i915/i915_drv.h  | 15 +++++++++++++++
  drivers/gpu/drm/i915/i915_perf.c | 23 ++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.c | 11 +++++++----
  drivers/gpu/drm/i915/intel_lrc.h |  3 +++
  4 files changed, 43 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +15 -0

index d6049c3f911b..5c12d2676435 100644
@@ -3851,4 +3851,19 @@ static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
  		return I915_HWS_CSB_WRITE_INDEX;
  }
 
+static inline struct intel_sseu
+intel_engine_prepare_sseu(struct intel_engine_cs *engine,
+			  struct intel_sseu sseu)
+{
+	struct drm_i915_private *i915 = engine->i915;
+
+	/*
+	 * 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).
+	 */
+	return i915->perf.oa.exclusive_stream ?
+		intel_device_default_sseu(i915) : sseu;
+}
+
  #endif

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

index ccb20230df2c..c2fc2399e0ed 100644
@@ -1631,7 +1631,8 @@ static void hsw_disable_metric_set(struct drm_i915_private *dev_priv)
   */
  static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
  					   u32 *reg_state,
-					   const struct i915_oa_config *oa_config)
+					   const struct i915_oa_config *oa_config,
+					   struct intel_sseu sseu)
  {
  	struct drm_i915_private *dev_priv = ctx->i915;
  	u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset;

@@ -1677,6 +1678,9 @@ 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(&INTEL_INFO(dev_priv)->sseu, sseu));
  }
 
  /*

@@ -1807,6 +1811,7 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr
  static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
  				       const struct i915_oa_config *oa_config)
  {
+	struct intel_sseu default_sseu = intel_device_default_sseu(dev_priv);
  	struct intel_engine_cs *engine = dev_priv->engine[RCS];
  	struct i915_gem_context *ctx;
  	int ret;

@@ -1854,7 +1859,8 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
  		ce->state->obj->mm.dirty = true;
  		regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
 
-		gen8_update_reg_state_unlocked(ctx, regs, oa_config);
+		gen8_update_reg_state_unlocked(ctx, regs, oa_config,
+					       oa_config ? default_sseu : ce->sseu);
 
  		i915_gem_object_unpin_map(ce->state->obj);
  	}

@@ -2226,14 +2232,21 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
  			    struct i915_gem_context *ctx,
  			    u32 *reg_state)
  {
+	struct drm_i915_private *i915 = engine->i915;
  	struct i915_perf_stream *stream;
 
  	if (engine->id != RCS)
  		return;
 
-	stream = engine->i915->perf.oa.exclusive_stream;
-	if (stream)
-		gen8_update_reg_state_unlocked(ctx, reg_state, stream->oa_config);
+	stream = i915->perf.oa.exclusive_stream;
+	if (stream) {
+		struct intel_sseu default_sseu =
+			intel_device_default_sseu(i915);
+
+		gen8_update_reg_state_unlocked(ctx, reg_state,
+					       stream->oa_config,
+					       default_sseu);
+	}
  }
 
  /**

drivers/gpu/drm/i915/intel_lrc.c +7 -4

index 7b2f2d6bb057..8a2997be7ef7 100644
@@ -2484,8 +2484,8 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine)
  	return logical_ring_init(engine);
  }
 
-static u32 make_rpcs(const struct sseu_dev_info *sseu,
-		     struct intel_sseu ctx_sseu)
+u32 gen8_make_rpcs(const struct sseu_dev_info *sseu,
+		   struct intel_sseu ctx_sseu)
  {
  	u32 rpcs = 0;
 

@@ -2635,10 +2635,13 @@ static void execlists_init_reg_state(u32 *regs,
  	}
 
  	if (rcs) {
+		struct intel_sseu sseu = to_intel_context(ctx, engine)->sseu;
+
  		regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
  		CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
-			make_rpcs(&INTEL_INFO(dev_priv)->sseu,
-				  to_intel_context(ctx, engine)->sseu));
+			gen8_make_rpcs(&INTEL_INFO(dev_priv)->sseu,
+				       intel_engine_prepare_sseu(engine,
+								 sseu)));
 
  		i915_oa_init_reg_state(engine, ctx, regs);
  	}

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

index f5a5502ecf70..bf3acdc3d0af 100644

Hey Tvrtko,

Thanks for taking over this series.

I've been talking to developers using the i915/perf interface and from their point of view, they expect the system to be in a stable configuration when doing measurements.

One issue with this patch on Gen11 is that it will lock the system in a configuration that isn't workable for media workloads (all subslices enabled).
So I think we should set the value for the locked configuration per generation (gen8-10: all slices/subslices, gen11: only subslices that contain VME samplers) so that we always get a functional configurations for all workloads. Could we want to select that configuration when opening perf?

Another question is how do we expose the selected value to the user. But that can be solved in a different series.

Cheers,

-
Lionel

Quoted 175 lines · Tvrtko Ursulin

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)

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

---
  drivers/gpu/drm/i915/i915_drv.h  | 15 +++++++++++++++

  drivers/gpu/drm/i915/i915_perf.c | 23 ++++++++++++++++++-----
  drivers/gpu/drm/i915/intel_lrc.c | 11 +++++++----
  drivers/gpu/drm/i915/intel_lrc.h |  3 +++
  4 files changed, 43 insertions(+), 9 deletions(-)

drivers/gpu/drm/i915/i915_drv.h +8 -0

index d6049c3f911b..5c12d2676435 100644
@@ -3851,4 +3851,19 @@ static inline int 
 intel_hws_csb_write_index(struct drm_i915_private *i915)
           return I915_HWS_CSB_WRITE_INDEX;
   }
+static inline struct intel_sseu
+intel_engine_prepare_sseu(struct intel_engine_cs *engine,
+              struct intel_sseu sseu)
+{
+    struct drm_i915_private *i915 = engine->i915;
+
+    /*
+     * 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). +     */
+    return i915->perf.oa.exclusive_stream ?
+        intel_device_default_sseu(i915) : sseu;

drivers/gpu/drm/i915/i915_drv.h +2 -0

+}
+

  #endif

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

index ccb20230df2c..c2fc2399e0ed 100644
@@ -1631,7 +1631,8 @@ static void hsw_disable_metric_set(struct 
 drm_i915_private *dev_priv)
    */
   static void gen8_update_reg_state_unlocked(struct i915_gem_context *ctx,
                          u32 *reg_state,
-                       const struct i915_oa_config *oa_config)
+                       const struct i915_oa_config *oa_config,
+                       struct intel_sseu sseu)
   {
       struct drm_i915_private *dev_priv = ctx->i915;

      u32 ctx_oactxctrl = dev_priv->perf.oa.ctx_oactxctrl_offset;

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

@@ -1677,6 +1678,9 @@ 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(&INTEL_INFO(dev_priv)->sseu, sseu));
   }
   /*

@@ -1807,6 +1811,7 @@ static int 

gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv, const struct i915_oa_config *oa_config) {
+    struct intel_sseu default_sseu = intel_device_default_sseu(dev_priv);
      struct intel_engine_cs *engine = dev_priv->engine[RCS]; struct i915_gem_context *ctx;
      int ret;

drivers/gpu/drm/i915/i915_perf.c +12 -4

@@ -1854,7 +1859,8 @@ static int gen8_configure_all_contexts(struct 
 drm_i915_private *dev_priv,
           ce->state->obj->mm.dirty = true;
           regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
-        gen8_update_reg_state_unlocked(ctx, regs, oa_config);
+        gen8_update_reg_state_unlocked(ctx, regs, oa_config,
+                           oa_config ? default_sseu : ce->sseu);
           i915_gem_object_unpin_map(ce->state->obj);
       }

@@ -2226,14 +2232,21 @@ void i915_oa_init_reg_state(struct 
 intel_engine_cs *engine,
                   struct i915_gem_context *ctx,
                   u32 *reg_state)
   {
+    struct drm_i915_private *i915 = engine->i915;
       struct i915_perf_stream *stream;
       if (engine->id != RCS)
           return;
-    stream = engine->i915->perf.oa.exclusive_stream;
-    if (stream)
-        gen8_update_reg_state_unlocked(ctx, reg_state, 
 stream->oa_config);
+    stream = i915->perf.oa.exclusive_stream;
+    if (stream) {
+        struct intel_sseu default_sseu =
+            intel_device_default_sseu(i915);
+
+        gen8_update_reg_state_unlocked(ctx, reg_state,
+                           stream->oa_config,
+                           default_sseu);
+    }
   }
   /**

drivers/gpu/drm/i915/intel_lrc.c +7 -4

index 7b2f2d6bb057..8a2997be7ef7 100644
@@ -2484,8 +2484,8 @@ int logical_xcs_ring_init(struct intel_engine_cs 
 *engine)
       return logical_ring_init(engine);
   }
-static u32 make_rpcs(const struct sseu_dev_info *sseu,
-             struct intel_sseu ctx_sseu)
+u32 gen8_make_rpcs(const struct sseu_dev_info *sseu,
+           struct intel_sseu ctx_sseu)
   {
       u32 rpcs = 0;

@@ -2635,10 +2635,13 @@ static void execlists_init_reg_state(u32 *regs,
       }
       if (rcs) {
+        struct intel_sseu sseu = to_intel_context(ctx, engine)->sseu;
+
           regs[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
           CTX_REG(regs, CTX_R_PWR_CLK_STATE, GEN8_R_PWR_CLK_STATE,
-            make_rpcs(&INTEL_INFO(dev_priv)->sseu,
-                  to_intel_context(ctx, engine)->sseu));
+            gen8_make_rpcs(&INTEL_INFO(dev_priv)->sseu,
+                       intel_engine_prepare_sseu(engine,
+                                 sseu)));
           i915_oa_init_reg_state(engine, ctx, regs);
       }

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

index f5a5502ecf70..bf3acdc3d0af 100644
@@ -104,4 +104,7 @@ void intel_lr_context_resume(struct 
 drm_i915_private *dev_priv);
   void intel_execlists_set_default_submission(struct intel_engine_cs *engine);
+u32 gen8_make_rpcs(const struct sseu_dev_info *sseu,
+           struct intel_sseu ctx_sseu);

drivers/gpu/drm/i915/intel_lrc.h void intel_lr_context_resume(struct drm_i915_private *dev_priv); · line 110

Quoted 2 lines

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

+
   #endif /* _INTEL_LRC_H_ */

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 9 lines · Lionel Landwerlin

Hey Tvrtko,

Thanks for taking over this series.

I've been talking to developers using the i915/perf interface and from their point of view, they expect the system to be in a stable configuration when doing measurements.

One issue with this patch on Gen11 is that it will lock the system in a configuration that isn't workable for media workloads (all subslices enabled).
So I think we should set the value for the locked configuration per generation (gen8-10: all slices/subslices, gen11: only subslices that contain VME samplers) so that we always get a functional configurations for all workloads. Could we want to select that configuration when opening perf?

This would be via i915_perf.c/gen8_configure_all_contexts?

Sounds like an unfortunate but workable compromise. As long as there doesn't appear another asymmetric slice feature in the future, which doesn't align with VME. Like one workloads wants slices 0 & 2, another wants 1 & 3, or whatever. If that happens then I don't know what we do apart from locking out perf/OA.

Quoted 1 line

Another question is how do we expose the selected value to the user. But that can be solved in a different series.

SSEU get_param would cut it? Although it would be perhaps be unexpected to get different results depending on whether perf/OA is active or not.. Hm... export device and available bitmasks via get param? Device bitmask would be fixed and available would change depending on whether perf/OA is active or not.

Quoted 12 lines · Tvrtko Ursulin

Quoted 9 lines · Lionel Landwerlin

Hey Tvrtko,

Thanks for taking over this series.

I've been talking to developers using the i915/perf interface and from their point of view, they expect the system to be in a stable configuration when doing measurements.

One issue with this patch on Gen11 is that it will lock the system in a configuration that isn't workable for media workloads (all subslices enabled).
So I think we should set the value for the locked configuration per generation (gen8-10: all slices/subslices, gen11: only subslices that contain VME samplers) so that we always get a functional configurations for all workloads. Could we want to select that configuration when opening perf?

Quoted 7 lines

This would be via i915_perf.c/gen8_configure_all_contexts?

Sounds like an unfortunate but workable compromise. As long as there doesn't appear another asymmetric slice feature in the future, which doesn't align with VME. Like one workloads wants slices 0 & 2, another wants 1 & 3, or whatever. If that happens then I don't know what we do apart from locking out perf/OA.

Quoted 1 line

Another question is how do we expose the selected value to the user. But that can be solved in a different series.

SSEU get_param would cut it? Although it would be perhaps be unexpected to get different results depending on whether perf/OA is active or not.. Hm... export device and available bitmasks via get param? Device bitmask would be fixed and available would change depending on whether perf/OA is active or not.

Big downside to this is that observing something via OA would cause slow down of the whole system - since the SSEU config would be locked to a subset of slices on Gen11. :( Regardless or not of the details.. say even if we choose to lock only if there is one non-default SSEU context, that still locks all whilst perf/OA is active.

That's quite bad I think. But the alternative is to lock out perf/OA while non-default SSEU contexts are present. Which is also very bad. :(

Overall I don't see a solution. Latter is maybe better by being more explicit? Who are the perf/OA users and what would they prefer?

Regards,

Tvrtko

From: Tvrtko Ursulin <[email protected]>

Global barrier is a facility to allow serialization between different timelines.

After calling i915_gem_set_global_barrier on a request, all following submissions on any engine will be set up as depending on this global barrier. Once the global barrier has been completed it automatically gets cleared and things continue as normal.

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

-------------------------------------------------------------------------
This code was part of the larger SSEU patch but I extracted it to be separate for ease of review and clarity. I think it originates from Chris Wilson so permission pending I will change the author and add appropriate S-o-B.
-------------------------------------------------------------------------

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

Cc Chris Wilson <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.h     | 27 +++++++++++++++++++++++++++
  drivers/gpu/drm/i915/i915_gem.c     |  2 ++
  drivers/gpu/drm/i915/i915_request.c | 16 ++++++++++++++++
  3 files changed, 45 insertions(+)

drivers/gpu/drm/i915/i915_drv.h +27 -0

index 5c12d2676435..643089ba01b9 100644
@@ -2098,6 +2098,16 @@ struct drm_i915_private {
  		u32 active_requests;
  		u32 request_serial;
 
+		/**
+		 * Global barrier for the ability to serialize ordering between
+		 * different timelines.
+		 *
+		 * Users can call i915_gem_set_global_barrier which will make
+		 * all subsequent submission be execute only after this barrier
+		 * has been completed.
+		 */
+		struct i915_gem_active global_barrier;
+
  		/**
  		 * Is the GPU currently considered idle, or busy executing
  		 * userspace requests? Whilst idle, we allow runtime power

@@ -3230,6 +3240,23 @@ i915_vm_to_ppgtt(struct i915_address_space *vm)
  	return container_of(vm, struct i915_hw_ppgtt, vm);
  }
 
+/**
+ * i915_gem_set_global_barrier - orders submission on different timelines
+ * @i915: i915 device private
+ * @rq: request after which new submissions can proceed
+ *
+ * Sets the passed in request as the serialization point for all subsequent
+ * submissions, regardless of the engine/timeline. Subsequent requests will not
+ * be submitted to GPU until the global barrier has been completed.
+ */
+static inline void
+i915_gem_set_global_barrier(struct drm_i915_private *i915,
+			    struct i915_request *rq)
+{
+	lockdep_assert_held(&i915->drm.struct_mutex);
+	i915_gem_active_set(&i915->gt.global_barrier, rq);
+}
+
  /* i915_gem_fence_reg.c */
  struct drm_i915_fence_reg *
  i915_reserve_fence(struct drm_i915_private *dev_priv);

drivers/gpu/drm/i915/i915_gem.c +2 -0

index 0453eb42a1a3..be462ef65786 100644
@@ -5752,6 +5752,8 @@ int i915_gem_init_early(struct drm_i915_private *dev_priv)
  	if (!dev_priv->priorities)
  		goto err_dependencies;
 
+	init_request_active(&dev_priv->gt.global_barrier, NULL);
+
  	INIT_LIST_HEAD(&dev_priv->gt.timelines);
  	INIT_LIST_HEAD(&dev_priv->gt.active_rings);
  	INIT_LIST_HEAD(&dev_priv->gt.closed_vma);

drivers/gpu/drm/i915/i915_request.c +12 -0

index 09ed48833b54..8b45f74dc748 100644
@@ -644,6 +644,18 @@ submit_notify(struct i915_sw_fence *fence, enum i915_sw_fence_notify state)
  	return NOTIFY_DONE;
  }
 
+static int add_global_barrier(struct i915_request *rq)
+{
+	struct i915_request *barrier;
+
+	barrier = i915_gem_active_raw(&rq->i915->gt.global_barrier,
+				      &rq->i915->drm.struct_mutex);
+	if (barrier)
+		return i915_request_await_dma_fence(rq, &barrier->fence);
+
+	return 0;
+}
+
  /**
   * i915_request_alloc - allocate a request structure
   *

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.

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 | 187 +++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_lrc.c        |  55 +++++++
 drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
 include/uapi/drm/i915_drm.h             |  43 ++++++
 4 files changed, 288 insertions(+), 1 deletion(-)

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

index 8a12984e7495..6d6220634e9e 100644
@@ -773,6 +773,91 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
        return 0;
 }
 
+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;

And I'd still recommend not using indirect access if we can apply the changes immediately.
-Chris

Quoted 4 lines

And I'd still recommend not using indirect access if we can apply the changes immediately.
-Chris

Hangs on Gen9 :(

Quoting Lionel Landwerlin (2018-08-14 16:11:36)

Quoted 6 lines · Chris Wilson

Quoted 4 lines

And I'd still recommend not using indirect access if we can apply the changes immediately.
-Chris

Hangs on Gen9 :(

How does modifying the context image of an idle (unpinned) context cause a hang?
-Chris

Quoted 7 lines · Chris Wilson

Quoting Lionel Landwerlin (2018-08-14 16:11:36)

Quoted 6 lines · Chris Wilson

Quoted 4 lines

And I'd still recommend not using indirect access if we can apply the changes immediately.
-Chris

Hangs on Gen9 :(

Quoted 4 lines

How does modifying the context image of an idle (unpinned) context cause a hang?
-Chris

I thought you meant a context modifying it's own RPCS register... no?

-
Lionel

Quoting Tvrtko Ursulin (2018-08-14 15:40:58)

Quoted 109 lines

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.

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 | 187 +++++++++++++++++++++++-
  drivers/gpu/drm/i915/intel_lrc.c        |  55 +++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
  include/uapi/drm/i915_drm.h             |  43 ++++++
  4 files changed, 288 insertions(+), 1 deletion(-)

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

index 8a12984e7495..6d6220634e9e 100644
@@ -773,6 +773,91 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
         return 0;
  }
 
+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;
+       struct i915_request *rq;
+       struct intel_ring *ring;
+       int ret;
+

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

Quoted 6 lines

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

+       lockdep_assert_held(&i915->drm.struct_mutex);
+
+       /* Submitting requests etc needs the hw awake. */
+       intel_runtime_pm_get(i915);
+
+       i915_retire_requests(i915);

?

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

Quoted 3 lines

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

+
+       /* Now use the RCS to actually reconfigure. */
+       engine = i915->engine[RCS];

? Modifying registers stored in another engine's context image.

Quoted 4 lines

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

+
+       rq = i915_request_alloc(engine, i915->kernel_context);
+       if (IS_ERR(rq)) {
+               ret = PTR_ERR(rq);

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

Quoted 4 lines

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

+               goto out_put;
+       }
+
+       ret = engine->emit_rpcs_config(rq, ctx, sseu);

It's just an LRI, I'd rather we do it directly unless there's evidence that there will be na explicit rpcs config instruction in future. It just doesn't seem general enough.

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

Quoted 4 lines

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

+       if (ret)
+               goto out_add;
+
+       /* Queue this switch after all other activity */

Only needs to be after the target ctx.

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

Quoted 5 lines

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

+       list_for_each_entry(ring, &i915->gt.active_rings, active_link) {
+               struct i915_request *prev;
+
+               prev = last_request_on_engine(ring->timeline, engine);

As constructed above you need target-engine + RCS.

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

Quoted 8 lines

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

+               if (prev)
+                       i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                                                        &prev->submit,
+                                                        I915_FENCE_GFP);
+       }
+
+       i915_gem_set_global_barrier(i915, rq);

This is just for a link from ctx-engine to this rq. Overkill much? Presumably this stems from using the wrong engine.

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

Quoted 4 lines

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

+
+out_add:
+       i915_request_add(rq);

And I'd still recommend not using indirect access if we can apply the changes immediately.

Quoted 110 lines

Quoting Tvrtko Ursulin (2018-08-14 15:40:58)

Quoted 109 lines

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.

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 | 187 +++++++++++++++++++++++-
  drivers/gpu/drm/i915/intel_lrc.c        |  55 +++++++
  drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
  include/uapi/drm/i915_drm.h             |  43 ++++++
  4 files changed, 288 insertions(+), 1 deletion(-)

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

index 8a12984e7495..6d6220634e9e 100644
@@ -773,6 +773,91 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
         return 0;
  }
 
+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;
+       struct i915_request *rq;
+       struct intel_ring *ring;
+       int ret;
+

Quoted 8 lines

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

Quoted 6 lines

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

+       lockdep_assert_held(&i915->drm.struct_mutex);
+
+       /* Submitting requests etc needs the hw awake. */
+       intel_runtime_pm_get(i915);
+
+       i915_retire_requests(i915);

?

I wondered myself but did not make myself dig through all the history of the series. Cannot think that it does anything useful in the current design.

Quoted 5 lines

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

Quoted 3 lines

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

+
+       /* Now use the RCS to actually reconfigure. */
+       engine = i915->engine[RCS];

? Modifying registers stored in another engine's context image.

Well, I was hoping design was kind of agreed by now.

Quoted 11 lines

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

Quoted 7 lines

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

+       rq = i915_request_alloc(engine, i915->kernel_context);
+       if (IS_ERR(rq)) {
+               ret = PTR_ERR(rq);
+               goto out_put;
+       }
+
+       ret = engine->emit_rpcs_config(rq, ctx, sseu);

It's just an LRI, I'd rather we do it directly unless there's evidence that there will be na explicit rpcs config instruction in future. It just doesn't seem general enough.

No complaints, can do.

Quoted 5 lines

Quoted 4 lines

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

+       if (ret)
+               goto out_add;
+
+       /* Queue this switch after all other activity */

Quoted 1 line

Only needs to be after the target ctx.

True, so find just the timeline belonging to target context. Some backpointers would be needed to find it. Or a walk and compare against target ce->ring->timeline->fence_context. Sounds like more than the latter isn't justified for this use case.

Quoted 6 lines

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

Quoted 4 lines

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

+       list_for_each_entry(ring, &i915->gt.active_rings, active_link) {
+               struct i915_request *prev;
+
+               prev = last_request_on_engine(ring->timeline, engine);

As constructed above you need target-engine + RCS.

Target engine is always RCS. Looks like the engine param to this function is pointless.

Quoted 8 lines

Quoted 7 lines

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

+               if (prev)
+                       i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                                                        &prev->submit,
+                                                        I915_FENCE_GFP);
+       }
+
+       i915_gem_set_global_barrier(i915, rq);

Quoted 2 lines

This is just for a link from ctx-engine to this rq. Overkill much? Presumably this stems from using the wrong engine.

AFAIU this is to ensure target context cannot get ahead of this request. Without it could overtake and then there is no guarantee execbufs following set param will be with new SSEU configuration.

Quoted 6 lines

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

Quoted 3 lines

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

+
+out_add:
+       i915_request_add(rq);

And I'd still recommend not using indirect access if we can apply the changes immediately.

Direct (CPU) access means blocking in set param until the context is idle.

Quoting Tvrtko Ursulin (2018-08-14 19:44:09)

Quoted 130 lines · Chris Wilson

Quoted 110 lines

Quoting Tvrtko Ursulin (2018-08-14 15:40:58)

Quoted 109 lines

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.

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 | 187 +++++++++++++++++++++++-
   drivers/gpu/drm/i915/intel_lrc.c        |  55 +++++++
   drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
   include/uapi/drm/i915_drm.h             |  43 ++++++
   4 files changed, 288 insertions(+), 1 deletion(-)

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

index 8a12984e7495..6d6220634e9e 100644
@@ -773,6 +773,91 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
          return 0;
   }
 
+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;
+       struct i915_request *rq;
+       struct intel_ring *ring;
+       int ret;
+

Quoted 8 lines

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

Quoted 6 lines

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

+       lockdep_assert_held(&i915->drm.struct_mutex);
+
+       /* Submitting requests etc needs the hw awake. */
+       intel_runtime_pm_get(i915);
+
+       i915_retire_requests(i915);

?

I wondered myself but did not make myself dig through all the history of the series. Cannot think that it does anything useful in the current design.

Quoted 5 lines

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

Quoted 3 lines

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

+
+       /* Now use the RCS to actually reconfigure. */
+       engine = i915->engine[RCS];

? Modifying registers stored in another engine's context image.

Quoted 1 line

Well, I was hoping design was kind of agreed by now.

:-p

This wasn't about using requests per-se, but raising the question of why use rcs to adjust the vcs context image. If we used vcs, the question of serialisation is then only on that engine.

Quoted 19 lines

Quoted 11 lines

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

Quoted 7 lines

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

+       rq = i915_request_alloc(engine, i915->kernel_context);
+       if (IS_ERR(rq)) {
+               ret = PTR_ERR(rq);
+               goto out_put;
+       }
+
+       ret = engine->emit_rpcs_config(rq, ctx, sseu);

It's just an LRI, I'd rather we do it directly unless there's evidence that there will be na explicit rpcs config instruction in future. It just doesn't seem general enough.

No complaints, can do.

Quoted 5 lines

Quoted 4 lines

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

+       if (ret)
+               goto out_add;
+
+       /* Queue this switch after all other activity */

Quoted 6 lines

Quoted 1 line

Only needs to be after the target ctx.

True, so find just the timeline belonging to target context. Some backpointers would be needed to find it. Or a walk and compare against target ce->ring->timeline->fence_context. Sounds like more than the latter isn't justified for this use case.

Right, we should be able to use ce->ring->timeline.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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

+       list_for_each_entry(ring, &i915->gt.active_rings, active_link) {
+               struct i915_request *prev;
+
+               prev = last_request_on_engine(ring->timeline, engine);

Quoted 4 lines

Quoted 1 line

As constructed above you need target-engine + RCS.

Target engine is always RCS. Looks like the engine param to this function is pointless.

Always always? At the beginning the speculation was that the other engines were going to get their own subslice parameters. I guess that was on the same design sheet as the VCS commands...

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

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

+               if (prev)
+                       i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                                                        &prev->submit,
+                                                        I915_FENCE_GFP);
+       }
+
+       i915_gem_set_global_barrier(i915, rq);

Quoted 6 lines

Quoted 2 lines

This is just for a link from ctx-engine to this rq. Overkill much? Presumably this stems from using the wrong engine.

AFAIU this is to ensure target context cannot get ahead of this request. Without it could overtake and then there is no guarantee execbufs following set param will be with new SSEU configuration.

Right, but we only need a fence on the target context to this request. The existing way would be to queue an empty request on the target with the await on this rq, but if you feel that is one request too many we can use a timeline barrier. Or if that is too narrow, an engine barrier. Over time, I can see that we may want all 3 barrier levels. The cost isn't too onerous.

Quoted 8 lines

Quoted 7 lines

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

Quoted 4 lines

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

+
+out_add:
+       i915_request_add(rq);

And I'd still recommend not using indirect access if we can apply the changes immediately.

Quoted 2 lines

Direct (CPU) access means blocking in set param until the context is idle.

No, it doesn't. It's is just a choice of whether you use request to a pinned context, or direct access if idle (literally ce->pin_count). Direct access is going to be near zero overhead.

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

+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;
+       struct i915_request *rq;
+       struct intel_ring *ring;
+       int ret;
+
+       lockdep_assert_held(&i915->drm.struct_mutex);
+
+       /* Submitting requests etc needs the hw awake. */
+       intel_runtime_pm_get(i915);
+
+       i915_retire_requests(i915);
+
+       /* Now use the RCS to actually reconfigure. */
+       engine = i915->engine[RCS];
+
+       rq = i915_request_alloc(engine, i915->kernel_context);
+       if (IS_ERR(rq)) {
+               ret = PTR_ERR(rq);
+               goto out_put;
+       }
+
+       ret = engine->emit_rpcs_config(rq, ctx, sseu);
+       if (ret)
+               goto out_add;
+
+       /* Queue this switch after all other activity */
+       list_for_each_entry(ring, &i915->gt.active_rings, active_link) {
+               struct i915_request *prev;
+
+               prev = last_request_on_engine(ring->timeline, engine);
+               if (prev)
+                       i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                                                        &prev->submit,
+                                                        I915_FENCE_GFP);

Quoting Tvrtko Ursulin (2018-08-14 15:40:58)

Quoted 45 lines

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

+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                                 struct intel_engine_cs *engine,
+                                 struct intel_sseu sseu)
+{
+       struct drm_i915_private *i915 = ctx->i915;
+       struct i915_request *rq;
+       struct intel_ring *ring;
+       int ret;
+
+       lockdep_assert_held(&i915->drm.struct_mutex);
+
+       /* Submitting requests etc needs the hw awake. */
+       intel_runtime_pm_get(i915);
+
+       i915_retire_requests(i915);
+
+       /* Now use the RCS to actually reconfigure. */
+       engine = i915->engine[RCS];
+
+       rq = i915_request_alloc(engine, i915->kernel_context);
+       if (IS_ERR(rq)) {
+               ret = PTR_ERR(rq);
+               goto out_put;
+       }
+
+       ret = engine->emit_rpcs_config(rq, ctx, sseu);
+       if (ret)
+               goto out_add;
+
+       /* Queue this switch after all other activity */
+       list_for_each_entry(ring, &i915->gt.active_rings, active_link) {
+               struct i915_request *prev;
+
+               prev = last_request_on_engine(ring->timeline, engine);
+               if (prev)
+                       i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                                                        &prev->submit,
+                                                        I915_FENCE_GFP);
+       }
+
+       i915_gem_set_global_barrier(i915, rq);
+
+out_add:
+       i915_request_add(rq);

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

Quoted 4 lines

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

+out_put:
+       intel_runtime_pm_put(i915);
+
+       return ret;

Looks like we should be able to hook this up to a selftest to confirm the modification does land in the target context image, and a SRM to confirm it loaded.

Quoted 4 lines

Quoting Tvrtko Ursulin (2018-08-14 15:40:58)
Looks like we should be able to hook this up to a selftest to confirm the modification does land in the target context image, and a SRM to confirm it loaded.

Lionel wrote an IGT which reads it back via SRM so that should be covered. I will be posting the IGT counterpart at some point as well, ideally when the agreement on the i915 side is there.

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.

Headline changes:

1.

No more master allow/disallow sysfs switch. Feature is unconditionally enabled for Gen11 and on other platforms it requires CAP_SYS_ADMIN.

*** To be discussed if this is a good idea or not. ***

2.

Two new patches due a) breaking out the global barrier, and b) fixing one GEM_BUG_ON regarding incorrent kernel context classification by i915_is_ggtt.

Otherwise please see individial patch change logs.

Main topic for the cover letter though is addressing the question of dynamic slice re-configuration performance impact.

Introduction into this problem space is that changing the (sub)slice configuration has a cost at context switch time in the order of tens of milli- seconds. (It varies per Gen and with different slice count transitions.)

So the question is whether a malicious unprivileged workload can negatively impact other clients. To try and answer this question I have extended gem_wsim and creating some test workloads. (Note that my testing was done on a Gen9 system. Overall message could be the same on Gen11 but needs to be verified.)

First test was a simulated video playback client running in parallel with a simulated game of both medium and high complexity (uses around 60% or 90% of the render engine respectively, and 7% of the blitter engine). I had two flavours of the playback client, one which runs normally and one which requests reduced slice configuration. Both workloads are targetting to run at 60fps.

Second test is the same but against a heavier simulated game workload, the one which uses around 90% of the render engine.

Results are achieved frames per second as observed from the game client:

No player Normal player SSEU enabled player
Medium game 59.6 59.6 59.6
Heavy game 59.7 58.4 58.1

Here we can see that the medium workload was not affected either by the normal or SSEU player, while the heavy workload did see a performance hit. Both with the video player running in parallel, and slighlty larger when the player was SSEU enabled.

Second test is running a malicious client (or clients) in parallel to the same simulated game workloads. These clients try to trigger many context switches by using multiple contexts with dependencies set up so request coalescing is defeated as much as possible.

I tested both with normal and SSEU enabled malicious clients:

DoS client SSEU DoS client
Medium game 59.5 59.6
Heavy game 57.8 55.4

For here we can see a similar picture as with the first test. Medium game client is not affected by either DoS client, while the heavy game client is, more so with the SSEU enabled attacker.

Quoted 1 line

From both tests I think the conclusion is that dynamic SSEU switching does

increase the magnitude of performance loss, especially with over-subscribed engines, due cost being proportional to context switch frequency.

Likelyhood is that it slightly lowers the utilization level at which this starts to happen, but does not introduce a completely new vector of attack - that is - where it was possible to DoS a system from an unprivileged client, it still is. In both cases (SSEU enabled or not), a malicious client has the option to grind the system to a halt, albeit it may need fewer submission threads to do so when it is SSEU enabled.

Chris Wilson (3):
drm/i915: Program RPCS for Broadwell
drm/i915: Record the sseu configuration per-context & engine drm/i915: Expose RPCS (SSEU) configuration to userspace

Lionel Landwerlin (3):
drm/i915/perf: simplify configure all context function drm/i915/perf: reuse intel_lrc ctx regs macro drm/i915/perf: lock powergating configuration to default when active

Tvrtko Ursulin (2):
drm/i915: Add global barrier support drm/i915: Explicitly mark Global GTT address spaces

  drivers/gpu/drm/i915/i915_drv.h         |  56 +++++++
  drivers/gpu/drm/i915/i915_gem.c         |   2 +
  drivers/gpu/drm/i915/i915_gem_context.c | 189 +++++++++++++++++++++++-
  drivers/gpu/drm/i915/i915_gem_context.h |   4 +
  drivers/gpu/drm/i915/i915_gem_gtt.c     |   2 +
  drivers/gpu/drm/i915/i915_gem_gtt.h     |   5 +-
  drivers/gpu/drm/i915/i915_perf.c        |  68 +++++----
  drivers/gpu/drm/i915/i915_request.c     |  16 ++
  drivers/gpu/drm/i915/i915_request.h     |  10 ++
  drivers/gpu/drm/i915/intel_lrc.c        |  87 ++++++++---
  drivers/gpu/drm/i915/intel_lrc.h        |   3 +
  drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
  include/uapi/drm/i915_drm.h             |  43 ++++++
  13 files changed, 439 insertions(+), 50 deletions(-)