Re: [PATCH 01/10] drm/i915/display: convert inner wakeref get towards get_if_in_use

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 06, 2024 at 07:15:45PM -0500, Rodrigo Vivi wrote:
> This patch brings no functional change. Since at this point of
> the code we are already asserting a wakeref was held, it means
> that we are with runtime_pm 'in_use' and in practical terms we
> are only bumping the pm_runtime usage counter and moving on.
> 
> However, xe driver has a lockdep annotation that warned us that
> if a sync resume was actually called at this point, we could have
> a deadlock because we are inside the power_domains->lock locked
> area and the resume would call the irq_reset, which would also
> try to get the power_domains->lock.
> 
> For this reason, let's convert this call to a safer option and
> calm lockdep on.
> 
> Cc: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display_power.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 6fd4fa52253a..4c5168a5bbf4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -646,7 +646,7 @@ release_async_put_domains(struct i915_power_domains *power_domains,
>  	 * power well disabling.
>  	 */
>  	assert_rpm_raw_wakeref_held(rpm);
> -	wakeref = intel_runtime_pm_get(rpm);
> +	wakeref = intel_runtime_pm_get_if_in_use(rpm);

On first glance that sequence looks like complete nonsense, and
thus likely to be cleaned up by someone later.

To me _noresume() would seem like the more sensible thing to use
here. And even that might still warrant a comment to explain
why that one is used specifically.

I'm also confused by the wakeref vs. wakelock stuff in the runtime pm
code. Is that there just because not all places track the wakerefs?
Do we still have those left?

>  
>  	for_each_power_domain(domain, mask) {
>  		/* Clear before put, so put's sanity check is happy. */
> -- 
> 2.43.2

-- 
Ville Syrjälä
Intel