Re: [PATCH v2 7/7] dma-buf: Write down some rules for vmap usage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 03.12.20 um 16:26 schrieb Daniel Vetter:
On Thu, Dec 03, 2020 at 03:02:59PM +0100, Thomas Zimmermann wrote:
Dma-buf's vmap and vunmap callbacks are undocumented and various
exporters currently have slightly different semantics for them. Add
documentation on how to implement and use these interfaces correctly.

v2:
	* document vmap semantics in struct dma_buf_ops
	* add TODO item for reviewing and maybe fixing dma-buf exporters

Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

I still don't think this works, we're breaking dma_buf_vmap for everyone
else here.

I removed the text on the importer. These notes for callers in the docs are more or less a consequence of the exporter semantics.

I thought we at least agreed on the exporter semantics in the other thread, didn't we?

What I'm trying to do is to write dome some rules for exporters, even if not all exporters follow them.

Given the circumstances, we should leave out this patch from the patchset.

Best regards
Thomas


---
  Documentation/gpu/todo.rst | 15 +++++++++++++
  include/drm/drm_gem.h      |  4 ++++
  include/linux/dma-buf.h    | 45 ++++++++++++++++++++++++++++++++++++++
  3 files changed, 64 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 009d8e6c7e3c..32bb797a84fc 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -505,6 +505,21 @@ Contact: Thomas Zimmermann <tzimmermann@xxxxxxx>, Christian König, Daniel Vette
  Level: Intermediate
+Enforce rules for dma-buf vmap and pin ops
+------------------------------------------
+
+Exporter implementations of vmap and pin in struct dma_buf_ops (and consequently
+struct drm_gem_object_funcs) use a variety of locking semantics. Some rely on
+the caller holding the dma-buf's reservation lock, some do their own locking,
+some don't require any locking. VRAM helpers even used to pin as part of vmap.
+
+We need to review each exporter and enforce the documented rules.
+
+Contact: Christian König, Daniel Vetter, Thomas Zimmermann <tzimmermann@xxxxxxx>
+
+Level: Advanced
+
+
  Core refactorings
  =================
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 5e6daa1c982f..1864c6a721b1 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -138,6 +138,8 @@ struct drm_gem_object_funcs {
  	 * drm_gem_dmabuf_vmap() helper.
  	 *
  	 * This callback is optional.
+	 *
+	 * See also struct dma_buf_ops.vmap
  	 */
  	int (*vmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
@@ -148,6 +150,8 @@ struct drm_gem_object_funcs {
  	 * drm_gem_dmabuf_vunmap() helper.
  	 *
  	 * This callback is optional.
+	 *
+	 * See also struct dma_buf_ops.vunmap
  	 */
  	void (*vunmap)(struct drm_gem_object *obj, struct dma_buf_map *map);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index cf72699cb2bc..dc81fdc01dda 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -267,7 +267,52 @@ struct dma_buf_ops {
  	 */
  	int (*mmap)(struct dma_buf *, struct vm_area_struct *vma);
+ /**
+	 * @vmap:

There's already a @vmap and @vunamp kerneldoc at the top comment, that
needs to be removed.
-Daniel

+	 *
+	 * Returns a virtual address for the buffer.
+	 *
+	 * Notes to callers:
+	 *
+	 * - Callers must hold the struct dma_buf.resv lock before calling
+	 *   this interface.
+	 *
+	 * - Callers must provide means to prevent the mappings from going
+	 *   stale, such as holding the reservation lock or providing a
+	 *   move-notify callback to the exporter.
+	 *
+	 * Notes to implementors:
+	 *
+	 * - Implementations must expect pairs of @vmap and @vunmap to be
+	 *   called frequently and should optimize for this case.
+	 *
+	 * - Implementations should avoid additional operations, such as
+	 *   pinning.
+	 *
+	 * - Implementations may expect the caller to hold the dma-buf's
+	 *   reservation lock to protect against concurrent calls and
+	 *   relocation.
+	 *
+	 * - Implementations may provide additional guarantees, such as working
+	 *   without holding the reservation lock.
+	 *
+	 * This callback is optional.
+	 *
+	 * Returns:
+	 *
+	 * 0 on success or a negative error code on failure.
+	 */
  	int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map);
+
+	/**
+	 * @vunmap:
+	 *
+	 * Releases the address previously returned by @vmap.
+	 *
+	 * This callback is optional.
+	 *
+	 * See also @vmap()
+	 */
  	void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map);
  };
--
2.29.2



--
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


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux