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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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?

> 
> 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
>>> >