[PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Ramesh Errabolu]

Enables users to update the default size of buffer used
in migration either from Sysmem to VRAM or vice versa.
The param GOBM refers to granularity of buffer migration,
and is specified in terms of log(numPages(buffer)). It
facilitates users of unregistered memory to control GOBM,
albeit at a coarse level

Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
 drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
 4 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e8c284aea1f2..73dd816b01f2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -237,6 +237,7 @@ extern int sched_policy;
 extern bool debug_evictions;
 extern bool no_system_mem_limit;
 extern int halt_if_hws_hang;
+extern uint amdgpu_svm_attr_gobm;
 #else
 static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
 static const bool __maybe_unused debug_evictions; /* = false */
@@ -313,6 +314,9 @@ extern int amdgpu_wbrf;
 /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
 #define AMDGPU_SWCTF_EXTRA_DELAY		50
 
+/* Default size of buffer to use in migrating buffer */
+#define AMDGPU_SVM_ATTR_GOBM	    9
+
 struct amdgpu_xcp_mgr;
 struct amdgpu_device;
 struct amdgpu_irq_src;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index b9529948f2b2..09c501753a3b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
 char *amdgpu_disable_cu;
 char *amdgpu_virtual_display;
 bool enforce_isolation;
+
+/* Specifies the default size of buffer to use in
+ * migrating buffer from Sysmem to VRAM and vice
+ * versa
+ *
+ * GOBM - Granularity of Buffer Migration
+ *
+ * Defined as log2(sizeof(buffer)/PAGE_SIZE)
+ */
+uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
+
 /*
  * OverDrive(bit 14) disabled by default
  * GFX DCS(bit 19) disabled by default
@@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
 MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
 module_param_named(msi, amdgpu_msi, int, 0444);
 
+/**
+ * DOC: svm_attr_gobm (uint)
+ * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa
+ */
+MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
+module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
+
 /**
  * DOC: lockup_timeout (string)
  * Set GPU scheduler timeout value in ms.
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
index 9ae9abc6eb43..c2e54b18c167 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
@@ -868,6 +868,18 @@ struct svm_range_list {
 	struct task_struct		*faulting_task;
 	/* check point ts decides if page fault recovery need be dropped */
 	uint64_t			checkpoint_ts[MAX_GPU_INSTANCE];
+
+	/* Indicates the default size to use in migrating
+	 * buffers of a process from Sysmem to VRAM and vice
+	 * versa. The max legal value cannot be greater than
+	 * 0x3F
+	 *
+	 * @note: A side effect of this symbol being part of
+	 * struct svm_range_list is that it forces all buffers
+	 * of the process of unregistered kind to use the same
+	 * size in buffer migration
+	 */
+	uint8_t attr_gobm;
 };
 
 /* Process data */
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index b44dec90969f..78c78baddb1f 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
 }
 
 static void
-svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
-				 uint8_t *granularity, uint32_t *flags)
+svm_range_set_default_attributes(int32_t *location,
+			int32_t *prefetch_loc, uint32_t *flags)
 {
 	*location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
 	*prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
-	*granularity = 9;
 	*flags =
 		KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
 }
@@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
 		bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
 			    MAX_GPU_INSTANCE);
 
+	prange->granularity = svms->attr_gobm;
 	svm_range_set_default_attributes(&prange->preferred_loc,
-					 &prange->prefetch_loc,
-					 &prange->granularity, &prange->flags);
+				&prange->prefetch_loc, &prange->flags);
 
 	pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
 
@@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
 
 	*is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);
 
+	/* Determine the starting and ending page of prange */
 	start_limit = max(vma->vm_start >> PAGE_SHIFT,
-		      (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
+		      (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
 	end_limit = min(vma->vm_end >> PAGE_SHIFT,
-		    (unsigned long)ALIGN(addr + 1, 2UL << 8));
+		    (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));
+
 	/* First range that starts after the fault address */
 	node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
 	if (node) {
@@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
 		if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
 			bitmap_set(svms->bitmap_supported, i, 1);
 
+	/* Bind granularity of buffer migration, either
+	 * the default size or one specified by the user
+	 */
+	svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
+	pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);
+
 	return 0;
 }
 
@@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
 	node = interval_tree_iter_first(&svms->objects, start, last);
 	if (!node) {
 		pr_debug("range attrs not found return default values\n");
-		svm_range_set_default_attributes(&location, &prefetch_loc,
-						 &granularity, &flags_and);
+		granularity = svms->attr_gobm;
+		svm_range_set_default_attributes(&location,
+					&prefetch_loc, &flags_and);
 		flags_or = flags_and;
 		if (p->xnack_enabled)
 			bitmap_copy(bitmap_access, svms->bitmap_supported,
-- 
2.34.1

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Philip Yang]

[-- Attachment #1: Type: text/html, Size: 8764 bytes --]

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Kasiviswanathan, Harish]

[-- Attachment #1: Type: text/plain, Size: 8552 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

Some comments inline.

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Philip Yang
Sent: Wednesday, August 28, 2024 11:49 AM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter



On 2024-08-26 15:34, Ramesh Errabolu wrote:

Enables users to update the default size of buffer used

in migration either from Sysmem to VRAM or vice versa.

The param GOBM refers to granularity of buffer migration,



[HK]: Include the module param name here.



and is specified in terms of log(numPages(buffer)). It

facilitates users of unregistered memory to control GOBM,
it is used for both registered and unregistered range cases.




albeit at a coarse level



Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>

---

 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++

 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++

 drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------

 4 files changed, 51 insertions(+), 9 deletions(-)



diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

index e8c284aea1f2..73dd816b01f2 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

@@ -237,6 +237,7 @@ extern int sched_policy;

 extern bool debug_evictions;

 extern bool no_system_mem_limit;

 extern int halt_if_hws_hang;

+extern uint amdgpu_svm_attr_gobm;

 #else

 static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;

 static const bool __maybe_unused debug_evictions; /* = false */

@@ -313,6 +314,9 @@ extern int amdgpu_wbrf;

 /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */

 #define AMDGPU_SWCTF_EXTRA_DELAY              50



+/* Default size of buffer to use in migrating buffer */

+#define AMDGPU_SVM_ATTR_GOBM       9

+



[HK]: Don’t think this #define is need. You can just use 9 directly as other module parameters for default value.



 struct amdgpu_xcp_mgr;

 struct amdgpu_device;

 struct amdgpu_irq_src;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

index b9529948f2b2..09c501753a3b 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

@@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;

 char *amdgpu_disable_cu;

 char *amdgpu_virtual_display;

 bool enforce_isolation;

+

+/* Specifies the default size of buffer to use in

+ * migrating buffer from Sysmem to VRAM and vice

+ * versa

+ *

+ * GOBM - Granularity of Buffer Migration

+ *

+ * Defined as log2(sizeof(buffer)/PAGE_SIZE)



[HK]: Use of log is unnecessary here and other places. You can just state is size is exponent of 2.





+ */

+uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;

+

 /*

  * OverDrive(bit 14) disabled by default

  * GFX DCS(bit 19) disabled by default

@@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);

 MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");

 module_param_named(msi, amdgpu_msi, int, 0444);



+/**

+ * DOC: svm_attr_gobm (uint)

+ * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa

+ */

+MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

+module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

+

 /**

  * DOC: lockup_timeout (string)

  * Set GPU scheduler timeout value in ms.

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

index 9ae9abc6eb43..c2e54b18c167 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

@@ -868,6 +868,18 @@ struct svm_range_list {

  struct task_struct       *faulting_task;

  /* check point ts decides if page fault recovery need be dropped */

  uint64_t                 checkpoint_ts[MAX_GPU_INSTANCE];

+

+ /* Indicates the default size to use in migrating

+  * buffers of a process from Sysmem to VRAM and vice

+  * versa. The max legal value cannot be greater than

+  * 0x3F

+  *

+  * @note: A side effect of this symbol being part of

+  * struct svm_range_list is that it forces all buffers

+  * of the process of unregistered kind to use the same

+  * size in buffer migration

+  */
The comment is good enough, note maybe misleading, not needed.




+ uint8_t attr_gobm;

 };



 /* Process data */

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

index b44dec90969f..78c78baddb1f 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

@@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)

 }



 static void

-svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,

-                         uint8_t *granularity, uint32_t *flags)

+svm_range_set_default_attributes(int32_t *location,

+                int32_t *prefetch_loc, uint32_t *flags)

 {

  *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

  *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

- *granularity = 9;

as svm_range_set_default_attributes is called in multiple places, add new parameter struct svm_range_list *svms, to remove the duplicate code.

*granularity = svms->attr_gobm;



  *flags =

         KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;

 }

@@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,

         bitmap_copy(prange->bitmap_access, svms->bitmap_supported,

                     MAX_GPU_INSTANCE);



+ prange->granularity = svms->attr_gobm;

  svm_range_set_default_attributes(&prange->preferred_loc,

-                                &prange->prefetch_loc,

-                                &prange->granularity, &prange->flags);

+                        &prange->prefetch_loc, &prange->flags);



  pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);



@@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,



  *is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);



+ /* Determine the starting and ending page of prange */
this comment is not helpful.




  start_limit = max(vma->vm_start >> PAGE_SHIFT,

-              (unsigned long)ALIGN_DOWN(addr, 2UL << 8));

+              (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
1UL << p->svms.attr_gobm



  end_limit = min(vma->vm_end >> PAGE_SHIFT,

-            (unsigned long)ALIGN(addr + 1, 2UL << 8));

+            (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));

1UL << p->svms.attr_gobm

Regards,

Philip



+

  /* First range that starts after the fault address */

  node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);

  if (node) {

@@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)

         if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))

                 bitmap_set(svms->bitmap_supported, i, 1);



+ /* Bind granularity of buffer migration, either

+  * the default size or one specified by the user

+  */

+ svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

+ pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);

+

  return 0;

 }



@@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,

  node = interval_tree_iter_first(&svms->objects, start, last);

  if (!node) {

         pr_debug("range attrs not found return default values\n");

-   svm_range_set_default_attributes(&location, &prefetch_loc,

-                                        &granularity, &flags_and);

+        granularity = svms->attr_gobm;

+   svm_range_set_default_attributes(&location,

+                          &prefetch_loc, &flags_and);

         flags_or = flags_and;

         if (p->xnack_enabled)

                 bitmap_copy(bitmap_access, svms->bitmap_supported,

[-- Attachment #2: Type: text/html, Size: 18690 bytes --]

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Chen, Xiaogang]

Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY 
api that allows user space to set migration granularity per prange. If 
both got set which will take precedence?

Regards

Xiaogang

On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Enables users to update the default size of buffer used
> in migration either from Sysmem to VRAM or vice versa.
> The param GOBM refers to granularity of buffer migration,
> and is specified in terms of log(numPages(buffer)). It
> facilitates users of unregistered memory to control GOBM,
> albeit at a coarse level
>
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>   4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e8c284aea1f2..73dd816b01f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -237,6 +237,7 @@ extern int sched_policy;
>   extern bool debug_evictions;
>   extern bool no_system_mem_limit;
>   extern int halt_if_hws_hang;
> +extern uint amdgpu_svm_attr_gobm;
>   #else
>   static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>   static const bool __maybe_unused debug_evictions; /* = false */
> @@ -313,6 +314,9 @@ extern int amdgpu_wbrf;
>   /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>   #define AMDGPU_SWCTF_EXTRA_DELAY               50
>
> +/* Default size of buffer to use in migrating buffer */
> +#define AMDGPU_SVM_ATTR_GOBM       9
> +
>   struct amdgpu_xcp_mgr;
>   struct amdgpu_device;
>   struct amdgpu_irq_src;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b9529948f2b2..09c501753a3b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>   char *amdgpu_disable_cu;
>   char *amdgpu_virtual_display;
>   bool enforce_isolation;
> +
> +/* Specifies the default size of buffer to use in
> + * migrating buffer from Sysmem to VRAM and vice
> + * versa
> + *
> + * GOBM - Granularity of Buffer Migration
> + *
> + * Defined as log2(sizeof(buffer)/PAGE_SIZE)
> + */
> +uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
> +
>   /*
>    * OverDrive(bit 14) disabled by default
>    * GFX DCS(bit 19) disabled by default
> @@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>   MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(msi, amdgpu_msi, int, 0444);
>
> +/**
> + * DOC: svm_attr_gobm (uint)
> + * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa
> + */
> +MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
> +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
> +
>   /**
>    * DOC: lockup_timeout (string)
>    * Set GPU scheduler timeout value in ms.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 9ae9abc6eb43..c2e54b18c167 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -868,6 +868,18 @@ struct svm_range_list {
>          struct task_struct              *faulting_task;
>          /* check point ts decides if page fault recovery need be dropped */
>          uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];
> +
> +       /* Indicates the default size to use in migrating
> +        * buffers of a process from Sysmem to VRAM and vice
> +        * versa. The max legal value cannot be greater than
> +        * 0x3F
> +        *
> +        * @note: A side effect of this symbol being part of
> +        * struct svm_range_list is that it forces all buffers
> +        * of the process of unregistered kind to use the same
> +        * size in buffer migration
> +        */
> +       uint8_t attr_gobm;
>   };
>
>   /* Process data */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index b44dec90969f..78c78baddb1f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>   }
>
>   static void
> -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
> -                                uint8_t *granularity, uint32_t *flags)
> +svm_range_set_default_attributes(int32_t *location,
> +                       int32_t *prefetch_loc, uint32_t *flags)
>   {
>          *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>          *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
> -       *granularity = 9;
>          *flags =
>                  KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>   }
> @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>                  bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>                              MAX_GPU_INSTANCE);
>
> +       prange->granularity = svms->attr_gobm;
>          svm_range_set_default_attributes(&prange->preferred_loc,
> -                                        &prange->prefetch_loc,
> -                                        &prange->granularity, &prange->flags);
> +                               &prange->prefetch_loc, &prange->flags);
>
>          pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>
> @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
>
>          *is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);
>
> +       /* Determine the starting and ending page of prange */
>          start_limit = max(vma->vm_start >> PAGE_SHIFT,
> -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
> +                     (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
>          end_limit = min(vma->vm_end >> PAGE_SHIFT,
> -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
> +                   (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));
> +
>          /* First range that starts after the fault address */
>          node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>          if (node) {
> @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>                  if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>                          bitmap_set(svms->bitmap_supported, i, 1);
>
> +       /* Bind granularity of buffer migration, either
> +        * the default size or one specified by the user
> +        */
> +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
> +       pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);
> +
>          return 0;
>   }
>
> @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>          node = interval_tree_iter_first(&svms->objects, start, last);
>          if (!node) {
>                  pr_debug("range attrs not found return default values\n");
> -               svm_range_set_default_attributes(&location, &prefetch_loc,
> -                                                &granularity, &flags_and);
> +               granularity = svms->attr_gobm;
> +               svm_range_set_default_attributes(&location,
> +                                       &prefetch_loc, &flags_and);
>                  flags_or = flags_and;
>                  if (p->xnack_enabled)
>                          bitmap_copy(bitmap_access, svms->bitmap_supported,
> --
> 2.34.1
>

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Errabolu, Ramesh]

Response inline

Regards,
Ramesh
 
-----Original Message-----
From: Chen, Xiaogang <Xiaogang.Chen@amd.com> 
Sent: Wednesday, August 28, 2024 2:43 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter


Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?

Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found. 

Regards

Xiaogang

On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Enables users to update the default size of buffer used in migration 
> either from Sysmem to VRAM or vice versa.
> The param GOBM refers to granularity of buffer migration, and is 
> specified in terms of log(numPages(buffer)). It facilitates users of 
> unregistered memory to control GOBM, albeit at a coarse level
>
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>   4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e8c284aea1f2..73dd816b01f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -237,6 +237,7 @@ extern int sched_policy;
>   extern bool debug_evictions;
>   extern bool no_system_mem_limit;
>   extern int halt_if_hws_hang;
> +extern uint amdgpu_svm_attr_gobm;
>   #else
>   static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>   static const bool __maybe_unused debug_evictions; /* = false */ @@ 
> -313,6 +314,9 @@ extern int amdgpu_wbrf;
>   /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>   #define AMDGPU_SWCTF_EXTRA_DELAY               50
>
> +/* Default size of buffer to use in migrating buffer */
> +#define AMDGPU_SVM_ATTR_GOBM       9
> +
>   struct amdgpu_xcp_mgr;
>   struct amdgpu_device;
>   struct amdgpu_irq_src;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b9529948f2b2..09c501753a3b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>   char *amdgpu_disable_cu;
>   char *amdgpu_virtual_display;
>   bool enforce_isolation;
> +
> +/* Specifies the default size of buffer to use in
> + * migrating buffer from Sysmem to VRAM and vice
> + * versa
> + *
> + * GOBM - Granularity of Buffer Migration
> + *
> + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint 
> +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
> +
>   /*
>    * OverDrive(bit 14) disabled by default
>    * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@ 
> module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>   MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(msi, amdgpu_msi, int, 0444);
>
> +/**
> + * DOC: svm_attr_gobm (uint)
> + * Size of buffer to use in migrating buffer from Sysmem to VRAM and 
> +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as 
> +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB"); 
> +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
> +
>   /**
>    * DOC: lockup_timeout (string)
>    * Set GPU scheduler timeout value in ms.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h 
> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 9ae9abc6eb43..c2e54b18c167 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -868,6 +868,18 @@ struct svm_range_list {
>          struct task_struct              *faulting_task;
>          /* check point ts decides if page fault recovery need be dropped */
>          uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];
> +
> +       /* Indicates the default size to use in migrating
> +        * buffers of a process from Sysmem to VRAM and vice
> +        * versa. The max legal value cannot be greater than
> +        * 0x3F
> +        *
> +        * @note: A side effect of this symbol being part of
> +        * struct svm_range_list is that it forces all buffers
> +        * of the process of unregistered kind to use the same
> +        * size in buffer migration
> +        */
> +       uint8_t attr_gobm;
>   };
>
>   /* Process data */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index b44dec90969f..78c78baddb1f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>   }
>
>   static void
> -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
> -                                uint8_t *granularity, uint32_t *flags)
> +svm_range_set_default_attributes(int32_t *location,
> +                       int32_t *prefetch_loc, uint32_t *flags)
>   {
>          *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>          *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
> -       *granularity = 9;
>          *flags =
>                  KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>   }
> @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>                  bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>                              MAX_GPU_INSTANCE);
>
> +       prange->granularity = svms->attr_gobm;
>          svm_range_set_default_attributes(&prange->preferred_loc,
> -                                        &prange->prefetch_loc,
> -                                        &prange->granularity, &prange->flags);
> +                               &prange->prefetch_loc, 
> + &prange->flags);
>
>          pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>
> @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct 
> kfd_process *p, int64_t addr,
>
>          *is_heap_stack = vma_is_initial_heap(vma) || 
> vma_is_initial_stack(vma);
>
> +       /* Determine the starting and ending page of prange */
>          start_limit = max(vma->vm_start >> PAGE_SHIFT,
> -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
> +                     (unsigned long)ALIGN_DOWN(addr, 1 << 
> + p->svms.attr_gobm));
>          end_limit = min(vma->vm_end >> PAGE_SHIFT,
> -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
> +                   (unsigned long)ALIGN(addr + 1, 1 << 
> + p->svms.attr_gobm));
> +
>          /* First range that starts after the fault address */
>          node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>          if (node) {
> @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>                  if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>                          bitmap_set(svms->bitmap_supported, i, 1);
>
> +       /* Bind granularity of buffer migration, either
> +        * the default size or one specified by the user
> +        */
> +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
> +       pr_debug("Granularity Of Buffer Migration: %d\n", 
> + svms->attr_gobm);
> +
>          return 0;
>   }
>
> @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>          node = interval_tree_iter_first(&svms->objects, start, last);
>          if (!node) {
>                  pr_debug("range attrs not found return default values\n");
> -               svm_range_set_default_attributes(&location, &prefetch_loc,
> -                                                &granularity, &flags_and);
> +               granularity = svms->attr_gobm;
> +               svm_range_set_default_attributes(&location,
> +                                       &prefetch_loc, &flags_and);
>                  flags_or = flags_and;
>                  if (p->xnack_enabled)
>                          bitmap_copy(bitmap_access, 
> svms->bitmap_supported,
> --
> 2.34.1
>

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Chen, Xiaogang]

[-- Attachment #1: Type: text/plain, Size: 9186 bytes --]


On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:
> Response inline
>
> Regards,
> Ramesh
>   
> -----Original Message-----
> From: Chen, Xiaogang<Xiaogang.Chen@amd.com>  
> Sent: Wednesday, August 28, 2024 2:43 PM
> To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter
>
>
> Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?
>
> Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found.

If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite this 
parameter value for a prange, then how to know which granularity take 
effect? That is decided when user set this parameter and when the api 
got used.

Regards

Xiaogang

>
> Regards
>
> Xiaogang
>
> On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>
>> Enables users to update the default size of buffer used in migration
>> either from Sysmem to VRAM or vice versa.
>> The param GOBM refers to granularity of buffer migration, and is
>> specified in terms of log(numPages(buffer)). It facilitates users of
>> unregistered memory to control GOBM, albeit at a coarse level
>>
>> Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>>    drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>>    drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>>    4 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index e8c284aea1f2..73dd816b01f2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -237,6 +237,7 @@ extern int sched_policy;
>>    extern bool debug_evictions;
>>    extern bool no_system_mem_limit;
>>    extern int halt_if_hws_hang;
>> +extern uint amdgpu_svm_attr_gobm;
>>    #else
>>    static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>>    static const bool __maybe_unused debug_evictions; /* = false */ @@
>> -313,6 +314,9 @@ extern int amdgpu_wbrf;
>>    /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>>    #define AMDGPU_SWCTF_EXTRA_DELAY               50
>>
>> +/* Default size of buffer to use in migrating buffer */
>> +#define AMDGPU_SVM_ATTR_GOBM       9
>> +
>>    struct amdgpu_xcp_mgr;
>>    struct amdgpu_device;
>>    struct amdgpu_irq_src;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index b9529948f2b2..09c501753a3b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>>    char *amdgpu_disable_cu;
>>    char *amdgpu_virtual_display;
>>    bool enforce_isolation;
>> +
>> +/* Specifies the default size of buffer to use in
>> + * migrating buffer from Sysmem to VRAM and vice
>> + * versa
>> + *
>> + * GOBM - Granularity of Buffer Migration
>> + *
>> + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint
>> +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
>> +
>>    /*
>>     * OverDrive(bit 14) disabled by default
>>     * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@
>> module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>>    MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>>    module_param_named(msi, amdgpu_msi, int, 0444);
>>
>> +/**
>> + * DOC: svm_attr_gobm (uint)
>> + * Size of buffer to use in migrating buffer from Sysmem to VRAM and
>> +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as
>> +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
>> +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
>> +
>>    /**
>>     * DOC: lockup_timeout (string)
>>     * Set GPU scheduler timeout value in ms.
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> index 9ae9abc6eb43..c2e54b18c167 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>> @@ -868,6 +868,18 @@ struct svm_range_list {
>>           struct task_struct              *faulting_task;
>>           /* check point ts decides if page fault recovery need be dropped */
>>           uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];
>> +
>> +       /* Indicates the default size to use in migrating
>> +        * buffers of a process from Sysmem to VRAM and vice
>> +        * versa. The max legal value cannot be greater than
>> +        * 0x3F
>> +        *
>> +        * @note: A side effect of this symbol being part of
>> +        * struct svm_range_list is that it forces all buffers
>> +        * of the process of unregistered kind to use the same
>> +        * size in buffer migration
>> +        */
>> +       uint8_t attr_gobm;
>>    };
>>
>>    /* Process data */
>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> index b44dec90969f..78c78baddb1f 100644
>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>> @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>>    }
>>
>>    static void
>> -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
>> -                                uint8_t *granularity, uint32_t *flags)
>> +svm_range_set_default_attributes(int32_t *location,
>> +                       int32_t *prefetch_loc, uint32_t *flags)
>>    {
>>           *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>           *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>> -       *granularity = 9;
>>           *flags =
>>                   KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>>    }
>> @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>>                   bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>>                               MAX_GPU_INSTANCE);
>>
>> +       prange->granularity = svms->attr_gobm;
>>           svm_range_set_default_attributes(&prange->preferred_loc,
>> -                                        &prange->prefetch_loc,
>> -                                        &prange->granularity, &prange->flags);
>> +                               &prange->prefetch_loc,
>> + &prange->flags);
>>
>>           pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>>
>> @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct
>> kfd_process *p, int64_t addr,
>>
>>           *is_heap_stack = vma_is_initial_heap(vma) ||
>> vma_is_initial_stack(vma);
>>
>> +       /* Determine the starting and ending page of prange */
>>           start_limit = max(vma->vm_start >> PAGE_SHIFT,
>> -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
>> +                     (unsigned long)ALIGN_DOWN(addr, 1 <<
>> + p->svms.attr_gobm));
>>           end_limit = min(vma->vm_end >> PAGE_SHIFT,
>> -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
>> +                   (unsigned long)ALIGN(addr + 1, 1 <<
>> + p->svms.attr_gobm));
>> +
>>           /* First range that starts after the fault address */
>>           node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>>           if (node) {
>> @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>>                   if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>>                           bitmap_set(svms->bitmap_supported, i, 1);
>>
>> +       /* Bind granularity of buffer migration, either
>> +        * the default size or one specified by the user
>> +        */
>> +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
>> +       pr_debug("Granularity Of Buffer Migration: %d\n",
>> + svms->attr_gobm);
>> +
>>           return 0;
>>    }
>>
>> @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>>           node = interval_tree_iter_first(&svms->objects, start, last);
>>           if (!node) {
>>                   pr_debug("range attrs not found return default values\n");
>> -               svm_range_set_default_attributes(&location, &prefetch_loc,
>> -                                                &granularity, &flags_and);
>> +               granularity = svms->attr_gobm;
>> +               svm_range_set_default_attributes(&location,
>> +                                       &prefetch_loc, &flags_and);
>>                   flags_or = flags_and;
>>                   if (p->xnack_enabled)
>>                           bitmap_copy(bitmap_access,
>> svms->bitmap_supported,
>> --
>> 2.34.1
>>

[-- Attachment #2: Type: text/html, Size: 9910 bytes --]

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Errabolu, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 9918 bytes --]

Responses inline

Regards,
Ramesh

From: Chen, Xiaogang <Xiaogang.Chen@amd.com>
Sent: Wednesday, August 28, 2024 3:01 PM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter



On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:

Response inline



Regards,

Ramesh



-----Original Message-----

From: Chen, Xiaogang <Xiaogang.Chen@amd.com><mailto:Xiaogang.Chen@amd.com>

Sent: Wednesday, August 28, 2024 2:43 PM

To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>

Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter





Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?



Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found.

If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite this parameter value for a prange, then how to know which granularity take effect? That is decided when user set this parameter and when the api got used.

Ramesh: The value bound by Kfd ioctl will take effect. In the life cycle of a prange it can go from the default value to one that is set by user via set_attr() call. However, it is generally understood that that users of set_attr() will not call it directly i.e. the rely on higher level apis from ROCr or HIP.

Regards

Xiaogang





Regards



Xiaogang



On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.





Enables users to update the default size of buffer used in migration

either from Sysmem to VRAM or vice versa.

The param GOBM refers to granularity of buffer migration, and is

specified in terms of log(numPages(buffer)). It facilitates users of

unregistered memory to control GOBM, albeit at a coarse level



Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>

---

  drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++

  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++

  drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++

  drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------

  4 files changed, 51 insertions(+), 9 deletions(-)



diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

index e8c284aea1f2..73dd816b01f2 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

@@ -237,6 +237,7 @@ extern int sched_policy;

  extern bool debug_evictions;

  extern bool no_system_mem_limit;

  extern int halt_if_hws_hang;

+extern uint amdgpu_svm_attr_gobm;

  #else

  static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;

  static const bool __maybe_unused debug_evictions; /* = false */ @@

-313,6 +314,9 @@ extern int amdgpu_wbrf;

  /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */

  #define AMDGPU_SWCTF_EXTRA_DELAY               50



+/* Default size of buffer to use in migrating buffer */

+#define AMDGPU_SVM_ATTR_GOBM       9

+

  struct amdgpu_xcp_mgr;

  struct amdgpu_device;

  struct amdgpu_irq_src;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

index b9529948f2b2..09c501753a3b 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

@@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;

  char *amdgpu_disable_cu;

  char *amdgpu_virtual_display;

  bool enforce_isolation;

+

+/* Specifies the default size of buffer to use in

+ * migrating buffer from Sysmem to VRAM and vice

+ * versa

+ *

+ * GOBM - Granularity of Buffer Migration

+ *

+ * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint

+amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;

+

  /*

   * OverDrive(bit 14) disabled by default

   * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@

module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);

  MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");

  module_param_named(msi, amdgpu_msi, int, 0444);



+/**

+ * DOC: svm_attr_gobm (uint)

+ * Size of buffer to use in migrating buffer from Sysmem to VRAM and

+vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as

+log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

+module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

+

  /**

   * DOC: lockup_timeout (string)

   * Set GPU scheduler timeout value in ms.

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

index 9ae9abc6eb43..c2e54b18c167 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

@@ -868,6 +868,18 @@ struct svm_range_list {

         struct task_struct              *faulting_task;

         /* check point ts decides if page fault recovery need be dropped */

         uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];

+

+       /* Indicates the default size to use in migrating

+        * buffers of a process from Sysmem to VRAM and vice

+        * versa. The max legal value cannot be greater than

+        * 0x3F

+        *

+        * @note: A side effect of this symbol being part of

+        * struct svm_range_list is that it forces all buffers

+        * of the process of unregistered kind to use the same

+        * size in buffer migration

+        */

+       uint8_t attr_gobm;

  };



  /* Process data */

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

index b44dec90969f..78c78baddb1f 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

@@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)

  }



  static void

-svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,

-                                uint8_t *granularity, uint32_t *flags)

+svm_range_set_default_attributes(int32_t *location,

+                       int32_t *prefetch_loc, uint32_t *flags)

  {

         *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

         *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

-       *granularity = 9;

         *flags =

                 KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;

  }

@@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,

                 bitmap_copy(prange->bitmap_access, svms->bitmap_supported,

                             MAX_GPU_INSTANCE);



+       prange->granularity = svms->attr_gobm;

         svm_range_set_default_attributes(&prange->preferred_loc,

-                                        &prange->prefetch_loc,

-                                        &prange->granularity, &prange->flags);

+                               &prange->prefetch_loc,

+ &prange->flags);



         pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);



@@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct

kfd_process *p, int64_t addr,



         *is_heap_stack = vma_is_initial_heap(vma) ||

vma_is_initial_stack(vma);



+       /* Determine the starting and ending page of prange */

         start_limit = max(vma->vm_start >> PAGE_SHIFT,

-                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));

+                     (unsigned long)ALIGN_DOWN(addr, 1 <<

+ p->svms.attr_gobm));

         end_limit = min(vma->vm_end >> PAGE_SHIFT,

-                   (unsigned long)ALIGN(addr + 1, 2UL << 8));

+                   (unsigned long)ALIGN(addr + 1, 1 <<

+ p->svms.attr_gobm));

+

         /* First range that starts after the fault address */

         node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);

         if (node) {

@@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)

                 if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))

                         bitmap_set(svms->bitmap_supported, i, 1);



+       /* Bind granularity of buffer migration, either

+        * the default size or one specified by the user

+        */

+       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

+       pr_debug("Granularity Of Buffer Migration: %d\n",

+ svms->attr_gobm);

+

         return 0;

  }



@@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,

         node = interval_tree_iter_first(&svms->objects, start, last);

         if (!node) {

                 pr_debug("range attrs not found return default values\n");

-               svm_range_set_default_attributes(&location, &prefetch_loc,

-                                                &granularity, &flags_and);

+               granularity = svms->attr_gobm;

+               svm_range_set_default_attributes(&location,

+                                       &prefetch_loc, &flags_and);

                 flags_or = flags_and;

                 if (p->xnack_enabled)

                         bitmap_copy(bitmap_access,

svms->bitmap_supported,

--

2.34.1



[-- Attachment #2: Type: text/html, Size: 22053 bytes --]

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Chen, Xiaogang]

[-- Attachment #1: Type: text/plain, Size: 12586 bytes --]


On 8/28/2024 3:26 PM, Errabolu, Ramesh wrote:
>
> Responses inline
>
> Regards,
>
> Ramesh
>
> *From:*Chen, Xiaogang <Xiaogang.Chen@amd.com>
> *Sent:* Wednesday, August 28, 2024 3:01 PM
> *To:* Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; 
> amd-gfx@lists.freedesktop.org
> *Subject:* Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
> module parameter
>
> On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:
>
>     Response inline
>
>     Regards,
>
>     Ramesh
>
>       
>
>     -----Original Message-----
>
>     From: Chen, Xiaogang<Xiaogang.Chen@amd.com>  <mailto:Xiaogang.Chen@amd.com>  
>
>     Sent: Wednesday, August 28, 2024 2:43 PM
>
>     To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org
>
>     Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter
>
>     Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?
>
>     Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found.
>
> If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite 
> this parameter value for a prange, then how to know which granularity 
> take effect? That is decided when user set this parameter and when the 
> api got used.
>
> Ramesh: The value bound by Kfd ioctl will take effect. In the life 
> cycle of a prange it can go from the default value to one that is set 
> by user via set_attr() call. However, it is generally understood that 
> that users of set_attr() will not call it directly i.e. the rely on 
> higher level apis from ROCr or HIP.
>
driver parameter can be set at run time, not only at boot time. It is 
not predictable  when user set this driver parameter and when the api 
got called.

Regards

Xiaogang

> Regards
>
> Xiaogang
>
>     Regards
>
>     Xiaogang
>
>     On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
>
>         Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>         Enables users to update the default size of buffer used in migration
>
>         either from Sysmem to VRAM or vice versa.
>
>         The param GOBM refers to granularity of buffer migration, and is
>
>         specified in terms of log(numPages(buffer)). It facilitates users of
>
>         unregistered memory to control GOBM, albeit at a coarse level
>
>         Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com>
>
>         ---
>
>            drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>
>            drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>
>            drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>
>            drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>
>            4 files changed, 51 insertions(+), 9 deletions(-)
>
>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>
>         b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>
>         index e8c284aea1f2..73dd816b01f2 100644
>
>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>
>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>
>         @@ -237,6 +237,7 @@ extern int sched_policy;
>
>            extern bool debug_evictions;
>
>            extern bool no_system_mem_limit;
>
>            extern int halt_if_hws_hang;
>
>         +extern uint amdgpu_svm_attr_gobm;
>
>            #else
>
>            static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>
>            static const bool __maybe_unused debug_evictions; /* = false */ @@
>
>         -313,6 +314,9 @@ extern int amdgpu_wbrf;
>
>            /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>
>            #define AMDGPU_SWCTF_EXTRA_DELAY               50
>
>         +/* Default size of buffer to use in migrating buffer */
>
>         +#define AMDGPU_SVM_ATTR_GOBM       9
>
>         +
>
>            struct amdgpu_xcp_mgr;
>
>            struct amdgpu_device;
>
>            struct amdgpu_irq_src;
>
>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>
>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>
>         index b9529948f2b2..09c501753a3b 100644
>
>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>
>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>
>         @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>
>            char *amdgpu_disable_cu;
>
>            char *amdgpu_virtual_display;
>
>            bool enforce_isolation;
>
>         +
>
>         +/* Specifies the default size of buffer to use in
>
>         + * migrating buffer from Sysmem to VRAM and vice
>
>         + * versa
>
>         + *
>
>         + * GOBM - Granularity of Buffer Migration
>
>         + *
>
>         + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint
>
>         +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
>
>         +
>
>            /*
>
>             * OverDrive(bit 14) disabled by default
>
>             * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@
>
>         module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>
>            MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>
>            module_param_named(msi, amdgpu_msi, int, 0444);
>
>         +/**
>
>         + * DOC: svm_attr_gobm (uint)
>
>         + * Size of buffer to use in migrating buffer from Sysmem to VRAM and
>
>         +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as
>
>         +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
>
>         +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
>
>         +
>
>            /**
>
>             * DOC: lockup_timeout (string)
>
>             * Set GPU scheduler timeout value in ms.
>
>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>
>         b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>
>         index 9ae9abc6eb43..c2e54b18c167 100644
>
>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>
>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>
>         @@ -868,6 +868,18 @@ struct svm_range_list {
>
>                   struct task_struct              *faulting_task;
>
>                   /* check point ts decides if page fault recovery need be dropped */
>
>                   uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];
>
>         +
>
>         +       /* Indicates the default size to use in migrating
>
>         +        * buffers of a process from Sysmem to VRAM and vice
>
>         +        * versa. The max legal value cannot be greater than
>
>         +        * 0x3F
>
>         +        *
>
>         +        * @note: A side effect of this symbol being part of
>
>         +        * struct svm_range_list is that it forces all buffers
>
>         +        * of the process of unregistered kind to use the same
>
>         +        * size in buffer migration
>
>         +        */
>
>         +       uint8_t attr_gobm;
>
>            };
>
>            /* Process data */
>
>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>
>         b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>
>         index b44dec90969f..78c78baddb1f 100644
>
>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>
>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>
>         @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>
>            }
>
>            static void
>
>         -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
>
>         -                                uint8_t *granularity, uint32_t *flags)
>
>         +svm_range_set_default_attributes(int32_t *location,
>
>         +                       int32_t *prefetch_loc, uint32_t *flags)
>
>            {
>
>                   *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>
>                   *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>
>         -       *granularity = 9;
>
>                   *flags =
>
>                           KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>
>            }
>
>         @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>
>                           bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>
>                                       MAX_GPU_INSTANCE);
>
>         +       prange->granularity = svms->attr_gobm;
>
>                   svm_range_set_default_attributes(&prange->preferred_loc,
>
>         -                                        &prange->prefetch_loc,
>
>         -                                        &prange->granularity, &prange->flags);
>
>         +                               &prange->prefetch_loc,
>
>         + &prange->flags);
>
>                   pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>
>         @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct
>
>         kfd_process *p, int64_t addr,
>
>                   *is_heap_stack = vma_is_initial_heap(vma) ||
>
>         vma_is_initial_stack(vma);
>
>         +       /* Determine the starting and ending page of prange */
>
>                   start_limit = max(vma->vm_start >> PAGE_SHIFT,
>
>         -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
>
>         +                     (unsigned long)ALIGN_DOWN(addr, 1 <<
>
>         + p->svms.attr_gobm));
>
>                   end_limit = min(vma->vm_end >> PAGE_SHIFT,
>
>         -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
>
>         +                   (unsigned long)ALIGN(addr + 1, 1 <<
>
>         + p->svms.attr_gobm));
>
>         +
>
>                   /* First range that starts after the fault address */
>
>                   node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>
>                   if (node) {
>
>         @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>
>                           if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>
>                                   bitmap_set(svms->bitmap_supported, i, 1);
>
>         +       /* Bind granularity of buffer migration, either
>
>         +        * the default size or one specified by the user
>
>         +        */
>
>         +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
>
>         +       pr_debug("Granularity Of Buffer Migration: %d\n",
>
>         + svms->attr_gobm);
>
>         +
>
>                   return 0;
>
>            }
>
>         @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>
>                   node = interval_tree_iter_first(&svms->objects, start, last);
>
>                   if (!node) {
>
>                           pr_debug("range attrs not found return default values\n");
>
>         -               svm_range_set_default_attributes(&location, &prefetch_loc,
>
>         -                                                &granularity, &flags_and);
>
>         +               granularity = svms->attr_gobm;
>
>         +               svm_range_set_default_attributes(&location,
>
>         +                                       &prefetch_loc, &flags_and);
>
>                           flags_or = flags_and;
>
>                           if (p->xnack_enabled)
>
>                                   bitmap_copy(bitmap_access,
>
>         svms->bitmap_supported,
>
>         --
>
>         2.34.1
>

[-- Attachment #2: Type: text/html, Size: 25361 bytes --]

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Felix Kuehling]

On 2024-08-26 15:34, Ramesh Errabolu wrote:
> Enables users to update the default size of buffer used
> in migration either from Sysmem to VRAM or vice versa.
> The param GOBM refers to granularity of buffer migration,
> and is specified in terms of log(numPages(buffer)). It
> facilitates users of unregistered memory to control GOBM,
> albeit at a coarse level

Can we change the name of this to something more human-readable? I 
suggest something like svm_default_granularity.


>
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>   4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e8c284aea1f2..73dd816b01f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -237,6 +237,7 @@ extern int sched_policy;
>   extern bool debug_evictions;
>   extern bool no_system_mem_limit;
>   extern int halt_if_hws_hang;
> +extern uint amdgpu_svm_attr_gobm;
>   #else
>   static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>   static const bool __maybe_unused debug_evictions; /* = false */
> @@ -313,6 +314,9 @@ extern int amdgpu_wbrf;
>   /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>   #define AMDGPU_SWCTF_EXTRA_DELAY		50
>   
> +/* Default size of buffer to use in migrating buffer */
> +#define AMDGPU_SVM_ATTR_GOBM	    9

I change this name, too. AMDGPU_SVM_DEFAULT_GRANULARITY.


> +
>   struct amdgpu_xcp_mgr;
>   struct amdgpu_device;
>   struct amdgpu_irq_src;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b9529948f2b2..09c501753a3b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>   char *amdgpu_disable_cu;
>   char *amdgpu_virtual_display;
>   bool enforce_isolation;
> +
> +/* Specifies the default size of buffer to use in
> + * migrating buffer from Sysmem to VRAM and vice
> + * versa
> + *
> + * GOBM - Granularity of Buffer Migration

This is really the granularity for page faults as well as migration. 
Hence the suggested name change. Also, GOBM is a new acronym. If you 
only define that in this comment, users won't know what it means. I'd 
remove this comment and instead include enough information in the 
PARM_DESC to let users know what this means. This information is visible 
to them in the output of the "modinfo" command.


> + *
> + * Defined as log2(sizeof(buffer)/PAGE_SIZE)
> + */
> +uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
> +
>   /*
>    * OverDrive(bit 14) disabled by default
>    * GFX DCS(bit 19) disabled by default
> @@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>   MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(msi, amdgpu_msi, int, 0444);
>   
> +/**
> + * DOC: svm_attr_gobm (uint)
> + * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa
> + */
> +MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

Suggested description: "Default SVM page fault granularity in 2^x pages, 
default 9 = 2MiB".


> +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

All the other writable options use permissions 0600. I'd stay consistent 
with that.


> +
>   /**
>    * DOC: lockup_timeout (string)
>    * Set GPU scheduler timeout value in ms.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 9ae9abc6eb43..c2e54b18c167 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -868,6 +868,18 @@ struct svm_range_list {
>   	struct task_struct		*faulting_task;
>   	/* check point ts decides if page fault recovery need be dropped */
>   	uint64_t			checkpoint_ts[MAX_GPU_INSTANCE];
> +
> +	/* Indicates the default size to use in migrating
> +	 * buffers of a process from Sysmem to VRAM and vice
> +	 * versa. The max legal value cannot be greater than
> +	 * 0x3F
> +	 *
> +	 * @note: A side effect of this symbol being part of
> +	 * struct svm_range_list is that it forces all buffers
> +	 * of the process of unregistered kind to use the same
> +	 * size in buffer migration
> +	 */
> +	uint8_t attr_gobm;

Attr is not a good name for this. Attr are the per-virtual address range 
attributes. This is the default setting for just one of these 
attributes. Instead of a long comment, just use a more descriptive name:

	uint8_t default_granularity;


>   };
>   
>   /* Process data */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index b44dec90969f..78c78baddb1f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>   }
>   
>   static void
> -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
> -				 uint8_t *granularity, uint32_t *flags)
> +svm_range_set_default_attributes(int32_t *location,
> +			int32_t *prefetch_loc, uint32_t *flags)

You broke the indentation here.


>   {
>   	*location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>   	*prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
> -	*granularity = 9;

I think I'd prefer to keep this here. Instead of removing this, add a 
struct svm_range_list *svms parameter so you can initialize this here.


>   	*flags =
>   		KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>   }
> @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>   		bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>   			    MAX_GPU_INSTANCE);
>   
> +	prange->granularity = svms->attr_gobm;
>   	svm_range_set_default_attributes(&prange->preferred_loc,
> -					 &prange->prefetch_loc,
> -					 &prange->granularity, &prange->flags);
> +				&prange->prefetch_loc, &prange->flags);

Broken indentation.


>   
>   	pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>   
> @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
>   
>   	*is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);
>   
> +	/* Determine the starting and ending page of prange */
>   	start_limit = max(vma->vm_start >> PAGE_SHIFT,
> -		      (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
> +		      (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
>   	end_limit = min(vma->vm_end >> PAGE_SHIFT,
> -		    (unsigned long)ALIGN(addr + 1, 2UL << 8));
> +		    (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));
> +
>   	/* First range that starts after the fault address */
>   	node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>   	if (node) {
> @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>   		if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>   			bitmap_set(svms->bitmap_supported, i, 1);
>   
> +	/* Bind granularity of buffer migration, either
> +	 * the default size or one specified by the user
> +	 */
> +	svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

0x3F is a little high as a limit. That's 63 decimal. 2^63 pages is more 
than the supported virtual address space in the HW. With 47 virtual 
address bits, 12 bits for the address within the page, the highest 
possible limit would be 35 decimal. A more useful limit would be maybe 
27 (512GB), which is the largest page size supported with 4-level paging.

Regards,
   Felix


> +	pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);
> +
>   	return 0;
>   }
>   
> @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>   	node = interval_tree_iter_first(&svms->objects, start, last);
>   	if (!node) {
>   		pr_debug("range attrs not found return default values\n");
> -		svm_range_set_default_attributes(&location, &prefetch_loc,
> -						 &granularity, &flags_and);
> +		granularity = svms->attr_gobm;
> +		svm_range_set_default_attributes(&location,
> +					&prefetch_loc, &flags_and);
>   		flags_or = flags_and;
>   		if (p->xnack_enabled)
>   			bitmap_copy(bitmap_access, svms->bitmap_supported,

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Felix Kuehling]

On 2024-08-28 16:34, Chen, Xiaogang wrote:
>
>
> On 8/28/2024 3:26 PM, Errabolu, Ramesh wrote:
>>
>> Responses inline
>>
>> Regards,
>>
>> Ramesh
>>
>> *From:*Chen, Xiaogang <Xiaogang.Chen@amd.com>
>> *Sent:* Wednesday, August 28, 2024 3:01 PM
>> *To:* Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; 
>> amd-gfx@lists.freedesktop.org
>> *Subject:* Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
>> module parameter
>>
>> On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:
>>
>>     Response inline
>>
>>     Regards,
>>
>>     Ramesh
>>
>>       
>>
>>     -----Original Message-----
>>
>>     From: Chen, Xiaogang<Xiaogang.Chen@amd.com>  <mailto:Xiaogang.Chen@amd.com>  
>>
>>     Sent: Wednesday, August 28, 2024 2:43 PM
>>
>>     To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org
>>
>>     Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter
>>
>>     Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?
>>
>>     Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found.
>>
>> If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite 
>> this parameter value for a prange, then how to know which granularity 
>> take effect? That is decided when user set this parameter and when 
>> the api got used.
>>
>> Ramesh: The value bound by Kfd ioctl will take effect. In the life 
>> cycle of a prange it can go from the default value to one that is set 
>> by user via set_attr() call. However, it is generally understood that 
>> that users of set_attr() will not call it directly i.e. the rely on 
>> higher level apis from ROCr or HIP.
>>
> driver parameter can be set at run time, not only at boot time. It is 
> not predictable  when user set this driver parameter and when the api 
> got called.
>
I don't think this is a problem. The module parameter determines the 
granularity if the application doesn't set the virtual address range 
attribute. The default is captured in the per-process svms structure. So 
all mappings of the same process will use the same default, even if the 
module parameter is changed after the process is started. The get_attr 
ioctl will always return the actual granularity, no matter whether it 
comes from the default or was overridden by user mode for the virtual 
address range.

Regards,
   Felix


> Regards
>
> Xiaogang
>
>> Regards
>>
>> Xiaogang
>>
>>     Regards
>>
>>     Xiaogang
>>
>>     On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
>>
>>         Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>
>>         Enables users to update the default size of buffer used in migration
>>
>>         either from Sysmem to VRAM or vice versa.
>>
>>         The param GOBM refers to granularity of buffer migration, and is
>>
>>         specified in terms of log(numPages(buffer)). It facilitates users of
>>
>>         unregistered memory to control GOBM, albeit at a coarse level
>>
>>         Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com>
>>
>>         ---
>>
>>            drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>>
>>            drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>>
>>            drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>>
>>            drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>>
>>            4 files changed, 51 insertions(+), 9 deletions(-)
>>
>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>
>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>
>>         index e8c284aea1f2..73dd816b01f2 100644
>>
>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>
>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>
>>         @@ -237,6 +237,7 @@ extern int sched_policy;
>>
>>            extern bool debug_evictions;
>>
>>            extern bool no_system_mem_limit;
>>
>>            extern int halt_if_hws_hang;
>>
>>         +extern uint amdgpu_svm_attr_gobm;
>>
>>            #else
>>
>>            static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>>
>>            static const bool __maybe_unused debug_evictions; /* = false */ @@
>>
>>         -313,6 +314,9 @@ extern int amdgpu_wbrf;
>>
>>            /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>>
>>            #define AMDGPU_SWCTF_EXTRA_DELAY               50
>>
>>         +/* Default size of buffer to use in migrating buffer */
>>
>>         +#define AMDGPU_SVM_ATTR_GOBM       9
>>
>>         +
>>
>>            struct amdgpu_xcp_mgr;
>>
>>            struct amdgpu_device;
>>
>>            struct amdgpu_irq_src;
>>
>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>
>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>
>>         index b9529948f2b2..09c501753a3b 100644
>>
>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>
>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>
>>         @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>>
>>            char *amdgpu_disable_cu;
>>
>>            char *amdgpu_virtual_display;
>>
>>            bool enforce_isolation;
>>
>>         +
>>
>>         +/* Specifies the default size of buffer to use in
>>
>>         + * migrating buffer from Sysmem to VRAM and vice
>>
>>         + * versa
>>
>>         + *
>>
>>         + * GOBM - Granularity of Buffer Migration
>>
>>         + *
>>
>>         + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint
>>
>>         +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
>>
>>         +
>>
>>            /*
>>
>>             * OverDrive(bit 14) disabled by default
>>
>>             * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@
>>
>>         module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>>
>>            MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>>
>>            module_param_named(msi, amdgpu_msi, int, 0444);
>>
>>         +/**
>>
>>         + * DOC: svm_attr_gobm (uint)
>>
>>         + * Size of buffer to use in migrating buffer from Sysmem to VRAM and
>>
>>         +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as
>>
>>         +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
>>
>>         +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);
>>
>>         +
>>
>>            /**
>>
>>             * DOC: lockup_timeout (string)
>>
>>             * Set GPU scheduler timeout value in ms.
>>
>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>
>>         b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>
>>         index 9ae9abc6eb43..c2e54b18c167 100644
>>
>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>
>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>
>>         @@ -868,6 +868,18 @@ struct svm_range_list {
>>
>>                   struct task_struct              *faulting_task;
>>
>>                   /* check point ts decides if page fault recovery need be dropped */
>>
>>                   uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];
>>
>>         +
>>
>>         +       /* Indicates the default size to use in migrating
>>
>>         +        * buffers of a process from Sysmem to VRAM and vice
>>
>>         +        * versa. The max legal value cannot be greater than
>>
>>         +        * 0x3F
>>
>>         +        *
>>
>>         +        * @note: A side effect of this symbol being part of
>>
>>         +        * struct svm_range_list is that it forces all buffers
>>
>>         +        * of the process of unregistered kind to use the same
>>
>>         +        * size in buffer migration
>>
>>         +        */
>>
>>         +       uint8_t attr_gobm;
>>
>>            };
>>
>>            /* Process data */
>>
>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>
>>         b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>
>>         index b44dec90969f..78c78baddb1f 100644
>>
>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>
>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>
>>         @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>>
>>            }
>>
>>            static void
>>
>>         -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
>>
>>         -                                uint8_t *granularity, uint32_t *flags)
>>
>>         +svm_range_set_default_attributes(int32_t *location,
>>
>>         +                       int32_t *prefetch_loc, uint32_t *flags)
>>
>>            {
>>
>>                   *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>
>>                   *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>
>>         -       *granularity = 9;
>>
>>                   *flags =
>>
>>                           KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>>
>>            }
>>
>>         @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>>
>>                           bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>>
>>                                       MAX_GPU_INSTANCE);
>>
>>         +       prange->granularity = svms->attr_gobm;
>>
>>                   svm_range_set_default_attributes(&prange->preferred_loc,
>>
>>         -                                        &prange->prefetch_loc,
>>
>>         -                                        &prange->granularity, &prange->flags);
>>
>>         +                               &prange->prefetch_loc,
>>
>>         + &prange->flags);
>>
>>                   pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>>
>>         @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct
>>
>>         kfd_process *p, int64_t addr,
>>
>>                   *is_heap_stack = vma_is_initial_heap(vma) ||
>>
>>         vma_is_initial_stack(vma);
>>
>>         +       /* Determine the starting and ending page of prange */
>>
>>                   start_limit = max(vma->vm_start >> PAGE_SHIFT,
>>
>>         -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
>>
>>         +                     (unsigned long)ALIGN_DOWN(addr, 1 <<
>>
>>         + p->svms.attr_gobm));
>>
>>                   end_limit = min(vma->vm_end >> PAGE_SHIFT,
>>
>>         -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
>>
>>         +                   (unsigned long)ALIGN(addr + 1, 1 <<
>>
>>         + p->svms.attr_gobm));
>>
>>         +
>>
>>                   /* First range that starts after the fault address */
>>
>>                   node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>>
>>                   if (node) {
>>
>>         @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>>
>>                           if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>>
>>                                   bitmap_set(svms->bitmap_supported, i, 1);
>>
>>         +       /* Bind granularity of buffer migration, either
>>
>>         +        * the default size or one specified by the user
>>
>>         +        */
>>
>>         +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);
>>
>>         +       pr_debug("Granularity Of Buffer Migration: %d\n",
>>
>>         + svms->attr_gobm);
>>
>>         +
>>
>>                   return 0;
>>
>>            }
>>
>>         @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>>
>>                   node = interval_tree_iter_first(&svms->objects, start, last);
>>
>>                   if (!node) {
>>
>>                           pr_debug("range attrs not found return default values\n");
>>
>>         -               svm_range_set_default_attributes(&location, &prefetch_loc,
>>
>>         -                                                &granularity, &flags_and);
>>
>>         +               granularity = svms->attr_gobm;
>>
>>         +               svm_range_set_default_attributes(&location,
>>
>>         +                                       &prefetch_loc, &flags_and);
>>
>>                           flags_or = flags_and;
>>
>>                           if (p->xnack_enabled)
>>
>>                                   bitmap_copy(bitmap_access,
>>
>>         svms->bitmap_supported,
>>
>>         --
>>
>>         2.34.1
>>

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Chen, Xiaogang]

[-- Attachment #1: Type: text/plain, Size: 15072 bytes --]


On 8/28/2024 4:05 PM, Felix Kuehling wrote:
>
> On 2024-08-28 16:34, Chen, Xiaogang wrote:
>>
>>
>> On 8/28/2024 3:26 PM, Errabolu, Ramesh wrote:
>>>
>>> Responses inline
>>>
>>> Regards,
>>>
>>> Ramesh
>>>
>>> *From:*Chen, Xiaogang <Xiaogang.Chen@amd.com>
>>> *Sent:* Wednesday, August 28, 2024 3:01 PM
>>> *To:* Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; 
>>> amd-gfx@lists.freedesktop.org
>>> *Subject:* Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
>>> module parameter
>>>
>>> On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:
>>>
>>>     Response inline
>>>
>>>     Regards,
>>>
>>>     Ramesh
>>>
>>>
>>>     -----Original Message-----
>>>
>>>     From: Chen, Xiaogang<Xiaogang.Chen@amd.com> 
>>> <mailto:Xiaogang.Chen@amd.com>
>>>     Sent: Wednesday, August 28, 2024 2:43 PM
>>>
>>>     To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com> 
>>> <mailto:Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org
>>>
>>>     Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
>>> module parameter
>>>
>>>     Why need this driver parameter? kfd has 
>>> KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set 
>>> migration granularity per prange. If both got set which will take 
>>> precedence?
>>>
>>>     Ramesh: Use of Kfd Ioctl is available to users of registered 
>>> memory. It allows users to control GOBM per buffer level, including 
>>> overwriting default value. For ranges that do not specify GOBM, the 
>>> default value will be found.
>>>
>>> If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite 
>>> this parameter value for a prange, then how to know which 
>>> granularity take effect? That is decided when user set this 
>>> parameter and when the api got used.
>>>
>>> Ramesh: The value bound by Kfd ioctl will take effect. In the life 
>>> cycle of a prange it can go from the default value to one that is 
>>> set by user via set_attr() call. However, it is generally understood 
>>> that that users of set_attr() will not call it directly i.e. the 
>>> rely on higher level apis from ROCr or HIP.
>>>
>> driver parameter can be set at run time, not only at boot time. It is 
>> not predictable  when user set this driver parameter and when the api 
>> got called.
>>
> I don't think this is a problem. The module parameter determines the 
> granularity if the application doesn't set the virtual address range 
> attribute. The default is captured in the per-process svms structure. 
> So all mappings of the same process will use the same default, even if 
> the module parameter is changed after the process is started. The 
> get_attr ioctl will always return the actual granularity, no matter 
> whether it comes from the default or was overridden by user mode for 
> the virtual address range.

My concern is there are two ways to set pragne's granularity, both can 
be used at run time. It can make confusion to know which one take effect 
as user can use driver parameter and api to change granularity with any 
timing.

Regards

Xiaogang

>
> Regards,
>   Felix
>
>
>> Regards
>>
>> Xiaogang
>>
>>> Regards
>>>
>>> Xiaogang
>>>
>>>     Regards
>>>
>>>     Xiaogang
>>>
>>>     On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
>>>
>>>         Caution: This message originated from an External Source. 
>>> Use proper caution when opening attachments, clicking links, or 
>>> responding.
>>>
>>>         Enables users to update the default size of buffer used in 
>>> migration
>>>
>>>         either from Sysmem to VRAM or vice versa.
>>>
>>>         The param GOBM refers to granularity of buffer migration, 
>>> and is
>>>
>>>         specified in terms of log(numPages(buffer)). It facilitates 
>>> users of
>>>
>>>         unregistered memory to control GOBM, albeit at a coarse level
>>>
>>>         Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com> 
>>> <mailto:Ramesh.Errabolu@amd.com>
>>>
>>>         ---
>>>
>>>            drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>>>
>>>            drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 
>>> +++++++++++++++++
>>>
>>>            drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>>>
>>>            drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 
>>> ++++++++++++++++---------
>>>
>>>            4 files changed, 51 insertions(+), 9 deletions(-)
>>>
>>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>
>>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>
>>>         index e8c284aea1f2..73dd816b01f2 100644
>>>
>>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>
>>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>
>>>         @@ -237,6 +237,7 @@ extern int sched_policy;
>>>
>>>            extern bool debug_evictions;
>>>
>>>            extern bool no_system_mem_limit;
>>>
>>>            extern int halt_if_hws_hang;
>>>
>>>         +extern uint amdgpu_svm_attr_gobm;
>>>
>>>            #else
>>>
>>>            static const int __maybe_unused sched_policy = 
>>> KFD_SCHED_POLICY_HWS;
>>>
>>>            static const bool __maybe_unused debug_evictions; /* = 
>>> false */ @@
>>>
>>>         -313,6 +314,9 @@ extern int amdgpu_wbrf;
>>>
>>>            /* Extra time delay(in ms) to eliminate the influence of 
>>> temperature momentary fluctuation */
>>>
>>>            #define AMDGPU_SWCTF_EXTRA_DELAY               50
>>>
>>>         +/* Default size of buffer to use in migrating buffer */
>>>
>>>         +#define AMDGPU_SVM_ATTR_GOBM       9
>>>
>>>         +
>>>
>>>            struct amdgpu_xcp_mgr;
>>>
>>>            struct amdgpu_device;
>>>
>>>            struct amdgpu_irq_src;
>>>
>>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>
>>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>
>>>         index b9529948f2b2..09c501753a3b 100644
>>>
>>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>
>>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>
>>>         @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>>>
>>>            char *amdgpu_disable_cu;
>>>
>>>            char *amdgpu_virtual_display;
>>>
>>>            bool enforce_isolation;
>>>
>>>         +
>>>
>>>         +/* Specifies the default size of buffer to use in
>>>
>>>         + * migrating buffer from Sysmem to VRAM and vice
>>>
>>>         + * versa
>>>
>>>         + *
>>>
>>>         + * GOBM - Granularity of Buffer Migration
>>>
>>>         + *
>>>
>>>         + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint
>>>
>>>         +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
>>>
>>>         +
>>>
>>>            /*
>>>
>>>             * OverDrive(bit 14) disabled by default
>>>
>>>             * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@
>>>
>>>         module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>>>
>>>            MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = 
>>> disable, -1 = auto)");
>>>
>>>            module_param_named(msi, amdgpu_msi, int, 0444);
>>>
>>>         +/**
>>>
>>>         + * DOC: svm_attr_gobm (uint)
>>>
>>>         + * Size of buffer to use in migrating buffer from Sysmem to 
>>> VRAM and
>>>
>>>         +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as
>>>
>>>         +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
>>>
>>>         +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, 
>>> uint, 0644);
>>>
>>>         +
>>>
>>>            /**
>>>
>>>             * DOC: lockup_timeout (string)
>>>
>>>             * Set GPU scheduler timeout value in ms.
>>>
>>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>
>>>         b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>
>>>         index 9ae9abc6eb43..c2e54b18c167 100644
>>>
>>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>
>>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>
>>>         @@ -868,6 +868,18 @@ struct svm_range_list {
>>>
>>>                   struct task_struct *faulting_task;
>>>
>>>                   /* check point ts decides if page fault recovery 
>>> need be dropped */
>>>
>>>                   uint64_t checkpoint_ts[MAX_GPU_INSTANCE];
>>>
>>>         +
>>>
>>>         +       /* Indicates the default size to use in migrating
>>>
>>>         +        * buffers of a process from Sysmem to VRAM and vice
>>>
>>>         +        * versa. The max legal value cannot be greater than
>>>
>>>         +        * 0x3F
>>>
>>>         +        *
>>>
>>>         +        * @note: A side effect of this symbol being part of
>>>
>>>         +        * struct svm_range_list is that it forces all buffers
>>>
>>>         +        * of the process of unregistered kind to use the same
>>>
>>>         +        * size in buffer migration
>>>
>>>         +        */
>>>
>>>         +       uint8_t attr_gobm;
>>>
>>>            };
>>>
>>>            /* Process data */
>>>
>>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>
>>>         b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>
>>>         index b44dec90969f..78c78baddb1f 100644
>>>
>>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>
>>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>
>>>         @@ -309,12 +309,11 @@ static void svm_range_free(struct 
>>> svm_range *prange, bool do_unmap)
>>>
>>>            }
>>>
>>>            static void
>>>
>>>         -svm_range_set_default_attributes(int32_t *location, int32_t 
>>> *prefetch_loc,
>>>
>>>         -                                uint8_t *granularity, 
>>> uint32_t *flags)
>>>
>>>         +svm_range_set_default_attributes(int32_t *location,
>>>
>>>         +                       int32_t *prefetch_loc, uint32_t *flags)
>>>
>>>            {
>>>
>>>                   *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>>
>>>                   *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>>
>>>         -       *granularity = 9;
>>>
>>>                   *flags =
>>>
>>>                           KFD_IOCTL_SVM_FLAG_HOST_ACCESS | 
>>> KFD_IOCTL_SVM_FLAG_COHERENT;
>>>
>>>            }
>>>
>>>         @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct 
>>> svm_range_list *svms, uint64_t start,
>>>
>>> bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>>>
>>>                                       MAX_GPU_INSTANCE);
>>>
>>>         +       prange->granularity = svms->attr_gobm;
>>>
>>> svm_range_set_default_attributes(&prange->preferred_loc,
>>>
>>>         - &prange->prefetch_loc,
>>>
>>>         - &prange->granularity, &prange->flags);
>>>
>>>         + &prange->prefetch_loc,
>>>
>>>         + &prange->flags);
>>>
>>>                   pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, 
>>> start, last);
>>>
>>>         @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct
>>>
>>>         kfd_process *p, int64_t addr,
>>>
>>>                   *is_heap_stack = vma_is_initial_heap(vma) ||
>>>
>>>         vma_is_initial_stack(vma);
>>>
>>>         +       /* Determine the starting and ending page of prange */
>>>
>>>                   start_limit = max(vma->vm_start >> PAGE_SHIFT,
>>>
>>>         -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 
>>> 8));
>>>
>>>         +                     (unsigned long)ALIGN_DOWN(addr, 1 <<
>>>
>>>         + p->svms.attr_gobm));
>>>
>>>                   end_limit = min(vma->vm_end >> PAGE_SHIFT,
>>>
>>>         -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
>>>
>>>         +                   (unsigned long)ALIGN(addr + 1, 1 <<
>>>
>>>         + p->svms.attr_gobm));
>>>
>>>         +
>>>
>>>                   /* First range that starts after the fault address */
>>>
>>>                   node = interval_tree_iter_first(&p->svms.objects, 
>>> addr + 1, ULONG_MAX);
>>>
>>>                   if (node) {
>>>
>>>         @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct 
>>> kfd_process *p)
>>>
>>>                           if 
>>> (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>>>
>>> bitmap_set(svms->bitmap_supported, i, 1);
>>>
>>>         +       /* Bind granularity of buffer migration, either
>>>
>>>         +        * the default size or one specified by the user
>>>
>>>         +        */
>>>
>>>         +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 
>>> 0x3F);
>>>
>>>         +       pr_debug("Granularity Of Buffer Migration: %d\n",
>>>
>>>         + svms->attr_gobm);
>>>
>>>         +
>>>
>>>                   return 0;
>>>
>>>            }
>>>
>>>         @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process 
>>> *p, struct mm_struct *mm,
>>>
>>>                   node = interval_tree_iter_first(&svms->objects, 
>>> start, last);
>>>
>>>                   if (!node) {
>>>
>>>                           pr_debug("range attrs not found return 
>>> default values\n");
>>>
>>>         - svm_range_set_default_attributes(&location, &prefetch_loc,
>>>
>>>         - &granularity, &flags_and);
>>>
>>>         +               granularity = svms->attr_gobm;
>>>
>>>         + svm_range_set_default_attributes(&location,
>>>
>>>         +     &prefetch_loc, &flags_and);
>>>
>>>                           flags_or = flags_and;
>>>
>>>                           if (p->xnack_enabled)
>>>
>>>                                   bitmap_copy(bitmap_access,
>>>
>>>         svms->bitmap_supported,
>>>
>>>         --
>>>
>>>         2.34.1
>>>

[-- Attachment #2: Type: text/html, Size: 68597 bytes --]

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Errabolu, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 13195 bytes --]


From: Chen, Xiaogang <Xiaogang.Chen@amd.com>
Sent: Wednesday, August 28, 2024 4:38 PM
To: Kuehling, Felix <Felix.Kuehling@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter



On 8/28/2024 4:05 PM, Felix Kuehling wrote:

On 2024-08-28 16:34, Chen, Xiaogang wrote:



On 8/28/2024 3:26 PM, Errabolu, Ramesh wrote:


Responses inline

Regards,

Ramesh

*From:*Chen, Xiaogang <Xiaogang.Chen@amd.com><mailto:Xiaogang.Chen@amd.com>
*Sent:* Wednesday, August 28, 2024 3:01 PM
*To:* Errabolu, Ramesh <Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
*Subject:* Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:

    Response inline

    Regards,

    Ramesh


    -----Original Message-----

    From: Chen, Xiaogang<Xiaogang.Chen@amd.com><mailto:Xiaogang.Chen@amd.com>  <mailto:Xiaogang.Chen@amd.com><mailto:Xiaogang.Chen@amd.com>
    Sent: Wednesday, August 28, 2024 2:43 PM

    To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org

    Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

    Why need this driver parameter? kfd has KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set migration granularity per prange. If both got set which will take precedence?

    Ramesh: Use of Kfd Ioctl is available to users of registered memory. It allows users to control GOBM per buffer level, including overwriting default value. For ranges that do not specify GOBM, the default value will be found.

If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite this parameter value for a prange, then how to know which granularity take effect? That is decided when user set this parameter and when the api got used.

Ramesh: The value bound by Kfd ioctl will take effect. In the life cycle of a prange it can go from the default value to one that is set by user via set_attr() call. However, it is generally understood that that users of set_attr() will not call it directly i.e. the rely on higher level apis from ROCr or HIP.
driver parameter can be set at run time, not only at boot time. It is not predictable  when user set this driver parameter and when the api got called.
I don't think this is a problem. The module parameter determines the granularity if the application doesn't set the virtual address range attribute. The default is captured in the per-process svms structure. So all mappings of the same process will use the same default, even if the module parameter is changed after the process is started. The get_attr ioctl will always return the actual granularity, no matter whether it comes from the default or was overridden by user mode for the virtual address range.

My concern is there are two ways to set pragne's granularity, both can be used at run time. It can make confusion to know which one take effect as user can use driver parameter and api to change granularity with any timing.

Ramesh: Users of this capability are expected to understand as to how this parameter works. Having said a brief documentation of how this param works could be written and attached to documentation. Hopefully users will find this additional  documentation helpful

Regards

Xiaogang



Regards,
  Felix



Regards

Xiaogang


Regards

Xiaogang

    Regards

    Xiaogang

    On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:

        Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

        Enables users to update the default size of buffer used in migration

        either from Sysmem to VRAM or vice versa.

        The param GOBM refers to granularity of buffer migration, and is

        specified in terms of log(numPages(buffer)). It facilitates users of

        unregistered memory to control GOBM, albeit at a coarse level

        Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>  <mailto:Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>

        ---

           drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++

           drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++

           drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++

           drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------

           4 files changed, 51 insertions(+), 9 deletions(-)

        diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

        b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

        index e8c284aea1f2..73dd816b01f2 100644

        --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

        +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

        @@ -237,6 +237,7 @@ extern int sched_policy;

           extern bool debug_evictions;

           extern bool no_system_mem_limit;

           extern int halt_if_hws_hang;

        +extern uint amdgpu_svm_attr_gobm;

           #else

           static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;

           static const bool __maybe_unused debug_evictions; /* = false */ @@

        -313,6 +314,9 @@ extern int amdgpu_wbrf;

           /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */

           #define AMDGPU_SWCTF_EXTRA_DELAY               50

        +/* Default size of buffer to use in migrating buffer */

        +#define AMDGPU_SVM_ATTR_GOBM       9

        +

           struct amdgpu_xcp_mgr;

           struct amdgpu_device;

           struct amdgpu_irq_src;

        diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

        b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

        index b9529948f2b2..09c501753a3b 100644

        --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

        +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

        @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;

           char *amdgpu_disable_cu;

           char *amdgpu_virtual_display;

           bool enforce_isolation;

        +

        +/* Specifies the default size of buffer to use in

        + * migrating buffer from Sysmem to VRAM and vice

        + * versa

        + *

        + * GOBM - Granularity of Buffer Migration

        + *

        + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint

        +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;

        +

           /*

            * OverDrive(bit 14) disabled by default

            * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@

        module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);

           MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");

           module_param_named(msi, amdgpu_msi, int, 0444);

        +/**

        + * DOC: svm_attr_gobm (uint)

        + * Size of buffer to use in migrating buffer from Sysmem to VRAM and

        +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as

        +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

        +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

        +

           /**

            * DOC: lockup_timeout (string)

            * Set GPU scheduler timeout value in ms.

        diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

        b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

        index 9ae9abc6eb43..c2e54b18c167 100644

        --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

        +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

        @@ -868,6 +868,18 @@ struct svm_range_list {

                  struct task_struct              *faulting_task;

                  /* check point ts decides if page fault recovery need be dropped */

                  uint64_t                        checkpoint_ts[MAX_GPU_INSTANCE];

        +

        +       /* Indicates the default size to use in migrating

        +        * buffers of a process from Sysmem to VRAM and vice

        +        * versa. The max legal value cannot be greater than

        +        * 0x3F

        +        *

        +        * @note: A side effect of this symbol being part of

        +        * struct svm_range_list is that it forces all buffers

        +        * of the process of unregistered kind to use the same

        +        * size in buffer migration

        +        */

        +       uint8_t attr_gobm;

           };

           /* Process data */

        diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

        b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

        index b44dec90969f..78c78baddb1f 100644

        --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

        +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

        @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)

           }

           static void

        -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,

        -                                uint8_t *granularity, uint32_t *flags)

        +svm_range_set_default_attributes(int32_t *location,

        +                       int32_t *prefetch_loc, uint32_t *flags)

           {

                  *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

                  *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

        -       *granularity = 9;

                  *flags =

                          KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;

           }

        @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,

                          bitmap_copy(prange->bitmap_access, svms->bitmap_supported,

                                      MAX_GPU_INSTANCE);

        +       prange->granularity = svms->attr_gobm;

                  svm_range_set_default_attributes(&prange->preferred_loc,

        -                                        &prange->prefetch_loc,

        -                                        &prange->granularity, &prange->flags);

        +                               &prange->prefetch_loc,

        + &prange->flags);

                  pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);

        @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct

        kfd_process *p, int64_t addr,

                  *is_heap_stack = vma_is_initial_heap(vma) ||

        vma_is_initial_stack(vma);

        +       /* Determine the starting and ending page of prange */

                  start_limit = max(vma->vm_start >> PAGE_SHIFT,

        -                     (unsigned long)ALIGN_DOWN(addr, 2UL << 8));

        +                     (unsigned long)ALIGN_DOWN(addr, 1 <<

        + p->svms.attr_gobm));

                  end_limit = min(vma->vm_end >> PAGE_SHIFT,

        -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));

        +                   (unsigned long)ALIGN(addr + 1, 1 <<

        + p->svms.attr_gobm));

        +

                  /* First range that starts after the fault address */

                  node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);

                  if (node) {

        @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)

                          if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))

                                  bitmap_set(svms->bitmap_supported, i, 1);

        +       /* Bind granularity of buffer migration, either

        +        * the default size or one specified by the user

        +        */

        +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

        +       pr_debug("Granularity Of Buffer Migration: %d\n",

        + svms->attr_gobm);

        +

                  return 0;

           }

        @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,

                  node = interval_tree_iter_first(&svms->objects, start, last);

                  if (!node) {

                          pr_debug("range attrs not found return default values\n");

        -               svm_range_set_default_attributes(&location, &prefetch_loc,

        -                                                &granularity, &flags_and);

        +               granularity = svms->attr_gobm;

        +               svm_range_set_default_attributes(&location,

        +                                       &prefetch_loc, &flags_and);

                          flags_or = flags_and;

                          if (p->xnack_enabled)

                                  bitmap_copy(bitmap_access,

        svms->bitmap_supported,

        --

        2.34.1

[-- Attachment #2: Type: text/html, Size: 29696 bytes --]

Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Felix Kuehling]

On 2024-08-28 17:38, Chen, Xiaogang wrote:
>
>
> On 8/28/2024 4:05 PM, Felix Kuehling wrote:
>>
>> On 2024-08-28 16:34, Chen, Xiaogang wrote:
>>>
>>>
>>> On 8/28/2024 3:26 PM, Errabolu, Ramesh wrote:
>>>>
>>>> Responses inline
>>>>
>>>> Regards,
>>>>
>>>> Ramesh
>>>>
>>>> *From:*Chen, Xiaogang <Xiaogang.Chen@amd.com>
>>>> *Sent:* Wednesday, August 28, 2024 3:01 PM
>>>> *To:* Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; 
>>>> amd-gfx@lists.freedesktop.org
>>>> *Subject:* Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
>>>> module parameter
>>>>
>>>> On 8/28/2024 2:52 PM, Errabolu, Ramesh wrote:
>>>>
>>>>     Response inline
>>>>
>>>>     Regards,
>>>>
>>>>     Ramesh
>>>>
>>>>
>>>>     -----Original Message-----
>>>>
>>>>     From: Chen, Xiaogang<Xiaogang.Chen@amd.com> 
>>>> <mailto:Xiaogang.Chen@amd.com>
>>>>     Sent: Wednesday, August 28, 2024 2:43 PM
>>>>
>>>>     To: Errabolu, Ramesh<Ramesh.Errabolu@amd.com> 
>>>> <mailto:Ramesh.Errabolu@amd.com>;amd-gfx@lists.freedesktop.org
>>>>
>>>>     Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW 
>>>> module parameter
>>>>
>>>>     Why need this driver parameter? kfd has 
>>>> KFD_IOCTL_SVM_ATTR_GRANULARITY api that allows user space to set 
>>>> migration granularity per prange. If both got set which will take 
>>>> precedence?
>>>>
>>>>     Ramesh: Use of Kfd Ioctl is available to users of registered 
>>>> memory. It allows users to control GOBM per buffer level, including 
>>>> overwriting default value. For ranges that do not specify GOBM, the 
>>>> default value will be found.
>>>>
>>>> If user space use KFD_IOCTL_SVM_ATTR_GRANULARITY it will overwrite 
>>>> this parameter value for a prange, then how to know which 
>>>> granularity take effect? That is decided when user set this 
>>>> parameter and when the api got used.
>>>>
>>>> Ramesh: The value bound by Kfd ioctl will take effect. In the life 
>>>> cycle of a prange it can go from the default value to one that is 
>>>> set by user via set_attr() call. However, it is generally 
>>>> understood that that users of set_attr() will not call it directly 
>>>> i.e. the rely on higher level apis from ROCr or HIP.
>>>>
>>> driver parameter can be set at run time, not only at boot time. It 
>>> is not predictable  when user set this driver parameter and when the 
>>> api got called.
>>>
>> I don't think this is a problem. The module parameter determines the 
>> granularity if the application doesn't set the virtual address range 
>> attribute. The default is captured in the per-process svms structure. 
>> So all mappings of the same process will use the same default, even 
>> if the module parameter is changed after the process is started. The 
>> get_attr ioctl will always return the actual granularity, no matter 
>> whether it comes from the default or was overridden by user mode for 
>> the virtual address range.
>
> My concern is there are two ways to set pragne's granularity, both can 
> be used at run time. It can make confusion to know which one take 
> effect as user can use driver parameter and api to change granularity 
> with any timing.
>
But it's not "any timing". For the module parameter to take effect, it 
has to be set _before_ the process starts. Any changes to the module 
parameter after the process starts will not take effect.

Regards,
   Felix


> Regards
>
> Xiaogang
>
>>
>> Regards,
>>   Felix
>>
>>
>>> Regards
>>>
>>> Xiaogang
>>>
>>>> Regards
>>>>
>>>> Xiaogang
>>>>
>>>>     Regards
>>>>
>>>>     Xiaogang
>>>>
>>>>     On 8/26/2024 2:34 PM, Ramesh Errabolu wrote:
>>>>
>>>>         Caution: This message originated from an External Source. 
>>>> Use proper caution when opening attachments, clicking links, or 
>>>> responding.
>>>>
>>>>         Enables users to update the default size of buffer used in 
>>>> migration
>>>>
>>>>         either from Sysmem to VRAM or vice versa.
>>>>
>>>>         The param GOBM refers to granularity of buffer migration, 
>>>> and is
>>>>
>>>>         specified in terms of log(numPages(buffer)). It facilitates 
>>>> users of
>>>>
>>>>         unregistered memory to control GOBM, albeit at a coarse level
>>>>
>>>>         Signed-off-by: Ramesh Errabolu<Ramesh.Errabolu@amd.com> 
>>>> <mailto:Ramesh.Errabolu@amd.com>
>>>>
>>>>         ---
>>>>
>>>>            drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>>>>
>>>>            drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 
>>>> +++++++++++++++++
>>>>
>>>>            drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>>>>
>>>>            drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 
>>>> ++++++++++++++++---------
>>>>
>>>>            4 files changed, 51 insertions(+), 9 deletions(-)
>>>>
>>>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>
>>>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>
>>>>         index e8c284aea1f2..73dd816b01f2 100644
>>>>
>>>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>
>>>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>>>
>>>>         @@ -237,6 +237,7 @@ extern int sched_policy;
>>>>
>>>>            extern bool debug_evictions;
>>>>
>>>>            extern bool no_system_mem_limit;
>>>>
>>>>            extern int halt_if_hws_hang;
>>>>
>>>>         +extern uint amdgpu_svm_attr_gobm;
>>>>
>>>>            #else
>>>>
>>>>            static const int __maybe_unused sched_policy = 
>>>> KFD_SCHED_POLICY_HWS;
>>>>
>>>>            static const bool __maybe_unused debug_evictions; /* = 
>>>> false */ @@
>>>>
>>>>         -313,6 +314,9 @@ extern int amdgpu_wbrf;
>>>>
>>>>            /* Extra time delay(in ms) to eliminate the influence of 
>>>> temperature momentary fluctuation */
>>>>
>>>>            #define AMDGPU_SWCTF_EXTRA_DELAY               50
>>>>
>>>>         +/* Default size of buffer to use in migrating buffer */
>>>>
>>>>         +#define AMDGPU_SVM_ATTR_GOBM       9
>>>>
>>>>         +
>>>>
>>>>            struct amdgpu_xcp_mgr;
>>>>
>>>>            struct amdgpu_device;
>>>>
>>>>            struct amdgpu_irq_src;
>>>>
>>>>         diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>
>>>>         b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>
>>>>         index b9529948f2b2..09c501753a3b 100644
>>>>
>>>>         --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>
>>>>         +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>>
>>>>         @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>>>>
>>>>            char *amdgpu_disable_cu;
>>>>
>>>>            char *amdgpu_virtual_display;
>>>>
>>>>            bool enforce_isolation;
>>>>
>>>>         +
>>>>
>>>>         +/* Specifies the default size of buffer to use in
>>>>
>>>>         + * migrating buffer from Sysmem to VRAM and vice
>>>>
>>>>         + * versa
>>>>
>>>>         + *
>>>>
>>>>         + * GOBM - Granularity of Buffer Migration
>>>>
>>>>         + *
>>>>
>>>>         + * Defined as log2(sizeof(buffer)/PAGE_SIZE)  */ uint
>>>>
>>>>         +amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
>>>>
>>>>         +
>>>>
>>>>            /*
>>>>
>>>>             * OverDrive(bit 14) disabled by default
>>>>
>>>>             * GFX DCS(bit 19) disabled by default @@ -320,6 +331,13 @@
>>>>
>>>>         module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>>>>
>>>>            MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = 
>>>> disable, -1 = auto)");
>>>>
>>>>            module_param_named(msi, amdgpu_msi, int, 0444);
>>>>
>>>>         +/**
>>>>
>>>>         + * DOC: svm_attr_gobm (uint)
>>>>
>>>>         + * Size of buffer to use in migrating buffer from Sysmem 
>>>> to VRAM and
>>>>
>>>>         +vice versa  */ MODULE_PARM_DESC(svm_attr_gobm, "Defined as
>>>>
>>>>         +log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");
>>>>
>>>>         +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, 
>>>> uint, 0644);
>>>>
>>>>         +
>>>>
>>>>            /**
>>>>
>>>>             * DOC: lockup_timeout (string)
>>>>
>>>>             * Set GPU scheduler timeout value in ms.
>>>>
>>>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>
>>>>         b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>
>>>>         index 9ae9abc6eb43..c2e54b18c167 100644
>>>>
>>>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>
>>>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>>>>
>>>>         @@ -868,6 +868,18 @@ struct svm_range_list {
>>>>
>>>>                   struct task_struct *faulting_task;
>>>>
>>>>                   /* check point ts decides if page fault recovery 
>>>> need be dropped */
>>>>
>>>>                   uint64_t checkpoint_ts[MAX_GPU_INSTANCE];
>>>>
>>>>         +
>>>>
>>>>         +       /* Indicates the default size to use in migrating
>>>>
>>>>         +        * buffers of a process from Sysmem to VRAM and vice
>>>>
>>>>         +        * versa. The max legal value cannot be greater than
>>>>
>>>>         +        * 0x3F
>>>>
>>>>         +        *
>>>>
>>>>         +        * @note: A side effect of this symbol being part of
>>>>
>>>>         +        * struct svm_range_list is that it forces all buffers
>>>>
>>>>         +        * of the process of unregistered kind to use the same
>>>>
>>>>         +        * size in buffer migration
>>>>
>>>>         +        */
>>>>
>>>>         +       uint8_t attr_gobm;
>>>>
>>>>            };
>>>>
>>>>            /* Process data */
>>>>
>>>>         diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>
>>>>         b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>
>>>>         index b44dec90969f..78c78baddb1f 100644
>>>>
>>>>         --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>
>>>>         +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>>>
>>>>         @@ -309,12 +309,11 @@ static void svm_range_free(struct 
>>>> svm_range *prange, bool do_unmap)
>>>>
>>>>            }
>>>>
>>>>            static void
>>>>
>>>>         -svm_range_set_default_attributes(int32_t *location, 
>>>> int32_t *prefetch_loc,
>>>>
>>>>         -                                uint8_t *granularity, 
>>>> uint32_t *flags)
>>>>
>>>>         +svm_range_set_default_attributes(int32_t *location,
>>>>
>>>>         +                       int32_t *prefetch_loc, uint32_t 
>>>> *flags)
>>>>
>>>>            {
>>>>
>>>>                   *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>>>
>>>>                   *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>>>>
>>>>         -       *granularity = 9;
>>>>
>>>>                   *flags =
>>>>
>>>>                           KFD_IOCTL_SVM_FLAG_HOST_ACCESS | 
>>>> KFD_IOCTL_SVM_FLAG_COHERENT;
>>>>
>>>>            }
>>>>
>>>>         @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct 
>>>> svm_range_list *svms, uint64_t start,
>>>>
>>>> bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>>>>
>>>>                                       MAX_GPU_INSTANCE);
>>>>
>>>>         +       prange->granularity = svms->attr_gobm;
>>>>
>>>> svm_range_set_default_attributes(&prange->preferred_loc,
>>>>
>>>>         - &prange->prefetch_loc,
>>>>
>>>>         - &prange->granularity, &prange->flags);
>>>>
>>>>         + &prange->prefetch_loc,
>>>>
>>>>         + &prange->flags);
>>>>
>>>>                   pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, 
>>>> start, last);
>>>>
>>>>         @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct
>>>>
>>>>         kfd_process *p, int64_t addr,
>>>>
>>>>                   *is_heap_stack = vma_is_initial_heap(vma) ||
>>>>
>>>>         vma_is_initial_stack(vma);
>>>>
>>>>         +       /* Determine the starting and ending page of prange */
>>>>
>>>>                   start_limit = max(vma->vm_start >> PAGE_SHIFT,
>>>>
>>>>         -                     (unsigned long)ALIGN_DOWN(addr, 2UL 
>>>> << 8));
>>>>
>>>>         +                     (unsigned long)ALIGN_DOWN(addr, 1 <<
>>>>
>>>>         + p->svms.attr_gobm));
>>>>
>>>>                   end_limit = min(vma->vm_end >> PAGE_SHIFT,
>>>>
>>>>         -                   (unsigned long)ALIGN(addr + 1, 2UL << 8));
>>>>
>>>>         +                   (unsigned long)ALIGN(addr + 1, 1 <<
>>>>
>>>>         + p->svms.attr_gobm));
>>>>
>>>>         +
>>>>
>>>>                   /* First range that starts after the fault 
>>>> address */
>>>>
>>>>                   node = interval_tree_iter_first(&p->svms.objects, 
>>>> addr + 1, ULONG_MAX);
>>>>
>>>>                   if (node) {
>>>>
>>>>         @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct 
>>>> kfd_process *p)
>>>>
>>>>                           if 
>>>> (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>>>>
>>>> bitmap_set(svms->bitmap_supported, i, 1);
>>>>
>>>>         +       /* Bind granularity of buffer migration, either
>>>>
>>>>         +        * the default size or one specified by the user
>>>>
>>>>         +        */
>>>>
>>>>         +       svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 
>>>> 0x3F);
>>>>
>>>>         +       pr_debug("Granularity Of Buffer Migration: %d\n",
>>>>
>>>>         + svms->attr_gobm);
>>>>
>>>>         +
>>>>
>>>>                   return 0;
>>>>
>>>>            }
>>>>
>>>>         @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process 
>>>> *p, struct mm_struct *mm,
>>>>
>>>>                   node = interval_tree_iter_first(&svms->objects, 
>>>> start, last);
>>>>
>>>>                   if (!node) {
>>>>
>>>>                           pr_debug("range attrs not found return 
>>>> default values\n");
>>>>
>>>>         - svm_range_set_default_attributes(&location, &prefetch_loc,
>>>>
>>>>         - &granularity, &flags_and);
>>>>
>>>>         +               granularity = svms->attr_gobm;
>>>>
>>>>         + svm_range_set_default_attributes(&location,
>>>>
>>>>         +     &prefetch_loc, &flags_and);
>>>>
>>>>                           flags_or = flags_and;
>>>>
>>>>                           if (p->xnack_enabled)
>>>>
>>>>                                   bitmap_copy(bitmap_access,
>>>>
>>>>         svms->bitmap_supported,
>>>>
>>>>         --
>>>>
>>>>         2.34.1
>>>>

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Errabolu, Ramesh]

[-- Attachment #1: Type: text/plain, Size: 9863 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

Responses inline. Will post new patch after testing

Regards,
Ramesh

From: Kasiviswanathan, Harish <Harish.Kasiviswanathan@amd.com>
Sent: Wednesday, August 28, 2024 1:37 PM
To: Yang, Philip <Philip.Yang@amd.com>; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org
Subject: RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter


[AMD Official Use Only - AMD Internal Distribution Only]

Some comments inline.

From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> On Behalf Of Philip Yang
Sent: Wednesday, August 28, 2024 11:49 AM
To: Errabolu, Ramesh <Ramesh.Errabolu@amd.com<mailto:Ramesh.Errabolu@amd.com>>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter



On 2024-08-26 15:34, Ramesh Errabolu wrote:

Enables users to update the default size of buffer used

in migration either from Sysmem to VRAM or vice versa.

The param GOBM refers to granularity of buffer migration,



[HK]: Include the module param name here.

[Ramesh] Changed param name to svm_default_granularity. It is included as part of the header.



and is specified in terms of log(numPages(buffer)). It

facilitates users of unregistered memory to control GOBM,
it is used for both registered and unregistered range cases.
Ramesh: Removed reference to unregistered memory as it is technically possible to update granularity by calling set_svm_attr() ioctl



albeit at a coarse level



Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com><mailto:Ramesh.Errabolu@amd.com>

---

 drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++

 drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++

 drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------

 4 files changed, 51 insertions(+), 9 deletions(-)



diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

index e8c284aea1f2..73dd816b01f2 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h

@@ -237,6 +237,7 @@ extern int sched_policy;

 extern bool debug_evictions;

 extern bool no_system_mem_limit;

 extern int halt_if_hws_hang;

+extern uint amdgpu_svm_attr_gobm;

 #else

 static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;

 static const bool __maybe_unused debug_evictions; /* = false */

@@ -313,6 +314,9 @@ extern int amdgpu_wbrf;

 /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */

 #define AMDGPU_SWCTF_EXTRA_DELAY              50



+/* Default size of buffer to use in migrating buffer */

+#define AMDGPU_SVM_ATTR_GOBM       9

+



[HK]: Don’t think this #define is need. You can just use 9 directly as other module parameters for default value.

[Ramesh] Removed it



 struct amdgpu_xcp_mgr;

 struct amdgpu_device;

 struct amdgpu_irq_src;

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

index b9529948f2b2..09c501753a3b 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

@@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;

 char *amdgpu_disable_cu;

 char *amdgpu_virtual_display;

 bool enforce_isolation;

+

+/* Specifies the default size of buffer to use in

+ * migrating buffer from Sysmem to VRAM and vice

+ * versa

+ *

+ * GOBM - Granularity of Buffer Migration

+ *

+ * Defined as log2(sizeof(buffer)/PAGE_SIZE)



[HK]: Use of log is unnecessary here and other places. You can just state is size is exponent of 2.

[Ramesh] Updated comment to state that this symbol server two purposes – buffer migration and page fault handling. Value of granularity is defined as log(2^Pages) so that aspect has to be highlighted. How this is interpreted, including any scaling is opaque



+ */

+uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;

+

 /*

  * OverDrive(bit 14) disabled by default

  * GFX DCS(bit 19) disabled by default

@@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);

 MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");

 module_param_named(msi, amdgpu_msi, int, 0444);



+/**

+ * DOC: svm_attr_gobm (uint)

+ * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa

+ */

+MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

+module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

+

 /**

  * DOC: lockup_timeout (string)

  * Set GPU scheduler timeout value in ms.

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

index 9ae9abc6eb43..c2e54b18c167 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h

@@ -868,6 +868,18 @@ struct svm_range_list {

  struct task_struct       *faulting_task;

  /* check point ts decides if page fault recovery need be dropped */

  uint64_t                 checkpoint_ts[MAX_GPU_INSTANCE];

+

+ /* Indicates the default size to use in migrating

+  * buffers of a process from Sysmem to VRAM and vice

+  * versa. The max legal value cannot be greater than

+  * 0x3F

+  *

+  * @note: A side effect of this symbol being part of

+  * struct svm_range_list is that it forces all buffers

+  * of the process of unregistered kind to use the same

+  * size in buffer migration

+  */
The comment is good enough, note maybe misleading, not needed.
[Ramesh]: Removed references to unregistered memory users in the NOTE section.




+ uint8_t attr_gobm;

 };



 /* Process data */

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

index b44dec90969f..78c78baddb1f 100644

--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c

@@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)

 }



 static void

-svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,

-                         uint8_t *granularity, uint32_t *flags)

+svm_range_set_default_attributes(int32_t *location,

+                int32_t *prefetch_loc, uint32_t *flags)

 {

  *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

  *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;

- *granularity = 9;

as svm_range_set_default_attributes is called in multiple places, add new parameter struct svm_range_list *svms, to remove the duplicate code.

Ramesh: Restored the original function signature i.e. set granularity inside the set_default_attributes() function

*granularity = svms->attr_gobm;



  *flags =

         KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;

 }

@@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,

         bitmap_copy(prange->bitmap_access, svms->bitmap_supported,

                     MAX_GPU_INSTANCE);



+ prange->granularity = svms->attr_gobm;

  svm_range_set_default_attributes(&prange->preferred_loc,

-                                &prange->prefetch_loc,

-                                &prange->granularity, &prange->flags);

+                        &prange->prefetch_loc, &prange->flags);



  pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);



@@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,



  *is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);



+ /* Determine the starting and ending page of prange */
this comment is not helpful.
Ramesh: removed it



  start_limit = max(vma->vm_start >> PAGE_SHIFT,

-              (unsigned long)ALIGN_DOWN(addr, 2UL << 8));

+              (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
1UL << p->svms.attr_gobm
Ramesh: done

  end_limit = min(vma->vm_end >> PAGE_SHIFT,

-            (unsigned long)ALIGN(addr + 1, 2UL << 8));

+            (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));

1UL << p->svms.attr_gobm

Ramesh: done

Regards,

Philip



+

  /* First range that starts after the fault address */

  node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);

  if (node) {

@@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)

         if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))

                 bitmap_set(svms->bitmap_supported, i, 1);



+ /* Bind granularity of buffer migration, either

+  * the default size or one specified by the user

+  */

+ svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

+ pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);

+

  return 0;

 }



@@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,

  node = interval_tree_iter_first(&svms->objects, start, last);

  if (!node) {

         pr_debug("range attrs not found return default values\n");

-   svm_range_set_default_attributes(&location, &prefetch_loc,

-                                        &granularity, &flags_and);

+        granularity = svms->attr_gobm;

+   svm_range_set_default_attributes(&location,

+                          &prefetch_loc, &flags_and);

         flags_or = flags_and;

         if (p->xnack_enabled)

                 bitmap_copy(bitmap_access, svms->bitmap_supported,

[-- Attachment #2: Type: text/html, Size: 22271 bytes --]

RE: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter

[Errabolu, Ramesh]

Responses inline, will post updated patch shortly. Look at my response for the Sysfs permission

Regards,
Ramesh
 
-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling@amd.com> 
Sent: Wednesday, August 28, 2024 3:59 PM
To: amd-gfx@lists.freedesktop.org; Errabolu, Ramesh <Ramesh.Errabolu@amd.com>
Subject: Re: [PATCH v2] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter


On 2024-08-26 15:34, Ramesh Errabolu wrote:
> Enables users to update the default size of buffer used in migration 
> either from Sysmem to VRAM or vice versa.
> The param GOBM refers to granularity of buffer migration, and is 
> specified in terms of log(numPages(buffer)). It facilitates users of 
> unregistered memory to control GOBM, albeit at a coarse level

Can we change the name of this to something more human-readable? I suggest something like svm_default_granularity.
Ramesh: Done per suggestion

>
> Signed-off-by: Ramesh Errabolu <Ramesh.Errabolu@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h     |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 18 +++++++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_priv.h   | 12 ++++++++++++
>   drivers/gpu/drm/amd/amdkfd/kfd_svm.c    | 26 ++++++++++++++++---------
>   4 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index e8c284aea1f2..73dd816b01f2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -237,6 +237,7 @@ extern int sched_policy;
>   extern bool debug_evictions;
>   extern bool no_system_mem_limit;
>   extern int halt_if_hws_hang;
> +extern uint amdgpu_svm_attr_gobm;
>   #else
>   static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;
>   static const bool __maybe_unused debug_evictions; /* = false */
> @@ -313,6 +314,9 @@ extern int amdgpu_wbrf;
>   /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */
>   #define AMDGPU_SWCTF_EXTRA_DELAY		50
>   
> +/* Default size of buffer to use in migrating buffer */
> +#define AMDGPU_SVM_ATTR_GOBM	    9

I change this name, too. AMDGPU_SVM_DEFAULT_GRANULARITY.
Ramesh: Removing this per input from other review. Also it is not used only in one location

> +
>   struct amdgpu_xcp_mgr;
>   struct amdgpu_device;
>   struct amdgpu_irq_src;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index b9529948f2b2..09c501753a3b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -169,6 +169,17 @@ uint amdgpu_sdma_phase_quantum = 32;
>   char *amdgpu_disable_cu;
>   char *amdgpu_virtual_display;
>   bool enforce_isolation;
> +
> +/* Specifies the default size of buffer to use in
> + * migrating buffer from Sysmem to VRAM and vice
> + * versa
> + *
> + * GOBM - Granularity of Buffer Migration

This is really the granularity for page faults as well as migration. 
Hence the suggested name change. Also, GOBM is a new acronym. If you 
only define that in this comment, users won't know what it means. I'd 
remove this comment and instead include enough information in the 
PARM_DESC to let users know what this means. This information is visible 
to them in the output of the "modinfo" command.


> + *
> + * Defined as log2(sizeof(buffer)/PAGE_SIZE)
> + */
> +uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;
> +
>   /*
>    * OverDrive(bit 14) disabled by default
>    * GFX DCS(bit 19) disabled by default
> @@ -320,6 +331,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
>   MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");
>   module_param_named(msi, amdgpu_msi, int, 0444);
>   
> +/**
> + * DOC: svm_attr_gobm (uint)
> + * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa
> + */
> +MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");

Suggested description: "Default SVM page fault granularity in 2^x pages, 
default 9 = 2MiB".
Ramesh: Elaborated the comment in the driver to include both aspects. Updated PARAM_DESC() to hint at its role on both

> +module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);

All the other writable options use permissions 0600. I'd stay consistent 
with that.
Ramesh: Keeping it as 644 to allow non-root users to be able to query this parameter.
This is similar to how params such as " queue_preemption_timeout_ms " and "timeout_period" are defined

> +
>   /**
>    * DOC: lockup_timeout (string)
>    * Set GPU scheduler timeout value in ms.
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 9ae9abc6eb43..c2e54b18c167 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@ -868,6 +868,18 @@ struct svm_range_list {
>   	struct task_struct		*faulting_task;
>   	/* check point ts decides if page fault recovery need be dropped */
>   	uint64_t			checkpoint_ts[MAX_GPU_INSTANCE];
> +
> +	/* Indicates the default size to use in migrating
> +	 * buffers of a process from Sysmem to VRAM and vice
> +	 * versa. The max legal value cannot be greater than
> +	 * 0x3F
> +	 *
> +	 * @note: A side effect of this symbol being part of
> +	 * struct svm_range_list is that it forces all buffers
> +	 * of the process of unregistered kind to use the same
> +	 * size in buffer migration
> +	 */
> +	uint8_t attr_gobm;

Attr is not a good name for this. Attr are the per-virtual address range 
attributes. This is the default setting for just one of these 
attributes. Instead of a long comment, just use a more descriptive name:

	uint8_t default_granularity;
Ramesh: Updated name to suggested, including an updated comment to emphasize its role in buffer migration and page fault handling.

>   };
>   
>   /* Process data */
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index b44dec90969f..78c78baddb1f 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)
>   }
>   
>   static void
> -svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,
> -				 uint8_t *granularity, uint32_t *flags)
> +svm_range_set_default_attributes(int32_t *location,
> +			int32_t *prefetch_loc, uint32_t *flags)

You broke the indentation here.
Ramesh: Do not see it. Also checkpatch script didn't complain

>   {
>   	*location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
>   	*prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;
> -	*granularity = 9;

I think I'd prefer to keep this here. Instead of removing this, add a 
struct svm_range_list *svms parameter so you can initialize this here.
Ramesh: Done, per your suggestion and others

>   	*flags =
>   		KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;
>   }
> @@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,
>   		bitmap_copy(prange->bitmap_access, svms->bitmap_supported,
>   			    MAX_GPU_INSTANCE);
>   
> +	prange->granularity = svms->attr_gobm;
>   	svm_range_set_default_attributes(&prange->preferred_loc,
> -					 &prange->prefetch_loc,
> -					 &prange->granularity, &prange->flags);
> +				&prange->prefetch_loc, &prange->flags);

Broken indentation.
Ramesh: Do not see it. Also checkpatch script didn't complain

>   
>   	pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);
>   
> @@ -2693,10 +2692,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,
>   
>   	*is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);
>   
> +	/* Determine the starting and ending page of prange */
>   	start_limit = max(vma->vm_start >> PAGE_SHIFT,
> -		      (unsigned long)ALIGN_DOWN(addr, 2UL << 8));
> +		      (unsigned long)ALIGN_DOWN(addr, 1 << p->svms.attr_gobm));
>   	end_limit = min(vma->vm_end >> PAGE_SHIFT,
> -		    (unsigned long)ALIGN(addr + 1, 2UL << 8));
> +		    (unsigned long)ALIGN(addr + 1, 1 << p->svms.attr_gobm));
> +
>   	/* First range that starts after the fault address */
>   	node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);
>   	if (node) {
> @@ -3240,6 +3241,12 @@ int svm_range_list_init(struct kfd_process *p)
>   		if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))
>   			bitmap_set(svms->bitmap_supported, i, 1);
>   
> +	/* Bind granularity of buffer migration, either
> +	 * the default size or one specified by the user
> +	 */
> +	svms->attr_gobm = min_t(u8, amdgpu_svm_attr_gobm, 0x3F);

0x3F is a little high as a limit. That's 63 decimal. 2^63 pages is more 
than the supported virtual address space in the HW. With 47 virtual 
address bits, 12 bits for the address within the page, the highest 
possible limit would be 35 decimal. A more useful limit would be maybe 
27 (512GB), which is the largest page size supported with 4-level paging.
Ramesh: I was thinking the same but went with existing code. Isn't the VA limit 48 instead of 47. Nonetheless, will apply 512 GiB

Regards,
   Felix


> +	pr_debug("Granularity Of Buffer Migration: %d\n", svms->attr_gobm);
> +
>   	return 0;
>   }
>   
> @@ -3767,8 +3774,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,
>   	node = interval_tree_iter_first(&svms->objects, start, last);
>   	if (!node) {
>   		pr_debug("range attrs not found return default values\n");
> -		svm_range_set_default_attributes(&location, &prefetch_loc,
> -						 &granularity, &flags_and);
> +		granularity = svms->attr_gobm;
> +		svm_range_set_default_attributes(&location,
> +					&prefetch_loc, &flags_and);
>   		flags_or = flags_and;
>   		if (p->xnack_enabled)
>   			bitmap_copy(bitmap_access, svms->bitmap_supported,