Re: [PATCH libdrm] amdgpu: add amdgpu_bo_handle_type_kms_noimport

["Michel Dänzer" <michel-otUistvHUpPR7s880joybQ@public.gmane.org>]

On 2018-07-17 08:50 AM, Christian König wrote:
> Am 16.07.2018 um 18:05 schrieb Michel Dänzer:
>> On 2018-07-13 08:47 PM, Marek Olšák wrote:
>>> On Fri, Jul 13, 2018 at 4:28 AM, Michel Dänzer <michel@daenzer.net>
>>> wrote:
>>>> On 2018-07-12 07:03 PM, Marek Olšák wrote:
>>>>> On Thu, Jul 12, 2018, 3:31 AM Michel Dänzer <michel@daenzer.net>
>>>>> wrote:
>>>>>> What is the rationale for this? I.e. why do you want to not store
>>>>>> some
>>>>>> handles in the hash table?
>>>>>
>>>>> Because I have the option.
>>>> Seems like you're expecting this patch to be accepted without providing
>>>> any real justification for it (here or in the corresponding Mesa
>>>> patch).
>>>> NAK from me if so.
>>> The real justification is implied by the patch. See:
>>> amdgpu_add_handle_to_table
>>> Like I said: There is no risk of regression and it simplifies one
>>> simple case trivially. We shouldn't have to even talk about it.
>> IMO you haven't provided enough justification for adding API which is
>> prone to breakage if used incorrectly.
>>
>> Other opinions?
> 
> I understand the reason why Marek wants to do this, but I agree that
> this is a little bit dangerous if used incorrectly.
> 
> On the other hand I don't see any other way to sanely handle it either.

Sanely handle what exactly? :) I still haven't seen any description of
an actual problem, other than "the handle is stored in the hash table".


>>>> I'd rather add the handle to the hash table in amdgpu_bo_alloc,
>>>> amdgpu_create_bo_from_user_mem and amdgpu_bo_import instead of in
>>>> amdgpu_bo_export, making amdgpu_bo_export(bo,
>>>> amdgpu_bo_handle_type_kms,
>>>> ...) essentially free. In the unlikely (since allocating a BO from the
>>>> kernel is expensive) case that the hash table shows up on profiles, we
>>>> can optimize it.
>>> The hash table isn't very good for high BO counts. The time complexity
>>> of a lookup is O(n).
>> A lookup is only needed in amdgpu_bo_import. amdgpu_bo_alloc and
>> amdgpu_create_bo_from_user_mem can just add the handle to the hash
>> bucket directly.
>>
>> Do you know of, or can you imagine, any workload where amdgpu_bo_import
>> is called often enough for this to be a concern?
> 
> MM interop with GFX usually imports BOs on each frame, so that can hurt
> here.

That's always the same set of BOs in the steady state, right? So it's
easy to make the repeated lookups fast, by moving them to the start of
their hash buckets.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx