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.

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.

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

I probably need to revisit this with fresh eyes tomorrow, but how come the prepared-ness and whatever is being prepared can't be put into a subclassed framebuffer? That seems more natural to me, than tracking it with the job. It's really the framebuffer we're preparing after all.

I guess if you have the same framebuffer in use for multiple things, you might need to track it separately? Can the mappings be shared in that case?

---

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) {

• 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;

It kind-of feels like this shouldn't be necessary, but I think avoiding it would mean either allocating the work_struct outside of the job, and tracking the connector there instead of in the job, or having two calls to unprepare - one in signal_completion and one in destroy_state.

It's probably not worth the gymnastics to avoid the backpointer... but for some reason the backpointer feels intangibly dirty to me.

Thanks, -Brian

• /**

• * @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