[4/6] media: platform: venus: Add optional LLCC path
Commit Message
Some newer SoCs (such as SM8350) have a third interconnect path. Add
it and make it optional.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
drivers/media/platform/qcom/venus/core.c | 19 +++++++++++++++++++
drivers/media/platform/qcom/venus/core.h | 3 +++
drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
3 files changed, 25 insertions(+)
Comments
On 04/08/2023 21:09, Konrad Dybcio wrote:
> Some newer SoCs (such as SM8350) have a third interconnect path. Add
> it and make it optional.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
> drivers/media/platform/qcom/venus/core.c | 19 +++++++++++++++++++
> drivers/media/platform/qcom/venus/core.h | 3 +++
> drivers/media/platform/qcom/venus/pm_helpers.c | 3 +++
> 3 files changed, 25 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index 0af45faec247..db363061748f 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
> if (IS_ERR(core->cpucfg_path))
> return PTR_ERR(core->cpucfg_path);
>
> + core->llcc_path = devm_of_icc_get(dev, "video-llcc");
> + if (IS_ERR(core->llcc_path)) {
> + /* LLCC path is optional */
> + if (PTR_ERR(core->llcc_path) == -ENODATA)
> + core->llcc_path = NULL;
> + else
> + return PTR_ERR(core->llcc_path);
> + }
> +
> core->irq = platform_get_irq(pdev, 0);
> if (core->irq < 0)
> return core->irq;
> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
> if (ret)
> goto err_cpucfg_path;
>
> + ret = icc_set_bw(core->llcc_path, 0, 0);
> + if (ret)
> + goto err_llcc_path;
> +
> ret = icc_set_bw(core->video_path, 0, 0);
> if (ret)
> goto err_video_path;
>
> return ret;
>
> +err_llcc_path:
> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
> err_video_path:
> icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> err_cpucfg_path:
> @@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
> if (ret)
> return ret;
>
> + ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
> + if (ret)
> + return ret;
> +
I would scream if someone left me this comment but...
In probe we have
video_path =
cpu_cfgpath =
llc_path =
suspend
icc_set_bw(cpu_cfgpath,);
icc_set_bw(llc_path,);
icc_set_bw(video_path,);
resume
icc_set_bw(video_path,);
icc_set_bw(llc_path,);
icc_set_bw(cpu_cfgpath,);
it would be nice to have probe match the ordering ...
> ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> if (ret)
> return ret;
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 2999c24392f5..45ed1551c2db 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -65,6 +65,7 @@ struct venus_resources {
> unsigned int bw_tbl_enc_size;
> const struct bw_tbl *bw_tbl_dec;
> unsigned int bw_tbl_dec_size;
> + bool has_llcc_path;
Why do you need this bool, you can get for llc_path == NULL
---
bod
On 04/08/2023 22:04, Bryan O'Donoghue wrote:
> you can get for llc_path == NULL
[sic] You can test.
---
bod
On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:
> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
> if (ret)
> goto err_cpucfg_path;
>
> + ret = icc_set_bw(core->llcc_path, 0, 0);
> + if (ret)
> + goto err_llcc_path;
> +
> ret = icc_set_bw(core->video_path, 0, 0);
> if (ret)
> goto err_video_path;
>
> return ret;
>
> +err_llcc_path:
> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
This looks wrong; you should not try to restore the video path bw which
you have not yet updated here.
Also error labels should be named after what they do, not after where
you jump from (e.g. to avoid mistakes like the above). Perhaps you can
clean up the existing labels before adding the new one.
> err_video_path:
> icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
> err_cpucfg_path:
Johan
On 7.08.2023 12:43, Johan Hovold wrote:
> On Fri, Aug 04, 2023 at 10:09:11PM +0200, Konrad Dybcio wrote:
>
>> @@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
>> if (ret)
>> goto err_cpucfg_path;
>>
>> + ret = icc_set_bw(core->llcc_path, 0, 0);
>> + if (ret)
>> + goto err_llcc_path;
>> +
>> ret = icc_set_bw(core->video_path, 0, 0);
>> if (ret)
>> goto err_video_path;
>>
>> return ret;
>>
>> +err_llcc_path:
>> + icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
>
> This looks wrong; you should not try to restore the video path bw which
> you have not yet updated here.
Oh whoops :D
>
> Also error labels should be named after what they do, not after where
> you jump from (e.g. to avoid mistakes like the above). Perhaps you can
> clean up the existing labels before adding the new one.
Ack, I wouldn't mind giving this some cleanup.
Konrad
@@ -302,6 +302,15 @@ static int venus_probe(struct platform_device *pdev)
if (IS_ERR(core->cpucfg_path))
return PTR_ERR(core->cpucfg_path);
+ core->llcc_path = devm_of_icc_get(dev, "video-llcc");
+ if (IS_ERR(core->llcc_path)) {
+ /* LLCC path is optional */
+ if (PTR_ERR(core->llcc_path) == -ENODATA)
+ core->llcc_path = NULL;
+ else
+ return PTR_ERR(core->llcc_path);
+ }
+
core->irq = platform_get_irq(pdev, 0);
if (core->irq < 0)
return core->irq;
@@ -479,12 +488,18 @@ static __maybe_unused int venus_runtime_suspend(struct device *dev)
if (ret)
goto err_cpucfg_path;
+ ret = icc_set_bw(core->llcc_path, 0, 0);
+ if (ret)
+ goto err_llcc_path;
+
ret = icc_set_bw(core->video_path, 0, 0);
if (ret)
goto err_video_path;
return ret;
+err_llcc_path:
+ icc_set_bw(core->video_path, kbps_to_icc(20000), 0);
err_video_path:
icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
err_cpucfg_path:
@@ -504,6 +519,10 @@ static __maybe_unused int venus_runtime_resume(struct device *dev)
if (ret)
return ret;
+ ret = icc_set_bw(core->llcc_path, kbps_to_icc(20000), 0);
+ if (ret)
+ return ret;
+
ret = icc_set_bw(core->cpucfg_path, kbps_to_icc(1000), 0);
if (ret)
return ret;
@@ -65,6 +65,7 @@ struct venus_resources {
unsigned int bw_tbl_enc_size;
const struct bw_tbl *bw_tbl_dec;
unsigned int bw_tbl_dec_size;
+ bool has_llcc_path;
const struct reg_val *reg_tbl;
unsigned int reg_tbl_size;
const struct hfi_ubwc_config *ubwc_conf;
@@ -134,6 +135,7 @@ struct venus_format {
* @vcodec1_clks: an array of vcodec1 struct clk pointers
* @video_path: an interconnect handle to video to/from memory path
* @cpucfg_path: an interconnect handle to cpu configuration path
+ * @llcc_path: an interconnect handle to video to/from llcc path
* @has_opp_table: does OPP table exist
* @pmdomains: an array of pmdomains struct device pointers
* @opp_dl_venus: an device-link for device OPP
@@ -186,6 +188,7 @@ struct venus_core {
struct clk *vcodec1_clks[VIDC_VCODEC_CLKS_NUM_MAX];
struct icc_path *video_path;
struct icc_path *cpucfg_path;
+ struct icc_path *llcc_path;
bool has_opp_table;
struct device *pmdomains[VIDC_PMDOMAINS_NUM_MAX];
struct device_link *opp_dl_venus;
@@ -237,6 +237,9 @@ static int load_scale_bw(struct venus_core *core)
dev_dbg(core->dev, VDBGL "total: avg_bw: %u, peak_bw: %u\n",
total_avg, total_peak);
+ if (core->res->has_llcc_path)
+ icc_set_bw(core->llcc_path, total_avg, total_peak);
+
return icc_set_bw(core->video_path, total_avg, total_peak);
}