Re: [PATCH] drm/xe: Allow mixed mappings for userptr

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

Hi,

On Tue, 2025-09-23 at 19:16 -0700, Matthew Brost wrote:
> On Tue, Sep 23, 2025 at 04:52:30PM +0200, Thomas Hellström wrote:
> > Hi,
> > 
> > On Wed, 2025-09-17 at 11:28 -0700, Matthew Brost wrote:
> > > Compute kernels often issue memory copies immediately after
> > > completion.
> > > If the memory being copied is an SVM pointer that was faulted
> > > into
> > > the
> > > device and then bound via userptr, it is undesirable to move that
> > > memory. Worse, if userptr is mixed between system and device
> > > memory,
> > > the
> > > bind operation may be rejected.
> > > 
> > > Xe already has the necessary plumbing to support userptr with
> > > mixed
> > > mappings. This update modifies GPUSVM's get_pages to correctly
> > > locate
> > > pages in such mixed mapping scenarios.
> > > 
> > > Fixes: ("9e9787414882 drm/xe/userptr: replace xe_hmm with
> > > gpusvm")
> > > Signed-off-bt: Matthew Brost <matthew.brost@intel.com>
> > 
> > s/bt/by/
> > 
> > Perhaps we need to let the PAT index discussion land before we
> > merge
> > this. If we support multiple placements we might need multiple PAT
> > indices...
> > 
> 
> We'd likely just get the pat_index from the userptr bind here, which
> the
> UMD likely expects to correspond to system memory. Either way, let's
> wrap up the PAT index discussion before introducing more complexity.
> Whatever we decide on will probably require a fixes patch here.
> Alternatively, we could take a suboptimal route in the fixes path by
> evicting userptr from device memory upon bind — similar to what was
> necessary in stable kernels before the userptr → GPUSVM transition
> landed.

So assuming that we can use a single PAT index for multiple placements,
and if not we extend the map operation to provide multiple PAT indices
then we should not regress.

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>


> 
> Matt
> 
> > Otherwise LGTM.
> > 
> > Thanks,
> > Thomas
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/drm_gpusvm.c    | 6 ++++--
> > >  drivers/gpu/drm/xe/xe_userptr.c | 1 +
> > >  include/drm/drm_gpusvm.h        | 4 ++++
> > >  3 files changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gpusvm.c
> > > b/drivers/gpu/drm/drm_gpusvm.c
> > > index eeeeb99cfdf6..64c0ea70eae3 100644
> > > --- a/drivers/gpu/drm/drm_gpusvm.c
> > > +++ b/drivers/gpu/drm/drm_gpusvm.c
> > > @@ -1361,7 +1361,8 @@ int drm_gpusvm_get_pages(struct drm_gpusvm
> > > *gpusvm,
> > >  		order = drm_gpusvm_hmm_pfn_to_order(pfns[i], i,
> > > npages);
> > >  		if (is_device_private_page(page) ||
> > >  		    is_device_coherent_page(page)) {
> > > -			if (zdd != page->zone_device_data && i >
> > > 0)
> > > {
> > > +			if (!ctx->allow_mixed &&
> > > +			    zdd != page->zone_device_data && i >
> > > 0)
> > > {
> > >  				err = -EOPNOTSUPP;
> > >  				goto err_unmap;
> > >  			}
> > > @@ -1397,7 +1398,8 @@ int drm_gpusvm_get_pages(struct drm_gpusvm
> > > *gpusvm,
> > >  		} else {
> > >  			dma_addr_t addr;
> > >  
> > > -			if (is_zone_device_page(page) ||
> > > pagemap) {
> > > +			if (is_zone_device_page(page) ||
> > > +			    (pagemap && !ctx->allow_mixed)) {
> > >  				err = -EOPNOTSUPP;
> > >  				goto err_unmap;
> > >  			}
> > > diff --git a/drivers/gpu/drm/xe/xe_userptr.c
> > > b/drivers/gpu/drm/xe/xe_userptr.c
> > > index 91d09af71ced..c628f58c085c 100644
> > > --- a/drivers/gpu/drm/xe/xe_userptr.c
> > > +++ b/drivers/gpu/drm/xe/xe_userptr.c
> > > @@ -54,6 +54,7 @@ int xe_vma_userptr_pin_pages(struct
> > > xe_userptr_vma
> > > *uvma)
> > >  	struct xe_device *xe = vm->xe;
> > >  	struct drm_gpusvm_ctx ctx = {
> > >  		.read_only = xe_vma_read_only(vma),
> > > +		.allow_mixed = true,
> > >  	};
> > >  
> > >  	lockdep_assert_held(&vm->lock);
> > > diff --git a/include/drm/drm_gpusvm.h b/include/drm/drm_gpusvm.h
> > > index 5434048a2ca4..97d2f57914bb 100644
> > > --- a/include/drm/drm_gpusvm.h
> > > +++ b/include/drm/drm_gpusvm.h
> > > @@ -235,6 +235,9 @@ struct drm_gpusvm {
> > >   * @read_only: operating on read-only memory
> > >   * @devmem_possible: possible to use device memory
> > >   * @devmem_only: use only device memory
> > > + * @allow_mixed: Allow mixed mappings in get pages. Mixing
> > > between
> > > system and
> > > + *               single dpagemap is supported, mixing between
> > > multiple dpagemap
> > > + *               is unsupported.
> > >   *
> > >   * Context that is DRM GPUSVM is operating in (i.e. user
> > > arguments).
> > >   */
> > > @@ -245,6 +248,7 @@ struct drm_gpusvm_ctx {
> > >  	unsigned int read_only :1;
> > >  	unsigned int devmem_possible :1;
> > >  	unsigned int devmem_only :1;
> > > +	unsigned int allow_mixed :1;
> > >  };
> > >  
> > >  int drm_gpusvm_init(struct drm_gpusvm *gpusvm,
> >