From patchwork Mon Jun 8 16:02:28 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomi Valkeinen X-Patchwork-Id: 6566601 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 31F7BC0020 for ; Mon, 8 Jun 2015 16:16:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id AB9BC204E2 for ; Mon, 8 Jun 2015 16:16:49 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 9D0C420544 for ; Mon, 8 Jun 2015 16:16:45 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 77BD36E67E; Mon, 8 Jun 2015 09:16:39 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from bear.ext.ti.com (bear.ext.ti.com [192.94.94.41]) by gabe.freedesktop.org (Postfix) with ESMTP id DCFFE6E65A for ; Mon, 8 Jun 2015 09:02:34 -0700 (PDT) Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by bear.ext.ti.com (8.13.7/8.13.7) with ESMTP id t58G2TGf009622; Mon, 8 Jun 2015 11:02:29 -0500 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id t58G2Tf7009850; Mon, 8 Jun 2015 11:02:29 -0500 Received: from dflp32.itg.ti.com (10.64.6.15) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.3.224.2; Mon, 8 Jun 2015 11:02:29 -0500 Received: from [192.168.2.6] (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp32.itg.ti.com (8.14.3/8.13.8) with ESMTP id t58G2SvJ004645; Mon, 8 Jun 2015 11:02:28 -0500 Message-ID: <5575BC94.4040108@ti.com> Date: Mon, 8 Jun 2015 19:02:28 +0300 From: Tomi Valkeinen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Laurent Pinchart Subject: Re: [PATCHv2 41/45] drm: omapdrm: remove omap_crtc_wait_page_flip References: <1433408582-9828-1-git-send-email-tomi.valkeinen@ti.com> <1433408582-9828-42-git-send-email-tomi.valkeinen@ti.com> <2119555.L6uFpPdt4e@avalon> In-Reply-To: <2119555.L6uFpPdt4e@avalon> X-Mailman-Approved-At: Mon, 08 Jun 2015 09:16:37 -0700 Cc: dri-devel@lists.freedesktop.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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 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 . */ -#include - #include #include #include @@ -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);