Hi Brian,
On Thu, Feb 21, 2019 at 04:02:37PM +0000, Brian Starkey wrote:
> On Thu, Feb 21, 2019 at 12:42:25PM +0200, Laurent Pinchart wrote:
> > 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.
>
> LGTM, it was a mistake not doing it like this from the start.
>
> I do have one suggestion below, but either way:
>
> Reviewed-by: Brian Starkey <brian.star...@arm.com>
>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@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;
>
> What do you think about adding a defensive WARN_ON(!job)?
I expect this function to be called from an atomic commit handler for
the writeback job, which will need to access the job's contents (in
particular the framebuffer). I thus think we'll never be called with a
NULL job here, so a WARN_ON would only consume a bit of CPU time and
memory without adding any safeguard. If your analysis differs and you
still think there's a risk, I'll add it.
> >> + 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
