On 02.09.25 10:26, Timur Kristóf wrote:
> On Tue, 2025-09-02 at 08:41 +0200, Christian König wrote:
>> On 01.09.25 12:00, Timur Kristóf wrote:
>>> The amdgpu_bo_create_kernel function takes a byte count,
>>> so we need to multiply the extra dword count by four.
>>> (The ring_size is already in bytes so that one is correct here.)
>>
>> Good catch, it just doesn't make a difference in practice since
>> everything is rounded up to 4k anyway.
>
> Yes. It looks like extra_dw is only used by jpeg_v1_0 and vcn_v4_0 and
> fortunately both of these are small enough that it doesn't cause a
> difference in practice.
>
> I'd still like to fix it though to avoid potential issues in the
> future.
Yeah, clearly agree.
>
>>
>> But I'm really wondering if we shouldn't replace the extra_dw with
>> extra_bytes instead.
>
> It's up to you. I'm happy to change it to extra_bytes if you think
> that's better.
The VCN code already specifies the extra_dw in bytes.
So I think it is better to rename the field and add an "* 4" to the calculation
in the jpeg code.
> I would prefer to keep extra_dw for two reasons:
>
> - Avoid unnecessary churn
> - Most of the code related to rings are in dwords, so IMO it's better
> to have the extra space in dwords too for consistency.
That more or less just a leftover from the early radeon days.
The GFX/SDMA/UVD engines worked with DW packets, but starting with VCE and then
later VCN, PSP and MES the FW teams moved over to using C structs which are
obviously counted in bytes.
>
>>
>> It should only be used by some multimedia engines anyway.
>
> Right. vcn_v4_0 has this:
> .extra_dw = sizeof(struct amdgpu_vcn_rb_metadata)
> Which would also need to be fixed because it's not really in dwords.
Yeah, exactly that.
Regards,
Christian.
>
>
>
>>>
>>> Fixes: c8c1a1d2ef04 ("drm/amdgpu: define and add extra dword for
>>> jpeg ring")
>>> Signed-off-by: Timur Kristóf <[email protected]>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 11 ++++++-----
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> index 6379bb25bf5c..13f0f0209cbe 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
>>> @@ -364,11 +364,12 @@ int amdgpu_ring_init(struct amdgpu_device
>>> *adev, struct amdgpu_ring *ring,
>>>
>>> /* Allocate ring buffer */
>>> if (ring->ring_obj == NULL) {
>>> - r = amdgpu_bo_create_kernel(adev, ring->ring_size
>>> + ring->funcs->extra_dw, PAGE_SIZE,
>>> - AMDGPU_GEM_DOMAIN_GTT,
>>> - &ring->ring_obj,
>>> - &ring->gpu_addr,
>>> - (void **)&ring->ring);
>>> + r = amdgpu_bo_create_kernel(adev, ring->ring_size
>>> + ring->funcs->extra_dw * 4,
>>> + PAGE_SIZE,
>>> + AMDGPU_GEM_DOMAIN_
>>> GTT,
>>> + &ring->ring_obj,
>>> + &ring->gpu_addr,
>>> + (void **)&ring-
>>>> ring);
>>> if (r) {
>>> dev_err(adev->dev, "(%d) ring create
>>> failed\n", r);
>>> kvfree(ring->ring_backup);
