| Message ID | 20241012-gpu-acd-v1-0-1e5e91aa95b6@quicinc.com |
|---|---|
| Headers | show |
| Series | Support for GPU ACD feature on Adreno X1-85 | expand |
On Sat, Oct 12, 2024 at 01:59:30AM +0530, Akhil P Oommen wrote: > Update GPU node to include acd level values. > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > --- > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > index a36076e3c56b..e6c500480eb1 100644 > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > @@ -3323,60 +3323,69 @@ zap-shader { > }; > > gpu_opp_table: opp-table { > - compatible = "operating-points-v2"; > + compatible = "operating-points-v2-adreno"; This nicely breaks all existing users of this DTS. Sorry, no. We are way past initial bringup/development. One year past. Best regards, Krzysztof
On Mon, Oct 14, 2024 at 09:40:13AM +0200, Krzysztof Kozlowski wrote: > On Sat, Oct 12, 2024 at 01:59:30AM +0530, Akhil P Oommen wrote: > > Update GPU node to include acd level values. > > > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > > --- > > arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > index a36076e3c56b..e6c500480eb1 100644 > > --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > > @@ -3323,60 +3323,69 @@ zap-shader { > > }; > > > > gpu_opp_table: opp-table { > > - compatible = "operating-points-v2"; > > + compatible = "operating-points-v2-adreno"; > > This nicely breaks all existing users of this DTS. Sorry, no. We are way > past initial bringup/development. One year past. It is not obvious to me how it breaks backward compatibility. Could you please elaborate a bit? I am aware that drivers should be backward compatible with DT, but not the other way. Are we talking about kernels other than Linux? Also, does including "operating-points-v2" too here help? -Akhil. > > Best regards, > Krzysztof >
On Wed, Oct 16, 2024 at 09:50:04AM +0200, Krzysztof Kozlowski wrote: > On 15/10/2024 21:35, Akhil P Oommen wrote: > > On Mon, Oct 14, 2024 at 09:40:13AM +0200, Krzysztof Kozlowski wrote: > >> On Sat, Oct 12, 2024 at 01:59:30AM +0530, Akhil P Oommen wrote: > >>> Update GPU node to include acd level values. > >>> > >>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > >>> --- > >>> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++++++++++- > >>> 1 file changed, 10 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > >>> index a36076e3c56b..e6c500480eb1 100644 > >>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi > >>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi > >>> @@ -3323,60 +3323,69 @@ zap-shader { > >>> }; > >>> > >>> gpu_opp_table: opp-table { > >>> - compatible = "operating-points-v2"; > >>> + compatible = "operating-points-v2-adreno"; > >> > >> This nicely breaks all existing users of this DTS. Sorry, no. We are way > >> past initial bringup/development. One year past. How do I identify when devicetree is considered stable? An arbitrary time period doesn't sound like a good idea. Is there a general consensus on this? X1E chipset is still considered under development at least till the end of this year, right? > > > > It is not obvious to me how it breaks backward compatibility. Could you > > I did not say "backward compatibility". I said existing users. > > > please elaborate a bit? I am aware that drivers should be backward > > compatible with DT, but not the other way. Are we talking about kernels other > > than Linux? > > > > Boot OpenBSD with new DTS. Previously: worked fine. Now: works less fine. > > We had exact talk about this during LPC. > > > Also, does including "operating-points-v2" too here help? > > Fallback? Yes, assuming these are compatible. Not much is explained in > the commit msg, except duplicating diff. That's not what the commit msg > is for. Okay. We can keep the fallback compatible string. -Akhil. > > > Best regards, > Krzysztof >
On 17/10/2024 08:12, Akhil P Oommen wrote: > On Wed, Oct 16, 2024 at 09:50:04AM +0200, Krzysztof Kozlowski wrote: >> On 15/10/2024 21:35, Akhil P Oommen wrote: >>> On Mon, Oct 14, 2024 at 09:40:13AM +0200, Krzysztof Kozlowski wrote: >>>> On Sat, Oct 12, 2024 at 01:59:30AM +0530, Akhil P Oommen wrote: >>>>> Update GPU node to include acd level values. >>>>> >>>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++++++++++- >>>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/x1e80100.dtsi b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>>>> index a36076e3c56b..e6c500480eb1 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/x1e80100.dtsi >>>>> @@ -3323,60 +3323,69 @@ zap-shader { >>>>> }; >>>>> >>>>> gpu_opp_table: opp-table { >>>>> - compatible = "operating-points-v2"; >>>>> + compatible = "operating-points-v2-adreno"; >>>> >>>> This nicely breaks all existing users of this DTS. Sorry, no. We are way >>>> past initial bringup/development. One year past. > > How do I identify when devicetree is considered stable? An arbitrary > time period doesn't sound like a good idea. Is there a general consensus > on this? > > X1E chipset is still considered under development at least till the end of this > year, right? Stable could be when people already get their consumer/final product with it. I got some weeks ago Lenovo T14s laptop and since yesterday working fine with Ubuntu: https://discourse.ubuntu.com/t/ubuntu-24-10-concept-snapdragon-x-elite/48800 All chipsets are under development, even old SM8450, but we avoid breaking it while doing that. Best regards, Krzysztof
On 11.10.2024 10:29 PM, Akhil P Oommen wrote: > ACD a.k.a Adaptive Clock Distribution is a feature which helps to reduce > the power consumption. In some chipsets, it is also a requirement to > support higher GPU frequencies. This patch adds support for GPU ACD by > sending necessary data to GMU and AOSS. The feature support for the > chipset is detected based on devicetree data. > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > --- [...] > + > + /* Initialize qmp node to talk to AOSS */ > + gmu->qmp = qmp_get(gmu->dev); > + if (IS_ERR(gmu->qmp)) { > + cmd->enable_by_level = 0; > + return dev_err_probe(gmu->dev, PTR_ERR(gmu->qmp), "Failed to initialize qmp\n"); > + } I'm still in favor of keeping qmp_get where it currently is, so that probe can fail/defer faster Konrad
On Mon, Oct 21, 2024 at 11:38:41AM +0200, Konrad Dybcio wrote: > On 11.10.2024 10:29 PM, Akhil P Oommen wrote: > > ACD a.k.a Adaptive Clock Distribution is a feature which helps to reduce > > the power consumption. In some chipsets, it is also a requirement to > > support higher GPU frequencies. This patch adds support for GPU ACD by > > sending necessary data to GMU and AOSS. The feature support for the > > chipset is detected based on devicetree data. > > > > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com> > > --- > > [...] > > > + > > + /* Initialize qmp node to talk to AOSS */ > > + gmu->qmp = qmp_get(gmu->dev); > > + if (IS_ERR(gmu->qmp)) { > > + cmd->enable_by_level = 0; > > + return dev_err_probe(gmu->dev, PTR_ERR(gmu->qmp), "Failed to initialize qmp\n"); > > + } > > I'm still in favor of keeping qmp_get where it currently is, so that > probe can fail/defer faster Sorry, I somehow missed this email from you until now. If it fails, then it probably doesn't matter if it is a bit late. But for defer, isn't there some optimizations to track the dependency from devicetree data? I am not entirely sure! Since qmp node is related to ACD, I felt it is better to: 1. Keep all acd probe related code in a single place. 2. Be more opportunistic in skipping qmp_get() wherever possible. But if you still have strong opinion on this, I can move it back in the next revision (v3). -Akhil > > Konrad
This series adds support for ACD feature for Adreno GPU which helps to lower the power consumption on GX rail and also sometimes is a requirement to enable higher GPU frequencies. At high level, following are the sequences required for ACD feature: 1. Identify the ACD level data for each regulator corner 2. Send a message to AOSS to switch voltage plan 3. Send a table with ACD level information to GMU during every gpu wake up For (1), it is better to keep ACD level data in devicetree because this value depends on the process node, voltage margins etc which are chipset specific. For instance, same GPU HW IP on a different chipset would have a different set of values. So, a new schema which extends opp-v2 is created to add a new property called "qcom,opp-acd-level". I would like to have more feedback on this, hence the RFC tag for this series. ACD support is dynamically detected based on the presence of "qcom,opp-acd-level" property in GPU's opp table. Also, qmp node should be present under GMU node in devicetree for communication with AOSS. The devicetree patch in this series adds the acd-level data for X1-85 GPU present in Snapdragon X1 Elite chipset. This series is rebased on top of drm-msm/msm-next. --- Akhil P Oommen (3): drm/msm/adreno: Add support for ACD dt-bindings: opp: Add v2-qcom-adreno vendor bindings arm64: dts: qcom: x1e80100: Add ACD levels for GPU .../bindings/opp/opp-v2-qcom-adreno.yaml | 84 ++++++++++++++++++++++ arch/arm64/boot/dts/qcom/x1e80100.dtsi | 11 ++- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 81 +++++++++++++++++---- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 1 + drivers/gpu/drm/msm/adreno/a6xx_hfi.c | 36 ++++++++++ drivers/gpu/drm/msm/adreno/a6xx_hfi.h | 21 ++++++ 6 files changed, 218 insertions(+), 16 deletions(-) --- base-commit: a20a91fb1bfac5d05ec5bcf9afe0c9363f6c8c93 change-id: 20240724-gpu-acd-6c1dc5dcf516 Best regards,