xe vs amdgpu userptr handling

[Christian König]

Am 08.02.24 um 10:43 schrieb Thomas Hellström:
> On Thu, 2024-02-08 at 10:38 +0100, Thomas Hellström wrote:
>> Hi,
>>
>> On Thu, 2024-02-08 at 07:30 +0100, Christian König wrote:
>>> Am 08.02.24 um 01:36 schrieb Dave Airlie:
>>>> Just cc'ing some folks. I've also added another question.
>>>>
>>>> On Wed, 7 Feb 2024 at 21:08, Maíra Canal <mcanal at igalia.com>
>>>> wrote:
>>>>> Adding another point to this discussion, would it make sense to
>>>>> somehow
>>>>> create a generic structure that all drivers, including shmem
>>>>> drivers,
>>>>> could use it?
>>>>>
>>>>> Best Regards,
>>>>> - Maíra
>>>>>
>>>>> On 2/7/24 03:56, Dave Airlie wrote:
>>>>>> I'm just looking over the userptr handling in both drivers,
>>>>>> and
>>>>>> of
>>>>>> course they've chosen different ways to represent things.
>>>>>> Again
>>>>>> this
>>>>>> is a divergence that is just going to get more annoying over
>>>>>> time and
>>>>>> eventually I'd like to make hmm/userptr driver independent as
>>>>>> much as
>>>>>> possible, so we get consistent semantics in userspace.
>>>>>>
>>>>>> AFAICS the main difference is that amdgpu builds the userptr
>>>>>> handling
>>>>>> inside a GEM object in the kernel, whereas xe doesn't bother
>>>>>> creating
>>>>>> a holding object and just handles things directly in the VM
>>>>>> binding
>>>>>> code.
>>>>>>
>>>>>> Is this just different thinking at different times here?
>>>>>> like since we have VM BIND in xe, it made sense not to bother
>>>>>> creating
>>>>>> a gem object for userptrs?
>>>>>> or is there some other advantages to going one way or the
>>>>>> other?
>>>>>>
>>>> So the current AMD code uses hmm to do userptr work, but xe
>>>> doesn't
>>>> again, why isn't xe using hmm here, I thought I remembered
>>>> scenarios
>>>> where plain mmu_notifiers weren't sufficient.
>>> Well amdgpu is using hmm_range_fault because that was made
>>> mandatory
>>> for
>>> the userptr handling.
>>>
>>> And yes pure mmu_notifiers are not sufficient, you need the
>>> sequence
>>> number + retry mechanism hmm_range_fault provides.
>>>
>>> Otherwise you open up security holes you can push an elephant
>>> through.
>>>
>>> Regards,
>>> Christian.
>> Xe also uses a retry mechanism, so no security hole. The main
>> difference is we use get_user_pages() + retry instead of
>> hmm_range_fault() + retry, with a net effect we're probably holding
>> the
>> page refcounts a little longer, but we drop it immediately after
>> obtaining the page pointers, and dirtying the pages.
>>
>> That said we should move over to hmm_range_fault() to align. I think
>> this was only a result of limited hmm knowledge when the xe code was
>> written.

Yeah, that makes sense. In this case it's just a missing cleanup.

>> As for the userptr not using a backing object in Xe, AFAIK that was
>> because a backing object was deemed unnecessary with VM_BIND. Joonas
>> or
>> Matt can probably provide more detail, but if we're going to do an
>> hmmptr, and have userptr only be a special case of that, then the
>> object is ofc out of the question.

Well how do you then store the dma_fence of the operation?

At least my long term plan was to move some of the logic necessary for 
hmm_range_fault based userptr handling into common GEM code.

One puzzle piece of that is the drm_exec object and for that to work 
userptrs would need to be based on GEM objects as well.

>> FWIW i915 also keeps an object, but it also pins the pages and relies
>> on the shrinker to release that pinning.

Well what exactly do you pin? The pages backing the userptr?

Cause that would be a no-go as well I think since it badly clashes with 
NUMA migrations and transparent huge pages.

Regards,
Christian.

>>
>> So yes, some common code would come in handy. From looking at all
>> implementations I'd
>>
>> - Use hmm_range_fault() - Probably want to temporarily get and lock
>> the
>> pages to dirty them at fault time, though, if gpu mapping is write-
>> enabled.
>> - Don't use a backing object - To be able to unify hmmptr and userptr
> Oh, and to clarify for people that haven't been following the gpuvm
> discussions and the xe SVM discussions,
>
> hmmptr is sparsely populated on-demand allocated (fault aware) and can
> do migration.
> userptr is allocated upfront and can't do migration.
>
> Idea has been to create helpers for these in drm_gpuvm().
>
> /Thomas
>
>
>
>> Thanks,
>> Thomas
>>
>>
>>
>>
>>
>>
>>
>>>> Dave.