Re: [PATCH 00/13] drm: Fix reservation locking for pin/unpin and console

["Christian König" <christian.koenig@amd.com>]

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@amd.com>

And Acked-by: Christian König <christian.koenig@amd.com> 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