On 10/21/26 23:47, О К wrote: > Hello everybody, > >> That HW is ancient! > > It is — and still in daily service here, which is exactly why I care. :) > >> The question is really if that works reliable or if that could break >> anything? > > Fair frame, so let me answer it directly. > > 1) The success path is bit-identical to stock. When the first attempt > works (cold boot, healthy resumes), the added code executes zero extra > instructions — the loop breaks immediately. > > 2) The failure path cannot leave the system worse than stock. Oh well, that is not even remotely true :)
The problem is that those old HW generations work with a rather rudimentary UVD FW signing. When that FW signing HW block thinks you are tampering with the UVD FW the device usually spontaneously falls of the bus. In other words you can't read/write the PCI config space of the device any more, reads to MMIO BARs only return 0xffffffff and a few seconds later the system becomes completely unresponsive and you can only power cycle it to get out of that. > Today a > failed ring test already ends in -EINVAL with UVD dead for the session. > The patch only adds up to two more bounded attempts (~250 ms each: > start, ring test, stop, 20 ms settle) and then falls through to the > same failure state. The display rings (0/3) and the rest of the resume > path never touch this code — and years of stock behavior prove the > machine resumes fine with UVD dead. Yeah that sounds unproblematic, the bit extra delay is not really a problem. > 3) The one real hazard — MMIO to an unclocked UVD block in the retry's > stop path — was caught by the Sashiko review and is fixed in the > [PATCH v3] already on the list: stop is now called only when clock > setup succeeded. v3 also drops the warm-up cycle and the widened poll > window I had in v2 — both reviews were right that they didn't belong; > our measurements agree (neither prevented first-attempt failures). Yeah that is definitely a good idea as well. IIRC there is also a bit in the LMI register which is called *_VPU_STALL_* or something like that. Setting that before a retry and waiting a few miliseconds is usually a good idea as well. > Measured on the target machine (HD 5650M, kernel 7.0.0): stock survives > resume with working UVD in ~50% of 42 measured cycles; patched has been > 12/12 on the test stand plus every daily resume since. > > If you prefer a tighter bound, I'm fine with 2 attempts instead of 3. That is not really the problem, question is rather have you seen any hard crashes with this? Regards, Christian. > >> when resume doesn't work at all any more then that would be really bad > > Agreed — and that scenario is exactly what the v3 guard is about. > > Thanks, > Oleg > > ср, 21 жовт. 2026 р. о 17:28 Christian König <[email protected]> пише: >> >> That HW is ancient! >> >> The question is really if that works reliable or if that could break >> anything? >> >> I mean when UVD doesn't work after resume is ugly, but when resume doesn't >> work at all any more then that would be really bad. >> >> Regards, >> Christian. >> >> On 10/21/26 15:29, Alex Deucher wrote: >>> + Leo, Christian >>> >>> On Tue, Oct 20, 2026 at 3:46 AM ukrainer8 <[email protected]> wrote: >>>> >>>> On Evergreen/Redwood (and likely other UVD v1/v2 hardware), resume from >>>> suspend intermittently fails the UVD ring test: >>>> >>>> radeon 0000:01:00.0: [drm] *ERROR* radeon: ring 5 test failed >>>> (0xCAFEDEAD) >>>> radeon 0000:01:00.0: failed initializing UVD (-22). >>>> >>>> Cold boot always succeeds; resume succeeds only in about half of the cycles >>>> (42 measured suspend/resume cycles on a Radeon HD 5650M: OK counts per >>>> 6-cycle run were 2, 3, 5, 6, 1, 5, 3). Once the ring test fails, UVD stays >>>> dead for the rest of the session and hardware video decode silently falls >>>> back to CPU. >>>> >>>> The resume path runs the same full asic startup as cold boot >>>> (evergreen_resume() -> evergreen_startup()), so the difference is the >>>> hardware state at entry, not a skipped call. Field data shows the first >>>> ring instance after resume is dead functionally, not by timeout: the ring >>>> test fails fast and deterministically, and only a full stop+init cycle >>>> revives the engine. >>>> >>>> Fix this the way the Windows driver effectively does it: silently cycle >>>> the engine once (clocks up, start, stop, 20 ms settle) before testing, >>>> then run the init sequence in a retry loop - on failure of clock setup, >>>> VCPU start or the ring test, fully stop UVD, let it settle for 20 ms and >>>> try again (up to 3 attempts). Each retry logs the failing step and the >>>> attempt number, which also makes future reports of this failure mode >>>> self-diagnosing. The ring-test polling window is widened from 100 ms to >>>> 2000 ms as spare capacity; the early-exit poll keeps healthy hardware on >>>> the fast path. >>>> >>>> On the same machine the patched driver went 6/6 OK across repeated test >>>> runs, with the warm-up absorbing the dead first instance and zero error >>>> lines in dmesg. >>>> >>>> v2: guard uvd_v1_0_stop() behind successful clock setup, per Sashiko AI >>>> review - MMIO writes to an unclocked UVD block can cause a PCI bus master >>>> abort and hard-hang the system. >>>> >>>> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1867 >>>> Link: https://bugs.launchpad.net/bugs/1865130 >>>> Tested-by: ukrainer8 <[email protected]> # Radeon HD 5650M, kernel >>>> 7.0.0 >>>> Signed-off-by: ukrainer8 <[email protected]> >>>> --- >>>> v1 -> v2: keep the clock-setup result in a separate variable and only >>>> call uvd_v1_0_stop() when clocks were actually raised (Sashiko AI review). >>>> Also folded in the silent warm-up cycle and the wider ring-test window >>>> validated in field testing since v1. >>>> drivers/gpu/drm/radeon/uvd_v1_0.c | 57 >>>> ++++++++++++++++++++++++++------------- >>>> --- a/drivers/gpu/drm/radeon/uvd_v1_0.c >>>> +++ b/drivers/gpu/drm/radeon/uvd_v1_0.c >>>> @@ -158,24 +158,58 @@ >>>> { >>>> 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); >>>> >>>> - r = uvd_v1_0_start(rdev); >>>> - if (r) >>>> - goto done; >>>> + /* 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); >>>> + } >>>> >>>> - ring->ready = true; >>>> - r = radeon_ring_test(rdev, R600_RING_TYPE_UVD_INDEX, ring); >>>> - if (r) { >>>> - ring->ready = false; >>>> - 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); >>>> + } >>>> + } >>>> + >>>> + 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 @@ >>>> 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 @@ >>>> 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); >>>> } >>>> >>>> - 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 { >>
