[PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

Daniel Vetter daniel at ffwll.ch
Tue Mar 4 01:24:54 PST 2014


On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Allow the driver to specify whether all new vblank requests after
> drm_vblank_off() should be rejected. And add a counterpart called
> drm_vblank_on() which will again allow vblank requests to come in.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Not really happy about this - drm_irq.c is already a giant mess, adding
more driver-specific hacks doesn't help. I think we need a few bits of
polish on top of your code:

- Add stern warnings to pre/post_modeset that they're inherently racy.

- Add calls to drm_vblank_off to every driver lacking them. Put it at the
  beginning of their crtc disable functions expect when there's a call to
  pre_modeset. Then it should be right after that.

- Sprinkle calls to drm_vblank_on over all drivers. Put them at the end of
  their crtc enable functions except when there's a call to post_modeset.
  Then put it right before that.

- Rip out the reject argument again and make it the default. If we have
  drm_vblank_off everywhere then all old vblank waits should complete and
  there's no userspace yet out there which races a modeset with vblank
  ioctl calls. Then only issue would be userspace which does vblank waits
  on disabled ioctls which a) is buggy b) we can easily fix with a driver
  quirk flag if _really_ needed.

This way the drm_irq.c mess will at least converge a bit and so should
help generic display servers (like Wayland) a lot.

I can volunteer for this if you want to punt on it yourself.
-Daniel

> ---
>  drivers/gpu/drm/armada/armada_crtc.c     |  2 +-
>  drivers/gpu/drm/drm_irq.c                | 29 ++++++++++++++++++++++++++++-
>  drivers/gpu/drm/exynos/exynos_drm_crtc.c |  2 +-
>  drivers/gpu/drm/gma500/gma_display.c     |  2 +-
>  drivers/gpu/drm/i915/intel_display.c     |  6 +++---
>  drivers/gpu/drm/tegra/dc.c               |  2 +-
>  include/drm/drmP.h                       |  4 +++-
>  7 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c
> index d8e3982..74317b2 100644
> --- a/drivers/gpu/drm/armada/armada_crtc.c
> +++ b/drivers/gpu/drm/armada/armada_crtc.c
> @@ -257,7 +257,7 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc)
>  	 * Tell the DRM core that vblank IRQs aren't going to happen for
>  	 * a while.  This cleans up any pending vblank events for us.
>  	 */
> -	drm_vblank_off(dev, dcrtc->num);
> +	drm_vblank_off(dev, dcrtc->num, false);
>  
>  	/* Handle any pending flip event. */
>  	spin_lock_irq(&dev->event_lock);
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3211158..6e5d820 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -890,6 +890,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>  	int ret = 0;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +
> +	if (dev->vblank[crtc].reject) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
>  	/* 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);
> @@ -917,6 +923,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>  			ret = -EINVAL;
>  		}
>  	}
> +
> + out:
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  
>  	return ret;
> @@ -947,8 +955,9 @@ EXPORT_SYMBOL(drm_vblank_put);
>   * drm_vblank_off - disable vblank events on a CRTC
>   * @dev: DRM device
>   * @crtc: CRTC in question
> + * @reject: reject drm_vblank_get() until drm_vblank_on() has been called?
>   */
> -void drm_vblank_off(struct drm_device *dev, int crtc)
> +void drm_vblank_off(struct drm_device *dev, int crtc, bool reject)
>  {
>  	struct drm_pending_vblank_event *e, *t;
>  	struct timeval now;
> @@ -956,6 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  	unsigned int seq;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	dev->vblank[crtc].reject = reject;
>  	vblank_disable_and_save(dev, crtc);
>  	wake_up(&dev->vblank[crtc].queue);
>  
> @@ -979,6 +989,22 @@ 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);
> +	dev->vblank[crtc].reject = false;
> +	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
> @@ -1224,6 +1250,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,
>  		    (((drm_vblank_count(dev, crtc) -
>  		       vblwait->request.sequence) <= (1 << 23)) ||
> +		     dev->vblank[crtc].reject ||
>  		     !dev->irq_enabled));
>  
>  	if (ret != -EINTR) {
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> index ebc0150..e2d6b9d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c
> @@ -68,7 +68,7 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		/* wait for the completion of page flip. */
>  		wait_event(exynos_crtc->pending_flip_queue,
>  				atomic_read(&exynos_crtc->pending_flip) == 0);
> -		drm_vblank_off(crtc->dev, exynos_crtc->pipe);
> +		drm_vblank_off(crtc->dev, exynos_crtc->pipe, false);
>  	}
>  
>  	exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms);
> diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
> index 386de2c..ff18220 100644
> --- a/drivers/gpu/drm/gma500/gma_display.c
> +++ b/drivers/gpu/drm/gma500/gma_display.c
> @@ -281,7 +281,7 @@ void gma_crtc_dpms(struct drm_crtc *crtc, int mode)
>  		REG_WRITE(VGACNTRL, VGA_DISP_DISABLE);
>  
>  		/* Turn off vblank interrupts */
> -		drm_vblank_off(dev, pipe);
> +		drm_vblank_off(dev, pipe, false);
>  
>  		/* Wait for vblank for the disable to take effect */
>  		gma_wait_for_vblank(dev);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f19e6ea..bab0d08 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3643,7 +3643,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
>  	int plane = intel_crtc->plane;
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
> -	drm_vblank_off(dev, pipe);
> +	drm_vblank_off(dev, pipe, false);
>  
>  	/* FBC must be disabled before disabling the plane on HSW. */
>  	if (dev_priv->fbc.plane == plane)
> @@ -3774,7 +3774,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  		encoder->disable(encoder);
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
> -	drm_vblank_off(dev, pipe);
> +	drm_vblank_off(dev, pipe, false);
>  
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> @@ -4239,7 +4239,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  
>  	/* Give the overlay scaler a chance to disable if it's on this pipe */
>  	intel_crtc_wait_for_pending_flips(crtc);
> -	drm_vblank_off(dev, pipe);
> +	drm_vblank_off(dev, pipe, false);
>  
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 9336006..480bfec 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -324,7 +324,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc)
>  		}
>  	}
>  
> -	drm_vblank_off(drm, dc->pipe);
> +	drm_vblank_off(drm, dc->pipe, false);
>  }
>  
>  static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc,
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index f974da9..ee40483 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1090,6 +1090,7 @@ struct drm_vblank_crtc {
>  	int crtc;			/* crtc index */
>  	bool enabled;			/* so we don't call enable more than
>  					   once per disable */
> +	bool reject;			/* reject drm_vblank_get()? */
>  };
>  
>  /**
> @@ -1400,7 +1401,8 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc,
>  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_off(struct drm_device *dev, int crtc, bool reject);
> +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.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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


More information about the dri-devel mailing list