Re: [PATCH-resent-to-correct-ml 3/8] drm/xe: Add scoped guards for xe_force_wake

Maarten Lankhorst
2025-02-04T22:28:05Z

Hey,

On 2025-02-04 17:30, Michal Wajdeczko wrote:
> Hi Maarten,
>
> On 04.02.2025 14:22, Maarten Lankhorst wrote:
>> Instead of finding bugs where we may or may not release force_wake, I've
>> decided to be inspired by the spinlock guards, and use the same ones to
>> do xe_force_wake handling.
>
> You may want to take a look at [1], which was based on [2], that
> introduce fw guard class (and it was already acked and reviewed).
> Merging was postponed only due to a request to prepare larger series
> that would convert all existing usages to the new model.
>
> And similar guard approach for our RPM was proposed in [3]
>
> Michal
>
> [1] https://patchwork.freedesktop.org/series/141516/
> [2] https://patchwork.freedesktop.org/series/134958/
> [3] https://patchwork.freedesktop.org/series/134955/

Excellent. I'm glad we're in agreement that doing forcewake handling in
guard handlers is a good thing. :-)

I have taken a look at the patch series. I think the approach I've taken
is a refinement of your series. Yours is already nearly there, but it
still keeps the rough edges of the original API.

To smooth them, I have created 2 constructors, xe_force_wake, and
xe_force_wake_get. The former is used if you want to run code regardless
whether it succeeds, the latter is when you do.

This allows code like:
scoped_cond_guard(xe_force_wake_get, return -ETIMEDOUT, fw,
XE_FORCE_WAKE_ALL) {}
to work flawlessly as intended, without having to check
xe_force_wake_ref_has_domain(XE_FORCE_WAKE_ALL);

I think this cleanup removes a nasty source of errors.

When you don't care about failure:
scoped_guard(xe_force_wake, fw, XE_FORCE_WAKE_ALL) {
if (!xe_force_wake_scope_has_domain(XE_FORCE_WAKE_ALL))
printk("Oh noez, anyway..\n");

/* Continue and pretend nothing happened */
}

And for optional code, same as scoped_cond_guard, but as scoped_guard:

scoped_guard(xe_force_wake_get, fw, XE_FORCE_WAKE_ALL) {
/* Only runs this block if acquire completely succeeded, otherwise use
xe_force_wake */
}

All in all, I'm open for bikesheds, but I think this has the potential
to improve xe_force_wake handling even further!

I wasn't aware of your previous attempt when I wrote this and fought
linux/cleanup.h, otherwise I would have taken that as a base and credit
your work.

Cheers,
~Maarten