Re: [PATCH 04/12] drm: Add drm_vblank_on()

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, May 14, 2014 at 08:51:06PM +0200, Daniel Vetter wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> drm_vblank_off() will turn off vblank interrupts, but as long as the
> refcount is elevated drm_vblank_get() will not re-enable them. This
> is a problem is someone is holding a vblank reference while a modeset is
> happening, and the driver requires vblank interrupt to work during that
> time.
> 
> Add drm_vblank_on() as a counterpart to drm_vblank_off() which will
> re-enabled vblank interrupts if the refcount is already elevated. This
> will allow drivers to choose the specific places in the modeset sequence
> at which vblank interrupts get disabled and enabled.
> 
> Testcase: igt/kms_flip/*-vs-suspend

Testcase: igt/kms_flip/*-vs-vblank-race

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> [danvet: Add Testcase tag for the igt I've written.]
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c            | 72 ++++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  include/drm/drmP.h                   |  1 +
>  3 files changed, 62 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 13d671ed3421..dd786d84daab 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -840,6 +840,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  }
>  
>  /**
> + * drm_vblank_enable - enable the vblank interrupt on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */
> +static int drm_vblank_enable(struct drm_device *dev, int crtc)
> +{
> +	int ret = 0;
> +
> +	assert_spin_locked(&dev->vbl_lock);
> +
> +	spin_lock(&dev->vblank_time_lock);
> +
> +	if (!dev->vblank[crtc].enabled) {
> +		/* Enable vblank irqs under vblank_time_lock protection.
> +		 * All vblank count & timestamp updates are held off
> +		 * until we are done reinitializing master counter and
> +		 * timestamps. Filtercode in drm_handle_vblank() will
> +		 * prevent double-accounting of same vblank interval.
> +		 */
> +		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[crtc].refcount);
> +		else {
> +			dev->vblank[crtc].enabled = true;
> +			drm_update_vblank_count(dev, crtc);
> +		}
> +	}
> +
> +	spin_unlock(&dev->vblank_time_lock);
> +
> +	return ret;
> +}
> +
> +/**
>   * drm_vblank_get - get a reference count on vblank events
>   * @dev: DRM device
>   * @crtc: which CRTC to own
> @@ -858,25 +893,7 @@ 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[crtc].refcount) == 1) {
> -		spin_lock(&dev->vblank_time_lock);
> -		if (!dev->vblank[crtc].enabled) {
> -			/* Enable vblank irqs under vblank_time_lock protection.
> -			 * All vblank count & timestamp updates are held off
> -			 * until we are done reinitializing master counter and
> -			 * timestamps. Filtercode in drm_handle_vblank() will
> -			 * prevent double-accounting of same vblank interval.
> -			 */
> -			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[crtc].refcount);
> -			else {
> -				dev->vblank[crtc].enabled = true;
> -				drm_update_vblank_count(dev, crtc);
> -			}
> -		}
> -		spin_unlock(&dev->vblank_time_lock);
> +		ret = drm_vblank_enable(dev, crtc);
>  	} else {
>  		if (!dev->vblank[crtc].enabled) {
>  			atomic_dec(&dev->vblank[crtc].refcount);
> @@ -946,6 +963,23 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  EXPORT_SYMBOL(drm_vblank_off);
>  
>  /**
> + * drm_vblank_on - enable vblank events on a CRTC
> + * @dev: DRM device
> + * @crtc: CRTC in question
> + */
> +void drm_vblank_on(struct drm_device *dev, int crtc)
> +{
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/* re-enable interrupts if there's are users left */
> +	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> +		WARN_ON(drm_vblank_enable(dev, crtc));
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_vblank_on);
> +
> +/**
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
>   * @dev: DRM device
>   * @crtc: CRTC in question
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b39d0367dd68..858c393b051f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3739,6 +3739,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  	 * happening.
>  	 */
>  	intel_wait_for_vblank(dev, intel_crtc->pipe);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  /* IPS only exists on ULT machines and is tied to pipe A. */
> @@ -3830,6 +3832,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 * to change the workaround. */
>  	haswell_mode_set_planes_workaround(intel_crtc);
>  	ilk_crtc_enable_planes(crtc);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -4353,6 +4357,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void i9xx_crtc_enable(struct drm_crtc *crtc)
> @@ -4400,6 +4406,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> +
> +	drm_vblank_on(dev, pipe);
>  }
>  
>  static void i9xx_pfit_disable(struct intel_crtc *crtc)
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 9d982d483f12..7339b2b00724 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1360,6 +1360,7 @@ extern bool 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_on(struct drm_device *dev, int crtc);
>  extern void drm_vblank_cleanup(struct drm_device *dev);
>  extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc,
>  				     struct timeval *tvblank, unsigned flags);
> -- 
> 1.8.3.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch