Re: [PATCH 4/6] drm/amdgpu: add checks if DMA-buf P2P is supported

From: Jason Gunthorpe <jgg@ziepe.ca>
To: christian.koenig@amd.com
Cc: David1.Zhou@amd.com, hch@infradead.org, daniel@ffwll.ch,
	dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org,
	linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org,
	Logan Gunthorpe <logang@deltatee.com>
Subject: Re: [PATCH 4/6] drm/amdgpu: add checks if DMA-buf P2P is supported
Date: Wed, 11 Mar 2020 11:38:35 -0300	[thread overview]
Message-ID: <20200311143835.GD31668@ziepe.ca> (raw)
In-Reply-To: <848c2f50-1529-f036-69cd-5104b7ebe27b@gmail.com>

On Wed, Mar 11, 2020 at 03:33:01PM +0100, Christian König wrote:
> Am 11.03.20 um 15:04 schrieb Jason Gunthorpe:
> > On Wed, Mar 11, 2020 at 02:51:56PM +0100, Christian König wrote:
> > > Check if we can do peer2peer on the PCIe bus.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > > index aef12ee2f1e3..bbf67800c8a6 100644
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > > @@ -38,6 +38,7 @@
> > >   #include <drm/amdgpu_drm.h>
> > >   #include <linux/dma-buf.h>
> > >   #include <linux/dma-fence-array.h>
> > > +#include <linux/pci-p2pdma.h>
> > >   /**
> > >    * amdgpu_gem_prime_vmap - &dma_buf_ops.vmap implementation
> > > @@ -179,6 +180,9 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf,
> > >   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> > >   	int r;
> > > +	if (pci_p2pdma_distance_many(adev->pdev, &attach->dev, 1, true) < 0)
> > > +		attach->peer2peer = false;
> > > +
> > Are there other related patches than this series?
> > 
> > p2p dma mapping needs to be done in common code, in p2pdma.c - ie this
> > open coding is missing the bus_offset stuff, at least.
> 
> Yeah, I'm aware of this. But I couldn't find a better way for now.

Well, it isn't optional :)

> > I really do not want to see drivers open code this stuff.
> > 
> > We already have a p2pdma API for handling the struct page case, so I
> > suggest adding some new p2pdma API to handle this for non-struct page
> > cases.
> > 
> > ie some thing like:
> > 
> > int 'p2pdma map bar'(
> >     struct pci_device *source,
> >     unsigned int source_bar_number,
> >     struct pci_device *dest,
> >     physaddr&len *array_of_offsets & length pairs into source bar,
> >     struct scatterlist *output_sgl)
> 
> Well that's exactly what I have to avoid since I don't have the array of
> offsets around and want to avoid constructing it.

Maybe it doesn't need an array of offsets - just a single offset and
callers can iterate the API?

> Similar problem for dma_map_resource(). My example does this on demand, but
> essentially we also have use cases where this is done only once.

I'm not sure if this is portable. Does any IOMMU HW need to know P2P
is happening to setup successfully? We currently support such a narrow
scope of HW for P2P..

> Ideally we would have some function to create an sgl based on some arbitrary
> collection of offsets and length inside a BAR.

Isn't that what I just proposed above ?

Jason