[PATCH v1 0/2] interconnect: Add path tagging support

[PATCH v1 0/2] interconnect: Add path tagging support

[Georgi Djakov]

SoCs that have multiple coexisting CPUs and DSPs, may have shared
interconnect buses between them. In such cases, each CPU/DSP may have
different bandwidth needs, depending on whether it is active or sleeping.
This means that we have to keep different bandwidth configurations for
the CPU (active/sleep). In such systems, usually there is a way to
communicate and synchronize this information with some firmware or pass
it to another processor responsible for monitoring and switching the
interconnect configurations based on the state of each CPU/DSP.

The above problem can be solved by introducing the path tagging concept,
that allows consumers to optionally attach a tag to each path they use.
This tag is used to differentiate between the aggregated bandwidth values
for each state. The tag is generic and how it's handled is up to the
platform specific interconnect provider drivers.

David Dai (1):
  interconnect: qcom: Add tagging and wake/sleep support for sdm845

Georgi Djakov (1):
  interconnect: Add support for path tags

 drivers/interconnect/core.c           |  27 ++++++-
 drivers/interconnect/qcom/sdm845.c    | 103 +++++++++++++++++++-------
 include/linux/interconnect-provider.h |   4 +-
 include/linux/interconnect.h          |   5 ++
 4 files changed, 106 insertions(+), 33 deletions(-)

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

[Georgi Djakov]

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;
 
 	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);
 
 	return 0;
@@ -396,6 +398,23 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
 }
 EXPORT_SYMBOL_GPL(of_icc_get);
 
+/**
+ * icc_set_tag() - set an optional tag on a path
+ * @path: the path we want to tag
+ * @tag: the tag value
+ *
+ * This function allows consumers to append a tag to the path, so that a
+ * different aggregation could be done based on this tag.
+ */
+void icc_set_tag(struct icc_path *path, u32 tag)
+{
+	if (!path)
+		return;
+
+	path->tag = tag;
+}
+EXPORT_SYMBOL_GPL(icc_set_tag);
+
 /**
  * icc_set_bw() - set bandwidth constraints on an interconnect path
  * @path: reference to the path returned by icc_get()
@@ -434,7 +453,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 		path->reqs[i].peak_bw = peak_bw;
 
 		/* aggregate requests for this node */
-		aggregate_requests(node);
+		aggregate_requests(node, path->tag);
 	}
 
 	ret = apply_constraints(path);
@@ -446,7 +465,7 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 			node = path->reqs[i].node;
 			path->reqs[i].avg_bw = old_avg;
 			path->reqs[i].peak_bw = old_peak;
-			aggregate_requests(node);
+			aggregate_requests(node, path->tag);
 		}
 		apply_constraints(path);
 	}
diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
index 4915b78da673..fb526004c82e 100644
--- a/drivers/interconnect/qcom/sdm845.c
+++ b/drivers/interconnect/qcom/sdm845.c
@@ -626,7 +626,7 @@ static void bcm_aggregate(struct qcom_icc_bcm *bcm)
 	bcm->dirty = false;
 }
 
-static int qcom_icc_aggregate(struct icc_node *node, u32 avg_bw,
+static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 			      u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
 {
 	size_t i;
diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
index 63caccadc2db..4ee19fd41568 100644
--- a/include/linux/interconnect-provider.h
+++ b/include/linux/interconnect-provider.h
@@ -45,8 +45,8 @@ struct icc_provider {
 	struct list_head	provider_list;
 	struct list_head	nodes;
 	int (*set)(struct icc_node *src, struct icc_node *dst);
-	int (*aggregate)(struct icc_node *node, u32 avg_bw, u32 peak_bw,
-			 u32 *agg_avg, u32 *agg_peak);
+	int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
+			 u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
 	struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
 	struct device		*dev;
 	int			users;
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index dc25864755ba..d70a914cba11 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -30,6 +30,7 @@ struct icc_path *icc_get(struct device *dev, const int src_id,
 struct icc_path *of_icc_get(struct device *dev, const char *name);
 void icc_put(struct icc_path *path);
 int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
+void icc_set_tag(struct icc_path *path, u32 tag);
 
 #else
 
@@ -54,6 +55,10 @@ static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 	return 0;
 }
 
+static inline void icc_set_tag(struct icc_path *path, u32 tag)
+{
+}
+
 #endif /* CONFIG_INTERCONNECT */
 
 #endif /* __LINUX_INTERCONNECT_H */

[PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845

[Georgi Djakov]

From: David Dai <daidavid1@codeaurora.org>

Add support for wake and sleep commands by using a tag to indicate
whether or not the aggregate and set requests are active only or
dual context for a particular path.

Signed-off-by: David Dai <daidavid1@codeaurora.org>
Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++--------
 1 file changed, 75 insertions(+), 26 deletions(-)

diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
index fb526004c82e..13499f681160 100644
--- a/drivers/interconnect/qcom/sdm845.c
+++ b/drivers/interconnect/qcom/sdm845.c
@@ -65,6 +65,12 @@ struct bcm_db {
 #define SDM845_MAX_BCMS		30
 #define SDM845_MAX_BCM_PER_NODE	2
 #define SDM845_MAX_VCD		10
+#define SDM845_MAX_CTX		2
+#define SDM845_EE_STATE		2
+#define EE_STATE_WAKE		0
+#define EE_STATE_SLEEP		1
+#define AO_CTX			0
+#define DUAL_CTX		1
 
 /**
  * struct qcom_icc_node - Qualcomm specific interconnect nodes
@@ -86,8 +92,8 @@ struct qcom_icc_node {
 	u16 num_links;
 	u16 channels;
 	u16 buswidth;
-	u64 sum_avg;
-	u64 max_peak;
+	u64 sum_avg[SDM845_MAX_CTX];
+	u64 max_peak[SDM845_MAX_CTX];
 	struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
 	size_t num_bcms;
 };
@@ -112,8 +118,8 @@ struct qcom_icc_bcm {
 	const char *name;
 	u32 type;
 	u32 addr;
-	u64 vote_x;
-	u64 vote_y;
+	u64 vote_x[SDM845_EE_STATE];
+	u64 vote_y[SDM845_EE_STATE];
 	bool dirty;
 	bool keepalive;
 	struct bcm_db aux_data;
@@ -555,7 +561,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
 		cmd->wait = true;
 }
 
-static void tcs_list_gen(struct list_head *bcm_list,
+static void tcs_list_gen(struct list_head *bcm_list, int ee_state,
 			 struct tcs_cmd tcs_list[SDM845_MAX_VCD],
 			 int n[SDM845_MAX_VCD])
 {
@@ -573,8 +579,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
 			commit = true;
 			cur_vcd_size = 0;
 		}
-		tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
-			    bcm->addr, commit);
+		tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[ee_state],
+			    bcm->vote_y[ee_state], bcm->addr, commit);
 		idx++;
 		n[batch]++;
 		/*
@@ -595,32 +601,42 @@ static void tcs_list_gen(struct list_head *bcm_list,
 
 static void bcm_aggregate(struct qcom_icc_bcm *bcm)
 {
-	size_t i;
-	u64 agg_avg = 0;
-	u64 agg_peak = 0;
+	size_t i, ctx;
+	u64 agg_avg[SDM845_MAX_CTX] = {0};
+	u64 agg_peak[SDM845_MAX_CTX] = {0};
 	u64 temp;
 
-	for (i = 0; i < bcm->num_nodes; i++) {
-		temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
-		do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
-		agg_avg = max(agg_avg, temp);
+	for (ctx = 0; ctx < SDM845_MAX_CTX; ctx++) {
+		for (i = 0; i < bcm->num_nodes; i++) {
+			temp = bcm->nodes[i]->sum_avg[ctx] * bcm->aux_data.width;
+			do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
+			agg_avg[ctx] = max(agg_avg[ctx], temp);
 
-		temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
-		do_div(temp, bcm->nodes[i]->buswidth);
-		agg_peak = max(agg_peak, temp);
+			temp = bcm->nodes[i]->max_peak[ctx] * bcm->aux_data.width;
+			do_div(temp, bcm->nodes[i]->buswidth);
+			agg_peak[ctx] = max(agg_peak[ctx], temp);
+		}
 	}
 
-	temp = agg_avg * 1000ULL;
+	temp = agg_avg[AO_CTX] + agg_avg[DUAL_CTX] * 1000ULL;
+	do_div(temp, bcm->aux_data.unit);
+	bcm->vote_x[EE_STATE_WAKE] = temp;
+
+	temp = max(agg_peak[AO_CTX], agg_peak[DUAL_CTX]) * 1000ULL;
+	do_div(temp, bcm->aux_data.unit);
+	bcm->vote_y[EE_STATE_WAKE] = temp;
+
+	temp = agg_avg[DUAL_CTX] * 1000ULL;
 	do_div(temp, bcm->aux_data.unit);
-	bcm->vote_x = temp;
+	bcm->vote_x[EE_STATE_SLEEP] = temp;
 
-	temp = agg_peak * 1000ULL;
+	temp = agg_peak[DUAL_CTX] * 1000ULL;
 	do_div(temp, bcm->aux_data.unit);
-	bcm->vote_y = temp;
+	bcm->vote_y[EE_STATE_SLEEP] = temp;
 
 	if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
-		bcm->vote_x = 1;
-		bcm->vote_y = 1;
+		bcm->vote_x[EE_STATE_WAKE] = 1;
+		bcm->vote_y[EE_STATE_WAKE] = 1;
 	}
 
 	bcm->dirty = false;
@@ -631,14 +647,16 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 {
 	size_t i;
 	struct qcom_icc_node *qn;
+	u32 ctx = 0;
 
 	qn = node->data;
+	ctx = (!!tag) ? AO_CTX : DUAL_CTX;
 
 	*agg_avg += avg_bw;
 	*agg_peak = max_t(u32, *agg_peak, peak_bw);
 
-	qn->sum_avg = *agg_avg;
-	qn->max_peak = *agg_peak;
+	qn->sum_avg[ctx] = *agg_avg;
+	qn->max_peak[ctx] = *agg_peak;
 
 	for (i = 0; i < qn->num_bcms; i++)
 		qn->bcms[i]->dirty = true;
@@ -675,7 +693,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 	 * Construct the command list based on a pre ordered list of BCMs
 	 * based on VCD.
 	 */
-	tcs_list_gen(&commit_list, cmds, commit_idx);
+	tcs_list_gen(&commit_list, EE_STATE_WAKE, cmds, commit_idx);
 
 	if (!commit_idx[0])
 		return ret;
@@ -693,6 +711,37 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 		return ret;
 	}
 
+	INIT_LIST_HEAD(&commit_list);
+
+	for (i = 0; i < qp->num_bcms; i++) {
+		/* Only generate WAKE and SLEEP commands if a resource's
+		 * requirements change as the execution environment transitions
+		 * between different power states.
+		 */
+		if (qp->bcms[i]->vote_x[EE_STATE_WAKE] !=
+		    qp->bcms[i]->vote_x[EE_STATE_SLEEP] ||
+		    qp->bcms[i]->vote_y[EE_STATE_WAKE] !=
+		    qp->bcms[i]->vote_y[EE_STATE_SLEEP]) {
+			list_add_tail(&qp->bcms[i]->list, &commit_list);
+		}
+	}
+
+	tcs_list_gen(&commit_list, EE_STATE_WAKE, cmds, commit_idx);
+
+	ret = rpmh_write_batch(qp->dev, RPMH_WAKE_ONLY_STATE, cmds, commit_idx);
+	if (ret) {
+		dev_err(qp->dev, "Error sending WAKE RPMH requests: %d\n", ret);
+		return ret;
+	}
+
+	tcs_list_gen(&commit_list, EE_STATE_SLEEP, cmds, commit_idx);
+
+	ret = rpmh_write_batch(qp->dev, RPMH_SLEEP_STATE, cmds, commit_idx);
+	if (ret) {
+		dev_err(qp->dev, "Error sending SLEEP RPMH requests: %d\n", ret);
+		return ret;
+	}
+
 	return ret;
 }
 

Re: [PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845

[Evan Green]

On Fri, Feb 8, 2019 at 9:22 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>
> From: David Dai <daidavid1@codeaurora.org>
>
> Add support for wake and sleep commands by using a tag to indicate
> whether or not the aggregate and set requests are active only or
> dual context for a particular path.
>
> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++--------
>  1 file changed, 75 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
> index fb526004c82e..13499f681160 100644
> --- a/drivers/interconnect/qcom/sdm845.c
> +++ b/drivers/interconnect/qcom/sdm845.c
> @@ -65,6 +65,12 @@ struct bcm_db {
>  #define SDM845_MAX_BCMS                30
>  #define SDM845_MAX_BCM_PER_NODE        2
>  #define SDM845_MAX_VCD         10
> +#define SDM845_MAX_CTX         2
> +#define SDM845_EE_STATE                2
> +#define EE_STATE_WAKE          0
> +#define EE_STATE_SLEEP         1
> +#define AO_CTX                 0
> +#define DUAL_CTX               1
>

I get really lost with these two sets of numbers here. I think this
needs some explanation in the comments. From staring at this what I've
understood so far is:
1) Clients pass in a tag that buckets their requests into either AO or
DUAL within the node. (Although, the requests all seem to get
aggregated together no matter what the tag is, more on that later).
2) During icc_set(), we go through the many-to-many mapping of BCMs to
nodes. For each BCM, we aggregate all the nodes it has, bucketed again
by AO or DUAL.
3) The actual votes sent to RPMh are in terms of WAKE and SLEEP.
4) The mapping from AO/DUAL to WAKE/SLEEP for a particular BCM is:
WAKE = AO+DUAL, SLEEP=DUAL.
5) qcom_icc_set() sends EE_STATE_WAKE stuff to RPMH_ACTIVE_ONLY_STATE.
6) If there's any difference between SLEEP and WAKE, then the
EE_STATE_WAKE commands are gathered together and sent to
RPMH_WAKE_ONLY_STATE, and all the EE_STATE_SLEEP commands are sent to
RPMH_SLEEP_STATE.

So ultimately the muxing ends up like
RPMH_ACTIVE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL
RPMH_SLEEP_STATE <-- EE_STATE_SLEEP <-- DUAL
RPMH_WAKE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL

Why do we need this complicated muxing to happen? Is it because we're
trying to avoid requiring drivers to specify three different paths in
the simple case, when all they care about is "use this bandwidth all
the time"?

What I think would make more sense would be to use the "tag" as a
bitfield instead. So you'd have

#define QCOM_ICC_TAG_ACTIVE_ONLY 0x00000001
#define QCOM_ICC_TAG_SLEEP 0x00000002
#define QCOM_ICC_TAG_WAKE 0x00000004
#define QCOM_ICC_TAG_ALL_TIMES (QCOM_ICC_TAG_ACTIVE_ONLY |
QCOM_ICC_TAG_SLEEP | QCOM_ICC_TAG_WAKE)

Drivers that don't care about sleep/wake sets would just not set a
tag, or set a tag of QCOM_ICC_TAG_ALL_TIMES. Then in
qcom_icc_aggregate(), you aggregate the same values up to three times,
one for each bit they have set. Finally in qcom_icc_set, you can pass
the votes directly down in their buckets, without doing a weird
mapping from AO/DUAL to SLEEP/WAKE/ACTIVE_ONLY.

The sticky part about this is that now rather than one set of sum/peak
values, there are now three independent ones, corresponding to each of
the tag bits. It seems like the interconnect core wouldn't want to
mandate whether providers use the tag as a bitfield or a value. So the
provider would need to keep not only the official set of aggregates
that are returned back to the core (probably ACTIVE_ONLY, or maybe a
max of the three), but also the two shadow copies internally for SLEEP
and WAKE.

If you organized the tag that way, the only extra thing you'd need is
a callback to the provider just before the core starts aggregation, so
that the provider can reset the shadow buckets to 0 the same way the
core resets the main sum/peak to 0 before looping.With this, we've
plumbed out the full abilities of RPMh to the client drivers, but
without complicating the simple case of "set this bandwidth all the
time". What do you think?

>  /**
>   * struct qcom_icc_node - Qualcomm specific interconnect nodes
> @@ -86,8 +92,8 @@ struct qcom_icc_node {
>         u16 num_links;
>         u16 channels;
>         u16 buswidth;
> -       u64 sum_avg;
> -       u64 max_peak;
> +       u64 sum_avg[SDM845_MAX_CTX];
> +       u64 max_peak[SDM845_MAX_CTX];
>         struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
>         size_t num_bcms;
>  };
> @@ -112,8 +118,8 @@ struct qcom_icc_bcm {
>         const char *name;
>         u32 type;
>         u32 addr;
> -       u64 vote_x;
> -       u64 vote_y;
> +       u64 vote_x[SDM845_EE_STATE];
> +       u64 vote_y[SDM845_EE_STATE];
>         bool dirty;
>         bool keepalive;
>         struct bcm_db aux_data;
> @@ -555,7 +561,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
>                 cmd->wait = true;
>  }
>
> -static void tcs_list_gen(struct list_head *bcm_list,
> +static void tcs_list_gen(struct list_head *bcm_list, int ee_state,
>                          struct tcs_cmd tcs_list[SDM845_MAX_VCD],
>                          int n[SDM845_MAX_VCD])
>  {
> @@ -573,8 +579,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
>                         commit = true;
>                         cur_vcd_size = 0;
>                 }
> -               tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
> -                           bcm->addr, commit);
> +               tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[ee_state],
> +                           bcm->vote_y[ee_state], bcm->addr, commit);
>                 idx++;
>                 n[batch]++;
>                 /*
> @@ -595,32 +601,42 @@ static void tcs_list_gen(struct list_head *bcm_list,
>
>  static void bcm_aggregate(struct qcom_icc_bcm *bcm)
>  {
> -       size_t i;
> -       u64 agg_avg = 0;
> -       u64 agg_peak = 0;
> +       size_t i, ctx;
> +       u64 agg_avg[SDM845_MAX_CTX] = {0};
> +       u64 agg_peak[SDM845_MAX_CTX] = {0};
>         u64 temp;
>
> -       for (i = 0; i < bcm->num_nodes; i++) {
> -               temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
> -               do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> -               agg_avg = max(agg_avg, temp);
> +       for (ctx = 0; ctx < SDM845_MAX_CTX; ctx++) {
> +               for (i = 0; i < bcm->num_nodes; i++) {
> +                       temp = bcm->nodes[i]->sum_avg[ctx] * bcm->aux_data.width;
> +                       do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
> +                       agg_avg[ctx] = max(agg_avg[ctx], temp);
>
> -               temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
> -               do_div(temp, bcm->nodes[i]->buswidth);
> -               agg_peak = max(agg_peak, temp);
> +                       temp = bcm->nodes[i]->max_peak[ctx] * bcm->aux_data.width;
> +                       do_div(temp, bcm->nodes[i]->buswidth);
> +                       agg_peak[ctx] = max(agg_peak[ctx], temp);
> +               }
>         }
>
> -       temp = agg_avg * 1000ULL;
> +       temp = agg_avg[AO_CTX] + agg_avg[DUAL_CTX] * 1000ULL;
> +       do_div(temp, bcm->aux_data.unit);
> +       bcm->vote_x[EE_STATE_WAKE] = temp;
> +
> +       temp = max(agg_peak[AO_CTX], agg_peak[DUAL_CTX]) * 1000ULL;
> +       do_div(temp, bcm->aux_data.unit);
> +       bcm->vote_y[EE_STATE_WAKE] = temp;
> +
> +       temp = agg_avg[DUAL_CTX] * 1000ULL;
>         do_div(temp, bcm->aux_data.unit);
> -       bcm->vote_x = temp;
> +       bcm->vote_x[EE_STATE_SLEEP] = temp;
>
> -       temp = agg_peak * 1000ULL;
> +       temp = agg_peak[DUAL_CTX] * 1000ULL;
>         do_div(temp, bcm->aux_data.unit);
> -       bcm->vote_y = temp;
> +       bcm->vote_y[EE_STATE_SLEEP] = temp;
>
>         if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
> -               bcm->vote_x = 1;
> -               bcm->vote_y = 1;
> +               bcm->vote_x[EE_STATE_WAKE] = 1;
> +               bcm->vote_y[EE_STATE_WAKE] = 1;
>         }
>
>         bcm->dirty = false;
> @@ -631,14 +647,16 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>  {
>         size_t i;
>         struct qcom_icc_node *qn;
> +       u32 ctx = 0;
>
>         qn = node->data;
> +       ctx = (!!tag) ? AO_CTX : DUAL_CTX;
>
>         *agg_avg += avg_bw;
>         *agg_peak = max_t(u32, *agg_peak, peak_bw);
>
> -       qn->sum_avg = *agg_avg;
> -       qn->max_peak = *agg_peak;
> +       qn->sum_avg[ctx] = *agg_avg;
> +       qn->max_peak[ctx] = *agg_peak;

I'm confused that you seem to aggregate agg_avg and agg_peak
regardless of what the tag is, but then save them into buckets as if
they had been kept separate. Doesn't this just end up as a mish-mash
of all tag aggregates, but whose final value is dependent on the order
of requests? (Said another way, what gets saved into each [ctx] bucket
is the the aggregate of all requests regardless of tag, but only up
until the last request of that tag type was seen.)

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

[Evan Green]

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);

>
>         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?

>
>         return 0;
> @@ -396,6 +398,23 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
>  }
>  EXPORT_SYMBOL_GPL(of_icc_get);
>

Re: [PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845

[David Dai]

On 3/8/2019 10:35 AM, Evan Green wrote:
> On Fri, Feb 8, 2019 at 9:22 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> From: David Dai <daidavid1@codeaurora.org>
>>
>> Add support for wake and sleep commands by using a tag to indicate
>> whether or not the aggregate and set requests are active only or
>> dual context for a particular path.
>>
>> Signed-off-by: David Dai <daidavid1@codeaurora.org>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>   drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++--------
>>   1 file changed, 75 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
>> index fb526004c82e..13499f681160 100644
>> --- a/drivers/interconnect/qcom/sdm845.c
>> +++ b/drivers/interconnect/qcom/sdm845.c
>> @@ -65,6 +65,12 @@ struct bcm_db {
>>   #define SDM845_MAX_BCMS                30
>>   #define SDM845_MAX_BCM_PER_NODE        2
>>   #define SDM845_MAX_VCD         10
>> +#define SDM845_MAX_CTX         2
>> +#define SDM845_EE_STATE                2
>> +#define EE_STATE_WAKE          0
>> +#define EE_STATE_SLEEP         1
>> +#define AO_CTX                 0
>> +#define DUAL_CTX               1
>>
> I get really lost with these two sets of numbers here. I think this
> needs some explanation in the comments. From staring at this what I've
> understood so far is:
> 1) Clients pass in a tag that buckets their requests into either AO or
> DUAL within the node. (Although, the requests all seem to get
> aggregated together no matter what the tag is, more on that later).
> 2) During icc_set(), we go through the many-to-many mapping of BCMs to
> nodes. For each BCM, we aggregate all the nodes it has, bucketed again
> by AO or DUAL.
> 3) The actual votes sent to RPMh are in terms of WAKE and SLEEP.
> 4) The mapping from AO/DUAL to WAKE/SLEEP for a particular BCM is:
> WAKE = AO+DUAL, SLEEP=DUAL.
> 5) qcom_icc_set() sends EE_STATE_WAKE stuff to RPMH_ACTIVE_ONLY_STATE.
> 6) If there's any difference between SLEEP and WAKE, then the
> EE_STATE_WAKE commands are gathered together and sent to
> RPMH_WAKE_ONLY_STATE, and all the EE_STATE_SLEEP commands are sent to
> RPMH_SLEEP_STATE.
>
> So ultimately the muxing ends up like
> RPMH_ACTIVE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL
> RPMH_SLEEP_STATE <-- EE_STATE_SLEEP <-- DUAL
> RPMH_WAKE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL
This muxing explanation is correct, there's also an inherent limitation 
in this muxing where RPMH_ACTIVE cannot differ from RPMH_WAKE.
>
> Why do we need this complicated muxing to happen? Is it because we're
> trying to avoid requiring drivers to specify three different paths in
> the simple case, when all they care about is "use this bandwidth all
> the time"?
That's a part of it, I don't have strong justification for this legacy 
way of supporting wake/sleep, so I'm open to new suggestions.
> What I think would make more sense would be to use the "tag" as a
> bitfield instead. So you'd have
>
> #define QCOM_ICC_TAG_ACTIVE_ONLY 0x00000001
> #define QCOM_ICC_TAG_SLEEP 0x00000002
> #define QCOM_ICC_TAG_WAKE 0x00000004
> #define QCOM_ICC_TAG_ALL_TIMES (QCOM_ICC_TAG_ACTIVE_ONLY |
> QCOM_ICC_TAG_SLEEP | QCOM_ICC_TAG_WAKE)
>
> Drivers that don't care about sleep/wake sets would just not set a
> tag, or set a tag of QCOM_ICC_TAG_ALL_TIMES. Then in
> qcom_icc_aggregate(), you aggregate the same values up to three times,
> one for each bit they have set. Finally in qcom_icc_set, you can pass
> the votes directly down in their buckets, without doing a weird
> mapping from AO/DUAL to SLEEP/WAKE/ACTIVE_ONLY.
Works for me, I'd still want to optimize out the WAKE/SLEEP commands 
that are equal to each other though.
> The sticky part about this is that now rather than one set of sum/peak
> values, there are now three independent ones, corresponding to each of
> the tag bits. It seems like the interconnect core wouldn't want to
> mandate whether providers use the tag as a bitfield or a value. So the
> provider would need to keep not only the official set of aggregates
> that are returned back to the core (probably ACTIVE_ONLY, or maybe a
> max of the three), but also the two shadow copies internally for SLEEP
> and WAKE.
I would ideally like to return the most accurate current state of the 
system which may be ACTIVE_ONLY or WAKE. We might also run into some 
confusion when printing out the summaries for each node. In a case where 
the votes for WAKE != ACTIVE(assuming one or more multiple consumers are 
intentionally doing this with multiple paths), the state of system will 
change depending on whether or not we've completed a sleep cycle(If it's 
prior to sleep, the ACTIVE_ONLY vote will be accurate, and post waking 
up the WAKE request will be correct) but this is more of a book keeping 
concern than anything else, as long as consumers understand that this is 
the expected behavior. On a side note, I don't like the ACTIVE_ONLY name 
designation for this which was brought over from the rpmh driver, it 
really means let's set the current state immediately and it has no 
association with the execution environment state unlike the other 
WAKE/SLEEP types.
>
> If you organized the tag that way, the only extra thing you'd need is
> a callback to the provider just before the core starts aggregation, so
> that the provider can reset the shadow buckets to 0 the same way the
> core resets the main sum/peak to 0 before looping.With this, we've
> plumbed out the full abilities of RPMh to the client drivers, but
> without complicating the simple case of "set this bandwidth all the
> time". What do you think?

I really like this idea for the most part, it certainly simplifies some 
of the muxing that happens underneath the layer and makes the 
aggregation to the RPMH driver a lot more transparent. Though we should 
probably just reset the shadow buckets to 0 at the end of a call into 
provider icc_set as part of cleaning up the state after we've committed 
some requests into hardware? I don't see a need to have to add another 
call back into the provider before aggregation.

>
>>   /**
>>    * struct qcom_icc_node - Qualcomm specific interconnect nodes
>> @@ -86,8 +92,8 @@ struct qcom_icc_node {
>>          u16 num_links;
>>          u16 channels;
>>          u16 buswidth;
>> -       u64 sum_avg;
>> -       u64 max_peak;
>> +       u64 sum_avg[SDM845_MAX_CTX];
>> +       u64 max_peak[SDM845_MAX_CTX];
>>          struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
>>          size_t num_bcms;
>>   };
>> @@ -112,8 +118,8 @@ struct qcom_icc_bcm {
>>          const char *name;
>>          u32 type;
>>          u32 addr;
>> -       u64 vote_x;
>> -       u64 vote_y;
>> +       u64 vote_x[SDM845_EE_STATE];
>> +       u64 vote_y[SDM845_EE_STATE];
>>          bool dirty;
>>          bool keepalive;
>>          struct bcm_db aux_data;
>> @@ -555,7 +561,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
>>                  cmd->wait = true;
>>   }
>>
>> -static void tcs_list_gen(struct list_head *bcm_list,
>> +static void tcs_list_gen(struct list_head *bcm_list, int ee_state,
>>                           struct tcs_cmd tcs_list[SDM845_MAX_VCD],
>>                           int n[SDM845_MAX_VCD])
>>   {
>> @@ -573,8 +579,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
>>                          commit = true;
>>                          cur_vcd_size = 0;
>>                  }
>> -               tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
>> -                           bcm->addr, commit);
>> +               tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[ee_state],
>> +                           bcm->vote_y[ee_state], bcm->addr, commit);
>>                  idx++;
>>                  n[batch]++;
>>                  /*
>> @@ -595,32 +601,42 @@ static void tcs_list_gen(struct list_head *bcm_list,
>>
>>   static void bcm_aggregate(struct qcom_icc_bcm *bcm)
>>   {
>> -       size_t i;
>> -       u64 agg_avg = 0;
>> -       u64 agg_peak = 0;
>> +       size_t i, ctx;
>> +       u64 agg_avg[SDM845_MAX_CTX] = {0};
>> +       u64 agg_peak[SDM845_MAX_CTX] = {0};
>>          u64 temp;
>>
>> -       for (i = 0; i < bcm->num_nodes; i++) {
>> -               temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
>> -               do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
>> -               agg_avg = max(agg_avg, temp);
>> +       for (ctx = 0; ctx < SDM845_MAX_CTX; ctx++) {
>> +               for (i = 0; i < bcm->num_nodes; i++) {
>> +                       temp = bcm->nodes[i]->sum_avg[ctx] * bcm->aux_data.width;
>> +                       do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
>> +                       agg_avg[ctx] = max(agg_avg[ctx], temp);
>>
>> -               temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
>> -               do_div(temp, bcm->nodes[i]->buswidth);
>> -               agg_peak = max(agg_peak, temp);
>> +                       temp = bcm->nodes[i]->max_peak[ctx] * bcm->aux_data.width;
>> +                       do_div(temp, bcm->nodes[i]->buswidth);
>> +                       agg_peak[ctx] = max(agg_peak[ctx], temp);
>> +               }
>>          }
>>
>> -       temp = agg_avg * 1000ULL;
>> +       temp = agg_avg[AO_CTX] + agg_avg[DUAL_CTX] * 1000ULL;
>> +       do_div(temp, bcm->aux_data.unit);
>> +       bcm->vote_x[EE_STATE_WAKE] = temp;
>> +
>> +       temp = max(agg_peak[AO_CTX], agg_peak[DUAL_CTX]) * 1000ULL;
>> +       do_div(temp, bcm->aux_data.unit);
>> +       bcm->vote_y[EE_STATE_WAKE] = temp;
>> +
>> +       temp = agg_avg[DUAL_CTX] * 1000ULL;
>>          do_div(temp, bcm->aux_data.unit);
>> -       bcm->vote_x = temp;
>> +       bcm->vote_x[EE_STATE_SLEEP] = temp;
>>
>> -       temp = agg_peak * 1000ULL;
>> +       temp = agg_peak[DUAL_CTX] * 1000ULL;
>>          do_div(temp, bcm->aux_data.unit);
>> -       bcm->vote_y = temp;
>> +       bcm->vote_y[EE_STATE_SLEEP] = temp;
>>
>>          if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
>> -               bcm->vote_x = 1;
>> -               bcm->vote_y = 1;
>> +               bcm->vote_x[EE_STATE_WAKE] = 1;
>> +               bcm->vote_y[EE_STATE_WAKE] = 1;
>>          }
>>
>>          bcm->dirty = false;
>> @@ -631,14 +647,16 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>>   {
>>          size_t i;
>>          struct qcom_icc_node *qn;
>> +       u32 ctx = 0;
>>
>>          qn = node->data;
>> +       ctx = (!!tag) ? AO_CTX : DUAL_CTX;
>>
>>          *agg_avg += avg_bw;
>>          *agg_peak = max_t(u32, *agg_peak, peak_bw);
>>
>> -       qn->sum_avg = *agg_avg;
>> -       qn->max_peak = *agg_peak;
>> +       qn->sum_avg[ctx] = *agg_avg;
>> +       qn->max_peak[ctx] = *agg_peak;
> I'm confused that you seem to aggregate agg_avg and agg_peak
> regardless of what the tag is, but then save them into buckets as if
> they had been kept separate. Doesn't this just end up as a mish-mash
> of all tag aggregates, but whose final value is dependent on the order
> of requests? (Said another way, what gets saved into each [ctx] bucket
> is the the aggregate of all requests regardless of tag, but only up
> until the last request of that tag type was seen.)
This was a mistake, the buckets should only aggregate among themselves, 
which also needs to cleaned up before the next set of aggregations.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845

[Evan Green]

On Wed, Mar 13, 2019 at 6:00 PM David Dai <daidavid1@codeaurora.org> wrote:
>
>
> On 3/8/2019 10:35 AM, Evan Green wrote:
> > On Fri, Feb 8, 2019 at 9:22 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
> >> From: David Dai <daidavid1@codeaurora.org>
> >>
> >> Add support for wake and sleep commands by using a tag to indicate
> >> whether or not the aggregate and set requests are active only or
> >> dual context for a particular path.
> >>
> >> Signed-off-by: David Dai <daidavid1@codeaurora.org>
> >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> >> ---
> >>   drivers/interconnect/qcom/sdm845.c | 101 +++++++++++++++++++++--------
> >>   1 file changed, 75 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
> >> index fb526004c82e..13499f681160 100644
> >> --- a/drivers/interconnect/qcom/sdm845.c
> >> +++ b/drivers/interconnect/qcom/sdm845.c
> >> @@ -65,6 +65,12 @@ struct bcm_db {
> >>   #define SDM845_MAX_BCMS                30
> >>   #define SDM845_MAX_BCM_PER_NODE        2
> >>   #define SDM845_MAX_VCD         10
> >> +#define SDM845_MAX_CTX         2
> >> +#define SDM845_EE_STATE                2
> >> +#define EE_STATE_WAKE          0
> >> +#define EE_STATE_SLEEP         1
> >> +#define AO_CTX                 0
> >> +#define DUAL_CTX               1
> >>
> > I get really lost with these two sets of numbers here. I think this
> > needs some explanation in the comments. From staring at this what I've
> > understood so far is:
> > 1) Clients pass in a tag that buckets their requests into either AO or
> > DUAL within the node. (Although, the requests all seem to get
> > aggregated together no matter what the tag is, more on that later).
> > 2) During icc_set(), we go through the many-to-many mapping of BCMs to
> > nodes. For each BCM, we aggregate all the nodes it has, bucketed again
> > by AO or DUAL.
> > 3) The actual votes sent to RPMh are in terms of WAKE and SLEEP.
> > 4) The mapping from AO/DUAL to WAKE/SLEEP for a particular BCM is:
> > WAKE = AO+DUAL, SLEEP=DUAL.
> > 5) qcom_icc_set() sends EE_STATE_WAKE stuff to RPMH_ACTIVE_ONLY_STATE.
> > 6) If there's any difference between SLEEP and WAKE, then the
> > EE_STATE_WAKE commands are gathered together and sent to
> > RPMH_WAKE_ONLY_STATE, and all the EE_STATE_SLEEP commands are sent to
> > RPMH_SLEEP_STATE.
> >
> > So ultimately the muxing ends up like
> > RPMH_ACTIVE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL
> > RPMH_SLEEP_STATE <-- EE_STATE_SLEEP <-- DUAL
> > RPMH_WAKE_ONLY_STATE <-- EE_STATE_WAKE <-- AO+DUAL
> This muxing explanation is correct, there's also an inherent limitation
> in this muxing where RPMH_ACTIVE cannot differ from RPMH_WAKE.
> >
> > Why do we need this complicated muxing to happen? Is it because we're
> > trying to avoid requiring drivers to specify three different paths in
> > the simple case, when all they care about is "use this bandwidth all
> > the time"?
> That's a part of it, I don't have strong justification for this legacy
> way of supporting wake/sleep, so I'm open to new suggestions.
> > What I think would make more sense would be to use the "tag" as a
> > bitfield instead. So you'd have
> >
> > #define QCOM_ICC_TAG_ACTIVE_ONLY 0x00000001
> > #define QCOM_ICC_TAG_SLEEP 0x00000002
> > #define QCOM_ICC_TAG_WAKE 0x00000004
> > #define QCOM_ICC_TAG_ALL_TIMES (QCOM_ICC_TAG_ACTIVE_ONLY |
> > QCOM_ICC_TAG_SLEEP | QCOM_ICC_TAG_WAKE)
> >
> > Drivers that don't care about sleep/wake sets would just not set a
> > tag, or set a tag of QCOM_ICC_TAG_ALL_TIMES. Then in
> > qcom_icc_aggregate(), you aggregate the same values up to three times,
> > one for each bit they have set. Finally in qcom_icc_set, you can pass
> > the votes directly down in their buckets, without doing a weird
> > mapping from AO/DUAL to SLEEP/WAKE/ACTIVE_ONLY.
> Works for me, I'd still want to optimize out the WAKE/SLEEP commands
> that are equal to each other though.

Yeah, that makes sense.

> > The sticky part about this is that now rather than one set of sum/peak
> > values, there are now three independent ones, corresponding to each of
> > the tag bits. It seems like the interconnect core wouldn't want to
> > mandate whether providers use the tag as a bitfield or a value. So the
> > provider would need to keep not only the official set of aggregates
> > that are returned back to the core (probably ACTIVE_ONLY, or maybe a
> > max of the three), but also the two shadow copies internally for SLEEP
> > and WAKE.
> I would ideally like to return the most accurate current state of the
> system which may be ACTIVE_ONLY or WAKE. We might also run into some
> confusion when printing out the summaries for each node. In a case where
> the votes for WAKE != ACTIVE(assuming one or more multiple consumers are
> intentionally doing this with multiple paths), the state of system will
> change depending on whether or not we've completed a sleep cycle(If it's
> prior to sleep, the ACTIVE_ONLY vote will be accurate, and post waking
> up the WAKE request will be correct) but this is more of a book keeping
> concern than anything else, as long as consumers understand that this is

Yeah, I thought about that too and agree it would be ideal if the
framework could report the actual state, rather than a guess. Maybe
someday when we have a clearer picture of what sort of automatic
switching other SoCs are capable of doing we could try to enlighten
the framework about these dynamic changes.

I had suggested reporting the max of the three buckets back to the
framework since I'm guessing most of the debugging will come down to
votes accidentally left on, so seeing an on vote might be helpful. But
then I guess that masks issues with votes not being set high enough,
or not being high enough after a resume... sigh.

> the expected behavior. On a side note, I don't like the ACTIVE_ONLY name
> designation for this which was brought over from the rpmh driver, it
> really means let's set the current state immediately and it has no
> association with the execution environment state unlike the other
> WAKE/SLEEP types.

I agree, it's very confusing. I'd be okay with another define like
SET_NOW whose value defines to ACTIVE_ONLY.

> >
> > If you organized the tag that way, the only extra thing you'd need is
> > a callback to the provider just before the core starts aggregation, so
> > that the provider can reset the shadow buckets to 0 the same way the
> > core resets the main sum/peak to 0 before looping.With this, we've
> > plumbed out the full abilities of RPMh to the client drivers, but
> > without complicating the simple case of "set this bandwidth all the
> > time". What do you think?
>
> I really like this idea for the most part, it certainly simplifies some
> of the muxing that happens underneath the layer and makes the
> aggregation to the RPMH driver a lot more transparent. Though we should
> probably just reset the shadow buckets to 0 at the end of a call into
> provider icc_set as part of cleaning up the state after we've committed
> some requests into hardware? I don't see a need to have to add another
> call back into the provider before aggregation.

Sure, that would work too. It bakes in a little bit of assumption
about how the framework is going to call the callbacks (ie it assumes
we will always do a set() following aggregate()), so it would be nice
to at least call that out in a comment when you aggregate in the
buckets.

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

[Georgi Djakov]

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);
>>