[PATCH 5/5] drm/amdgpu: fix shadow BO restoring

Jerry.Zhang@xxxxxxx (Zhang, Jerry(Junwei)) · Tue, 18 Sep 2018 14:15:37 +0800



On 09/14/2018 07:54 PM, Christian KÃ¶nig wrote:
> Am 13.09.2018 um 11:29 schrieb Zhang, Jerry(Junwei):
>> On 09/11/2018 05:56 PM, Christian KÃ¶nig wrote:
>>> Don't grab the reservation lock any more and simplify the handling 
>>> quite
>>> a bit.
>>>
>>> Signed-off-by: Christian KÃ¶nig <christian.koenig at amd.com>
>>> ---
>>> Â  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 109 
>>> ++++++++---------------------
>>> Â  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |Â  46 ++++--------
>>> Â  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |Â Â  8 +--
>>> Â  3 files changed, 43 insertions(+), 120 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 5eba66ecf668..20bb702f5c7f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -2940,54 +2940,6 @@ static int 
>>> amdgpu_device_ip_post_soft_reset(struct amdgpu_device *adev)
>>> Â Â Â Â Â  return 0;
>>> Â  }
>>> Â  -/**
>>> - * amdgpu_device_recover_vram_from_shadow - restore shadowed VRAM 
>>> buffers
>>> - *
>>> - * @adev: amdgpu_device pointer
>>> - * @ring: amdgpu_ring for the engine handling the buffer operations
>>> - * @bo: amdgpu_bo buffer whose shadow is being restored
>>> - * @fence: dma_fence associated with the operation
>>> - *
>>> - * Restores the VRAM buffer contents from the shadow in GTT. Used to
>>> - * restore things like GPUVM page tables after a GPU reset where
>>> - * the contents of VRAM might be lost.
>>> - * Returns 0 on success, negative error code on failure.
>>> - */
>>> -static int amdgpu_device_recover_vram_from_shadow(struct 
>>> amdgpu_device *adev,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_ring *ring,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_bo *bo,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct dma_fence **fence)
>>> -{
>>> -Â Â Â  uint32_t domain;
>>> -Â Â Â  int r;
>>> -
>>> -Â Â Â  if (!bo->shadow)
>>> -Â Â Â Â Â Â Â  return 0;
>>> -
>>> -Â Â Â  r = amdgpu_bo_reserve(bo, true);
>>> -Â Â Â  if (r)
>>> -Â Â Â Â Â Â Â  return r;
>>> -Â Â Â  domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
>>> -Â Â Â  /* if bo has been evicted, then no need to recover */
>>> -Â Â Â  if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>> -Â Â Â Â Â Â Â  r = amdgpu_bo_validate(bo->shadow);
>>> -Â Â Â Â Â Â Â  if (r) {
>>> -Â Â Â Â Â Â Â Â Â Â Â  DRM_ERROR("bo validate failed!\n");
>>> -Â Â Â Â Â Â Â Â Â Â Â  goto err;
>>> -Â Â Â Â Â Â Â  }
>>> -
>>> -Â Â Â Â Â Â Â  r = amdgpu_bo_restore_from_shadow(adev, ring, bo,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  NULL, fence, true);
>>> -Â Â Â Â Â Â Â  if (r) {
>>> -Â Â Â Â Â Â Â Â Â Â Â  DRM_ERROR("recover page table failed!\n");
>>> -Â Â Â Â Â Â Â Â Â Â Â  goto err;
>>> -Â Â Â Â Â Â Â  }
>>> -Â Â Â  }
>>> -err:
>>> -Â Â Â  amdgpu_bo_unreserve(bo);
>>> -Â Â Â  return r;
>>> -}
>>> -
>>> Â  /**
>>> Â Â  * amdgpu_device_recover_vram - Recover some VRAM contents
>>> Â Â  *
>>> @@ -2996,16 +2948,15 @@ static int 
>>> amdgpu_device_recover_vram_from_shadow(struct amdgpu_device *adev,
>>> Â Â  * Restores the contents of VRAM buffers from the shadows in GTT.Â  
>>> Used to
>>> Â Â  * restore things like GPUVM page tables after a GPU reset where
>>> Â Â  * the contents of VRAM might be lost.
>>> - * Returns 0 on success, 1 on failure.
>>> + *
>>> + * Returns:
>>> + * 0 on success, negative error code on failure.
>>> Â Â  */
>>> Â  static int amdgpu_device_recover_vram(struct amdgpu_device *adev)
>>> Â  {
>>> -Â Â Â  struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
>>> -Â Â Â  struct amdgpu_bo *bo, *tmp;
>>> Â Â Â Â Â  struct dma_fence *fence = NULL, *next = NULL;
>>> -Â Â Â  long r = 1;
>>> -Â Â Â  int i = 0;
>>> -Â Â Â  long tmo;
>>> +Â Â Â  struct amdgpu_bo *shadow;
>>> +Â Â Â  long r = 1, tmo;
>>> Â  Â Â Â Â Â  if (amdgpu_sriov_runtime(adev))
>>> Â Â Â Â Â Â Â Â Â  tmo = msecs_to_jiffies(8000);
>>> @@ -3014,44 +2965,40 @@ static int amdgpu_device_recover_vram(struct 
>>> amdgpu_device *adev)
>>> Â  Â Â Â Â Â  DRM_INFO("recover vram bo from shadow start\n");
>>> Â Â Â Â Â  mutex_lock(&adev->shadow_list_lock);
>>> -Â Â Â  list_for_each_entry_safe(bo, tmp, &adev->shadow_list, 
>>> shadow_list) {
>>> -Â Â Â Â Â Â Â  next = NULL;
>>> -Â Â Â Â Â Â Â  amdgpu_device_recover_vram_from_shadow(adev, ring, bo, &next);
>>> +Â Â Â  list_for_each_entry(shadow, &adev->shadow_list, shadow_list) {
>>> +
>>> +Â Â Â Â Â Â Â  /* No need to recover an evicted BO */
>>> +Â Â Â Â Â Â Â  if (shadow->tbo.mem.mem_type != TTM_PL_TT ||
>>> +Â Â Â Â Â Â Â Â Â Â Â  shadow->parent->tbo.mem.mem_type != TTM_PL_VRAM)
>> is there a change that shadow bo evicted to other domain?
>> like SYSTEM?
>
> Yes, that's why I test "!= TTM_PL_TT" here.
>
> What can happen is that either the shadow or the page table or page 
> directory is evicted.
>
> But in this case we don't need to restore anything because of patch #1 
> in this series.

Thanks, then it's
Acked-by: Junwei Zhang <Jerry.Zhang at amd.com>

Regards,
Jerry

>
> Regards,
> Christian.
>
>>
>> Regards,
>> Jerry
>>> +Â Â Â Â Â Â Â Â Â Â Â  continue;
>>> +
>>> +Â Â Â Â Â Â Â  r = amdgpu_bo_restore_shadow(shadow, &next);
>>> +Â Â Â Â Â Â Â  if (r)
>>> +Â Â Â Â Â Â Â Â Â Â Â  break;
>>> +
>>> Â Â Â Â Â Â Â Â Â  if (fence) {
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â  r = dma_fence_wait_timeout(fence, false, tmo);
>>> -Â Â Â Â Â Â Â Â Â Â Â  if (r == 0)
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  pr_err("wait fence %p[%d] timeout\n", fence, i);
>>> -Â Â Â Â Â Â Â Â Â Â Â  else if (r < 0)
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  pr_err("wait fence %p[%d] interrupted\n", fence, i);
>>> -Â Â Â Â Â Â Â Â Â Â Â  if (r < 1) {
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  dma_fence_put(fence);
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  fence = next;
>>> +Â Â Â Â Â Â Â Â Â Â Â  dma_fence_put(fence);
>>> +Â Â Â Â Â Â Â Â Â Â Â  fence = next;
>>> +Â Â Â Â Â Â Â Â Â Â Â  if (r <= 0)
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  break;
>>> -Â Â Â Â Â Â Â Â Â Â Â  }
>>> -Â Â Â Â Â Â Â Â Â Â Â  i++;
>>> +Â Â Â Â Â Â Â  } else {
>>> +Â Â Â Â Â Â Â Â Â Â Â  fence = next;
>>> Â Â Â Â Â Â Â Â Â  }
>>> -
>>> -Â Â Â Â Â Â Â  dma_fence_put(fence);
>>> -Â Â Â Â Â Â Â  fence = next;
>>> Â Â Â Â Â  }
>>> Â Â Â Â Â  mutex_unlock(&adev->shadow_list_lock);
>>> Â  -Â Â Â  if (fence) {
>>> -Â Â Â Â Â Â Â  r = dma_fence_wait_timeout(fence, false, tmo);
>>> -Â Â Â Â Â Â Â  if (r == 0)
>>> -Â Â Â Â Â Â Â Â Â Â Â  pr_err("wait fence %p[%d] timeout\n", fence, i);
>>> -Â Â Â Â Â Â Â  else if (r < 0)
>>> -Â Â Â Â Â Â Â Â Â Â Â  pr_err("wait fence %p[%d] interrupted\n", fence, i);
>>> -
>>> -Â Â Â  }
>>> +Â Â Â  if (fence)
>>> +Â Â Â Â Â Â Â  tmo = dma_fence_wait_timeout(fence, false, tmo);
>>> Â Â Â Â Â  dma_fence_put(fence);
>>> Â  -Â Â Â  if (r > 0)
>>> -Â Â Â Â Â Â Â  DRM_INFO("recover vram bo from shadow done\n");
>>> -Â Â Â  else
>>> +Â Â Â  if (r <= 0 || tmo <= 0) {
>>> Â Â Â Â Â Â Â Â Â  DRM_ERROR("recover vram bo from shadow failed\n");
>>> +Â Â Â Â Â Â Â  return -EIO;
>>> +Â Â Â  }
>>> Â  -Â Â Â  return (r > 0) ? 0 : 1;
>>> +Â Â Â  DRM_INFO("recover vram bo from shadow done\n");
>>> +Â Â Â  return 0;
>>> Â  }
>>> Â  Â  /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 3a6f92de5504..5b6d5fcc2151 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -537,7 +537,7 @@ static int amdgpu_bo_create_shadow(struct 
>>> amdgpu_device *adev,
>>> Â Â Â Â Â  if (!r) {
>>> Â Â Â Â Â Â Â Â Â  bo->shadow->parent = amdgpu_bo_ref(bo);
>>> Â Â Â Â Â Â Â Â Â  mutex_lock(&adev->shadow_list_lock);
>>> -Â Â Â Â Â Â Â  list_add_tail(&bo->shadow_list, &adev->shadow_list);
>>> +Â Â Â Â Â Â Â  list_add_tail(&bo->shadow->shadow_list, &adev->shadow_list);
>>> Â Â Â Â Â Â Â Â Â  mutex_unlock(&adev->shadow_list_lock);
>>> Â Â Â Â Â  }
>>> Â  @@ -669,13 +669,10 @@ int amdgpu_bo_validate(struct amdgpu_bo *bo)
>>> Â  }
>>> Â  Â  /**
>>> - * amdgpu_bo_restore_from_shadow - restore an &amdgpu_bo buffer object
>>> - * @adev: amdgpu device object
>>> - * @ring: amdgpu_ring for the engine handling the buffer operations
>>> - * @bo: &amdgpu_bo buffer to be restored
>>> - * @resv: reservation object with embedded fence
>>> + * amdgpu_bo_restore_shadow - restore an &amdgpu_bo shadow
>>> + *
>>> + * @shadow: &amdgpu_bo shadow to be restored
>>> Â Â  * @fence: dma_fence associated with the operation
>>> - * @direct: whether to submit the job directly
>>> Â Â  *
>>> Â Â  * Copies a buffer object's shadow content back to the object.
>>> Â Â  * This is used for recovering a buffer from its shadow in case of 
>>> a gpu
>>> @@ -684,36 +681,19 @@ int amdgpu_bo_validate(struct amdgpu_bo *bo)
>>> Â Â  * Returns:
>>> Â Â  * 0 for success or a negative error code on failure.
>>> Â Â  */
>>> -int amdgpu_bo_restore_from_shadow(struct amdgpu_device *adev,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_ring *ring,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_bo *bo,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct reservation_object *resv,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct dma_fence **fence,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  bool direct)
>>> +int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow, struct 
>>> dma_fence **fence)
>>> Â  Â  {
>>> -Â Â Â  struct amdgpu_bo *shadow = bo->shadow;
>>> -Â Â Â  uint64_t bo_addr, shadow_addr;
>>> -Â Â Â  int r;
>>> -
>>> -Â Â Â  if (!shadow)
>>> -Â Â Â Â Â Â Â  return -EINVAL;
>>> -
>>> -Â Â Â  bo_addr = amdgpu_bo_gpu_offset(bo);
>>> -Â Â Â  shadow_addr = amdgpu_bo_gpu_offset(bo->shadow);
>>> -
>>> -Â Â Â  r = reservation_object_reserve_shared(bo->tbo.resv);
>>> -Â Â Â  if (r)
>>> -Â Â Â Â Â Â Â  goto err;
>>> +Â Â Â  struct amdgpu_device *adev = amdgpu_ttm_adev(shadow->tbo.bdev);
>>> +Â Â Â  struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
>>> +Â Â Â  uint64_t shadow_addr, parent_addr;
>>> Â  -Â Â Â  r = amdgpu_copy_buffer(ring, shadow_addr, bo_addr,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  amdgpu_bo_size(bo), resv, fence,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  direct, false);
>>> -Â Â Â  if (!r)
>>> -Â Â Â Â Â Â Â  amdgpu_bo_fence(bo, *fence, true);
>>> +Â Â Â  shadow_addr = amdgpu_bo_gpu_offset(shadow);
>>> +Â Â Â  parent_addr = amdgpu_bo_gpu_offset(shadow->parent);
>>> Â  -err:
>>> -Â Â Â  return r;
>>> +Â Â Â  return amdgpu_copy_buffer(ring, shadow_addr, parent_addr,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  amdgpu_bo_size(shadow), NULL, fence,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  true, false);
>>> Â  }
>>> Â  Â  /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> index 907fdf46d895..363db417fb2e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>> @@ -273,12 +273,8 @@ int amdgpu_bo_backup_to_shadow(struct 
>>> amdgpu_device *adev,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct reservation_object *resv,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct dma_fence **fence, bool direct);
>>> Â  int amdgpu_bo_validate(struct amdgpu_bo *bo);
>>> -int amdgpu_bo_restore_from_shadow(struct amdgpu_device *adev,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_ring *ring,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct amdgpu_bo *bo,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct reservation_object *resv,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct dma_fence **fence,
>>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  bool direct);
>>> +int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow,
>>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct dma_fence **fence);
>>> Â  uint32_t amdgpu_bo_get_preferred_pin_domain(struct amdgpu_device 
>>> *adev,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  uint32_t domain);
>>
>