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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 13, 2020 at 04:29:49PM +0100, Maxime Ripard wrote:
> 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>

Should probably wait with merging until we have the entire vc4 user ready
too. And I think the kerneldoc needs to be further improved, see
suggestions below.

> ---
>  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

I think a link from drm_atomic_helper_setup_commit to this new hook here
would be useful too, maybe together with a note that waiting for these
additional synchronization points can be done at the start of
atomic_commit_tail.

> +	 * 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);

I think the kerneldoc for drm_private_obj should also explain when it is
necessary to use this, and when not:

- when the private state is a tied to an existing drm object (drm_crtc,
  drm_plane or drm_crtc) and never moves, then sufficient synchronization
  is already guaranteed by that superior object. This could even hold
  when the private object can be e.g. reassigned between planes, but
  always stays on the same crtc.

- if the private state object can be globally reassigned, then
  drm_crtc_commit synchronization points need to be set up in
  ->atomic_commit_setup and waited on as the first step in
  ->atomic_commit_tail


Also I just realized that a drm_private_state->obj backpointer to
drm_private_obj might be rather useful. Or at least more consistent with
all the other state objects.

Cheers, Daniel


>  };
>  
>  #endif
> -- 
> 2.28.0
> 

-- 
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