Hello, Mr. Kukjin. 2011/3/8 Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>: > 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@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
