[PATCH] drm: send vblank event with the attached sequence rather than current

[PATCH] drm: send vblank event with the attached sequence rather than current

[Mark Yacoub]

From: Mark Yacoub <markyacoub@google.com>

[Why]
drm_handle_vblank_events loops over vblank_event_list to send any event
that is current or has passed.
More than 1 event could be pending with past sequence time that need to
be send. This can be a side effect of drivers without hardware vblank
counter and they depend on the difference in the timestamps and the
frame/field duration calculated in drm_update_vblank_count. This can
lead to 1 vblirq being ignored due to very small diff, resulting in a
subsequent vblank with 2 pending vblank events to be sent, each with a
unique sequence expected by user space.

[How]
Send each pending vblank event with the sequence it's waiting on instead
of assigning the current sequence to all of them.

Fixes igt@kms_flip "Unexpected frame sequence"
Tested on Jacuzzi (MT8183)

Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
---
 drivers/gpu/drm/drm_vblank.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 3417e1ac79185..47da8056abc14 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -1902,7 +1902,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
-		send_vblank_event(dev, e, seq, now);
+		send_vblank_event(dev, e, e->sequence, now);
 	}
 
 	if (crtc && crtc->funcs->get_vblank_timestamp)
-- 
2.34.0.rc2.393.gf8c9666880-goog

Re: [PATCH] drm: send vblank event with the attached sequence rather than current

[Ville Syrjälä]

On Thu, Dec 02, 2021 at 10:11:55AM -0500, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@google.com>
> 
> [Why]
> drm_handle_vblank_events loops over vblank_event_list to send any event
> that is current or has passed.
> More than 1 event could be pending with past sequence time that need to
> be send. This can be a side effect of drivers without hardware vblank
> counter and they depend on the difference in the timestamps and the
> frame/field duration calculated in drm_update_vblank_count. This can
> lead to 1 vblirq being ignored due to very small diff, resulting in a
> subsequent vblank with 2 pending vblank events to be sent, each with a
> unique sequence expected by user space.
> 
> [How]
> Send each pending vblank event with the sequence it's waiting on instead
> of assigning the current sequence to all of them.
> 
> Fixes igt@kms_flip "Unexpected frame sequence"
> Tested on Jacuzzi (MT8183)
> 
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
>  drivers/gpu/drm/drm_vblank.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 3417e1ac79185..47da8056abc14 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1902,7 +1902,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>  
>  		list_del(&e->base.link);
>  		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, now);
> +		send_vblank_event(dev, e, e->sequence, now);

This doesn't look right. The timestamp corresponds to 'seq' not
e->sequence (ie. whatever sequqnece number the user asked to wait
for).

>  	}
>  
>  	if (crtc && crtc->funcs->get_vblank_timestamp)
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog

-- 
Ville Syrjälä
Intel

Re: [PATCH] drm: send vblank event with the attached sequence rather than current

[Matthias Brugger]

Hi Mark,

On 02/12/2021 16:11, Mark Yacoub wrote:
> From: Mark Yacoub <markyacoub@google.com>
> 

please make sure to add the linux-mediatek mailinglist in any follow-up 
communication.

Regards,
Matthias

> [Why]
> drm_handle_vblank_events loops over vblank_event_list to send any event
> that is current or has passed.
> More than 1 event could be pending with past sequence time that need to
> be send. This can be a side effect of drivers without hardware vblank
> counter and they depend on the difference in the timestamps and the
> frame/field duration calculated in drm_update_vblank_count. This can
> lead to 1 vblirq being ignored due to very small diff, resulting in a
> subsequent vblank with 2 pending vblank events to be sent, each with a
> unique sequence expected by user space.
> 
> [How]
> Send each pending vblank event with the sequence it's waiting on instead
> of assigning the current sequence to all of them.
> 
> Fixes igt@kms_flip "Unexpected frame sequence"
> Tested on Jacuzzi (MT8183)
> 
> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> ---
>   drivers/gpu/drm/drm_vblank.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 3417e1ac79185..47da8056abc14 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -1902,7 +1902,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>   
>   		list_del(&e->base.link);
>   		drm_vblank_put(dev, pipe);
> -		send_vblank_event(dev, e, seq, now);
> +		send_vblank_event(dev, e, e->sequence, now);
>   	}
>   
>   	if (crtc && crtc->funcs->get_vblank_timestamp)
> 

Re: [PATCH] drm: send vblank event with the attached sequence rather than current

[Michel Dänzer]

On 2021-12-03 16:58, Matthias Brugger wrote:
> Hi Mark,
> 
> On 02/12/2021 16:11, Mark Yacoub wrote:
>> From: Mark Yacoub <markyacoub@google.com>
>>
> 
> please make sure to add the linux-mediatek mailinglist in any follow-up communication.
> 
> Regards,
> Matthias
> 
>> [Why]
>> drm_handle_vblank_events loops over vblank_event_list to send any event
>> that is current or has passed.
>> More than 1 event could be pending with past sequence time that need to
>> be send. This can be a side effect of drivers without hardware vblank
>> counter and they depend on the difference in the timestamps and the
>> frame/field duration calculated in drm_update_vblank_count. This can
>> lead to 1 vblirq being ignored due to very small diff,

That sounds like the real issue which needs to be addressed. As Ville wrote, the sequence value in the event is supposed to be the current sequence, not the one which was specified originally by user space. So this change would basically break the universe. ;)


>> resulting in a subsequent vblank with 2 pending vblank events to be sent, each with a
>> unique sequence expected by user space.
>>
>> [How]
>> Send each pending vblank event with the sequence it's waiting on instead
>> of assigning the current sequence to all of them.
>>
>> Fixes igt@kms_flip "Unexpected frame sequence"
>> Tested on Jacuzzi (MT8183)
>>
>> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
>> ---
>>   drivers/gpu/drm/drm_vblank.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
>> index 3417e1ac79185..47da8056abc14 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -1902,7 +1902,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
>>             list_del(&e->base.link);
>>           drm_vblank_put(dev, pipe);
>> -        send_vblank_event(dev, e, seq, now);
>> +        send_vblank_event(dev, e, e->sequence, now);
>>       }
>>         if (crtc && crtc->funcs->get_vblank_timestamp)
>>



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

Re: [PATCH] drm: send vblank event with the attached sequence rather than current

[Mark Yacoub]

On Fri, Dec 3, 2021 at 1:03 PM Michel Dänzer <michel.daenzer@mailbox.org> wrote:
>
> On 2021-12-03 16:58, Matthias Brugger wrote:
> > Hi Mark,
> >
> > On 02/12/2021 16:11, Mark Yacoub wrote:
> >> From: Mark Yacoub <markyacoub@google.com>
> >>
> >
> > please make sure to add the linux-mediatek mailinglist in any follow-up communication.
> >
> > Regards,
> > Matthias
> >
> >> [Why]
> >> drm_handle_vblank_events loops over vblank_event_list to send any event
> >> that is current or has passed.
> >> More than 1 event could be pending with past sequence time that need to
> >> be send. This can be a side effect of drivers without hardware vblank
> >> counter and they depend on the difference in the timestamps and the
> >> frame/field duration calculated in drm_update_vblank_count. This can
> >> lead to 1 vblirq being ignored due to very small diff,
>
> That sounds like the real issue which needs to be addressed. As Ville wrote, the sequence value in the event is supposed to be the current sequence, not the one which was specified originally by user space. So this change would basically break the universe. ;)
>
I don't wanna be the reason to break the universe :)) I guess I
misunderstood what the call does, will look into the real issue more.
Thanks everyone!
>
> >> resulting in a subsequent vblank with 2 pending vblank events to be sent, each with a
> >> unique sequence expected by user space.
> >>
> >> [How]
> >> Send each pending vblank event with the sequence it's waiting on instead
> >> of assigning the current sequence to all of them.
> >>
> >> Fixes igt@kms_flip "Unexpected frame sequence"
> >> Tested on Jacuzzi (MT8183)
> >>
> >> Signed-off-by: Mark Yacoub <markyacoub@chromium.org>
> >> ---
> >>   drivers/gpu/drm/drm_vblank.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> >> index 3417e1ac79185..47da8056abc14 100644
> >> --- a/drivers/gpu/drm/drm_vblank.c
> >> +++ b/drivers/gpu/drm/drm_vblank.c
> >> @@ -1902,7 +1902,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
> >>             list_del(&e->base.link);
> >>           drm_vblank_put(dev, pipe);
> >> -        send_vblank_event(dev, e, seq, now);
> >> +        send_vblank_event(dev, e, e->sequence, now);
> >>       }
> >>         if (crtc && crtc->funcs->get_vblank_timestamp)
> >>
>
>
>
> --
> Earthling Michel Dänzer            |                  https://redhat.com
> Libre software enthusiast          |         Mesa and Xwayland developer