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(-)
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;
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
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;
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); }
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
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.
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! /
¯\_(ツ)_/¯
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);
}
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:)
Since for userspace, it should have been failed or a test only case, so writebace fence should not be signaled.
Best regards, Lowry
dma_fence_put(job->out_fence);- kfree(job);
}
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
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);
}
dri-devel@lists.freedesktop.org
-
Brian Starkey -
james qian wang (Arm Technology China) -
Liviu Dudau -
Lowry Li (Arm Technology China)