Samsung IOMMU videobuf2 allocator and s5p-fimc update

samsung/include/plat/fimc-core.h
index 81a3bfe..945a99d 100644

arch/arm/plat-samsung/include/plat/fimc-core.h +0 -0

@@ -38,6 +38,11 @@ static inline void s3c_fimc_setname(int id, char *name)
 		s5p_device_fimc2.name = name;
 		break;

From: Andrzej Pietrasiewicz <[email protected]>

This patch performs a complete rewrite of sysmmu driver for Samsung platform:
- the new version introduces an api to construct device private page tables and enables to use device private address space mode
- 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
- added support for sysmmu clocks
- some other minor API chages required by upcoming videobuf2 allocator

Signed-off-by Andrzej Pietrasiewicz <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Signed-off-by Marek Szyprowski <[email protected]>

Quoted 11 lines · Marek Szyprowski

From: Andrzej Pietrasiewicz <[email protected]>

This patch performs a complete rewrite of sysmmu driver for Samsung platform:
- the new version introduces an api to construct device private page tables and enables to use device private address space mode
- 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
- added support for sysmmu clocks
- some other minor API chages required by upcoming videobuf2 allocator

Quoted 3 lines

Signed-off-by Andrzej Pietrasiewicz <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

Signed-off-by Marek Szyprowski <[email protected]>

Please explain why create a new IOMMU API when we already have two generic ones (include/linux/iommu.h and include/linux/dma-mapping.h).

Is there something that cannot be done with the common code? The first approach should be to extend the existing APIs to do what you need.

We followed the style of iommu API for other mainline ARM platforms (both OMAP and MSM also have custom API for their iommu modules). I've briefly checked include/linux/iommu.h API and I've noticed that it has been designed mainly for KVM support. There is also include/linux/intel-iommu.h interface, but I it is very specific to intel gfx chips.

Is there any example how include/linux/dma-mapping.h interface can be used for iommu mappings?

Quoted 5 lines · Marek Szyprowski

We followed the style of iommu API for other mainline ARM platforms (both OMAP and MSM also have custom API for their iommu modules). I've briefly checked include/linux/iommu.h API and I've noticed that it has been designed mainly for KVM support. There is also include/linux/intel-iommu.h interface, but I it is very specific to intel gfx chips.

The MSM code actually uses the generic iommu.h code, using register_iommu, so the drivers can use the regular iommu_map.

I believe the omap code predates the iommu API, and should really be changed to use that. At least it was added before I started reviewing the code.

The iommu API is not really meant to be KVM specific, it's just that the in-tree users are basically limited to KVM at the moment. Another user that is coming up soon is the vmio device driver that can be used to transparently pass devices to user space. The idea behind the IOMMU API is that you can map arbitrary bus addresses to physical memory addresses, but it does not deal with allocating the bus addresses or providing buffer management such as cache flushes.

Quoted 2 lines

Is there any example how include/linux/dma-mapping.h interface can be used for iommu mappings?

The dma-mapping API is the normal interface that you should use for IOMMUs that sit between DMA devices and kernel memory. The idea is that you completely abstract the concept of an IOMMU so the device driver uses the same code for talking to a device with an IOMMU and another device with a linear mapping or an swiotlb bounce buffer.

This means that the user of the dma-mapping API does not get to choose the bus addresses, but instead you use the API to get a bus address for a chunk of memory, and then you can pass that address to a device.

See arch/powerpc/kernel/iommu.c and arch/x86/kernel/amd_iommu.c for common examples of how this is implemented. The latter one actually implements both the iommu_ops for iommu.h and dma_map_ops for dma-mapping.h.

Quoted 7 lines

The iommu API is not really meant to be KVM specific, it's just that the in-tree users are basically limited to KVM at the moment. Another user that is coming up soon is the vmio device driver that can be used to transparently pass devices to user space. The idea behind the IOMMU API is that you can map arbitrary bus addresses to physical memory addresses, but it does not deal with allocating the bus addresses or providing buffer management such as cache flushes.

Yea, I've noticed this and this basically what we expect from iommu driver. However the iommu.h API requires a separate call to map each single memory page. This is quite ineffective approach and imho the API need to be extended to allow mapping of the arbitrary set of pages.

Quoted 8 lines · Marek Szyprowski

Quoted 7 lines

The iommu API is not really meant to be KVM specific, it's just that the in-tree users are basically limited to KVM at the moment. Another user that is coming up soon is the vmio device driver that can be used to transparently pass devices to user space. The idea behind the IOMMU API is that you can map arbitrary bus addresses to physical memory addresses, but it does not deal with allocating the bus addresses or providing buffer management such as cache flushes.

Quoted 4 lines

Yea, I've noticed this and this basically what we expect from iommu driver. However the iommu.h API requires a separate call to map each single memory page. This is quite ineffective approach and imho the API need to be extended to allow mapping of the arbitrary set of pages.

We can always discuss extensions to the existing infrastructure, adding an interface for mapping an array of page pointers in the iommu API sounds like a good idea.

I also think that we should not really have separate iommu and dma-mapping interfaces, but rather have a portable way to define an iommu so that it can be used through the dma-mapping interfaces. I'm not asking you to do that as a prerequisite to merging your driver, but it may be good to keep in mind that the current situation is still lacking and that any suggestion for improving this as part of your work to support the samsung IOMMU is welcome.

Note that the ARM implementation of the dma-mapping.h interface currently does not support IOMMUs, but that could be changed by wrapping it using the include/asm-generic/dma-mapping-common.h infrastructure.

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · Marek Szyprowski

Quoted 7 lines

The iommu API is not really meant to be KVM specific, it's just that the in-tree users are basically limited to KVM at the moment. Another user that is coming up soon is the vmio device driver that can be used to transparently pass devices to user space. The idea behind the IOMMU API is that you can map arbitrary bus addresses to physical memory addresses, but it does not deal with allocating the bus addresses or providing buffer management such as cache flushes.

Yea, I've noticed this and this basically what we expect from iommu driver. However the iommu.h API requires a separate call to map each single memory page. This is quite ineffective approach and imho the API need to be extended to allow mapping of the arbitrary set of pages.

Quoted 3 lines

We can always discuss extensions to the existing infrastructure, adding an interface for mapping an array of page pointers in the iommu API sounds like a good idea.

We will investigate this API further. From the first sight it looks it won't take much work to port/rewrite our driver to fit into iommu.h API.

Quoted 7 lines

I also think that we should not really have separate iommu and dma-mapping interfaces, but rather have a portable way to define an iommu so that it can be used through the dma-mapping interfaces. I'm not asking you to do that as a prerequisite to merging your driver, but it may be good to keep in mind that the current situation is still lacking and that any suggestion for improving this as part of your work to support the samsung IOMMU is welcome.

Well creating a portable iommu framework and merging it with dma-mapping interface looks like a much harder (and time consuming) task. There is definitely a need for it. I hope that it can be developed incrementally starting from the current iommu.h and dma-mapping.h interfaces.

Please note that there might be some subtle differences
in the hardware that such framework must be aware. The first obvious one is the hardware design. Some platform has central iommu unit, other (like Samsung Exynos4) has a separate iommu unit per each device driver (this is still a simplification, because a video codec device has 2 memory interfaces and 2 iommu units). Currently I probably have not enough knowledge to predict the other possible issues that need to be taken into account in the portable and generic iommu/dma-mapping frame-work.

Quoted 3 lines

Note that the ARM implementation of the dma-mapping.h interface currently does not support IOMMUs, but that could be changed by wrapping it using the include/asm-generic/dma-mapping-common.h infrastructure.

ARM dma-mapping framework also requires some additional research for better DMA support (there are still issues with multiple mappings to be resolved).

Quoted 16 lines · Marek Szyprowski

Quoted 14 lines · Arnd Bergmann

Quoted 12 lines · Marek Szyprowski

Quoted 7 lines

The iommu API is not really meant to be KVM specific, it's just that the in-tree users are basically limited to KVM at the moment. Another user that is coming up soon is the vmio device driver that can be used to transparently pass devices to user space. The idea behind the IOMMU API is that you can map arbitrary bus addresses to physical memory addresses, but it does not deal with allocating the bus addresses or providing buffer management such as cache flushes.

Yea, I've noticed this and this basically what we expect from iommu driver. However the iommu.h API requires a separate call to map each single memory page. This is quite ineffective approach and imho the API need to be extended to allow mapping of the arbitrary set of pages.

Quoted 6 lines

Quoted 3 lines

We can always discuss extensions to the existing infrastructure, adding an interface for mapping an array of page pointers in the iommu API sounds like a good idea.

We will investigate this API further. From the first sight it looks it won't take much work to port/rewrite our driver to fit into iommu.h API.

Ok, sounds good.

Quoted 8 lines

Quoted 7 lines

I also think that we should not really have separate iommu and dma-mapping interfaces, but rather have a portable way to define an iommu so that it can be used through the dma-mapping interfaces. I'm not asking you to do that as a prerequisite to merging your driver, but it may be good to keep in mind that the current situation is still lacking and that any suggestion for improving this as part of your work to support the samsung IOMMU is welcome.

Quoted 4 lines

Well creating a portable iommu framework and merging it with dma-mapping interface looks like a much harder (and time consuming) task. There is definitely a need for it. I hope that it can be developed incrementally starting from the current iommu.h and dma-mapping.h interfaces.

Yes, that is the idea. Maybe we should add it to the list things that the Linaro kernel working group can target for the November release?

Quoted 7 lines

Please note that there might be some subtle differences
in the hardware that such framework must be aware. The first obvious one is the hardware design. Some platform has central iommu unit, other (like Samsung Exynos4) has a separate iommu unit per each device driver (this is still a simplification, because a video codec device has 2 memory interfaces and 2 iommu units). Currently I probably have not enough knowledge to predict the other possible issues that need to be taken into account in the portable and generic iommu/dma-mapping frame-work.

The dma-mapping API can deal well with one IOMMU per device, but would need some tricks to work with one device that has two separate IOMMUs.

I'm not very familar with the iommu API, but in the common KVM scenario, you need one IOMMU per device, so it should handle that just fine as well.

Quoted 6 lines

Quoted 3 lines

Note that the ARM implementation of the dma-mapping.h interface currently does not support IOMMUs, but that could be changed by wrapping it using the include/asm-generic/dma-mapping-common.h infrastructure.

ARM dma-mapping framework also requires some additional research for better DMA support (there are still issues with multiple mappings to be resolved).

You mean mapping the same memory into multiple devices, or a different problem?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

does not support IOMMUs, but that could be changed by wrapping it using the include/asm-generic/dma-mapping-common.h infrastructure.

ARM dma-mapping framework also requires some additional research for better DMA support (there are still issues with multiple mappings to be resolved).

Quoted 1 line

You mean mapping the same memory into multiple devices, or a different problem?

Mapping the same memory area multiple times with different cache settings is not legal on ARMv7+ systems. Currently the problems might caused by the low-memory kernel linear mapping and second mapping created for example by dma_alloc_coherent() function.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

does not support IOMMUs, but that could be changed by wrapping it using the include/asm-generic/dma-mapping-common.h infrastructure.

ARM dma-mapping framework also requires some additional research for better DMA support (there are still issues with multiple mappings to be resolved).

Quoted 6 lines

Quoted 1 line

You mean mapping the same memory into multiple devices, or a different problem?

Mapping the same memory area multiple times with different cache settings is not legal on ARMv7+ systems. Currently the problems might caused by the low-memory kernel linear mapping and second mapping created for example by dma_alloc_coherent() function.

Yes, I know this problem, but I don't think the case you describe is a serious limitation (there are more interesting cases, though): dma_map_single() etc will create additional *bus* addresses for a physical address, not additional virtual addresses.

dma_alloc_coherent should allocate memory that is not also mapped cached, which is what I thought we do correctly.

I have also noticed that dma_map_single/page/sg() can map physical memory into an arbitrary device address region.
But it is not enough solution for various kinds of IOMMUs. As Kukjin Kim addressed, we need to support larger page size than 4KB because we can reduce TLB miss when we have larger page size.

Our IOMMU(system mmu) supports all page size of ARM architecture including 16MB, 1MB, 64KB and 4KB.
Since the largest size supported by buddy system of 32-bit architecture is 4MB, our system support all page sizes except 16MB.
We proved that larger page size is helpful for DMA performance significantly (more than 10%, approximately).
Big page size is not a problem for peripheral devices
because their address space is not suffer from external fragmentation.

Quoted 6 lines

I have also noticed that dma_map_single/page/sg() can map physical memory into an arbitrary device address region.
But it is not enough solution for various kinds of IOMMUs. As Kukjin Kim addressed, we need to support larger page size than 4KB because we can reduce TLB miss when we have larger page size.

Quoted 8 lines

Our IOMMU(system mmu) supports all page size of ARM architecture including 16MB, 1MB, 64KB and 4KB.
Since the largest size supported by buddy system of 32-bit architecture is 4MB, our system support all page sizes except 16MB.
We proved that larger page size is helpful for DMA performance significantly (more than 10%, approximately).
Big page size is not a problem for peripheral devices
because their address space is not suffer from external fragmentation.

1. dma_map_single() et.al. is used for mapping *system* *RAM* for devices ? using whatever is necessary. ?It must not be used for trying to setup ? arbitary other mappings.

2. It doesn't matter where the memory for dma_map_single() et.al. comes ? from provided the virtual address is a valid system RAM address or ? the struct page * is a valid struct page in the memory map (iow, you ? can't create this yourself.)

3. In the case of an IOMMU, the DMA API does not limit you to only using ? 4K pages to setup the IOMMU mappings. ?You can use whatever you like ? provided the hardware can cope with it. ?You can coalesce several ? existing entries together provided you track what you're doing and can ? undo what's been done when the mapping is no longer required.

So really there's no reason not to use 64K, 1M and 16M IOMMU entries if that's the size of buffer which has been passed to the DMA API.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

2011/3/14 Russell King - ARM Linux <[email protected]>:

Quoted 20 lines · KyongHo Cho

Quoted 6 lines

I have also noticed that dma_map_single/page/sg() can map physical memory into an arbitrary device address region.
But it is not enough solution for various kinds of IOMMUs. As Kukjin Kim addressed, we need to support larger page size than 4KB because we can reduce TLB miss when we have larger page size.

Quoted 8 lines

Our IOMMU(system mmu) supports all page size of ARM architecture including 16MB, 1MB, 64KB and 4KB.
Since the largest size supported by buddy system of 32-bit architecture is 4MB, our system support all page sizes except 16MB.
We proved that larger page size is helpful for DMA performance significantly (more than 10%, approximately).
Big page size is not a problem for peripheral devices
because their address space is not suffer from external fragmentation.

1. dma_map_single() et.al. is used for mapping *system* *RAM* for devices ? using whatever is necessary. ?It must not be used for trying to setup ? arbitary other mappings.

Quoted 4 lines

2. It doesn't matter where the memory for dma_map_single() et.al. comes ? from provided the virtual address is a valid system RAM address or ? the struct page * is a valid struct page in the memory map (iow, you ? can't create this yourself.)

You mean that we cannot have arbitrary virtual address mapping for iommu based device?

actually, we have memory mapping to arbitrary device virtual address space, not kernel virtual address space.

Quoted 15 lines

3. In the case of an IOMMU, the DMA API does not limit you to only using ? 4K pages to setup the IOMMU mappings. ?You can use whatever you like ? provided the hardware can cope with it. ?You can coalesce several ? existing entries together provided you track what you're doing and can ? undo what's been done when the mapping is no longer required.

So really there's no reason not to use 64K, 1M and 16M IOMMU entries if that's the size of buffer which has been passed to the DMA API.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 24 lines

2011/3/14 Russell King - ARM Linux <[email protected]>:

Quoted 23 lines · KyongHo Cho

Quoted 6 lines

I have also noticed that dma_map_single/page/sg() can map physical memory into an arbitrary device address region.
But it is not enough solution for various kinds of IOMMUs. As Kukjin Kim addressed, we need to support larger page size than 4KB because we can reduce TLB miss when we have larger page size.

Quoted 8 lines

Our IOMMU(system mmu) supports all page size of ARM architecture including 16MB, 1MB, 64KB and 4KB.
Since the largest size supported by buddy system of 32-bit architecture is 4MB, our system support all page sizes except 16MB.
We proved that larger page size is helpful for DMA performance significantly (more than 10%, approximately).
Big page size is not a problem for peripheral devices
because their address space is not suffer from external fragmentation.

1. dma_map_single() et.al. is used for mapping *system* *RAM* for devices using whatever is necessary. It must not be used for trying to setup arbitary other mappings.

2. It doesn't matter where the memory for dma_map_single() et.al. comes from provided the virtual address is a valid system RAM address or the struct page * is a valid struct page in the memory map (iow, you can't create this yourself.)

Quoted 2 lines

You mean that we cannot have arbitrary virtual address mapping for iommu based device?

No. I mean exactly what I said - I'm talking about the DMA API in the above two points. The implication is that you can not create arbitary mappings of non-system RAM with the DMA API.

Quoted 17 lines

actually, we have memory mapping to arbitrary device virtual address space, not kernel virtual address space.

Quoted 14 lines

3. In the case of an IOMMU, the DMA API does not limit you to only using 4K pages to setup the IOMMU mappings. You can use whatever you like provided the hardware can cope with it. You can coalesce several existing entries together provided you track what you're doing and can undo what's been done when the mapping is no longer required.

So really there's no reason not to use 64K, 1M and 16M IOMMU entries if that's the size of buffer which has been passed to the DMA API.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Russell King - ARM Linux ? ?:

Quoted 25 lines · InKi Dae

Quoted 24 lines

2011/3/14 Russell King - ARM Linux <[email protected]>:

Quoted 23 lines · KyongHo Cho

Quoted 6 lines

I have also noticed that dma_map_single/page/sg() can map physical memory into an arbitrary device address region.
But it is not enough solution for various kinds of IOMMUs. As Kukjin Kim addressed, we need to support larger page size than 4KB because we can reduce TLB miss when we have larger page size.

Quoted 8 lines

Our IOMMU(system mmu) supports all page size of ARM architecture including 16MB, 1MB, 64KB and 4KB.
Since the largest size supported by buddy system of 32-bit architecture is 4MB, our system support all page sizes except 16MB.
We proved that larger page size is helpful for DMA performance significantly (more than 10%, approximately).
Big page size is not a problem for peripheral devices
because their address space is not suffer from external fragmentation.

1. dma_map_single() et.al. is used for mapping *system* *RAM* for devices using whatever is necessary. It must not be used for trying to setup arbitary other mappings.

2. It doesn't matter where the memory for dma_map_single() et.al. comes from provided the virtual address is a valid system RAM address or the struct page * is a valid struct page in the memory map (iow, you can't create this yourself.)

Quoted 7 lines

Quoted 2 lines

You mean that we cannot have arbitrary virtual address mapping for iommu based device?

No. I mean exactly what I said - I'm talking about the DMA API in the above two points. The implication is that you can not create arbitary mappings of non-system RAM with the DMA API.

sorry but I couldn't understand exactly what you said. could you give me your answer one more time?
does non-system RAM mean reserved memory regions? if not, is it arbitrary virtual address space that isn't kernel or user virtual address space and is the space for iommu based deivce?

I think we can consider another solution for the various requirements. I think one of the most possible solutions is VCMM.
Or we can enhance include/linux/iommu.h with reference of VCMM.

pgd is better for more compatibility.

Hello,

This patch series introduces new type of videbuf2 memory allocator - vb2-s5p-iommu. This allocator can be used only on Samsung SoCs that have IOMMU module. Currently only Samsung EXYNOS4 (former S5PV310) platform has SYSMMU modules. The allocator is then used by s5p-fimc driver. To make it possible some additional changes are required. Mainly 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).

We decided to use driver private address space mode of the iommu driver. This way each vb2-s5p-iommu client gets it's own address space for memory buffers. This reduces kernel virtual memory fragmentation as well as solves some non-trivial page table updates issues. The drawback is the fact that the interface for s5p-sysmmu has been changed.

This IOMMU allocator has no dependences on other subsystems besides Samsung platfrom core. 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 EXYNOS4 (S5PV310) platform. Support for S5PV210/S5PC110 platform still depends on CMA allocator that needs more discussion on memory management mailing list and development. The patches with updated s5p-mfc and s5p-tv drivers will follow.

To get FIMC module working on EXYNOS4/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: S5PV310: Add platform definitions for FIMC
- adds basic platform resources for FIMC modules (for s5p-fimc driver)

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

[PATCH 3/7] ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver
- a complete rewrite of sysmmu driver for Samsung platform:
- the new version introduces device private page tables (address space) mode
- simplified the resource management (no more horrible single platform device with 32 resources is needed)
- some other API chages required by upcoming videobuf2 allocator

[PATCH 4/7] v4l: videobuf2: add Samsung SYSMMU (IOMMU) based allocator
- introduces new memory allocator for videobuf2, it uses s5p-sysmmu iommu driver, memory for video buffers is acuired by alloc_page() kernel function

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

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

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

The patch series is based on git://linuxtv.org/media_tree.git tree, staging/for_v2.6.39 branch with the following Samsung platform patches:
1. [PATCH] ARM: Samsung: change suspend/resume code to depend on CONFIG_SUSPEND
http://www.mail-archive.com/linux-samsung-

2. [PATCH v2] ARM: S5PC210: add support for i2c PMICs on Universal_C210

http://www.mail-archive.com/linux-samsung-

This series has not been rebased onto the latest changes (S5PV310 renamed to EXYNOS4) in
git://git.kernel.org/pub/scm/linux/kernel/git/kgene/linux-samsung.git,
for-next branch. We will rebase them soon, but first we want to get feedback and comments on the s5p-iommu videobuf2 allocator idea.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

Complete patch summary:

Andrzej Pietrasiewicz (2):
ARM: Samsung: update/rewrite Samsung SYSMMU (IOMMU) driver
v4l: videobuf2: add Samsung SYSMMU (IOMMU) based allocator

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

Sylwester Nawrocki (1):
ARM: S5PV310: Add platform definitions for FIMC

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

 arch/arm/mach-s5pv310/Kconfig                    |    6 +
 arch/arm/mach-s5pv310/clock.c                    |   91 ++
 arch/arm/mach-s5pv310/cpu.c                      |    7 +
 arch/arm/mach-s5pv310/dev-pd.c                   |   93 ++-
 arch/arm/mach-s5pv310/dev-sysmmu.c               |  582 +++++++++----
 arch/arm/mach-s5pv310/include/mach/irqs.h        |   40 +-
 arch/arm/mach-s5pv310/include/mach/map.h         |    8 +
 arch/arm/mach-s5pv310/include/mach/regs-clock.h  |   12 +
 arch/arm/mach-s5pv310/include/mach/regs-sysmmu.h |   23 +-
 arch/arm/mach-s5pv310/include/mach/sysmmu.h      |  122 ---
 arch/arm/mach-s5pv310/mach-universal_c210.c      |   20 +
 arch/arm/plat-s5p/Kconfig                        |   22 +-
 arch/arm/plat-s5p/Makefile                       |    1 +
 arch/arm/plat-s5p/dev-fimc3.c                    |   43 +
 arch/arm/plat-s5p/include/plat/sysmmu.h          |  127 +++
 arch/arm/plat-s5p/sysmmu.c                       |  988 +++++++++++++++--

--

 arch/arm/plat-samsung/include/plat/devs.h        |    3 +-
 arch/arm/plat-samsung/include/plat/fimc-core.h   |    5 +
 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         |   36 +-
 drivers/media/video/s5p-fimc/fimc-mem.h          |   87 ++
 drivers/media/video/videobuf2-s5p-iommu.c        |  444 ++++++++++
 include/media/videobuf2-s5p-iommu.h              |   50 ++
 26 files changed, 2129 insertions(+), 703 deletions(-)
 rewrite arch/arm/mach-s5pv310/dev-sysmmu.c (86%)
 delete mode 100644 arch/arm/mach-s5pv310/include/mach/sysmmu.h
 create mode 100644 arch/arm/plat-s5p/dev-fimc3.c
 create mode 100644 arch/arm/plat-s5p/include/plat/sysmmu.h
 rewrite arch/arm/plat-s5p/sysmmu.c (85%)
 create mode 100644 drivers/media/video/s5p-fimc/fimc-mem.h
 create mode 100644 drivers/media/video/videobuf2-s5p-iommu.c
 create mode 100644 include/media/videobuf2-s5p-iommu.h

--
1.7.1.569.g6f426

Hello,

There are comments for your System MMU driver below.

It's good that System MMU has functionality of mapping but System MMU have to use other mapping of virtual memory allocator.

And would be better to change sysmmu_list to use array of defined in s5p_sysmmu_ip enumeration, so that can get enhancement of memory space usage, speed, and readability of codes.

TLB replacement policy does not need to use LRU. Of course, current System MMU also needs it. I think, the round robin is enough, because to access memory has no temporal locality and to make LRU need to access to System MMU register one more. The reset value is round robin.

In the setting of SHARED page table in s5p_sysmmu_control_locked, get the page table base address of ARM core from cp15 register now. But current->mm-

Quoted 1 line

pgd is better for more compatibility.

When it make page table with PRIVATE page table methods, the size of the structure to manage the second page table is quite big. It is much better rather that to make slab with cache size of 1KB.

Besides, the page mapping implementation is not safe in your System MMU driver. Because only first one confirms primary page table entry, when it assigns four second page tables consecutively at a time.

The System MMU driver cannot apply runtime pm by oneself with calling pm_runtime_put_sync(). The reason is because a device with System MMU can on/off power. I think just clock gating is enough. However, I can't find clock enable/disable in your driver.

By PRIVATE page table method, each system MMU comes to have a page table only for oneself. In this case, the problem is that each MFC System MMU L and R having another page table.

In your System MMU driver, the page size is always 4KB crucially. This says TLB thrashing and produces a result to lose a TLB hit rate. It is a big problem with the device such as rotator which does not do sequential access especially.

And the IRQ handler just outputs only a message. It should be implemented in call back function to be able to handle from each device driver.

When it sets System MMU in SHARED page table, kernel virtual memory is broken by a method such as s5p_sysmmu_map_area()

Hello,

On Tuesday, March 08, 2011 8:29 AM Kukjin Kim wrote:

Quoted 3 lines

Hello,

There are comments for your System MMU driver below.

Please take into account that this was an initial version of our SYSMMU driver to start the discussion, so there were a few minor problems left. Thanks for pointing them out btw. :)

Quoted 2 lines

It's good that System MMU has functionality of mapping but System MMU have to use other mapping of virtual memory allocator.

Could you elaborate on this? I'm not sure I understand right your problem.

SYSMMU driver is a low-level driver for the SYSMMU module. The driver should provide all the basic functionality that is (or might be) hardware dependent. Creating a mapping is one of such elementary functionalities. Sysmmu client (let it be videobuf2-s5p-iommu, vcmm, maybe even other driver directly) should not need to know the format of page descriptors or the way they are arranged in the memory. The sysmmu should provide low level functions to create and remove a mapping. Managing a virtual space is something that MIGHT be client dependent and should be left to the client.

This design allows for different approaches to coexist. Videobuf2-s5p-iommu client will manage the virtual space with gen_alloc framework, while vcmm will use its own methods.

It would be great if one decide to unify iommu interfaces across the kernel, but this will be a long road. We need to start from something simple (platform private) and working first.

Quoted 3 lines

And would be better to change sysmmu_list to use array of defined in s5p_sysmmu_ip enumeration, so that can get enhancement of memory space usage, speed, and readability of codes.

Yes, the list can be simplified to an array, but this is really a minor issue.

Quoted 4 lines

TLB replacement policy does not need to use LRU. Of course, current System MMU also needs it. I think, the round robin is enough, because to access memory has no temporal locality and to make LRU need to access to System MMU register one more. The reset value is round robin.

Well, the best possibility is to allow sysmmu clients to decide which policy should be used. For some devices (I'm thinking of MFC) LRU policy might give a little speedup.

Quoted 3 lines

In the setting of SHARED page table in s5p_sysmmu_control_locked, get the page table base address of ARM core from cp15 register now. But current->mm-

Quoted 1 line

pgd is better for more compatibility.

Right, this way the pgd table pointer can be acquired in a more system friendly way.

Quoted 3 lines

When it make page table with PRIVATE page table methods, the size of the structure to manage the second page table is quite big. It is much better rather that to make slab with cache size of 1KB.

Yes, our initial driver uses directly 4KB pages to manage 4 consecutive second page tables. However usually the allocations of client devices will be few but quite large each. So most of pages used to hold second level page tables will be effectively reused.

You are right however that the approach with a slab with 1KB units will result in code that is cleaner and easier to understand.

Quoted 3 lines

Besides, the page mapping implementation is not safe in your System MMU driver. Because only first one confirms primary page table entry, when it assigns four second page tables consecutively at a time.

That's a direct result of the 4-second-level-at-once method of allocating second level pages, but this can be cleaned by using 1KB with slab.

Quoted 4 lines

The System MMU driver cannot apply runtime pm by oneself with calling pm_runtime_put_sync(). The reason is because a device with System MMU can on/off power. I think just clock gating is enough. However, I can't find clock enable/disable in your driver.

Clock is enabled in probe() and disabled in remove(). pm_runtime_get/put_sync() only increases/decreases use count of a respective power domain, so the actual device driver also has to call pm_runtime_get/put. Calling pm_runtime_put_sync() will not shut down the power if the sysmmu client driver has called pm_runtime_get() without pm_runtime_put(). I see no problems here. Could you elaborate your issue?

Quoted 4 lines

By PRIVATE page table method, each system MMU comes to have a page table only for oneself. In this case, the problem is that each MFC System MMU L and R having another page table.

Yes, true. This is consequence of the MFC hardware design and the fact that it has 2 AXI master interfaces and 2 SYSMMU controllers. Each of them have to be configured separately. Each of them has a separate virtual driver's address space. Such configuration is used by the MFC driver posted in v7 patch series.

Quoted 4 lines

In your System MMU driver, the page size is always 4KB crucially. This says TLB thrashing and produces a result to lose a TLB hit rate. It is a big problem with the device such as rotator which does not do sequential access especially.

The page size is set to 4KB, because Linux kernel uses 4KB pages by default. Once support for other page size is available in the kernel, then sysmmu can be extended also.

Quoted 2 lines

And the IRQ handler just outputs only a message. It should be implemented in call back function to be able to handle from each device driver.

This was only for debugging purpose, but you are right that the sysmmu API in this area need to be extended.

Quoted 2 lines

When it sets System MMU in SHARED page table, kernel virtual memory is broken by a method such as s5p_sysmmu_map_area()

Yes, there should be a check added to prevent messing with ARM pgd in SHARED mode.

Best regards
--
Marek Szyprowski
Samsung Poland R&D Center

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel