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

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

On 8/17/2021 3:13 PM, Quan, Evan wrote:
> [AMD Official Use Only]
> 
> +Leo to share his insights
> 
>> -----Original Message-----
>> From: Lazar, Lijo <Lijo.Lazar@amd.com>
>> Sent: Tuesday, August 17, 2021 3:28 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Chen, Guchun
>> <Guchun.Chen@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>
>> Subject: Re: [PATCH] drm/amdgpu: properly powergate Polaris12 UVD/VCE
>> on suspend
>>
>>
>>
>> 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?
> [Quan, Evan] The reason we chose flush_delayed_work instead of cancel_delayed_work_sync is we think those operations performed in idle_work(dpm disablement, powergating) seems needed considering the action is 'suspend'. So, instead of "cancel", maybe waiting for them completion is more proper.

But it will do so only if the work is scheduled - so it doesn't seem to 
be a prerequisite for suspend. If it was a prerequisite, then the 
existing code is missing that (so that it gets done for all cases).

>>
>> Then this may be a potential issue for other suspend calls also where work is
>> pending to be launched when hardware is suspended.
> [Quan, Evan] Do you mean we need to check whether there is similar issue for other IPs?
> 

Yes, if there are cases where other IPs may schedule a delayed work and 
call hw_fini without cancelling the work.

Thanks,
Lijo

> BR
> Evan
>>
>> 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;
>>>