[Intel-gfx] [PATCH 08/15] drm/i915: Expose engine properties via sysfs

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-10-14 11:17:54)
> 
> On 14/10/2019 10:07, Chris Wilson wrote:
> > +static ssize_t
> > +caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     struct intel_engine_cs *engine = kobj_to_engine(kobj);
> > +     const char * const *repr;
> > +     int num_repr, n;
> > +     ssize_t len;
> > +
> > +     switch (engine->class) {
> > +     case VIDEO_DECODE_CLASS:
> > +             repr = vcs_caps;
> > +             num_repr = ARRAY_SIZE(vcs_caps);
> > +             break;
> > +
> > +     case VIDEO_ENHANCEMENT_CLASS:
> > +             repr = vecs_caps;
> > +             num_repr = ARRAY_SIZE(vecs_caps);
> > +             break;
> > +
> > +     default:
> > +             repr = NULL;
> > +             num_repr = 0;
> > +             break;
> > +     }
> > +
> > +     len = 0;
> > +     for_each_set_bit(n,
> > +                      (unsigned long *)&engine->uabi_capabilities,
> > +                      BITS_PER_TYPE(typeof(engine->uabi_capabilities))) {
> > +             if (GEM_WARN_ON(n >= num_repr || !repr[n]))
> > +                     len += snprintf(buf + len, PAGE_SIZE - len,
> > +                                     "[%x] ", n);
> > +             else
> > +                     len += snprintf(buf + len, PAGE_SIZE - len,
> > +                                     "%s ", repr[n]);
> > +             if (GEM_WARN_ON(len >= PAGE_SIZE))
> > +                     break;
> > +     }
> > +     return repr_trim(buf, len);
> > +}
> > +
> > +static struct kobj_attribute caps_attr =
> > +__ATTR(capabilities, 0444, caps_show, NULL);
> > +
> > +static ssize_t
> > +all_caps_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     struct intel_engine_cs *engine = kobj_to_engine(kobj);
> > +     const char * const *repr;
> > +     int num_repr, n;
> > +     ssize_t len;
> > +
> > +     switch (engine->class) {
> > +     case VIDEO_DECODE_CLASS:
> > +             repr = vcs_caps;
> > +             num_repr = ARRAY_SIZE(vcs_caps);
> > +             break;
> > +
> > +     case VIDEO_ENHANCEMENT_CLASS:
> > +             repr = vecs_caps;
> > +             num_repr = ARRAY_SIZE(vecs_caps);
> > +             break;
> > +
> > +     default:
> > +             repr = NULL;
> > +             num_repr = 0;
> > +             break;
> > +     }
> > +
> > +     len = 0;
> > +     for (n = 0; n < num_repr; n++) {
> > +             if (!repr[n])
> > +                     continue;
> > +
> > +             len += snprintf(buf + len, PAGE_SIZE - len, "%s ", repr[n]);
> > +             if (GEM_WARN_ON(len >= PAGE_SIZE))
> > +                     break;
> > +     }
> > +     return repr_trim(buf, len);
> > +}
> 
> Would it be worth consolidating like:
> 
> __caps_show(engine, buf, len, mask, warn_unknown)
> {
> ...
>         switch(engine->class...
> ...
>         for_each_set_bit(...mask...) {
>                 if (n >= repr || !repr[n]) {
>                         if (warn_unknown)
>                                 GEM_WARN_ON(...);
>                         else
>                                 len += snprinf...
>                 } else {
>                         len += snprintf...
>                 }
>         }
> ...
> }
> 
> caps_show()
> {
> ...
>         len = __caps_show(engine, buf, len, engine->uabi_capabilities, true);
> ...
> }
> 
> all_caps_show()
> {
> ...
>         len = __caps_show(engine, buf, len, ~0, false);
> ...
> }

I thought it would look ugly and distract from the intent. One is to
list the bits, the other the array of string.

But there is a certain appeal to having just one setup and loop.
-Chris