Re: [RFC PATCH v1 1/2] dma-buf: keep DMABUF_DEBUG off by default

[Rob Clark]

On Thu, Sep 24, 2026 at 8:23 AM Rob Clark <rob.clark@xxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 24, 2026 at 7:54 AM Jianfeng Liu <liujianfeng1994@xxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > On Thu, Sep 24, 2026 at 7:01 AM Rob Clark wrote:
> > > So the assessment of what is going wrong looks pretty wrong..  VM_BIND
> > > should never lead to iommu_map_sgtable() (which is never used for gpu
> > > per-process pgtables), for example.. but is used for mapping for
> > > scanout.  And pages are never used for mapping in either path.
> > >
> > > However there are a few places where sg->length is used (in iommu code
> > > and msm).. AFAICT dma_buf_wrap_sg_table() zeroing out sg->length is
> > > what the actual problem here is, rather than any use of struct page.
> >
> > Thanks for the correction - you're right, I mis-traced the GPU path.
> > The per-process pgtable mapping goes through
> > msm_iommu_pagetable_map(), which walks the sg_table with sg->length
> > and sg_phys(). With the wrapper zeroing sg->length it iterates the
> > entries, maps nothing at all and still returns 0 - which explains
> > the UCHE translation faults without any error anywhere, and is a
> > nastier failure mode than the async-bind-failure story I wrote in
> > the commit log.
> >
> > With that corrected picture, DMABUF_DEBUG=y breaks msm in the map
> > paths themselves: msm_iommu_pagetable_map() for the GPU and
> > iommu_map_sg() for scanout both consume sg->length, and
> > dma_buf_wrap_sg_table() zeroes it, so every mapping of a
> > page-stripped sg_table silently maps nothing. On top of that msm
> > also uses sg_phys() in those paths and
> > drm_prime_sg_to_page_array() for the page array, so even with
> > sg->length preserved, page-less entries would map garbage
> > physical addresses instead of failing loudly.
>
> oh, ugg.. I overlooked that sg_phys() uses the pages under the hood..
> I think using sg_dma_len()/sg_dma_address() instead should be fine in
> msm.
>
> Otherwise, (other than the fault path) obj->pages is mostly a proxy
> for "is backing storage allocated".. we could just use obj->sgt for
> that instead.
>
> > So it looks like this needs work on both sides:
> >
> >  - dma-buf: preserve sg->length in the debug wrapper, so
> >    sg->length consumers at least fail loudly instead of silently
> >    mapping nothing. I think that is what both Christian's "we
> >    should probably change that" and your "zeroing out sg->length
> >    is what the actual problem is" are pointing at.
> >
> >  - msm: stop consuming struct page and sg->length of imported
> >    sg_tables, i.e. build the GPU and scanout mappings from the DMA
> >    addresses, plus the drm_prime_sg_to_page_array() cleanup.
> >
> > Is that the right split, and is there a preferred direction for
> > the msm side?
>
> On the msm side, I think we should be using
> sg_dma_len()/sg_dma_address() for the immediate issue.

Hmm, so sg_dma_len()/sg_dma_address() are zero for page backed sgt, so
it isn't quite so simple as I imagined.  In theory, it needs
dma_map_sgtable() but not really sure that is going to dtrt with the
way iommu is used on our platform.  And I'm probably not going to have
time to dig into this until after next week.

Please send a revert for commit 143755bdabaa9 ("dma-buf: Make
DMABUF_DEBUG default to y on
DEBUG_KERNEL kernels") for v7.3

BR,
-R

> Maybe we can just do the same on the iommu side, but not sure what
> various sharp edges might exist with other iommu users.  We could also
> just stop using iommu_map_sgtable() and inline our own iommu_map_sg()?
>  It looks like there are a few other drm drivers that use
> iommu_map_sgtable() so it might be worth at least trying to fix this
> in iommu.  Presumably if there are issues with that approach an iommu
> maintainer would speak up.
>
> For the longer term cleanup, I think we should use msm_obj->sgt
> instead of msm_obj->pages for "is there backing storage", and allow
> msm_obj->pages to be NULL for imported dma_buf's
>
> Given the late stage for v7.3, the immediate thing we should do is
> revert the CONFIG_DMABUF_DEBUG change IMHO, and try again once we have
> sorted out what to do about the iommu_map_sgtable() path.
>
> BR,
> -R
>
> > > (And yeah, I should get rid of use of drm_prime_sg_to_page_array()..
> > > but that cleanup that I haven't found time for shouldn't be the
> > > problem here.)
> >
> > Agreed on it not being what produced the faults - but it is part
> > of the same contract problem, see below.
> >
> > Also answering Bryan's review of patch 2, which is in a different
> > branch of this thread:
> >
> > On Thu, Sep 24, 2026 at 10:36 AM Bryan O'Donoghue wrote:
> > > Why is the fix Adreno specific ?
> > >
> > > Shouldn't this function be ammended with
> > >
> > >  > +    if (filled != npages)
> > >
> > > instead ?
> >
> > It isn't meant to be - msm_gem_import() is the shared GPU/DPU
> > import path. And putting the fill-count check into
> > drm_prime_sg_to_page_array() itself would indeed be the better
> > generic version of that guard; I checked the other callers
> > (etnaviv, omapdrm, vmwgfx, xen) and none of them expects a
> > partial fill either. But with the corrected analysis above, the
> > page array isn't what produced the GPU faults, so neither variant
> > is a real fix. I'm not asking for either patch to be merged - the
> > series is a bug report with code attached, sent to get exactly
> > this discussion going, which is also why it carries the RFC
> > prefix.
> >
> > > This very much looks like an LLM generated patch - the commit log, the
> > > large comment in the code and TBH the solution too.
> >
> > Sorry about that - the patches were drafted with LLM assistance
> > and I should have declared that up front. Any later version will
> > carry a proper declaration.
> >
> > Thanks all!
> > Jianfeng