[Intel-xe] [PATCH 1/2] drm/xe/pm: Wrap pm_runtime_suspended with power.lock

[Matthew Brost]

On Fri, Jun 16, 2023 at 07:41:14PM +0530, Anshuman Gupta wrote:
> pm_runtime_suspended() doc has requirement to use it with
> PM lock of @dev to get the trusted value.
> Wrap pm_runtime_suspended within xe_pm_runtime_suspended()
> with proper locking.
> 
> Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_pm.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> index b7b57f10ba25..04c5353ba870 100644
> --- a/drivers/gpu/drm/xe/xe_pm.c
> +++ b/drivers/gpu/drm/xe/xe_pm.c
> @@ -210,11 +210,23 @@ int xe_pm_runtime_put(struct xe_device *xe)
>  	return pm_runtime_put_autosuspend(xe->drm.dev);
>  }
>  
> +static bool xe_pm_runtime_suspended(struct xe_device *xe)
> +{
> +	unsigned long flags;
> +	bool suspended;
> +
> +	spin_lock_irqsave(&xe->drm.dev->power.lock, flags);
> +	suspended = pm_runtime_suspended(xe->drm.dev);
> +	spin_unlock_irqrestore(&xe->drm.dev->power.lock, flags);

A few things.

First, you probably can make this spin_lock_irq / spin_unlock_irq
as IRQ contexts are not really used in Xe it is doubtful this function
would ever be called from an unknown context.

Second, I have questions if this patch is actually doing anything
useful. Yes, I can see doc which indicates that pm_runtime_suspended can
only be trusted if the lock is held but looking at the code paths where
xe_pm_runtime_suspended is called unsure how this lock helps.

e.g. How is this better?
if (trusted value)
	now the trusted value can change, do something
	
Than?
if (untrusted value)
	do something

Also if you look all over kernel pm_runtime_suspended is called
frequently dev->power.lock.

Matt

> +
> +	return suspended;
> +}
> +
>  /* Return true if resume operation happened and usage count was increased */
>  bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe)
>  {
>  	/* In case we are suspended we need to immediately wake up */
> -	if (pm_runtime_suspended(xe->drm.dev))
> +	if (xe_pm_runtime_suspended(xe))
>  		return !pm_runtime_resume_and_get(xe->drm.dev);
>  
>  	return false;
> @@ -222,6 +234,6 @@ bool xe_pm_runtime_resume_if_suspended(struct xe_device *xe)
>  
>  int xe_pm_runtime_get_if_active(struct xe_device *xe)
>  {
> -	WARN_ON(pm_runtime_suspended(xe->drm.dev));
> +	WARN_ON(xe_pm_runtime_suspended(xe));
>  	return pm_runtime_get_if_active(xe->drm.dev, true);
>  }
> -- 
> 2.38.0
>