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