Re: [PATCH] drm: idiot-proof vblank

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
> After spending slightly more time than I'd care to admit debugging the
> various and presumably spectacular way things fail when you pass too low
> a value to drm_vblank_init() (thanks console-lock for not letting me see
> the carnage!), I decided it might be a good idea to add some sanity
> checking.

Hmm. Could we instead do some kind of cross checking in
drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this
stuff all over the place? I guess the checks would need to happen both
ways since the driver might call those in either order.

> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> btw, I wonder if we could add a module param hack to toss initial modeset
> off to a workqueue to sneak it out from the tyranny of console_lock?
> 
>  drivers/gpu/drm/drm_irq.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123a..6f16a104 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>   */
>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>  {
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return 0;
>  	return atomic_read(&dev->vblank[crtc].count);
>  }
>  EXPORT_SYMBOL(drm_vblank_count);
> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
>  {
>  	u32 cur_vblank;
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return 0;
> +
>  	/* Read timestamp from slot of _vblank_time ringbuffer
>  	 * that corresponds to current vblank count. Retry if
>  	 * count has incremented during readout. This works like
> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>  	unsigned long irqflags;
>  	int ret = 0;
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return -EINVAL;
> +
>  	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) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  {
>  	BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return;
> +
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&
>  	    (drm_vblank_offdelay > 0))
> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  	unsigned long irqflags;
>  	unsigned int seq;
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return;
> +
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	vblank_disable_and_save(dev, crtc);
>  	wake_up(&dev->vblank[crtc].queue);
> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  {
>  	unsigned long irqflags;
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return;
> +
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	/* re-enable interrupts if there's are users left */
>  	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> @@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
>  	/* vblank is not initialized (IRQ not installed ?), or has been freed */
>  	if (!dev->num_crtcs)
>  		return;
> +
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return;
> +
>  	/*
>  	 * To avoid all the problems that might happen if interrupts
>  	 * were enabled/disabled around or between these calls, we just
> @@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  	if (!dev->num_crtcs)
>  		return false;
>  
> +	if (WARN_ON(crtc >= dev->num_crtcs))
> +		return false;
> +
>  	/* Need timestamp lock to prevent concurrent execution with
>  	 * vblank enable/disable, as this would cause inconsistent
>  	 * or corrupted timestamps and vblank counts.
> -- 
> 1.9.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC