Messages in this thread | | | From | Rob Clark <> | Date | Sat, 3 Aug 2019 07:47:08 -0700 | Subject | Re: please revert "drm/msm: Use the correct dma_sync calls in msm_gem" |
| |
On Fri, Aug 2, 2019 at 11:19 PM Christoph Hellwig <hch@lst.de> wrote: > > On Fri, Aug 02, 2019 at 03:06:10PM -0700, Rob Clark wrote: > > Sorry, this is just a temporary band-aid for v5.3 to get things > > working again. Yes, I realize it is a complete hack. > > My main problem is here that you badly hack a around a problem without > talking to the relevant maintainers, and by abusing even more internal > APIs. As said get_dma_ops isn't really for driver use (although we > have a few that use it for not quite as bad reason we are trying to get > rid off). And as also said your abuse of the DMA API will blow up > with dma-debug use quite badly. You might also corrupt the dma_address > in the scatterlist in ways that aren't intended - as the call to > dma_map_sg will allocate new iova space you are getting different > results from whatever you expect to actually get from your iommu API > usage. This might or might no matter in the end, but you really should > consult the maintainers first. > > > The root problem is that I'm using the DMA API in the first place. I > > don't actually use the DMA API to map buffers, for various reasons, > > but instead manage the iommu_domain directly. > > Yes, and this has been going on for years, without any obvious attempt > to address it at the API level before..
99% of my time goes to mesa and r/e, so having the argument about dealing w/ cache directly simply wasn't a big enough fire to deal with until now, unfortunately.
(Admittedly, there is room here for someone with more bandwidth to take on drm/msm maintainer role.. but someone needs to do it. Sean has been pitching in on the display side more recently, which has been a big help.)
> > Because arm/arm64 cache ops are not exported to modules, so currently > > I need to abuse the DMA API for cache operations (really just to clean > > pages if I need to mmap them uncached/writecombine). Originally I was > > doing that w/ dma_{map,unmap}_sg. But to avoid debug splats I > > switched that to dma_sync_sg (see > > 0036bc73ccbe7e600a3468bf8e8879b122252274). But now it seems the > > dma-direct ops are unhappy w/ dma_sync without a dma_map (AFAICT). > > Russell has been very strict about not exporting the cache ops, and all > for the right reasons. Cache maintainance for not dma coherent devices > is hard, and without a proper API that has arch input for even which > calls are used for cache flushing chances of bugs are extremely high. > > I see two proper ways out of this mess: either we actually make msm > use the DMA API, so I'd be curious of what is missing that forces you > to use the low-level iommu API. Or we need to enhance the iommu API > with a similar ownership concepts as the DMA API. Which probably is > a good thing even if we move msm over to the DMA API.
There are a few reasons we need to manage the GPU's address space directly, most of which are stalled on some iommu changes, that I wish I had more time to push for. In particular, we need to move to per-context pagetables (so each GL context has it's own GPU address space). Once we get there, we'd also like to enable SVM/SVA so GPU can share address space with the userspace process (which requires stall/resume and iommu fault handler to run in a context that can sleep).
Fitting that into the DMA API doesn't really make sense to me.
I'm not entirely sure that fitting cache maintenance into the IOMMU makes a huge amount of sense either, since the issue is really about the CPU cache. And I doubt the GPU is going to fit into a nice policy of page ownership. There are a number of cases where both CPU and GPU are accessing the same buffers, and unmapping/remapping to iommu is either not possible (because GPU is actively accessing another part of the buffer) or prohibitive from a performance standpoint.
As far as cache maintenance being hard, I'm not really sure I buy that.. at least not for arm64. (And probably not even w/ the limited # of armv7 cores that can be paired with drm/msm.)
> > (On some generations of hw, the iommu is attached to the device node > > that maps to the drm device, which is passed to dma_map/dma_sync. On > > other generations the iommu is attached to a sub-device. Changing > > this would break dtb compatibility.. so for now I need to handle both > > iommu-ops and direct-ops cases.) > > Or you always call call on a struct device that has the iommu, that > is match on the generic, and pick a different device. That would in > many ways seem preferably over the current hack, even if that also is > just a horribly band-aid.
I suppose picking a device w/ iommu would *mostly* work, except for a2xx (which has no IOMMU, but has it's own internal GPUMMU instead.. so there is no device with iommu ops)
We could perhaps, based on compatible, set a flag to use either dma_sync_* or dma_map_* which would avoid using get_dma_ops() (but otherwise doesn't seem like much of an improvement, I guess)
BR, -R
| |