<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Aptos;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:12.0pt;
font-family:"Aptos",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
font-size:10.0pt;
font-family:"Courier New";}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Aptos",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt">Responses are in-line. Will post updated patch after testing<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt"><o:p> </o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;color:black">Regards,</span><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;color:black">Ramesh</span><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"><o:p></o:p></span></p>
</div>
<p class="MsoNormal"><span style="font-size:11.0pt;color:black"> </span><span style="font-size:11.0pt"><o:p></o:p></span></p>
<div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Yang, Philip <Philip.Yang@amd.com>
<br>
<b>Sent:</b> Thursday, August 22, 2024 9:28 AM<br>
<b>To:</b> Errabolu, Ramesh <Ramesh.Errabolu@amd.com>; amd-gfx@lists.freedesktop.org<br>
<b>Subject:</b> Re: [PATCH] drm/amdgpu: Surface svm_attr_gobm, a RW module parameter<o:p></o:p></span></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">On 2024-08-21 19:22, Ramesh Errabolu wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>KFD's design of unified memory (UM) does not allow users to<o:p></o:p></pre>
<pre>configure the size of buffer used in migrating buffer either<o:p></o:p></pre>
<pre>from Sysmem to VRAM or vice versa. <o:p></o:p></pre>
</blockquote>
<p>This is not true, app can change range granularity attribute, to configure the buffer migration size. This module parameter is used to config the default range granularity.<o:p></o:p></p>
<p><span style="font-size:11.0pt">Ramesh: Will update commit summary to emphasize the patch as updating default migration size that will benefit users of unregistered memory<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>This patch remedies this<o:p></o:p></pre>
<pre>gap, albeit at a coarse level, for workloads that deal with<o:p></o:p></pre>
<pre>unregistered memory<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>Signed-off-by: Ramesh Errabolu <a href="mailto:Ramesh.Errabolu@amd.com"><Ramesh.Errabolu@amd.com></a><o:p></o:p></pre>
<pre>---<o:p></o:p></pre>
<pre> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++++<o:p></o:p></pre>
<pre> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 16 ++++++++++++++<o:p></o:p></pre>
<pre> drivers/gpu/drm/amd/amdkfd/kfd_priv.h | 12 ++++++++++<o:p></o:p></pre>
<pre> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 29 +++++++++++++++++--------<o:p></o:p></pre>
<pre> 4 files changed, 52 insertions(+), 9 deletions(-)<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h<o:p></o:p></pre>
<pre>index e8c284aea1f2..73dd816b01f2 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h<o:p></o:p></pre>
<pre>@@ -237,6 +237,7 @@ extern int sched_policy;<o:p></o:p></pre>
<pre> extern bool debug_evictions;<o:p></o:p></pre>
<pre> extern bool no_system_mem_limit;<o:p></o:p></pre>
<pre> extern int halt_if_hws_hang;<o:p></o:p></pre>
<pre>+extern uint amdgpu_svm_attr_gobm;<o:p></o:p></pre>
</blockquote>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre> #else<o:p></o:p></pre>
<pre> static const int __maybe_unused sched_policy = KFD_SCHED_POLICY_HWS;<o:p></o:p></pre>
<pre> static const bool __maybe_unused debug_evictions; /* = false */<o:p></o:p></pre>
<pre>@@ -313,6 +314,9 @@ extern int amdgpu_wbrf;<o:p></o:p></pre>
<pre> /* Extra time delay(in ms) to eliminate the influence of temperature momentary fluctuation */<o:p></o:p></pre>
<pre> #define AMDGPU_SWCTF_EXTRA_DELAY 50<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>+/* Default size of buffer to use in migrating buffer */<o:p></o:p></pre>
<pre>+#define AMDGPU_SVM_ATTR_GOBM 9<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> struct amdgpu_xcp_mgr;<o:p></o:p></pre>
<pre> struct amdgpu_device;<o:p></o:p></pre>
<pre> struct amdgpu_irq_src;<o:p></o:p></pre>
<pre>diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c<o:p></o:p></pre>
<pre>index b9529948f2b2..e195e1cf0f28 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c<o:p></o:p></pre>
<pre>@@ -169,6 +169,15 @@ uint amdgpu_sdma_phase_quantum = 32;<o:p></o:p></pre>
<pre> char *amdgpu_disable_cu;<o:p></o:p></pre>
<pre> char *amdgpu_virtual_display;<o:p></o:p></pre>
<pre> bool enforce_isolation;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+/* Specifies the default size of buffer to use in<o:p></o:p></pre>
<pre>+ * migrating buffer from Sysmem to VRAM and vice<o:p></o:p></pre>
<pre>+ * versa<o:p></o:p></pre>
<pre>+ *<o:p></o:p></pre>
<pre>+ * Defined as log2(sizeof(buffer)/PAGE_SIZE)<o:p></o:p></pre>
<pre>+ */<o:p></o:p></pre>
<pre>+uint amdgpu_svm_attr_gobm = AMDGPU_SVM_ATTR_GOBM;<o:p></o:p></pre>
</blockquote>
<p>/* add explanation, GOBM : granularity of buffer migration<o:p></o:p></p>
<p><span style="font-size:11.0pt">Ramesh: Done<o:p></o:p></span></p>
<p>Use u8 type, the same type used in prange->granularity<o:p></o:p></p>
<p>u8 amdgpu_svm_default_gobm = AMDGPU_SVM_DEFAULT_GOBM <o:p></o:p></p>
<p><span style="font-size:11.0pt">Ramesh: Leaving it as uint. u8 cannot be used as it is not a supported by in “include/linux/modueparam.h” file<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> /*<o:p></o:p></pre>
<pre> * OverDrive(bit 14) disabled by default<o:p></o:p></pre>
<pre> * GFX DCS(bit 19) disabled by default<o:p></o:p></pre>
<pre>@@ -320,6 +329,13 @@ module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);<o:p></o:p></pre>
<pre> MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 = auto)");<o:p></o:p></pre>
<pre> module_param_named(msi, amdgpu_msi, int, 0444);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>+/**<o:p></o:p></pre>
<pre>+ * DOC: svm_attr_gobm (uint)<o:p></o:p></pre>
<pre>+ * Size of buffer to use in migrating buffer from Sysmem to VRAM and vice versa<o:p></o:p></pre>
<pre>+ */<o:p></o:p></pre>
<pre>+MODULE_PARM_DESC(svm_attr_gobm, "Defined as log2(sizeof(buffer)/PAGE_SIZE), e.g. 9 for 2 MiB");<o:p></o:p></pre>
<pre>+module_param_named(svm_attr_gobm, amdgpu_svm_attr_gobm, uint, 0644);<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> /**<o:p></o:p></pre>
<pre> * DOC: lockup_timeout (string)<o:p></o:p></pre>
<pre> * Set GPU scheduler timeout value in ms.<o:p></o:p></pre>
<pre>diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<o:p></o:p></pre>
<pre>index 7bba6bed2f48..07b202ab008a 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h<o:p></o:p></pre>
<pre>@@ -866,6 +866,18 @@ struct svm_range_list {<o:p></o:p></pre>
<pre> struct delayed_work restore_work;<o:p></o:p></pre>
<pre> DECLARE_BITMAP(bitmap_supported, MAX_GPU_INSTANCE);<o:p></o:p></pre>
<pre> struct task_struct *faulting_task;<o:p></o:p></pre>
<pre>+<o:p></o:p></pre>
<pre>+ /* Indicates the default size to use in migrating<o:p></o:p></pre>
<pre>+ * buffers of a process from Sysmem to VRAM and vice<o:p></o:p></pre>
<pre>+ * versa. The max legal value cannot be greater than<o:p></o:p></pre>
<pre>+ * 0x3F<o:p></o:p></pre>
<pre>+ *<o:p></o:p></pre>
<pre>+ * @note: A side effect of this symbol being part of<o:p></o:p></pre>
<pre>+ * struct svm_range_list is that it forces all buffers<o:p></o:p></pre>
<pre>+ * of the process of unregistered kind to use the same<o:p></o:p></pre>
<pre>+ * size in buffer migration<o:p></o:p></pre>
<pre>+ */<o:p></o:p></pre>
<pre>+ uint8_t attr_gobm;<o:p></o:p></pre>
<pre> };<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> /* Process data */<o:p></o:p></pre>
<pre>diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>index 10b1a1f64198..fcfe5543a3c0 100644<o:p></o:p></pre>
<pre>--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c<o:p></o:p></pre>
<pre>@@ -309,12 +309,11 @@ static void svm_range_free(struct svm_range *prange, bool do_unmap)<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> static void<o:p></o:p></pre>
<pre>-svm_range_set_default_attributes(int32_t *location, int32_t *prefetch_loc,<o:p></o:p></pre>
<pre>- uint8_t *granularity, uint32_t *flags)<o:p></o:p></pre>
<pre>+svm_range_set_default_attributes(int32_t *location,<o:p></o:p></pre>
<pre>+ int32_t *prefetch_loc, uint32_t *flags)<o:p></o:p></pre>
<pre> {<o:p></o:p></pre>
<pre> *location = KFD_IOCTL_SVM_LOCATION_UNDEFINED;<o:p></o:p></pre>
<pre> *prefetch_loc = KFD_IOCTL_SVM_LOCATION_UNDEFINED;<o:p></o:p></pre>
<pre>- *granularity = 9;<o:p></o:p></pre>
<pre> *flags =<o:p></o:p></pre>
<pre> KFD_IOCTL_SVM_FLAG_HOST_ACCESS | KFD_IOCTL_SVM_FLAG_COHERENT;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre>@@ -358,9 +357,9 @@ svm_range *svm_range_new(struct svm_range_list *svms, uint64_t start,<o:p></o:p></pre>
<pre> bitmap_copy(prange->bitmap_access, svms->bitmap_supported,<o:p></o:p></pre>
<pre> MAX_GPU_INSTANCE);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>+ prange->granularity = svms->attr_gobm;<o:p></o:p></pre>
<pre> svm_range_set_default_attributes(&prange->preferred_loc,<o:p></o:p></pre>
<pre>- &prange->prefetch_loc,<o:p></o:p></pre>
<pre>- &prange->granularity, &prange->flags);<o:p></o:p></pre>
<pre>+ &prange->prefetch_loc, &prange->flags);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> pr_debug("svms 0x%p [0x%llx 0x%llx]\n", svms, start, last);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>@@ -2685,10 +2684,12 @@ svm_range_get_range_boundaries(struct kfd_process *p, int64_t addr,<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> *is_heap_stack = vma_is_initial_heap(vma) || vma_is_initial_stack(vma);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>+ /* Determine the starting and ending page of prange */<o:p></o:p></pre>
<pre> start_limit = max(vma->vm_start >> PAGE_SHIFT,<o:p></o:p></pre>
<pre>- (unsigned long)ALIGN_DOWN(addr, 2UL << 8));<o:p></o:p></pre>
<pre>+ (unsigned long)ALIGN_DOWN(addr, (1 << p->svms.attr_gobm)));<o:p></o:p></pre>
</blockquote>
<p class="MsoNormal">1UL << p->svms.attr_gobm <o:p></o:p></p>
<p class="MsoNormal"><span style="font-size:11.0pt">Ramesh: Done<o:p></o:p></span></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre> end_limit = min(vma->vm_end >> PAGE_SHIFT,<o:p></o:p></pre>
<pre>- (unsigned long)ALIGN(addr + 1, 2UL << 8));<o:p></o:p></pre>
<pre>+ (unsigned long)ALIGN(addr + 1, (1 << p->svms.attr_gobm)));<o:p></o:p></pre>
</blockquote>
<pre>1UL << p->svms.attr_gobm<o:p></o:p></pre>
<pre><span style="font-size:11.0pt;font-family:"Aptos",sans-serif">Ramesh: Done<o:p></o:p></span></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>+<o:p></o:p></pre>
<pre> /* First range that starts after the fault address */<o:p></o:p></pre>
<pre> node = interval_tree_iter_first(&p->svms.objects, addr + 1, ULONG_MAX);<o:p></o:p></pre>
<pre> if (node) {<o:p></o:p></pre>
<pre>@@ -3211,6 +3212,15 @@ int svm_range_list_init(struct kfd_process *p)<o:p></o:p></pre>
<pre> if (KFD_IS_SVM_API_SUPPORTED(p->pdds[i]->dev->adev))<o:p></o:p></pre>
<pre> bitmap_set(svms->bitmap_supported, i, 1);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>+ /* Bind granularity of buffer migration, either<o:p></o:p></pre>
<pre>+ * the default size or one specified by the user<o:p></o:p></pre>
<pre>+ */<o:p></o:p></pre>
<pre>+ svms->attr_gobm = amdgpu_svm_attr_gobm;<o:p></o:p></pre>
<pre>+ if (unlikely(amdgpu_svm_attr_gobm != AMDGPU_SVM_ATTR_GOBM))<o:p></o:p></pre>
<pre>+ svms->attr_gobm = min_t(uint32_t, amdgpu_svm_attr_gobm, 0x3F);<o:p></o:p></pre>
</blockquote>
<p>to simplify, remove the if check and always use this:<o:p></o:p></p>
<pre>svms->attr_gobm = min_t(uint32_t, amdgpu_svm_attr_gobm, 0x3F);<o:p></o:p></pre>
<pre><span style="font-size:11.0pt;font-family:"Aptos",sans-serif">Ramesh: done<o:p></o:p></span></pre>
<pre><span style="font-size:11.0pt;font-family:"Aptos",sans-serif"><o:p> </o:p></span></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>+ pr_debug("%s() Granularity Of Buffer Migration: %d\n",<o:p></o:p></pre>
<pre>+ __func__, svms->attr_gobm);<o:p></o:p></pre>
</blockquote>
<p>pr_debug can output function name if adding +pf dynamically, __func__ is not needed.<o:p></o:p></p>
<p><span style="font-size:11.0pt">Ramesh: done</span><o:p></o:p></p>
<p>Regards,<o:p></o:p></p>
<p>Philip<o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre><o:p> </o:p></pre>
<pre>+<o:p></o:p></pre>
<pre> return 0;<o:p></o:p></pre>
<pre> }<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>@@ -3738,8 +3748,9 @@ svm_range_get_attr(struct kfd_process *p, struct mm_struct *mm,<o:p></o:p></pre>
<pre> node = interval_tree_iter_first(&svms->objects, start, last);<o:p></o:p></pre>
<pre> if (!node) {<o:p></o:p></pre>
<pre> pr_debug("range attrs not found return default values\n");<o:p></o:p></pre>
<pre>- svm_range_set_default_attributes(&location, &prefetch_loc,<o:p></o:p></pre>
<pre>- &granularity, &flags_and);<o:p></o:p></pre>
<pre>+ granularity = svms->attr_gobm;<o:p></o:p></pre>
<pre>+ svm_range_set_default_attributes(&location,<o:p></o:p></pre>
<pre>+ &prefetch_loc, &flags_and);<o:p></o:p></pre>
<pre> flags_or = flags_and;<o:p></o:p></pre>
<pre> if (p->xnack_enabled)<o:p></o:p></pre>
<pre> bitmap_copy(bitmap_access, svms->bitmap_supported,<o:p></o:p></pre>
</blockquote>
</div>
</body>
</html>