Re: [PATCH v5 14/19] drm: writeback: Add job prepare and cleanup operations

Hi Laurent,

On Thu, Feb 21, 2019 at 12:32:07PM +0200, Laurent Pinchart wrote:

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. Add two new optional connector helper operations, .prepare_writeback_job() and .cleanup_writeback_job() to support this.

I'm having a bit of a hard time parsing the above paragraph. I think that what you are saying is that you need to prepare and cleanup the framebuffers that writeback jobs have, but it also can be read that you need to prepare/cleanup the actual jobs. If the latter, then I'm curious to know what is special about the jobs that you need preparing/cleaning up.

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(). 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.

The drm_writeback_job structure is extended with a priv field to let drivers store per-job data, such as mappings related to the writeback framebuffer.

Could the driver store in this priv field a structure that contains the connector, whereby removing the need for a back-pointer?

For internal plumbing reasons the drm_writeback_job structure needs to store a back-pointer to the drm_writeback_connector. To avoid pushing too much writeback-specific knowledge to drm_atomic_uapi.c, create a drm_writeback_set_fb() function, move the writeback job setup code there, and set the connector backpointer. The prepare_signaling() function doesn't need to allocate writeback jobs and can ignore connectors without a job, as it is called after the writeback jobs are allocated to store framebuffers, and a writeback fence with a framebuffer is an invalid configuration that gets rejected by the commit check.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

drivers/gpu/drm/drm_atomic_helper.c | 11 ++++++ drivers/gpu/drm/drm_atomic_uapi.c | 31 +++++------------ drivers/gpu/drm/drm_writeback.c | 43 ++++++++++++++++++++++++ include/drm/drm_modeset_helper_vtables.h | 7 ++++ include/drm/drm_writeback.h | 28 ++++++++++++++- 5 files changed, 96 insertions(+), 24 deletions(-)

This patch is currently missing documentation for the .prepare_writeback_job() and .cleanup_writeback_job() operations. I plan to fix this, but first wanted feedback on the direction taken. I'm not entirely happy with the priv pointer in the drm_writeback_job structure, but adding a full state duplicate/destroy machinery for that structure was equally unappealing to me.

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 6fe2303fccd9..70a4886c6e65 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2245,10 +2245,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done); int drm_atomic_helper_prepare_planes(struct drm_device *dev, struct drm_atomic_state *state) {

• struct drm_connector *connector;

• struct drm_connector_state *new_conn_state; struct drm_plane *plane; struct drm_plane_state *new_plane_state; int ret, i, j;

• for_each_new_connector_in_state(state, connector, new_conn_state, i) {

• if (!new_conn_state->writeback_job)
  

• 	continue;
  

• ret = drm_writeback_prepare_job(new_conn_state->writeback_job);
  

• if (ret < 0)
  

• 	return ret;
  

• }

• for_each_new_plane_in_state(state, plane, new_plane_state, i) { const struct drm_plane_helper_funcs *funcs;

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index c40889888a16..e802152a01ad 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -647,28 +647,15 @@ drm_atomic_plane_get_property(struct drm_plane *plane, return 0; }

-static struct drm_writeback_job * -drm_atomic_get_writeback_job(struct drm_connector_state *conn_state) -{

• WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
• if (!conn_state->writeback_job)

• conn_state->writeback_job =
  

• 	kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
  

• return conn_state->writeback_job;

-}

static int drm_atomic_set_writeback_fb_for_connector( struct drm_connector_state *conn_state, struct drm_framebuffer *fb) {

• struct drm_writeback_job *job =

• drm_atomic_get_writeback_job(conn_state);
  

• if (!job)

• return -ENOMEM;
  

• int ret;

• drm_framebuffer_assign(&job->fb, fb);

• ret = drm_writeback_set_fb(conn_state, fb);

• if (ret < 0)

• return ret;
  
  

  if (fb) DRM_DEBUG_ATOMIC("Set [FB:%d] for connector state %p\n",

@@ -1158,19 +1145,17 @@ static int prepare_signaling(struct drm_device *dev,

for_each_new_connector_in_state(state, conn, conn_state, i) { struct drm_writeback_connector *wb_conn;

• struct drm_writeback_job *job;
  

  struct drm_out_fence_state *f; struct dma_fence *fence; s32 __user *fence_ptr;

• if (!conn_state->writeback_job)
  

• 	continue;
  

• fence_ptr = get_out_fence_for_connector(state, conn); if (!fence_ptr) continue;

• job = drm_atomic_get_writeback_job(conn_state);
  

• if (!job)
  

• 	return -ENOMEM;
  

• f = krealloc(*fence_state, sizeof(**fence_state) * (*num_fences + 1), GFP_KERNEL); if (!f)

@@ -1192,7 +1177,7 @@ static int prepare_signaling(struct drm_device *dev, return ret; }

• job->out_fence = fence;
  

• conn_state->writeback_job->out_fence = fence;
  

  }

  /*

diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index afb1ae6e0ecb..4678d61d634a 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -239,6 +239,42 @@ int drm_writeback_connector_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_writeback_connector_init);

+int drm_writeback_set_fb(struct drm_connector_state *conn_state,

• 	 struct drm_framebuffer *fb)
  

+{

• WARN_ON(conn_state->connector->connector_type != DRM_MODE_CONNECTOR_WRITEBACK);
• if (!conn_state->writeback_job) {

• conn_state->writeback_job =
  

• 	kzalloc(sizeof(*conn_state->writeback_job), GFP_KERNEL);
  

• if (!conn_state->writeback_job)
  

• 	return -ENOMEM;
  

• conn_state->writeback_job->connector =
  

• 	drm_connector_to_writeback(conn_state->connector);
  

• }
• drm_framebuffer_assign(&conn_state->writeback_job->fb, fb);
• return 0;

+}

+int drm_writeback_prepare_job(struct drm_writeback_job *job) +{

• struct drm_writeback_connector *connector = job->connector;
• const struct drm_connector_helper_funcs *funcs =

• connector->base.helper_private;
  

• int ret;
• if (funcs->cleanup_writeback_job) {

This feels weird, did you mean to actually check that the funcs->prepare_writeback_job hook is implemented?

Otherwise, things look good to me!

Best regards, Liviu

• ret = funcs->prepare_writeback_job(connector, job);
  

• if (ret < 0)
  

• 	return ret;
  

• }
• job->prepared = true;
• return 0;

+}

/**

• drm_writeback_queue_job - Queue a writeback job for later signalling
• @wb_connector: The writeback connector to queue a job on

@@ -275,6 +311,13 @@ EXPORT_SYMBOL(drm_writeback_queue_job);

void drm_writeback_cleanup_job(struct drm_writeback_job *job) {

• struct drm_writeback_connector *connector = job->connector;
• const struct drm_connector_helper_funcs *funcs =

• connector->base.helper_private;
  

• if (job->prepared && funcs->cleanup_writeback_job)

• funcs->cleanup_writeback_job(connector, job);
  

• if (job->fb) drm_framebuffer_put(job->fb);

diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 61142aa0ab23..73d03fe66799 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -49,6 +49,8 @@ */

enum mode_set_atomic; +struct drm_writeback_connector; +struct drm_writeback_job;

/**

• struct drm_crtc_helper_funcs - helper operations for CRTCs

@@ -989,6 +991,11 @@ struct drm_connector_helper_funcs { */ void (*atomic_commit)(struct drm_connector *connector, struct drm_connector_state *state);

• int (*prepare_writeback_job)(struct drm_writeback_connector *connector,

• 		     struct drm_writeback_job *job);
  

• void (*cleanup_writeback_job)(struct drm_writeback_connector *connector,

• 		      struct drm_writeback_job *job);
  

};

/** diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h index 47662c362743..777c14c847f0 100644 --- a/include/drm/drm_writeback.h +++ b/include/drm/drm_writeback.h @@ -79,6 +79,20 @@ struct drm_writeback_connector { };

struct drm_writeback_job {

• /**

• * @connector:
  

• *
  

• * Back-pointer to the writeback connector associated with the job
  

• */
  

• struct drm_writeback_connector *connector;
• /**

• * @prepared:
  

• *
  

• * Set when the job has been prepared with drm_writeback_prepare_job()
  

• */
  

• bool prepared;
• /**
  • @cleanup_work:

@@ -98,7 +112,7 @@ struct drm_writeback_job { * @fb: * * Framebuffer to be written to by the writeback connector. Do not set

• * directly, use drm_atomic_set_writeback_fb_for_connector()
  

• * directly, use drm_writeback_set_fb()
  

  */ struct drm_framebuffer *fb;

@@ -108,6 +122,13 @@ struct drm_writeback_job { * Fence which will signal once the writeback has completed */ struct dma_fence *out_fence;

• /**

• * @priv:
  

• *
  

• * Driver-private data
  

• */
  

• void *priv;

};

static inline struct drm_writeback_connector * @@ -122,6 +143,11 @@ int drm_writeback_connector_init(struct drm_device *dev, const struct drm_encoder_helper_funcs *enc_helper_funcs, const u32 *formats, int n_formats);

+int drm_writeback_set_fb(struct drm_connector_state *conn_state,

• 	 struct drm_framebuffer *fb);
  

+int drm_writeback_prepare_job(struct drm_writeback_job *job);

void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector, struct drm_connector_state *conn_state);

-- Regards,

Laurent Pinchart