Skip to contentSkip to conversation

Samsung IOMMU videobuf2 allocator and s5p-fimc update

PATCH Superseded Marek Szyprowski 4 participants 32 messages Apr 18, 2011 – Apr 26, 2011

Subsystems (5)
lore
Share & tools
lore

Marek Szyprowski

author maintainer
#0 lore raw

v3 cover Samsung IOMMU videobuf2 allocator and s5p-fimc update

Hello,

This is a third version of the Samsung IOMMU driver (see patch #2) and videobuf2 allocator for IOMMU mapped memory (see patch #4) as well as FIMC driver update. This update brings some minor bugfixes to Samsung IOMMU (SYSMMU) driver and support for pages larger than 4KiB in videobuf2-dma-iommu allocator.

The main change from the first version of the Samsung IOMMU patches is a complete rewrite of the IOMMU driver API. As suggested by Arnd Bergmann we decided to drop the custom interface and use the kernel wide, common iommu API defined in linux/include/iommu.h. This way the videobuf2 iommu allocator become much more generic framework and it is no longer tied to any particular iommu implementation.

This patch series introduces new type of videbuf2 memory allocator - vb2-dma-iommu. This allocator can be used on the platforms that support linux/include/iommu.h style IOMMU driver. An IOMMU driver for Samsung EXYNOS4 (called SYSMMU) platform is also included. The allocator and IOMMU driver is then used by s5p-fimc driver. To make it possible some additional changes are required. Mainly the platform support for s5p-fimc for EXYNOS4 machines need to be defined. The proposed solution has been tested on Universal C210 board (Samsung S5PC210/EXYNOS4 based). This IOMMU allocator has no dependences on any external subsystems.

We also ported s5p-mfc and s5p-tv drivers to this allocator, they will be posted in separate patch series. This will enable to get them working on mainline Linux kernel for EXYNOS4 platform. Support for S5PV210/S5PC110 platform still depends on CMA allocator that needs more discussion on memory management mailing list and further development. The patches with updated s5p-mfc and s5p-tv drivers will follow.

To get FIMC module working on EXYNOS4 platform on UniversalC210 board we also added support for power domains and power gating.

This patch series contains a collection of patches for various platform subsystems. Here is a detailed list:

[PATCH 1/7] ARM: EXYNOS4: power domains: fixes and code cleanup
- adds support for block gating in Samsung power domain driver and performs some cleanup

[PATCH 2/7] ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver
- a complete rewrite of sysmmu driver for Samsung platform, now uses linux/include/iommu.h api (key patch in this series)

[PATCH 3/7] v4l: videobuf2: dma-sg: move some generic functions to memops
- a little cleanup and preparations for the dma-iommu allocator

[PATCH 4/7] v4l: videobuf2: add IOMMU based DMA memory allocator
- introduces new memory allocator for videobuf2 for drivers that support iommu dma memory mappings (key patch in this series)

[PATCH 5/7] v4l: s5p-fimc: add pm_runtime support
- adds support for pm_runtime in s5p-fimc driver

[PATCH 6/7] v4l: s5p-fimc: Add support for vb2-dma-iommu allocator
- adds support for the newly introduces videbuf2-s5p-iommu allocator on EXYNOS4 platform

[PATCH 7/7] ARM: EXYNOS4: enable FIMC on Universal_C210
- adds all required machine definitions to get FIMC modules working on Universal C210 boards

Changelog:

V3:
- minor bugfixes in SYSMMU driver
- added complete support for 64KiB and 1MiB pages to videobuf2-dma-iommu allocator

V2: http://68.183.106.108/lists/arm-kernel/msg120636.html
- custom SYSMMU interface has been dropped in favour of linux/include/iommu.h and rewritten SYSMMU driver again
- added support to SYSMMU for mapping pages larger than 4Kb
- dropped ARM shared mode
- videobuf2-s5p-iommu allocator has been renamed to videobuf2-dma-iommu, because it has no dependenco on any Samsung platform specific API, the allocator still uses only 4Kb pages, but this will be changed in the next version
- dropped FIMC platform patch that have been merged mainline
- rebased all patches onto Linux kernel v2.6.39-rc1

V1: http://www.spinics.net/lists/linux-media/msg29751.html

Best regards

Signature · 55 lines
Marek Szyprowski
Samsung Poland R&D Center



Complete patch summary:

Andrzej Pietrasiewicz (3):
  ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver
  v4l: videobuf2: dma-sg: move some generic functions to memops
  v4l: videobuf2: add IOMMU based DMA memory allocator

Marek Szyprowski (3):
  v4l: s5p-fimc: add pm_runtime support
  v4l: s5p-fimc: Add support for vb2-dma-iommu allocator
  ARM: EXYNOS4: enable FIMC on Universal_C210

Tomasz Stanislawski (1):
  ARM: EXYNOS4: power domains: fixes and code cleanup

 arch/arm/mach-exynos4/Kconfig                   |    6 +
 arch/arm/mach-exynos4/clock.c                   |   68 +-
 arch/arm/mach-exynos4/dev-pd.c                  |   93 ++-
 arch/arm/mach-exynos4/dev-sysmmu.c              |  615 ++++++++-----
 arch/arm/mach-exynos4/include/mach/irqs.h       |   35 +-
 arch/arm/mach-exynos4/include/mach/regs-clock.h |    7 +
 arch/arm/mach-exynos4/include/mach/sysmmu.h     |   46 -
 arch/arm/mach-exynos4/mach-universal_c210.c     |   22 +
 arch/arm/plat-s5p/Kconfig                       |   20 +-
 arch/arm/plat-s5p/include/plat/sysmmu.h         |  241 +++--
 arch/arm/plat-s5p/sysmmu.c                      | 1191 +++++++++++++++++------
 arch/arm/plat-samsung/include/plat/devs.h       |    2 +-
 arch/arm/plat-samsung/include/plat/pd.h         |    1 +
 drivers/media/video/Kconfig                     |   11 +-
 drivers/media/video/Makefile                    |    1 +
 drivers/media/video/s5p-fimc/fimc-capture.c     |    9 +-
 drivers/media/video/s5p-fimc/fimc-core.c        |   38 +-
 drivers/media/video/s5p-fimc/fimc-core.h        |    1 +
 drivers/media/video/s5p-fimc/fimc-mem.h         |  104 ++
 drivers/media/video/videobuf2-dma-iommu.c       |  762 +++++++++++++++
 drivers/media/video/videobuf2-dma-sg.c          |   37 +-
 drivers/media/video/videobuf2-memops.c          |   76 ++
 include/media/videobuf2-dma-iommu.h             |   48 +
 include/media/videobuf2-memops.h                |    5 +
 24 files changed, 2638 insertions(+), 801 deletions(-)
 rewrite arch/arm/mach-exynos4/dev-sysmmu.c (88%)
 delete mode 100644 arch/arm/mach-exynos4/include/mach/sysmmu.h
 rewrite arch/arm/plat-s5p/include/plat/sysmmu.h (83%)
 rewrite arch/arm/plat-s5p/sysmmu.c (87%)
 create mode 100644 drivers/media/video/s5p-fimc/fimc-mem.h
 create mode 100644 drivers/media/video/videobuf2-dma-iommu.c
 create mode 100644 include/media/videobuf2-dma-iommu.h

-- 
1.7.1.569.g6f426
author maintainer
#1 lore raw

PATCH v3 1/7 ARM: EXYNOS4: power domains: fixes and code cleanup

From: Tomasz Stanislawski <[email protected]>

This patch extends power domain driver with support for enabling and disabling modules in S5P_CLKGATE_BLOCK register. It also performs a little code cleanup to avoid confusion between exynos4_device_pd array index and power domain id.

---
 arch/arm/mach-exynos4/dev-pd.c                  |   93 +++++++++++++++++------
 arch/arm/mach-exynos4/include/mach/regs-clock.h |    7 ++
 arch/arm/plat-samsung/include/plat/pd.h         |    1 +
 3 files changed, 79 insertions(+), 22 deletions(-)
arch/arm/mach-exynos4/dev-pd.c +71 -22
index 3273f25..44c6597 100644
@@ -16,13 +16,17 @@
 #include <linux/delay.h>
 
 #include <mach/regs-pmu.h>
+#include <mach/regs-clock.h>
 
 #include <plat/pd.h>
 
+static DEFINE_SPINLOCK(gate_block_slock);
+
 static int exynos4_pd_enable(struct device *dev)
 {
 	struct samsung_pd_info *pdata =  dev->platform_data;
 	u32 timeout;
+	int ret = 0;
 
 	__raw_writel(S5P_INT_LOCAL_PWR_EN, pdata->base);
 
@@ -31,21 +35,39 @@ static int exynos4_pd_enable(struct device *dev)
 	while ((__raw_readl(pdata->base + 0x4) & S5P_INT_LOCAL_PWR_EN)
 		!= S5P_INT_LOCAL_PWR_EN) {
 		if (timeout == 0) {
-			printk(KERN_ERR "Power domain %s enable failed.\n",
-				dev_name(dev));
-			return -ETIMEDOUT;
+			dev_err(dev, "enable failed\n");
+			ret = -ETIMEDOUT;
+			goto done;
 		}
 		timeout--;
 		udelay(100);
 	}
 
-	return 0;
+	/* configure clk gate mask if it is present */
+	if (pdata->gate_mask) {
+		unsigned long flags;
+		unsigned long value;
+
+		spin_lock_irqsave(&gate_block_slock, flags);
+
+		value  = __raw_readl(S5P_CLKGATE_BLOCK);
+		value |= pdata->gate_mask;
+		__raw_writel(value, S5P_CLKGATE_BLOCK);
+
+		spin_unlock_irqrestore(&gate_block_slock, flags);
+	}
+
+done:
+	dev_info(dev, "enable finished\n");
+
+	return ret;
 }
 
 static int exynos4_pd_disable(struct device *dev)
 {
 	struct samsung_pd_info *pdata =  dev->platform_data;
 	u32 timeout;
+	int ret = 0;
 
 	__raw_writel(0, pdata->base);
 
@@ -53,81 +75,108 @@ static int exynos4_pd_disable(struct device *dev)
 	timeout = 10;
 	while (__raw_readl(pdata->base + 0x4) & S5P_INT_LOCAL_PWR_EN) {
 		if (timeout == 0) {
-			printk(KERN_ERR "Power domain %s disable failed.\n",
-				dev_name(dev));
-			return -ETIMEDOUT;
+			dev_err(dev, "disable failed\n");
+			ret = -ETIMEDOUT;
+			goto done;
 		}
 		timeout--;
 		udelay(100);
 	}
 
-	return 0;
+	if (pdata->gate_mask) {
+		unsigned long flags;
+		unsigned long value;
+
+		spin_lock_irqsave(&gate_block_slock, flags);
+
+		value  = __raw_readl(S5P_CLKGATE_BLOCK);
+		value &= ~pdata->gate_mask;
+		__raw_writel(value, S5P_CLKGATE_BLOCK);
+
+		spin_unlock_irqrestore(&gate_block_slock, flags);
+	}
+done:
+	dev_info(dev, "disable finished\n");
+
+	return ret;
 }
 
 struct platform_device exynos4_device_pd[] = {
-	{
+	[PD_MFC] = {
 		.name		= "samsung-pd",
-		.id		= 0,
+		.id		= PD_MFC,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_MFC_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_MFC,
 			},
 		},
-	}, {
+	},
+	[PD_G3D] = {
 		.name		= "samsung-pd",
-		.id		= 1,
+		.id		= PD_G3D,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_G3D_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_G3D,
 			},
 		},
-	}, {
+	},
+	[PD_LCD0] = {
 		.name		= "samsung-pd",
-		.id		= 2,
+		.id		= PD_LCD0,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_LCD0_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_LCD0,
 			},
 		},
-	}, {
+	},
+	[PD_LCD1] = {
 		.name		= "samsung-pd",
-		.id		= 3,
+		.id		= PD_LCD1,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_LCD1_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_LCD1,
 			},
 		},
-	}, {
+	},
+	[PD_TV] = {
 		.name		= "samsung-pd",
-		.id		= 4,
+		.id		= PD_TV,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_TV_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_TV,
 			},
 		},
-	}, {
+	},
+	[PD_CAM] = {
 		.name		= "samsung-pd",
-		.id		= 5,
+		.id		= PD_CAM,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
 				.disable	= exynos4_pd_disable,
 				.base		= S5P_PMU_CAM_CONF,
+				.gate_mask	= S5P_CLKGATE_BLOCK_CAM,
 			},
 		},
-	}, {
+	},
+	[PD_GPS] = {
 		.name		= "samsung-pd",
-		.id		= 6,
+		.id		= PD_GPS,
 		.dev = {
 			.platform_data = &(struct samsung_pd_info) {
 				.enable		= exynos4_pd_enable,
arch/arm/mach-exynos4/include/mach/regs-clock.h +7 -0
index 6e311c1..2c1472b 100644
@@ -171,6 +171,13 @@
 #define S5P_CLKDIV_BUS_GPLR_SHIFT	(4)
 #define S5P_CLKDIV_BUS_GPLR_MASK	(0x7 << S5P_CLKDIV_BUS_GPLR_SHIFT)
 
+#define S5P_CLKGATE_BLOCK_CAM		(1 << 0)
+#define S5P_CLKGATE_BLOCK_TV		(1 << 1)
+#define S5P_CLKGATE_BLOCK_MFC		(1 << 2)
+#define S5P_CLKGATE_BLOCK_G3D		(1 << 3)
+#define S5P_CLKGATE_BLOCK_LCD0		(1 << 4)
+#define S5P_CLKGATE_BLOCK_LCD1		(1 << 5)
+
 /* Compatibility defines and inclusion */
 
 #include <mach/regs-pmu.h>
arch/arm/plat-samsung/include/plat/pd.h +1 -0
index abb4bc3..ef545ed 100644
@@ -15,6 +15,7 @@ struct samsung_pd_info {
 	int (*enable)(struct device *dev);
 	int (*disable)(struct device *dev);
 	void __iomem *base;
+	unsigned long gate_mask;
 };
 
 enum exynos4_pd_block {
Signature · 1 line
1.7.1.569.g6f426
author maintainer
#2 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

In reply to Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch performs a complete rewrite of sysmmu driver for Samsung platform:
- simplified the resource management: no more single platform device with 32 resources is needed, better fits into linux driver model, each sysmmu instance has it's own resource definition
- the new version uses kernel wide common iommu api defined in include/iommu.h
- cleaned support for sysmmu clocks
- added support for custom fault handlers and tlb replacement policy

---
 arch/arm/mach-exynos4/clock.c               |   68 +-
 arch/arm/mach-exynos4/dev-sysmmu.c          |  615 +++++++++------
 arch/arm/mach-exynos4/include/mach/irqs.h   |   35 +-
 arch/arm/mach-exynos4/include/mach/sysmmu.h |   46 -
 arch/arm/plat-s5p/Kconfig                   |   20 +-
 arch/arm/plat-s5p/include/plat/sysmmu.h     |  241 ++++---
 arch/arm/plat-s5p/sysmmu.c                  | 1191 ++++++++++++++++++++-------
 arch/arm/plat-samsung/include/plat/devs.h   |    2 +-
 8 files changed, 1478 insertions(+), 740 deletions(-)
 rewrite arch/arm/mach-exynos4/dev-sysmmu.c (88%)
 delete mode 100644 arch/arm/mach-exynos4/include/mach/sysmmu.h

rewrite arch/arm/plat-s5p/include/plat/sysmmu.h (83%) rewrite arch/arm/plat-s5p/sysmmu.c (87%)

arch/arm/mach-exynos4/clock.c +39 -29
index 871f9d5..963195e 100644
@@ -20,10 +20,10 @@
 #include <plat/pll.h>
 #include <plat/s5p-clock.h>
 #include <plat/clock-clksrc.h>
+#include <plat/sysmmu.h>
 
 #include <mach/map.h>
 #include <mach/regs-clock.h>
-#include <mach/sysmmu.h>
 
 static struct clk clk_sclk_hdmi27m = {
 	.name		= "sclk_hdmi27m",
@@ -127,6 +127,11 @@ static int exynos4_clk_ip_perir_ctrl(struct clk *clk, int enable)
 	return s5p_gatectrl(S5P_CLKGATE_IP_PERIR, clk, enable);
 }
 
+static int exynos4_clk_ip_dmc_ctrl(struct clk *clk, int enable)
+{
+	return s5p_gatectrl(S5P_CLKGATE_IP_DMC, clk, enable);
+}
+
 /* Core list of CMU_CPU side */
 
 static struct clksrc_clk clk_mout_apll = {
@@ -614,75 +619,80 @@ static struct clk init_clocks_off[] = {
 		.enable		= exynos4_clk_ip_peril_ctrl,
 		.ctrlbit	= (1 << 13),
 	}, {
-		.name		= "SYSMMU_MDMA",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_MDMA,
 		.enable		= exynos4_clk_ip_image_ctrl,
 		.ctrlbit	= (1 << 5),
 	}, {
-		.name		= "SYSMMU_FIMC0",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMC0,
 		.enable		= exynos4_clk_ip_cam_ctrl,
 		.ctrlbit	= (1 << 7),
 	}, {
-		.name		= "SYSMMU_FIMC1",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMC1,
 		.enable		= exynos4_clk_ip_cam_ctrl,
 		.ctrlbit	= (1 << 8),
 	}, {
-		.name		= "SYSMMU_FIMC2",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMC2,
 		.enable		= exynos4_clk_ip_cam_ctrl,
 		.ctrlbit	= (1 << 9),
 	}, {
-		.name		= "SYSMMU_FIMC3",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMC3,
 		.enable		= exynos4_clk_ip_cam_ctrl,
 		.ctrlbit	= (1 << 10),
 	}, {
-		.name		= "SYSMMU_JPEG",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_JPEG,
 		.enable		= exynos4_clk_ip_cam_ctrl,
 		.ctrlbit	= (1 << 11),
 	}, {
-		.name		= "SYSMMU_FIMD0",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMD0,
 		.enable		= exynos4_clk_ip_lcd0_ctrl,
 		.ctrlbit	= (1 << 4),
 	}, {
-		.name		= "SYSMMU_FIMD1",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_FIMD1,
 		.enable		= exynos4_clk_ip_lcd1_ctrl,
 		.ctrlbit	= (1 << 4),
 	}, {
-		.name		= "SYSMMU_PCIe",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_PCIe,
 		.enable		= exynos4_clk_ip_fsys_ctrl,
 		.ctrlbit	= (1 << 18),
 	}, {
-		.name		= "SYSMMU_G2D",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_G2D,
 		.enable		= exynos4_clk_ip_image_ctrl,
 		.ctrlbit	= (1 << 3),
 	}, {
-		.name		= "SYSMMU_ROTATOR",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_ROTATOR,
 		.enable		= exynos4_clk_ip_image_ctrl,
 		.ctrlbit	= (1 << 4),
 	}, {
-		.name		= "SYSMMU_TV",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_TV,
 		.enable		= exynos4_clk_ip_tv_ctrl,
 		.ctrlbit	= (1 << 4),
 	}, {
-		.name		= "SYSMMU_MFC_L",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_MFC_L,
 		.enable		= exynos4_clk_ip_mfc_ctrl,
 		.ctrlbit	= (1 << 1),
 	}, {
-		.name		= "SYSMMU_MFC_R",
-		.id		= -1,
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_MFC_R,
 		.enable		= exynos4_clk_ip_mfc_ctrl,
 		.ctrlbit	= (1 << 2),
+	}, {
+		.name		= "sysmmu",
+		.id		= S5P_SYSMMU_SSS,
+		.enable		= exynos4_clk_ip_dmc_ctrl,
+		.ctrlbit	= (1 << 12),
 	}
 };
 
arch/arm/mach-exynos4/dev-sysmmu.c +383 -232

615 lines not on this page · raw message

arch/arm/mach-exynos4/include/mach/irqs.h +18 -17
index 5d03730..ad1d00c 100644
@@ -55,23 +55,23 @@
 #define COMBINER_GROUP(x)	((x) * MAX_IRQ_IN_COMBINER + IRQ_SPI(64))
 #define COMBINER_IRQ(x, y)	(COMBINER_GROUP(x) + y)
 
-#define IRQ_SYSMMU_MDMA0_0	COMBINER_IRQ(4, 0)
-#define IRQ_SYSMMU_SSS_0	COMBINER_IRQ(4, 1)
-#define IRQ_SYSMMU_FIMC0_0	COMBINER_IRQ(4, 2)
-#define IRQ_SYSMMU_FIMC1_0	COMBINER_IRQ(4, 3)
-#define IRQ_SYSMMU_FIMC2_0	COMBINER_IRQ(4, 4)
-#define IRQ_SYSMMU_FIMC3_0	COMBINER_IRQ(4, 5)
-#define IRQ_SYSMMU_JPEG_0	COMBINER_IRQ(4, 6)
-#define IRQ_SYSMMU_2D_0		COMBINER_IRQ(4, 7)
-
-#define IRQ_SYSMMU_ROTATOR_0	COMBINER_IRQ(5, 0)
-#define IRQ_SYSMMU_MDMA1_0	COMBINER_IRQ(5, 1)
-#define IRQ_SYSMMU_LCD0_M0_0	COMBINER_IRQ(5, 2)
-#define IRQ_SYSMMU_LCD1_M1_0	COMBINER_IRQ(5, 3)
-#define IRQ_SYSMMU_TV_M0_0	COMBINER_IRQ(5, 4)
-#define IRQ_SYSMMU_MFC_M0_0	COMBINER_IRQ(5, 5)
-#define IRQ_SYSMMU_MFC_M1_0	COMBINER_IRQ(5, 6)
-#define IRQ_SYSMMU_PCIE_0	COMBINER_IRQ(5, 7)
+#define IRQ_SYSMMU_MDMA0	COMBINER_IRQ(4, 0)
+#define IRQ_SYSMMU_SSS		COMBINER_IRQ(4, 1)
+#define IRQ_SYSMMU_FIMC0	COMBINER_IRQ(4, 2)
+#define IRQ_SYSMMU_FIMC1	COMBINER_IRQ(4, 3)
+#define IRQ_SYSMMU_FIMC2	COMBINER_IRQ(4, 4)
+#define IRQ_SYSMMU_FIMC3	COMBINER_IRQ(4, 5)
+#define IRQ_SYSMMU_JPEG		COMBINER_IRQ(4, 6)
+#define IRQ_SYSMMU_2D		COMBINER_IRQ(4, 7)
+
+#define IRQ_SYSMMU_ROTATOR	COMBINER_IRQ(5, 0)
+#define IRQ_SYSMMU_MDMA1	COMBINER_IRQ(5, 1)
+#define IRQ_SYSMMU_FIMD0	COMBINER_IRQ(5, 2)
+#define IRQ_SYSMMU_FIMD1	COMBINER_IRQ(5, 3)
+#define IRQ_SYSMMU_TV		COMBINER_IRQ(5, 4)
+#define IRQ_SYSMMU_MFC_L	COMBINER_IRQ(5, 5)
+#define IRQ_SYSMMU_MFC_R	COMBINER_IRQ(5, 6)
+#define IRQ_SYSMMU_PCIE		COMBINER_IRQ(5, 7)
 
 #define IRQ_PDMA0		COMBINER_IRQ(21, 0)
 #define IRQ_PDMA1		COMBINER_IRQ(21, 1)
@@ -157,4 +157,5 @@
 /* Set the default NR_IRQS */
 #define NR_IRQS			(IRQ_GPIO_END)
 
+
 #endif /* __ASM_ARCH_IRQS_H */
arch/arm/mach-exynos4/include/mach/sysmmu.h deleted +0 -46

46 lines not on this page · raw message

arch/arm/plat-s5p/Kconfig +12 -8
index 8492297..9a7805b 100644
@@ -42,14 +42,6 @@ config S5P_HRT
 	help
 	  Use the High Resolution timer support
 
-comment "System MMU"
-
-config S5P_SYSTEM_MMU
-	bool "S5P SYSTEM MMU"
-	depends on ARCH_EXYNOS4
-	help
-	  Say Y here if you want to enable System MMU
-
 config S5P_DEV_FIMC0
 	bool
 	help
@@ -89,3 +81,15 @@ config S5P_SETUP_MIPIPHY
 	bool
 	help
 	  Compile in common setup code for MIPI-CSIS and MIPI-DSIM devices
+
+comment "System MMU"
+
+config IOMMU_API
+	bool
+
+config S5P_SYSTEM_MMU
+	bool "S5P SYSTEM MMU"
+	depends on ARCH_EXYNOS4
+	select IOMMU_API
+	help
+	  Say Y here if you want to enable System MMU
arch/arm/plat-s5p/include/plat/sysmmu.h +146 -95

241 lines not on this page · raw message

arch/arm/plat-s5p/sysmmu.c +879 -312

1,191 lines not on this page · raw message

arch/arm/plat-samsung/include/plat/devs.h +1 -1
index f0da6b7..0ae5dd0 100644
@@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
 extern struct platform_device s5p_device_mipi_csis0;
 extern struct platform_device s5p_device_mipi_csis1;
 
-extern struct platform_device exynos4_device_sysmmu;
+extern struct platform_device exynos4_device_sysmmu[];
 
 /* s3c2440 specific devices */
 
Signature · 1 line
1.7.1.569.g6f426
#3 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 9 lines · Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch performs a complete rewrite of sysmmu driver for Samsung platform:
- simplified the resource management: no more single platform device with 32 resources is needed, better fits into linux driver model, each sysmmu instance has it's own resource definition
- the new version uses kernel wide common iommu api defined in include/iommu.h
- cleaned support for sysmmu clocks
- added support for custom fault handlers and tlb replacement policy

Looks like good progress, but I fear that there is still quite a bit more work needed here.

arch/arm/plat-s5p/sysmmu.c line 45

Quoted 8 lines
arch/arm/plat-s5p/sysmmu.c +8 -0
+static int debug;
+module_param(debug, int, 0644);
+
+#define sysmmu_debug(level, fmt, arg...)                                \
+       do {                                                             \
+               if (debug >= level)                                      \
+                       printk(KERN_DEBUG "[%s] " fmt, __func__, ## arg);\
+       } while (0)

Just use dev_dbg() here, the kernel already has all the infrastructure.

Quoted 33 lines
arch/arm/plat-s5p/sysmmu.c +33 -0
+
+#define generic_extract(l, s, entry) \
+                               ((entry) & l##LPT_##s##_MASK)
+#define flpt_get_1m(entry)     generic_extract(F, 1M, deref_va(entry))
+#define flpt_get_16m(entry)    generic_extract(F, 16M, deref_va(entry))
+#define slpt_get_4k(entry)     generic_extract(S, 4K, deref_va(entry))
+#define slpt_get_64k(entry)    generic_extract(S, 64K, deref_va(entry))
+
+#define generic_entry(l, s, entry) \
+                               (generic_extract(l, s, entry)  | PAGE_##s)
+#define flpt_ent_4k_64k(entry) generic_entry(F, 4K_64K, entry)
+#define flpt_ent_1m(entry)     generic_entry(F, 1M, entry)
+#define flpt_ent_16m(entry)    generic_entry(F, 16M, entry)
+#define slpt_ent_4k(entry)     generic_entry(S, 4K, entry)
+#define slpt_ent_64k(entry)    generic_entry(S, 64K, entry)
+
+#define page_4k_64k(entry)     (deref_va(entry) & PAGE_4K_64K)
+#define page_1m(entry)         (deref_va(entry) & PAGE_1M)
+#define page_16m(entry)                ((deref_va(entry) & PAGE_16M) == PAGE_16M)
+#define page_4k(entry)         (deref_va(entry) & PAGE_4K)
+#define page_64k(entry)                (deref_va(entry) & PAGE_64K)
+
+#define generic_pg_offs(l, s, va) \
+                               (va & ~l##LPT_##s##_MASK)
+#define pg_offs_1m(va)         generic_pg_offs(F, 1M, va)
+#define pg_offs_16m(va)                generic_pg_offs(F, 16M, va)
+#define pg_offs_4k(va)         generic_pg_offs(S, 4K, va)
+#define pg_offs_64k(va)                generic_pg_offs(S, 64K, va)
+
+#define flpt_index(va)         (((va) >> FLPT_IDX_SHIFT) & FLPT_IDX_MASK)
+
+#define generic_offset(l, va)  (((va) >> l##LPT_OFFS_SHIFT) & l##LPT_OFFS_MASK)
+#define flpt_offs(va)          generic_offset(F, va)

arch/arm/plat-s5p/sysmmu.c line 110

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+#define slpt_offs(va)          generic_offset(S, va)
+
+#define invalidate_slpt_ent(slpt_va) (deref_va(slpt_va) = 0UL)
+
+#define get_irq_callb(cb) \
+                               (s5p_domain->irq_callb ? \
+                                       (s5p_domain->irq_callb->cb ? \
+                                       s5p_domain->irq_callb->cb : \
+                                       s5p_sysmmu_irq_callb.cb) \
+                               : s5p_sysmmu_irq_callb.cb)

These macros are really confusing, especially the ones that implicitly access variables with a specific name. How about converting them into inline functions?

Quoted 1 line

+phys_addr_t s5p_iova_to_phys(struct iommu_domain *domain, unsigned long iova)

This should be static.

Quoted 12 lines
arch/arm/plat-s5p/sysmmu.c +12 -0
+struct device *s5p_sysmmu_get(enum s5p_sysmmu_ip ip)
+{
+       struct device *ret = NULL;
+       unsigned long flags;
+
+       spin_lock_irqsave(&sysmmu_slock, flags);
+       if (sysmmu_table[ip]) {
+               try_module_get(THIS_MODULE);
+               ret = sysmmu_table[ip]->dev;
+       }
+       spin_unlock_irqrestore(&sysmmu_slock, flags);
+

arch/arm/plat-s5p/sysmmu.c line 561

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+       return ret;
+}
+EXPORT_SYMBOL_GPL(s5p_sysmmu_get);
+
+void s5p_sysmmu_put(void *dev)
+{
+       BUG_ON(!dev);
+       module_put(THIS_MODULE);
+}
+EXPORT_SYMBOL_GPL(s5p_sysmmu_put);

These look wrong for a number of reasons:

* try_module_get(THIS_MODULE) makes no sense at all, the idea of the try_module_get is to pin down another module that was calling down, which I suppose is not needed here.

* This extends the generic IOMMU API in platform specific ways, don't do that.

* I think you can do without these functions by including a pointer to the iommu structure in dev_archdata, see arch/powerpc/include/asm/device.h for an example.

Quoted 6 lines
arch/arm/plat-s5p/sysmmu.c +6 -0
+void s5p_sysmmu_domain_irq_callb(struct iommu_domain *domain,
+                           struct s5p_sysmmu_irq_callb *ops, void *priv)
+{
+       struct s5p_sysmmu_domain *s5p_domain = domain->priv;
+       s5p_domain->irq_callb = ops;
+       s5p_domain->irq_callb_priv = priv;

arch/arm/plat-s5p/sysmmu.c line 578

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+}
+EXPORT_SYMBOL(s5p_sysmmu_domain_irq_callb);
+
+
+void s5p_sysmmu_domain_tlb_policy(struct iommu_domain *domain, int policy)
+{
+       struct s5p_sysmmu_domain *s5p_domain = domain->priv;
+       s5p_domain->policy = policy;
+}
+EXPORT_SYMBOL(s5p_sysmmu_domain_tlb_policy);

More private extensions that should not be here. Better extend the generic IOMMU API to contain callbacks for these if they are required, and document them in a generic location.

Quoted 5 lines
arch/arm/plat-s5p/sysmmu.c +5 -0
+static void s5p_sysmmu_irq_page_fault(struct iommu_domain *domain, int reason,
+                                     unsigned long addr, void *priv)
+{
+       sysmmu_debug(3, "%s: Faulting virtual address: 0x%08lx\n",
+                    irq_reasons[reason], addr);

arch/arm/plat-s5p/sysmmu.c line 594

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+       BUG();
+}
+
+static void s5p_sysmmu_irq_generic_callb(struct iommu_domain *domain,
+                                        int reason, unsigned long addr,
+                                        void *priv)
+{
+       sysmmu_debug(3, "%s\n", irq_reasons[reason]);
+       BUG();
+}

Why BUG() here? The backtrace of an interrupt handler should be rather uninteresting, and you just end up in panic() since this is run from an interrupt handler.

Quoted 66 lines
arch/arm/plat-s5p/sysmmu.c +66 -0
+static struct s5p_sysmmu_irq_callb s5p_sysmmu_irq_callb = {
+       .page_fault = s5p_sysmmu_irq_page_fault,
+       .ar_fault = s5p_sysmmu_irq_generic_callb,
+       .aw_fault = s5p_sysmmu_irq_generic_callb,
+       .bus_error = s5p_sysmmu_irq_generic_callb,
+       .ar_security = s5p_sysmmu_irq_generic_callb,
+       .ar_prot = s5p_sysmmu_irq_generic_callb,
+       .aw_security = s5p_sysmmu_irq_generic_callb,
+       .aw_prot = s5p_sysmmu_irq_generic_callb,
+};
+
+static irqreturn_t s5p_sysmmu_irq(int irq, void *dev_id)
+{
+       struct s5p_sysmmu_info *sysmmu = dev_id;
+       struct s5p_sysmmu_domain *s5p_domain = sysmmu->domain->priv;
+       unsigned int reg_INT_STATUS;
+
+       if (false == sysmmu->enabled)
+               return IRQ_HANDLED;
+
+       reg_INT_STATUS = readl(sysmmu->regs + S5P_INT_STATUS);
+       if (reg_INT_STATUS & 0xFF) {
+               S5P_IRQ_CB(cb);
+               enum s5p_sysmmu_fault reason = 0;
+               unsigned long fault = 0;
+               unsigned reg = 0;
+               cb = NULL;
+               switch (reg_INT_STATUS & 0xFF) {
+               case 0x1:
+                       cb = get_irq_callb(page_fault);
+                       reason = S5P_SYSMMU_PAGE_FAULT;
+                       reg = S5P_PAGE_FAULT_ADDR;
+                       break;
+               case 0x2:
+                       cb = get_irq_callb(ar_fault);
+                       reason = S5P_SYSMMU_AR_FAULT;
+                       reg = S5P_AR_FAULT_ADDR;
+                       break;
+               case 0x4:
+                       cb = get_irq_callb(aw_fault);
+                       reason = S5P_SYSMMU_AW_FAULT;
+                       reg = S5P_AW_FAULT_ADDR;
+                       break;
+               case 0x8:
+                       cb = get_irq_callb(bus_error);
+                       reason = S5P_SYSMMU_BUS_ERROR;
+                       /* register common to page fault and bus error */
+                       reg = S5P_PAGE_FAULT_ADDR;
+                       break;
+               case 0x10:
+                       cb = get_irq_callb(ar_security);
+                       reason = S5P_SYSMMU_AR_SECURITY;
+                       reg = S5P_AR_FAULT_ADDR;
+                       break;
+               case 0x20:
+                       cb = get_irq_callb(ar_prot);
+                       reason = S5P_SYSMMU_AR_PROT;
+                       reg = S5P_AR_FAULT_ADDR;
+                       break;
+               case 0x40:
+                       cb = get_irq_callb(aw_security);
+                       reason = S5P_SYSMMU_AW_SECURITY;
+                       reg = S5P_AW_FAULT_ADDR;
+                       break;
+               case 0x80:
+                       cb = get_irq_callb(aw_prot);

arch/arm/plat-s5p/sysmmu.c line 671

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+                       reason = S5P_SYSMMU_AW_PROT;
+                       reg = S5P_AW_FAULT_ADDR;
+                       break;
+               }
+               fault = readl(sysmmu->regs + reg);
+               cb(sysmmu->domain, reason, fault, s5p_domain->irq_callb_priv);
+               writel(reg_INT_STATUS, sysmmu->regs + S5P_INT_CLEAR);
+       }
+       return IRQ_HANDLED;
+}

I think it would be more readable and more efficient to just use a lookup table here instead of the long switch/case statement.

Quoted 18 lines
arch/arm/plat-s5p/sysmmu.c +18 -0
+static int
+s5p_sysmmu_suspend(struct platform_device *pdev, pm_message_t state)
+{
+       int ret = 0;
+       sysmmu_debug(3, "begin\n");
+
+       return ret;
+}
+
+static int s5p_sysmmu_resume(struct platform_device *pdev)
+{
+       int ret = 0;
+       sysmmu_debug(3, "begin\n");
+
+       return ret;
+}
+
+static int s5p_sysmmu_runtime_suspend(struct device *dev)

arch/arm/plat-s5p/sysmmu.c line 819

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+{
+       sysmmu_debug(3, "begin\n");
+       return 0;
+}
+
+static int s5p_sysmmu_runtime_resume(struct device *dev)
+{
+       sysmmu_debug(3, "begin\n");
+       return 0;
+}

Why even provide these when they don't do anything?

Quoted 17 lines
arch/arm/plat-s5p/sysmmu.c +17 -0
+static int __init
+s5p_sysmmu_register(void)
+{
+       int ret;
+
+       sysmmu_debug(3, "Registering sysmmu driver...\n");
+
+       slpt_cache = kmem_cache_create("slpt_cache", 1024, 1024,
+                                      SLAB_HWCACHE_ALIGN, NULL);
+       if (!slpt_cache) {
+               printk(KERN_ERR
+                       "%s: failed to allocated slpt cache\n", __func__);
+               return -ENOMEM;
+       }
+
+       ret = platform_driver_register(&s5p_sysmmu_driver);
+

arch/arm/plat-s5p/sysmmu.c line 864

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+       if (ret) {
+               printk(KERN_ERR
+                       "%s: failed to register sysmmu driver\n", __func__);
+               return -EINVAL;
+       }
+
+       register_iommu(&s5p_sysmmu_ops);
+
+       return ret;
+}

When you register the iommu unconditionally, it becomes impossible for this driver to coexist with other iommu drivers in the same kernel, which does against the concept of having a platform driver for this.

It might be better to call the s5p_sysmmu_register function from the board files and have no platform devices at all if each IOMMU is always bound to a specific device anyway.

arch/arm/plat-samsung/include/plat/devs.h extern struct platform_device s5p_device_fimc3; · line 145

Quoted 10 lines
arch/arm/plat-samsung/include/plat/devs.h +1 -1
index f0da6b7..0ae5dd0 100644
@@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
 extern struct platform_device s5p_device_mipi_csis0;
 extern struct platform_device s5p_device_mipi_csis1;
 
-extern struct platform_device exynos4_device_sysmmu;
+extern struct platform_device exynos4_device_sysmmu[];

Why is this a global variable? I would expect this to be private to the implementation.

Arnd

author maintainer
#4 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 9 lines · Arnd Bergmann
Quoted 3 lines · Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch performs a complete rewrite of sysmmu driver for Samsung

platform:

Quoted 4 lines

- simplified the resource management: no more single platform device with 32 resources is needed, better fits into linux driver model, each sysmmu instance has it's own resource definition
- the new version uses kernel wide common iommu api defined in

Quoted 6 lines

include/iommu.h

Quoted 2 lines

- cleaned support for sysmmu clocks
- added support for custom fault handlers and tlb replacement policy

Looks like good progress, but I fear that there is still quite a bit more work needed here.

Thanks for your comments! I've snipped the minor implementation comments and focused only on the core iommu API.

(snipped)

Quoted 29 lines
Quoted 12 lines
arch/arm/plat-s5p/sysmmu.c +12 -0
+struct device *s5p_sysmmu_get(enum s5p_sysmmu_ip ip)
+{
+       struct device *ret = NULL;
+       unsigned long flags;
+
+       spin_lock_irqsave(&sysmmu_slock, flags);
+       if (sysmmu_table[ip]) {
+               try_module_get(THIS_MODULE);
+               ret = sysmmu_table[ip]->dev;
+       }
+       spin_unlock_irqrestore(&sysmmu_slock, flags);
+

arch/arm/plat-s5p/sysmmu.c line 561

Quoted 10 lines
arch/arm/plat-s5p/sysmmu.c +10 -0
+       return ret;
+}
+EXPORT_SYMBOL_GPL(s5p_sysmmu_get);
+
+void s5p_sysmmu_put(void *dev)
+{
+       BUG_ON(!dev);
+       module_put(THIS_MODULE);
+}
+EXPORT_SYMBOL_GPL(s5p_sysmmu_put);

These look wrong for a number of reasons:

* try_module_get(THIS_MODULE) makes no sense at all, the idea of the try_module_get is to pin down another module that was calling down, which I suppose is not needed here.

Quoted 6 lines

* This extends the generic IOMMU API in platform specific ways, don't do that.

* I think you can do without these functions by including a pointer to the iommu structure in dev_archdata, see arch/powerpc/include/asm/device.h for an example.

We heavily based our solution on the iommu implementation found in arch/arm/mach-msm/{devices-iommu,iommu,iommu_dev}.c

The s5p_sysmmu_get/put functions are equivalent for msm_iommu_{get,put}_ctx.

(snipped)

Quoted 22 lines
Quoted 22 lines
arch/arm/plat-s5p/sysmmu.c +22 -0
+static int
+s5p_sysmmu_suspend(struct platform_device *pdev, pm_message_t state)
+{
+       int ret = 0;
+       sysmmu_debug(3, "begin\n");
+
+       return ret;
+}
+
+static int s5p_sysmmu_resume(struct platform_device *pdev)
+{
+       int ret = 0;
+       sysmmu_debug(3, "begin\n");
+
+       return ret;
+}
+
+static int s5p_sysmmu_runtime_suspend(struct device *dev)
+{
+       sysmmu_debug(3, "begin\n");
+       return 0;
+}
Quoted 8 lines

arch/arm/plat-s5p/sysmmu.c line 819

Quoted 6 lines
arch/arm/plat-s5p/sysmmu.c +6 -0
+
+static int s5p_sysmmu_runtime_resume(struct device *dev)
+{
+       sysmmu_debug(3, "begin\n");
+       return 0;
+}

Why even provide these when they don't do anything?

Because they are required by pm_runtime. If no runtime_{suspend,resume} methods are provided, the pm_runtime core will not call proper methods on parent device for pmruntime_{get,put}_sync(). The parent device for each sysmmu platform device is the power domain the sysmmu belongs to.

I know this is crazy, but this is the only way it can be handled now with runtime_pm.

Quoted 34 lines

arch/arm/plat-s5p/sysmmu.c

Quoted 12 lines
arch/arm/plat-s5p/sysmmu.c +12 -0
+static int __init
+s5p_sysmmu_register(void)
+{
+       int ret;
+
+       sysmmu_debug(3, "Registering sysmmu driver...\n");
+
+       slpt_cache = kmem_cache_create("slpt_cache", 1024, 1024,
+                                      SLAB_HWCACHE_ALIGN, NULL);
+       if (!slpt_cache) {
+               printk(KERN_ERR
+                       "%s: failed to allocated slpt cache\n",

__func__);

arch/arm/plat-s5p/sysmmu.c

Quoted 8 lines
arch/arm/plat-s5p/sysmmu.c +8 -0
+               return -ENOMEM;
+       }
+
+       ret = platform_driver_register(&s5p_sysmmu_driver);
+
+       if (ret) {
+               printk(KERN_ERR
+                       "%s: failed to register sysmmu driver\n",

__func__);

arch/arm/plat-s5p/sysmmu.c line 864

Quoted 7 lines
arch/arm/plat-s5p/sysmmu.c +7 -0
+               return -EINVAL;
+       }
+
+       register_iommu(&s5p_sysmmu_ops);
+
+       return ret;
+}

When you register the iommu unconditionally, it becomes impossible for this driver to coexist with other iommu drivers in the same kernel, which does against the concept of having a platform driver for this.

Quoted 3 lines

It might be better to call the s5p_sysmmu_register function from the board files and have no platform devices at all if each IOMMU is always bound to a specific device anyway.

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

I think it might be beneficial to describe a bit more our hardware (Exynos4 platform). There are a number of multimedia blocks. Each has it's own IOMMU controller. Each IOMMU controller has his own set of hardware registers and irq. There is also a GPU unit (Mali) which has it's own IOMMU hardware, incompatible with the SYSMMU, so right now it is ignored.

The multimedia blocks are modeled as platform devices and are independent of platform type (same multimedia blocks can be found on other Samsung machines, like for example s5pv210/s5pc110), see arch/arm/plat-s5p/dev-*.c and arch/arm/plat-samsung/dev-*.c.

Platform driver data defined in the above files are registered by each board startup code, usually by platform_add_devices(), for more details please check arch/arm/mach-s5pv210/mach-goni.c. There is struct platform_device *goni_devices[] array which get registered in the last line in goni_machine_init() function.

For IOMMU controllers on Exynos4 we created an array of platform devices:
extern struct platform_device exynos4_device_sysmmu[];

Now the board startup code registers only these sysmmu controllers (instances) that are required on the particular board. See "[PATCH 7/7]
ARM: EXYNOS4: enable FIMC on Universal_C210":
@@ -613,6 +616,15 @@ static struct platform_device *universal_devices[]

__initdata = {
        &s3c_device_hsmmc2,
        &s3c_device_hsmmc3,
        &s3c_device_i2c5,
+       &s5p_device_fimc0,
+       &s5p_device_fimc1,
+       &s5p_device_fimc2,
+       &s5p_device_fimc3,
+       &exynos4_device_pd[PD_CAM],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC0],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC1],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC2],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC3],

We need to map the above structure into linux/iommu.h api.

The domain defined in iommu api are quite straightforward. Each domain is just a set of mappings between physical addresses (phys) and io addresses (iova).

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device *dev);

We assumed that they just assign the domain (mapping) to particular instance of iommu. However the driver need to get somehow the pointer to the iommu instance. That's why we added the s5p_sysmmu_{get,put} functions.

Now I see that you want to make the clients (drivers) to provide their own struct device pointer to the iommu_{attach,detach}_device() function instead of giving there a pointer to iommu device. Am I right? We will need some kind of mapping between multimedia devices and particular instanced of sysmmu controllers.

There will be also some problems with such approach. Mainly we have a multimedia codec module, which have 2 memory controllers (for faster transfers) and 2 iommu controllers. How can we handle such case?

Quoted 9 lines

arch/arm/plat-

Quoted 1 line
arch/arm/plat- renamed from arch/arm/plat-samsung/include/plat/devs.h +0 -0

samsung/include/plat/devs.h

Quoted 7 lines

index f0da6b7..0ae5dd0 100644

arch/arm/plat-samsung/include/plat/devs.h +0 -0
@@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
 extern struct platform_device s5p_device_mipi_csis0;
 extern struct platform_device s5p_device_mipi_csis1;
 
Quoted 5 lines

arch/arm/plat-samsung/include/plat/devs.h extern struct platform_device s5p_device_fimc3; · line 145

Quoted 2 lines
arch/arm/plat-samsung/include/plat/devs.h +1 -1
-extern struct platform_device exynos4_device_sysmmu;
+extern struct platform_device exynos4_device_sysmmu[];

Why is this a global variable? I would expect this to be private to the implementation.

To allow each board to register only particular instances of sysmmu controllers.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#5 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

(adding Joerg to Cc)

Quoted 13 lines · Marek Szyprowski
Quoted 6 lines

These look wrong for a number of reasons:

* try_module_get(THIS_MODULE) makes no sense at all, the idea of the try_module_get is to pin down another module that was calling down, which I suppose is not needed here.

Quoted 6 lines

* This extends the generic IOMMU API in platform specific ways, don't do that.

* I think you can do without these functions by including a pointer to the iommu structure in dev_archdata, see arch/powerpc/include/asm/device.h for an example.

Quoted 6 lines

We heavily based our solution on the iommu implementation found in arch/arm/mach-msm/{devices-iommu,iommu,iommu_dev}.c

The s5p_sysmmu_get/put functions are equivalent for msm_iommu_{get,put}_ctx.

(snipped)

Yes, I'm sorry about this. I commented on the early versions of the MSM driver, but then did not do another review of the version that actually got merged. That should also be fixed, ideally we can come up with a way that works for both drivers.

Quoted 9 lines
Quoted 1 line

Why even provide these when they don't do anything?

Because they are required by pm_runtime. If no runtime_{suspend,resume} methods are provided, the pm_runtime core will not call proper methods on parent device for pmruntime_{get,put}_sync(). The parent device for each sysmmu platform device is the power domain the sysmmu belongs to.

I know this is crazy, but this is the only way it can be handled now with runtime_pm.

Please don't try to work around kernel features when they don't fit what you are doing. The intent of the way that runtime_pm works is to make life easier for driver writers, not harder ;-)

I can see three ways that would be better solutions:

1. change the runtime_pm subsystem to allow it to ignore some devices in an easy way.

2. change the device layout if the sysmmu. If the iommu device is a child of the device that it is responsible for, I guess you don't have this problem.

3. Not represent the iommu as a device at all, just as a property of another device.

Quoted 4 lines
Quoted 3 lines

When you register the iommu unconditionally, it becomes impossible for this driver to coexist with other iommu drivers in the same kernel, which does against the concept of having a platform driver for this.

Quoted 8 lines
Quoted 3 lines

It might be better to call the s5p_sysmmu_register function from the board files and have no platform devices at all if each IOMMU is always bound to a specific device anyway.

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

Quoted 38 lines

I think it might be beneficial to describe a bit more our hardware (Exynos4 platform). There are a number of multimedia blocks. Each has it's own IOMMU controller. Each IOMMU controller has his own set of hardware registers and irq. There is also a GPU unit (Mali) which has it's own IOMMU hardware, incompatible with the SYSMMU, so right now it is ignored.

The multimedia blocks are modeled as platform devices and are independent of platform type (same multimedia blocks can be found on other Samsung machines, like for example s5pv210/s5pc110), see arch/arm/plat-s5p/dev-*.c and arch/arm/plat-samsung/dev-*.c.

Platform driver data defined in the above files are registered by each board startup code, usually by platform_add_devices(), for more details please check arch/arm/mach-s5pv210/mach-goni.c. There is struct platform_device *goni_devices[] array which get registered in the last line in goni_machine_init() function.

For IOMMU controllers on Exynos4 we created an array of platform devices:
extern struct platform_device exynos4_device_sysmmu[];

Now the board startup code registers only these sysmmu controllers (instances) that are required on the particular board. See "[PATCH 7/7]
ARM: EXYNOS4: enable FIMC on Universal_C210":

(quoted diff) +9 -0
@@ -613,6 +616,15 @@ static struct platform_device *universal_devices[]
 __initdata = {
        &s3c_device_hsmmc2,
        &s3c_device_hsmmc3,
        &s3c_device_i2c5,
+       &s5p_device_fimc0,
+       &s5p_device_fimc1,
+       &s5p_device_fimc2,
+       &s5p_device_fimc3,
+       &exynos4_device_pd[PD_CAM],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC0],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC1],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC2],
+       &exynos4_device_sysmmu[S5P_SYSMMU_FIMC3],
 
Quoted 1 line

We need to map the above structure into linux/iommu.h api.

Thanks for the background information.

Quoted 17 lines

The domain defined in iommu api are quite straightforward. Each domain is just a set of mappings between physical addresses (phys) and io addresses (iova).

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device *dev);

We assumed that they just assign the domain (mapping) to particular instance of iommu. However the driver need to get somehow the pointer to the iommu instance. That's why we added the s5p_sysmmu_{get,put} functions.

Now I see that you want to make the clients (drivers) to provide their own struct device pointer to the iommu_{attach,detach}_device() function instead of giving there a pointer to iommu device. Am I right? We will need some kind of mapping between multimedia devices and particular instanced of sysmmu controllers.

Quoted 3 lines

There will be also some problems with such approach. Mainly we have a multimedia codec module, which have 2 memory controllers (for faster transfers) and 2 iommu controllers. How can we handle such case?

It's not quite how the domains are meant to be used. In the AMD IOMMU that the API is based on, any number of devices can share one domain, and devices might be able to have mappings in multiple domains.

The domain really reflects the user, not the device here, which makes more sense if you think of virtual machines than of multimedia devices.

I would suggest that you just use a single iommu_domain globally for all in-kernel users.

Quoted 9 lines
Quoted 9 lines

arch/arm/plat-

Quoted 1 line
arch/arm/plat- renamed from arch/arm/plat-samsung/include/plat/devs.h +0 -0

samsung/include/plat/devs.h

Quoted 7 lines

index f0da6b7..0ae5dd0 100644

arch/arm/plat-samsung/include/plat/devs.h +0 -0
@@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
 extern struct platform_device s5p_device_mipi_csis0;
 extern struct platform_device s5p_device_mipi_csis1;
 
Quoted 7 lines
Quoted 5 lines

arch/arm/plat-samsung/include/plat/devs.h extern struct platform_device s5p_device_fimc3; · line 145

Quoted 2 lines
arch/arm/plat-samsung/include/plat/devs.h +1 -1
-extern struct platform_device exynos4_device_sysmmu;
+extern struct platform_device exynos4_device_sysmmu[];

Why is this a global variable? I would expect this to be private to the implementation.

To allow each board to register only particular instances of sysmmu controllers.

That sounds like an unnecessarily complicated way of doing it. This would be another reason to not make each one a device, but have something else in struct device take care of it.

Arnd

#6 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 6 lines · Arnd Bergmann

(adding Joerg to Cc)

Quoted 2 lines · Marek Szyprowski
Quoted 2 lines

* This extends the generic IOMMU API in platform specific ways, don't do that.

This is certainly not a good idea. Please Cc me on IOMMU-API changes in the future to that I can have a look at it. This is also a good way to prevent misunderstandings (which I found some in this mail).

Quoted 5 lines
Quoted 4 lines

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

The current IOMMU-API interface is very simple. It delegates the selection of the particular IOMMU device to the IOMMU driver. Handle this selection above the IOMMU driver is a complex thing to do. We will need some kind of generic IOMMU support in the device-core and attach IOMMUs to device sub-trees.

A simpler and less intrusive solution is to implement some wrapper code which dispatches the IOMMU-API calls to the IOMMU driver implementation required for that device.

Quoted 10 lines
Quoted 10 lines

I think it might be beneficial to describe a bit more our hardware (Exynos4 platform). There are a number of multimedia blocks. Each has it's own IOMMU controller. Each IOMMU controller has his own set of hardware registers and irq. There is also a GPU unit (Mali) which has it's own IOMMU hardware, incompatible with the SYSMMU, so right now it is ignored.

The multimedia blocks are modeled as platform devices and are independent of platform type (same multimedia blocks can be found on other Samsung machines, like for example s5pv210/s5pc110), see arch/arm/plat-s5p/dev-*.c and arch/arm/plat-samsung/dev-*.c.

Question: Does every platform device has a different type of IOMMU? Or are the IOMMUs on all of these platform devices similar enough to be handled by a single driver?

Quoted 3 lines
Quoted 3 lines

The domain defined in iommu api are quite straightforward. Each domain is just a set of mappings between physical addresses (phys) and io addresses (iova).

Each domain represents an address space. In the AMD IOMMU this is basically a page-table.

Quoted 2 lines
Quoted 2 lines

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device *dev);

Right, and each driver can allocate its own domains.

Quoted 3 lines
Quoted 3 lines

We assumed that they just assign the domain (mapping) to particular instance of iommu. However the driver need to get somehow the pointer to the iommu instance. That's why we added the s5p_sysmmu_{get,put} functions.

The functions attach a specific device to an IOMMU domain (== address space). These devices might be behind different IOMMUs and it is the responsibility of the IOMMU driver to setup everything correctly.

Quoted 3 lines

It's not quite how the domains are meant to be used. In the AMD IOMMU that the API is based on, any number of devices can share one domain, and devices might be able to have mappings in multiple domains.

Yes, any number of devices can be assigned to one domain, but each device only belongs to one domain at each point in time. But it is possible to detach a device from one domain and attach it to another.

Regards,

Joerg

Signature · 5 lines
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
#7 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 20 lines · Joerg
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

The current IOMMU-API interface is very simple. It delegates the selection of the particular IOMMU device to the IOMMU driver. Handle this selection above the IOMMU driver is a complex thing to do. We will need some kind of generic IOMMU support in the device-core and attach IOMMUs to device sub-trees.

Quoted 3 lines

A simpler and less intrusive solution is to implement some wrapper code which dispatches the IOMMU-API calls to the IOMMU driver implementation required for that device.

Right. We already do that for the dma-mapping API on some architectures, and I suppose we can consolidate the mechanism here, possibly into something that ends up in the common struct device rather than in the archdata.

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

I think it might be beneficial to describe a bit more our hardware (Exynos4 platform). There are a number of multimedia blocks. Each has it's own IOMMU controller. Each IOMMU controller has his own set of hardware registers and irq. There is also a GPU unit (Mali) which has it's own IOMMU hardware, incompatible with the SYSMMU, so right now it is ignored.

Quoted 8 lines
Quoted 4 lines
Quoted 4 lines

The multimedia blocks are modeled as platform devices and are independent of platform type (same multimedia blocks can be found on other Samsung machines, like for example s5pv210/s5pc110), see arch/arm/plat-s5p/dev-*.c and arch/arm/plat-samsung/dev-*.c.

Question: Does every platform device has a different type of IOMMU? Or are the IOMMUs on all of these platform devices similar enough to be handled by a single driver?

As Marek explained in the thread before you got on Cc, they are all the same, except for the graphics core (Mali) that has a different one but currently disables that.

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device *dev);

Right, and each driver can allocate its own domains.

For the cases that use the normal dma-mapping API, I guess there only needs to be one domain to cover the kernel, which can then be hidden in the driver provides the dma_map_ops based on an iommu_ops.

Quoted 7 lines
Quoted 3 lines

It's not quite how the domains are meant to be used. In the AMD IOMMU that the API is based on, any number of devices can share one domain, and devices might be able to have mappings in multiple domains.

Yes, any number of devices can be assigned to one domain, but each device only belongs to one domain at each point in time. But it is possible to detach a device from one domain and attach it to another.

I was thinking of the SR-IOV case, where a single hardware device is represented as multiple logical devices. As far as I understand, each logical devices can only belong to one domain, but they don't all have to be the same.

Arnd

#8 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 21 lines · Arnd Bergmann
Quoted 20 lines · Joerg
Quoted 5 lines · Arnd Bergmann
Quoted 4 lines

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

The current IOMMU-API interface is very simple. It delegates the selection of the particular IOMMU device to the IOMMU driver. Handle this selection above the IOMMU driver is a complex thing to do. We will need some kind of generic IOMMU support in the device-core and attach IOMMUs to device sub-trees.

Quoted 8 lines
Quoted 3 lines

A simpler and less intrusive solution is to implement some wrapper code which dispatches the IOMMU-API calls to the IOMMU driver implementation required for that device.

Right. We already do that for the dma-mapping API on some architectures, and I suppose we can consolidate the mechanism here, possibly into something that ends up in the common struct device rather than in the archdata.

The struct device solution is very much what I meant by adding this into the device-core code :)

Quoted 7 lines
Quoted 3 lines

Question: Does every platform device has a different type of IOMMU? Or are the IOMMUs on all of these platform devices similar enough to be handled by a single driver?

As Marek explained in the thread before you got on Cc, they are all the same, except for the graphics core (Mali) that has a different one but currently disables that.

Then it is no problem at all. The IOMMU driver can find out itself which IOMMU needs to be used for which device. The x86 implementations already do this.

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device *dev);

Right, and each driver can allocate its own domains.

For the cases that use the normal dma-mapping API, I guess there only needs to be one domain to cover the kernel, which can then be hidden in the driver provides the dma_map_ops based on an iommu_ops.

Yes, for dma-api usage one domain is sufficient. But using one domain for each device has benefits too. It reduces lock-contention on the domain side and also increases security by isolating the devices from each other.

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

It's not quite how the domains are meant to be used. In the AMD IOMMU that the API is based on, any number of devices can share one domain, and devices might be able to have mappings in multiple domains.

Quoted 8 lines
Quoted 3 lines

Yes, any number of devices can be assigned to one domain, but each device only belongs to one domain at each point in time. But it is possible to detach a device from one domain and attach it to another.

I was thinking of the SR-IOV case, where a single hardware device is represented as multiple logical devices. As far as I understand, each logical devices can only belong to one domain, but they don't all have to be the same.

Well, right, the IOMMU-API makes no distinction between PF and VF. Each function is just a pci_dev which can independently assigned to a domain. So if 'device' means a physical card with virtual functions then yes, a device can be attached to multiple domains, one domain per VF.

Joerg

Signature · 5 lines
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
author maintainer
#9 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

In reply to Arnd Bergmann

Hello,

Quoted 19 lines · Arnd Bergmann
Quoted 9 lines · Marek Szyprowski
Quoted 9 lines

These look wrong for a number of reasons:

* try_module_get(THIS_MODULE) makes no sense at all, the idea of the try_module_get is to pin down another module that was calling down, which I suppose is not needed here.

* This extends the generic IOMMU API in platform specific ways, don't do that.

Quoted 8 lines
Quoted 3 lines

* I think you can do without these functions by including a pointer to the iommu structure in dev_archdata, see arch/powerpc/include/asm/device.h for an example.

We heavily based our solution on the iommu implementation found in arch/arm/mach-msm/{devices-iommu,iommu,iommu_dev}.c

The s5p_sysmmu_get/put functions are equivalent for

Quoted 8 lines

msm_iommu_{get,put}_ctx.

Quoted 2 lines

(snipped)

Yes, I'm sorry about this. I commented on the early versions of the MSM driver, but then did not do another review of the version that actually got merged. That should also be fixed, ideally we can come up with a way that works for both drivers.

OK, so it looks that we misunderstood the IOMMU API basing on the particular implementation.

Quoted 19 lines
Quoted 9 lines
Quoted 1 line

Why even provide these when they don't do anything?

Because they are required by pm_runtime. If no runtime_{suspend,resume} methods are provided, the pm_runtime core will not call proper methods on parent device for pmruntime_{get,put}_sync(). The parent device for each sysmmu platform device is the power domain the sysmmu belongs to.

I know this is crazy, but this is the only way it can be handled now with runtime_pm.

Please don't try to work around kernel features when they don't fit what you are doing. The intent of the way that runtime_pm works is to make life easier for driver writers, not harder ;-)

I can see three ways that would be better solutions:

1. change the runtime_pm subsystem to allow it to ignore some devices in an easy way.

Quoted 6 lines

2. change the device layout if the sysmmu. If the iommu device is a child of the device that it is responsible for, I guess you don't have this problem.

3. Not represent the iommu as a device at all, just as a property of another device.

Ok, we will handle this issue somehow. I consider this a minor issue and I would like to focus on the IOMMU/dma-mapping APIs first.

Quoted 13 lines
Quoted 4 lines
Quoted 3 lines

When you register the iommu unconditionally, it becomes impossible for this driver to coexist with other iommu drivers in the same kernel, which does against the concept of having a platform driver for this.

Quoted 8 lines
Quoted 3 lines

It might be better to call the s5p_sysmmu_register function from the board files and have no platform devices at all if each IOMMU is always bound to a specific device anyway.

Ok, it looks I don't fully get how this iommu.h should be used. It looks that there can be only one instance of iommu ops registered in the system, so only one iommu driver can be activated. You are right that the iommu driver has to be registered on first probe().

Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

Ok. I understand. IOMMU API is quite nice abstraction of the IOMMU chip. dma-mapping API is something much more complex that creates the actual mapping for various sets of the devices. IMHO the right direction will be to create dma-mapping implementation that will be just a client of the IOMMU API. What's your opinion?

(snipped)

Quoted 32 lines
Quoted 2 lines

The domain defined in iommu api are quite straightforward. Each domain is just a set of mappings between physical addresses (phys) and io

addresses

Quoted 4 lines

(iova).

For the drivers the most important are the following functions:
iommu_{attach,detach}_device(struct iommu_domain *domain, struct device

*dev);

Quoted 2 lines

We assumed that they just assign the domain (mapping) to particular

instance

Quoted 4 lines

of iommu. However the driver need to get somehow the pointer to the iommu instance. That's why we added the s5p_sysmmu_{get,put} functions.

Now I see that you want to make the clients (drivers) to provide their

own

Quoted 1 line

struct device pointer to the iommu_{attach,detach}_device() function

instead of

Quoted 1 line

giving there a pointer to iommu device. Am I right? We will need some

kind of

Quoted 5 lines

mapping between multimedia devices and particular instanced of sysmmu controllers.

There will be also some problems with such approach. Mainly we have a multimedia codec module, which have 2 memory controllers (for faster

transfers)

Quoted 1 line

and 2 iommu controllers. How can we handle such case?

It's not quite how the domains are meant to be used. In the AMD IOMMU that the API is based on, any number of devices can share one domain, and devices might be able to have mappings in multiple domains.

Quoted 5 lines

The domain really reflects the user, not the device here, which makes more sense if you think of virtual machines than of multimedia devices.

I would suggest that you just use a single iommu_domain globally for all in-kernel users.

There are cases where having a separate mapping for each device makes sense. It definitely increases the security and helps to find some bugs in the drivers.

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for allocations for the second IOMMU controller.

Quoted 16 lines
Quoted 1 line
Quoted 1 line

Review

Quoted 1 line
(quoted diff) +0 -0

b/arch/arm/plat-

Quoted 14 lines
Quoted 13 lines

samsung/include/plat/devs.h

arch/arm/plat-samsung/include/plat/devs.h extern struct platform_device s5p_device_fimc3; · line 145

Quoted 9 lines

index f0da6b7..0ae5dd0 100644

arch/arm/plat-samsung/include/plat/devs.h +1 -1
@@ -142,7 +142,7 @@ extern struct platform_device s5p_device_fimc3;
 extern struct platform_device s5p_device_mipi_csis0;
 extern struct platform_device s5p_device_mipi_csis1;
 
-extern struct platform_device exynos4_device_sysmmu;
+extern struct platform_device exynos4_device_sysmmu[];

Why is this a global variable? I would expect this to be private to the implementation.

Quoted 6 lines
Quoted 1 line

To allow each board to register only particular instances of sysmmu

controllers.

That sounds like an unnecessarily complicated way of doing it. This would be another reason to not make each one a device, but have something else in struct device take care of it.

Well, having each iommu instantiated as a separate device has some advantages, but I agree that having it automatically registered together with the corresponding multimedia (client) device will simplify a lot of things. Same rules should imho apply to power domain drivers.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#10 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 8 lines · Marek Szyprowski
Quoted 8 lines

1. change the runtime_pm subsystem to allow it to ignore some devices in an easy way.

2. change the device layout if the sysmmu. If the iommu device is a child of the device that it is responsible for, I guess you don't have this problem.

Quoted 5 lines
Quoted 2 lines

3. Not represent the iommu as a device at all, just as a property of another device.

Ok, we will handle this issue somehow. I consider this a minor issue and I would like to focus on the IOMMU/dma-mapping APIs first.

Yes, agreed.

Quoted 8 lines
Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

Quoted 5 lines

Ok. I understand. IOMMU API is quite nice abstraction of the IOMMU chip. dma-mapping API is something much more complex that creates the actual mapping for various sets of the devices. IMHO the right direction will be to create dma-mapping implementation that will be just a client of the IOMMU API. What's your opinion?

Sounds good. I think we should put it into a new drivers/iommu, along with your specific iommu implementation, and then we can convert the existing ones over to use that.

Note that this also requires using dma-mapping-common.h, which we currently don't on ARM.

Quoted 10 lines
Quoted 5 lines

The domain really reflects the user, not the device here, which makes more sense if you think of virtual machines than of multimedia devices.

I would suggest that you just use a single iommu_domain globally for all in-kernel users.

There are cases where having a separate mapping for each device makes sense. It definitely increases the security and helps to find some bugs in the drivers.

Quoted 5 lines

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for allocations for the second IOMMU controller.

Good question.

How do you even decide which controller to use from the driver? I would need to understand better what you are trying to do to give a good recommendation.

Arnd

author maintainer
#11 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 14 lines · Arnd Bergmann
Quoted 8 lines
Quoted 7 lines

That is a limitation of the current implementation. We might want to change that anyway, e.g. to handle the mali IOMMU along with yours. I believe the reason for allowing only one IOMMU type so far has been that nobody required more than one. As I mentioned, the IOMMU API is rather new and has not been ported to much variety of hardware, unlike the dma-mapping API, which does support multiple different IOMMUs in a single system.

Quoted 5 lines

Ok. I understand. IOMMU API is quite nice abstraction of the IOMMU chip. dma-mapping API is something much more complex that creates the actual mapping for various sets of the devices. IMHO the right direction will be to create dma-mapping implementation that will be just a client of the IOMMU API. What's your opinion?

Quoted 3 lines

Sounds good. I think we should put it into a new drivers/iommu, along with your specific iommu implementation, and then we can convert the existing ones over to use that.

I see, this sounds quite reasonable. I think I finally got how this should be implemented.

The only question is how a device can allocate a buffer that will be most convenient for IOMMU mapping (i.e. will require least entries to map)?

IOMMU can create a contiguous mapping for ANY set of pages, but it performs much better if the pages are grouped into 64KiB or 1MiB areas.

Can device allocate a buffer without mapping it into kernel space?

The problem that still left to be resolved is the fact the dma_coherent_alloc() should also be able to use IOMMU. This would however trigger the problem of double mappings with different cache attributes:
dma api might require to create coherent (==non-cached mappings), while all low-memory is still mapped with (super)sections as cached, what is against ARM CPU specification and might cause unpredicted behavior especially on CPUs that do speculative prefetch. Right now this problem has been ignored in dma-mappings implementation, but there have been some patches posted to resolve this by reserving some area exclusively for dma coherent mappings:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100822/focus=100913

Right now I would like to postpone resolving this issue because the Samsung iommu task already became really big.

Quoted 2 lines

Note that this also requires using dma-mapping-common.h, which we currently don't on ARM.

Yes, I noticed this, shouldn't be much problem, imho.

Quoted 16 lines
Quoted 1 line
Quoted 1 line

The domain really reflects the user, not the device here, which makes

more

Quoted 6 lines
Quoted 4 lines

sense if you think of virtual machines than of multimedia devices.

I would suggest that you just use a single iommu_domain globally for all in-kernel users.

There are cases where having a separate mapping for each device makes

sense.

Quoted 7 lines

It definitely increases the security and helps to find some bugs in the drivers.

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for

Quoted 8 lines

allocations

Quoted 1 line

for the second IOMMU controller.

Good question.

How do you even decide which controller to use from the driver? I would need to understand better what you are trying to do to give a good recommendation.

Both controllers are used by the hardware depending on the buffer type. For example, buffers with chroma video data are accessed by first (called 'left') memory channel, the others (with luma video data) - by the second channel (called 'right'). Each memory channel is limited to 256MiB address space and best performance is achieved when buffers are allocated in separate physical memory banks (the boards usually have 2 or more memory banks, memory is not interleaved).

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#12 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 9 lines · Marek Szyprowski
Quoted 3 lines · Arnd Bergmann

Sounds good. I think we should put it into a new drivers/iommu, along with your specific iommu implementation, and then we can convert the existing ones over to use that.

I see, this sounds quite reasonable. I think I finally got how this should be implemented.

Quoted 7 lines

The only question is how a device can allocate a buffer that will be most convenient for IOMMU mapping (i.e. will require least entries to map)?

IOMMU can create a contiguous mapping for ANY set of pages, but it performs much better if the pages are grouped into 64KiB or 1MiB areas.

Can device allocate a buffer without mapping it into kernel space?

Not today as far as I know. You can register coherent memory per device using dma_declare_coherent_memory(), which will be used to back dma_alloc_coherent(), but I believe it is always mapped right now.

This can of course be changed.

Quoted 12 lines

The problem that still left to be resolved is the fact the dma_coherent_alloc() should also be able to use IOMMU. This would however trigger the problem of double mappings with different cache attributes:
dma api might require to create coherent (==non-cached mappings), while all low-memory is still mapped with (super)sections as cached, what is against ARM CPU specification and might cause unpredicted behavior especially on CPUs that do speculative prefetch. Right now this problem has been ignored in dma-mappings implementation, but there have been some patches posted to resolve this by reserving some area exclusively for dma coherent mappings:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/100822/focus=100913

Quoted 2 lines

Right now I would like to postpone resolving this issue because the Samsung iommu task already became really big.

Agreed.

Quoted 13 lines
Quoted 4 lines
Quoted 4 lines

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for

Quoted 8 lines

allocations

Quoted 1 line

for the second IOMMU controller.

Good question.

How do you even decide which controller to use from the driver? I would need to understand better what you are trying to do to give a good recommendation.

Quoted 7 lines

Both controllers are used by the hardware depending on the buffer type. For example, buffers with chroma video data are accessed by first (called 'left') memory channel, the others (with luma video data) - by the second channel (called 'right'). Each memory channel is limited to 256MiB address space and best performance is achieved when buffers are allocated in separate physical memory banks (the boards usually have 2 or more memory banks, memory is not interleaved).

Ok, I see. Having one device per channel as you suggested could probably work around this, and it's at least consistent with how you'd represent IOMMUs in the device tree. It is not ideal because it makes the video driver more complex when it now has to deal with multiple struct device that it binds to, but I can't think of any nicer way either.

Arnd

author maintainer
#13 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 15 lines · Arnd Bergmann
Quoted 7 lines · Marek Szyprowski
Quoted 3 lines · Arnd Bergmann

Sounds good. I think we should put it into a new drivers/iommu, along with your specific iommu implementation, and then we can convert the existing ones over to use that.

I see, this sounds quite reasonable. I think I finally got how this

should

Quoted 6 lines

be implemented.

The only question is how a device can allocate a buffer that will be most convenient for IOMMU mapping (i.e. will require least entries to map)?

IOMMU can create a contiguous mapping for ANY set of pages, but it

Quoted 8 lines

performs

Quoted 3 lines

much better if the pages are grouped into 64KiB or 1MiB areas.

Can device allocate a buffer without mapping it into kernel space?

Not today as far as I know. You can register coherent memory per device using dma_declare_coherent_memory(), which will be used to back dma_alloc_coherent(), but I believe it is always mapped right now.

This is not exactly what I meant.

As we have IOMMU, the device driver can access any system memory. However the performance will be better if the buffer is composed of larger contiguous parts (like 64KiB or 1MiB). I would like to avoid putting logic that manages buffer allocation into the device drivers. It would be best if such buffers could be allocated by a single call to dma-mapping API.

Right now there is dma_alloc_coherent() function, which is used by the drivers to allocate a contiguous block of memory and map it to DMA addresses. With IOMMU implementation it is quite easy to provide a replacement for it that will allocate some set of pages and map into device virtual address space as a contiguous buffer.

This will have the advantage that the same multimedia device driver will work on both systems - Samsung S5PC110 (without IOMMU) and Exynos4 (with IOMMU).

However dma_alloc_coherent() besides allocating memory also implies some particular type of memory mapping for it. IMHO it might be a good idea to separate these 2 things (allocation and mapping) somewhere in the future.

On systems with IOMMU the dma_map_sg() can be also used to create a mapping in device virtual address space, but the driver will still need to allocate the memory by itself.

(snipped)

Quoted 23 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

Getting back to our video codec - it has 2 IOMMU controllers. The

codec

Quoted 1 line
Quoted 1 line
Quoted 1 line

hardware is able to address only 256MiB of space. Do you have an idea

how

Quoted 1 line
Quoted 1 line
Quoted 1 line

this can be handled with dma-mapping API? The only idea that comes to

my

Quoted 6 lines
Quoted 6 lines
Quoted 1 line

mind is to provide a second, fake 'struct device' and use it for

allocations

Quoted 1 line

for the second IOMMU controller.

Good question.

Quoted 8 lines
Quoted 3 lines

How do you even decide which controller to use from the driver? I would need to understand better what you are trying to do to give a good recommendation.

Both controllers are used by the hardware depending on the buffer type. For example, buffers with chroma video data are accessed by first (called 'left') memory channel, the others (with luma video data) - by the second channel (called 'right'). Each memory channel is limited to 256MiB

address

Quoted 2 lines

space and best performance is achieved when buffers are allocated in separate physical memory banks (the boards usually have 2 or more memory

Quoted 8 lines

banks,

Quoted 1 line

memory is not interleaved).

Ok, I see. Having one device per channel as you suggested could probably work around this, and it's at least consistent with how you'd represent IOMMUs in the device tree. It is not ideal because it makes the video driver more complex when it now has to deal with multiple struct device that it binds to, but I can't think of any nicer way either.

Well, this will definitely complicate the codec driver. I wonder if allowing the driver to kmalloc(sizeof(struct device))) and copy the relevant data from the 'proper' struct device will be better idea. It is still hack but definitely less intrusive for the driver.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#14 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 28 lines · Marek Szyprowski
Quoted 4 lines · Arnd Bergmann
Quoted 3 lines · Marek Szyprowski

The only question is how a device can allocate a buffer that will be most convenient for IOMMU mapping (i.e. will require least entries to map)?

Quoted 8 lines
Quoted 4 lines

IOMMU can create a contiguous mapping for ANY set of pages, but it performs much better if the pages are grouped into 64KiB or 1MiB areas.

Can device allocate a buffer without mapping it into kernel space?

Not today as far as I know. You can register coherent memory per device using dma_declare_coherent_memory(), which will be used to back dma_alloc_coherent(), but I believe it is always mapped right now.

This is not exactly what I meant.

As we have IOMMU, the device driver can access any system memory. However the performance will be better if the buffer is composed of larger contiguous parts (like 64KiB or 1MiB). I would like to avoid putting logic that manages buffer allocation into the device drivers. It would be best if such buffers could be allocated by a single call to dma-mapping API.

Right now there is dma_alloc_coherent() function, which is used by the drivers to allocate a contiguous block of memory and map it to DMA addresses. With IOMMU implementation it is quite easy to provide a replacement for it that will allocate some set of pages and map into device virtual address space as a contiguous buffer.

Quoted 3 lines

This will have the advantage that the same multimedia device driver will work on both systems - Samsung S5PC110 (without IOMMU) and Exynos4 (with IOMMU).

Right.

Quoted 7 lines

However dma_alloc_coherent() besides allocating memory also implies some particular type of memory mapping for it. IMHO it might be a good idea to separate these 2 things (allocation and mapping) somewhere in the future.

On systems with IOMMU the dma_map_sg() can be also used to create a mapping in device virtual address space, but the driver will still need to allocate the memory by itself.

Note that dma_map_sg() is the "streaming mapping", which provides a cacheable buffer all the time, while dma_alloc_coherent() and is the "coherent mapping".

There is also dma_alloc_noncoherent(), which you can use to allocate a buffer for the streaming mapping. This is currently not implemented on ARM, but if I understand you correctly, adding this would do what you want.

Quoted 10 lines
Quoted 5 lines

Ok, I see. Having one device per channel as you suggested could probably work around this, and it's at least consistent with how you'd represent IOMMUs in the device tree. It is not ideal because it makes the video driver more complex when it now has to deal with multiple struct device that it binds to, but I can't think of any nicer way either.

Well, this will definitely complicate the codec driver. I wonder if allowing the driver to kmalloc(sizeof(struct device))) and copy the relevant data from the 'proper' struct device will be better idea. It is still hack but definitely less intrusive for the driver.

No, I think that would be much worse, it definitely destroys all kinds of assumptions that the core code makes about devices. However, I don't think it's much of a problem to just create two child devices and use them from the main driver, you don't really need to create a device_driver to bind to each of them.

Arnd

author maintainer
#15 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 49 lines · Arnd Bergmann
Quoted 3 lines · Marek Szyprowski
Quoted 2 lines · Arnd Bergmann
Quoted 1 line · Marek Szyprowski

The only question is how a device can allocate a buffer that will be

most

Quoted 1 line
Quoted 1 line
Quoted 1 line

convenient for IOMMU mapping (i.e. will require least entries to

map)?

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

IOMMU can create a contiguous mapping for ANY set of pages, but it

performs

Quoted 4 lines
Quoted 4 lines
Quoted 3 lines

much better if the pages are grouped into 64KiB or 1MiB areas.

Can device allocate a buffer without mapping it into kernel space?

Quoted 8 lines
Quoted 3 lines

Not today as far as I know. You can register coherent memory per device using dma_declare_coherent_memory(), which will be used to back dma_alloc_coherent(), but I believe it is always mapped right now.

This is not exactly what I meant.

As we have IOMMU, the device driver can access any system memory. However the performance will be better if the buffer is composed of larger

contiguous

Quoted 1 line

parts (like 64KiB or 1MiB). I would like to avoid putting logic that

manages

Quoted 1 line

buffer allocation into the device drivers. It would be best if such

buffers

Quoted 4 lines

could be allocated by a single call to dma-mapping API.

Right now there is dma_alloc_coherent() function, which is used by the drivers to allocate a contiguous block of memory and map it to DMA

addresses.

Quoted 1 line

With IOMMU implementation it is quite easy to provide a replacement for

it

Quoted 6 lines

that will allocate some set of pages and map into device virtual address space as a contiguous buffer.

This will have the advantage that the same multimedia device driver will work on both systems - Samsung S5PC110 (without IOMMU) and Exynos4 (with IOMMU).

Right.

Quoted 5 lines

However dma_alloc_coherent() besides allocating memory also implies some particular type of memory mapping for it. IMHO it might be a good idea to separate these 2 things (allocation and mapping) somewhere in the future.

On systems with IOMMU the dma_map_sg() can be also used to create a

mapping

Quoted 8 lines
Quoted 1 line

in device virtual address space, but the driver will still need to

allocate

Quoted 1 line

the memory by itself.

Note that dma_map_sg() is the "streaming mapping", which provides a cacheable
buffer all the time, while dma_alloc_coherent() and is the "coherent mapping".

Ok.

Quoted 4 lines

There is also dma_alloc_noncoherent(), which you can use to allocate a buffer
for the streaming mapping. This is currently not implemented on ARM, but if I understand you correctly, adding this would do what you want.

Ok, I got it. Implementing dma_alloc_noncoherent() as well as dma_map_sg() for non-IOMMU cases also makes some sense and will simplify the drivers imho.

Quoted 13 lines
Quoted 1 line
Quoted 1 line

Ok, I see. Having one device per channel as you suggested could

probably

Quoted 6 lines
Quoted 4 lines

work around this, and it's at least consistent with how you'd represent IOMMUs in the device tree. It is not ideal because it makes the video driver more complex when it now has to deal with multiple struct device that it binds to, but I can't think of any nicer way either.

Well, this will definitely complicate the codec driver. I wonder if

allowing

Quoted 3 lines

the driver to kmalloc(sizeof(struct device))) and copy the relevant data from the 'proper' struct device will be better idea. It is still hack but definitely less intrusive for the driver.

Quoted 5 lines

No, I think that would be much worse, it definitely destroys all kinds of assumptions that the core code makes about devices. However, I don't think it's much of a problem to just create two child devices and use them from the main driver, you don't really need to create a device_driver to bind to each of them.

I must have missed something. Video codec is a platform device and struct device pointer is gathered from it (&pdev->dev). How can I define child devices and attach them to the platform device?

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#16 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 9 lines · Marek Szyprowski
Quoted 5 lines

No, I think that would be much worse, it definitely destroys all kinds of assumptions that the core code makes about devices. However, I don't think it's much of a problem to just create two child devices and use them from the main driver, you don't really need to create a device_driver to bind to each of them.

I must have missed something. Video codec is a platform device and struct device pointer is gathered from it (&pdev->dev). How can I define child devices and attach them to the platform device?

There are a number of ways:

* Do device_create() with &pdev->dev as the parent, inside of the codec driver, with a new class you create for this purpose
* Do device_register() for a device, in the same way
* Create the additional platform devices in the platform code, with their parents pointing to the code device, then look for them using device_for_each_child in the driver
* Create two codec devices in parallel and bind to both with your driver, ideally splitting up the resources between the two devices in a meaningful way.

None of them are extremely nice, but it's not that hard either. You should probably prototype a few of these approaches to see which one is the least ugly one.

Arnd

author maintainer
#17 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 13 lines · Arnd Bergmann
Quoted 1 line · Marek Szyprowski
Quoted 1 line

No, I think that would be much worse, it definitely destroys all kinds

of

Quoted 1 line
Quoted 1 line

assumptions that the core code makes about devices. However, I don't

think

Quoted 7 lines
Quoted 3 lines

it's much of a problem to just create two child devices and use them from the main driver, you don't really need to create a device_driver to bind to each of them.

I must have missed something. Video codec is a platform device and struct device pointer is gathered from it (&pdev->dev). How can I define child devices and attach them to the platform device?

Quoted 8 lines

There are a number of ways:

* Do device_create() with &pdev->dev as the parent, inside of the codec driver, with a new class you create for this purpose
* Do device_register() for a device, in the same way
* Create the additional platform devices in the platform code, with their parents pointing to the code device, then look for them using device_for_each_child in the driver

IMHO this will be the cleanest way. Thanks for the idea.

Quoted 3 lines

* Create two codec devices in parallel and bind to both with your driver, ideally splitting up the resources between the two devices in a meaningful way.

Video codec has only standard 2 resources - ioregs and irq, so there is not much left for such splitting.

Quoted 3 lines

None of them are extremely nice, but it's not that hard either. You should probably prototype a few of these approaches to see which one is the least ugly one.

Ok. Today while iterating over the hardware requirements I noticed one more thing. Our codec hardware has one more, odd requirement for video buffers. The DMA addresses need to be aligned to 8KiB or 16KiB (depending on buffer type). Do you have any idea how this can be handled in a generic way?

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#18 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 6 lines · Marek Szyprowski
Quoted 3 lines

* Create two codec devices in parallel and bind to both with your driver, ideally splitting up the resources between the two devices in a meaningful way.

Video codec has only standard 2 resources - ioregs and irq, so there is not much left for such splitting.

Ok, I see.

Quoted 9 lines
Quoted 3 lines

None of them are extremely nice, but it's not that hard either. You should probably prototype a few of these approaches to see which one is the least ugly one.

Ok. Today while iterating over the hardware requirements I noticed one more thing. Our codec hardware has one more, odd requirement for video buffers. The DMA addresses need to be aligned to 8KiB or 16KiB (depending on buffer type). Do you have any idea how this can be handled in a generic way?

I don't think you can force the mappings to be aligned to that size in the streaming mapping, but you should be able to just align inside of dma_map_single etc and map a larger region.

For the allocation functions (dma_alloc_coherent, dma_alloc_noncoherent), using alloc_pages to allocate multiples of the size you need should always give you aligned buffers because of the way that the underlying buddy allocator works.

Arnd

author maintainer
#19 lore raw

PATCH v3 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Hello,

Quoted 20 lines · Arnd Bergmann
Quoted 6 lines · Marek Szyprowski
Quoted 3 lines

* Create two codec devices in parallel and bind to both with your driver, ideally splitting up the resources between the two devices in a meaningful way.

Video codec has only standard 2 resources - ioregs and irq, so there is not much left for such splitting.

Ok, I see.

Quoted 9 lines
Quoted 3 lines

None of them are extremely nice, but it's not that hard either. You should probably prototype a few of these approaches to see which one is the least ugly one.

Ok. Today while iterating over the hardware requirements I noticed one more thing. Our codec hardware has one more, odd requirement for video buffers. The DMA addresses need to be aligned to 8KiB or 16KiB (depending on buffer type). Do you have any idea how this can be handled in a generic way?

Quoted 8 lines

I don't think you can force the mappings to be aligned to that size in the streaming mapping, but you should be able to just align inside of dma_map_single etc and map a larger region.

For the allocation functions (dma_alloc_coherent, dma_alloc_noncoherent), using alloc_pages to allocate multiples of the size you need should always give you aligned buffers because of the way that the underlying buddy allocator works.

Well, I thought about the alignment of the IOVA mapping. I will probably handle it with some additional archdata stuff.

I've started hacking ARM dma-mapping interface to get support for dma-mapping-common.h and then to integrate with Samsung IOMMU driver. I hope to post the initial version before Linaro meeting in Budapest.

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#20 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

In reply to Marek Szyprowski
Quoted 5 lines · Marek Szyprowski

Ok. I understand. IOMMU API is quite nice abstraction of the IOMMU chip. dma-mapping API is something much more complex that creates the actual mapping for various sets of the devices. IMHO the right direction will be to create dma-mapping implementation that will be just a client of the IOMMU API. What's your opinion?

Definitly agreed. I plan this since some time but never found the time to implement it. In the end we can have a generic dma-ops implementation that works for all iommu-api implementations.

Quoted 5 lines

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for allocations for the second IOMMU controller.

The GPU IOMMUs can probably be handled in the GPU driver if they are that different. Recent PCIe GPUs on x86 have their own IOMMUs too which are very device specific and are handled in the device driver.

Regards,

Joerg

Signature · 5 lines
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
#21 lore raw

PATCH 2/7 ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver

Quoted 9 lines · Joerg
Quoted 5 lines

Getting back to our video codec - it has 2 IOMMU controllers. The codec hardware is able to address only 256MiB of space. Do you have an idea how this can be handled with dma-mapping API? The only idea that comes to my mind is to provide a second, fake 'struct device' and use it for allocations for the second IOMMU controller.

The GPU IOMMUs can probably be handled in the GPU driver if they are that different. Recent PCIe GPUs on x86 have their own IOMMUs too which are very device specific and are handled in the device driver.

I tend to disagree with this one, and would suggest that the GPUs should actually provide their own iommu_ops, even if they are the only users of these.

However, this is a minor point that we don't need to worry about today.

Arnd

author maintainer
#22 lore raw

PATCH v3 3/7 v4l: videobuf2: dma-sg: move some generic functions to memops

In reply to Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch moves some generic code to videobuf2-memops. This code will be later used by the iommu allocator. This patch adds also vma locking in user pointer mode.

---
 drivers/media/video/videobuf2-dma-sg.c |   37 +++++----------
 drivers/media/video/videobuf2-memops.c |   76 ++++++++++++++++++++++++++++++++
 include/media/videobuf2-memops.h       |    5 ++
 3 files changed, 93 insertions(+), 25 deletions(-)
drivers/media/video/videobuf2-dma-sg.c +12 -25
index b2d9485..240abaa 100644
@@ -29,6 +29,7 @@ struct vb2_dma_sg_buf {
 	struct vb2_dma_sg_desc		sg_desc;
 	atomic_t			refcount;
 	struct vb2_vmarea_handler	handler;
+	struct vm_area_struct		*vma;
 };
 
 static void vb2_dma_sg_put(void *buf_priv);
@@ -150,15 +151,9 @@ static void *vb2_dma_sg_get_userptr(void *alloc_ctx, unsigned long vaddr,
 	if (!buf->pages)
 		goto userptr_fail_pages_array_alloc;
 
-	down_read(&current->mm->mmap_sem);
-	num_pages_from_user = get_user_pages(current, current->mm,
-					     vaddr & PAGE_MASK,
-					     buf->sg_desc.num_pages,
-					     write,
-					     1, /* force */
-					     buf->pages,
-					     NULL);
-	up_read(&current->mm->mmap_sem);
+	num_pages_from_user = vb2_get_user_pages(vaddr, buf->sg_desc.num_pages,
+					     buf->pages, write, &buf->vma);
+
 	if (num_pages_from_user != buf->sg_desc.num_pages)
 		goto userptr_fail_get_user_pages;
 
@@ -177,6 +172,8 @@ userptr_fail_get_user_pages:
 	       num_pages_from_user, buf->sg_desc.num_pages);
 	while (--num_pages_from_user >= 0)
 		put_page(buf->pages[num_pages_from_user]);
+	if (buf->vma)
+		vb2_put_vma(buf->vma);
 	kfree(buf->pages);
 
 userptr_fail_pages_array_alloc:
@@ -200,6 +197,8 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
 	       __func__, buf->sg_desc.num_pages);
 	if (buf->vaddr)
 		vm_unmap_ram(buf->vaddr, buf->sg_desc.num_pages);
+	if (buf->vma)
+		vb2_put_vma(buf->vma);
 	while (--i >= 0) {
 		if (buf->write)
 			set_page_dirty_lock(buf->pages[i]);
@@ -236,28 +235,16 @@ static unsigned int vb2_dma_sg_num_users(void *buf_priv)
 static int vb2_dma_sg_mmap(void *buf_priv, struct vm_area_struct *vma)
 {
 	struct vb2_dma_sg_buf *buf = buf_priv;
-	unsigned long uaddr = vma->vm_start;
-	unsigned long usize = vma->vm_end - vma->vm_start;
-	int i = 0;
+	int ret;
 
 	if (!buf) {
 		printk(KERN_ERR "No memory to map\n");
 		return -EINVAL;
 	}
 
-	do {
-		int ret;
-
-		ret = vm_insert_page(vma, uaddr, buf->pages[i++]);
-		if (ret) {
-			printk(KERN_ERR "Remapping memory, error: %d\n", ret);
-			return ret;
-		}
-
-		uaddr += PAGE_SIZE;
-		usize -= PAGE_SIZE;
-	} while (usize > 0);
-
+	ret = vb2_insert_pages(vma, buf->pages);
+	if (ret)
+		return ret;
 
 	/*
 	 * Use common vm_area operations to track buffer refcount.
drivers/media/video/videobuf2-memops.c +76 -0

82 lines not on this page · raw message

include/media/videobuf2-memops.h +5 -0
index 84e1f6c..f8a0886 100644
@@ -41,5 +41,10 @@ int vb2_mmap_pfn_range(struct vm_area_struct *vma, unsigned long paddr,
 struct vm_area_struct *vb2_get_vma(struct vm_area_struct *vma);
 void vb2_put_vma(struct vm_area_struct *vma);
 
+int vb2_get_user_pages(unsigned long vaddr, unsigned int num_pages,
+		       struct page **pages, int write,
+		       struct vm_area_struct **vma);
+
+int vb2_insert_pages(struct vm_area_struct *vma, struct page **pages);
 
 #endif
Signature · 1 line
1.7.1.569.g6f426
author maintainer
#23 lore raw

PATCH v3 4/7 v4l: videobuf2: add IOMMU based DMA memory allocator

In reply to Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch adds new videobuf2 memory allocator dedicated to devices that supports IOMMU DMA mappings. A device with IOMMU module and a driver with include/iommu.h compatible interface is required. This allocator aquires memory with standard alloc_page() call and doesn't suffer from memory fragmentation issues. The allocator support following page sizes:
4KiB, 64KiB, 1MiB and 16MiB to reduce iommu translation overhead.

---
 drivers/media/video/Kconfig               |    8 +-
 drivers/media/video/Makefile              |    1 +
 drivers/media/video/videobuf2-dma-iommu.c |  762 +++++++++++++++++++++++++++++
 include/media/videobuf2-dma-iommu.h       |   48 ++
 4 files changed, 818 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/video/videobuf2-dma-iommu.c
 create mode 100644 include/media/videobuf2-dma-iommu.h
drivers/media/video/Kconfig +7 -1
index 4498b94..40d7bcc 100644
@@ -60,12 +60,18 @@ config VIDEOBUF2_VMALLOC
 	select VIDEOBUF2_MEMOPS
 	tristate
 
-
 config VIDEOBUF2_DMA_SG
 	#depends on HAS_DMA
 	select VIDEOBUF2_CORE
 	select VIDEOBUF2_MEMOPS
 	tristate
+
+config VIDEOBUF2_DMA_IOMMU
+	select GENERIC_ALLOCATOR
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_MEMOPS
+	tristate
+
 #
 # Multimedia Video device configuration
 #
drivers/media/video/Makefile +1 -0
index ace5d8b..04136f6 100644
@@ -118,6 +118,7 @@ obj-$(CONFIG_VIDEOBUF2_MEMOPS)		+= videobuf2-memops.o
 obj-$(CONFIG_VIDEOBUF2_VMALLOC)		+= videobuf2-vmalloc.o
 obj-$(CONFIG_VIDEOBUF2_DMA_CONTIG)	+= videobuf2-dma-contig.o
 obj-$(CONFIG_VIDEOBUF2_DMA_SG)		+= videobuf2-dma-sg.o
+obj-$(CONFIG_VIDEOBUF2_DMA_IOMMU)	+= videobuf2-dma-iommu.o
 
 obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
 
drivers/media/video/videobuf2-dma-iommu.c new file +762 -0

762 lines not on this page · raw message

include/media/videobuf2-dma-iommu.h new file +48 -0

48 lines not on this page · raw message

Signature · 1 line
1.7.1.569.g6f426
#24 lore raw

PATCH 4/7 v4l: videobuf2: add IOMMU based DMA memory allocator

Quoted 8 lines · Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch adds new videobuf2 memory allocator dedicated to devices that supports IOMMU DMA mappings. A device with IOMMU module and a driver with include/iommu.h compatible interface is required. This allocator aquires memory with standard alloc_page() call and doesn't suffer from memory fragmentation issues. The allocator support following page sizes:
4KiB, 64KiB, 1MiB and 16MiB to reduce iommu translation overhead.

My feeling is that this is not the right abstraction. Why can't you just implement the regular dma-mapping.h interfaces for your IOMMU so that the videobuf code can use the existing allocators?

Arnd

author maintainer
#25 lore raw

PATCH v3 4/7 v4l: videobuf2: add IOMMU based DMA memory allocator

Hello,

Quoted 10 lines · Arnd Bergmann
Quoted 8 lines · Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch adds new videobuf2 memory allocator dedicated to devices that supports IOMMU DMA mappings. A device with IOMMU module and a driver with include/iommu.h compatible interface is required. This allocator aquires memory with standard alloc_page() call and doesn't suffer from memory fragmentation issues. The allocator support following page sizes:
4KiB, 64KiB, 1MiB and 16MiB to reduce iommu translation overhead.

Quoted 3 lines

My feeling is that this is not the right abstraction. Why can't you just implement the regular dma-mapping.h interfaces for your IOMMU so that the videobuf code can use the existing allocators?

I'm not really sure which existing videobuf2 allocators might transparently support IOMMU interface yet

Do you think that all iommu operations can be hidden behind dma_map_single and dma_unmap_single?

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center
#26 lore raw

PATCH 4/7 v4l: videobuf2: add IOMMU based DMA memory allocator

Quoted 10 lines · Marek Szyprowski
Quoted 3 lines · Arnd Bergmann

My feeling is that this is not the right abstraction. Why can't you just implement the regular dma-mapping.h interfaces for your IOMMU so that the videobuf code can use the existing allocators?

I'm not really sure which existing videobuf2 allocators might transparently support IOMMU interface yet

Do you think that all iommu operations can be hidden behind dma_map_single and dma_unmap_single?

That is one of the intentions of the DMA API.

#27 lore raw

PATCH 4/7 v4l: videobuf2: add IOMMU based DMA memory allocator

Quoted 6 lines · Russell King - ARM Linux
Quoted 5 lines · Marek Szyprowski
Quoted 3 lines · Arnd Bergmann

My feeling is that this is not the right abstraction. Why can't you just implement the regular dma-mapping.h interfaces for your IOMMU so that the videobuf code can use the existing allocators?

Quoted 7 lines
Quoted 5 lines

I'm not really sure which existing videobuf2 allocators might transparently support IOMMU interface yet

Do you think that all iommu operations can be hidden behind dma_map_single and dma_unmap_single?

That is one of the intentions of the DMA API.

Exactly.

All architectures that support IOMMUs today do that, see:

arch/alpha/kernel/pci_iommu.c
arch/ia64/hp/common/sba_iommu.c
arch/powerpc/kernel/dma-iommu.c
arch/sparc/kernel/iommu.c
arch/x86/kernel/amd_iommu.c

ARM would be the first one to combine an IOMMU with potentially noncoherent DMA, but there is no fundamental reason why we shouldn't be able to transparently support an IOMMU.

Ideally, I think we should first find an architecture-independent way to define an IOMMU in one place instead of having to do both the iommu.h and dma-mapping.h interfaces, but I wouldn't require Samsung to do that in order to support their IOMMU. Doing support for the dma-mapping.h interface should be sufficient there.

Arnd

author maintainer
#28 lore raw

PATCH v3 5/7 v4l: s5p-fimc: add pm_runtime support

In reply to Marek Szyprowski

This patch adds basic support for pm_runtime to s5p-fimc driver. PM runtime support is required to enable the driver on S5PV310 series with power domain driver enabled.

---
 drivers/media/video/s5p-fimc/fimc-capture.c |    5 +++++
 drivers/media/video/s5p-fimc/fimc-core.c    |   14 ++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
drivers/media/video/s5p-fimc/fimc-capture.c +5 -0
index 95f8b4e1..f697ed1 100644
@@ -18,6 +18,7 @@
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/list.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
@@ -398,6 +399,8 @@ static int fimc_capture_open(struct file *file)
 	if (fimc_m2m_active(fimc))
 		return -EBUSY;
 
+	pm_runtime_get_sync(&fimc->pdev->dev);
+
 	if (++fimc->vid_cap.refcnt == 1) {
 		ret = fimc_isp_subdev_init(fimc, 0);
 		if (ret) {
@@ -428,6 +431,8 @@ static int fimc_capture_close(struct file *file)
 		fimc_subdev_unregister(fimc);
 	}
 
+	pm_runtime_put_sync(&fimc->pdev->dev);
+
 	return 0;
 }
 
drivers/media/video/s5p-fimc/fimc-core.c +14 -0
index 6c919b3..ead5c0a 100644
@@ -20,6 +20,7 @@
 #include <linux/interrupt.h>
 #include <linux/device.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/list.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -1410,6 +1411,8 @@ static int fimc_m2m_open(struct file *file)
 	if (fimc->vid_cap.refcnt > 0)
 		return -EBUSY;
 
+	pm_runtime_get_sync(&fimc->pdev->dev);
+
 	fimc->m2m.refcnt++;
 	set_bit(ST_OUTDMA_RUN, &fimc->state);
 
@@ -1452,6 +1455,8 @@ static int fimc_m2m_release(struct file *file)
 	if (--fimc->m2m.refcnt <= 0)
 		clear_bit(ST_OUTDMA_RUN, &fimc->state);
 
+	pm_runtime_put_sync(&fimc->pdev->dev);
+
 	return 0;
 }
 
@@ -1649,6 +1654,11 @@ static int fimc_probe(struct platform_device *pdev)
 		goto err_req_region;
 	}
 
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	pm_runtime_get_sync(&pdev->dev);
+
 	fimc->num_clocks = MAX_FIMC_CLOCKS - 1;
 
 	/* Check if a video capture node needs to be registered. */
@@ -1706,6 +1716,8 @@ static int fimc_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "%s(): fimc-%d registered successfully\n",
 		__func__, fimc->id);
 
+	pm_runtime_put_sync(&pdev->dev);
+
 	return 0;
 
 err_m2m:
@@ -1740,6 +1752,8 @@ static int __devexit fimc_remove(struct platform_device *pdev)
 
 	vb2_dma_contig_cleanup_ctx(fimc->alloc_ctx);
 
+	pm_runtime_disable(&pdev->dev);
+
 	iounmap(fimc->regs);
 	release_resource(fimc->regs_res);
 	kfree(fimc->regs_res);
Signature · 1 line
1.7.1.569.g6f426
author maintainer
#29 lore raw

PATCH v3 6/7 v4l: s5p-fimc: Add support for vb2-dma-iommu allocator

In reply to Marek Szyprowski

This patch adds support for videobuf2-dma-iommu allocator to s5p-fimc driver. This allocator is selected only on systems that contains support for S5P SYSMMU module (like EXYNOS4 platform). Otherwise the standard videobuf2-dma-contig is used.

---
 drivers/media/video/Kconfig                 |    3 +-
 drivers/media/video/s5p-fimc/fimc-capture.c |    4 +-
 drivers/media/video/s5p-fimc/fimc-core.c    |   24 ++++---
 drivers/media/video/s5p-fimc/fimc-core.h    |    1 +
 drivers/media/video/s5p-fimc/fimc-mem.h     |  104 +++++++++++++++++++++++++++
 5 files changed, 123 insertions(+), 13 deletions(-)
 create mode 100644 drivers/media/video/s5p-fimc/fimc-mem.h
drivers/media/video/Kconfig +2 -1
index 40d7bcc..bf2d55d 100644
@@ -1031,7 +1031,8 @@ config VIDEO_MEM2MEM_TESTDEV
 config  VIDEO_SAMSUNG_S5P_FIMC
 	tristate "Samsung S5P FIMC (video postprocessor) driver"
 	depends on VIDEO_DEV && VIDEO_V4L2 && PLAT_S5P
-	select VIDEOBUF2_DMA_CONTIG
+	select VIDEOBUF2_DMA_IOMMU if S5P_SYSTEM_MMU
+	select VIDEOBUF2_DMA_CONTIG if !S5P_SYSTEM_MMU
 	select V4L2_MEM2MEM_DEV
 	help
 	  This is a v4l2 driver for the S5P camera interface
drivers/media/video/s5p-fimc/fimc-capture.c +2 -2
index f697ed1..714f0df 100644
@@ -29,9 +29,9 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/videobuf2-core.h>
-#include <media/videobuf2-dma-contig.h>
 
 #include "fimc-core.h"
+#include "fimc-mem.h"
 
 static struct v4l2_subdev *fimc_subdev_register(struct fimc_dev *fimc,
 					    struct s5p_fimc_isp_info *isp_info)
@@ -884,7 +884,7 @@ int fimc_register_capture_device(struct fimc_dev *fimc)
 	q->io_modes = VB2_MMAP | VB2_USERPTR;
 	q->drv_priv = fimc->vid_cap.ctx;
 	q->ops = &fimc_capture_qops;
-	q->mem_ops = &vb2_dma_contig_memops;
+	q->mem_ops = &fimc_vb2_allocator_memops;
 	q->buf_struct_size = sizeof(struct fimc_vid_buffer);
 
 	vb2_queue_init(q);
drivers/media/video/s5p-fimc/fimc-core.c +14 -10
index ead5c0a..594c471 100644
@@ -27,9 +27,9 @@
 #include <linux/clk.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-core.h>
-#include <media/videobuf2-dma-contig.h>
 
 #include "fimc-core.h"
+#include "fimc-mem.h"
 
 static char *fimc_clocks[MAX_FIMC_CLOCKS] = {
 	"sclk_fimc", "fimc", "sclk_cam"
@@ -457,7 +457,7 @@ int fimc_prepare_addr(struct fimc_ctx *ctx, struct vb2_buffer *vb,
 	dbg("memplanes= %d, colplanes= %d, pix_size= %d",
 		frame->fmt->memplanes, frame->fmt->colplanes, pix_size);
 
-	paddr->y = vb2_dma_contig_plane_paddr(vb, 0);
+	paddr->y = fimc_vb2_plane_addr(vb, 0);
 
 	if (frame->fmt->memplanes == 1) {
 		switch (frame->fmt->colplanes) {
@@ -485,10 +485,10 @@ int fimc_prepare_addr(struct fimc_ctx *ctx, struct vb2_buffer *vb,
 		}
 	} else {
 		if (frame->fmt->memplanes >= 2)
-			paddr->cb = vb2_dma_contig_plane_paddr(vb, 1);
+			paddr->cb = fimc_vb2_plane_addr(vb, 1);
 
 		if (frame->fmt->memplanes == 3)
-			paddr->cr = vb2_dma_contig_plane_paddr(vb, 2);
+			paddr->cr = fimc_vb2_plane_addr(vb, 2);
 	}
 
 	dbg("PHYS_ADDR: y= 0x%X  cb= 0x%X cr= 0x%X ret= %d",
@@ -1378,7 +1378,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->io_modes = VB2_MMAP | VB2_USERPTR;
 	src_vq->drv_priv = ctx;
 	src_vq->ops = &fimc_qops;
-	src_vq->mem_ops = &vb2_dma_contig_memops;
+	src_vq->mem_ops = &fimc_vb2_allocator_memops;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
 	ret = vb2_queue_init(src_vq);
@@ -1390,7 +1390,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->io_modes = VB2_MMAP | VB2_USERPTR;
 	dst_vq->drv_priv = ctx;
 	dst_vq->ops = &fimc_qops;
-	dst_vq->mem_ops = &vb2_dma_contig_memops;
+	dst_vq->mem_ops = &fimc_vb2_allocator_memops;
 	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 
 	return vb2_queue_init(dst_vq);
@@ -1688,12 +1688,15 @@ static int fimc_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
-	/* Initialize contiguous memory allocator */
-	fimc->alloc_ctx = vb2_dma_contig_init_ctx(&fimc->pdev->dev);
+	/* Initialize memory allocator */
+	fimc->alloc_ctx = fimc_vb2_allocator_init(pdev, fimc);
 	if (IS_ERR(fimc->alloc_ctx)) {
 		ret = PTR_ERR(fimc->alloc_ctx);
 		goto err_irq;
 	}
+	ret = fimc_vb2_allocator_enable(fimc->alloc_ctx);
+	if (ret)
+		goto err_irq;
 
 	ret = fimc_register_m2m_device(fimc);
 	if (ret)
@@ -1750,7 +1753,8 @@ static int __devexit fimc_remove(struct platform_device *pdev)
 
 	fimc_clk_release(fimc);
 
-	vb2_dma_contig_cleanup_ctx(fimc->alloc_ctx);
+	fimc_vb2_allocator_disable(fimc->alloc_ctx);
+	fimc_vb2_allocator_cleanup(fimc->alloc_ctx, fimc);
 
 	pm_runtime_disable(&pdev->dev);
 
@@ -1907,7 +1911,7 @@ static struct platform_device_id fimc_driver_ids[] = {
 		.name		= "s5pv210-fimc",
 		.driver_data	= (unsigned long)&fimc_drvdata_s5pv210,
 	}, {
-		.name		= "s5pv310-fimc",
+		.name		= "exynos4-fimc",
 		.driver_data	= (unsigned long)&fimc_drvdata_s5pv310,
 	},
 	{},
drivers/media/video/s5p-fimc/fimc-core.h +1 -0
index 3beb1e5..0f23547 100644
@@ -423,6 +423,7 @@ struct fimc_dev {
 	struct fimc_vid_cap		vid_cap;
 	unsigned long			state;
 	struct vb2_alloc_ctx		*alloc_ctx;
+	struct device			*iommu_dev;
 };
 
 /**
drivers/media/video/s5p-fimc/fimc-mem.h new file +104 -0

104 lines not on this page · raw message

Signature · 1 line
1.7.1.569.g6f426
author maintainer
#30 lore raw

PATCH v3 7/7 ARM: EXYNOS4: enable FIMC on Universal_C210

In reply to Marek Szyprowski

This patch adds definitions to enable support for s5p-fimc driver together with required power domains and sysmmu controller on Universal C210 board.

---
 arch/arm/mach-exynos4/Kconfig               |    6 ++++++
 arch/arm/mach-exynos4/mach-universal_c210.c |   22 ++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
arch/arm/mach-exynos4/Kconfig +6 -0
index e849f67..544a594 100644
@@ -148,12 +148,18 @@ config MACH_ARMLEX4210
 config MACH_UNIVERSAL_C210
 	bool "Mobile UNIVERSAL_C210 Board"
 	select CPU_EXYNOS4210
+	select S5P_DEV_FIMC0
+	select S5P_DEV_FIMC1
+	select S5P_DEV_FIMC2
+	select S5P_DEV_FIMC3
 	select S3C_DEV_HSMMC
 	select S3C_DEV_HSMMC2
 	select S3C_DEV_HSMMC3
 	select S3C_DEV_I2C1
 	select S3C_DEV_I2C5
 	select S5P_DEV_ONENAND
+	select EXYNOS4_DEV_PD
+	select EXYNOS4_DEV_SYSMMU
 	select EXYNOS4_SETUP_I2C1
 	select EXYNOS4_SETUP_I2C5
 	select EXYNOS4_SETUP_SDHCI
arch/arm/mach-exynos4/mach-universal_c210.c +22 -0
index 97d329f..7ff2f5f 100644
@@ -27,9 +27,12 @@
 #include <plat/cpu.h>
 #include <plat/devs.h>
 #include <plat/iic.h>
+#include <plat/pd.h>
 #include <plat/sdhci.h>
+#include <plat/sysmmu.h>
 
 #include <mach/map.h>
+#include <mach/regs-clock.h>
 
 /* Following are default values for UCON, ULCON and UFCON UART registers */
 #define UNIVERSAL_UCON_DEFAULT	(S3C2410_UCON_TXILEVEL |	\
@@ -613,6 +616,15 @@ static struct platform_device *universal_devices[] __initdata = {
 	&s3c_device_hsmmc2,
 	&s3c_device_hsmmc3,
 	&s3c_device_i2c5,
+	&s5p_device_fimc0,
+	&s5p_device_fimc1,
+	&s5p_device_fimc2,
+	&s5p_device_fimc3,
+	&exynos4_device_pd[PD_CAM],
+	&exynos4_device_sysmmu[S5P_SYSMMU_FIMC0],
+	&exynos4_device_sysmmu[S5P_SYSMMU_FIMC1],
+	&exynos4_device_sysmmu[S5P_SYSMMU_FIMC2],
+	&exynos4_device_sysmmu[S5P_SYSMMU_FIMC3],
 
 	/* Universal Devices */
 	&universal_gpio_keys,
@@ -638,6 +650,16 @@ static void __init universal_machine_init(void)
 
 	/* Last */
 	platform_add_devices(universal_devices, ARRAY_SIZE(universal_devices));
+
+	s5p_device_fimc0.dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	s5p_device_fimc1.dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	s5p_device_fimc2.dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	s5p_device_fimc3.dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	exynos4_device_sysmmu[S5P_SYSMMU_FIMC0].dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	exynos4_device_sysmmu[S5P_SYSMMU_FIMC1].dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	exynos4_device_sysmmu[S5P_SYSMMU_FIMC2].dev.parent = &exynos4_device_pd[PD_CAM].dev;
+	exynos4_device_sysmmu[S5P_SYSMMU_FIMC3].dev.parent = &exynos4_device_pd[PD_CAM].dev;
+
 }
 
 MACHINE_START(UNIVERSAL_C210, "UNIVERSAL_C210")
Signature · 1 line
1.7.1.569.g6f426
author maintainer
#31 lore raw

v3 cover Samsung IOMMU videobuf2 allocator and s5p-fimc update

In reply to Marek Szyprowski

Hello,

Quoted 5 lines · Marek Szyprowski

This is a third version of the Samsung IOMMU driver (see patch #2) and videobuf2 allocator for IOMMU mapped memory (see patch #4) as well as FIMC driver update. This update brings some minor bugfixes to Samsung IOMMU (SYSMMU) driver and support for pages larger than 4KiB in videobuf2-dma-iommu allocator.

snip

Quoted 11 lines

This patch series contains a collection of patches for various platform subsystems. Here is a detailed list:

[PATCH 1/7] ARM: EXYNOS4: power domains: fixes and code cleanup
- adds support for block gating in Samsung power domain driver and performs some cleanup

[PATCH 2/7] ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver
- a complete rewrite of sysmmu driver for Samsung platform, now uses linux/include/iommu.h api (key patch in this series)

Quoted 6 lines

[PATCH 3/7] v4l: videobuf2: dma-sg: move some generic functions to memops
- a little cleanup and preparations for the dma-iommu allocator

[PATCH 4/7] v4l: videobuf2: add IOMMU based DMA memory allocator
- introduces new memory allocator for videobuf2 for drivers that support iommu dma memory mappings (key patch in this series)

I was in a bit hurry and I forgot to mention that the above patch relies on some improvements to gen_alloc framework. The required 2 patches can be found in the following patch series:
https://lkml.org/lkml/2011/3/31/213

"[PATCH 01/12] lib: bitmap: Added alignment offset for bitmap_find_next_zero_area()"
https://lkml.org/lkml/2011/3/31/211

"[PATCH 02/12] lib: genalloc: Generic allocator improvements"
https://lkml.org/lkml/2011/3/31/207

For easier testing I've uploaded the whole IOMMU patch series and prerequisites to public GIT repository to vb2-iommu branch (will be available in a about 2 hours):

http://git.infradead.org/users/kmpark/linux-2.6-samsung/shortlog/refs/heads/vb2-
iommu

git://git.infradead.org/users/kmpark/linux-2.6-samsung vb2-iommu

Best regards

Signature · 2 lines
Marek Szyprowski
Samsung Poland R&D Center