[PATCH] [Karmic SRU] [Lucid] drm/i915: Fix sync to vblank when VGA output is turned off

[Andy Whitcroft]

On Wed, Dec 09, 2009 at 02:02:47PM +0100, Alberto Milone wrote:
> Hi all,
> 
> SRU Justification:
> 
> The problem described in this email was reported in a private OEM bug report 
> for Dell and has been solved by upstream. It's a regression in the drm code 
> which causes a massive system slowdown if we turn off the VGA output.
> 
> The patch is sane and minimal and is available in the drm-intel branch and in 
> the linux-next branch (see the link to the commit in the bug report).
> 
> I have applied (and slightly adapted, as it didn't apply cleanly to our lucid 
> and karmic git branches) and tested the patch successfully in Karmic.
> 
> The integration of this patch is very important for our Dell projects and for 
> the Ubuntu desktop at large.
> 
> Please include the attached patch in both Karmic and Lucid ASAP.
> 
> Bug #494461
> 
> Thanks in advance for your time.
> 
> Regards,
> 
> -- 
> Alberto Milone
> Sustaining Engineer (system)
> Foundations Team
> Canonical OEM Services

> From 778c902640530371a169ad1c03566e7c51b09874 Mon Sep 17 00:00:00 2001
> From: Li Peng <peng.li at linux.intel.com>
> Date: Mon, 9 Nov 2009 12:51:22 +0800
> Subject: [PATCH 1/1] drm/i915: Fix sync to vblank when VGA output is turned off
> 
> In current vblank-wait implementation, if we turn off VGA output,
> drm_wait_vblank will still wait on the disabled pipe until timeout,
> because vblank on the pipe is assumed be enabled. This would cause
> slow system response on some system such as moblin.
> 
> This patch resolve the issue by adding a drm helper function
> drm_vblank_off which explicitly clear vblank_enabled[crtc], wake up
> any waiting queue and save last vblank counter before turning off
> crtc. It also slightly change drm_vblank_get to ensure that we will
> will return immediately if trying to wait on a disabled pipe.
> 
> Signed-off-by: Li Peng <peng.li at intel.com>
> Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> [anholt: hand-applied for conflicts with overlay changes]
> Signed-off-by: Eric Anholt <eric at anholt.net>
> ---
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index f85aaf2..f298434 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -402,15 +402,21 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	/* Going from 0->1 means we have to enable interrupts again */
> -	if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1 &&
> -	    !dev->vblank_enabled[crtc]) {
> -		ret = dev->driver->enable_vblank(dev, crtc);
> -		DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
> -		if (ret)
> +	if (atomic_add_return(1, &dev->vblank_refcount[crtc]) == 1) {
> +		if (!dev->vblank_enabled[crtc]) {
> +			ret = dev->driver->enable_vblank(dev, crtc);
> +			DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
> +			if (ret)
> +				atomic_dec(&dev->vblank_refcount[crtc]);
> +			else {
> +				dev->vblank_enabled[crtc] = 1;
> +				drm_update_vblank_count(dev, crtc);
> +			}
> +		}
> +	} else {
> +		if (!dev->vblank_enabled[crtc]) {
>  			atomic_dec(&dev->vblank_refcount[crtc]);
> -		else {
> -			dev->vblank_enabled[crtc] = 1;
> -			drm_update_vblank_count(dev, crtc);
> +			ret = -EINVAL;
>  		}
>  	}
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -437,6 +443,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  }
>  EXPORT_SYMBOL(drm_vblank_put);
>  
> +void drm_vblank_off(struct drm_device *dev, int crtc)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	DRM_WAKEUP(&dev->vbl_queue[crtc]);
> +	dev->vblank_enabled[crtc] = 0;
> +	dev->last_vblank[crtc] = dev->driver->get_vblank_counter(dev, crtc);
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_vblank_off);
> +
>  /**
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 318ba47..652e9ac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1540,6 +1540,7 @@ static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		intel_update_watermarks(dev);
>  		/* Give the overlay scaler a chance to disable if it's on this pipe */
>  		//intel_crtc_dpms_video(crtc, FALSE); TODO
> +		drm_vblank_off(dev, pipe);
>  
>  		/* Disable the VGA plane that we never use */
>  		i915_disable_vga(dev);
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 45b67d9..4637dce 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1268,6 +1268,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, int crtc);
>  extern void drm_handle_vblank(struct drm_device *dev, int crtc);
>  extern int drm_vblank_get(struct drm_device *dev, int crtc);
>  extern void drm_vblank_put(struct drm_device *dev, int crtc);
> +extern void drm_vblank_off(struct drm_device *dev, int crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);
>  /* Modesetting support */
>  extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);

Although the patch is bit bit looking the change seems reasonable to me
and its testable.

Acked-by: Andy Whitcroft <apw at canonical.com>

-apw