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

[Matthew Brost]

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.

Matt

> Lucas De Marchi
> 
> > 
> > -	xe_force_wake_put(gt_to_fw(gt), fw_ref);
> > -err_fw:
> > 	xe_pm_runtime_put(xe);
> > }
> > 
> > -- 
> > 2.43.0
> >