Re: [PATCH 05/14] drm/amdgpu/vce: Clear VCPU BO before copying firmware to it

["Timur Kristóf" <timur.kristof@gmail.com>]

On Wed, 2025-10-29 at 11:19 +0100, Christian König wrote:
> On 10/28/25 23:06, Timur Kristóf wrote:
> > The VCPU BO doesn't only contain the VCE firmware but also other
> > ranges that the VCE uses for its stack and data. Let's initialize
> > this to zero to avoid having garbage in the VCPU BO.
> 
> Absolutely clear NAK.
> 
> This is intentionally not initialized on resume to avoid breaking
> encode sessions which existed before suspend.

How can there be encode sessions from before suspend?
I think that there can't be.

As far as I see, before suspend we wait for the VCE to go idle, meaning
that we wait for all pending work to finish.
amdgpu_vce_suspend has a comment which says:
suspending running encoding sessions isn't supported

> Why exactly is that an issue?

We need to clear at least some of the BO for the VCE1 firmware
validation mechanism. This is done in a memset in vce_v1_0_load_fw in
the old radeon driver.

Also I think it's a good idea to avoid having garbage in the VCPU BO.

> The VCE FW BO should be cleared to zero after initial allocation?

To clarify, are you suggesting that I move the memset to after the BO
creation, and then never clear it again? Or are you saying that
amdgpu_bo_create_reserved already clears the BO?

> 
> Regards,
> Christian.
> 
> > 
> > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > index b9060bcd4806..eaa06dbef5c4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > @@ -310,6 +310,7 @@ int amdgpu_vce_resume(struct amdgpu_device
> > *adev)
> >  	offset = le32_to_cpu(hdr->ucode_array_offset_bytes);
> >  
> >  	if (drm_dev_enter(adev_to_drm(adev), &idx)) {
> > +		memset32(cpu_addr, 0, amdgpu_bo_size(adev-
> > >vce.vcpu_bo) / 4);
> >  		memcpy_toio(cpu_addr, adev->vce.fw->data + offset,
> >  			    adev->vce.fw->size - offset);
> >  		drm_dev_exit(idx);