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

[Christian König]

Am 26.11.20 um 13:14 schrieb Thomas Zimmermann:
> 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.

Ah, ok :) I was wondering for a moment if I have missed something.

Cheers,
Christian.

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