Re: [PATCH v2 05/30] drm/xe/pm: Add scope-based cleanup helper for runtime PM

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Michal Wajdeczko (2025-11-12 16:53:46-03:00)
>
>
>On 11/11/2025 12:20 AM, Matt Roper wrote:
>> Add a scope-based helpers for runtime PM that may be used to simplify
>> cleanup logic and potentially avoid goto-based cleanup.
>> 
>> For example, using
>> 
>>         guard(xe_pm_runtime)(xe);
>> 
>
>for the record:
>
>last year [1] use of DEFINE_GUARD for our RPM was considered almost as API abuse ;)
>but now [2] [3] it is used exactly for such scenarios
>
>[1] https://patchwork.freedesktop.org/patch/599366/?series=134955&rev=1
>[2] https://elixir.bootlin.com/linux/v6.18-rc5/source/include/linux/pm_runtime.h#L617
>[3] https://elixir.bootlin.com/linux/v6.18-rc5/source/drivers/tty/serial/8250/8250.h#L191

If something breaks in the future, we are not the only ones to be
blamed? :-)

Maybe we should ask for an ack from people maintaining
include/linux/cleanup.h?

>
>> will get runtime PM and cause a corresponding put to occur automatically
>> when the current scope is exited.  'xe_pm_runtime_noresume' can be used
>> as a guard replacement for the corresponding 'noresume' variant.
>> There's also an xe_pm_runtime_ioctl conditional guard that can be used
>> as a replacement for xe_runtime_ioctl():
>> 
>>         ACQUIRE(xe_pm_runtime_ioctl, pm)(xe);
>>         if ((ret = ACQUIRE_ERR(xe_pm_runtime_ioctl, &pm)) < 0)
>>                 /* failed */
>> 
>> In a few rare cases (such as gt_reset_worker()) we need to ensure that
>> runtime PM is dropped when the function is exited by any means
>> (including error paths), but the function does not need to acquire
>> runtime PM because that has already been done earlier by a different
>> function.  For these special cases, an 'xe_pm_runtime_release_only'
>> guard can be used to handle the release without doing an acquisition.
>> 
>> These guards will be used in future patches to eliminate some of our
>> goto-based cleanup.
>> 
>> v2:
>>  - Specify success condition for xe_pm runtime_ioctl as _RET >= 0 so
>>    that positive values will be properly identified as success and
>>    trigger destructor cleanup properly.
>> 
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_pm.h | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>> index f7f89a18b6fc..6b27039e7b2d 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.h
>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>> @@ -6,6 +6,7 @@
>>  #ifndef _XE_PM_H_
>>  #define _XE_PM_H_
>>  
>> +#include <linux/cleanup.h>
>>  #include <linux/pm_runtime.h>
>>  
>>  #define DEFAULT_VRAM_THRESHOLD 300 /* in MB */
>> @@ -37,4 +38,20 @@ int xe_pm_block_on_suspend(struct xe_device *xe);
>>  void xe_pm_might_block_on_suspend(void);
>>  int xe_pm_module_init(void);
>>  
>> +static inline void __xe_pm_runtime_noop(struct xe_device *xe) {}
>> +
>> +DEFINE_GUARD(xe_pm_runtime, struct xe_device *,
>> +             xe_pm_runtime_get(_T), xe_pm_runtime_put(_T))
>> +DEFINE_GUARD(xe_pm_runtime_noresume, struct xe_device *,
>> +             xe_pm_runtime_get_noresume(_T), xe_pm_runtime_put(_T))
>> +DEFINE_GUARD_COND(xe_pm_runtime, _ioctl, xe_pm_runtime_get_ioctl(_T), _RET >= 0)

It would also be good to document about these in the kerneldoc for the
get variants.

--
Gustavo Sousa

>> +
>> +/*
>> + * Used when a function needs to release runtime PM in all possible cases
>> + * and error paths, but the wakeref was already acquired by a different
>> + * function (i.e., get() has already happened so only a put() is needed).
>> + */
>> +DEFINE_GUARD(xe_pm_runtime_release_only, struct xe_device *,
>> +             __xe_pm_runtime_noop(_T), xe_pm_runtime_put(_T));
>
>maybe instead defining the noop() helper, we can just skip this param:
>
>DEFINE_GUARD(xe_pm_runtime_release_only, struct xe_device *, , xe_pm_runtime_put(_T));
>
>> +
>>  #endif
>
>either way,
>
>Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>