On Tue, Feb 27, 2024 at 6:38 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: > > Dma-buf locking semantics require the caller of pin and unpin to hold > the buffer's reservation lock. Fix DRM to adhere to the specs. This > enables to fix the locking in DRM's console emulation. Similar changes > for vmap and mmap have been posted at [1][2] > > Most DRM drivers and memory managers acquire the buffer object's > reservation lock within their GEM pin and unpin callbacks. This > violates dma-buf locking semantics. We get away with it because PRIME > does not provide pin/unpin, but attach/detach, for which the locking > semantics is correct. > > Patches 1 to 8 rework DRM GEM code in various implementations to > acquire the reservation lock when entering the pin and unpin callbacks. > This prepares them for the next patch. Drivers that are not affected > by these patches either don't acquire the reservation lock (amdgpu) > or don't need preparation (loongson). > > Patch 9 moves reservation locking from the GEM pin/unpin callbacks > into drm_gem_pin() and drm_gem_unpin(). As PRIME uses these functions > internally it still gets the reservation lock. > > With the updated GEM callbacks, the rest of the patchset fixes the > fbdev emulation's buffer locking. Fbdev emulation needs to keep its > GEM buffer object inplace while updating its content. This required > a implicit pinning and apparently amdgpu didn't do this at all. > > Patch 10 introduces drm_client_buffer_vmap_local() and _vunmap_local(). > The former function map a GEM buffer into the kernel's address space > with regular vmap operations, but keeps holding the reservation lock. > The _vunmap_local() helper undoes the vmap and releases the lock. The > updated GEM callbacks make this possible. Between the two calls, the > fbdev emulation can update the buffer content without have the buffer > moved or evicted. Update fbdev-generic to use vmap_local helpers, > which fix amdgpu. The idea of adding a "local vmap" has previously been > attempted at [3] in a different form. > > Patch 11 adds implicit pinning to the DRM client's regular vmap > helper so that long-term vmap'ed buffers won't be evicted. This only > affects fbdev-dma, but GEM DMA helpers don't require pinning. So > there are no practical changes. > > Patches 12 and 13 remove implicit pinning from the vmap and vunmap > operations in gem-vram and qxl. These pin operations are not supposed > to be part of vmap code, but were required to keep the buffers in place > for fbdev emulation. With the conversion o ffbdev-generic to to > vmap_local helpers, that code can finally be removed. > > Tested with amdgpu, nouveau, radeon, simpledrm and vc4. > > [1] https://patchwork.freedesktop.org/series/106371/ > [2] https://patchwork.freedesktop.org/series/116001/ > [3] https://patchwork.freedesktop.org/series/84732/ > > Thomas Zimmermann (13): > drm/gem-shmem: Acquire reservation lock in GEM pin/unpin callbacks > drm/gem-vram: Acquire reservation lock in GEM pin/unpin callbacks > drm/msm: Provide msm_gem_get_pages_locked() > drm/msm: Acquire reservation lock in GEM pin/unpin callback > drm/nouveau: Provide nouveau_bo_{pin,unpin}_locked() > drm/nouveau: Acquire reservation lock in GEM pin/unpin callbacks > drm/qxl: Provide qxl_bo_{pin,unpin}_locked() > drm/qxl: Acquire reservation lock in GEM pin/unpin callbacks > drm/gem: Acquire reservation lock in drm_gem_{pin/unpin}() > drm/fbdev-generic: Fix locking with drm_client_buffer_vmap_local() > drm/client: Pin vmap'ed GEM buffers > drm/gem-vram: Do not pin buffer objects for vmap > drm/qxl: Do not pin buffer objects for vmap > > drivers/gpu/drm/drm_client.c | 92 ++++++++++++++++++--- > drivers/gpu/drm/drm_fbdev_generic.c | 4 +- > drivers/gpu/drm/drm_gem.c | 34 +++++++- > drivers/gpu/drm/drm_gem_shmem_helper.c | 6 +- > drivers/gpu/drm/drm_gem_vram_helper.c | 101 ++++++++++-------------- > drivers/gpu/drm/drm_internal.h | 2 + > drivers/gpu/drm/loongson/lsdc_gem.c | 13 +-- > drivers/gpu/drm/msm/msm_gem.c | 20 ++--- > drivers/gpu/drm/msm/msm_gem.h | 4 +- > drivers/gpu/drm/msm/msm_gem_prime.c | 20 +++-- > drivers/gpu/drm/nouveau/nouveau_bo.c | 43 +++++++--- > drivers/gpu/drm/nouveau/nouveau_bo.h | 2 + > drivers/gpu/drm/nouveau/nouveau_prime.c | 8 +- > drivers/gpu/drm/qxl/qxl_object.c | 26 +++--- > drivers/gpu/drm/qxl/qxl_object.h | 2 + > drivers/gpu/drm/qxl/qxl_prime.c | 4 +- > drivers/gpu/drm/radeon/radeon_prime.c | 11 --- > drivers/gpu/drm/vmwgfx/vmwgfx_gem.c | 25 ++---- > include/drm/drm_client.h | 10 +++ > include/drm/drm_gem.h | 3 + > include/drm/drm_gem_shmem_helper.h | 7 +- > 21 files changed, 265 insertions(+), 172 deletions(-) > > > base-commit: 7291e2e67dff0ff573900266382c9c9248a7dea5 > prerequisite-patch-id: bdfa0e6341b30cc9d7647172760b3473007c1216 > prerequisite-patch-id: bc27ac702099f481890ae2c7c4a9c531f4a62d64 > prerequisite-patch-id: f5d4bf16dc45334254527c2e31ee21ba4582761c > prerequisite-patch-id: 734c87e610747779aa41be12eb9e4c984bdfa743 > prerequisite-patch-id: 0aa359f6144c4015c140c8a6750be19099c676fb > prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24 > prerequisite-patch-id: cbc453ee02fae02af22fbfdce56ab732c7a88c36 > -- > 2.43.2 > That's a really nice cleanup! I already gave a r-b for 9/13. For the rest: Acked-by: Zack Rusin <zack.rusin@xxxxxxxxxxxx> z