[Intel-gfx] [PATCH v4 06/21] drm/i915: Prepare to dynamic dma-buf locking specification

[Ruhl, Michael J]

>-----Original Message-----
>From: Dmitry Osipenko <digetx at gmail.com>
>Sent: Friday, September 2, 2022 6:39 AM
>To: Ruhl, Michael J <michael.j.ruhl at intel.com>; Dmitry Osipenko
><dmitry.osipenko at collabora.com>; Jani Nikula <jani.nikula at linux.intel.com>;
>Joonas Lahtinen <joonas.lahtinen at linux.intel.com>; Vivi, Rodrigo
><rodrigo.vivi at intel.com>; Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>;
>Thomas Hellström <thomas_os at shipmail.org>; Christian König
><christian.koenig at amd.com>; Chris Wilson <chris at chris-wilson.co.uk>
>Cc: dri-devel at lists.freedesktop.org; linux-kernel at vger.kernel.org; linux-
>media at vger.kernel.org; linaro-mm-sig at lists.linaro.org; amd-
>gfx at lists.freedesktop.org; intel-gfx at lists.freedesktop.org;
>kernel at collabora.com; virtualization at lists.linux-foundation.org; linux-
>rdma at vger.kernel.org; linux-arm-msm at vger.kernel.org; David Airlie
><airlied at linux.ie>; Gerd Hoffmann <kraxel at redhat.com>; Gurchetan Singh
><gurchetansingh at chromium.org>; Chia-I Wu <olvaffe at gmail.com>; Daniel
>Vetter <daniel at ffwll.ch>; Daniel Almeida <daniel.almeida at collabora.com>;
>Gert Wollny <gert.wollny at collabora.com>; Gustavo Padovan
><gustavo.padovan at collabora.com>; Daniel Stone <daniel at fooishbar.org>;
>Tomeu Vizoso <tomeu.vizoso at collabora.com>; Maarten Lankhorst
><maarten.lankhorst at linux.intel.com>; Maxime Ripard
><mripard at kernel.org>; Thomas Zimmermann <tzimmermann at suse.de>;
>Rob Clark <robdclark at gmail.com>; Sumit Semwal
><sumit.semwal at linaro.org>; Pan, Xinhui <Xinhui.Pan at amd.com>; Thierry
>Reding <thierry.reding at gmail.com>; Tomasz Figa <tfiga at chromium.org>;
>Marek Szyprowski <m.szyprowski at samsung.com>; Mauro Carvalho Chehab
><mchehab at kernel.org>; Alex Deucher <alexander.deucher at amd.com>;
>Qiang Yu <yuq825 at gmail.com>; Srinivas Kandagatla
><srinivas.kandagatla at linaro.org>; Amol Maheshwari
><amahesh at qti.qualcomm.com>; Jason Gunthorpe <jgg at ziepe.ca>; Leon
>Romanovsky <leon at kernel.org>; Gross, Jurgen <jgross at suse.com>; Stefano
>Stabellini <sstabellini at kernel.org>; Oleksandr Tyshchenko
><oleksandr_tyshchenko at epam.com>; Tomi Valkeinen <tomba at kernel.org>;
>Russell King <linux at armlinux.org.uk>; Lucas Stach <l.stach at pengutronix.de>;
>Christian Gmeiner <christian.gmeiner at 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