Re: [PATCH v2 1/2] drm/xe: Introduce force-wake guard class

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Nov 19, 2024 at 09:26:42PM +0100, Michal Wajdeczko wrote:
> 
> 
> On 19.11.2024 20:59, Lucas De Marchi wrote:
> > On Tue, Nov 19, 2024 at 09:21:17PM +0200, Raag Jadav wrote:
> >> On Tue, Nov 19, 2024 at 01:47:28PM -0500, Rodrigo Vivi wrote:
> >>> On Mon, Nov 18, 2024 at 07:45:10PM +0100, Michal Wajdeczko wrote:
> >>> > There is support for 'classes' with constructor and destructor
> >>> > semantics that can be used for any scope-based resource management,
> >>> > like our domain force-wake management.
> >>>
> >>> There is, okay...
> >>> It can be, okay...
> >>>
> >>> But why do we want?
> >>> What are the advantages?
> >>
> >> Makes error cases easier to handle + less code.
> 
> and this 'easier' is not just that as it also means that code is much
> more robust as it's the compiler job to take care of all the cleanup,
> which could be tricky when doing early exits from different paths, or
> due to late updates to the code done by someone else
> 
> >> We significantly cleaned up intel pinctrl and gpio drivers a few
> >> months ago
> >> and almost halved the footprint in some cases.
> > 
> > I'm ok with using this pattern as I already expressed before.
> > Particularly seeing other subsystems adopting it and being used well in
> > the core kernel as well.
> > 
> > Main ask here is to provide proper justification in the commit message.
> 
> there was a little more in the cover letter:
> 
> ""
> This should allow us to use:
> 
> 	CLASS(xe_fw, var)(fw, XE_FORCEWAKE_ALL);
> or
> 	guard(xe_fw)(fw, XE_FW_GT);
> or
> 	scoped_guard(xe_fw, fw, XE_FW_GT)
> 		foo();
> 
> without any concern of leaking the force-wake references.
> ""
> 
> do you want/need more?

this block here also just say 'allow to use' but fail to describe
why we should use. But the upper block started by Raav seems a good
why. Just make sure that that is in the commit message and

Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > Lucas De Marchi
> > 
> >>
> >> Raag
> >>
> >>> >
> >>> > Define xe_fw class that can be used for force-wake management.
> >>> >
> >>> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >>> > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> >>> > ---
> >>> > v2: aligned to new force-wake usage model (Michal)
> >>> > ---
> >>> >  drivers/gpu/drm/xe/xe_force_wake.h       | 12 ++++++++++++
> >>> >  drivers/gpu/drm/xe/xe_force_wake_types.h | 10 ++++++++++
> >>> >  2 files changed, 22 insertions(+)
> >>> >
> >>> > diff --git a/drivers/gpu/drm/xe/xe_force_wake.h b/drivers/gpu/drm/
> >>> xe/xe_force_wake.h
> >>> > index 0e3e84bfa51c..e46e44f7eb70 100644
> >>> > --- a/drivers/gpu/drm/xe/xe_force_wake.h
> >>> > +++ b/drivers/gpu/drm/xe/xe_force_wake.h
> >>> > @@ -6,6 +6,8 @@
> >>> >  #ifndef _XE_FORCE_WAKE_H_
> >>> >  #define _XE_FORCE_WAKE_H_
> >>> >
> >>> > +#include <linux/cleanup.h>
> >>> > +
> >>> >  #include "xe_assert.h"
> >>> >  #include "xe_force_wake_types.h"
> >>> >
> >>> > @@ -61,4 +63,14 @@ xe_force_wake_ref_has_domain(unsigned int
> >>> fw_ref, enum xe_force_wake_domains dom
> >>> >      return fw_ref & domain;
> >>> >  }
> >>> >
> >>> > +DEFINE_CLASS(xe_fw, struct xe_force_wake_guard,
> >>> > +         xe_force_wake_put(_T.fw, _T.ref),
> >>> > +         ({ (struct xe_force_wake_guard){ fw,
> >>> xe_force_wake_get(fw, domains) }; }),
> >>> > +         struct xe_force_wake *fw, enum xe_force_wake_domains
> >>> domains);
> >>> > +
> >>> > +static inline void *class_xe_fw_lock_ptr(class_xe_fw_t *_T)
> >>> > +{
> >>> > +    return _T->ref ? _T : NULL;
> >>> > +}
> >>> > +
> >>> >  #endif
> >>> > diff --git a/drivers/gpu/drm/xe/xe_force_wake_types.h b/drivers/
> >>> gpu/drm/xe/xe_force_wake_types.h
> >>> > index 899fbbcb3ea9..15456a24f707 100644
> >>> > --- a/drivers/gpu/drm/xe/xe_force_wake_types.h
> >>> > +++ b/drivers/gpu/drm/xe/xe_force_wake_types.h
> >>> > @@ -85,4 +85,14 @@ struct xe_force_wake {
> >>> >      struct xe_force_wake_domain domains[XE_FW_DOMAIN_ID_COUNT];
> >>> >  };
> >>> >
> >>> > +/**
> >>> > + * struct xe_force_wake_guard - Xe force wake guard
> >>> > + */
> >>> > +struct xe_force_wake_guard {
> >>> > +    /** @fw: force wake manager */
> >>> > +    struct xe_force_wake *fw;
> >>> > +    /** @ref: opaque reference to woken domains */
> >>> > +    unsigned int ref;
> >>> > +};
> >>> > +
> >>> >  #endif
> >>> > --
> >>> > 2.43.0
> >>> >
>