On Thu, Feb 21, 2019 at 11:56:09PM +0200, Laurent Pinchart wrote: > 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.
My thinking was that it's up to drivers to make the check for a job, and that a driver might get that wrong (and bring down the kernel). With the old signature, it was entirely obvious that you needed a job as it was one of the arguments to the function. Now it _should_ be obvious from the naming, but it's not explicit anymore. Anyway, I don't feel strongly about it. You'd hope a buggy driver would hit the panic almost immediately during development and never do any harm. -Brian > > > >> + 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