| Message ID | 20180522180002.11522-8-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/05/2018 19:00, 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. > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 +++ > drivers/gpu/drm/i915/i915_gem_context.c | 52 ++++++++++++++++++++++++- > drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++ > 3 files changed, 86 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 09cfcfe1c339..0fccec29fdda 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1843,6 +1843,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 allow_sseu; I think spelling out "dynamic" in patch titles, commit messages and even in this variable name would be better and clearer. > } contexts; > > u32 fdi_rx_config; > @@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) > return ctx; > } > > +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv, bool allowed); > +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv); > + > 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 5c5a12f1c265..815a9d1c29f3 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, > break; > } > > - if (!capable(CAP_SYS_ADMIN)) { > + if (!dev_priv->contexts.allow_sseu && > + !capable(CAP_SYS_ADMIN)) { So the thing I mentioned in the previous patch. My thinking was not to fail it if sysfs toggle is disabled, but just don't do dynamic switching. That way the software stack works in all cases but user has the option of tuning his system. I mean it can still be done but in userspace. Set param fails, userspace goes for a fall back. Only difference is I guess whether it is useful to allow switching at runtime. I am imagining running some 3d and media, and then toggling the knob to see what happens to power and performance on the fly. But maybe that is not so interesting. Along the same lines I was thinking CAP_SYS_ADMIN limitation could be dropped. Both points are for a wider discussion I guess. > ret = -EPERM; > break; > } > @@ -1058,6 +1059,55 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, > return ret; > } > > +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv, > + bool allowed) > +{ > + struct intel_engine_cs *engine = dev_priv->engine[RCS]; > + int ret = 0; > + > + if (!engine->emit_rpcs_config) > + return -ENODEV; > + > + mutex_lock(&dev_priv->drm.struct_mutex); This should be interruptible so user can Ctrl-C if it is stuck. > + > + /* > + * 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) { > + union intel_sseu default_sseu = > + intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu); > + struct i915_gem_context *ctx; > + > + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { > + ret = i915_gem_context_reconfigure_sseu(ctx, engine, > + default_sseu); > + if (ret) > + break; > + } > + } > + > + dev_priv->contexts.allow_sseu = allowed; > + > + mutex_unlock(&dev_priv->drm.struct_mutex); > + return ret; > +} > + > +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv) > +{ > + struct intel_engine_cs *engine = dev_priv->engine[RCS]; > + bool ret; > + > + if (!engine->emit_rpcs_config) > + return false; > + > + mutex_lock(&dev_priv->drm.struct_mutex); > + ret = dev_priv->contexts.allow_sseu; > + mutex_unlock(&dev_priv->drm.struct_mutex); I guess this mutex does nothing in this case. > + return ret; > +} > + > #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..9fd15b138ac9 100644 > --- a/drivers/gpu/drm/i915/i915_sysfs.c > +++ b/drivers/gpu/drm/i915/i915_sysfs.c > @@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev, > + struct device_attribute *attr, char *buf) > +{ > + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", ret); Propagate ENODEV all the way by making i915_gem_contexts_get_allow_sseu return an int? > +} > + > +static ssize_t gem_allow_sseu_store(struct device *kdev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > + u32 val; > + ssize_t ret; Chris will complain about Christmas trees. :) (And possible about new use of dev_priv throughout the series but that battle is in stale mate.) > + > + ret = kstrtou32(buf, 0, &val); > + if (ret) > + return ret; > + > + i915_gem_contexts_set_allow_sseu(dev_priv, val != 0); I would enforce 0 or 1 to allow (hypothetical) future expansion with more values. > + > + return ret ?: count; > +} > + > +static DEVICE_ATTR_RW(gem_allow_sseu); I would like to open up a round of bike-shedding of the knob name. :) How about allow_dynamic_sseu? gem_ or not, don't know. > + > static const struct attribute *gen6_attrs[] = { > &dev_attr_gt_act_freq_mhz.attr, > &dev_attr_gt_cur_freq_mhz.attr, > @@ -492,6 +520,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_gem_allow_sseu.attr, > NULL, > }; > > @@ -505,6 +534,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_gem_allow_sseu.attr, > NULL, > }; > > Regards, Tvrtko
On 23/05/18 16:30, Tvrtko Ursulin wrote: > > On 22/05/2018 19:00, 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. >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++ >> drivers/gpu/drm/i915/i915_gem_context.c | 52 ++++++++++++++++++++++++- >> drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++ >> 3 files changed, 86 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 09cfcfe1c339..0fccec29fdda 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1843,6 +1843,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 allow_sseu; > > I think spelling out "dynamic" in patch titles, commit messages and > even in this variable name would be better and clearer. Done. > >> } contexts; >> u32 fdi_rx_config; >> @@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct >> drm_i915_file_private *file_priv, u32 id) >> return ctx; >> } >> +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private >> *dev_priv, bool allowed); >> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >> *dev_priv); >> + >> 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 5c5a12f1c265..815a9d1c29f3 100644 >> --- a/drivers/gpu/drm/i915/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >> @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct >> drm_device *dev, void *data, >> break; >> } >> - if (!capable(CAP_SYS_ADMIN)) { >> + if (!dev_priv->contexts.allow_sseu && >> + !capable(CAP_SYS_ADMIN)) { > > So the thing I mentioned in the previous patch. My thinking was not to > fail it if sysfs toggle is disabled, but just don't do dynamic > switching. That way the software stack works in all cases but user has > the option of tuning his system. > > I mean it can still be done but in userspace. Set param fails, > userspace goes for a fall back. > > Only difference is I guess whether it is useful to allow switching at > runtime. I am imagining running some 3d and media, and then toggling > the knob to see what happens to power and performance on the fly. But > maybe that is not so interesting. > > Along the same lines I was thinking CAP_SYS_ADMIN limitation could be > dropped. > > Both points are for a wider discussion I guess. Okay, let's get Dmitry input on this. > >> ret = -EPERM; >> break; >> } >> @@ -1058,6 +1059,55 @@ int i915_gem_context_reset_stats_ioctl(struct >> drm_device *dev, >> return ret; >> } >> +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private >> *dev_priv, >> + bool allowed) >> +{ >> + struct intel_engine_cs *engine = dev_priv->engine[RCS]; >> + int ret = 0; >> + >> + if (!engine->emit_rpcs_config) >> + return -ENODEV; >> + >> + mutex_lock(&dev_priv->drm.struct_mutex); > > This should be interruptible so user can Ctrl-C if it is stuck. Ah right, changing. > >> + >> + /* >> + * 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) { >> + union intel_sseu default_sseu = >> + intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu); >> + struct i915_gem_context *ctx; >> + >> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { >> + ret = i915_gem_context_reconfigure_sseu(ctx, engine, >> + default_sseu); >> + if (ret) >> + break; >> + } >> + } >> + >> + dev_priv->contexts.allow_sseu = allowed; >> + >> + mutex_unlock(&dev_priv->drm.struct_mutex); >> + return ret; >> +} >> + >> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >> *dev_priv) >> +{ >> + struct intel_engine_cs *engine = dev_priv->engine[RCS]; >> + bool ret; >> + >> + if (!engine->emit_rpcs_config) >> + return false; >> + >> + mutex_lock(&dev_priv->drm.struct_mutex); >> + ret = dev_priv->contexts.allow_sseu; >> + mutex_unlock(&dev_priv->drm.struct_mutex); > > I guess this mutex does nothing in this case. Yeah, I'm not sure whether I can read a boolean or whether it should be an atomic... > >> + return ret; >> +} >> + >> #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..9fd15b138ac9 100644 >> --- a/drivers/gpu/drm/i915/i915_sysfs.c >> +++ b/drivers/gpu/drm/i915/i915_sysfs.c >> @@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev, >> + struct device_attribute *attr, char *buf) >> +{ >> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >> + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", ret); > > Propagate ENODEV all the way by making > i915_gem_contexts_get_allow_sseu return an int? Duh! Well spotted. > >> +} >> + >> +static ssize_t gem_allow_sseu_store(struct device *kdev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >> + u32 val; >> + ssize_t ret; > > Chris will complain about Christmas trees. :) > > (And possible about new use of dev_priv throughout the series but that > battle is in stale mate.) Which one is it? :) i915 or dev_priv? > >> + >> + ret = kstrtou32(buf, 0, &val); >> + if (ret) >> + return ret; >> + >> + i915_gem_contexts_set_allow_sseu(dev_priv, val != 0); > > I would enforce 0 or 1 to allow (hypothetical) future expansion with > more values. Okay. > >> + >> + return ret ?: count; >> +} >> + >> +static DEVICE_ATTR_RW(gem_allow_sseu); > > I would like to open up a round of bike-shedding of the knob name. :) > > How about allow_dynamic_sseu? gem_ or not, don't know. Sure, no problem. > >> + >> static const struct attribute *gen6_attrs[] = { >> &dev_attr_gt_act_freq_mhz.attr, >> &dev_attr_gt_cur_freq_mhz.attr, >> @@ -492,6 +520,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_gem_allow_sseu.attr, >> NULL, >> }; >> @@ -505,6 +534,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_gem_allow_sseu.attr, >> NULL, >> }; >> > > Regards, > > Tvrtko >
On 23/05/18 16:30, Tvrtko Ursulin wrote: >> >> +{ >> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >> + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); >> + >> + return snprintf(buf, PAGE_SIZE, "%d\n", ret); > > Propagate ENODEV all the way by making > i915_gem_contexts_get_allow_sseu return an int? Actually should a cat on a sysfs entry fail with ENODEV?
On 23/05/2018 18:33, Lionel Landwerlin wrote: > On 23/05/18 16:30, Tvrtko Ursulin wrote: >> >> On 22/05/2018 19:00, 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. >>> >>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_drv.h | 5 +++ >>> drivers/gpu/drm/i915/i915_gem_context.c | 52 ++++++++++++++++++++++++- >>> drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++ >>> 3 files changed, 86 insertions(+), 1 deletion(-) [snip] >> >>> } contexts; >>> u32 fdi_rx_config; >>> @@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct >>> drm_i915_file_private *file_priv, u32 id) >>> return ctx; >>> } >>> +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private >>> *dev_priv, bool allowed); >>> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >>> *dev_priv); >>> + >>> 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 5c5a12f1c265..815a9d1c29f3 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>> @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct >>> drm_device *dev, void *data, >>> break; >>> } >>> - if (!capable(CAP_SYS_ADMIN)) { >>> + if (!dev_priv->contexts.allow_sseu && >>> + !capable(CAP_SYS_ADMIN)) { >> >> So the thing I mentioned in the previous patch. My thinking was not to >> fail it if sysfs toggle is disabled, but just don't do dynamic >> switching. That way the software stack works in all cases but user has >> the option of tuning his system. >> >> I mean it can still be done but in userspace. Set param fails, >> userspace goes for a fall back. >> >> Only difference is I guess whether it is useful to allow switching at >> runtime. I am imagining running some 3d and media, and then toggling >> the knob to see what happens to power and performance on the fly. But >> maybe that is not so interesting. >> >> Along the same lines I was thinking CAP_SYS_ADMIN limitation could be >> dropped. >> >> Both points are for a wider discussion I guess. > > Okay, let's get Dmitry input on this. To expand, my thinking is to let media stack configure its contexts for fewer slices, but let the user/sysadmin decide whether 3d/compute performance is more important, or media. Downside is that with this approach you would have to re-configure all contexts when sysfs toggle is changed from zero to one. [snip] >> >>> + >>> + /* >>> + * 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) { >>> + union intel_sseu default_sseu = >>> + intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu); >>> + struct i915_gem_context *ctx; >>> + >>> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { >>> + ret = i915_gem_context_reconfigure_sseu(ctx, engine, >>> + default_sseu); >>> + if (ret) >>> + break; >>> + } >>> + } >>> + >>> + dev_priv->contexts.allow_sseu = allowed; >>> + >>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>> + return ret; >>> +} >>> + >>> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >>> *dev_priv) >>> +{ >>> + struct intel_engine_cs *engine = dev_priv->engine[RCS]; >>> + bool ret; >>> + >>> + if (!engine->emit_rpcs_config) >>> + return false; >>> + >>> + mutex_lock(&dev_priv->drm.struct_mutex); >>> + ret = dev_priv->contexts.allow_sseu; >>> + mutex_unlock(&dev_priv->drm.struct_mutex); >> >> I guess this mutex does nothing in this case. > > Yeah, I'm not sure whether I can read a boolean or whether it should be > an atomic... You can just read a boolean. > >> >>> + return ret; >>> +} >>> + >>> #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..9fd15b138ac9 100644 >>> --- a/drivers/gpu/drm/i915/i915_sysfs.c >>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c >>> @@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >>> + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); >>> + >>> + return snprintf(buf, PAGE_SIZE, "%d\n", ret); >> >> Propagate ENODEV all the way by making >> i915_gem_contexts_get_allow_sseu return an int? > > Duh! Well spotted. I think I confused set and get while reviewing. It is probably fine to read back zero when the feature is not supported and just fail the write. But could ENODEV here as well, my opinion is not strong. > >> >>> +} >>> + >>> +static ssize_t gem_allow_sseu_store(struct device *kdev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >>> + u32 val; >>> + ssize_t ret; >> >> Chris will complain about Christmas trees. :) >> >> (And possible about new use of dev_priv throughout the series but that >> battle is in stale mate.) > > Which one is it? :) i915 or dev_priv? i915 unless I915_READ/WRITE are used in the function. :( Regards, Tvrtko
On 24/05/18 11:39, Tvrtko Ursulin wrote: > > On 23/05/2018 18:33, Lionel Landwerlin wrote: >> On 23/05/18 16:30, Tvrtko Ursulin wrote: >>> >>> On 22/05/2018 19:00, 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. >>>> >>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/i915_drv.h | 5 +++ >>>> drivers/gpu/drm/i915/i915_gem_context.c | 52 >>>> ++++++++++++++++++++++++- >>>> drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++ >>>> 3 files changed, 86 insertions(+), 1 deletion(-) > > [snip] > >>> >>>> } contexts; >>>> u32 fdi_rx_config; >>>> @@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct >>>> drm_i915_file_private *file_priv, u32 id) >>>> return ctx; >>>> } >>>> +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private >>>> *dev_priv, bool allowed); >>>> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >>>> *dev_priv); >>>> + >>>> 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 5c5a12f1c265..815a9d1c29f3 100644 >>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>>> @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct >>>> drm_device *dev, void *data, >>>> break; >>>> } >>>> - if (!capable(CAP_SYS_ADMIN)) { >>>> + if (!dev_priv->contexts.allow_sseu && >>>> + !capable(CAP_SYS_ADMIN)) { >>> >>> So the thing I mentioned in the previous patch. My thinking was not >>> to fail it if sysfs toggle is disabled, but just don't do dynamic >>> switching. That way the software stack works in all cases but user >>> has the option of tuning his system. >>> >>> I mean it can still be done but in userspace. Set param fails, >>> userspace goes for a fall back. >>> >>> Only difference is I guess whether it is useful to allow switching >>> at runtime. I am imagining running some 3d and media, and then >>> toggling the knob to see what happens to power and performance on >>> the fly. But maybe that is not so interesting. >>> >>> Along the same lines I was thinking CAP_SYS_ADMIN limitation could >>> be dropped. >>> >>> Both points are for a wider discussion I guess. >> >> Okay, let's get Dmitry input on this. > > To expand, my thinking is to let media stack configure its contexts > for fewer slices, but let the user/sysadmin decide whether 3d/compute > performance is more important, or media. > > Downside is that with this approach you would have to re-configure all > contexts when sysfs toggle is changed from zero to one. > > [snip] Not an issue for me. Only concern is whether this might make things easier to debug/spot when sseu configuration requests are silently discarded. > >>> >>>> + >>>> + /* >>>> + * 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) { >>>> + union intel_sseu default_sseu = >>>> + intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu); >>>> + struct i915_gem_context *ctx; >>>> + >>>> + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { >>>> + ret = i915_gem_context_reconfigure_sseu(ctx, engine, >>>> + default_sseu); >>>> + if (ret) >>>> + break; >>>> + } >>>> + } >>>> + >>>> + dev_priv->contexts.allow_sseu = allowed; >>>> + >>>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>>> + return ret; >>>> +} >>>> + >>>> +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private >>>> *dev_priv) >>>> +{ >>>> + struct intel_engine_cs *engine = dev_priv->engine[RCS]; >>>> + bool ret; >>>> + >>>> + if (!engine->emit_rpcs_config) >>>> + return false; >>>> + >>>> + mutex_lock(&dev_priv->drm.struct_mutex); >>>> + ret = dev_priv->contexts.allow_sseu; >>>> + mutex_unlock(&dev_priv->drm.struct_mutex); >>> >>> I guess this mutex does nothing in this case. >> >> Yeah, I'm not sure whether I can read a boolean or whether it should >> be an atomic... > > You can just read a boolean. > >> >>> >>>> + return ret; >>>> +} >>>> + >>>> #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..9fd15b138ac9 100644 >>>> --- a/drivers/gpu/drm/i915/i915_sysfs.c >>>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c >>>> @@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev, >>>> + struct device_attribute *attr, char *buf) >>>> +{ >>>> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >>>> + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); >>>> + >>>> + return snprintf(buf, PAGE_SIZE, "%d\n", ret); >>> >>> Propagate ENODEV all the way by making >>> i915_gem_contexts_get_allow_sseu return an int? >> >> Duh! Well spotted. > > I think I confused set and get while reviewing. > > It is probably fine to read back zero when the feature is not > supported and just fail the write. But could ENODEV here as well, my > opinion is not strong. I think it makes sense to return 0 in case of ENODEV and actually error out with ENODEV when trying to write a new value. > >> >>> >>>> +} >>>> + >>>> +static ssize_t gem_allow_sseu_store(struct device *kdev, >>>> + struct device_attribute *attr, >>>> + const char *buf, size_t count) >>>> +{ >>>> + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >>>> + u32 val; >>>> + ssize_t ret; >>> >>> Chris will complain about Christmas trees. :) >>> >>> (And possible about new use of dev_priv throughout the series but >>> that battle is in stale mate.) >> >> Which one is it? :) i915 or dev_priv? > > i915 unless I915_READ/WRITE are used in the function. :( > > Regards, > > Tvrtko >
On 24/05/18 11:39, Tvrtko Ursulin wrote: >>>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>>> @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct >>>> drm_device *dev, void *data, >>>> break; >>>> } >>>> - if (!capable(CAP_SYS_ADMIN)) { >>>> + if (!dev_priv->contexts.allow_sseu && >>>> + !capable(CAP_SYS_ADMIN)) { >>> >>> So the thing I mentioned in the previous patch. My thinking was not >>> to fail it if sysfs toggle is disabled, but just don't do dynamic >>> switching. That way the software stack works in all cases but user >>> has the option of tuning his system. >>> >>> I mean it can still be done but in userspace. Set param fails, >>> userspace goes for a fall back. >>> >>> Only difference is I guess whether it is useful to allow switching >>> at runtime. I am imagining running some 3d and media, and then >>> toggling the knob to see what happens to power and performance on >>> the fly. But maybe that is not so interesting. >>> >>> Along the same lines I was thinking CAP_SYS_ADMIN limitation could >>> be dropped. >>> >>> Both points are for a wider discussion I guess. >> >> Okay, let's get Dmitry input on this. > > To expand, my thinking is to let media stack configure its contexts > for fewer slices, but let the user/sysadmin decide whether 3d/compute > performance is more important, or media. > > Downside is that with this approach you would have to re-configure all > contexts when sysfs toggle is changed from zero to one. Right, I've updated this series locally with Tvrkto's latest comments on v7. The issue discussed above is the last remaining detail on which some kind of decision needs to be taken. Ccing Joonas who might have an input. Cheers, - Lionel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 09cfcfe1c339..0fccec29fdda 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1843,6 +1843,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 allow_sseu; } contexts; u32 fdi_rx_config; @@ -3274,6 +3276,9 @@ i915_gem_context_lookup(struct drm_i915_file_private *file_priv, u32 id) return ctx; } +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv, bool allowed); +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv); + 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 5c5a12f1c265..815a9d1c29f3 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -981,7 +981,8 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data, break; } - if (!capable(CAP_SYS_ADMIN)) { + if (!dev_priv->contexts.allow_sseu && + !capable(CAP_SYS_ADMIN)) { ret = -EPERM; break; } @@ -1058,6 +1059,55 @@ int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, return ret; } +int i915_gem_contexts_set_allow_sseu(struct drm_i915_private *dev_priv, + bool allowed) +{ + struct intel_engine_cs *engine = dev_priv->engine[RCS]; + int ret = 0; + + if (!engine->emit_rpcs_config) + return -ENODEV; + + mutex_lock(&dev_priv->drm.struct_mutex); + + /* + * 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) { + union intel_sseu default_sseu = + intel_sseu_from_device_sseu(&INTEL_INFO(dev_priv)->sseu); + struct i915_gem_context *ctx; + + list_for_each_entry(ctx, &dev_priv->contexts.list, link) { + ret = i915_gem_context_reconfigure_sseu(ctx, engine, + default_sseu); + if (ret) + break; + } + } + + dev_priv->contexts.allow_sseu = allowed; + + mutex_unlock(&dev_priv->drm.struct_mutex); + return ret; +} + +bool i915_gem_contexts_get_allow_sseu(struct drm_i915_private *dev_priv) +{ + struct intel_engine_cs *engine = dev_priv->engine[RCS]; + bool ret; + + if (!engine->emit_rpcs_config) + return false; + + mutex_lock(&dev_priv->drm.struct_mutex); + ret = dev_priv->contexts.allow_sseu; + mutex_unlock(&dev_priv->drm.struct_mutex); + return ret; +} + #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..9fd15b138ac9 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -483,6 +483,34 @@ 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 gem_allow_sseu_show(struct device *kdev, + struct device_attribute *attr, char *buf) +{ + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); + int ret = i915_gem_contexts_get_allow_sseu(dev_priv); + + return snprintf(buf, PAGE_SIZE, "%d\n", ret); +} + +static ssize_t gem_allow_sseu_store(struct device *kdev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); + u32 val; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + i915_gem_contexts_set_allow_sseu(dev_priv, val != 0); + + return ret ?: count; +} + +static DEVICE_ATTR_RW(gem_allow_sseu); + static const struct attribute *gen6_attrs[] = { &dev_attr_gt_act_freq_mhz.attr, &dev_attr_gt_cur_freq_mhz.attr, @@ -492,6 +520,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_gem_allow_sseu.attr, NULL, }; @@ -505,6 +534,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_gem_allow_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. Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 +++ drivers/gpu/drm/i915/i915_gem_context.c | 52 ++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_sysfs.c | 30 ++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-)