<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> Am 12.03.25 um 10:23 schrieb Zhang, Jesse(Jie): <blockquote type="cite" cite="mid:DM4PR12MB5152C8596272C0BED39068B4E3D02@DM4PR12MB5152.namprd12.prod.outlook.com"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style>@font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face {font-family:DengXian; panose-1:2 1 6 0 3 1 1 1 1 1;}@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;}@font-face {font-family:"\@DengXian"; panose-1:2 1 6 0 3 1 1 1 1 1;}p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0in; font-size:12.0pt; font-family:"Aptos",sans-serif;}pre {mso-style-priority:99; mso-style-link:"HTML Preformatted Char"; margin:0in; margin-bottom:.0001pt; 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",serif;}span.EmailStyle20 {mso-style-type:personal-reply; font-family:"Arial",sans-serif; color:windowtext;}.MsoChpDefault {mso-style-type:export-only; font-size:10.0pt; mso-ligatures:none;}div.WordSection1 {page:WordSection1;}</style> [AMD Official Use Only - AMD Internal Distribution Only] <div> <div class="WordSection1"> <o:p> </o:p> <o:p> </o:p> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> From: Koenig, Christian <a class="moz-txt-link-rfc2396E" href="mailto:Christian.Koenig@amd.com"><Christian.Koenig@amd.com></a> Sent: Wednesday, March 12, 2025 4:39 PM To: Zhang, Jesse(Jie) <a class="moz-txt-link-rfc2396E" href="mailto:Jesse.Zhang@amd.com"><Jesse.Zhang@amd.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Cc: Deucher, Alexander <a class="moz-txt-link-rfc2396E" href="mailto:Alexander.Deucher@amd.com"><Alexander.Deucher@amd.com></a>; Kim, Jonathan <a class="moz-txt-link-rfc2396E" href="mailto:Jonathan.Kim@amd.com"><Jonathan.Kim@amd.com></a>; Zhu, Jiadong <a class="moz-txt-link-rfc2396E" href="mailto:Jiadong.Zhu@amd.com"><Jiadong.Zhu@amd.com></a> Subject: Re: [PATCH 1/7] drm/amd/amdgpu: Simplify SDMA reset mechanism by removing dynamic callbacks<o:p></o:p> </div> </div> <o:p> </o:p> Am 12.03.25 um 09:15 schrieb Zhang, Jesse(Jie): <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> [SNIP9 <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre>-<o:p></o:p></pre> <pre>+ gfx_ring->funcs->stop_queue(adev, instance_id);<o:p></o:p></pre> </blockquote> <pre><o:p> </o:p></pre> <pre>Yeah that starts to look good. Question here is who is calling amdgpu_sdma_reset_engine()?<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>If this call comes from engine specific code we might not need the start/stop_queue callbacks all together.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre> Kfd and sdma v4/v5/v5_2 will call amdgpu_sdma_reset_engine, and start/stop_queue callbacks are only implemented in sdmav4/sdmav5/sdma5_2.<o:p></o:p></pre> </blockquote> Why would the KFD call this as well? Because it detects an issue with a SDMA user queue If yes I would rather suggest that the KFD calls the reset function of the paging queue. Since this reset function is specific to the SDMA HW generation anyway you don't need those extra functions to abstract starting and stopping of the queue for each HW generation. <o:p></o:p> kfd can't call reset function directly, unless we add a parameter src to distinguish kfd and kgd in reset function, like this:<o:p></o:p> int (*reset)(struct amdgpu_ring *ring, unsigned int vmid, int src );<o:p></o:p> As Alex said in another thread,<o:p></o:p> We need to distinguish kfd and kgd in reset.<o:p></o:p> If kfd triggers a reset, kgd must save healthy jobs and recover jobs after reset.<o:p></o:p> If kgd triggers a reset, kgd must abandon bad jobs after reset.(and perhaps kfd needs to save its healthy jobs for recovery). </div> </div> </blockquote> I don't think the source of the reset should be relevant to the reset procedure. The source is basically just the first one who runs into a timeout, that can be both KFD and KGD. But the cause of the timeout is not necessary the one who signals that a timeout happens. So as far as I can see you should not have that as parameter either. <blockquote type="cite" cite="mid:DM4PR12MB5152C8596272C0BED39068B4E3D02@DM4PR12MB5152.namprd12.prod.outlook.com"> <div> <div class="WordSection1"> <o:p></o:p> <o:p> </o:p> If we can add a parameter, I am ok for that solution too.<o:p></o:p> <o:p> </o:p> Additionally:<o:p></o:p> For sdma6/7, when a queue reset fails, we may need to fall back to an engine reset for a attempt. </div> </div> </blockquote> Yeah, but that should be trivial. Regards, Christian. <blockquote type="cite" cite="mid:DM4PR12MB5152C8596272C0BED39068B4E3D02@DM4PR12MB5152.namprd12.prod.outlook.com"> <div> <div class="WordSection1"> <o:p></o:p> <o:p> </o:p> Thanks<o:p></o:p> Jesse<o:p></o:p> Regards, Christian. <o:p></o:p> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre><o:p> </o:p></pre> <pre><o:p> </o:p></pre> <pre>Thanks<o:p></o:p></pre> <pre>Jesse<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>Regards,<o:p></o:p></pre> <pre>Christian.<o:p></o:p></pre> <pre><o:p> </o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt"> <pre> /* Perform the SDMA reset for the specified instance */<o:p></o:p></pre> <pre> ret = amdgpu_dpm_reset_sdma(adev, 1 << instance_id);<o:p></o:p></pre> <pre> if (ret) {<o:p></o:p></pre> <pre>@@ -591,18 +573,7 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id, b<o:p></o:p></pre> <pre> goto exit;<o:p></o:p></pre> <pre> }<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre>- /* Invoke all registered post_reset callbacks */<o:p></o:p></pre> <pre>- list_for_each_entry(funcs, &adev->sdma.reset_callback_list, list) {<o:p></o:p></pre> <pre>- if (funcs->post_reset) {<o:p></o:p></pre> <pre>- ret = funcs->post_reset(adev, instance_id);<o:p></o:p></pre> <pre>- if (ret) {<o:p></o:p></pre> <pre>- dev_err(adev->dev,<o:p></o:p></pre> <pre>- "afterReset callback failed for instance %u: %d\n",<o:p></o:p></pre> <pre>- instance_id, ret);<o:p></o:p></pre> <pre>- goto exit;<o:p></o:p></pre> <pre>- }<o:p></o:p></pre> <pre>- }<o:p></o:p></pre> <pre>- }<o:p></o:p></pre> <pre>+ gfx_ring->funcs->start_queue(adev, instance_id);<o:p></o:p></pre> <pre><o:p> </o:p></pre> <pre> exit:<o:p></o:p></pre> <pre> /* Restart the scheduler's work queue for the GFX and page rings<o:p></o:p></pre> <pre>diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c<o:p></o:p></pre> <pre>b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c<o:p></o:p></pre> <pre>index fd34dc138081..c1f7ccff9c4e 100644<o:p></o:p></pre> <pre>--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c<o:p></o:p></pre> <pre>+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c<o:p></o:p></pre> <pre>@@ -2132,6 +2132,8 @@ static const struct amdgpu_ring_funcs sdma_v4_4_2_ring_funcs = {<o:p></o:p></pre> <pre> .emit_reg_wait = sdma_v4_4_2_ring_emit_reg_wait,<o:p></o:p></pre> <pre> .emit_reg_write_reg_wait = amdgpu_ring_emit_reg_write_reg_wait_helper,<o:p></o:p></pre> <pre> .reset = sdma_v4_4_2_reset_queue,<o:p></o:p></pre> <pre>+ .stop_queue = sdma_v4_4_2_stop_queue,<o:p></o:p></pre> <pre>+ .start_queue = sdma_v4_4_2_restore_queue,<o:p></o:p></pre> <pre> .is_guilty = sdma_v4_4_2_ring_is_guilty, };<o:p></o:p></pre> <pre><o:p> </o:p></pre> </blockquote> <pre><o:p> </o:p></pre> </blockquote> <o:p> </o:p> </div> </div> </blockquote> </body> </html>