linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] interconnect: Fix sync-state issues
@ 2021-06-25 21:28 Mike Tipton
  2021-06-25 21:28 ` [PATCH 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Mike Tipton @ 2021-06-25 21:28 UTC (permalink / raw)
  To: djakov
  Cc: bjorn.andersson, agross, saravanak, okukatla, linux-arm-msm,
	linux-pm, linux-kernel, Mike Tipton

These patches fix a couple of sync-state bugs that either cause the initial BW
floors to be ignored entirely, or to be never removed after sync-state is
called.

Mike Tipton (4):
  interconnect: Zero initial BW after sync-state
  interconnect: Always call pre_aggregate before aggregate
  interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes
  interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate

 drivers/interconnect/core.c          |  7 ++++++-
 drivers/interconnect/qcom/icc-rpmh.c | 20 ++++++++++----------
 2 files changed, 16 insertions(+), 11 deletions(-)

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH 1/4] interconnect: Zero initial BW after sync-state
  2021-06-25 21:28 [PATCH 0/4] interconnect: Fix sync-state issues Mike Tipton
@ 2021-06-25 21:28 ` Mike Tipton
  2021-07-01 16:56   ` okukatla
  2021-06-25 21:28 ` [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate Mike Tipton
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Mike Tipton @ 2021-06-25 21:28 UTC (permalink / raw)
  To: djakov
  Cc: bjorn.andersson, agross, saravanak, okukatla, linux-arm-msm,
	linux-pm, linux-kernel, Mike Tipton

The initial BW values may be used by providers to enforce floors. Zero
these values after sync-state so that providers know when to stop
enforcing them.

Fixes: b1d681d8d324 ("interconnect: Add sync state support")
Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
---
 drivers/interconnect/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 8a1e70e00876..945121e18b5c 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1106,6 +1106,8 @@ void icc_sync_state(struct device *dev)
 		dev_dbg(p->dev, "interconnect provider is in synced state\n");
 		list_for_each_entry(n, &p->nodes, node_list) {
 			if (n->init_avg || n->init_peak) {
+				n->init_avg = 0;
+				n->init_peak = 0;
 				aggregate_requests(n);
 				p->set(n, n);
 			}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate
  2021-06-25 21:28 [PATCH 0/4] interconnect: Fix sync-state issues Mike Tipton
  2021-06-25 21:28 ` [PATCH 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
@ 2021-06-25 21:28 ` Mike Tipton
  2021-07-01 18:55   ` okukatla
  2021-06-25 21:28 ` [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
  2021-06-25 21:28 ` [PATCH 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate Mike Tipton
  3 siblings, 1 reply; 12+ messages in thread
From: Mike Tipton @ 2021-06-25 21:28 UTC (permalink / raw)
  To: djakov
  Cc: bjorn.andersson, agross, saravanak, okukatla, linux-arm-msm,
	linux-pm, linux-kernel, Mike Tipton

The pre_aggregate callback isn't called in all cases before calling
aggregate. Add the missing calls so providers can rely on consistent
framework behavior.

Fixes: d3703b3e255f ("interconnect: Aggregate before setting initial bandwidth")
Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
---
 drivers/interconnect/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 945121e18b5c..cfd54c90a6bb 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -973,9 +973,12 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
 	}
 	node->avg_bw = node->init_avg;
 	node->peak_bw = node->init_peak;
-	if (provider->aggregate)
+	if (provider->aggregate) {
+		if (provider->pre_aggregate)
+			provider->pre_aggregate(node);
 		provider->aggregate(node, 0, node->init_avg, node->init_peak,
 				    &node->avg_bw, &node->peak_bw);
+	}
 	provider->set(node, node);
 	node->avg_bw = 0;
 	node->peak_bw = 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes
  2021-06-25 21:28 [PATCH 0/4] interconnect: Fix sync-state issues Mike Tipton
  2021-06-25 21:28 ` [PATCH 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
  2021-06-25 21:28 ` [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate Mike Tipton
@ 2021-06-25 21:28 ` Mike Tipton
  2021-06-26  2:27   ` kernel test robot
  2021-07-01 18:48   ` okukatla
  2021-06-25 21:28 ` [PATCH 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate Mike Tipton
  3 siblings, 2 replies; 12+ messages in thread
From: Mike Tipton @ 2021-06-25 21:28 UTC (permalink / raw)
  To: djakov
  Cc: bjorn.andersson, agross, saravanak, okukatla, linux-arm-msm,
	linux-pm, linux-kernel, Mike Tipton

We currently only enforce BW floors for a subset of nodes in a path.
All BCMs that need updating are queued in the pre_aggregate/aggregate
phase. The first set() commits all queued BCMs and subsequent set()
calls short-circuit without committing anything. Since the floor BW
isn't set in sum_avg/max_peak until set(), then some BCMs are committed
before their associated nodes reflect the floor.

Set the floor as each node is being aggregated. This ensures that all
all relevant floors are set before the BCMs are committed.

Fixes: 266cd33b5913 ("interconnect: qcom: Ensure that the floor bandwidth value is enforced")
Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
---
 drivers/interconnect/qcom/icc-rpmh.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index bf01d09dba6c..f118f57eae37 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -57,6 +57,11 @@ int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 			qn->sum_avg[i] += avg_bw;
 			qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
 		}
+
+		if (node->init_avg || node->init_peak) {
+			qn->sum_avg[i] = max_t(u64, qn->sum_avg[i], node->init_avg);
+			qn->max_peak[i] = max_t(u64, qn->max_peak[i], node->init_peak);
+		}
 	}
 
 	*agg_avg += avg_bw;
@@ -90,11 +95,6 @@ int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
 	qp = to_qcom_provider(node->provider);
 	qn = node->data;
 
-	qn->sum_avg[QCOM_ICC_BUCKET_AMC] = max_t(u64, qn->sum_avg[QCOM_ICC_BUCKET_AMC],
-						 node->avg_bw);
-	qn->max_peak[QCOM_ICC_BUCKET_AMC] = max_t(u64, qn->max_peak[QCOM_ICC_BUCKET_AMC],
-						  node->peak_bw);
-
 	qcom_icc_bcm_voter_commit(qp->voter);
 
 	return 0;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate
  2021-06-25 21:28 [PATCH 0/4] interconnect: Fix sync-state issues Mike Tipton
                   ` (2 preceding siblings ...)
  2021-06-25 21:28 ` [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
@ 2021-06-25 21:28 ` Mike Tipton
  3 siblings, 0 replies; 12+ messages in thread
From: Mike Tipton @ 2021-06-25 21:28 UTC (permalink / raw)
  To: djakov
  Cc: bjorn.andersson, agross, saravanak, okukatla, linux-arm-msm,
	linux-pm, linux-kernel, Mike Tipton

We're only adding BCMs to the commit list in aggregate(), but there are
cases where pre_aggregate() is called without subsequently calling
aggregate(). In particular, in icc_sync_state() when a node with initial
BW has zero requests. Since BCMs aren't added to the commit list in
these cases, we don't actually send the zero BW request to HW. So the
resources remain on unnecessarily.

Add BCMs to the commit list in pre_aggregate() instead, which is always
called even when there are no requests.

Fixes: 976daac4a1c5 ("interconnect: qcom: Consolidate interconnect RPMh support")
Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
---
 drivers/interconnect/qcom/icc-rpmh.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/interconnect/qcom/icc-rpmh.c b/drivers/interconnect/qcom/icc-rpmh.c
index f118f57eae37..b26fda0588e0 100644
--- a/drivers/interconnect/qcom/icc-rpmh.c
+++ b/drivers/interconnect/qcom/icc-rpmh.c
@@ -20,13 +20,18 @@ void qcom_icc_pre_aggregate(struct icc_node *node)
 {
 	size_t i;
 	struct qcom_icc_node *qn;
+	struct qcom_icc_provider *qp;
 
 	qn = node->data;
+	qp = to_qcom_provider(node->provider);
 
 	for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
 		qn->sum_avg[i] = 0;
 		qn->max_peak[i] = 0;
 	}
+
+	for (i = 0; i < qn->num_bcms; i++)
+		qcom_icc_bcm_voter_add(qp->voter, qn->bcms[i]);
 }
 EXPORT_SYMBOL_GPL(qcom_icc_pre_aggregate);
 
@@ -44,10 +49,8 @@ int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 {
 	size_t i;
 	struct qcom_icc_node *qn;
-	struct qcom_icc_provider *qp;
 
 	qn = node->data;
-	qp = to_qcom_provider(node->provider);
 
 	if (!tag)
 		tag = QCOM_ICC_TAG_ALWAYS;
@@ -67,9 +70,6 @@ int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
 	*agg_avg += avg_bw;
 	*agg_peak = max_t(u32, *agg_peak, peak_bw);
 
-	for (i = 0; i < qn->num_bcms; i++)
-		qcom_icc_bcm_voter_add(qp->voter, qn->bcms[i]);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(qcom_icc_aggregate);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes
  2021-06-25 21:28 ` [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
@ 2021-06-26  2:27   ` kernel test robot
  2021-07-01 18:48   ` okukatla
  1 sibling, 0 replies; 12+ messages in thread
From: kernel test robot @ 2021-06-26  2:27 UTC (permalink / raw)
  To: Mike Tipton, djakov
  Cc: kbuild-all, bjorn.andersson, agross, saravanak, okukatla,
	linux-arm-msm, linux-pm, linux-kernel, Mike Tipton

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.13-rc7 next-20210625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mike-Tipton/interconnect-Fix-sync-state-issues/20210626-053132
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 55fcd4493da5ac8a0f7a0b3b5ae8448aee2041bb
config: arm64-allyesconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/ad01b68a345bdae6c24bd45720f4a01a2f9be49b
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Tipton/interconnect-Fix-sync-state-issues/20210626-053132
        git checkout ad01b68a345bdae6c24bd45720f4a01a2f9be49b
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/interconnect/qcom/icc-rpmh.c: In function 'qcom_icc_set':
>> drivers/interconnect/qcom/icc-rpmh.c:87:24: warning: variable 'qn' set but not used [-Wunused-but-set-variable]
      87 |  struct qcom_icc_node *qn;
         |                        ^~


vim +/qn +87 drivers/interconnect/qcom/icc-rpmh.c

976daac4a1c581 David Dai     2020-02-28   76  
976daac4a1c581 David Dai     2020-02-28   77  /**
976daac4a1c581 David Dai     2020-02-28   78   * qcom_icc_set - set the constraints based on path
976daac4a1c581 David Dai     2020-02-28   79   * @src: source node for the path to set constraints on
976daac4a1c581 David Dai     2020-02-28   80   * @dst: destination node for the path to set constraints on
976daac4a1c581 David Dai     2020-02-28   81   *
976daac4a1c581 David Dai     2020-02-28   82   * Return: 0 on success, or an error code otherwise
976daac4a1c581 David Dai     2020-02-28   83   */
976daac4a1c581 David Dai     2020-02-28   84  int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
976daac4a1c581 David Dai     2020-02-28   85  {
976daac4a1c581 David Dai     2020-02-28   86  	struct qcom_icc_provider *qp;
266cd33b591385 Georgi Djakov 2020-10-22  @87  	struct qcom_icc_node *qn;
976daac4a1c581 David Dai     2020-02-28   88  	struct icc_node *node;
976daac4a1c581 David Dai     2020-02-28   89  
976daac4a1c581 David Dai     2020-02-28   90  	if (!src)
976daac4a1c581 David Dai     2020-02-28   91  		node = dst;
976daac4a1c581 David Dai     2020-02-28   92  	else
976daac4a1c581 David Dai     2020-02-28   93  		node = src;
976daac4a1c581 David Dai     2020-02-28   94  
976daac4a1c581 David Dai     2020-02-28   95  	qp = to_qcom_provider(node->provider);
266cd33b591385 Georgi Djakov 2020-10-22   96  	qn = node->data;
266cd33b591385 Georgi Djakov 2020-10-22   97  
976daac4a1c581 David Dai     2020-02-28   98  	qcom_icc_bcm_voter_commit(qp->voter);
976daac4a1c581 David Dai     2020-02-28   99  
976daac4a1c581 David Dai     2020-02-28  100  	return 0;
976daac4a1c581 David Dai     2020-02-28  101  }
976daac4a1c581 David Dai     2020-02-28  102  EXPORT_SYMBOL_GPL(qcom_icc_set);
976daac4a1c581 David Dai     2020-02-28  103  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77613 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] interconnect: Zero initial BW after sync-state
  2021-06-25 21:28 ` [PATCH 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
@ 2021-07-01 16:56   ` okukatla
  2021-07-12 15:41     ` Mike Tipton
  0 siblings, 1 reply; 12+ messages in thread
From: okukatla @ 2021-07-01 16:56 UTC (permalink / raw)
  To: Mike Tipton
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 2021-06-26 02:58, Mike Tipton wrote:
> The initial BW values may be used by providers to enforce floors. Zero
> these values after sync-state so that providers know when to stop
> enforcing them.
> 
> Fixes: b1d681d8d324 ("interconnect: Add sync state support")
> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
> ---
>  drivers/interconnect/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 8a1e70e00876..945121e18b5c 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1106,6 +1106,8 @@ void icc_sync_state(struct device *dev)
>  		dev_dbg(p->dev, "interconnect provider is in synced state\n");
>  		list_for_each_entry(n, &p->nodes, node_list) {
>  			if (n->init_avg || n->init_peak) {
> +				n->init_avg = 0;
> +				n->init_peak = 0;
nit: It is good to reset init/floor levels back to zero, but we don't 
need to do this as we have sync_state flag to let providers know when to 
stop enforcing.
>  				aggregate_requests(n);
>  				p->set(n, n);
>  			}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes
  2021-06-25 21:28 ` [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
  2021-06-26  2:27   ` kernel test robot
@ 2021-07-01 18:48   ` okukatla
  2021-07-12 15:45     ` Mike Tipton
  1 sibling, 1 reply; 12+ messages in thread
From: okukatla @ 2021-07-01 18:48 UTC (permalink / raw)
  To: Mike Tipton
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 2021-06-26 02:58, Mike Tipton wrote:
> We currently only enforce BW floors for a subset of nodes in a path.
> All BCMs that need updating are queued in the pre_aggregate/aggregate
> phase. The first set() commits all queued BCMs and subsequent set()
> calls short-circuit without committing anything. Since the floor BW
> isn't set in sum_avg/max_peak until set(), then some BCMs are committed
> before their associated nodes reflect the floor.
> 
> Set the floor as each node is being aggregated. This ensures that all
> all relevant floors are set before the BCMs are committed.
> 
> Fixes: 266cd33b5913 ("interconnect: qcom: Ensure that the floor
> bandwidth value is enforced")
> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
> ---
>  drivers/interconnect/qcom/icc-rpmh.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpmh.c
> b/drivers/interconnect/qcom/icc-rpmh.c
> index bf01d09dba6c..f118f57eae37 100644
> --- a/drivers/interconnect/qcom/icc-rpmh.c
> +++ b/drivers/interconnect/qcom/icc-rpmh.c
> @@ -57,6 +57,11 @@ int qcom_icc_aggregate(struct icc_node *node, u32
> tag, u32 avg_bw,
>  			qn->sum_avg[i] += avg_bw;
>  			qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
>  		}
> +
> +		if (node->init_avg || node->init_peak) {
> +			qn->sum_avg[i] = max_t(u64, qn->sum_avg[i], node->init_avg);
> +			qn->max_peak[i] = max_t(u64, qn->max_peak[i], node->init_peak);
> +		}
Hi Mike,
Original problem is BCMs not getting added to commit_list for unused 
nodes, right? that is solved by moving *_bcm_voter_add() to 
pre_aggregate().
I could not get why we need to do above change, we are enforcing node 
votes with floor votes in framework + below code snippet that you 
removed.
How would adding this code in qcom_icc_aggregate() make difference? Is 
there any other issue that i am not to able to get?
>  	}
> 
>  	*agg_avg += avg_bw;
> @@ -90,11 +95,6 @@ int qcom_icc_set(struct icc_node *src, struct 
> icc_node *dst)
>  	qp = to_qcom_provider(node->provider);
>  	qn = node->data;
> 
> -	qn->sum_avg[QCOM_ICC_BUCKET_AMC] = max_t(u64,
> qn->sum_avg[QCOM_ICC_BUCKET_AMC],
> -						 node->avg_bw);
> -	qn->max_peak[QCOM_ICC_BUCKET_AMC] = max_t(u64,
> qn->max_peak[QCOM_ICC_BUCKET_AMC],
> -						  node->peak_bw);
> -
>  	qcom_icc_bcm_voter_commit(qp->voter);
> 
>  	return 0;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate
  2021-06-25 21:28 ` [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate Mike Tipton
@ 2021-07-01 18:55   ` okukatla
  2021-07-12 15:39     ` Mike Tipton
  0 siblings, 1 reply; 12+ messages in thread
From: okukatla @ 2021-07-01 18:55 UTC (permalink / raw)
  To: Mike Tipton
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 2021-06-26 02:58, Mike Tipton wrote:
> The pre_aggregate callback isn't called in all cases before calling
> aggregate. Add the missing calls so providers can rely on consistent
> framework behavior.
> 
> Fixes: d3703b3e255f ("interconnect: Aggregate before setting initial 
> bandwidth")
> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
> ---
>  drivers/interconnect/core.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 945121e18b5c..cfd54c90a6bb 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -973,9 +973,12 @@ void icc_node_add(struct icc_node *node, struct
> icc_provider *provider)
>  	}
>  	node->avg_bw = node->init_avg;
>  	node->peak_bw = node->init_peak;
> -	if (provider->aggregate)
> +	if (provider->aggregate) {
> +		if (provider->pre_aggregate)
> +			provider->pre_aggregate(node);
nit: we can invoke pre_aggregate() out side of if (qcom_icc_aggregate).

>  		provider->aggregate(node, 0, node->init_avg, node->init_peak,
>  				    &node->avg_bw, &node->peak_bw);
> +	}
>  	provider->set(node, node);
>  	node->avg_bw = 0;
>  	node->peak_bw = 0;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate
  2021-07-01 18:55   ` okukatla
@ 2021-07-12 15:39     ` Mike Tipton
  0 siblings, 0 replies; 12+ messages in thread
From: Mike Tipton @ 2021-07-12 15:39 UTC (permalink / raw)
  To: okukatla
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 7/1/2021 11:55 AM, okukatla@codeaurora.org wrote:
> On 2021-06-26 02:58, Mike Tipton wrote:
>> The pre_aggregate callback isn't called in all cases before calling
>> aggregate. Add the missing calls so providers can rely on consistent
>> framework behavior.
>>
>> Fixes: d3703b3e255f ("interconnect: Aggregate before setting initial 
>> bandwidth")
>> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
>> ---
>>  drivers/interconnect/core.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 945121e18b5c..cfd54c90a6bb 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -973,9 +973,12 @@ void icc_node_add(struct icc_node *node, struct
>> icc_provider *provider)
>>      }
>>      node->avg_bw = node->init_avg;
>>      node->peak_bw = node->init_peak;
>> -    if (provider->aggregate)
>> +    if (provider->aggregate) {
>> +        if (provider->pre_aggregate)
>> +            provider->pre_aggregate(node);
> nit: we can invoke pre_aggregate() out side of if (qcom_icc_aggregate).

Sure, will update this.

> 
>>          provider->aggregate(node, 0, node->init_avg, node->init_peak,
>>                      &node->avg_bw, &node->peak_bw);
>> +    }
>>      provider->set(node, node);
>>      node->avg_bw = 0;
>>      node->peak_bw = 0;

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 1/4] interconnect: Zero initial BW after sync-state
  2021-07-01 16:56   ` okukatla
@ 2021-07-12 15:41     ` Mike Tipton
  0 siblings, 0 replies; 12+ messages in thread
From: Mike Tipton @ 2021-07-12 15:41 UTC (permalink / raw)
  To: okukatla
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 7/1/2021 9:56 AM, okukatla@codeaurora.org wrote:
> On 2021-06-26 02:58, Mike Tipton wrote:
>> The initial BW values may be used by providers to enforce floors. Zero
>> these values after sync-state so that providers know when to stop
>> enforcing them.
>>
>> Fixes: b1d681d8d324 ("interconnect: Add sync state support")
>> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
>> ---
>>  drivers/interconnect/core.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 8a1e70e00876..945121e18b5c 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -1106,6 +1106,8 @@ void icc_sync_state(struct device *dev)
>>          dev_dbg(p->dev, "interconnect provider is in synced state\n");
>>          list_for_each_entry(n, &p->nodes, node_list) {
>>              if (n->init_avg || n->init_peak) {
>> +                n->init_avg = 0;
>> +                n->init_peak = 0;
> nit: It is good to reset init/floor levels back to zero, but we don't 
> need to do this as we have sync_state flag to let providers know when to 
> stop enforcing.

The synced_state variable is static to this file. It's not exposed to 
providers. In fact, we could entirely remove synced_state with this 
patch since it's unnecessary after zeroing the initial floors.

>>                  aggregate_requests(n);
>>                  p->set(n, n);
>>              }

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes
  2021-07-01 18:48   ` okukatla
@ 2021-07-12 15:45     ` Mike Tipton
  0 siblings, 0 replies; 12+ messages in thread
From: Mike Tipton @ 2021-07-12 15:45 UTC (permalink / raw)
  To: okukatla
  Cc: djakov, bjorn.andersson, agross, saravanak, linux-arm-msm,
	linux-pm, linux-kernel, mdtipton=codeaurora.org

On 7/1/2021 11:48 AM, okukatla@codeaurora.org wrote:
> On 2021-06-26 02:58, Mike Tipton wrote:
>> We currently only enforce BW floors for a subset of nodes in a path.
>> All BCMs that need updating are queued in the pre_aggregate/aggregate
>> phase. The first set() commits all queued BCMs and subsequent set()
>> calls short-circuit without committing anything. Since the floor BW
>> isn't set in sum_avg/max_peak until set(), then some BCMs are committed
>> before their associated nodes reflect the floor.
>>
>> Set the floor as each node is being aggregated. This ensures that all
>> all relevant floors are set before the BCMs are committed.
>>
>> Fixes: 266cd33b5913 ("interconnect: qcom: Ensure that the floor
>> bandwidth value is enforced")
>> Signed-off-by: Mike Tipton <mdtipton@codeaurora.org>
>> ---
>>  drivers/interconnect/qcom/icc-rpmh.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpmh.c
>> b/drivers/interconnect/qcom/icc-rpmh.c
>> index bf01d09dba6c..f118f57eae37 100644
>> --- a/drivers/interconnect/qcom/icc-rpmh.c
>> +++ b/drivers/interconnect/qcom/icc-rpmh.c
>> @@ -57,6 +57,11 @@ int qcom_icc_aggregate(struct icc_node *node, u32
>> tag, u32 avg_bw,
>>              qn->sum_avg[i] += avg_bw;
>>              qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
>>          }
>> +
>> +        if (node->init_avg || node->init_peak) {
>> +            qn->sum_avg[i] = max_t(u64, qn->sum_avg[i], node->init_avg);
>> +            qn->max_peak[i] = max_t(u64, qn->max_peak[i], 
>> node->init_peak);
>> +        }
> Hi Mike,
> Original problem is BCMs not getting added to commit_list for unused 
> nodes, right? that is solved by moving *_bcm_voter_add() to 
> pre_aggregate().
> I could not get why we need to do above change, we are enforcing node 
> votes with floor votes in framework + below code snippet that you removed.
> How would adding this code in qcom_icc_aggregate() make difference? Is 
> there any other issue that i am not to able to get?

This series fixes a couple of separate issues. One is not removing the 
initial floor after sync_state for unvoted paths, which the change to 
add BCMs to the commit list in pre_aggregate addresses. The other issue 
is not properly enforcing the initial floor *before* sync_state, which 
this patch addresses.

We only commit to HW what we've aggregated in our internal, 
provider-specific buckets (AMC, WAKE, SLEEP) during 
pre_aggregate/aggregate. All BCMs in the path are added to the commit 
list during this stage. Everything in the commit list is voted on the 
*first* set() callback for the path. The commit list is empty for every 
subsequent set() callback, so nothing actually happens except in the 
first set().

The original snippet below in qcom_icc_set() partially re-aggregates AMC 
with BW values from the icc_node struct. This is to capture the floor 
enforced by the framework itself. However, this is only for a single 
node in the path, which means it's also only for a single BCM in the 
path. Since we commit all BCMs on the first set(), this means most BCMs 
in the path don't see the floor BW at the time we commit them. This can 
lead to some BCMs being under-voted, or in some cases completely off.

So, instead of aggregating the floor for each node in qcom_icc_set(), 
this patch moves it to qcom_icc_aggregate(). This means all floors are 
captured internally before committing the BCMs.

>>      }
>>
>>      *agg_avg += avg_bw;
>> @@ -90,11 +95,6 @@ int qcom_icc_set(struct icc_node *src, struct 
>> icc_node *dst)
>>      qp = to_qcom_provider(node->provider);
>>      qn = node->data;
>>
>> -    qn->sum_avg[QCOM_ICC_BUCKET_AMC] = max_t(u64,
>> qn->sum_avg[QCOM_ICC_BUCKET_AMC],
>> -                         node->avg_bw);
>> -    qn->max_peak[QCOM_ICC_BUCKET_AMC] = max_t(u64,
>> qn->max_peak[QCOM_ICC_BUCKET_AMC],
>> -                          node->peak_bw);
>> -
>>      qcom_icc_bcm_voter_commit(qp->voter);
>>
>>      return 0;

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-07-12 15:45 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 21:28 [PATCH 0/4] interconnect: Fix sync-state issues Mike Tipton
2021-06-25 21:28 ` [PATCH 1/4] interconnect: Zero initial BW after sync-state Mike Tipton
2021-07-01 16:56   ` okukatla
2021-07-12 15:41     ` Mike Tipton
2021-06-25 21:28 ` [PATCH 2/4] interconnect: Always call pre_aggregate before aggregate Mike Tipton
2021-07-01 18:55   ` okukatla
2021-07-12 15:39     ` Mike Tipton
2021-06-25 21:28 ` [PATCH 3/4] interconnect: qcom: icc-rpmh: Ensure floor BW is enforced for all nodes Mike Tipton
2021-06-26  2:27   ` kernel test robot
2021-07-01 18:48   ` okukatla
2021-07-12 15:45     ` Mike Tipton
2021-06-25 21:28 ` [PATCH 4/4] interconnect: qcom: icc-rpmh: Add BCMs to commit list in pre_aggregate Mike Tipton

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).