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

[Philipp Zabel <p.zabel@pengutronix.de>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel