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