[PATCH v2 7/7] dma-buf: Write down some rules for vmap usage
Christian König
christian.koenig at amd.com
Wed Dec 9 11:06:58 UTC 2020
Am 09.12.20 um 11:16 schrieb Daniel Vetter:
> [SNIP]
>>>> At least I would be fine with that. For now amdgpu is the only exporter who
>>>> implements the explicit pin/unpin semantics anyway.
>>> Yup, I think that makes sense (if it works). Maybe we could use something
>>> like:
>>>
>>> a) dma_buf pin exists, driver is dynamic. This means dma_buf_vmap needs to
>>> first pin, then call ->vmap. dma_buf_vmap_local otoh can directly call
>>> ->vmap since the exporter relies on either a pin or dma_resv_lock.
>>>
>>> b) dma_buf pin not implement, driver is a legacy pile. dma_buf_vmap will
>>> pin (somewhere at least, or rely on some implicit pin), dma_buf_vmap_local
>>> doesn't work and should fail.
>> I think I read in the dma-buf documentation that pin is supposed to put
>> the BO in a domain that is suitable for scanout. Now I don't really
>> trust this to work. Amdgpu, radeon and nouveau put it into the GTT
>> region. Qxl appears to put it wherever it is.
> Uh that sounds wrong, or at least not the full complexity. ->pin's
> main use right now is to freeze the dma-buf into system memory when
> there's a non-dynamic attachement for a dynamic exporter. There's also
> a dma_buf_pin call in amdgpu, and I think that only works for scanout
> on integrated gpu. Pinning to vram would break sharing for all
> existing dma-buf users.
>
> Christian can you perhaps explain when amdgpu uses dma_buf_pin()?
You guys are writing mails faster than I can answer :)
dma_buf_pin() should be used when the buffer can't move any more and it
is preferred to bin in a location which is accessible by all devices,
that usually means system memory.
> My take is the ->pin callback should clarify that it should pin into
> system memory, for legacy (non-dynamic) dma-buf users.
I though I've wrote that on the documentation for the pin callback, but
looks like I've forgotten to do this.
> And dma_buf_pin() should gain some kerneldoc which then states that "This
> should only be used in limited use cases like scanout and not for
> temporary pin operations." I think the problem with this kerneldoc is
> simply that it tries to document the exporter and importer side of the
> contract in one place and makes a mess of it, plus leaves the actual
> importer side function undocumented. I think the kerneldoc also
> predates the final patch version, and we didn't update it fully.
That's a really good point and I noticed this mess before as well.
How about this: We document the exporter side on the dma-buf function
table (because the exporter should implement those), and the importer
side on the dma-buf functions which then uses the exporter functions.
Regards,
Christian.
>>> I think for less transition work fbdev helpers could first try
>>> dma_resv_lock + dma_buf_vmap_local, if that fails, drop the dma_resv_lock
>>> and do the pinning dma_buf_vmap. That way we don't have to convert shmem
>>> helpers over to dma_resv locking, which should help.
>> I have meanwhile made a patchset that updates helpers for cma, shmem and
>> vram with vmap_local; and converts fbdev emulation as well. It needs a
>> bit more testing before being posted.
> Nice, even better!
> -Daniel
>
>> Best regards
>> Thomas
>>
>>> And ttm drivers would do the new clean interface, so at least everyone
>>> using dma_resv today is all fine. Intel's conversion to dma_resv lock is
>>> in-flight, but that needs a conversion to the dynamic interface anyway,
>>> the current code splats. And dynamic brings means explicit pin/unpin
>>> callbacks, so should be good too.
>>> -Daniel
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F400054%2F%3Fseries%3D83765%26rev%3D1&data=04%7C01%7Cchristian.koenig%40amd.com%7C0a737292d0b643fd671a08d89c2b8d77%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431058168569423%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nT89kiTCaZKRORv%2B49%2F2zmW2zV5I6UaArkiKDx%2F4%2Bfw%3D&reserved=0
>>>>>
>>>>>> - The new one, which allows vmapping with just dma_resv locked, and
>>>>>> should
>>>>>> have some caching in exporters.
>>>>>>
>>>>>> Breaking code and then adding todos about that is kinda not so cool
>>>>>> approach here imo.
>>>>>>
>>>>>> Also I guess ttm_bo_vmap should have a check that either the buffer is
>>>>>> pinned, or dma_resv_lock is held.
>>>>>>
>>>>>> Cheers, Daniel
>>>>>>
>>>>>>
>>>>>>
>>>>>>> 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 at 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 at 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
>>>>>>>>>
>>>>>>> --
>>>>>>> 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>> --
>> 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
>>
>
More information about the dri-devel
mailing list