Re: [Intel-gfx] [PATCH] drm/i915/gt: make a gt sysfs group and move power management files

[Andi Shyti <andi.shyti@intel.com>]

Hi Tvrtko,

> > > > +	}
> > > > +
> > > > +	intel_gt_sysfs_pm_remove(gt, root);
> > > > +	kobject_put(root);
> > > 
> > > Maybe stick to the same terminology regarding root and parent.
> > 
> > yes.
> > 
> > > Get/put on the parent looks unbalanced. Both register and unregister take a
> > > reference and only unregister releases it. But do you even need a reference?
> > 
> > why? I take it here:
> > 
> > static inline struct kobject *gt_to_parent_obj(struct intel_gt *gt)
> > {
> > 	return kobject_get(&gt->i915->drm.primary->kdev->kobj);
> > }
> > 
> > at the beginning (when the driver is loaded) and I release it at
> > the end (when the driver is unloaded). Am I not seeing something?
> 
> Gt_to_parent_obj at the top of intel_gt_sysfs_register balances out with the
> put at the end of the same function. What balances out gt_to_parent_obj from
> intel_gt_sysfs_register?

And... you are right!
either nothing or too many :)

> > > I am also tempted by the _once alternative, but then it makes less sense to
> > > include name & pid.
> > 
> > It's true, it can be an unrelenting message, and I thought of it,
> > but if the user is resilient at reading out from the wrong
> > directory, why shouldn't I :)
> 
> Because we always try to avoid emitting spammy logs when they can be easily
> triggered by userspace. Can we do rate limit? I think that could work well
> with logging the process name & pid.

yes, if two people suggested the same thing, most probably that's
the right thing to do.

> Also, we need an entry in Documentation/ABI/obsolete/.

I was waiting this patch to get in shape before adding the
interface to obsolete.

I will include it in the next patch.

Thanks a lot for the review,
Andi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx