diff mbox

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

Message ID 1393009415-27651-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjala Feb. 21, 2014, 7:03 p.m. UTC
From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
---
 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(-)

Comments

Jesse Barnes Feb. 26, 2014, 7:41 p.m. UTC | #1
On Fri, 21 Feb 2014 21:03:33 +0200
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@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@linux.intel.com>
> ---
>  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);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Daniel Vetter March 4, 2014, 9:24 a.m. UTC | #2
On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjala March 5, 2014, 12:38 p.m. UTC | #3
On Tue, Mar 04, 2014 at 10:24:54AM +0100, Daniel Vetter wrote:
> On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@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@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.

Much appreciated. I'll punt.

My hope was that other people would fix their own mess after seeing how
i915 did it, and then we could rip out the crap, but if you're feeling
the urge to do it all upfront I certainly won't object.
Daniel Vetter March 5, 2014, 1:55 p.m. UTC | #4
On Wed, Mar 05, 2014 at 02:38:25PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 04, 2014 at 10:24:54AM +0100, Daniel Vetter wrote:
> > On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@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@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.
> 
> Much appreciated. I'll punt.
> 
> My hope was that other people would fix their own mess after seeing how
> i915 did it, and then we could rip out the crap, but if you're feeling
> the urge to do it all upfront I certainly won't object.

My experience tells me that the only way to fix a cross-driver mess is to
simple charge ahead. If driver maintainers are asleep they'll end up with
a broken driver, but that usually gets their attention. Pleas and praying
don't ;-)
-Daniel
diff mbox

Patch

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);