Message-ID: <CAJs_Fx5r0qbtJPC+roiXaWz0SbPVtnDyWun6TB6a3HhQAEfk3A@mail.gmail.com>
Date:   Sat, 3 Aug 2019 07:47:08 -0700
From:   Rob Clark <robdclark@...omium.org>
To:     Christoph Hellwig <hch@....de>
Cc:     Stephan Gerhold <stephan@...hold.net>,
        Sean Paul <seanpaul@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>,
        torvalds@...ux-foundation.org
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@....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

Powered by blists - more mailing lists