On Fri, 2019-10-11 at 12:36 +0100, Chris Wilson wrote: > Preliminary stub to add engines underneath /sys/class/drm/cardN/, so > that we can expose properties on each engine to the sysadmin. > > To start with we have basic analogues of the i915_query ioctl so that > we > can pretty print engine discovery from the shell, and flesh out the > directory structure. Later we will add writeable sysadmin properties > such > as per-engine timeout controls. > > An example tree of the engine properties on Braswell: > /sys/class/drm/card0 > └── engine > ├── bcs0 > │ ├── capabilities > │ ├── class > │ ├── instance > │ └── name > ├── rcs0 > │ ├── capabilities > │ ├── class > │ ├── instance > │ └── name > ├── vcs0 > │ ├── capabilities > │ ├── class > │ ├── instance > │ └── name > └── vecs0 > ├── capabilities > ├── class > ├── instance > └── name > > v2: Include stringified capabilities > > Signed-off-by: Chris Wilson <[email protected]> > Cc: Joonas Lahtinen <[email protected]> > Cc: Tvrtko Ursulin <[email protected]> > Cc: Daniele Ceraolo Spurio <[email protected]> > Cc: Rodrigo Vivi <[email protected]> > Acked-by: Rodrigo Vivi <[email protected]> > --- > drivers/gpu/drm/i915/Makefile | 3 +- > drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 177 > +++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_engine_sysfs.h | 14 ++ > drivers/gpu/drm/i915/i915_sysfs.c | 3 + > 4 files changed, 196 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.c > create mode 100644 drivers/gpu/drm/i915/gt/intel_engine_sysfs.h > > diff --git a/drivers/gpu/drm/i915/Makefile > b/drivers/gpu/drm/i915/Makefile > index e791d9323b51..cd9a10ba2516 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -78,8 +78,9 @@ gt-y += \ > gt/intel_breadcrumbs.o \ > gt/intel_context.o \ > gt/intel_engine_cs.o \ > - gt/intel_engine_pool.o \ > gt/intel_engine_pm.o \ > + gt/intel_engine_pool.o \ > + gt/intel_engine_sysfs.o \ > gt/intel_engine_user.o \ > gt/intel_gt.o \ > gt/intel_gt_irq.o \ > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c > b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c > new file mode 100644 > index 000000000000..f6e4822f8928 > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c > @@ -0,0 +1,177 @@ > +/* > + * SPDX-License-Identifier: MIT > + * > + * Copyright © 2019 Intel Corporation > + */ > + > +#include <linux/kobject.h> > +#include <linux/sysfs.h> > + > +#include "i915_drv.h" > +#include "intel_engine.h" > +#include "intel_engine_sysfs.h" > + > +struct kobj_engine { > + struct kobject base; > + struct intel_engine_cs *engine; > +}; > + > +static struct intel_engine_cs *kobj_to_engine(struct kobject *kobj) > +{ > + return container_of(kobj, struct kobj_engine, base)->engine; > +} > + > +static ssize_t > +name_show(struct kobject *kobj, struct kobj_attribute *attr, char > *buf) > +{ > + return sprintf(buf, "%s\n", kobj_to_engine(kobj)->name); > +} > + > +static struct kobj_attribute name_attr = > +__ATTR(name, 0444, name_show, NULL); > + > +static ssize_t > +class_show(struct kobject *kobj, struct kobj_attribute *attr, char > *buf) > +{ > + return sprintf(buf, "%d\n", kobj_to_engine(kobj)->uabi_class); > +} > + > +static struct kobj_attribute class_attr = > +__ATTR(class, 0444, class_show, NULL); > + > +static ssize_t > +inst_show(struct kobject *kobj, struct kobj_attribute *attr, char > *buf) > +{ > + return sprintf(buf, "%d\n", kobj_to_engine(kobj)- > >uabi_instance); > +} > + > +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 ssize_t > +caps_show(struct kobject *kobj, struct kobj_attribute *attr, char > *buf) > +{ > + static const char *vcs_repr[] = { > + [ilog2(I915_VIDEO_CLASS_CAPABILITY_HEVC)] = "hevc", > + [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = > "sfc", > + }; > + static const char *vecs_repr[] = { > + [ilog2(I915_VIDEO_AND_ENHANCE_CLASS_CAPABILITY_SFC)] = > "sfc", > + };
Is this something we can add as some sort of property directly to the
capability definitions? Keeping a separate definition here seems like a
maintainability concern.
> + struct intel_engine_cs *engine = kobj_to_engine(kobj);
> + const char **repr;
> + int num_repr, n;
> + ssize_t len;
> +
> + switch (engine->class) {
> + case VIDEO_DECODE_CLASS:
> + repr = vcs_repr;
> + num_repr = ARRAY_SIZE(vcs_repr);
> + break;
> +
> + case VIDEO_ENHANCEMENT_CLASS:
> + repr = vecs_repr;
> + num_repr = ARRAY_SIZE(vecs_repr);
> + 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 (n < num_repr && repr[n])
> + len += snprintf(buf + len, PAGE_SIZE - len,
> + "%s ", repr[n]);
> + else
> + len += snprintf(buf + len, PAGE_SIZE - len,
> + "[%d] ", n);
> + }
> + return repr_trim(buf, len);
> +}
> +
> +static struct kobj_attribute caps_attr =
> +__ATTR(capabilities, 0444, caps_show, NULL);
> +
> +static void kobj_engine_release(struct kobject *kobj)
> +{
> + kfree(kobj);
> +}
> +
> +static struct kobj_type kobj_engine_type = {
> + .release = kobj_engine_release,
> + .sysfs_ops = &kobj_sysfs_ops
> +};
> +
> +static struct kobject *
> +kobj_engine(struct kobject *dir, struct intel_engine_cs *engine)
> +{
> + struct kobj_engine *ke;
> +
> + ke = kzalloc(sizeof(*ke), GFP_KERNEL);
> + if (!ke)
> + return NULL;
> +
> + kobject_init(&ke->base, &kobj_engine_type);
> + ke->engine = engine;
> +
> + if (kobject_add(&ke->base, dir, "%s", engine->name)) {
> + kobject_put(&ke->base);
> + return NULL;
> + }
> +
> + /* xfer ownership to sysfs tree */
> + return &ke->base;
> +}
> +
> +void intel_engines_add_sysfs(struct drm_i915_private *i915)
> +{
> + static const struct attribute *files[] = {
> + &name_attr.attr,
> + &class_attr.attr,
> + &inst_attr.attr,
> + &caps_attr.attr,
> + NULL
> + };
> +
> + struct device *kdev = i915->drm.primary->kdev;
> + struct intel_engine_cs *engine;
> + struct kobject *dir;
> +
> + dir = kobject_create_and_add("engine", &kdev->kobj);
> + if (!dir)
> + return;
> +
> + for_each_uabi_engine(engine, i915) {
> + struct kobject *kobj;
> +
> + kobj = kobj_engine(dir, engine);
> + if (!kobj)
> + goto err_engine;
> +
> + if (sysfs_create_files(kobj, files))
> + goto err_engine;
> +
> + if (0) {
> +err_engine:
Can you explain why we need this goto/if 0 over a simple print/break
under the if(sysfs_create_files()) call above? At a glance this just
seems overly complicated.
Thanks,
Stuart
> + dev_err(kdev, "Failed to add sysfs engine
> '%s'\n",
> + engine->name);
> + break;
> + }
> + }
> +}
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
> b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
> new file mode 100644
> index 000000000000..ef44a745b70a
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.h
> @@ -0,0 +1,14 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#ifndef INTEL_ENGINE_SYSFS_H
> +#define INTEL_ENGINE_SYSFS_H
> +
> +struct drm_i915_private;
> +
> +void intel_engines_add_sysfs(struct drm_i915_private *i915);
> +
> +#endif /* INTEL_ENGINE_SYSFS_H */
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index bf039b8ba593..7b665f69f301 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -30,6 +30,7 @@
> #include <linux/stat.h>
> #include <linux/sysfs.h>
>
> +#include "gt/intel_engine_sysfs.h"
> #include "gt/intel_rc6.h"
>
> #include "i915_drv.h"
> @@ -616,6 +617,8 @@ void i915_setup_sysfs(struct drm_i915_private
> *dev_priv)
> DRM_ERROR("RPS sysfs setup failed\n");
>
> i915_setup_error_capture(kdev);
> +
> + intel_engines_add_sysfs(dev_priv);
> }
>
> void i915_teardown_sysfs(struct drm_i915_private *dev_priv)
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
