Nouveau - Dec 2015 - [PATCH] drm/nouveau: Fix pre-nv50 pageflip events

On Mon, Nov 02, 2015 at 04:45:00PM +0900, Michel Dänzer
wrote:
> On 31.10.2015 06:55, Daniel Vetter wrote:
> > Apparently pre-nv50 pageflip events happen before the actual vblank
> > period. Therefore that functionality got semi-disabled in
> > 
> > commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28
> > Author: Mario Kleiner <mario.kleiner.de at gmail.com>
> > Date:   Tue May 13 00:42:08 2014 +0200
> > 
> >     drm/nouveau/kms/nv04-nv40: fix pageflip events via special case.
> > 
> > Unfortunately that hack got uprooted in
> > 
> > commit cc1ef118fc099295ae6aabbacc8af94d8d8885eb
> > Author: Thierry Reding <treding at nvidia.com>
> > Date:   Wed Aug 12 17:00:31 2015 +0200
> > 
> >     drm/irq: Make pipe unsigned and name consistent
> > 
> > Trigering a warning when trying to sample the vblank timestamp for a
> > non-existing pipe. There's a few ways to fix this:
> > 
> > - Open-code the old behaviour, which just enshrines this slight
> >   breakage of the userspace ABI.
> > 
> > - Revert Mario's commit and again inflict broken timestamps, again
not
> >   pretty.
> > 
> > - Fix this for real by delaying the pageflip TS until the next vblank
> >   interrupt, thereby making it accurate.
> > 
> > This patch implements the third option. Since having a page flip
> > interrupt that happens when the pageflip gets armed and not when it
> > completes in the next vblank seems to be fairly common (older i915 hw
> > works very similarly) create a new helper to arm vblank events for
> > such drivers.
> 
> What happens when the page flip interrupt arrives during a vertical
> blank period?  Presumably the userspace event will be deferred until the
> next vertical blank period, but the flip might already take effect in
> the current one.
Hm yeah there's a tiny race if your update handler for the pageflip can
race with your vblank handler. That's impossible here since it's all
done
from the same hw irq hanlder, and since that is single-threaded there
shouldn't be a problem, as long as vblank handling are pageflip are
ordered correctly.

Might be worth a note in the kerneldoc though that this function isn't
perfectly foolproof.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

On Mon, Nov 16, 2015 at 4:11 PM, Daniel Vetter <daniel at ffwll.ch>
wrote:
> On Mon, Nov 02, 2015 at 04:45:00PM +0900, Michel Dänzer wrote:
>> On 31.10.2015 06:55, Daniel Vetter wrote:
>> > Apparently pre-nv50 pageflip events happen before the actual
vblank
>> > period. Therefore that functionality got semi-disabled in
>> >
>> > commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28
>> > Author: Mario Kleiner <mario.kleiner.de at gmail.com>
>> > Date:   Tue May 13 00:42:08 2014 +0200
>> >
>> >     drm/nouveau/kms/nv04-nv40: fix pageflip events via special
case.
>> >
>> > Unfortunately that hack got uprooted in
>> >
>> > commit cc1ef118fc099295ae6aabbacc8af94d8d8885eb
>> > Author: Thierry Reding <treding at nvidia.com>
>> > Date:   Wed Aug 12 17:00:31 2015 +0200
>> >
>> >     drm/irq: Make pipe unsigned and name consistent
>> >
>> > Trigering a warning when trying to sample the vblank timestamp for
a
>> > non-existing pipe. There's a few ways to fix this:
>> >
>> > - Open-code the old behaviour, which just enshrines this slight
>> >   breakage of the userspace ABI.
>> >
>> > - Revert Mario's commit and again inflict broken timestamps,
again not
>> >   pretty.
>> >
>> > - Fix this for real by delaying the pageflip TS until the next
vblank
>> >   interrupt, thereby making it accurate.
>> >
>> > This patch implements the third option. Since having a page flip
>> > interrupt that happens when the pageflip gets armed and not when
it
>> > completes in the next vblank seems to be fairly common (older i915
hw
>> > works very similarly) create a new helper to arm vblank events for
>> > such drivers.
>>
>> What happens when the page flip interrupt arrives during a vertical
>> blank period?  Presumably the userspace event will be deferred until
the
>> next vertical blank period, but the flip might already take effect in
>> the current one.
>
> Hm yeah there's a tiny race if your update handler for the pageflip can
> race with your vblank handler. That's impossible here since it's
all done
> from the same hw irq hanlder, and since that is single-threaded there
> shouldn't be a problem, as long as vblank handling are pageflip are
> ordered correctly.
>
> Might be worth a note in the kerneldoc though that this function isn't
> perfectly foolproof.
> -Daniel

Is there any updates in this respect?

drm-nouveau-Fix-pre-nv50-pageflip-events-v4.patch
https://patchwork.kernel.org/patch/7591531

https://bugzilla.kernel.org/show_bug.cgi?id=106431
Reported:     2015-10-21

On Tue, Dec 01, 2015 at 04:08:16PM +0100, poma wrote:
> On Mon, Nov 16, 2015 at 4:11 PM, Daniel Vetter <daniel at ffwll.ch>
wrote:
> > On Mon, Nov 02, 2015 at 04:45:00PM +0900, Michel Dänzer wrote:
> >> On 31.10.2015 06:55, Daniel Vetter wrote:
> >> > Apparently pre-nv50 pageflip events happen before the actual
vblank
> >> > period. Therefore that functionality got semi-disabled in
> >> >
> >> > commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28
> >> > Author: Mario Kleiner <mario.kleiner.de at gmail.com>
> >> > Date:   Tue May 13 00:42:08 2014 +0200
> >> >
> >> >     drm/nouveau/kms/nv04-nv40: fix pageflip events via
special case.
> >> >
> >> > Unfortunately that hack got uprooted in
> >> >
> >> > commit cc1ef118fc099295ae6aabbacc8af94d8d8885eb
> >> > Author: Thierry Reding <treding at nvidia.com>
> >> > Date:   Wed Aug 12 17:00:31 2015 +0200
> >> >
> >> >     drm/irq: Make pipe unsigned and name consistent
> >> >
> >> > Trigering a warning when trying to sample the vblank
timestamp for a
> >> > non-existing pipe. There's a few ways to fix this:
> >> >
> >> > - Open-code the old behaviour, which just enshrines this
slight
> >> >   breakage of the userspace ABI.
> >> >
> >> > - Revert Mario's commit and again inflict broken
timestamps, again not
> >> >   pretty.
> >> >
> >> > - Fix this for real by delaying the pageflip TS until the
next vblank
> >> >   interrupt, thereby making it accurate.
> >> >
> >> > This patch implements the third option. Since having a page
flip
> >> > interrupt that happens when the pageflip gets armed and not
when it
> >> > completes in the next vblank seems to be fairly common (older
i915 hw
> >> > works very similarly) create a new helper to arm vblank
events for
> >> > such drivers.
> >>
> >> What happens when the page flip interrupt arrives during a
vertical
> >> blank period?  Presumably the userspace event will be deferred
until the
> >> next vertical blank period, but the flip might already take effect
in
> >> the current one.
> >
> > Hm yeah there's a tiny race if your update handler for the
pageflip can
> > race with your vblank handler. That's impossible here since
it's all done
> > from the same hw irq hanlder, and since that is single-threaded there
> > shouldn't be a problem, as long as vblank handling are pageflip
are
> > ordered correctly.
> >
> > Might be worth a note in the kerneldoc though that this function
isn't
> > perfectly foolproof.
> > -Daniel
> 
> 
> Is there any updates in this respect?
> 
> drm-nouveau-Fix-pre-nv50-pageflip-events-v4.patch
> https://patchwork.kernel.org/patch/7591531
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=106431
> Reported:     2015-10-21
Ben Skeggs asleep probably. Dave, can you pls pick this up?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch