| Message ID | 20190814084701.25455-2-stanimir.varbanov@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Venus interconnect support for sdm845 | expand |
Hi Stan, On 8/14/19 11:47, Stanimir Varbanov wrote: > This aims to add a requests for bandwidth scaling depending > on the resolution and framerate (macroblocks per second). The > exact value ff the requested bandwidth is get from a s/ff/of/ > pre-calculated tables for encoder and decoder. > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > --- > drivers/media/platform/qcom/venus/core.c | 34 +++++++++++ > drivers/media/platform/qcom/venus/core.h | 14 +++++ > drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++- > 3 files changed, 114 insertions(+), 1 deletion(-) It looks like venus can be built-in, so how about the case when venus is built-in and the interconnect provider is a module? Maybe add a dependency in Kconfig to depend on INTERCONNECT || !INTERCONNECT? > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > index 0acc7576cc58..19cbe9d5d028 100644 > --- a/drivers/media/platform/qcom/venus/core.c > +++ b/drivers/media/platform/qcom/venus/core.c > @@ -5,6 +5,7 @@ > */ > #include <linux/clk.h> > #include <linux/init.h> > +#include <linux/interconnect.h> > #include <linux/ioctl.h> > #include <linux/list.h> > #include <linux/module.h> > @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev) > if (IS_ERR(core->base)) > return PTR_ERR(core->base); > > + core->video_path = of_icc_get(dev, "video-mem"); > + if (IS_ERR(core->video_path)) > + return PTR_ERR(core->video_path); > + > + core->cpucfg_path = of_icc_get(dev, "cpu-cfg"); > + if (IS_ERR(core->cpucfg_path)) > + return PTR_ERR(core->cpucfg_path); > + > core->irq = platform_get_irq(pdev, 0); > if (core->irq < 0) > return core->irq; > @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev) > if (ret) > return ret; > > + ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000)); > + if (ret) > + return ret; > + > ret = hfi_create(core, &venus_core_ops); > if (ret) > return ret; > @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev) > pm_runtime_put_sync(dev); > pm_runtime_disable(dev); > > + icc_put(core->video_path); > + icc_put(core->cpucfg_path); > + Do you have any plans to scale the bandwidth on suspend/resume too? Thanks, Georgi
Hi Georgi, Thanks for the review! On 8/20/19 12:34 PM, Georgi Djakov wrote: > Hi Stan, > > On 8/14/19 11:47, Stanimir Varbanov wrote: >> This aims to add a requests for bandwidth scaling depending >> on the resolution and framerate (macroblocks per second). The >> exact value ff the requested bandwidth is get from a > > s/ff/of/ > >> pre-calculated tables for encoder and decoder. >> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >> --- >> drivers/media/platform/qcom/venus/core.c | 34 +++++++++++ >> drivers/media/platform/qcom/venus/core.h | 14 +++++ >> drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++- >> 3 files changed, 114 insertions(+), 1 deletion(-) > > It looks like venus can be built-in, so how about the case when venus is > built-in and the interconnect provider is a module? Maybe add a dependency in > Kconfig to depend on INTERCONNECT || !INTERCONNECT? yes, I forgot about that dependency. > >> >> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c >> index 0acc7576cc58..19cbe9d5d028 100644 >> --- a/drivers/media/platform/qcom/venus/core.c >> +++ b/drivers/media/platform/qcom/venus/core.c >> @@ -5,6 +5,7 @@ >> */ >> #include <linux/clk.h> >> #include <linux/init.h> >> +#include <linux/interconnect.h> >> #include <linux/ioctl.h> >> #include <linux/list.h> >> #include <linux/module.h> >> @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev) >> if (IS_ERR(core->base)) >> return PTR_ERR(core->base); >> >> + core->video_path = of_icc_get(dev, "video-mem"); >> + if (IS_ERR(core->video_path)) >> + return PTR_ERR(core->video_path); >> + >> + core->cpucfg_path = of_icc_get(dev, "cpu-cfg"); >> + if (IS_ERR(core->cpucfg_path)) >> + return PTR_ERR(core->cpucfg_path); >> + >> core->irq = platform_get_irq(pdev, 0); >> if (core->irq < 0) >> return core->irq; >> @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev) >> if (ret) >> return ret; >> >> + ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000)); >> + if (ret) >> + return ret; >> + >> ret = hfi_create(core, &venus_core_ops); >> if (ret) >> return ret; >> @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev) >> pm_runtime_put_sync(dev); >> pm_runtime_disable(dev); >> >> + icc_put(core->video_path); >> + icc_put(core->cpucfg_path); >> + > > Do you have any plans to scale the bandwidth on suspend/resume too? Yes, we definitely need that in suspend/resume, but I guess the plan should be add it once we implement pm_runtime autosuspend functionality in order to easily test that.
On Tue 20 Aug 02:34 PDT 2019, Georgi Djakov wrote: > Hi Stan, > > On 8/14/19 11:47, Stanimir Varbanov wrote: > > This aims to add a requests for bandwidth scaling depending > > on the resolution and framerate (macroblocks per second). The > > exact value ff the requested bandwidth is get from a > > s/ff/of/ > > > pre-calculated tables for encoder and decoder. > > > > Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> > > --- > > drivers/media/platform/qcom/venus/core.c | 34 +++++++++++ > > drivers/media/platform/qcom/venus/core.h | 14 +++++ > > drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++- > > 3 files changed, 114 insertions(+), 1 deletion(-) > > It looks like venus can be built-in, so how about the case when venus is > built-in and the interconnect provider is a module? Maybe add a dependency in > Kconfig to depend on INTERCONNECT || !INTERCONNECT? > I've been struggling down this road for remoteproc et al for a long time, I strongly suggest that you make the INTERCONNECT config bool, to ensure that we don't see this problem for every client. The interconnect framework should hide the fact that the provider is module. But with this in place is there actually a dependency? Won't the include file provide stubs in the case of !INTERCONNECT? Regards, Bjorn > > > > diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c > > index 0acc7576cc58..19cbe9d5d028 100644 > > --- a/drivers/media/platform/qcom/venus/core.c > > +++ b/drivers/media/platform/qcom/venus/core.c > > @@ -5,6 +5,7 @@ > > */ > > #include <linux/clk.h> > > #include <linux/init.h> > > +#include <linux/interconnect.h> > > #include <linux/ioctl.h> > > #include <linux/list.h> > > #include <linux/module.h> > > @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev) > > if (IS_ERR(core->base)) > > return PTR_ERR(core->base); > > > > + core->video_path = of_icc_get(dev, "video-mem"); > > + if (IS_ERR(core->video_path)) > > + return PTR_ERR(core->video_path); > > + > > + core->cpucfg_path = of_icc_get(dev, "cpu-cfg"); > > + if (IS_ERR(core->cpucfg_path)) > > + return PTR_ERR(core->cpucfg_path); > > + > > core->irq = platform_get_irq(pdev, 0); > > if (core->irq < 0) > > return core->irq; > > @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev) > > if (ret) > > return ret; > > > > + ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000)); > > + if (ret) > > + return ret; > > + > > ret = hfi_create(core, &venus_core_ops); > > if (ret) > > return ret; > > @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev) > > pm_runtime_put_sync(dev); > > pm_runtime_disable(dev); > > > > + icc_put(core->video_path); > > + icc_put(core->cpucfg_path); > > + > > Do you have any plans to scale the bandwidth on suspend/resume too? > > Thanks, > Georgi
Hi Bjorn, On 9/4/19 07:22, Bjorn Andersson wrote: > On Tue 20 Aug 02:34 PDT 2019, Georgi Djakov wrote: > >> Hi Stan, >> >> On 8/14/19 11:47, Stanimir Varbanov wrote: >>> This aims to add a requests for bandwidth scaling depending >>> on the resolution and framerate (macroblocks per second). The >>> exact value ff the requested bandwidth is get from a >> >> s/ff/of/ >> >>> pre-calculated tables for encoder and decoder. >>> >>> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> >>> --- >>> drivers/media/platform/qcom/venus/core.c | 34 +++++++++++ >>> drivers/media/platform/qcom/venus/core.h | 14 +++++ >>> drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++- >>> 3 files changed, 114 insertions(+), 1 deletion(-) >> >> It looks like venus can be built-in, so how about the case when venus is >> built-in and the interconnect provider is a module? Maybe add a dependency in >> Kconfig to depend on INTERCONNECT || !INTERCONNECT? >> > > I've been struggling down this road for remoteproc et al for a long > time, I strongly suggest that you make the INTERCONNECT config bool, to > ensure that we don't see this problem for every client. Thanks for the comment. Well, i was expecting that we will have to make it bool one day. Viresh actually already sent a patch [1]. Maybe you can add an Ack? > > The interconnect framework should hide the fact that the provider is > module. > Correct. > > But with this in place is there actually a dependency? Won't the include > file provide stubs in the case of !INTERCONNECT? There are stubs, so we are fine. Thanks, Georgi [1] https://lore.kernel.org/linux-pm/b789cce388dd1f2906492f307dea6780c398bc6a.1567065991.git.viresh.kumar@linaro.org/
diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c index 0acc7576cc58..19cbe9d5d028 100644 --- a/drivers/media/platform/qcom/venus/core.c +++ b/drivers/media/platform/qcom/venus/core.c @@ -5,6 +5,7 @@ */ #include <linux/clk.h> #include <linux/init.h> +#include <linux/interconnect.h> #include <linux/ioctl.h> #include <linux/list.h> #include <linux/module.h> @@ -239,6 +240,14 @@ static int venus_probe(struct platform_device *pdev) if (IS_ERR(core->base)) return PTR_ERR(core->base); + core->video_path = of_icc_get(dev, "video-mem"); + if (IS_ERR(core->video_path)) + return PTR_ERR(core->video_path); + + core->cpucfg_path = of_icc_get(dev, "cpu-cfg"); + if (IS_ERR(core->cpucfg_path)) + return PTR_ERR(core->cpucfg_path); + core->irq = platform_get_irq(pdev, 0); if (core->irq < 0) return core->irq; @@ -273,6 +282,10 @@ static int venus_probe(struct platform_device *pdev) if (ret) return ret; + ret = icc_set_bw(core->cpucfg_path, 0, kbps_to_icc(1000)); + if (ret) + return ret; + ret = hfi_create(core, &venus_core_ops); if (ret) return ret; @@ -355,6 +368,9 @@ static int venus_remove(struct platform_device *pdev) pm_runtime_put_sync(dev); pm_runtime_disable(dev); + icc_put(core->video_path); + icc_put(core->cpucfg_path); + v4l2_device_unregister(&core->v4l2_dev); return ret; @@ -464,9 +480,27 @@ static const struct freq_tbl sdm845_freq_table[] = { { 244800, 100000000 }, /* 1920x1080@30 */ }; +static const struct bw_tbl sdm845_bw_table_enc[] = { + { 1944000, 1612000, 0, 2416000, 0 }, /* 3840x2160@60 */ + { 972000, 951000, 0, 1434000, 0 }, /* 3840x2160@30 */ + { 489600, 723000, 0, 973000, 0 }, /* 1920x1080@60 */ + { 244800, 370000, 0, 495000, 0 }, /* 1920x1080@30 */ +}; + +static const struct bw_tbl sdm845_bw_table_dec[] = { + { 2073600, 3929000, 0, 5551000, 0 }, /* 4096x2160@60 */ + { 1036800, 1987000, 0, 2797000, 0 }, /* 4096x2160@30 */ + { 489600, 1040000, 0, 1298000, 0 }, /* 1920x1080@60 */ + { 244800, 530000, 0, 659000, 0 }, /* 1920x1080@30 */ +}; + static const struct venus_resources sdm845_res = { .freq_tbl = sdm845_freq_table, .freq_tbl_size = ARRAY_SIZE(sdm845_freq_table), + .bw_tbl_enc = sdm845_bw_table_enc, + .bw_tbl_enc_size = ARRAY_SIZE(sdm845_bw_table_enc), + .bw_tbl_dec = sdm845_bw_table_dec, + .bw_tbl_dec_size = ARRAY_SIZE(sdm845_bw_table_dec), .clks = {"core", "iface", "bus" }, .clks_num = 3, .max_load = 3110400, /* 4096x2160@90 */ diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h index 959eaa550f4e..4b0eb4627ba0 100644 --- a/drivers/media/platform/qcom/venus/core.h +++ b/drivers/media/platform/qcom/venus/core.h @@ -26,10 +26,22 @@ struct reg_val { u32 value; }; +struct bw_tbl { + u32 mbs_per_sec; + u32 avg; + u32 peak; + u32 avg_10bit; + u32 peak_10bit; +}; + struct venus_resources { u64 dma_mask; const struct freq_tbl *freq_tbl; unsigned int freq_tbl_size; + const struct bw_tbl *bw_tbl_enc; + unsigned int bw_tbl_enc_size; + const struct bw_tbl *bw_tbl_dec; + unsigned int bw_tbl_dec_size; const struct reg_val *reg_tbl; unsigned int reg_tbl_size; const char * const clks[VIDC_CLKS_NUM_MAX]; @@ -114,6 +126,8 @@ struct venus_core { struct clk *core1_clk; struct clk *core0_bus_clk; struct clk *core1_bus_clk; + struct icc_path *video_path; + struct icc_path *cpucfg_path; struct video_device *vdev_dec; struct video_device *vdev_enc; struct v4l2_device v4l2_dev; diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c index 1ad96c25ab09..f18458921f5d 100644 --- a/drivers/media/platform/qcom/venus/helpers.c +++ b/drivers/media/platform/qcom/venus/helpers.c @@ -5,6 +5,7 @@ */ #include <linux/clk.h> #include <linux/iopoll.h> +#include <linux/interconnect.h> #include <linux/list.h> #include <linux/mutex.h> #include <linux/pm_runtime.h> @@ -388,6 +389,65 @@ static u32 load_per_type(struct venus_core *core, u32 session_type) return mbs_per_sec; } +static void mbs_to_bw(struct venus_inst *inst, u32 mbs, u32 *avg, u32 *peak) +{ + const struct venus_resources *res = inst->core->res; + const struct bw_tbl *bw_tbl; + unsigned int num_rows, i; + + *avg = 0; + *peak = 0; + + if (mbs == 0) + return; + + if (inst->session_type == VIDC_SESSION_TYPE_ENC) { + num_rows = res->bw_tbl_enc_size; + bw_tbl = res->bw_tbl_enc; + } else if (inst->session_type == VIDC_SESSION_TYPE_DEC) { + num_rows = res->bw_tbl_dec_size; + bw_tbl = res->bw_tbl_dec; + } else { + return; + } + + if (!bw_tbl || num_rows == 0) + return; + + for (i = 0; i < num_rows; i++) { + if (mbs > bw_tbl[i].mbs_per_sec) + break; + + if (inst->dpb_fmt & HFI_COLOR_FORMAT_10_BIT_BASE) { + *avg = bw_tbl[i].avg_10bit; + *peak = bw_tbl[i].peak_10bit; + } else { + *avg = bw_tbl[i].avg; + *peak = bw_tbl[i].peak; + } + } +} + +static int load_scale_bw(struct venus_core *core) +{ + struct venus_inst *inst = NULL; + u32 mbs_per_sec, avg, peak, total_avg = 0, total_peak = 0; + + mutex_lock(&core->lock); + list_for_each_entry(inst, &core->instances, list) { + mbs_per_sec = load_per_instance(inst); + mbs_to_bw(inst, mbs_per_sec, &avg, &peak); + total_avg += avg; + total_peak += peak; + } + mutex_unlock(&core->lock); + + dev_dbg(core->dev, "total: avg_bw: %u, peak_bw: %u\n", + total_avg, total_peak); + + return icc_set_bw(core->video_path, total_avg, total_peak); +} + int venus_helper_load_scale_clocks(struct venus_core *core) { const struct freq_tbl *table = core->res->freq_tbl; @@ -431,10 +491,15 @@ int venus_helper_load_scale_clocks(struct venus_core *core) if (ret) goto err; + ret = load_scale_bw(core); + if (ret) + goto err; + return 0; err: - dev_err(dev, "failed to set clock rate %lu (%d)\n", freq, ret); + dev_err(dev, "failed to set clock rate %lu or bandwidth (%d)\n", + freq, ret); return ret; } EXPORT_SYMBOL_GPL(venus_helper_load_scale_clocks);
This aims to add a requests for bandwidth scaling depending on the resolution and framerate (macroblocks per second). The exact value ff the requested bandwidth is get from a pre-calculated tables for encoder and decoder. Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org> --- drivers/media/platform/qcom/venus/core.c | 34 +++++++++++ drivers/media/platform/qcom/venus/core.h | 14 +++++ drivers/media/platform/qcom/venus/helpers.c | 67 ++++++++++++++++++++- 3 files changed, 114 insertions(+), 1 deletion(-)