* [PATCH v1 0/2] Free the writeback_job when it with an empty fb @ 2019-07-31 11:04 Lowry Li (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 0 siblings, 2 replies; 16+ messages in thread From: Lowry Li (Arm Technology China) @ 2019-07-31 11:04 UTC (permalink / raw) To: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey Cc: Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi, This serie aims at adding the check and free the writeback_job when it with an empty fb in drm_atomic_connector_check(). Also did some refine in drm_writeback_signal_completion() to clear the fence pointer when writeback job signaled. This patchset is based at the drm-misc-fixes branch. Lowry Li (Arm Technology China) (2): drm: Free the writeback_job when it with an empty fb drm: Clear the fence pointer when writeback job signaled .../drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- drivers/gpu/drm/drm_atomic.c | 13 ++++++++---- drivers/gpu/drm/drm_writeback.c | 23 ++++++++++++++-------- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- drivers/gpu/drm/vc4/vc4_txp.c | 5 ++--- 6 files changed, 31 insertions(+), 21 deletions(-) -- 1.9.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 1/2] drm: Free the writeback_job when it with an empty fb 2019-07-31 11:04 [PATCH v1 0/2] Free the writeback_job when it with an empty fb Lowry Li (Arm Technology China) @ 2019-07-31 11:04 ` Lowry Li (Arm Technology China) 2019-07-31 13:03 ` Liviu Dudau 2019-09-23 7:24 ` [v1,1/2] " james qian wang (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 1 sibling, 2 replies; 16+ messages in thread From: Lowry Li (Arm Technology China) @ 2019-07-31 11:04 UTC (permalink / raw) To: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey Cc: Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com> Adds the check if the writeback_job with an empty fb, then it should be freed in atomic_check phase. With this change, the driver users will not check empty fb case any more. So refined accordingly. Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> --- drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- drivers/gpu/drm/drm_atomic.c | 13 +++++++++---- drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- drivers/gpu/drm/vc4/vc4_txp.c | 5 ++--- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c index 617e1f7..d6103dd 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c @@ -43,9 +43,8 @@ struct komeda_data_flow_cfg dflow; int err; - if (!writeback_job || !writeback_job->fb) { + if (!writeback_job) return 0; - } if (!crtc_st->active) { DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n"); diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c index 2e81252..a59227b 100644 --- a/drivers/gpu/drm/arm/malidp_mw.c +++ b/drivers/gpu/drm/arm/malidp_mw.c @@ -130,7 +130,7 @@ static void malidp_mw_connector_destroy(struct drm_connector *connector) struct drm_framebuffer *fb; int i, n_planes; - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + if (!conn_state->writeback_job) return 0; fb = conn_state->writeback_job->fb; @@ -247,7 +247,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm, mw_state = to_mw_state(conn_state); - if (conn_state->writeback_job && conn_state->writeback_job->fb) { + if (conn_state->writeback_job) { struct drm_framebuffer *fb = conn_state->writeback_job->fb; DRM_DEV_DEBUG_DRIVER(drm->dev, diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 419381a..14aeaf7 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -430,10 +430,15 @@ static int drm_atomic_connector_check(struct drm_connector *connector, return -EINVAL; } - if (writeback_job->out_fence && !writeback_job->fb) { - DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", - connector->base.id, connector->name); - return -EINVAL; + if (!writeback_job->fb) { + if (writeback_job->out_fence) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", + connector->base.id, connector->name); + return -EINVAL; + } + + drm_writeback_cleanup_job(writeback_job); + state->writeback_job = NULL; } return 0; diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c index ae07290..04efa78d 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c @@ -147,7 +147,7 @@ static int rcar_du_wb_enc_atomic_check(struct drm_encoder *encoder, struct drm_device *dev = encoder->dev; struct drm_framebuffer *fb; - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + if (!conn_state->writeback_job) return 0; fb = conn_state->writeback_job->fb; @@ -221,7 +221,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, unsigned int i; state = rcrtc->writeback.base.state; - if (!state || !state->writeback_job || !state->writeback_job->fb) + if (!state || !state->writeback_job) return; fb = state->writeback_job->fb; diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 96f91c1..e92fa12 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -229,7 +229,7 @@ static int vc4_txp_connector_atomic_check(struct drm_connector *conn, int i; conn_state = drm_atomic_get_new_connector_state(state, conn); - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) + if (!conn_state->writeback_job) return 0; crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); @@ -269,8 +269,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, u32 ctrl; int i; - if (WARN_ON(!conn_state->writeback_job || - !conn_state->writeback_job->fb)) + if (WARN_ON(!conn_state->writeback_job)) return; mode = &conn_state->crtc->state->adjusted_mode; -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 1/2] drm: Free the writeback_job when it with an empty fb 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) @ 2019-07-31 13:03 ` Liviu Dudau 2019-09-23 7:24 ` [v1,1/2] " james qian wang (Arm Technology China) 1 sibling, 0 replies; 16+ messages in thread From: Liviu Dudau @ 2019-07-31 13:03 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Wed, Jul 31, 2019 at 11:04:38AM +0000, Lowry Li (Arm Technology China) wrote: > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com> > > Adds the check if the writeback_job with an empty fb, then it should > be freed in atomic_check phase. > > With this change, the driver users will not check empty fb case any more. > So refined accordingly. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> > --- > drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- > drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- > drivers/gpu/drm/drm_atomic.c | 13 +++++++++---- > drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- > drivers/gpu/drm/vc4/vc4_txp.c | 5 ++--- > 5 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > index 617e1f7..d6103dd 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > @@ -43,9 +43,8 @@ > struct komeda_data_flow_cfg dflow; > int err; > > - if (!writeback_job || !writeback_job->fb) { > + if (!writeback_job) > return 0; > - } > > if (!crtc_st->active) { > DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n"); > diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c > index 2e81252..a59227b 100644 > --- a/drivers/gpu/drm/arm/malidp_mw.c > +++ b/drivers/gpu/drm/arm/malidp_mw.c > @@ -130,7 +130,7 @@ static void malidp_mw_connector_destroy(struct drm_connector *connector) > struct drm_framebuffer *fb; > int i, n_planes; > > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > fb = conn_state->writeback_job->fb; > @@ -247,7 +247,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm, > > mw_state = to_mw_state(conn_state); > > - if (conn_state->writeback_job && conn_state->writeback_job->fb) { > + if (conn_state->writeback_job) { > struct drm_framebuffer *fb = conn_state->writeback_job->fb; > > DRM_DEV_DEBUG_DRIVER(drm->dev, > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 419381a..14aeaf7 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -430,10 +430,15 @@ static int drm_atomic_connector_check(struct drm_connector *connector, > return -EINVAL; > } > > - if (writeback_job->out_fence && !writeback_job->fb) { > - DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", > - connector->base.id, connector->name); > - return -EINVAL; > + if (!writeback_job->fb) { > + if (writeback_job->out_fence) { > + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", > + connector->base.id, connector->name); > + return -EINVAL; > + } > + > + drm_writeback_cleanup_job(writeback_job); > + state->writeback_job = NULL; > } > > return 0; > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > index ae07290..04efa78d 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > @@ -147,7 +147,7 @@ static int rcar_du_wb_enc_atomic_check(struct drm_encoder *encoder, > struct drm_device *dev = encoder->dev; > struct drm_framebuffer *fb; > > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > fb = conn_state->writeback_job->fb; > @@ -221,7 +221,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > unsigned int i; > > state = rcrtc->writeback.base.state; > - if (!state || !state->writeback_job || !state->writeback_job->fb) > + if (!state || !state->writeback_job) > return; > > fb = state->writeback_job->fb; > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > index 96f91c1..e92fa12 100644 > --- a/drivers/gpu/drm/vc4/vc4_txp.c > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > @@ -229,7 +229,7 @@ static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > int i; > > conn_state = drm_atomic_get_new_connector_state(state, conn); > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > @@ -269,8 +269,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, > u32 ctrl; > int i; > > - if (WARN_ON(!conn_state->writeback_job || > - !conn_state->writeback_job->fb)) > + if (WARN_ON(!conn_state->writeback_job)) > return; > > mode = &conn_state->crtc->state->adjusted_mode; > -- > 1.9.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v1,1/2] drm: Free the writeback_job when it with an empty fb 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) 2019-07-31 13:03 ` Liviu Dudau @ 2019-09-23 7:24 ` james qian wang (Arm Technology China) 1 sibling, 0 replies; 16+ messages in thread From: james qian wang (Arm Technology China) @ 2019-09-23 7:24 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey, linux-renesas-soc, nd, maxime.ripard, Jonathan Chai (Arm Technology China), linux-kernel, dri-devel, Julien Yin (Arm Technology China), kieran.bingham+renesas, laurent.pinchart, Ayan Halder, sean On Wed, Jul 31, 2019 at 11:04:38AM +0000, Lowry Li (Arm Technology China) wrote: > From: "Lowry Li (Arm Technology China)" <Lowry.Li@arm.com> > > Adds the check if the writeback_job with an empty fb, then it should > be freed in atomic_check phase. > > With this change, the driver users will not check empty fb case any more. > So refined accordingly. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com> Looks good to me. Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> And will push it to drm-misc-fixes James > --- > drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 3 +-- > drivers/gpu/drm/arm/malidp_mw.c | 4 ++-- > drivers/gpu/drm/drm_atomic.c | 13 +++++++++---- > drivers/gpu/drm/rcar-du/rcar_du_writeback.c | 4 ++-- > drivers/gpu/drm/vc4/vc4_txp.c | 5 ++--- > 5 files changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > index 617e1f7..d6103dd 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c > @@ -43,9 +43,8 @@ > struct komeda_data_flow_cfg dflow; > int err; > > - if (!writeback_job || !writeback_job->fb) { > + if (!writeback_job) > return 0; > - } > > if (!crtc_st->active) { > DRM_DEBUG_ATOMIC("Cannot write the composition result out on a inactive CRTC.\n"); > diff --git a/drivers/gpu/drm/arm/malidp_mw.c b/drivers/gpu/drm/arm/malidp_mw.c > index 2e81252..a59227b 100644 > --- a/drivers/gpu/drm/arm/malidp_mw.c > +++ b/drivers/gpu/drm/arm/malidp_mw.c > @@ -130,7 +130,7 @@ static void malidp_mw_connector_destroy(struct drm_connector *connector) > struct drm_framebuffer *fb; > int i, n_planes; > > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > fb = conn_state->writeback_job->fb; > @@ -247,7 +247,7 @@ void malidp_mw_atomic_commit(struct drm_device *drm, > > mw_state = to_mw_state(conn_state); > > - if (conn_state->writeback_job && conn_state->writeback_job->fb) { > + if (conn_state->writeback_job) { > struct drm_framebuffer *fb = conn_state->writeback_job->fb; > > DRM_DEV_DEBUG_DRIVER(drm->dev, > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 419381a..14aeaf7 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -430,10 +430,15 @@ static int drm_atomic_connector_check(struct drm_connector *connector, > return -EINVAL; > } > > - if (writeback_job->out_fence && !writeback_job->fb) { > - DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", > - connector->base.id, connector->name); > - return -EINVAL; > + if (!writeback_job->fb) { > + if (writeback_job->out_fence) { > + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] requesting out-fence without framebuffer\n", > + connector->base.id, connector->name); > + return -EINVAL; > + } > + > + drm_writeback_cleanup_job(writeback_job); > + state->writeback_job = NULL; > } > > return 0; > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > index ae07290..04efa78d 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_writeback.c > @@ -147,7 +147,7 @@ static int rcar_du_wb_enc_atomic_check(struct drm_encoder *encoder, > struct drm_device *dev = encoder->dev; > struct drm_framebuffer *fb; > > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > fb = conn_state->writeback_job->fb; > @@ -221,7 +221,7 @@ void rcar_du_writeback_setup(struct rcar_du_crtc *rcrtc, > unsigned int i; > > state = rcrtc->writeback.base.state; > - if (!state || !state->writeback_job || !state->writeback_job->fb) > + if (!state || !state->writeback_job) > return; > > fb = state->writeback_job->fb; > diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c > index 96f91c1..e92fa12 100644 > --- a/drivers/gpu/drm/vc4/vc4_txp.c > +++ b/drivers/gpu/drm/vc4/vc4_txp.c > @@ -229,7 +229,7 @@ static int vc4_txp_connector_atomic_check(struct drm_connector *conn, > int i; > > conn_state = drm_atomic_get_new_connector_state(state, conn); > - if (!conn_state->writeback_job || !conn_state->writeback_job->fb) > + if (!conn_state->writeback_job) > return 0; > > crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); > @@ -269,8 +269,7 @@ static void vc4_txp_connector_atomic_commit(struct drm_connector *conn, > u32 ctrl; > int i; > > - if (WARN_ON(!conn_state->writeback_job || > - !conn_state->writeback_job->fb)) > + if (WARN_ON(!conn_state->writeback_job)) > return; > > mode = &conn_state->crtc->state->adjusted_mode; ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-07-31 11:04 [PATCH v1 0/2] Free the writeback_job when it with an empty fb Lowry Li (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) @ 2019-07-31 11:04 ` Lowry Li (Arm Technology China) 2019-07-31 13:15 ` Liviu Dudau ` (3 more replies) 1 sibling, 4 replies; 16+ messages in thread From: Lowry Li (Arm Technology China) @ 2019-07-31 11:04 UTC (permalink / raw) To: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey Cc: Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd During it signals the completion of a writeback job, after releasing the out_fence, we'd clear the pointer. Check if fence left over in drm_writeback_cleanup_job(), release it. Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> --- drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index ff138b6..43d9e3b 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) if (job->fb) drm_framebuffer_put(job->fb); + if (job->out_fence) + dma_fence_put(job->out_fence); + kfree(job); } EXPORT_SYMBOL(drm_writeback_cleanup_job); @@ -366,25 +369,29 @@ static void cleanup_work(struct work_struct *work) { unsigned long flags; struct drm_writeback_job *job; + struct dma_fence *out_fence; spin_lock_irqsave(&wb_connector->job_lock, flags); job = list_first_entry_or_null(&wb_connector->job_queue, struct drm_writeback_job, list_entry); - if (job) { + if (job) list_del(&job->list_entry); - if (job->out_fence) { - if (status) - dma_fence_set_error(job->out_fence, status); - dma_fence_signal(job->out_fence); - dma_fence_put(job->out_fence); - } - } + spin_unlock_irqrestore(&wb_connector->job_lock, flags); if (WARN_ON(!job)) return; + out_fence = job->out_fence; + if (out_fence) { + if (status) + dma_fence_set_error(out_fence, status); + dma_fence_signal(out_fence); + dma_fence_put(out_fence); + job->out_fence = NULL; + } + INIT_WORK(&job->cleanup_work, cleanup_work); queue_work(system_long_wq, &job->cleanup_work); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) @ 2019-07-31 13:15 ` Liviu Dudau [not found] ` <20190801063055.GA17887@lowry.li@arm.com> 2019-07-31 13:20 ` Brian Starkey ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: Liviu Dudau @ 2019-07-31 13:15 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi Lowry, On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > During it signals the completion of a writeback job, after releasing > the out_fence, we'd clear the pointer. > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > --- > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index ff138b6..43d9e3b 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > if (job->fb) > drm_framebuffer_put(job->fb); > > + if (job->out_fence) > + dma_fence_put(job->out_fence); > + > kfree(job); > } This change looks good. > EXPORT_SYMBOL(drm_writeback_cleanup_job); > @@ -366,25 +369,29 @@ static void cleanup_work(struct work_struct *work) > { > unsigned long flags; > struct drm_writeback_job *job; > + struct dma_fence *out_fence; > > spin_lock_irqsave(&wb_connector->job_lock, flags); > job = list_first_entry_or_null(&wb_connector->job_queue, > struct drm_writeback_job, > list_entry); > - if (job) { > + if (job) > list_del(&job->list_entry); > - if (job->out_fence) { > - if (status) > - dma_fence_set_error(job->out_fence, status); > - dma_fence_signal(job->out_fence); > - dma_fence_put(job->out_fence); *Here* > - } > - } > + > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > > if (WARN_ON(!job)) > return; > > + out_fence = job->out_fence; > + if (out_fence) { > + if (status) > + dma_fence_set_error(out_fence, status); > + dma_fence_signal(out_fence); > + dma_fence_put(out_fence); > + job->out_fence = NULL; > + } > + I don't get the point of this change. Why not just add job->out_fence = NULL where *Here* is? Best regards, Liviu > INIT_WORK(&job->cleanup_work, cleanup_work); > queue_work(system_long_wq, &job->cleanup_work); > } > -- > 1.9.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20190801063055.GA17887@lowry.li@arm.com>]
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled [not found] ` <20190801063055.GA17887@lowry.li@arm.com> @ 2019-08-01 9:58 ` Liviu Dudau 0 siblings, 0 replies; 16+ messages in thread From: Liviu Dudau @ 2019-08-01 9:58 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Thu, Aug 01, 2019 at 06:31:13AM +0000, Lowry Li (Arm Technology China) wrote: > Hi Liviu, > > On Wed, Jul 31, 2019 at 01:15:25PM +0000, Liviu Dudau wrote: > > Hi Lowry, > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > During it signals the completion of a writeback job, after releasing > > > the out_fence, we'd clear the pointer. > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > --- > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > index ff138b6..43d9e3b 100644 > > > --- a/drivers/gpu/drm/drm_writeback.c > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > if (job->fb) > > > drm_framebuffer_put(job->fb); > > > > > > + if (job->out_fence) > > > + dma_fence_put(job->out_fence); > > > + > > > kfree(job); > > > } > > > > This change looks good. > > > > > EXPORT_SYMBOL(drm_writeback_cleanup_job); > > > @@ -366,25 +369,29 @@ static void cleanup_work(struct work_struct *work) > > > { > > > unsigned long flags; > > > struct drm_writeback_job *job; > > > + struct dma_fence *out_fence; > > > > > > spin_lock_irqsave(&wb_connector->job_lock, flags); > > > job = list_first_entry_or_null(&wb_connector->job_queue, > > > struct drm_writeback_job, > > > list_entry); > > > - if (job) { > > > + if (job) > > > list_del(&job->list_entry); > > > - if (job->out_fence) { > > > - if (status) > > > - dma_fence_set_error(job->out_fence, status); > > > - dma_fence_signal(job->out_fence); > > > - dma_fence_put(job->out_fence); > > > > *Here* > > > > > - } > > > - } > > > + > > > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > > > > > > if (WARN_ON(!job)) > > > return; > > > > > > + out_fence = job->out_fence; > > > + if (out_fence) { > > > + if (status) > > > + dma_fence_set_error(out_fence, status); > > > + dma_fence_signal(out_fence); > > > + dma_fence_put(out_fence); > > > + job->out_fence = NULL; > > > + } > > > + > > > > I don't get the point of this change. Why not just add job->out_fence = NULL > > where *Here* is? > > > > Best regards, > > Liviu > Besides setting NULL, also did a refine by moving the fence operation > out of the lock block. OK, now it makes sense. May I suggest you add that to the commit message? Otherwise, Acked-by: Liviu Dudau <liviu.dudau@arm.com> Best regards, Liviu > > Best regards, > Lowry > > > INIT_WORK(&job->cleanup_work, cleanup_work); > > > queue_work(system_long_wq, &job->cleanup_work); > > > } > > > -- > > > 1.9.1 > > > > > > > -- > > ==================== > > | I would like to | > > | fix the world, | > > | but they're not | > > | giving me the | > > \ source code! / > > --------------- > > ¯\_(ツ)_/¯ > > -- > Regards, > Lowry -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 2019-07-31 13:15 ` Liviu Dudau @ 2019-07-31 13:20 ` Brian Starkey [not found] ` <20190801063351.GB17887@lowry.li@arm.com> 2019-08-05 13:10 ` Brian Starkey 2019-09-23 7:25 ` [v1,2/2] " james qian wang (Arm Technology China) 3 siblings, 1 reply; 16+ messages in thread From: Brian Starkey @ 2019-07-31 13:20 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi Lowry, Thanks for this cleanup. On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > During it signals the completion of a writeback job, after releasing > the out_fence, we'd clear the pointer. > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > --- > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index ff138b6..43d9e3b 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > if (job->fb) > drm_framebuffer_put(job->fb); > > + if (job->out_fence) I'm thinking it might be a good idea to signal the fence with an error here, if it's not already signaled. Otherwise, if there's someone waiting (which there shouldn't be), they're going to be waiting a very long time :-) Thanks, -Brian > + dma_fence_put(job->out_fence); > + > kfree(job); > } ^ permalink raw reply [flat|nested] 16+ messages in thread
[parent not found: <20190801063351.GB17887@lowry.li@arm.com>]
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled [not found] ` <20190801063351.GB17887@lowry.li@arm.com> @ 2019-08-02 9:29 ` Brian Starkey 2019-08-02 9:43 ` Daniel Vetter 2019-08-02 10:09 ` james qian wang (Arm Technology China) 0 siblings, 2 replies; 16+ messages in thread From: Brian Starkey @ 2019-08-02 9:29 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi Lowry, On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > Hi Brian, > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > Hi Lowry, > > > > Thanks for this cleanup. > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > During it signals the completion of a writeback job, after releasing > > > the out_fence, we'd clear the pointer. > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > --- > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > index ff138b6..43d9e3b 100644 > > > --- a/drivers/gpu/drm/drm_writeback.c > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > if (job->fb) > > > drm_framebuffer_put(job->fb); > > > > > > + if (job->out_fence) > > > > I'm thinking it might be a good idea to signal the fence with an error > > here, if it's not already signaled. Otherwise, if there's someone > > waiting (which there shouldn't be), they're going to be waiting a very > > long time :-) > > > > Thanks, > > -Brian > > > Here it happened at atomic_check failed and test only commit. For both > cases, the commit has been dropped and it's only a clean up. So here better > not be treated as an error case:) If anyone else has a reference on the fence, then IMO it absolutely is an error to reach this point without the fence being signaled - because it means that the fence will never be signaled. I don't think the API gives you a way to check if this is the last reference, so it's safest to just make sure the fence is signalled before dropping the reference. It just feels wrong to me to have the possibility of a dangling fence which is never going to get signalled; and it's an easy defensive step to make sure it can never happen. I know it _shouldn't_ happen, but we often put in handling for cases which shouldn't happen, because they frequently do happen :-) > > Since for userspace, it should have been failed or a test only case, so > writebace fence should not be signaled. It's not only userspace that can wait on fences (and in fact this fence will never even reach userspace if the commit fails), the driver may have taken a copy to use for "something". Cheers, -Brian > > Best regards, > Lowry > > > + dma_fence_put(job->out_fence); > > > + > > > kfree(job); > > > } > > -- > Regards, > Lowry ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-08-02 9:29 ` Brian Starkey @ 2019-08-02 9:43 ` Daniel Vetter 2019-08-02 9:45 ` Daniel Vetter 2019-08-02 10:09 ` james qian wang (Arm Technology China) 1 sibling, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-08-02 9:43 UTC (permalink / raw) To: Brian Starkey Cc: Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Fri, Aug 2, 2019 at 11:29 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > Hi Lowry, > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > Hi Brian, > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > Hi Lowry, > > > > > > Thanks for this cleanup. > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > During it signals the completion of a writeback job, after releasing > > > > the out_fence, we'd clear the pointer. > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > > --- > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > index ff138b6..43d9e3b 100644 > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > if (job->fb) > > > > drm_framebuffer_put(job->fb); > > > > > > > > + if (job->out_fence) > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > here, if it's not already signaled. Otherwise, if there's someone > > > waiting (which there shouldn't be), they're going to be waiting a very > > > long time :-) > > > > > > Thanks, > > > -Brian > > > > > Here it happened at atomic_check failed and test only commit. For both > > cases, the commit has been dropped and it's only a clean up. So here better > > not be treated as an error case:) > > If anyone else has a reference on the fence, then IMO it absolutely is > an error to reach this point without the fence being signaled - > because it means that the fence will never be signaled. > > I don't think the API gives you a way to check if this is the last > reference, so it's safest to just make sure the fence is signalled > before dropping the reference. > > It just feels wrong to me to have the possibility of a dangling fence > which is never going to get signalled; and it's an easy defensive step > to make sure it can never happen. > > I know it _shouldn't_ happen, but we often put in handling for cases > which shouldn't happen, because they frequently do happen :-) We're not as paranoid with the vblank fences either, so not sure why we need to be this paranoid with writeback fences. If your driver grabs anything from the atomic state in ->atomic_check it's buggy anyway. If you want to fix this properly I think we need to move the call to prepare_signalling() in between atomic_check and atomic_commit. Then I think it makes sense to also force-complete the fence on error ... -Daniel > > Since for userspace, it should have been failed or a test only case, so > > writebace fence should not be signaled. > > It's not only userspace that can wait on fences (and in fact this > fence will never even reach userspace if the commit fails), the driver > may have taken a copy to use for "something". > > Cheers, > -Brian > > > > > Best regards, > > Lowry > > > > + dma_fence_put(job->out_fence); > > > > + > > > > kfree(job); > > > > } > > > > -- > > Regards, > > Lowry -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-08-02 9:43 ` Daniel Vetter @ 2019-08-02 9:45 ` Daniel Vetter 2019-08-02 10:09 ` Brian Starkey 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-08-02 9:45 UTC (permalink / raw) To: Brian Starkey Cc: Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Fri, Aug 2, 2019 at 11:43 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Fri, Aug 2, 2019 at 11:29 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > > > Hi Lowry, > > > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > > Hi Brian, > > > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > > Hi Lowry, > > > > > > > > Thanks for this cleanup. > > > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > > During it signals the completion of a writeback job, after releasing > > > > > the out_fence, we'd clear the pointer. > > > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > > > --- > > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > > index ff138b6..43d9e3b 100644 > > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > > if (job->fb) > > > > > drm_framebuffer_put(job->fb); > > > > > > > > > > + if (job->out_fence) > > > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > > here, if it's not already signaled. Otherwise, if there's someone > > > > waiting (which there shouldn't be), they're going to be waiting a very > > > > long time :-) > > > > > > > > Thanks, > > > > -Brian > > > > > > > Here it happened at atomic_check failed and test only commit. For both > > > cases, the commit has been dropped and it's only a clean up. So here better > > > not be treated as an error case:) > > > > If anyone else has a reference on the fence, then IMO it absolutely is > > an error to reach this point without the fence being signaled - > > because it means that the fence will never be signaled. > > > > I don't think the API gives you a way to check if this is the last > > reference, so it's safest to just make sure the fence is signalled > > before dropping the reference. > > > > It just feels wrong to me to have the possibility of a dangling fence > > which is never going to get signalled; and it's an easy defensive step > > to make sure it can never happen. > > > > I know it _shouldn't_ happen, but we often put in handling for cases > > which shouldn't happen, because they frequently do happen :-) > > We're not as paranoid with the vblank fences either, so not sure why > we need to be this paranoid with writeback fences. If your driver > grabs anything from the atomic state in ->atomic_check it's buggy > anyway. > > If you want to fix this properly I think we need to move the call to > prepare_signalling() in between atomic_check and atomic_commit. Then I > think it makes sense to also force-complete the fence on error ... > > > > Since for userspace, it should have been failed or a test only case, so > > > writebace fence should not be signaled. > > > > It's not only userspace that can wait on fences (and in fact this > > fence will never even reach userspace if the commit fails), the driver > > may have taken a copy to use for "something". I forgot to add: you can check this by looking at the fence reference count. A WARN_ON if that's more than 1 on cleanup (but also for the out fences) could be a nice addition. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-08-02 9:45 ` Daniel Vetter @ 2019-08-02 10:09 ` Brian Starkey 2019-08-02 14:06 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Brian Starkey @ 2019-08-02 10:09 UTC (permalink / raw) To: Daniel Vetter Cc: Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi Daniel, On Fri, Aug 02, 2019 at 11:45:13AM +0200, Daniel Vetter wrote: > On Fri, Aug 2, 2019 at 11:43 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Fri, Aug 2, 2019 at 11:29 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > > > > > Hi Lowry, > > > > > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > > > Hi Brian, > > > > > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > > > Hi Lowry, > > > > > > > > > > Thanks for this cleanup. > > > > > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > > > During it signals the completion of a writeback job, after releasing > > > > > > the out_fence, we'd clear the pointer. > > > > > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > > > > --- > > > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > > > index ff138b6..43d9e3b 100644 > > > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > > > if (job->fb) > > > > > > drm_framebuffer_put(job->fb); > > > > > > > > > > > > + if (job->out_fence) > > > > > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > > > here, if it's not already signaled. Otherwise, if there's someone > > > > > waiting (which there shouldn't be), they're going to be waiting a very > > > > > long time :-) > > > > > > > > > > Thanks, > > > > > -Brian > > > > > > > > > Here it happened at atomic_check failed and test only commit. For both > > > > cases, the commit has been dropped and it's only a clean up. So here better > > > > not be treated as an error case:) > > > > > > If anyone else has a reference on the fence, then IMO it absolutely is > > > an error to reach this point without the fence being signaled - > > > because it means that the fence will never be signaled. > > > > > > I don't think the API gives you a way to check if this is the last > > > reference, so it's safest to just make sure the fence is signalled > > > before dropping the reference. > > > > > > It just feels wrong to me to have the possibility of a dangling fence > > > which is never going to get signalled; and it's an easy defensive step > > > to make sure it can never happen. > > > > > > I know it _shouldn't_ happen, but we often put in handling for cases > > > which shouldn't happen, because they frequently do happen :-) > > > > We're not as paranoid with the vblank fences either, so not sure why > > we need to be this paranoid with writeback fences. If your driver > > grabs anything from the atomic state in ->atomic_check it's buggy > > anyway. > > > > If you want to fix this properly I think we need to move the call to > > prepare_signalling() in between atomic_check and atomic_commit. Then I > > think it makes sense to also force-complete the fence on error ... Well, fair enough. I'm struggling with "that's too paranoid" vs "fix it properly" though? Is it a "problem" worth fixing or not? It seems natural to me to do the fence cleanup in the cleanup function for the object which owns the fence. > > > > > > Since for userspace, it should have been failed or a test only case, so > > > > writebace fence should not be signaled. > > > > > > It's not only userspace that can wait on fences (and in fact this > > > fence will never even reach userspace if the commit fails), the driver > > > may have taken a copy to use for "something". > > I forgot to add: you can check this by looking at the fence reference > count. A WARN_ON if that's more than 1 on cleanup (but also for the > out fences) could be a nice addition. Do we really want to be looking at the fence internals directly like that? Cheers, -Brian > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-08-02 10:09 ` Brian Starkey @ 2019-08-02 14:06 ` Daniel Vetter 0 siblings, 0 replies; 16+ messages in thread From: Daniel Vetter @ 2019-08-02 14:06 UTC (permalink / raw) To: Brian Starkey Cc: Daniel Vetter, Lowry Li (Arm Technology China), Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Fri, Aug 02, 2019 at 10:09:05AM +0000, Brian Starkey wrote: > Hi Daniel, > > On Fri, Aug 02, 2019 at 11:45:13AM +0200, Daniel Vetter wrote: > > On Fri, Aug 2, 2019 at 11:43 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > On Fri, Aug 2, 2019 at 11:29 AM Brian Starkey <Brian.Starkey@arm.com> wrote: > > > > > > > > Hi Lowry, > > > > > > > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > > > > Hi Brian, > > > > > > > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > > > > Hi Lowry, > > > > > > > > > > > > Thanks for this cleanup. > > > > > > > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > > > > During it signals the completion of a writeback job, after releasing > > > > > > > the out_fence, we'd clear the pointer. > > > > > > > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > > > > index ff138b6..43d9e3b 100644 > > > > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > > > > if (job->fb) > > > > > > > drm_framebuffer_put(job->fb); > > > > > > > > > > > > > > + if (job->out_fence) > > > > > > > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > > > > here, if it's not already signaled. Otherwise, if there's someone > > > > > > waiting (which there shouldn't be), they're going to be waiting a very > > > > > > long time :-) > > > > > > > > > > > > Thanks, > > > > > > -Brian > > > > > > > > > > > Here it happened at atomic_check failed and test only commit. For both > > > > > cases, the commit has been dropped and it's only a clean up. So here better > > > > > not be treated as an error case:) > > > > > > > > If anyone else has a reference on the fence, then IMO it absolutely is > > > > an error to reach this point without the fence being signaled - > > > > because it means that the fence will never be signaled. > > > > > > > > I don't think the API gives you a way to check if this is the last > > > > reference, so it's safest to just make sure the fence is signalled > > > > before dropping the reference. > > > > > > > > It just feels wrong to me to have the possibility of a dangling fence > > > > which is never going to get signalled; and it's an easy defensive step > > > > to make sure it can never happen. > > > > > > > > I know it _shouldn't_ happen, but we often put in handling for cases > > > > which shouldn't happen, because they frequently do happen :-) > > > > > > We're not as paranoid with the vblank fences either, so not sure why > > > we need to be this paranoid with writeback fences. If your driver > > > grabs anything from the atomic state in ->atomic_check it's buggy > > > anyway. > > > > > > If you want to fix this properly I think we need to move the call to > > > prepare_signalling() in between atomic_check and atomic_commit. Then I > > > think it makes sense to also force-complete the fence on error ... > > Well, fair enough. I'm struggling with "that's too paranoid" vs "fix > it properly" though? Is it a "problem" worth fixing or not? Up to you to decide that. > It seems natural to me to do the fence cleanup in the cleanup function > for the object which owns the fence. > > > > > > > > > Since for userspace, it should have been failed or a test only case, so > > > > > writebace fence should not be signaled. > > > > > > > > It's not only userspace that can wait on fences (and in fact this > > > > fence will never even reach userspace if the commit fails), the driver > > > > may have taken a copy to use for "something". > > > > I forgot to add: you can check this by looking at the fence reference > > count. A WARN_ON if that's more than 1 on cleanup (but also for the > > out fences) could be a nice addition. > > Do we really want to be looking at the fence internals directly like > that? Wrap it up in a helper like dma_fence_release_private or whatever, which combines the check and (hopefully final) _put(). Might need a better name. -Daniel > > Cheers, > -Brian > > > -Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-08-02 9:29 ` Brian Starkey 2019-08-02 9:43 ` Daniel Vetter @ 2019-08-02 10:09 ` james qian wang (Arm Technology China) 1 sibling, 0 replies; 16+ messages in thread From: james qian wang (Arm Technology China) @ 2019-08-02 10:09 UTC (permalink / raw) To: Brian Starkey Cc: Lowry Li (Arm Technology China), Liviu Dudau, maarten.lankhorst, seanpaul, airlied, daniel, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd On Fri, Aug 02, 2019 at 05:29:20PM +0800, Brian Starkey wrote: > Hi Lowry, > > On Thu, Aug 01, 2019 at 06:34:08AM +0000, Lowry Li (Arm Technology China) wrote: > > Hi Brian, > > > > On Wed, Jul 31, 2019 at 09:20:04PM +0800, Brian Starkey wrote: > > > Hi Lowry, > > > > > > Thanks for this cleanup. > > > > > > On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > > > > During it signals the completion of a writeback job, after releasing > > > > the out_fence, we'd clear the pointer. > > > > > > > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > > > > > > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > > > > --- > > > > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > > > > index ff138b6..43d9e3b 100644 > > > > --- a/drivers/gpu/drm/drm_writeback.c > > > > +++ b/drivers/gpu/drm/drm_writeback.c > > > > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > > > > if (job->fb) > > > > drm_framebuffer_put(job->fb); > > > > > > > > + if (job->out_fence) > > > > > > I'm thinking it might be a good idea to signal the fence with an error > > > here, if it's not already signaled. Otherwise, if there's someone > > > waiting (which there shouldn't be), they're going to be waiting a very > > > long time :-) > > > > > > Thanks, > > > -Brian > > > > > Here it happened at atomic_check failed and test only commit. For both > > cases, the commit has been dropped and it's only a clean up. So here better > > not be treated as an error case:) > > If anyone else has a reference on the fence, then IMO it absolutely is > an error to reach this point without the fence being signaled - > because it means that the fence will never be signaled. > > I don't think the API gives you a way to check if this is the last > reference, so it's safest to just make sure the fence is signalled > before dropping the reference. > > It just feels wrong to me to have the possibility of a dangling fence > which is never going to get signalled; and it's an easy defensive step > to make sure it can never happen. > > I know it _shouldn't_ happen, but we often put in handling for cases > which shouldn't happen, because they frequently do happen :-) > > > > > Since for userspace, it should have been failed or a test only case, so > > writebace fence should not be signaled. > > It's not only userspace that can wait on fences (and in fact this > fence will never even reach userspace if the commit fails), the driver > may have taken a copy to use for "something". > Maybe we can add a wb_fence canceling into complete_signaling() for the atomic_check failed cleanup like the crtc->out_fence. Then if in this place we still can got a fence here, that must be a error we signal and WARN it Thanks James > Cheers, > -Brian > > > > > Best regards, > > Lowry > > > > + dma_fence_put(job->out_fence); > > > > + > > > > kfree(job); > > > > } > > > > -- > > Regards > > Lowry ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 2019-07-31 13:15 ` Liviu Dudau 2019-07-31 13:20 ` Brian Starkey @ 2019-08-05 13:10 ` Brian Starkey 2019-09-23 7:25 ` [v1,2/2] " james qian wang (Arm Technology China) 3 siblings, 0 replies; 16+ messages in thread From: Brian Starkey @ 2019-08-05 13:10 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, james qian wang (Arm Technology China), maarten.lankhorst, seanpaul, airlied, daniel, Julien Yin (Arm Technology China), maxime.ripard, eric, kieran.bingham+renesas, sean, laurent.pinchart, Jonathan Chai (Arm Technology China), Ayan Halder, dri-devel, linux-kernel, linux-renesas-soc, nd Hi Lowry, Based on Daniel's input, this patch looks fine: Reviewed-by: Brian Starkey <brian.starkey@arm.com> I think there's some opportunity for improvement around prepare_signaling/complete_signaling, but that can be treated as separate from fixing this bug. Thanks, -Brian On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > During it signals the completion of a writeback job, after releasing > the out_fence, we'd clear the pointer. > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > --- > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index ff138b6..43d9e3b 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > if (job->fb) > drm_framebuffer_put(job->fb); > > + if (job->out_fence) > + dma_fence_put(job->out_fence); > + > kfree(job); > } > EXPORT_SYMBOL(drm_writeback_cleanup_job); > @@ -366,25 +369,29 @@ static void cleanup_work(struct work_struct *work) > { > unsigned long flags; > struct drm_writeback_job *job; > + struct dma_fence *out_fence; > > spin_lock_irqsave(&wb_connector->job_lock, flags); > job = list_first_entry_or_null(&wb_connector->job_queue, > struct drm_writeback_job, > list_entry); > - if (job) { > + if (job) > list_del(&job->list_entry); > - if (job->out_fence) { > - if (status) > - dma_fence_set_error(job->out_fence, status); > - dma_fence_signal(job->out_fence); > - dma_fence_put(job->out_fence); > - } > - } > + > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > > if (WARN_ON(!job)) > return; > > + out_fence = job->out_fence; > + if (out_fence) { > + if (status) > + dma_fence_set_error(out_fence, status); > + dma_fence_signal(out_fence); > + dma_fence_put(out_fence); > + job->out_fence = NULL; > + } > + > INIT_WORK(&job->cleanup_work, cleanup_work); > queue_work(system_long_wq, &job->cleanup_work); > } > -- > 1.9.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [v1,2/2] drm: Clear the fence pointer when writeback job signaled 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) ` (2 preceding siblings ...) 2019-08-05 13:10 ` Brian Starkey @ 2019-09-23 7:25 ` james qian wang (Arm Technology China) 3 siblings, 0 replies; 16+ messages in thread From: james qian wang (Arm Technology China) @ 2019-09-23 7:25 UTC (permalink / raw) To: Lowry Li (Arm Technology China) Cc: Liviu Dudau, maarten.lankhorst, seanpaul, airlied, daniel, Brian Starkey, linux-renesas-soc, nd, maxime.ripard, Jonathan Chai (Arm Technology China), linux-kernel, dri-devel, Julien Yin (Arm Technology China), kieran.bingham+renesas, laurent.pinchart, Ayan Halder, sean On Wed, Jul 31, 2019 at 11:04:45AM +0000, Lowry Li (Arm Technology China) wrote: > During it signals the completion of a writeback job, after releasing > the out_fence, we'd clear the pointer. > > Check if fence left over in drm_writeback_cleanup_job(), release it. > > Signed-off-by: Lowry Li (Arm Technology China) <lowry.li@arm.com> > Reviewed-by: Brian Starkey <brian.starkey@arm.com> Looks good to me. Reviewed-by: James Qian Wang (Arm Technology China) <james.qian.wang@arm.com> will push it to drm-misc-fixes James > --- > drivers/gpu/drm/drm_writeback.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c > index ff138b6..43d9e3b 100644 > --- a/drivers/gpu/drm/drm_writeback.c > +++ b/drivers/gpu/drm/drm_writeback.c > @@ -324,6 +324,9 @@ void drm_writeback_cleanup_job(struct drm_writeback_job *job) > if (job->fb) > drm_framebuffer_put(job->fb); > > + if (job->out_fence) > + dma_fence_put(job->out_fence); > + > kfree(job); > } > EXPORT_SYMBOL(drm_writeback_cleanup_job); > @@ -366,25 +369,29 @@ static void cleanup_work(struct work_struct *work) > { > unsigned long flags; > struct drm_writeback_job *job; > + struct dma_fence *out_fence; > > spin_lock_irqsave(&wb_connector->job_lock, flags); > job = list_first_entry_or_null(&wb_connector->job_queue, > struct drm_writeback_job, > list_entry); > - if (job) { > + if (job) > list_del(&job->list_entry); > - if (job->out_fence) { > - if (status) > - dma_fence_set_error(job->out_fence, status); > - dma_fence_signal(job->out_fence); > - dma_fence_put(job->out_fence); > - } > - } > + > spin_unlock_irqrestore(&wb_connector->job_lock, flags); > > if (WARN_ON(!job)) > return; > > + out_fence = job->out_fence; > + if (out_fence) { > + if (status) > + dma_fence_set_error(out_fence, status); > + dma_fence_signal(out_fence); > + dma_fence_put(out_fence); > + job->out_fence = NULL; > + } > + > INIT_WORK(&job->cleanup_work, cleanup_work); > queue_work(system_long_wq, &job->cleanup_work); > } ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-09-23 7:25 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-31 11:04 [PATCH v1 0/2] Free the writeback_job when it with an empty fb Lowry Li (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 1/2] drm: " Lowry Li (Arm Technology China) 2019-07-31 13:03 ` Liviu Dudau 2019-09-23 7:24 ` [v1,1/2] " james qian wang (Arm Technology China) 2019-07-31 11:04 ` [PATCH v1 2/2] drm: Clear the fence pointer when writeback job signaled Lowry Li (Arm Technology China) 2019-07-31 13:15 ` Liviu Dudau [not found] ` <20190801063055.GA17887@lowry.li@arm.com> 2019-08-01 9:58 ` Liviu Dudau 2019-07-31 13:20 ` Brian Starkey [not found] ` <20190801063351.GB17887@lowry.li@arm.com> 2019-08-02 9:29 ` Brian Starkey 2019-08-02 9:43 ` Daniel Vetter 2019-08-02 9:45 ` Daniel Vetter 2019-08-02 10:09 ` Brian Starkey 2019-08-02 14:06 ` Daniel Vetter 2019-08-02 10:09 ` james qian wang (Arm Technology China) 2019-08-05 13:10 ` Brian Starkey 2019-09-23 7:25 ` [v1,2/2] " james qian wang (Arm Technology China)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).