[3/9] drm/tegra: gem: Remove premature import restrictions
diff mbox series

Message ID 20191128153741.2380419-4-thierry.reding@gmail.com
State Superseded
Headers show
Series
  • [1/9] drm/tegra: hub: Remove bogus connection mutex check
Related show

Commit Message

Thierry Reding Nov. 28, 2019, 3:37 p.m. UTC
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>
---
 drivers/gpu/drm/tegra/gem.c | 7 -------
 1 file changed, 7 deletions(-)

Comments

Daniel Vetter Nov. 29, 2019, 9:12 a.m. UTC | #1
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?
-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
Thierry Reding Nov. 29, 2019, 10:33 a.m. UTC | #2
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.

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 Nov. 29, 2019, 8:06 p.m. UTC | #3
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

Patch
diff mbox series

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;