[Intel-gfx] [PATCH v6 7/7] drm/i915: add a sysfs entry to let users set sseu configs
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Thu May 24 10:45:34 UTC 2018
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 at 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
>
More information about the Intel-gfx
mailing list