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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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+renesas@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