| Message ID | 20230103173059.265856-1-konrad.dybcio@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/4] interconnect: qcom: rpm: Don't set QoS params before non-zero bw is requested | expand |
On 03/01/2023 17:30, Konrad Dybcio wrote: > Until now, the icc-rpm driver unconditionally set QoS params, even on > empty requests. This is superfluous and the downstream counterpart does > not do it. Follow it by doing the same. > > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/interconnect/qcom/icc-rpm.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c > index 43b9ce0dcb6a..06e0fee547ab 100644 > --- a/drivers/interconnect/qcom/icc-rpm.c > +++ b/drivers/interconnect/qcom/icc-rpm.c > @@ -193,6 +193,12 @@ static int qcom_icc_qos_set(struct icc_node *node, u64 sum_bw) > struct qcom_icc_provider *qp = to_qcom_provider(node->provider); > struct qcom_icc_node *qn = node->data; > > + /* Defer setting QoS until the first non-zero bandwidth request. */ > + if (!(node->avg_bw || node->peak_bw)) { > + dev_dbg(node->provider->dev, "NOT Setting QoS for %s\n", qn->name); > + return 0; > + } > + > dev_dbg(node->provider->dev, "Setting QoS for %s\n", qn->name); > > switch (qp->type) { Doesn't downstream clear the registers on a zero allocation request ? 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_bimc.c#L1302 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_bimc.c#L1318 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_bimc.c#L1367 msm_bus_bimc_set_qos_bw() { /* Only calculate if there's a requested bandwidth and window */ if (qbw->bw && qbw->ws) { }else /* Clear bandwidth registers */ set_qos_bw_regs(base, mas_index, 0, 0, 0, 0, 0); } --- bod
On 4.01.2023 00:07, Bryan O'Donoghue wrote: > On 03/01/2023 17:30, Konrad Dybcio wrote: >> Until now, the icc-rpm driver unconditionally set QoS params, even on >> empty requests. This is superfluous and the downstream counterpart does >> not do it. Follow it by doing the same. >> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/interconnect/qcom/icc-rpm.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c >> index 43b9ce0dcb6a..06e0fee547ab 100644 >> --- a/drivers/interconnect/qcom/icc-rpm.c >> +++ b/drivers/interconnect/qcom/icc-rpm.c >> @@ -193,6 +193,12 @@ static int qcom_icc_qos_set(struct icc_node *node, u64 sum_bw) >> struct qcom_icc_provider *qp = to_qcom_provider(node->provider); >> struct qcom_icc_node *qn = node->data; >> + /* Defer setting QoS until the first non-zero bandwidth request. */ >> + if (!(node->avg_bw || node->peak_bw)) { >> + dev_dbg(node->provider->dev, "NOT Setting QoS for %s\n", qn->name); >> + return 0; >> + } >> + >> dev_dbg(node->provider->dev, "Setting QoS for %s\n", qn->name); >> switch (qp->type) { > > Doesn't downstream clear the registers on a zero allocation request ? > > 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_bimc.c#L1302 > > 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_bimc.c#L1318 > > 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_bimc.c#L1367 > > msm_bus_bimc_set_qos_bw() > { > /* Only calculate if there's a requested bandwidth and window */ > if (qbw->bw && qbw->ws) { > }else > /* Clear bandwidth registers */ > set_qos_bw_regs(base, mas_index, 0, 0, 0, 0, 0); > } Yes, looks like that's the case, but also it's only for BIMC, not for NOC: 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_noc.c#L246 Moreover, it only concerns QoS parameters that are not supported on mainline (Grant Period, Grant Count, Threshold Lo/Me/Hi) [1], so that pretty much addresses your worries, I think.. And FWIW that's definitely not the case anymore for QNOC (and BIMC for that matter) on msm-5.4: https://github.com/sonyxperiadev/kernel/blob/aosp/LA.UM.9.14.r1/drivers/interconnect/qcom/icc-rpm.c#L217 Konrad [1] Note: msm8939 seems to be a somewhat heavy user of these properties, maybe it would be worth looking into implementing them? > > --- > bod
diff --git a/drivers/interconnect/qcom/icc-rpm.c b/drivers/interconnect/qcom/icc-rpm.c index 43b9ce0dcb6a..06e0fee547ab 100644 --- a/drivers/interconnect/qcom/icc-rpm.c +++ b/drivers/interconnect/qcom/icc-rpm.c @@ -193,6 +193,12 @@ static int qcom_icc_qos_set(struct icc_node *node, u64 sum_bw) struct qcom_icc_provider *qp = to_qcom_provider(node->provider); struct qcom_icc_node *qn = node->data; + /* Defer setting QoS until the first non-zero bandwidth request. */ + if (!(node->avg_bw || node->peak_bw)) { + dev_dbg(node->provider->dev, "NOT Setting QoS for %s\n", qn->name); + return 0; + } + dev_dbg(node->provider->dev, "Setting QoS for %s\n", qn->name); switch (qp->type) {
Until now, the icc-rpm driver unconditionally set QoS params, even on empty requests. This is superfluous and the downstream counterpart does not do it. Follow it by doing the same. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/interconnect/qcom/icc-rpm.c | 6 ++++++ 1 file changed, 6 insertions(+)