Re: [PATCH v2] drm/radeon: retry UVD v1 init on failure after resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/radeon: unclocked UVD MMIO in uvd_v1_0_init() when warm-up clocks fail
- [Medium] drm/radeon: excessive busy-wait and redundant unguarded warm-up in uvd_v1_0_init()
--

commit 6ca2d05b74a007ba26b71ef0af031a5857a4aa3e
Author: ukrainer8 <kolomiiec@gmail.com>

drm/radeon: retry UVD v1 init on failure after resume

This patch adds an unconditional warm-up cycle and a 3-attempt retry loop to
uvd_v1_0_init(), and increases the polling timeout in uvd_v1_0_ring_test() to
2 seconds to recover from UVD initialization failures after system resume.

Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1867
Link: https://bugs.launchpad.net/bugs/1865130

> diff --git a/drivers/gpu/drm/radeon/uvd_v1_0.c b/drivers/gpu/drm/radeon/uvd_v1_0.c
> index 5e6607e16244b..79d3b8cd32388 100644
> --- a/drivers/gpu/drm/radeon/uvd_v1_0.c
> +++ b/drivers/gpu/drm/radeon/uvd_v1_0.c
> @@ -158,24 +158,58 @@ int uvd_v1_0_init(struct radeon_device *rdev)
>  {
>  	struct radeon_ring *ring = &rdev->ring[R600_RING_TYPE_UVD_INDEX];
>  	uint32_t tmp;
> -	int r;
> +	int r, retry, clk_err;
>  
> -	/* raise clocks while booting up the VCPU */
> +	/* warm-up: silently cycle the engine once (like the Windows driver),
> +	 * the first ring instance after resume is dead functionally */
>  	if (rdev->family < CHIP_RV740)
>  		radeon_set_uvd_clocks(rdev, 10000, 10000);
>  	else
>  		radeon_set_uvd_clocks(rdev, 53300, 40000);
> +	uvd_v1_0_start(rdev);
> +	uvd_v1_0_stop(rdev);
> +	mdelay(20);

[Severity: High]
What happens in uvd_v1_0_init() if radeon_set_uvd_clocks() fails during this
warm-up block?

In the retry loop right below, the return value of radeon_set_uvd_clocks() is
saved in clk_err, and both uvd_v1_0_start() and uvd_v1_0_stop() are guarded by
!clk_err because MMIO accesses to an unclocked UVD block can cause a PCI bus
master abort and hard-hang the system.

Here in the warm-up block, the return value of radeon_set_uvd_clocks() is
ignored (for example, if radeon_uvd_send_upll_ctlreq(), sumo_set_uvd_clock(),
or cik_set_uvd_clock() returns -ETIMEDOUT, or radeon_uvd_calc_upll_dividers()
returns -EINVAL), and uvd_v1_0_start() and uvd_v1_0_stop() are called
unconditionally. Won't that immediately perform MMIO reads and writes to the
unclocked UVD registers (such as UVD_CGC_GATE, UVD_MASTINT_EN, UVD_LMI_CTRL2,
UVD_RB_ARB_CTRL, UVD_SOFT_RESET, and UVD_STATUS) before the guarded retry loop
is ever reached?

In addition, is this unconditional warm-up cycle necessary given that the
3-attempt retry loop below already stops UVD, delays 20 ms, and retries from
scratch when a failure occurs? Running the warm-up unconditionally on every
cold boot and healthy resume across all R600 through CIK GPUs adds ~200 ms of
busy-wait mdelay() overhead (programming UPLL, starting and stopping the VCPU,
waiting 20 ms, and then immediately reprogramming UPLL a second time on the
first retry iteration while clocks are already raised), and if the VCPU is
unresponsive, uvd_v1_0_start() busy-waits for ~10.2 seconds of mdelay() in
the warm-up before repeating it 3 more times in the retry loop (~40.8 seconds
total).

> +
> +	/* after resume the first ring test can race with the hardware; retry */
> +	for (retry = 0; retry < 3; retry++) {
> +		/* raise clocks while booting up the VCPU */
> +		if (rdev->family < CHIP_RV740)
> +			clk_err = radeon_set_uvd_clocks(rdev, 10000, 10000);
> +		else
> +			clk_err = radeon_set_uvd_clocks(rdev, 53300, 40000);
> +		r = clk_err;
> +		if (r)
> +			drm_warn(&rdev->ddev, "radeon: raising UVD clocks failed (%d), attempt %d\n",
> +				 r, retry + 1);
> +
> +		if (!r) {
> +			r = uvd_v1_0_start(rdev);
> +			if (r)
> +				drm_warn(&rdev->ddev, "radeon: starting UVD failed (%d), attempt %d\n",
> +					 r, retry + 1);
> +		}
>  
> -	r = uvd_v1_0_start(rdev);
> -	if (r)
> -		goto done;
> +		if (!r) {
> +			ring->ready = true;
> +			r = radeon_ring_test(rdev, R600_RING_TYPE_UVD_INDEX, ring);
> +			if (r) {
> +				ring->ready = false;
> +				drm_warn(&rdev->ddev, "radeon: UVD ring test failed (%d), attempt %d\n",
> +					 r, retry + 1);
> +			}
> +		}
>  
> -	ring->ready = true;
> -	r = radeon_ring_test(rdev, R600_RING_TYPE_UVD_INDEX, ring);
> -	if (r) {
> -		ring->ready = false;
> -		goto done;
> +		if (!r)
> +			break;
> +
> +		/* MMIO is only safe when the block is actually clocked */
> +		if (!clk_err)
> +			uvd_v1_0_stop(rdev);
> +		if (retry < 2)
> +			mdelay(20);
>  	}
> +	if (r)
> +		goto done;
>  
>  	r = radeon_ring_lock(rdev, ring, 10);
>  	if (r) {
> @@ -421,7 +455,8 @@ void uvd_v1_0_stop(struct radeon_device *rdev)
>  int uvd_v1_0_ring_test(struct radeon_device *rdev, struct radeon_ring *ring)
>  {
>  	uint32_t tmp = 0;
> -	unsigned i;
> +	/* after resume the ring buffer controller needs ~1.5-2s to settle */
> +	unsigned i, timeout = 2000000;
>  	int r;
>  
>  	WREG32(UVD_CONTEXT_ID, 0xCAFEDEAD);
> @@ -434,14 +469,14 @@ int uvd_v1_0_ring_test(struct radeon_device *rdev, struct radeon_ring *ring)
>  	radeon_ring_write(ring, PACKET0(UVD_CONTEXT_ID, 0));
>  	radeon_ring_write(ring, 0xDEADBEEF);
>  	radeon_ring_unlock_commit(rdev, ring, false);
> -	for (i = 0; i < rdev->usec_timeout; i++) {
> +	for (i = 0; i < timeout; i++) {
>  		tmp = RREG32(UVD_CONTEXT_ID);
>  		if (tmp == 0xDEADBEEF)
>  			break;
>  		udelay(1);
>  	}

[Severity: Medium]
Could replacing rdev->usec_timeout (100,000 usec) with a hardcoded timeout of
2,000,000 iterations in uvd_v1_0_ring_test() stall a CPU core for several
seconds per failed attempt?

Because uvd_v1_0_ring_test() has no early-exit condition on failure and only
fails after exhausting all loop iterations, any failed ring test attempt will
execute 2,000,000 iterations of synchronous PCIe MMIO RREG32(UVD_CONTEXT_ID)
plus udelay(1) in a non-sleeping busy-wait loop. Across 3 failed attempts in
uvd_v1_0_init(), this will busy-wait for 6 to 15+ seconds during boot, system
resume, or GPU reset.

Also, the comment here states that the ring buffer controller needs ~1.5-2s to
settle after resume, whereas the commit message states that the first ring
instance after resume is dead functionally, not by timeout, and only a full
stop+init cycle revives the engine. If a stop+init cycle is what revives the
engine, is widening the busy-wait poll window from 100 ms to 2000 ms needed?

>  
> -	if (i < rdev->usec_timeout) {
> +	if (i < timeout) {
>  		drm_info(&rdev->ddev, "ring test on %d succeeded in %d usecs\n",
>  			 ring->idx, i);
>  	} else {

့

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