Re: [PATCH v1 1/2] interconnect: Add support for path tags

[Georgi Djakov <georgi.djakov@linaro.org>]

Hi Evan,

On 3/8/19 20:40, Evan Green wrote:
> On Fri, Feb 8, 2019 at 9:21 AM Georgi Djakov <georgi.djakov@linaro.org> 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@linaro.org>
>> ---
>>  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);
>>