RE: [PATCH v4 01/13] dma-buf: Add vmap_local and vnumap_local operations

["Ruhl, Michael J" <michael.j.ruhl@intel.com>]

>-----Original Message-----
>From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>Thomas Zimmermann
>Sent: Friday, January 8, 2021 4:43 AM
>To: sumit.semwal@linaro.org; christian.koenig@amd.com;
>airlied@redhat.com; daniel@ffwll.ch; maarten.lankhorst@linux.intel.com;
>mripard@kernel.org; kraxel@redhat.com; hdegoede@redhat.com;
>sean@poorly.run; eric@anholt.net; sam@ravnborg.org
>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>; dri-devel@lists.freedesktop.org;
>virtualization@lists.linux-foundation.org; linaro-mm-sig@lists.linaro.org;
>Thomas Zimmermann <tzimmermann@suse.de>; linux-
>media@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@suse.de>
>Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>Suggested-by: Daniel Vetter <daniel.vetter@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?

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@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel