Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4778 bytes --]

Hi

Am 13.11.20 um 16:29 schrieb Maxime Ripard:
> Private objects storing a state shared across all CRTCs need to be
> carefully handled to avoid a use-after-free issue.
> 
> The proper way to do this to track all the commits using that shared
> state and wait for the previous commits to be done before going on with
> the current one to avoid the reordering of commits that could occur.
> 
> However, this commit setup needs to be done after
> drm_atomic_helper_setup_commit(), because before the CRTC commit
> structure hasn't been allocated before, and before the workqueue is
> scheduled, because we would be potentially reordered already otherwise.
> 
> That means that drivers currently have to roll their own
> drm_atomic_helper_commit() function, even though it would be identical
> if not for the commit setup.
> 
> Let's introduce a hook to do so that would be called as part of
> drm_atomic_helper_commit, allowing us to reuse the atomic helpers.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c      |  6 ++++++
>   include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ddd0e3239150..7d69c7844dfc 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>   	struct drm_plane *plane;
>   	struct drm_plane_state *old_plane_state, *new_plane_state;
>   	struct drm_crtc_commit *commit;
> +	const struct drm_mode_config_helper_funcs *funcs;
>   	int i, ret;
>   
> +	funcs = state->dev->mode_config.helper_private;
> +
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		commit = kzalloc(sizeof(*commit), GFP_KERNEL);
>   		if (!commit)
> @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
>   		new_plane_state->commit = drm_crtc_commit_get(commit);
>   	}
>   
> +	if (funcs && funcs->atomic_commit_setup)
> +		return funcs->atomic_commit_setup(state);
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index f2de050085be..56470baf0513 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs {
>   	 * drm_atomic_helper_commit_tail().
>   	 */
>   	void (*atomic_commit_tail)(struct drm_atomic_state *state);
> +
> +	/**
> +	 * @atomic_commit_setup:
> +	 *
> +	 * This hook is used by the default atomic_commit() hook implemented in
> +	 * drm_atomic_helper_commit() together with the nonblocking helpers (see
> +	 * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It
> +	 * is not used by the atomic helpers.
> +	 *
> +	 * This function is called at the end of
> +	 * drm_atomic_helper_setup_commit(), so once the commit has been
> +	 * properly setup across the generic DRM object states. It allows
> +	 * drivers to do some additional commit tracking that isn't related to a
> +	 * CRTC, plane or connector, typically a private object.
> +	 *
> +	 * This hook is optional.
> +	 */
> +	int (*atomic_commit_setup)(struct drm_atomic_state *state);

It feels hacky and screwed-on to me. I'd suggest to add an 
atomic_commit_prepare callback that is called by drm_atomic_helper where 
it currently calls drm_atomic_helper_setup_commit(). The default 
implementation  would include setup_commit and prepare_planes. Some 
example code for drm_atomic_helper.c

static int commit_prepare(state)
{
	drm_atomic_helper_setup_commit(state)

	drm_atomic_helper_prepare_planes(state)
}

int drm_atomic_helper_commit()
{
	if (async_update) {
		...
	}

	if (funcs->atomic_commit_prepare)
		funcs->atomic_commit_prepare(state)
	else
		commit_prepare(state)

	/* the rest of the current function below */
	INIT_WORK(&state->commit_work, commit_work);
	...
}

Drivers that implement atomic_commit_prepare would be expected to call 
drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() 
or their own implementation of them.

The whole construct mimics how commit tails work.

Best regards
Thomas


>   };
>   
>   #endif
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]