Thomas Zimmermann
2021-Jan-12 07:45 UTC
[PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local operations
Hi Am 08.01.21 um 17:12 schrieb Ruhl, Michael J:>> -----Original Message----- >> From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On Behalf Of >> Thomas Zimmermann >> Sent: Friday, January 8, 2021 4:43 AM >> To: sumit.semwal at linaro.org; christian.koenig at amd.com; >> airlied at redhat.com; daniel at ffwll.ch; maarten.lankhorst at linux.intel.com; >> mripard at kernel.org; kraxel at redhat.com; hdegoede at redhat.com; >> sean at poorly.run; eric at anholt.net; sam at ravnborg.org >> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>; dri-devel at lists.freedesktop.org; >> virtualization at lists.linux-foundation.org; linaro-mm-sig at lists.linaro.org; >> Thomas Zimmermann <tzimmermann at suse.de>; linux- >> media at vger.kernel.org >> Subject: [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local >> operations >> >> The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are >> allowed to pin the buffer or acquire the buffer's reservation object >> lock. >> >> This is a problem for callers that only require a short-term mapping >> of the buffer without the pinning, or callers that have special locking >> requirements. These may suffer from unnecessary overhead or interfere >> with regular pin operations. >> >> The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and >> their rsp callbacks in struct dma_buf_ops provide an alternative without >> pinning or reservation locking. Callers are responsible for these >> operations. >> >> v4: >> * update documentation (Daniel) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch> >> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch> >> --- >> drivers/dma-buf/dma-buf.c | 81 >> +++++++++++++++++++++++++++++++++++++++ >> include/linux/dma-buf.h | 34 ++++++++++++++++ >> 2 files changed, 115 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index b8465243eca2..01f9c74d97fa 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -1295,6 +1295,87 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, >> struct dma_buf_map *map) >> } >> EXPORT_SYMBOL_GPL(dma_buf_vunmap); >> >> +/** >> + * dma_buf_vmap_local - Create virtual mapping for the buffer object into >> kernel >> + * address space. >> + * @dmabuf: [in] buffer to vmap >> + * @map: [out] returns the vmap pointer >> + * >> + * Unlike dma_buf_vmap() this is a short term mapping and will not pin >> + * the buffer. The struct dma_resv for the @dmabuf must be locked until >> + * dma_buf_vunmap_local() is called. >> + * >> + * Returns: >> + * 0 on success, or a negative errno code otherwise. >> + */ >> +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >> *map) >> +{ >> + struct dma_buf_map ptr; >> + int ret = 0; >> + >> + dma_buf_map_clear(map); >> + >> + if (WARN_ON(!dmabuf)) >> + return -EINVAL; >> + >> + dma_resv_assert_held(dmabuf->resv); >> + >> + if (!dmabuf->ops->vmap_local) >> + return -EINVAL; > > You are clearing the map, and then doing the above checks. > > Is it ok to change the map info and then exit on error?In vmap_local map argument returns the mapping's address. Callers are expected to check the return code. But I would expect a careless caller to not check, or check for map being cleared. Clearing it here first is the save thing to do. Best regards Thomas> > Mike > >> + mutex_lock(&dmabuf->lock); >> + if (dmabuf->vmapping_counter) { >> + dmabuf->vmapping_counter++; >> + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >> + *map = dmabuf->vmap_ptr; >> + goto out_unlock; >> + } >> + >> + BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); >> + >> + ret = dmabuf->ops->vmap_local(dmabuf, &ptr); >> + if (WARN_ON_ONCE(ret)) >> + goto out_unlock; >> + >> + dmabuf->vmap_ptr = ptr; >> + dmabuf->vmapping_counter = 1; >> + >> + *map = dmabuf->vmap_ptr; >> + >> +out_unlock: >> + mutex_unlock(&dmabuf->lock); >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_vmap_local); >> + >> +/** >> + * dma_buf_vunmap_local - Unmap a vmap obtained by >> dma_buf_vmap_local. >> + * @dmabuf: [in] buffer to vunmap >> + * @map: [in] vmap pointer to vunmap >> + * >> + * Release a mapping established with dma_buf_vmap_local(). >> + */ >> +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >> dma_buf_map *map) >> +{ >> + if (WARN_ON(!dmabuf)) >> + return; >> + >> + dma_resv_assert_held(dmabuf->resv); >> + >> + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); >> + BUG_ON(dmabuf->vmapping_counter == 0); >> + BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); >> + >> + mutex_lock(&dmabuf->lock); >> + if (--dmabuf->vmapping_counter == 0) { >> + if (dmabuf->ops->vunmap_local) >> + dmabuf->ops->vunmap_local(dmabuf, map); >> + dma_buf_map_clear(&dmabuf->vmap_ptr); >> + } >> + mutex_unlock(&dmabuf->lock); >> +} >> +EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); >> + >> #ifdef CONFIG_DEBUG_FS >> static int dma_buf_debug_show(struct seq_file *s, void *unused) >> { >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index 628681bf6c99..aeed754b5467 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -264,6 +264,38 @@ struct dma_buf_ops { >> >> int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); >> void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map >> *map); >> + >> + /** >> + * @vmap_local: >> + * >> + * Creates a virtual mapping for the buffer into kernel address space. >> + * >> + * This callback establishes short-term mappings for situations where >> + * callers only use the buffer for a bounded amount of time; such as >> + * updates to the framebuffer or reading back contained information. >> + * In contrast to the regular @vmap callback, vmap_local does never >> pin >> + * the buffer to a specific domain or acquire the buffer's reservation >> + * lock. >> + * >> + * This is called with the &dma_buf.resv object locked. Callers must >> hold >> + * the lock until after removing the mapping with @vunmap_local. >> + * >> + * This callback is optional. >> + * >> + * Returns: >> + * >> + * 0 on success or a negative error code on failure. >> + */ >> + int (*vmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >> *map); >> + >> + /** >> + * @vunmap_local: >> + * >> + * Removes a virtual mapping that was established by @vmap_local. >> + * >> + * This callback is optional. >> + */ >> + void (*vunmap_local)(struct dma_buf *dmabuf, struct dma_buf_map >> *map); >> }; >> >> /** >> @@ -501,4 +533,6 @@ int dma_buf_mmap(struct dma_buf *, struct >> vm_area_struct *, >> unsigned long); >> int dma_buf_vmap(struct dma_buf *dmabuf, struct dma_buf_map *map); >> void dma_buf_vunmap(struct dma_buf *dmabuf, struct dma_buf_map >> *map); >> +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map >> *map); >> +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct >> dma_buf_map *map); >> #endif /* __DMA_BUF_H__ */ >> -- >> 2.29.2 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel at lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20210112/754c0bb7/attachment-0001.sig>