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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 20, 2020 at 9:39 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 19.11.20 um 16:32 schrieb Daniel Vetter:
> > On Thu, Nov 19, 2020 at 10:59:42AM +0100, Thomas Zimmermann wrote:
> >> 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.
> >
> > Yeah what I suggested is a bit much midlayer, but we've done what you
> > suggested above with all drivers rolling their own atomic_commit. It
> > wasn't pretty. There's still drivers like vc4 which haven't switched, and
> > which have their own locking and synchronization.
> >
> > Hence why I think the callback approach here is better, gives drivers less
> > excuses to roll their own and make a mess.
>
> But it sounds like you'll regret this. As soon as a driver has to do
> additional stuff at the beginning of the setup function, another helper
> will be required, and so on. Maybe rather go with the commit_prepare
> helper and push driver authors to not use it.

For the other thing we already have callbacks (it's prepare_plane).
The use case for this is also fairly minimal (and this should be clear
when the kerneldoc is fully updated).

The thing is, avoiding the midlayer mistake doesn't mean no callbacks.
It just means the topmost entry point should be a driver callback too,
and ideally all the pieces are still fairly modular. We check all
these boxes.

Your option otoh means a bunch more code in vc4 (after Maxime's series
is done) for not much reason. Plus I'm really not seeing the concern.
Also, rule of thumb is to do clean design when we have 3 cases, and
hack things up for the first 2. We're at 1.

Also note that the 2nd part of this is also not in the
atomic_commit_tail callback. But we get away with that because the
driver handling can be done at the top of atomic_commit_tail, hence
there's no need for an atomic_commit_wait_for_dependencies.
-Daniel

>
> Best regards
> Thomas
>
> > -Daniel
> >
>
> --
> 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



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