Re: [RFC 0/3] FW guard class

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Jun 17, 2024 at 02:00:55PM -0400, Rodrigo Vivi wrote:
> On Mon, Jun 17, 2024 at 05:24:24PM +0000, Matthew Brost wrote:
> > On Mon, Jun 17, 2024 at 04:34:27PM +0200, Michal Wajdeczko wrote:
> > > There is support for 'classes' with constructor and destructor
> > > semantics that can be used for any scope-based resource management,
> > > like device force-wake management.
> > > 
> > > Add necessary definitions explicitly, since existing macros from
> > > linux/cleanup.h can't deal with our specific requirements yet.
> > > 
> > > This should allow us to use:
> > > 
> > > 	scoped_guard(xe_fw, fw, XE_FW_GT)
> > > 		foo();
> > > or
> > > 	CLASS(xe_fw, var)(fw, XE_FW_GT);
> > > 
> > > without any concern of leaking the force-wake references.
> > > 
> > > Note: this is preliminary code as right now it's unclear how to
> > > correctly handle errors from the force-wake functions.
> > > 
> > 
> > I'm personally don't like this at all. IMO it obfuscate the code with
> > little real benefit. This is just an opinion though, others opinions may
> > differ from mine.
> 
> Well, on the positive side, it is not adding a driver only thing like
> i915's with_runtime_pm() macro.
> 
> But I'm also not sure if I like the overall idea anyway:
> 
> - I don't like adding C++isms in a pure C code. Specially something not
> so standard and common that will decrease the ramp-up time for newcomers.
> - It looks like and extra overhead on the object creation destruction.
> - It looks not flexible for handling different cases... like forcewake for
> instance where we might want to ignore the ack timeout in some cases.

Thanks for writing this out. I agree 100% on all your points.

Matt

> 
> > 
> > Matt
> > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > 
> > > Michal Wajdeczko (3):
> > >   drm/xe: Introduce force-wake guard class
> > >   drm/xe: Use new FW guard in xe_mocs.c
> > >   drm/xe: Use new FW guard in xe_pat.c
> > > 
> > >  drivers/gpu/drm/xe/xe_force_wake.h       | 48 +++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_force_wake_types.h | 12 +++++
> > >  drivers/gpu/drm/xe/xe_mocs.c             | 12 +----
> > >  drivers/gpu/drm/xe/xe_pat.c              | 60 ++++++++----------------
> > >  4 files changed, 82 insertions(+), 50 deletions(-)
> > > 
> > > -- 
> > > 2.43.0
> > >