Re: [PATCH v1 4/6] drm: writeback: document callbacks

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@ravnborg.org>
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: David Airlie <airlied@linux.ie>
> > ---
> >  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@ffwll.ch>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel