[PATCH 1/3] drm/i915: Show engine flags in sysfs

[Tvrtko Ursulin]

On 02/06/2020 16:55, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-06-02 16:42:45)
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> Show engine flags together with other per-engine data in sysfs.
>>
>> We need this in order to be able to support heterogenous feature sets
>> between the engines which are currently not visible from the scheduler
>> caps interface.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/sysfs_engines.c | 86 +++++++++++++++++++++----
>>   1 file changed, 75 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 535cc1169e54..36e0c2fcebef 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -48,6 +48,79 @@ inst_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
>>   static struct kobj_attribute inst_attr =
>>   __ATTR(instance, 0444, inst_show, NULL);
>>   
>> +static ssize_t repr_trim(char *buf, ssize_t len)
>> +{
>> +       /* Trim off the trailing space and replace with a newline */
>> +       if (len > PAGE_SIZE)
>> +               len = PAGE_SIZE;
>> +       if (len > 0)
>> +               buf[len - 1] = '\n';
>> +
>> +       return len;
>> +}
>> +
>> +static const char * const engine_flags[] = {
>> +       [ilog2(I915_ENGINE_USING_CMD_PARSER)] = "cmdparser",
>> +       [ilog2(I915_ENGINE_SUPPORTS_STATS)] = "busy_stats",
>> +       [ilog2(I915_ENGINE_HAS_PREEMPTION)] = "preemption",
>> +       [ilog2(I915_ENGINE_HAS_SEMAPHORES)] = "semaphores",
>> +       [ilog2(I915_ENGINE_HAS_TIMESLICES)] = "timeslicing",
>> +       [ilog2(I915_ENGINE_NEEDS_BREADCRUMB_TASKLET)] = "", /* non-abi */
>> +       [ilog2(I915_ENGINE_IS_VIRTUAL)] = "", /* non-abi */
>> +       [ilog2(I915_ENGINE_HAS_RELATIVE_MMIO)] = "relative_mmio",
>> +       [ilog2(I915_ENGINE_REQUIRES_CMD_PARSER)] = "requires_cmdparser",
> 
> s/requires_//; to the user they only should know that there is one.
> 
> using/requires should be an internal detail. Probably we should make
> requires be non-abi and also mark up using-cmd-parser for gen9. (For
> reporting simplicity.)

Yeah I did not figure out the difference between "requires" and "using" 
on a quick glance so that probably means there is scope for improvement.

> Although we should get around to fleshing out the ability to use
> timeslicing effectively from userspace :p

What do you mean by this?

>> +};
>> +
>> +static ssize_t
>> +__flags_show(struct intel_engine_cs *engine, unsigned int flags, char *buf,
>> +            bool show_unknown)
>> +{
>> +       const unsigned int count = ARRAY_SIZE(engine_flags);
>> +       unsigned int n;
>> +       ssize_t len;
>> +
>> +       BUILD_BUG_ON(!typecheck(typeof(flags), engine->flags));
>> +       BUILD_BUG_ON(count > BITS_PER_TYPE(typeof(flags)));
>> +
>> +       len = 0;
>> +       for_each_set_bit(n,
>> +                        (unsigned long *)&flags,
>> +                        show_unknown ? BITS_PER_TYPE(typeof(flags)) : count) {
>> +               if (n >= count || !engine_flags[n]) {
>> +                       if (GEM_WARN_ON(show_unknown))
>> +                               len += snprintf(buf + len, PAGE_SIZE - len,
>> +                                               "[%x] ", n);
>> +               } else {
>> +                       if (strlen(engine_flags[n]))
> 
> Good, you hid the extra spaces.
> 
> Heh, I wonder if we should alphabetically sort the strings :)
> Just so we don't get into trouble with the order becoming ABI
> (constraining the flag values).

We have to draw the line somewhere and before sorting looks like the 
place to do it. :) It should be obvious to anyone remotely that the list 
is not ordered.

Regards,

Tvrtko