From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukas Wunner Subject: Re: [PATCH v2 1/1] drm: msm: Replace dma_map_sg with dma_sync_sg* Date: Wed, 28 Nov 2018 15:19:49 +0100 Message-ID: <20181128141949.6mww6g2ze3qkzxhj@wunner.de> References: <20181126213710.3084-1-vivek.gautam@codeaurora.org> <20181128073940.GA13072@infradead.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20181128073940.GA13072@infradead.org> Sender: linux-kernel-owner@vger.kernel.org To: Christoph Hellwig Cc: Vivek Gautam , airlied@linux.ie, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, tfiga@chromium.org, Sean Paul , freedreno@lists.freedesktop.org List-Id: linux-arm-msm@vger.kernel.org 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