Messages in this thread | | | Date | Thu, 19 Jan 2023 14:18:12 -0800 | Subject | Re: [PATCH] interconnect: Skip call into provider if initial bw is zero | From | Vivek Aknurwar <> |
| |
Hi Bryan, Thanks for taking time to review the patch.
On 1/13/2023 5:40 PM, Bryan O'Donoghue wrote: > On 14/01/2023 01:24, Bryan O'Donoghue wrote: >> On 13/01/2023 22:07, Vivek Aknurwar wrote: >>> Currently framework sets bw even when init bw requirements are zero >>> during >>> provider registration, thus resulting bulk of set bw to hw. >>> Avoid this behaviour by skipping provider set bw calls if init bw is >>> zero. >>> >>> Signed-off-by: Vivek Aknurwar <quic_viveka@quicinc.com> >>> --- >>> drivers/interconnect/core.c | 17 ++++++++++------- >>> 1 file changed, 10 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c >>> index 25debde..43ed595 100644 >>> --- a/drivers/interconnect/core.c >>> +++ b/drivers/interconnect/core.c >>> @@ -977,14 +977,17 @@ void icc_node_add(struct icc_node *node, struct >>> icc_provider *provider) >>> node->avg_bw = node->init_avg; >>> node->peak_bw = node->init_peak; >>> - if (provider->pre_aggregate) >>> - provider->pre_aggregate(node); >>> - >>> - if (provider->aggregate) >>> - provider->aggregate(node, 0, node->init_avg, node->init_peak, >>> - &node->avg_bw, &node->peak_bw); >>> + if (node->avg_bw || node->peak_bw) { >>> + if (provider->pre_aggregate) >>> + provider->pre_aggregate(node); >>> + >>> + if (provider->aggregate) >>> + provider->aggregate(node, 0, node->init_avg, >>> node->init_peak, >>> + &node->avg_bw, &node->peak_bw); >>> + if (provider->set) >>> + provider->set(node, node); >>> + } >>> - provider->set(node, node); >>> node->avg_bw = 0; >>> node->peak_bw = 0; >> >> I have the same comment/question for this patch that I had for the >> qcom arch specific version of it. This patch seems to be doing at a >> higher level what the patch below was doing at a lower level. >> >> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m0c90588d0d1e2ab88c39be8f5f3a8f0b61396349 >> >> what happens to earlier silicon - qcom silicon which previously made >> explicit zero requests ?
This patch is to optimize and avoid all those bw 0 requests on each node addition during probe (which results in rpmh remote calls) for upcoming targets.
>> >> https://lore.kernel.org/lkml/1039a507-c4cd-e92f-dc29-1e2169ce5078@linaro.org/T/#m589e8280de470e038249bb362634221771d845dd >> >> https://lkml.org/lkml/2023/1/3/1232 >> >> Isn't it a better idea to let lower layer drivers differentiate what >> they do ?
AFAIU lower layer driver can/should not differentiate between normal flow calls vs made as a result from probe/initialization of driver. Hence even bw 0 request is honored as like client in general wish to vote 0 as in an normal use case.
>> >> For example on pre 5.4 qcom kernel silicon we might choose to set the >> value to zero "because that's what the reference code did" but on >> newer silicon we might opt to skip the zero configuration ? >> >> I'm happy to be shown the error of my ways but, absent testing to >> *show* it doesn't impact existing legacy silicon, I think we should be >> wary of this change. >> >> --- >> bod > > Oh, and what is the effect on Samsung and i.MX silicon interconnect > providers of skipping the zero set ?
If interconnect providers are trying to clear bw votes coming from boot-loader then best place to clear those is in sync-state call back.
> > --- > bod
| |