Re: [PATCH 3/9] drm/tegra: gem: Remove premature import restrictions

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Nov 29, 2019 at 11:33:45AM +0100, Thierry Reding wrote:
> On Fri, Nov 29, 2019 at 10:12:44AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 28, 2019 at 04:37:35PM +0100, Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > It's not known at import time whether or not all users of a DMA-BUF will
> > > be able to deal with non-contiguous memory. Each user needs to verify at
> > > map-time whether it can access the buffer.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > 
> > I'm not seeing any other check for nents ... does this mean that there's
> > not actually any block that requires contig mem?
> 
> All the blocks require contiguous memory. However, they are all behind
> an IOMMU and in practice will always end up mapping the buffers through
> the IOMMU. Techically this check should now be in tegra_dc_pin(), which
> is called by the ->prepare_fb() callback. I didn't add it because there
> are no practical use-cases where this happens, although I guess you
> could come up with a kernel and DTB combination where this is actually
> possible by jumping through some hoops.
> 
> This fix here is to make Tegra DRM interoperation with Nouveau work
> again since that's currently broken after moving to the IOMMU-backed DMA
> API as an alternative to explicit IOMMU usage. With explicit IOMMU usage
> (that's the if corresponding to the else removed below) the IOMMU domain
> was shared between the display controllers at the driver level, so it
> was fine to make this determination in the else branch because this was
> the case where no IOMMU was in play. After the move to the DMA API, this
> else branch is also taken when the DMA API is backed by an IOMMU and at
> it is unfortunately not known at import time which display controller
> ends up scanning out the DMA BUF, nor if that display controller is
> behind an IOMMU. We only know that when the actual mapping takes place,
> so we'd need to look at sgt->nents after dma_map_sg() in in
> tegra_dc_pin().
> 
> I'll add that check there, just in case anyone manages to conjure up
> such a configuration.

Imo just paste the above explanation into the commit message and

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Or also add the check, but imo the explanation here is the important part.
-Daniel

> 
> Thierry
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/tegra/gem.c | 7 -------
> > >  1 file changed, 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> > > index 6dfad56eee2b..bc15b430156d 100644
> > > --- a/drivers/gpu/drm/tegra/gem.c
> > > +++ b/drivers/gpu/drm/tegra/gem.c
> > > @@ -440,13 +440,6 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
> > >  		err = tegra_bo_iommu_map(tegra, bo);
> > >  		if (err < 0)
> > >  			goto detach;
> > > -	} else {
> > > -		if (bo->sgt->nents > 1) {
> > > -			err = -EINVAL;
> > > -			goto detach;
> > > -		}
> > > -
> > > -		bo->iova = sg_dma_address(bo->sgt->sgl);
> > >  	}
> > >  
> > >  	bo->gem.import_attach = attach;
> > > -- 
> > > 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel