2017-04-27 16:17:57

by Nikola Pajkovsky

Subject: [RFC] drm/amd/amdgpu: get rid of else branch

This is super simple elimination of else branch and I should
probably even use unlikely in

if (ring->count_dw < count_dw) {

However, amdgpu_ring_write() has similar if condition, but does not
return after DRM_ERROR and it looks suspicious. On error, we still
adding v to ring and keeping count_dw-- below zero.

if (ring->count_dw <= 0)
DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n");
ring->ring[ring->wptr++] = v;
ring->wptr &= ring->ptr_mask;
ring->count_dw--;

I can obviously be totaly wrong. Hmm?

--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------8<
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index c1b913541739..c6f4f874ea68 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1596,28 +1596,29 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring, void *sr

if (ring->count_dw < count_dw) {
DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n");
- } else {
- occupied = ring->wptr & ring->ptr_mask;
- dst = (void *)&ring->ring[occupied];
- chunk1 = ring->ptr_mask + 1 - occupied;
- chunk1 = (chunk1 >= count_dw) ? count_dw: chunk1;
- chunk2 = count_dw - chunk1;
- chunk1 <<= 2;
- chunk2 <<= 2;
-
- if (chunk1)
- memcpy(dst, src, chunk1);
-
- if (chunk2) {
- src += chunk1;
- dst = (void *)ring->ring;
- memcpy(dst, src, chunk2);
- }
-
- ring->wptr += count_dw;
- ring->wptr &= ring->ptr_mask;
- ring->count_dw -= count_dw;
+ return;
}
+
+ occupied = ring->wptr & ring->ptr_mask;
+ dst = (void *)&ring->ring[occupied];
+ chunk1 = ring->ptr_mask + 1 - occupied;
+ chunk1 = (chunk1 >= count_dw) ? count_dw: chunk1;
+ chunk2 = count_dw - chunk1;
+ chunk1 <<= 2;
+ chunk2 <<= 2;
+
+ if (chunk1)
+ memcpy(dst, src, chunk1);
+
+ if (chunk2) {
+ src += chunk1;
+ dst = (void *)ring->ring;
+ memcpy(dst, src, chunk2);
+ }
+
+ ring->wptr += count_dw;
+ ring->wptr &= ring->ptr_mask;
+ ring->count_dw -= count_dw;
}

static inline struct amdgpu_sdma_instance *
--
2.12.2

2017-04-28 08:52:17

by Christian König

Subject: Re: [RFC] drm/amd/amdgpu: get rid of else branch

Am 27.04.2017 um 18:17 schrieb Nikola Pajkovsky:
> This is super simple elimination of else branch and I should
> probably even use unlikely in
>
> if (ring->count_dw < count_dw) {
>
> However, amdgpu_ring_write() has similar if condition, but does not
> return after DRM_ERROR and it looks suspicious. On error, we still
> adding v to ring and keeping count_dw-- below zero.
>
> if (ring->count_dw <= 0)
> DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n");
> ring->ring[ring->wptr++] = v;
> ring->wptr &= ring->ptr_mask;
> ring->count_dw--;
>
> I can obviously be totaly wrong. Hmm?

That's just choosing the lesser evil.

When we write more DW to the ring than expected it is possible (but not
likely) that we override stuff on the ring buffer which is still
executed by the command processor leading to a possible CP crash.

But when we completely drop the write the commands in the ring buffer
will certainly be invalid and so the CP will certainly crash sooner or
later.

Please add the unlikely() as well and then send out the patch with a
signed-of-by line and I will be happy to push it into our upstream branch.

Regards,
Christian.

>
> --------8<--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------8<
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index c1b913541739..c6f4f874ea68 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1596,28 +1596,29 @@ static inline void amdgpu_ring_write_multiple(struct amdgpu_ring *ring, void *sr
>
> if (ring->count_dw < count_dw) {
> DRM_ERROR("amdgpu: writing more dwords to the ring than expected!\n");
> - } else {
> - occupied = ring->wptr & ring->ptr_mask;
> - dst = (void *)&ring->ring[occupied];
> - chunk1 = ring->ptr_mask + 1 - occupied;
> - chunk1 = (chunk1 >= count_dw) ? count_dw: chunk1;
> - chunk2 = count_dw - chunk1;
> - chunk1 <<= 2;
> - chunk2 <<= 2;
> -
> - if (chunk1)
> - memcpy(dst, src, chunk1);
> -
> - if (chunk2) {
> - src += chunk1;
> - dst = (void *)ring->ring;
> - memcpy(dst, src, chunk2);
> - }
> -
> - ring->wptr += count_dw;
> - ring->wptr &= ring->ptr_mask;
> - ring->count_dw -= count_dw;
> + return;
> }
> +
> + occupied = ring->wptr & ring->ptr_mask;
> + dst = (void *)&ring->ring[occupied];
> + chunk1 = ring->ptr_mask + 1 - occupied;
> + chunk1 = (chunk1 >= count_dw) ? count_dw: chunk1;
> + chunk2 = count_dw - chunk1;
> + chunk1 <<= 2;
> + chunk2 <<= 2;
> +
> + if (chunk1)
> + memcpy(dst, src, chunk1);
> +
> + if (chunk2) {
> + src += chunk1;
> + dst = (void *)ring->ring;
> + memcpy(dst, src, chunk2);
> + }
> +
> + ring->wptr += count_dw;
> + ring->wptr &= ring->ptr_mask;
> + ring->count_dw -= count_dw;
> }
>
> static inline struct amdgpu_sdma_instance *