[PATCH 0/2] Improve enable_mask handling

[PATCH 0/2] Improve enable_mask handling

[Konrad Dybcio]

As pointed out by Bjorn and Mike in [1], we can simplify the handling
of enable_mask-based BCMs. This series attemps to do so and fixes a bug
that snuck in.

Gave a quick spin on 8450, doesn't seem to have exploded.

[1] https://lore.kernel.org/linux-arm-msm/113b50f8-35f6-73fc-4fc9-302262927c5e@quicinc.com/

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (2):
      interconnect: qcom: bcm-voter: Improve enable_mask handling
      interconnect: qcom: bcm-voter: Use enable_maks for keepalive voting

 drivers/interconnect/qcom/bcm-voter.c | 40 +++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 7 deletions(-)
---
base-commit: 535e616fd036bf8f2307b0f02a1912cf81deed4c
change-id: 20230811-topic-icc_fix_1he-40a3948b08f7

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/2] interconnect: qcom: bcm-voter: Improve enable_mask handling

[Konrad Dybcio]

We don't need all the complex arithmetic for BCMs utilizing enable_mask,
as all we need to do is to determine whether there's any user (or
keepalive) asking for it to be on.

Separate the logic for such BCMs for a small speed boost.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/bcm-voter.c | 40 +++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
index d5f2a6b5376b..82433f35717f 100644
--- a/drivers/interconnect/qcom/bcm-voter.c
+++ b/drivers/interconnect/qcom/bcm-voter.c
@@ -58,6 +58,33 @@ static u64 bcm_div(u64 num, u32 base)
 	return num;
 }
 
+/* BCMs with enable_mask use one-hot-encoding for on/off signaling */
+static void bcm_aggregate_1he(struct qcom_icc_bcm *bcm)
+{
+	struct qcom_icc_node *node;
+	int bucket, i;
+
+	for (bucket = 0; bucket < QCOM_ICC_NUM_BUCKETS; bucket++) {
+		for (i = 0; i < bcm->num_nodes; i++) {
+			node = bcm->nodes[i];
+
+			/* If any vote in this bucket exists, keep the BCM enabled */
+			if (node->sum_avg[bucket] || node->max_peak[bucket]) {
+				bcm->vote_x[bucket] = 0;
+				bcm->vote_y[bucket] = bcm->enable_mask;
+				break;
+			}
+		}
+	}
+
+	if (bcm->keepalive) {
+		bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
+		bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
+		bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
+		bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
+	}
+}
+
 static void bcm_aggregate(struct qcom_icc_bcm *bcm)
 {
 	struct qcom_icc_node *node;
@@ -83,11 +110,6 @@ static void bcm_aggregate(struct qcom_icc_bcm *bcm)
 
 		temp = agg_peak[bucket] * bcm->vote_scale;
 		bcm->vote_y[bucket] = bcm_div(temp, bcm->aux_data.unit);
-
-		if (bcm->enable_mask && (bcm->vote_x[bucket] || bcm->vote_y[bucket])) {
-			bcm->vote_x[bucket] = 0;
-			bcm->vote_y[bucket] = bcm->enable_mask;
-		}
 	}
 
 	if (bcm->keepalive && bcm->vote_x[QCOM_ICC_BUCKET_AMC] == 0 &&
@@ -260,8 +282,12 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
 		return 0;
 
 	mutex_lock(&voter->lock);
-	list_for_each_entry(bcm, &voter->commit_list, list)
-		bcm_aggregate(bcm);
+	list_for_each_entry(bcm, &voter->commit_list, list) {
+		if (bcm->enable_mask)
+			bcm_aggregate_1he(bcm);
+		else
+			bcm_aggregate(bcm);
+	}
 
 	/*
 	 * Pre sort the BCMs based on VCD for ease of generating a command list

-- 
2.41.0

Re: [PATCH 1/2] interconnect: qcom: bcm-voter: Improve enable_mask handling

[Bjorn Andersson]

On Fri, Aug 11, 2023 at 01:55:07PM +0200, Konrad Dybcio wrote:
> We don't need all the complex arithmetic for BCMs utilizing enable_mask,
> as all we need to do is to determine whether there's any user (or
> keepalive) asking for it to be on.
> 
> Separate the logic for such BCMs for a small speed boost.
> 

Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Reviewed-by: Bjorn Andersson <quic_bjorande@quicinc.com>

Regards,
Bjorn

Re: [PATCH 1/2] interconnect: qcom: bcm-voter: Improve enable_mask handling

[Mike Tipton]

On Fri, Aug 11, 2023 at 01:55:07PM +0200, Konrad Dybcio wrote:
> We don't need all the complex arithmetic for BCMs utilizing enable_mask,
> as all we need to do is to determine whether there's any user (or
> keepalive) asking for it to be on.
> 
> Separate the logic for such BCMs for a small speed boost.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/interconnect/qcom/bcm-voter.c | 40 +++++++++++++++++++++++++++++------
>  1 file changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
> index d5f2a6b5376b..82433f35717f 100644
> --- a/drivers/interconnect/qcom/bcm-voter.c
> +++ b/drivers/interconnect/qcom/bcm-voter.c
> @@ -58,6 +58,33 @@ static u64 bcm_div(u64 num, u32 base)
>  	return num;
>  }
>  
> +/* BCMs with enable_mask use one-hot-encoding for on/off signaling */
> +static void bcm_aggregate_1he(struct qcom_icc_bcm *bcm)

Suggest renaming this to bcm_aggregate_mask(). It's not actually a
one-hot encoding. Certain mask-based BCMs can have more than a single
bit set in their mask. Most will only have one, but some will have more.
Especially once QCOM_ICC_TAG_PERF_MODE is ported over. This tag sets an
additional bit in the ACV mask, but only when clients explicitly vote
for it, as opposed to always setting just the default ACV bit.

> +{
> +	struct qcom_icc_node *node;
> +	int bucket, i;
> +
> +	for (bucket = 0; bucket < QCOM_ICC_NUM_BUCKETS; bucket++) {
> +		for (i = 0; i < bcm->num_nodes; i++) {
> +			node = bcm->nodes[i];
> +
> +			/* If any vote in this bucket exists, keep the BCM enabled */
> +			if (node->sum_avg[bucket] || node->max_peak[bucket]) {
> +				bcm->vote_x[bucket] = 0;
> +				bcm->vote_y[bucket] = bcm->enable_mask;
> +				break;
> +			}

This will leave stale masks in vote_y after all clients have removed
their votes. The bcm->vote_x and bcm->vote_y arrays aren't cleared
before calling the aggregate functions. The original bcm_aggregate()
aggregates in zero-initialized local buffers before assigning the result
to bcm. Here, you could just assign vote_x and vote_y to 0 in the outer
bucket loop.

> +		}
> +	}
> +
> +	if (bcm->keepalive) {
> +		bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
> +		bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
> +		bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
> +		bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
> +	}
> +}
> +
>  static void bcm_aggregate(struct qcom_icc_bcm *bcm)
>  {
>  	struct qcom_icc_node *node;
> @@ -83,11 +110,6 @@ static void bcm_aggregate(struct qcom_icc_bcm *bcm)
>  
>  		temp = agg_peak[bucket] * bcm->vote_scale;
>  		bcm->vote_y[bucket] = bcm_div(temp, bcm->aux_data.unit);
> -
> -		if (bcm->enable_mask && (bcm->vote_x[bucket] || bcm->vote_y[bucket])) {
> -			bcm->vote_x[bucket] = 0;
> -			bcm->vote_y[bucket] = bcm->enable_mask;
> -		}
>  	}
>  
>  	if (bcm->keepalive && bcm->vote_x[QCOM_ICC_BUCKET_AMC] == 0 &&
> @@ -260,8 +282,12 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
>  		return 0;
>  
>  	mutex_lock(&voter->lock);
> -	list_for_each_entry(bcm, &voter->commit_list, list)
> -		bcm_aggregate(bcm);
> +	list_for_each_entry(bcm, &voter->commit_list, list) {
> +		if (bcm->enable_mask)
> +			bcm_aggregate_1he(bcm);
> +		else
> +			bcm_aggregate(bcm);
> +	}
>  
>  	/*
>  	 * Pre sort the BCMs based on VCD for ease of generating a command list
> 
> -- 
> 2.41.0
> 

[PATCH 2/2] interconnect: qcom: bcm-voter: Use enable_maks for keepalive voting

[Konrad Dybcio]

BCMs with an enable_mask expect to only have that specific value written
to them. The current implementation only works by miracle for BCMs with
enable mask == BIT(0), as the minimal vote we've been using so far just
so happens to be equal to that.

Use the correct value with keepalive voting.

Fixes: d8630f050d3f ("interconnect: qcom: Add support for mask-based BCMs")
Reported-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/bcm-voter.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
index 82433f35717f..a28b87eec3da 100644
--- a/drivers/interconnect/qcom/bcm-voter.c
+++ b/drivers/interconnect/qcom/bcm-voter.c
@@ -78,10 +78,10 @@ static void bcm_aggregate_1he(struct qcom_icc_bcm *bcm)
 	}
 
 	if (bcm->keepalive) {
-		bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
-		bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
-		bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
-		bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
+		bcm->vote_x[QCOM_ICC_BUCKET_AMC] = bcm->enable_mask;
+		bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = bcm->enable_mask;
+		bcm->vote_y[QCOM_ICC_BUCKET_AMC] = bcm->enable_mask;
+		bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = bcm->enable_mask;
 	}
 }
 

-- 
2.41.0

Re: [PATCH 0/2] Improve enable_mask handling

[Konrad Dybcio]

On 11.08.2023 13:55, Konrad Dybcio wrote:
> As pointed out by Bjorn and Mike in [1], we can simplify the handling
> of enable_mask-based BCMs. This series attemps to do so and fixes a bug
> that snuck in.
> 
> Gave a quick spin on 8450, doesn't seem to have exploded.
> 
> [1] https://lore.kernel.org/linux-arm-msm/113b50f8-35f6-73fc-4fc9-302262927c5e@quicinc.com/
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
+CC Mike's QUIC address, looks like the tree I based it on didn't
yet catch the CAF mailmap patch

Konrad