Re: [PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active

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

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@ideasonboard.com>
> > ---
> > 
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel