[PATCH] drm/xe: sysfs_ops needs to be defined on parent directory
Upadhyay, Tejas
tejas.upadhyay at intel.com
Wed Mar 26 14:50:24 UTC 2025
> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: 26 March 2025 20:13
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH] drm/xe: sysfs_ops needs to be defined on parent
> directory
>
> On Wed, Mar 26, 2025 at 04:14:21AM +0000, Upadhyay, Tejas wrote:
> >
> >
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > > Sent: Wednesday, March 26, 2025 12:43 AM
> > > To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> > > Cc: intel-xe at lists.freedesktop.org; Ghimiray, Himal Prasad
> > > <himal.prasad.ghimiray at intel.com>
> > > Subject: Re: [PATCH] drm/xe: sysfs_ops needs to be defined on parent
> > > directory
> > >
> > > On Wed, Mar 19, 2025 at 05:43:49PM +0530, 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.
> > >
> > > Please bare with me, but I'm having a hard time to follow this
> > > reasoning and the patch, why, and everything else going on here...
> > >
> > > >
> > > > Fixes: 3f0e14651ab0 ("drm/xe: Runtime PM wake on every sysfs
> > > > call")
> > >
> > > Why this patch is claiming to fix this? Please note that this
> > > mentioned patch just replace the default kobj_sysfs_ops per the new
> > > introduced xe_hw_engine_class_sysfs_ops. They are basically identical
> functions.
> > > The only difference is that the new one call our runtime pm wrappers
> > > around the calls that we need.
> >
> > Earlier we were not doing any common ops for all attr set/get, each
> attribute has individual setter/getter. With 3f0e14651ab0 ("drm/xe: Runtime
> PM wake on every sysfs call"), we introduced common ops on all attr set/get
> and which is must to be called now. Thus I though of adding fixes. Please let
> me know if you guys think otherwise I will just remove, not strong objection.
>
> I'm sorry for the noise. I probably had enough coffee today, so yes, this is the
> right Fixes tag ;)
>
> >
> > >
> > > It never touched anything that this patch is touching below.
> > >
> > > Perhaps if we also need on the sysfs on upper directory, perhaps we
> > > need to also replace the default sysfs_ops on other places like in the
> upper directory?
> > >
> > > > 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;
> > > > +}
> > >
> > > I couldn't spot any difference from the both chunks above. So this
> > > patch is more moving things around than doing any change right?
> > > perhaps a different patch or a mention on the commit message itself?
> >
> > The change is, immediate parent directory should be defined with
> attr_show/store ops if we want it to be called on every attr set/get call.
> Currently we had sysfs_ops defined on parent to parent directory which does
> not get called on underlying child's attr set/get call.
>
> Okay, now I could spot the change:
>
> inside kobj_xe_hw_engine_class()
> - kobject_init(&keclass->base, &kobj_xe_hw_engine_type);
> + kobject_init(&keclass->base, &xe_hw_engine_sysfs_kobj_type);
>
> and inside xe_hw_engine_class_sysfs_init()
>
> - kobject_init(kobj, &xe_hw_engine_sysfs_kobj_type);
> + kobject_init(kobj, &kobj_xe_hw_engine_type);
>
> With this you invert where the runtime_pm is called moving it to the right
> parent.
> I partially agree with the fix, but I don't agree with the patch itself.
> Not only because I got confused, but because it creates inconsistency:
>
> kobj_xe_hw_engine_class now uses xe_hw_engine_sysfs_kobj_type and
> xe_hw_engine_class_sysfs_init now uses kobj_xe_hw_engine_type
>
> If we want to invert, we need to invert the sysfs_ops inside the type functions.
> But also, I don't believe that there is the problem on leaving the runtime_pm
> hooks in the parent of parent. Just in case we end up adding some extra file
> there.
>
> So, What about simply:
>
> static const struct kobj_type kobj_xe_hw_engine_type = {
> .release = kobj_xe_hw_engine_release,
> - .sysfs_ops = &kobj_sysfs_ops
> + .sysfs_ops = &xe_hw_engine_class_sysfs_ops,
> };
>
> Thanks for catching that up and fixing it, Rodrigo.
Only problem with this approach is, .defaults will need in its ops, as it is also using kobj_xe_hw_engine_type and .default has only getter method. Let me rethink if I can do something for .defaults
Tejas
>
> >
> > Tejas
> > >
> > > > 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);
> > >
> > > now it looks like this is the only real chunk of this patch, last
> > > touched when you added it:
> > > 038ff941afe2 ("drm/xe: Add sysfs entries for engines under its GT")
> > >
> > > And now, after this patch, who is now using
> xe_hw_engine_sysfs_kobj_type?
> > >
> > > Looking further to both types, perhaps we need to kill the
> > > kobj_xe_hw_engine_type itself in favor of the xe_ one?
> > >
> > > Or perhaps we need something like this:
> > >
> > > static const struct kobj_type kobj_xe_hw_engine_type = {
> > > .release = kobj_xe_hw_engine_release,
> > > - .sysfs_ops = &kobj_sysfs_ops
> > > + .sysfs_ops = &xe_hw_engine_class_sysfs_ops,
> > > };
> > >
> > > >
> > > > err = kobject_add(kobj, gt->sysfs, "engines");
> > > > if (err)
> > > > --
> > > > 2.34.1
> > > >
More information about the Intel-xe
mailing list