Re: [PATCH v5 7/7] drm/i915: Expose RPCS (SSEU) configuration to userspace

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 16/05/18 16:40, Tvrtko Ursulin wrote:

On 15/05/2018 10:05, Tvrtko Ursulin wrote:

On 14/05/2018 16:56, Lionel Landwerlin wrote:
From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>

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)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100899
Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
c: Dmitry Rogozhkin <dmitry.v.rogozhkin@xxxxxxxxx>
CC: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
CC: Zhipeng Gong <zhipeng.gong@xxxxxxxxx>
CC: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_gem_context.c | 169 ++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.c        | 103 ++++++++++-----
  drivers/gpu/drm/i915/intel_ringbuffer.c |   2 +
  drivers/gpu/drm/i915/intel_ringbuffer.h |   4 +
  include/uapi/drm/i915_drm.h             |  38 ++++++
  5 files changed, 281 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 01310c99e032..0b72a771c3f3 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -734,6 +734,110 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
      return 0;
  }
+static int
+intel_sseu_from_user_sseu(const struct sseu_dev_info *sseu,
+              const struct drm_i915_gem_context_param_sseu *user_sseu,
+              union intel_sseu *ctx_sseu)
+{
+    if ((user_sseu->slice_mask & ~sseu->slice_mask) != 0 ||
+        user_sseu->slice_mask == 0)
+        return -EINVAL;
+
+    if ((user_sseu->subslice_mask & ~sseu->subslice_mask[0]) != 0 ||
+        user_sseu->subslice_mask == 0)
+        return -EINVAL;
+
+    if (user_sseu->min_eus_per_subslice > sseu->max_eus_per_subslice)
+        return -EINVAL;
+
+    if (user_sseu->max_eus_per_subslice > sseu->max_eus_per_subslice || +        user_sseu->max_eus_per_subslice < user_sseu->min_eus_per_subslice ||
+        user_sseu->max_eus_per_subslice == 0)
+        return -EINVAL;
+
+    ctx_sseu->slice_mask = user_sseu->slice_mask;
+    ctx_sseu->subslice_mask = user_sseu->subslice_mask;
+    ctx_sseu->min_eus_per_subslice = user_sseu->min_eus_per_subslice;
+    ctx_sseu->max_eus_per_subslice = user_sseu->max_eus_per_subslice;
+
+    return 0;
+}
+
+static int
+i915_gem_context_reconfigure_sseu(struct i915_gem_context *ctx,
+                  struct intel_engine_cs *engine,
+                  union intel_sseu sseu)
+{
+    struct drm_i915_private *dev_priv = ctx->i915;
+    struct i915_timeline *timeline;
+    struct i915_request *rq;
+    union intel_sseu actual_sseu;
+    enum intel_engine_id id;
+    int ret;
+
+    /*
+     * First notify user when this capability is not available so that it
+     * can be detected with any valid input.
+     */
+    if (!engine->emit_rpcs_config)
+        return -ENODEV;
+
+    if (to_intel_context(ctx, engine)->sseu.value == sseu.value)

Are there other uses for the value union in the series? Think whether it could be dropped and memcmp used here for simplicity.

+        return 0;
+
+    lockdep_assert_held(&dev_priv->drm.struct_mutex);
+
+    i915_retire_requests(dev_priv);
+
+    /* Now use the RCS to actually reconfigure. */
+    engine = dev_priv->engine[RCS];
+
+    rq = i915_request_alloc(engine, dev_priv->kernel_context);
+    if (IS_ERR(rq))
+        return PTR_ERR(rq);
+
+    /*
+     * If i915/perf is active, we want a stable powergating configuration +     * on the system. Act as if we recorded the user's request but program
+     * the default sseu configuration. When i915/perf is stopped, the
+     * recorded configuration will be programmed.
+     */
+    actual_sseu = dev_priv->perf.oa.exclusive_stream ?

This feels it should jump out more since there is no obvious connection between the two. Probably a helper of some sort like intel_sanitize_sseu? Or intel_apply_sseu? Merge? ... ?

The comment would then be in the helper which I think would be better than sprinkle OA knowledge into context params code.

Furthermore, that idea to have a global sysfs knob would fit into this helper.

echo 1 > $i915_sysfs_root/allow_dynamic_slice_configuration

Or something.. As long as it defaults to off I think both camps should be happy.

Regards,

Tvrtko

Thanks Tvrtko,

I was thinking the same as a simple first step.
I can add that in another revision.

Cheers,

-
Lionel



+ intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu) :
+        sseu;
+
+    ret = engine->emit_rpcs_config(rq, ctx, actual_sseu);
+    if (ret) {
+        __i915_request_add(rq, true);
+        return ret;
+    }
+
+    /* Queue this switch after all other activity */
+    list_for_each_entry(timeline, &dev_priv->gt.timelines, link) {
+        struct i915_request *prev;
+
+        prev = last_request_on_engine(timeline, engine);
+        if (prev)
+ i915_sw_fence_await_sw_fence_gfp(&rq->submit,
+                             &prev->submit,
+                             I915_FENCE_GFP);
+    }
+
+    __i915_request_add(rq, true);

This is actually the bit I reading the patch for. So this I think is much better/safer than previous idling. However one concern, and maybe not a concern but just something which needs to be explained in the uAPI, is what it means with regards to the point from which the new configuration becomes live.

Could preemption for instance make it not defined enough? Could we, or should we, also link this request somehow onto the issuing context timeline so it must be first there? Hm, should we use the user context instead of the kernel one, and set the highest priority? But would have to avoid triggering preemption.

+    /*
+     * Apply the configuration to all engine. Our hardware doesn't
+     * currently support different configurations for each engine.
+     */
+    for_each_engine(engine, dev_priv, id) {
+        struct intel_context *ce = to_intel_context(ctx, engine);
+
+        ce->sseu.value = sseu.value;
+    }
+
+    return 0;
+}
+
  int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
                      struct drm_file *file)
  {
@@ -771,6 +875,37 @@ int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
      case I915_CONTEXT_PARAM_PRIORITY:
          args->value = ctx->sched.priority;
          break;
+    case I915_CONTEXT_PARAM_SSEU: {
+        struct drm_i915_gem_context_param_sseu param_sseu;
+        struct intel_engine_cs *engine;
+        struct intel_context *ce;
+
+        if (copy_from_user(&param_sseu, u64_to_user_ptr(args->value),
+                   sizeof(param_sseu))) {
+            ret = -EFAULT;
+            break;
+        }
+
+        engine = intel_engine_lookup_user(to_i915(dev),
+                          param_sseu.class,
+                          param_sseu.instance);
+        if (!engine) {
+            ret = -EINVAL;
+            break;
+        }
+
+        ce = &ctx->__engine[engine->id];

to_intel_context(ctx, engine) ?

+
+        param_sseu.slice_mask = ce->sseu.slice_mask;
+        param_sseu.subslice_mask = ce->sseu.subslice_mask;
+        param_sseu.min_eus_per_subslice = ce->sseu.min_eus_per_subslice; +        param_sseu.max_eus_per_subslice = ce->sseu.max_eus_per_subslice;
+
+        if (copy_to_user(u64_to_user_ptr(args->value), &param_sseu,
+                 sizeof(param_sseu)))
+            ret = -EFAULT;
+        break;
+    }
      default:
          ret = -EINVAL;
          break;
@@ -845,7 +980,41 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                  ctx->sched.priority = priority;
          }
          break;
+    case I915_CONTEXT_PARAM_SSEU:
+        {
+            struct drm_i915_private *dev_priv = to_i915(dev);
+            struct drm_i915_gem_context_param_sseu user_sseu;
+            struct intel_engine_cs *engine;
+            union intel_sseu ctx_sseu;
+
+            if (args->size) {
+                ret = -EINVAL;
+                break;
+            }
+
+            if (copy_from_user(&user_sseu, u64_to_user_ptr(args->value),
+                       sizeof(user_sseu))) {
+                ret = -EFAULT;
+                break;
+            }
+
+            engine = intel_engine_lookup_user(dev_priv,
+                              user_sseu.class,
+                              user_sseu.instance);
+            if (!engine) {
+                ret = -EINVAL;
+                break;
+            }
+
+            ret = intel_sseu_from_user_sseu(&INTEL_INFO(dev_priv)->sseu,
+                            &user_sseu, &ctx_sseu);

Should setter have a helper as well?

+            if (ret)
+                break;
+            ret = i915_gem_context_reconfigure_sseu(ctx, engine,
+                                ctx_sseu);
+        }
+        break;
      default:
          ret = -EINVAL;
          break;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 320b416482e1..4e0216f33c75 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2184,6 +2184,72 @@ static void gen8_emit_breadcrumb_rcs(struct i915_request *request, u32 *cs)
  }
  static const int gen8_emit_breadcrumb_rcs_sz = 8 + WA_TAIL_DWORDS;
+u32 gen8_make_rpcs(const struct sseu_dev_info *sseu,
+           union intel_sseu ctx_sseu)
+{
+    u32 rpcs = 0;
+
+    /*
+     * Starting in Gen9, render power gating can leave
+     * slice/subslice/EU in a partially enabled state. We
+     * must make an explicit request through RPCS for full
+     * enablement.
+     */
+    if (sseu->has_slice_pg) {
+        rpcs |= GEN8_RPCS_S_CNT_ENABLE;
+        rpcs |= hweight8(ctx_sseu.slice_mask) << GEN8_RPCS_S_CNT_SHIFT;
+        rpcs |= GEN8_RPCS_ENABLE;
+    }
+
+    if (sseu->has_subslice_pg) {
+        rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
+        rpcs |= hweight8(ctx_sseu.subslice_mask) <<
+            GEN8_RPCS_SS_CNT_SHIFT;
+        rpcs |= GEN8_RPCS_ENABLE;
+    }
+
+    if (sseu->has_eu_pg) {
+        rpcs |= ctx_sseu.min_eus_per_subslice <<
+            GEN8_RPCS_EU_MIN_SHIFT;
+        rpcs |= ctx_sseu.max_eus_per_subslice <<
+            GEN8_RPCS_EU_MAX_SHIFT;
+        rpcs |= GEN8_RPCS_ENABLE;
+    }
+
+    return rpcs;
+}
+
+static int gen8_emit_rpcs_config(struct i915_request *rq,
+                 struct i915_gem_context *ctx,
+                 union intel_sseu sseu)
+{
+    struct drm_i915_private *dev_priv = rq->i915;
+    struct intel_context *ce = to_intel_context(ctx, dev_priv->engine[RCS]);
+    u64 offset;
+    u32 *cs;
+
+    /* Let the deferred state allocation take care of this. */
+    if (!ce->state)
+        return 0;
+
+    cs = intel_ring_begin(rq, 4);
+    if (IS_ERR(cs))
+        return PTR_ERR(cs);
+
+    offset = ce->state->node.start +
+        LRC_STATE_PN * PAGE_SIZE +
+        (CTX_R_PWR_CLK_STATE + 1) * 4;
+
+    *cs++ = MI_STORE_DWORD_IMM_GEN4;
+    *cs++ = lower_32_bits(offset);
+    *cs++ = upper_32_bits(offset);
+    *cs++ = gen8_make_rpcs(&INTEL_INFO(dev_priv)->sseu, sseu);
+
+    intel_ring_advance(rq, cs);
+
+    return 0;
+}
+
  static int gen8_init_rcs_context(struct i915_request *rq)
  {
      int ret;
@@ -2274,6 +2340,8 @@ logical_ring_default_vfuncs(struct intel_engine_cs *engine)
      engine->emit_breadcrumb = gen8_emit_breadcrumb;
      engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_sz;
+    engine->emit_rpcs_config = gen8_emit_rpcs_config;
+
      engine->set_default_submission = execlists_set_default_submission;
      if (INTEL_GEN(engine->i915) < 11) {
@@ -2422,41 +2490,6 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine)
      return logical_ring_init(engine);
  }
-u32 gen8_make_rpcs(const struct sseu_dev_info *sseu,
-           union intel_sseu ctx_sseu)
-{
-    u32 rpcs = 0;
-
-    /*
-     * Starting in Gen9, render power gating can leave
-     * slice/subslice/EU in a partially enabled state. We
-     * must make an explicit request through RPCS for full
-     * enablement.
-    */
-    if (sseu->has_slice_pg) {
-        rpcs |= GEN8_RPCS_S_CNT_ENABLE;
-        rpcs |= hweight8(ctx_sseu.slice_mask) << GEN8_RPCS_S_CNT_SHIFT;
-        rpcs |= GEN8_RPCS_ENABLE;
-    }
-
-    if (sseu->has_subslice_pg) {
-        rpcs |= GEN8_RPCS_SS_CNT_ENABLE;
-        rpcs |= hweight8(ctx_sseu.subslice_mask) <<
-            GEN8_RPCS_SS_CNT_SHIFT;
-        rpcs |= GEN8_RPCS_ENABLE;
-    }
-
-    if (sseu->has_eu_pg) {
-        rpcs |= ctx_sseu.min_eus_per_subslice <<
-            GEN8_RPCS_EU_MIN_SHIFT;
-        rpcs |= ctx_sseu.max_eus_per_subslice <<
-            GEN8_RPCS_EU_MAX_SHIFT;
-        rpcs |= GEN8_RPCS_ENABLE;
-    }
-
-    return rpcs;
-}
-
  static u32 intel_lr_indirect_ctx_offset(struct intel_engine_cs *engine)
  {
      u32 indirect_ctx_offset;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 8f19349a6055..44fb3a1cf8f9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2026,6 +2026,8 @@ static void intel_ring_default_vfuncs(struct drm_i915_private *dev_priv,
              engine->emit_breadcrumb_sz++;
      }
+    engine->emit_rpcs_config = NULL; /* Only supported on Gen8+ */
+
      engine->set_default_submission = i9xx_set_default_submission;
      if (INTEL_GEN(dev_priv) >= 6)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index cc7e73730469..9745f4ab8214 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -463,6 +463,10 @@ struct intel_engine_cs {
      void        (*emit_breadcrumb)(struct i915_request *rq, u32 *cs);
      int        emit_breadcrumb_sz;
+    int        (*emit_rpcs_config)(struct i915_request *rq,
+                        struct i915_gem_context *ctx,
+                        union intel_sseu sseu);
+
      /* Pass the request to the hardware queue (e.g. directly into
       * the legacy ringbuffer or to the end of an execlist).
       *
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 7f5634ce8e88..24b90836ce1d 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1456,9 +1456,47 @@ struct drm_i915_gem_context_param {
  #define   I915_CONTEXT_MAX_USER_PRIORITY    1023 /* inclusive */
  #define   I915_CONTEXT_DEFAULT_PRIORITY        0
  #define   I915_CONTEXT_MIN_USER_PRIORITY    -1023 /* inclusive */
+    /*
+     * When using the following param, value should be a pointer to
+     * drm_i915_gem_context_param_sseu.
+     */
+#define I915_CONTEXT_PARAM_SSEU        0x7
      __u64 value;
  };
+struct drm_i915_gem_context_param_sseu {
+    /*
+     * Engine class & instance to be configured or queried.
+     */
+    __u32 class;
+    __u32 instance;
+
+    /*
+     * Mask of slices to enable for the context. Valid values are a subset
+     * of the bitmask value returned for I915_PARAM_SLICE_MASK.
+     */
+    __u8 slice_mask;
+
+    /*
+     * Mask of subslices to enable for the context. Valid values are a
+     * subset of the bitmask value return by I915_PARAM_SUBSLICE_MASK.
+     */
+    __u8 subslice_mask;
+
+    /*
+     * Minimum/Maximum number of EUs to enable per subslice for the
+     * context. min_eus_per_subslice must be inferior or equal to
+     * max_eus_per_subslice.
+     */
+    __u8 min_eus_per_subslice;
+    __u8 max_eus_per_subslice;
+
+    /*
+     * Unused for now. Must be cleared to zero.
+     */
+    __u32 rsvd;
+};
+
  enum drm_i915_oa_format {
      I915_OA_FORMAT_A13 = 1,        /* HSW only */
      I915_OA_FORMAT_A29,        /* HSW only */


Regards,

Tvrtko


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux