Re: [RFC 1/2] drm/xe: Introduce PM guard classes

[Lucas De Marchi]

On Mon, Jun 17, 2024 at 04:55:30PM GMT, Michal Wajdeczko wrote:
> On 17.06.2024 16:37, Lucas De Marchi wrote:
> > On Mon, Jun 17, 2024 at 04:01:19PM GMT, Michal Wajdeczko wrote:
> > > There is support for 'classes' with constructor and destructor
> > > semantics that can be used for scope-based resource management,
> > > like our device power management.
> > >
> > > Use provided macros from linux/cleanup.h to generate required
> > > code definitions.
> > >
> > > This should allow us to use:
> > >
> > >     scoped_guard(xe_pm, xe)
> > >         foo();
> > > or
> > >     CLASS(xe_pm, var)(xe);
> > >
> > > without any concern of leaking the pm reference.
> > >
> > > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/xe/xe_pm.h | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
> > > index 104a21ae6dfd..26293a3b18af 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.h
> > > +++ b/drivers/gpu/drm/xe/xe_pm.h
> > > @@ -6,6 +6,7 @@
> > > #ifndef _XE_PM_H_
> > > #define _XE_PM_H_
> > >
> > > +#include <linux/cleanup.h>
> >
> > Cool, I didn't know we had  helpers in the kernel for attribute cleanup
> > and friends.
> >
> > > #include <linux/pm_runtime.h>
> > >
> > > #define DEFAULT_VRAM_THRESHOLD 300 /* in MB */
> > > @@ -33,4 +34,8 @@ int xe_pm_set_vram_threshold(struct xe_device *xe,
> > > u32 threshold);
> > > void xe_pm_d3cold_allowed_toggle(struct xe_device *xe);
> > > struct task_struct *xe_pm_read_callback_task(struct xe_device *xe);
> > >
> > > +DEFINE_GUARD(xe_pm, struct xe_device *, xe_pm_runtime_get(_T),
> > > xe_pm_runtime_put(_T))
> >
> > per linux/cleanup.h doc it seems DEFINE_GUARD() was intended
> > specifically for locks though:
> >
> >  * DEFINE_GUARD(name, type, lock, unlock):
> >  *      trivial wrapper around DEFINE_CLASS() above specifically
> >  *      for locks.
> >
> > we probably shouldn't abuse it or get the doc changed
>
> probably the latter as this is exactly what we want
>
> besides, maybe we can argue that what we do is actually 'locking' since
> this is a required step, like obtaining mutex, before we start to use
> the device

the problem is that if then later there's something really specific to
locks like plugging lockdep into these. Then our abuse of these
interfaces would probably make people angry. Let's Cc maintainers and
get their opinion. Cc'ing lkml and maintainers.

thanks
Lucas De Marchi

> > Lucas De Marchi
> >
> > > +DEFINE_GUARD_COND(xe_pm, _if_active, xe_pm_runtime_get_if_active(_T))
> > > +DEFINE_GUARD_COND(xe_pm, _if_in_use, xe_pm_runtime_get_if_in_use(_T))
> > > +
> > > #endif
> > > -- 
> > > 2.43.0