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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4042 bytes --]

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@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@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.

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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel