[PATCH v2 2/2] drm/xe: Use new force-wake guard class in xe_mocs.c

[Lucas De Marchi]

On Wed, Nov 20, 2024 at 08:55:06PM -0800, Matthew Brost wrote:
>On Tue, Nov 19, 2024 at 02:03:12PM -0600, Lucas De Marchi wrote:
>> On Mon, Nov 18, 2024 at 07:45:11PM +0100, Michal Wajdeczko wrote:
>> > Start using new xe_fw class to minimize risk of leaking force-wake
>> > references.
>> >
>> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> > ---
>> > v2: rebased
>> > ---
>> > drivers/gpu/drm/xe/xe_mocs.c | 10 +++-------
>> > 1 file changed, 3 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_mocs.c b/drivers/gpu/drm/xe/xe_mocs.c
>> > index 54d199b5cfb2..f18aaa6595c7 100644
>> > --- a/drivers/gpu/drm/xe/xe_mocs.c
>> > +++ b/drivers/gpu/drm/xe/xe_mocs.c
>> > @@ -776,19 +776,15 @@ void xe_mocs_dump(struct xe_gt *gt, struct drm_printer *p)
>> > {
>> > 	struct xe_device *xe = gt_to_xe(gt);
>> > 	struct xe_mocs_info table;
>> > -	unsigned int fw_ref, flags;
>> > +	unsigned int flags;
>> >
>> > 	flags = get_mocs_settings(xe, &table);
>> >
>> > 	xe_pm_runtime_get_noresume(xe);
>> > -	fw_ref = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
>> > -	if (!fw_ref)
>> > -		goto err_fw;
>> >
>> > -	table.ops->dump(&table, flags, gt, p);
>> > +	scoped_guard(xe_fw, gt_to_fw(gt), XE_FW_GT)
>> > +		table.ops->dump(&table, flags, gt, p);
>>
>>
>> afair one caveat is that you can't have a goto that jumps over guard
>> because of the usage of the cleanup attribute. Is that still the case or
>> do we have some magic convincing the compiler to do the right thing?
>>
>> once we get agreement on the approach, please let's convert everything
>> that can be converted to scoped guard so we don't have to keep both
>> approaches for a long time.
>>
>
>+1. I've come around to using (scoped) guard but if we commit to this I
>do think we should try to overhaul the driver to use this more or less
>everywhere it makes sense (e.g. fw, spin locks, mutexes, perhaps some pm
>refs where the ref is only held within scope, also maybe also tmp object
>refs?). Having a single sematic IMO makes it easier to quickly understand
>different code paths. Maybe we even put the agreement in some kernel doc
>too.
>
>Can stage this a piece at time too, e.g., A series which converts spin
>locks to scoped guard, etc...
>
>Anything simple which also doesn't fix into patterns handled by scoped
>guard we could also review if the complexity is required and if it is
>flag for comments so we can understand why it is needed. I know for sure
>I've added odd coding patterns which are required, don't have a comment
>why I did it, only to forget why later which is not great for long term
>maintainability.
>
>Obviously complex locking patterns, e.g., the VM lock's + drm exec, will
>still need to be open coded /w unwinds but that locking itself is or
>should be documented too.

Ack to get started with the xe_fw and then we can provide separate
series to convert the others.

Lucas De Marchi