Re: [PATCH 04/13] drm/amdgpu/vce: Clear VCPU BO before copying firmware to it (v2)

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

On Fri, 2025-11-07 at 10:25 +0100, Christian König wrote:
> On 11/6/25 19:44, 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.
> > 
> > v2:
> > - Only clear BO after creation, not on resume.
> > 
> > Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> 
> For now this patch here is Reviewed-by: Christian König
> <christian.koenig@amd.com> since it addresses a clear problem and
> potentially even need to be back-ported to older kernels.

Thank you, I agree.

> 
> But I think we should clean that up more full after we landed VCE1
> support.

Yes, I'm happy to continue this work after the VCE 1 support lands.

> 
> Assuming that it hold true that VCE1-3 can't continue with sessions
> after suspend resume we should do something like this:
> 
> 1. Remove all amdgpu_bo_kmap(adev->vce.vcpu_bo, &cpu_addr).
> 	As kernel BO the VCE FW BO is pinned and mapped on creation
> time.

This is already done by patch 6 of this series:
"Save/restore and pin VCPU BO for all VCE (v2)"

> 
> 2. Rename amdgpu_vce_resume() into amdgpu_vce_reload_fw() and add the
> memset_io() there like you originally planned.

Also done by patch 6 of this series, except for the rename.

> 
> 3. Also add resetting the VCE FW handles into amdgpu_vce_reload_fw().
> 
>    E.g. something like this:
>   	for (i = 0; i < AMDGPU_MAX_VCE_HANDLES; ++i) {
>   		atomic_set(&adev->vce.handles[i], 0);
>   		adev->vce.filp[i] = NULL;
> 	}
> 
>    This way the kernel will reject submissions when userspace tries
> to use the same FW handles as before the suspend/resume and prevent
> the HW from crashing.
> 
> Does that sounds like a plan to you?

Yes, that sounds like a good plan to me.

> 
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > index b9060bcd4806..e028ad0d3b7a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> > @@ -187,6 +187,8 @@ int amdgpu_vce_sw_init(struct amdgpu_device
> > *adev, unsigned long size)
> >  		return r;
> >  	}
> >  
> > +	memset_io(adev->vce.cpu_addr, 0, size);
> > +
> >  	for (i = 0; i < AMDGPU_MAX_VCE_HANDLES; ++i) {
> >  		atomic_set(&adev->vce.handles[i], 0);
> >  		adev->vce.filp[i] = NULL;