[PATCH RFC 0/3] Support for GPU ACD feature on Adreno X1-85

[PATCH RFC 0/3] Support for GPU ACD feature on Adreno X1-85

[Akhil P Oommen]

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,
-- 
Akhil P Oommen <quic_akhilpo@quicinc.com>

[PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

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(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 37927bdd6fbe..09fb3f397dbb 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -1021,14 +1021,6 @@ int a6xx_gmu_resume(struct a6xx_gpu *a6xx_gpu)
 
 	gmu->hung = false;
 
-	/* Notify AOSS about the ACD state (unimplemented for now => disable it) */
-	if (!IS_ERR(gmu->qmp)) {
-		ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}",
-			       0 /* Hardcode ACD to be disabled for now */);
-		if (ret)
-			dev_err(gmu->dev, "failed to send GPU ACD state\n");
-	}
-
 	/* Turn on the resources */
 	pm_runtime_get_sync(gmu->dev);
 
@@ -1476,6 +1468,64 @@ static int a6xx_gmu_pwrlevels_probe(struct a6xx_gmu *gmu)
 	return a6xx_gmu_rpmh_votes_init(gmu);
 }
 
+static int a6xx_gmu_acd_probe(struct a6xx_gmu *gmu)
+{
+	struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
+	struct a6xx_hfi_acd_table *cmd = &gmu->acd_table;
+	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
+	struct msm_gpu *gpu = &adreno_gpu->base;
+	int ret, i, cmd_idx = 0;
+
+	cmd->version = 1;
+	cmd->stride = 1;
+	cmd->enable_by_level = 0;
+
+	/* Skip freq = 0 and parse acd-level for rest of the OPPs */
+	for (i = 1; i < gmu->nr_gpu_freqs; i++) {
+		struct dev_pm_opp *opp;
+		struct device_node *np;
+		unsigned long freq;
+		u32 val;
+
+		freq = gmu->gpu_freqs[i];
+		opp = dev_pm_opp_find_freq_exact(&gpu->pdev->dev, freq, true);
+		np = dev_pm_opp_get_of_node(opp);
+
+		ret = of_property_read_u32(np, "qcom,opp-acd-level", &val);
+		of_node_put(np);
+		dev_pm_opp_put(opp);
+		if (ret == -EINVAL)
+			continue;
+		else if (ret) {
+			DRM_DEV_ERROR(gmu->dev, "Unable to read acd level for freq %lu\n", freq);
+			return ret;
+		}
+
+		cmd->enable_by_level |= BIT(i);
+		cmd->data[cmd_idx++] = val;
+	}
+
+	cmd->num_levels = cmd_idx;
+
+	/* We are done here if ACD is not required for any of the OPPs */
+	if (!cmd->enable_by_level)
+		return 0;
+
+	/* 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");
+	}
+
+	/* Notify AOSS about the ACD state */
+	ret = qmp_send(gmu->qmp, "{class: gpu, res: acd, val: %d}", 1);
+	if (ret)
+		DRM_DEV_ERROR(gmu->dev, "failed to send GPU ACD state\n");
+
+	return 0;
+}
+
 static int a6xx_gmu_clocks_probe(struct a6xx_gmu *gmu)
 {
 	int ret = devm_clk_bulk_get_all(gmu->dev, &gmu->clocks);
@@ -1792,12 +1842,6 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 		goto detach_cxpd;
 	}
 
-	gmu->qmp = qmp_get(gmu->dev);
-	if (IS_ERR(gmu->qmp) && adreno_is_a7xx(adreno_gpu)) {
-		ret = PTR_ERR(gmu->qmp);
-		goto remove_device_link;
-	}
-
 	init_completion(&gmu->pd_gate);
 	complete_all(&gmu->pd_gate);
 	gmu->pd_nb.notifier_call = cxpd_notifier_cb;
@@ -1811,6 +1855,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 	/* Get the power levels for the GMU and GPU */
 	a6xx_gmu_pwrlevels_probe(gmu);
 
+	ret = a6xx_gmu_acd_probe(gmu);
+	if (ret)
+		goto detach_gxpd;
+
 	/* Set up the HFI queues */
 	a6xx_hfi_init(gmu);
 
@@ -1821,7 +1869,10 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node)
 
 	return 0;
 
-remove_device_link:
+detach_gxpd:
+	if (!IS_ERR_OR_NULL(gmu->gxpd))
+		dev_pm_domain_detach(gmu->gxpd, false);
+
 	device_link_del(link);
 
 detach_cxpd:
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index 94b6c5cab6f4..2690511149ed 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -81,6 +81,7 @@ struct a6xx_gmu {
 	int nr_gpu_freqs;
 	unsigned long gpu_freqs[16];
 	u32 gx_arc_votes[16];
+	struct a6xx_hfi_acd_table acd_table;
 
 	int nr_gmu_freqs;
 	unsigned long gmu_freqs[4];
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
index cdb3f6e74d3e..af94e339188b 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_hfi.c
@@ -659,6 +659,38 @@ static int a6xx_hfi_send_bw_table(struct a6xx_gmu *gmu)
 		NULL, 0);
 }
 
+#define HFI_FEATURE_ACD 12
+
+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);
+	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 {

-- 
2.45.2

[PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

Add a new schema which extends opp-v2 to support a new vendor specific
property required for Adreno GPUs found in Qualcomm's SoCs. The new
property called "qcom,opp-acd-level" carries a u32 value recommended
for each opp needs to be shared to GMU during runtime.

Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
---
 .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
new file mode 100644
index 000000000000..9fb828e9da86
--- /dev/null
+++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
@@ -0,0 +1,84 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Adreno compatible OPP supply
+
+description:
+  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
+  ACD related information tailored for the specific chipset. This binding
+  provides the information needed to describe such a hardware value.
+
+maintainers:
+  - Rob Clark <robdclark@gmail.com>
+
+allOf:
+  - $ref: opp-v2-base.yaml#
+
+properties:
+  compatible:
+    const: operating-points-v2-adreno
+
+patternProperties:
+  '^opp-?[0-9]+$':
+    type: object
+    additionalProperties: false
+
+    properties:
+      opp-hz: true
+
+      opp-level: true
+
+      opp-peak-kBps: true
+
+      opp-supported-hw: true
+
+      qcom,opp-acd-level:
+        description: |
+          A positive value representing the acd level associated with this
+          OPP node. This value is shared to GMU during GPU wake up. It may
+          not be present for some OPPs and GMU will disable ACD while
+          transitioning to that OPP.
+        $ref: /schemas/types.yaml#/definitions/uint32
+
+    required:
+      - opp-hz
+      - opp-level
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    gpu_opp_table: opp-table {
+        compatible = "operating-points-v2-adreno";
+
+        opp-550000000 {
+                opp-hz = /bits/ 64 <550000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
+                opp-peak-kBps = <6074219>;
+                qcom,opp-acd-level = <0xc0285ffd>;
+        };
+
+        opp-390000000 {
+                opp-hz = /bits/ 64 <390000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+                opp-peak-kBps = <3000000>;
+                qcom,opp-acd-level = <0xc0285ffd>;
+        };
+
+        opp-300000000 {
+                opp-hz = /bits/ 64 <300000000>;
+                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
+                opp-peak-kBps = <2136719>;
+                /* Intentionally left out qcom,opp-acd-level property here */
+        };
+
+    };

-- 
2.45.2

[PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Akhil P Oommen]

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";
 
 				opp-1100000000 {
 					opp-hz = /bits/ 64 <1100000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_TURBO_L1>;
 					opp-peak-kBps = <16500000>;
+					qcom,opp-acd-level = <0xa82a5ffd>;
 				};
 
 				opp-1000000000 {
 					opp-hz = /bits/ 64 <1000000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_TURBO>;
 					opp-peak-kBps = <14398438>;
+					qcom,opp-acd-level = <0xa82b5ffd>;
 				};
 
 				opp-925000000 {
 					opp-hz = /bits/ 64 <925000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_NOM_L1>;
 					opp-peak-kBps = <14398438>;
+					qcom,opp-acd-level = <0xa82b5ffd>;
 				};
 
 				opp-800000000 {
 					opp-hz = /bits/ 64 <800000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_NOM>;
 					opp-peak-kBps = <12449219>;
+					qcom,opp-acd-level = <0xa82c5ffd>;
 				};
 
 				opp-744000000 {
 					opp-hz = /bits/ 64 <744000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L2>;
 					opp-peak-kBps = <10687500>;
+					qcom,opp-acd-level = <0x882e5ffd>;
 				};
 
 				opp-687000000 {
 					opp-hz = /bits/ 64 <687000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
 					opp-peak-kBps = <8171875>;
+					qcom,opp-acd-level = <0x882e5ffd>;
 				};
 
 				opp-550000000 {
 					opp-hz = /bits/ 64 <550000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
 					opp-peak-kBps = <6074219>;
+					qcom,opp-acd-level = <0xc0285ffd>;
 				};
 
 				opp-390000000 {
 					opp-hz = /bits/ 64 <390000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
 					opp-peak-kBps = <3000000>;
+					qcom,opp-acd-level = <0xc0285ffd>;
 				};
 
 				opp-300000000 {
 					opp-hz = /bits/ 64 <300000000>;
 					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
 					opp-peak-kBps = <2136719>;
+					qcom,opp-acd-level = <0xc02b5ffd>;
 				};
 			};
 		};

-- 
2.45.2

Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Krzysztof Kozlowski]

On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
> Add a new schema which extends opp-v2 to support a new vendor specific
> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> property called "qcom,opp-acd-level" carries a u32 value recommended
> for each opp needs to be shared to GMU during runtime.
> 
> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> ---
>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> new file mode 100644
> index 000000000000..9fb828e9da86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Adreno compatible OPP supply
> +
> +description:
> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> +  ACD related information tailored for the specific chipset. This binding
> +  provides the information needed to describe such a hardware value.
> +
> +maintainers:
> +  - Rob Clark <robdclark@gmail.com>
> +
> +allOf:
> +  - $ref: opp-v2-base.yaml#
> +
> +properties:
> +  compatible:
> +    const: operating-points-v2-adreno
> +
> +patternProperties:
> +  '^opp-?[0-9]+$':
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      opp-hz: true
> +
> +      opp-level: true
> +
> +      opp-peak-kBps: true
> +
> +      opp-supported-hw: true
> +
> +      qcom,opp-acd-level:
> +        description: |
> +          A positive value representing the acd level associated with this

What is acd?

> +          OPP node. This value is shared to GMU during GPU wake up. It may

What is GMU?

> +          not be present for some OPPs and GMU will disable ACD while

acd or ACD?

> +          transitioning to that OPP.
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +
> +    required:
> +      - opp-hz
> +      - opp-level
> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +
> +    gpu_opp_table: opp-table {
> +        compatible = "operating-points-v2-adreno";
> +
> +        opp-550000000 {
> +                opp-hz = /bits/ 64 <550000000>;
> +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
> +                opp-peak-kBps = <6074219>;
> +                qcom,opp-acd-level = <0xc0285ffd>;
> +        };
> +
> +        opp-390000000 {
> +                opp-hz = /bits/ 64 <390000000>;
> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +                opp-peak-kBps = <3000000>;
> +                qcom,opp-acd-level = <0xc0285ffd>;

That's the same value used everywhere. What's the point? Just encode it
in the driver.

> +        };
> +
> +        opp-300000000 {
> +                opp-hz = /bits/ 64 <300000000>;
> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
> +                opp-peak-kBps = <2136719>;
> +                /* Intentionally left out qcom,opp-acd-level property here */
> +        };
> +

Stray blank line

> +    };
> 
> -- 
> 2.45.2
> 

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Krzysztof Kozlowski]

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

Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On Mon, Oct 14, 2024 at 09:39:01AM +0200, Krzysztof Kozlowski wrote:
> On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
> > Add a new schema which extends opp-v2 to support a new vendor specific
> > property required for Adreno GPUs found in Qualcomm's SoCs. The new
> > property called "qcom,opp-acd-level" carries a u32 value recommended
> > for each opp needs to be shared to GMU during runtime.
> > 
> > Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> > ---
> >  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> > new file mode 100644
> > index 000000000000..9fb828e9da86
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Adreno compatible OPP supply
> > +
> > +description:
> > +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> > +  ACD related information tailored for the specific chipset. This binding
> > +  provides the information needed to describe such a hardware value.
> > +
> > +maintainers:
> > +  - Rob Clark <robdclark@gmail.com>
> > +
> > +allOf:
> > +  - $ref: opp-v2-base.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: operating-points-v2-adreno
> > +
> > +patternProperties:
> > +  '^opp-?[0-9]+$':
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      opp-hz: true
> > +
> > +      opp-level: true
> > +
> > +      opp-peak-kBps: true
> > +
> > +      opp-supported-hw: true
> > +
> > +      qcom,opp-acd-level:
> > +        description: |
> > +          A positive value representing the acd level associated with this
> 
> What is acd?

Adaptive Clock Distribution, a fancy name for clock throttling during voltage
droop. I will update the description to capture this.

> 
> > +          OPP node. This value is shared to GMU during GPU wake up. It may
> 
> What is GMU?

A co-processor which does power management for Adreno GPU.

> 
> > +          not be present for some OPPs and GMU will disable ACD while
> 
> acd or ACD?

should be uppercase everywhere in description.

> 
> > +          transitioning to that OPP.
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +    required:
> > +      - opp-hz
> > +      - opp-level
> > +
> > +required:
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Drop blank line
> 
> > +    #include <dt-bindings/power/qcom-rpmpd.h>
> > +
> > +    gpu_opp_table: opp-table {
> > +        compatible = "operating-points-v2-adreno";
> > +
> > +        opp-550000000 {
> > +                opp-hz = /bits/ 64 <550000000>;
> > +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
> > +                opp-peak-kBps = <6074219>;
> > +                qcom,opp-acd-level = <0xc0285ffd>;
> > +        };
> > +
> > +        opp-390000000 {
> > +                opp-hz = /bits/ 64 <390000000>;
> > +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> > +                opp-peak-kBps = <3000000>;
> > +                qcom,opp-acd-level = <0xc0285ffd>;
> 
> That's the same value used everywhere. What's the point? Just encode it
> in the driver.

I will update this to keep a different value. In a real implmentation,
these values may vary between OPPs. For eg:, please check the DT patch
in this series:

https://patchwork.freedesktop.org/patch/619413/

-Akhil

> 
> > +        };
> > +
> > +        opp-300000000 {
> > +                opp-hz = /bits/ 64 <300000000>;
> > +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS_D1>;
> > +                opp-peak-kBps = <2136719>;
> > +                /* Intentionally left out qcom,opp-acd-level property here */
> > +        };
> > +
> 
> Stray blank line
> 
> > +    };
> > 
> > -- 
> > 2.45.2
> > 

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Akhil P Oommen]

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
> 

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Krzysztof Kozlowski]

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.
> 
> 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.


Best regards,
Krzysztof

Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Krzysztof Kozlowski]

On 15/10/2024 21:13, Akhil P Oommen wrote:
> On Mon, Oct 14, 2024 at 09:39:01AM +0200, Krzysztof Kozlowski wrote:
>> On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
>>> Add a new schema which extends opp-v2 to support a new vendor specific
>>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
>>> property called "qcom,opp-acd-level" carries a u32 value recommended
>>> for each opp needs to be shared to GMU during runtime.
>>>
>>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
>>> ---
>>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
>>>  1 file changed, 84 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>> new file mode 100644
>>> index 000000000000..9fb828e9da86
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
>>> @@ -0,0 +1,84 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Qualcomm Adreno compatible OPP supply
>>> +
>>> +description:
>>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
>>> +  ACD related information tailored for the specific chipset. This binding
>>> +  provides the information needed to describe such a hardware value.
>>> +
>>> +maintainers:
>>> +  - Rob Clark <robdclark@gmail.com>
>>> +
>>> +allOf:
>>> +  - $ref: opp-v2-base.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: operating-points-v2-adreno
>>> +
>>> +patternProperties:
>>> +  '^opp-?[0-9]+$':
>>> +    type: object
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      opp-hz: true
>>> +
>>> +      opp-level: true
>>> +
>>> +      opp-peak-kBps: true
>>> +
>>> +      opp-supported-hw: true
>>> +
>>> +      qcom,opp-acd-level:
>>> +        description: |
>>> +          A positive value representing the acd level associated with this
>>
>> What is acd?
> 
> Adaptive Clock Distribution, a fancy name for clock throttling during voltage
> droop. I will update the description to capture this.
> 
>>
>>> +          OPP node. This value is shared to GMU during GPU wake up. It may
>>
>> What is GMU?
> 
> A co-processor which does power management for Adreno GPU.

Everything, except obvious GPU, should be explained. GMU is not really
that obvious:
https://en.wikipedia.org/wiki/GMU

> 
>>
>>> +          not be present for some OPPs and GMU will disable ACD while
>>
>> acd or ACD?
> 
> should be uppercase everywhere in description.
> 
>>
>>> +          transitioning to that OPP.
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +
>>> +    required:
>>> +      - opp-hz
>>> +      - opp-level
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +
>>
>> Drop blank line
>>
>>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>>> +
>>> +    gpu_opp_table: opp-table {
>>> +        compatible = "operating-points-v2-adreno";
>>> +
>>> +        opp-550000000 {
>>> +                opp-hz = /bits/ 64 <550000000>;
>>> +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
>>> +                opp-peak-kBps = <6074219>;
>>> +                qcom,opp-acd-level = <0xc0285ffd>;
>>> +        };
>>> +
>>> +        opp-390000000 {
>>> +                opp-hz = /bits/ 64 <390000000>;
>>> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
>>> +                opp-peak-kBps = <3000000>;
>>> +                qcom,opp-acd-level = <0xc0285ffd>;
>>
>> That's the same value used everywhere. What's the point? Just encode it
>> in the driver.
> 
> I will update this to keep a different value. In a real implmentation,
> these values may vary between OPPs. For eg:, please check the DT patch
> in this series:
> 
> https://patchwork.freedesktop.org/patch/619413/

OK. I still have concerns that it is just some magic hex value. Which
looks exactly how downstream code. No explanation, no meaning: neither
in property description nor in actual value (at least I could not spot it).

And why this is hex? Unit of "level" is either some logical meaning,
like "high" or "low", or some unit, e.g. Hertz or kBps. None of them are
hex values in real world.

Best regards,
Krzysztof

Re: [PATCH RFC 2/3] dt-bindings: opp: Add v2-qcom-adreno vendor bindings

[Akhil P Oommen]

On Wed, Oct 16, 2024 at 09:53:58AM +0200, Krzysztof Kozlowski wrote:
> On 15/10/2024 21:13, Akhil P Oommen wrote:
> > On Mon, Oct 14, 2024 at 09:39:01AM +0200, Krzysztof Kozlowski wrote:
> >> On Sat, Oct 12, 2024 at 01:59:29AM +0530, Akhil P Oommen wrote:
> >>> Add a new schema which extends opp-v2 to support a new vendor specific
> >>> property required for Adreno GPUs found in Qualcomm's SoCs. The new
> >>> property called "qcom,opp-acd-level" carries a u32 value recommended
> >>> for each opp needs to be shared to GMU during runtime.
> >>>
> >>> Signed-off-by: Akhil P Oommen <quic_akhilpo@quicinc.com>
> >>> ---
> >>>  .../bindings/opp/opp-v2-qcom-adreno.yaml           | 84 ++++++++++++++++++++++
> >>>  1 file changed, 84 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>> new file mode 100644
> >>> index 000000000000..9fb828e9da86
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/opp/opp-v2-qcom-adreno.yaml
> >>> @@ -0,0 +1,84 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/opp/opp-v2-qcom-adreno.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Qualcomm Adreno compatible OPP supply
> >>> +
> >>> +description:
> >>> +  Adreno GPUs present in Qualcomm's Snapdragon chipsets uses an OPP specific
> >>> +  ACD related information tailored for the specific chipset. This binding
> >>> +  provides the information needed to describe such a hardware value.
> >>> +
> >>> +maintainers:
> >>> +  - Rob Clark <robdclark@gmail.com>
> >>> +
> >>> +allOf:
> >>> +  - $ref: opp-v2-base.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: operating-points-v2-adreno
> >>> +
> >>> +patternProperties:
> >>> +  '^opp-?[0-9]+$':
> >>> +    type: object
> >>> +    additionalProperties: false
> >>> +
> >>> +    properties:
> >>> +      opp-hz: true
> >>> +
> >>> +      opp-level: true
> >>> +
> >>> +      opp-peak-kBps: true
> >>> +
> >>> +      opp-supported-hw: true
> >>> +
> >>> +      qcom,opp-acd-level:
> >>> +        description: |
> >>> +          A positive value representing the acd level associated with this
> >>
> >> What is acd?
> > 
> > Adaptive Clock Distribution, a fancy name for clock throttling during voltage
> > droop. I will update the description to capture this.
> > 
> >>
> >>> +          OPP node. This value is shared to GMU during GPU wake up. It may
> >>
> >> What is GMU?
> > 
> > A co-processor which does power management for Adreno GPU.
> 
> Everything, except obvious GPU, should be explained. GMU is not really
> that obvious:
> https://en.wikipedia.org/wiki/GMU

Will do.

> 
> > 
> >>
> >>> +          not be present for some OPPs and GMU will disable ACD while
> >>
> >> acd or ACD?
> > 
> > should be uppercase everywhere in description.
> > 
> >>
> >>> +          transitioning to that OPP.
> >>> +        $ref: /schemas/types.yaml#/definitions/uint32
> >>> +
> >>> +    required:
> >>> +      - opp-hz
> >>> +      - opp-level
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +
> >>> +additionalProperties: false
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +
> >>
> >> Drop blank line
> >>
> >>> +    #include <dt-bindings/power/qcom-rpmpd.h>
> >>> +
> >>> +    gpu_opp_table: opp-table {
> >>> +        compatible = "operating-points-v2-adreno";
> >>> +
> >>> +        opp-550000000 {
> >>> +                opp-hz = /bits/ 64 <550000000>;
> >>> +                opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
> >>> +                opp-peak-kBps = <6074219>;
> >>> +                qcom,opp-acd-level = <0xc0285ffd>;
> >>> +        };
> >>> +
> >>> +        opp-390000000 {
> >>> +                opp-hz = /bits/ 64 <390000000>;
> >>> +                opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> >>> +                opp-peak-kBps = <3000000>;
> >>> +                qcom,opp-acd-level = <0xc0285ffd>;
> >>
> >> That's the same value used everywhere. What's the point? Just encode it
> >> in the driver.
> > 
> > I will update this to keep a different value. In a real implmentation,
> > these values may vary between OPPs. For eg:, please check the DT patch
> > in this series:
> > 
> > https://patchwork.freedesktop.org/patch/619413/
> 
> OK. I still have concerns that it is just some magic hex value. Which
> looks exactly how downstream code. No explanation, no meaning: neither
> in property description nor in actual value (at least I could not spot it).
> 
> And why this is hex? Unit of "level" is either some logical meaning,
> like "high" or "low", or some unit, e.g. Hertz or kBps. None of them are
> hex values in real world.

This value (which is identified after characterization) encodes a voltage
threshold for the ACD hardware and few other knobs required for each OPP.
The intepretation of the bitfields changes between SoCs.

Another point is that ACD is a requirement for higher GPU frequencies to
meet the hw spec. So OPP dt node is the natural place to keep this info,
which also helps to share this data between different OS.

-Akhil

> 
> Best regards,
> Krzysztof
> 

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Akhil P Oommen]

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
> 

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Krzysztof Kozlowski]

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

Re: [PATCH RFC 3/3] arm64: dts: qcom: x1e80100: Add ACD levels for GPU

[Akhil P Oommen]

On Thu, Oct 17, 2024 at 09:05:50AM +0200, Krzysztof Kozlowski wrote:
> 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.
> 
I still have questions about the practicality especially in IoT/Auto chipsets,
but I will try to get it clarified when I face them.

I will go ahead and send out the v2 series addressing the suggestions.

-Akhil.

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Konrad Dybcio]

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

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

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

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Konrad Dybcio]

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

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[neil.armstrong]

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

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

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

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[neil.armstrong]

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
> 

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

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
>>
> 

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[neil.armstrong]

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
>>>
>>
> 

Re: [PATCH RFC 1/3] drm/msm/adreno: Add support for ACD

[Akhil P Oommen]

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
>>>>
>>>
>>
>