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+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.
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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
