Re: [PATCH] drm/amdgpu: simplify padding calculations

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2019-10-26 08:05, Koenig, Christian wrote:
> Am 25.10.19 um 23:51 schrieb Tuikov, Luben:
>> On 2019-10-25 2:54 a.m., Koenig, Christian wrote:
>>> Am 25.10.19 um 01:44 schrieb Tuikov, Luben:
>>>> Simplify padding calculations.
>>>>
>>>> Signed-off-by: Luben Tuikov <luben.tuikov@xxxxxxx>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/cik_sdma.c  |  4 ++--
>>>>    drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c |  4 ++--
>>>>    drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c |  4 ++--
>>>>    drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c |  4 ++--
>>>>    drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 14 +++++++++-----
>>>>    5 files changed, 17 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
>>>> index c45304f1047c..1ea9e18d6f08 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik_sdma.c
>>>> @@ -228,7 +228,7 @@ static void cik_sdma_ring_emit_ib(struct amdgpu_ring *ring,
>>>>    	u32 extra_bits = vmid & 0xf;
>>>>    
>>>>    	/* IB packet must end on a 8 DW boundary */
>>>> -	cik_sdma_ring_insert_nop(ring, (12 - (lower_32_bits(ring->wptr) & 7)) % 8);
>>>> +	cik_sdma_ring_insert_nop(ring, (4-lower_32_bits(ring->wptr)) & 7);
>>>>    
>>>>    	amdgpu_ring_write(ring, SDMA_PACKET(SDMA_OPCODE_INDIRECT_BUFFER, 0, extra_bits));
>>>>    	amdgpu_ring_write(ring, ib->gpu_addr & 0xffffffe0); /* base must be 32 byte aligned */
>>>> @@ -811,7 +811,7 @@ static void cik_sdma_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib)
>>>>    	u32 pad_count;
>>>>    	int i;
>>>>    
>>>> -	pad_count = (8 - (ib->length_dw & 0x7)) % 8;
>>>> +	pad_count = (-ib->length_dw) & 7;
>>>>    	for (i = 0; i < pad_count; i++)
>>>>    		if (sdma && sdma->burst_nop && (i == 0))
>>>>    			ib->ptr[ib->length_dw++] =
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
>>>> index a10175838013..d340f179401a 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v2_4.c
>>>> @@ -255,7 +255,7 @@ static void sdma_v2_4_ring_emit_ib(struct amdgpu_ring *ring,
>>>>    	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>>    
>>>>    	/* IB packet must end on a 8 DW boundary */
>>>> -	sdma_v2_4_ring_insert_nop(ring, (10 - (lower_32_bits(ring->wptr) & 7)) % 8);
>>>> +	sdma_v2_4_ring_insert_nop(ring, (2-lower_32_bits(ring->wptr)) & 7);
>>>>    
>>>>    	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_INDIRECT) |
>>>>    			  SDMA_PKT_INDIRECT_HEADER_VMID(vmid & 0xf));
>>>> @@ -750,7 +750,7 @@ static void sdma_v2_4_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib
>>>>    	u32 pad_count;
>>>>    	int i;
>>>>    
>>>> -	pad_count = (8 - (ib->length_dw & 0x7)) % 8;
>>>> +	pad_count = (-ib->length_dw) & 7;
>>>>    	for (i = 0; i < pad_count; i++)
>>>>    		if (sdma && sdma->burst_nop && (i == 0))
>>>>    			ib->ptr[ib->length_dw++] =
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>>> index 5f4e2c616241..5c3c310188b6 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v3_0.c
>>>> @@ -429,7 +429,7 @@ static void sdma_v3_0_ring_emit_ib(struct amdgpu_ring *ring,
>>>>    	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>>    
>>>>    	/* IB packet must end on a 8 DW boundary */
>>>> -	sdma_v3_0_ring_insert_nop(ring, (10 - (lower_32_bits(ring->wptr) & 7)) % 8);
>>>> +	sdma_v3_0_ring_insert_nop(ring, (2-lower_32_bits(ring->wptr)) & 7);
>>>>    
>>>>    	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_INDIRECT) |
>>>>    			  SDMA_PKT_INDIRECT_HEADER_VMID(vmid & 0xf));
>>>> @@ -1021,7 +1021,7 @@ static void sdma_v3_0_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib
>>>>    	u32 pad_count;
>>>>    	int i;
>>>>    
>>>> -	pad_count = (8 - (ib->length_dw & 0x7)) % 8;
>>>> +	pad_count = (-ib->length_dw) & 7;
>>>>    	for (i = 0; i < pad_count; i++)
>>>>    		if (sdma && sdma->burst_nop && (i == 0))
>>>>    			ib->ptr[ib->length_dw++] =
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>>> index 45bd538ba97e..7c71c88e38a4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>>> @@ -698,7 +698,7 @@ static void sdma_v4_0_ring_emit_ib(struct amdgpu_ring *ring,
>>>>    	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>>    
>>>>    	/* IB packet must end on a 8 DW boundary */
>>>> -	sdma_v4_0_ring_insert_nop(ring, (10 - (lower_32_bits(ring->wptr) & 7)) % 8);
>>>> +	sdma_v4_0_ring_insert_nop(ring, (2-lower_32_bits(ring->wptr)) & 7);
>>>>    
>>>>    	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_INDIRECT) |
>>>>    			  SDMA_PKT_INDIRECT_HEADER_VMID(vmid & 0xf));
>>>> @@ -1580,7 +1580,7 @@ static void sdma_v4_0_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib
>>>>    	u32 pad_count;
>>>>    	int i;
>>>>    
>>>> -	pad_count = (8 - (ib->length_dw & 0x7)) % 8;
>>>> +	pad_count = (-ib->length_dw) & 7;
>>>>    	for (i = 0; i < pad_count; i++)
>>>>    		if (sdma && sdma->burst_nop && (i == 0))
>>>>    			ib->ptr[ib->length_dw++] =
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>>>> index 0c41b4fdc58b..67ede9e4df01 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>>>> @@ -382,8 +382,12 @@ static void sdma_v5_0_ring_emit_ib(struct amdgpu_ring *ring,
>>>>    	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>>    	uint64_t csa_mc_addr = amdgpu_sdma_get_csa_mc_addr(ring, vmid);
>>>>    
>>>> -	/* IB packet must end on a 8 DW boundary */
>>>> -	sdma_v5_0_ring_insert_nop(ring, (10 - (lower_32_bits(ring->wptr) & 7)) % 8);
>>>> +	/* An IB packet must end on a 8 DW boundary--the next dword must
>>>> +	 * be on a 8-dword boundary. Our IB below is 6 dwords long,
>>> That should probably read "our IB command" or "our IB packet".
>> Done.
>>
>>>> +	 * thus add x number of NOPs, such that
>>>> +	 * wptr + 6 + x = 8k, k >= 0.
>>> Maybe write (wrptr + 6 + x) % 8 = 0 to make that more obvious.
>> That's less obvious. In my mind I always translate it to
>> a congruence expression. It's what I used to derive the algebraic
>> simplifications of this patch.
>>
>> I'll add the highly cryptic "(wrptr + 6 + x) % 8 = 0" to make you happy,
>> below the algebraic expression already there.
> 
> The problem I see here is that people who are used to GPUs will think 
> that k is a float and not immediately get what's meant here.
> 
>>>> +	 */
>>>> +	sdma_v5_0_ring_insert_nop(ring, (2-lower_32_bits(ring->wptr)) & 7);
>>> Spaces between operators please.
>> Yeah, I wanted to put an emphasis on the AND-op, but I'll add them.
> 
> What would be really nice to have is to keep the 10 as number of DW in 
> the IB packet around.

Why? Why 10?

As far as I can see, THE IB packet is 6 dwords, we want to pad with some
number of dwords, in order to make sure that the wptr + 6, ends
up on a boundary multiple of 8. I.e. we want to solve exactly this,
for x:

wptr + 6 + x = 8k, k >= 0.

x = -(wtpr + 6) = -6 - wptr = 2 - wptr (mod 8)

Also note that, 10 = 2 = -6 (mod 8).

I believe the patch is good as is.

Did you see v2 of the patch?

Regards,
Luben


> 
> Maybe add an "static inline amdgpu_ring_unalign(ring, 10, 8)" helper or 
> something like that, the insert_nop is a ring callback anyway IIRC.
> 
> Regards,
> Christian.
> 
>>
>> Regards,
>> Luben
>>
>>
>>> Apart form that looks good to me,
>>> Christian.
>>>
>>>>    
>>>>    	amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_INDIRECT) |
>>>>    			  SDMA_PKT_INDIRECT_HEADER_VMID(vmid & 0xf));
>>>> @@ -1086,10 +1090,10 @@ static void sdma_v5_0_vm_set_pte_pde(struct amdgpu_ib *ib,
>>>>    }
>>>>    
>>>>    /**
>>>> - * sdma_v5_0_ring_pad_ib - pad the IB to the required number of dw
>>>> - *
>>>> + * sdma_v5_0_ring_pad_ib - pad the IB
>>>>     * @ib: indirect buffer to fill with padding
>>>>     *
>>>> + * Pad the IB with NOPs to a boundary multiple of 8.
>>>>     */
>>>>    static void sdma_v5_0_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib)
>>>>    {
>>>> @@ -1097,7 +1101,7 @@ static void sdma_v5_0_ring_pad_ib(struct amdgpu_ring *ring, struct amdgpu_ib *ib
>>>>    	u32 pad_count;
>>>>    	int i;
>>>>    
>>>> -	pad_count = (8 - (ib->length_dw & 0x7)) % 8;
>>>> +	pad_count = (-ib->length_dw) & 0x7;
>>>>    	for (i = 0; i < pad_count; i++)
>>>>    		if (sdma && sdma->burst_nop && (i == 0))
>>>>    			ib->ptr[ib->length_dw++] =
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux