-----Original Message----- From: Dmitry Osipenko digetx@gmail.com Sent: Friday, September 2, 2022 6:39 AM To: Ruhl, Michael J michael.j.ruhl@intel.com; Dmitry Osipenko dmitry.osipenko@collabora.com; Jani Nikula jani.nikula@linux.intel.com; Joonas Lahtinen joonas.lahtinen@linux.intel.com; Vivi, Rodrigo rodrigo.vivi@intel.com; Tvrtko Ursulin tvrtko.ursulin@linux.intel.com; Thomas Hellström thomas_os@shipmail.org; Christian König christian.koenig@amd.com; Chris Wilson chris@chris-wilson.co.uk Cc: dri-devel@lists.freedesktop.org; linux-kernel@vger.kernel.org; linux- media@vger.kernel.org; linaro-mm-sig@lists.linaro.org; amd- gfx@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; kernel@collabora.com; virtualization@lists.linux-foundation.org; linux- rdma@vger.kernel.org; linux-arm-msm@vger.kernel.org; David Airlie airlied@linux.ie; Gerd Hoffmann kraxel@redhat.com; Gurchetan Singh gurchetansingh@chromium.org; Chia-I Wu olvaffe@gmail.com; Daniel Vetter daniel@ffwll.ch; Daniel Almeida daniel.almeida@collabora.com; Gert Wollny gert.wollny@collabora.com; Gustavo Padovan gustavo.padovan@collabora.com; Daniel Stone daniel@fooishbar.org; Tomeu Vizoso tomeu.vizoso@collabora.com; Maarten Lankhorst maarten.lankhorst@linux.intel.com; Maxime Ripard mripard@kernel.org; Thomas Zimmermann tzimmermann@suse.de; Rob Clark robdclark@gmail.com; Sumit Semwal sumit.semwal@linaro.org; Pan, Xinhui Xinhui.Pan@amd.com; Thierry Reding thierry.reding@gmail.com; Tomasz Figa tfiga@chromium.org; Marek Szyprowski m.szyprowski@samsung.com; Mauro Carvalho Chehab mchehab@kernel.org; Alex Deucher alexander.deucher@amd.com; Qiang Yu yuq825@gmail.com; Srinivas Kandagatla srinivas.kandagatla@linaro.org; Amol Maheshwari amahesh@qti.qualcomm.com; Jason Gunthorpe jgg@ziepe.ca; Leon Romanovsky leon@kernel.org; Gross, Jurgen jgross@suse.com; Stefano Stabellini sstabellini@kernel.org; Oleksandr Tyshchenko oleksandr_tyshchenko@epam.com; Tomi Valkeinen tomba@kernel.org; Russell King linux@armlinux.org.uk; Lucas Stach l.stach@pengutronix.de; Christian Gmeiner christian.gmeiner@gmail.com Subject: Re: [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking specification
02.09.2022 13:31, Dmitry Osipenko пишет:
01.09.2022 17:02, Ruhl, Michael J пишет: ...
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -331,7 +331,19 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915, continue; }
/** dma_buf_unmap_attachment() requires reservation to be* locked. The imported GEM shouldn't share reservation lock,* so it's safe to take the lock.*/if (obj->base.import_attach)i915_gem_object_lock(obj, NULL);There is a lot of stuff going here. Taking the lock may be premature...
__i915_gem_object_pages_fini(obj);The i915_gem_dmabuf.c:i915_gem_object_put_pages_dmabuf is where unmap_attachment is actually called, would it make more sense to make do the locking there?
The __i915_gem_object_put_pages() is invoked with a held reservation lock, while freeing object is a special time when we know that GEM is unused.
The __i915_gem_free_objects() was taking the lock two weeks ago until the change made Chris Wilson [1] reached linux-next.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-
next.git/commit/?id=2826d447fbd60e6a05e53d5f918bceb8c04e315c
I don't think we can take the lock within i915_gem_object_put_pages_dmabuf(), it may/should deadlock other code
paths.
On the other hand, we can check whether the GEM's refcount number is zero in i915_gem_object_put_pages_dmabuf() and then take the lock if it's zero.
Also, seems it should be possible just to bail out from i915_gem_object_put_pages_dmabuf() if refcount=0. The further drm_prime_gem_destroy() will take care of unmapping. Perhaps this could be the best option, I'll give it a test.
i915_gem_object_put_pages() is uses the SG, and the usage for drm_prim_gem_destroy()
from __i915_gem_free_objects() doesn't use the SG because it has been "freed" already, I am not sure if that would work...
Hmm.. with that in mind, maybe moving the base.import_attach check to __i915_gem_object_put_pages with your attach check?
atomic_set(&obj->mm.pages_pin_count, 0); if (obj->base.import) i915_gem_object_lock(obj, NULL);
__i915_gem_object_put_pages(obj);
if (obj->base.import) i915_gem_object_unlock(obj, NULL); GEM_BUG_ON(i915_gem_object_has_pages(obj));
Pretty much one step up from the dmabuf interface, but we are guaranteed to not have any pinned pages?
The other caller of __i915_gem_object_pages_fini is the i915_ttm move_notify which should not conflict (export side, not import side).
Since it appears that not locking during the clean up is desirable, trying to make sure take the lock is taken at the last moment might be the right path?
M