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

["Hellstrom, Thomas" <thomas.hellstrom@intel.com>]

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@intel.com>
> Co-developed-by: Niranjana Vishwanathapura
> <niranjana.vishwanathapura@intel.com>
> Signed-off-by: Niranjana Vishwanathapura
> <niranjana.vishwanathapura@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@intel.com>
> Cc: Brian Welty <brian.welty@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:

>  	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)

/Thomas


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