[PATCH 2/9] drm/tegra: gem: Properly pin imported buffers

[Daniel Vetter]

On Fri, Nov 29, 2019 at 11:15:37AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:10:38AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:34PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding at nvidia.com>
> > > 
> > > Buffers that are imported from a DMA-BUF don't have pages allocated with
> > > them. At the same time an SG table for them can't be derived using the
> > > DMA API helpers because the necessary information doesn't exist. However
> > > there's already an SG table that was created during import, so this can
> > > simply be duplicated.
> > > 
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  drivers/gpu/drm/tegra/gem.c | 43 +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 43 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> > > index 746dae32c484..6dfad56eee2b 100644
> > > --- a/drivers/gpu/drm/tegra/gem.c
> > > +++ b/drivers/gpu/drm/tegra/gem.c
> > > @@ -27,6 +27,29 @@ static void tegra_bo_put(struct host1x_bo *bo)
> > >  	drm_gem_object_put_unlocked(&obj->gem);
> > >  }
> > >  
> > > +/* XXX move this into lib/scatterlist.c? */
> > > +static int sg_alloc_table_from_sg(struct sg_table *sgt, struct scatterlist *sg,
> > > +				  unsigned int nents, gfp_t gfp_mask)
> > > +{
> > > +	struct scatterlist *dst;
> > > +	unsigned int i;
> > > +	int err;
> > > +
> > > +	err = sg_alloc_table(sgt, nents, gfp_mask);
> > > +	if (err < 0)
> > > +		return err;
> > > +
> > > +	dst = sgt->sgl;
> > > +
> > > +	for (i = 0; i < nents; i++) {
> > > +		sg_set_page(dst, sg_page(sg), sg->length, 0);
> > > +		dst = sg_next(dst);
> > > +		sg = sg_next(sg);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo,
> > >  				     dma_addr_t *phys)
> > >  {
> > > @@ -52,11 +75,31 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo,
> > >  		return ERR_PTR(-ENOMEM);
> > >  
> > >  	if (obj->pages) {
> > > +		/*
> > > +		 * If the buffer object was allocated from the explicit IOMMU
> > > +		 * API code paths, construct an SG table from the pages.
> > > +		 */
> > >  		err = sg_alloc_table_from_pages(sgt, obj->pages, obj->num_pages,
> > >  						0, obj->gem.size, GFP_KERNEL);
> > >  		if (err < 0)
> > >  			goto free;
> > > +	} else if (obj->sgt) {
> > > +		/*
> > > +		 * If the buffer object already has an SG table but no pages
> > > +		 * were allocated for it, it means the buffer was imported and
> > > +		 * the SG table needs to be copied to avoid overwriting any
> > > +		 * other potential users of the original SG table.
> > > +		 */
> > > +		err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents,
> > > +					     GFP_KERNEL);
> > 
> > Why duplicate this instead of just handing out obj->sgt, and then in unpin
> > making sure you don't release it? You could also only map/unmap the
> > dma_buf here in your pin/unpin, but that's a pile of work plus the mapping
> > is cached anyway so won't change a thing.
> 
> The problem with just handing out obj->sgt is that these buffers may be
> used by several of the host1x engines in the same job. This means that
> they may end up getting dma_map()'ed by multiple devices. dma_map_*()
> stores the DMA addresses for the buffer in the SG entries, so subsequent
> calls would effectively overwrite the earlier mappings, so we need a new
> SG table for each device.

So strictly speaking, i.e. if you don't want to contribute to the living
mess that's called dma-buf, you're supposed to dma_buf_attach each and
every device (definitely those with different iommu/dma-api routing). That
entire "let me peak behind the abstraction" thing is kinda not how this
was meant to be used. But the first drm prime drivers did it really badly,
plus drm_prime.c is somewhat backwards in some regards by always attaching
for you right away to /something/, but maybe not the right thing.

Adding more dma-buf interface abuse doesn't feel like the right thing
really.

Also, you're not supposed to call dma_map_sg on the sg tables you get from
dma-buf, they're supposed to be mapped already.
-Daniel


> 
> Thierry
> 
> > -Daniel
> > 
> > > +		if (err < 0)
> > > +			goto free;
> > >  	} else {
> > > +		/*
> > > +		 * If the buffer object had no pages allocated and if it was
> > > +		 * not imported, it had to be allocated with the DMA API, so
> > > +		 * the DMA API helper can be used.
> > > +		 */
> > >  		err = dma_get_sgtable(dev, sgt, obj->vaddr, obj->iova,
> > >  				      obj->gem.size);
> > >  		if (err < 0)
> > > -- 
> > > 2.23.0
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch