Convert arch/arm to use iommu-dma

With the IOMMU ops now looking much the same shape as iommu_dma_ops, switch them out in favour of the iommu-dma library, currently enhanced with temporary workarounds that allow it to also sit underneath the arch-specific API. With that in place, we can now start converting the remaining IOMMU drivers and consumers to work with IOMMU API default domains instead.

Signed-off-by Robin Murphy <[email protected]>

...

Now that arch/arm is wired up for default domains and iommu-dma, remove the add_device workaround.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 10 ----------
 1 file changed, 10 deletions(-)

drivers/iommu/arm/arm-smmu/arm-smmu.c +0 -4

index 09c42af9f31e..4e52d8cb67dd 100644
@@ -1164,17 +1164,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return -ENXIO;
 	}
 
-	/*
-	 * FIXME: The arch/arm DMA API code tries to attach devices to its own
-	 * domains between of_xlate() and probe_device() - we have no way to cope
-	 * with that, so until ARM gets converted to rely on groups and default

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for groups and DMA domains to replace the shared mapping workaround.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/mtk_iommu.h    |   2 -
 drivers/iommu/mtk_iommu_v1.c | 153 +++++++++++------------------------
 2 files changed, 48 insertions(+), 107 deletions(-)

drivers/iommu/mtk_iommu.h +0 -2

index 122925dbe547..6253e98d810c 100644
@@ -67,8 +67,6 @@ struct mtk_iommu_data {
 	struct iommu_device		iommu;
 	const struct mtk_iommu_plat_data *plat_data;
 
-	struct dma_iommu_mapping	*mapping; /* For mtk_iommu_v1.c */
-
 	struct list_head		list;
 	struct mtk_smi_larb_iommu	larb_imu[MTK_LARB_NR_MAX];
 };

drivers/iommu/mtk_iommu_v1.c +48 -105

index 82ddfe9170d4..40c89b8d3ac4 100644
@@ -28,7 +28,6 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <asm/barrier.h>
-#include <asm/dma-iommu.h>
 #include <linux/init.h>
 #include <dt-bindings/memory/mt2701-larb-port.h>
 #include <soc/mediatek/smi.h>

@@ -240,13 +239,18 @@ static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
 {
 	struct mtk_iommu_domain *dom;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
 	if (!dom)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(&dom->domain)) {
+		kfree(dom);
+		return NULL;
+	}
+
 	return &dom->domain;
 }
 

@@ -257,6 +261,7 @@ static void mtk_iommu_domain_free(struct iommu_domain *domain)
 
 	dma_free_coherent(data->dev, M2701_IOMMU_PGT_SIZE,
 			dom->pgt_va, dom->pgt_pa);
+	iommu_put_dma_cookie(domain);
 	kfree(to_mtk_domain(domain));
 }
 

@@ -265,14 +270,8 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
 {
 	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
 	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
-	struct dma_iommu_mapping *mtk_mapping;
 	int ret;
 
-	/* Only allow the domain created internally. */
-	mtk_mapping = data->mapping;
-	if (mtk_mapping->domain != domain)
-		return 0;
-
 	if (!data->m4u_dom) {
 		data->m4u_dom = dom;
 		ret = mtk_iommu_domain_finalise(data);

@@ -358,18 +357,39 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct iommu_domain *domain,
 
 static const struct iommu_ops mtk_iommu_ops;
 
-/*
- * MTK generation one iommu HW only support one iommu domain, and all the client
- * sharing the same iova address space.
- */
-static int mtk_iommu_create_mapping(struct device *dev,
-				    struct of_phandle_args *args)
+static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct mtk_iommu_data *data;
+
+	if (!fwspec || fwspec->ops != &mtk_iommu_ops)
+		return ERR_PTR(-ENODEV); /* Not a iommu client device */
+
+	data = dev_iommu_priv_get(dev);
+
+	return &data->iommu;
+}
+
+static void mtk_iommu_release_device(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+	if (!fwspec || fwspec->ops != &mtk_iommu_ops)
+		return;
+
+	iommu_fwspec_free(dev);
+}
+
+static struct iommu_group *mtk_iommu_device_group(struct device *dev)
+{
+	struct mtk_iommu_data *data = dev_iommu_priv_get(dev);
+
+	return iommu_group_ref_get(data->m4u_group);
+}
+
+static int mtk_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
 	struct platform_device *m4updev;
-	struct dma_iommu_mapping *mtk_mapping;
-	int ret;
 
 	if (args->args_count != 1) {
 		dev_err(dev, "invalid #iommu-cells(%d) property for IOMMU\n",

@@ -377,15 +397,6 @@ static int mtk_iommu_create_mapping(struct device *dev,
 		return -EINVAL;
 	}
 
-	if (!fwspec) {
-		ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_ops);
-		if (ret)
-			return ret;
-		fwspec = dev_iommu_fwspec_get(dev);
-	} else if (dev_iommu_fwspec_get(dev)->ops != &mtk_iommu_ops) {
-		return -EINVAL;
-	}
-
 	if (!dev_iommu_priv_get(dev)) {
 		/* Get the m4u device */
 		m4updev = of_find_device_by_node(args->np);

@@ -395,83 +406,7 @@ static int mtk_iommu_create_mapping(struct device *dev,
 		dev_iommu_priv_set(dev, platform_get_drvdata(m4updev));
 	}
 
-	ret = iommu_fwspec_add_ids(dev, args->args, 1);
-	if (ret)
-		return ret;
-
-	data = dev_iommu_priv_get(dev);
-	mtk_mapping = data->mapping;
-	if (!mtk_mapping) {
-		/* MTK iommu support 4GB iova address space. */
-		mtk_mapping = arm_iommu_create_mapping(&platform_bus_type,
-						0, 1ULL << 32);
-		if (IS_ERR(mtk_mapping))
-			return PTR_ERR(mtk_mapping);
-
-		data->mapping = mtk_mapping;
-	}
-
-	return 0;
-}
-
-static int mtk_iommu_def_domain_type(struct device *dev)
-{
-	return IOMMU_DOMAIN_UNMANAGED;
-}
-
-static struct iommu_device *mtk_iommu_probe_device(struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-	struct of_phandle_args iommu_spec;
-	struct of_phandle_iterator it;
-	struct mtk_iommu_data *data;
-	int err;
-
-	of_for_each_phandle(&it, err, dev->of_node, "iommus",
-			"#iommu-cells", -1) {
-		int count = of_phandle_iterator_args(&it, iommu_spec.args,
-					MAX_PHANDLE_ARGS);
-		iommu_spec.np = of_node_get(it.node);
-		iommu_spec.args_count = count;
-
-		mtk_iommu_create_mapping(dev, &iommu_spec);
-
-		/* dev->iommu_fwspec might have changed */
-		fwspec = dev_iommu_fwspec_get(dev);
-
-		of_node_put(iommu_spec.np);
-	}
-
-	if (!fwspec || fwspec->ops != &mtk_iommu_ops)
-		return ERR_PTR(-ENODEV); /* Not a iommu client device */
-
-	data = dev_iommu_priv_get(dev);
-
-	return &data->iommu;
-}
-
-static void mtk_iommu_probe_finalize(struct device *dev)
-{
-	struct dma_iommu_mapping *mtk_mapping;
-	struct mtk_iommu_data *data;
-	int err;
-
-	data        = dev_iommu_priv_get(dev);
-	mtk_mapping = data->mapping;
-
-	err = arm_iommu_attach_device(dev, mtk_mapping);
-	if (err)
-		dev_err(dev, "Can't create IOMMU mapping - DMA-OPS will not work\n");
-}
-
-static void mtk_iommu_release_device(struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-	if (!fwspec || fwspec->ops != &mtk_iommu_ops)
-		return;
-
-	iommu_fwspec_free(dev);
+	return iommu_fwspec_add_ids(dev, args->args, 1);
 }
 
 static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)

@@ -524,10 +459,9 @@ static const struct iommu_ops mtk_iommu_ops = {
 	.unmap		= mtk_iommu_unmap,
 	.iova_to_phys	= mtk_iommu_iova_to_phys,
 	.probe_device	= mtk_iommu_probe_device,
-	.probe_finalize = mtk_iommu_probe_finalize,
 	.release_device	= mtk_iommu_release_device,
-	.def_domain_type = mtk_iommu_def_domain_type,
-	.device_group	= generic_device_group,
+	.device_group	= mtk_iommu_device_group,
+	.of_xlate	= mtk_iommu_of_xlate,
 	.pgsize_bitmap	= ~0UL << MT2701_IOMMU_PAGE_SHIFT,
 };
 

@@ -626,6 +560,14 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	/*
+	 * MTK generation one iommu HW only support one iommu domain,
+	 * and all the client sharing the same iova address space.
+	 */
+	data->m4u_group = iommu_group_alloc();
+	if (IS_ERR(data->m4u_group))
+		return PTR_ERR(data->m4u_group);
+
 	if (!iommu_present(&platform_bus_type))
 		bus_set_iommu(&platform_bus_type,  &mtk_iommu_ops);
 

@@ -636,6 +578,7 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 {
 	struct mtk_iommu_data *data = platform_get_drvdata(pdev);
 
+	iommu_group_put(data->m4u_group);
 	iommu_device_sysfs_remove(&data->iommu);
 	iommu_device_unregister(&data->iommu);
 

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/msm_iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +6 -2

index 3615cd6241c4..f34efcbb0b2b 100644
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
+#include <linux/dma-iommu.h>
 #include <linux/errno.h>
 #include <linux/io.h>
 #include <linux/io-pgtable.h>

@@ -314,13 +315,16 @@ static struct iommu_domain *msm_iommu_domain_alloc(unsigned type)
 {
        struct msm_priv *priv;
 
-       if (type != IOMMU_DOMAIN_UNMANAGED)
+       if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
                return NULL;
 
        priv = kzalloc(sizeof(*priv), GFP_KERNEL);
        if (!priv)
                goto fail_nomem;
 
+       if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(&priv->domain))
+               goto fail_nomem;
+
        INIT_LIST_HEAD(&priv->list_attached);
 
        priv->domain.geometry.aperture_start = 0;

@@ -339,6 +343,7 @@ static void msm_iommu_domain_free(struct iommu_domain *domain)
        struct msm_priv *priv;
        unsigned long flags;
 
+       iommu_put_dma_cookie(domain);
        spin_lock_irqsave(&msm_iommu_lock, flags);
        priv = to_msm_priv(domain);
        kfree(priv);
--

2.28.0.dirty

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Side note, I suspect we'll end up needing something like 0e764a01015dfebff8a8ffd297d74663772e248a .. if someone can dig a 32b device out of the closet and dust it off, the fix is easy enough. Just wanted to mention that here so anyone with a 32b device could find what is needed.

BR,
-R

Quoted 54 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/msm_iommu.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +6 -2

index 3615cd6241c4..f34efcbb0b2b 100644
@@ -8,6 +8,7 @@
  #include <linux/kernel.h>
  #include <linux/init.h>
  #include <linux/platform_device.h>
+#include <linux/dma-iommu.h>
  #include <linux/errno.h>
  #include <linux/io.h>
  #include <linux/io-pgtable.h>

@@ -314,13 +315,16 @@ static struct iommu_domain *msm_iommu_domain_alloc(unsigned type)
  {
         struct msm_priv *priv;
 
-       if (type != IOMMU_DOMAIN_UNMANAGED)
+       if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
                 return NULL;
 
         priv = kzalloc(sizeof(*priv), GFP_KERNEL);
         if (!priv)
                 goto fail_nomem;
 
+       if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(&priv->domain))
+               goto fail_nomem;
+
         INIT_LIST_HEAD(&priv->list_attached);
 
         priv->domain.geometry.aperture_start = 0;

@@ -339,6 +343,7 @@ static void msm_iommu_domain_free(struct iommu_domain *domain)
         struct msm_priv *priv;
         unsigned long flags;
 
+       iommu_put_dma_cookie(domain);
         spin_lock_irqsave(&msm_iommu_lock, flags);
         priv = to_msm_priv(domain);
         kfree(priv);
--

2.28.0.dirty

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 5 lines · Rob Clark

Side note, I suspect we'll end up needing something like 0e764a01015dfebff8a8ffd297d74663772e248a .. if someone can dig a 32b device out of the closet and dust it off, the fix is easy enough. Just wanted to mention that here so anyone with a 32b device could find what is needed.

FWIW there shouldn't be any material change here - the generic default domain is installed under the same circumstances as the Arm dma_iommu_mapping was, so if any platform does have an issue, then it should already have started 4 years with f78ebca8ff3d ("iommu/msm: Add support for generic master bindings").

Robin.

Quoted 55 lines

BR,
-R

Quoted 50 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/msm_iommu.c | 7 ++++++-
  1 file changed, 6 insertions(+), 1 deletion(-)

drivers/iommu/msm_iommu.c +6 -2

index 3615cd6241c4..f34efcbb0b2b 100644
@@ -8,6 +8,7 @@
  #include <linux/kernel.h>
  #include <linux/init.h>
  #include <linux/platform_device.h>
+#include <linux/dma-iommu.h>
  #include <linux/errno.h>
  #include <linux/io.h>
  #include <linux/io-pgtable.h>

@@ -314,13 +315,16 @@ static struct iommu_domain *msm_iommu_domain_alloc(unsigned type)
  {
         struct msm_priv *priv;
 
-       if (type != IOMMU_DOMAIN_UNMANAGED)
+       if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
                 return NULL;
 
         priv = kzalloc(sizeof(*priv), GFP_KERNEL);
         if (!priv)
                 goto fail_nomem;
 
+       if (type == IOMMU_DOMAIN_DMA && iommu_get_dma_cookie(&priv->domain))
+               goto fail_nomem;
+
         INIT_LIST_HEAD(&priv->list_attached);
 
         priv->domain.geometry.aperture_start = 0;

@@ -339,6 +343,7 @@ static void msm_iommu_domain_free(struct iommu_domain *domain)
         struct msm_priv *priv;
         unsigned long flags;
 
+       iommu_put_dma_cookie(domain);
         spin_lock_irqsave(&msm_iommu_lock, flags);
         priv = to_msm_priv(domain);
         kfree(priv);
--

2.28.0.dirty

Quoted 4 lines

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/omap-iommu.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

drivers/iommu/omap-iommu.c +10 -1

index 71f29c0927fc..ea25c2fe0418 100644
@@ -9,6 +9,7 @@
  *		Paul Mundt and Toshihiro Kobayashi
  */
 
+#include <linux/dma-iommu.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/slab.h>

@@ -1574,13 +1575,19 @@ static struct iommu_domain *omap_iommu_domain_alloc(unsigned type)
 {
 	struct omap_iommu_domain *omap_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
 		return NULL;
 
 	omap_domain = kzalloc(sizeof(*omap_domain), GFP_KERNEL);
 	if (!omap_domain)
 		return NULL;
 
+	if (type == IOMMU_DOMAIN_DMA &&
+	    iommu_get_dma_cookie(&omap_domain->domain)) {
+		kfree(omap_domain);
+		return NULL;
+	}
+
 	spin_lock_init(&omap_domain->lock);
 
 	omap_domain->domain.geometry.aperture_start = 0;

@@ -1601,6 +1608,7 @@ static void omap_iommu_domain_free(struct iommu_domain *domain)
 	if (omap_domain->dev)
 		_omap_iommu_detach_dev(omap_domain, omap_domain->dev);
 
+	iommu_put_dma_cookie(&omap_domain->domain);
 	kfree(omap_domain);
 }
 

@@ -1736,6 +1744,17 @@ static struct iommu_group *omap_iommu_device_group(struct device *dev)
 	return group;
 }
 
+static int omap_iommu_of_xlate(struct device *dev,

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/tegra-gart.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

drivers/iommu/tegra-gart.c +1 -0

index fac720273889..e081387080f6 100644
@@ -9,6 +9,7 @@
 
 #define dev_fmt(fmt)	"gart: " fmt
 
+#include <linux/dma-iommu.h>
 #include <linux/io.h>
 #include <linux/iommu.h>
 #include <linux/moduleparam.h>

20.08.2020 18:08, Robin Murphy пишет:

Quoted 21 lines

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/tegra-gart.c | 17 ++++++++++++-----
  1 file changed, 12 insertions(+), 5 deletions(-)

drivers/iommu/tegra-gart.c +1 -0

index fac720273889..e081387080f6 100644
@@ -9,6 +9,7 @@
 
  #define dev_fmt(fmt)	"gart: " fmt
 
+#include <linux/dma-iommu.h>
  #include <linux/io.h>
  #include <linux/iommu.h>
  #include <linux/moduleparam.h>

drivers/iommu/tegra-gart.c static struct iommu_domain *gart_iommu_domain_alloc(unsigned type) · line 147

Quoted 2 lines

drivers/iommu/tegra-gart.c +0 -0

@@ -145,16 +146,22 @@ static struct iommu_domain *gart_iommu_domain_alloc(unsigned type)
  {
  	struct iommu_domain *domain;

Hello, Robin!

Tegra20 GART isn't a real IOMMU, but a small relocation aperture. We would only want to use it for a temporal mappings (managed by GPU driver) for the time while GPU hardware is busy and working with a sparse DMA buffers, the driver will take care of unmapping the sparse buffers once GPU work is finished [1]. In a case of contiguous DMA buffers, we want to bypass the IOMMU and use buffer's phys address because GART aperture is small and all buffers simply can't fit into GART for a complex GPU operations that involve multiple buffers [2][3]. The upstream GPU driver still doesn't support GART, but eventually it needs to be changed.

[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L489

[2]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/gart.c#L542

[3]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/tegra/uapi/patching.c#L90

drivers/iommu/tegra-gart.c static struct iommu_domain *gart_iommu_domain_alloc(unsigned type) · line 150

Quoted 3 lines

drivers/iommu/tegra-gart.c +1 -1

-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
  		return NULL;

Will a returned NULL tell to IOMMU core that implicit domain shouldn't be used? Is it possible to leave this driver as-is?

Quoted 2 lines

Will a returned NULL tell to IOMMU core that implicit domain shouldn't be used? Is it possible to leave this driver as-is?

The aim of this patch was just to make the conversion without functional changes wherever possible, i.e. maintain an equivalent to the existing ARM behaviour of allocating its own implicit domains for everything. It doesn't represent any judgement of whether that was ever appropriate for this driver in the first place ;)

Hopefully my other reply already covered the degree of control drivers can have with proper default domains, but do shout if anything wasn't clear.

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

Quoted 7 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, implement the corresponding driver-side support for DMA domains.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/tegra-smmu.c | 37 +++++++++++++++++++++----------------
  1 file changed, 21 insertions(+), 16 deletions(-)

We can't do that yet because it will currently still break for use-cases such as display where we don't properly set up identity mappings during boot. The result is that the dma-iommu code will enable translations before the driver gets a chance to set up any mappings and if the display controller was left on by the bootloader, scanning out a splash screen, this causes faults between the point where dma-iommu is being set up for the display controller and where the display controller starts mapping its own buffers (rather than the ones mapped by the bootloader).

That said, I do have a series that I've been carrying around for longer than I've wanted that does exactly this for Tegra SMMU and I'd prefer if you could drop this particular change from your series so that I can keep working on resolving the identity mapping issues first.

Now that arch/arm is wired up for default domains and iommu-dma, we can consolidate the shared mapping code onto the generic IOMMU API version, and retire the arch-specific implementation.

Signed-off-by Robin Murphy <[email protected]>

---
This is a cheeky revert of 07dc3678bacc ("drm/exynos: Fix cleanup of IOMMU related objects"), plus removal of the remaining arm_iommu_* references on top.

---
  drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  5 +-
  drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_dma.c       | 61 +++----------------
  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  6 +-
  drivers/gpu/drm/exynos/exynos_drm_fimc.c      |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_gsc.c       |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_rotator.c   |  5 +-
  drivers/gpu/drm/exynos/exynos_drm_scaler.c    |  6 +-
  drivers/gpu/drm/exynos/exynos_mixer.c         |  7 +--
  11 files changed, 29 insertions(+), 86 deletions(-)

drivers/gpu/drm/exynos/exynos5433_drm_decon.c +2 -3

index 1f79bc2a881e..8428ae12dfa5 100644
@@ -55,7 +55,6 @@ static const char * const decon_clks_name[] = {
  struct decon_context {
  	struct device			*dev;
  	struct drm_device		*drm_dev;
-	void				*dma_priv;
  	struct exynos_drm_crtc		*crtc;
  	struct exynos_drm_plane		planes[WINDOWS_NR];
  	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -645,7 +644,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 
  	decon_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, dev);
  }
 
  static void decon_unbind(struct device *dev, struct device *master, void *data)

@@ -655,7 +654,7 @@ static void decon_unbind(struct device *dev, struct device *master, void *data)
  	decon_atomic_disable(ctx->crtc);
 
  	/* detach this sub driver from iommu mapping if supported. */
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
  }
 
  static const struct component_ops decon_component_ops = {

drivers/gpu/drm/exynos/exynos7_drm_decon.c +2 -3

index f2d87a7445c7..e7b58097ccdc 100644
@@ -40,7 +40,6 @@
  struct decon_context {
  	struct device			*dev;
  	struct drm_device		*drm_dev;
-	void				*dma_priv;
  	struct exynos_drm_crtc		*crtc;
  	struct exynos_drm_plane		planes[WINDOWS_NR];
  	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -128,13 +127,13 @@ static int decon_ctx_initialize(struct decon_context *ctx,
 
  	decon_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, ctx->dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, ctx->dev);
  }
 
  static void decon_ctx_remove(struct decon_context *ctx)
  {
  	/* detach this sub driver from iommu mapping if supported. */
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
  }
 
  static u32 decon_calc_clkdiv(struct decon_context *ctx,

drivers/gpu/drm/exynos/exynos_drm_dma.c +9 -52

index 58b89ec11b0e..fd5f9bcf1857 100644
@@ -14,19 +14,6 @@
 
  #include "exynos_drm_drv.h"
 
-#if defined(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#else
-#define arm_iommu_create_mapping(...)	({ NULL; })
-#define arm_iommu_attach_device(...)	({ -ENODEV; })
-#define arm_iommu_release_mapping(...)	({ })
-#define arm_iommu_detach_device(...)	({ })
-#define to_dma_iommu_mapping(dev) NULL
-#endif
-
-#if !defined(CONFIG_IOMMU_DMA)
-#define iommu_dma_init_domain(...) ({ -EINVAL; })
-#endif
 
  #define EXYNOS_DEV_ADDR_START	0x20000000
  #define EXYNOS_DEV_ADDR_SIZE	0x40000000

@@ -58,7 +45,7 @@ static inline void clear_dma_max_seg_size(struct device *dev)
   * mapping.
   */
  static int drm_iommu_attach_device(struct drm_device *drm_dev,
-				struct device *subdrv_dev, void **dma_priv)
+				struct device *subdrv_dev)
  {
  	struct exynos_drm_private *priv = drm_dev->dev_private;
  	int ret = 0;

@@ -73,22 +60,7 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
  	if (ret)
  		return ret;
 
-	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
-		/*
-		 * Keep the original DMA mapping of the sub-device and
-		 * restore it on Exynos DRM detach, otherwise the DMA
-		 * framework considers it as IOMMU-less during the next
-		 * probe (in case of deferred probe or modular build)
-		 */
-		*dma_priv = to_dma_iommu_mapping(subdrv_dev);
-		if (*dma_priv)
-			arm_iommu_detach_device(subdrv_dev);
-
-		ret = arm_iommu_attach_device(subdrv_dev, priv->mapping);
-	} else if (IS_ENABLED(CONFIG_IOMMU_DMA)) {
-		ret = iommu_attach_device(priv->mapping, subdrv_dev);
-	}
-
+	ret = iommu_attach_device(priv->mapping, subdrv_dev);
  	if (ret)
  		clear_dma_max_seg_size(subdrv_dev);
 

@@ -105,21 +77,15 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
   * mapping
   */
  static void drm_iommu_detach_device(struct drm_device *drm_dev,
-				    struct device *subdrv_dev, void **dma_priv)
+				struct device *subdrv_dev)
  {
  	struct exynos_drm_private *priv = drm_dev->dev_private;
 
-	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
-		arm_iommu_detach_device(subdrv_dev);
-		arm_iommu_attach_device(subdrv_dev, *dma_priv);
-	} else if (IS_ENABLED(CONFIG_IOMMU_DMA))
-		iommu_detach_device(priv->mapping, subdrv_dev);
-
+	iommu_detach_device(priv->mapping, subdrv_dev);
  	clear_dma_max_seg_size(subdrv_dev);
  }
 
-int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
-			    void **dma_priv)
+int exynos_drm_register_dma(struct drm_device *drm, struct device *dev)
  {
  	struct exynos_drm_private *priv = drm->dev_private;
 

@@ -133,27 +99,20 @@ int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
  		return 0;
 
  	if (!priv->mapping) {
-		void *mapping;
-
-		if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))
-			mapping = arm_iommu_create_mapping(&platform_bus_type,
-				EXYNOS_DEV_ADDR_START, EXYNOS_DEV_ADDR_SIZE);
-		else if (IS_ENABLED(CONFIG_IOMMU_DMA))
-			mapping = iommu_get_domain_for_dev(priv->dma_dev);
+		void *mapping = iommu_get_domain_for_dev(priv->dma_dev);
 
  		if (IS_ERR(mapping))
  			return PTR_ERR(mapping);
  		priv->mapping = mapping;
  	}
 
-	return drm_iommu_attach_device(drm, dev, dma_priv);
+	return drm_iommu_attach_device(drm, dev);
  }
 
-void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev,
-			       void **dma_priv)
+void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev)
  {
  	if (IS_ENABLED(CONFIG_EXYNOS_IOMMU))
-		drm_iommu_detach_device(drm, dev, dma_priv);
+		drm_iommu_detach_device(drm, dev);
  }
 
  void exynos_drm_cleanup_dma(struct drm_device *drm)

@@ -163,7 +122,5 @@ void exynos_drm_cleanup_dma(struct drm_device *drm)
  	if (!IS_ENABLED(CONFIG_EXYNOS_IOMMU))
  		return;
 
-	arm_iommu_release_mapping(priv->mapping);
-	priv->mapping = NULL;
  	priv->dma_dev = NULL;
  }

drivers/gpu/drm/exynos/exynos_drm_drv.h +2 -4

index 6ae9056e7a18..d4d21d8cfb90 100644
@@ -223,10 +223,8 @@ static inline bool is_drm_iommu_supported(struct drm_device *drm_dev)
  	return priv->mapping ? true : false;
  }
 
-int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
-			    void **dma_priv);
-void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev,
-			       void **dma_priv);
+int exynos_drm_register_dma(struct drm_device *drm, struct device *dev);
+void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev);
  void exynos_drm_cleanup_dma(struct drm_device *drm);
 
  #ifdef CONFIG_DRM_EXYNOS_DPI

drivers/gpu/drm/exynos/exynos_drm_fimc.c +2 -3

index 29ab8be8604c..8ea2e1d77802 100644
@@ -97,7 +97,6 @@ struct fimc_scaler {
  struct fimc_context {
  	struct exynos_drm_ipp ipp;
  	struct drm_device *drm_dev;
-	void		*dma_priv;
  	struct device	*dev;
  	struct exynos_drm_ipp_task	*task;
  	struct exynos_drm_ipp_formats	*formats;

@@ -1134,7 +1133,7 @@ static int fimc_bind(struct device *dev, struct device *master, void *data)
 
  	ctx->drm_dev = drm_dev;
  	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
  	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
  			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -1154,7 +1153,7 @@ static void fimc_unbind(struct device *dev, struct device *master,
  	struct exynos_drm_ipp *ipp = &ctx->ipp;
 
  	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(drm_dev, dev);
  }
 
  static const struct component_ops fimc_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_fimd.c +2 -3

index bb67cad8371f..21aec38702fc 100644
@@ -167,7 +167,6 @@ static struct fimd_driver_data exynos5420_fimd_driver_data = {
  struct fimd_context {
  	struct device			*dev;
  	struct drm_device		*drm_dev;
-	void				*dma_priv;
  	struct exynos_drm_crtc		*crtc;
  	struct exynos_drm_plane		planes[WINDOWS_NR];
  	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -1091,7 +1090,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
  	if (is_drm_iommu_supported(drm_dev))
  		fimd_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, dev);
  }
 
  static void fimd_unbind(struct device *dev, struct device *master,

@@ -1101,7 +1100,7 @@ static void fimd_unbind(struct device *dev, struct device *master,
 
  	fimd_atomic_disable(ctx->crtc);
 
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
 
  	if (ctx->encoder)
  		exynos_dpi_remove(ctx->encoder);

drivers/gpu/drm/exynos/exynos_drm_g2d.c +2 -3

index 03be31427181..256ceafd9945 100644
@@ -232,7 +232,6 @@ struct g2d_runqueue_node {
 
  struct g2d_data {
  	struct device			*dev;
-	void				*dma_priv;
  	struct clk			*gate_clk;
  	void __iomem			*regs;
  	int				irq;

@@ -1410,7 +1409,7 @@ static int g2d_bind(struct device *dev, struct device *master, void *data)
  		return ret;
  	}
 
-	ret = exynos_drm_register_dma(drm_dev, dev, &g2d->dma_priv);
+	ret = exynos_drm_register_dma(drm_dev, dev);
  	if (ret < 0) {
  		dev_err(dev, "failed to enable iommu.\n");
  		g2d_fini_cmdlist(g2d);

@@ -1435,7 +1434,7 @@ static void g2d_unbind(struct device *dev, struct device *master, void *data)
  	priv->g2d_dev = NULL;
 
  	cancel_work_sync(&g2d->runqueue_work);
-	exynos_drm_unregister_dma(g2d->drm_dev, dev, &g2d->dma_priv);
+	exynos_drm_unregister_dma(g2d->drm_dev, dev);
  }
 
  static const struct component_ops g2d_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_gsc.c +2 -3

index 45e9aee8366a..88b6fcaa20be 100644
@@ -97,7 +97,6 @@ struct gsc_scaler {
  struct gsc_context {
  	struct exynos_drm_ipp ipp;
  	struct drm_device *drm_dev;
-	void		*dma_priv;
  	struct device	*dev;
  	struct exynos_drm_ipp_task	*task;
  	struct exynos_drm_ipp_formats	*formats;

@@ -1170,7 +1169,7 @@ static int gsc_bind(struct device *dev, struct device *master, void *data)
 
  	ctx->drm_dev = drm_dev;
  	ctx->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
  	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
  			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -1190,7 +1189,7 @@ static void gsc_unbind(struct device *dev, struct device *master,
  	struct exynos_drm_ipp *ipp = &ctx->ipp;
 
  	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(drm_dev, dev);
  }
 
  static const struct component_ops gsc_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_rotator.c +2 -3

index 2d94afba031e..f22fa0d2621a 100644
@@ -56,7 +56,6 @@ struct rot_variant {
  struct rot_context {
  	struct exynos_drm_ipp ipp;
  	struct drm_device *drm_dev;
-	void		*dma_priv;
  	struct device	*dev;
  	void __iomem	*regs;
  	struct clk	*clock;

@@ -244,7 +243,7 @@ static int rotator_bind(struct device *dev, struct device *master, void *data)
 
  	rot->drm_dev = drm_dev;
  	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &rot->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
  	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
  			   DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE,

@@ -262,7 +261,7 @@ static void rotator_unbind(struct device *dev, struct device *master,
  	struct exynos_drm_ipp *ipp = &rot->ipp;
 
  	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(rot->drm_dev, rot->dev, &rot->dma_priv);
+	exynos_drm_unregister_dma(rot->drm_dev, rot->dev);
  }
 
  static const struct component_ops rotator_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_scaler.c +2 -4

index ce1857138f89..0c560566bd2e 100644
@@ -39,7 +39,6 @@ struct scaler_data {
  struct scaler_context {
  	struct exynos_drm_ipp		ipp;
  	struct drm_device		*drm_dev;
-	void				*dma_priv;
  	struct device			*dev;
  	void __iomem			*regs;
  	struct clk			*clock[SCALER_MAX_CLK];

@@ -451,7 +450,7 @@ static int scaler_bind(struct device *dev, struct device *master, void *data)
 
  	scaler->drm_dev = drm_dev;
  	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &scaler->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
  	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
  			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -471,8 +470,7 @@ static void scaler_unbind(struct device *dev, struct device *master,
  	struct exynos_drm_ipp *ipp = &scaler->ipp;
 
  	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(scaler->drm_dev, scaler->dev,
-				  &scaler->dma_priv);
+	exynos_drm_unregister_dma(scaler->drm_dev, scaler->dev);
  }
 
  static const struct component_ops scaler_component_ops = {

drivers/gpu/drm/exynos/exynos_mixer.c +1 -3

index af192e5a16ef..18972e605c5d 100644
@@ -94,7 +94,6 @@ struct mixer_context {
  	struct platform_device *pdev;
  	struct device		*dev;
  	struct drm_device	*drm_dev;
-	void			*dma_priv;
  	struct exynos_drm_crtc	*crtc;
  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
  	unsigned long		flags;

@@ -895,14 +894,12 @@ static int mixer_initialize(struct mixer_context *mixer_ctx,
  		}
  	}
 
-	return exynos_drm_register_dma(drm_dev, mixer_ctx->dev,
-				       &mixer_ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, mixer_ctx->dev);

Now that arch/arm is wired up for default domains and iommu-dma, we can consolidate the shared mapping code onto the generic IOMMU API version, and retire the arch-specific implementation.

Signed-off-by Robin Murphy <[email protected]>

---
This is a cheeky revert of 07dc3678bacc ("drm/exynos: Fix cleanup of IOMMU related objects"), plus removal of the remaining arm_iommu_* references on top.

---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  5 +-
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_dma.c       | 61 +++----------------
 drivers/gpu/drm/exynos/exynos_drm_drv.h       |  6 +-
 drivers/gpu/drm/exynos/exynos_drm_fimc.c      |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_g2d.c       |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_gsc.c       |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_rotator.c   |  5 +-
 drivers/gpu/drm/exynos/exynos_drm_scaler.c    |  6 +-
 drivers/gpu/drm/exynos/exynos_mixer.c         |  7 +--
 11 files changed, 29 insertions(+), 86 deletions(-)

drivers/gpu/drm/exynos/exynos5433_drm_decon.c +2 -3

index 1f79bc2a881e..8428ae12dfa5 100644
@@ -55,7 +55,6 @@ static const char * const decon_clks_name[] = {
 struct decon_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
-	void				*dma_priv;
 	struct exynos_drm_crtc		*crtc;
 	struct exynos_drm_plane		planes[WINDOWS_NR];
 	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -645,7 +644,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 
 	decon_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, dev);
 }
 
 static void decon_unbind(struct device *dev, struct device *master, void *data)

@@ -655,7 +654,7 @@ static void decon_unbind(struct device *dev, struct device *master, void *data)
 	decon_atomic_disable(ctx->crtc);
 
 	/* detach this sub driver from iommu mapping if supported. */
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
 }
 
 static const struct component_ops decon_component_ops = {

drivers/gpu/drm/exynos/exynos7_drm_decon.c +2 -3

index f2d87a7445c7..e7b58097ccdc 100644
@@ -40,7 +40,6 @@
 struct decon_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
-	void				*dma_priv;
 	struct exynos_drm_crtc		*crtc;
 	struct exynos_drm_plane		planes[WINDOWS_NR];
 	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -128,13 +127,13 @@ static int decon_ctx_initialize(struct decon_context *ctx,
 
 	decon_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, ctx->dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, ctx->dev);
 }
 
 static void decon_ctx_remove(struct decon_context *ctx)
 {
 	/* detach this sub driver from iommu mapping if supported. */
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
 }
 
 static u32 decon_calc_clkdiv(struct decon_context *ctx,

drivers/gpu/drm/exynos/exynos_drm_dma.c +9 -52

index 58b89ec11b0e..fd5f9bcf1857 100644
@@ -14,19 +14,6 @@
 
 #include "exynos_drm_drv.h"
 
-#if defined(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#else
-#define arm_iommu_create_mapping(...)	({ NULL; })
-#define arm_iommu_attach_device(...)	({ -ENODEV; })
-#define arm_iommu_release_mapping(...)	({ })
-#define arm_iommu_detach_device(...)	({ })
-#define to_dma_iommu_mapping(dev) NULL
-#endif
-
-#if !defined(CONFIG_IOMMU_DMA)
-#define iommu_dma_init_domain(...) ({ -EINVAL; })
-#endif
 
 #define EXYNOS_DEV_ADDR_START	0x20000000
 #define EXYNOS_DEV_ADDR_SIZE	0x40000000

@@ -58,7 +45,7 @@ static inline void clear_dma_max_seg_size(struct device *dev)
  * mapping.
  */
 static int drm_iommu_attach_device(struct drm_device *drm_dev,
-				struct device *subdrv_dev, void **dma_priv)
+				struct device *subdrv_dev)
 {
 	struct exynos_drm_private *priv = drm_dev->dev_private;
 	int ret = 0;

@@ -73,22 +60,7 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
 	if (ret)
 		return ret;
 
-	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
-		/*
-		 * Keep the original DMA mapping of the sub-device and
-		 * restore it on Exynos DRM detach, otherwise the DMA
-		 * framework considers it as IOMMU-less during the next
-		 * probe (in case of deferred probe or modular build)
-		 */
-		*dma_priv = to_dma_iommu_mapping(subdrv_dev);
-		if (*dma_priv)
-			arm_iommu_detach_device(subdrv_dev);
-
-		ret = arm_iommu_attach_device(subdrv_dev, priv->mapping);
-	} else if (IS_ENABLED(CONFIG_IOMMU_DMA)) {
-		ret = iommu_attach_device(priv->mapping, subdrv_dev);
-	}
-
+	ret = iommu_attach_device(priv->mapping, subdrv_dev);
 	if (ret)
 		clear_dma_max_seg_size(subdrv_dev);
 

@@ -105,21 +77,15 @@ static int drm_iommu_attach_device(struct drm_device *drm_dev,
  * mapping
  */
 static void drm_iommu_detach_device(struct drm_device *drm_dev,
-				    struct device *subdrv_dev, void **dma_priv)
+				struct device *subdrv_dev)
 {
 	struct exynos_drm_private *priv = drm_dev->dev_private;
 
-	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
-		arm_iommu_detach_device(subdrv_dev);
-		arm_iommu_attach_device(subdrv_dev, *dma_priv);
-	} else if (IS_ENABLED(CONFIG_IOMMU_DMA))
-		iommu_detach_device(priv->mapping, subdrv_dev);
-
+	iommu_detach_device(priv->mapping, subdrv_dev);
 	clear_dma_max_seg_size(subdrv_dev);
 }
 
-int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
-			    void **dma_priv)
+int exynos_drm_register_dma(struct drm_device *drm, struct device *dev)
 {
 	struct exynos_drm_private *priv = drm->dev_private;
 

@@ -133,27 +99,20 @@ int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
 		return 0;
 
 	if (!priv->mapping) {
-		void *mapping;
-
-		if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU))
-			mapping = arm_iommu_create_mapping(&platform_bus_type,
-				EXYNOS_DEV_ADDR_START, EXYNOS_DEV_ADDR_SIZE);
-		else if (IS_ENABLED(CONFIG_IOMMU_DMA))
-			mapping = iommu_get_domain_for_dev(priv->dma_dev);
+		void *mapping = iommu_get_domain_for_dev(priv->dma_dev);
 
 		if (IS_ERR(mapping))
 			return PTR_ERR(mapping);
 		priv->mapping = mapping;
 	}
 
-	return drm_iommu_attach_device(drm, dev, dma_priv);
+	return drm_iommu_attach_device(drm, dev);
 }
 
-void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev,
-			       void **dma_priv)
+void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev)
 {
 	if (IS_ENABLED(CONFIG_EXYNOS_IOMMU))
-		drm_iommu_detach_device(drm, dev, dma_priv);
+		drm_iommu_detach_device(drm, dev);
 }
 
 void exynos_drm_cleanup_dma(struct drm_device *drm)

@@ -163,7 +122,5 @@ void exynos_drm_cleanup_dma(struct drm_device *drm)
 	if (!IS_ENABLED(CONFIG_EXYNOS_IOMMU))
 		return;
 
-	arm_iommu_release_mapping(priv->mapping);
-	priv->mapping = NULL;
 	priv->dma_dev = NULL;
 }

drivers/gpu/drm/exynos/exynos_drm_drv.h +2 -4

index 6ae9056e7a18..d4d21d8cfb90 100644
@@ -223,10 +223,8 @@ static inline bool is_drm_iommu_supported(struct drm_device *drm_dev)
 	return priv->mapping ? true : false;
 }
 
-int exynos_drm_register_dma(struct drm_device *drm, struct device *dev,
-			    void **dma_priv);
-void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev,
-			       void **dma_priv);
+int exynos_drm_register_dma(struct drm_device *drm, struct device *dev);
+void exynos_drm_unregister_dma(struct drm_device *drm, struct device *dev);
 void exynos_drm_cleanup_dma(struct drm_device *drm);
 
 #ifdef CONFIG_DRM_EXYNOS_DPI

drivers/gpu/drm/exynos/exynos_drm_fimc.c +2 -3

index 29ab8be8604c..8ea2e1d77802 100644
@@ -97,7 +97,6 @@ struct fimc_scaler {
 struct fimc_context {
 	struct exynos_drm_ipp ipp;
 	struct drm_device *drm_dev;
-	void		*dma_priv;
 	struct device	*dev;
 	struct exynos_drm_ipp_task	*task;
 	struct exynos_drm_ipp_formats	*formats;

@@ -1134,7 +1133,7 @@ static int fimc_bind(struct device *dev, struct device *master, void *data)
 
 	ctx->drm_dev = drm_dev;
 	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
 	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
 			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -1154,7 +1153,7 @@ static void fimc_unbind(struct device *dev, struct device *master,
 	struct exynos_drm_ipp *ipp = &ctx->ipp;
 
 	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(drm_dev, dev);
 }
 
 static const struct component_ops fimc_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_fimd.c +2 -3

index bb67cad8371f..21aec38702fc 100644
@@ -167,7 +167,6 @@ static struct fimd_driver_data exynos5420_fimd_driver_data = {
 struct fimd_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
-	void				*dma_priv;
 	struct exynos_drm_crtc		*crtc;
 	struct exynos_drm_plane		planes[WINDOWS_NR];
 	struct exynos_drm_plane_config	configs[WINDOWS_NR];

@@ -1091,7 +1090,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 	if (is_drm_iommu_supported(drm_dev))
 		fimd_clear_channels(ctx->crtc);
 
-	return exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, dev);
 }
 
 static void fimd_unbind(struct device *dev, struct device *master,

@@ -1101,7 +1100,7 @@ static void fimd_unbind(struct device *dev, struct device *master,
 
 	fimd_atomic_disable(ctx->crtc);
 
-	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(ctx->drm_dev, ctx->dev);
 
 	if (ctx->encoder)
 		exynos_dpi_remove(ctx->encoder);

drivers/gpu/drm/exynos/exynos_drm_g2d.c +2 -3

index 03be31427181..256ceafd9945 100644
@@ -232,7 +232,6 @@ struct g2d_runqueue_node {
 
 struct g2d_data {
 	struct device			*dev;
-	void				*dma_priv;
 	struct clk			*gate_clk;
 	void __iomem			*regs;
 	int				irq;

@@ -1410,7 +1409,7 @@ static int g2d_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
-	ret = exynos_drm_register_dma(drm_dev, dev, &g2d->dma_priv);
+	ret = exynos_drm_register_dma(drm_dev, dev);
 	if (ret < 0) {
 		dev_err(dev, "failed to enable iommu.\n");
 		g2d_fini_cmdlist(g2d);

@@ -1435,7 +1434,7 @@ static void g2d_unbind(struct device *dev, struct device *master, void *data)
 	priv->g2d_dev = NULL;
 
 	cancel_work_sync(&g2d->runqueue_work);
-	exynos_drm_unregister_dma(g2d->drm_dev, dev, &g2d->dma_priv);
+	exynos_drm_unregister_dma(g2d->drm_dev, dev);
 }
 
 static const struct component_ops g2d_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_gsc.c +2 -3

index 45e9aee8366a..88b6fcaa20be 100644
@@ -97,7 +97,6 @@ struct gsc_scaler {
 struct gsc_context {
 	struct exynos_drm_ipp ipp;
 	struct drm_device *drm_dev;
-	void		*dma_priv;
 	struct device	*dev;
 	struct exynos_drm_ipp_task	*task;
 	struct exynos_drm_ipp_formats	*formats;

@@ -1170,7 +1169,7 @@ static int gsc_bind(struct device *dev, struct device *master, void *data)
 
 	ctx->drm_dev = drm_dev;
 	ctx->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
 	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
 			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -1190,7 +1189,7 @@ static void gsc_unbind(struct device *dev, struct device *master,
 	struct exynos_drm_ipp *ipp = &ctx->ipp;
 
 	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(drm_dev, dev, &ctx->dma_priv);
+	exynos_drm_unregister_dma(drm_dev, dev);
 }
 
 static const struct component_ops gsc_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_rotator.c +2 -3

index 2d94afba031e..f22fa0d2621a 100644
@@ -56,7 +56,6 @@ struct rot_variant {
 struct rot_context {
 	struct exynos_drm_ipp ipp;
 	struct drm_device *drm_dev;
-	void		*dma_priv;
 	struct device	*dev;
 	void __iomem	*regs;
 	struct clk	*clock;

@@ -244,7 +243,7 @@ static int rotator_bind(struct device *dev, struct device *master, void *data)
 
 	rot->drm_dev = drm_dev;
 	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &rot->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
 	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
 			   DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE,

@@ -262,7 +261,7 @@ static void rotator_unbind(struct device *dev, struct device *master,
 	struct exynos_drm_ipp *ipp = &rot->ipp;
 
 	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(rot->drm_dev, rot->dev, &rot->dma_priv);
+	exynos_drm_unregister_dma(rot->drm_dev, rot->dev);
 }
 
 static const struct component_ops rotator_component_ops = {

drivers/gpu/drm/exynos/exynos_drm_scaler.c +2 -4

index ce1857138f89..0c560566bd2e 100644
@@ -39,7 +39,6 @@ struct scaler_data {
 struct scaler_context {
 	struct exynos_drm_ipp		ipp;
 	struct drm_device		*drm_dev;
-	void				*dma_priv;
 	struct device			*dev;
 	void __iomem			*regs;
 	struct clk			*clock[SCALER_MAX_CLK];

@@ -451,7 +450,7 @@ static int scaler_bind(struct device *dev, struct device *master, void *data)
 
 	scaler->drm_dev = drm_dev;
 	ipp->drm_dev = drm_dev;
-	exynos_drm_register_dma(drm_dev, dev, &scaler->dma_priv);
+	exynos_drm_register_dma(drm_dev, dev);
 
 	exynos_drm_ipp_register(dev, ipp, &ipp_funcs,
 			DRM_EXYNOS_IPP_CAP_CROP | DRM_EXYNOS_IPP_CAP_ROTATE |

@@ -471,8 +470,7 @@ static void scaler_unbind(struct device *dev, struct device *master,
 	struct exynos_drm_ipp *ipp = &scaler->ipp;
 
 	exynos_drm_ipp_unregister(dev, ipp);
-	exynos_drm_unregister_dma(scaler->drm_dev, scaler->dev,
-				  &scaler->dma_priv);
+	exynos_drm_unregister_dma(scaler->drm_dev, scaler->dev);
 }
 
 static const struct component_ops scaler_component_ops = {

drivers/gpu/drm/exynos/exynos_mixer.c +1 -3

index af192e5a16ef..18972e605c5d 100644
@@ -94,7 +94,6 @@ struct mixer_context {
 	struct platform_device *pdev;
 	struct device		*dev;
 	struct drm_device	*drm_dev;
-	void			*dma_priv;
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	unsigned long		flags;

@@ -895,14 +894,12 @@ static int mixer_initialize(struct mixer_context *mixer_ctx,
 		}
 	}
 
-	return exynos_drm_register_dma(drm_dev, mixer_ctx->dev,
-				       &mixer_ctx->dma_priv);
+	return exynos_drm_register_dma(drm_dev, mixer_ctx->dev);

Now that arch/arm is wired up for default domains and iommu-dma, we no longer need to work around the arch-private mapping.

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/staging/media/tegra-vde/iommu.c | 12 ------------
 1 file changed, 12 deletions(-)

drivers/staging/media/tegra-vde/iommu.c +0 -5

index 6af863d92123..4f770189ed34 100644
@@ -10,10 +10,6 @@
 #include <linux/kernel.h>
 #include <linux/platform_device.h>
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
 #include "vde.h"
 
 int tegra_vde_iommu_map(struct tegra_vde *vde,

@@ -70,14 +66,6 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
 	if (!vde->group)
 		return 0;
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

Alternatively, the Tegra SMMU could be changed such that the devices will be attached to a domain at the time of a first IOMMU mapping invocation instead of attaching at the time of attach_dev() callback invocation.

Or maybe even IOMMU core could be changed to attach devices at the time of the first IOMMU mapping invocation? This could be a universal solution for all drivers.

20.08.2020 18:08, Robin Murphy пишет:

Quoted 29 lines

Now that arch/arm is wired up for default domains and iommu-dma, we no longer need to work around the arch-private mapping.

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/staging/media/tegra-vde/iommu.c | 12 ------------
  1 file changed, 12 deletions(-)

drivers/staging/media/tegra-vde/iommu.c +0 -5

index 6af863d92123..4f770189ed34 100644
@@ -10,10 +10,6 @@
  #include <linux/kernel.h>
  #include <linux/platform_device.h>
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
-#include <asm/dma-iommu.h>
-#endif
-
  #include "vde.h"
 
  int tegra_vde_iommu_map(struct tegra_vde *vde,

@@ -70,14 +66,6 @@ int tegra_vde_iommu_init(struct tegra_vde *vde)
  	if (!vde->group)
  		return 0;
 
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)

drivers/staging/media/tegra-vde/iommu.c int tegra_vde_iommu_init(struct tegra_vde *vde) · line 71

Quoted 10 lines

drivers/staging/media/tegra-vde/iommu.c +0 -7

-	if (dev->archdata.mapping) {
-		struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);
-
-		arm_iommu_detach_device(dev);
-		arm_iommu_release_mapping(mapping);
-	}
-#endif
  	vde->domain = iommu_domain_alloc(&platform_bus_type);
  	if (!vde->domain) {
  		err = -ENOMEM;

Hello, Robin! Thank you for yours work!

Some drivers, like this Tegra VDE (Video Decoder Engine) driver for example, do not want to use implicit IOMMU domain.

Tegra VDE driver
relies on explicit IOMMU domain in a case of Tegra SMMU because VDE hardware can't access last page of the AS and because driver wants to reserve some fixed addresses [1].

[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/iommu.c#L100

Tegra30 SoC supports up to 4 domains, hence it's not possible to afford wasting unused implicit domains. I think this needs to be addressed before this patch could be applied.

Would it be possible for IOMMU drivers to gain support for filtering out devices in iommu_domain_alloc(dev, type)? Then perhaps Tegra SMMU driver could simply return NULL in a case of type=IOMMU_DOMAIN_DMA and dev=tegra-vde.

Alternatively, the Tegra SMMU could be changed such that the devices will be attached to a domain at the time of a first IOMMU mapping invocation instead of attaching at the time of attach_dev() callback invocation.

Or maybe even IOMMU core could be changed to attach devices at the time of the first IOMMU mapping invocation? This could be a universal solution for all drivers.

Quoted 4 lines

Hello, Robin! Thank you for yours work!

Some drivers, like this Tegra VDE (Video Decoder Engine) driver for example, do not want to use implicit IOMMU domain.

That isn't (intentionally) changing here - the only difference should be that instead of having the ARM-special implicit domain, which you have to kick out of the way with the ARM-specific API before you're able to attach your own domain, the implicit domain is now a proper IOMMU API default domain, which automatically gets bumped by your attach. The default domains should still only be created in the same cases that the ARM dma_iommu_mappings were.

Quoted 5 lines

Tegra VDE driver
relies on explicit IOMMU domain in a case of Tegra SMMU because VDE hardware can't access last page of the AS and because driver wants to reserve some fixed addresses [1].

Quoted 7 lines

[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/iommu.c#L100

Tegra30 SoC supports up to 4 domains, hence it's not possible to afford wasting unused implicit domains. I think this needs to be addressed before this patch could be applied.

Yeah, there is one subtle change in behaviour from removing the ARM layer on top of the core API, in that the IOMMU driver will no longer see an explicit detach call. Thus it does stand to benefit from being a bit cleverer about noticing devices being moved from one domain to another by an attach call, either by releasing the hardware context for the inactive domain once the device(s) are moved across to the new one, or by simply reprogramming the hardware context in-place for the new domain's address space without allocating a new one at all (most of the drivers that don't have multiple contexts already handle the latter approach quite well).

Quoted 4 lines

Would it be possible for IOMMU drivers to gain support for filtering out devices in iommu_domain_alloc(dev, type)? Then perhaps Tegra SMMU driver could simply return NULL in a case of type=IOMMU_DOMAIN_DMA and dev=tegra-vde.

If you can implement IOMMU_DOMAIN_IDENTITY by allowing the relevant devices to bypass translation entirely without needing a hardware context (or at worst, can spare one context which all identity-mapped logical domains can share), then you could certainly do that kind of filtering with the .def_domain_type callback if you really wanted to. As above, the intent is that that shouldn't be necessary for this particular case, since only one of a group's default domain and explicitly attached domain can be live at any given time, so the driver should be able to take advantage of that.

If you simply have more active devices (groups) than available contexts then yes, you probably would want to do some filtering to decide who deserves a translation domain and who doesn't, but in that case you should already have had a long-standing problem with the ARM implicit domains.

Quoted 8 lines

Alternatively, the Tegra SMMU could be changed such that the devices will be attached to a domain at the time of a first IOMMU mapping invocation instead of attaching at the time of attach_dev() callback invocation.

Or maybe even IOMMU core could be changed to attach devices at the time of the first IOMMU mapping invocation? This could be a universal solution for all drivers.

I suppose technically you could do that within an IOMMU driver already (similar to how some defer most of setup that logically belongs to ->domain_alloc until the first ->attach_dev). It's a bit grim from the caller's PoV though, in terms of the failure mode being non-obvious and having no real way to recover. Again, you'd be better off simply making decisions up-front at domain_alloc or attach time based on the domain type.

21.08.2020 03:11, Robin Murphy пишет:
...

Quoted 67 lines

Quoted 4 lines

Hello, Robin! Thank you for yours work!

Some drivers, like this Tegra VDE (Video Decoder Engine) driver for example, do not want to use implicit IOMMU domain.

That isn't (intentionally) changing here - the only difference should be that instead of having the ARM-special implicit domain, which you have to kick out of the way with the ARM-specific API before you're able to attach your own domain, the implicit domain is now a proper IOMMU API default domain, which automatically gets bumped by your attach. The default domains should still only be created in the same cases that the ARM dma_iommu_mappings were.

Quoted 5 lines

Tegra VDE driver
relies on explicit IOMMU domain in a case of Tegra SMMU because VDE hardware can't access last page of the AS and because driver wants to reserve some fixed addresses [1].

Quoted 7 lines

[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/iommu.c#L100

Tegra30 SoC supports up to 4 domains, hence it's not possible to afford wasting unused implicit domains. I think this needs to be addressed before this patch could be applied.

Yeah, there is one subtle change in behaviour from removing the ARM layer on top of the core API, in that the IOMMU driver will no longer see an explicit detach call. Thus it does stand to benefit from being a bit cleverer about noticing devices being moved from one domain to another by an attach call, either by releasing the hardware context for the inactive domain once the device(s) are moved across to the new one, or by simply reprogramming the hardware context in-place for the new domain's address space without allocating a new one at all (most of the drivers that don't have multiple contexts already handle the latter approach quite well).

Quoted 4 lines

Would it be possible for IOMMU drivers to gain support for filtering out devices in iommu_domain_alloc(dev, type)? Then perhaps Tegra SMMU driver could simply return NULL in a case of type=IOMMU_DOMAIN_DMA and dev=tegra-vde.

If you can implement IOMMU_DOMAIN_IDENTITY by allowing the relevant devices to bypass translation entirely without needing a hardware context (or at worst, can spare one context which all identity-mapped logical domains can share), then you could certainly do that kind of filtering with the .def_domain_type callback if you really wanted to. As above, the intent is that that shouldn't be necessary for this particular case, since only one of a group's default domain and explicitly attached domain can be live at any given time, so the driver should be able to take advantage of that.

If you simply have more active devices (groups) than available contexts then yes, you probably would want to do some filtering to decide who deserves a translation domain and who doesn't, but in that case you should already have had a long-standing problem with the ARM implicit domains.

Quoted 8 lines

Alternatively, the Tegra SMMU could be changed such that the devices will be attached to a domain at the time of a first IOMMU mapping invocation instead of attaching at the time of attach_dev() callback invocation.

Or maybe even IOMMU core could be changed to attach devices at the time of the first IOMMU mapping invocation? This could be a universal solution for all drivers.

Quoted 6 lines

I suppose technically you could do that within an IOMMU driver already (similar to how some defer most of setup that logically belongs to ->domain_alloc until the first ->attach_dev). It's a bit grim from the caller's PoV though, in terms of the failure mode being non-obvious and having no real way to recover. Again, you'd be better off simply making decisions up-front at domain_alloc or attach time based on the domain type.

Robin, thank you very much for the clarifications!

In accordance to yours comments, this patch can't be applied until Tegra SMMU will support IOMMU_DOMAIN_IDENTITY and implement def_domain_type() callback that returns IOMMU_DOMAIN_IDENTITY for the VDE device.

Otherwise you're breaking the VDE driver because dma_buf_map_attachment() [1] returns the IOMMU SGT of the implicit domain which is then mapped into the VDE's explicit domain [2], and this is a nonsense.

[1]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/dmabuf-cache.c#L102

[2]
https://elixir.bootlin.com/linux/v5.9-rc1/source/drivers/staging/media/tegra-vde/dmabuf-cache.c#L122

Hence, either VDE driver should bypass iommu_dma_ops from the start or it needs a way to kick out the ops, like it does this using ARM's arm_iommu_detach_device().

The same applies to the Tegra GPU devices, otherwise you're breaking them as well because Tegra DRM is sensible to implicit vs explicit domain.

BTW, I tried to apply this series and T30 doesn't boot anymore. I don't have more info for now.

Quoted 10 lines

Robin, thank you very much for the clarifications!

In accordance to yours comments, this patch can't be applied until Tegra SMMU will support IOMMU_DOMAIN_IDENTITY and implement def_domain_type() callback that returns IOMMU_DOMAIN_IDENTITY for the VDE device.

Otherwise you're breaking the VDE driver because dma_buf_map_attachment() [1] returns the IOMMU SGT of the implicit domain which is then mapped into the VDE's explicit domain [2], and this is a nonsense.

It's true that iommu_dma_ops will do some work in the unattached default domain, but non-coherent cache maintenance will still be performed correctly on the underlying memory, which is really all that you care about for this case. As for tegra_vde_iommu_map(), that seems to do the right thing in only referencing the physical side of the scatterlist (via iommu_map_sg()) and ignoring the DMA side, so things ought to work out OK even if it is a little non-obvious.

Quoted 8 lines

Hence, either VDE driver should bypass iommu_dma_ops from the start or it needs a way to kick out the ops, like it does this using ARM's arm_iommu_detach_device().

The same applies to the Tegra GPU devices, otherwise you're breaking them as well because Tegra DRM is sensible to implicit vs explicit domain.

Note that Tegra DRM will only be as broken as its current state on arm64, and I was under the impression that that was OK now - at least I don't recall seeing any complaints since 43c5bf11a610. Although that commit and the one before it are resolving the scalability issue that they describe, it was very much in my mind at the time that they also have the happy side-effect described above - the default domain isn't *completely* out of the way, but it's far enough that sensible cases should be able to work as expected.

Quoted 2 lines

BTW, I tried to apply this series and T30 doesn't boot anymore. I don't have more info for now.

Yeah, I'm still trying to get to the bottom of whether it's actually working as intended at all, even on my RK3288. So far my debugging instrumentation has been confusingly inconclusive :/

24.08.2020 17:01, Robin Murphy пишет:
...

Quoted 11 lines

Quoted 10 lines

Robin, thank you very much for the clarifications!

In accordance to yours comments, this patch can't be applied until Tegra SMMU will support IOMMU_DOMAIN_IDENTITY and implement def_domain_type() callback that returns IOMMU_DOMAIN_IDENTITY for the VDE device.

Otherwise you're breaking the VDE driver because dma_buf_map_attachment() [1] returns the IOMMU SGT of the implicit domain which is then mapped into the VDE's explicit domain [2], and this is a nonsense.

Quoted 7 lines

It's true that iommu_dma_ops will do some work in the unattached default domain, but non-coherent cache maintenance will still be performed correctly on the underlying memory, which is really all that you care about for this case. As for tegra_vde_iommu_map(), that seems to do the right thing in only referencing the physical side of the scatterlist (via iommu_map_sg()) and ignoring the DMA side, so things ought to work out OK even if it is a little non-obvious.

I'll need to double-check this, it's indeed not clear to me right now.

I see that if Tegra DRM driver uses implicit IOMMU domain, then when VDE driver imports DMA-buf from Terga DRM and the imported buffer will be auto-mapped to the implicit VDE IOVA [1].

[1]
https://elixir.bootlin.com/linux/v5.9-rc2/source/drivers/gpu/drm/tegra/gem.c#L574

Quoted 9 lines

Quoted 8 lines

Hence, either VDE driver should bypass iommu_dma_ops from the start or it needs a way to kick out the ops, like it does this using ARM's arm_iommu_detach_device().

The same applies to the Tegra GPU devices, otherwise you're breaking them as well because Tegra DRM is sensible to implicit vs explicit domain.

Quoted 8 lines

Note that Tegra DRM will only be as broken as its current state on arm64, and I was under the impression that that was OK now - at least I don't recall seeing any complaints since 43c5bf11a610. Although that commit and the one before it are resolving the scalability issue that they describe, it was very much in my mind at the time that they also have the happy side-effect described above - the default domain isn't *completely* out of the way, but it's far enough that sensible cases should be able to work as expected.

The Tegra DRM has a very special quirk for ARM32 that was added in this commit [2] and driver relies on checking of whether explicit or implicit IOMMU is used in order to activate the quirk.

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=273da5a046965ccf0ec79eb63f2d5173467e20fa

Once the implicit IOMMU is used for the DRM driver, the quirk no longer works (if I'm not missing something). This problem needs to be resolved before implicit IOMMU could be used by the Tegra DRM on ARM32.

Quoted 6 lines

Quoted 2 lines

BTW, I tried to apply this series and T30 doesn't boot anymore. I don't have more info for now.

Yeah, I'm still trying to get to the bottom of whether it's actually working as intended at all, even on my RK3288. So far my debugging instrumentation has been confusingly inconclusive :/

Surely it will take some time to resolve all the problems and it's great that you're pushing this work!

I'll try to help with fixing the ARM32 Tegra side of the problems. I added this to my "TODO" list and should be able to take a closer look during of this/next weeks!

Quoted 10 lines

The Tegra DRM has a very special quirk for ARM32 that was added in this commit [2] and driver relies on checking of whether explicit or implicit IOMMU is used in order to activate the quirk.

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=273da5a046965ccf0ec79eb63f2d5173467e20fa

Once the implicit IOMMU is used for the DRM driver, the quirk no longer works (if I'm not missing something). This problem needs to be resolved before implicit IOMMU could be used by the Tegra DRM on ARM32.

I do have a patch lying around somewhere that implements the mapping cache that was referenced in the above commit. Let me know if I should dig that up and send it out.

Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually.

Signed-off-by Robin Murphy <[email protected]>

Hi Robin,

Quoted 5 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually.

Signed-off-by Robin Murphy <[email protected]>

Thanks for the patch.

I haven't looked at the details but it seems that this causes the buffer memory allocation to be physically contiguous, which causes a failure to allocate video buffers of entirely normal size. I guess that was not intentional?

-----------------8<---------------------------

[  218.934448] WARNING: CPU: 0 PID: 1994 at mm/page_alloc.c:4859 __alloc_pages_nodemask+0x9c/0xb1c
[  218.943847] Modules linked in: omap3_isp videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 videobuf2_common leds_as3645a smiapp v4l2_flash_led_class led_class_flash v4l2_fwnode smiapp_pll videodev leds_gpio mc led_class
[  218.964660] CPU: 0 PID: 1994 Comm: yavta Not tainted 5.9.0-rc1-dirty #1818
[  218.972442] Hardware name: Generic OMAP36xx (Flattened Device Tree)
[  218.978973] Backtrace:
[  218.981842] [<c010bf90>] (dump_backtrace) from [<c010c350>] (show_stack+0x20/0x24)
[  218.989715]  r7:00000000 r6:00000009 r5:c08f03bc r4:c08f2fef
[  218.995880] [<c010c330>] (show_stack) from [<c03d3328>] (dump_stack+0x28/0x30)
[  219.003631] [<c03d3300>] (dump_stack) from [<c012e324>] (__warn+0x100/0x118)
[  219.010955]  r5:c08f03bc r4:00000000
[  219.014953] [<c012e224>] (__warn) from [<c012e6f4>] (warn_slowpath_fmt+0x84/0xa8)
[  219.022949]  r9:c0232090 r8:c08f03bc r7:c0b08a88 r6:00000009 r5:000012fb r4:00000000
[  219.031036] [<c012e674>] (warn_slowpath_fmt) from [<c0232090>] (__alloc_pages_nodemask+0x9c/0xb1c)
[  219.040557]  r9:c0185c3c r8:00000000 r7:010ec000 r6:00000000 r5:0000000d r4:00000000
[  219.048858] [<c0231ff4>] (__alloc_pages_nodemask) from [<c01108f0>] (__dma_alloc_buffer.constprop.14+0x3c/0x90)
[  219.059570]  r10:00000cc0 r9:c0185c3c r8:00000000 r7:010ec000 r6:0000000d r5:c0b08a88
[  219.067901]  r4:00000cc0
[  219.070587] [<c01108b4>] (__dma_alloc_buffer.constprop.14) from [<c0110a6c>] (remap_allocator_alloc+0x34/0x7c)
[  219.081207]  r9:c0185c3c r8:00000247 r7:e6d7fb84 r6:010ec000 r5:c0b08a88 r4:00000001
[  219.089263] [<c0110a38>] (remap_allocator_alloc) from [<c010f4f4>] (__dma_alloc+0x124/0x21c)
[  219.098236]  r9:ed99fc10 r8:e69aa890 r7:00000000 r6:ffffffff r5:c0b08a88 r4:e6fdd680
[  219.106536] [<c010f3d0>] (__dma_alloc) from [<c010f69c>] (arm_dma_alloc+0x68/0x74)
[  219.114654]  r10:00000cc0 r9:c0185c3c r8:00000cc0 r7:e69aa890 r6:010ec000 r5:ed99fc10
[  219.122985]  r4:00000000
[  219.125671] [<c010f634>] (arm_dma_alloc) from [<c0185c3c>] (dma_alloc_attrs+0xe4/0x120)
[  219.134216]  r9:00000000 r8:e69aa890 r7:010ec000 r6:c0b08a88 r5:ed99fc10 r4:c010f634
[  219.142517] [<c0185b58>] (dma_alloc_attrs) from [<bf095c3c>] (vb2_dc_alloc+0xcc/0x108 [videobuf2_dma_contig])
[  219.153076]  r10:e6885ca8 r9:e6abfc48 r8:00000002 r7:00000000 r6:010ec000 r5:ed99fc10
[  219.161407]  r4:e69aa880
[  219.164184] [<bf095b70>] (vb2_dc_alloc [videobuf2_dma_contig]) from [<bf080fd0>] (__vb2_queue_alloc+0x258/0x4a4 [videobuf2_common])
[  219.176696]  r8:bf095b70 r7:010ec000 r6:00000000 r5:e6885ca8 r4:e6abfc00
[  219.183959] [<bf080d78>] (__vb2_queue_alloc [videobuf2_common]) from [<bf0833a0>] (vb2_core_reqbufs+0x408/0x498 [videobuf2_common])
[  219.196533]  r10:e6885ce8 r9:00000000 r8:e6d7fe24 r7:e6d7fcec r6:bf09ced4 r5:bf088580
[  219.204895]  r4:e6885ca8
[  219.207672] [<bf082f98>] (vb2_core_reqbufs [videobuf2_common]) from [<bf08e1cc>] (vb2_reqbufs+0x64/0x70 [videobuf2_v4l2])
[  219.219268]  r10:00000000 r9:bf032bc0 r8:c0145608 r7:bf0ad4a4 r6:e6885ca8 r5:00000000
[  219.227600]  r4:e6d7fe24
[  219.230499] [<bf08e168>] (vb2_reqbufs [videobuf2_v4l2]) from [<bf09d7b4>] (isp_video_reqbufs+0x40/0x54 [omap3_isp])
[  219.241607]  r7:bf0ad4a4 r6:e6d7fe24 r5:e6885c00 r4:e6cca928
[  219.247924] [<bf09d774>] (isp_video_reqbufs [omap3_isp]) from [<bf01de4c>] (v4l_reqbufs+0x4c/0x50 [videodev])
[  219.258514]  r7:bf0ad4a4 r6:e6885c00 r5:e6d7fe24 r4:e7efbec0
[  219.264984] [<bf01de00>] (v4l_reqbufs [videodev]) from [<bf01eeb4>] (__video_do_ioctl+0x2d8/0x414 [videodev])
[  219.275512]  r7:bf01de00 r6:00000000 r5:00000000 r4:e6cca2e0
[  219.281982] [<bf01ebdc>] (__video_do_ioctl [videodev]) from [<bf01fa1c>] (video_usercopy+0x144/0x508 [videodev])
[  219.292816]  r10:e7efbec0 r9:c0145608 r8:e6d7fe24 r7:00000000 r6:00000000 r5:bf01ebdc
[  219.300933]  r4:c0145608
[  219.304168] [<bf01f8d8>] (video_usercopy [videodev]) from [<bf01fdfc>] (video_ioctl2+0x1c/0x24 [videodev])
[  219.314453]  r10:e7fbfda0 r9:e7efbec0 r8:00000003 r7:00000000 r6:bee658f4 r5:c0145608
[  219.322784]  r4:e7efbec0
[  219.325775] [<bf01fde0>] (video_ioctl2 [videodev]) from [<bf01814c>] (v4l2_ioctl+0x50/0x64 [videodev])
[  219.335845] [<bf0180fc>] (v4l2_ioctl [videodev]) from [<c02654a0>] (vfs_ioctl+0x30/0x44)
[  219.344482]  r7:00000000 r6:e7efbec0 r5:bee658f4 r4:c0145608
[  219.350402] [<c0265470>] (vfs_ioctl) from [<c0265e9c>] (sys_ioctl+0xdc/0x7ec)
[  219.358062] [<c0265dc0>] (sys_ioctl) from [<c0100080>] (ret_fast_syscall+0x0/0x28)
[  219.366149] Exception stack(0xe6d7ffa8 to 0xe6d7fff0)
[  219.371673] ffa0:                   00000000 bee65c1a 00000003 c0145608 bee658f4 00000001
[  219.380157] ffc0: 00000000 bee65c1a 00000000 00000036 000009a0 00000000 0000ef30 010eb400
[  219.388885] ffe0: 0001716c bee65104 0000b588 b6e413ac
[  219.394409]  r10:00000036 r9:e6d7e000 r8:c0100244 r7:00000036 r6:00000000 r5:bee65c1a
[  219.402740]  r4:00000000
[  219.405426] irq event stamp: 5075
[  219.408905] hardirqs last  enabled at (5083): [<c01778b0>] console_unlock+0x4cc/0x524
[  219.417297] hardirqs last disabled at (5092): [<c01777ac>] console_unlock+0x3c8/0x524
[  219.425628] softirqs last  enabled at (4532): [<c01017d8>] __do_softirq+0x1f0/0x490
[  219.433837] softirqs last disabled at (4493): [<c0132c20>] irq_exit+0xe4/0x160
[  219.441558] ---[ end trace 8c56810633cf24db ]---
[  219.446502] omap3isp 480bc000.isp: dma_alloc_coherent of size 17743872 failed

-----------------8<---------------------------

Quoted 10 lines · Sakari Ailus

Hi Robin,

Quoted 5 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually.

Signed-off-by Robin Murphy <[email protected]>

Thanks for the patch.

Many thanks for testing so quickly!

Quoted 4 lines

I haven't looked at the details but it seems that this causes the buffer memory allocation to be physically contiguous, which causes a failure to allocate video buffers of entirely normal size. I guess that was not intentional?

Hmm, it looks like the device ends up with the wrong DMA ops, which implies something didn't go as expected with the earlier IOMMU setup and default domain creation. Chances are that either I missed some subtlety in the omap_iommu change, or I've fundamentally misjudged how the ISP probing works and it never actually goes down the of_iommu_configure() path in the first place. Do you get any messages from the IOMMU layer earlier on during boot?

Quoted 19 lines · Robin Murphy

Quoted 10 lines · Sakari Ailus

Hi Robin,

Quoted 5 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually.

Signed-off-by Robin Murphy <[email protected]>

Thanks for the patch.

Many thanks for testing so quickly!

Quoted 4 lines

I haven't looked at the details but it seems that this causes the buffer memory allocation to be physically contiguous, which causes a failure to allocate video buffers of entirely normal size. I guess that was not intentional?

Quoted 6 lines

Hmm, it looks like the device ends up with the wrong DMA ops, which implies something didn't go as expected with the earlier IOMMU setup and default domain creation. Chances are that either I missed some subtlety in the omap_iommu change, or I've fundamentally misjudged how the ISP probing works and it never actually goes down the of_iommu_configure() path in the first place. Do you get any messages from the IOMMU layer earlier on during boot?

I do get these:

[    2.934936] iommu: Default domain type: Translated
[    2.940917] omap-iommu 480bd400.mmu: 480bd400.mmu registered
[    2.946899] platform 480bc000.isp: Adding to iommu group 0

Quoted 20 lines · Sakari Ailus

Quoted 19 lines · Robin Murphy

Quoted 10 lines · Sakari Ailus

Hi Robin,

Quoted 5 lines · Robin Murphy

Now that arch/arm is wired up for default domains and iommu-dma, devices behind IOMMUs will get mappings set up automatically as appropriate, so there is no need for drivers to do so manually.

Signed-off-by Robin Murphy <[email protected]>

Thanks for the patch.

Many thanks for testing so quickly!

Quoted 4 lines

I haven't looked at the details but it seems that this causes the buffer memory allocation to be physically contiguous, which causes a failure to allocate video buffers of entirely normal size. I guess that was not intentional?

Quoted 6 lines

Quoted 6 lines

Hmm, it looks like the device ends up with the wrong DMA ops, which implies something didn't go as expected with the earlier IOMMU setup and default domain creation. Chances are that either I missed some subtlety in the omap_iommu change, or I've fundamentally misjudged how the ISP probing works and it never actually goes down the of_iommu_configure() path in the first place. Do you get any messages from the IOMMU layer earlier on during boot?

Quoted 7 lines

I do get these:

[    2.934936] iommu: Default domain type: Translated [    2.940917] omap-iommu 480bd400.mmu: 480bd400.mmu registered [    2.946899] platform 480bc000.isp: Adding to iommu group 0

So that much looks OK, if there are no obvious errors. Unfortunately there's no easy way to tell exactly what of_iommu_configure() is doing (beyond enabling a couple of vague debug messages). The first thing I'll do tomorrow is double-check whether it's really working on my boards here, or whether I was just getting lucky with CMA... (I assume you don't have CMA enabled if you're ending up in remap_allocator_alloc())

Robin.