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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 6571 bytes --]

Hi

Am 26.11.20 um 13:08 schrieb Christian König:
>> [...]
>> Yeah, I remember a bug report about frequent page-table modifications 
>> wrt to vram helpers. So we implemented the lazy unmapping / vmap 
>> caching, as suggested by Christian back them. My guess is that 
>> anything TTM-based can use a similar pattern. Christian probably knows 
>> the corner cases.
> 
> My memory is failing me, what corner case are you talking about?

Haha! :D What I meant was that if there were corner cases, you'd know 
about them. From your comment, I guess there are none.

Best regards
Thomas

> 
> Christian.
> 
>>
>> CMA seems obviously working correctly already.
>>
>> For SHMEM, I'd have to figure out the reference counting of the pages 
>> involved. My guess is that the vunmap in drm_gem_shmem_vunmap() could 
>> be moved into the free callback, plus a few other modifications.
>>
>> Best regards
>> Thomas
>>
>>>
>>> But if you're willing to do all that conversion of callers, then of
>>> course I'm not stopping you. Not at all, it's great to see that kind
>>> of maze untangled.
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> That should give us at least some way forward to gradually conver 
>>>>> all the
>>>>> drivers and helpers over to dma_resv locking.
>>>>> -Daniel
>>>>>
>>>>>> The pin count is currently maintained by the vmap implementation 
>>>>>> in vram
>>>>>> helpers. Calling vmap is an implicit pin; calling vunmap is an 
>>>>>> implicit
>>>>>> unpin. This prevents eviction in the damage worker. But now I was 
>>>>>> told than
>>>>>> pinning is only for BOs that are controlled by userspace and 
>>>>>> internal users
>>>>>> should acquire the resv lock. So vram helpers have to be fixed, 
>>>>>> actually.
>>>>>>
>>>>>> In vram helpers, unmapping does not mean eviction. The unmap 
>>>>>> operation only
>>>>>> marks the BO as unmappable. The real unmap happens when the 
>>>>>> eviction takes
>>>>>> place. This avoids many modifications to the page tables. IOW an 
>>>>>> unpinned,
>>>>>> unmapped BO will remain in VRAM until the memory is actually needed.
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> So I'm still not seeing how this can go boom.
>>>>>>>
>>>>>>> Now long term it'd be nice to cut everything over to dma_resv 
>>>>>>> locking, but
>>>>>>> the issue there is that beyond ttm, none of the helpers (and few 
>>>>>>> of the
>>>>>>> drivers) use dma_resv. So this is a fairly big uphill battle. Quick
>>>>>>> interim fix seems like the right solution to me.
>>>>>>> -Daniel
>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Best regards
>>>>>>>>> Thomas
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>> Given how this "do the right locking" is a can of worms (and I 
>>>>>>>>>> think
>>>>>>>>>> it's
>>>>>>>>>> worse than what you dug out already) I think the 
>>>>>>>>>> fb_helper.lock hack is
>>>>>>>>>> perfectly good enough.
>>>>>>>>>>
>>>>>>>>>> I'm also somewhat worried that starting to use dma_resv lock 
>>>>>>>>>> in generic
>>>>>>>>>> code, while many helpers/drivers still have their hand-rolled 
>>>>>>>>>> locking,
>>>>>>>>>> will make conversion over to dma_resv needlessly more 
>>>>>>>>>> complicated.
>>>>>>>>>> -Daniel
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> 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@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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>> -- 
>>>> 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

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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 8003 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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