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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Arnd,

On Friday 16 Sep 2016 14:02:35 Arnd Bergmann wrote:
> On Friday, September 16, 2016 12:48:23 PM CEST Laurent Pinchart wrote:
> > On Friday 16 Sep 2016 11:07:48 Arnd Bergmann wrote:
> >> On Thursday, September 15, 2016 9:56:51 PM CEST Vinod Koul wrote:
> >>> On Wed, Aug 10, 2016 at 11:07:10PM +0530, Vinod Koul wrote:
> >>>> On Wed, Aug 10, 2016 at 01:22:13PM +0200, Niklas Söderlund wrote:
> >>
> >> I had not looked at the series earlier, but this version looks entirely
> >> reasonable to me, so
> >> 
> >> Acked-by: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> 
> >> 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.
> 
> I wasn't thinking quite that far, though that is also a theoretical
> problem. However, the simple solution would be to have a bit in the DMA
> specifier let the driver know whether translation is needed or not.
> 
> The simpler case I was thinking of is where the entire DMA engine
> either goes through an IOMMU or doesn't (depending on the integration
> into the SoC), so we'd have to find out through some DT property
> or compatible string in the DMA enginen driver.

Don't we already get that information from the iommus DT property ? If the DMA 
engine goes through an IOMMU the property will be set, otherwise it will not.

> > 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.
> 
> Do you mean a theoretical problem, or a chip that you already know exists?

That's theoretical. The problem I'm facing today is a DMA engine whose 
channels are served by different ports of the same IOMMU. This works in a 
suboptimal way because I have to keep all the IOMMU ports enabled regardless 
of whether they're used or not, as the DMA engine and IOMMU APIs don't carry 
channel information.

> > 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).
> > 
> > Now I see that struct dma_attrs has been replaced by unsigned long in
> > 
> > commit 00085f1efa387a8ce100e3734920f7639c80caa3
> > Author: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > 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 :-(
> 
> Agreed, that can become interesting.

Does the above-mentioned patch really fix a performance, memory consumption or 
other issue ?

-- 
Regards,

Laurent Pinchart