Re: [PATCH 2/3] drm/amdgpu: allocate gart memory when it's required (v2)

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 06/26/2018 11:53 PM, Felix Kuehling wrote:
> 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@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.

Felix, thanks for your comments.
Check the code again, it seems to get GTT bo for kernel execution from user event or process.
if so, that's really not needed.
Going to remove it in v3.

BTW, kfd2kgd_calls kfd2kgd looks duplicated in amdkfd_gfx_v7/8/9.c
we may initialize it in a common place(at least for common members).
If it has other purpose, please ignore that.

Jerry

>
> 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(&gtt_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], &gtt_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);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx