[Intel-gfx] [PATCH v6 7/7] drm/i915: add a sysfs entry to let users set sseu configs

[Tvrtko Ursulin]

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]

>>
>>> +
>>> +    /*
>>> +     * 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