Re: [PATCH] drm/i915: Expose engine properties via sysfs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Tvrtko Ursulin (2019-09-18 11:26:57)
> 
> On 18/09/2019 10:23, 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.
> 
> Do we also envisage a need for these future things we'll expose to be 
> per-context-engine and not just per physical engine?

At the moment, I only have plans for sysadmin controls of physical
engines. I expect users to configure their own contexts from ioctls,
with restrictions imposed by cgroups, which should cover per-context-engines.

> > 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.
> 
> It would be good to show an example of the layout in commit text.

/sys/class/drm/card0
└── engine
    ├── bcs0
    │   ├── class
    │   ├── heartbeat_interval_ms
    │   ├── instance
    │   ├── mmio_base
    │   └── name
    ├── rcs0
    │   ├── class
    │   ├── heartbeat_interval_ms
    │   ├── instance
    │   ├── mmio_base
    │   └── name
    ├── vcs0
    │   ├── class
    │   ├── heartbeat_interval_ms
    │   ├── instance
    │   ├── mmio_base
    │   └── name
    └── vecs0
        ├── class
        ├── heartbeat_interval_ms
        ├── instance
        ├── mmio_base
        └── name


> 
> > 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>
> > ---
> >   drivers/gpu/drm/i915/Makefile                |   3 +-
> >   drivers/gpu/drm/i915/gt/intel_engine_sysfs.c | 113 +++++++++++++++++++
> >   drivers/gpu/drm/i915/gt/intel_engine_sysfs.h |  14 +++
> >   drivers/gpu/drm/i915/i915_sysfs.c            |   4 +
> >   4 files changed, 133 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 658b930d34a8..bbea0d4dadd6 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -76,8 +76,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..51b4b3f2a808
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_sysfs.c
> > @@ -0,0 +1,113 @@
> > +/*
> > + * 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);
> 
> Name contains our internal instance number which I think we don't want 
> to export.

No it doesn't :-p

> On the other hand we could think of dmesg as user visible so could 
> consider tweaking engine->name to be built from uabi components.

Already done.

> > +}
> > +
> > +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 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 ssize_t
> > +mmio_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +     return sprintf(buf, "0x%x\n", kobj_to_engine(kobj)->mmio_base);
> > +}
> 
> Nice try ;) but I suggest you leave adding mmio for a separate patch.

Just look the other way for once.

> > +static struct kobj_attribute name_attr = __ATTR(name, 0444, name_show, NULL);
> > +static struct kobj_attribute class_attr = __ATTR(class, 0444, class_show, NULL);
> > +static struct kobj_attribute inst_attr = __ATTR(instance, 0444, inst_show, NULL);
> > +static struct kobj_attribute mmio_attr = __ATTR(instance, 0444, mmio_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;
> 
> Could embed kobj into the engine?

My thinking was if we did that, the natural hierarchy would be
i915->gt->engine, and I wasn't ready to fully commit to that level of
detail.

> 
> > +
> > +     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;
> > +     }
> > +
> > +     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,
> > +             &mmio_attr.attr,
> > +     };
> > +
> > +     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)
> > +                     continue;
> 
> Could be consistent and log an error in this case as well.

We didn't need to log an error for create_files, sysfs does itself as
well. It just has a __must_check even though its author went on a
crusade saying "it's not fatal, stop propagating the error!". Grr.

> > +             if (sysfs_create_files(kobj, files)) {
> > +                     dev_err(kdev, "Failed to add sysfs engine '%s'\n",
> > +                             engine->name);
> > +                     break;
> 
> Leaks kobj.

kobj is hooked into and owned by the sysfs tree.

(Or I may be leaking it in the kobject_add...)

> > 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 d8a3b180c084..6b88d934927a 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -30,6 +30,8 @@
> >   #include <linux/stat.h>
> >   #include <linux/sysfs.h>
> >   
> > +#include "gt/intel_engine_sysfs.h"
> > +
> >   #include "i915_drv.h"
> >   #include "i915_sysfs.h"
> >   #include "intel_pm.h"
> > @@ -618,6 +620,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);
> 
> Or gt?

We want the flat uabi engine list which is tucked away under i915.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux