Re: [RFC 0/3] FW guard class

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

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

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