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 <[email protected]> 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/[email protected]?part=1
