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

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

[Stylon Wang]

[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)
-- 
2.33.0

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

[Kazlauskas, Nicholas]

On 2021-10-26 7:07 a.m., 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>

I think this is okay, but I'll write out how I think these work here in 
case anyone has corrections.

Both variants allow the thread to sleep, but the interruptible variant 
can waken due to signals. These signals are a secondary wakeup event and 
would require use to restart the wait and (probably) keep track of how 
long we were waiting before.

We want wakeup only on completion, so we should be using the 
`wait_for_completion_timeout()` variants instead in most (if not all?) 
cases in our display driver.

This probably has some nuances that matter more for different variants 
of UAPI, but with this understanding I think this is:

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Now, if we could revive that patch series I had from the other year and 
outright drop `do_aquire_global_lock()`...

Regards,
Nicholas Kazlauskas

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

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

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

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

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

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

[Simon Ser]

On Tuesday, October 26th, 2021 at 18:03, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:

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

libdrm handles EINTR but not ERESTARTSYS. So, if the kernel returns ERESTARTSYS
then Mutter will error out.

Should the kernel be fixed to return EINTR instead of ERESTARTSYS, or should
libdrm be fixed to handle ERESTARTSYS?

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

[Michel Dänzer]

On 2021-10-26 18:08, Simon Ser wrote:
> On Tuesday, October 26th, 2021 at 18:03, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:
> 
>> 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.
> 
> libdrm handles EINTR but not ERESTARTSYS. So, if the kernel returns ERESTARTSYS
> then Mutter will error out.
> 
> Should the kernel be fixed to return EINTR instead of ERESTARTSYS, or should
> libdrm be fixed to handle ERESTARTSYS?

The kernel is supposed to convert ERESTARTSYS to EINTR when returning to user space (when the kernel can't restart the system call itself), so user space doesn't need to handle the former.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [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@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
>

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

[Wang, Chao-kai (Stylon)]

[-- Attachment #1: Type: text/plain, Size: 3854 bytes --]

[AMD Official Use Only]

Hi Michel,

The problem with -ERESTARTSYS is the same half-baked atomic state with modifications we made in the interrupted atomic check, is reused in the next retry and fails the atomic check. What we expect in the next retry is with the original atomic state. I am going to dig deeper and see if at DRM side we can go back to use to the original atomic state in the retry.


Regards

Stylon Wang

MTS Software Development Eng.  |  AMD
Display Solution Team

O +(886) 2-3789-3667 ext. 23667  C +(886) 921-897-142

----------------------------------------------------------------------------------------------------------------------------------

6F, 3, YuanCyu St (NanKang Software Park) Taipei, Taiwan

Facebook<https://www.facebook.com/AMD> |  Twitter<https://twitter.com/AMD> |  amd.com<http://www.amd.com/>



________________________________
From: Michel Dänzer <michel@daenzer.net>
Sent: October 26, 2021 11:51 PM
To: Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Wentland, Harry <Harry.Wentland@amd.com>; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; contact@emersion.fr <contact@emersion.fr>; Kazlauskas, Nicholas <Nicholas.Kazlauskas@amd.com>
Subject: Re: [PATCH] drm/amd/display: Fix error handling on waiting for completion

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.


--
Earthling Michel Dänzer            |                  https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fredhat.com%2F&amp;data=04%7C01%7Cstylon.wang%40amd.com%7C251ee7aba8574015713a08d998986a5f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637708602663589383%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=LcSFyj%2FJ9VYbNfxJQRjpiRAcurbzTbD5yUVysxzpmXs%3D&amp;reserved=0
Libre software enthusiast          |         Mesa and Xwayland developer

[-- Attachment #2: Type: text/html, Size: 10318 bytes --]

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

[Michel Dänzer]

[ Adding dri-devel ]

On 2021-11-01 16:00, Wang, Chao-kai (Stylon) wrote:
> 
> The problem with -ERESTARTSYS is the same half-baked atomic state with modifications we made in the interrupted atomic check, is reused in the next retry and fails the atomic check. What we expect in the next retry is with the original atomic state. I am going to dig deeper and see if at DRM side we can go back to use to the original atomic state in the retry.

I suspect either DC/DM needs to be able to handle the modified state on retry, or it needs to restore the original state before it returns -ERESTARTSYS.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

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

[Michel Dänzer]

[ Adding dri-devel ]

On 2021-11-01 16:00, Wang, Chao-kai (Stylon) wrote:
> 
> The problem with -ERESTARTSYS is the same half-baked atomic state with modifications we made in the interrupted atomic check, is reused in the next retry and fails the atomic check. What we expect in the next retry is with the original atomic state. I am going to dig deeper and see if at DRM side we can go back to use to the original atomic state in the retry.

I suspect either DC/DM needs to be able to handle the modified state on retry, or it needs to restore the original state before it returns -ERESTARTSYS.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer