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.
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
