<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style> </head> <body dir="ltr"> [AMD Official Use Only - General] <div> <div class="moz-cite-prefix">On 2023-10-03 17:37, Felix Kuehling wrote: </div> <blockquote type="cite">On 2023-10-03 16:50, Philip Yang wrote: <blockquote type="cite" style="color: rgb(0, 124, 255);">If there is no VRAM domain, bo_node is NULL and this causes crash. Refactor the change, and use the module parameter as higher privilege. Need another patch to support override PTE flag on APU. Fixes: 55d7e2001c7e ("drm/amdgpu: Add EXT_COHERENT memory allocation flags") Signed-off-by: Philip Yang <a href="mailto:Philip.Yang@amd.com" class="moz-txt-link-rfc2396E OWAAutoLink" data-loopstyle="linkonly" id="OWA5551543b-0498-5b81-127d-578773d2e4b2"> <Philip.Yang@amd.com></a> --- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index 0d88698ae33f..305b2c54edfa 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -1248,26 +1248,22 @@ svm_range_get_pte_flags(struct kfd_node *node, break; case IP_VERSION(9, 4, 3): mtype_local = amdgpu_mtype_local == 1 ? AMDGPU_VM_MTYPE_NC : - (amdgpu_mtype_local == 2 ? AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW); + (amdgpu_mtype_local == 2 || ext_coherent ? + AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW); </blockquote> We had some offline discussion where I thought that MTYPE_NC should become MTYPE_UC when ext_coherent is enabled to get the desired memory semantics. With that idea in mind, this would become a bit more messy, but here it goes, as clean as I can make it: - mtype_local = amdgpu_mtype_local == 1 ? AMDGPU_VM_MTYPE_NC : - (amdgpu_mtype_local == 2 ? AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW); + mtype_local = amdgpu_mtype_local == 1 && !ext_coherent ? AMDGPU_VM_MTYPE_NC : + (amdgpu_mtype_local == 1 && ext_coherent ? AMDGPU_VM_MTYPE_UC : + (amdgpu_mtype_local == 2 || ext_coherent ? AMDGPU_VM_MTYPE_CC : + AMDGPU_VM_MTYPE_RW)); </blockquote> That ternary looks fairly gnarly. I think it would be worth the extra ink to write mtype_local = amdgpu_mtype_local == 1 ? AMDGPU_VM_MTYPE_NC : (amdgpu_mtype_local == 2 ? AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW); if (ext_coherent) { if (amdgpu_mtype_local = 1) mtype_local = AMDGPU_VM_MTYPE_UC; else mtype_local = AMDGPU_VM_MTYPE_CC; } <blockquote type="cite">But maybe that could be fixed up in a follow up patch. Either way, for the purpose of fixing the crash, this patch is Reviewed-by: Felix Kuehling <a href="mailto:Felix.Kuehling@amd.com" class="moz-txt-link-rfc2396E OWAAutoLink" data-loopstyle="linkonly" id="OWA0fb4eb3e-e5a8-e7ad-ed11-bf5f07139e99"> <Felix.Kuehling@amd.com></a> <blockquote type="cite" style="color: rgb(0, 124, 255);"> snoop = true; if (uncached) { mapping_flags |= AMDGPU_VM_MTYPE_UC; - } else if (ext_coherent) { - /* local HBM region close to partition */ - if (bo_node->adev == node->adev && - (!bo_node->xcp || !node->xcp || bo_node->xcp->mem_id == node->xcp->mem_id)) - mapping_flags |= AMDGPU_VM_MTYPE_CC; - else - mapping_flags |= AMDGPU_VM_MTYPE_UC; } else if (domain == SVM_RANGE_VRAM_DOMAIN) { /* local HBM region close to partition */ if (bo_node->adev == node->adev && (!bo_node->xcp || !node->xcp || bo_node->xcp->mem_id == node->xcp->mem_id)) mapping_flags |= mtype_local; - /* local HBM region far from partition or remote XGMI GPU */ - else if (svm_nodes_in_same_hive(bo_node, node)) + /* local HBM region far from partition or remote XGMI GPU + * with regular system scope coherence + */ + else if (svm_nodes_in_same_hive(bo_node, node) && !ext_coherent) mapping_flags |= AMDGPU_VM_MTYPE_NC; - /* PCIe P2P */ + /* PCIe P2P or extended system scope coherence */ else mapping_flags |= AMDGPU_VM_MTYPE_UC; </blockquote> </blockquote> Would probably clearer if these two branches were swapped so the first was (!svm_nodes_in_same_hive(bo_node, node) || ext_coherent) Not a required change, though. <blockquote type="cite"> <blockquote type="cite" style="color: rgb(0, 124, 255);"> /* system memory accessed by the APU */ </blockquote> </blockquote> This patch as written causes ext_coherent to no longer affect gfx9.4.3 APU devices, which it should. The following (or equivalent) needs to be added just below this hunk if (num_possible_nodes() <= 1) mapping_flags |= mtype_local; else - mapping_flags |= AMDGPU_VM_MTYPE_NC; + mapping_flags |= ext_coherent ? AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC; </div> </body> </html>