Nice, looks totally valid to me.
Feel free to add to patch #2, #9, #10, #11 and #12 Reviewed-by:
Christian König <christian.koenig@xxxxxxx>
And Acked-by: Christian König <christian.koenig@xxxxxxx> to the rest.
Regards,
Christian.
Am 27.02.24 um 11:14 schrieb Thomas Zimmermann:
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
