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 <[email protected]>
> > ---
> > 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);
