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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
