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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 8715 bytes --]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
    Signed-off-by: Ben Skeggs <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Mario Kleiner <mario.kleiner.de-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Ben Skeggs <bskeggs-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Ilia Mirkin <imirkin-FrUbXkNCsVf2fBVCVOL8/A@public.gmane.org>
> Signed-off-by: Daniel Vetter <daniel.vetter-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> 
> 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
> + * @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;
> +
> +	assert_spin_locked(&dev->event_lock);
> +
> +	e->pipe = pipe;
> +	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
> + * @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);
> +		}
>  	}
>  
> -	/* 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);
>  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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/nouveau