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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx