On Fri, Nov 20, 2020 at 2:34 PM Maxime Ripard <maxime@xxxxxxxxxx> 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
