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?

Maxime