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@xxxxxxxxx > > 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
