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

[Daniel Vetter]

On Fri, Nov 20, 2020 at 2:34 PM Maxime Ripard <maxime at cerno.tech> wrote:
>
> Hi Daniel,
>
> Thanks for your review
>
> On Fri, Nov 13, 2020 at 10:02:40PM +0100, Daniel Vetter wrote:
> > > +    * 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
>
> Does the comment added on patch 2 sufficient for you, or would you
> extend it / make it clearer?

Lol stateless reviewer. Yeah I think the text there is good, but we
probably want to make sure there's links to all the other pieces in
all the places. So maybe replace "typically in a private object" with
"tracked in a struct drm_private_obj" so we get that link. And maybe a
note to look there for additional information.

Same in the other places. In generally I think you can never have too
many links in kerneldoc, since they're both useful in the generated
html, but also for navigating the code with cscope or similar (at
least here this works splendidly).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch