On Thu, May 02, 2019 at 04:24:30PM -0400, Sean Paul wrote:

From: Sean Paul seanpaul@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@ideasonboard.com

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

Cc: Liviu Dudau liviu.dudau@arm.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Signed-off-by: Sean Paul seanpaul@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