Hi Tomi,
On Thursday 15 Dec 2016 14:52:47 Tomi Valkeinen wrote:
> On 14/12/16 02:27, Laurent Pinchart wrote:
> > Instead of going through a complicated private IRQ registration
> > mechanism, handle the vblank interrupt activation with the standard
> > drm_crtc_vblank_get() and drm_crtc_vblank_put() mechanism. This will let
> > the DRM core keep the vblank interrupt enabled as long as needed to
> > update the frame counter.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> >
> > drivers/gpu/drm/omapdrm/omap_crtc.c | 38 ++++++++++++--------------------
> > drivers/gpu/drm/omapdrm/omap_drv.h | 1 +
> > drivers/gpu/drm/omapdrm/omap_irq.c | 4 +++-
> > 3 files changed, 18 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 827ac46a6d5e..1f5372042706
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -36,8 +36,6 @@ struct omap_crtc {
> >
> > struct videomode vm;
> >
> > - struct omap_drm_irq vblank_irq;
> > -
> > bool ignore_digit_sync_lost;
> > bool enabled;
> >
> > @@ -304,25 +302,24 @@ void omap_crtc_error_irq(struct drm_crtc *crtc,
> > uint32_t irqstatus)
> > DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name,
irqstatus);
> > }
> >
> > -static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t
> > irqstatus)
> > +void omap_crtc_vblank_irq(struct drm_crtc *crtc)
> > {
> > - struct omap_crtc *omap_crtc =
> > - container_of(irq, struct omap_crtc, vblank_irq);
> > - struct drm_device *dev = omap_crtc->base.dev;
> > - struct drm_crtc *crtc = &omap_crtc->base;
> > + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > + bool pending;
> >
> > if (dispc_mgr_go_busy(omap_crtc->channel))
> > return;
> >
> > DBG("%s: apply done", omap_crtc->name);
> >
> > - __omap_irq_unregister(dev, &omap_crtc->vblank_irq);
> > -
> > spin_lock(&crtc->dev->event_lock);
> >
> > - WARN_ON(!omap_crtc->pending);
> > + pending = omap_crtc->pending;
> >
> > omap_crtc->pending = false;
> > spin_unlock(&crtc->dev->event_lock);
> >
> > + if (pending)
> > + drm_crtc_vblank_put(crtc);
> > +
>
> I think there's a race.
>
> - irq: we get vblank irq
> - irq: GO is not set, so dispc_mgr_go_busy() check doesn't trigger
> - flush: we set GO
> - flush: we lock event_lock and set the pending & event, and unlock
> - irq: irq handler continues, sees pending and event, and thinks that
> the config has been taken into use, but in reality GO bit is set and
> it'll happen only on next vblank.
I think you're right. I'll try to fix it.
> > /* wake up userspace */
> > omap_crtc_complete_page_flip(&omap_crtc->base);
> >
> > @@ -340,8 +337,6 @@ static void omap_crtc_destroy(struct drm_crtc *crtc)
> >
> > DBG("%s", omap_crtc->name);
> >
> > - WARN_ON(omap_crtc->vblank_irq.registered);
> > -
> > drm_crtc_cleanup(crtc);
> >
> > kfree(omap_crtc);
> > @@ -353,14 +348,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc)
> >
> > DBG("%s", omap_crtc->name);
> >
> > + drm_crtc_vblank_on(crtc);
> > + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +
> > spin_lock_irq(&crtc->dev->event_lock);
> > WARN_ON(omap_crtc->pending);
> > omap_crtc->pending = true;
> > spin_unlock_irq(&crtc->dev->event_lock);
> > -
> > - omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
> > -
> > - drm_crtc_vblank_on(crtc);
> > }
> >
> > static void omap_crtc_disable(struct drm_crtc *crtc)
> > @@ -414,8 +408,6 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> > {
> > struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >
> > - WARN_ON(omap_crtc->vblank_irq.registered);
> > -
> > if (crtc->state->color_mgmt_changed) {
> > struct drm_color_lut *lut = NULL;
> > uint length = 0;
> > @@ -441,6 +433,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> >
> > DBG("%s: GO", omap_crtc->name);
> >
> > + dispc_mgr_go(omap_crtc->channel);
> > +
> > + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> I don't like this style very much. I think WARN()s should be without
> side effects.
>
> Also, WARN only gives benefit when we don't know what the call stack is.
> Afaik, there's only one way omap_crtc_atomic_flush can be called, so
> it's just extra spam and dev_err or dev_warn should be enough.
I've used it because the equivalent statements testing omap_crtc-
>vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more vocal,
it really gets the point across. As the function really should not return an
error unless in case of a driver bug, I don't think it will generate any spam.
I don't care too much though, I can replace it with a dev_err() if you insist.
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
