Re: [PATCH 0/9] drm: Revert general use of struct drm_gem_object.dma_buf

[Simona Vetter <simona.vetter@ffwll.ch>]

On Tue, Jul 15, 2025 at 09:41:12AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.07.25 um 14:39 schrieb Simona Vetter:
> > On Fri, Jul 11, 2025 at 11:35:15AM +0200, Thomas Zimmermann wrote:
> > > Revert the use of drm_gem_object.dma_buf back to .import_attach->dmabuf
> > > in the affected places. Also revert any fixes on top. Separates references
> > > to imported and exported DMA bufs within a GEM object; as before.
> > > 
> > > Using the dma_buf as the one authoritative field for the DMA buf turns
> > > out to be fragile. The GEM object's dma_buf pointer can be NULL if
> > > userspace releases the GEM handle too early. Sima mentioned that the fix
> > > in commit 5307dce878d4 ("drm/gem: Acquire references on GEM handles for
> > > framebuffers") is conceptionally broken. Linus still notices boot-up
> > > hangs that might be related.
> > > 
> > > Reverting the whole thing is the only sensible action here.
> > > 
> > > Tested on virtio; and amdgpu, simpledrm plus udl for dma-buf sharing.
> > > 
> > > Thomas Zimmermann (9):
> > >    Revert "drm/framebuffer: Acquire internal references on GEM handles"
> > >    Revert "drm/gem: Acquire references on GEM handles for framebuffers"
> > Ok, I think all the below we should still apply for -fixes, because
> > fundamentally gem_bo->dma_buf is not invariant over the lifetime of the
> > buffer, while gem_bo->import_attach.dmabuf is. And so we blow up.
> > 
> > For display drivers the handle_count reference mostly papers over the
> > issues, but even display drivers are allowed to keep internal references
> > to the underlying gem bo for longer. So there could be a bunch of really
> > tricky bugs lurking.
> > 
> > For render drivers it's even clearer, they don't have framebuffers as
> > objects, so there the fb handle_count references does not help.
> > 
> > I'm not opposed to trying to unify these fields for imported dma_buf, but
> > currently they're just not. Hence all the reverts.
> 
> Thanks for the write up.
> 
> > 
> > The patches also need Fixes: and as needed, cc: stable added for merging.
> > With that and the above text as additional justification added:
> > 
> > Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
> > 
> > Also we'd need to chase down any addiotional conversions that have only
> > landed in -next meanwhile of course.
> > 
> > ₣or the handle_count patches I'm less sure. I don't think they're
> > justified for fixing the gem_bo->dma_buf NULL pointer issues, but they do
> > probably help with the GETFB/2 confusion Christian has pointed out.
> > Personally my preference is:
> > 1. Apply the two reverts.
> > 2. Create an igt testcase for the GETFB confusion
> > 3. Figure out what the right fix for that is, which might or might not be
> > the handle_count reference of drm_fb.
> > 
> > But with my maintainer hat on I don't mind about the exact path, as long
> > as we get there somehow. If you decide to do land the reverts, they also
> > have my:
> > 
> > Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
> 
> Let's first revert all the dma_buf switching in drm-misc and other trees.
> They should
> be easy. If we revert the framebuffer-related A changes first, we might end
> up with
> these intermediate errors again.
> 
> There's no hurry with the framebuffer changes. We can address them after
> upstream
> picked up the dma-buf reverts.

Yeah I think that's the most prudent path forward, otherwise we might
accidentally regress linux-next in an avoidable way.
-Sima

> 
> Best regards
> Thomas
> 
> > 
> > Cheers, Sima
> > 
> > >    Revert "drm/virtio: Use dma_buf from GEM object instance"
> > >    Revert "drm/vmwgfx: Use dma_buf from GEM object instance"
> > >    Revert "drm/etnaviv: Use dma_buf from GEM object instance"
> > >    Revert "drm/prime: Use dma_buf from GEM object instance"
> > >    Revert "drm/gem-framebuffer: Use dma_buf from GEM object instance"
> > >    Revert "drm/gem-shmem: Use dma_buf from GEM object instance"
> > >    Revert "drm/gem-dma: Use dma_buf from GEM object instance"
> > > 
> > >   drivers/gpu/drm/drm_framebuffer.c            | 31 +---------
> > >   drivers/gpu/drm/drm_gem.c                    | 64 +++-----------------
> > >   drivers/gpu/drm/drm_gem_dma_helper.c         |  2 +-
> > >   drivers/gpu/drm/drm_gem_framebuffer_helper.c |  8 ++-
> > >   drivers/gpu/drm/drm_gem_shmem_helper.c       |  4 +-
> > >   drivers/gpu/drm/drm_internal.h               |  2 -
> > >   drivers/gpu/drm/drm_prime.c                  |  8 ++-
> > >   drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c  |  4 +-
> > >   drivers/gpu/drm/virtio/virtgpu_prime.c       |  5 +-
> > >   drivers/gpu/drm/vmwgfx/vmwgfx_gem.c          |  6 +-
> > >   include/drm/drm_framebuffer.h                |  7 ---
> > >   11 files changed, 35 insertions(+), 106 deletions(-)
> > > 
> > > -- 
> > > 2.50.0
> > > 
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch