On 2019-03-28 3:47 p.m., Liu, Shaoyun wrote: > I think we only care about the context device (adev) and the real device > this bo been allocated(robj->tbo.bdev). The bo_va or base don't have > the device pointer directly, it have a pointer to bo which should be > the same as robj here. We can move the same_hive check out of the > loop .
That doesn't make sense. The "same hive" check only makes sense if one of the devices is the one where the memory is physically located (robj->tbo.bdev), and the other one is where the memory is accessed from. That only makes sense inside the loop. The amdgpu_vm_bo_base should tell you the device that's mapping and potentially accessing the memory over XGMI. You could get it like this: mapping_adev = base->vm->root.base.bo->tbo.bdev; Regards, Felix > > Regards > > shaoyun.liu > > > On 2019-03-28 3:18 p.m., Kuehling, Felix wrote: >> On 2019-03-28 1:55 p.m., Liu, Shaoyun wrote: >>> Avoid unnecessary XGMI hight pstate trigger when mapping none-vram memory >>> for peer device >>> >>> Change-Id: I1881deff3da19f1f4b58d5765db03a590092a5b2 >>> Signed-off-by: shaoyunl <[email protected]> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 13 +++++++++++++ >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++- >>> 2 files changed, 15 insertions(+), 1 deletion(-) >>> I think we only care about the context device (adev) and the real device >>> this bo been allocated , >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> index 9ee8d7a..82dc2b6 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> @@ -31,6 +31,7 @@ >>> #include <drm/amdgpu_drm.h> >>> #include "amdgpu.h" >>> #include "amdgpu_display.h" >>> +#include "amdgpu_xgmi.h" >>> >>> void amdgpu_gem_object_free(struct drm_gem_object *gobj) >>> { >>> @@ -666,6 +667,8 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void >>> *data, >>> struct amdgpu_device *adev = dev->dev_private; >>> struct drm_amdgpu_gem_op *args = data; >>> struct drm_gem_object *gobj; >>> + struct amdgpu_vm_bo_base *base; >>> + struct amdgpu_bo_va *bo_va; >>> struct amdgpu_bo *robj; >>> int r; >>> >>> @@ -704,6 +707,16 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void >>> *data, >>> amdgpu_bo_unreserve(robj); >>> break; >>> } >>> + for (base = robj->vm_bo; base; base = base->next) { >>> + bo_va = container_of(base, struct amdgpu_bo_va, base); >>> + if (bo_va && >>> + >>> amdgpu_xgmi_same_hive(adev,amdgpu_ttm_adev(robj->tbo.bdev))) { >> adev and robj->tbo.bdev are the same in each loop iteration. Shouldn't >> we get one of the devices from the bo_va or amdgpu_vm_bo_base? >> >> Regards, >> Felix >> >> >>> + r = -EINVAL; >>> + amdgpu_bo_unreserve(robj); >>> + goto out; >>> + } >>> + } >>> + >>> robj->preferred_domains = args->value & >>> (AMDGPU_GEM_DOMAIN_VRAM | >>> >>> AMDGPU_GEM_DOMAIN_GTT | >>> >>> AMDGPU_GEM_DOMAIN_CPU); >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index 3d221f0..eb242a1 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -2025,7 +2025,8 @@ struct amdgpu_bo_va *amdgpu_vm_bo_add(struct >>> amdgpu_device *adev, >>> INIT_LIST_HEAD(&bo_va->valids); >>> INIT_LIST_HEAD(&bo_va->invalids); >>> >>> - if (bo && amdgpu_xgmi_same_hive(adev, amdgpu_ttm_adev(bo->tbo.bdev))) { >>> + if (bo && amdgpu_xgmi_same_hive(adev, amdgpu_ttm_adev(bo->tbo.bdev)) && >>> + (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM)) { >>> bo_va->is_xgmi = true; >>> mutex_lock(&adev->vm_manager.lock_pstate); >>> /* Power up XGMI if it can be potentially used */ >> _______________________________________________ >> amd-gfx mailing list >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
