Comments inline [FK]
On 2018-06-26 04:35 AM, Junwei Zhang wrote:
> Instead of calling gart memory on every bo pin,
> allocates it on demand
>
> v2: fix error handling
>
> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c | 6 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 6 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c | 14 ++++++++++++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 8 ++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 15 +++++++++------
> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 5 +++++
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++++++++++--
> 8 files changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> index 98e3bf8..e3ed08d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.c
> @@ -280,6 +280,12 @@ int alloc_gtt_mem(struct kgd_dev *kgd, size_t size,
> goto allocate_mem_pin_bo_failed;
> }
>
> + r = amdgpu_ttm_alloc_gart(&bo->tbo);
> + if (r) {
> + dev_err(adev->dev, "%p bind failed\n", bo);
> + goto allocate_mem_kmap_bo_failed;
> + }
> +
> r = amdgpu_bo_kmap(bo, &cpu_ptr_tmp);
> if (r) {
> dev_err(adev->dev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 079af8a..2c0bc7c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1593,6 +1593,12 @@ int amdgpu_amdkfd_gpuvm_map_gtt_bo_to_kernel(struct kgd_dev *kgd,
> goto pin_failed;
> }
>
> + ret = amdgpu_ttm_alloc_gart(&bo->tbo);
> + if (ret) {
> + pr_err("%p bind failed\n", bo);
> + goto kmap_failed;
> + }
> +
[FK] I think think in this case we don't need the GART mapping. This
function doesn't query the GPU address of the pinned buffer. The only
reason we pin the BO here is to keep the kernel mapping valid.
Regards,
 Felix
> ret = amdgpu_bo_kmap(bo, kptr);
> if (ret) {
> pr_err("Failed to map bo to kernel. ret %d\n", ret);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
> index cb88d7e..3079ea8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c
> @@ -96,11 +96,16 @@ static void amdgpu_benchmark_move(struct amdgpu_device *adev, unsigned size,
> if (unlikely(r != 0))
> goto out_cleanup;
> r = amdgpu_bo_pin(sobj, sdomain);
> - saddr = amdgpu_bo_gpu_offset(sobj);
> + if (r) {
> + amdgpu_bo_unreserve(sobj);
> + goto out_cleanup;
> + }
> + r = amdgpu_ttm_alloc_gart(&sobj->tbo);
> amdgpu_bo_unreserve(sobj);
> if (r) {
> goto out_cleanup;
> }
> + saddr = amdgpu_bo_gpu_offset(sobj);
> bp.domain = ddomain;
> r = amdgpu_bo_create(adev, &bp, &dobj);
> if (r) {
> @@ -110,11 +115,16 @@ static void amdgpu_benchmark_move(struct amdgpu_device *adev, unsigned size,
> if (unlikely(r != 0))
> goto out_cleanup;
> r = amdgpu_bo_pin(dobj, ddomain);
> - daddr = amdgpu_bo_gpu_offset(dobj);
> + if (r) {
> + amdgpu_bo_unreserve(sobj);
> + goto out_cleanup;
> + }
> + r = amdgpu_ttm_alloc_gart(&dobj->tbo);
> amdgpu_bo_unreserve(dobj);
> if (r) {
> goto out_cleanup;
> }
> + daddr = amdgpu_bo_gpu_offset(dobj);
>
> if (adev->mman.buffer_funcs) {
> time = amdgpu_benchmark_do_move(adev, size, saddr, daddr, n);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index 036b6f7..7d6a36b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -194,6 +194,12 @@ int amdgpu_display_crtc_page_flip_target(struct drm_crtc *crtc,
> goto unreserve;
> }
>
> + r = amdgpu_ttm_alloc_gart(&new_abo->tbo);
> + if (unlikely(r != 0)) {
> + DRM_ERROR("%p bind failed\n", new_abo);
> + goto unpin;
> + }
> +
> r = reservation_object_get_fences_rcu(new_abo->tbo.resv, &work->excl,
> &work->shared_count,
> &work->shared);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> index 462b7a1..cd68a2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
> @@ -173,6 +173,14 @@ static int amdgpufb_create_pinned_object(struct amdgpu_fbdev *rfbdev,
> amdgpu_bo_unreserve(abo);
> goto out_unref;
> }
> +
> + ret = amdgpu_ttm_alloc_gart(&abo->tbo);
> + if (ret) {
> + amdgpu_bo_unreserve(abo);
> + dev_err(adev->dev, "%p bind failed\n", abo);
> + goto out_unref;
> + }
> +
> ret = amdgpu_bo_kmap(abo, NULL);
> amdgpu_bo_unreserve(abo);
> if (ret) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 79cdbf1..7f7c221 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -257,6 +257,13 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
> dev_err(adev->dev, "(%d) kernel bo pin failed\n", r);
> goto error_unreserve;
> }
> +
> + r = amdgpu_ttm_alloc_gart(&(*bo_ptr)->tbo);
> + if (r) {
> + dev_err(adev->dev, "%p bind failed\n", *bo_ptr);
> + goto error_unpin;
> + }
> +
> if (gpu_addr)
> *gpu_addr = amdgpu_bo_gpu_offset(*bo_ptr);
>
> @@ -270,6 +277,8 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
>
> return 0;
>
> +error_unpin:
> + amdgpu_bo_unpin(*bo_ptr);
> error_unreserve:
> amdgpu_bo_unreserve(*bo_ptr);
>
> @@ -903,12 +912,6 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
> goto error;
> }
>
> - r = amdgpu_ttm_alloc_gart(&bo->tbo);
> - if (unlikely(r)) {
> - dev_err(adev->dev, "%p bind failed\n", bo);
> - goto error;
> - }
> -
> bo->pin_count = 1;
>
> domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> index 622affc..d6eeea1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> @@ -102,6 +102,11 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> DRM_ERROR("Failed to pin GTT object %d\n", i);
> goto out_lclean_unres;
> }
> + r = amdgpu_ttm_alloc_gart(>t_obj[i]->tbo);
> + if (r) {
> + DRM_ERROR("%p bind failed\n", gtt_obj[i]);
> + goto out_lclean_unpin;
> + }
> gart_addr = amdgpu_bo_gpu_offset(gtt_obj[i]);
>
> r = amdgpu_bo_kmap(gtt_obj[i], >t_map);
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 31652c1e..d433428 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3110,13 +3110,22 @@ static int dm_plane_helper_prepare_fb(struct drm_plane *plane,
> domain = AMDGPU_GEM_DOMAIN_VRAM;
>
> r = amdgpu_bo_pin(rbo, domain);
> - amdgpu_bo_unreserve(rbo);
> -
> if (unlikely(r != 0)) {
> if (r != -ERESTARTSYS)
> DRM_ERROR("Failed to pin framebuffer with error %d\n", r);
> + amdgpu_bo_unreserve(rbo);
> + return r;
> + }
> +
> + r = amdgpu_ttm_alloc_gart(&rbo->tbo);
> + if (unlikely(r != 0)) {
> + amdgpu_bo_unpin(rbo);
> + amdgpu_bo_unreserve(rbo);
> + DRM_ERROR("%p bind failed\n", rbo);
> return r;
> }
> + amdgpu_bo_unreserve(rbo);
> +
> afb->address = amdgpu_bo_gpu_offset(rbo);
>
> amdgpu_bo_ref(rbo);
