Re: [PATCH v5 12/19] drm: writeback: Cleanup job ownership handling when queuing job

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: James Qian Wang <james.qian.wang@arm.com>,
	Liviu Dudau <Liviu.Dudau@arm.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v5 12/19] drm: writeback: Cleanup job ownership handling when queuing job
Date: Thu, 21 Feb 2019 12:42:25 +0200	[thread overview]
Message-ID: <20190221104225.GJ3451@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20190221103212.28764-13-laurent.pinchart+renesas@ideasonboard.com>

Forgot to CC Eric, sorry about that.

On Thu, Feb 21, 2019 at 12:32:05PM +0200, Laurent Pinchart wrote:
> The drm_writeback_queue_job() function takes ownership of the passed job
> and requires the caller to manually set the connector state
> writeback_job pointer to NULL. To simplify drivers and avoid errors
> (such as the missing NULL set in the vc4 driver), pass the connector
> state pointer to the function instead of the job pointer, and set the
> writeback_job pointer to NULL internally.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/arm/malidp_mw.c |  3 +--
>  drivers/gpu/drm/drm_writeback.c | 15 ++++++++++-----
>  drivers/gpu/drm/vc4/vc4_txp.c   |  2 +-
>  include/drm/drm_writeback.h     |  2 +-
>  4 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c
> index 041a64dc7167..87627219ce3b 100644
> --- a/drivers/gpu/drm/arm/malidp_mw.c
> +++ b/drivers/gpu/drm/arm/malidp_mw.c
> @@ -252,8 +252,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm,
>  				     &mw_state->addrs[0],
>  				     mw_state->format);
>  
> -		drm_writeback_queue_job(mw_conn, conn_state->writeback_job);
> -		conn_state->writeback_job = NULL;
> +		drm_writeback_queue_job(mw_conn, conn_state);
>  		hwdev->hw->enable_memwrite(hwdev, mw_state->addrs,
>  					   mw_state->pitches, mw_state->n_planes,
>  					   fb->width, fb->height, mw_state->format,
> diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
> index c20e6fe00cb3..338b993d7c9f 100644
> --- a/drivers/gpu/drm/drm_writeback.c
> +++ b/drivers/gpu/drm/drm_writeback.c
> @@ -242,11 +242,12 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
>  /**
>   * drm_writeback_queue_job - Queue a writeback job for later signalling
>   * @wb_connector: The writeback connector to queue a job on
> - * @job: The job to queue
> + * @conn_state: The connector state containing the job to queue
>   *
> - * This function adds a job to the job_queue for a writeback connector. It
> - * should be considered to take ownership of the writeback job, and so any other
> - * references to the job must be cleared after calling this function.
> + * This function adds the job contained in @conn_state to the job_queue for a
> + * writeback connector. It takes ownership of the writeback job and sets the
> + * @conn_state->writeback_job to NULL, and so no access to the job may be
> + * performed by the caller after this function returns.
>   *
>   * Drivers must ensure that for a given writeback connector, jobs are queued in
>   * exactly the same order as they will be completed by the hardware (and
> @@ -258,10 +259,14 @@ EXPORT_SYMBOL(drm_writeback_connector_init);
>   * See also: drm_writeback_signal_completion()
>   */
>  void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> -			     struct drm_writeback_job *job)
> +			     struct drm_connector_state *conn_state)
>  {
> +	struct drm_writeback_job *job;
>  	unsigned long flags;
>  
> +	job = conn_state->writeback_job;
> +	conn_state->writeback_job = NULL;
> +
>  	spin_lock_irqsave(&wb_connector->job_lock, flags);
>  	list_add_tail(&job->list_entry, &wb_connector->job_queue);
>  	spin_unlock_irqrestore(&wb_connector->job_lock, flags);
> diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
> index aa279b5b0de7..5dabd91f2d7e 100644
> --- a/drivers/gpu/drm/vc4/vc4_txp.c
> +++ b/drivers/gpu/drm/vc4/vc4_txp.c
> @@ -327,7 +327,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn,
>  
>  	TXP_WRITE(TXP_DST_CTRL, ctrl);
>  
> -	drm_writeback_queue_job(&txp->connector, conn_state->writeback_job);
> +	drm_writeback_queue_job(&txp->connector, conn_state);
>  }
>  
>  static const struct drm_connector_helper_funcs vc4_txp_connector_helper_funcs = {
> diff --git a/include/drm/drm_writeback.h b/include/drm/drm_writeback.h
> index 23df9d463003..47662c362743 100644
> --- a/include/drm/drm_writeback.h
> +++ b/include/drm/drm_writeback.h
> @@ -123,7 +123,7 @@ int drm_writeback_connector_init(struct drm_device *dev,
>  				 const u32 *formats, int n_formats);
>  
>  void drm_writeback_queue_job(struct drm_writeback_connector *wb_connector,
> -			     struct drm_writeback_job *job);
> +			     struct drm_connector_state *conn_state);
>  
>  void drm_writeback_cleanup_job(struct drm_writeback_job *job);
>  

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel