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

Hi Brian,

On Fri, Feb 22, 2019 at 01:50:01PM +0000, Brian Starkey wrote:

On Fri, Feb 22, 2019 at 12:12:00AM +0200, Laurent Pinchart wrote:

...

On Thu, Feb 21, 2019 at 06:12:03PM +0000, Brian Starkey wrote:

...

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.

In my patch series I indeed need to track information related to the framebuffer only, but is it a good idea to limit writeback prepare/cleanup to that ? Other drivers may have different needs.

IMO better to write the code for the case we know, rather than speculate.

...

Furthermore, the mapping needs to exist for the lifetime of the writeback job, so I'd have to add a counter to the framebuffer subclass to track these too. While doable, it starts sounding a bit like a hack, and may create race conditions.

You're probably right. As you were saying to Daniel - writeback things don't have the same lifetime as the state, so the job is the only place to store things which would normally go in the state.

...

...

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?

Speaking about my case, the mapping is per-CRTC, so if I use the same framebuffer for multiple CRTCs I'd need separate mappings. Other drivers may have simpler or more complex needs.

When you'll revisit this with fresh eyes, I'd like to know if you think it would be worth it replacing the priv pointer with allocate/destroy operations to allow subclassing the writeback job. We could possibly do without the destroy operation if we mandate embedding the writeback job as the first field of the subclass and usage of kmalloc (& friends) to allocate the subclass structure. We could even do without the allocate operation if we just stored the size of the subclass in the writeback connector itself, but that would depart from what we usually do in DRM.

I think enabling the job to be subclassed would be a fine solution, with functions which are as close as possible to the other DRM objects.

I'm not sure there's a lot of advantage in skipping allocate/destroy; it should be fine to just have helpers for the common case.

I wonder if renaming it to "writeback_state" instead of "writeback_job" is too confusing (because it doesn't _quite_ act like the other states, even though its purpose is very similar).

I'd keep job, as it works a bit differently from states.

...

I would also like to know if I should create a writeback connector operations structure to store the prepare/cleanup and perhaps allocate/destroy operations instead of adding them to the connector helper funcs.

I think just adding the functions to the existing connector operations is overall less clutter, but I don't mind either way.

That would be the connector helper functions, not the connector functions, right ?

Daniel, do you have a preference ?

...

...

...

---

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.

I don't like the second option as unprepare could potentially be costly, and should thus not be called from signal_completion that may run in interrupt context. The first option is doable, but I think it will result in even worse code.

...

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

A writeback job exists in the context of a writeback connector, why do you feel the backpointer is dirty ?

...

...

• /**

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