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@suse.de

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

---

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@suse.de, 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@suse.de

+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