Re: [PATCH V4 2/9] interconnect: Set peak requirement as twice of average

From: Georgi Djakov <georgi.djakov@linaro.org>
To: Akash Asthana <akashast@codeaurora.org>, broonie@kernel.org
Cc: gregkh@linuxfoundation.org, agross@kernel.org,
	bjorn.andersson@linaro.org, wsa@the-dreams.de,
	mark.rutland@arm.com, robh+dt@kernel.org,
	linux-i2c@vger.kernel.org, linux-spi@vger.kernel.org,
	devicetree@vger.kernel.org, swboyd@chromium.org,
	mgautam@codeaurora.org, linux-arm-msm@vger.kernel.org,
	linux-serial@vger.kernel.org, mka@chromium.org,
	dianders@chromium.org, evgreen@chromium.org,
	Linux PM list <linux-pm@vger.kernel.org>,
	Mike Tipton <mdtipton@codeaurora.org>,
	Sean Sweeney <seansw@qti.qualcomm.com>
Subject: Re: [PATCH V4 2/9] interconnect: Set peak requirement as twice of average
Date: Tue, 28 Apr 2020 13:53:21 +0300	[thread overview]
Message-ID: <94e7ad8d-2680-1e62-8072-703d6e220341@linaro.org> (raw)
In-Reply-To: <7a79688c-3b9b-c7c1-2973-fca0c4b2c78b@codeaurora.org>

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