[PATCH] drm/xe: sysfs_ops needs to be defined on parent directory
Rodrigo Vivi
rodrigo.vivi at intel.com
Tue Mar 25 19:14:11 UTC 2025
On Tue, Mar 25, 2025 at 03:18:40PM +0530, Ghimiray, Himal Prasad wrote:
>
>
> On 19-03-2025 17:43, Tejas Upadhyay wrote:
> > Currently, xe_hw_engine_sysfs_kobj_type is defining sysfs_ops
> > on wrong directory. Sysfs_ops needs to be defined on immediate
> > parent directory to be able to called on each attribute set/get.
> >
> > Fixes: 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs call")
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c | 67 +++++++++----------
> > 1 file changed, 33 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> > index b53e8d2accdb..25592f178482 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.c
> > @@ -492,39 +492,6 @@ static const struct attribute * const files[] = {
> > NULL
> > };
> > -static void kobj_xe_hw_engine_class_fini(void *arg)
> > -{
> > - struct kobject *kobj = arg;
> > -
> > - sysfs_remove_files(kobj, files);
> > - kobject_put(kobj);
> > -}
> > -
> > -static struct kobj_eclass *
> > -kobj_xe_hw_engine_class(struct xe_device *xe, struct kobject *parent, const char *name)
> > -{
> > - struct kobj_eclass *keclass;
> > - int err = 0;
> > -
> > - keclass = kzalloc(sizeof(*keclass), GFP_KERNEL);
> > - if (!keclass)
> > - return NULL;
> > -
> > - kobject_init(&keclass->base, &kobj_xe_hw_engine_type);
> > - if (kobject_add(&keclass->base, parent, "%s", name)) {
> > - kobject_put(&keclass->base);
> > - return NULL;
> > - }
> > - keclass->xe = xe;
> > -
> > - err = devm_add_action_or_reset(xe->drm.dev, kobj_xe_hw_engine_class_fini,
> > - &keclass->base);
> > - if (err)
> > - return NULL;
> > -
> > - return keclass;
> > -}
> > -
> > static void hw_engine_class_defaults_fini(void *arg)
> > {
> > struct kobject *kobj = arg;
> > @@ -611,6 +578,38 @@ static const struct kobj_type xe_hw_engine_sysfs_kobj_type = {
> > .sysfs_ops = &xe_hw_engine_class_sysfs_ops,
> > };
> > +static void kobj_xe_hw_engine_class_fini(void *arg)
> > +{
> > + struct kobject *kobj = arg;
> > +
> > + sysfs_remove_files(kobj, files);
> > + kobject_put(kobj);
> > +}
> > +
> > +static struct kobj_eclass *
> > +kobj_xe_hw_engine_class(struct xe_device *xe, struct kobject *parent, const char *name)
> > +{
> > + struct kobj_eclass *keclass;
> > + int err = 0;
> > +
> > + keclass = kzalloc(sizeof(*keclass), GFP_KERNEL);
> > + if (!keclass)
> > + return NULL;
> > +
> > + kobject_init(&keclass->base, &xe_hw_engine_sysfs_kobj_type);
> > + if (kobject_add(&keclass->base, parent, "%s", name)) {
> > + kobject_put(&keclass->base);
> > + return NULL;
> > + }
> > + keclass->xe = xe;
> > +
> > + err = devm_add_action_or_reset(xe->drm.dev, kobj_xe_hw_engine_class_fini,
> > + &keclass->base);
> > + if (err)
> > + return NULL;
> > +
> > + return keclass;
> > +}
> > static void hw_engine_class_sysfs_fini(void *arg)
> > {
> > struct kobject *kobj = arg;
> > @@ -640,7 +639,7 @@ int xe_hw_engine_class_sysfs_init(struct xe_gt *gt)
> > if (!kobj)
> > return -ENOMEM;
> > - kobject_init(kobj, &xe_hw_engine_sysfs_kobj_type);
> > + kobject_init(kobj, &kobj_xe_hw_engine_type);
>
>
> The patch makes sense if we need to use xe_pm_runtime_get and
> xe_pm_runtime_put to show and store the attributes. Are there any hardware
> read/writes associated with these attribute store/show operations? If not,
> why do we need pm_runtime just for software states?
>
> +Rodrigo, for his views here.
Better safe than sorry. The goal is to protect every entry point on the
upper (outer) levels that might interact with or trigger some memory operation.
>
> > err = kobject_add(kobj, gt->sysfs, "engines");
> > if (err)
>
More information about the Intel-xe
mailing list