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: Tue, 03 May 2016 10:11:56 -0700 Message-ID: <877ffbqd5v.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> <87wpncot4u.fsf@eliezer.anholt.net> <5728B39E.3070104@collabora.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0972363209==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 225496E18B for ; Tue, 3 May 2016 17:12:01 +0000 (UTC) In-Reply-To: <5728B39E.3070104@collabora.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Robert Foss , 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, linux-kernel@vger.kernel.orgmaarten.lankhorst@linux.intel.com Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0972363209== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Robert Foss writes: > On 05/02/2016 08:57 PM, Eric Anholt wrote: >> 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_c= rtc.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_dr= v.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); >>> >>> /* 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_km= s.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 *d= ev, >>> 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 || >>> + 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. >> > > So what you're saying is that the set of CRTCs in state > might not contain all CRTCs and that this check might be incomplete for=20 > that reason? > > I'm fairly new to these waters, so don't hesitate to tell me if I seem > to be misunderstanding something or am on a wild goose chase of some sort. > > So you're suggesting something like this instead of > the per CRTC check: > > - /* Make sure that any outstanding modesets have finished. */ > - ret =3D down_interruptible(&vc4->async_modeset); > - if (ret) { > - kfree(c); > - return ret; > - } > + /* Make sure that any outstanding modesets have finished. */ > + ret =3D down_trylock(&vc4->async_modeset); > + if (ret) { > + kfree(c); > + return -EBUSY; > + } > > I've quickly tested the above patch and it does seem to work and fulfill= =20 > all of my requirements. Note that you'd want to trylock only in the async case. In the sync case, you'd still want to block. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJXKNvdAAoJELXWKTbR/J7oPSwP/isIjkWAv3ZLndGVDSJhwV5c dfbcqWOhJgoIUpDhD2OVCqG65kQL00Jd42L9Mx3argCtEcsvmdYNh2T9iATg6LZG J0BypvuI4T0m5zWyuf1E97uFAzxBZu8OYtlkYwVKDK4B88GJ2WRP82TN0b/dYc52 T7QEncLiHuVRCN+JukY3IBhcSDTXzoqwWr65B4XketfJMblzhiOq3BgXQiACgGvm T2exBRpWAUaMl8urog+PcWX5RjUMP/nJMG4sJvr6AhIhjWrrkNfOHaWQOIMt3FVz pZxoSoMkSaFJwHj4xWH7f4zYebuA0ZPqSMDmKWQwBE0ZZvhnoNZXMoavH6VDPDFU PDiMcrAim9sevgx8BmlhMMRR8geD3wNc07xPEKg09Kab2Xw8fGjik3HQTJOB8BOR ch89zICSiMwSZ7X5F0pCPcwKUp6e2MBHDYBhbyLvp6lpUEs/KfgFoyXj+wzuYu0K 9AMb9IFCLNPFGZy8OFcMCJI2kkiJBt7Z+nbWJaKyLAB31CpNGLbeHlfKSRZ4dAt9 VdNpGPy6aoFDjv5pe3AqKTXUvMqEQ0nTfAv+j8t3cLWmam9hpyZDw4D19I3LzXRb j06DcQUGZNeIEc/qsqcW/GZpQEzi1YNhDY80tFKxpwfQNpamhVDO55rT0IEUGQKZ jil+sgNQS3nhw8qc4l/N =Ele+ -----END PGP SIGNATURE----- --=-=-=-- --===============0972363209== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0972363209==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934172AbcECRMF (ORCPT ); Tue, 3 May 2016 13:12:05 -0400 Received: from anholt.net ([50.246.234.109]:46810 "EHLO anholt.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932593AbcECRMB (ORCPT ); Tue, 3 May 2016 13:12:01 -0400 From: Eric Anholt To: Robert Foss , 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, linux-kernel@vger.kernel.org, maarten.lankhorst@linux.intel.com Cc: dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event. In-Reply-To: <5728B39E.3070104@collabora.com> References: <1462217122-13071-1-git-send-email-robert.foss@collabora.com> <1462217122-13071-2-git-send-email-robert.foss@collabora.com> <87wpncot4u.fsf@eliezer.anholt.net> <5728B39E.3070104@collabora.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Tue, 03 May 2016 10:11:56 -0700 Message-ID: <877ffbqd5v.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 writes: > On 05/02/2016 08:57 PM, Eric Anholt wrote: >> 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_c= rtc.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_dr= v.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); >>> >>> /* 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_km= s.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 *d= ev, >>> 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 || >>> + 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. >> > > So what you're saying is that the set of CRTCs in state > might not contain all CRTCs and that this check might be incomplete for=20 > that reason? > > I'm fairly new to these waters, so don't hesitate to tell me if I seem > to be misunderstanding something or am on a wild goose chase of some sort. > > So you're suggesting something like this instead of > the per CRTC check: > > - /* Make sure that any outstanding modesets have finished. */ > - ret =3D down_interruptible(&vc4->async_modeset); > - if (ret) { > - kfree(c); > - return ret; > - } > + /* Make sure that any outstanding modesets have finished. */ > + ret =3D down_trylock(&vc4->async_modeset); > + if (ret) { > + kfree(c); > + return -EBUSY; > + } > > I've quickly tested the above patch and it does seem to work and fulfill= =20 > all of my requirements. Note that you'd want to trylock only in the async case. In the sync case, you'd still want to block. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBCgAGBQJXKNvdAAoJELXWKTbR/J7oPSwP/isIjkWAv3ZLndGVDSJhwV5c dfbcqWOhJgoIUpDhD2OVCqG65kQL00Jd42L9Mx3argCtEcsvmdYNh2T9iATg6LZG J0BypvuI4T0m5zWyuf1E97uFAzxBZu8OYtlkYwVKDK4B88GJ2WRP82TN0b/dYc52 T7QEncLiHuVRCN+JukY3IBhcSDTXzoqwWr65B4XketfJMblzhiOq3BgXQiACgGvm T2exBRpWAUaMl8urog+PcWX5RjUMP/nJMG4sJvr6AhIhjWrrkNfOHaWQOIMt3FVz pZxoSoMkSaFJwHj4xWH7f4zYebuA0ZPqSMDmKWQwBE0ZZvhnoNZXMoavH6VDPDFU PDiMcrAim9sevgx8BmlhMMRR8geD3wNc07xPEKg09Kab2Xw8fGjik3HQTJOB8BOR ch89zICSiMwSZ7X5F0pCPcwKUp6e2MBHDYBhbyLvp6lpUEs/KfgFoyXj+wzuYu0K 9AMb9IFCLNPFGZy8OFcMCJI2kkiJBt7Z+nbWJaKyLAB31CpNGLbeHlfKSRZ4dAt9 VdNpGPy6aoFDjv5pe3AqKTXUvMqEQ0nTfAv+j8t3cLWmam9hpyZDw4D19I3LzXRb j06DcQUGZNeIEc/qsqcW/GZpQEzi1YNhDY80tFKxpwfQNpamhVDO55rT0IEUGQKZ jil+sgNQS3nhw8qc4l/N =Ele+ -----END PGP SIGNATURE----- --=-=-=--