Re: [PATCH v6 7/8] media: qcom: venus: Add msm8939 resource struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Non-monotonic frequency table causes scaling inversions and dead code.
- [High] Clock rate array aliasing leaves `vcodec1_core` unscaled.
- [High] Missing PM domain detachment causes a resource leak on probe failure/unbind.
--

commit 568a6ece0caefd3f300b18b1d182b85169c54db9
Author: André Apitzsch <git@apitzsch.eu>

media: qcom: venus: Add msm8939 resource struct

This commit adds configuration data and the related compatible string to
support the msm8939 platform in the venus media driver.

> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -688,6 +688,46 @@ static const struct venus_resources msm8916_res = {
>  	.enc_nodename = "video-encoder",
>  };
>  
> +static const struct freq_tbl msm8939_freq_table[] = {
> +	{ 489600, 266670000 },	/* 1080p @ 60 */
> +	{ 244800, 133330000 },	/* 1080p @ 30 */
> +	{ 244800, 200000000 },	/* 1080p @ 30 */
> +	{ 220800, 133330000 },	/* 720p @ 60 */
> +	{ 108000, 133330000 },	/* 720p @ 30 */
> +	{ 108000, 200000000 },	/* 720p @ 30 */

The freq_tbl algorithm in load_scale_v1() iterates sequentially over this
table and stops when mbs_per_sec > table[i].load.

Since there are duplicate load thresholds here (e.g., 244800 and 108000),
does this ordering cause scaling inversions and leave the 133.33MHz entries
as dead code?

For a load of 108001, the loop breaks at the first 108000 entry, falling
back to the previous frequency (133.33MHz). However, for a lower load of
exactly 108000, it continues past the first entry and breaks at 72000,
acquiring the higher 200MHz frequency.

> +	{ 72000, 133330000 },	/* VGA @ 60 */
> +	{ 36000, 133330000 },	/* VGA @ 30 */
> +};

[ ... ]

> +static const struct venus_resources msm8939_res = {
> +	.freq_tbl = msm8939_freq_table,
> +	.freq_tbl_size = ARRAY_SIZE(msm8939_freq_table),
> +	.reg_tbl = msm8939_reg_preset,
> +	.reg_tbl_size = ARRAY_SIZE(msm8939_reg_preset),
> +	.clks = { "core", "iface", "bus", },
> +	.clks_num = 3,
> +	.vcodec0_clks = { "vcodec0_core", "vcodec1_core" },
> +	.vcodec1_clks = { "vcodec0_core", "vcodec1_core" },

Are both clock arrays intended to have "vcodec0_core" at index 0?

The dynamic frequency scaling logic in core_clks_set_rate() applies the
required frequency explicitly to index 0 of both arrays:

    clk_set_rate(core->vcodec0_clks[0], freq);
    clk_set_rate(core->vcodec1_clks[0], freq);

Because "vcodec0_core" is at index 0 for both arrays, this sets
vcodec0_core twice and leaves vcodec1_core unscaled.

> +	.vcodec_clks_num = 2,
> +	.vcodec_pmdomains = (const char *[]) { "venus", "vcodec0", "vcodec1" },

Because previous V1 resources did not define vcodec_pmdomains, the domain
attachment in vcodec_domains_get_v1() would return early.

Now that this patch populates the array, will this cause a PM domain
resource leak on probe failure or driver unbind?

The corresponding teardown function core_put_v1() is currently empty and
lacks a call to dev_pm_domain_detach_list() to clean up the domains
attached by vcodec_domains_get_v1().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260510-msm8939-venus-rfc-v6-0-e69465375900@gmail.com?part=7