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

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

Hi Brian,

On Fri, Feb 22, 2019 at 01:50:01PM +0000, Brian Starkey wrote:
> On Fri, Feb 22, 2019 at 12:12:00AM +0200, Laurent Pinchart wrote:
> > 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+renesas@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'd keep job, as it works a bit differently from states.

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

That would be the connector helper functions, not the connector
functions, right ?

Daniel, do you have a preference ?

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