On Fri, Feb 22, 2019 at 12:12:00AM +0200, Laurent Pinchart wrote: > Hi Brian, > > 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+rene...@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 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. -Brian > > > > --- > > > 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); > > > > > -- > Regards, > > Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel