| Message ID | 20190221103212.28764-13-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | R-Car DU display writeback support | expand |
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); >
On Thu, Feb 21, 2019 at 12:42:25PM +0200, Laurent Pinchart wrote: > 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. 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.starkey@arm.com> > > > > 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; What do you think about adding a defensive WARN_ON(!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
Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> writes: > 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> Acked-by: Eric Anholt <eric@anholt.net>
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.starkey@arm.com> > > >> 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; > > 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); > >>
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.starkey@arm.com> > > > > >> 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; > > > > 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
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> Acked-by: Liviu Dudau <liviu.dudau@arm.com> Best regards, Liviu > --- > 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 >
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);
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(-)