On Fri, Dec 4, 2020 at 4:11 PM Maxime Ripard <maxime@xxxxxxxxxx> 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@xxxxxxxx>
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
r-b: me too
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
> include/drm/drm_modeset_helper_vtables.h | 21 +++++++++++++++++++++
> 2 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f9170b4b22e7..f754e21b96eb 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2034,6 +2034,9 @@ crtc_or_fake_commit(struct drm_atomic_state *state, struct drm_crtc *crtc)
> * should always call this function from their
> * &drm_mode_config_funcs.atomic_commit hook.
> *
> + * Drivers that need to extend the commit setup to private objects can use the
> + * &drm_mode_config_helper_funcs.atomic_commit_setup hook.
> + *
> * To be able to use this support drivers need to use a few more helper
> * functions. drm_atomic_helper_wait_for_dependencies() must be called before
> * actually committing the hardware state, and for nonblocking commits this call
> @@ -2077,8 +2080,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)
> @@ -2155,6 +2161,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 4efec30f8bad..0ebb3f191bbc 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -1406,6 +1406,27 @@ 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, tracked in a &drm_private_obj structure.
> + *
> + * Note that the documentation of &drm_private_obj has more details on
> + * how one should implement this.
> + *
> + * This hook is optional.
> + */
> + int (*atomic_commit_setup)(struct drm_atomic_state *state);
> };
>
> #endif
> --
> 2.28.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
