Hi Liviu, On Tue, Feb 26, 2019 at 06:39:40PM +0000, Liviu Dudau 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. > > 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.
I meant the framebuffers, but I wouldn't be surprised if the jobs were later extended with more fields that would require similar preparation. That's why I think prepare/cleanup job operations make sense. > > 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? The priv field points to an opaque structure, forcing drivers to use a standard structure there would make the API more complex in my opinion, without any added value I can see. > > 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+rene...@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? Good catch. I'll fix this. > Otherwise, things look good to me! Thank you. > > + 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel