On Tue, Apr 15, 2025 at 02:52:54PM +0200, Christian König wrote: > Am 15.04.25 um 14:40 schrieb Thomas Zimmermann: > > Hi > > > > Am 15.04.25 um 14:19 schrieb Christian König: > >> Am 15.04.25 um 12:45 schrieb Thomas Zimmermann: > >>> Hi > >>> > >>> Am 15.04.25 um 11:39 schrieb Christian König: > >>>> Am 15.04.25 um 11:20 schrieb Thomas Zimmermann: > >>>>> Test struct drm_gem_object.import_attach to detect imported objects > >>>>> during cleanup. At that point, the imported DMA buffer might have > >>>>> already been released and the dma_buf field is NULL. The object's > >>>>> free callback then does a cleanup as for native objects. > >>>> I don't think that this is a good idea. > >>>> > >>>> The DMA-buf is separately reference counted through the import > >>>> attachment. > >>> I understand that it's not ideal, but testing for import_attach to be set > >>> is what we currently do throughout drivers. Putting this behind an > >>> interface is already a step forward. > >>> > >>>>> Happens for calls to drm_mode_destroy_dumb_ioctl() that eventually > >>>>> clear the dma_buf field in drm_gem_object_exported_dma_buf_free(). > >>>> That is for exported DMA-buf and should *NEVER* be used for imported > >>>> ones. > >>> Did you look at the discussion at the Closes tag? Where else could > >>> dma-buf be cleared? > >> Yeah, I've seen that. The solution is just completely wrong. > >> > >> See for the export case obj->dma_buf points to the exported DMA-buf and > >> causes a circle dependency when not set to NULL when the last handle is > >> released. > >> > >> But for the import case obj->dma_buf is actually not that relevant. > >> Instead obj->import_attach->dma_buf should be used. > > > > So if I understand correctly, the tests in that helper should be done by > > looking at import_attach->dma_buf. > > At least in theory yes. IIRC we also set obj->dma_buf to the same value > as import_attach->dma_buf for convenient so that code doesn't need to > check both.
Uh, given that we already have a confusion between in the importer and exporter cases I think it'd be better to more strictly separate them than to mix them up even more for convenience. We need more clarity here instead. > But it can be that obj->dma_buf is already NULL while the import > attachment is still in the process of being cleaned up. So there are a > couple of cases where we have to look at obj->import_attach->dma_buf. Yeah this sounds better imo. > > The long-term goal is to make import_attach optional because its setup > > requires a DMA-capable device. > > HUI WHAT? > > Dmitry and I put quite some effort into being able to create an import_attach > without the requirement to have a DMA-capable device. > > The last puzzle piece of that landed a month ago in the form of this patch > here: > > commit b72f66f22c0e39ae6684c43fead774c13db24e73 > Author: Christian König <christian.koe...@amd.com> > Date: Tue Feb 11 17:20:53 2025 +0100 > > dma-buf: drop caching of sg_tables > > That was purely for the transition from static to dynamic dma-buf > handling and can be removed again now. > > Signed-off-by: Christian König <christian.koe...@amd.com> > Reviewed-by: Simona Vetter <simona.vet...@ffwll.ch> > Reviewed-by: Dmitry Osipenko <dmitry.osipe...@collabora.com> > Link: > https://patchwork.freedesktop.org/patch/msgid/20250211163109.12200-5-christian.koe...@amd.com > > When you don't create an import attachment the exporter wouldn't know that > his buffer is actually used which is usually a quite bad idea. This is im entirely unrelated because ... > This is for devices who only want to do a vmap of the buffer, isn't it? ... it's for the vmap only case, where you might not even have a struct device. Or definitely not a reasonable one, like maybe a faux_bus device or some device on a bus that really doesn't do dma (e.g. spi or i2c), and where hence dma_buf_map_attachment is just something you never ever want to do. I think we might want to transform obj->import_attach into a union or tagged pointer or something like that, which can cover both cases. And maybe a drm_gem_bo_imported_dma_buf() helper that gives you the dma_buf no matter what if it's imported, or NULL if it's allocated on that drm_device? Cheers, Sima > > Regards, > Christian. > > > > > Best regards > > Thomas > > > >> > >> Regards, > >> Christian. > >> > >>> Best regards > >>> Thomas > >>> > >>>> Regards, > >>>> Christian. > >>>> > >>>>> Signed-off-by: Thomas Zimmermann <tzimmerm...@suse.de> > >>>>> Fixes: b57aa47d39e9 ("drm/gem: Test for imported GEM buffers with > >>>>> helper") > >>>>> Reported-by: Andy Yan <andys...@163.com> > >>>>> Closes: > >>>>> https://lore.kernel.org/dri-devel/38d09d34.4354.196379aa560.coremail.andys...@163.com/ > >>>>> Tested-by: Andy Yan <andys...@163.com> > >>>>> Cc: Thomas Zimmermann <tzimmerm...@suse.de> > >>>>> Cc: Anusha Srivatsa <asriv...@redhat.com> > >>>>> Cc: Christian König <christian.koe...@amd.com> > >>>>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com> > >>>>> Cc: Maxime Ripard <mrip...@kernel.org> > >>>>> Cc: David Airlie <airl...@gmail.com> > >>>>> Cc: Simona Vetter <sim...@ffwll.ch> > >>>>> Cc: Sumit Semwal <sumit.sem...@linaro.org> > >>>>> Cc: "Christian König" <christian.koe...@amd.com> > >>>>> Cc: dri-devel@lists.freedesktop.org > >>>>> Cc: linux-me...@vger.kernel.org > >>>>> Cc: linaro-mm-...@lists.linaro.org > >>>>> --- > >>>>> include/drm/drm_gem.h | 8 +++++++- > >>>>> 1 file changed, 7 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h > >>>>> index 9b71f7a9f3f8..f09b8afcf86d 100644 > >>>>> --- a/include/drm/drm_gem.h > >>>>> +++ b/include/drm/drm_gem.h > >>>>> @@ -589,7 +589,13 @@ static inline bool > >>>>> drm_gem_object_is_shared_for_memory_stats(struct drm_gem_obje > >>>>> static inline bool drm_gem_is_imported(const struct drm_gem_object > >>>>> *obj) > >>>>> { > >>>>> /* The dma-buf's priv field points to the original GEM object. */ > >>>>> - return obj->dma_buf && (obj->dma_buf->priv != obj); > >>>>> + return (obj->dma_buf && (obj->dma_buf->priv != obj)) || > >>>>> + /* > >>>>> + * TODO: During object release, the dma-buf might already > >>>>> + * be gone. For now keep testing import_attach, but > >>>>> + * this should be removed at some point. > >>>>> + */ > >>>>> + obj->import_attach; > >>>>> } > >>>>> #ifdef CONFIG_LOCKDEP > > > -- Simona Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
