Hello, You are right, and thank you for the precise catch. On my hardware the failure mode is different — clock setup has never failed across 42+ suspend/resume cycles; what fails is the ring test — so the unguarded uvd_v1_0_stop() never fired in practice. But that is luck, not design: if radeon_set_uvd_clocks() ever fails, the retry loop would issue MMIO writes to an unclocked UVD block, with exactly the bus-abort/hang risk you describe. The guard is cheap and obviously correct: v2 keeps the clock-setup result in a separate variable and calls uvd_v1_0_stop() only when clocks were actually raised. I have sent [PATCH v2] with this fix to dri-devel. Thanks again for the review.
Regards Oleg чт, 15 жовт. 2026 р. о 11:11 <[email protected]> пише: > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) > to consider: > - [High] drm/radeon: system hang due to unclocked UVD register access on > clock setup failure > -- > > commit 9840a4a45eacb2033c0f3f3ceb29f75910196145 > Author: ukrainer8 <[email protected]> > Subject: drm/radeon: retry UVD v1 init on failure after resume > > This commit introduces a retry loop for UVD v1 initialization during resume, > as the ring test can sometimes fail due to racing with hardware. On failure, > it stops UVD, waits 20ms, and retries the initialization steps up to 3 times. > > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1867 > Link: https://bugs.launchpad.net/bugs/1865130 > > > --- a/drivers/gpu/drm/radeon/uvd_v1_0.c > > +++ b/drivers/gpu/drm/radeon/uvd_v1_0.c > > @@ -158,24 +158,45 @@ 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; > > + > > + /* 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) > > + r = radeon_set_uvd_clocks(rdev, 10000, 10000); > > + else > > + r = radeon_set_uvd_clocks(rdev, 53300, 40000); > > + if (r) > > + drm_warn(&rdev->ddev, "radeon: raising UVD clocks > > failed (%d), attempt %d\n", > > + r, retry + 1); > > [ ... ] > > > + if (!r) > > + break; > > > > - ring->ready = true; > > - r = radeon_ring_test(rdev, R600_RING_TYPE_UVD_INDEX, ring); > > - if (r) { > > - ring->ready = false; > > - goto done; > > + uvd_v1_0_stop(rdev); > ^^^^^^^^^^^^^^^^^^^ > [Severity: High] > Is it safe to call uvd_v1_0_stop() when radeon_set_uvd_clocks() fails? > > If clock setup fails, r is non-zero, and the loop bypasses the break > condition to unconditionally call uvd_v1_0_stop(). > > This function performs MMIO writes to UVD registers like UVD_RBC_RB_CNTL: > > drivers/gpu/drm/radeon/uvd_v1_0.c:uvd_v1_0_stop() { > /* force RBC into idle state */ > WREG32(UVD_RBC_RB_CNTL, 0x11010101); > ... > } > > Accessing these registers when the UVD IP block is unclocked can cause a PCI > bus master abort and lead to a hard system hang. Should there be a check to > ensure clocks are successfully raised before attempting to stop the hardware? > > > + if (retry < 2) > > + mdelay(20); > > } > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=1
