* [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
@ 2020-09-25 8:46 Daniel Vetter
2020-09-25 8:46 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
2021-11-05 20:47 ` [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Kazlauskas, Nicholas
0 siblings, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-09-25 8:46 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development, Pekka Paalanen
When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
pull in arbitrary other resources, including CRTCs (e.g. when
reconfiguring global resources).
But in nonblocking mode userspace has then no idea this happened,
which can lead to spurious EBUSY calls, both:
- when that other CRTC is currently busy doing a page_flip the
ALLOW_MODESET commit can fail with an EBUSY
- on the other CRTC a normal atomic flip can fail with EBUSY because
of the additional commit inserted by the kernel without userspace's
knowledge
For blocking commits this isn't a problem, because everyone else will
just block until all the CRTC are reconfigured. Only thing userspace
can notice is the dropped frames without any reason for why frames got
dropped.
Consensus is that we need new uapi to handle this properly, but no one
has any idea what exactly the new uapi should look like. Since this
has been shipping for years already compositors need to deal no matter
what, so as a first step just try to enforce this across drivers
better with some checks.
v2: Add comments and a WARN_ON to enforce this only when allowed - we
don't want to silently convert page flips into blocking plane updates
just because the driver is buggy.
v3: Fix inverted WARN_ON (Pekka).
v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
rules for drivers.
v5: Make the WARNING more informative (Daniel)
v6: Add unconditional debug output for compositor hackers to figure
out what's going on when they get an EBUSY (Daniel)
v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Simon Ser <contact@emersion.fr>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 58527f151984..aac9122f1da2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
* needed. It will also grab the relevant CRTC lock to make sure that the state
* is consistent.
*
+ * WARNING: Drivers may only add new CRTC states to a @state if
+ * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
+ * not created by userspace through an IOCTL call.
+ *
* Returns:
*
* Either the allocated state or the error code encoded into the pointer. When
@@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
struct drm_crtc_state *new_crtc_state;
struct drm_connector *conn;
struct drm_connector_state *conn_state;
+ unsigned requested_crtc = 0;
+ unsigned affected_crtc = 0;
int i, ret = 0;
DRM_DEBUG_ATOMIC("checking %p\n", state);
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+ requested_crtc |= drm_crtc_mask(crtc);
+
for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
if (ret) {
@@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
}
}
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
+ affected_crtc |= drm_crtc_mask(crtc);
+
+ /*
+ * For commits that allow modesets drivers can add other CRTCs to the
+ * atomic commit, e.g. when they need to reallocate global resources.
+ * This can cause spurious EBUSY, which robs compositors of a very
+ * effective sanity check for their drawing loop. Therefor only allow
+ * drivers to add unrelated CRTC states for modeset commits.
+ *
+ * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
+ * so compositors know what's going on.
+ */
+ if (affected_crtc != requested_crtc) {
+ DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
+ requested_crtc, affected_crtc);
+ WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
+ requested_crtc, affected_crtc);
+ }
+
return 0;
}
EXPORT_SYMBOL(drm_atomic_check_only);
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-25 8:46 [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Daniel Vetter
@ 2020-09-25 8:46 ` Daniel Vetter
2020-09-25 9:11 ` Pekka Paalanen
2020-09-29 15:48 ` Daniel Stone
2021-11-05 20:47 ` [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Kazlauskas, Nicholas
1 sibling, 2 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-09-25 8:46 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development,
Sean Paul, Pekka Paalanen
Hopefully we'll have the drm crash recorder RSN, but meanwhile
compositors would like to know a bit better why they get an EBUSY.
v2: Move misplaced hunk to the right patch (Pekka)
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Simon Ser <contact@emersion.fr>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_atomic_helper.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e8abaaaa7fd1..6b3bfabac26c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1740,8 +1740,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
* overridden by a previous synchronous update's state.
*/
if (old_plane_state->commit &&
- !try_wait_for_completion(&old_plane_state->commit->hw_done))
+ !try_wait_for_completion(&old_plane_state->commit->hw_done)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] inflight previous commit preventing async commit\n",
+ plane->base.id, plane->name);
return -EBUSY;
+ }
return funcs->atomic_async_check(plane, new_plane_state);
}
@@ -1964,6 +1967,9 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
* commit with nonblocking ones. */
if (!completed && nonblock) {
spin_unlock(&crtc->commit_lock);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a previous commit\n",
+ crtc->base.id, crtc->name);
+
return -EBUSY;
}
} else if (i == 1) {
@@ -2132,8 +2138,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
/* Userspace is not allowed to get ahead of the previous
* commit with nonblocking ones. */
if (nonblock && old_conn_state->commit &&
- !try_wait_for_completion(&old_conn_state->commit->flip_done))
+ !try_wait_for_completion(&old_conn_state->commit->flip_done)) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a previous commit\n",
+ conn->base.id, conn->name);
+
return -EBUSY;
+ }
/* Always track connectors explicitly for e.g. link retraining. */
commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);
@@ -2147,8 +2157,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
/* Userspace is not allowed to get ahead of the previous
* commit with nonblocking ones. */
if (nonblock && old_plane_state->commit &&
- !try_wait_for_completion(&old_plane_state->commit->flip_done))
+ !try_wait_for_completion(&old_plane_state->commit->flip_done)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous commit\n",
+ plane->base.id, plane->name);
+
return -EBUSY;
+ }
/* Always track planes explicitly for async pageflip support. */
commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-25 8:46 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
@ 2020-09-25 9:11 ` Pekka Paalanen
2020-09-29 15:48 ` Daniel Stone
1 sibling, 0 replies; 11+ messages in thread
From: Pekka Paalanen @ 2020-09-25 9:11 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Sean Paul, DRI Development
[-- Attachment #1.1: Type: text/plain, Size: 378 bytes --]
On Fri, 25 Sep 2020 10:46:51 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hopefully we'll have the drm crash recorder RSN, but meanwhile
> compositors would like to know a bit better why they get an EBUSY.
>
> v2: Move misplaced hunk to the right patch (Pekka)
Hi,
both patches
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Thanks,
pq
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-25 8:46 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
2020-09-25 9:11 ` Pekka Paalanen
@ 2020-09-29 15:48 ` Daniel Stone
2020-10-08 9:31 ` Daniel Vetter
1 sibling, 1 reply; 11+ messages in thread
From: Daniel Stone @ 2020-09-29 15:48 UTC (permalink / raw)
To: Daniel Vetter
Cc: Intel Graphics Development, DRI Development, Sean Paul,
Daniel Vetter, Pekka Paalanen
Hi,
On Fri, 25 Sep 2020 at 09:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hopefully we'll have the drm crash recorder RSN, but meanwhile
> compositors would like to know a bit better why they get an EBUSY.
Thanks a lot, this is super helpful! Both patches are:
Reviewed-by: Daniel Stone <daniels@collabora.com>
Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-29 15:48 ` Daniel Stone
@ 2020-10-08 9:31 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2020-10-08 9:31 UTC (permalink / raw)
To: Daniel Stone
Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development,
DRI Development, Sean Paul, Pekka Paalanen
On Tue, Sep 29, 2020 at 04:48:39PM +0100, Daniel Stone wrote:
> Hi,
>
> On Fri, 25 Sep 2020 at 09:46, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Hopefully we'll have the drm crash recorder RSN, but meanwhile
> > compositors would like to know a bit better why they get an EBUSY.
>
> Thanks a lot, this is super helpful! Both patches are:
> Reviewed-by: Daniel Stone <daniels@collabora.com>
Ok, both patches queued up for 5.11, let's see what gives :-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
2020-09-25 8:46 [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Daniel Vetter
2020-09-25 8:46 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
@ 2021-11-05 20:47 ` Kazlauskas, Nicholas
2021-11-08 15:07 ` Daniel Vetter
1 sibling, 1 reply; 11+ messages in thread
From: Kazlauskas, Nicholas @ 2021-11-05 20:47 UTC (permalink / raw)
To: Daniel Vetter, DRI Development
Cc: Intel Graphics Development, Pekka Paalanen, Daniel Vetter
Hi Daniel,
Just got bitten by this warning when trying to do some refactoring in
amdgpu for trying to get rid of the DRM private object we use for our DC
state.
From a userspace perspective I understand that we want to avoid judder,
-EBUSY and other issues affecting the compositor from kernel having to
drag these CRTCs (or their planes) into the atomic state.
For bandwidth validation we need to understand the state of all CRTCs
and planes in use. Existing driver code maintains this as part of a
global state object in a DRM private atomic state. We have stalls in
atomic check (bad) to avoid freeing this state or modifying it at the
wrong times which avoid hitting this warning but essentially cause the
same judder issue.
While most hardware has independent pipes, I think almost all hardware
ends up having the memory interface/bandwidth as a global shared
resource that software state can't really abstract around.
There are cases where we know that there will be no (or minimal) impact
to the overall memory requirements for particular DRM updates. Our
validation already "over-allocates" for common display changes - page
flips, some format changes, cursor enable/disable. But for most cases
outside of that we do want to pull in _all_ the CRTCs and planes.
On our HW you won't get a blankout unless you're actually modifying a
stream timing itself so I think the ALLOW_MODESET flag is overkill here.
Rejecting the commit when the flag isn't set also ends up breaking
userspace in the process since it expects commits like pageflips between
different tiling modes to succeed with the legacy IOCTLs.
Any ideas about this? I missed the IRC discussion regarding this before
so I'm not sure if we have any alternatives that were dropped in favor
of this.
Regards,
Nicholas Kazlauskas
On 2020-09-25 4:46 a.m., Daniel Vetter wrote:
> When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> pull in arbitrary other resources, including CRTCs (e.g. when
> reconfiguring global resources).
>
> But in nonblocking mode userspace has then no idea this happened,
> which can lead to spurious EBUSY calls, both:
> - when that other CRTC is currently busy doing a page_flip the
> ALLOW_MODESET commit can fail with an EBUSY
> - on the other CRTC a normal atomic flip can fail with EBUSY because
> of the additional commit inserted by the kernel without userspace's
> knowledge
>
> For blocking commits this isn't a problem, because everyone else will
> just block until all the CRTC are reconfigured. Only thing userspace
> can notice is the dropped frames without any reason for why frames got
> dropped.
>
> Consensus is that we need new uapi to handle this properly, but no one
> has any idea what exactly the new uapi should look like. Since this
> has been shipping for years already compositors need to deal no matter
> what, so as a first step just try to enforce this across drivers
> better with some checks.
>
> v2: Add comments and a WARN_ON to enforce this only when allowed - we
> don't want to silently convert page flips into blocking plane updates
> just because the driver is buggy.
>
> v3: Fix inverted WARN_ON (Pekka).
>
> v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
> rules for drivers.
>
> v5: Make the WARNING more informative (Daniel)
>
> v6: Add unconditional debug output for compositor hackers to figure
> out what's going on when they get an EBUSY (Daniel)
>
> v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
>
> References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
> Bugzilla: https://gitlab.freedesktop.org/wayland/weston/-/issues/24#note_9568
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 58527f151984..aac9122f1da2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
> * needed. It will also grab the relevant CRTC lock to make sure that the state
> * is consistent.
> *
> + * WARNING: Drivers may only add new CRTC states to a @state if
> + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
> + * not created by userspace through an IOCTL call.
> + *
> * Returns:
> *
> * Either the allocated state or the error code encoded into the pointer. When
> @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> struct drm_crtc_state *new_crtc_state;
> struct drm_connector *conn;
> struct drm_connector_state *conn_state;
> + unsigned requested_crtc = 0;
> + unsigned affected_crtc = 0;
> int i, ret = 0;
>
> DRM_DEBUG_ATOMIC("checking %p\n", state);
>
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> + requested_crtc |= drm_crtc_mask(crtc);
> +
> for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> if (ret) {
> @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> }
> }
>
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> + affected_crtc |= drm_crtc_mask(crtc);
> +
> + /*
> + * For commits that allow modesets drivers can add other CRTCs to the
> + * atomic commit, e.g. when they need to reallocate global resources.
> + * This can cause spurious EBUSY, which robs compositors of a very
> + * effective sanity check for their drawing loop. Therefor only allow
> + * drivers to add unrelated CRTC states for modeset commits.
> + *
> + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
> + * so compositors know what's going on.
> + */
> + if (affected_crtc != requested_crtc) {
> + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
> + requested_crtc, affected_crtc);
> + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
> + requested_crtc, affected_crtc);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_atomic_check_only);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
2021-11-05 20:47 ` [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Kazlauskas, Nicholas
@ 2021-11-08 15:07 ` Daniel Vetter
2021-11-08 15:32 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2021-11-08 15:07 UTC (permalink / raw)
To: Kazlauskas, Nicholas
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter, Pekka Paalanen
On Fri, Nov 05, 2021 at 04:47:29PM -0400, Kazlauskas, Nicholas wrote:
> Hi Daniel,
>
> Just got bitten by this warning when trying to do some refactoring in amdgpu
> for trying to get rid of the DRM private object we use for our DC state.
>
> From a userspace perspective I understand that we want to avoid judder,
> -EBUSY and other issues affecting the compositor from kernel having to drag
> these CRTCs (or their planes) into the atomic state.
>
> For bandwidth validation we need to understand the state of all CRTCs and
> planes in use. Existing driver code maintains this as part of a global state
> object in a DRM private atomic state. We have stalls in atomic check (bad)
> to avoid freeing this state or modifying it at the wrong times which avoid
> hitting this warning but essentially cause the same judder issue.
>
> While most hardware has independent pipes, I think almost all hardware ends
> up having the memory interface/bandwidth as a global shared resource that
> software state can't really abstract around.
>
> There are cases where we know that there will be no (or minimal) impact to
> the overall memory requirements for particular DRM updates. Our validation
> already "over-allocates" for common display changes - page flips, some
> format changes, cursor enable/disable. But for most cases outside of that we
> do want to pull in _all_ the CRTCs and planes.
>
> On our HW you won't get a blankout unless you're actually modifying a stream
> timing itself so I think the ALLOW_MODESET flag is overkill here.
>
> Rejecting the commit when the flag isn't set also ends up breaking userspace
> in the process since it expects commits like pageflips between different
> tiling modes to succeed with the legacy IOCTLs.
>
> Any ideas about this? I missed the IRC discussion regarding this before so
> I'm not sure if we have any alternatives that were dropped in favor of this.
We have, I while ago I had a lengthy discussion with I think Maxime about
how this is done properly. I think Maxime volunteered to type up some docs
even. tldr;
- Have some global state for this stuff (using the private state helper
stuff ideally, handrolling very much not adviced, might even make sense
to put some of your dc state/structs in there)
- Both the crtc/plane and the global state hold copies of your limits. The
global state in addition also holds the current settings for your
derived values (like clocks or allocations or whatever you have).
- Rules are that only grabbing the global state is enough to read all your
crtc/plane requirements (since you have a read-only copy of that in your
global state). Only for changing them do you also have to have the
corresponding crtc/plane state. Furthermore you _only_ grab the global
objects if the commit reasonably changes your local requirements (i.e.
more planes or whatever).
- You might need to have a cascade here (plane -> crtc and crct -> global
state), and/or maybe multiple different global states. Don't try to put
everything into one, it's better to have a separate private state
handling for each separate thing, at least generally.
- No more "we have to take all objects all the time, always", so no more
over-locking and over-sync.
- Integrating this into DC is probably going to be "fun". Could be that
you first need to convert a pile of the dc_ structs into driver private
state stuff, similar in spirit to the refactor we've done for plane/crtc
state before DC was landed.
Pls check with Maxime that this is documented somewhere as implementation
pattern (it's really common, and your aproach of "grab all crtc/plane" is
the really common wrong approach). And ofc happy to discuss how to best
solve this for DC, but maybe irc is better for that.
Cheers, Daniel
>
> Regards,
> Nicholas Kazlauskas
>
> On 2020-09-25 4:46 a.m., Daniel Vetter wrote:
> > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > pull in arbitrary other resources, including CRTCs (e.g. when
> > reconfiguring global resources).
> >
> > But in nonblocking mode userspace has then no idea this happened,
> > which can lead to spurious EBUSY calls, both:
> > - when that other CRTC is currently busy doing a page_flip the
> > ALLOW_MODESET commit can fail with an EBUSY
> > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > of the additional commit inserted by the kernel without userspace's
> > knowledge
> >
> > For blocking commits this isn't a problem, because everyone else will
> > just block until all the CRTC are reconfigured. Only thing userspace
> > can notice is the dropped frames without any reason for why frames got
> > dropped.
> >
> > Consensus is that we need new uapi to handle this properly, but no one
> > has any idea what exactly the new uapi should look like. Since this
> > has been shipping for years already compositors need to deal no matter
> > what, so as a first step just try to enforce this across drivers
> > better with some checks.
> >
> > v2: Add comments and a WARN_ON to enforce this only when allowed - we
> > don't want to silently convert page flips into blocking plane updates
> > just because the driver is buggy.
> >
> > v3: Fix inverted WARN_ON (Pekka).
> >
> > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
> > rules for drivers.
> >
> > v5: Make the WARNING more informative (Daniel)
> >
> > v6: Add unconditional debug output for compositor hackers to figure
> > out what's going on when they get an EBUSY (Daniel)
> >
> > v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
> >
> > References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html
> > Bugzilla: https://gitlab.freedesktop.org/wayland/weston/-/issues/24#note_9568
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> > Cc: Simon Ser <contact@emersion.fr>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++
> > 1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 58527f151984..aac9122f1da2 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
> > * needed. It will also grab the relevant CRTC lock to make sure that the state
> > * is consistent.
> > *
> > + * WARNING: Drivers may only add new CRTC states to a @state if
> > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
> > + * not created by userspace through an IOCTL call.
> > + *
> > * Returns:
> > *
> > * Either the allocated state or the error code encoded into the pointer. When
> > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > struct drm_crtc_state *new_crtc_state;
> > struct drm_connector *conn;
> > struct drm_connector_state *conn_state;
> > + unsigned requested_crtc = 0;
> > + unsigned affected_crtc = 0;
> > int i, ret = 0;
> > DRM_DEBUG_ATOMIC("checking %p\n", state);
> > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > + requested_crtc |= drm_crtc_mask(crtc);
> > +
> > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> > ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> > if (ret) {
> > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > }
> > }
> > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > + affected_crtc |= drm_crtc_mask(crtc);
> > +
> > + /*
> > + * For commits that allow modesets drivers can add other CRTCs to the
> > + * atomic commit, e.g. when they need to reallocate global resources.
> > + * This can cause spurious EBUSY, which robs compositors of a very
> > + * effective sanity check for their drawing loop. Therefor only allow
> > + * drivers to add unrelated CRTC states for modeset commits.
> > + *
> > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
> > + * so compositors know what's going on.
> > + */
> > + if (affected_crtc != requested_crtc) {
> > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
> > + requested_crtc, affected_crtc);
> > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
> > + requested_crtc, affected_crtc);
> > + }
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL(drm_atomic_check_only);
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
2021-11-08 15:07 ` Daniel Vetter
@ 2021-11-08 15:32 ` Kazlauskas, Nicholas
2021-11-08 15:41 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Kazlauskas, Nicholas @ 2021-11-08 15:32 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter, Pekka Paalanen
On 2021-11-08 10:07 a.m., Daniel Vetter wrote:
> On Fri, Nov 05, 2021 at 04:47:29PM -0400, Kazlauskas, Nicholas wrote:
>> Hi Daniel,
>>
>> Just got bitten by this warning when trying to do some refactoring in amdgpu
>> for trying to get rid of the DRM private object we use for our DC state.
>>
>> From a userspace perspective I understand that we want to avoid judder,
>> -EBUSY and other issues affecting the compositor from kernel having to drag
>> these CRTCs (or their planes) into the atomic state.
>>
>> For bandwidth validation we need to understand the state of all CRTCs and
>> planes in use. Existing driver code maintains this as part of a global state
>> object in a DRM private atomic state. We have stalls in atomic check (bad)
>> to avoid freeing this state or modifying it at the wrong times which avoid
>> hitting this warning but essentially cause the same judder issue.
>>
>> While most hardware has independent pipes, I think almost all hardware ends
>> up having the memory interface/bandwidth as a global shared resource that
>> software state can't really abstract around.
>>
>> There are cases where we know that there will be no (or minimal) impact to
>> the overall memory requirements for particular DRM updates. Our validation
>> already "over-allocates" for common display changes - page flips, some
>> format changes, cursor enable/disable. But for most cases outside of that we
>> do want to pull in _all_ the CRTCs and planes.
>>
>> On our HW you won't get a blankout unless you're actually modifying a stream
>> timing itself so I think the ALLOW_MODESET flag is overkill here.
>>
>> Rejecting the commit when the flag isn't set also ends up breaking userspace
>> in the process since it expects commits like pageflips between different
>> tiling modes to succeed with the legacy IOCTLs.
>>
>> Any ideas about this? I missed the IRC discussion regarding this before so
>> I'm not sure if we have any alternatives that were dropped in favor of this.
>
> We have, I while ago I had a lengthy discussion with I think Maxime about
> how this is done properly. I think Maxime volunteered to type up some docs
> even. tldr;
>
> - Have some global state for this stuff (using the private state helper
> stuff ideally, handrolling very much not adviced, might even make sense
> to put some of your dc state/structs in there)
This is what we do today, but DRM private objects have no
synchronization like CRTCs do. Any time a commit modifies our bandwidth
calculations we need to propagate those changes back up to the global state.
Read only access to the global state can be concurrent, but as soon as
anything needs to write and update it then we need to force ordering.
The bug we had before was that competing commits could complete in a
different order than they would be programmed to HW if they were
non-blocking.
>
> - Both the crtc/plane and the global state hold copies of your limits. The
> global state in addition also holds the current settings for your
> derived values (like clocks or allocations or whatever you have).
There are per pipe limits but there are also global limits on the whole
system.
For example, you may pass validation for a configuration applied to a
single CRTC and plane alone. But if you were to run 4 CRTCs using that
same valid configuration it would fail.
The resource in question here isn't a pipe, an encoder, or something of
that nature - it's just the memory bandwidth available on the system.
On an APU you wouldn't have enough memory bandwidth to drive multiple
CRTCs at "extreme" resolutions or refresh rates.
It also doesn't seem right to block the user from running any display at
these modes just because you can't enable 4 identical displays at the
same time.
>
> - Rules are that only grabbing the global state is enough to read all your
> crtc/plane requirements (since you have a read-only copy of that in your
> global state). Only for changing them do you also have to have the
> corresponding crtc/plane state. Furthermore you _only_ grab the global
> objects if the commit reasonably changes your local requirements (i.e.
> more planes or whatever).
>
> - You might need to have a cascade here (plane -> crtc and crct -> global
> state), and/or maybe multiple different global states. Don't try to put
> everything into one, it's better to have a separate private state
> handling for each separate thing, at least generally.
>
> - No more "we have to take all objects all the time, always", so no more
> over-locking and over-sync.
>
> - Integrating this into DC is probably going to be "fun". Could be that
> you first need to convert a pile of the dc_ structs into driver private
> state stuff, similar in spirit to the refactor we've done for plane/crtc
> state before DC was landed.
...so while we can do CRTC and plane validation it doesn't seem like
there's a good place for that global system level validation that we
need to happen.
>
> Pls check with Maxime that this is documented somewhere as implementation
> pattern (it's really common, and your aproach of "grab all crtc/plane" is
> the really common wrong approach). And ofc happy to discuss how to best
> solve this for DC, but maybe irc is better for that.
>
> Cheers, Daniel
I can try and hop on IRC sometime to chat about this with you and the
others.
Thanks for the response!
Regards,
Nicholas Kazlauskas
>
>>
>> Regards,
>> Nicholas Kazlauskas
>>
>> On 2020-09-25 4:46 a.m., Daniel Vetter wrote:
>>> When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
>>> pull in arbitrary other resources, including CRTCs (e.g. when
>>> reconfiguring global resources).
>>>
>>> But in nonblocking mode userspace has then no idea this happened,
>>> which can lead to spurious EBUSY calls, both:
>>> - when that other CRTC is currently busy doing a page_flip the
>>> ALLOW_MODESET commit can fail with an EBUSY
>>> - on the other CRTC a normal atomic flip can fail with EBUSY because
>>> of the additional commit inserted by the kernel without userspace's
>>> knowledge
>>>
>>> For blocking commits this isn't a problem, because everyone else will
>>> just block until all the CRTC are reconfigured. Only thing userspace
>>> can notice is the dropped frames without any reason for why frames got
>>> dropped.
>>>
>>> Consensus is that we need new uapi to handle this properly, but no one
>>> has any idea what exactly the new uapi should look like. Since this
>>> has been shipping for years already compositors need to deal no matter
>>> what, so as a first step just try to enforce this across drivers
>>> better with some checks.
>>>
>>> v2: Add comments and a WARN_ON to enforce this only when allowed - we
>>> don't want to silently convert page flips into blocking plane updates
>>> just because the driver is buggy.
>>>
>>> v3: Fix inverted WARN_ON (Pekka).
>>>
>>> v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
>>> rules for drivers.
>>>
>>> v5: Make the WARNING more informative (Daniel)
>>>
>>> v6: Add unconditional debug output for compositor hackers to figure
>>> out what's going on when they get an EBUSY (Daniel)
>>>
>>> v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
>>>
>>> References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2018-July%2F182281.html&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zmfJd7M5ZnI5if%2FHM%2FYnBQdoLsWtfjwQW%2BaX0R6Hrj0%3D&reserved=0
>>> Bugzilla: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fwayland%2Fweston%2F-%2Fissues%2F24%23note_9568&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ynHonfkaMn%2BC%2BsykK1prK%2BAXi9eg3CbIO1vbG8eNjT0%3D&reserved=0
>>> Cc: Daniel Stone <daniel@fooishbar.org>
>>> Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
>>> Cc: Simon Ser <contact@emersion.fr>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>> drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++
>>> 1 file changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 58527f151984..aac9122f1da2 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
>>> * needed. It will also grab the relevant CRTC lock to make sure that the state
>>> * is consistent.
>>> *
>>> + * WARNING: Drivers may only add new CRTC states to a @state if
>>> + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
>>> + * not created by userspace through an IOCTL call.
>>> + *
>>> * Returns:
>>> *
>>> * Either the allocated state or the error code encoded into the pointer. When
>>> @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>> struct drm_crtc_state *new_crtc_state;
>>> struct drm_connector *conn;
>>> struct drm_connector_state *conn_state;
>>> + unsigned requested_crtc = 0;
>>> + unsigned affected_crtc = 0;
>>> int i, ret = 0;
>>> DRM_DEBUG_ATOMIC("checking %p\n", state);
>>> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
>>> + requested_crtc |= drm_crtc_mask(crtc);
>>> +
>>> for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>>> ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
>>> if (ret) {
>>> @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>>> }
>>> }
>>> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
>>> + affected_crtc |= drm_crtc_mask(crtc);
>>> +
>>> + /*
>>> + * For commits that allow modesets drivers can add other CRTCs to the
>>> + * atomic commit, e.g. when they need to reallocate global resources.
>>> + * This can cause spurious EBUSY, which robs compositors of a very
>>> + * effective sanity check for their drawing loop. Therefor only allow
>>> + * drivers to add unrelated CRTC states for modeset commits.
>>> + *
>>> + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
>>> + * so compositors know what's going on.
>>> + */
>>> + if (affected_crtc != requested_crtc) {
>>> + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
>>> + requested_crtc, affected_crtc);
>>> + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
>>> + requested_crtc, affected_crtc);
>>> + }
>>> +
>>> return 0;
>>> }
>>> EXPORT_SYMBOL(drm_atomic_check_only);
>>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY
2021-11-08 15:32 ` Kazlauskas, Nicholas
@ 2021-11-08 15:41 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2021-11-08 15:41 UTC (permalink / raw)
To: Kazlauskas, Nicholas
Cc: Daniel Vetter, Intel Graphics Development, DRI Development,
Daniel Vetter, Pekka Paalanen
On Mon, Nov 08, 2021 at 10:32:04AM -0500, Kazlauskas, Nicholas wrote:
> On 2021-11-08 10:07 a.m., Daniel Vetter wrote:
> > On Fri, Nov 05, 2021 at 04:47:29PM -0400, Kazlauskas, Nicholas wrote:
> > > Hi Daniel,
> > >
> > > Just got bitten by this warning when trying to do some refactoring in amdgpu
> > > for trying to get rid of the DRM private object we use for our DC state.
> > >
> > > From a userspace perspective I understand that we want to avoid judder,
> > > -EBUSY and other issues affecting the compositor from kernel having to drag
> > > these CRTCs (or their planes) into the atomic state.
> > >
> > > For bandwidth validation we need to understand the state of all CRTCs and
> > > planes in use. Existing driver code maintains this as part of a global state
> > > object in a DRM private atomic state. We have stalls in atomic check (bad)
> > > to avoid freeing this state or modifying it at the wrong times which avoid
> > > hitting this warning but essentially cause the same judder issue.
> > >
> > > While most hardware has independent pipes, I think almost all hardware ends
> > > up having the memory interface/bandwidth as a global shared resource that
> > > software state can't really abstract around.
> > >
> > > There are cases where we know that there will be no (or minimal) impact to
> > > the overall memory requirements for particular DRM updates. Our validation
> > > already "over-allocates" for common display changes - page flips, some
> > > format changes, cursor enable/disable. But for most cases outside of that we
> > > do want to pull in _all_ the CRTCs and planes.
> > >
> > > On our HW you won't get a blankout unless you're actually modifying a stream
> > > timing itself so I think the ALLOW_MODESET flag is overkill here.
> > >
> > > Rejecting the commit when the flag isn't set also ends up breaking userspace
> > > in the process since it expects commits like pageflips between different
> > > tiling modes to succeed with the legacy IOCTLs.
> > >
> > > Any ideas about this? I missed the IRC discussion regarding this before so
> > > I'm not sure if we have any alternatives that were dropped in favor of this.
> >
> > We have, I while ago I had a lengthy discussion with I think Maxime about
> > how this is done properly. I think Maxime volunteered to type up some docs
> > even. tldr;
> >
> > - Have some global state for this stuff (using the private state helper
> > stuff ideally, handrolling very much not adviced, might even make sense
> > to put some of your dc state/structs in there)
>
> This is what we do today, but DRM private objects have no synchronization
> like CRTCs do. Any time a commit modifies our bandwidth calculations we need
> to propagate those changes back up to the global state.
>
> Read only access to the global state can be concurrent, but as soon as
> anything needs to write and update it then we need to force ordering.
>
> The bug we had before was that competing commits could complete in a
> different order than they would be programmed to HW if they were
> non-blocking.
Yeah it's probably high time we add a drm_crtc_commit to drm private state
objects and make this work.
Also maybe rename it to drm_hw_commit or so since it's not about crtc only
anymore.
> > - Both the crtc/plane and the global state hold copies of your limits. The
> > global state in addition also holds the current settings for your
> > derived values (like clocks or allocations or whatever you have).
> There are per pipe limits but there are also global limits on the whole
> system.
>
> For example, you may pass validation for a configuration applied to a single
> CRTC and plane alone. But if you were to run 4 CRTCs using that same valid
> configuration it would fail.
>
> The resource in question here isn't a pipe, an encoder, or something of that
> nature - it's just the memory bandwidth available on the system.
>
> On an APU you wouldn't have enough memory bandwidth to drive multiple CRTCs
> at "extreme" resolutions or refresh rates.
>
> It also doesn't seem right to block the user from running any display at
> these modes just because you can't enable 4 identical displays at the same
> time.
Yeah this is just standard stuff. And yes you do _not_ want to force the
limit to be global / #crtcs. It must be dynamic as you enable/disable
crtcs.
So I'm a bit confused what you mean here?
> > - Rules are that only grabbing the global state is enough to read all your
> > crtc/plane requirements (since you have a read-only copy of that in your
> > global state). Only for changing them do you also have to have the
> > corresponding crtc/plane state. Furthermore you _only_ grab the global
> > objects if the commit reasonably changes your local requirements (i.e.
> > more planes or whatever).
> >
> > - You might need to have a cascade here (plane -> crtc and crct -> global
> > state), and/or maybe multiple different global states. Don't try to put
> > everything into one, it's better to have a separate private state
> > handling for each separate thing, at least generally.
> >
> > - No more "we have to take all objects all the time, always", so no more
> > over-locking and over-sync.
> >
> > - Integrating this into DC is probably going to be "fun". Could be that
> > you first need to convert a pile of the dc_ structs into driver private
> > state stuff, similar in spirit to the refactor we've done for plane/crtc
> > state before DC was landed.
>
> ...so while we can do CRTC and plane validation it doesn't seem like there's
> a good place for that global system level validation that we need to happen.
Nah, you need to do that afterwards in your dc_atomic_check, iff the
global state has been added. crtc/plane will pull the global state in if
any requirements/limits they have have changed.
Doing the global checks in the objects atomic_check callbacks wont work,
those only a) check whether limits/requirements they have changed and b)
if so, grab the right corresponding global state and update the values in
there.
> > Pls check with Maxime that this is documented somewhere as implementation
> > pattern (it's really common, and your aproach of "grab all crtc/plane" is
> > the really common wrong approach). And ofc happy to discuss how to best
> > solve this for DC, but maybe irc is better for that.
> >
> > Cheers, Daniel
>
> I can try and hop on IRC sometime to chat about this with you and the
> others.
>
> Thanks for the response!
Cheers, Daniel
>
> Regards,
> Nicholas Kazlauskas
>
> >
> > >
> > > Regards,
> > > Nicholas Kazlauskas
> > >
> > > On 2020-09-25 4:46 a.m., Daniel Vetter wrote:
> > > > When doing an atomic modeset with ALLOW_MODESET drivers are allowed to
> > > > pull in arbitrary other resources, including CRTCs (e.g. when
> > > > reconfiguring global resources).
> > > >
> > > > But in nonblocking mode userspace has then no idea this happened,
> > > > which can lead to spurious EBUSY calls, both:
> > > > - when that other CRTC is currently busy doing a page_flip the
> > > > ALLOW_MODESET commit can fail with an EBUSY
> > > > - on the other CRTC a normal atomic flip can fail with EBUSY because
> > > > of the additional commit inserted by the kernel without userspace's
> > > > knowledge
> > > >
> > > > For blocking commits this isn't a problem, because everyone else will
> > > > just block until all the CRTC are reconfigured. Only thing userspace
> > > > can notice is the dropped frames without any reason for why frames got
> > > > dropped.
> > > >
> > > > Consensus is that we need new uapi to handle this properly, but no one
> > > > has any idea what exactly the new uapi should look like. Since this
> > > > has been shipping for years already compositors need to deal no matter
> > > > what, so as a first step just try to enforce this across drivers
> > > > better with some checks.
> > > >
> > > > v2: Add comments and a WARN_ON to enforce this only when allowed - we
> > > > don't want to silently convert page flips into blocking plane updates
> > > > just because the driver is buggy.
> > > >
> > > > v3: Fix inverted WARN_ON (Pekka).
> > > >
> > > > v4: Drop the uapi changes, only add a WARN_ON for now to enforce some
> > > > rules for drivers.
> > > >
> > > > v5: Make the WARNING more informative (Daniel)
> > > >
> > > > v6: Add unconditional debug output for compositor hackers to figure
> > > > out what's going on when they get an EBUSY (Daniel)
> > > >
> > > > v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville)
> > > >
> > > > References: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2018-July%2F182281.html&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zmfJd7M5ZnI5if%2FHM%2FYnBQdoLsWtfjwQW%2BaX0R6Hrj0%3D&reserved=0
> > > > Bugzilla: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fwayland%2Fweston%2F-%2Fissues%2F24%23note_9568&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C1b07173e73194ee6563a08d9a2c97eef%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719808574816469%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=ynHonfkaMn%2BC%2BsykK1prK%2BAXi9eg3CbIO1vbG8eNjT0%3D&reserved=0
> > > > Cc: Daniel Stone <daniel@fooishbar.org>
> > > > Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
> > > > Cc: Simon Ser <contact@emersion.fr>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > > drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++
> > > > 1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > > index 58527f151984..aac9122f1da2 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free);
> > > > * needed. It will also grab the relevant CRTC lock to make sure that the state
> > > > * is consistent.
> > > > *
> > > > + * WARNING: Drivers may only add new CRTC states to a @state if
> > > > + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit
> > > > + * not created by userspace through an IOCTL call.
> > > > + *
> > > > * Returns:
> > > > *
> > > > * Either the allocated state or the error code encoded into the pointer. When
> > > > @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > > > struct drm_crtc_state *new_crtc_state;
> > > > struct drm_connector *conn;
> > > > struct drm_connector_state *conn_state;
> > > > + unsigned requested_crtc = 0;
> > > > + unsigned affected_crtc = 0;
> > > > int i, ret = 0;
> > > > DRM_DEBUG_ATOMIC("checking %p\n", state);
> > > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > > + requested_crtc |= drm_crtc_mask(crtc);
> > > > +
> > > > for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> > > > ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
> > > > if (ret) {
> > > > @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> > > > }
> > > > }
> > > > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> > > > + affected_crtc |= drm_crtc_mask(crtc);
> > > > +
> > > > + /*
> > > > + * For commits that allow modesets drivers can add other CRTCs to the
> > > > + * atomic commit, e.g. when they need to reallocate global resources.
> > > > + * This can cause spurious EBUSY, which robs compositors of a very
> > > > + * effective sanity check for their drawing loop. Therefor only allow
> > > > + * drivers to add unrelated CRTC states for modeset commits.
> > > > + *
> > > > + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
> > > > + * so compositors know what's going on.
> > > > + */
> > > > + if (affected_crtc != requested_crtc) {
> > > > + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n",
> > > > + requested_crtc, affected_crtc);
> > > > + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n",
> > > > + requested_crtc, affected_crtc);
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > > EXPORT_SYMBOL(drm_atomic_check_only);
> > > >
> > >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-23 10:57 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
@ 2020-09-25 8:27 ` Pekka Paalanen
0 siblings, 0 replies; 11+ messages in thread
From: Pekka Paalanen @ 2020-09-25 8:27 UTC (permalink / raw)
To: Daniel Vetter
Cc: Daniel Vetter, Intel Graphics Development, Sean Paul, DRI Development
[-- Attachment #1.1: Type: text/plain, Size: 4088 bytes --]
On Wed, 23 Sep 2020 12:57:37 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Hopefully we'll have the drm crash recorder RSN, but meanwhile
> compositors would like to know a bit better why they get an EBUSY.
>
These debug messages will be especially useful with the flight
recorder, but also without. :-)
...
> ---
> drivers/gpu/drm/drm_atomic.c | 4 ++--
> drivers/gpu/drm/drm_atomic_helper.c | 20 +++++++++++++++++---
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index e22669b64521..6864e520269d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1272,7 +1272,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>
> DRM_DEBUG_ATOMIC("checking %p\n", state);
>
> - for_each_new_crtc_in_state(state, crtc, old_crtc_state, i)
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> requested_crtc |= drm_crtc_mask(crtc);
>
> for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> @@ -1322,7 +1322,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> }
> }
>
> - for_each_new_crtc_in_state(state, crtc, old_crtc_state, i)
> + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
> affected_crtc |= drm_crtc_mask(crtc);
Oops, these belong in the previous patch?
>
> /*
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e8abaaaa7fd1..6b3bfabac26c 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1740,8 +1740,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> * overridden by a previous synchronous update's state.
> */
> if (old_plane_state->commit &&
> - !try_wait_for_completion(&old_plane_state->commit->hw_done))
> + !try_wait_for_completion(&old_plane_state->commit->hw_done)) {
> + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] inflight previous commit preventing async commit\n",
> + plane->base.id, plane->name);
> return -EBUSY;
> + }
>
> return funcs->atomic_async_check(plane, new_plane_state);
> }
> @@ -1964,6 +1967,9 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
> * commit with nonblocking ones. */
> if (!completed && nonblock) {
> spin_unlock(&crtc->commit_lock);
> + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a previous commit\n",
> + crtc->base.id, crtc->name);
> +
> return -EBUSY;
> }
> } else if (i == 1) {
> @@ -2132,8 +2138,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> /* Userspace is not allowed to get ahead of the previous
> * commit with nonblocking ones. */
> if (nonblock && old_conn_state->commit &&
> - !try_wait_for_completion(&old_conn_state->commit->flip_done))
> + !try_wait_for_completion(&old_conn_state->commit->flip_done)) {
> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a previous commit\n",
> + conn->base.id, conn->name);
> +
> return -EBUSY;
> + }
>
> /* Always track connectors explicitly for e.g. link retraining. */
> commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);
> @@ -2147,8 +2157,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> /* Userspace is not allowed to get ahead of the previous
> * commit with nonblocking ones. */
> if (nonblock && old_plane_state->commit &&
> - !try_wait_for_completion(&old_plane_state->commit->flip_done))
> + !try_wait_for_completion(&old_plane_state->commit->flip_done)) {
> + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous commit\n",
> + plane->base.id, plane->name);
> +
> return -EBUSY;
> + }
>
> /* Always track planes explicitly for async pageflip support. */
> commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
The new debug messages sound good to me.
Thanks,
pq
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] drm/atomic: debug output for EBUSY
2020-09-23 10:57 Daniel Vetter
@ 2020-09-23 10:57 ` Daniel Vetter
2020-09-25 8:27 ` Pekka Paalanen
0 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2020-09-23 10:57 UTC (permalink / raw)
To: DRI Development
Cc: Daniel Vetter, Daniel Vetter, Intel Graphics Development,
Sean Paul, Pekka Paalanen
Hopefully we'll have the drm crash recorder RSN, but meanwhile
compositors would like to know a bit better why they get an EBUSY.
Cc: Sean Paul <seanpaul@chromium.org>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Cc: Simon Ser <contact@emersion.fr>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
drivers/gpu/drm/drm_atomic.c | 4 ++--
drivers/gpu/drm/drm_atomic_helper.c | 20 +++++++++++++++++---
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index e22669b64521..6864e520269d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1272,7 +1272,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_new_crtc_in_state(state, crtc, old_crtc_state, i)
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
requested_crtc |= drm_crtc_mask(crtc);
for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
@@ -1322,7 +1322,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
}
}
- for_each_new_crtc_in_state(state, crtc, old_crtc_state, i)
+ for_each_new_crtc_in_state(state, crtc, new_crtc_state, i)
affected_crtc |= drm_crtc_mask(crtc);
/*
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index e8abaaaa7fd1..6b3bfabac26c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1740,8 +1740,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
* overridden by a previous synchronous update's state.
*/
if (old_plane_state->commit &&
- !try_wait_for_completion(&old_plane_state->commit->hw_done))
+ !try_wait_for_completion(&old_plane_state->commit->hw_done)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] inflight previous commit preventing async commit\n",
+ plane->base.id, plane->name);
return -EBUSY;
+ }
return funcs->atomic_async_check(plane, new_plane_state);
}
@@ -1964,6 +1967,9 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
* commit with nonblocking ones. */
if (!completed && nonblock) {
spin_unlock(&crtc->commit_lock);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a previous commit\n",
+ crtc->base.id, crtc->name);
+
return -EBUSY;
}
} else if (i == 1) {
@@ -2132,8 +2138,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
/* Userspace is not allowed to get ahead of the previous
* commit with nonblocking ones. */
if (nonblock && old_conn_state->commit &&
- !try_wait_for_completion(&old_conn_state->commit->flip_done))
+ !try_wait_for_completion(&old_conn_state->commit->flip_done)) {
+ DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a previous commit\n",
+ conn->base.id, conn->name);
+
return -EBUSY;
+ }
/* Always track connectors explicitly for e.g. link retraining. */
commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc);
@@ -2147,8 +2157,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
/* Userspace is not allowed to get ahead of the previous
* commit with nonblocking ones. */
if (nonblock && old_plane_state->commit &&
- !try_wait_for_completion(&old_plane_state->commit->flip_done))
+ !try_wait_for_completion(&old_plane_state->commit->flip_done)) {
+ DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous commit\n",
+ plane->base.id, plane->name);
+
return -EBUSY;
+ }
/* Always track planes explicitly for async pageflip support. */
commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-08 15:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25 8:46 [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Daniel Vetter
2020-09-25 8:46 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
2020-09-25 9:11 ` Pekka Paalanen
2020-09-29 15:48 ` Daniel Stone
2020-10-08 9:31 ` Daniel Vetter
2021-11-05 20:47 ` [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Kazlauskas, Nicholas
2021-11-08 15:07 ` Daniel Vetter
2021-11-08 15:32 ` Kazlauskas, Nicholas
2021-11-08 15:41 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2020-09-23 10:57 Daniel Vetter
2020-09-23 10:57 ` [PATCH 2/2] drm/atomic: debug output for EBUSY Daniel Vetter
2020-09-25 8:27 ` Pekka Paalanen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).