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@xxxxxxxxxx>
> ---
>  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
>