From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event. Date: Mon, 02 May 2016 17:57:37 -0700 Message-ID: <87wpncot4u.fsf@eliezer.anholt.net> References: <1462217122-13071-1-git-send-email-robert.foss@collabora.com> <1462217122-13071-2-git-send-email-robert.foss@collabora.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1209096304==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 71D586E10A for ; Tue, 3 May 2016 00:57:42 +0000 (UTC) In-Reply-To: <1462217122-13071-2-git-send-email-robert.foss@collabora.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: daniel.vetter@ffwll.ch, airlied@linux.ieaniel.vetter@ffwll.ch, fengguang.wu@intel.com, maarten.lankhorst@linux.intel.com, julia.lawall@lip6.fr, alexander.deucher@amd.com, daniels@collabora.com, derekf@osg.samsung.com, varadgautam@gmail.com Cc: Robert Foss , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1209096304== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable robert.foss@collabora.com writes: > From: Robert Foss > > As per the docs, atomic_commit should return -EBUSY "if an asycnhronous > update is requested and there is an earlier update pending". Note: docs cited here are drm_crtc.h, and the whole quote is: * - -EBUSY, if an asynchronous updated is requested and there is * an earlier updated pending. Drivers are allowed to support a queue * of outstanding updates, but currently no driver supports that. * Note that drivers must wait for preceding updates to complete if a * synchronous update is requested, they are not allowed to fail the * commit in that case. > Signed-off-by: Robert Foss > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++ > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 20 ++++++++++++++++++-- > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crt= c.c > index 355ee4b..43193a3 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver =3D { > .of_match_table =3D vc4_crtc_dt_match, > }, > }; > + > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc) > +{ > + assert_spin_locked(&crtc->dev->event_lock); > + return to_vc4_crtc(crtc)->event; > +} > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index fa2ad15..54c1fb5 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver; > int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id); > void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id); > int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg); > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc); >=20=20 > /* vc4_debugfs.c */ > int vc4_debugfs_init(struct drm_minor *minor); > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 4718ae5..dc157a1e 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev, > bool async) > { > struct vc4_dev *vc4 =3D to_vc4_dev(dev); > - int ret; > - int i; > uint64_t wait_seqno =3D 0; > struct vc4_commit *c; > + struct drm_crtc_state *crtc_state; > + struct drm_crtc *crtc; > + unsigned long flags; > + int i, ret; > + > + if (async) { > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + > + spin_lock_irqsave(&dev->event_lock, flags); > + > + if (crtc->state->event ||=20 > + vc4_crtc_has_pending_event(crtc)) { > + spin_unlock_irqrestore(&dev->event_lock, flags); > + return -EBUSY; > + } > + spin_unlock_irqrestore(&dev->event_lock, flags); > + } > + } By my reading, the point of returning -EBUSY here is just to avoid blocking when the compositor asked for nonblocking. You're checking that each CRTC involved in the commit doesn't have a queued pageflip event, except that then we immediately proceed to: /* Make sure that any outstanding modesets have finished. */ ret =3D down_interruptible(&vc4->async_modeset); if (ret) { kfree(c); return ret; } so this per-CRTC check doesn't prevent blocking if the set of CRTCs for this commit was disjoint from the last one, right? To implement the minimal behavior, I think we'd want to just down_trylock instead in the async case, I think. And to implement the disjoint CRTC thing you were trying for, I think we'd need to track a mask kind of like msm's pending_crtcs. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJXJ/eCAAoJELXWKTbR/J7oEkQQALbJZNItmTyDrALnGOZg0/5C cVkpVNsmuix+n+aW+vxxBZXf2Xx8pj7KdsI+2HswNAeapBKXZ0ihOUa7UIdauKMm M98SP+qHdVq1sE+DGf8G97+woR/gmXx2H8PCLZV6bmOXlsTWnPUPXS8JvPfuJagw vcWwTnpFphBYzMIPBX4wTfWJhh/vpd9Ox0321EBzFcb9QZAuTEedaRVqv9nFopQf yH4jHb0J/DIbeXVgKS/6CNVpyNl9vqguv4Kt3mQ9rEfowtD8FOs82Y/EbbG8ln6/ jyJtKXOB1gHkhmG72Fqot+M6W6dWeLmvzKYTAfoy8v+XKsm0G7jiuOx/utSNY6eE vM3CFBOzM38sPoUJHP6CzsvlsNnzSGPk6EfhnWoI+ege5Us5FNYQIUdXOio6KYZ9 ukZBfpNboFBDpMnEO3Wn8lurDZ4UVfaP96s3VIzMugAUbjt4PO+4AplCEB0bOP6G f0A2c557bqMuQjEdyageVqtuwDRGRtf5yF914DJzYuIas/g42VQ9cmQ+nqO+WjK8 51MvhnJRAOqz5eGVjvDBMQEQpXJm/1evEYXaSF/V34lFCRTCmtB8GKnTH/QXwwy5 Oky6auQlTIVpKkWe9SjaIWGzcCdFxaN/S6GVTmqOz3eV0uZKg5gUna8kb6U8piO5 qs1aFNO4ah5+5RZdOWiT =ElIg -----END PGP SIGNATURE----- --=-=-=-- --===============1209096304== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1209096304==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964822AbcECA6U (ORCPT ); Mon, 2 May 2016 20:58:20 -0400 Received: from anholt.net ([50.246.234.109]:43969 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933822AbcECA5n (ORCPT ); Mon, 2 May 2016 20:57:43 -0400 From: Eric Anholt To: robert.foss@collabora.com, daniel.vetter@ffwll.ch, airlied@linux.ie, aniel.vetter@ffwll.ch, fengguang.wu@intel.com, maarten.lankhorst@linux.intel.com, julia.lawall@lip6.fr, alexander.deucher@amd.com, daniels@collabora.com, derekf@osg.samsung.com, varadgautam@gmail.com Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Robert Foss Subject: Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event. In-Reply-To: <1462217122-13071-2-git-send-email-robert.foss@collabora.com> References: <1462217122-13071-1-git-send-email-robert.foss@collabora.com> <1462217122-13071-2-git-send-email-robert.foss@collabora.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 02 May 2016 17:57:37 -0700 Message-ID: <87wpncot4u.fsf@eliezer.anholt.net> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable robert.foss@collabora.com writes: > From: Robert Foss > > As per the docs, atomic_commit should return -EBUSY "if an asycnhronous > update is requested and there is an earlier update pending". Note: docs cited here are drm_crtc.h, and the whole quote is: * - -EBUSY, if an asynchronous updated is requested and there is * an earlier updated pending. Drivers are allowed to support a queue * of outstanding updates, but currently no driver supports that. * Note that drivers must wait for preceding updates to complete if a * synchronous update is requested, they are not allowed to fail the * commit in that case. > Signed-off-by: Robert Foss > --- > drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++ > drivers/gpu/drm/vc4/vc4_drv.h | 1 + > drivers/gpu/drm/vc4/vc4_kms.c | 20 ++++++++++++++++++-- > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crt= c.c > index 355ee4b..43193a3 100644 > --- a/drivers/gpu/drm/vc4/vc4_crtc.c > +++ b/drivers/gpu/drm/vc4/vc4_crtc.c > @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver =3D { > .of_match_table =3D vc4_crtc_dt_match, > }, > }; > + > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc) > +{ > + assert_spin_locked(&crtc->dev->event_lock); > + return to_vc4_crtc(crtc)->event; > +} > diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h > index fa2ad15..54c1fb5 100644 > --- a/drivers/gpu/drm/vc4/vc4_drv.h > +++ b/drivers/gpu/drm/vc4/vc4_drv.h > @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver; > int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id); > void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id); > int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg); > +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc); >=20=20 > /* vc4_debugfs.c */ > int vc4_debugfs_init(struct drm_minor *minor); > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index 4718ae5..dc157a1e 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev, > bool async) > { > struct vc4_dev *vc4 =3D to_vc4_dev(dev); > - int ret; > - int i; > uint64_t wait_seqno =3D 0; > struct vc4_commit *c; > + struct drm_crtc_state *crtc_state; > + struct drm_crtc *crtc; > + unsigned long flags; > + int i, ret; > + > + if (async) { > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + > + spin_lock_irqsave(&dev->event_lock, flags); > + > + if (crtc->state->event ||=20 > + vc4_crtc_has_pending_event(crtc)) { > + spin_unlock_irqrestore(&dev->event_lock, flags); > + return -EBUSY; > + } > + spin_unlock_irqrestore(&dev->event_lock, flags); > + } > + } By my reading, the point of returning -EBUSY here is just to avoid blocking when the compositor asked for nonblocking. You're checking that each CRTC involved in the commit doesn't have a queued pageflip event, except that then we immediately proceed to: /* Make sure that any outstanding modesets have finished. */ ret =3D down_interruptible(&vc4->async_modeset); if (ret) { kfree(c); return ret; } so this per-CRTC check doesn't prevent blocking if the set of CRTCs for this commit was disjoint from the last one, right? To implement the minimal behavior, I think we'd want to just down_trylock instead in the async case, I think. And to implement the disjoint CRTC thing you were trying for, I think we'd need to track a mask kind of like msm's pending_crtcs. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJXJ/eCAAoJELXWKTbR/J7oEkQQALbJZNItmTyDrALnGOZg0/5C cVkpVNsmuix+n+aW+vxxBZXf2Xx8pj7KdsI+2HswNAeapBKXZ0ihOUa7UIdauKMm M98SP+qHdVq1sE+DGf8G97+woR/gmXx2H8PCLZV6bmOXlsTWnPUPXS8JvPfuJagw vcWwTnpFphBYzMIPBX4wTfWJhh/vpd9Ox0321EBzFcb9QZAuTEedaRVqv9nFopQf yH4jHb0J/DIbeXVgKS/6CNVpyNl9vqguv4Kt3mQ9rEfowtD8FOs82Y/EbbG8ln6/ jyJtKXOB1gHkhmG72Fqot+M6W6dWeLmvzKYTAfoy8v+XKsm0G7jiuOx/utSNY6eE vM3CFBOzM38sPoUJHP6CzsvlsNnzSGPk6EfhnWoI+ege5Us5FNYQIUdXOio6KYZ9 ukZBfpNboFBDpMnEO3Wn8lurDZ4UVfaP96s3VIzMugAUbjt4PO+4AplCEB0bOP6G f0A2c557bqMuQjEdyageVqtuwDRGRtf5yF914DJzYuIas/g42VQ9cmQ+nqO+WjK8 51MvhnJRAOqz5eGVjvDBMQEQpXJm/1evEYXaSF/V34lFCRTCmtB8GKnTH/QXwwy5 Oky6auQlTIVpKkWe9SjaIWGzcCdFxaN/S6GVTmqOz3eV0uZKg5gUna8kb6U8piO5 qs1aFNO4ah5+5RZdOWiT =ElIg -----END PGP SIGNATURE----- --=-=-=--