Le lundi 27 juillet 2026 à 17:11 +0200, Francesco Saverio Pavone a écrit : > Self-review after running this on a Rock 5B+. The reorder closes the > clk leak, but it opens a window I missed when I sent it. > Moving the PM teardown ahead of rkvdec_v4l2_cleanup() is necessary, > for the drvdata reason in the commit message. The side effect is that > video_unregister_device() is now the last thing remove() does, so > /dev/videoN stays open-able while the empty IOMMU domain is already > freed. > A job queued in that window reaches rkvdec_iommu_restore() and touches > the freed domain. Before the reorder, cleanup ran first and the window > did not exist. > Splitting the helper keeps both properties: unregister the media and > video devices first, which stops new jobs, then tear down PM, then > release the m2m and V4L2 device. Only that last step clears drvdata, > so rkvdec_runtime_suspend() still finds its state. > > cancel_delayed_work_sync(&rkvdec->watchdog_work); > > rkvdec_v4l2_unregister(rkvdec); /* media + video unregister */ > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > if (rkvdec->empty_domain) > iommu_domain_free(rkvdec->empty_domain); > > pm_runtime_disable(&pdev->dev); > rkvdec_v4l2_release(rkvdec); /* media cleanup, m2m, v4l2 */ > > Running here on 7.2-rc5: unbind then rebind returns the rkvdec clock > enable counts to their pre-unbind values and /dev/video0 comes back. > I can send this as a v5 carrying both changes, or as a follow-up patch > on top of v4, whichever you prefer. Looking forward you v5. Nicolas > Thanks, > Francesco > > Il giorno ven 17 lug 2026 alle ore 17:45 Francesco Saverio Pavone > ha scritto: > > > > From: Jonas Karlman > > > > remove() calls pm_runtime_disable() before > > pm_runtime_dont_use_autosuspend(), so the second call can never suspend > > the device: it reaches rpm_idle(), which returns -EACCES once PM runtime > > is disabled. The probe error path has had the two the other way round > > since the driver was merged. > > > > This shows up when the device is unbound while the 100ms autosuspend > > window is still open, which is what an rmmod right after a decode does. > > device_release_driver() calls pm_runtime_put_sync() before .remove(), > > and rpm_idle() adds RPM_AUTO on its own, so that put only arms the > > autosuspend timer. pm_runtime_disable() then cancels the timer, and > > pm_runtime_reinit() relabels the device suspended without calling the > > driver back. The clk_bulk reference taken by rkvdec_runtime_resume() is > > never dropped, and a later probe does not reclaim it, so every such > > unbind leaks one enable count. > > > > Drop autosuspend first, so the callback still runs and releases the > > clocks. > > > > The PM calls also have to move ahead of rkvdec_v4l2_cleanup() rather > > than just swap with each other. rkvdec_runtime_suspend() looks its state > > up with dev_get_drvdata(), and v4l2_device_unregister() clears it: > > struct rkvdec_dev has v4l2_device as its first member, so > > &rkvdec->v4l2_dev and rkvdec are the same address and the check in > > v4l2_device_disconnect() matches. That is harmless today because > > pm_runtime_disable() suppresses the callback, but once the callback can > > run, a suspend after the V4L2 teardown dereferences NULL. Swapping only > > the two PM calls oopses on every unbind. > > > > Fixes: cd33c830448b ("media: rkvdec: Add the rkvdec driver") > > Signed-off-by: Jonas Karlman > > [fsp: wrote the commit message; the diff is unchanged] > > Tested-by: Francesco Saverio Pavone > > Assisted-by: Claude:claude-opus-4-8 > > Signed-off-by: Francesco Saverio Pavone > > --- > > Changes in v4: > > - No functional change. Resent as its own thread (no In-Reply-To on v2), > > per Nicolas's note that a new version should be its own thread for > > patchwork tracking. Same diff and same commit message as v3. > > > > Changes in v3: > > - Rewrote the commit message, and dropped the VP9 claim from v1 and v2. > > Those said this fixed a VP9 inter-prediction bug on RK3588, green chroma > > from the second ALTREF frame onward. The bug is real, but this is not > > what fixes it, and I should have established that before sending v1. > > > > What happened: I took this patch out of chewitt's tree along with two > > others and tested the three as a batch. The green is fixed by "media: > > rkvdec: implement reset controls" from Alex Bee, which adds the > > reset_control handling that recovers the VDPU381 after a transient error > > (COLMV_REF_ERR_STA and friends) instead of leaving it dirty for the next > > inter frame. Randy Li's PMU idle export goes with it. This patch was the > > third one in that batch and got the credit. > > > > Retested this week on the same Rock 5B+ with an unpatched driver: a VP9 > > Profile 0 1080p clip with alt-ref frames decodes byte-identical to the > > libvpx reference, across five rmmod/insmod cycles and after an unbind > > inside the autosuspend window. The green does not come back, because the > > reset_control work is in the tree I test on. Sorry for the review and the > > testing you spent on that basis. > > > > - Worth flagging separately: mainline rkvdec has no reset_control support > > at all, so the VDPU381 is never recovered after a transient error. That > > is a real gap, it is just not this patch. I can write it up properly if > > that is useful. > > > > - The diff is unchanged from v1 and v2. It is Jonas's 2020 commit verbatim, > > and his original one-line subject already described exactly what it does. > > The wrong story was mine, not his. > > > > - The subject changed with the message: "media: rkvdec: fix PM runtime > > teardown ordering in remove" in v1 and v2, "media: rkvdec: fix clk > > reference leak on unbind" here, since that is what it actually fixes. > > > > - What is left is measured. With a dev_info() at the top of > > rkvdec_runtime_suspend(), autosuspend_delay raised to 60s to take the > > timer out of the race, and unbind driven through sysfs: > > unpatched: 0 suspend callbacks, aclk_rkvdec0 enable_count 1 -> 2 > > patched: 1 suspend callback, enable_count 1 -> 1 > > The leak survives rmmod and accumulates one per unbind. With > > autosuspend_delay=0 both orders suspend once, which is the control: the > > difference only exists inside the window. > > > > - Fixes: was wrong in v1 and v2. ff8c5622f9f7 has the two pm_runtime calls > > as context and only added iommu_domain_free(). cd33c830448b added remove() > > with the reversed order, and the probe error path with the right one, so > > the tag points there now. > > > > - Dropped Cc: stable. A clk reference leaked on unbind is not backport > > material, and the tag was only there for the VP9 claim. > > > > - Dropped your Reviewed-by and Tested-by from v2: they were given for a fix > > to something else. > > > > - Not included, happy to send as follow-ups: clearing empty_domain after > > iommu_domain_free(), and hoisting the unregisters to the top of remove() > > as you suggested on v2. > > > > Tested on a Radxa Rock 5B+ (RK3588) on a 7.1 tree where the six calls in > > rkvdec_v4l2_cleanup() are open-coded; the executed sequence is the one this > > patch produces. VP9 decode stays byte-identical to libvpx, and five > > rmmod/insmod cycles leave dmesg clean. > > > > Link to v1: https://lore.kernel.org/all/20260518105413.42147-1-pavone.lawyer@gmail.com/ > > Link to v2: https://lore.kernel.org/all/20260518145414.64514-1-pavone.lawyer@gmail.com/ > > Link to v3: https://lore.kernel.org/all/20260717150440.77079-1-pavone.lawyer@gmail.com/ > > drivers/media/platform/rockchip/rkvdec/rkvdec.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c > > index 1d1e9bfef8e9..0ec3fca9cccc 100644 > > --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c > > +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c > > @@ -1869,12 +1869,13 @@ static void rkvdec_remove(struct platform_device *pdev) > > > > cancel_delayed_work_sync(&rkvdec->watchdog_work); > > > > - rkvdec_v4l2_cleanup(rkvdec); > > - pm_runtime_disable(&pdev->dev); > > pm_runtime_dont_use_autosuspend(&pdev->dev); > > > > if (rkvdec->empty_domain) > > iommu_domain_free(rkvdec->empty_domain); > > + > > + pm_runtime_disable(&pdev->dev); > > + rkvdec_v4l2_cleanup(rkvdec); > > } > > > > #ifdef CONFIG_PM > > -- > > 2.54.0 > >