Hi Marek, On Friday 04 April 2014 14:23:57 Marek Szyprowski wrote: > Hello, > > I'm sorry for a delay, I've got back from my holidays and I'm still busy > trying to get thought all the emails. No worries. > On 2014-03-17 23:44, Laurent Pinchart wrote: > > On Monday 17 March 2014 14:58:24 Suman Anna wrote: > > > On 03/16/2014 04:54 PM, Sakari Ailus wrote: > > > > On Fri, Mar 14, 2014 at 12:00:16PM +0100, Laurent Pinchart wrote: > > > >> Hi Suman, > > > >> > > > >> (CC'ing Joerg Roedel and Marek Szyprowski for the core IOMMU > > > >> discussion) > > > >> > > > >> On Thursday 13 March 2014 21:33:37 Suman Anna wrote: > > > >>> On 03/07/2014 06:46 PM, Laurent Pinchart wrote: > > > >>>> Hello, > > > >>>> > > > >>>> This patch set fixes miscellaneous issues with the OMAP IOMMU > > > >>>> driver, found when trying to port the OMAP3 ISP away from omap- > > > >>>> iovmm to the ARM DMA API. The biggest issue is fixed by patch 5/5, > > > >>>> while the other patches fix smaller problems that I've noticed when > > > >>>> reading the code, without experiencing them at runtime. > > > >>>> > > > >>>> I'd like to take this as an opportunity to discuss OMAP IOMMU > > > >>>> integration with the ARM DMA mapping implementation. The idea is to > > > >>>> hide the IOMMU completely behind the DMA mapping API, making it > > > >>>> completely transparent to drivers. > > > >>> > > > >>> Thanks for starting the discussion. > > > >>> > > > >>>> A drivers will only need to allocate memory with dma_alloc_*, and > > > >>>> behind the scene the ARM DMA mapping implementation will find out > > > >>>> that the device is behind an IOMMU and will map the buffers through > > > >>>> the IOMMU, returning an I/O VA address to the driver. No direct > > > >>>> call to the OMAP IOMMU driver or to the IOMMU core should be > > > >>>> necessary anymore. > > > >>>> > > > >>>> To use the IOMMU the ARM DMA implementation requires a VA mapping > > > >>>> to be created with a call to arm_iommu_create_mapping() and to then > > > >>>> be attached to the device with arm_iommu_attach_device(). This is > > > >>>> currently not performed by the OMAP IOMMU driver, I have thus added > > > >>>> that code to the OMAP3 ISP driver for now. I believe this to still > > > >>>> be an improvement compared to the current situation, as it allows > > > >>>> getting rid of custom memory allocation code in the OMAP3 ISP > > > >>>> driver and custom I/O VA space management in omap-iovmm. However, > > > >>>> that code should ideally be removed from the driver. The question > > > >>>> is, where should it be moved to ? > > > >>>> > > > >>>> One possible solution would be to add the code to the OMAP IOMMU > > > >>>> driver. However, this would require all OMAP IOMMU users to be > > > >>>> converted to the ARM DMA API. I assume there would be issues that I > > > >>>> don't foresee though. > > > >>> > > > >>> Yeah, I think this will pose some problems for the other main user > > > >>> of IOMMUs - the remoteproc devices (DSP, Dual-Cortex M3/M4 > > > >>> processors in OMAP4 and beyond). A remoteproc device also needs to > > > >>> map memory at specific addresses for its code and data sections, and > > > >>> not rely on a I/O VA address being given to it. The firmware > > > >>> sections are already linked at specific addresses, and so remoteproc > > > >>> needs to allocate memory, load the firmware and map into appropriate > > > >>> device addresses. We are doing this currently usage a combination of > > > >>> CMA memory to get contiguous memory (there are some restrictions for > > > >>> certain sections) and iommu_map/unmap API to program the MMU with > > > >>> these pages. This usage is different from what is expected from > > > >>> exchanging buffers, which can be allocated from a predefined mapping > > > >>> range. Even that one is tricky if we need to support different cache > > > >>> properties/attributes as the cache configuration is in general local > > > >>> to these processors. > > > >> > > > >> Right, we indeed need two levels of API, one for drivers such as > > > >> remoteproc that need direct control of the IOMMU, and one for drivers > > > >> that only need to map buffers without any additional requirement. In > > > >> the second case the drivers should ideally use the DMA mapping API > > > >> not even be aware that an IOMMU is present. This would require moving > > > >> the ARM mapping allocation out of the client driver. > > > > > > Actually, I think there is one another use case, even with remoteprocs > > > which is to runtime map buffers. This is different from the firmware > > > management. The memory for buffers could have been allocated from other > > > subsystems, but remoteproc would need just to manage the VA space and > > > map. > > > > Right, although you might not always have to manage the VA space in that > > case. Anyway, if your driver needs to manage the VA space for the > > firmware, it should be able to manage the VA space for the buffers using > > the same IOMMU API. > > I've already seen some use cases which require to give a client device > limited access to DMA mapping IOMMU related structures. If we agree on API, > I see no problems to add such calls to dma-mapping. However in the most > common case I would like to hide the presence of IOMMU from the client > device at all. I agree with you. I'd like to hide the IOMMU completely by default, but some drivers will need fine-grained control over the IOMMU and will thus likely need direct access. I really see two ways to solve this, either allowing drivers to manage the IOMMU directly when required, or extending the DMA API to allow some degree of IOMMU management in cooperation with the DMA API implementation. I'm not sure which is best, as I don't know all our current uses cases for direct IOMMU access in details, and I can't obviously foresee all the future use cases. To achieve transparent IOMMU handling for drivers we need to move the arm_iommu_create_mapping() and arm_iommu_attach_device() calls from bus masters (IOMMU clients) device drivers to a more central location. This could be the IOMMU drivers, the IOMMU core, the bus drivers, ... However, if we decide to allow drivers direct control over the IOMMU, we also need to make those calls conditional, and thus give a way to device drivers to opt-out. > > > >> The IOMMU core or the IOMMU driver will need to know whether the > > > >> driver expects to control the IOMMU directly or to have it managed > > > >> transparently. As this is a software configuration I don't think the > > > >> information belongs to DT. The question is, how should this > > > >> information be conveyed? > > > > > > Can this be done through iommu_domain_set_attr? But that means the > > > client driver has to dictate this. The iommu driver can be configured > > > appropriately based on this. > > > > The problem with that approach is that IOMMU client (bus master) drivers > > would need a pointer to the IOMMU domain. That's what we want to avoid in > > the first place :-) > > > > > > The driver would need to know that, I think. > > > > The IOMMU client driver would know that. Or, to be accurate, it should > > either know that it wants to manage the IOMMU directly, or should ignore > > the IOMMU completely. > > > > > > Currently the DMA mapping API doesn't allow explicit addressing to > > > > IOVA address space AFAIK. The IOMMU API does. It's a good question how > > > > to do this as I don't think there's even a way for the driver to > > > > explicitly obtain access to the IOMMU. > > > > A driver can't access the IOMMU device directly, but the IOMMU API uses > > IOMMU domains, not IOMMU devices. Drivers can create domains and then > > access the IOMMU API. The association with a particular IOMMU instance is > > currently left to the IOMMU driver, that usually gets that information > > from platform data or DT in a driver-specific way. I think this should be > > standardized, especially in the DT case. > > > > What drivers can't do at the moment is accessing the IOMMU API using a > > domain they haven't created themselves. A two drivers requiring direct > > access to an IOMMU sharing the same domain wouldn't be possible. That > > might not be a problem we need to solve now though. > > > > > > The virtual address space allocation would need to take into account > > > > that some of the address space is actually mapped outside it. The iova > > > > library can do this already. > > > > This only needs to be taken into account for drivers that require direct > > control of the IOMMU. Those drivers are currently expected to manage the > > whole VA space themselves. They can do so internally, or using a library > > such as iova, yes, but they won't use the DMA API IOMMU integration. > > > > > >>>> I'm not even sure whether the OMAP IOMMU driver would be the best > > > >>>> place to put that code. Ideally VA spaces should be created by the > > > >>>> platform somehow, and mapping of devices to IOMMUs should be > > > >>>> handled by the IOMMU core instead of the IOMMU drivers. We're not > > > >>>> there yet, and the path might not be straightforward, hence this > > > >>>> attempt to start a constructive discussion. > > > >>>> > > > >>>> A second completely unrelated problem that I'd like to get feedback > > > >>>> on is the suspend/resume support in the OMAP IOMMU driver, or > > > >>>> rather the lack thereof. The driver exports omap_iommu_save_ctx() > > > >>>> and omap_iommu_restore_ctx() functions and expects the IOMMU users > > > >>>> to call them directly. This is really hackish to say the least. A > > > >>>> proper suspend/resume implementation shouldn't be difficult to > > > >>>> implement, and I'm wondering whether the lack of proper power > > > >>>> management support comes from historical reasons only, or if there > > > >>>> are problems I might not have considered. > > > >>> > > > >>> Agreed, the current code definitely needs a cleanup and better > > > >>> organization (like folding into runtime suspend ops). The main thing > > > >>> is that we need the IOMMU to be activated as long as its parent > > > >>> device is active (like the omap3isp or a remoteproc device). And > > > >>> this behaviour needs separation from configuring it and programming > > > >>> for the first time. Only a user can dictate when an IOMMU is not > > > >>> needed. So we either need an API to activate or have access to the > > > >>> iommu's dev object somehow so that we can use the pm_runtime_get/put > > > >>> API to hold a usage counter and let the runtime suspend ops take > > > >>> care of save/restore without tearing down the domain or detaching > > > >>> the device. > > > >> > > > >> The OMAP IOMMU driver calls pm_runtime_get_sync() in iommu_enable(), > > > >> and pm_runtime_put_sync() in iommu_disable(). The enable/disable > > > >> functions are called in the attach and detach handlers (as well as in > > > >> the fault handler for the disable function). As long as a device is > > > >> attached the IOMMU will thus not be suspended by runtime PM, but > > > >>> could be suspended by system PM. > > > > > > This would all need some rearranging of course. It is also not about > > > just disabling the clocks, we may have to assert the resets as well, due > > > to couple of issues on OMAP4 and beyond. > > > > Right. > > > > > >> This leads to two separate issues. The first one is that the IOMMU > > > >> will be powered on as long as a device is attached, even if the > > > >> device isn't in use. This consumes power needlessly. It would > > > >> probably be better to more the enable/disable calls to the map/unmap > > > >> handlers, as the IOMMU should be powered off when no mapping exists > > > >> (or am I missing something ?). > > > > > > > > In general, I wouldn't make the assumption that no mappings can exist > > > > if the IOMMU is powered off. This would mean that just keeping buffers > > > > around for later use would keep hardware powered. Memory allocation > > > > can be a very time-consuming task which often is done when the entire > > > > system is booted up (depending on the user space naturally), not when > > > > a particular device node is opened. > > > > > > Yeah, agreed. remoteproc firmware memory will stay mapped in even when > > > we power off IOMMU, we just preserve the required IOMMU context. I > > > expect the suspend/resume code to preserve any locked TLBs and the TTB > > > address. The page table entries would be auto-preserved as long as we > > > don't free the table. > > > > I don't want to force the IOMMU to be powered on when a mapping exists, > > but at the moment the driver powers it on when a device is attached, > > which is even worse. Moving the pm_runtime_get/set calls to map/unmap > > would be an improvement compared to the current situation, but is > > obviously not the perfect solution. > > Basically the IOMMU should be enabled when client device called > pm_runtime_get_sync() and can disabled after client's pm_runtime_put(). Too > bad that this cannot be easily done. Too bad indeed :-) > Initially I hooked IOMMU device as a parent device for the client device. > This worked quite fine until pm_domain has been introduced. With pm_domains > things are much more complicated, because we cannot use parent relationship > (it won't work). For internal use a hack proposed by Cho KyongHo, where > IOMMU driver replaces pm_domain_ops of the client device. I'm working on > finding a less hacky solution. I was thinking about using some kind of PM notifier, but that might be a very naive approach. Have you considered it ? > For a reference, please check drivers/iommu/exynos-iommu.c file from the > following repo: > https://review.tizen.org/git/?p=platform/kernel/linux-3.10.git;a=shortlog;h= > refs/heads/tizen -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
