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

[Christian König]

Am 26.10.21 um 18:03 schrieb Kazlauskas, Nicholas:
> 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 at 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.

Yeah, that is a very common problem. -ERESTARTSYS should never be 
logged, not even on debug level, since it is part of normal operation.

Regards,
Christian.

>
> 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
>