Re: [PATCH] drm/nouveau: Fix pre-nv50 pageflip events

[Mario Kleiner <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi,

i just sent out a (v2) of Daniels patch, with my review comments and 
reviewed-by for the code already applied to the code for convenience. 
Interspersed below in the patch the review comments for a few small bugs.

This and Daniels original patch is only compile tested. I still have 
that GeForce 7800 GTX, but unfortunately i don't have the original PC 
anymore for testing it. Today i tried to put the card as a 2nd non-boot 
card into a MacPro for testing, but the EFI based Mac apparently didn't 
like that old PC card that much, so testing was a no go. Bootup ended 
with some nouveau MMIO read and write faults and then lockup. Usually 
more recent NVidia PC cards do work in Macs under Linux with nouveau as 
non-boot gpus, but for some reason this one doesn't.

Anyway, after digging through my old e-mail conversation with Ben from a 
year ago, i think Daniel's patch should work and solve the problem quite 
elegantly:

iirc Ben explained to me that on pre-nv50, nouveau_flip_complete() 
(which calls nouveau_finish_page_flip()), is not triggered by an actual 
pageflip interrupt, but by a fifo software interrupt programmed to fire 
shortly before the vblank. On my test card it fired in the last scanline 
before vblank, probably at the end of active scanout. 
nouveau_flip_complete() would first call nouveau_finish_page_flip() to 
send the pageflip event, and then manually flip to the new framebuffer 
by calling  nv_set_crtc_base(). I think/assume nv_set_crtc_base() is not 
itself synchronized to vblank, so we should get the correct behaviour:

1. Shortly before start of vblank: fifo sw interrupt -> 
nouveau_flip_complete() -> nouveau_finish_page_flip() queues pageflip 
event for later delivery by vblank irq handler -> nv_set_crtc_base() 
flips to the new fb. Return from irq.

2. A few scanlines later, vblank irq fires -> drm_handle_vblank() 
updates vblank count and timestamps -> drm_handle_vblank_events() 
dispatches queued pageflip completion event from 1), now tagged with 
proper vblank count and timestamp of flip completion.

thanks,
-mario


On 11/06/2015 06:19 PM, Thierry Reding wrote:
> Cc += Mario Kleiner, Mario, can you take a look whether this proposed
> solution makes sense and fixes the issues you were seeing back when you
> posted the patch in commit:
>
> commit af4870e406126b7ac0ae7c7ce5751f25ebe60f28
> Author: Mario Kleiner <mario.kleiner.de@gmail.com>
> Date:   Tue May 13 00:42:08 2014 +0200
>
>      drm/nouveau/kms/nv04-nv40: fix pageflip events via special case.
>
>      Cards with nv04 display engine can't reliably use vblank
>      counts and timestamps computed via drm_handle_vblank(), as
>      the function gets invoked after sending the pageflip events.
>
>      Fix this by defaulting to the old crtcid = -1 fallback path
>      on <= NV-50 cards, and only using the precise path on NV-50
>      and later.
>
>      Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
>      Signed-off-by: Ben Skeggs <bskeggs@redhat.com>
>      Cc: <stable@vger.kernel.org> # 3.13+
>
> Do you happen to still have the setup around where you saw this?
>
> Thierry
>
> On Fri, Oct 30, 2015 at 10:55:40PM +0100, 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@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@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.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=106431
>> Cc: Thierry Reding <treding@nvidia.com>
>> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
>> Cc: Ben Skeggs <bskeggs@redhat.com>
>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>
>> Note that due to lack of hw this is completely untested. But I think
>> it's the right way to fix this.
>> -Daniel
>> ---
>>   drivers/gpu/drm/drm_irq.c                 | 56 ++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/nouveau/nouveau_display.c | 16 ++++-----
>>   include/drm/drmP.h                        |  4 +++
>>   3 files changed, 66 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 46dbc34b81ba..b3e1f58666a6 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -972,7 +972,8 @@ static void send_vblank_event(struct drm_device *dev,
>>   		struct drm_pending_vblank_event *e,
>>   		unsigned long seq, struct timeval *now)
>>   {
>> -	WARN_ON_SMP(!spin_is_locked(&dev->event_lock));
>> +	assert_spin_locked(&dev->event_lock);
>> +
>>   	e->event.sequence = seq;
>>   	e->event.tv_sec = now->tv_sec;
>>   	e->event.tv_usec = now->tv_usec;
>> @@ -985,6 +986,59 @@ static void send_vblank_event(struct drm_device *dev,
>>   }
>>
>>   /**
>> + * drm_arm_vblank_event - arm vblanke event after pageflip

Typo vblanke -> vblank

>> + * @dev: DRM device
>> + * @pipe: CRTC index
>> + * @e: the event to prepare to send
>> + *
>> + * A lot of drivers need to generate vblank events for the very next vblank
>> + * interrupt. For example when the page flip interrupt happens when the page
>> + * flip gets armed, but not when it actually executes within the next vblank
>> + * period. This helper function implements exactly the required vblank arming
>> + * behaviour.
>> + *
>> + * Caller must hold event lock. Caller must also hold a vblank reference for the
>> + * event @e, which will be dropped when the next vblank arrives.
>> + *
>> + * This is the legacy version of drm_crtc_arm_vblank_event().
>> + */
>> +void drm_arm_vblank_event(struct drm_device *dev, unsigned int pipe,
>> +			  struct drm_pending_vblank_event *e)
>> +{
>> +	struct timeval now;
>> +	unsigned int seq;

Dead code: now and seq are not used.

>> +	assert_spin_locked(&dev->event_lock);
>> +
>> +	e->pipe = pipe;

-> Add this missing init:

+       e->event.sequence = drm_vblank_count(dev, pipe);

Otherwise target sequence number for dispatch of the pageflip event by 
drm_handle_vblank_events() will be zero and we get in trouble for a 
failing comparison for running vblank count > 2^23 and clients probably 
hang?

>> +	list_add_tail(&e->base.link, &dev->vblank_event_list);
>> +}
>> +EXPORT_SYMBOL(drm_arm_vblank_event);
>> +
>> +/**
>> + * drm_arm_vblank_event - arm vblanke event after pageflip

vblanke -> vblank

>> + * @crtc: the source CRTC of the vblank event
>> + * @e: the event to send
>> + *
>> + * A lot of drivers need to generate vblank events for the very next vblank
>> + * interrupt. For example when the page flip interrupt happens when the page
>> + * flip gets armed, but not when it actually executes within the next vblank
>> + * period. This helper function implements exactly the required vblank arming
>> + * behaviour.
>> + *
>> + * Caller must hold event lock. Caller must also hold a vblank reference for the
>> + * event @e, which will be dropped when the next vblank arrives.
>> + *
>> + * This is the native KMS version of drm_send_vblank_event().
>> + */
>> +void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
>> +			       struct drm_pending_vblank_event *e)
>> +{
>> +	drm_arm_vblank_event(crtc->dev, drm_crtc_index(crtc), e);
>> +}
>> +EXPORT_SYMBOL(drm_crtc_arm_vblank_event);
>> +
>> +/**
>>    * drm_send_vblank_event - helper to send vblank event after pageflip
>>    * @dev: DRM device
>>    * @pipe: CRTC index
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
>> index 184445d4abbf..041e5f84538c 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_display.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_display.c
>> @@ -826,7 +826,6 @@ nouveau_finish_page_flip(struct nouveau_channel *chan,
>>   	struct drm_device *dev = drm->dev;
>>   	struct nouveau_page_flip_state *s;
>>   	unsigned long flags;
>> -	int crtcid = -1;
>>
>>   	spin_lock_irqsave(&dev->event_lock, flags);
>>
>> @@ -838,16 +837,15 @@ nouveau_finish_page_flip(struct nouveau_channel *chan,
>>
>>   	s = list_first_entry(&fctx->flip, struct nouveau_page_flip_state, head);
>>   	if (s->event) {
>> -		/* Vblank timestamps/counts are only correct on >= NV-50 */
>> -		if (drm->device.info.family >= NV_DEVICE_INFO_V0_TESLA)
>> -			crtcid = s->crtc;
>> -
>> -		drm_send_vblank_event(dev, crtcid, s->event);
>> +		if (drm->device.info.family < NV_DEVICE_INFO_V0_TESLA) {
>> +			drm_arm_vblank_event(dev, s->crtc, s->event);
>> +		} else {
>> +			drm_send_vblank_event(dev, s->crtc, s->event);
>> +			/* Give up ownership of vblank for page-flipped crtc */
>> +			drm_vblank_put(dev, s->crtc);
>> +		}
>>   	}

Need a drm_vblank_put() here in a else branch, so refcounting is 
balanced in case of !s->event.

>>
>> -	/* Give up ownership of vblank for page-flipped crtc */
>> -	drm_vblank_put(dev, s->crtc);
>> -
>>   	list_del(&s->head);
>>   	if (ps)
>>   		*ps = *s;
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index eb513341b6ee..4c91ac419d5d 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -948,6 +948,10 @@ extern void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
>>   				  struct drm_pending_vblank_event *e);
>>   extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>>   				       struct drm_pending_vblank_event *e);
>> +void drm_send_vblank_event(struct drm_device *dev, unsigned int pipe,
>> +			   struct drm_pending_vblank_event *e);
>> +void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>> +				struct drm_pending_vblank_event *e);

-> Copy & Paste error, wrong function prototype.

>>   extern bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe);
>>   extern bool drm_crtc_handle_vblank(struct drm_crtc *crtc);
>>   extern int drm_vblank_get(struct drm_device *dev, unsigned int pipe);
>> --
>> 2.5.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau