Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE on suspend

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 8/17/2021 12:10 PM, Evan Quan wrote:
> If the powergating of UVD/VCE is in process, wait for its completion
> before proceeding(suspending). This can fix some hangs observed on
> suspending when UVD/VCE still using(e.g. issue "pm-suspend" when video
> is still playing).
> 
> Change-Id: I36f39d9731e0a9638b52d5d92558b0ee9c23a9ed
> Signed-off-by: Evan Quan <evan.quan@amd.com>
> Signed-off-by: xinhui pan <xinhui.pan@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c | 5 +++++
>   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 5 +++++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> index 4eebf973a065..2fdce572baeb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v6_0.c
> @@ -554,6 +554,11 @@ static int uvd_v6_0_suspend(void *handle)
>   	int r;
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> +	/*
> +	 * If the powergating is in process, wait for its completion.
> +	 */
> +	flush_delayed_work(&adev->uvd.idle_work);
> +
If running idle is a prerequisite before going to suspend, then 
something else is missing here.

Otherwise, the hang looks more like a pending work launched after 
hardware is suspended and trying to access hardware. As the hardware is 
going to be suspended anyway, doesn't it work with 
cancel_delayed_work_sync - making sure that nothing is going to be 
launched later to access hardware?

Then this may be a potential issue for other suspend calls also where 
work is pending to be launched when hardware is suspended.

Thanks,
Lijo

>   	r = uvd_v6_0_hw_fini(adev);
>   	if (r)
>   		return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> index 6d9108fa22e0..f0adecd5ec0b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> @@ -503,6 +503,11 @@ static int vce_v3_0_suspend(void *handle)
>   	int r;
>   	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>   
> +	/*
> +	 * If the powergating is in process, wait for its completion.
> +	 */
> +	flush_delayed_work(&adev->vce.idle_work);
> +
>   	r = vce_v3_0_hw_fini(adev);
>   	if (r)
>   		return r;
>