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: Tuesday, January 12, 2021 2:45 AM
>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; 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: linaro-mm-sig@lists.linaro.org; Daniel Vetter <daniel.vetter@ffwll.ch>;
>virtualization@lists.linux-foundation.org; dri-devel@lists.freedesktop.org;
>linux-media@vger.kernel.org
>Subject: Re: [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@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?
>
>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.

Ahh, a pre-emptive attempt to make sure the caller doesn't cause more
problems on error. 😊

Thanks,

M

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel