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

From: Daniel Vetter <daniel@ffwll.ch>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	David Airlie <airlied@linux.ie>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Liviu Dudau <liviu.dudau@arm.com>,
	dri-devel@lists.freedesktop.org,
	Andrzej Hajda <a.hajda@samsung.com>,
	Nirmoy Das <nirmoy.das@amd.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Mihail Atanassov <Mihail.Atanassov@arm.com>,
	Emil Velikov <emil.velikov@collabora.com>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	David Francis <David.Francis@amd.com>,
	James Qian Wang <james.qian.wang@arm.com>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Jonas Karlman <jonas@kwiboo.se>,
	Mikita Lipski <mikita.lipski@amd.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	Alex Deucher <alexander.deucher@amd.com>
Subject: Re: [PATCH v1 4/6] drm: writeback: document callbacks
Date: Tue, 31 Mar 2020 09:53:18 +0200	[thread overview]
Message-ID: <20200331075318.GF2363188@phenom.ffwll.local> (raw)
In-Reply-To: <20200328132025.19910-5-sam@ravnborg.org>

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 framebuffers for planes.
> +	 * 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.
> +	 */
>  	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
> +	 * 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>

Also Laurent owes you one, I've been pestering to fill this gap in his
docs since forever ...

Cheers, Daniel

>  };
> -- 
> 2.20.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel