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

[Jianfeng Liu <liujianfeng1994@gmail.com>]

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.

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?

> (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