| Message ID | 20200923105737.2943649-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/atomic: document and enforce rules around "spurious" EBUSY | expand |
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
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);
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(-)