[PATCH 2/2] drm/doc: Document the writeback prepare/cleanup jobs

[Daniel Vetter]

On Thu, May 02, 2019 at 04:24:30PM -0400, Sean Paul wrote:
> From: Sean Paul <seanpaul at chromium.org>
> 
> These functions were missing documentation.
> 
> Fixes the warning:
> ../include/drm/drm_modeset_helper_vtables.h:1000: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
> ../include/drm/drm_modeset_helper_vtables.h:1000: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
> 
> Fixes: 9d2230dc1351 ("drm: writeback: Add job prepare and cleanup operations")
> Cc: Laurent Pinchart <laurent.pinchart+renesas at ideasonboard.com>

Oh Laurent didn't type documentation, I'll remember that one :-)

> Cc: Liviu Dudau <liviu.dudau at arm.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <maxime.ripard at bootlin.com>
> Cc: Sean Paul <sean at poorly.run>
> Cc: David Airlie <airlied at linux.ie>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
>  drivers/gpu/drm/drm_writeback.c          | 20 ++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h | 20 ++++++++++++++++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index 79ac014701c8..ef6f85966546 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -258,6 +258,15 @@ int drm_writeback_set_fb(struct drm_connector_state *conn_state,
>  	return 0;
>  }
>  
> +/**
> + * drm_writeback_prepare_job - Gives driver a chance to prepare for writeback
> + * @job: The writeback job to be prepared
> + *
> + * This function is called from &drm_atomic_helper_prepare_planes(), and allows
> + * the driver to setup the upcoming writeback job. The job will be
> + * destroyed/cleaned up in &drm_writeback_cleanup_job when the connector state
> + * is destroyed.

This is missing the standard "Returns 0 on success, negative error codes
on failures."
> + */
>  int drm_writeback_prepare_job(struct drm_writeback_job *job)
>  {
>  	struct drm_writeback_connector *connector = job->connector;
> @@ -310,6 +319,17 @@ void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
>  }
>  EXPORT_SYMBOL(drm_writeback_queue_job);
>  
> +/**
> + * drm_writeback_cleanup_job - Gives driver a chance to cleanup a writeback job
> + * @job: The writeback job to be cleaned up
> + *
> + * This function is called from &atomic_helper_connector_destroy_state() and
> + * allows the driver to clean up the now finished writeback job.
> + *
> + * Notice the asymmetry from prepare, this is called on destroy to
> + * ensure the job is not destroyed before than the writeback operation

s/than// I think.

But I'm not really clear on what exactly the asymmetry is.

Plus it's not only called from destry_state, but also through a worker
that's launched by drm_writeback_signal_completion.

I think you're docs here don't reflect reality. But yeah I think there's
some asymmetry, as in cleanup_job is always called, even if prepare_job
wasn't. Not the greatest interface I think, probably should rename cleanup
to release/free. Since that's what it's supposed to do I think.

> + * has completed.
> + */
>  void drm_writeback_cleanup_job(struct drm_writeback_job *job)
>  {
>  	struct drm_writeback_connector *connector = job->connector;
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 8f3602811eb5..a0017f37ef55 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -992,8 +992,28 @@ struct drm_connector_helper_funcs {
>  	void (*atomic_commit)(struct drm_connector *connector,
>  			      struct drm_connector_state *state);
>  
> +	/**
> +	 * @prepare_writeback_job:
> +	 *
> +	 * Called from &drm_atomic_helper_prepare_planes(), provides the driver
> +	 * a chance to setup things in preparation for a writeback job.
> +	 *

Missing standard boilerplate about return values.

> +	 * This hook is optional.
> +	 */
>  	int (*prepare_writeback_job)(struct drm_writeback_connector *connector,
>  				     struct drm_writeback_job *job);
> +
> +	/**
> +	 * @cleanup_writeback_job:
> +	 *
> +	 * Called from &drm_atomic_helper_connector_destroy_state(), and
> +	 * provides the driver an opportunity to cleanup the writeback job.
> +	 * Notice the asymmetry from prepare, this is called on destroy to
> +	 * ensure the job is not destroyed before than the writeback operation
> +	 * has completed.
> +	 *
> +	 * This hook is optional.
> +	 */
>  	void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,
>  				      struct drm_writeback_job *job);
>  };

So I looked through the writeback code some more, and realized that this
here is probably more helper code (it's mostly called from helpers, the
vfuncs are in helpers), but it's in core code.

I think before we document the details of this we also need to rethink the
layering here, atm it looks like someone smashed a penne vertically into
our very nice kms core/helper lasagna ... Would be really nice to clean
this up too and either make it all core (which feels wrong) or move into
helpers (probably needs a writeback_helper.c for this, or we stuff it into
drm_atomic_helper.c).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch