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?
And what is the issue that is being worked around?
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.)
>
> 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)
