| Message ID | 1586946198-13912-3-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
Hi Akash, On 4/15/20 13:23, Akash Asthana wrote: > Lot of ICC clients are not aware of their actual peak requirement, > most commonly they tend to guess their peak requirement as > (some factor) * avg_bw. > > Centralize random peak guess as twice of average, out into the core > to maintain consistency across the clients. Client can always > override this setting if they got a better idea. I am still not convinced that this is a good idea. If the factor is a random value, then i think that the default factor should be 1. According to your previous reply, it seems that from geni we are requesting double peak bandwidth to compensate for other clients which are not requesting bandwidth for themselves. IMO, this is a bit hacky. Instead of requesting double peak bandwidth, IIUC the correct thing to do here is to request peak_bw = avg_bw for geni. And instead of trying to compensate for other clients "stealing" bandwidth, can't we make these clients vote for their own bandwidth? Or if they really can't, this should be handled elsewhere - maybe in the interconnect platform driver we can reserve some amount of minimum bandwidth for such cases? Thanks, Georgi
Hi Georgi, On 4/23/2020 3:01 PM, Georgi Djakov wrote: > Hi Akash, > > On 4/15/20 13:23, Akash Asthana wrote: >> Lot of ICC clients are not aware of their actual peak requirement, >> most commonly they tend to guess their peak requirement as >> (some factor) * avg_bw. >> >> Centralize random peak guess as twice of average, out into the core >> to maintain consistency across the clients. Client can always >> override this setting if they got a better idea. > I am still not convinced that this is a good idea. If the factor is a random > value, then i think that the default factor should be 1. > > According to your previous reply, it seems that from geni we are requesting > double peak bandwidth to compensate for other clients which are not requesting > bandwidth for themselves. IMO, this is a bit hacky. > > Instead of requesting double peak bandwidth, IIUC the correct thing to do here > is to request peak_bw = avg_bw for geni. And instead of trying to compensate for > other clients "stealing" bandwidth, can't we make these clients vote for their > own bandwidth? Or if they really can't, this should be handled elsewhere - maybe > in the interconnect platform driver we can reserve some amount of minimum > bandwidth for such cases? Okay, probably we can correct clients vote for their own bandwidth or reserve some minimum BW from interconnect platform driver is case of any latency issue observed. I will drop this change in next version. Will it create any difference if peak_bw = 0 instead of peak_bw = avg_bw? In my understanding peak_bw <= avg_bw is no-ops, it won't impact the NOC speed. Regards, Akash > > Thanks, > Georgi
Hi Akash, On 4/28/20 12:46, Akash Asthana wrote: > Hi Georgi, > > On 4/23/2020 3:01 PM, Georgi Djakov wrote: >> Hi Akash, >> >> On 4/15/20 13:23, Akash Asthana wrote: >>> Lot of ICC clients are not aware of their actual peak requirement, >>> most commonly they tend to guess their peak requirement as >>> (some factor) * avg_bw. >>> >>> Centralize random peak guess as twice of average, out into the core >>> to maintain consistency across the clients. Client can always >>> override this setting if they got a better idea. >> I am still not convinced that this is a good idea. If the factor is a random >> value, then i think that the default factor should be 1. >> >> According to your previous reply, it seems that from geni we are requesting >> double peak bandwidth to compensate for other clients which are not requesting >> bandwidth for themselves. IMO, this is a bit hacky. >> >> Instead of requesting double peak bandwidth, IIUC the correct thing to do here >> is to request peak_bw = avg_bw for geni. And instead of trying to compensate for >> other clients "stealing" bandwidth, can't we make these clients vote for their >> own bandwidth? Or if they really can't, this should be handled elsewhere - maybe >> in the interconnect platform driver we can reserve some amount of minimum >> bandwidth for such cases? > > Okay, probably we can correct clients vote for their own bandwidth or reserve > some minimum BW from interconnect platform driver is case of any latency issue > observed. Yes, this sounds like the correct thing to do. > > I will drop this change in next version. > > Will it create any difference if peak_bw = 0 instead of peak_bw = avg_bw? In my > understanding peak_bw <= avg_bw is no-ops, it won't impact the NOC speed. It will not have impact on the NOC speed, but it does not make much logical sense to have peak_bw = 0 or peak_bw < avg_bw. In the geni case, i think what we want to do is peak_bw = avg_bw. Thanks, Georgi
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c index f5699ed..ad3938e 100644 --- a/drivers/interconnect/core.c +++ b/drivers/interconnect/core.c @@ -516,6 +516,10 @@ EXPORT_SYMBOL_GPL(icc_set_tag); * The @path can be NULL when the "interconnects" DT properties is missing, * which will mean that no constraints will be set. * + * This function assumes peak_bw as twice of avg_bw, if peak_bw is not mentioned + * explicitly by clients. Clients can pass peak_bw as 0 < peak_bw <=avg_bw to + * make it a noops. + * * Returns 0 on success, or an appropriate error code otherwise. */ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) @@ -531,6 +535,12 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) if (WARN_ON(IS_ERR(path) || !path->num_nodes)) return -EINVAL; + /* + * Assume peak requirement as twice of avg requirement, if it is not + * mentioned explicitly. + */ + peak_bw = peak_bw ? peak_bw : 2 * avg_bw; + mutex_lock(&icc_lock); old_avg = path->reqs[0].avg_bw;
Lot of ICC clients are not aware of their actual peak requirement, most commonly they tend to guess their peak requirement as (some factor) * avg_bw. Centralize random peak guess as twice of average, out into the core to maintain consistency across the clients. Client can always override this setting if they got a better idea. Signed-off-by: Akash Asthana <akashast@codeaurora.org> --- drivers/interconnect/core.c | 10 ++++++++++ 1 file changed, 10 insertions(+)