Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <20c88807-8513-a816-aed9-5cd67eb5c1ed@collabora.com>
Date:   Sat, 25 Mar 2023 17:58:41 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v13 01/10] drm/shmem-helper: Switch to reservation lock
From:   Dmitry Osipenko <dmitry.osipenko@collabora.com>
To:     =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>
Cc:     dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        kernel@collabora.com, virtualization@lists.linux-foundation.org,
        intel-gfx@lists.freedesktop.org, David Airlie <airlied@gmail.com>,
        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>,
        Gustavo Padovan <gustavo.padovan@collabora.com>,
        Daniel Stone <daniel@fooishbar.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Qiang Yu <yuq825@gmail.com>,
        Steven Price <steven.price@arm.com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
        Rob Herring <robh@kernel.org>
References: <20230314022659.1816246-1-dmitry.osipenko@collabora.com>
 <20230314022659.1816246-2-dmitry.osipenko@collabora.com>
 <6b5644cf-6229-f99b-d429-a45d724493ee@collabora.com>
Content-Language: en-US
In-Reply-To: <6b5644cf-6229-f99b-d429-a45d724493ee@collabora.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 3/15/23 16:46, Dmitry Osipenko wrote:
> On 3/14/23 05:26, Dmitry Osipenko wrote:
>> @@ -633,7 +605,10 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>>  		return ret;
>>  	}
>>  
>> +	dma_resv_lock(shmem->base.resv, NULL);
>>  	ret = drm_gem_shmem_get_pages(shmem);
>> +	dma_resv_unlock(shmem->base.resv);
> 
> Intel CI reported locking problem [1] here. It actually was also
> reported for v12, but I missed that report because of the other noisy
> reports.
> 
> [1]
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_114671v2/shard-snb5/igt@prime_vgem@sync@rcs0.html
> 
> The test does the following:
> 
> 1. creates vgem
> 2. export it do dmabuf
> 3. mmaps dmabuf
> 
> There is an obvious deadlock there. The DRM code assumes that mmap() is
> unlocked, while for dma-buf it's locked. I'll see how to fix it for v14.
> 

Christian, there is a deadlock problem in drm_gem_shmem_mmap() once we
move drm-shmem to use resv lock. The current dma-buf locking policy
claims that importer holds the lock for mmap(), but DRM code assumes
that obj->mmap() handles the locking itself and then the same
obj->mmap() code path is used by both dma-buf DRM and a usual DRM object
paths. Hence importer -> dma_buf_mmap_internal()[takes the lock] ->
exporter -> drm_gem_shmem_mmap()[takes the lock] deadlocks.

I was looking at how to fix it and to me the best option is to change
the dma-buf locking policy, making exporter responsible for handling the
resv lock. Changing DRM code mmap lockings might be possible too [moving
locking to drm_gem_mmap_obj()], but will be very messy and doesn't feel
intuitive.

Want to get yours thoughts on this before sending out the dma-buf mmap()
policy-change patch. Does the new mmap() locking policy sound good to
you? Thanks!

-- 
Best regards,
Dmitry