On 06/06/15 07:09, Laurent Pinchart wrote: > 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. Yes, I think what omap_atomic_complete() is supposed to do is to wait until everything is done for the affected crtcs. But you are right, it wasn't quite correct. > 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. I reworked the event/flip_wait code, removing this patch and 42 and 43. I didn't have time to clean it up properly, but I'm probably not able to work on it for the following days, so I'll post it now. Quick testing went fine. I also pushed it to omapdrm-atomic-v2 branch. From 8d6429616783c335d20bf8ebaf8cc4dc447d0385 Mon Sep 17 00:00:00 2001 From: Tomi Valkeinen <tomi.valkeinen@xxxxxx> Date: Fri, 29 May 2015 16:01:18 +0300 Subject: [PATCH] drm: omapdrm: new vblank and event handling Rework the crtc event/flip_wait system as follows: - If we enable a crtc (full modeset), we set omap_crtc->pending and register vblank irq. - If we need to set GO bit (page flip), we do the same but also set the GO bit. - On vblank we unregister the irq, clear the 'pending' flag, send vblank event to userspace if crtc->state->event != NULL, and wake up 'pending_wait' wq. - In omap_atomic_complete() we wait for the 'pending' flag to get reset for all enabled crtcs using 'pending_wait wq. The above ensures that we send the events to userspace in vblank, and that after the wait in omap_atomic_complete() everything for the affected crtcs has been completed. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 8d2bf8565ddd..9b067d10e8d2 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -17,8 +17,6 @@ * this program. If not, see <http://www.gnu.org/licenses/>. */ -#include <linux/completion.h> - #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -49,13 +47,10 @@ struct omap_crtc { struct omap_drm_irq vblank_irq; struct omap_drm_irq error_irq; - /* pending event */ - struct drm_pending_vblank_event *event; - wait_queue_head_t flip_wait; - - struct completion completion; - bool ignore_digit_sync_lost; + + bool pending; + wait_queue_head_t pending_wait; }; /* ----------------------------------------------------------------------------- @@ -81,6 +76,15 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc) return omap_crtc->channel; } +int omap_crtc_wait_pending(struct drm_crtc *crtc) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + + return wait_event_timeout(omap_crtc->pending_wait, + !omap_crtc->pending, + msecs_to_jiffies(50)); +} + /* ----------------------------------------------------------------------------- * DSS Manager Functions */ @@ -253,77 +257,45 @@ static const struct dss_mgr_ops mgr_ops = { * Setup, Flush and Page Flip */ -static void omap_crtc_complete_page_flip(struct drm_crtc *crtc) +static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus) { - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - struct drm_pending_vblank_event *event; - struct drm_device *dev = crtc->dev; - unsigned long flags; - - spin_lock_irqsave(&dev->event_lock, flags); - - event = omap_crtc->event; - omap_crtc->event = NULL; - - if (event) { - list_del(&event->base.link); - - /* - * Queue the event for delivery if it's still linked to a file - * handle, otherwise just destroy it. - */ - if (event->base.file_priv) - drm_crtc_send_vblank_event(crtc, event); - else - event->base.destroy(&event->base); + struct omap_crtc *omap_crtc = + container_of(irq, struct omap_crtc, error_irq); - wake_up(&omap_crtc->flip_wait); - drm_crtc_vblank_put(crtc); + if (omap_crtc->ignore_digit_sync_lost) { + irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT; + if (!irqstatus) + return; } - spin_unlock_irqrestore(&dev->event_lock, flags); + DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus); } -static bool omap_crtc_page_flip_pending(struct drm_crtc *crtc) +static void omap_crtc_complete_page_flip(struct drm_crtc *crtc) { - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + struct drm_pending_vblank_event *event; 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); + event = crtc->state->event; - 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))) + if (!event) return; - dev_warn(crtc->dev->dev, "page flip timeout!\n"); - - omap_crtc_complete_page_flip(crtc); -} + spin_lock_irqsave(&dev->event_lock, flags); -static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqstatus) -{ - struct omap_crtc *omap_crtc = - container_of(irq, struct omap_crtc, error_irq); + list_del(&event->base.link); - if (omap_crtc->ignore_digit_sync_lost) { - irqstatus &= ~DISPC_IRQ_SYNC_LOST_DIGIT; - if (!irqstatus) - return; - } + /* + * Queue the event for delivery if it's still linked to a file + * handle, otherwise just destroy it. + */ + if (event->base.file_priv) + drm_crtc_send_vblank_event(crtc, event); + else + event->base.destroy(&event->base); - DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus); + spin_unlock_irqrestore(&dev->event_lock, flags); } static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus) @@ -336,12 +308,19 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus) return; DBG("%s: apply done", omap_crtc->name); + __omap_irq_unregister(dev, &omap_crtc->vblank_irq); - /* wakeup userspace */ + mb(); + WARN_ON(!omap_crtc->pending); + omap_crtc->pending = false; + mb(); + + /* wake up userspace */ omap_crtc_complete_page_flip(&omap_crtc->base); - complete(&omap_crtc->completion); + /* wake up omap_atomic_complete */ + wake_up(&omap_crtc->pending_wait); } /* ----------------------------------------------------------------------------- @@ -375,6 +354,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc) DBG("%s", omap_crtc->name); + WARN_ON(omap_crtc->pending); + + omap_crtc->pending = true; + mb(); + + omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); + drm_crtc_vblank_on(crtc); } @@ -384,7 +370,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); } @@ -405,19 +390,7 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc *crtc) static void omap_crtc_atomic_begin(struct drm_crtc *crtc) { - struct drm_pending_vblank_event *event = crtc->state->event; - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - struct drm_device *dev = crtc->dev; - unsigned long flags; - - if (event) { - WARN_ON(omap_crtc->event); - WARN_ON(drm_crtc_vblank_get(crtc) != 0); - spin_lock_irqsave(&dev->event_lock, flags); - omap_crtc->event = event; - spin_unlock_irqrestore(&dev->event_lock, flags); - } } static void omap_crtc_atomic_flush(struct drm_crtc *crtc) @@ -425,16 +398,16 @@ 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); + WARN_ON(omap_crtc->pending); if (dispc_mgr_is_enabled(omap_crtc->channel)) { DBG("%s: GO", omap_crtc->name); + omap_crtc->pending = true; + mb(); + dispc_mgr_go(omap_crtc->channel); omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); - - WARN_ON(!wait_for_completion_timeout(&omap_crtc->completion, - msecs_to_jiffies(100))); - reinit_completion(&omap_crtc->completion); } crtc->invert_dimensions = !!(crtc->primary->state->rotation & @@ -534,8 +507,7 @@ 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); + init_waitqueue_head(&omap_crtc->pending_wait); omap_crtc->channel = channel; omap_crtc->name = channel_names[channel]; diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 50f555530e55..e8f4001d9d31 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -66,6 +66,25 @@ struct omap_atomic_state_commit { u32 crtcs; }; +static void omap_atomic_wait_for_gos(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; + int i, ret; + + for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + if (!crtc->state->enable) + continue; + + ret = omap_crtc_wait_pending(crtc); + + if (!ret) + dev_warn(dev->dev, + "atomic flush timeout (pipe %u)!\n", i); + } +} + static void omap_atomic_complete(struct omap_atomic_state_commit *commit) { struct drm_device *dev = commit->dev; @@ -79,7 +98,7 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit) drm_atomic_helper_commit_planes(dev, old_state); drm_atomic_helper_commit_modeset_enables(dev, old_state); - drm_atomic_helper_wait_for_vblanks(dev, old_state); + omap_atomic_wait_for_gos(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 0b7a055bf007..ae2df41f216f 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -147,6 +147,7 @@ void omap_crtc_pre_init(void); void omap_crtc_pre_uninit(void); struct drm_crtc *omap_crtc_init(struct drm_device *dev, struct drm_plane *plane, enum omap_channel channel, int id); +int omap_crtc_wait_pending(struct drm_crtc *crtc); struct drm_plane *omap_plane_init(struct drm_device *dev, int id, enum drm_plane_type type);
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
