Quoting Tvrtko Ursulin (2019-10-11 13:16:33)
>
> On 11/10/2019 12:36, 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 <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Acked-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > 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",
> > + };
> > + 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);
>
> Maybe we should also add a GEM_WARN_ON to reminds us to add new caps in
> here during development? Presumes some IGT to exercise this of course,
> or unfortunately. :)
iirc, I remember correctly we do a fs-walk over sysfs and cat each file,
so yeah we should be covered if we add a GEM_WARN_ON.
> Minor conversation point - hex or decimal?
hex with no leading 0x just to confuse :)
> > +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;
>
> Do we need to kobject_put in this case?
Indeed.
> > + if (0) {
> > +err_engine:
> > + dev_err(kdev, "Failed to add sysfs engine '%s'\n",
> > + engine->name);
>
> Maybe not error but warning? Driver continues to work fine but userspace
> need to be aware.. hm.. tricky one.
Yeah, it's definitely more than a mere notice that the driver is
misbehaving as we want to use it as a control interface. If the sysadmin
cannot setup the system as they require it, we expect their users to
complain later on.
> > + 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)
> >
>
> Looks fine in principle.
>
> I am tempted to go with some of the engine->flags from the start but I
> don't have an use case apart from saying that it sounds like it makes sense.
The problem I saw with engine->flags is that are mostly internal
details; the public parts become caps.scheduler. That seemed reasonable
to add, but where? cardN/scheduler (same level as cardN/engine)? But
there's a large overlap between that and the per-engine controls. So
cardN/engine/scheduler? Then the user has to remember that not all
entries are engines.
I suppose cardN/engine/all/ would fit the pattern of some other sysfs
interfaces [cardN/engine/all/scheduler/caps?] Only problem then is you
would reasonably expect to have some global controls :)
> Preemption cap may make sense in conjunction with heartbeat interval to
> tell userspace something more?
>
> Busy_stats might make sense for people interested in doing perf stat?
>
> What worries me slightly is not being able to tell if a cap is supported
> just not exported, or not supported but exported, if you are able to
> follow. :) Which would happen if we decided to later add something from
> flags.
Along that path we end up with bcs0/available_flags and bcs0/flags (or
bcs0/available_capabilities and bcs0/capabilities). Or
cardN/engine/version?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
