Hi Sam,
Thank you for the patch.
On Tue, Mar 31, 2020 at 09:53:18AM +0200, Daniel Vetter wrote:
> On Sat, Mar 28, 2020 at 02:20:23PM +0100, Sam Ravnborg wrote:
> > Document the callbacks:
> > drm_connector_helper_funcs.prepare_writeback_job
> > drm_connector_helper_funcs.cleanup_writeback_job
> >
> > The documentation was pulled from the changelong introducing the
> > callbacks, originally written by Laurent.
> >
> > Addign the missing documentation fixes the following warnings:
> > drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
> > drm_modeset_helper_vtables.h:1052: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
> >
> > Signed-off-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> > Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: David Airlie <airlied@xxxxxxxx>
> > ---
> > include/drm/drm_modeset_helper_vtables.h | 31 ++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> > index 7c20b1c8b6a7..c51bca1ffec7 100644
> > --- a/include/drm/drm_modeset_helper_vtables.h
> > +++ b/include/drm/drm_modeset_helper_vtables.h
> > @@ -1075,8 +1075,39 @@ struct drm_connector_helper_funcs {
> > void (*atomic_commit)(struct drm_connector *connector,
> > struct drm_connector_state *state);
> >
> > + /**
> > + * @prepare_writeback_job:
>
> Formatting looks funny, your linebreaks here won't go into the generated
> html and are a bit unusual. I'd remove them and just flow this as a full
> paragraph.
>
> > + *
> > + * As writeback jobs contain a framebuffer, drivers may need to
> > + * prepare and cleanup them the same way they can prepare and
"cleanup them" or "clean them up" ?
> > + * cleanup framebuffers for planes.
This would be "clean up" too.
> > + * This optional connector operation is used to support the
> > + * preparation of writeback jobs.
> > + * The job prepare operation is called from
> > + * drm_atomic_helper_prepare_planes() to avoid a new atomic commit
> > + * helper that would need to be called by all drivers not using
> > + * drm_atomic_helper_commit().
>
> I'd delete "to avoid a new ..." until the end of the sentence. That feels
> more like stuff in the commit message/review than kernel docs for driver
> writers.
>
> Instead maybe add "... for struct &drm_writeback_connector connectors
> only." This gives us a nice link to the writeback docs, and makes it clear
> that this isn't some general prep/cleanup thing. Similar addition below.
>
> > + *
> > + * This hook is optional.
> > + *
> > + * This callback is used by the atomic modeset helpers.
"hook" or "callback", you decide, but let's be consistent :-) I'd go for
"operation" personally as that's what is used above. Same for the
cleanup_writeback_job operation below.
> > + */
> > int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
> > struct drm_writeback_job *job);
> > + /**
> > + * @cleanup_writeback_job:
> > + *
> > + * This optional connector operation is used to support the
> > + * cleanup of writeback jobs.
> > + * The job cleanup operation is called from the existing
> > + * drm_writeback_cleanup_job() function, invoked both when
> > + * destroying the job as part of a aborted commit, or when
s/a aborted/an aborted/
> > + * the job completes.
> > + *
> > + * This hook is optional.
> > + *
> > + * This callback is used by the atomic modeset helpers.
> > + */
> > void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
> > struct drm_writeback_job *job);
>
> With the bikesheds addressed as you see fit:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Also Laurent owes you one, I've been pestering to fill this gap in his
> docs since forever ...
I do. Sorry for letting you fix it.
> > };
--
Regards,
Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
