Re: [PATCH 5/5] drm/irq: Don't call ->get_vblank_counter directly from irq_uninstall/cleanup

[Imre Deak <imre.deak@intel.com>]

On pe, 2015-02-13 at 21:03 +0100, Daniel Vetter wrote:
> The pipe might already have been shut down, and then it's not a good
> idea to call hw accessor functions. Instead use the same logic as
> drm_vblank_off which has all the necessary checks to avoid troubles or
> inconsistency.
> 
> Noticed by Imre while reviewing my patches to remove some sanity
> checks from ->get_vblank_counter.
> 
> v2: Try harder. disable_and_save can still access the vblank stuff
> when vblank->enabled isn't set. It has to, since vlbank irq could be
> disable but the pipe is still on when being called from
> drm_vblank_off. But we still want to use that code for more code
> sharing. So add a check for vblank->enabled on top - if that's not set
> we shouldn't have anyone waiting for the vblank. If we have that's a
> pretty serious bug.
> 
> The other issue that Imre spotted is drm_vblank_cleanup. That code
> again calls disable_and_save and so suffers from the same issues. But
> really drm_irq_uninstall should have cleaned that all up, so replace
> the code with WARN_ON. Note that we can't delete the timer cleanup
> since drivers aren't required to use drm_irq_install/uninstall, but
> can do their own irq handling.
> 
> v3: Make it clear that all that gunk in drm_irq_uninstall is really
> just bandaids for UMS races between the irq/vblank code. In UMS
> userspace is in control of enabling/disabling interrupts in general
> and vblanks specifically.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 1e5fb1b994d7..885fb756fed5 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -269,7 +269,6 @@ static void vblank_disable_fn(unsigned long arg)
>  void drm_vblank_cleanup(struct drm_device *dev)
>  {
>  	int crtc;
> -	unsigned long irqflags;
>  
>  	/* Bail if the driver didn't call drm_vblank_init() */
>  	if (dev->num_crtcs == 0)
> @@ -278,11 +277,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
>  		struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  
> -		del_timer_sync(&vblank->disable_timer);
> +		WARN_ON(vblank->enabled);
>  
> -		spin_lock_irqsave(&dev->vbl_lock, irqflags);
> -		vblank_disable_and_save(dev, crtc);
> -		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> +		del_timer_sync(&vblank->disable_timer);
>  	}

Looking also at non-i915 drivers, drm_vblank_cleanup is called always
before drm_irq_uninstall. Due to 'dev->num_crtcs = 0' in
drm_vblank_cleanup the loop in drm_irq_uninstall will be bypassed and
vblank_disable_and_save won't be called. This is mainly a concern for
UMS as you commented below, but if we care about that I'd prefer doing
the same thing above as you did in drm_irq_uninstall. Don't do anything
if vblanks are disabled otherwise warn only for KMS and call
vblank_disable_and_save and wake_up the vblank queue.

With that this series looks ok to me.

>  
>  	kfree(dev->vblank);
> @@ -468,17 +465,23 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	dev->irq_enabled = false;
>  
>  	/*
> -	 * Wake up any waiters so they don't hang.
> +	 * Wake up any waiters so they don't hang. This is just to paper over
> +	 * isssues for UMS drivers which aren't in full control of their
> +	 * vblank/irq handling. KMS drivers must ensure that vblanks are all
> +	 * disabled when uninstalling the irq handler.
>  	 */
>  	if (dev->num_crtcs) {
>  		spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  		for (i = 0; i < dev->num_crtcs; i++) {
>  			struct drm_vblank_crtc *vblank = &dev->vblank[i];
>  
> +			if (!vblank->enabled)
> +				continue;
> +
> +			WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET));
> +
> +			vblank_disable_and_save(dev, i);
>  			wake_up(&vblank->queue);
> -			vblank->enabled = false;
> -			vblank->last =
> -				dev->driver->get_vblank_counter(dev, i);
>  		}
>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  	}


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx