[Intel-gfx] [PATCH] drm: don't decrement vblank wait refcount if we never took a ref

[Jesse Barnes]

Dave, please pick this one up (just grab it from the parent message).

Thanks,
Jesse

On Thursday 19 February 2009 06:55:28 Chris Wilson wrote:
> On Mon, 2009-02-16 at 10:55 -0800, Jesse Barnes wrote:
> > On Sunday, February 15, 2009 4:02 pm Chris Wilson wrote:
> > > On my i915, the flip never occurs and we wait forever on the the
> > > vblank. So I presume the command we sent the chip is invalid, or we
> > > miss the irq? (I double-checked with lockdep in case I had missed an
> > > obvious dead-lock.) Comments on the patch itself inline.
> >
> > Thanks a lot for looking at this and testing.
> >
> > Hm, maybe you're not getting vblank interrupts at all?  Do you see the
> > count increasing?  Is the IRQ registered?
>
> Apparently, the vblank is never being enabled... Adding a BUG_ON() to
> drm_vblank_put() soon identified the unbalanced culprit. So it appears
> that drm_vblank_pre_modeset() can fail to enable vblank, obviously
> because at that point we have never enabled vblank in pipeconf - but we
> unconditionally decrement the vblank reference counter afterwards. Ok,
> so now I see lots of vblank interrupts but the display is still snafu.
>
> The following patch is a crude fix.
>
> >From 1c4fa54654a604e0ea604553b983dca737a76e99 Mon Sep 17 00:00:00 2001
>
> From: Chris Wilson <chris at chris-wilson.co.uk>
> Date: Thu, 19 Feb 2009 14:48:22 +0000
> Subject: [PATCH] drm: Correct unbalanced drm_vblank_put() during mode
> setting.
>
> The first time we install a mode, the vblank will be disabled for a pipe
> and so drm_vblank_get() in drm_vblank_pre_modeset() will fail. As we
> unconditionally call drm_vblank_put() afterwards, the vblank reference
> counter becomes unbalanced.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_irq.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3795dbc..93e677a 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -435,6 +435,8 @@ EXPORT_SYMBOL(drm_vblank_get);
>   */
>  void drm_vblank_put(struct drm_device *dev, int crtc)
>  {
> +	BUG_ON (atomic_read (&dev->vblank_refcount[crtc]) == 0);
> +
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&dev->vblank_refcount[crtc]))
>  		mod_timer(&dev->vblank_disable_timer, jiffies + 5*DRM_HZ);
> @@ -460,8 +462,9 @@ void drm_vblank_pre_modeset(struct drm_device *dev,
> int crtc)
>  	 * so that interrupts remain enabled in the interim.
>  	 */
>  	if (!dev->vblank_inmodeset[crtc]) {
> -		dev->vblank_inmodeset[crtc] = 1;
> -		drm_vblank_get(dev, crtc);
> +		dev->vblank_inmodeset[crtc] = 0x1;
> +		if (drm_vblank_get(dev, crtc) == 0)
> +			dev->vblank_inmodeset[crtc] |= 0x2;
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_pre_modeset);
> @@ -473,9 +476,12 @@ void drm_vblank_post_modeset(struct drm_device
> *dev, int crtc)
>  	if (dev->vblank_inmodeset[crtc]) {
>  		spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  		dev->vblank_disable_allowed = 1;
> -		dev->vblank_inmodeset[crtc] = 0;
>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> -		drm_vblank_put(dev, crtc);
> +
> +		if (dev->vblank_inmodeset[crtc] & 0x2)
> +			drm_vblank_put(dev, crtc);
> +
> +		dev->vblank_inmodeset[crtc] = 0;
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_post_modeset);

-- 
Jesse Barnes, Intel Open Source Technology Center