Re: [PATCH v3 12/20] drm: omapdrm: Prevent processing the same event multiple times

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Tuesday 27 Sep 2016 23:05:20 Daniel Kurtz wrote:
> On Mon, Sep 19, 2016 at 8:27 PM, Laurent Pinchart wrote:
> > The vblank interrupt is disabled after one occurrence, preventing the
> > atomic update event from being processed twice. However, this also
> > prevents the software frame counter from being updated correctly that
> > would require vblank interrupts to be kept enabled while the CRTC is
> > active.
> > 
> > In preparation for vblank interrupt fixes, make sure that the atomic
> > update event will be processed once only when the vblank interrupt will
> > be kept enabled.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_crtc.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index a0c26592fc69..8fef6558197b
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -43,6 +43,7 @@ struct omap_crtc {
> >         bool enabled;
> >         bool pending;
> >         wait_queue_head_t pending_wait;
> > +       struct drm_pending_vblank_event *event;
> >  };
> >  
> >  /* ---------------------------------------------------------------------
> > @@ -260,11 +261,15 @@ static const struct dss_mgr_ops mgr_ops = {
> > 
> >  static void omap_crtc_complete_page_flip(struct drm_crtc *crtc)
> >  {
> > +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >         struct drm_pending_vblank_event *event;
> >         struct drm_device *dev = crtc->dev;
> >         unsigned long flags;
> > 
> > -       event = crtc->state->event;
> > +       spin_lock_irqsave(&dev->event_lock, flags);
> > +       event = omap_crtc->event;
> > +       omap_crtc->event = NULL;
> > +       spin_unlock_irqrestore(&dev->event_lock, flags);
> 
> Perhaps just hold the lock until after drm_crtc_send_vblank_event(crtc,
> event)?

Good point, I'll do that.

> >         if (!event)
> >                 return;
> > 
> > @@ -384,12 +389,23 @@ static int omap_crtc_atomic_check(struct drm_crtc
> > *crtc,
> >  }
> >  
> >  static void omap_crtc_atomic_begin(struct drm_crtc *crtc,
> > -                                  struct drm_crtc_state *old_crtc_state)
> > +                                  struct drm_crtc_state *old_crtc_state)
> >  {
> > +       struct drm_pending_vblank_event *event = crtc->state->event;
> > +       struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > +       unsigned long flags;
> > +
> > +       if (event) {
> > +               WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +
> > +               spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +               omap_crtc->event = event;
> > +               spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > +       }
> 
> AFAICT, the only reason to use the spin_lock_irqsave here is if there
> is a possibility that the vblank irq can run in parallel to
> atomic_begin().
> 
> If this is the case, then setting omap_crtc->event here in
> atomic_begin() may not work exactly as you wish.
> It sets up a race: the vblank irq may call
> omap_crtc_complete_page_flip and send the event between here and when
> the "GO" bit is set in atomic_flush(), and thus, it may send the event
> too early, on the wrong vblank.
> 
> By the way, it looks like there is the same race, but just much
> shorter, with the "omap_crtc->pending" flag.  Since it is set in
> atomic_flush() before hitting the go bit - there is a tiny window
> where an irq could arrive and see pending = true for the wrong vblank.

This should be fixed in patch 14/20 ("drm: omapdrm: Keep vblank interrupt 
enabled while CRTC is active") in this series.

> I'm not familiar enough with this driver, however, to tell if it is
> really possible to get an irq here or not.

It shouldn't be as the driver only enables interrupts after setting the GO 
bit. However, that logic is changed in the aforementioned patch, which will 
introduce the race condition you described. I'll move the code to the atomic 
flush handler in this patch to fix it.

> >  }
> >  
> >  static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
> > -                                  struct drm_crtc_state *old_crtc_state)
> > +                                  struct drm_crtc_state *old_crtc_state)
> >  {
> >         struct omap_crtc *omap_crtc = to_omap_crtc(crtc);

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel