[PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Chris Wilson]

On Fri, Mar 17, 2017 at 11:47:51AM +0200, Ville Syrjälä wrote:
> On Thu, Mar 16, 2017 at 11:47:48PM +0000, Chris Wilson wrote:
> > @@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg)
> >  	unsigned long irqflags;
> >  
> >  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > -	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
> > +	if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) {
> 
> Hmm. Aren't most of these accesses inside the lock? Looks like you're
> marking everything READ/WRITE_ONCE()?

There's like 3 different locks here. Afaict, the correct one for
serialising vblank->enabled was dev->vblank_time_lock. Every access
outside of that lock, I marked up as READ_ONCE.

Oh, you are using vbl_lock as the barrier? That's not as clear for
disable:

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 53a526c7b24d..f447ed07ef95 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -325,6 +325,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
        struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
        unsigned long irqflags;
 
+       assert_spin_locked(&dev->vbl_lock);
+
        /* Prevent vblank irq processing while disabling vblank irqs,
         * so no updates of timestamps or count can happen after we've
         * disabled. Needed to prevent races in case of delayed irq's.


> > @@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
> >  	if (WARN_ON(pipe >= dev->num_crtcs))
> >  		return false;
> >  
> > +	if (!READ_ONCE(vblank->enabled))
> > +		return false;
> 
> This to me looks like it could theoretically cause us to
> miss an interrupt.
> 
> 1. enable_irq()
> 2. drm_update_vblank_count()
> 3. irq fires
> 4. drm_handle_vblank() doesn't do anything
> 5. enabled=true

Sure. There's a danger you miss the irq anyway, and so the last action
after enabling the interrupt should be to process any completed events -
that's implicitly handled by enabling the interrupt in advance of adding
the first event. In the scenario above, there should be nothing to do.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre