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

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Fri, Dec 4, 2020 at 4:11 PM Maxime Ripard <maxime@cerno.tech> 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>

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