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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 05.03.24 um 22:58 schrieb Dmitry Osipenko:
> On 2/27/24 13:14, Thomas Zimmermann 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
> The patches look good. I gave them fbtest on virtio-gpu, no problems
> spotted.
>
> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> # virtio-gpu

Great, thanks a lot. If no other reviews come in, I'll land the patchset 
within the next days.

Best regards
Thomas

>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)