Hi Am 01.12.20 um 10:13 schrieb Christian König:
Am 01.12.20 um 09:32 schrieb Thomas Zimmermann:Hi Am 30.11.20 um 16:33 schrieb Christian König:Am 30.11.20 um 16:30 schrieb Daniel Vetter:On Mon, Nov 30, 2020 at 01:04:26PM +0100, Thomas Zimmermann wrote:This is a dma-buf hook, which means just documenting the rules you'd like to have here isn't enough. We need to roll this out at the dma-buf level,Mapping a GEM object's buffer into kernel address space prevents the buffer from being evicted from VRAM, which in turn may result in out-of-memory errors. It's therefore required to only vmap GEM BOs forshort periods of time; unless the GEM implementation provides additionalguarantees. Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx> --- drivers/gpu/drm/drm_prime.c | 6 ++++++ include/drm/drm_gem.h | 16 ++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7db55fce35d8..9c9ece9833e0 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -669,6 +669,12 @@ EXPORT_SYMBOL(drm_gem_unmap_dma_buf);* callback. Calls into &drm_gem_object_funcs.vmap for device specific handling.* The kernel virtual address is returned in map. *+ * To prevent the GEM object from being relocated, callers must hold the GEM + * object's reservation lock from when calling this function until releasing the + * mapping. Holding onto a mapping and the associated reservation lock for an + * unbound time may result in out-of-memory errors. Calls to drm_gem_dmabuf_vmap() + * should therefore be accompanied by a call to drm_gem_dmabuf_vunmap().+ * * Returns 0 on success or a negative errno code otherwise.and enforce it. Enforce it = assert_lock_heldRoll out = review everyone. Because this goes through dma-buf it'll come back through shmem helpers (and other helpers and other subsystems) backto any driver using vmap for gpu buffers. This includes the media subsystem, and the media subsystem definitely doesn't cope with justtemporarily mapping buffers. So there we need to pin them, which I thinkmeans we'll need 2 version of dma_buf_vmap - one that's temporary and requires we hold dma_resv lock, the other requires that the buffer is pinned.OR start to proper use the dma_buf_pin/dma_buf_unpin functions which I added to cover this use case as well.While I generally agree, here are some thoughts:I found all generic pin functions useless, because they don't allow for specifying where to pin. With fbdev emulation, this means that console buffers might never make it to VRAM for scanout. If anything, the policy should be that pin always pins in HW-accessible memory.Yes, completely agree. The major missing part here are some kind of usage flags what we want to do with the buffer.
Not sure, but wasn't that not wanted by someone? I don't really remember.Given Daniel's reply about pin, it really feels like we have contradicting policies for this interface.
Pin has quite a bit of overhead (more locking, buffer movement), so it should be the second choice after regular vmap. To make both work together, pin probably relies on holding the reservation lock internally.There is a dma_resv_assert_held() at the beginning of those two functions.Therefore I think we still would want some additional helpers, such as:pin_unlocked(), which acquires the resv lock, calls regular pin and then drops the resv lock. Same for unpin_unlocked()vmap_pinned(), which enforces that the buffer has been pinned and then calls regalar vmap. Same for vunmap_pinned()I would rather open code that in each driver, hiding the locking logic in some midlayer is usually not a good idea.
These helpers are less about hiding logic and more about making drivers do the right thing. The idea behind pin_unlocked() is that it drops the reservation lock immediately before returning. I suspect that some driver might not do that with an open-coded version. And vmap_pinned() does check for the BO to be pinned. Regular vmap would assert for the reservation lock instead.
Best regards Thomas
Regards, Christian.A typical pattern with these functions would look like this. drm_gem_object bo; dma_buf_map map; init() { pin_unlocked(bo); vmap_pinned(bo, map); } worker() { begin_cpu_access() // access bo via map end_cpu_access() } fini() { vunmap_pinned(bo, map); unpin_unlocked(bo); } init() while (...) { worker() } fini() Is that reasonable for media drivers? Best regards ThomasCheers, Christian.That's what I meant with that this approach here is very sprawling :-/ -Daniel_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
