Re: [PATCH] drm/xe/pm: Also avoid missing outer rpm warning on system suspend

[Imre Deak <imre.deak@intel.com>]

On Tue, Dec 17, 2024 at 06:05:47PM -0500, Rodrigo Vivi wrote:
> We have some cases where display is releasing power domains at
> release_async_put_domains() where intel_runtime_pm_get_noresume()
> is called, but no outer protection. In Xe this will trigger our
> traditional warning.

I suppose by outer protection you mean an RPM reference that is
guaranteed to be held at the point (that is right before)
release_async_put_domains() calls intel_runtime_pm_get_noresume(). This
is guaranteed, i.e. such an RPM reference is held by definition (by the
power domain reference that is being put).

Instead, the actual reason for triggering the warn - IIUC - is that
intel_runtime_pm_get_if_in_use() called from
xe_pm_runtime_get_noresume() (probably for the exact reason to check if
an outer RPM is held) fails if it is called while system suspending /
resuming. This is the same scenario as when
intel_runtime_pm_get_if_in_use() would fail if called during runtime
suspending / resuming and - worked around earlier I assume - by
suppressing the warning in this case using xe_pm_suspending_or_resuming().

So in this fix the above workaround to suppress the warning is just
extended to the system suspend/resume case.

> However, this case should be safe because it is triggered from the
> system suspend path, where we certainly won't be transitioning to rpm
> suspend.
> 
> This wouldn't happen if the display pm sequences, including
> all irq flow was in sync between i915 and xe. So, while we
> don't get there, let's not raise warnings when we are in this
> system suspend path.

I think the issue fixed in this patch is just a consequence of how the
outer RPM check works using xe_pm_suspending_or_resuming() and wouldn't
change even after the IRQ related issues are fixed.

> Suggested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

With the above understanding:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_pm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index a6761cb769b2..c6e57af0144c 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/fault-inject.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/suspend.h>
>  
>  #include <drm/drm_managed.h>
>  #include <drm/ttm/ttm_placement.h>
> @@ -607,7 +608,8 @@ static bool xe_pm_suspending_or_resuming(struct xe_device *xe)
>  	struct device *dev = xe->drm.dev;
>  
>  	return dev->power.runtime_status == RPM_SUSPENDING ||
> -		dev->power.runtime_status == RPM_RESUMING;
> +		dev->power.runtime_status == RPM_RESUMING ||
> +		pm_suspend_target_state != PM_SUSPEND_ON;
>  #else
>  	return false;
>  #endif
> -- 
> 2.47.1
>