Re: [PATCH v2 8/8] drm/etnaviv: implement per-process address spaces on MMUv2

On Fri, 2019-07-05 at 19:17 +0200, Lucas Stach wrote:

This builds on top of the MMU contexts introduced earlier. Instead of having one context per GPU core, each GPU client receives its own context.

On MMUv1 this still means a single shared pagetable set is used by all clients, but on MMUv2 there is now a distinct set of pagetables for each client. As the command fetch is also translated via the MMU on MMUv2 the kernel command ringbuffer is mapped into each of the client pagetables.

As the MMU context switch is a bit of a heavy operation, due to the needed cache and TLB flushing, this patch implements a lazy way of switching the MMU context. The kernel does not have its own MMU context, but reuses the last client context for all of its operations. This has some visible impact, as the GPU can now only be started once a client has submitted some work and we got the client MMU context assigned. Also the MMU context has a different lifetime than the general client context, as the GPU might still execute the kernel command buffer in the context of a client even after the client has completed all GPU work and has been terminated. Only when the GPU is runtime suspended or switches to another clients MMU context is the old context freed up.

Signed-off-by: Lucas Stach l.stach@pengutronix.de

Reviewed-by: Philipp Zabel p.zabel@pengutronix.de

I just have two nitpicks below:

---

drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 59 ++++++++--- drivers/gpu/drm/etnaviv/etnaviv_drv.c | 38 ++++++- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 6 +- drivers/gpu/drm/etnaviv/etnaviv_dump.c | 4 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 5 +- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 4 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 10 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 100 ++++++++----------- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 4 - drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 10 +- drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 17 +++- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 42 ++++++-- drivers/gpu/drm/etnaviv/etnaviv_mmu.h | 11 +- 13 files changed, 199 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c index 022134238184..9bdebe045a31 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c

[...]

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c index cf49f0e2e1cb..99c20094295c 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c @@ -290,6 +290,8 @@ static void etnaviv_iommu_context_free(struct kref *kref) struct etnaviv_iommu_context *context = container_of(kref, struct etnaviv_iommu_context, refcount);

• etnaviv_cmdbuf_suballoc_unmap(context, &context->cmdbuf_mapping);
• context->global->ops->free(context);

} void etnaviv_iommu_context_put(struct etnaviv_iommu_context *context) @@ -298,12 +300,28 @@ void etnaviv_iommu_context_put(struct etnaviv_iommu_context *context) }

struct etnaviv_iommu_context * -etnaviv_iommu_context_init(struct etnaviv_iommu_global *global) +etnaviv_iommu_context_init(struct etnaviv_iommu_global *global,

• 	   struct etnaviv_cmdbuf_suballoc *suballoc)
  

{

• struct etnaviv_iommu_context *ctx;
• int ret;
• if (global->version == ETNAVIV_IOMMU_V1)

• return etnaviv_iommuv1_context_alloc(global);
  

• ctx = etnaviv_iommuv1_context_alloc(global);
  

  else

• return etnaviv_iommuv2_context_alloc(global);
  

• ctx = etnaviv_iommuv2_context_alloc(global);
  

• if (!ctx)

• return NULL;
  

• ret = etnaviv_cmdbuf_suballoc_map(suballoc, ctx, &ctx->cmdbuf_mapping,

• 			  global->memory_base);
  

• if (ret) {

• etnaviv_iommu_context_put(ctx);
  

This will call etnaviv_cmdbuf_suballoc_unmap in etnaviv_iommu_context_free above, even though etnaviv_cmdbuf_suballoc_map didn't succeed. See below.

• return NULL;
  

• }
• return ctx;

}

void etnaviv_iommu_restore(struct etnaviv_gpu *gpu, @@ -319,6 +337,12 @@ int etnaviv_iommu_get_suballoc_va(struct etnaviv_iommu_context *context, { mutex_lock(&context->lock);

• if (mapping->use > 0) {

• mapping->use++;
  

• mutex_unlock(&context->lock);
  

• return 0;
  

• }
• /*
  • For MMUv1 we don't add the suballoc region to the pagetables, as
  • those GPUs can only work with cmdbufs accessed through the linear

@@ -341,7 +365,6 @@ int etnaviv_iommu_get_suballoc_va(struct etnaviv_iommu_context *context, mapping->iova = node->start; ret = etnaviv_context_map(context, node->start, paddr, size, ETNAVIV_PROT_READ);

Maybe squash this into "drm/etnaviv: split out cmdbuf mapping into address space" instead.

if (ret < 0) {
	drm_mm_remove_node(node);
	mutex_unlock(&context->lock);

@@ -364,15 +387,14 @@ void etnaviv_iommu_put_suballoc_va(struct etnaviv_iommu_context *context, { struct drm_mm_node *node = &mapping->vram_node;

• if (!mapping->use)

• return;
  

• mapping->use = 0;

• mutex_lock(&context->lock);
• mapping->use--;

See above, when called from the etnaviv_iommu_context_init error path, mapping->use wraps from 0 to UINT_MAX ...

• if (context->global->version == ETNAVIV_IOMMU_V1)

• if (mapping->use > 0 || context->global->version == ETNAVIV_IOMMU_V1) {

• mutex_unlock(&context->lock);
  

... which is > 0, so we return here.

This works out, but it does look a bit weird.

regards Philipp