[Intel-gfx] [PATCH] drm: document and enforce rules around "spurious" EBUSY from atomic_commit

[Pekka Paalanen]

On Tue, 22 Sep 2020 20:18:34 +0200
Daniel Vetter <daniel.vetter at ffwll.ch> 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.

Dropped all addresses, because gmail refused to send this email
otherwise.

> ---
>  drivers/gpu/drm/drm_atomic.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 58527f151984..ef106e7153a6 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, old_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,24 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	for_each_new_crtc_in_state(state, crtc, old_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
> +	 * this for modeset commits.
> +	 *
> +	 * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output
> +	 * so compositors know what's going on.

Hi,

I think telling userspace the affected_crtc mask would only solve half
of the problem: it would allow userspace to avoid attempting flips on
the other affected CRTCs until this modeset is done, but it doesn't
stop this non-blocking modeset from EBUSY'ing because other affected
CRTCs are busy flipping.

If the aim is to indicate userspace bugs with EBUSY, then EBUSY because
of other CRTCs needs to be differentiable from EBUSY due to a mistake
on this CRTC. Maybe the CRTC mask should instead be "conflicting/busy
CRTCs", not simply "affected CRTCS"?

Userspace might also be designed to always avoid modesets while any
CRTC is busy flipping. In that case any EBUSY would be an indication of
a (userspace) bug and a "busy CRTCs" mask could help pinpoint the issue.

If userspace does a TEST_ONLY commit with a modeset on one CRTC and the
driver pulls in another CRTC that is currently busy, will the test
commit return with EBUSY?

If yes, and *if* userspace is single-threaded wrt. to KMS updates,
that might offer a way to work around it in userspace. But if userspace
is flipping other CRTCs from other threads, TEST_ONLY commit does not
help because another thread may cut in and make a CRTC busy.


Thanks,
pq

> +	 */
> +	if (affected_crtc != requested_crtc) {
> +		/* adding other CRTC is only allowed for modeset commits */
> +		WARN_ON(!state->allow_modeset);
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_check_only);

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20200923/28e0080a/attachment-0001.sig>