[PATCH] drm/amdgpu: fix some uninitialized variables
Christian König
christian.koenig at amd.com
Wed Apr 24 08:11:36 UTC 2024
Am 24.04.24 um 04:04 schrieb Zhang, Jesse(Jie):
> [AMD Official Use Only - General]
>
> Hi Alex,
>
> -----Original Message-----
> From: Alex Deucher <alexdeucher at gmail.com>
> Sent: Wednesday, April 24, 2024 9:46 AM
> To: Zhang, Jesse(Jie) <Jesse.Zhang at amd.com>
> Cc: amd-gfx at lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>; Huang, Tim <Tim.Huang at amd.com>
> Subject: Re: [PATCH] drm/amdgpu: fix some uninitialized variables
>
> On Tue, Apr 23, 2024 at 9:27 PM Jesse Zhang <jesse.zhang at amd.com> wrote:
>> Fix some variables not initialized before use.
>> Scan them out using Synopsys tools.
>>
>> Signed-off-by: Jesse Zhang <Jesse.Zhang at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c | 5 +++++
>> drivers/gpu/drm/amd/amdgpu/atom.c | 1 +
>> drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c | 3 ++-
>> drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 3 ++-
>> drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c | 3 ++-
> Please split out the SDMA changes into a separate patch.
>
> More comments below on the other hunks.
>
>> 6 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> index 59acf424a078..60d97cd14855 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>> @@ -742,7 +742,7 @@ int amdgpu_vce_ring_parse_cs(struct amdgpu_cs_parser *p,
>> uint32_t destroyed = 0;
>> uint32_t created = 0;
>> uint32_t allocated = 0;
>> - uint32_t tmp, handle = 0;
>> + uint32_t tmp = 0, handle = 0;
> Can you elaborate on what the issue is here? Presumably it's warning about size being passed as a parameter in the function?
> [Zhang, Jesse(Jie)] Using uninitialized value *size when calling amdgpu_vce_cs_reloc for case 0x03000001. Because uint32_t *size = &tmp;
> I'm not sure if other commands initialize the size before running case 0x03000001.
Ah! Yeah, that handling is actually correct. The size might be
uninitialized in this function when the command stream isn't valid.
We could instead set size to NULL and check that everywhere, but that
would probably be overkill.
Well we could silence the warning by setting tmp to zero, but that
actually doesn't improve anything.
Regards,
Christian.
>
>> uint32_t *size = &tmp;
>> unsigned int idx;
>> int i, r = 0;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> index 677eb141554e..13125ddd5e86 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c
>> @@ -410,6 +410,11 @@ static void amdgpu_vcn_idle_work_handler(struct work_struct *work)
>> else
>> new_state.fw_based =
>> VCN_DPG_STATE__UNPAUSE;
>>
>> + if (amdgpu_fence_count_emitted(adev->jpeg.inst->ring_dec))
>> + new_state.jpeg = VCN_DPG_STATE__PAUSE;
>> + else
>> + new_state.jpeg =
>> + VCN_DPG_STATE__UNPAUSE;
>> +
>> adev->vcn.pause_dpg_mode(adev, j, &new_state);
>> }
>>
> This should be a separate patch as well.
> Thanks for your reminder, Alex, I will.
>
>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
>> b/drivers/gpu/drm/amd/amdgpu/atom.c
>> index 72362df352f6..d552e013354c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/atom.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/atom.c
>> @@ -1243,6 +1243,7 @@ static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index,
>> ectx.ps_size = params_size;
>> ectx.abort = false;
>> ectx.last_jump = 0;
>> + ectx.last_jump_jiffies = 0;
>> if (ws) {
>> ectx.ws = kcalloc(4, ws, GFP_KERNEL);
>> ectx.ws_size = ws;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>> index 45a2d0a5a2d7..b7d33d78bce0 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
>> @@ -999,7 +999,8 @@ static int sdma_v5_0_ring_test_ring(struct amdgpu_ring *ring)
>> r = amdgpu_ring_alloc(ring, 20);
>> if (r) {
>> DRM_ERROR("amdgpu: dma failed to lock ring %d (%d).\n", ring->idx, r);
>> - amdgpu_device_wb_free(adev, index);
>> + if (!ring->is_mes_queue)
>> + amdgpu_device_wb_free(adev, index);
>> return r;
>> }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> index 43e64b2da575..cc9e961f0078 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
>> @@ -839,7 +839,8 @@ static int sdma_v5_2_ring_test_ring(struct amdgpu_ring *ring)
>> r = amdgpu_ring_alloc(ring, 20);
>> if (r) {
>> DRM_ERROR("amdgpu: dma failed to lock ring %d (%d).\n", ring->idx, r);
>> - amdgpu_device_wb_free(adev, index);
>> + if (!ring->is_mes_queue)
>> + amdgpu_device_wb_free(adev, index);
>> return r;
>> }
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>> index 1f4877195213..c833b6b8373b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>> @@ -861,7 +861,8 @@ static int sdma_v6_0_ring_test_ring(struct amdgpu_ring *ring)
>> r = amdgpu_ring_alloc(ring, 5);
>> if (r) {
>> DRM_ERROR("amdgpu: dma failed to lock ring %d (%d).\n", ring->idx, r);
>> - amdgpu_device_wb_free(adev, index);
>> + if (!ring->is_mes_queue)
>> + amdgpu_device_wb_free(adev, index);
>> return r;
>> }
>>
>> --
>> 2.25.1
>>
More information about the amd-gfx
mailing list