Re: [PATCH 3/7] drm/amdgpu/vce1: Correct firmware offset mask

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

On Monday, April 20, 2026 2:23:18 PM Central European Summer Time Christian 
König wrote:
> On 4/20/26 14:10, Timur Kristóf wrote:
> > It's 0x0fffffff and not 0x7fffffff.
> > 
> > Fixes: d4a640d4b9f3 ("drm/amdgpu/vce1: Implement VCE1 IP block (v2)")
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> > 
> >  drivers/gpu/drm/amd/amdgpu/vce_v1_0.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v1_0.c
> > b/drivers/gpu/drm/amd/amdgpu/vce_v1_0.c index
> > 2fe931366985a..ce993b57b0e9f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vce_v1_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vce_v1_0.c
> > @@ -313,17 +313,17 @@ static int vce_v1_0_mc_resume(struct amdgpu_device
> > *adev)> 
> >  	offset =  adev->vce.gpu_addr + AMDGPU_VCE_FIRMWARE_OFFSET;
> >  	size = VCE_V1_0_FW_SIZE;
> > 
> > -	WREG32(mmVCE_VCPU_CACHE_OFFSET0, offset & 0x7fffffff);
> > +	WREG32(mmVCE_VCPU_CACHE_OFFSET0, offset & 0x0fffffff);
> > 
> >  	WREG32(mmVCE_VCPU_CACHE_SIZE0, size);
> >  	
> >  	offset += size;
> >  	size = VCE_V1_0_STACK_SIZE;
> > 
> > -	WREG32(mmVCE_VCPU_CACHE_OFFSET1, offset & 0x7fffffff);
> > +	WREG32(mmVCE_VCPU_CACHE_OFFSET1, offset & 0x0fffffff);
> > 
> >  	WREG32(mmVCE_VCPU_CACHE_SIZE1, size);
> >  	
> >  	offset += size;
> >  	size = VCE_V1_0_DATA_SIZE;
> > 
> > -	WREG32(mmVCE_VCPU_CACHE_OFFSET2, offset & 0x7fffffff);
> > +	WREG32(mmVCE_VCPU_CACHE_OFFSET2, offset & 0x0fffffff);
> 
> We actually have that as VCE_VCPU_CACHE_OFFSET*__OFFSET_MASK in the headers,
> would probably be a good idea to use that one instead.
> 
> Additional to that limiting the value actually doesn't make much sense, that
> just hides the problem when we really get an offset which is to large.
> 
> We should probably rather have a WARN_ON(offset &
> ~VCE_VCPU_CACHE_OFFSET2__OFFSET_MASK) directly above the register write.
> 
> Apart from that the patch looks good to me.
> 
> Regards,
> Christian.

Hi Christian,

Thanks, I agree and I'll add the necessary changes.
That being said, with some further testing it seems that VCE fails to 
initialize when the gpu_addr is 128 MiB or more, which makes me think that we 
should use 0x07ffffff instead of 0x0fffffff. Does that sound reasonable to you?

Thanks,
Timur


> 
> >  	WREG32(mmVCE_VCPU_CACHE_SIZE2, size);
> >  	
> >  	WREG32_P(mmVCE_LMI_CTRL2, 0x0, ~0x100);
> > 
> > @@ -531,7 +531,7 @@ static int vce_v1_0_early_init(struct amdgpu_ip_block
> > *ip_block)> 
> >  static int vce_v1_0_ensure_vcpu_bo_32bit_addr(struct amdgpu_device *adev)
> >  {
> >  
> >  	u64 bo_size = amdgpu_bo_size(adev->vce.vcpu_bo);
> > 
> > -	u64 max_vcpu_bo_addr = 0xffffffff - bo_size;
> > +	u64 max_vcpu_bo_addr = 0x0fffffff - bo_size;
> > 
> >  	u64 num_pages = ALIGN(bo_size, AMDGPU_GPU_PAGE_SIZE) /
> >  	AMDGPU_GPU_PAGE_SIZE; u64 pa = amdgpu_gmc_vram_pa(adev,
> >  	adev->vce.vcpu_bo);
> >  	u64 flags = AMDGPU_PTE_READABLE | AMDGPU_PTE_WRITEABLE |
> >  	AMDGPU_PTE_VALID;