Being able to have the refcount manipulation in an assignment makes it much easier to parse the code.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 3 +-- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 3 +-- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 3 +-- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 6 ++---- drivers/gpu/drm/etnaviv/etnaviv_mmu.h | 4 +++- 5 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c index 76d38561c910..cf741c5c82d2 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c @@ -397,8 +397,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state, if (switch_mmu_context) { struct etnaviv_iommu_context *old_context = gpu->mmu_context;
- etnaviv_iommu_context_get(mmu_context); - gpu->mmu_context = mmu_context; + gpu->mmu_context = etnaviv_iommu_context_get(mmu_context); etnaviv_iommu_context_put(old_context); }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index b8fa6ed3dd73..fb7a33b88fc0 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -303,8 +303,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( list_del(&mapping->obj_node); }
- etnaviv_iommu_context_get(mmu_context); - mapping->context = mmu_context; + mapping->context = etnaviv_iommu_context_get(mmu_context); mapping->use = 1;
ret = etnaviv_iommu_map_gem(mmu_context, etnaviv_obj, diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..486259e154af 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -532,8 +532,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, goto err_submit_objects;
submit->ctx = file->driver_priv; - etnaviv_iommu_context_get(submit->ctx->mmu); - submit->mmu_context = submit->ctx->mmu; + submit->mmu_context = etnaviv_iommu_context_get(submit->ctx->mmu); submit->exec_state = args->exec_state; submit->flags = args->flags;
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 4102bcea3341..c8b9b0cc4442 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1365,12 +1365,10 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) }
if (!gpu->mmu_context) { - etnaviv_iommu_context_get(submit->mmu_context); - gpu->mmu_context = submit->mmu_context; + gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context); etnaviv_gpu_start_fe_idleloop(gpu); } else { - etnaviv_iommu_context_get(gpu->mmu_context); - submit->prev_mmu_context = gpu->mmu_context; + submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); }
if (submit->nr_pmrs) { diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.h b/drivers/gpu/drm/etnaviv/etnaviv_mmu.h index d1d6902fd13b..e4a0b7d09c2e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.h @@ -105,9 +105,11 @@ void etnaviv_iommu_dump(struct etnaviv_iommu_context *ctx, void *buf); struct etnaviv_iommu_context * etnaviv_iommu_context_init(struct etnaviv_iommu_global *global, struct etnaviv_cmdbuf_suballoc *suballoc); -static inline void etnaviv_iommu_context_get(struct etnaviv_iommu_context *ctx) +static inline struct etnaviv_iommu_context * +etnaviv_iommu_context_get(struct etnaviv_iommu_context *ctx) { kref_get(&ctx->refcount); + return ctx; } void etnaviv_iommu_context_put(struct etnaviv_iommu_context *ctx); void etnaviv_iommu_restore(struct etnaviv_gpu *gpu,
The prev context is the MMU context at the time of the job queueing in hardware. As a job might be queued multiple times due to recovery after a GPU hang, we need to make sure to put the stale prev MMU context from a prior queuing, to avoid the reference and thus the MMU context leaking.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index c8b9b0cc4442..c1b9c5cbed11 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1368,6 +1368,8 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context); etnaviv_gpu_start_fe_idleloop(gpu); } else { + if (submit->prev_mmu_context) + etnaviv_iommu_context_put(submit->prev_mmu_context); submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); }
While the DMA frontend can only be active when the MMU context is set, the reverse isn't necessarily true, as the frontend can be stopped while the MMU state is kept. Stop treating mmu_context being set as a indication that the frontend is running and instead add a explicit property.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 10 ++++++++-- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 1 + 2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index c1b9c5cbed11..325858cfc2c3 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -569,6 +569,8 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu) /* We rely on the GPU running, so program the clock */ etnaviv_gpu_update_clock(gpu);
+ gpu->fe_running = false; + return 0; }
@@ -631,6 +633,8 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch) VIVS_MMUv2_SEC_COMMAND_CONTROL_ENABLE | VIVS_MMUv2_SEC_COMMAND_CONTROL_PREFETCH(prefetch)); } + + gpu->fe_running = true; }
static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu) @@ -1364,7 +1368,7 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) goto out_unlock; }
- if (!gpu->mmu_context) { + if (!gpu->fe_running) { gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context); etnaviv_gpu_start_fe_idleloop(gpu); } else { @@ -1573,7 +1577,7 @@ int etnaviv_gpu_wait_idle(struct etnaviv_gpu *gpu, unsigned int timeout_ms)
static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu) { - if (gpu->initialized && gpu->mmu_context) { + if (gpu->initialized && gpu->fe_running) { /* Replace the last WAIT with END */ mutex_lock(&gpu->lock); etnaviv_buffer_end(gpu); @@ -1588,6 +1592,8 @@ static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu)
etnaviv_iommu_context_put(gpu->mmu_context); gpu->mmu_context = NULL; + + gpu->fe_running = false; }
gpu->exec_state = -1; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h index 8ea48697d132..1c75c8ed5bce 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h @@ -101,6 +101,7 @@ struct etnaviv_gpu { struct workqueue_struct *wq; struct drm_gpu_scheduler sched; bool initialized; + bool fe_running;
/* 'ring'-buffer: */ struct etnaviv_cmdbuf buffer;
The MMU state may be kept across a runtime suspend/resume cycle, as we avoid a full hardware reset to keep the latency of the runtime PM small.
Don't pretend that the MMU state is lost in driver state. The MMU context is pushed out when new HW jobs with a different context are coming in. The only exception to this is when the GPU is unbound, in which case we need to make sure to also free the last active context.
Cc: stable@vger.kernel.org # 5.4 Reported-by: Michael Walle michael@walle.cc Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 325858cfc2c3..973843c35fca 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -1590,9 +1590,6 @@ static int etnaviv_gpu_hw_suspend(struct etnaviv_gpu *gpu) */ etnaviv_gpu_wait_idle(gpu, 100);
- etnaviv_iommu_context_put(gpu->mmu_context); - gpu->mmu_context = NULL; - gpu->fe_running = false; }
@@ -1741,6 +1738,9 @@ static void etnaviv_gpu_unbind(struct device *dev, struct device *master, etnaviv_gpu_hw_suspend(gpu); #endif
+ if (gpu->mmu_context) + etnaviv_iommu_context_put(gpu->mmu_context); + if (gpu->initialized) { etnaviv_cmdbuf_free(&gpu->buffer); etnaviv_iommu_global_fini(gpu);
When the GPU is reset both the current exec state, as well as all MMU state is lost. Move the driver side state tracking into the reset function to keep hardware and software state from diverging.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 973843c35fca..9c710924df6b 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -570,6 +570,8 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu) etnaviv_gpu_update_clock(gpu);
gpu->fe_running = false; + gpu->exec_state = -1; + gpu->mmu_context = NULL;
return 0; } @@ -830,7 +832,6 @@ int etnaviv_gpu_init(struct etnaviv_gpu *gpu) /* Now program the hardware */ mutex_lock(&gpu->lock); etnaviv_gpu_hw_init(gpu); - gpu->exec_state = -1; mutex_unlock(&gpu->lock);
pm_runtime_mark_last_busy(gpu->dev); @@ -1055,8 +1056,6 @@ void etnaviv_gpu_recover_hang(struct etnaviv_gpu *gpu) spin_unlock(&gpu->event_spinlock);
etnaviv_gpu_hw_init(gpu); - gpu->exec_state = -1; - gpu->mmu_context = NULL;
mutex_unlock(&gpu->lock); pm_runtime_mark_last_busy(gpu->dev);
After a reset the GPU is no longer using the MMU context and may be restarted with a different context. While the mmu_state proeprly was cleared, the context wasn't unreferenced, leading to a memory leak.
Cc: stable@vger.kernel.org # 5.4 Reported-by: Michael Walle michael@walle.cc Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 9c710924df6b..f420c4f14657 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -571,6 +571,8 @@ static int etnaviv_hw_reset(struct etnaviv_gpu *gpu)
gpu->fe_running = false; gpu->exec_state = -1; + if (gpu->mmu_context) + etnaviv_iommu_context_put(gpu->mmu_context); gpu->mmu_context = NULL;
return 0;
Move the refcount manipulation of the MMU context to the point where the hardware state is programmed. At that point it is also known if a previous MMU state is still there, or the state needs to be reprogrammed with a potentially different context.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 24 +++++++++++----------- drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 4 ++++ drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 8 ++++++++ 3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index f420c4f14657..1fa98ce870f7 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -641,17 +641,19 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch) gpu->fe_running = true; }
-static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu) +static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu, + struct etnaviv_iommu_context *context) { - u32 address = etnaviv_cmdbuf_get_va(&gpu->buffer, - &gpu->mmu_context->cmdbuf_mapping); u16 prefetch; + u32 address;
/* setup the MMU */ - etnaviv_iommu_restore(gpu, gpu->mmu_context); + etnaviv_iommu_restore(gpu, context);
/* Start command processor */ prefetch = etnaviv_buffer_init(gpu); + address = etnaviv_cmdbuf_get_va(&gpu->buffer, + &gpu->mmu_context->cmdbuf_mapping);
etnaviv_gpu_start_fe(gpu, address, prefetch); } @@ -1369,14 +1371,12 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) goto out_unlock; }
- if (!gpu->fe_running) { - gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context); - etnaviv_gpu_start_fe_idleloop(gpu); - } else { - if (submit->prev_mmu_context) - etnaviv_iommu_context_put(submit->prev_mmu_context); - submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); - } + if (!gpu->fe_running) + etnaviv_gpu_start_fe_idleloop(gpu, submit->mmu_context); + + if (submit->prev_mmu_context) + etnaviv_iommu_context_put(submit->prev_mmu_context); + submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context);
if (submit->nr_pmrs) { gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c index 1a7c89a67bea..afe5dd6a9925 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c @@ -92,6 +92,10 @@ static void etnaviv_iommuv1_restore(struct etnaviv_gpu *gpu, struct etnaviv_iommuv1_context *v1_context = to_v1_context(context); u32 pgtable;
+ if (gpu->mmu_context) + etnaviv_iommu_context_put(gpu->mmu_context); + gpu->mmu_context = etnaviv_iommu_context_get(context); + /* set base addresses */ gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_RA, context->global->memory_base); gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_FE, context->global->memory_base); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c index f8bf488e9d71..d664ae29ae20 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c @@ -172,6 +172,10 @@ static void etnaviv_iommuv2_restore_nonsec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_CONTROL) & VIVS_MMUv2_CONTROL_ENABLE) return;
+ if (gpu->mmu_context) + etnaviv_iommu_context_put(gpu->mmu_context); + gpu->mmu_context = etnaviv_iommu_context_get(context); + prefetch = etnaviv_buffer_config_mmuv2(gpu, (u32)v2_context->mtlb_dma, (u32)context->global->bad_page_dma); @@ -192,6 +196,10 @@ static void etnaviv_iommuv2_restore_sec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_SEC_CONTROL) & VIVS_MMUv2_SEC_CONTROL_ENABLE) return;
+ if (gpu->mmu_context) + etnaviv_iommu_context_put(gpu->mmu_context); + gpu->mmu_context = etnaviv_iommu_context_get(context); + gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_LOW, lower_32_bits(context->global->v2.pta_dma)); gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_HIGH,
Am Fr., 20. Aug. 2021 um 22:18 Uhr schrieb Lucas Stach l.stach@pengutronix.de:
Move the refcount manipulation of the MMU context to the point where the hardware state is programmed. At that point it is also known if a previous MMU state is still there, or the state needs to be reprogrammed with a potentially different context.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 24 +++++++++++----------- drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 4 ++++ drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 8 ++++++++ 3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index f420c4f14657..1fa98ce870f7 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -641,17 +641,19 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch) gpu->fe_running = true; }
-static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu) +static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu,
struct etnaviv_iommu_context *context){
u32 address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); u16 prefetch;
u32 address; /* setup the MMU */
etnaviv_iommu_restore(gpu, gpu->mmu_context);
etnaviv_iommu_restore(gpu, context); /* Start command processor */ prefetch = etnaviv_buffer_init(gpu);address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); etnaviv_gpu_start_fe(gpu, address, prefetch);} @@ -1369,14 +1371,12 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) goto out_unlock; }
if (!gpu->fe_running) {gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context);etnaviv_gpu_start_fe_idleloop(gpu);} else {if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context);}
if (!gpu->fe_running)etnaviv_gpu_start_fe_idleloop(gpu, submit->mmu_context);if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); if (submit->nr_pmrs) { gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c index 1a7c89a67bea..afe5dd6a9925 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c @@ -92,6 +92,10 @@ static void etnaviv_iommuv1_restore(struct etnaviv_gpu *gpu, struct etnaviv_iommuv1_context *v1_context = to_v1_context(context); u32 pgtable;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);/* set base addresses */ gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_RA, context->global->memory_base); gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_FE, context->global->memory_base);diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c index f8bf488e9d71..d664ae29ae20 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c @@ -172,6 +172,10 @@ static void etnaviv_iommuv2_restore_nonsec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_CONTROL) & VIVS_MMUv2_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);prefetch = etnaviv_buffer_config_mmuv2(gpu, (u32)v2_context->mtlb_dma, (u32)context->global->bad_page_dma);@@ -192,6 +196,10 @@ static void etnaviv_iommuv2_restore_sec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_SEC_CONTROL) & VIVS_MMUv2_SEC_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);
I have seen this pattern now more than two times - maybe put the assignment of a new mmu context into its own function?
gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_LOW, lower_32_bits(context->global->v2.pta_dma)); gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_HIGH,-- 2.30.2
Am Dienstag, dem 24.08.2021 um 09:24 +0200 schrieb Christian Gmeiner:
Am Fr., 20. Aug. 2021 um 22:18 Uhr schrieb Lucas Stach l.stach@pengutronix.de:
Move the refcount manipulation of the MMU context to the point where the hardware state is programmed. At that point it is also known if a previous MMU state is still there, or the state needs to be reprogrammed with a potentially different context.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 24 +++++++++++----------- drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 4 ++++ drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 8 ++++++++ 3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index f420c4f14657..1fa98ce870f7 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -641,17 +641,19 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch) gpu->fe_running = true; }
-static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu) +static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu,
struct etnaviv_iommu_context *context){
u32 address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); u16 prefetch;
u32 address; /* setup the MMU */
etnaviv_iommu_restore(gpu, gpu->mmu_context);
etnaviv_iommu_restore(gpu, context); /* Start command processor */ prefetch = etnaviv_buffer_init(gpu);address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); etnaviv_gpu_start_fe(gpu, address, prefetch);} @@ -1369,14 +1371,12 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) goto out_unlock; }
if (!gpu->fe_running) {gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context);etnaviv_gpu_start_fe_idleloop(gpu);} else {if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context);}
if (!gpu->fe_running)etnaviv_gpu_start_fe_idleloop(gpu, submit->mmu_context);if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); if (submit->nr_pmrs) { gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c index 1a7c89a67bea..afe5dd6a9925 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c @@ -92,6 +92,10 @@ static void etnaviv_iommuv1_restore(struct etnaviv_gpu *gpu, struct etnaviv_iommuv1_context *v1_context = to_v1_context(context); u32 pgtable;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);/* set base addresses */ gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_RA, context->global->memory_base); gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_FE, context->global->memory_base);diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c index f8bf488e9d71..d664ae29ae20 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c @@ -172,6 +172,10 @@ static void etnaviv_iommuv2_restore_nonsec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_CONTROL) & VIVS_MMUv2_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);prefetch = etnaviv_buffer_config_mmuv2(gpu, (u32)v2_context->mtlb_dma, (u32)context->global->bad_page_dma);@@ -192,6 +196,10 @@ static void etnaviv_iommuv2_restore_sec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_SEC_CONTROL) & VIVS_MMUv2_SEC_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);I have seen this pattern now more than two times - maybe put the assignment of a new mmu context into its own function?
Yea, I thought about having some Gallium style reference handling functions, but that would change the code even more. Since I intend to have this series go into stable I wanted to keep the changes to a minimum for now. I was already on the fence with the first patch in this series, but that one provides very obvious legibility improvements, making it easier to review this series.
Would you agree to leave it like that for the stable series and clean it up in a follow up change?
Regards, Lucas
gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_LOW, lower_32_bits(context->global->v2.pta_dma)); gpu_write(gpu, VIVS_MMUv2_PTA_ADDRESS_HIGH,-- 2.30.2
Hi Lucas
Am Di., 24. Aug. 2021 um 09:54 Uhr schrieb Lucas Stach l.stach@pengutronix.de:
Am Dienstag, dem 24.08.2021 um 09:24 +0200 schrieb Christian Gmeiner:
Am Fr., 20. Aug. 2021 um 22:18 Uhr schrieb Lucas Stach l.stach@pengutronix.de:
Move the refcount manipulation of the MMU context to the point where the hardware state is programmed. At that point it is also known if a previous MMU state is still there, or the state needs to be reprogrammed with a potentially different context.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 24 +++++++++++----------- drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 4 ++++ drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 8 ++++++++ 3 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index f420c4f14657..1fa98ce870f7 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c @@ -641,17 +641,19 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32 address, u16 prefetch) gpu->fe_running = true; }
-static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu) +static void etnaviv_gpu_start_fe_idleloop(struct etnaviv_gpu *gpu,
struct etnaviv_iommu_context *context){
u32 address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); u16 prefetch;
u32 address; /* setup the MMU */
etnaviv_iommu_restore(gpu, gpu->mmu_context);
etnaviv_iommu_restore(gpu, context); /* Start command processor */ prefetch = etnaviv_buffer_init(gpu);address = etnaviv_cmdbuf_get_va(&gpu->buffer,&gpu->mmu_context->cmdbuf_mapping); etnaviv_gpu_start_fe(gpu, address, prefetch);} @@ -1369,14 +1371,12 @@ struct dma_fence *etnaviv_gpu_submit(struct etnaviv_gem_submit *submit) goto out_unlock; }
if (!gpu->fe_running) {gpu->mmu_context = etnaviv_iommu_context_get(submit->mmu_context);etnaviv_gpu_start_fe_idleloop(gpu);} else {if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context);}
if (!gpu->fe_running)etnaviv_gpu_start_fe_idleloop(gpu, submit->mmu_context);if (submit->prev_mmu_context)etnaviv_iommu_context_put(submit->prev_mmu_context);submit->prev_mmu_context = etnaviv_iommu_context_get(gpu->mmu_context); if (submit->nr_pmrs) { gpu->event[event[1]].sync_point = &sync_point_perfmon_sample_pre;diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c index 1a7c89a67bea..afe5dd6a9925 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu.c @@ -92,6 +92,10 @@ static void etnaviv_iommuv1_restore(struct etnaviv_gpu *gpu, struct etnaviv_iommuv1_context *v1_context = to_v1_context(context); u32 pgtable;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);/* set base addresses */ gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_RA, context->global->memory_base); gpu_write(gpu, VIVS_MC_MEMORY_BASE_ADDR_FE, context->global->memory_base);diff --git a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c index f8bf488e9d71..d664ae29ae20 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c @@ -172,6 +172,10 @@ static void etnaviv_iommuv2_restore_nonsec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_CONTROL) & VIVS_MMUv2_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);prefetch = etnaviv_buffer_config_mmuv2(gpu, (u32)v2_context->mtlb_dma, (u32)context->global->bad_page_dma);@@ -192,6 +196,10 @@ static void etnaviv_iommuv2_restore_sec(struct etnaviv_gpu *gpu, if (gpu_read(gpu, VIVS_MMUv2_SEC_CONTROL) & VIVS_MMUv2_SEC_CONTROL_ENABLE) return;
if (gpu->mmu_context)etnaviv_iommu_context_put(gpu->mmu_context);gpu->mmu_context = etnaviv_iommu_context_get(context);I have seen this pattern now more than two times - maybe put the assignment of a new mmu context into its own function?
Yea, I thought about having some Gallium style reference handling functions, but that would change the code even more. Since I intend to have this series go into stable I wanted to keep the changes to a minimum for now. I was already on the fence with the first patch in this series, but that one provides very obvious legibility improvements, making it easier to review this series.
Would you agree to leave it like that for the stable series and clean it up in a follow up change?
Sure... sounds like a good plan.
When we forcefully evict a mapping from the the address space and thus the MMU context, the MMU context is leaked, as the mapping no longer points to it, so it doesn't get freed when the GEM object is destroyed. Add the mssing context put to fix the leak.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc --- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c index dab1b58006d8..9fb1a2aadbcb 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c @@ -199,6 +199,7 @@ static int etnaviv_iommu_find_iova(struct etnaviv_iommu_context *context, */ list_for_each_entry_safe(m, n, &list, scan_node) { etnaviv_iommu_remove_mapping(context, m); + etnaviv_iommu_context_put(m->context); m->context = NULL; list_del_init(&m->mmu_node); list_del_init(&m->scan_node);
Am Fr., 20. Aug. 2021 um 22:18 Uhr schrieb Lucas Stach l.stach@pengutronix.de:
When we forcefully evict a mapping from the the address space and thus the MMU context, the MMU context is leaked, as the mapping no longer points to it, so it doesn't get freed when the GEM object is destroyed. Add the mssing context put to fix the leak.
Cc: stable@vger.kernel.org # 5.4 Signed-off-by: Lucas Stach l.stach@pengutronix.de Tested-by: Michael Walle michael@walle.cc
Series is: Reviewed-by: Christian Gmeiner christian.gmeiner@gmail.com
drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c index dab1b58006d8..9fb1a2aadbcb 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c @@ -199,6 +199,7 @@ static int etnaviv_iommu_find_iova(struct etnaviv_iommu_context *context, */ list_for_each_entry_safe(m, n, &list, scan_node) { etnaviv_iommu_remove_mapping(context, m);
etnaviv_iommu_context_put(m->context); m->context = NULL; list_del_init(&m->mmu_node); list_del_init(&m->scan_node);-- 2.30.2
dri-devel@lists.freedesktop.org
-
Christian Gmeiner -
Lucas Stach