[PATCH 0/5] OPP and devfreq for all Adrenos

[PATCH 0/5] OPP and devfreq for all Adrenos

[Konrad Dybcio]

This series is a combination of [1] and a subset of [2] and some new
stuff.

With it, devfreq is used on all a2xx-a6xx (including gmu and
gmu-wrapper) and all clk_set_rate(core clock) calls are dropped in
favour of dev_pm_opp_set_rate, which - drumroll - lets us scale
the voltage domain. DT patches making use of that will be sent
separately.

On top of that, a5xx gets a call to enable icc scaling from the OPP
tables. No SoCs implementing a2xx have icc support yet and a3/4xx
SoCs have separate logic for that, which will be updated at a later
time.

Getting this in for 6.4 early would be appreciated, as that would
allow for getting GMU wrapper GPUs up (without VDD&icc scaling they
can only run at lower freqs, which is.. ehhh..)

Changes:
- a3xx busy: use the _1 counter as per msm-3.x instead of _0
- a6xx-series-opp: basically rewrite, ensure compat with all gens
- a2/4xx busy: new patch
- a5xx icc: new patch

[1] https://lore.kernel.org/linux-arm-msm/20230130093809.2079314-1-konrad.dybcio@linaro.org/
[2] https://lore.kernel.org/linux-arm-msm/20230214173145.2482651-1-konrad.dybcio@linaro.org/

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (5):
      drm/msm/adreno: Use OPP for every GPU generation
      drm/msm/a2xx: Implement .gpu_busy
      drm/msm/a3xx: Implement .gpu_busy
      drm/msm/a4xx: Implement .gpu_busy
      drm/msm/a5xx: Enable optional icc voting from OPP tables

 drivers/gpu/drm/msm/adreno/a2xx_gpu.c   | 28 ++++++++++
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 11 ++++
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 11 ++++
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   |  4 ++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
 drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
 drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
 7 files changed, 99 insertions(+), 55 deletions(-)
---
base-commit: f4ed0868966d96203fee6f2782508746ded2ce3f
change-id: 20230222-konrad-longbois-next-86d1a69532c2

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 0/5] OPP and devfreq for all Adrenos

[Konrad Dybcio]

This series is a combination of [1] and a subset of [2] and some new
stuff.

With it, devfreq is used on all a2xx-a6xx (including gmu and
gmu-wrapper) and all clk_set_rate(core clock) calls are dropped in
favour of dev_pm_opp_set_rate, which - drumroll - lets us scale
the voltage domain. DT patches making use of that will be sent
separately.

On top of that, a5xx gets a call to enable icc scaling from the OPP
tables. No SoCs implementing a2xx have icc support yet and a3/4xx
SoCs have separate logic for that, which will be updated at a later
time.

Getting this in for 6.4 early would be appreciated, as that would
allow for getting GMU wrapper GPUs up (without VDD&icc scaling they
can only run at lower freqs, which is.. ehhh..)

Changes:
- a3xx busy: use the _1 counter as per msm-3.x instead of _0
- a6xx-series-opp: basically rewrite, ensure compat with all gens
- a2/4xx busy: new patch
- a5xx icc: new patch

[1] https://lore.kernel.org/linux-arm-msm/20230130093809.2079314-1-konrad.dybcio@linaro.org/
[2] https://lore.kernel.org/linux-arm-msm/20230214173145.2482651-1-konrad.dybcio@linaro.org/

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (5):
      drm/msm/adreno: Use OPP for every GPU generation
      drm/msm/a2xx: Implement .gpu_busy
      drm/msm/a3xx: Implement .gpu_busy
      drm/msm/a4xx: Implement .gpu_busy
      drm/msm/a5xx: Enable optional icc voting from OPP tables

 drivers/gpu/drm/msm/adreno/a2xx_gpu.c   | 28 ++++++++++
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   | 11 ++++
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   | 11 ++++
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c   |  4 ++
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
 drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
 drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
 7 files changed, 99 insertions(+), 55 deletions(-)
---
base-commit: f4ed0868966d96203fee6f2782508746ded2ce3f
change-id: 20230222-konrad-longbois-next-86d1a69532c2

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

Some older GPUs (namely a2xx with no opp tables at all and a320 with
downstream-remnants gpu pwrlevels) used not to have OPP tables. They
both however had just one frequency defined, making it extremely easy
to construct such an OPP table from within the driver if need be.

Do so and switch all clk_set_rate calls on core_clk to their OPP
counterparts.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
 drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
 drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
 3 files changed, 45 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index ce6b76c45b6f..9b940c0f063f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
 			ring->id);
 }
 
-/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
-static int adreno_get_legacy_pwrlevels(struct device *dev)
-{
-	struct device_node *child, *node;
-	int ret;
-
-	node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
-	if (!node) {
-		DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
-		return -ENXIO;
-	}
-
-	for_each_child_of_node(node, child) {
-		unsigned int val;
-
-		ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
-		if (ret)
-			continue;
-
-		/*
-		 * Skip the intentionally bogus clock value found at the bottom
-		 * of most legacy frequency tables
-		 */
-		if (val != 27000000)
-			dev_pm_opp_add(dev, val, 0);
-	}
-
-	of_node_put(node);
-
-	return 0;
-}
-
-static void adreno_get_pwrlevels(struct device *dev,
+static int adreno_get_pwrlevels(struct device *dev,
 		struct msm_gpu *gpu)
 {
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	unsigned long freq = ULONG_MAX;
 	struct dev_pm_opp *opp;
 	int ret;
 
 	gpu->fast_rate = 0;
 
-	/* You down with OPP? */
-	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
-		ret = adreno_get_legacy_pwrlevels(dev);
-	else {
-		ret = devm_pm_opp_of_add_table(dev);
-		if (ret)
-			DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
-	}
-
-	if (!ret) {
+	/* devm_pm_opp_of_add_table may error out but will still create an OPP table */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret == -ENODEV) {
+		/* Special cases for ancient hw with ancient DT bindings */
+		if (adreno_is_a2xx(adreno_gpu)) {
+			dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
+			dev_pm_opp_add(dev, 200000000, 0);
+			gpu->fast_rate = 200000000;
+		} else if (adreno_is_a320(adreno_gpu)) {
+			dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
+			dev_pm_opp_add(dev, 450000000, 0);
+			gpu->fast_rate = 450000000;
+		} else {
+			DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
+			return -ENODEV;
+		}
+	} else if (ret) {
+		DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
+		return ret;
+	} else {
 		/* Find the fastest defined rate */
 		opp = dev_pm_opp_find_freq_floor(dev, &freq);
-		if (!IS_ERR(opp)) {
+
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		else {
 			gpu->fast_rate = freq;
 			dev_pm_opp_put(opp);
 		}
 	}
 
-	if (!gpu->fast_rate) {
-		dev_warn(dev,
-			"Could not find a clock rate. Using a reasonable default\n");
-		/* Pick a suitably safe clock speed for any target */
-		gpu->fast_rate = 200000000;
-	}
-
 	DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
+
+	return 0;
 }
 
 int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
@@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	struct adreno_rev *rev = &config->rev;
 	const char *gpu_name;
 	u32 speedbin;
+	int ret;
+
+	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
+	if (IS_ERR(devm_clk_get(dev, "core"))) {
+		/*
+		 * If "core" is absent, go for the legacy clock name.
+		 * If we got this far in probing, it's a given one of them exists.
+		 */
+		devm_pm_opp_set_clkname(dev, "core_clk");
+	} else
+		devm_pm_opp_set_clkname(dev, "core");
 
 	adreno_gpu->funcs = funcs;
 	adreno_gpu->info = adreno_info(config->rev);
@@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 
 	adreno_gpu_config.nr_rings = nr_rings;
 
-	adreno_get_pwrlevels(dev, gpu);
+	ret = adreno_get_pwrlevels(dev, gpu);
+	if (ret)
+		return ret;
 
 	pm_runtime_set_autosuspend_delay(dev,
 		adreno_gpu->info->inactive_period);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 380249500325..cdcb00df3f25 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
 static int enable_clk(struct msm_gpu *gpu)
 {
 	if (gpu->core_clk && gpu->fast_rate)
-		clk_set_rate(gpu->core_clk, gpu->fast_rate);
+		dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
 
 	/* Set the RBBM timer rate to 19.2Mhz */
 	if (gpu->rbbmtimer_clk)
@@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
 	 * will be rounded down to zero anyway so it all works out.
 	 */
 	if (gpu->core_clk)
-		clk_set_rate(gpu->core_clk, 27000000);
+		dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
 
 	if (gpu->rbbmtimer_clk)
 		clk_set_rate(gpu->rbbmtimer_clk, 0);
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index e27dbf12b5e8..ea70c1c32d94 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
 		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
 		mutex_unlock(&df->lock);
 	} else {
-		clk_set_rate(gpu->core_clk, *freq);
+		dev_pm_opp_set_rate(dev, *freq);
 	}
 
 	dev_pm_opp_put(opp);

-- 
2.39.2

[PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

Some older GPUs (namely a2xx with no opp tables at all and a320 with
downstream-remnants gpu pwrlevels) used not to have OPP tables. They
both however had just one frequency defined, making it extremely easy
to construct such an OPP table from within the driver if need be.

Do so and switch all clk_set_rate calls on core_clk to their OPP
counterparts.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
 drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
 drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
 3 files changed, 45 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index ce6b76c45b6f..9b940c0f063f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
 			ring->id);
 }
 
-/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
-static int adreno_get_legacy_pwrlevels(struct device *dev)
-{
-	struct device_node *child, *node;
-	int ret;
-
-	node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
-	if (!node) {
-		DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
-		return -ENXIO;
-	}
-
-	for_each_child_of_node(node, child) {
-		unsigned int val;
-
-		ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
-		if (ret)
-			continue;
-
-		/*
-		 * Skip the intentionally bogus clock value found at the bottom
-		 * of most legacy frequency tables
-		 */
-		if (val != 27000000)
-			dev_pm_opp_add(dev, val, 0);
-	}
-
-	of_node_put(node);
-
-	return 0;
-}
-
-static void adreno_get_pwrlevels(struct device *dev,
+static int adreno_get_pwrlevels(struct device *dev,
 		struct msm_gpu *gpu)
 {
+	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	unsigned long freq = ULONG_MAX;
 	struct dev_pm_opp *opp;
 	int ret;
 
 	gpu->fast_rate = 0;
 
-	/* You down with OPP? */
-	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
-		ret = adreno_get_legacy_pwrlevels(dev);
-	else {
-		ret = devm_pm_opp_of_add_table(dev);
-		if (ret)
-			DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
-	}
-
-	if (!ret) {
+	/* devm_pm_opp_of_add_table may error out but will still create an OPP table */
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret == -ENODEV) {
+		/* Special cases for ancient hw with ancient DT bindings */
+		if (adreno_is_a2xx(adreno_gpu)) {
+			dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
+			dev_pm_opp_add(dev, 200000000, 0);
+			gpu->fast_rate = 200000000;
+		} else if (adreno_is_a320(adreno_gpu)) {
+			dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
+			dev_pm_opp_add(dev, 450000000, 0);
+			gpu->fast_rate = 450000000;
+		} else {
+			DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
+			return -ENODEV;
+		}
+	} else if (ret) {
+		DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
+		return ret;
+	} else {
 		/* Find the fastest defined rate */
 		opp = dev_pm_opp_find_freq_floor(dev, &freq);
-		if (!IS_ERR(opp)) {
+
+		if (IS_ERR(opp))
+			return PTR_ERR(opp);
+		else {
 			gpu->fast_rate = freq;
 			dev_pm_opp_put(opp);
 		}
 	}
 
-	if (!gpu->fast_rate) {
-		dev_warn(dev,
-			"Could not find a clock rate. Using a reasonable default\n");
-		/* Pick a suitably safe clock speed for any target */
-		gpu->fast_rate = 200000000;
-	}
-
 	DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
+
+	return 0;
 }
 
 int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
@@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	struct adreno_rev *rev = &config->rev;
 	const char *gpu_name;
 	u32 speedbin;
+	int ret;
+
+	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
+	if (IS_ERR(devm_clk_get(dev, "core"))) {
+		/*
+		 * If "core" is absent, go for the legacy clock name.
+		 * If we got this far in probing, it's a given one of them exists.
+		 */
+		devm_pm_opp_set_clkname(dev, "core_clk");
+	} else
+		devm_pm_opp_set_clkname(dev, "core");
 
 	adreno_gpu->funcs = funcs;
 	adreno_gpu->info = adreno_info(config->rev);
@@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 
 	adreno_gpu_config.nr_rings = nr_rings;
 
-	adreno_get_pwrlevels(dev, gpu);
+	ret = adreno_get_pwrlevels(dev, gpu);
+	if (ret)
+		return ret;
 
 	pm_runtime_set_autosuspend_delay(dev,
 		adreno_gpu->info->inactive_period);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 380249500325..cdcb00df3f25 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
 static int enable_clk(struct msm_gpu *gpu)
 {
 	if (gpu->core_clk && gpu->fast_rate)
-		clk_set_rate(gpu->core_clk, gpu->fast_rate);
+		dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
 
 	/* Set the RBBM timer rate to 19.2Mhz */
 	if (gpu->rbbmtimer_clk)
@@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
 	 * will be rounded down to zero anyway so it all works out.
 	 */
 	if (gpu->core_clk)
-		clk_set_rate(gpu->core_clk, 27000000);
+		dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
 
 	if (gpu->rbbmtimer_clk)
 		clk_set_rate(gpu->rbbmtimer_clk, 0);
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index e27dbf12b5e8..ea70c1c32d94 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
 		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
 		mutex_unlock(&df->lock);
 	} else {
-		clk_set_rate(gpu->core_clk, *freq);
+		dev_pm_opp_set_rate(dev, *freq);
 	}
 
 	dev_pm_opp_put(opp);

-- 
2.39.2

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Some older GPUs (namely a2xx with no opp tables at all and a320 with
> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
> both however had just one frequency defined, making it extremely easy
> to construct such an OPP table from within the driver if need be.
> 
> Do so and switch all clk_set_rate calls on core_clk to their OPP
> counterparts.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>   drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>   3 files changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index ce6b76c45b6f..9b940c0f063f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>   			ring->id);
>   }
>   
> -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
> -static int adreno_get_legacy_pwrlevels(struct device *dev)
> -{
> -	struct device_node *child, *node;
> -	int ret;
> -
> -	node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
> -	if (!node) {
> -		DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
> -		return -ENXIO;
> -	}
> -
> -	for_each_child_of_node(node, child) {
> -		unsigned int val;
> -
> -		ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
> -		if (ret)
> -			continue;
> -
> -		/*
> -		 * Skip the intentionally bogus clock value found at the bottom
> -		 * of most legacy frequency tables
> -		 */
> -		if (val != 27000000)
> -			dev_pm_opp_add(dev, val, 0);
> -	}
> -
> -	of_node_put(node);
> -
> -	return 0;
> -}
> -
> -static void adreno_get_pwrlevels(struct device *dev,
> +static int adreno_get_pwrlevels(struct device *dev,
>   		struct msm_gpu *gpu)
>   {
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	unsigned long freq = ULONG_MAX;
>   	struct dev_pm_opp *opp;
>   	int ret;
>   
>   	gpu->fast_rate = 0;
>   
> -	/* You down with OPP? */
> -	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
> -		ret = adreno_get_legacy_pwrlevels(dev);
> -	else {
> -		ret = devm_pm_opp_of_add_table(dev);
> -		if (ret)
> -			DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> -	}
> -
> -	if (!ret) {
> +	/* devm_pm_opp_of_add_table may error out but will still create an OPP table */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret == -ENODEV) {
> +		/* Special cases for ancient hw with ancient DT bindings */
> +		if (adreno_is_a2xx(adreno_gpu)) {
> +			dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
> +			dev_pm_opp_add(dev, 200000000, 0);
> +			gpu->fast_rate = 200000000;

We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will 
get it from our freshly generated opp table.

> +		} else if (adreno_is_a320(adreno_gpu)) {
> +			dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
> +			dev_pm_opp_add(dev, 450000000, 0);
> +			gpu->fast_rate = 450000000;
> +		} else {
> +			DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
> +			return -ENODEV;
> +		}
> +	} else if (ret) {
> +		DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> +		return ret;
> +	} else {
>   		/* Find the fastest defined rate */
>   		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> -		if (!IS_ERR(opp)) {
> +
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		else {
>   			gpu->fast_rate = freq;
>   			dev_pm_opp_put(opp);
>   		}
>   	}
>   
> -	if (!gpu->fast_rate) {
> -		dev_warn(dev,
> -			"Could not find a clock rate. Using a reasonable default\n");
> -		/* Pick a suitably safe clock speed for any target */
> -		gpu->fast_rate = 200000000;
> -	}
> -
>   	DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
> +
> +	return 0;
>   }
>   
>   int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> @@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   	struct adreno_rev *rev = &config->rev;
>   	const char *gpu_name;
>   	u32 speedbin;
> +	int ret;
> +
> +	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */

I'd rephrase this to '...done before devm_pm_opp_of_add_table(), or 
dev_pm_opp_set_config() will...'. It took me a while to find correct 
limitation.

I wanted to move the code below to msm_gpu_init(), but after digging in 
found that it's not possible.


> +	if (IS_ERR(devm_clk_get(dev, "core"))) {
> +		/*
> +		 * If "core" is absent, go for the legacy clock name.
> +		 * If we got this far in probing, it's a given one of them exists.
> +		 */
> +		devm_pm_opp_set_clkname(dev, "core_clk");
> +	} else
> +		devm_pm_opp_set_clkname(dev, "core");
>   
>   	adreno_gpu->funcs = funcs;
>   	adreno_gpu->info = adreno_info(config->rev);
> @@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   
>   	adreno_gpu_config.nr_rings = nr_rings;
>   
> -	adreno_get_pwrlevels(dev, gpu);
> +	ret = adreno_get_pwrlevels(dev, gpu);
> +	if (ret)
> +		return ret;
>   
>   	pm_runtime_set_autosuspend_delay(dev,
>   		adreno_gpu->info->inactive_period);
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 380249500325..cdcb00df3f25 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
>   static int enable_clk(struct msm_gpu *gpu)
>   {
>   	if (gpu->core_clk && gpu->fast_rate)
> -		clk_set_rate(gpu->core_clk, gpu->fast_rate);
> +		dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
>   
>   	/* Set the RBBM timer rate to 19.2Mhz */
>   	if (gpu->rbbmtimer_clk)
> @@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
>   	 * will be rounded down to zero anyway so it all works out.
>   	 */
>   	if (gpu->core_clk)
> -		clk_set_rate(gpu->core_clk, 27000000);
> +		dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
>   
>   	if (gpu->rbbmtimer_clk)
>   		clk_set_rate(gpu->rbbmtimer_clk, 0);
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index e27dbf12b5e8..ea70c1c32d94 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>   		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>   		mutex_unlock(&df->lock);
>   	} else {
> -		clk_set_rate(gpu->core_clk, *freq);
> +		dev_pm_opp_set_rate(dev, *freq);
>   	}
>   
>   	dev_pm_opp_put(opp);
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Some older GPUs (namely a2xx with no opp tables at all and a320 with
> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
> both however had just one frequency defined, making it extremely easy
> to construct such an OPP table from within the driver if need be.
> 
> Do so and switch all clk_set_rate calls on core_clk to their OPP
> counterparts.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>   drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>   3 files changed, 45 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index ce6b76c45b6f..9b940c0f063f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>   			ring->id);
>   }
>   
> -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
> -static int adreno_get_legacy_pwrlevels(struct device *dev)
> -{
> -	struct device_node *child, *node;
> -	int ret;
> -
> -	node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
> -	if (!node) {
> -		DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
> -		return -ENXIO;
> -	}
> -
> -	for_each_child_of_node(node, child) {
> -		unsigned int val;
> -
> -		ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
> -		if (ret)
> -			continue;
> -
> -		/*
> -		 * Skip the intentionally bogus clock value found at the bottom
> -		 * of most legacy frequency tables
> -		 */
> -		if (val != 27000000)
> -			dev_pm_opp_add(dev, val, 0);
> -	}
> -
> -	of_node_put(node);
> -
> -	return 0;
> -}
> -
> -static void adreno_get_pwrlevels(struct device *dev,
> +static int adreno_get_pwrlevels(struct device *dev,
>   		struct msm_gpu *gpu)
>   {
> +	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>   	unsigned long freq = ULONG_MAX;
>   	struct dev_pm_opp *opp;
>   	int ret;
>   
>   	gpu->fast_rate = 0;
>   
> -	/* You down with OPP? */
> -	if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
> -		ret = adreno_get_legacy_pwrlevels(dev);
> -	else {
> -		ret = devm_pm_opp_of_add_table(dev);
> -		if (ret)
> -			DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> -	}
> -
> -	if (!ret) {
> +	/* devm_pm_opp_of_add_table may error out but will still create an OPP table */
> +	ret = devm_pm_opp_of_add_table(dev);
> +	if (ret == -ENODEV) {
> +		/* Special cases for ancient hw with ancient DT bindings */
> +		if (adreno_is_a2xx(adreno_gpu)) {
> +			dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
> +			dev_pm_opp_add(dev, 200000000, 0);
> +			gpu->fast_rate = 200000000;

We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will 
get it from our freshly generated opp table.

> +		} else if (adreno_is_a320(adreno_gpu)) {
> +			dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
> +			dev_pm_opp_add(dev, 450000000, 0);
> +			gpu->fast_rate = 450000000;
> +		} else {
> +			DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
> +			return -ENODEV;
> +		}
> +	} else if (ret) {
> +		DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> +		return ret;
> +	} else {
>   		/* Find the fastest defined rate */
>   		opp = dev_pm_opp_find_freq_floor(dev, &freq);
> -		if (!IS_ERR(opp)) {
> +
> +		if (IS_ERR(opp))
> +			return PTR_ERR(opp);
> +		else {
>   			gpu->fast_rate = freq;
>   			dev_pm_opp_put(opp);
>   		}
>   	}
>   
> -	if (!gpu->fast_rate) {
> -		dev_warn(dev,
> -			"Could not find a clock rate. Using a reasonable default\n");
> -		/* Pick a suitably safe clock speed for any target */
> -		gpu->fast_rate = 200000000;
> -	}
> -
>   	DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
> +
> +	return 0;
>   }
>   
>   int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> @@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   	struct adreno_rev *rev = &config->rev;
>   	const char *gpu_name;
>   	u32 speedbin;
> +	int ret;
> +
> +	/* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */

I'd rephrase this to '...done before devm_pm_opp_of_add_table(), or 
dev_pm_opp_set_config() will...'. It took me a while to find correct 
limitation.

I wanted to move the code below to msm_gpu_init(), but after digging in 
found that it's not possible.


> +	if (IS_ERR(devm_clk_get(dev, "core"))) {
> +		/*
> +		 * If "core" is absent, go for the legacy clock name.
> +		 * If we got this far in probing, it's a given one of them exists.
> +		 */
> +		devm_pm_opp_set_clkname(dev, "core_clk");
> +	} else
> +		devm_pm_opp_set_clkname(dev, "core");
>   
>   	adreno_gpu->funcs = funcs;
>   	adreno_gpu->info = adreno_info(config->rev);
> @@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>   
>   	adreno_gpu_config.nr_rings = nr_rings;
>   
> -	adreno_get_pwrlevels(dev, gpu);
> +	ret = adreno_get_pwrlevels(dev, gpu);
> +	if (ret)
> +		return ret;
>   
>   	pm_runtime_set_autosuspend_delay(dev,
>   		adreno_gpu->info->inactive_period);
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 380249500325..cdcb00df3f25 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
>   static int enable_clk(struct msm_gpu *gpu)
>   {
>   	if (gpu->core_clk && gpu->fast_rate)
> -		clk_set_rate(gpu->core_clk, gpu->fast_rate);
> +		dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
>   
>   	/* Set the RBBM timer rate to 19.2Mhz */
>   	if (gpu->rbbmtimer_clk)
> @@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
>   	 * will be rounded down to zero anyway so it all works out.
>   	 */
>   	if (gpu->core_clk)
> -		clk_set_rate(gpu->core_clk, 27000000);
> +		dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
>   
>   	if (gpu->rbbmtimer_clk)
>   		clk_set_rate(gpu->rbbmtimer_clk, 0);
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index e27dbf12b5e8..ea70c1c32d94 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>   		gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>   		mutex_unlock(&df->lock);
>   	} else {
> -		clk_set_rate(gpu->core_clk, *freq);
> +		dev_pm_opp_set_rate(dev, *freq);
>   	}
>   
>   	dev_pm_opp_put(opp);
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

On 22.02.2023 23:38, Dmitry Baryshkov wrote:
> On 22/02/2023 23:47, Konrad Dybcio wrote:
>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>> both however had just one frequency defined, making it extremely easy
>> to construct such an OPP table from within the driver if need be.
>>
>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>> counterparts.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>   drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>   3 files changed, 45 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index ce6b76c45b6f..9b940c0f063f 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>               ring->id);
>>   }
>>   -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>> -{
>> -    struct device_node *child, *node;
>> -    int ret;
>> -
>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>> -    if (!node) {
>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>> -        return -ENXIO;
>> -    }
>> -
>> -    for_each_child_of_node(node, child) {
>> -        unsigned int val;
>> -
>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>> -        if (ret)
>> -            continue;
>> -
>> -        /*
>> -         * Skip the intentionally bogus clock value found at the bottom
>> -         * of most legacy frequency tables
>> -         */
>> -        if (val != 27000000)
>> -            dev_pm_opp_add(dev, val, 0);
>> -    }
>> -
>> -    of_node_put(node);
>> -
>> -    return 0;
>> -}
>> -
>> -static void adreno_get_pwrlevels(struct device *dev,
>> +static int adreno_get_pwrlevels(struct device *dev,
>>           struct msm_gpu *gpu)
>>   {
>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       unsigned long freq = ULONG_MAX;
>>       struct dev_pm_opp *opp;
>>       int ret;
>>         gpu->fast_rate = 0;
>>   -    /* You down with OPP? */
>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>> -        ret = adreno_get_legacy_pwrlevels(dev);
>> -    else {
>> -        ret = devm_pm_opp_of_add_table(dev);
>> -        if (ret)
>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>> -    }
>> -
>> -    if (!ret) {
>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>> +    ret = devm_pm_opp_of_add_table(dev);
>> +    if (ret == -ENODEV) {
>> +        /* Special cases for ancient hw with ancient DT bindings */
>> +        if (adreno_is_a2xx(adreno_gpu)) {
>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>> +            dev_pm_opp_add(dev, 200000000, 0);
>> +            gpu->fast_rate = 200000000;
> 
> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
It's not reached in this code path.

> 
>> +        } else if (adreno_is_a320(adreno_gpu)) {
>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>> +            dev_pm_opp_add(dev, 450000000, 0);
>> +            gpu->fast_rate = 450000000;
>> +        } else {
>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>> +            return -ENODEV;
>> +        }
>> +    } else if (ret) {
>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>> +        return ret;
>> +    } else {
>>           /* Find the fastest defined rate */
>>           opp = dev_pm_opp_find_freq_floor(dev, &freq);
>> -        if (!IS_ERR(opp)) {
>> +
>> +        if (IS_ERR(opp))
>> +            return PTR_ERR(opp);
>> +        else {
>>               gpu->fast_rate = freq;
>>               dev_pm_opp_put(opp);
>>           }
>>       }
>>   -    if (!gpu->fast_rate) {
>> -        dev_warn(dev,
>> -            "Could not find a clock rate. Using a reasonable default\n");
>> -        /* Pick a suitably safe clock speed for any target */
>> -        gpu->fast_rate = 200000000;
>> -    }
>> -
>>       DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>> +
>> +    return 0;
>>   }
>>     int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
>> @@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>       struct adreno_rev *rev = &config->rev;
>>       const char *gpu_name;
>>       u32 speedbin;
>> +    int ret;
>> +
>> +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
> 
> I'd rephrase this to '...done before devm_pm_opp_of_add_table(), or dev_pm_opp_set_config() will...'. It took me a while to find correct limitation.
> 
> I wanted to move the code below to msm_gpu_init(), but after digging in found that it's not possible.
Ack, I wasted some time on this too..

Konrad
> 
> 
>> +    if (IS_ERR(devm_clk_get(dev, "core"))) {
>> +        /*
>> +         * If "core" is absent, go for the legacy clock name.
>> +         * If we got this far in probing, it's a given one of them exists.
>> +         */
>> +        devm_pm_opp_set_clkname(dev, "core_clk");
>> +    } else
>> +        devm_pm_opp_set_clkname(dev, "core");
>>         adreno_gpu->funcs = funcs;
>>       adreno_gpu->info = adreno_info(config->rev);
>> @@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>         adreno_gpu_config.nr_rings = nr_rings;
>>   -    adreno_get_pwrlevels(dev, gpu);
>> +    ret = adreno_get_pwrlevels(dev, gpu);
>> +    if (ret)
>> +        return ret;
>>         pm_runtime_set_autosuspend_delay(dev,
>>           adreno_gpu->info->inactive_period);
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>> index 380249500325..cdcb00df3f25 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>> @@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
>>   static int enable_clk(struct msm_gpu *gpu)
>>   {
>>       if (gpu->core_clk && gpu->fast_rate)
>> -        clk_set_rate(gpu->core_clk, gpu->fast_rate);
>> +        dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
>>         /* Set the RBBM timer rate to 19.2Mhz */
>>       if (gpu->rbbmtimer_clk)
>> @@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
>>        * will be rounded down to zero anyway so it all works out.
>>        */
>>       if (gpu->core_clk)
>> -        clk_set_rate(gpu->core_clk, 27000000);
>> +        dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
>>         if (gpu->rbbmtimer_clk)
>>           clk_set_rate(gpu->rbbmtimer_clk, 0);
>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> index e27dbf12b5e8..ea70c1c32d94 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>           gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>           mutex_unlock(&df->lock);
>>       } else {
>> -        clk_set_rate(gpu->core_clk, *freq);
>> +        dev_pm_opp_set_rate(dev, *freq);
>>       }
>>         dev_pm_opp_put(opp);
>>
> 

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

On 22.02.2023 23:38, Dmitry Baryshkov wrote:
> On 22/02/2023 23:47, Konrad Dybcio wrote:
>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>> both however had just one frequency defined, making it extremely easy
>> to construct such an OPP table from within the driver if need be.
>>
>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>> counterparts.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>   drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>   drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>   3 files changed, 45 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> index ce6b76c45b6f..9b940c0f063f 100644
>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>               ring->id);
>>   }
>>   -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>> -{
>> -    struct device_node *child, *node;
>> -    int ret;
>> -
>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>> -    if (!node) {
>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>> -        return -ENXIO;
>> -    }
>> -
>> -    for_each_child_of_node(node, child) {
>> -        unsigned int val;
>> -
>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>> -        if (ret)
>> -            continue;
>> -
>> -        /*
>> -         * Skip the intentionally bogus clock value found at the bottom
>> -         * of most legacy frequency tables
>> -         */
>> -        if (val != 27000000)
>> -            dev_pm_opp_add(dev, val, 0);
>> -    }
>> -
>> -    of_node_put(node);
>> -
>> -    return 0;
>> -}
>> -
>> -static void adreno_get_pwrlevels(struct device *dev,
>> +static int adreno_get_pwrlevels(struct device *dev,
>>           struct msm_gpu *gpu)
>>   {
>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>       unsigned long freq = ULONG_MAX;
>>       struct dev_pm_opp *opp;
>>       int ret;
>>         gpu->fast_rate = 0;
>>   -    /* You down with OPP? */
>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>> -        ret = adreno_get_legacy_pwrlevels(dev);
>> -    else {
>> -        ret = devm_pm_opp_of_add_table(dev);
>> -        if (ret)
>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>> -    }
>> -
>> -    if (!ret) {
>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>> +    ret = devm_pm_opp_of_add_table(dev);
>> +    if (ret == -ENODEV) {
>> +        /* Special cases for ancient hw with ancient DT bindings */
>> +        if (adreno_is_a2xx(adreno_gpu)) {
>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>> +            dev_pm_opp_add(dev, 200000000, 0);
>> +            gpu->fast_rate = 200000000;
> 
> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
It's not reached in this code path.

> 
>> +        } else if (adreno_is_a320(adreno_gpu)) {
>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>> +            dev_pm_opp_add(dev, 450000000, 0);
>> +            gpu->fast_rate = 450000000;
>> +        } else {
>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>> +            return -ENODEV;
>> +        }
>> +    } else if (ret) {
>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>> +        return ret;
>> +    } else {
>>           /* Find the fastest defined rate */
>>           opp = dev_pm_opp_find_freq_floor(dev, &freq);
>> -        if (!IS_ERR(opp)) {
>> +
>> +        if (IS_ERR(opp))
>> +            return PTR_ERR(opp);
>> +        else {
>>               gpu->fast_rate = freq;
>>               dev_pm_opp_put(opp);
>>           }
>>       }
>>   -    if (!gpu->fast_rate) {
>> -        dev_warn(dev,
>> -            "Could not find a clock rate. Using a reasonable default\n");
>> -        /* Pick a suitably safe clock speed for any target */
>> -        gpu->fast_rate = 200000000;
>> -    }
>> -
>>       DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>> +
>> +    return 0;
>>   }
>>     int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
>> @@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>       struct adreno_rev *rev = &config->rev;
>>       const char *gpu_name;
>>       u32 speedbin;
>> +    int ret;
>> +
>> +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
> 
> I'd rephrase this to '...done before devm_pm_opp_of_add_table(), or dev_pm_opp_set_config() will...'. It took me a while to find correct limitation.
> 
> I wanted to move the code below to msm_gpu_init(), but after digging in found that it's not possible.
Ack, I wasted some time on this too..

Konrad
> 
> 
>> +    if (IS_ERR(devm_clk_get(dev, "core"))) {
>> +        /*
>> +         * If "core" is absent, go for the legacy clock name.
>> +         * If we got this far in probing, it's a given one of them exists.
>> +         */
>> +        devm_pm_opp_set_clkname(dev, "core_clk");
>> +    } else
>> +        devm_pm_opp_set_clkname(dev, "core");
>>         adreno_gpu->funcs = funcs;
>>       adreno_gpu->info = adreno_info(config->rev);
>> @@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>         adreno_gpu_config.nr_rings = nr_rings;
>>   -    adreno_get_pwrlevels(dev, gpu);
>> +    ret = adreno_get_pwrlevels(dev, gpu);
>> +    if (ret)
>> +        return ret;
>>         pm_runtime_set_autosuspend_delay(dev,
>>           adreno_gpu->info->inactive_period);
>> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
>> index 380249500325..cdcb00df3f25 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu.c
>> @@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
>>   static int enable_clk(struct msm_gpu *gpu)
>>   {
>>       if (gpu->core_clk && gpu->fast_rate)
>> -        clk_set_rate(gpu->core_clk, gpu->fast_rate);
>> +        dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
>>         /* Set the RBBM timer rate to 19.2Mhz */
>>       if (gpu->rbbmtimer_clk)
>> @@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
>>        * will be rounded down to zero anyway so it all works out.
>>        */
>>       if (gpu->core_clk)
>> -        clk_set_rate(gpu->core_clk, 27000000);
>> +        dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
>>         if (gpu->rbbmtimer_clk)
>>           clk_set_rate(gpu->rbbmtimer_clk, 0);
>> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> index e27dbf12b5e8..ea70c1c32d94 100644
>> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
>> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
>>           gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
>>           mutex_unlock(&df->lock);
>>       } else {
>> -        clk_set_rate(gpu->core_clk, *freq);
>> +        dev_pm_opp_set_rate(dev, *freq);
>>       }
>>         dev_pm_opp_put(opp);
>>
> 

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Dmitry Baryshkov]

On 23/02/2023 00:40, Konrad Dybcio wrote:
> 
> 
> On 22.02.2023 23:38, Dmitry Baryshkov wrote:
>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>>> both however had just one frequency defined, making it extremely easy
>>> to construct such an OPP table from within the driver if need be.
>>>
>>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>>> counterparts.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>>    drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>>    3 files changed, 45 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index ce6b76c45b6f..9b940c0f063f 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>>                ring->id);
>>>    }
>>>    -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>>> -{
>>> -    struct device_node *child, *node;
>>> -    int ret;
>>> -
>>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>>> -    if (!node) {
>>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>>> -        return -ENXIO;
>>> -    }
>>> -
>>> -    for_each_child_of_node(node, child) {
>>> -        unsigned int val;
>>> -
>>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>>> -        if (ret)
>>> -            continue;
>>> -
>>> -        /*
>>> -         * Skip the intentionally bogus clock value found at the bottom
>>> -         * of most legacy frequency tables
>>> -         */
>>> -        if (val != 27000000)
>>> -            dev_pm_opp_add(dev, val, 0);
>>> -    }
>>> -
>>> -    of_node_put(node);
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static void adreno_get_pwrlevels(struct device *dev,
>>> +static int adreno_get_pwrlevels(struct device *dev,
>>>            struct msm_gpu *gpu)
>>>    {
>>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>        unsigned long freq = ULONG_MAX;
>>>        struct dev_pm_opp *opp;
>>>        int ret;
>>>          gpu->fast_rate = 0;
>>>    -    /* You down with OPP? */
>>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>>> -        ret = adreno_get_legacy_pwrlevels(dev);
>>> -    else {
>>> -        ret = devm_pm_opp_of_add_table(dev);
>>> -        if (ret)
>>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> -    }
>>> -
>>> -    if (!ret) {
>>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>>> +    ret = devm_pm_opp_of_add_table(dev);
>>> +    if (ret == -ENODEV) {
>>> +        /* Special cases for ancient hw with ancient DT bindings */
>>> +        if (adreno_is_a2xx(adreno_gpu)) {
>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>>> +            dev_pm_opp_add(dev, 200000000, 0);
>>> +            gpu->fast_rate = 200000000;
>>
>> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
> It's not reached in this code path.

I see. I got lost in all the ifs. What do you think about turning it 
into the main code path, since after this code block we always have a 
valid OPP table?

> 
>>
>>> +        } else if (adreno_is_a320(adreno_gpu)) {
>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>>> +            dev_pm_opp_add(dev, 450000000, 0);
>>> +            gpu->fast_rate = 450000000;
>>> +        } else {
>>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>>> +            return -ENODEV;
>>> +        }
>>> +    } else if (ret) {
>>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> +        return ret;
>>> +    } else {
>>>            /* Find the fastest defined rate */
>>>            opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>> -        if (!IS_ERR(opp)) {
>>> +
>>> +        if (IS_ERR(opp))
>>> +            return PTR_ERR(opp);
>>> +        else {
>>>                gpu->fast_rate = freq;
>>>                dev_pm_opp_put(opp);
>>>            }
>>>        }
>>>    -    if (!gpu->fast_rate) {
>>> -        dev_warn(dev,
>>> -            "Could not find a clock rate. Using a reasonable default\n");
>>> -        /* Pick a suitably safe clock speed for any target */
>>> -        gpu->fast_rate = 200000000;
>>> -    }
>>> -
>>>        DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>>> +
>>> +    return 0;
>>>    }
>>>      int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,


-- 
With best wishes
Dmitry

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Dmitry Baryshkov]

On 23/02/2023 00:40, Konrad Dybcio wrote:
> 
> 
> On 22.02.2023 23:38, Dmitry Baryshkov wrote:
>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>>> both however had just one frequency defined, making it extremely easy
>>> to construct such an OPP table from within the driver if need be.
>>>
>>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>>> counterparts.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>>    drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>>    3 files changed, 45 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index ce6b76c45b6f..9b940c0f063f 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>>                ring->id);
>>>    }
>>>    -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>>> -{
>>> -    struct device_node *child, *node;
>>> -    int ret;
>>> -
>>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>>> -    if (!node) {
>>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>>> -        return -ENXIO;
>>> -    }
>>> -
>>> -    for_each_child_of_node(node, child) {
>>> -        unsigned int val;
>>> -
>>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>>> -        if (ret)
>>> -            continue;
>>> -
>>> -        /*
>>> -         * Skip the intentionally bogus clock value found at the bottom
>>> -         * of most legacy frequency tables
>>> -         */
>>> -        if (val != 27000000)
>>> -            dev_pm_opp_add(dev, val, 0);
>>> -    }
>>> -
>>> -    of_node_put(node);
>>> -
>>> -    return 0;
>>> -}
>>> -
>>> -static void adreno_get_pwrlevels(struct device *dev,
>>> +static int adreno_get_pwrlevels(struct device *dev,
>>>            struct msm_gpu *gpu)
>>>    {
>>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>        unsigned long freq = ULONG_MAX;
>>>        struct dev_pm_opp *opp;
>>>        int ret;
>>>          gpu->fast_rate = 0;
>>>    -    /* You down with OPP? */
>>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>>> -        ret = adreno_get_legacy_pwrlevels(dev);
>>> -    else {
>>> -        ret = devm_pm_opp_of_add_table(dev);
>>> -        if (ret)
>>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> -    }
>>> -
>>> -    if (!ret) {
>>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>>> +    ret = devm_pm_opp_of_add_table(dev);
>>> +    if (ret == -ENODEV) {
>>> +        /* Special cases for ancient hw with ancient DT bindings */
>>> +        if (adreno_is_a2xx(adreno_gpu)) {
>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>>> +            dev_pm_opp_add(dev, 200000000, 0);
>>> +            gpu->fast_rate = 200000000;
>>
>> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
> It's not reached in this code path.

I see. I got lost in all the ifs. What do you think about turning it 
into the main code path, since after this code block we always have a 
valid OPP table?

> 
>>
>>> +        } else if (adreno_is_a320(adreno_gpu)) {
>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>>> +            dev_pm_opp_add(dev, 450000000, 0);
>>> +            gpu->fast_rate = 450000000;
>>> +        } else {
>>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>>> +            return -ENODEV;
>>> +        }
>>> +    } else if (ret) {
>>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>> +        return ret;
>>> +    } else {
>>>            /* Find the fastest defined rate */
>>>            opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>> -        if (!IS_ERR(opp)) {
>>> +
>>> +        if (IS_ERR(opp))
>>> +            return PTR_ERR(opp);
>>> +        else {
>>>                gpu->fast_rate = freq;
>>>                dev_pm_opp_put(opp);
>>>            }
>>>        }
>>>    -    if (!gpu->fast_rate) {
>>> -        dev_warn(dev,
>>> -            "Could not find a clock rate. Using a reasonable default\n");
>>> -        /* Pick a suitably safe clock speed for any target */
>>> -        gpu->fast_rate = 200000000;
>>> -    }
>>> -
>>>        DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>>> +
>>> +    return 0;
>>>    }
>>>      int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,


-- 
With best wishes
Dmitry

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

On 23.02.2023 00:16, Dmitry Baryshkov wrote:
> On 23/02/2023 00:40, Konrad Dybcio wrote:
>>
>>
>> On 22.02.2023 23:38, Dmitry Baryshkov wrote:
>>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>>>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>>>> both however had just one frequency defined, making it extremely easy
>>>> to construct such an OPP table from within the driver if need be.
>>>>
>>>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>>>> counterparts.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>>>    drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>>>    3 files changed, 45 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> index ce6b76c45b6f..9b940c0f063f 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>>>                ring->id);
>>>>    }
>>>>    -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>>>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>>>> -{
>>>> -    struct device_node *child, *node;
>>>> -    int ret;
>>>> -
>>>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>>>> -    if (!node) {
>>>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>>>> -        return -ENXIO;
>>>> -    }
>>>> -
>>>> -    for_each_child_of_node(node, child) {
>>>> -        unsigned int val;
>>>> -
>>>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>>>> -        if (ret)
>>>> -            continue;
>>>> -
>>>> -        /*
>>>> -         * Skip the intentionally bogus clock value found at the bottom
>>>> -         * of most legacy frequency tables
>>>> -         */
>>>> -        if (val != 27000000)
>>>> -            dev_pm_opp_add(dev, val, 0);
>>>> -    }
>>>> -
>>>> -    of_node_put(node);
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -
>>>> -static void adreno_get_pwrlevels(struct device *dev,
>>>> +static int adreno_get_pwrlevels(struct device *dev,
>>>>            struct msm_gpu *gpu)
>>>>    {
>>>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>        unsigned long freq = ULONG_MAX;
>>>>        struct dev_pm_opp *opp;
>>>>        int ret;
>>>>          gpu->fast_rate = 0;
>>>>    -    /* You down with OPP? */
>>>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>>>> -        ret = adreno_get_legacy_pwrlevels(dev);
>>>> -    else {
>>>> -        ret = devm_pm_opp_of_add_table(dev);
>>>> -        if (ret)
>>>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>>> -    }
>>>> -
>>>> -    if (!ret) {
>>>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>>>> +    ret = devm_pm_opp_of_add_table(dev);
>>>> +    if (ret == -ENODEV) {
>>>> +        /* Special cases for ancient hw with ancient DT bindings */
>>>> +        if (adreno_is_a2xx(adreno_gpu)) {
>>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>>>> +            dev_pm_opp_add(dev, 200000000, 0);
>>>> +            gpu->fast_rate = 200000000;
>>>
>>> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
>> It's not reached in this code path.
> 
> I see. I got lost in all the ifs. What do you think about turning it into the main code path, since after this code block we always have a valid OPP table?
Sounds good!

Konrad
> 
>>
>>>
>>>> +        } else if (adreno_is_a320(adreno_gpu)) {
>>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>>>> +            dev_pm_opp_add(dev, 450000000, 0);
>>>> +            gpu->fast_rate = 450000000;
>>>> +        } else {
>>>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>>>> +            return -ENODEV;
>>>> +        }
>>>> +    } else if (ret) {
>>>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>>> +        return ret;
>>>> +    } else {
>>>>            /* Find the fastest defined rate */
>>>>            opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>>> -        if (!IS_ERR(opp)) {
>>>> +
>>>> +        if (IS_ERR(opp))
>>>> +            return PTR_ERR(opp);
>>>> +        else {
>>>>                gpu->fast_rate = freq;
>>>>                dev_pm_opp_put(opp);
>>>>            }
>>>>        }
>>>>    -    if (!gpu->fast_rate) {
>>>> -        dev_warn(dev,
>>>> -            "Could not find a clock rate. Using a reasonable default\n");
>>>> -        /* Pick a suitably safe clock speed for any target */
>>>> -        gpu->fast_rate = 200000000;
>>>> -    }
>>>> -
>>>>        DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>>>> +
>>>> +    return 0;
>>>>    }
>>>>      int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> 
> 

Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

On 23.02.2023 00:16, Dmitry Baryshkov wrote:
> On 23/02/2023 00:40, Konrad Dybcio wrote:
>>
>>
>> On 22.02.2023 23:38, Dmitry Baryshkov wrote:
>>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>>> Some older GPUs (namely a2xx with no opp tables at all and a320 with
>>>> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
>>>> both however had just one frequency defined, making it extremely easy
>>>> to construct such an OPP table from within the driver if need be.
>>>>
>>>> Do so and switch all clk_set_rate calls on core_clk to their OPP
>>>> counterparts.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
>>>>    drivers/gpu/drm/msm/msm_gpu.c           |  4 +-
>>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   |  2 +-
>>>>    3 files changed, 45 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> index ce6b76c45b6f..9b940c0f063f 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>>> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
>>>>                ring->id);
>>>>    }
>>>>    -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
>>>> -static int adreno_get_legacy_pwrlevels(struct device *dev)
>>>> -{
>>>> -    struct device_node *child, *node;
>>>> -    int ret;
>>>> -
>>>> -    node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
>>>> -    if (!node) {
>>>> -        DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
>>>> -        return -ENXIO;
>>>> -    }
>>>> -
>>>> -    for_each_child_of_node(node, child) {
>>>> -        unsigned int val;
>>>> -
>>>> -        ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
>>>> -        if (ret)
>>>> -            continue;
>>>> -
>>>> -        /*
>>>> -         * Skip the intentionally bogus clock value found at the bottom
>>>> -         * of most legacy frequency tables
>>>> -         */
>>>> -        if (val != 27000000)
>>>> -            dev_pm_opp_add(dev, val, 0);
>>>> -    }
>>>> -
>>>> -    of_node_put(node);
>>>> -
>>>> -    return 0;
>>>> -}
>>>> -
>>>> -static void adreno_get_pwrlevels(struct device *dev,
>>>> +static int adreno_get_pwrlevels(struct device *dev,
>>>>            struct msm_gpu *gpu)
>>>>    {
>>>> +    struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>        unsigned long freq = ULONG_MAX;
>>>>        struct dev_pm_opp *opp;
>>>>        int ret;
>>>>          gpu->fast_rate = 0;
>>>>    -    /* You down with OPP? */
>>>> -    if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
>>>> -        ret = adreno_get_legacy_pwrlevels(dev);
>>>> -    else {
>>>> -        ret = devm_pm_opp_of_add_table(dev);
>>>> -        if (ret)
>>>> -            DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>>> -    }
>>>> -
>>>> -    if (!ret) {
>>>> +    /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
>>>> +    ret = devm_pm_opp_of_add_table(dev);
>>>> +    if (ret == -ENODEV) {
>>>> +        /* Special cases for ancient hw with ancient DT bindings */
>>>> +        if (adreno_is_a2xx(adreno_gpu)) {
>>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
>>>> +            dev_pm_opp_add(dev, 200000000, 0);
>>>> +            gpu->fast_rate = 200000000;
>>>
>>> We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will get it from our freshly generated opp table.
>> It's not reached in this code path.
> 
> I see. I got lost in all the ifs. What do you think about turning it into the main code path, since after this code block we always have a valid OPP table?
Sounds good!

Konrad
> 
>>
>>>
>>>> +        } else if (adreno_is_a320(adreno_gpu)) {
>>>> +            dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
>>>> +            dev_pm_opp_add(dev, 450000000, 0);
>>>> +            gpu->fast_rate = 450000000;
>>>> +        } else {
>>>> +            DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
>>>> +            return -ENODEV;
>>>> +        }
>>>> +    } else if (ret) {
>>>> +        DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
>>>> +        return ret;
>>>> +    } else {
>>>>            /* Find the fastest defined rate */
>>>>            opp = dev_pm_opp_find_freq_floor(dev, &freq);
>>>> -        if (!IS_ERR(opp)) {
>>>> +
>>>> +        if (IS_ERR(opp))
>>>> +            return PTR_ERR(opp);
>>>> +        else {
>>>>                gpu->fast_rate = freq;
>>>>                dev_pm_opp_put(opp);
>>>>            }
>>>>        }
>>>>    -    if (!gpu->fast_rate) {
>>>> -        dev_warn(dev,
>>>> -            "Could not find a clock rate. Using a reasonable default\n");
>>>> -        /* Pick a suitably safe clock speed for any target */
>>>> -        gpu->fast_rate = 200000000;
>>>> -    }
>>>> -
>>>>        DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
>>>> +
>>>> +    return 0;
>>>>    }
>>>>      int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> 
> 

[PATCH 2/5] drm/msm/a2xx: Implement .gpu_busy

[Konrad Dybcio]

Implement gpu_busy based on the downstream msm-3.4 code [1]. This
allows us to use devfreq on this old old old hardware!

[1] https://github.com/LineageOS/android_kernel_sony_apq8064/blob/lineage-16.0/drivers/gpu/msm/adreno_a2xx.c#L1975

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index c67089a7ebc1..6258c98e5a88 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -481,6 +481,33 @@ a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
 	return aspace;
 }
 
+/* While the precise size of this field is unknown, it holds at least these three values.. */
+#define PERF_MODE_CNT		GENMASK(2, 0)
+ #define PERF_STATE_RESET	0x0
+ #define PERF_STATE_ENABLE	0x1
+ #define PERF_STATE_FREEZE	0x2
+static u64 a2xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	/* Freeze the counter */
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_FREEZE));
+
+	busy_cycles = gpu_read64(gpu, REG_A2XX_RBBM_PERFCOUNTER1_LO);
+
+	/* Reset the counter */
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_RESET));
+
+	/* Re-enable the performance monitors */
+	gpu_rmw(gpu, REG_A2XX_RBBM_PM_OVERRIDE2, BIT(6), BIT(6));
+	gpu_write(gpu, REG_A2XX_RBBM_PERFCOUNTER1_SELECT, 1);
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_ENABLE));
+
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a2xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
@@ -502,6 +529,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a2xx_gpu_busy,
 		.gpu_state_get = a2xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = a2xx_create_address_space,

-- 
2.39.2

[PATCH 2/5] drm/msm/a2xx: Implement .gpu_busy

[Konrad Dybcio]

Implement gpu_busy based on the downstream msm-3.4 code [1]. This
allows us to use devfreq on this old old old hardware!

[1] https://github.com/LineageOS/android_kernel_sony_apq8064/blob/lineage-16.0/drivers/gpu/msm/adreno_a2xx.c#L1975

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
index c67089a7ebc1..6258c98e5a88 100644
--- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
@@ -481,6 +481,33 @@ a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
 	return aspace;
 }
 
+/* While the precise size of this field is unknown, it holds at least these three values.. */
+#define PERF_MODE_CNT		GENMASK(2, 0)
+ #define PERF_STATE_RESET	0x0
+ #define PERF_STATE_ENABLE	0x1
+ #define PERF_STATE_FREEZE	0x2
+static u64 a2xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	/* Freeze the counter */
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_FREEZE));
+
+	busy_cycles = gpu_read64(gpu, REG_A2XX_RBBM_PERFCOUNTER1_LO);
+
+	/* Reset the counter */
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_RESET));
+
+	/* Re-enable the performance monitors */
+	gpu_rmw(gpu, REG_A2XX_RBBM_PM_OVERRIDE2, BIT(6), BIT(6));
+	gpu_write(gpu, REG_A2XX_RBBM_PERFCOUNTER1_SELECT, 1);
+	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_ENABLE));
+
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a2xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
@@ -502,6 +529,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a2xx_gpu_busy,
 		.gpu_state_get = a2xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = a2xx_create_address_space,

-- 
2.39.2

Re: [PATCH 2/5] drm/msm/a2xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Implement gpu_busy based on the downstream msm-3.4 code [1]. This
> allows us to use devfreq on this old old old hardware!
> 
> [1] https://github.com/LineageOS/android_kernel_sony_apq8064/blob/lineage-16.0/drivers/gpu/msm/adreno_a2xx.c#L1975
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index c67089a7ebc1..6258c98e5a88 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -481,6 +481,33 @@ a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
>   	return aspace;
>   }
>   
> +/* While the precise size of this field is unknown, it holds at least these three values.. */
> +#define PERF_MODE_CNT		GENMASK(2, 0)
> + #define PERF_STATE_RESET	0x0
> + #define PERF_STATE_ENABLE	0x1
> + #define PERF_STATE_FREEZE	0x2

These should go into a2xx.xml.h

LGTM otherwise.

> +static u64 a2xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
> +{
> +	u64 busy_cycles;
> +
> +	/* Freeze the counter */
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_FREEZE));
> +
> +	busy_cycles = gpu_read64(gpu, REG_A2XX_RBBM_PERFCOUNTER1_LO);
> +
> +	/* Reset the counter */
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_RESET));
> +
> +	/* Re-enable the performance monitors */
> +	gpu_rmw(gpu, REG_A2XX_RBBM_PM_OVERRIDE2, BIT(6), BIT(6));
> +	gpu_write(gpu, REG_A2XX_RBBM_PERFCOUNTER1_SELECT, 1);
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_ENABLE));
> +
> +	*out_sample_rate = clk_get_rate(gpu->core_clk);
> +
> +	return busy_cycles;
> +}
> +
>   static u32 a2xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
>   {
>   	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
> @@ -502,6 +529,7 @@ static const struct adreno_gpu_funcs funcs = {
>   #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
>   		.show = adreno_show,
>   #endif
> +		.gpu_busy = a2xx_gpu_busy,
>   		.gpu_state_get = a2xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
>   		.create_address_space = a2xx_create_address_space,
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/5] drm/msm/a2xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Implement gpu_busy based on the downstream msm-3.4 code [1]. This
> allows us to use devfreq on this old old old hardware!
> 
> [1] https://github.com/LineageOS/android_kernel_sony_apq8064/blob/lineage-16.0/drivers/gpu/msm/adreno_a2xx.c#L1975
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a2xx_gpu.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> index c67089a7ebc1..6258c98e5a88 100644
> --- a/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a2xx_gpu.c
> @@ -481,6 +481,33 @@ a2xx_create_address_space(struct msm_gpu *gpu, struct platform_device *pdev)
>   	return aspace;
>   }
>   
> +/* While the precise size of this field is unknown, it holds at least these three values.. */
> +#define PERF_MODE_CNT		GENMASK(2, 0)
> + #define PERF_STATE_RESET	0x0
> + #define PERF_STATE_ENABLE	0x1
> + #define PERF_STATE_FREEZE	0x2

These should go into a2xx.xml.h

LGTM otherwise.

> +static u64 a2xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
> +{
> +	u64 busy_cycles;
> +
> +	/* Freeze the counter */
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_FREEZE));
> +
> +	busy_cycles = gpu_read64(gpu, REG_A2XX_RBBM_PERFCOUNTER1_LO);
> +
> +	/* Reset the counter */
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_RESET));
> +
> +	/* Re-enable the performance monitors */
> +	gpu_rmw(gpu, REG_A2XX_RBBM_PM_OVERRIDE2, BIT(6), BIT(6));
> +	gpu_write(gpu, REG_A2XX_RBBM_PERFCOUNTER1_SELECT, 1);
> +	gpu_write(gpu, REG_A2XX_CP_PERFMON_CNTL, FIELD_PREP(PERF_MODE_CNT, PERF_STATE_ENABLE));
> +
> +	*out_sample_rate = clk_get_rate(gpu->core_clk);
> +
> +	return busy_cycles;
> +}
> +
>   static u32 a2xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
>   {
>   	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
> @@ -502,6 +529,7 @@ static const struct adreno_gpu_funcs funcs = {
>   #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
>   		.show = adreno_show,
>   #endif
> +		.gpu_busy = a2xx_gpu_busy,
>   		.gpu_state_get = a2xx_gpu_state_get,
>   		.gpu_state_put = adreno_gpu_state_put,
>   		.create_address_space = a2xx_create_address_space,
> 

-- 
With best wishes
Dmitry

[PATCH 3/5] drm/msm/a3xx: Implement .gpu_busy

[Konrad Dybcio]

Add support for gpu_busy on a3xx, which is required for devfreq
support.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 948785ed07bb..c86b377f6f0d 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -477,6 +477,16 @@ static struct msm_gpu_state *a3xx_gpu_state_get(struct msm_gpu *gpu)
 	return state;
 }
 
+static u64 a3xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	busy_cycles = gpu_read64(gpu, REG_A3XX_RBBM_PERFCTR_RBBM_1_LO);
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a3xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
@@ -498,6 +508,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a3xx_gpu_busy,
 		.gpu_state_get = a3xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = adreno_create_address_space,

-- 
2.39.2

[PATCH 3/5] drm/msm/a3xx: Implement .gpu_busy

[Konrad Dybcio]

Add support for gpu_busy on a3xx, which is required for devfreq
support.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 948785ed07bb..c86b377f6f0d 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -477,6 +477,16 @@ static struct msm_gpu_state *a3xx_gpu_state_get(struct msm_gpu *gpu)
 	return state;
 }
 
+static u64 a3xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	busy_cycles = gpu_read64(gpu, REG_A3XX_RBBM_PERFCTR_RBBM_1_LO);
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a3xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_AXXX_CP_RB_RPTR);
@@ -498,6 +508,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a3xx_gpu_busy,
 		.gpu_state_get = a3xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = adreno_create_address_space,

-- 
2.39.2

Re: [PATCH 3/5] drm/msm/a3xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add support for gpu_busy on a3xx, which is required for devfreq
> support.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> #ifc6410
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 3/5] drm/msm/a3xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add support for gpu_busy on a3xx, which is required for devfreq
> support.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> #ifc6410
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 4/5] drm/msm/a4xx: Implement .gpu_busy

[Konrad Dybcio]

Add support for gpu_busy on a4xx, which is required for devfreq
support.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 3e09d3a7a0ac..715436cb3996 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -611,6 +611,16 @@ static int a4xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
 	return 0;
 }
 
+static u64 a4xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	busy_cycles = gpu_read64(gpu, REG_A4XX_RBBM_PERFCTR_RBBM_1_LO);
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a4xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_A4XX_CP_RB_RPTR);
@@ -632,6 +642,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a4xx_gpu_busy,
 		.gpu_state_get = a4xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = adreno_create_address_space,

-- 
2.39.2

[PATCH 4/5] drm/msm/a4xx: Implement .gpu_busy

[Konrad Dybcio]

Add support for gpu_busy on a4xx, which is required for devfreq
support.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 3e09d3a7a0ac..715436cb3996 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -611,6 +611,16 @@ static int a4xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
 	return 0;
 }
 
+static u64 a4xx_gpu_busy(struct msm_gpu *gpu, unsigned long *out_sample_rate)
+{
+	u64 busy_cycles;
+
+	busy_cycles = gpu_read64(gpu, REG_A4XX_RBBM_PERFCTR_RBBM_1_LO);
+	*out_sample_rate = clk_get_rate(gpu->core_clk);
+
+	return busy_cycles;
+}
+
 static u32 a4xx_get_rptr(struct msm_gpu *gpu, struct msm_ringbuffer *ring)
 {
 	ring->memptrs->rptr = gpu_read(gpu, REG_A4XX_CP_RB_RPTR);
@@ -632,6 +642,7 @@ static const struct adreno_gpu_funcs funcs = {
 #if defined(CONFIG_DEBUG_FS) || defined(CONFIG_DEV_COREDUMP)
 		.show = adreno_show,
 #endif
+		.gpu_busy = a4xx_gpu_busy,
 		.gpu_state_get = a4xx_gpu_state_get,
 		.gpu_state_put = adreno_gpu_state_put,
 		.create_address_space = adreno_create_address_space,

-- 
2.39.2

Re: [PATCH 4/5] drm/msm/a4xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add support for gpu_busy on a4xx, which is required for devfreq
> support.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 4/5] drm/msm/a4xx: Implement .gpu_busy

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add support for gpu_busy on a4xx, which is required for devfreq
> support.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Konrad Dybcio]

Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
handle bus voting as part of power level setting.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index d09221f97f71..a33af0cc27b6 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 	/* Set up the preemption specific bits and pieces for each ringbuffer */
 	a5xx_preempt_init(gpu);
 
+	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return gpu;
 }

-- 
2.39.2

[PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Konrad Dybcio]

Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
handle bus voting as part of power level setting.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index d09221f97f71..a33af0cc27b6 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
 	/* Set up the preemption specific bits and pieces for each ringbuffer */
 	a5xx_preempt_init(gpu);
 
+	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return gpu;
 }

-- 
2.39.2

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
> handle bus voting as part of power level setting.

This can probably go to the generic code path rather than sticking it 
into a5xx only.

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index d09221f97f71..a33af0cc27b6 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>   	/* Set up the preemption specific bits and pieces for each ringbuffer */
>   	a5xx_preempt_init(gpu);
>   
> +	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	return gpu;
>   }
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Dmitry Baryshkov]

On 22/02/2023 23:47, Konrad Dybcio wrote:
> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
> handle bus voting as part of power level setting.

This can probably go to the generic code path rather than sticking it 
into a5xx only.

> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index d09221f97f71..a33af0cc27b6 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>   	/* Set up the preemption specific bits and pieces for each ringbuffer */
>   	a5xx_preempt_init(gpu);
>   
> +	ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
>   	return gpu;
>   }
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Konrad Dybcio]

On 22.02.2023 23:12, Dmitry Baryshkov wrote:
> On 22/02/2023 23:47, Konrad Dybcio wrote:
>> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
>> handle bus voting as part of power level setting.
> 
> This can probably go to the generic code path rather than sticking it into a5xx only.
The reasoning is explained in the cover letter, a3xx/a4xx already
have "raw" icc set calls which would require more work (and above
all, testing) to untangle while keeping backwards compat, this is
a midterm solution that would allow getting scaling to work earlier.

Konrad
> 
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> index d09221f97f71..a33af0cc27b6 100644
>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>       /* Set up the preemption specific bits and pieces for each ringbuffer */
>>       a5xx_preempt_init(gpu);
>>   +    ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
>>       return gpu;
>>   }
>>
> 

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Konrad Dybcio]

On 22.02.2023 23:12, Dmitry Baryshkov wrote:
> On 22/02/2023 23:47, Konrad Dybcio wrote:
>> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
>> handle bus voting as part of power level setting.
> 
> This can probably go to the generic code path rather than sticking it into a5xx only.
The reasoning is explained in the cover letter, a3xx/a4xx already
have "raw" icc set calls which would require more work (and above
all, testing) to untangle while keeping backwards compat, this is
a midterm solution that would allow getting scaling to work earlier.

Konrad
> 
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> index d09221f97f71..a33af0cc27b6 100644
>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>       /* Set up the preemption specific bits and pieces for each ringbuffer */
>>       a5xx_preempt_init(gpu);
>>   +    ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
>> +    if (ret)
>> +        return ERR_PTR(ret);
>> +
>>       return gpu;
>>   }
>>
> 

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Dmitry Baryshkov]

On 23/02/2023 00:14, Konrad Dybcio wrote:
> 
> 
> On 22.02.2023 23:12, Dmitry Baryshkov wrote:
>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
>>> handle bus voting as part of power level setting.
>>
>> This can probably go to the generic code path rather than sticking it into a5xx only.
> The reasoning is explained in the cover letter, a3xx/a4xx already
> have "raw" icc set calls which would require more work (and above
> all, testing) to untangle while keeping backwards compat, this is
> a midterm solution that would allow getting scaling to work earlier.

Those two platforms call icc_set_bw() during setup, however their opp 
tables do not contain BW settings, making dev_pm_opp_of_find_icc_paths() 
nop. So, I think, we might as well call this function on a3xx/a4xx, 
making the code future proof.

> 
> Konrad
>>
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> index d09221f97f71..a33af0cc27b6 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>>        /* Set up the preemption specific bits and pieces for each ringbuffer */
>>>        a5xx_preempt_init(gpu);
>>>    +    ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>>        return gpu;
>>>    }
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 5/5] drm/msm/a5xx: Enable optional icc voting from OPP tables

[Dmitry Baryshkov]

On 23/02/2023 00:14, Konrad Dybcio wrote:
> 
> 
> On 22.02.2023 23:12, Dmitry Baryshkov wrote:
>> On 22/02/2023 23:47, Konrad Dybcio wrote:
>>> Add the dev_pm_opp_of_find_icc_paths() call to let the OPP framework
>>> handle bus voting as part of power level setting.
>>
>> This can probably go to the generic code path rather than sticking it into a5xx only.
> The reasoning is explained in the cover letter, a3xx/a4xx already
> have "raw" icc set calls which would require more work (and above
> all, testing) to untangle while keeping backwards compat, this is
> a midterm solution that would allow getting scaling to work earlier.

Those two platforms call icc_set_bw() during setup, however their opp 
tables do not contain BW settings, making dev_pm_opp_of_find_icc_paths() 
nop. So, I think, we might as well call this function on a3xx/a4xx, 
making the code future proof.

> 
> Konrad
>>
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> index d09221f97f71..a33af0cc27b6 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
>>> @@ -1775,5 +1775,9 @@ struct msm_gpu *a5xx_gpu_init(struct drm_device *dev)
>>>        /* Set up the preemption specific bits and pieces for each ringbuffer */
>>>        a5xx_preempt_init(gpu);
>>>    +    ret = dev_pm_opp_of_find_icc_paths(&pdev->dev, NULL);
>>> +    if (ret)
>>> +        return ERR_PTR(ret);
>>> +
>>>        return gpu;
>>>    }
>>>
>>

-- 
With best wishes
Dmitry