Re: [PATCHv2 41/45] drm: omapdrm: remove omap_crtc_wait_page_flip

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

Hi Tomi,

Thank you for the patch.

On Thursday 04 June 2015 12:02:58 Tomi Valkeinen wrote:
> omap_crtc_disable() waits for pending page flips to finish. However,
> omap_crtc_disable() is always called via omap_atomic_complete() and we
> never have page flips going on at that time.

Why is that ? Because our omap_atomic_complete() implementation waits for 
vblanks before allowing the next atomic commit to run, and that our vblank IRQ 
handler completes all pending page flips ? If so, I believe we have a few 
corner cases that won't work properly.

For instance, if the user performs a full mode set on a CRTC without change 
the framebuffer, an event can be queued but 
drm_atomic_helper_wait_for_vblanks() won't wait for vblank on that CRTC as 
framebuffer_changed() will return false. If the next commit then disables the 
CRTC the event might not have completed yet.

> So remove the omap_crtc_wait_page_flip() and related code.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/omap_crtc.c | 32 --------------------------------
>  1 file changed, 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c index abfafd1600b8..2c0d91a67418
> 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -51,7 +51,6 @@ struct omap_crtc {
> 
>  	/* pending event */
>  	struct drm_pending_vblank_event *event;
> -	wait_queue_head_t flip_wait;
> 
>  	struct completion completion;
> 
> @@ -277,41 +276,12 @@ static void omap_crtc_complete_page_flip(struct
> drm_crtc *crtc) else
>  			event->base.destroy(&event->base);
> 
> -		wake_up(&omap_crtc->flip_wait);
>  		drm_crtc_vblank_put(crtc);
>  	}
> 
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  }
> 
> -static bool omap_crtc_page_flip_pending(struct drm_crtc *crtc)
> -{
> -	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> -	struct drm_device *dev = crtc->dev;
> -	unsigned long flags;
> -	bool pending;
> -
> -	spin_lock_irqsave(&dev->event_lock, flags);
> -	pending = omap_crtc->event != NULL;
> -	spin_unlock_irqrestore(&dev->event_lock, flags);
> -
> -	return pending;
> -}
> -
> -static void omap_crtc_wait_page_flip(struct drm_crtc *crtc)
> -{
> -	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> -
> -	if (wait_event_timeout(omap_crtc->flip_wait,
> -			       !omap_crtc_page_flip_pending(crtc),
> -			       msecs_to_jiffies(50)))
> -		return;
> -
> -	dev_warn(crtc->dev->dev, "page flip timeout!\n");
> -
> -	omap_crtc_complete_page_flip(crtc);
> -}
> -
>  static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t
> irqstatus) {
>  	struct omap_crtc *omap_crtc =
> @@ -404,7 +374,6 @@ static void omap_crtc_disable(struct drm_crtc *crtc)
> 
>  	DBG("%s", omap_crtc->name);
> 
> -	omap_crtc_wait_page_flip(crtc);
>  	drm_crtc_vblank_off(crtc);
>  }
> 
> @@ -541,7 +510,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
> 
>  	crtc = &omap_crtc->base;
> 
> -	init_waitqueue_head(&omap_crtc->flip_wait);
>  	init_completion(&omap_crtc->completion);
> 
>  	omap_crtc->channel = channel;

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel