Re: [PATCH v7 7/7] drm/i915: add a sysfs entry to let users set sseu configs

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

 



On 24/05/18 16:35, Tvrtko Ursulin wrote:

On 24/05/2018 15:54, Lionel Landwerlin wrote:
There are concerns about denial of service around the per context sseu
configuration capability. In a previous commit introducing the
capability we allowed it only for capable users. This changes adds a
new debugfs entry to let any user configure its own context
powergating setup.

v2: Rename sysfs entry (Tvrtko)
     Lock interruptible the device in sysfs (Tvrtko)
     Fix dropped error code in getting dynamic sseu value (Tvrtko)
     s/dev_priv/i915/ (Tvrtko)

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h         |  6 ++++
  drivers/gpu/drm/i915/i915_gem_context.c | 47 ++++++++++++++++++++++++-
  drivers/gpu/drm/i915/i915_sysfs.c       | 40 +++++++++++++++++++++
  3 files changed, 92 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 916a301d8c22..3db75c56a9f7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1842,6 +1842,8 @@ struct drm_i915_private {
          struct ida hw_ida;
  #define MAX_CONTEXT_HW_ID (1<<21) /* exclusive */
  #define GEN11_MAX_CONTEXT_HW_ID (1<<11) /* exclusive */
+
+        bool dynamic_sseu;
      } contexts;
        u32 fdi_rx_config;
@@ -3275,6 +3277,10 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id)
      return ctx;
  }
  +int i915_gem_contexts_set_dynamic_sseu_locked(struct drm_i915_private *i915,
+                          bool allowed);
+bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915);
+
  int i915_perf_open_ioctl(struct drm_device *dev, void *data,
               struct drm_file *file);
  int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 30736efe0fcd..afb7db95aa3b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -983,7 +983,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
                  break;
              }
  -            if (!capable(CAP_SYS_ADMIN)) {
+            if (!i915->contexts.dynamic_sseu &&
+                !capable(CAP_SYS_ADMIN)) {
                  ret = -EPERM;
                  break;
              }
@@ -1065,6 +1066,50 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
      return ret;
  }
  +int i915_gem_contexts_set_dynamic_sseu_locked(struct drm_i915_private *i915,
+                          bool allowed)

I think you can drop the _locked suffix since we normally only use it when there are two flavours of otherwise the same function name. And you have lockdep_assert_held so that makes it obvious.


Okay


+{
+    struct intel_engine_cs *engine = i915->engine[RCS];
+    int ret = 0;
+
+    lockdep_assert_held(&i915->drm.struct_mutex);
+
+    if (!engine->emit_rpcs_config)
+        return -ENODEV;
+
+    /*
+     * When we allow each context to configure its powergating
+     * configuration, there is no need to put the configurations back to
+     * the default, it should already be the case.
+     */
+    if (!allowed) {

Do you want to optimize with "if (!allowed && i915->context.dynamic_sseu)"?

Yep!


+        struct intel_sseu default_sseu =
+ intel_sseu_from_device_sseu(&INTEL_INFO(i915)->sseu);

Quick grep around patches tells me this function is always called with the same parameter.  Unless I missed something perhaps it would be more readable to rename it to something shorter. Like maybe:

default_sseu = intel_device_default_sseu(i915);

Just thinking if that would read nicer than &INTEL_INFO(i915)->sseu everywhere?

Sure.


Regards,

Tvrtko

+        struct i915_gem_context *ctx;
+
+        list_for_each_entry(ctx, &i915->contexts.list, link) {
+            ret = i915_gem_context_reconfigure_sseu(ctx, engine,
+                                default_sseu);
+            if (ret)
+                break;
+        }
+    }
+
+    i915->contexts.dynamic_sseu = allowed;
+
+    return ret;
+}
+
+bool i915_gem_contexts_get_dynamic_sseu(struct drm_i915_private *i915)
+{
+    struct intel_engine_cs *engine = i915->engine[RCS];
+
+    if (!engine->emit_rpcs_config)
+        return false;
+
+    return i915->contexts.dynamic_sseu;
+}
+
  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
  #include "selftests/mock_context.c"
  #include "selftests/i915_gem_context.c"
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index e5e6f6bb2b05..c323cab59ec7 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -483,6 +483,44 @@ static ssize_t gt_rp_mhz_show(struct device *kdev, struct device_attribute *attr
      return snprintf(buf, PAGE_SIZE, "%d\n", val);
  }
  +static ssize_t allow_dynamic_sseu_show(struct device *kdev,
+                       struct device_attribute *attr,
+                       char *buf)
+{
+    struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
+    bool value = i915_gem_contexts_get_dynamic_sseu(i915);
+
+    return snprintf(buf, PAGE_SIZE, "%d\n", value);
+}
+
+static ssize_t allow_dynamic_sseu_store(struct device *kdev,
+                    struct device_attribute *attr,
+                    const char *buf, size_t count)
+{
+    struct drm_i915_private *i915 = kdev_minor_to_i915(kdev);
+    ssize_t ret;
+    u32 val;
+
+    ret = kstrtou32(buf, 0, &val);
+    if (ret)
+        return ret;
+
+    if (val != 0 && val != 1)
+        return -EINVAL;
+
+    ret = mutex_lock_interruptible(&i915->drm.struct_mutex);
+    if (ret)
+        return ret;
+
+    ret = i915_gem_contexts_set_dynamic_sseu_locked(i915, val != 0);
+
+    mutex_unlock(&i915->drm.struct_mutex);
+
+    return ret ?: count;
+}
+
+static DEVICE_ATTR_RW(allow_dynamic_sseu);
+
  static const struct attribute *gen6_attrs[] = {
      &dev_attr_gt_act_freq_mhz.attr,
      &dev_attr_gt_cur_freq_mhz.attr,
@@ -492,6 +530,7 @@ static const struct attribute *gen6_attrs[] = {
      &dev_attr_gt_RP0_freq_mhz.attr,
      &dev_attr_gt_RP1_freq_mhz.attr,
      &dev_attr_gt_RPn_freq_mhz.attr,
+    &dev_attr_allow_dynamic_sseu.attr,
      NULL,
  };
  @@ -505,6 +544,7 @@ static const struct attribute *vlv_attrs[] = {
      &dev_attr_gt_RP1_freq_mhz.attr,
      &dev_attr_gt_RPn_freq_mhz.attr,
      &dev_attr_vlv_rpe_freq_mhz.attr,
+    &dev_attr_allow_dynamic_sseu.attr,
      NULL,
  };



_______________________________________________
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