| Message ID | 20191128153741.2380419-4-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tegra: Miscellaneous fixes | expand |
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
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
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
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;