[v2 13/31] drm/xe/svm: Introduce DRM_XE_SVM kernel config

[Zeng, Oak]

Hi Matt,

I replied a few to those review comments. Then I was interrupted by the v3 respin of this series.

I now come back to revisit those comments, to make sure those comments are either get address in v3, or I reply if I have more comments.

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, April 10, 2024 5:13 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>; Bommu, Krishnaiah
> <krishnaiah.bommu at intel.com>; Thomas.Hellstrom at linux.intel.com; Welty,
> Brian <brian.welty at intel.com>
> Subject: Re: [v2 13/31] drm/xe/svm: Introduce DRM_XE_SVM kernel config
> 
> On Tue, Apr 09, 2024 at 04:17:24PM -0400, Oak Zeng wrote:
> > Introduce a DRM_XE_SVM kernel config entry for
> 
> Maybe consider another name for this? I could see use cases for non-SVM
> where we still want private pages mapped (e.g. VRAM userptrs on
> non-faulting devices). Don't really have suggestion but worth
> considering.

It is first time I heard the concept of VRAM userptr. Do we plan to support it?

On a non-faulting device, you would have to allocate vram for userptr at vm_bind time, right?

> 
> > xe svm feature. xe svm feature allows share
> > virtual address space between CPU and GPU program.
> >
> > v1: Improve commit message (Thomas)
> >     Avoid using #if directive (Thomas)
> >
> > Signed-off-by: Oak Zeng <oak.zeng at intel.com>
> > Co-developed-by: Niranjana Vishwanathapura
> <niranjana.vishwanathapura at intel.com>
> > Signed-off-by: Niranjana Vishwanathapura
> <niranjana.vishwanathapura at intel.com>
> > Cc: Matthew Brost <matthew.brost at intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom at intel.com>
> > Cc: Brian Welty <brian.welty at intel.com>
> > ---
> >  drivers/gpu/drm/xe/Kconfig   | 21 +++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_tile.c |  7 +++++--
> >  2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> > index 449a1ecbc92a..0accb2cb81d6 100644
> > --- a/drivers/gpu/drm/xe/Kconfig
> > +++ b/drivers/gpu/drm/xe/Kconfig
> > @@ -84,6 +84,27 @@ config DRM_XE_FORCE_PROBE
> >  	  4571.
> >
> >  	  Use "!*" to block the probe of the driver for all known devices.
> > +config DRM_XE_SVM
> > +	bool "Enable Shared Virtual Memory support in xe"
> > +	depends on DRM_XE
> > +	depends on ARCH_ENABLE_MEMORY_HOTPLUG
> > +	depends on ARCH_ENABLE_MEMORY_HOTREMOVE
> > +	depends on MEMORY_HOTPLUG
> > +	depends on MEMORY_HOTREMOVE
> > +	depends on ARCH_HAS_PTE_DEVMAP
> > +	depends on SPARSEMEM_VMEMMAP
> > +	depends on ZONE_DEVICE
> > +	depends on DEVICE_PRIVATE
> > +	depends on MMU
> > +	select HMM_MIRROR
> > +	select MMU_NOTIFIER
> > +	default y
> > +	help
> > +	  Choose this option if you want Shared Virtual Memory (SVM)
> > +	  support in xe. With SVM, virtual address space is shared
> > +	  between CPU and GPU. This means any virtual address such
> > +	  as malloc or mmap returns, variables on stack, or global
> > +	  memory pointers, can be used for GPU transparently.
> >
> >  menu "drm/Xe Debugging"
> >  depends on DRM_XE
> > diff --git a/drivers/gpu/drm/xe/xe_tile.c b/drivers/gpu/drm/xe/xe_tile.c
> > index f1c4f9de51df..a1a436912fe3 100644
> > --- a/drivers/gpu/drm/xe/xe_tile.c
> > +++ b/drivers/gpu/drm/xe/xe_tile.c
> > @@ -159,9 +159,12 @@ static int tile_ttm_mgr_init(struct xe_tile *tile)
> >   */
> >  int xe_tile_init_noalloc(struct xe_tile *tile)
> >  {
> > -	struct xe_device *xe = tile_to_xe(tile);
> > +	struct xe_device __maybe_unused *xe;
> 
> Just assign this here blindly? The __maybe_unused should suppress the
> warning CONFIG_DRM_XE_SVM is false and should just compile out if it is.

Did you mean:

struct xe_device __maybe_unused *xe = tile_to_xe(tile);

we have an extra xe assignment when DRM_XE_SVM is not enabled.

Also if we assign blindly, we wouldn't need __maybe_unused, as xe is always assigned (so used)

Oak


> 
> Matt
> 
> >  	int err;
> >
> > +	if (IS_ENABLED(CONFIG_DRM_XE_SVM))
> > +		xe = tile_to_xe(tile);
> > +
> >  	xe_device_mem_access_get(tile_to_xe(tile));
> >
> >  	err = tile_ttm_mgr_init(tile);
> > @@ -177,7 +180,7 @@ int xe_tile_init_noalloc(struct xe_tile *tile)
> >
> >  	xe_tile_sysfs_init(tile);
> >
> > -	if (xe->info.has_usm)
> > +	if (IS_ENABLED(CONFIG_DRM_XE_SVM) && xe->info.has_usm)
> >  		xe_devm_add(tile, &tile->mem.vram);
> >  err_mem_access:
> >  	xe_device_mem_access_put(tile_to_xe(tile));
> > --
> > 2.26.3
> >