[PATCH] drm/msm/dpu: set geometry for iommu domain

[Jordan Crouse]

On Tue, Nov 27, 2018 at 03:58:13PM -0800, Jeykumar Sankaran wrote:
> Specify geometry for DPU iommu domain which sets
> the address space for gem allocations.
> 
> Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
> Suggested-by: Jordan Crouse <jcrouse at codeaurora.org>
> Suggested-by: Vivek Gautam <vivek.gautam at codeaurora.org>

There is a lot of confusion surrounding all of this, so let me spend a bit of
time to explain.

Like other platform devices with a iommu attached the mdss and gpu devices have
a default DMA IOMMU domain attached to them at enumeration time. The intent is
that if a device uses the DMA API it hides the mechanics of iova management.

However in the case of the GPU (and by extension the display) we *DO* want to
manage our own IOMMU space. This is partially for historical reasons (we
started doing so in the era just before the DMA API became a thing) and also for
future reasons. Eventually we will want to be able to do per-process pagetables
for the GPU and for that we need manual control of the iova domain.

So long story shortx, we don't want to be involved with the DMA domain. In fact,
we would GREATLY prefer if we didn't have it, but thats much wider reaching
discussion that doesn't make sense here (but if you want to get involved in the
fight let me know).

In the place of the dma domain we have the internal concept of address spaces
which manages iova allocation for a specific IOMMU domain. Today this is either
the mdss or the gpu domain (because we use different iommus) but in the future
there will be a new address space for each process for per-process pagetables.

In the upstream code we reuse the domain geometry struct to set up the range for
the address space allocator which is why this patch is needed because the dpu
needs to provide that clue to the address space.

That said, some folks are getting this patch confused with the patch that
Vivek sent out [1] (drm: msm: Replace dma_map_sg with dma_sync_sg). 

A pre-release iteration of this patch did an additional thing - instead
of creating a new iommu domain it re-used the DMA domain and registered it with
the msm iommu engine which meant that we started crossing the streams and
mapping the GEM buffers manually onto the DMA space. This is bad. In fact it
started to conflict with the DMA domain which lead directly to Vivek's patch.

This isn't to say that Vivek's patch isn't appropriate - we knew all along we
were abusing the DMA API for fun and profit, and we might as well abuse
dma_sync_sg instead of dma_map_sg. But it isn't strictly needed if we create a
new domain and not reuse the DMA domain.

Hopefully this clarifies things. If not, hit me up in IRC and I'll try to
explain it again. In the meantime, this patch is correct and useful so:

Acked-by: Jordan Crouse <jcrouse at codeaurora.org>

[1] https://patchwork.freedesktop.org/patch/263914/


> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7d931ae..efceddf1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -823,6 +823,9 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
>  	if (!domain)
>  		return 0;
>  
> +	domain->geometry.aperture_start = 0x1000;
> +	domain->geometry.aperture_end = 0xffffffff;
> +
>  	aspace = msm_gem_address_space_create(dpu_kms->dev->dev,
>  			domain, "dpu1");
>  	if (IS_ERR(aspace)) {

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project