Re: [PATCH v2 5/8] drm/etnaviv: rework MMU handling

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

This reworks the MMU handling to make it possible to have multiple MMU contexts. A context is basically one instance of GPU page tables. Currently we have one set of page tables per GPU, which isn't all that clever, as it has the following two consequences:

1. All GPU clients (aka processes) are sharing the same pagetables, which means

there is no isolation between clients, but only between GPU assigned memory spaces and the rest of the system. Better than nothing, but also not great.

2. Clients operating on the same set of buffers with different etnaviv GPU

cores, e.g. a workload using both the 2D and 3D GPU, need to map the used buffers into the pagetable sets of each used GPU.

This patch reworks all the MMU handling to introduce the abstraction of the MMU context. A context can be shared across different GPU cores, as long as they have compatible MMU implementations, which is the case for all systems with Vivante GPUs seen in the wild.

As MMUv1 is not able to change pagetables on the fly, without a "stop the world" operation, which stops GPU, changes pagetables via CPU interaction, restarts GPU, the implementation introduces a shared context on MMUv1, which is returned whenever there is a request for a new context.

This patch assigns a MMU context to each GPU, so on MMUv2 systems there is still one set of pagetables per GPU, but due to the shared context MMUv1 systems see a change in behavior as now a single pagetable set is used across all GPU cores.

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

drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 8 +- drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.c | 8 +- drivers/gpu/drm/etnaviv/etnaviv_cmdbuf.h | 6 +- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 4 +- drivers/gpu/drm/etnaviv/etnaviv_dump.c | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +- drivers/gpu/drm/etnaviv/etnaviv_gem.h | 2 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 20 +- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 3 +- drivers/gpu/drm/etnaviv/etnaviv_iommu.c | 151 ++++++------ drivers/gpu/drm/etnaviv/etnaviv_iommu.h | 20 -- drivers/gpu/drm/etnaviv/etnaviv_iommu_v2.c | 264 +++++++++------------ drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 264 +++++++++++++-------- drivers/gpu/drm/etnaviv/etnaviv_mmu.h | 88 +++++-- 14 files changed, 441 insertions(+), 413 deletions(-) delete mode 100644 drivers/gpu/drm/etnaviv/etnaviv_iommu.h

[...]

--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -223,12 +223,12 @@ int etnaviv_gem_mmap_offset(struct drm_gem_object *obj, u64 *offset)

static struct etnaviv_vram_mapping * etnaviv_gem_get_vram_mapping(struct etnaviv_gem_object *obj,

• 	     struct etnaviv_iommu *mmu)
  

• 	     struct etnaviv_iommu_context *context)
  

{ struct etnaviv_vram_mapping *mapping;

list_for_each_entry(mapping, &obj->vram_list, obj_node) {

• if (mapping->mmu == mmu)
  

• if (mapping->context == context)
  return mapping;
  

  }

@@ -266,7 +266,7 @@ struct etnaviv_vram_mapping *etnaviv_gem_mapping_get( */ if (mapping->use == 0) { mutex_lock(&gpu->mmu->lock);

• 	if (mapping->mmu == gpu->mmu)
  

• 	if (mapping->context == gpu->mmu)
  

Is there a reason that mmu parameters and mapping->mmu are renamed to context, but gpu->mmu is not?

Could be renamed to gpu->mmu_context for consistency.

[...]

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c index 7b396ac5dba5..a53fecd17fa9 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c

[...]

@@ -1684,11 +1690,11 @@ static void etnaviv_gpu_unbind(struct device *dev, struct device *master, if (gpu->initialized) { etnaviv_cmdbuf_free(&gpu->buffer); etnaviv_cmdbuf_suballoc_unmap(gpu->mmu, &gpu->cmdbuf_mapping);

• etnaviv_iommu_destroy(gpu->mmu);
  

• etnaviv_iommu_context_put(gpu->mmu);
  

• etnaviv_iommu_global_fini(gpu);
  

  gpu->initialized = false; }

This should be fixed in a previous patch.

[...]

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c index 3348d9962177..cf49f0e2e1cb 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c

[...]

@@ -391,21 +369,107 @@ void etnaviv_iommu_put_suballoc_va(struct etnaviv_iommu *mmu,

mapping->use = 0;

• if (mmu->version == ETNAVIV_IOMMU_V1)

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

• mutex_lock(&mmu->lock);
• etnaviv_domain_unmap(mmu->domain, node->start, node->size);

• mutex_lock(&context->lock);
• etnaviv_context_unmap(context, node->start, node->size); drm_mm_remove_node(node);

• mutex_unlock(&mmu->lock);

• mutex_unlock(&context->lock);

+}

+size_t etnaviv_iommu_dump_size(struct etnaviv_iommu_context *context) +{

• return context->global->ops->dump_size(context);

}

-size_t etnaviv_iommu_dump_size(struct etnaviv_iommu *iommu) +void etnaviv_iommu_dump(struct etnaviv_iommu_context *context, void *buf) {

• return iommu->domain->ops->dump_size(iommu->domain);

• context->global->ops->dump(context, buf);

}

-void etnaviv_iommu_dump(struct etnaviv_iommu *iommu, void *buf) +extern const struct etnaviv_iommu_ops etnaviv_iommuv1_ops; +extern const struct etnaviv_iommu_ops etnaviv_iommuv2_ops;

These should be moved into a header that is also included where the ops are defined.

Apart from this, Reviewed-by: Philipp Zabel p.zabel@pengutronix.de

regards Philipp