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.st...@pengutronix.de>
Reviewed-by: Philipp Zabel <p.za...@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
