[PATCH] drm/amdgpu: add missing error handling for amdgpu_ring_alloc()

Zhou, Bob Bob.Zhou at amd.com
Wed Jun 26 03:15:41 UTC 2024


[AMD Official Use Only - AMD Internal Distribution Only]

Hi Christian

Thanks for your suggestion, I agree with you.
I will submit the next version to drop the return value from amdgpu_ring_alloc().

Regards,
Bob

-----Original Message-----
From: Koenig, Christian <Christian.Koenig at amd.com>
Sent: 2024年6月21日 17:28
To: Zhou, Bob <Bob.Zhou at amd.com>; amd-gfx at lists.freedesktop.org; Huang, Tim <Tim.Huang at amd.com>; Deucher, Alexander <Alexander.Deucher at amd.com>
Subject: Re: [PATCH] drm/amdgpu: add missing error handling for amdgpu_ring_alloc()

Am 21.06.24 um 11:24 schrieb Bob Zhou:
> Fix the unchecked return value warning reported by Coverity, so add
> error handling.

That seems to be completely superfluous. The only case when
amdgpu_ring_alloc() returns an error is when we try to allocate more than the maximum submission size.

And in that case we will get quite a warning already.

I strongly suggest to instead drop the return value from amdgpu_ring_alloc().

Regards,
Christian.

>
> Signed-off-by: Bob Zhou <bob.zhou at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c      | 7 +++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c      | 4 +++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c | 6 ++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c      | 3 ++-
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c        | 8 ++++++--
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c       | 6 +++++-
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c       | 6 +++++-
>   drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c       | 6 +++++-
>   8 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 82452606ae6c..25cab6a8d478 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -1005,7 +1005,8 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg, uint32_t xcc_
>               pr_err("critical bug! too many kiq readers\n");
>               goto failed_unlock;
>       }
> -     amdgpu_ring_alloc(ring, 32);
> +     if (amdgpu_ring_alloc(ring, 32))
> +             goto failed_unlock;
>       amdgpu_ring_emit_rreg(ring, reg, reg_val_offs);
>       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
>       if (r)
> @@ -1071,7 +1072,8 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v, uint3
>       }
>
>       spin_lock_irqsave(&kiq->ring_lock, flags);
> -     amdgpu_ring_alloc(ring, 32);
> +     if (amdgpu_ring_alloc(ring, 32))
> +             goto failed_unlock;
>       amdgpu_ring_emit_wreg(ring, reg, v);
>       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT);
>       if (r)
> @@ -1107,6 +1109,7 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev,
> uint32_t reg, uint32_t v, uint3
>
>   failed_undo:
>       amdgpu_ring_undo(ring);
> +failed_unlock:
>       spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq_write:
>       dev_err(adev->dev, "failed to write reg:%x\n", reg); diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> index 3a7622611916..778941f47c96 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gmc.c
> @@ -768,7 +768,8 @@ void amdgpu_gmc_fw_reg_write_reg_wait(struct amdgpu_device *adev,
>       }
>
>       spin_lock_irqsave(&kiq->ring_lock, flags);
> -     amdgpu_ring_alloc(ring, 32);
> +     if (amdgpu_ring_alloc(ring, 32))
> +             goto failed_unlock;
>       amdgpu_ring_emit_reg_write_reg_wait(ring, reg0, reg1,
>                                           ref, mask);
>       r = amdgpu_fence_emit_polling(ring, &seq, MAX_KIQ_REG_WAIT); @@
> -798,6 +799,7 @@ void amdgpu_gmc_fw_reg_write_reg_wait(struct
> amdgpu_device *adev,
>
>   failed_undo:
>       amdgpu_ring_undo(ring);
> +failed_unlock:
>       spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq:
>       dev_err(adev->dev, "failed to write reg %x wait reg %x\n", reg0,
> reg1); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> index d234b7ccfaaf..01864990a192 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring_mux.c
> @@ -63,12 +63,14 @@ static void amdgpu_ring_mux_copy_pkt_from_sw_ring(struct amdgpu_ring_mux *mux,
>               return;
>       }
>       if (start > end) {
> -             amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - start);
> +             if (amdgpu_ring_alloc(real_ring, (ring->ring_size >> 2) + end - start))
> +                     return;
>               amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[start],
>                                          (ring->ring_size >> 2) - start);
>               amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[0], end);
>       } else {
> -             amdgpu_ring_alloc(real_ring, end - start);
> +             if (amdgpu_ring_alloc(real_ring, end - start))
> +                     return;
>               amdgpu_ring_write_multiple(real_ring, (void *)&ring->ring[start], end - start);
>       }
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c
> index bad232859972..d7d68e61506d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c
> @@ -610,7 +610,8 @@ static int vpe_ring_preempt_ib(struct amdgpu_ring
> *ring)
>
>       /* emit the trailing fence */
>       ring->trail_seq += 1;
> -     amdgpu_ring_alloc(ring, 10);
> +     if (amdgpu_ring_alloc(ring, 10))
> +             return -ENOMEM;
>       vpe_ring_emit_fence(ring, ring->trail_fence_gpu_addr, ring->trail_seq, 0);
>       amdgpu_ring_commit(ring);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 2929c8972ea7..810f7f7e279d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -4093,7 +4093,8 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev)
>               pr_err("critical bug! too many kiq readers\n");
>               goto failed_unlock;
>       }
> -     amdgpu_ring_alloc(ring, 32);
> +     if (amdgpu_ring_alloc(ring, 32))
> +             goto failed_unlock;
>       amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4));
>       amdgpu_ring_write(ring, 9 |     /* src: register*/
>                               (5 << 8) |      /* dst: memory */
> @@ -5639,7 +5640,10 @@ static int gfx_v9_0_ring_preempt_ib(struct amdgpu_ring *ring)
>       amdgpu_ring_set_preempt_cond_exec(ring, false);
>
>       ring->trail_seq += 1;
> -     amdgpu_ring_alloc(ring, 13);
> +     if (amdgpu_ring_alloc(ring, 13)) {
> +             spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +             return -ENOMEM;
> +     }
>       gfx_v9_0_ring_emit_fence(ring, ring->trail_fence_gpu_addr,
>                                ring->trail_seq, AMDGPU_FENCE_FLAG_EXEC |
> AMDGPU_FENCE_FLAG_INT);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> index b7d33d78bce0..07ca9264085b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> @@ -1490,7 +1490,11 @@ static int sdma_v5_0_ring_preempt_ib(struct
> amdgpu_ring *ring)
>
>       /* emit the trailing fence */
>       ring->trail_seq += 1;
> -     amdgpu_ring_alloc(ring, 10);
> +     r = amdgpu_ring_alloc(ring, 10);
> +     if (r) {
> +             DRM_ERROR("ring %d failed to be allocated\n", ring->idx);
> +             return r;
> +     }
>       sdma_v5_0_ring_emit_fence(ring, ring->trail_fence_gpu_addr,
>                                 ring->trail_seq, 0);
>       amdgpu_ring_commit(ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> index cc9e961f0078..d51d5bd7de30 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> @@ -1345,7 +1345,11 @@ static int sdma_v5_2_ring_preempt_ib(struct
> amdgpu_ring *ring)
>
>       /* emit the trailing fence */
>       ring->trail_seq += 1;
> -     amdgpu_ring_alloc(ring, 10);
> +     r = amdgpu_ring_alloc(ring, 10);
> +     if (r) {
> +             DRM_ERROR("ring %d failed to be allocated\n", ring->idx);
> +             return r;
> +     }
>       sdma_v5_2_ring_emit_fence(ring, ring->trail_fence_gpu_addr,
>                                 ring->trail_seq, 0);
>       amdgpu_ring_commit(ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> index c833b6b8373b..e82426459cc7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
> @@ -1371,7 +1371,11 @@ static int sdma_v6_0_ring_preempt_ib(struct
> amdgpu_ring *ring)
>
>       /* emit the trailing fence */
>       ring->trail_seq += 1;
> -     amdgpu_ring_alloc(ring, 10);
> +     r = amdgpu_ring_alloc(ring, 10);
> +     if (r) {
> +             DRM_ERROR("ring %d failed to be allocated\n", ring->idx);
> +             return r;
> +     }
>       sdma_v6_0_ring_emit_fence(ring, ring->trail_fence_gpu_addr,
>                                 ring->trail_seq, 0);
>       amdgpu_ring_commit(ring);



More information about the amd-gfx mailing list