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