[PATCH v2 2/3] iommu/dma: Use fast DMA domain lookup

Robin Murphy robin.murphy at arm.com
Fri Sep 28 08:31:10 PDT 2018


On 28/09/18 15:21, Marek Szyprowski wrote:
> Hi Robin,
> 
> On 2018-09-28 15:52, Robin Murphy wrote:
>> On 28/09/18 14:26, Marek Szyprowski wrote:
>>> On 2018-09-12 17:24, Robin Murphy wrote:
>>>> Most parts of iommu-dma already assume they are operating on a default
>>>> domain set up by iommu_dma_init_domain(), and can be converted straight
>>>> over to avoid the refcounting bottleneck. MSI page mappings may be in
>>>> an unmanaged domain with an explicit MSI-only cookie, so retain the
>>>> non-specific lookup, but that's OK since they're far from a contended
>>>> fast path either way.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>>
>>> This breaks Exynos DRM driver with Exynos IOMMU on ARM64 (Exynos5433).
>>> Exynos DRM creates it's own domain, attach all devices which performs
>>> DMA access (typically CRTC devices) to it and uses standard DMA-mapping
>>> calls to allocate/map buffers. This way it can use the same one DMA
>>> address for each buffer regardless of the CRTC/display/processing
>>> device.
>>>
>>> This no longer works with this patch. The simplest solution to fix this
>>> is add API to change default_domain to the one allocated by the Exynos
>>> DRM driver.
>>
>> Ugh, I hadn't realised there were any drivers trying to fake up their
>> own default domains - that's always going to be fragile. In fact, one
>> of my proposals for un-breaking Tegra and other DRM drivers involves
>> preventing iommu-dma from trying to operate on non-IOMMU_DOMAIN_DMA
>> domains at all, which would stop that from working permanently.
> 
> IMHO there should be a way to let drivers like DRM to use DMA-mapping
> infrastructure on their own iommu domain. Better provide such API to
> avoid hacking in the DRM drivers to get this done without help from
> IOMMU core.

The tricky part is how to reconcile that with those other drivers which 
want to do explicit IOMMU management with their own domain but still use 
the DMA API for coherency of the underlying memory. I do have a couple 
of half-formed ideas, but they're not quite there yet.

>> Can Exynos not put all the DRM components into a single group, or at
>> least just pick one of the real default domains to attach everyone to
>> instead of allocating a fake one?
> 
> Exynos DRM components are not in the single group. Currently
> iommu-groups have no meaning on Exynos5433 and Exynos IOMMU simply
> allocate one group per each iommu-master device in the system.

Yeah, a group spanning multiple IOMMU devices would have been a bit of a 
hack for your topology, but still *technically* possible ;)

> Exynos DRM already selects one of its component devices as the 'main DMA
> device'. It is being used for managing buffers if no IOMMU is available,
> so it can also use its iommu domain instead of allocating a fake one.
> I've checked and it fixes Exynos DRM now. The question is how to merge
> this fix to keep bisectability.
> 
> From: Marek Szyprowski <m.szyprowski at samsung.com>
> Date: Fri, 28 Sep 2018 16:17:27 +0200
> Subject: [PATCH] drm/exynos: Use selected dma_dev default iommu domain
> instead
>    of a fake one
> 
> Instead of allocating a fake IOMMU domain for all Exynos DRM components,
> simply reuse the default IOMMU domain of the already selected DMA device.
> This allows some design changes in IOMMU framework without breaking IOMMU
> support in Exynos DRM.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> ---
>    drivers/gpu/drm/exynos/exynos_drm_iommu.h | 34 ++++-------------------
>    1 file changed, 6 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_iommu.h
> b/drivers/gpu/drm/exynos/exynos_drm_iommu.h
> index 87f6b5672e11..51d3b7dcd529 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_iommu.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_iommu.h
> @@ -55,37 +55,12 @@ static inline void __exynos_iommu_detach(struct
> exynos_drm_private *priv,
>    static inline int __exynos_iommu_create_mapping(struct
> exynos_drm_private *priv,
>                        unsigned long start, unsigned long size)
>    {
> -    struct iommu_domain *domain;
> -    int ret;
> -
> -    domain = iommu_domain_alloc(priv->dma_dev->bus);
> -    if (!domain)
> -        return -ENOMEM;
> -
> -    ret = iommu_get_dma_cookie(domain);
> -    if (ret)
> -        goto free_domain;
> -
> -    ret = iommu_dma_init_domain(domain, start, size, NULL);
> -    if (ret)
> -        goto put_cookie;
> -
> -    priv->mapping = domain;
> +    priv->mapping = iommu_get_dma_domain(priv->dma_dev);

iommu_get_domain_for_dev(), please - this isn't a performance-critical 
path inside a DMA API implementation, plus without an actual dependency 
on this series maybe there's a chance of sneaking it into 4.19?

(you could always still check domain->type == IOMMU_DOMAIN_DMA if you 
want to be really really paranoid.)

>        return 0;
> -
> -put_cookie:
> -    iommu_put_dma_cookie(domain);
> -free_domain:
> -    iommu_domain_free(domain);
> -    return ret;
>    }
> 
>    static inline void __exynos_iommu_release_mapping(struct
> exynos_drm_private *priv)
>    {
> -    struct iommu_domain *domain = priv->mapping;
> -
> -    iommu_put_dma_cookie(domain);
> -    iommu_domain_free(domain);
>        priv->mapping = NULL;
>    }
> 
> @@ -94,7 +69,9 @@ static inline int __exynos_iommu_attach(struct
> exynos_drm_private *priv,
>    {
>        struct iommu_domain *domain = priv->mapping;
> 
> -    return iommu_attach_device(domain, dev);
> +    if (dev != priv->dma_dev)
> +        return iommu_attach_device(domain, dev);
> +    return 0;
>    }
> 
>    static inline void __exynos_iommu_detach(struct exynos_drm_private *priv,
> @@ -102,7 +79,8 @@ static inline void __exynos_iommu_detach(struct
> exynos_drm_private *priv,
>    {
>        struct iommu_domain *domain = priv->mapping;
> 
> -    iommu_detach_device(domain, dev);
> +    if (dev != priv->dma_dev)
> +        iommu_detach_device(domain, dev);

Strictly you could skip that check, as detaching the master device from 
its real default domain is just a no-op, but I guess maintaining the 
symmetry is probably more intuitive.

Robin.

>    }
>    #else
>    #error Unsupported architecture and IOMMU/DMA-mapping glue code
> 
>   > ...
> 
> Best regards
> 



More information about the linux-arm-kernel mailing list