Re: [PATCH] media: rkvdec: Do not keep the clocks enabled from probe

[Nicolas Dufresne <nicolas.dufresne@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 3231 bytes --]

Le vendredi 30 octobre 2026 à 03:33 +0200, Igor Velkov via B4 Relay a écrit :
> From: Igor Velkov <iav@iav.lv>
> 
> Since commit 6a846f7d72c7 ("media: rkvdec: Enable all clocks without
> naming them") the clocks are fetched with devm_clk_bulk_get_all_enabled(),
> which keeps them enabled from probe until unbind. rkvdec_runtime_suspend()
> disables them on top of that, so their enable count never drops to zero
> and the clock gates stay open while the decoder is idle.
> 
> On RK3399 this also breaks restore from hibernation when the boot kernel
> has not enabled these clocks, for example with rkvdec built as a module:
> the gates are closed when the restored kernel takes over, while it still
> counts the clocks as enabled. Powering on the vdu power domain then finds
> the count at 1 and leaves the gates closed, and the first QoS register
> write of the domain never returns.
> 
> Get the clocks without enabling them and let runtime PM enable them while
> the decoder is in use, as before that commit.
> 
> Fixes: 6a846f7d72c7 ("media: rkvdec: Enable all clocks without naming them")
> Cc: stable@vger.kernel.org # v7.0+
> Signed-off-by: Igor Velkov <iav@iav.lv>
> ---
> Tested on ROCK Pi 4A (RK3399) with v7.3, rkvdec and panfrost built as
> modules, panfrost not loaded:
> - idle decoder: the vdu clocks have enable_count 0 (1 without the patch);
> - hibernate, power cycle, restore: 3 of 3 (0 of 3 without the patch, the
>   restored kernel stops after bringing the secondary CPUs up);
> - H.264 decode with GStreamer v4l2slh264dec, 300 frames of 720p High
>   profile: output identical to avdec_h264 (md5 of all frames).
> The QoS hang was traced with debug prints in rockchip_pd_power() on a
> 7.3-based kernel.
> 
> This is separate from the unbind leak in Jonas's patch that Francesco
> posted [1]: here the extra enable comes from probe.
> 
> [1] https://lore.kernel.org/all/20260717154505.83935-1-pavone.lawyer@gmail.com/
> ---
>  drivers/media/platform/rockchip/rkvdec/rkvdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> index 061281f903f3..45c541ae3029 100644
> --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
> @@ -1788,7 +1788,7 @@ static int rkvdec_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &rkvdec->clocks);
> +	ret = devm_clk_bulk_get_all(&pdev->dev, &rkvdec->clocks);

With the explanation above, it seems pretty clear we have an issue. Now, the
problem with this is that PM ops are set with:

  #define pm_ptr(_ptr) PTR_IF(IS_ENABLED(CONFIG_PM), (_ptr))

So if CONFIG_PM is not set, the initial resume will never occur, and first mmio
access will likely hang the CPU. We need a clean pattern to handle this, and
perhaps review other similar drivers while at it.

Nicolas

>  	if (ret < 0)
>  		return ret;
>  
> 
> ---
> base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
> change-id: 20261030-rkvdec-runtime-pm-clocks-41adcf141271
> 
> Best regards,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]