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

[Tomi Valkeinen]

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 at 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.


>  	/* 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.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/059ad801/attachment.sig>