LinuxTV Patchwork [v6,14/18] drm: writeback: Add job prepare and cleanup operations

login
register
mail settings
Submitter Laurent Pinchart
Date March 13, 2019, 12:05 a.m.
Message ID <20190313000532.7087-15-laurent.pinchart+renesas@ideasonboard.com>
Download mbox | patch
Permalink /patch/55026/
State New
Headers show

Comments

Laurent Pinchart - March 13, 2019, 12:05 a.m.
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>
---
Changes since v5:

- Export drm_writeback_prepare_job()
- Check for .prepare_writeback_job() in drm_writeback_prepare_job()
---
 drivers/gpu/drm/drm_atomic_helper.c      | 11 ++++++
 drivers/gpu/drm/drm_atomic_uapi.c        | 31 +++++------------
 drivers/gpu/drm/drm_writeback.c          | 44 ++++++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h |  7 ++++
 include/drm/drm_writeback.h              | 28 ++++++++++++++-
 5 files changed, 97 insertions(+), 24 deletions(-)
Liviu Dudau - March 15, 2019, 5:54 p.m.
On Wed, Mar 13, 2019 at 02:05:28AM +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>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
> Changes since v5:
> 
> - Export drm_writeback_prepare_job()
> - Check for .prepare_writeback_job() in drm_writeback_prepare_job()
> ---
>  drivers/gpu/drm/drm_atomic_helper.c      | 11 ++++++
>  drivers/gpu/drm/drm_atomic_uapi.c        | 31 +++++------------
>  drivers/gpu/drm/drm_writeback.c          | 44 ++++++++++++++++++++++++
>  include/drm/drm_modeset_helper_vtables.h |  7 ++++
>  include/drm/drm_writeback.h              | 28 ++++++++++++++-
>  5 files changed, 97 insertions(+), 24 deletions(-)
> 
> 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 1b497d3530b5..79ac014701c8 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -239,6 +239,43 @@ 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->prepare_writeback_job) {
> +		ret = funcs->prepare_writeback_job(connector, job);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	job->prepared = true;
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_writeback_prepare_job);
> +
>  /**
>   * drm_writeback_queue_job - Queue a writeback job for later signalling
>   * @wb_connector: The writeback connector to queue a job on
> @@ -275,6 +312,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
>

Patch

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 1b497d3530b5..79ac014701c8 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -239,6 +239,43 @@  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->prepare_writeback_job) {
+		ret = funcs->prepare_writeback_job(connector, job);
+		if (ret < 0)
+			return ret;
+	}
+
+	job->prepared = true;
+	return 0;
+}
+EXPORT_SYMBOL(drm_writeback_prepare_job);
+
 /**
  * drm_writeback_queue_job - Queue a writeback job for later signalling
  * @wb_connector: The writeback connector to queue a job on
@@ -275,6 +312,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);
 

Privacy Policy