Re: [PATCH 2/2] drm/radeon/vce1: enable the VCE trap interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/radeon: vce_v1_0_start() writes VCE1 VCE_SYS_INT_EN offset on VCE2
--

commit 081ec1fcedbbac29c36bb9a81a87f74e67af09fd
Author: Matheus Tagliari <mptagliari@gmail.com>

drm/radeon/vce1: enable the VCE trap interrupt

This commit defines VCE_SYS_INT_EN and VCE_SYS_INT_TRAP_INTERRUPT_EN in
sid.h and enables the trap interrupt at the end of vce_v1_0_start() so VCE
fences are processed by the interrupt handler.

> diff --git a/drivers/gpu/drm/radeon/sid.h b/drivers/gpu/drm/radeon/sid.h
> index f9267b026f8de..ba322de844b3b 100644
> --- a/drivers/gpu/drm/radeon/sid.h
> +++ b/drivers/gpu/drm/radeon/sid.h
> @@ -1878,6 +1878,8 @@
>  #define	DMA_PACKET_NOP					  0xf
>  
>  #define VCE_STATUS					0x20004
> +#define VCE_SYS_INT_EN					0x20d00
> +#define		VCE_SYS_INT_TRAP_INTERRUPT_EN		(1 << 3)
>  #define VCE_VCPU_CNTL					0x20014
>  #define		VCE_CLK_EN				(1 << 0)
>  #define VCE_VCPU_CACHE_OFFSET0				0x20024
> diff --git a/drivers/gpu/drm/radeon/vce_v1_0.c b/drivers/gpu/drm/radeon/vce_v1_0.c
> index bdfbcf14b8640..0866e7fb0b0ef 100644
> --- a/drivers/gpu/drm/radeon/vce_v1_0.c
> +++ b/drivers/gpu/drm/radeon/vce_v1_0.c
> @@ -350,6 +350,10 @@ int vce_v1_0_start(struct radeon_device *rdev)
>  		return r;
>  	}
>  
> +	/* enable the trap interrupt so that VCE fences are processed */
> +	WREG32_P(VCE_SYS_INT_EN, VCE_SYS_INT_TRAP_INTERRUPT_EN,
> +		 ~VCE_SYS_INT_TRAP_INTERRUPT_EN);

[Severity: High]
Does writing VCE_SYS_INT_EN unconditionally in vce_v1_0_start() access the
wrong MMIO register offset on CIK (VCE 2.0) hardware?

In drm/radeon, VCE 2.0 reuses vce_v1_0_init() and vce_v1_0_start() during
startup, resume, and dynamic powergating transitions:

drivers/gpu/drm/radeon/cik.c:cik_vce_resume() {
	...
	r = vce_v1_0_init(rdev);
}

drivers/gpu/drm/radeon/kv_dpm.c:kv_dpm_powergate_vce() {
	...
		if (pi->caps_vce_pg) {
			kv_notify_message_to_smu(rdev, PPSMC_MSG_VCEPowerON);
			vce_v2_0_resume(rdev);
			vce_v1_0_start(rdev);
		}
}

Because drivers/gpu/drm/radeon/vce_v1_0.c includes sid.h, VCE_SYS_INT_EN
evaluates to the VCE 1.0 byte offset 0x20d00 here. On VCE 2.0, however,
VCE_SYS_INT_EN is at byte offset 0x21300 in drivers/gpu/drm/radeon/cikd.h
(where offset 0x20d00 falls within the VCE 2.0 UENC register range), and
VCE 2.0 already enables VCE_SYS_INT_TRAP_INTERRUPT_EN at 0x21300 inside
vce_v2_0_resume() in drivers/gpu/drm/radeon/vce_v2_0.c:

drivers/gpu/drm/radeon/vce_v2_0.c:vce_v2_0_resume() {
	...
	WREG32_P(VCE_LMI_CTRL2, 0x0, ~0x100);

	WREG32_P(VCE_SYS_INT_EN, VCE_SYS_INT_TRAP_INTERRUPT_EN,
		 ~VCE_SYS_INT_TRAP_INTERRUPT_EN);

	vce_v2_0_init_cg(rdev);
}

Should this register write be placed in vce_v1_0_resume() (matching
vce_v2_0_resume()), or guarded so it is only executed on VCE 1.0 hardware?

> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021114122.94008-1-mptagliari@gmail.com?part=2