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 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
On 22.10.2024 12:09 AM, Akhil P Oommen wrote: > 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! There's devlink for clocks/supplies etc, it doesn't apply universally for all phandle references IIUC. > > 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). I suppose the answer is yes, I have a strong opinion :D Konrad
On 11/10/2024 22:29, 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> > --- > 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 +++++++++ > 4 files changed, 124 insertions(+), 15 deletions(-) > <snip> > + > +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) > +{ > + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; > + struct a6xx_hfi_msg_feature_ctrl msg = { > + .feature = HFI_FEATURE_ACD, > + .enable = 1, > + .data = 0, > + }; > + int ret; > + > + if (!acd_table->enable_by_level) > + return 0; > + > + /* Enable ACD feature at GMU */ > + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, sizeof(msg), NULL, 0); > + if (ret) { > + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); > + return ret; > + } > + > + /* Send ACD table to GMU */ > + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), NULL, 0); This looks wrong, in this exact code, you never use the acd_table... perhaps it should be acd_table here > + if (ret) { > + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); > + return ret; > + } > + > + return 0; > +} > + > static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) > { > struct a6xx_hfi_msg_test msg = { 0 }; > @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int boot_state) > if (ret) > return ret; > > + ret = a6xx_hfi_enable_acd(gmu); > + if (ret) > + return ret; > + > ret = a6xx_hfi_send_core_fw_start(gmu); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > index 528110169398..51864c8ad0e6 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h > @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { > u32 header; > }; > > +#define HFI_H2F_MSG_ACD 7 > +#define MAX_ACD_STRIDE 2 > + > +struct a6xx_hfi_acd_table { > + u32 header; > + u32 version; > + u32 enable_by_level; > + u32 stride; > + u32 num_levels; > + u32 data[16 * MAX_ACD_STRIDE]; > +}; > + > #define HFI_H2F_MSG_START 10 > > struct a6xx_hfi_msg_start { > u32 header; > }; > > +#define HFI_H2F_FEATURE_CTRL 11 > + > +struct a6xx_hfi_msg_feature_ctrl { > + u32 header; > + u32 feature; > + u32 enable; > + u32 data; > +}; > + > #define HFI_H2F_MSG_CORE_FW_START 14 > > struct a6xx_hfi_msg_core_fw_start { > Thanks, Neil
On 11/4/2024 9:14 PM, neil.armstrong@linaro.org wrote: > On 11/10/2024 22:29, 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> >> --- >> 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 +++++++++ >> 4 files changed, 124 insertions(+), 15 deletions(-) >> > > <snip> > >> + >> +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >> +{ >> + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; >> + struct a6xx_hfi_msg_feature_ctrl msg = { >> + .feature = HFI_FEATURE_ACD, >> + .enable = 1, >> + .data = 0, >> + }; >> + int ret; >> + >> + if (!acd_table->enable_by_level) >> + return 0; >> + >> + /* Enable ACD feature at GMU */ >> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, >> sizeof(msg), NULL, 0); >> + if (ret) { >> + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); >> + return ret; >> + } >> + >> + /* Send ACD table to GMU */ >> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), >> NULL, 0); > > This looks wrong, in this exact code, you never use the acd_table... > perhaps it should be acd_table here Whoops! Weirdly gmu didn't explode when I tested. Thanks for your keen eye. -Akhil. > >> + if (ret) { >> + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) >> { >> struct a6xx_hfi_msg_test msg = { 0 }; >> @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int >> boot_state) >> if (ret) >> return ret; >> + ret = a6xx_hfi_enable_acd(gmu); >> + if (ret) >> + return ret; >> + >> ret = a6xx_hfi_send_core_fw_start(gmu); >> if (ret) >> return ret; >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/ >> msm/adreno/a6xx_hfi.h >> index 528110169398..51864c8ad0e6 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >> @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { >> u32 header; >> }; >> +#define HFI_H2F_MSG_ACD 7 >> +#define MAX_ACD_STRIDE 2 >> + >> +struct a6xx_hfi_acd_table { >> + u32 header; >> + u32 version; >> + u32 enable_by_level; >> + u32 stride; >> + u32 num_levels; >> + u32 data[16 * MAX_ACD_STRIDE]; >> +}; >> + >> #define HFI_H2F_MSG_START 10 >> struct a6xx_hfi_msg_start { >> u32 header; >> }; >> +#define HFI_H2F_FEATURE_CTRL 11 >> + >> +struct a6xx_hfi_msg_feature_ctrl { >> + u32 header; >> + u32 feature; >> + u32 enable; >> + u32 data; >> +}; >> + >> #define HFI_H2F_MSG_CORE_FW_START 14 >> struct a6xx_hfi_msg_core_fw_start { >> > > Thanks, > Neil
On 06/11/2024 02:44, Akhil P Oommen wrote: > On 11/4/2024 9:14 PM, neil.armstrong@linaro.org wrote: >> On 11/10/2024 22:29, 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> >>> --- >>> 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 +++++++++ >>> 4 files changed, 124 insertions(+), 15 deletions(-) >>> >> >> <snip> >> >>> + >>> +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >>> +{ >>> + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; >>> + struct a6xx_hfi_msg_feature_ctrl msg = { >>> + .feature = HFI_FEATURE_ACD, >>> + .enable = 1, >>> + .data = 0, >>> + }; >>> + int ret; >>> + >>> + if (!acd_table->enable_by_level) >>> + return 0; >>> + >>> + /* Enable ACD feature at GMU */ >>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, >>> sizeof(msg), NULL, 0); >>> + if (ret) { >>> + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); >>> + return ret; >>> + } >>> + >>> + /* Send ACD table to GMU */ >>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), >>> NULL, 0); >> >> This looks wrong, in this exact code, you never use the acd_table... >> perhaps it should be acd_table here > > Whoops! Weirdly gmu didn't explode when I tested. > > Thanks for your keen eye. You're welcome ! I've been trying to enable this on SM8650, but HFI_H2F_MSG_ACD fails. My changes: ================><================================ diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c index 7c96d6f8aaa9..bd9d586f245e 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c @@ -682,7 +682,7 @@ static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) } /* Send ACD table to GMU */ - ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, sizeof(*acd_table), NULL, 0); + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, sizeof(struct a6xx_hfi_acd_table), NULL, 0); if (ret) { DRM_DEV_ERROR(gmu->dev, "Unable to send ACD table (%d)\n", ret); return ret; ================><================================ with the appropriate qcom,opp-acd-level in DT taken from downstream, I get: [ 6.946184] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* Message (null) id 4 timed out waiting for response [ 6.958697] platform 3d6a000.gmu: [drm:a6xx_hfi_start [msm]] *ERROR* Unable to send ACD table (-110) is there something missing ? Neil > > -Akhil. > >> >>> + if (ret) { >>> + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) >>> { >>> struct a6xx_hfi_msg_test msg = { 0 }; >>> @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int >>> boot_state) >>> if (ret) >>> return ret; >>> + ret = a6xx_hfi_enable_acd(gmu); >>> + if (ret) >>> + return ret; >>> + >>> ret = a6xx_hfi_send_core_fw_start(gmu); >>> if (ret) >>> return ret; >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/ >>> msm/adreno/a6xx_hfi.h >>> index 528110169398..51864c8ad0e6 100644 >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>> @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { >>> u32 header; >>> }; >>> +#define HFI_H2F_MSG_ACD 7 >>> +#define MAX_ACD_STRIDE 2 >>> + >>> +struct a6xx_hfi_acd_table { >>> + u32 header; >>> + u32 version; >>> + u32 enable_by_level; >>> + u32 stride; >>> + u32 num_levels; >>> + u32 data[16 * MAX_ACD_STRIDE]; >>> +}; >>> + >>> #define HFI_H2F_MSG_START 10 >>> struct a6xx_hfi_msg_start { >>> u32 header; >>> }; >>> +#define HFI_H2F_FEATURE_CTRL 11 >>> + >>> +struct a6xx_hfi_msg_feature_ctrl { >>> + u32 header; >>> + u32 feature; >>> + u32 enable; >>> + u32 data; >>> +}; >>> + >>> #define HFI_H2F_MSG_CORE_FW_START 14 >>> struct a6xx_hfi_msg_core_fw_start { >>> >> >> Thanks, >> Neil >
On 11/7/2024 2:25 PM, neil.armstrong@linaro.org wrote: > On 06/11/2024 02:44, Akhil P Oommen wrote: >> On 11/4/2024 9:14 PM, neil.armstrong@linaro.org wrote: >>> On 11/10/2024 22:29, 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> >>>> --- >>>> 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 +++++++++ >>>> 4 files changed, 124 insertions(+), 15 deletions(-) >>>> >>> >>> <snip> >>> >>>> + >>>> +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >>>> +{ >>>> + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; >>>> + struct a6xx_hfi_msg_feature_ctrl msg = { >>>> + .feature = HFI_FEATURE_ACD, >>>> + .enable = 1, >>>> + .data = 0, >>>> + }; >>>> + int ret; >>>> + >>>> + if (!acd_table->enable_by_level) >>>> + return 0; >>>> + >>>> + /* Enable ACD feature at GMU */ >>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, >>>> sizeof(msg), NULL, 0); >>>> + if (ret) { >>>> + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + /* Send ACD table to GMU */ >>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), >>>> NULL, 0); >>> >>> This looks wrong, in this exact code, you never use the acd_table... >>> perhaps it should be acd_table here >> >> Whoops! Weirdly gmu didn't explode when I tested. >> >> Thanks for your keen eye. > > You're welcome ! > > I've been trying to enable this on SM8650, but HFI_H2F_MSG_ACD fails. > > My changes: > ================><================================ > diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/ > msm/adreno/a6xx_hfi.c > index 7c96d6f8aaa9..bd9d586f245e 100644 > --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c > @@ -682,7 +682,7 @@ static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) > } > > /* Send ACD table to GMU */ > - ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, > sizeof(*acd_table), NULL, 0); > + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, &acd_table -> acd_table here? -Akhil > sizeof(struct a6xx_hfi_acd_table), NULL, 0); > if (ret) { > DRM_DEV_ERROR(gmu->dev, "Unable to send ACD table > (%d)\n", ret); > return ret; > ================><================================ > > with the appropriate qcom,opp-acd-level in DT taken from downstream, I get: > [ 6.946184] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* Message (null) id 4 timed out waiting for response > [ 6.958697] platform 3d6a000.gmu: [drm:a6xx_hfi_start [msm]] *ERROR* > Unable to send ACD table (-110) > > is there something missing ? > > Neil > >> >> -Akhil. >> >>> >>>> + if (ret) { >>>> + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) >>>> { >>>> struct a6xx_hfi_msg_test msg = { 0 }; >>>> @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int >>>> boot_state) >>>> if (ret) >>>> return ret; >>>> + ret = a6xx_hfi_enable_acd(gmu); >>>> + if (ret) >>>> + return ret; >>>> + >>>> ret = a6xx_hfi_send_core_fw_start(gmu); >>>> if (ret) >>>> return ret; >>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/ >>>> msm/adreno/a6xx_hfi.h >>>> index 528110169398..51864c8ad0e6 100644 >>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>> @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { >>>> u32 header; >>>> }; >>>> +#define HFI_H2F_MSG_ACD 7 >>>> +#define MAX_ACD_STRIDE 2 >>>> + >>>> +struct a6xx_hfi_acd_table { >>>> + u32 header; >>>> + u32 version; >>>> + u32 enable_by_level; >>>> + u32 stride; >>>> + u32 num_levels; >>>> + u32 data[16 * MAX_ACD_STRIDE]; >>>> +}; >>>> + >>>> #define HFI_H2F_MSG_START 10 >>>> struct a6xx_hfi_msg_start { >>>> u32 header; >>>> }; >>>> +#define HFI_H2F_FEATURE_CTRL 11 >>>> + >>>> +struct a6xx_hfi_msg_feature_ctrl { >>>> + u32 header; >>>> + u32 feature; >>>> + u32 enable; >>>> + u32 data; >>>> +}; >>>> + >>>> #define HFI_H2F_MSG_CORE_FW_START 14 >>>> struct a6xx_hfi_msg_core_fw_start { >>>> >>> >>> Thanks, >>> Neil >> >
On 07/11/2024 13:46, Akhil P Oommen wrote: > On 11/7/2024 2:25 PM, neil.armstrong@linaro.org wrote: >> On 06/11/2024 02:44, Akhil P Oommen wrote: >>> On 11/4/2024 9:14 PM, neil.armstrong@linaro.org wrote: >>>> On 11/10/2024 22:29, 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> >>>>> --- >>>>> 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 +++++++++ >>>>> 4 files changed, 124 insertions(+), 15 deletions(-) >>>>> >>>> >>>> <snip> >>>> >>>>> + >>>>> +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >>>>> +{ >>>>> + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; >>>>> + struct a6xx_hfi_msg_feature_ctrl msg = { >>>>> + .feature = HFI_FEATURE_ACD, >>>>> + .enable = 1, >>>>> + .data = 0, >>>>> + }; >>>>> + int ret; >>>>> + >>>>> + if (!acd_table->enable_by_level) >>>>> + return 0; >>>>> + >>>>> + /* Enable ACD feature at GMU */ >>>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, >>>>> sizeof(msg), NULL, 0); >>>>> + if (ret) { >>>>> + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + /* Send ACD table to GMU */ >>>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), >>>>> NULL, 0); >>>> >>>> This looks wrong, in this exact code, you never use the acd_table... >>>> perhaps it should be acd_table here >>> >>> Whoops! Weirdly gmu didn't explode when I tested. >>> >>> Thanks for your keen eye. >> >> You're welcome ! >> >> I've been trying to enable this on SM8650, but HFI_H2F_MSG_ACD fails. >> >> My changes: >> ================><================================ >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/ >> msm/adreno/a6xx_hfi.c >> index 7c96d6f8aaa9..bd9d586f245e 100644 >> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c >> @@ -682,7 +682,7 @@ static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >> } >> >> /* Send ACD table to GMU */ >> - ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, >> sizeof(*acd_table), NULL, 0); >> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, > > &acd_table -> acd_table here? Damn, good catch ! Ok so it didn't explode anymore, but still fails: ===== [ 7.083258] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* Message HFI_H2F_MSG_GX_BW_PERF_VOTE id 7 timed out waiting for response [ 7.149709] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* Unexpected message id 7 on the response queue [ 7.149744] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* The HFI response queue is unexpectedly empty [ 7.165163] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* Unexpected message id 8 on the response queue [ 7.165188] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 [msm]] *ERROR* The HFI response queue is unexpectedly empty ==== Seems with ACD enabled, first vote can take up to 100ms, and downstream has 1s timeout, with a larger timeout I got it to work ! Thanks, Neil > > -Akhil > >> sizeof(struct a6xx_hfi_acd_table), NULL, 0); >> if (ret) { >> DRM_DEV_ERROR(gmu->dev, "Unable to send ACD table >> (%d)\n", ret); >> return ret; >> ================><================================ >> >> with the appropriate qcom,opp-acd-level in DT taken from downstream, I get: >> [ 6.946184] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 >> [msm]] *ERROR* Message (null) id 4 timed out waiting for response >> [ 6.958697] platform 3d6a000.gmu: [drm:a6xx_hfi_start [msm]] *ERROR* >> Unable to send ACD table (-110) >> >> is there something missing ? >> >> Neil >> >>> >>> -Akhil. >>> >>>> >>>>> + if (ret) { >>>>> + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) >>>>> { >>>>> struct a6xx_hfi_msg_test msg = { 0 }; >>>>> @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int >>>>> boot_state) >>>>> if (ret) >>>>> return ret; >>>>> + ret = a6xx_hfi_enable_acd(gmu); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> ret = a6xx_hfi_send_core_fw_start(gmu); >>>>> if (ret) >>>>> return ret; >>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/ >>>>> msm/adreno/a6xx_hfi.h >>>>> index 528110169398..51864c8ad0e6 100644 >>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>>> @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { >>>>> u32 header; >>>>> }; >>>>> +#define HFI_H2F_MSG_ACD 7 >>>>> +#define MAX_ACD_STRIDE 2 >>>>> + >>>>> +struct a6xx_hfi_acd_table { >>>>> + u32 header; >>>>> + u32 version; >>>>> + u32 enable_by_level; >>>>> + u32 stride; >>>>> + u32 num_levels; >>>>> + u32 data[16 * MAX_ACD_STRIDE]; >>>>> +}; >>>>> + >>>>> #define HFI_H2F_MSG_START 10 >>>>> struct a6xx_hfi_msg_start { >>>>> u32 header; >>>>> }; >>>>> +#define HFI_H2F_FEATURE_CTRL 11 >>>>> + >>>>> +struct a6xx_hfi_msg_feature_ctrl { >>>>> + u32 header; >>>>> + u32 feature; >>>>> + u32 enable; >>>>> + u32 data; >>>>> +}; >>>>> + >>>>> #define HFI_H2F_MSG_CORE_FW_START 14 >>>>> struct a6xx_hfi_msg_core_fw_start { >>>>> >>>> >>>> Thanks, >>>> Neil >>> >> >
On 11/7/2024 8:01 PM, neil.armstrong@linaro.org wrote: > On 07/11/2024 13:46, Akhil P Oommen wrote: >> On 11/7/2024 2:25 PM, neil.armstrong@linaro.org wrote: >>> On 06/11/2024 02:44, Akhil P Oommen wrote: >>>> On 11/4/2024 9:14 PM, neil.armstrong@linaro.org wrote: >>>>> On 11/10/2024 22:29, 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> >>>>>> --- >>>>>> 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 +++++++++ >>>>>> 4 files changed, 124 insertions(+), 15 deletions(-) >>>>>> >>>>> >>>>> <snip> >>>>> >>>>>> + >>>>>> +static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >>>>>> +{ >>>>>> + struct a6xx_hfi_acd_table *acd_table = &gmu->acd_table; >>>>>> + struct a6xx_hfi_msg_feature_ctrl msg = { >>>>>> + .feature = HFI_FEATURE_ACD, >>>>>> + .enable = 1, >>>>>> + .data = 0, >>>>>> + }; >>>>>> + int ret; >>>>>> + >>>>>> + if (!acd_table->enable_by_level) >>>>>> + return 0; >>>>>> + >>>>>> + /* Enable ACD feature at GMU */ >>>>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_FEATURE_CTRL, &msg, >>>>>> sizeof(msg), NULL, 0); >>>>>> + if (ret) { >>>>>> + DRM_DEV_ERROR(gmu->dev, "Unable to enable ACD (%d)\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + /* Send ACD table to GMU */ >>>>>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &msg, sizeof(msg), >>>>>> NULL, 0); >>>>> >>>>> This looks wrong, in this exact code, you never use the acd_table... >>>>> perhaps it should be acd_table here >>>> >>>> Whoops! Weirdly gmu didn't explode when I tested. >>>> >>>> Thanks for your keen eye. >>> >>> You're welcome ! >>> >>> I've been trying to enable this on SM8650, but HFI_H2F_MSG_ACD fails. >>> >>> My changes: >>> ================><================================ >>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/ >>> msm/adreno/a6xx_hfi.c >>> index 7c96d6f8aaa9..bd9d586f245e 100644 >>> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c >>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c >>> @@ -682,7 +682,7 @@ static int a6xx_hfi_enable_acd(struct a6xx_gmu *gmu) >>> } >>> >>> /* Send ACD table to GMU */ >>> - ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, >>> sizeof(*acd_table), NULL, 0); >>> + ret = a6xx_hfi_send_msg(gmu, HFI_H2F_MSG_ACD, &acd_table, >> >> &acd_table -> acd_table here? > > Damn, good catch ! > > Ok so it didn't explode anymore, but still fails: > ===== > [ 7.083258] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* Message HFI_H2F_MSG_GX_BW_PERF_VOTE id 7 timed out > waiting for response > [ 7.149709] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* Unexpected message id 7 on the response queue > [ 7.149744] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* The HFI response queue is unexpectedly empty > [ 7.165163] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* Unexpected message id 8 on the response queue > [ 7.165188] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 > [msm]] *ERROR* The HFI response queue is unexpectedly empty > ==== > > Seems with ACD enabled, first vote can take up to 100ms, and downstream > has 1s timeout, with a larger timeout I got it to work ! Yes, there is an additional overhead during first perf vote. Thanks for the heads up. I am yet to test with fixes. -Akhil. > > Thanks, > Neil >> >> -Akhil >> >>> sizeof(struct a6xx_hfi_acd_table), NULL, 0); >>> if (ret) { >>> DRM_DEV_ERROR(gmu->dev, "Unable to send ACD table >>> (%d)\n", ret); >>> return ret; >>> ================><================================ >>> >>> with the appropriate qcom,opp-acd-level in DT taken from downstream, >>> I get: >>> [ 6.946184] platform 3d6a000.gmu: [drm:a6xx_hfi_send_msg.constprop.0 >>> [msm]] *ERROR* Message (null) id 4 timed out waiting for response >>> [ 6.958697] platform 3d6a000.gmu: [drm:a6xx_hfi_start [msm]] *ERROR* >>> Unable to send ACD table (-110) >>> >>> is there something missing ? >>> >>> Neil >>> >>>> >>>> -Akhil. >>>> >>>>> >>>>>> + if (ret) { >>>>>> + DRM_DEV_ERROR(gmu->dev, "Unable to ACD table (%d)\n", ret); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int a6xx_hfi_send_test(struct a6xx_gmu *gmu) >>>>>> { >>>>>> struct a6xx_hfi_msg_test msg = { 0 }; >>>>>> @@ -756,6 +788,10 @@ int a6xx_hfi_start(struct a6xx_gmu *gmu, int >>>>>> boot_state) >>>>>> if (ret) >>>>>> return ret; >>>>>> + ret = a6xx_hfi_enable_acd(gmu); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> ret = a6xx_hfi_send_core_fw_start(gmu); >>>>>> if (ret) >>>>>> return ret; >>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h b/drivers/gpu/drm/ >>>>>> msm/adreno/a6xx_hfi.h >>>>>> index 528110169398..51864c8ad0e6 100644 >>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.h >>>>>> @@ -151,12 +151,33 @@ struct a6xx_hfi_msg_test { >>>>>> u32 header; >>>>>> }; >>>>>> +#define HFI_H2F_MSG_ACD 7 >>>>>> +#define MAX_ACD_STRIDE 2 >>>>>> + >>>>>> +struct a6xx_hfi_acd_table { >>>>>> + u32 header; >>>>>> + u32 version; >>>>>> + u32 enable_by_level; >>>>>> + u32 stride; >>>>>> + u32 num_levels; >>>>>> + u32 data[16 * MAX_ACD_STRIDE]; >>>>>> +}; >>>>>> + >>>>>> #define HFI_H2F_MSG_START 10 >>>>>> struct a6xx_hfi_msg_start { >>>>>> u32 header; >>>>>> }; >>>>>> +#define HFI_H2F_FEATURE_CTRL 11 >>>>>> + >>>>>> +struct a6xx_hfi_msg_feature_ctrl { >>>>>> + u32 header; >>>>>> + u32 feature; >>>>>> + u32 enable; >>>>>> + u32 data; >>>>>> +}; >>>>>> + >>>>>> #define HFI_H2F_MSG_CORE_FW_START 14 >>>>>> struct a6xx_hfi_msg_core_fw_start { >>>>>> >>>>> >>>>> Thanks, >>>>> Neil >>>> >>> >> >