Re: xe vs amdgpu userptr handling

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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@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.
> 
> 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.
> 
> FWIW i915 also keeps an object, but it also pins the pages and relies
> on the shrinker to release that pinning.
> 
> 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.
> > 
>