[PATCH 2/8] drm/xe/svm: Add DRM_XE_SVM kernel config entry

[Zeng, Oak]

> -----Original Message-----
> From: Hellstrom, Thomas <thomas.hellstrom at intel.com>
> Sent: Tuesday, March 19, 2024 5:26 AM
> To: intel-xe at lists.freedesktop.org; Zeng, Oak <oak.zeng at intel.com>
> Cc: Brost, Matthew <matthew.brost at intel.com>; Welty, Brian
> <brian.welty at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 2/8] drm/xe/svm: Add DRM_XE_SVM kernel config entry
> 
> Hi, Oak,
> 
> On Mon, 2024-03-18 at 22:55 -0400, Oak Zeng wrote:
> > DRM_XE_SVM kernel config entry is added so
> > xe svm feature can be configured before kernel
> > compilation.
> 
> Please use imperative language in commit messages.
> 
> 
> >
> > 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 |  4 ++++
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
> > index 1a556d087e63..e244165459c5 100644
> > --- a/drivers/gpu/drm/xe/Kconfig
> > +++ b/drivers/gpu/drm/xe/Kconfig
> > @@ -83,6 +83,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..b52a00a6b5d5 100644
> > --- a/drivers/gpu/drm/xe/xe_tile.c
> > +++ b/drivers/gpu/drm/xe/xe_tile.c
> > @@ -159,7 +159,9 @@ static int tile_ttm_mgr_init(struct xe_tile
> > *tile)
> >   */
> >  int xe_tile_init_noalloc(struct xe_tile *tile)
> >  {
> > +#if IS_ENABLED(CONFIG_DRM_XE_SVM)
> >  	struct xe_device *xe = tile_to_xe(tile);
> > +#endif
> 
> Avoid using conditional compilation in this way inside functions if
> possible, please see below:

Below is doable.

For above, do you want something like:

#if IS_ENABLED(CONFIG_DRM_XE_SVM)
     struct xe_device *xe;
#endif

If (IS_ENABLED(CONFIG_DRM_XE_SVM))
	Xe = tile_to_xe(tile);

It looks like we have to use #if in above code... otherwise, the definition of xe will be not used when DRM_XE_SVM is not defined...

Maybe keep the original for above?
> 
> >  	int err;
> >
> >  	xe_device_mem_access_get(tile_to_xe(tile));
> > @@ -177,8 +179,10 @@ int xe_tile_init_noalloc(struct xe_tile *tile)
> >
> >  	xe_tile_sysfs_init(tile);
> >
> > +#if IS_ENABLED(CONFIG_DRM_XE_SVM)
> >  	if (xe->info.has_usm)
> 
> Could be:
> 	if (IS_ENABLED(CONFIG_DRM_XE_SVM) && tile_to_xe(tile)-
> >info.has_usm)

Will fix.

Oak
> 
> /Thomas
> 
> 
> >  		xe_devm_add(tile, &tile->mem.vram);
> > +#endif
> >  err_mem_access:
> >  	xe_device_mem_access_put(tile_to_xe(tile));
> >  	return err;