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. 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. > 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 for devices who only want to do a vmap of the buffer, isn't it? 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 >
