[PATCH] drm/amdgpu: Refactor amdgpu_move_blit

[Christian König]

Am 05.10.2017 um 23:56 schrieb Kasiviswanathan, Harish:
> -----Original Message-----
> From: Deucher, Alexander
> Sent: Thursday, October 05, 2017 3:10 PM
> To: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>
> Subject: RE: [PATCH] drm/amdgpu: Refactor amdgpu_move_blit
>
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf
>> Of Harish Kasiviswanathan
>> Sent: Thursday, October 05, 2017 2:30 PM
>> To: amd-gfx at lists.freedesktop.org
>> Cc: Kasiviswanathan, Harish
>> Subject: [PATCH] drm/amdgpu: Refactor amdgpu_move_blit
>>
>> Add more generic function amdgpu_copy_ttm_mem_to_mem() that supports
>> arbitrary copy size, offsets and two BOs (source & dest.).
>>
>> This is useful for KFD Cross Memory Attach feature where data needs to
>> be copied from BOs from different processes
>>
>> Change-Id: I848d541a84a1c2d12827d9dcf6d9054d854b4159
>> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 159
>> ++++++++++++++++++++------------
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h |   7 ++
>>   2 files changed, 107 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> index 1086f03..e5415fc 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
>> @@ -289,97 +289,138 @@ static uint64_t amdgpu_mm_node_addr(struct
>> ttm_buffer_object *bo,
>>   	return addr;
>>   }
>>
>> -static int amdgpu_move_blit(struct ttm_buffer_object *bo,
>> -			    bool evict, bool no_wait_gpu,
>> -			    struct ttm_mem_reg *new_mem,
>> -			    struct ttm_mem_reg *old_mem)
>> +/**
>> + * amdgpu_copy_ttm_mem_to_mem - Helper function for copy
>> + *
>> + * @bo, @mem and @offset: All are array of 2 items. The function
>> +copies
>> @size
>> + * bytes from {mem[0] + offset[0]} to {mem[1] + offset[1]}. bo[0] and
>> +bo[1]
>> + * could be same BO for a move and different for a BO to BO copy.
>> + *
>> + * @f: Returns the last fence if multiple jobs are submitted.
>> + */
>> +int amdgpu_copy_ttm_mem_to_mem(struct amdgpu_device *adev,
>> +			       struct ttm_buffer_object *bo[2],
>> +			       struct ttm_mem_reg *mem[2],
>> +			       uint64_t offset[2],
>> +			       uint64_t size,
>> +			       struct reservation_object *resv,
>> +			       struct dma_fence **f)
> Please document which is src and which is dst.  I think from a readability standpoint, it would be better to pass explicit src and dst parameters rather than an array.
>
> Alex
>
> [HK]: I will add the comment. Yes I thought of using src & dst but the number of parameters will increase (already a large number) from 7 to 10.

Yes agree completely, this array thing is really ugly.

You could add a structure describing source and destination to decrease 
the number of parameters.

>> +			if (mem[i]->mem_type == TTM_PL_TT &&
>> +				!amdgpu_gtt_mgr_is_allocated(mem[i])) {

BTW: The coding style here looks incorrect. The second line should be 
indented a bit less.

Regards,
Christian.