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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> >
> > 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@xxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
> > > 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
