[2/4] interconnect: qcom: rpm: Set QoS parameters regardless of RPM bw setting

  QoS parameters and RPM bandwidth requests are wholly separate. Setting one
should only depend on the description of the interconnect node and not
whether the other is present. If we vote through RPM, QoS parameters
should be set so that the bus controller can make better decisions.
If we don't vote through RPM, QoS parameters should be set regardless,
as we're requesting additional bandwidth by setting the interconnect
clock rates.

The Fixes tag references the commit in which this logic was added, it
has since been shuffled around to a different file, but it's the one
where it originates from.

Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

On 03/01/2023 17:30, Konrad Dybcio wrote:
> QoS parameters and RPM bandwidth requests are wholly separate. Setting one
> should only depend on the description of the interconnect node and not
> whether the other is present. If we vote through RPM, QoS parameters
> should be set so that the bus controller can make better decisions.

Is that true ?

> If we don't vote through RPM, QoS parameters should be set regardless,
> as we're requesting additional bandwidth by setting the interconnect
> clock rates.
> 
> The Fixes tag references the commit in which this logic was added, it
> has since been shuffled around to a different file, but it's the one
> where it originates from.
> 
> Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
> index 06e0fee547ab..a190a0a839c8 100644
> --- a/drivers/interconnect/qcom/icc-rpm.c
> +++ b/drivers/interconnect/qcom/icc-rpm.c
> @@ -252,8 +252,10 @@ static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
>   		ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>   		if (ret)
>   			return ret;
> -	} else if (qn->qos.qos_mode != -1) {
> -		/* set bandwidth directly from the AP */
> +	}
> +
> +	if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
> +		/* Set QoS params from the AP */
>   		ret = qcom_icc_qos_set(n, sum_bw);
>   		if (ret)
>   			return ret;

Taking the example of

static struct qcom_icc_node bimc_snoc_slv = {
         .name = "bimc_snoc_slv",
         .id = MSM8939_BIMC_SNOC_SLV,
         .buswidth = 16,
         .mas_rpm_id = -1,
         .slv_rpm_id = 2,
         .num_links = ARRAY_SIZE(bimc_snoc_slv_links),
         .links = bimc_snoc_slv_links,
};

#define NOC_QOS_MODE_INVALID -1
ap_owned == false
qos_mode == NOC_QOS_MODE_FIXED


if (!qn->qos.ap_owned) {
	/* bod: this will run */
	/* send bandwidth request message to the RPM processor */
	ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
	if (ret)
		return ret;
} else if (qn->qos.qos_mode != -1) {
	/* bod: this will not run */
	/* set bandwidth directly from the AP */
	ret = qcom_icc_qos_set(n, sum_bw);
	if (ret)
		return ret;
}

and your proposed change

if (!qn->qos.ap_owned) {
	/* bod: this will run */
	/* send bandwidth request message to the RPM processor */
	ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
	if (ret)
		return ret;
}

if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
	/* bod: this will run */
	/* set bandwidth directly from the AP */
	ret = qcom_icc_qos_set(n, sum_bw);
	if (ret)
		return ret;
}

however if we look downstream we have the concept of ap_owned

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L194

https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L208

In simple terms
if (node_info->ap_owned) {
	ret = fabdev->noc_ops.set_bw(node_info,
									} else {
	ret = send_rpm_msg(node_device);
}

I agree your code does what it says on the tin but, whats the overall 
justification to depart from the downstream logic ?

---
bod
  

On 03/01/2023 23:43, Bryan O'Donoghue wrote:
> On 03/01/2023 17:30, Konrad Dybcio wrote:
> I agree your code does what it says on the tin but, whats the overall 
> justification to depart from the downstream logic ?
> 
> ---
> bod

and can we prove with a meaningful test a behaviour change ? Better 
throughput ? Lower thermals ?

---
bod
  

On 4.01.2023 00:43, Bryan O'Donoghue wrote:
> On 03/01/2023 17:30, Konrad Dybcio wrote:
>> QoS parameters and RPM bandwidth requests are wholly separate. Setting one
>> should only depend on the description of the interconnect node and not
>> whether the other is present. If we vote through RPM, QoS parameters
>> should be set so that the bus controller can make better decisions.
> 
> Is that true ?
> 
>> If we don't vote through RPM, QoS parameters should be set regardless,
>> as we're requesting additional bandwidth by setting the interconnect
>> clock rates.
>>
>> The Fixes tag references the commit in which this logic was added, it
>> has since been shuffled around to a different file, but it's the one
>> where it originates from.
>>
>> Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 06e0fee547ab..a190a0a839c8 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -252,8 +252,10 @@ static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
>>           ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>>           if (ret)
>>               return ret;
>> -    } else if (qn->qos.qos_mode != -1) {
>> -        /* set bandwidth directly from the AP */
>> +    }
>> +
>> +    if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>> +        /* Set QoS params from the AP */
>>           ret = qcom_icc_qos_set(n, sum_bw);
>>           if (ret)
>>               return ret;
> 
> Taking the example of
> 
> static struct qcom_icc_node bimc_snoc_slv = {
>         .name = "bimc_snoc_slv",
>         .id = MSM8939_BIMC_SNOC_SLV,
>         .buswidth = 16,
>         .mas_rpm_id = -1,
>         .slv_rpm_id = 2,
>         .num_links = ARRAY_SIZE(bimc_snoc_slv_links),
>         .links = bimc_snoc_slv_links,
> };
> 
> #define NOC_QOS_MODE_INVALID -1
> ap_owned == false
> qos_mode == NOC_QOS_MODE_FIXED
> 
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> } else if (qn->qos.qos_mode != -1) {
>     /* bod: this will not run */
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> and your proposed change
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>     /* bod: this will run */
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> however if we look downstream we have the concept of ap_owned
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L194
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L208
> 
> In simple terms
> if (node_info->ap_owned) {
>     ret = fabdev->noc_ops.set_bw(node_info,
>                                     } else {
>     ret = send_rpm_msg(node_device);
> }
> 
> I agree your code does what it says on the tin but, whats the overall justification to depart from the downstream logic ?
Okay, so maybe it would be worth checking with Qualcomm what it's
supposed to do. On msm-5.4 setting QoS is done unconditionally,
no matter if the node has valid (!= -1) rpm mas/slv IDs.

https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.9.14.r1/drivers/interconnect/qcom/icc-rpm.c#L97

It may be something that began with newer SoCs, or maybe the
carried-with-us-ever-since-3.4 chonky msm_bus driver had a bug..
or maybe the msm-5.4 interconnect impl has a bug.. We really
won't know unless somebody can confirm it for us..

My understanding would be such that the QoS parameters are always
set from the AP and RPM just scales the bandwidth on certain nodes,
like it scales power and frequency for some lines/devices. That
may or may not be true or might also depend on the SoC / RPM fw..

And even if RPM sets these values internally, it shouldn't hurt to
adjust them from AP again, but that would both deserve a different
comment and would be a rather bad design, as tuning the values
would require a rpm firmware update (and we know how vendors treat
firmware updates), so that might have been something qc engineers
took into account..

tldr: new soc good (*), old soc bad-or-no-effect (*), should ask QC

Konrad
> 
> ---
> bod
  

On 4.01.2023 00:43, Bryan O'Donoghue wrote:
> On 03/01/2023 17:30, Konrad Dybcio wrote:
>> QoS parameters and RPM bandwidth requests are wholly separate. Setting one
>> should only depend on the description of the interconnect node and not
>> whether the other is present. If we vote through RPM, QoS parameters
>> should be set so that the bus controller can make better decisions.
> 
> Is that true ?
> 
>> If we don't vote through RPM, QoS parameters should be set regardless,
>> as we're requesting additional bandwidth by setting the interconnect
>> clock rates.
>>
>> The Fixes tag references the commit in which this logic was added, it
>> has since been shuffled around to a different file, but it's the one
>> where it originates from.
>>
>> Fixes: f80a1d414328 ("interconnect: qcom: Add SDM660 interconnect provider driver")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/interconnect/qcom/icc-rpm.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c
>> index 06e0fee547ab..a190a0a839c8 100644
>> --- a/drivers/interconnect/qcom/icc-rpm.c
>> +++ b/drivers/interconnect/qcom/icc-rpm.c
>> @@ -252,8 +252,10 @@ static int __qcom_icc_set(struct icc_node *n, struct qcom_icc_node *qn,
>>           ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>>           if (ret)
>>               return ret;
>> -    } else if (qn->qos.qos_mode != -1) {
>> -        /* set bandwidth directly from the AP */
>> +    }
>> +
>> +    if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>> +        /* Set QoS params from the AP */
>>           ret = qcom_icc_qos_set(n, sum_bw);
>>           if (ret)
>>               return ret;
> 
> Taking the example of
> 
> static struct qcom_icc_node bimc_snoc_slv = {
>         .name = "bimc_snoc_slv",
>         .id = MSM8939_BIMC_SNOC_SLV,
>         .buswidth = 16,
>         .mas_rpm_id = -1,
>         .slv_rpm_id = 2,
>         .num_links = ARRAY_SIZE(bimc_snoc_slv_links),
>         .links = bimc_snoc_slv_links,
> };
> 
> #define NOC_QOS_MODE_INVALID -1
> ap_owned == false
> qos_mode == NOC_QOS_MODE_FIXED
> 
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> } else if (qn->qos.qos_mode != -1) {
>     /* bod: this will not run */
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> and your proposed change
> 
> if (!qn->qos.ap_owned) {
>     /* bod: this will run */
>     /* send bandwidth request message to the RPM processor */
>     ret = qcom_icc_rpm_set(qn->mas_rpm_id, qn->slv_rpm_id, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> if (qn->qos.qos_mode != NOC_QOS_MODE_INVALID) {
>     /* bod: this will run */
Also, this will not run with the next patch, perhaps i should
have ordered them differently (or perhaps the issue it solves
should have never been introduced :P).

Konrad
>     /* set bandwidth directly from the AP */
>     ret = qcom_icc_qos_set(n, sum_bw);
>     if (ret)
>         return ret;
> }
> 
> however if we look downstream we have the concept of ap_owned
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L194
> 
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.BR.1.2.9-00810-8x09.0/drivers/platform/msm/msm_bus/msm_bus_fabric_adhoc.c#L208
> 
> In simple terms
> if (node_info->ap_owned) {
>     ret = fabdev->noc_ops.set_bw(node_info,
>                                     } else {
>     ret = send_rpm_msg(node_device);
> }
> 
> I agree your code does what it says on the tin but, whats the overall justification to depart from the downstream logic ?
> 
> ---
> bod
  

On 04/01/2023 00:39, Konrad Dybcio wrote:
> tldr: new soc good (*), old soc bad-or-no-effect (*), should ask QC

I think this needs to be tested before application. If it is benign on < 
kernel 5.4 SoCs then that's fine too but, it does need to be validated 
as so.

I suspect there's no silicon dependency, it is probably "just how the 
code was written" without any silicon dependency backing it up but, I 
think we need to do the work to prove that before applying.

An RFT with some interconnect settings targeted for test on pre 5.4 and 
post 5.4 SoCs would do.

---
bod