On 02.09.25 09:42, Timur Kristóf wrote:
> On Tue, 2025-09-02 at 08:39 +0200, Christian König wrote:
>> On 01.09.25 12:00, Timur Kristóf wrote:
>>> Technically not necessary, but clear the extra dwords too,
>>> so that the command processors don't read uninitialized memory.
>>
>> That is most likely a really bad idea.
>>
>> The extra DWs are filled with a specific pattern for a HW workaround.
>
> I was unaware so it looked to me like the extra dwords just remain
> uninitialized.
>
> Where in the code does that happen?
See vcn_v4_0_init_ring_metadata() for an example.
> And what is the issue that is being worked around?
IIRC on the VCN4 engine there is a HW bug which triggered when some metadata
for the ring was not in the same X megabyte segment of the ring buffer. So we
just put this small metadata structure directly after the ring.
On some jpeg engine you had to insert commands after the end of the ring to
actually make the ring work reliable. See
jpeg_v1_0_decode_ring_set_patch_ring().
> Also, while we are at it, how was it possible to initialize that
> without causing VM faults? Considering that the allocated BO was not
> sufficiently large to hold the extra dwords. (That is fixed by the
> first patch of this series.)
BOs for VRAM and GTT are always rounded to the next 4KiB. The workaround need
something like 100 bytes or 64 DW, so that always worked.
BTW: I just found that vcn_v4_0_unified_ring_vm_funcs() is using extra_dw as
bytes already :)
Regards,
Christian.
>
>>
>> Clearing them to NOPs makes no sense at all and potentially even
>> breaks the HW workaround.
>>
>> 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.h | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> index 7670f5d82b9e..6a55a85744a9 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
>>> @@ -474,6 +474,11 @@ static inline void
>>> amdgpu_ring_clear_ring(struct amdgpu_ring *ring)
>>> while (i <= ring->buf_mask)
>>> ring->ring[i++] = ring->funcs->nop;
>>>
>>> + /* Technically not necessary, but clear the extra dwords
>>> too,
>>> + * so that the command processors don't read uninitialized
>>> memory.
>>> + */
>>> + for (i = 0; i < ring->funcs->extra_dw; i++)
>>> + ring->ring[ring->ring_size / 4 + i] = ring->funcs-
>>>> nop;
>>> }
>>>
>>> static inline void amdgpu_ring_write(struct amdgpu_ring *ring,
>>> uint32_t v)
