[PATCH 1/8] drm/etnaviv: return context from etnaviv_iommu_context_get

[PATCH 1/8] drm/etnaviv: return context from etnaviv_iommu_context_get

[Lucas Stach]

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,
-- 
2.30.2

[PATCH 2/8] drm/etnaviv: put submit prev MMU context when it exists

[Lucas Stach]

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);
 	}
 
-- 
2.30.2

[PATCH 3/8] drm/etnaviv: stop abusing mmu_context as FE running marker

[Lucas Stach]

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;
-- 
2.30.2

[PATCH 4/8] drm/etnaviv: keep MMU context across runtime suspend/resume

[Lucas Stach]

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);
-- 
2.30.2

[PATCH 5/8] drm/etnaviv: exec and MMU state is lost when resetting the GPU

[Lucas Stach]

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);
-- 
2.30.2

[PATCH 6/8] drm/etnaviv: fix MMU context leak on GPU reset

[Lucas Stach]

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;
-- 
2.30.2

[PATCH 7/8] drm/etnaviv: reference MMU context when setting up hardware state

[Lucas Stach]

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,
-- 
2.30.2

[PATCH 8/8] drm/etnaviv: add missing MMU context put when reaping MMU mapping

[Lucas Stach]

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);
-- 
2.30.2

Re: [PATCH 7/8] drm/etnaviv: reference MMU context when setting up hardware state

[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?

>         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
>


-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH 7/8] drm/etnaviv: reference MMU context when setting up hardware state

[Lucas Stach]

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
> > 
> 
> 

Re: [PATCH 7/8] drm/etnaviv: reference MMU context when setting up hardware state

[Christian Gmeiner]

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.

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH 8/8] drm/etnaviv: add missing MMU context put when reaping MMU mapping

[Christian Gmeiner]

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
>


-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy