This series patches mainly for ARM64 supporting. To do this, it first add virtual iommu slave device which DRM can attach to, convert DRM driver to use common iommu API instead of the ARM32 functions, and then use DMA API in iommu driver to map, to flush cache.
The v2 patches make a lot changes vs v1, so please forget the v1.
Shunqian Zheng (4): iommu/rockchip: support virtual iommu slave device ARM: dts: rockchip: add virtual iommu for display drm: rockchip: use common iommu api to attach iommu iommu/rockchip: use DMA API to map, to flush cache
Simon Xue (3): iommu/rockchip: fix devm_{request,free}_irq parameter iommu/rockchip: add map_sg callback for rk_iommu_ops iommu/rockchip: enable rockchip iommu on ARM64 platform
arch/arm/boot/dts/rk3288.dtsi | 6 ++ drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 ++++++++++++++++-------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + drivers/iommu/Kconfig | 2 +- drivers/iommu/rockchip-iommu.c | 151 ++++++++++++++++++---------- 5 files changed, 193 insertions(+), 97 deletions(-)
From: Simon Xue xxm@rock-chips.com
Even though the iommu shares irq with its master, using the *dev of iommu instead of master's *dev for devm_{request,free}_irq makes things clear.
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- drivers/iommu/rockchip-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index c7d6156..ec0ce62 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -807,7 +807,7 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
iommu->domain = domain;
- ret = devm_request_irq(dev, iommu->irq, rk_iommu_irq, + ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq, IRQF_SHARED, dev_name(dev), iommu); if (ret) return ret; @@ -860,7 +860,7 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, } rk_iommu_disable_stall(iommu);
- devm_free_irq(dev, iommu->irq, iommu); + devm_free_irq(iommu->dev, iommu->irq, iommu);
iommu->domain = NULL;
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
From: Simon Xue xxm@rock-chips.com
Even though the iommu shares irq with its master, using the *dev of iommu instead of master's *dev for devm_{request,free}_irq makes things clear.
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/rockchip-iommu.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Tomasz Figa tfiga@chromium.org
Best regards, Tomasz
From: Simon Xue xxm@rock-chips.com
The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg() that requires the callback iommu_ops .map_sg(). Adding the default_iommu_map_sg() to rockchip iommu accordingly.
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng xxm@rock-chips.com --- drivers/iommu/rockchip-iommu.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index ec0ce62..3c16ec3 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -1022,6 +1022,7 @@ static const struct iommu_ops rk_iommu_ops = { .detach_dev = rk_iommu_detach_device, .map = rk_iommu_map, .unmap = rk_iommu_unmap, + .map_sg = default_iommu_map_sg, .add_device = rk_iommu_add_device, .remove_device = rk_iommu_remove_device, .iova_to_phys = rk_iommu_iova_to_phys,
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
From: Simon Xue xxm@rock-chips.com
The iommu_dma_alloc() in iommu/dma-iommu.c calls iommu_map_sg() that requires the callback iommu_ops .map_sg(). Adding the default_iommu_map_sg() to rockchip iommu accordingly.
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng xxm@rock-chips.com
drivers/iommu/rockchip-iommu.c | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Tomasz Figa tfiga@chromium.org
Best regards, Tomasz
An virtual master device like DRM need to attach to iommu domain to share the domain with VOP(the one with actual iommu slave). We currently check the group is NULL to indicate a virtual master, which is not true since we decide to use the common iommu api to attach device in DRM.
With this patch, we can probe a virtual iommu device and allow the DRM attaching to it. The virtual iommu is needed also because we want convert to use DMA API for map/unmap, cache flush, so that DRM buffer alloc still work even VOP is disabled.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- drivers/iommu/rockchip-iommu.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 3c16ec3..d6c3051 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -75,6 +75,11 @@
#define IOMMU_REG_POLL_COUNT_FAST 1000
+/* A virtual iommu in device-tree registered without reg or + * interrupts, so the num_mmu is zero. + */ +#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0) + struct rk_iommu_domain { struct list_head iommus; u32 *dt; /* page directory table */ @@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, int ret, i; phys_addr_t dte_addr;
- /* - * Allow 'virtual devices' (e.g., drm) to attach to domain. - * Such a device does not belong to an iommu group. - */ iommu = rk_iommu_from_dev(dev); - if (!iommu) + + iommu->domain = domain; + if (RK_IOMMU_IS_VIRTUAL(iommu)) { + dev_dbg(dev, "Attach virtual device to iommu domain\n"); return 0; + }
ret = rk_iommu_enable_stall(iommu); if (ret) @@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, if (ret) return ret;
- iommu->domain = domain;
ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq, IRQF_SHARED, dev_name(dev), iommu); @@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, unsigned long flags; int i;
- /* Allow 'virtual devices' (eg drm) to detach from domain */ iommu = rk_iommu_from_dev(dev); - if (!iommu) + + iommu->domain = NULL; + if (RK_IOMMU_IS_VIRTUAL(iommu)) { + dev_dbg(dev, "Master with virtual iommu detached from domain\n"); return; + }
spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_del_init(&iommu->node); @@ -862,8 +869,6 @@ static void rk_iommu_detach_device(struct iommu_domain *domain,
devm_free_irq(iommu->dev, iommu->irq, iommu);
- iommu->domain = NULL; - dev_dbg(dev, "Detached from iommu domain\n"); }
@@ -1034,6 +1039,7 @@ static int rk_iommu_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct rk_iommu *iommu; struct resource *res; + int num_res = pdev->num_resources; int i;
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL); @@ -1043,12 +1049,19 @@ static int rk_iommu_probe(struct platform_device *pdev) platform_set_drvdata(pdev, iommu); iommu->dev = dev; iommu->num_mmu = 0; - iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * iommu->num_mmu, + + if (!num_res) { + iommu->bases = NULL; + dev_info(dev, "this is a virtual iommu\n"); + return 0; + } + + iommu->bases = devm_kzalloc(dev, sizeof(*iommu->bases) * num_res, GFP_KERNEL); if (!iommu->bases) return -ENOMEM;
- for (i = 0; i < pdev->num_resources; i++) { + for (i = 0; i < num_res; i++) { res = platform_get_resource(pdev, IORESOURCE_MEM, i); if (!res) continue;
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
An virtual master device like DRM need to attach to iommu domain to share the domain with VOP(the one with actual iommu slave). We currently check the group is NULL to indicate a virtual master, which is not true since we decide to use the common iommu api to attach device in DRM.
With this patch, we can probe a virtual iommu device and allow the DRM attaching to it. The virtual iommu is needed also because we want convert to use DMA API for map/unmap, cache flush, so that DRM buffer alloc still work even VOP is disabled.
I'm not really convinced that this is a good idea. This will require creating fake devices in the system and generally looks really hacky. Please see my alternative proposal inline.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/rockchip-iommu.c | 37 +++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index 3c16ec3..d6c3051 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -75,6 +75,11 @@
#define IOMMU_REG_POLL_COUNT_FAST 1000
+/* A virtual iommu in device-tree registered without reg or
- interrupts, so the num_mmu is zero.
- */
+#define RK_IOMMU_IS_VIRTUAL(iommu) (iommu->num_mmu == 0)
struct rk_iommu_domain { struct list_head iommus; u32 *dt; /* page directory table */ @@ -789,13 +794,13 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, int ret, i; phys_addr_t dte_addr;
/** Allow 'virtual devices' (e.g., drm) to attach to domain.* Such a device does not belong to an iommu group.*/ iommu = rk_iommu_from_dev(dev);if (!iommu)
Could we instead allocate such virtual rk_iommu struct here (dev could be used as iommu->dev for logging purposes and a fake group could be allocated too)?
iommu->domain = domain;if (RK_IOMMU_IS_VIRTUAL(iommu)) {dev_dbg(dev, "Attach virtual device to iommu domain\n"); return 0;} ret = rk_iommu_enable_stall(iommu); if (ret)@@ -805,7 +810,6 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, if (ret) return ret;
iommu->domain = domain; ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq, IRQF_SHARED, dev_name(dev), iommu);@@ -842,10 +846,13 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, unsigned long flags; int i;
/* Allow 'virtual devices' (eg drm) to detach from domain */ iommu = rk_iommu_from_dev(dev);if (!iommu)
iommu->domain = NULL;
I don't think it's a good idea to set the domain to NULL before disabling the real IOMMU. It might still trigger an interrupt at this point and things won't behave correctly. I guess the original line could be left as is and simply same assignment added under the if below.
Best regards, Tomasz
An virtual iommu without reg or interrupts for display. Adding this according to iommu driver changes.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- arch/arm/boot/dts/rk3288.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 7fa932f..4cd535f 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -219,9 +219,15 @@ clock-names = "timer", "pclk"; };
+ display_mmu: virtual-iommu { + compatible = "rockchip,iommu"; + #iommu-cells = <0>; + }; + display-subsystem { compatible = "rockchip,display-subsystem"; ports = <&vopl_out>, <&vopb_out>; + iommus = <&display_mmu>; };
sdmmc: dwmmc@ff0c0000 {
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
An virtual iommu without reg or interrupts for display. Adding this according to iommu driver changes.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
arch/arm/boot/dts/rk3288.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 7fa932f..4cd535f 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -219,9 +219,15 @@ clock-names = "timer", "pclk"; };
display_mmu: virtual-iommu {compatible = "rockchip,iommu";#iommu-cells = <0>;};
Device tree should describe real hardware and so it isn't really a good idea to add such virtual iommu, especially using a compatible string of a real device.
Please see my comments to patch 3/7 for an alternative idea.
Best regards, Tomasz
Rockchip DRM used the arm special API, arm_iommu_*(), to attach iommu for ARM32 SoCs. This patch convert to common iommu API so it would support ARM64 like RK3399.
The general idea is domain_alloc(), attach_device() and arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++++++++++++++++++--------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + 2 files changed, 89 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index f5a68fc..7965a66 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -14,8 +14,6 @@ * GNU General Public License for more details. */
-#include <asm/dma-iommu.h> - #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> @@ -24,6 +22,8 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/component.h> +#include <linux/dma-iommu.h> +#include <linux/iommu.h>
#include "rockchip_drm_drv.h" #include "rockchip_drm_fb.h" @@ -46,7 +46,8 @@ static bool is_support_iommu = true; int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, struct device *dev) { - struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping; + struct rockchip_drm_private *private = drm_dev->dev_private; + struct iommu_domain *domain = private->domain; int ret;
if (!is_support_iommu) @@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
- return arm_iommu_attach_device(dev, mapping); + ret = iommu_attach_device(domain, dev); + + if (ret) { + dev_err(dev, "Failed to attach iommu device\n"); + return ret; + } + arch_setup_dma_ops(dev, 0x00000000, SZ_2G, + (struct iommu_ops *)dev->bus->iommu_ops, false); + return 0; }
void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, struct device *dev) { - if (!is_support_iommu) - return; + struct rockchip_drm_private *private = drm_dev->dev_private; + struct iommu_domain *domain = private->domain;
- arm_iommu_detach_device(dev); + if (is_support_iommu) + iommu_detach_device(domain, dev); }
int rockchip_register_crtc_funcs(struct drm_crtc *crtc, @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev, priv->crtc_funcs[pipe]->disable_vblank(crtc); }
+static int rockchip_drm_init_iommu(struct drm_device *drm_dev) +{ + struct rockchip_drm_private *private = drm_dev->dev_private; + struct device *dev = drm_dev->dev; + int ret; + + dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), + GFP_KERNEL); + if (!dev->dma_parms) { + ret = -ENOMEM; + return ret; + } + + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); + if (ret) { + dev_err(dev, "Failed to set coherent mask\n"); + return ret; + } + + dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); + + private->domain = iommu_domain_alloc(&platform_bus_type); + if (!private->domain) + return -ENOMEM; + + ret = iommu_get_dma_cookie(private->domain); + if (ret) { + dev_err(dev, "Failed to get dma cookie\n"); + goto err_free_domain; + } + + ret = iommu_dma_init_domain(private->domain, 0x00000000, SZ_2G); + if (ret) { + dev_err(dev, "Failed to init domain\n"); + goto err_put_cookie; + } + + ret = rockchip_drm_dma_attach_device(drm_dev, dev); + if (ret) { + dev_err(dev, "Failed to attach device\n"); + goto err_put_cookie; + } + + return 0; + +err_put_cookie: + iommu_put_dma_cookie(private->domain); +err_free_domain: + iommu_domain_free(private->domain); + + return ret; +} + +static void rockchip_iommu_cleanup(struct drm_device *drm_dev) +{ + struct rockchip_drm_private *private = drm_dev->dev_private; + + iommu_put_dma_cookie(private->domain); + iommu_domain_free(private->domain); +} + static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) { struct rockchip_drm_private *private; - struct dma_iommu_mapping *mapping = NULL; struct device *dev = drm_dev->dev; struct drm_connector *connector; int ret; @@ -153,38 +223,18 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags)
rockchip_drm_mode_config_init(drm_dev);
- dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms), - GFP_KERNEL); - if (!dev->dma_parms) { - ret = -ENOMEM; - goto err_config_cleanup; - } - if (is_support_iommu) { - /* TODO(djkurtz): fetch the mapping start/size from somewhere */ - mapping = arm_iommu_create_mapping(&platform_bus_type, - 0x00000000, - SZ_2G); - if (IS_ERR(mapping)) { - ret = PTR_ERR(mapping); - goto err_config_cleanup; - } - - ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); + ret = rockchip_drm_init_iommu(drm_dev); if (ret) - goto err_release_mapping; - - dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); - - ret = arm_iommu_attach_device(dev, mapping); - if (ret) - goto err_release_mapping; + goto err_config_cleanup; }
/* Try to bind all sub drivers. */ ret = component_bind_all(dev, drm_dev); - if (ret) - goto err_detach_device; + if (ret) { + dev_err(dev, "Failed to bind components\n"); + goto err_iommu_cleanup; + }
/* * All components are now added, we can publish the connector sysfs @@ -222,21 +272,17 @@ static int rockchip_drm_load(struct drm_device *drm_dev, unsigned long flags) if (ret) goto err_vblank_cleanup;
- if (is_support_iommu) - arm_iommu_release_mapping(mapping); return 0; + err_vblank_cleanup: drm_vblank_cleanup(drm_dev); err_kms_helper_poll_fini: drm_kms_helper_poll_fini(drm_dev); err_unbind: component_unbind_all(dev, drm_dev); -err_detach_device: - if (is_support_iommu) - arm_iommu_detach_device(dev); -err_release_mapping: +err_iommu_cleanup: if (is_support_iommu) - arm_iommu_release_mapping(mapping); + rockchip_iommu_cleanup(drm_dev); err_config_cleanup: drm_mode_config_cleanup(drm_dev); drm_dev->dev_private = NULL; @@ -252,7 +298,7 @@ static int rockchip_drm_unload(struct drm_device *drm_dev) drm_kms_helper_poll_fini(drm_dev); component_unbind_all(dev, drm_dev); if (is_support_iommu) - arm_iommu_detach_device(dev); + rockchip_iommu_cleanup(drm_dev); drm_mode_config_cleanup(drm_dev); drm_dev->dev_private = NULL;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index 56f43a3..1e2a666 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -70,6 +70,7 @@ struct rockchip_drm_private { const struct rockchip_crtc_funcs *crtc_funcs[ROCKCHIP_MAX_CRTC];
struct rockchip_atomic_commit commit; + struct iommu_domain *domain; };
void rockchip_drm_atomic_work(struct work_struct *work);
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Rockchip DRM used the arm special API, arm_iommu_*(), to attach iommu for ARM32 SoCs. This patch convert to common iommu API so it would support ARM64 like RK3399.
The general idea is domain_alloc(), attach_device() and arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++++++++++++++++++--------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + 2 files changed, 89 insertions(+), 42 deletions(-)
Please see my comments inline.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index f5a68fc..7965a66 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -14,8 +14,6 @@
- GNU General Public License for more details.
*/
-#include <asm/dma-iommu.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h> @@ -24,6 +22,8 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/component.h> +#include <linux/dma-iommu.h> +#include <linux/iommu.h>
#include "rockchip_drm_drv.h" #include "rockchip_drm_fb.h" @@ -46,7 +46,8 @@ static bool is_support_iommu = true; int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, struct device *dev) {
struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
struct rockchip_drm_private *private = drm_dev->dev_private;struct iommu_domain *domain = private->domain; int ret; if (!is_support_iommu)@@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
return arm_iommu_attach_device(dev, mapping);
ret = iommu_attach_device(domain, dev);
nit: Unnecessary blank line.
if (ret) {dev_err(dev, "Failed to attach iommu device\n");return ret;}
nit: On the other hand, a blank line here would improve readability.
arch_setup_dma_ops(dev, 0x00000000, SZ_2G,(struct iommu_ops *)dev->bus->iommu_ops, false);
This is casting a const pointer to a non-const pointer. which isn't really a good idea. I can see that arch_setup_dma_ops() requires a writable pointer, though. Looking at the implementations of arch_setup_dma_ops() around the platforms (namely arm and arm64...), it makes me wonder if the prototype shouldn't be changed to const instead.
return 0;}
void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, struct device *dev) {
if (!is_support_iommu)return;
struct rockchip_drm_private *private = drm_dev->dev_private;struct iommu_domain *domain = private->domain;
arm_iommu_detach_device(dev);
if (is_support_iommu)iommu_detach_device(domain, dev);}
int rockchip_register_crtc_funcs(struct drm_crtc *crtc, @@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev, priv->crtc_funcs[pipe]->disable_vblank(crtc); }
+static int rockchip_drm_init_iommu(struct drm_device *drm_dev) +{
struct rockchip_drm_private *private = drm_dev->dev_private;struct device *dev = drm_dev->dev;int ret;dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),GFP_KERNEL);if (!dev->dma_parms) {ret = -ENOMEM;return ret;
nit: return -ENOMEM;
}ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));if (ret) {dev_err(dev, "Failed to set coherent mask\n");return ret;}dma_set_max_seg_size(dev, DMA_BIT_MASK(32));private->domain = iommu_domain_alloc(&platform_bus_type);if (!private->domain)return -ENOMEM;ret = iommu_get_dma_cookie(private->domain);if (ret) {dev_err(dev, "Failed to get dma cookie\n");goto err_free_domain;}ret = iommu_dma_init_domain(private->domain, 0x00000000, SZ_2G);
I guess djkurtz's TODO comment could be preserved here.
Best regards, Tomasz
Hi,
On 2016年06月10日 16:03, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Rockchip DRM used the arm special API, arm_iommu_*(), to attach iommu for ARM32 SoCs. This patch convert to common iommu API so it would support ARM64 like RK3399.
The general idea is domain_alloc(), attach_device() and arch_setup_dma_ops() to set dma_ops manually for DRM at the last.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 130 +++++++++++++++++++--------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + 2 files changed, 89 insertions(+), 42 deletions(-)
Please see my comments inline.
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c index f5a68fc..7965a66 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c @@ -14,8 +14,6 @@
- GNU General Public License for more details.
*/
-#include <asm/dma-iommu.h>
- #include <drm/drmP.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_helper.h>
@@ -24,6 +22,8 @@ #include <linux/module.h> #include <linux/of_graph.h> #include <linux/component.h> +#include <linux/dma-iommu.h> +#include <linux/iommu.h>
#include "rockchip_drm_drv.h" #include "rockchip_drm_fb.h" @@ -46,7 +46,8 @@ static bool is_support_iommu = true; int rockchip_drm_dma_attach_device(struct drm_device *drm_dev, struct device *dev) {
struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
struct rockchip_drm_private *private = drm_dev->dev_private;struct iommu_domain *domain = private->domain; int ret; if (!is_support_iommu)@@ -58,16 +59,25 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
return arm_iommu_attach_device(dev, mapping);
ret = iommu_attach_device(domain, dev);nit: Unnecessary blank line.
Will fix it in v3.
if (ret) {dev_err(dev, "Failed to attach iommu device\n");return ret;}nit: On the other hand, a blank line here would improve readability.
Will fix it in v3.
arch_setup_dma_ops(dev, 0x00000000, SZ_2G,(struct iommu_ops *)dev->bus->iommu_ops, false);This is casting a const pointer to a non-const pointer. which isn't really a good idea. I can see that arch_setup_dma_ops() requires a writable pointer, though. Looking at the implementations of arch_setup_dma_ops() around the platforms (namely arm and arm64...), it makes me wonder if the prototype shouldn't be changed to const instead.
Actually, kernel-next changed iommu_ops to const by: 53c92d7 iommu: of: enforce const-ness of struct iommu_ops
Will remove casting in the v3.
return 0;}
void rockchip_drm_dma_detach_device(struct drm_device *drm_dev, struct device *dev) {
if (!is_support_iommu)return;
struct rockchip_drm_private *private = drm_dev->dev_private;struct iommu_domain *domain = private->domain;
arm_iommu_detach_device(dev);
if (is_support_iommu)iommu_detach_device(domain, dev);}
int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
@@ -132,10 +142,70 @@ static void rockchip_drm_crtc_disable_vblank(struct drm_device *dev, priv->crtc_funcs[pipe]->disable_vblank(crtc); }
+static int rockchip_drm_init_iommu(struct drm_device *drm_dev) +{
struct rockchip_drm_private *private = drm_dev->dev_private;struct device *dev = drm_dev->dev;int ret;dev->dma_parms = devm_kzalloc(dev, sizeof(*dev->dma_parms),GFP_KERNEL);if (!dev->dma_parms) {ret = -ENOMEM;return ret;nit: return -ENOMEM;
Will fix it in v3.
}ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));if (ret) {dev_err(dev, "Failed to set coherent mask\n");return ret;}dma_set_max_seg_size(dev, DMA_BIT_MASK(32));private->domain = iommu_domain_alloc(&platform_bus_type);if (!private->domain)return -ENOMEM;ret = iommu_get_dma_cookie(private->domain);if (ret) {dev_err(dev, "Failed to get dma cookie\n");goto err_free_domain;}ret = iommu_dma_init_domain(private->domain, 0x00000000, SZ_2G);I guess djkurtz's TODO comment could be preserved here.
Agree.
Thank you very much. Shunqian
Best regards, Tomasz
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++--------------- 1 file changed, 71 insertions(+), 42 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index d6c3051..aafea6e 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -4,8 +4,6 @@ * published by the Free Software Foundation. */
-#include <asm/cacheflush.h> -#include <asm/pgtable.h> #include <linux/compiler.h> #include <linux/delay.h> #include <linux/device.h> @@ -61,8 +59,7 @@ #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
-#define NUM_DT_ENTRIES 1024 -#define NUM_PT_ENTRIES 1024 +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
#define SPAGE_ORDER 12 #define SPAGE_SIZE (1 << SPAGE_ORDER) @@ -82,7 +79,9 @@
struct rk_iommu_domain { struct list_head iommus; + struct device *dev; u32 *dt; /* page directory table */ + dma_addr_t dt_dma; spinlock_t iommus_lock; /* lock for iommus list */ spinlock_t dt_lock; /* lock for modifying page directory table */
@@ -98,14 +97,12 @@ struct rk_iommu { struct iommu_domain *domain; /* domain to which iommu is attached */ };
-static inline void rk_table_flush(u32 *va, unsigned int count) +static inline void rk_table_flush(struct device *dev, dma_addr_t dma, + unsigned int count) { - phys_addr_t pa_start = virt_to_phys(va); - phys_addr_t pa_end = virt_to_phys(va + count); - size_t size = pa_end - pa_start; + size_t size = count * 4;
- __cpuc_flush_dcache_area(va, size); - outer_flush_range(pa_start, pa_end); + dma_sync_single_range_for_device(dev, dma, 0, size, DMA_TO_DEVICE); }
static struct rk_iommu_domain *to_rk_domain(struct iommu_domain *dom) @@ -188,10 +185,9 @@ static inline bool rk_dte_is_pt_valid(u32 dte) return dte & RK_DTE_PT_VALID; }
-static u32 rk_mk_dte(u32 *pt) +static inline u32 rk_mk_dte(dma_addr_t pt_dma) { - phys_addr_t pt_phys = virt_to_phys(pt); - return (pt_phys & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID; + return (pt_dma & RK_DTE_PT_ADDRESS_MASK) | RK_DTE_PT_VALID; }
/* @@ -609,12 +605,14 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain, dma_addr_t iova) { u32 *page_table, *dte_addr; + u32 dte_index = rk_iova_dte_index(iova); u32 dte; phys_addr_t pt_phys; + dma_addr_t pt_dma;
assert_spin_locked(&rk_domain->dt_lock);
- dte_addr = &rk_domain->dt[rk_iova_dte_index(iova)]; + dte_addr = &rk_domain->dt[dte_index]; dte = *dte_addr; if (rk_dte_is_pt_valid(dte)) goto done; @@ -623,19 +621,27 @@ static u32 *rk_dte_get_page_table(struct rk_iommu_domain *rk_domain, if (!page_table) return ERR_PTR(-ENOMEM);
- dte = rk_mk_dte(page_table); - *dte_addr = dte; + pt_dma = dma_map_single(rk_domain->dev, page_table, + SPAGE_SIZE, DMA_TO_DEVICE); + if (dma_mapping_error(rk_domain->dev, pt_dma)) { + dev_err(rk_domain->dev, "dma mapping error\n"); + free_page((unsigned long)page_table); + return ERR_PTR(-ENOMEM); + }
- rk_table_flush(page_table, NUM_PT_ENTRIES); - rk_table_flush(dte_addr, 1); + dte = rk_mk_dte(pt_dma); + *dte_addr = dte;
+ rk_table_flush(rk_domain->dev, pt_dma, NUM_TLB_ENTRIES); + rk_table_flush(rk_domain->dev, rk_domain->dt_dma + dte_index * 4, 1); done: pt_phys = rk_dte_pt_address(dte); return (u32 *)phys_to_virt(pt_phys); }
static size_t rk_iommu_unmap_iova(struct rk_iommu_domain *rk_domain, - u32 *pte_addr, dma_addr_t iova, size_t size) + u32 *pte_addr, dma_addr_t pte_dma, + size_t size) { unsigned int pte_count; unsigned int pte_total = size / SPAGE_SIZE; @@ -650,14 +656,14 @@ static size_t rk_iommu_unmap_iova(struct rk_iommu_domain *rk_domain, pte_addr[pte_count] = rk_mk_pte_invalid(pte); }
- rk_table_flush(pte_addr, pte_count); + rk_table_flush(rk_domain->dev, pte_dma, pte_count);
return pte_count * SPAGE_SIZE; }
static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, - dma_addr_t iova, phys_addr_t paddr, size_t size, - int prot) + dma_addr_t pte_dma, dma_addr_t iova, + phys_addr_t paddr, size_t size, int prot) { unsigned int pte_count; unsigned int pte_total = size / SPAGE_SIZE; @@ -676,7 +682,7 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, paddr += SPAGE_SIZE; }
- rk_table_flush(pte_addr, pte_count); + rk_table_flush(rk_domain->dev, pte_dma, pte_total);
/* * Zap the first and last iova to evict from iotlb any previously @@ -689,7 +695,8 @@ static int rk_iommu_map_iova(struct rk_iommu_domain *rk_domain, u32 *pte_addr, return 0; unwind: /* Unmap the range of iovas that we just mapped */ - rk_iommu_unmap_iova(rk_domain, pte_addr, iova, pte_count * SPAGE_SIZE); + rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, + pte_count * SPAGE_SIZE);
iova += pte_count * SPAGE_SIZE; page_phys = rk_pte_page_address(pte_addr[pte_count]); @@ -704,8 +711,9 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova, { struct rk_iommu_domain *rk_domain = to_rk_domain(domain); unsigned long flags; - dma_addr_t iova = (dma_addr_t)_iova; + dma_addr_t pte_dma, iova = (dma_addr_t)_iova; u32 *page_table, *pte_addr; + u32 dte_index, pte_index; int ret;
spin_lock_irqsave(&rk_domain->dt_lock, flags); @@ -723,8 +731,13 @@ static int rk_iommu_map(struct iommu_domain *domain, unsigned long _iova, return PTR_ERR(page_table); }
- pte_addr = &page_table[rk_iova_pte_index(iova)]; - ret = rk_iommu_map_iova(rk_domain, pte_addr, iova, paddr, size, prot); + dte_index = rk_domain->dt[rk_iova_dte_index(iova)]; + pte_index = rk_iova_pte_index(iova); + pte_addr = &page_table[pte_index]; + pte_dma = rk_dte_pt_address(dte_index) + pte_index * 4; + ret = rk_iommu_map_iova(rk_domain, pte_addr, pte_dma, iova, + paddr, size, prot); + spin_unlock_irqrestore(&rk_domain->dt_lock, flags);
return ret; @@ -735,7 +748,7 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova, { struct rk_iommu_domain *rk_domain = to_rk_domain(domain); unsigned long flags; - dma_addr_t iova = (dma_addr_t)_iova; + dma_addr_t pte_dma, iova = (dma_addr_t)_iova; phys_addr_t pt_phys; u32 dte; u32 *pte_addr; @@ -759,7 +772,8 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
pt_phys = rk_dte_pt_address(dte); pte_addr = (u32 *)phys_to_virt(pt_phys) + rk_iova_pte_index(iova); - unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, iova, size); + pte_dma = pt_phys + rk_iova_pte_index(iova) * 4; + unmap_size = rk_iommu_unmap_iova(rk_domain, pte_addr, pte_dma, size);
spin_unlock_irqrestore(&rk_domain->dt_lock, flags);
@@ -776,8 +790,6 @@ static struct rk_iommu *rk_iommu_from_dev(struct device *dev) struct rk_iommu *rk_iommu;
group = iommu_group_get(dev); - if (!group) - return NULL; iommu_dev = iommu_group_get_iommudata(group); rk_iommu = dev_get_drvdata(iommu_dev); iommu_group_put(group); @@ -792,9 +804,21 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, struct rk_iommu_domain *rk_domain = to_rk_domain(domain); unsigned long flags; int ret, i; - phys_addr_t dte_addr;
iommu = rk_iommu_from_dev(dev); + /* Set the domain dev to virtual one if exist */ + if (RK_IOMMU_IS_VIRTUAL(iommu) || !rk_domain->dev) + rk_domain->dev = iommu->dev; + + if (!rk_domain->dt_dma) { + rk_domain->dt_dma = dma_map_single(rk_domain->dev, rk_domain->dt, + SPAGE_SIZE, DMA_TO_DEVICE); + if (dma_mapping_error(rk_domain->dev, rk_domain->dt_dma)) + return -ENOMEM; + + rk_table_flush(rk_domain->dev, rk_domain->dt_dma, + NUM_TLB_ENTRIES); + }
iommu->domain = domain; if (RK_IOMMU_IS_VIRTUAL(iommu)) { @@ -804,28 +828,27 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
ret = rk_iommu_enable_stall(iommu); if (ret) - return ret; + goto unmap;
ret = rk_iommu_force_reset(iommu); if (ret) - return ret; + goto unmap;
ret = devm_request_irq(iommu->dev, iommu->irq, rk_iommu_irq, IRQF_SHARED, dev_name(dev), iommu); if (ret) - return ret; + goto unmap;
- dte_addr = virt_to_phys(rk_domain->dt); for (i = 0; i < iommu->num_mmu; i++) { - rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, dte_addr); + rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, rk_domain->dt_dma); rk_iommu_command(iommu->bases[i], RK_MMU_CMD_ZAP_CACHE); rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, RK_MMU_IRQ_MASK); }
ret = rk_iommu_enable_paging(iommu); if (ret) - return ret; + goto unmap;
spin_lock_irqsave(&rk_domain->iommus_lock, flags); list_add_tail(&iommu->node, &rk_domain->iommus); @@ -836,6 +859,10 @@ static int rk_iommu_attach_device(struct iommu_domain *domain, rk_iommu_disable_stall(iommu);
return 0; +unmap: + dma_unmap_single(rk_domain->dev, rk_domain->dt_dma, SPAGE_SIZE, + DMA_TO_DEVICE); + return ret; }
static void rk_iommu_detach_device(struct iommu_domain *domain, @@ -846,8 +873,10 @@ static void rk_iommu_detach_device(struct iommu_domain *domain, unsigned long flags; int i;
- iommu = rk_iommu_from_dev(dev); + rk_domain->dev = NULL;
+ /* Allow 'virtual devices' (eg drm) to detach from domain */ + iommu = rk_iommu_from_dev(dev); iommu->domain = NULL; if (RK_IOMMU_IS_VIRTUAL(iommu)) { dev_dbg(dev, "Master with virtual iommu detached from domain\n"); @@ -883,6 +912,8 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) if (!rk_domain) return NULL;
+ rk_domain->dev = NULL; + rk_domain->dt_dma = 0; /* * rk32xx iommus use a 2 level pagetable. * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries. @@ -892,8 +923,6 @@ static struct iommu_domain *rk_iommu_domain_alloc(unsigned type) if (!rk_domain->dt) goto err_dt;
- rk_table_flush(rk_domain->dt, NUM_DT_ENTRIES); - spin_lock_init(&rk_domain->iommus_lock); spin_lock_init(&rk_domain->dt_lock); INIT_LIST_HEAD(&rk_domain->iommus); @@ -912,7 +941,7 @@ static void rk_iommu_domain_free(struct iommu_domain *domain)
WARN_ON(!list_empty(&rk_domain->iommus));
- for (i = 0; i < NUM_DT_ENTRIES; i++) { + for (i = 0; i < NUM_TLB_ENTRIES; i++) { u32 dte = rk_domain->dt[i]; if (rk_dte_is_pt_valid(dte)) { phys_addr_t pt_phys = rk_dte_pt_address(dte);
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
Other than that, please see some comments inline.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++--------------- 1 file changed, 71 insertions(+), 42 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index d6c3051..aafea6e 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -4,8 +4,6 @@
- published by the Free Software Foundation.
*/
-#include <asm/cacheflush.h> -#include <asm/pgtable.h> #include <linux/compiler.h> #include <linux/delay.h> #include <linux/device.h> @@ -61,8 +59,7 @@ #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
-#define NUM_DT_ENTRIES 1024 -#define NUM_PT_ENTRIES 1024 +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
Is it necessary to change this in this patch? In general, it's not a good idea to mix multiple logical changes together.
#define SPAGE_ORDER 12 #define SPAGE_SIZE (1 << SPAGE_ORDER) @@ -82,7 +79,9 @@
struct rk_iommu_domain { struct list_head iommus;
struct device *dev; u32 *dt; /* page directory table */dma_addr_t dt_dma; spinlock_t iommus_lock; /* lock for iommus list */ spinlock_t dt_lock; /* lock for modifying page directory table */@@ -98,14 +97,12 @@ struct rk_iommu { struct iommu_domain *domain; /* domain to which iommu is attached */ };
-static inline void rk_table_flush(u32 *va, unsigned int count) +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
unsigned int count){
phys_addr_t pa_start = virt_to_phys(va);phys_addr_t pa_end = virt_to_phys(va + count);size_t size = pa_end - pa_start;
size_t size = count * 4;
It would be a good idea to specify what "count" is. I'm a bit confused that before it meant bytes and now some multiple of 4?
Best regards, Tomasz
Hi
On 2016年06月10日 17:10, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
I try to use dma_alloc_coherent() to replace the dma_map_single(), but it doesn't work for me properly. Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after attaching to iommu, so when the iommu domain need to alloc a new page in rk_iommu_map(), it would call: rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> iommu_map() --> rk_iommu_map()
Then I try to reserve memory for coherent so that, dma_alloc_coherent() calls dma_alloc_from_coherent() but not ops->alloc(). But it doesn't work too because when DRM request buffer it never uses iommu.
Other than that, please see some comments inline.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++--------------- 1 file changed, 71 insertions(+), 42 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index d6c3051..aafea6e 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -4,8 +4,6 @@
- published by the Free Software Foundation.
*/
-#include <asm/cacheflush.h> -#include <asm/pgtable.h> #include <linux/compiler.h> #include <linux/delay.h> #include <linux/device.h> @@ -61,8 +59,7 @@ #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
-#define NUM_DT_ENTRIES 1024 -#define NUM_PT_ENTRIES 1024 +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
Is it necessary to change this in this patch? In general, it's not a good idea to mix multiple logical changes together.
Sure, will restore changes in v3.
#define SPAGE_ORDER 12 #define SPAGE_SIZE (1 << SPAGE_ORDER) @@ -82,7 +79,9 @@
struct rk_iommu_domain { struct list_head iommus;
struct device *dev; u32 *dt; /* page directory table */dma_addr_t dt_dma; spinlock_t iommus_lock; /* lock for iommus list */ spinlock_t dt_lock; /* lock for modifying page directory table */@@ -98,14 +97,12 @@ struct rk_iommu { struct iommu_domain *domain; /* domain to which iommu is attached */ };
-static inline void rk_table_flush(u32 *va, unsigned int count) +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
unsigned int count)
phys_addr_t pa_start = virt_to_phys(va);phys_addr_t pa_end = virt_to_phys(va + count);size_t size = pa_end - pa_start;
size_t size = count * 4;It would be a good idea to specify what "count" is. I'm a bit confused that before it meant bytes and now some multiple of 4?
"count" means PT/DT entry count to flush here. I would add some more comment on it.
Thank you very much, Shunqian
Best regards, Tomasz
Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng shunqian.zheng@gmail.com wrote:
Hi
On 2016年06月10日 17:10, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
I try to use dma_alloc_coherent() to replace the dma_map_single(), but it doesn't work for me properly. Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after attaching to iommu, so when the iommu domain need to alloc a new page in rk_iommu_map(), it would call: rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> iommu_map() --> rk_iommu_map()
It shouldn't call iommu_map(), because the IOMMU is not behind another IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the IOMMU struct device and not the DRM device?
Best regards, Tomasz
HI,
On 2016年06月13日 18:21, Tomasz Figa wrote:
On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng shunqian.zheng@gmail.com wrote:
Hi
On 2016年06月10日 17:10, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
I try to use dma_alloc_coherent() to replace the dma_map_single(), but it doesn't work for me properly. Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after attaching to iommu, so when the iommu domain need to alloc a new page in rk_iommu_map(), it would call: rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> iommu_map() --> rk_iommu_map()
It shouldn't call iommu_map(), because the IOMMU is not behind another IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the IOMMU struct device and not the DRM device?
I called dma_alloc_coherent() with DRM device but not the IOMMU device, because DRM didn't attach to any iommu. Even allocating an virtual one when attaching, the iommu->dev is DRM device though. Am I right here?
Thank you very much, Shunqian
Best regards, Tomasz
On Mon, Jun 13, 2016 at 7:31 PM, Shunqian Zheng shunqian.zheng@gmail.com wrote:
HI,
On 2016年06月13日 18:21, Tomasz Figa wrote:
On Mon, Jun 13, 2016 at 6:56 PM, Shunqian Zheng shunqian.zheng@gmail.com wrote:
Hi
On 2016年06月10日 17:10, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
I try to use dma_alloc_coherent() to replace the dma_map_single(), but it doesn't work for me properly. Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after attaching to iommu, so when the iommu domain need to alloc a new page in rk_iommu_map(), it would call: rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> iommu_map() --> rk_iommu_map()
It shouldn't call iommu_map(), because the IOMMU is not behind another IOMMU. Are you sure you called dma_alloc_coherent() on behalf of the IOMMU struct device and not the DRM device?
I called dma_alloc_coherent() with DRM device but not the IOMMU device, because DRM didn't attach to any iommu. Even allocating an virtual one when attaching, the iommu->dev is DRM device though. Am I right here?
What I meant, is that even though rk_iommu_map() is called for DRM device, the page table allocation happening inside should be called for the IOMMU device itself, because it's the consumer of these page tables.
Best regards, Tomasz
On 13/06/16 10:56, Shunqian Zheng wrote:
Hi
On 2016年06月10日 17:10, Tomasz Figa wrote:
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
Use DMA API instead of architecture internal functions like __cpuc_flush_dcache_area() etc.
To support the virtual device like DRM the virtual slave iommu added in the previous patch, attaching to which the DRM can use it own domain->dev for dma_map_*(), dma_sync_*() even VOP is disabled.
With this patch, this driver is available for ARM64 like RK3399.
Could we instead simply allocate coherent memory for page tables using dma_alloc_coherent() and skip any flushing on CPU side completely? If I'm looking correctly, the driver only reads back the page directory when checking if there is a need to allocate new page table, so there shouldn't be any significant penalty for disabling the cache.
I try to use dma_alloc_coherent() to replace the dma_map_single(), but it doesn't work for me properly. Because the DRM uses the iommu_dma_ops instead the swiotlb_dma_ops after attaching to iommu, so when the iommu domain need to alloc a new page in rk_iommu_map(), it would call: rk_iommu_map() --> dma_alloc_coherent() --> ops->alloc() --> iommu_map() --> rk_iommu_map()
That sounds more like you're passing the wrong device around somewhere, since this approach is working fine for other IOMMUs; specifically, the flow goes:
dma_alloc_coherent(DRM dev) // for buffer --> ops->alloc(DRM dev) --> iommu_dma_alloc(DRM dev) --> iommu_map() --> dma_alloc_coherent(IOMMU dev) // for pagetable --> ops->alloc(IOMMU dev) --> swiotlb_alloc(IOMMU dev)
There shouldn't be any need for this "virtual IOMMU" at all. I think the Exynos DRM driver is in a similar situation of having multiple devices (involving multiple IOMMUs) backing the virtual DRM device, and that doesn't seem to be doing anything this crazy so it's probably worth taking a look at.
Robin.
Then I try to reserve memory for coherent so that, dma_alloc_coherent() calls dma_alloc_from_coherent() but not ops->alloc(). But it doesn't work too because when DRM request buffer it never uses iommu.
Other than that, please see some comments inline.
Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/rockchip-iommu.c | 113 ++++++++++++++++++++++++++--------------- 1 file changed, 71 insertions(+), 42 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c index d6c3051..aafea6e 100644 --- a/drivers/iommu/rockchip-iommu.c +++ b/drivers/iommu/rockchip-iommu.c @@ -4,8 +4,6 @@
- published by the Free Software Foundation.
*/
-#include <asm/cacheflush.h> -#include <asm/pgtable.h> #include <linux/compiler.h> #include <linux/delay.h> #include <linux/device.h> @@ -61,8 +59,7 @@ #define RK_MMU_IRQ_BUS_ERROR 0x02 /* bus read error */ #define RK_MMU_IRQ_MASK (RK_MMU_IRQ_PAGE_FAULT | RK_MMU_IRQ_BUS_ERROR)
-#define NUM_DT_ENTRIES 1024 -#define NUM_PT_ENTRIES 1024 +#define NUM_TLB_ENTRIES 1024 /* for both DT and PT */
Is it necessary to change this in this patch? In general, it's not a good idea to mix multiple logical changes together.
Sure, will restore changes in v3.
#define SPAGE_ORDER 12 #define SPAGE_SIZE (1 << SPAGE_ORDER) @@ -82,7 +79,9 @@
struct rk_iommu_domain { struct list_head iommus;
struct device *dev; u32 *dt; /* page directory table */dma_addr_t dt_dma; spinlock_t iommus_lock; /* lock for iommus list */ spinlock_t dt_lock; /* lock for modifying page directorytable */
@@ -98,14 +97,12 @@ struct rk_iommu { struct iommu_domain *domain; /* domain to which iommu is attached */ };
-static inline void rk_table_flush(u32 *va, unsigned int count) +static inline void rk_table_flush(struct device *dev, dma_addr_t dma,
unsigned int count)
phys_addr_t pa_start = virt_to_phys(va);phys_addr_t pa_end = virt_to_phys(va + count);size_t size = pa_end - pa_start;
size_t size = count * 4;It would be a good idea to specify what "count" is. I'm a bit confused that before it meant bytes and now some multiple of 4?
"count" means PT/DT entry count to flush here. I would add some more comment on it.
Thank you very much, Shunqian
Best regards, Tomasz
Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
From: Simon Xue xxm@rock-chips.com
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com --- drivers/iommu/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index ad08603..5572621 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -218,7 +218,7 @@ config OMAP_IOMMU_DEBUG
config ROCKCHIP_IOMMU bool "Rockchip IOMMU Support" - depends on ARM + depends on ARM || ARM64 depends on ARCH_ROCKCHIP || COMPILE_TEST select IOMMU_API select ARM_DMA_USE_IOMMU
Hi,
On Wed, Jun 8, 2016 at 10:26 PM, Shunqian Zheng zhengsq@rock-chips.com wrote:
From: Simon Xue xxm@rock-chips.com
It is usual a good practice to include at least a single sentence here, even if the patch is trivial. In this case it could say
"This patch makes it possible to compile the rockchip-iommu driver on ARM64 platform to be used with 64-bit SoCs equipped with this type of IOMMU."
Signed-off-by: Simon Xue xxm@rock-chips.com Signed-off-by: Shunqian Zheng zhengsq@rock-chips.com
drivers/iommu/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Assuming that the above is fixed:
Reviewed-by: Tomasz Figa tfiga@chromium.org
Best regards, Tomasz
dri-devel@lists.freedesktop.org
-
Robin Murphy -
Shunqian Zheng -
Shunqian Zheng -
Tomasz Figa -
Tomasz Figa