[PATCH 1/8] drm/gem: Write down some rules for vmap usage
Thomas Zimmermann
tzimmermann at suse.de
Tue Dec 1 12:33:37 UTC 2020
Hi
Am 01.12.20 um 13:14 schrieb Christian König:
> Am 01.12.20 um 12:30 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 01.12.20 um 11:34 schrieb Christian König:
>>>> [...]
>>>> In patch 6 of this series, there's ast cursor code that acquires two
>>>> BO's reservation locks and vmaps them afterwards. That's probably
>>>> how you intend to use dma_buf_vmap_local.
>>>>
>>>> However, I think it's more logically to have a vmap callback that
>>>> only does the actual vmap. This is all that exporters would have to
>>>> implement.
>>>>
>>>> And with that, build one helper that pins before vmap and one helper
>>>> that gets the resv lock.
>>>
>>> I don't think that this is will work nor is it a good approach.
>>>
>>> See the ast cursor handling for example. You need to acquire two BOs
>>> here, not just one. And this can't be done cleanly with a single vmap
>>> call.
>>
>> That seems to be a misunderstanding.
>>
>> I don't mentioned it explicitly, but there's of course another helper
>> that only vmaps and nothing else. This would be useful for cases like
>> the cursor code. So there would be:
>>
>> dma_buf_vmap() - pin + vmap
>> dma_buf_vmap_local() - lock + vmap
>> dma_buf_vmap_locked() - only vmap; caller has set up the BOs
>
> Well that zoo of helpers will certainly get a NAK from my side.
>
> See interfaces like this should implement simple functions and not hide
> what's actually needs to be done inside the drivers using this interface.
If 9 of 10 invocations use the same pattern, why not put that pattern in
a helper? I see nothing wrong with that.
>
> What we could do is to add a pin count to the DMA-buf and then do
> WARN_ON(dma_buf->pin_count || dma_resv_lock_help(dma_buf->resv)) in the
> vmap/vunmap calls.
Most of the vmap code is either CMA or SHMEM GEM stuff. They don't need
to pin. It's just baggage to them. The TTM stuff that does need pinning
is the minority.
>
>>
>> I did some conversion of drivers that use vram and shmem. They
>> occasionally update a buffer (ast cursors) or flush a BO from system
>> memory to HW (udl, cirrus, mgag200). In terms of these 3 interfaces: I
>> never needed dma_buf_vmap() because pinning was never really required
>> here. Almost all of the cases were handled by dma_buf_vmap_local().
>> And the ast cursor code uses the equivalent of dma_buf_vmap_locked().
>
> Yeah, that is kind of expected. I was already wondering as well why we
> didn't used the reservation lock more extensively.
As a side note, I found only 6 trivial implementations of vmap outside
of drivers/gpu/drm. I cannot find a single implementation of pin there.
What am I missing?
Best regards
Thomas
>
> Regards,
> Christian.
>
>>
>> The driver exporting the buffer would only have to implement vmap()
>> and pin() interfaces. Each does only its one thing and would assume
>> that the caller holds the lock.
>>
>> Best regards
>> Thomas
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> I know that it might require some work on exporting drivers. But
>>>> with your proposal, we probably need another dma-buf callback just
>>>> for vmap_local. (?) That seems even less appealing to me.
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> Trying to shovel both semantics into one interface, depending upon
>>>>> which implementation we have backing the buffer, doesn't work indeed.
>>>>>
>>>>> Also on the pin topic, I think neither interface should require
>>>>> callers to explicitly pin anything. For existing users it should
>>>>> happen automatically behind the scenes imo, that's what they're
>>>>> expecting.
>>>>> -Daniel
>>>>>
>>>>>
>>>>>>> I think we could use what we've done for dynamic dma-buf attachment
>>>>>>> (which also change locking rules) and just have new functions for
>>>>>>> the
>>>>>>> new way (i.e. short term vmap protected by dma_resv lock. Maybe call
>>>>>>> these dma_buf_vmap_local, in the spirit of the new kmap_local which
>>>>>>> are currently under discussion. I think _local suffix is better, for
>>>>>>> otherwise people might do something silly like
>>>>>>>
>>>>>>> dma_resv_lock();
>>>>>>> dma_buf_vmap_locked();
>>>>>>> dma_resv_unlock();
>>>>>>>
>>>>>>> /* actual access maybe even in some other thread */
>>>>>>>
>>>>>>> dma_buf_resv_lock();
>>>>>>> dma_buf_vunmap_unlocked();
>>>>>>> dma_resv_unlock();
>>>>>>>
>>>>>>> _local suffix is better at telling that the resulting pointer has
>>>>>>> very
>>>>>>> limited use (essentially just local to the calling context, if you
>>>>>>> don't change any locking or anything).
>>>>>>
>>>>>> _local sounds good.
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> I think encouraging importers to call dma_buf_pin/unpin isn't a good
>>>>>>> idea. Yes dynamic ones need it, but maybe we should check for that
>>>>>>> somehow in the exporterd interface (atm only amdgpu is using it).
>>>>>>> -Daniel
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Best regards
>>>>>>>> Thomas
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>> Christian.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> That's what I meant with that this approach here is very
>>>>>>>>>> sprawling :-/
>>>>>>>>>> -Daniel
>>>>>>>>>>
>>>>>>>>>>> */
>>>>>>>>>>> int drm_gem_dmabuf_vmap(struct dma_buf *dma_buf, struct
>>>>>>>>>>> dma_buf_map
>>>>>>>>>>> *map)
>>>>>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>>>>>>>>>> index 5e6daa1c982f..7c34cd5ec261 100644
>>>>>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>>>>>> @@ -137,7 +137,21 @@ struct drm_gem_object_funcs {
>>>>>>>>>>> * Returns a virtual address for the buffer. Used by the
>>>>>>>>>>> * drm_gem_dmabuf_vmap() helper.
>>>>>>>>>>> *
>>>>>>>>>>> + * Notes to implementors:
>>>>>>>>>>> + *
>>>>>>>>>>> + * - Implementations must expect pairs of @vmap and
>>>>>>>>>>> @vunmap to be
>>>>>>>>>>> + * called frequently and should optimize for this case.
>>>>>>>>>>> + *
>>>>>>>>>>> + * - Implemenations may expect the caller to hold the
>>>>>>>>>>> GEM object's
>>>>>>>>>>> + * reservation lock to protect against concurrent
>>>>>>>>>>> calls and
>>>>>>>>>>> relocation
>>>>>>>>>>> + * of the GEM object.
>>>>>>>>>>> + *
>>>>>>>>>>> + * - Implementations may provide additional guarantees
>>>>>>>>>>> (e.g.,
>>>>>>>>>>> working
>>>>>>>>>>> + * without holding the reservation lock).
>>>>>>>>>>> + *
>>>>>>>>>>> * This callback is optional.
>>>>>>>>>>> + *
>>>>>>>>>>> + * See also drm_gem_dmabuf_vmap()
>>>>>>>>>>> */
>>>>>>>>>>> int (*vmap)(struct drm_gem_object *obj, struct
>>>>>>>>>>> dma_buf_map *map);
>>>>>>>>>>> @@ -148,6 +162,8 @@ struct drm_gem_object_funcs {
>>>>>>>>>>> * drm_gem_dmabuf_vunmap() helper.
>>>>>>>>>>> *
>>>>>>>>>>> * This callback is optional.
>>>>>>>>>>> + *
>>>>>>>>>>> + * See also @vmap.
>>>>>>>>>>> */
>>>>>>>>>>> void (*vunmap)(struct drm_gem_object *obj, struct
>>>>>>>>>>> dma_buf_map
>>>>>>>>>>> *map);
>>>>>>>>>>> --
>>>>>>>>>>> 2.29.2
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> dri-devel mailing list
>>>>>>>>> dri-devel at 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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201201/2c751387/attachment-0001.sig>
More information about the dri-devel
mailing list