Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg*

[Lukas Wunner <lukas@wunner.de>]

On Tue, Nov 27, 2018 at 11:39:40PM -0800, Christoph Hellwig wrote:
> > +		/*
> > +		 * dma_sync_sg_*() flush the physical pages, so point
> > +		 * sg->dma_address to the physical ones for the right behavior.
> > +		 */
> > +		for_each_sg(msm_obj->sgt->sgl, s, msm_obj->sgt->nents, i)
> > +			sg_dma_address(s) = sg_phys(s);
> > +
> 
> I'm sorry, but this is completely bogus and not acceptable.
> 
> The only place that is allowed to initialize sg_dma_address is
> dma_map_sg.  If the default dma ops don't work for your setup we have
> major a problem and need to fix the dma api / iommu integration instead
> of hacking around it.

Somewhat related, I recently submitted a patch for the spi-bcm2835.c
driver to overcome a limitation of the DMA engine and the spi0 master
on the Raspberry Pi:

http://lists.infradead.org/pipermail/linux-rpi-kernel/2018-November/008275.html

If a buffer needs to be sent or received which was vmalloc'ed (hence is
not contiguous in physical memory) and starts in the middle of a page
with an offset not a multiple of 4, the buffer could not be transmitted
with DMA so far because the spi0 master requires 4 byte writes to the
FIFO and the DMA engine is incapable of combining multiple sglist
entries of arbitrary length into a continuous stream of 4 byte chunks.

The solution I've found is to transfer the first few bytes of the first
sglist entry with programmed I/O such that the remainder has a length
which is a multiple of 4.  For this to work, I have to massage the first
sglist entry, i.e. I have to add the number of bytes transmitted with
PIO to the DMA address and subtract it from the sglist entry's length.
After the DMA engine has done its job, I undo those changes to the
sglist.

Could you comment on the acceptibility of this approach?  If you do not
deem it acceptable, could you make a suggestion how the dma-mapping API
shall be amended to support such use cases?

(I sure hope the approach *is* acceptable, getting this to work at all
required endless fiddling.  The BCM2835 is in some ways a not so great
SoC and could do with better documentation, to put it mildly.)

(Note: I've since amended the above-linked patch to sync only the modified
cachelines back to memory, not the full first sglist entry.  I've also
switched to the sg_dma_address() and sg_dma_len() syntax instead of
referencing the sglist entry members directly, this appears to be more
common in the tree even for l-values.  I was going to post these changes
as v2 shortly.)

Thanks,

Lukas