Re: [RFC 0/3] FW guard class

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

On Mon, Jun 17, 2024 at 09:24:42PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 17.06.2024 20:00, 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.
> 
> except that is more robust than hand-crafted code that is error prone,
> like this snippet from wedged_mode_set():
> 
> 	xe_pm_runtime_get(xe);
> 	for_each_gt(gt, xe, id) {
> 		ret = xe_guc_ads(...);
> 		if (ret) {
> 			xe_gt_err(gt, "...");
> 			return -EIO;
> 		}
> 	}
> 	xe_pm_runtime_put(xe);
> 
> and thanks to PM guard class we could avoid such mistakes for free:
> 
> 	scoped_guard(xe_pm, xe) {
> 		for_each_gt(gt, xe, id) {
> 			ret = xe_guc_ads(...);
> 			if (ret) {
> 				xe_gt_err(gt, "...");
> 				return -EIO;

Just responding with a question here - haven't looked at the rest of the
comments.

How is this not still a bug? Looking at scoped_guard, it appears to be a
magic macro for loop which acquires / releases a lock or in your
purposed case a PM or FW ref. Doesn't the 'return -EIO' skip the release
step? I see coding patterns like above in the kernel [1] so I do assume
this works, just confused how it works.

With that, any code which isn't easily understandable IMO is a negative
ROI as it just creates confusion in the long / makes problems harder to
understand. Again this is just my opinion.

Matt

[1] https://elixir.bootlin.com/linux/latest/source/drivers/iio/imu/bmi323/bmi323_core.c#L1544

> 			}
> 		}
> 	}
> 
> > 
> > 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.
> 
> does it mean that the use of other guard patterns seen elsewhere in the
> tree is now prohibited on the Xe driver ? like:
> 
> 	scoped_guard(mutex, &lock)
> 		foo();
> 
> 	scoped_guard(spinlock, &lock)
> 		foo();
> 	...
> 
> > - It looks like and extra overhead on the object creation destruction.
> 
> from cleanup.h doc is sounds there is none:
> 
>  "And through the magic of value-propagation and dead-code-elimination,
> it eliminates the actual cleanup call and compiles into:"
> 
> 
> > - It looks not flexible for handling different cases... like forcewake for
> > instance where we might want to ignore the ack timeout in some cases.
> 
> there is scoped_cond_guard() that likely will be able to deal with it,
> but I guess we first need to cleanup existing force_wake api as expected
> flow is not clear and there are different approaches in the driver how
> to deal with errors
> 
> > 
> >>
> >> 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
> >>>