Re: [PATCH] drm/amd/display: Fix error handling on waiting for completion

["Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>]

On 2021-10-26 11:51 a.m., Michel Dänzer wrote:
> On 2021-10-26 13:07, Stylon Wang wrote:
>> [Why]
>> In GNOME Settings->Display the switching from mirror mode to single display
>> occasionally causes wait_for_completion_interruptible_timeout() to return
>> -ERESTARTSYS and fails atomic check.
>>
>> [How]
>> Replace the call with wait_for_completion_timeout() since the waiting for
>> hw_done and flip_done completion doesn't need to worry about interruption
>> from signal.
>>
>> Signed-off-by: Stylon Wang <stylon.wang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 4cd64529b180..b8f4ff323de1 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -9844,10 +9844,10 @@ static int do_aquire_global_lock(struct drm_device *dev,
>>   		 * Make sure all pending HW programming completed and
>>   		 * page flips done
>>   		 */
>> -		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
>> +		ret = wait_for_completion_timeout(&commit->hw_done, 10*HZ);
>>   
>>   		if (ret > 0)
>> -			ret = wait_for_completion_interruptible_timeout(
>> +			ret = wait_for_completion_timeout(
>>   					&commit->flip_done, 10*HZ);
>>   
>>   		if (ret == 0)
>>
> 
> The *_interruptible_* variant is needed so that the display manager process can be killed while it's waiting here, which could take up to 10 seconds (per the timeout).
> 
> What's the problem with -ERESTARTSYS? Either the ioctl should be restarted automatically, or if it bounces back to user space, that needs to be able to retry the ioctl while it returns -1 and errno == EINTR. drmIoctl handles this transparently.
> 
> 

Thanks for the insight Michel!

If it's just an error in the log without a functional issue then maybe 
we should downgrade it to a debug statement in the case where it returns 
-ERESTARTSYS.

If this is a functional issue (DRM not automatically retrying the 
commit?) then maybe we should take a deeper look into the IOCTL itself.

Regards,
Nicholas Kazlauskas