| Message ID | 20180524145444.19302-8-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@intel.com> > --- > 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. > +{ > + 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)"? > + 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? 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, > }; > >
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@intel.com> >> --- >> 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, >> }; >> >
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) +{ + 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) { + struct intel_sseu default_sseu = + intel_sseu_from_device_sseu(&INTEL_INFO(i915)->sseu); + 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, };
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@intel.com> --- 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(-)