From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCHv2 41/45] drm: omapdrm: remove omap_crtc_wait_page_flip Date: Mon, 8 Jun 2015 19:02:28 +0300 Message-ID: <5575BC94.4040108@ti.com> References: <1433408582-9828-1-git-send-email-tomi.valkeinen@ti.com> <1433408582-9828-42-git-send-email-tomi.valkeinen@ti.com> <2119555.L6uFpPdt4e@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0070652603==" Return-path: 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) In-Reply-To: <2119555.L6uFpPdt4e@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0070652603== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="60nWu72RGn492MsuHbsCVwTPFCLPivrBd" --60nWu72RGn492MsuHbsCVwTPFCLPivrBd Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 06/06/15 07:09, Laurent Pinchart wrote: > Hi Tomi, >=20 > Thank you for the patch. >=20 > 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. >=20 > Why is that ? Because our omap_atomic_complete() implementation waits f= or=20 > vblanks before allowing the next atomic commit to run, and that our vbl= ank IRQ=20 > handler completes all pending page flips ? If so, I believe we have a f= ew=20 > corner cases that won't work properly. Yes, I think what omap_atomic_complete() is supposed to do is to wait unt= il everything is done for the affected crtcs. But you are right, it wasn't q= uite correct. > For instance, if the user performs a full mode set on a CRTC without ch= ange=20 > the framebuffer, an event can be queued but=20 > drm_atomic_helper_wait_for_vblanks() won't wait for vblank on that CRTC= as=20 > framebuffer_changed() will return false. If the next commit then disabl= es the=20 > 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 i= t for the following days, so I'll post it now. Quick testing went fine. I also pushed it to omapdrm-atomic-v2 branch. =46rom 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 !=3D 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/omapdr= m/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 . */ =20 -#include - #include #include #include @@ -49,13 +47,10 @@ struct omap_crtc { struct omap_drm_irq vblank_irq; struct omap_drm_irq error_irq; =20 - /* 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; }; =20 /* ---------------------------------------------------------------------= -------- @@ -81,6 +76,15 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *c= rtc) return omap_crtc->channel; } =20 +int omap_crtc_wait_pending(struct drm_crtc *crtc) +{ + struct omap_crtc *omap_crtc =3D 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 =3D { * Setup, Flush and Page Flip */ =20 -static void omap_crtc_complete_page_flip(struct drm_crtc *crtc) +static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqst= atus) { - struct omap_crtc *omap_crtc =3D to_omap_crtc(crtc); - struct drm_pending_vblank_event *event; - struct drm_device *dev =3D crtc->dev; - unsigned long flags; - - spin_lock_irqsave(&dev->event_lock, flags); - - event =3D omap_crtc->event; - omap_crtc->event =3D 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 =3D + container_of(irq, struct omap_crtc, error_irq); =20 - wake_up(&omap_crtc->flip_wait); - drm_crtc_vblank_put(crtc); + if (omap_crtc->ignore_digit_sync_lost) { + irqstatus &=3D ~DISPC_IRQ_SYNC_LOST_DIGIT; + if (!irqstatus) + return; } =20 - spin_unlock_irqrestore(&dev->event_lock, flags); + DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus)= ; } =20 -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 =3D to_omap_crtc(crtc); + struct drm_pending_vblank_event *event; struct drm_device *dev =3D crtc->dev; unsigned long flags; - bool pending; =20 - spin_lock_irqsave(&dev->event_lock, flags); - pending =3D omap_crtc->event !=3D NULL; - spin_unlock_irqrestore(&dev->event_lock, flags); + event =3D crtc->state->event; =20 - return pending; -} - -static void omap_crtc_wait_page_flip(struct drm_crtc *crtc) -{ - struct omap_crtc *omap_crtc =3D 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; =20 - dev_warn(crtc->dev->dev, "page flip timeout!\n"); - - omap_crtc_complete_page_flip(crtc); -} + spin_lock_irqsave(&dev->event_lock, flags); =20 -static void omap_crtc_error_irq(struct omap_drm_irq *irq, uint32_t irqst= atus) -{ - struct omap_crtc *omap_crtc =3D - container_of(irq, struct omap_crtc, error_irq); + list_del(&event->base.link); =20 - if (omap_crtc->ignore_digit_sync_lost) { - irqstatus &=3D ~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); =20 - DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus)= ; + spin_unlock_irqrestore(&dev->event_lock, flags); } =20 static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqs= tatus) @@ -336,12 +308,19 @@ static void omap_crtc_vblank_irq(struct omap_drm_ir= q *irq, uint32_t irqstatus) return; =20 DBG("%s: apply done", omap_crtc->name); + __omap_irq_unregister(dev, &omap_crtc->vblank_irq); =20 - /* wakeup userspace */ + mb(); + WARN_ON(!omap_crtc->pending); + omap_crtc->pending =3D false; + mb(); + + /* wake up userspace */ omap_crtc_complete_page_flip(&omap_crtc->base); =20 - complete(&omap_crtc->completion); + /* wake up omap_atomic_complete */ + wake_up(&omap_crtc->pending_wait); } =20 /* ---------------------------------------------------------------------= -------- @@ -375,6 +354,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc) =20 DBG("%s", omap_crtc->name); =20 + WARN_ON(omap_crtc->pending); + + omap_crtc->pending =3D true; + mb(); + + omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); + drm_crtc_vblank_on(crtc); } =20 @@ -384,7 +370,6 @@ static void omap_crtc_disable(struct drm_crtc *crtc) =20 DBG("%s", omap_crtc->name); =20 - omap_crtc_wait_page_flip(crtc); drm_crtc_vblank_off(crtc); } =20 @@ -405,19 +390,7 @@ static void omap_crtc_mode_set_nofb(struct drm_crtc = *crtc) =20 static void omap_crtc_atomic_begin(struct drm_crtc *crtc) { - struct drm_pending_vblank_event *event =3D crtc->state->event; - struct omap_crtc *omap_crtc =3D to_omap_crtc(crtc); - struct drm_device *dev =3D crtc->dev; - unsigned long flags; - - if (event) { - WARN_ON(omap_crtc->event); - WARN_ON(drm_crtc_vblank_get(crtc) !=3D 0); =20 - spin_lock_irqsave(&dev->event_lock, flags); - omap_crtc->event =3D event; - spin_unlock_irqrestore(&dev->event_lock, flags); - } } =20 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 =3D to_omap_crtc(crtc); =20 WARN_ON(omap_crtc->vblank_irq.registered); + WARN_ON(omap_crtc->pending); =20 if (dispc_mgr_is_enabled(omap_crtc->channel)) { DBG("%s: GO", omap_crtc->name); =20 + omap_crtc->pending =3D 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); } =20 crtc->invert_dimensions =3D !!(crtc->primary->state->rotation & @@ -534,8 +507,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *de= v, =20 crtc =3D &omap_crtc->base; =20 - init_waitqueue_head(&omap_crtc->flip_wait); - init_completion(&omap_crtc->completion); + init_waitqueue_head(&omap_crtc->pending_wait); =20 omap_crtc->channel =3D channel; omap_crtc->name =3D 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; }; =20 +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 =3D 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 =3D commit->dev; @@ -79,7 +98,7 @@ static void omap_atomic_complete(struct omap_atomic_sta= te_commit *commit) drm_atomic_helper_commit_planes(dev, old_state); drm_atomic_helper_commit_modeset_enables(dev, old_state); =20 - drm_atomic_helper_wait_for_vblanks(dev, old_state); + omap_atomic_wait_for_gos(dev, old_state); =20 drm_atomic_helper_cleanup_planes(dev, old_state); =20 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); =20 struct drm_plane *omap_plane_init(struct drm_device *dev, int id, enum drm_plane_type type); --60nWu72RGn492MsuHbsCVwTPFCLPivrBd Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVdbyUAAoJEPo9qoy8lh71vQQP/AyKVra2GSQss+Efr9bFmIch 1LpDctSTsI7ejIGLRcCmtsmPmlPezfv3+S5L5b5Wukf5D8AxfS6psfdcFhNQq2cG tYoMA6VRYCg0qh+Pz0EdBzIA2PxZ4b3EibVgNPjWRb6879dUw6l5AI3gZ7H07qRJ qBawnRHaW/6+ZPTyZC0tvKvFgg3tMFybf0eRtr/IDwmJhP8lAygYqLQrCcRb8txf 5Md8XxfE28hJrdACsaWfbwX3gN8wJvFyPuiF4MY7NZEfi6m4c8SCLrydutv107qE xEDi0jbIDPwkdpYAQVPF85v/AmHrdY8LAg4nEHSH17fUpFr5inkUeGBzq0OvZE9Z JD4/R8qn3o198qPRWYbcAVbNuTf49w5h5/myNcLzHXCQdSkbUFBKk7GAiCDaUcRH 5FFIcTfMbeTZ/g0grDMFaxHfHSib1esYlydaK9MGCI5dc69iE1nUyssbotsuVsiC jzKybGVYxwdh42kfHWf4bMKrNHok4K9O/am+Dn+d46RPw7mcgb8oDsGeYS8KGGNh fXWwP2++fZKRYGYkQys7hhQhZw03ICG/WpagpPQkLPgyZRrZD6VJE9hhVFJ4ztzQ e5XFib0GhzutNq+8MyxJdhoyQG/+tEBd99c4Bt0ruDZvZxUkbX0lm3cIpexOdaLi IwqohZkROXhtQXkgrthT =Zlvr -----END PGP SIGNATURE----- --60nWu72RGn492MsuHbsCVwTPFCLPivrBd-- --===============0070652603== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0070652603==--