<html 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=Windows-1252"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="en-CN" link="#467886" vlink="#96607D" style="word-wrap:break-word"> [AMD Official Use Only - AMD Internal Distribution Only] <div> <div class="WordSection1"> Hi <a id="OWAAMD5EB853895CF064988F720901BE37DA9" href="mailto:Christian.Koenig@amd.com"> @Koenig, Christian</a>,<o:p></o:p> <o:p> </o:p> Thank you for the feedback. I have revised the patch according to your suggestions and sent out the v2 patch list. Please help review. Thank you! mail titles of v2 patchlist: [PATCH v2 0/3] enable switching to new gpu index for hibernate on SRIOV.<o:p></o:p> [PATCH v2 1/3] drm/amdgpu: update XGMI physical node id and GMC configs on resume<o:p></o:p> [PATCH v2 2/3] drm/amdgpu: update GPU addresses for SMU and PSP<o:p></o:p> [PATCH v2 3/3] drm/amdgpu: enable pdb0 for hibernation on SRIOV<o:p></o:p> <o:p> </o:p> Regards<o:p></o:p> Sam<o:p></o:p> <o:p> </o:p> <div id="mail-editor-reference-message-container"> <div> <div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm"> From: Koenig, Christian <Christian.Koenig@amd.com> Date: Monday, April 28, 2025 at 19:30 To: Zhang, GuoQing (Sam) <GuoQing.Zhang@amd.com>, Christian König <ckoenig.leichtzumerken@gmail.com>, amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>, Deucher, Alexander <Alexander.Deucher@amd.com> Cc: Zhao, Victor <Victor.Zhao@amd.com>, Chang, HaiJun <HaiJun.Chang@amd.com>, Deng, Emily <Emily.Deng@amd.com>, Zhang, Owen(SRDC) <Owen.Zhang2@amd.com> Subject: Re: [PATCH 4/6] drm/amdgpu: enable pdb0 for hibernation on SRIOV<o:p></o:p> </div> <div> On 4/24/25 05:38, Zhang, GuoQing (Sam) wrote: > Hi Christian, > > Thank you for the review and the feedback.I will update the patch according to > your feedback. > > Please see my 2 inline comments below. Please make sure to always CC my work mail address, otherwise I will only take a look the next time I work through the mailing lists. > >> > index d90e9daf5a50..83a3444c69d9 100644 > >> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > >> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > >> > @@ -287,8 +287,14 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev, > >> > goto error_unpin; > >> > } > >> > > >> > - if (gpu_addr) > >> > + if (gpu_addr) { > >> > *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr); > >> > + if (!adev->gmc.xgmi.connected_to_cpu && adev->gmc.enable_pdb0) { > >> > + if ((*bo_ptr)->tbo.resource->mem_type == TTM_PL_VRAM) { > >> > + *gpu_addr -= amdgpu_ttm_domain_start(adev, TTM_PL_VRAM); > >> > + } > >> > + } > >> > + } > >> > >> Please NAK to that approach here. The GPU offset should still point into the mapped VRAM. > > This change is to change to the default GPU address from FB aperture type to > pdb0 type in this centralized place so that I don’t need to change every > callsite of amdgpu_bo_create_reserved(). > > Could you suggest a better approach if this approach is not acceptable? The whole code is completely superflous. When PDB0 is used the vram_start is adjusted and you don't need to do anything here. See function amdgpu_gmc_sysvm_location(). You probably need to adjust that to have a static setup instead of using the XGMI node infos. >> > @@ -1719,6 +1723,14 @@ static void gmc_v9_0_vram_gtt_location(struct amdgpu_device *adev, > >> > { > >> > u64 base = adev->mmhub.funcs->get_fb_location(adev); > >> > > >> > + if (adev->gmc.xgmi.connected_to_cpu || adev->gmc.enable_pdb0) { > >> > + adev->gmc.vmid0_page_table_depth = 1; > >> > + adev->gmc.vmid0_page_table_block_size = 12; > >> > + } else { > >> > + adev->gmc.vmid0_page_table_depth = 0; > >> > + adev->gmc.vmid0_page_table_block_size = 0; > >> > + } > >> > + > >> > >> What is the justification to moving that stuff around? > > vmid0_page_table_block_size is used in new code in amdgpu_gmc_sysvm_location(). > See the call sequence below. > > gmc_v9_0_sw_init > > - gmc_v9_0_mc_init > > - gmc_v9_0_vram_gtt_location, > > - vmid0_page_table_block_size = 12, **new > location** > > - amdgpu_gmc_sysvm_location > > - use > **vmid0_page_table_block_size** > > - gmc_v9_0_gart_init, > > - assign vmid0_page_table_block_size, **old location** That is noteven remotely corect. See the code in gmc_v9_0_vram_gtt_location(). You use amdgpu_gmc_sysvm_location() when PDB0 is allocted and you use gmc_v9_0_vram_gtt_location() when it isn't. But adjusting this function here doesn't make any sense at all. Regards, Christian. </div> </div> </div> </div> </div> </body> </html>