[PATCH/RFC 0/7] Samsung IOMMU videobuf2 allocator and s5p-fimc update

[InKi Dae]

Hello, Mr. Kukjin.


2011/3/8 Marek Szyprowski <m.szyprowski at samsung.com>:
> Hello,
>
> On Tuesday, March 08, 2011 8:29 AM Kukjin Kim wrote:
>
>> 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. :)
>
>
>> It's good that System MMU has functionality of mapping but System MMU have
>> to use other mapping of virtual memory allocator.
>
you mean that other page sizes such as large, small page and so on
should be supported in mapping also? otherwise use 64k not 4k page?

> 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.
>
>> 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.
>
>> 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.
>
>> 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-
>> >pgd is better for more compatibility.
>
> Right, this way the pgd table pointer can be acquired in a more system friendly
> way.
>
>> 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.
>
>> 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.
>
>> 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?
>
>>
>> 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.
>
>> 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.
>
>> 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.
>
>> 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
>