Hi Evan,
On 3/8/19 20:40, Evan Green wrote:
> On Fri, Feb 8, 2019 at 9:21 AM Georgi Djakov <georgi.djakov@xxxxxxxxxx> wrote:
>>
>> Consumers may have use cases with different bandwidth requirements based
>> on the system or driver state. The consumer driver can append a specific
>> tag to the path and pass this information to the interconnect platform
>> driver to do the aggregation based on this state.
>>
>> Introduce icc_set_tag() function that will allow the consumers to append
>> an optional tag to each path. The aggregation of these tagged paths is
>> platform specific.
>>
>> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
>> ---
>> drivers/interconnect/core.c | 27 +++++++++++++++++++++++----
>> drivers/interconnect/qcom/sdm845.c | 2 +-
>> include/linux/interconnect-provider.h | 4 ++--
>> include/linux/interconnect.h | 5 +++++
>> 4 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 6005a1c189f6..31eacd0f5349 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -42,10 +42,12 @@ struct icc_req {
>>
>> /**
>> * struct icc_path - interconnect path structure
>> + * @tag: path tag (optional)
>> * @num_nodes: number of hops (nodes)
>> * @reqs: array of the requests applicable to this path of nodes
>> */
>> struct icc_path {
>> + u32 tag;
>> size_t num_nodes;
>> struct icc_req reqs[];
>> };
>> @@ -206,7 +208,7 @@ static struct icc_path *path_find(struct device *dev, struct icc_node *src,
>> * implementing its own aggregate() function.
>> */
>>
>> -static int aggregate_requests(struct icc_node *node)
>> +static int aggregate_requests(struct icc_node *node, u32 tag)
>> {
>> struct icc_provider *p = node->provider;
>> struct icc_req *r;
>> @@ -215,7 +217,7 @@ static int aggregate_requests(struct icc_node *node)
>> node->peak_bw = 0;
>
> For my suggestion in patch 2, this is where you'd need the callback to
> reset the SLEEP/WAKE shadow aggregation buckets that the provider is
> keeping, since the core just reset the values here. I wonder if you'd
> want to pass the &node->avg_bw and &node->peak_bw in with this
> callback as well... not sure if that's useful. Something like:
>
> if (p->begin_aggregation)
> p->begin_aggregation(node);
Currently this seems only Qcom specific, so i would prefer to wait and
see if there are similar need for other platforms before adding a new
callback. IMO the best option for now would be to reset the values in
the platform driver after they are committed.
>>
>> hlist_for_each_entry(r, &node->req_list, req_node)
>> - p->aggregate(node, r->avg_bw, r->peak_bw,
>> + p->aggregate(node, tag, r->avg_bw, r->peak_bw,
>> &node->avg_bw, &node->peak_bw);
>
> Wait I'm confused. The tag is associated with a particular path. But
> now aren't you looping over all the other requests and coloring them
> with whatever tag this is? Don't you need to store the tag with the
> request, and then pass r->tag for each r rather than the tag from
> whichever request happened to initiate the re-aggregation?
Yes indeed. I think i am missing a patch here. Thanks for catching it!
BR,
Georgi
>>
>> return 0;
>> @@ -396,6 +398,23 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
>> }
>> EXPORT_SYMBOL_GPL(of_icc_get);
>>
