[PATCH 4/7] drm/radeon: Pin buffers while they are vmap'ed

Wed Nov 25 08:28:56 UTC 2020

Hi

Am 24.11.20 um 15:06 schrieb Christian König:
> Am 24.11.20 um 14:56 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 24.11.20 um 14:36 schrieb Christian König:
>>> Am 24.11.20 um 13:15 schrieb Thomas Zimmermann:
>>>> [SNIP]
>>>>>>>> First I wanted to put this into drm_gem_ttm_vmap/vunmap(), but 
>>>>>>>> then wondered why ttm_bo_vmap() doe not acquire the lock 
>>>>>>>> internally? I'd expect that vmap/vunmap are close together and 
>>>>>>>> do not overlap for the same BO. 
>>>>>>>
>>>>>>> We have use cases like the following during command submission:
>>>>>>>
>>>>>>> 1. lock
>>>>>>> 2. map
>>>>>>> 3. copy parts of the BO content somewhere else or patch it with 
>>>>>>> additional information
>>>>>>> 4. unmap
>>>>>>> 5. submit BO to the hardware
>>>>>>> 6. add hardware fence to the BO to make sure it doesn't move
>>>>>>> 7. unlock
>>>>>>>
>>>>>>> That use case won't be possible with vmap/vunmap if we move the 
>>>>>>> lock/unlock into it and I hope to replace the kmap/kunmap 
>>>>>>> functions with them in the near term.
>>>>>>>
>>>>>>>> Otherwise, acquiring the reservation lock would require another 
>>>>>>>> ref-counting variable or per-driver code.
>>>>>>>
>>>>>>> Hui, why that? Just put this into drm_gem_ttm_vmap/vunmap() 
>>>>>>> helper as you initially planned.
>>>>>>
>>>>>> Given your example above, step one would acquire the lock, and 
>>>>>> step two would also acquire the lock as part of the vmap 
>>>>>> implementation. Wouldn't this fail (At least during unmap or 
>>>>>> unlock steps) ?
>>>>>
>>>>> Oh, so you want to nest them? No, that is a rather bad no-go.
>>>>
>>>> I don't want to nest/overlap them. My question was whether that 
>>>> would be required. Apparently not.
>>>>
>>>> While the console's BO is being set for scanout, it's protected from 
>>>> movement via the pin/unpin implementation, right?
>>>
>>> Yes, correct.
>>>
>>>> The driver does not acquire the resv lock for longer periods. I'm 
>>>> asking because this would prevent any console-buffer updates while 
>>>> the console is being displayed.
>>>
>>> Correct as well, we only hold the lock for things like command 
>>> submission, pinning, unpinning etc etc....
>>>
>>
>> Thanks for answering my questions.
>>
>>>>
>>>>>
>>>>> You need to make sure that the lock is only taken from the FB path 
>>>>> which wants to vmap the object.
>>>>>
>>>>> Why don't you lock the GEM object from the caller in the generic FB 
>>>>> implementation?
>>>>
>>>> With the current blitter code, it breaks abstraction. if vmap/vunmap 
>>>> hold the lock implicitly, things would be easier.
>>>
>>> Do you have a link to the code?
>>
>> It's the damage blitter in the fbdev code. [1] While it flushes the 
>> shadow buffer into the BO, the BO has to be kept in place. I already 
>> changed it to lock struct drm_fb_helper.lock, but I don't think this 
>> is enough. TTM could still evict the BO concurrently.
> 
> Yeah, that's correct.
> 
> But I still don't fully understand the problem. You just need to change 
> the code like this:
> 
>      mutex_lock(&fb_helper->lock);
>      dma_resv_lock(buffer->gem->resv, NULL);
> 
>      ret = drm_client_buffer_vmap(buffer, &map);
>      if (ret)
>          goto out;
> 
>      dst = map;
>      drm_fb_helper_damage_blit_real(fb_helper, clip, &dst);
> 
>      drm_client_buffer_vunmap(buffer);
> 
> out:
>      dma_resv_unlock(buffer->gem->resv);
>      mutex_unlock(&fb_helper->lock);
> 

Yes, that's the code I had in mind.

> 
> You could abstract that in drm_client functions as well, but I don't 
> really see the value in that.

The fbdev code tries hard to not use GEM directly, but to wrap 
everything behind client interfaces. I'm not sure if I like that, but 
for now I'd stick to this design.

Best regards
Thomas

> 
> Regards,
> Christian.
> 
>> There's no recursion taking place, so I guess the reservation lock 
>> could be acquired/release in drm_client_buffer_vmap/vunmap(), or a 
>> separate pair of DRM client functions could do the locking.
>>
>> Best regards
>> Thomas
>>
>> [1] 
>> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/drm_fb_helper.c?id=ac60f3f3090115d21f028bffa2dcfb67f695c4f2#n394 
>>
>>
>>>
>>> Please note that the reservation lock you need to take here is part 
>>> of the GEM object.
>>>
>>> Usually we design things in the way that the code needs to take a 
>>> lock which protects an object, then do some operations with the 
>>> object and then release the lock again.
>>>
>>> Having in the lock inside the operation can be done as well, but 
>>> returning with it is kind of unusual design.
>>>
>>>> Sorry for the noob questions. I'm still trying to understand the 
>>>> implications of acquiring these locks.
>>>
>>> Well this is the reservation lock of the GEM object we are talking 
>>> about here. We need to take that for a couple of different 
>>> operations, vmap/vunmap doesn't sound like a special case to me.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>
>>> _______________________________________________
>>> 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_0x680DC11D530B7A23.asc
Type: application/pgp-keys
Size: 7435 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20201125/adc45868/attachment-0001.key>
-------------- 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/amd-gfx/attachments/20201125/adc45868/attachment-0001.sig>