[PATCHv9 0/6] dmaengine: rcar-dmac: add iommu support for slave transfers

On 16/09/16 13:05, Laurent Pinchart wrote:
[...]

quoted

quoted

quoted

One concern I have is that we might get an awkward situation if we ever
encounter one DMA engine hardware that is used in different systems that
all have an IOMMU, but on some of them the connection between the DMA
master and the slave FIFO bypasses the IOMMU while on others the IOMMU
is required.

Do you mean systems where some of the channels of a specific DMA engine go
through the IOMMU while others do not ? We indeed have no solution today
for such a situation.

The problem is a bit broader than that, we'll also have an issue with DMA
engines that have different channels served by different IOMMUs. I recall
discussing this in the past with you, and the solution you proposed was to
add a channel index to struct dma_attrs seems good to me. To support the
case where some channels don't go through an IOMMU we would only need
support for null entries in the IOMMUs list associated with a device (for
instance in the DT case null entries in the iommus property).

I think at that point we just create the channels as child devices of
the main dmaengine device so they each get their own DMA ops, and can do
whatever. The Qualcomm HIDMA driver already does that for a very similar
reason (so that the IOMMU can map individual channels into different
guest VMs).

That's another option, but it seems more like a workaround to me, instead of a 
proper solution to fix the more global problem of multiple memory paths within 
a single device. I have other hardware devices that can act as bus masters 
through different paths (for instance a display-related device that fetches 
data and commands through different paths). Luckily so far all those paths are 
served by the same IOMMU, but there's no guarantee this will remain true in 
the future. Furthermore, even today, the IOMMU connected to that device has 
the ability to selectively enable and disable its ports. I have to keep them 
all enabled due to the lack of channel information in the DMA mapping and 
IOMMU APIs, leading to increased power consumption.

Indeed, I think both the Exynos and Rockchip IOMMU drivers already do
cater for a device mastering though multiple discrete IOMMUs, not being
the fancy multi-port multi-context ones like yours and mine.

I guess what we could really do with is a decent abstraction of
multi-master peripherals at the device level; a "threads within the same
process" sort of granularity, as it were. I'd envisage it more along the
lines of how we handle NUMA, i.e. dma_map_page_attrs(...) becomes a
wrapper for dma_map_page_attrs_multi(..., CHANNEL_ALL), and trickier
users can call the latter with the a more specific channel(s) argument
(maybe it's a bitmask rather than an index). Meanwhile,
dev->archdata.dma_ops may point to a device-specific array of
dma_map_ops, which the DMA API backend iterates over if necessary.

Strangely, that doesn't actually sound too horrible.

Robin.

quoted

quoted

Now I see that struct dma_attrs has been replaced by unsigned long in

commit 00085f1efa387a8ce100e3734920f7639c80caa3
Author: Krzysztof Kozlowski [off-list ref]
Date:   Wed Aug 3 13:46:00 2016 -0700

    dma-mapping: use unsigned long for dma_attrs

We still have enough bits to reserve some of them for a channel number,
but I'm not very happy with that patch as I can see how a future proposal
to handle the channel number through the DMA attributes will get rejected
on the grounds of bits starvation then :-(

quoted

I don't have any idea for how this could be handled in a generic way, so
my best answer here is to hope we never get there, and if we do, handle
it using some local hack in the driver.