[PATCH 0/8] drm/msm/dpu: cleanup dpu_core_perf module

[PATCH 0/8] drm/msm/dpu: cleanup dpu_core_perf module

[Dmitry Baryshkov]

Apply several cleanups to the DPU's core_perf module.

Dmitry Baryshkov (8):
  drm/msm/dpu: drop enum dpu_core_perf_data_bus_id
  drm/msm/dpu: drop performance tuning modes
  drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib
  drm/msm/dpu: rework indentation in dpu_core_perf
  drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param
  drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code
  drm/msm/dpu: drop dpu_core_perf_destroy()
  drm/msm/dpu: remove unused fields from struct dpu_core_perf

 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 199 ++++--------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  55 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |   8 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   3 +-
 4 files changed, 47 insertions(+), 218 deletions(-)

-- 
2.39.2

[PATCH 1/8] drm/msm/dpu: drop enum dpu_core_perf_data_bus_id

[Dmitry Baryshkov]

Drop the leftover of bus-client -> interconnect conversion, the enum
dpu_core_perf_data_bus_id.

Fixes: cb88482e2570 ("drm/msm/dpu: clean up references of DPU custom bus scaling")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e3795995e145..29bb8ee2bc26 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -14,19 +14,6 @@
 
 #define	DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE	412500000
 
-/**
- * enum dpu_core_perf_data_bus_id - data bus identifier
- * @DPU_CORE_PERF_DATA_BUS_ID_MNOC: DPU/MNOC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_LLCC: MNOC/LLCC data bus
- * @DPU_CORE_PERF_DATA_BUS_ID_EBI: LLCC/EBI data bus
- */
-enum dpu_core_perf_data_bus_id {
-	DPU_CORE_PERF_DATA_BUS_ID_MNOC,
-	DPU_CORE_PERF_DATA_BUS_ID_LLCC,
-	DPU_CORE_PERF_DATA_BUS_ID_EBI,
-	DPU_CORE_PERF_DATA_BUS_ID_MAX,
-};
-
 /**
  * struct dpu_core_perf_params - definition of performance parameters
  * @max_per_pipe_ib: maximum instantaneous bandwidth request
-- 
2.39.2

Re: [PATCH 1/8] drm/msm/dpu: drop enum dpu_core_perf_data_bus_id

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> Drop the leftover of bus-client -> interconnect conversion, the enum
> dpu_core_perf_data_bus_id.
> 
> Fixes: cb88482e2570 ("drm/msm/dpu: clean up references of DPU custom bus scaling")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e3795995e145..29bb8ee2bc26 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -14,19 +14,6 @@
>  
>  #define	DPU_PERF_DEFAULT_MAX_CORE_CLK_RATE	412500000
>  
> -/**
> - * enum dpu_core_perf_data_bus_id - data bus identifier
> - * @DPU_CORE_PERF_DATA_BUS_ID_MNOC: DPU/MNOC data bus
> - * @DPU_CORE_PERF_DATA_BUS_ID_LLCC: MNOC/LLCC data bus
> - * @DPU_CORE_PERF_DATA_BUS_ID_EBI: LLCC/EBI data bus
> - */
> -enum dpu_core_perf_data_bus_id {
> -	DPU_CORE_PERF_DATA_BUS_ID_MNOC,
> -	DPU_CORE_PERF_DATA_BUS_ID_LLCC,
> -	DPU_CORE_PERF_DATA_BUS_ID_EBI,
> -	DPU_CORE_PERF_DATA_BUS_ID_MAX,
> -};
> -
>  /**
>   * struct dpu_core_perf_params - definition of performance parameters
>   * @max_per_pipe_ib: maximum instantaneous bandwidth request

Re: [PATCH 1/8] drm/msm/dpu: drop enum dpu_core_perf_data_bus_id

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> Drop the leftover of bus-client -> interconnect conversion, the enum
> dpu_core_perf_data_bus_id.
> 
> Fixes: cb88482e2570 ("drm/msm/dpu: clean up references of DPU custom bus scaling")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

It seems unused, hence

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

Since its just dropping unused enum, will see if I can combine with some 
other more critical fixes into a MR but not one on its own.

[PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Dmitry Baryshkov]

DPU performance module contains code to change performance state
calculations. In addition to normal (sum plane and CRTC requirements),
it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
since they can easily end up with the display underruns. Userspace also
should not depend on these modes availability, since they are tuned
through debugfs, which might not be available.

Drop relevant code to simplify performance state calculations.

Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 99 +------------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 19 ----
 2 files changed, 4 insertions(+), 114 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 1d9d83d7b99e..9902febc72c0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -17,20 +17,6 @@
 #include "dpu_crtc.h"
 #include "dpu_core_perf.h"
 
-/**
- * enum dpu_perf_mode - performance tuning mode
- * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
- * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
- * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
- * @DPU_PERF_MODE_MAX: maximum value, used for error checking
- */
-enum dpu_perf_mode {
-	DPU_PERF_MODE_NORMAL,
-	DPU_PERF_MODE_MINIMUM,
-	DPU_PERF_MODE_FIXED,
-	DPU_PERF_MODE_MAX
-};
-
 /**
  * _dpu_core_perf_calc_bw() - to calculate BW per crtc
  * @kms:  pointer to the dpu_kms
@@ -118,19 +104,9 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
 
 	memset(perf, 0, sizeof(struct dpu_core_perf_params));
 
-	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
-		perf->bw_ctl = 0;
-		perf->max_per_pipe_ib = 0;
-		perf->core_clk_rate = 0;
-	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
-		perf->bw_ctl = kms->perf.fix_core_ab_vote;
-		perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
-		perf->core_clk_rate = kms->perf.fix_core_clk_rate;
-	} else {
-		perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
-		perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
-		perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
-	}
+	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
+	perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
+	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
 
 	DRM_DEBUG_ATOMIC(
 		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
@@ -286,7 +262,7 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
 
 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 {
-	u64 clk_rate = kms->perf.perf_tune.min_core_clk;
+	u64 clk_rate = 0;
 	struct drm_crtc *crtc;
 	struct dpu_crtc_state *dpu_cstate;
 
@@ -300,9 +276,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 		}
 	}
 
-	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
-		clk_rate = kms->perf.fix_core_clk_rate;
-
 	DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
 
 	return clk_rate;
@@ -409,62 +382,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 
 #ifdef CONFIG_DEBUG_FS
 
-static ssize_t _dpu_core_perf_mode_write(struct file *file,
-		    const char __user *user_buf, size_t count, loff_t *ppos)
-{
-	struct dpu_core_perf *perf = file->private_data;
-	const struct dpu_perf_cfg *cfg = perf->catalog->perf;
-	u32 perf_mode = 0;
-	int ret;
-
-	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
-	if (ret)
-		return ret;
-
-	if (perf_mode >= DPU_PERF_MODE_MAX)
-		return -EINVAL;
-
-	if (perf_mode == DPU_PERF_MODE_FIXED) {
-		DRM_INFO("fix performance mode\n");
-	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
-		/* run the driver with max clk and BW vote */
-		perf->perf_tune.min_core_clk = perf->max_core_clk_rate;
-		perf->perf_tune.min_bus_vote =
-				(u64) cfg->max_bw_high * 1000;
-		DRM_INFO("minimum performance mode\n");
-	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
-		/* reset the perf tune params to 0 */
-		perf->perf_tune.min_core_clk = 0;
-		perf->perf_tune.min_bus_vote = 0;
-		DRM_INFO("normal performance mode\n");
-	}
-	perf->perf_tune.mode = perf_mode;
-
-	return count;
-}
-
-static ssize_t _dpu_core_perf_mode_read(struct file *file,
-			char __user *buff, size_t count, loff_t *ppos)
-{
-	struct dpu_core_perf *perf = file->private_data;
-	int len;
-	char buf[128];
-
-	len = scnprintf(buf, sizeof(buf),
-			"mode %d min_mdp_clk %llu min_bus_vote %llu\n",
-			perf->perf_tune.mode,
-			perf->perf_tune.min_core_clk,
-			perf->perf_tune.min_bus_vote);
-
-	return simple_read_from_buffer(buff, count, ppos, buf, len);
-}
-
-static const struct file_operations dpu_core_perf_mode_fops = {
-	.open = simple_open,
-	.read = _dpu_core_perf_mode_read,
-	.write = _dpu_core_perf_mode_write,
-};
-
 int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 {
 	struct dpu_core_perf *perf = &dpu_kms->perf;
@@ -489,14 +406,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 			(u32 *)&catalog->perf->min_llcc_ib);
 	debugfs_create_u32("min_dram_ib", 0600, entry,
 			(u32 *)&catalog->perf->min_dram_ib);
-	debugfs_create_file("perf_mode", 0600, entry,
-			(u32 *)perf, &dpu_core_perf_mode_fops);
-	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
-			&perf->fix_core_clk_rate);
-	debugfs_create_u64("fix_core_ib_vote", 0600, entry,
-			&perf->fix_core_ib_vote);
-	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
-			&perf->fix_core_ab_vote);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 29bb8ee2bc26..e02cc2324af2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -26,18 +26,6 @@ struct dpu_core_perf_params {
 	u64 core_clk_rate;
 };
 
-/**
- * struct dpu_core_perf_tune - definition of performance tuning control
- * @mode: performance mode
- * @min_core_clk: minimum core clock
- * @min_bus_vote: minimum bus vote
- */
-struct dpu_core_perf_tune {
-	u32 mode;
-	u64 min_core_clk;
-	u64 min_bus_vote;
-};
-
 /**
  * struct dpu_core_perf - definition of core performance context
  * @dev: Pointer to drm device
@@ -48,9 +36,6 @@ struct dpu_core_perf_tune {
  * @max_core_clk_rate: maximum allowable core clock rate
  * @perf_tune: debug control for performance tuning
  * @enable_bw_release: debug control for bandwidth release
- * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
- * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
- * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
  */
 struct dpu_core_perf {
 	struct drm_device *dev;
@@ -59,11 +44,7 @@ struct dpu_core_perf {
 	struct clk *core_clk;
 	u64 core_clk_rate;
 	u64 max_core_clk_rate;
-	struct dpu_core_perf_tune perf_tune;
 	u32 enable_bw_release;
-	u64 fix_core_clk_rate;
-	u64 fix_core_ib_vote;
-	u64 fix_core_ab_vote;
 };
 
 /**
-- 
2.39.2

Re: [PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> DPU performance module contains code to change performance state
> calculations. In addition to normal (sum plane and CRTC requirements),
> it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
> since they can easily end up with the display underruns. Userspace also
> should not depend on these modes availability, since they are tuned
> through debugfs, which might not be available.
> 
> Drop relevant code to simplify performance state calculations.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Tested-by: Konrad Dybcio <konrad.dybcio@linaro.org> # SM6375
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 99 +------------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 19 ----
>  2 files changed, 4 insertions(+), 114 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 1d9d83d7b99e..9902febc72c0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -17,20 +17,6 @@
>  #include "dpu_crtc.h"
>  #include "dpu_core_perf.h"
>  
> -/**
> - * enum dpu_perf_mode - performance tuning mode
> - * @DPU_PERF_MODE_NORMAL: performance controlled by user mode client
> - * @DPU_PERF_MODE_MINIMUM: performance bounded by minimum setting
> - * @DPU_PERF_MODE_FIXED: performance bounded by fixed setting
> - * @DPU_PERF_MODE_MAX: maximum value, used for error checking
> - */
> -enum dpu_perf_mode {
> -	DPU_PERF_MODE_NORMAL,
> -	DPU_PERF_MODE_MINIMUM,
> -	DPU_PERF_MODE_FIXED,
> -	DPU_PERF_MODE_MAX
> -};
> -
>  /**
>   * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>   * @kms:  pointer to the dpu_kms
> @@ -118,19 +104,9 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
>  
>  	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>  
> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM) {
> -		perf->bw_ctl = 0;
> -		perf->max_per_pipe_ib = 0;
> -		perf->core_clk_rate = 0;
> -	} else if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED) {
> -		perf->bw_ctl = kms->perf.fix_core_ab_vote;
> -		perf->max_per_pipe_ib = kms->perf.fix_core_ib_vote;
> -		perf->core_clk_rate = kms->perf.fix_core_clk_rate;
> -	} else {
> -		perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
> -		perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
> -		perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
> -	}
> +	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
> +	perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
>  
>  	DRM_DEBUG_ATOMIC(
>  		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
> @@ -286,7 +262,7 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
>  
>  static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>  {
> -	u64 clk_rate = kms->perf.perf_tune.min_core_clk;
> +	u64 clk_rate = 0;
>  	struct drm_crtc *crtc;
>  	struct dpu_crtc_state *dpu_cstate;
>  
> @@ -300,9 +276,6 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>  		}
>  	}
>  
> -	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> -		clk_rate = kms->perf.fix_core_clk_rate;
> -
>  	DRM_DEBUG_ATOMIC("clk:%llu\n", clk_rate);
>  
>  	return clk_rate;
> @@ -409,62 +382,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  
>  #ifdef CONFIG_DEBUG_FS
>  
> -static ssize_t _dpu_core_perf_mode_write(struct file *file,
> -		    const char __user *user_buf, size_t count, loff_t *ppos)
> -{
> -	struct dpu_core_perf *perf = file->private_data;
> -	const struct dpu_perf_cfg *cfg = perf->catalog->perf;
> -	u32 perf_mode = 0;
> -	int ret;
> -
> -	ret = kstrtouint_from_user(user_buf, count, 0, &perf_mode);
> -	if (ret)
> -		return ret;
> -
> -	if (perf_mode >= DPU_PERF_MODE_MAX)
> -		return -EINVAL;
> -
> -	if (perf_mode == DPU_PERF_MODE_FIXED) {
> -		DRM_INFO("fix performance mode\n");
> -	} else if (perf_mode == DPU_PERF_MODE_MINIMUM) {
> -		/* run the driver with max clk and BW vote */
> -		perf->perf_tune.min_core_clk = perf->max_core_clk_rate;
> -		perf->perf_tune.min_bus_vote =
> -				(u64) cfg->max_bw_high * 1000;
> -		DRM_INFO("minimum performance mode\n");
> -	} else if (perf_mode == DPU_PERF_MODE_NORMAL) {
> -		/* reset the perf tune params to 0 */
> -		perf->perf_tune.min_core_clk = 0;
> -		perf->perf_tune.min_bus_vote = 0;
> -		DRM_INFO("normal performance mode\n");
> -	}
> -	perf->perf_tune.mode = perf_mode;
> -
> -	return count;
> -}
> -
> -static ssize_t _dpu_core_perf_mode_read(struct file *file,
> -			char __user *buff, size_t count, loff_t *ppos)
> -{
> -	struct dpu_core_perf *perf = file->private_data;
> -	int len;
> -	char buf[128];
> -
> -	len = scnprintf(buf, sizeof(buf),
> -			"mode %d min_mdp_clk %llu min_bus_vote %llu\n",
> -			perf->perf_tune.mode,
> -			perf->perf_tune.min_core_clk,
> -			perf->perf_tune.min_bus_vote);
> -
> -	return simple_read_from_buffer(buff, count, ppos, buf, len);
> -}
> -
> -static const struct file_operations dpu_core_perf_mode_fops = {
> -	.open = simple_open,
> -	.read = _dpu_core_perf_mode_read,
> -	.write = _dpu_core_perf_mode_write,
> -};
> -
>  int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  {
>  	struct dpu_core_perf *perf = &dpu_kms->perf;
> @@ -489,14 +406,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  			(u32 *)&catalog->perf->min_llcc_ib);
>  	debugfs_create_u32("min_dram_ib", 0600, entry,
>  			(u32 *)&catalog->perf->min_dram_ib);
> -	debugfs_create_file("perf_mode", 0600, entry,
> -			(u32 *)perf, &dpu_core_perf_mode_fops);
> -	debugfs_create_u64("fix_core_clk_rate", 0600, entry,
> -			&perf->fix_core_clk_rate);
> -	debugfs_create_u64("fix_core_ib_vote", 0600, entry,
> -			&perf->fix_core_ib_vote);
> -	debugfs_create_u64("fix_core_ab_vote", 0600, entry,
> -			&perf->fix_core_ab_vote);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index 29bb8ee2bc26..e02cc2324af2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -26,18 +26,6 @@ struct dpu_core_perf_params {
>  	u64 core_clk_rate;
>  };
>  
> -/**
> - * struct dpu_core_perf_tune - definition of performance tuning control
> - * @mode: performance mode
> - * @min_core_clk: minimum core clock
> - * @min_bus_vote: minimum bus vote
> - */
> -struct dpu_core_perf_tune {
> -	u32 mode;
> -	u64 min_core_clk;
> -	u64 min_bus_vote;
> -};
> -
>  /**
>   * struct dpu_core_perf - definition of core performance context
>   * @dev: Pointer to drm device
> @@ -48,9 +36,6 @@ struct dpu_core_perf_tune {
>   * @max_core_clk_rate: maximum allowable core clock rate
>   * @perf_tune: debug control for performance tuning
>   * @enable_bw_release: debug control for bandwidth release
> - * @fix_core_clk_rate: fixed core clock request in Hz used in mode 2
> - * @fix_core_ib_vote: fixed core ib vote in bps used in mode 2
> - * @fix_core_ab_vote: fixed core ab vote in bps used in mode 2
>   */
>  struct dpu_core_perf {
>  	struct drm_device *dev;
> @@ -59,11 +44,7 @@ struct dpu_core_perf {
>  	struct clk *core_clk;
>  	u64 core_clk_rate;
>  	u64 max_core_clk_rate;
> -	struct dpu_core_perf_tune perf_tune;
>  	u32 enable_bw_release;
> -	u64 fix_core_clk_rate;
> -	u64 fix_core_ib_vote;
> -	u64 fix_core_ab_vote;
>  };
>  
>  /**

Re: [PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> DPU performance module contains code to change performance state
> calculations. In addition to normal (sum plane and CRTC requirements),
> it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
> since they can easily end up with the display underruns. Userspace also
> should not depend on these modes availability, since they are tuned
> through debugfs, which might not be available.
> 
> Drop relevant code to simplify performance state calculations.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Sorry but NAK on this change for two reasons:

This mode is not for usermode to depend on but for debugging. I have 
personally used both the perf max and perf min modes for debug.

1) The purpose is that, if you do see an underrun, you can force the 
perf mode as it will select max clk and bw rate

So something like below:

localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 300000000 > 
fix_core_clk_rate

This will allow us to force the clk to a particular value to see at what 
point it starts underruning

Also you can even do

localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 1 > perf_mode

This will automatically max out the clk and BW

With this, you can figure out if underrun is a performance related 
underrun or a misconfiguration. We used it even recently to debug the 
performance issue with pclk reduction

2) Sometimes, you even want to force an underrun to debug devcoredump OR 
the recovery code. Forcing the min clk mode by doing

localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 19200000 > 
fix_core_clk_rate
localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode

Is the easiest way to trigger the recovery handler.

Hence I am not at all convinced of dropping this.

Re: [PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 00:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> > DPU performance module contains code to change performance state
> > calculations. In addition to normal (sum plane and CRTC requirements),
> > it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
> > since they can easily end up with the display underruns. Userspace also
> > should not depend on these modes availability, since they are tuned
> > through debugfs, which might not be available.
> >
> > Drop relevant code to simplify performance state calculations.
> >
> > Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
>
> Sorry but NAK on this change for two reasons:
>
> This mode is not for usermode to depend on but for debugging. I have
> personally used both the perf max and perf min modes for debug.
>
> 1) The purpose is that, if you do see an underrun, you can force the
> perf mode as it will select max clk and bw rate
>
> So something like below:
>
> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 300000000 >
> fix_core_clk_rate
>
> This will allow us to force the clk to a particular value to see at what
> point it starts underruning
>
> Also you can even do
>
> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 1 > perf_mode
>
> This will automatically max out the clk and BW
>
> With this, you can figure out if underrun is a performance related
> underrun or a misconfiguration. We used it even recently to debug the
> performance issue with pclk reduction

Hmm, 1 is minimum, not maxumum.

>
> 2) Sometimes, you even want to force an underrun to debug devcoredump OR
> the recovery code. Forcing the min clk mode by doing
>
> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 19200000 >
> fix_core_clk_rate
> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
>
> Is the easiest way to trigger the recovery handler.
>
> Hence I am not at all convinced of dropping this.

I see, thanks for sharing the usecases. However I still think that it
is overcomplicated for the debugging feature. What about dropping all
perf modes and providing just 'override_core_clk_rate' and
'override_avg_bw', 'override_peak_bw'?

-- 
With best wishes
Dmitry

Re: [PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Abhinav Kumar]

On 7/3/2023 3:20 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 00:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
>>> DPU performance module contains code to change performance state
>>> calculations. In addition to normal (sum plane and CRTC requirements),
>>> it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
>>> since they can easily end up with the display underruns. Userspace also
>>> should not depend on these modes availability, since they are tuned
>>> through debugfs, which might not be available.
>>>
>>> Drop relevant code to simplify performance state calculations.
>>>
>>> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>
>> Sorry but NAK on this change for two reasons:
>>
>> This mode is not for usermode to depend on but for debugging. I have
>> personally used both the perf max and perf min modes for debug.
>>
>> 1) The purpose is that, if you do see an underrun, you can force the
>> perf mode as it will select max clk and bw rate
>>
>> So something like below:
>>
>> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
>> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 300000000 >
>> fix_core_clk_rate
>>
>> This will allow us to force the clk to a particular value to see at what
>> point it starts underruning
>>
>> Also you can even do
>>
>> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 1 > perf_mode
>>
>> This will automatically max out the clk and BW
>>
>> With this, you can figure out if underrun is a performance related
>> underrun or a misconfiguration. We used it even recently to debug the
>> performance issue with pclk reduction
> 
> Hmm, 1 is minimum, not maxumum.
> 

The name is kind of confusing.

Yes 1 is min perf mode but it maxes out the clocks and BW.

I guess its named that way because a min perf mode gives you the minimum 
savings in terms of power.

>>
>> 2) Sometimes, you even want to force an underrun to debug devcoredump OR
>> the recovery code. Forcing the min clk mode by doing
>>
>> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 19200000 >
>> fix_core_clk_rate
>> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
>>
>> Is the easiest way to trigger the recovery handler.
>>
>> Hence I am not at all convinced of dropping this.
> 
> I see, thanks for sharing the usecases. However I still think that it
> is overcomplicated for the debugging feature. What about dropping all
> perf modes and providing just 'override_core_clk_rate' and
> 'override_avg_bw', 'override_peak_bw'?
> 

No, we need both. Let me explain why:

1) Having just the min perf mode, directly uses the max clk and bw. This 
is useful when you just want to run at the max and see the behavior

2) If you want to figure out what is the sweet spot where the issue does 
not happen you need the "fixed" mode to find the range where the issue 
doesnt happen

This is one of the oldest and most effective debugging mechanisms.

I dont want to touch this and I personally use this quite often.

Re: [PATCH 2/8] drm/msm/dpu: drop performance tuning modes

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 01:26, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/3/2023 3:20 PM, Dmitry Baryshkov wrote:
> > On Tue, 4 Jul 2023 at 00:40, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> >>> DPU performance module contains code to change performance state
> >>> calculations. In addition to normal (sum plane and CRTC requirements),
> >>> it can work in 'minimal' or 'fixed' modes. Both modes are impractical,
> >>> since they can easily end up with the display underruns. Userspace also
> >>> should not depend on these modes availability, since they are tuned
> >>> through debugfs, which might not be available.
> >>>
> >>> Drop relevant code to simplify performance state calculations.
> >>>
> >>> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>
> >> Sorry but NAK on this change for two reasons:
> >>
> >> This mode is not for usermode to depend on but for debugging. I have
> >> personally used both the perf max and perf min modes for debug.
> >>
> >> 1) The purpose is that, if you do see an underrun, you can force the
> >> perf mode as it will select max clk and bw rate
> >>
> >> So something like below:
> >>
> >> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
> >> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 300000000 >
> >> fix_core_clk_rate
> >>
> >> This will allow us to force the clk to a particular value to see at what
> >> point it starts underruning
> >>
> >> Also you can even do
> >>
> >> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 1 > perf_mode
> >>
> >> This will automatically max out the clk and BW
> >>
> >> With this, you can figure out if underrun is a performance related
> >> underrun or a misconfiguration. We used it even recently to debug the
> >> performance issue with pclk reduction
> >
> > Hmm, 1 is minimum, not maxumum.
> >
>
> The name is kind of confusing.
>
> Yes 1 is min perf mode but it maxes out the clocks and BW.
>
> I guess its named that way because a min perf mode gives you the minimum
> savings in terms of power.
>
> >>
> >> 2) Sometimes, you even want to force an underrun to debug devcoredump OR
> >> the recovery code. Forcing the min clk mode by doing
> >>
> >> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 19200000 >
> >> fix_core_clk_rate
> >> localhost /sys/kernel/debug/dri/1/debug/core_perf # echo 2 > perf_mode
> >>
> >> Is the easiest way to trigger the recovery handler.
> >>
> >> Hence I am not at all convinced of dropping this.
> >
> > I see, thanks for sharing the usecases. However I still think that it
> > is overcomplicated for the debugging feature. What about dropping all
> > perf modes and providing just 'override_core_clk_rate' and
> > 'override_avg_bw', 'override_peak_bw'?
> >
>
> No, we need both. Let me explain why:
>
> 1) Having just the min perf mode, directly uses the max clk and bw. This
> is useful when you just want to run at the max and see the behavior
>
> 2) If you want to figure out what is the sweet spot where the issue does
> not happen you need the "fixed" mode to find the range where the issue
> doesnt happen
>
> This is one of the oldest and most effective debugging mechanisms.
>
> I dont want to touch this and I personally use this quite often.

Ack, I'll see if I can keep the interface / idea. I still think that
override + in-kernel limits should do the same trick as the current
'maximum' does.



-- 
With best wishes
Dmitry

[PATCH 3/8] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Dmitry Baryshkov]

The max_per_pipe_ib is a constant across all CRTCs and is read from the
catalog. Drop corresponding calculations and read the value directly at
icc_set_bw() time.

Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 17 +++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
 3 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 9902febc72c0..ba146af73bc5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -105,13 +105,12 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
 	memset(perf, 0, sizeof(struct dpu_core_perf_params));
 
 	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
-	perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
 	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
 
 	DRM_DEBUG_ATOMIC(
-		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
+		"crtc=%d clk_rate=%llu core_ab=%llu\n",
 			crtc->base.id, perf->core_clk_rate,
-			perf->max_per_pipe_ib, perf->bw_ctl);
+			perf->bw_ctl);
 }
 
 int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
@@ -199,9 +198,6 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 				dpu_crtc_get_client_type(tmp_crtc)) {
 			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
-			perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
-					dpu_cstate->new_perf.max_per_pipe_ib);
-
 			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
 
 			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
@@ -217,7 +213,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
 
 	for (i = 0; i < kms->num_paths; i++)
-		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
+		icc_set_bw(kms->path[i], avg_bw, kms->catalog->perf->min_dram_ib);
 
 	return ret;
 }
@@ -320,15 +316,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 		 * 2. new bandwidth vote - "ab or ib vote" is lower
 		 *    than current vote at end of commit or stop.
 		 */
-		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
-			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
-			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
-			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
+		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
+		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
 			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
 				crtc->base.id, params_changed,
 				new->bw_ctl, old->bw_ctl);
 			old->bw_ctl = new->bw_ctl;
-			old->max_per_pipe_ib = new->max_per_pipe_ib;
 			update_bus = true;
 		}
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e02cc2324af2..2bf7836f79bb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -16,12 +16,10 @@
 
 /**
  * struct dpu_core_perf_params - definition of performance parameters
- * @max_per_pipe_ib: maximum instantaneous bandwidth request
  * @bw_ctl: arbitrated bandwidth request
  * @core_clk_rate: core clock rate request
  */
 struct dpu_core_perf_params {
-	u64 max_per_pipe_ib;
 	u64 bw_ctl;
 	u64 core_clk_rate;
 };
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 1edf2b6b0a26..ff5d306b95ed 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1400,8 +1400,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
 	seq_printf(s, "core_clk_rate: %llu\n",
 			dpu_crtc->cur_perf.core_clk_rate);
 	seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
-	seq_printf(s, "max_per_pipe_ib: %llu\n",
-				dpu_crtc->cur_perf.max_per_pipe_ib);
 
 	return 0;
 }
-- 
2.39.2

Re: [PATCH 3/8] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> The max_per_pipe_ib is a constant across all CRTCs and is read from the
> catalog. Drop corresponding calculations and read the value directly at
> icc_set_bw() time.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
This looks good, but doesn't apply on next-20230620

Konrad
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 17 +++++------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
>  3 files changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 9902febc72c0..ba146af73bc5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -105,13 +105,12 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
>  	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>  
>  	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
> -	perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
>  	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
>  
>  	DRM_DEBUG_ATOMIC(
> -		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
> +		"crtc=%d clk_rate=%llu core_ab=%llu\n",
>  			crtc->base.id, perf->core_clk_rate,
> -			perf->max_per_pipe_ib, perf->bw_ctl);
> +			perf->bw_ctl);
>  }
>  
>  int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> @@ -199,9 +198,6 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  				dpu_crtc_get_client_type(tmp_crtc)) {
>  			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
>  
> -			perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
> -					dpu_cstate->new_perf.max_per_pipe_ib);
> -
>  			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
>  
>  			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
> @@ -217,7 +213,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>  
>  	for (i = 0; i < kms->num_paths; i++)
> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
> +		icc_set_bw(kms->path[i], avg_bw, kms->catalog->perf->min_dram_ib);
>  
>  	return ret;
>  }
> @@ -320,15 +316,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  		 * 2. new bandwidth vote - "ab or ib vote" is lower
>  		 *    than current vote at end of commit or stop.
>  		 */
> -		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
> -			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
> -			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
> -			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
> +		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
> +		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
>  			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
>  				crtc->base.id, params_changed,
>  				new->bw_ctl, old->bw_ctl);
>  			old->bw_ctl = new->bw_ctl;
> -			old->max_per_pipe_ib = new->max_per_pipe_ib;
>  			update_bus = true;
>  		}
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e02cc2324af2..2bf7836f79bb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -16,12 +16,10 @@
>  
>  /**
>   * struct dpu_core_perf_params - definition of performance parameters
> - * @max_per_pipe_ib: maximum instantaneous bandwidth request
>   * @bw_ctl: arbitrated bandwidth request
>   * @core_clk_rate: core clock rate request
>   */
>  struct dpu_core_perf_params {
> -	u64 max_per_pipe_ib;
>  	u64 bw_ctl;
>  	u64 core_clk_rate;
>  };
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 1edf2b6b0a26..ff5d306b95ed 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1400,8 +1400,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
>  	seq_printf(s, "core_clk_rate: %llu\n",
>  			dpu_crtc->cur_perf.core_clk_rate);
>  	seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
> -	seq_printf(s, "max_per_pipe_ib: %llu\n",
> -				dpu_crtc->cur_perf.max_per_pipe_ib);
>  
>  	return 0;
>  }

Re: [PATCH 3/8] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Dmitry Baryshkov]

On 20/06/2023 13:46, Konrad Dybcio wrote:
> On 20.06.2023 02:08, Dmitry Baryshkov wrote:
>> The max_per_pipe_ib is a constant across all CRTCs and is read from the
>> catalog. Drop corresponding calculations and read the value directly at
>> icc_set_bw() time.
>>
>> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> This looks good, but doesn't apply on next-20230620

Hmm. It applied w/o any issues here.

> 
> Konrad
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 17 +++++------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  2 --
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  2 --
>>   3 files changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> index 9902febc72c0..ba146af73bc5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> @@ -105,13 +105,12 @@ static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
>>   	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>>   
>>   	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
>> -	perf->max_per_pipe_ib = kms->catalog->perf->min_dram_ib;
>>   	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
>>   
>>   	DRM_DEBUG_ATOMIC(
>> -		"crtc=%d clk_rate=%llu core_ib=%llu core_ab=%llu\n",
>> +		"crtc=%d clk_rate=%llu core_ab=%llu\n",
>>   			crtc->base.id, perf->core_clk_rate,
>> -			perf->max_per_pipe_ib, perf->bw_ctl);
>> +			perf->bw_ctl);
>>   }
>>   
>>   int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>> @@ -199,9 +198,6 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>   				dpu_crtc_get_client_type(tmp_crtc)) {
>>   			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
>>   
>> -			perf.max_per_pipe_ib = max(perf.max_per_pipe_ib,
>> -					dpu_cstate->new_perf.max_per_pipe_ib);
>> -
>>   			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
>>   
>>   			DRM_DEBUG_ATOMIC("crtc=%d bw=%llu paths:%d\n",
>> @@ -217,7 +213,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>>   	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>>   
>>   	for (i = 0; i < kms->num_paths; i++)
>> -		icc_set_bw(kms->path[i], avg_bw, perf.max_per_pipe_ib);
>> +		icc_set_bw(kms->path[i], avg_bw, kms->catalog->perf->min_dram_ib);
>>   
>>   	return ret;
>>   }
>> @@ -320,15 +316,12 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>   		 * 2. new bandwidth vote - "ab or ib vote" is lower
>>   		 *    than current vote at end of commit or stop.
>>   		 */
>> -		if ((params_changed && ((new->bw_ctl > old->bw_ctl) ||
>> -			(new->max_per_pipe_ib > old->max_per_pipe_ib)))	||
>> -			(!params_changed && ((new->bw_ctl < old->bw_ctl) ||
>> -			(new->max_per_pipe_ib < old->max_per_pipe_ib)))) {
>> +		if ((params_changed && new->bw_ctl > old->bw_ctl) ||
>> +		    (!params_changed && new->bw_ctl < old->bw_ctl)) {
>>   			DRM_DEBUG_ATOMIC("crtc=%d p=%d new_bw=%llu,old_bw=%llu\n",
>>   				crtc->base.id, params_changed,
>>   				new->bw_ctl, old->bw_ctl);
>>   			old->bw_ctl = new->bw_ctl;
>> -			old->max_per_pipe_ib = new->max_per_pipe_ib;
>>   			update_bus = true;
>>   		}
>>   
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>> index e02cc2324af2..2bf7836f79bb 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>> @@ -16,12 +16,10 @@
>>   
>>   /**
>>    * struct dpu_core_perf_params - definition of performance parameters
>> - * @max_per_pipe_ib: maximum instantaneous bandwidth request
>>    * @bw_ctl: arbitrated bandwidth request
>>    * @core_clk_rate: core clock rate request
>>    */
>>   struct dpu_core_perf_params {
>> -	u64 max_per_pipe_ib;
>>   	u64 bw_ctl;
>>   	u64 core_clk_rate;
>>   };
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 1edf2b6b0a26..ff5d306b95ed 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1400,8 +1400,6 @@ static int dpu_crtc_debugfs_state_show(struct seq_file *s, void *v)
>>   	seq_printf(s, "core_clk_rate: %llu\n",
>>   			dpu_crtc->cur_perf.core_clk_rate);
>>   	seq_printf(s, "bw_ctl: %llu\n", dpu_crtc->cur_perf.bw_ctl);
>> -	seq_printf(s, "max_per_pipe_ib: %llu\n",
>> -				dpu_crtc->cur_perf.max_per_pipe_ib);
>>   
>>   	return 0;
>>   }

-- 
With best wishes
Dmitry

Re: [PATCH 3/8] drm/msm/dpu: drop dpu_core_perf_params::max_per_pipe_ib

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> The max_per_pipe_ib is a constant across all CRTCs and is read from the
> catalog. Drop corresponding calculations and read the value directly at
> icc_set_bw() time.
> 
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

This will need to be dropped as well as we cannot use patch 2 of this 
series. Because with the perf mode, this can be tweaked.

[PATCH 4/8] drm/msm/dpu: rework indentation in dpu_core_perf

[Dmitry Baryshkov]

dpu_core_perf.c contains several multi-line conditions which are hard to
comprehent because of the indentation. Rework the identation of these
conditions to make it easier to understand them.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index ba146af73bc5..f8d5c87d0915 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -148,8 +148,8 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 
 	drm_for_each_crtc(tmp_crtc, crtc->dev) {
 		if (tmp_crtc->enabled &&
-		    (dpu_crtc_get_client_type(tmp_crtc) ==
-				curr_client_type) && (tmp_crtc != crtc)) {
+		    dpu_crtc_get_client_type(tmp_crtc) == curr_client_type &&
+		    tmp_crtc != crtc) {
 			struct dpu_crtc_state *tmp_cstate =
 				to_dpu_crtc_state(tmp_crtc->state);
 
@@ -194,8 +194,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 
 	drm_for_each_crtc(tmp_crtc, crtc->dev) {
 		if (tmp_crtc->enabled &&
-			curr_client_type ==
-				dpu_crtc_get_client_type(tmp_crtc)) {
+		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
 			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
 
 			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
@@ -325,10 +324,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 			update_bus = true;
 		}
 
-		if ((params_changed &&
-			(new->core_clk_rate > old->core_clk_rate)) ||
-			(!params_changed &&
-			(new->core_clk_rate < old->core_clk_rate))) {
+		if ((params_changed && new->core_clk_rate > old->core_clk_rate) ||
+		    (!params_changed && new->core_clk_rate < old->core_clk_rate)) {
 			old->core_clk_rate = new->core_clk_rate;
 			update_clk = true;
 		}
-- 
2.39.2

Re: [PATCH 4/8] drm/msm/dpu: rework indentation in dpu_core_perf

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> dpu_core_perf.c contains several multi-line conditions which are hard to
> comprehent because of the indentation. Rework the identation of these
> conditions to make it easier to understand them.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index ba146af73bc5..f8d5c87d0915 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -148,8 +148,8 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  
>  	drm_for_each_crtc(tmp_crtc, crtc->dev) {
>  		if (tmp_crtc->enabled &&
> -		    (dpu_crtc_get_client_type(tmp_crtc) ==
> -				curr_client_type) && (tmp_crtc != crtc)) {
> +		    dpu_crtc_get_client_type(tmp_crtc) == curr_client_type &&
> +		    tmp_crtc != crtc) {
>  			struct dpu_crtc_state *tmp_cstate =
>  				to_dpu_crtc_state(tmp_crtc->state);
>  
> @@ -194,8 +194,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  
>  	drm_for_each_crtc(tmp_crtc, crtc->dev) {
>  		if (tmp_crtc->enabled &&
> -			curr_client_type ==
> -				dpu_crtc_get_client_type(tmp_crtc)) {
> +		    curr_client_type == dpu_crtc_get_client_type(tmp_crtc)) {
>  			dpu_cstate = to_dpu_crtc_state(tmp_crtc->state);
>  
>  			perf.bw_ctl += dpu_cstate->new_perf.bw_ctl;
> @@ -325,10 +324,8 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  			update_bus = true;
>  		}
>  
> -		if ((params_changed &&
> -			(new->core_clk_rate > old->core_clk_rate)) ||
> -			(!params_changed &&
> -			(new->core_clk_rate < old->core_clk_rate))) {
> +		if ((params_changed && new->core_clk_rate > old->core_clk_rate) ||
> +		    (!params_changed && new->core_clk_rate < old->core_clk_rate)) {
>  			old->core_clk_rate = new->core_clk_rate;
>  			update_clk = true;
>  		}

Re: [PATCH 4/8] drm/msm/dpu: rework indentation in dpu_core_perf

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> dpu_core_perf.c contains several multi-line conditions which are hard to
> comprehent because of the indentation. Rework the identation of these
> conditions to make it easier to understand them.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

I think some of the checks were like this due to checkpatch's earlier 
limit being 80 chars.

With that relaxed, this should be fine. Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

I am unable to apply this in my branch for some reason but, hope this 
doesnt break checkpatch now :)

[PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Dmitry Baryshkov]

The stop_req is true only in the dpu_crtc_disable() case, when
crtc->enable has already been set to false. This renders the stop_req
argument useless. Remove it completely.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index f8d5c87d0915..773e641eab28 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
 }
 
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-		int params_changed, bool stop_req)
+			      int params_changed)
 {
 	struct dpu_core_perf_params *new, *old;
 	bool update_bus = false, update_clk = false;
@@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 	dpu_crtc = to_dpu_crtc(crtc);
 	dpu_cstate = to_dpu_crtc_state(crtc->state);
 
-	DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
-			crtc->base.id, stop_req, kms->perf.core_clk_rate);
+	DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
+			crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
 
 	old = &dpu_crtc->cur_perf;
 	new = &dpu_cstate->new_perf;
 
-	if (crtc->enabled && !stop_req) {
+	if (crtc->enabled) {
 		/*
 		 * cases for bus bandwidth update.
 		 * 1. new bandwidth vote - "ab or ib vote" is higher
@@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 	}
 
 	trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
-		new->core_clk_rate, stop_req, update_bus, update_clk);
+		new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
 
 	if (update_bus) {
 		ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
@@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 	if (update_clk) {
 		clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
 
-		trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
+		trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
 
 		clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
 		ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index 2bf7836f79bb..c29ec72984b8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
  * dpu_core_perf_crtc_update - update performance of the given crtc
  * @crtc: Pointer to crtc
  * @params_changed: true if crtc parameters are modified
- * @stop_req: true if this is a stop request
  * return: zero if success, or error code otherwise
  */
 int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
-		int params_changed, bool stop_req);
+			      int params_changed);
 
 /**
  * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ff5d306b95ed..214229d11e3e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
 void dpu_crtc_complete_commit(struct drm_crtc *crtc)
 {
 	trace_dpu_crtc_complete_commit(DRMID(crtc));
-	dpu_core_perf_crtc_update(crtc, 0, false);
+	dpu_core_perf_crtc_update(crtc, 0);
 	_dpu_crtc_complete_flip(crtc);
 }
 
@@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
 		return;
 
 	/* update performance setting before crtc kickoff */
-	dpu_core_perf_crtc_update(crtc, 1, false);
+	dpu_core_perf_crtc_update(crtc, 1);
 
 	/*
 	 * Final plane updates: Give each plane a chance to complete all
@@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
 		atomic_set(&dpu_crtc->frame_pending, 0);
 	}
 
-	dpu_core_perf_crtc_update(crtc, 0, true);
+	dpu_core_perf_crtc_update(crtc, 0);
 
 	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
-- 
2.39.2

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> The stop_req is true only in the dpu_crtc_disable() case, when
> crtc->enable has already been set to false. This renders the stop_req
> argument useless. Remove it completely.
> 

What about the enable case?

That time dpu_crtc->enabled  will be false but you need valid clock and 
BW that time when you want to enable.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
>   3 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index f8d5c87d0915..773e641eab28 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>   }
>   
>   int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> -		int params_changed, bool stop_req)
> +			      int params_changed)
>   {
>   	struct dpu_core_perf_params *new, *old;
>   	bool update_bus = false, update_clk = false;
> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   	dpu_crtc = to_dpu_crtc(crtc);
>   	dpu_cstate = to_dpu_crtc_state(crtc->state);
>   
> -	DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
> -			crtc->base.id, stop_req, kms->perf.core_clk_rate);
> +	DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
> +			crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
>   
>   	old = &dpu_crtc->cur_perf;
>   	new = &dpu_cstate->new_perf;
>   
> -	if (crtc->enabled && !stop_req) {
> +	if (crtc->enabled) {
>   		/*
>   		 * cases for bus bandwidth update.
>   		 * 1. new bandwidth vote - "ab or ib vote" is higher
> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   	}
>   
>   	trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
> -		new->core_clk_rate, stop_req, update_bus, update_clk);
> +		new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
>   
>   	if (update_bus) {
>   		ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   	if (update_clk) {
>   		clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
>   
> -		trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
> +		trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
>   
>   		clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
>   		ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index 2bf7836f79bb..c29ec72984b8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>    * dpu_core_perf_crtc_update - update performance of the given crtc
>    * @crtc: Pointer to crtc
>    * @params_changed: true if crtc parameters are modified
> - * @stop_req: true if this is a stop request
>    * return: zero if success, or error code otherwise
>    */
>   int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> -		int params_changed, bool stop_req);
> +			      int params_changed);
>   
>   /**
>    * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index ff5d306b95ed..214229d11e3e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>   void dpu_crtc_complete_commit(struct drm_crtc *crtc)
>   {
>   	trace_dpu_crtc_complete_commit(DRMID(crtc));
> -	dpu_core_perf_crtc_update(crtc, 0, false);
> +	dpu_core_perf_crtc_update(crtc, 0);
>   	_dpu_crtc_complete_flip(crtc);
>   }
>   
> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>   		return;
>   
>   	/* update performance setting before crtc kickoff */
> -	dpu_core_perf_crtc_update(crtc, 1, false);
> +	dpu_core_perf_crtc_update(crtc, 1);
>   
>   	/*
>   	 * Final plane updates: Give each plane a chance to complete all
> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>   		atomic_set(&dpu_crtc->frame_pending, 0);
>   	}
>   
> -	dpu_core_perf_crtc_update(crtc, 0, true);
> +	dpu_core_perf_crtc_update(crtc, 0);
>   
>   	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>   		dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> > The stop_req is true only in the dpu_crtc_disable() case, when
> > crtc->enable has already been set to false. This renders the stop_req
> > argument useless. Remove it completely.
> >
>
> What about the enable case?
>
> That time dpu_crtc->enabled  will be false but you need valid clock and
> BW that time when you want to enable.

Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
atomic_enable, while dpu_core_perf_crtc_update() is called further,
during atomic_flush, or from the CRTC event handler. And both these
cases have stop_req as false.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
> >   3 files changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index f8d5c87d0915..773e641eab28 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> >   }
> >
> >   int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > -             int params_changed, bool stop_req)
> > +                           int params_changed)
> >   {
> >       struct dpu_core_perf_params *new, *old;
> >       bool update_bus = false, update_clk = false;
> > @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >       dpu_crtc = to_dpu_crtc(crtc);
> >       dpu_cstate = to_dpu_crtc_state(crtc->state);
> >
> > -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
> > -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
> > +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
> > +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
> >
> >       old = &dpu_crtc->cur_perf;
> >       new = &dpu_cstate->new_perf;
> >
> > -     if (crtc->enabled && !stop_req) {
> > +     if (crtc->enabled) {
> >               /*
> >                * cases for bus bandwidth update.
> >                * 1. new bandwidth vote - "ab or ib vote" is higher
> > @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >       }
> >
> >       trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
> > -             new->core_clk_rate, stop_req, update_bus, update_clk);
> > +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
> >
> >       if (update_bus) {
> >               ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
> > @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >       if (update_clk) {
> >               clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
> >
> > -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
> > +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
> >
> >               clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
> >               ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > index 2bf7836f79bb..c29ec72984b8 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> >    * dpu_core_perf_crtc_update - update performance of the given crtc
> >    * @crtc: Pointer to crtc
> >    * @params_changed: true if crtc parameters are modified
> > - * @stop_req: true if this is a stop request
> >    * return: zero if success, or error code otherwise
> >    */
> >   int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> > -             int params_changed, bool stop_req);
> > +                           int params_changed);
> >
> >   /**
> >    * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index ff5d306b95ed..214229d11e3e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
> >   void dpu_crtc_complete_commit(struct drm_crtc *crtc)
> >   {
> >       trace_dpu_crtc_complete_commit(DRMID(crtc));
> > -     dpu_core_perf_crtc_update(crtc, 0, false);
> > +     dpu_core_perf_crtc_update(crtc, 0);
> >       _dpu_crtc_complete_flip(crtc);
> >   }
> >
> > @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
> >               return;
> >
> >       /* update performance setting before crtc kickoff */
> > -     dpu_core_perf_crtc_update(crtc, 1, false);
> > +     dpu_core_perf_crtc_update(crtc, 1);
> >
> >       /*
> >        * Final plane updates: Give each plane a chance to complete all
> > @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >               atomic_set(&dpu_crtc->frame_pending, 0);
> >       }
> >
> > -     dpu_core_perf_crtc_update(crtc, 0, true);
> > +     dpu_core_perf_crtc_update(crtc, 0);
> >
> >       drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> >               dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Abhinav Kumar]

On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
>>> The stop_req is true only in the dpu_crtc_disable() case, when
>>> crtc->enable has already been set to false. This renders the stop_req
>>> argument useless. Remove it completely.
>>>
>>
>> What about the enable case?
>>
>> That time dpu_crtc->enabled  will be false but you need valid clock and
>> BW that time when you want to enable.
> 
> Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
> atomic_enable, while dpu_core_perf_crtc_update() is called further,
> during atomic_flush, or from the CRTC event handler. And both these
> cases have stop_req as false.
> 

But the first enable will happen with a commit too right?

Thats the one I am referring to.

>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
>>>    3 files changed, 10 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index f8d5c87d0915..773e641eab28 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>    }
>>>
>>>    int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>> -             int params_changed, bool stop_req)
>>> +                           int params_changed)
>>>    {
>>>        struct dpu_core_perf_params *new, *old;
>>>        bool update_bus = false, update_clk = false;
>>> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>        dpu_crtc = to_dpu_crtc(crtc);
>>>        dpu_cstate = to_dpu_crtc_state(crtc->state);
>>>
>>> -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
>>> -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
>>> +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
>>> +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
>>>
>>>        old = &dpu_crtc->cur_perf;
>>>        new = &dpu_cstate->new_perf;
>>>
>>> -     if (crtc->enabled && !stop_req) {
>>> +     if (crtc->enabled) {
>>>                /*
>>>                 * cases for bus bandwidth update.
>>>                 * 1. new bandwidth vote - "ab or ib vote" is higher
>>> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>        }
>>>
>>>        trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
>>> -             new->core_clk_rate, stop_req, update_bus, update_clk);
>>> +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
>>>
>>>        if (update_bus) {
>>>                ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
>>> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>        if (update_clk) {
>>>                clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
>>>
>>> -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
>>> +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
>>>
>>>                clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
>>>                ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> index 2bf7836f79bb..c29ec72984b8 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>     * dpu_core_perf_crtc_update - update performance of the given crtc
>>>     * @crtc: Pointer to crtc
>>>     * @params_changed: true if crtc parameters are modified
>>> - * @stop_req: true if this is a stop request
>>>     * return: zero if success, or error code otherwise
>>>     */
>>>    int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>> -             int params_changed, bool stop_req);
>>> +                           int params_changed);
>>>
>>>    /**
>>>     * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index ff5d306b95ed..214229d11e3e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>>>    void dpu_crtc_complete_commit(struct drm_crtc *crtc)
>>>    {
>>>        trace_dpu_crtc_complete_commit(DRMID(crtc));
>>> -     dpu_core_perf_crtc_update(crtc, 0, false);
>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>        _dpu_crtc_complete_flip(crtc);
>>>    }
>>>
>>> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>>>                return;
>>>
>>>        /* update performance setting before crtc kickoff */
>>> -     dpu_core_perf_crtc_update(crtc, 1, false);
>>> +     dpu_core_perf_crtc_update(crtc, 1);
>>>
>>>        /*
>>>         * Final plane updates: Give each plane a chance to complete all
>>> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>>>                atomic_set(&dpu_crtc->frame_pending, 0);
>>>        }
>>>
>>> -     dpu_core_perf_crtc_update(crtc, 0, true);
>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>
>>>        drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>>>                dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> 
> 
> 

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 01:55, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
> > On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> >>> The stop_req is true only in the dpu_crtc_disable() case, when
> >>> crtc->enable has already been set to false. This renders the stop_req
> >>> argument useless. Remove it completely.
> >>>
> >>
> >> What about the enable case?
> >>
> >> That time dpu_crtc->enabled  will be false but you need valid clock and
> >> BW that time when you want to enable.
> >
> > Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
> > atomic_enable, while dpu_core_perf_crtc_update() is called further,
> > during atomic_flush, or from the CRTC event handler. And both these
> > cases have stop_req as false.
> >
>
> But the first enable will happen with a commit too right?

Which one? Could you please point to it?

>
> Thats the one I am referring to.
>
> >>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
> >>>    3 files changed, 10 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> index f8d5c87d0915..773e641eab28 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> >>>    }
> >>>
> >>>    int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>> -             int params_changed, bool stop_req)
> >>> +                           int params_changed)
> >>>    {
> >>>        struct dpu_core_perf_params *new, *old;
> >>>        bool update_bus = false, update_clk = false;
> >>> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>        dpu_crtc = to_dpu_crtc(crtc);
> >>>        dpu_cstate = to_dpu_crtc_state(crtc->state);
> >>>
> >>> -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
> >>> -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
> >>> +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
> >>> +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
> >>>
> >>>        old = &dpu_crtc->cur_perf;
> >>>        new = &dpu_cstate->new_perf;
> >>>
> >>> -     if (crtc->enabled && !stop_req) {
> >>> +     if (crtc->enabled) {
> >>>                /*
> >>>                 * cases for bus bandwidth update.
> >>>                 * 1. new bandwidth vote - "ab or ib vote" is higher
> >>> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>        }
> >>>
> >>>        trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
> >>> -             new->core_clk_rate, stop_req, update_bus, update_clk);
> >>> +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
> >>>
> >>>        if (update_bus) {
> >>>                ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
> >>> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>        if (update_clk) {
> >>>                clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
> >>>
> >>> -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
> >>> +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
> >>>
> >>>                clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
> >>>                ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> index 2bf7836f79bb..c29ec72984b8 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> >>>     * dpu_core_perf_crtc_update - update performance of the given crtc
> >>>     * @crtc: Pointer to crtc
> >>>     * @params_changed: true if crtc parameters are modified
> >>> - * @stop_req: true if this is a stop request
> >>>     * return: zero if success, or error code otherwise
> >>>     */
> >>>    int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>> -             int params_changed, bool stop_req);
> >>> +                           int params_changed);
> >>>
> >>>    /**
> >>>     * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> index ff5d306b95ed..214229d11e3e 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
> >>>    void dpu_crtc_complete_commit(struct drm_crtc *crtc)
> >>>    {
> >>>        trace_dpu_crtc_complete_commit(DRMID(crtc));
> >>> -     dpu_core_perf_crtc_update(crtc, 0, false);
> >>> +     dpu_core_perf_crtc_update(crtc, 0);
> >>>        _dpu_crtc_complete_flip(crtc);
> >>>    }
> >>>
> >>> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
> >>>                return;
> >>>
> >>>        /* update performance setting before crtc kickoff */
> >>> -     dpu_core_perf_crtc_update(crtc, 1, false);
> >>> +     dpu_core_perf_crtc_update(crtc, 1);
> >>>
> >>>        /*
> >>>         * Final plane updates: Give each plane a chance to complete all
> >>> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >>>                atomic_set(&dpu_crtc->frame_pending, 0);
> >>>        }
> >>>
> >>> -     dpu_core_perf_crtc_update(crtc, 0, true);
> >>> +     dpu_core_perf_crtc_update(crtc, 0);
> >>>
> >>>        drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> >>>                dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Abhinav Kumar]

On 7/3/2023 4:01 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 01:55, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
>>> On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
>>>>> The stop_req is true only in the dpu_crtc_disable() case, when
>>>>> crtc->enable has already been set to false. This renders the stop_req
>>>>> argument useless. Remove it completely.
>>>>>
>>>>
>>>> What about the enable case?
>>>>
>>>> That time dpu_crtc->enabled  will be false but you need valid clock and
>>>> BW that time when you want to enable.
>>>
>>> Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
>>> atomic_enable, while dpu_core_perf_crtc_update() is called further,
>>> during atomic_flush, or from the CRTC event handler. And both these
>>> cases have stop_req as false.
>>>
>>
>> But the first enable will happen with a commit too right?
> 
> Which one? Could you please point to it?
> 

My question was that

drm_atomic_helper_commit_planes which calls 
drm_crtc_helper_funcs::atomic_flush (which calls 
dpu_core_perf_crtc_update) is called before 
drm_crtc_helper_funcs::atomic_enable which gets called only during 
drm_atomic_helper_commit_modeset_enables in msm_atomic_commit_tail

Wouldnt that get broken now?


>>
>> Thats the one I am referring to.
>>
>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
>>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
>>>>>     3 files changed, 10 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> index f8d5c87d0915..773e641eab28 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>>>     }
>>>>>
>>>>>     int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>> -             int params_changed, bool stop_req)
>>>>> +                           int params_changed)
>>>>>     {
>>>>>         struct dpu_core_perf_params *new, *old;
>>>>>         bool update_bus = false, update_clk = false;
>>>>> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>         dpu_crtc = to_dpu_crtc(crtc);
>>>>>         dpu_cstate = to_dpu_crtc_state(crtc->state);
>>>>>
>>>>> -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
>>>>> -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
>>>>> +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
>>>>> +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
>>>>>
>>>>>         old = &dpu_crtc->cur_perf;
>>>>>         new = &dpu_cstate->new_perf;
>>>>>
>>>>> -     if (crtc->enabled && !stop_req) {
>>>>> +     if (crtc->enabled) {
>>>>>                 /*
>>>>>                  * cases for bus bandwidth update.
>>>>>                  * 1. new bandwidth vote - "ab or ib vote" is higher
>>>>> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>         }
>>>>>
>>>>>         trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
>>>>> -             new->core_clk_rate, stop_req, update_bus, update_clk);
>>>>> +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
>>>>>
>>>>>         if (update_bus) {
>>>>>                 ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
>>>>> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>         if (update_clk) {
>>>>>                 clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
>>>>>
>>>>> -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
>>>>> +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
>>>>>
>>>>>                 clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
>>>>>                 ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> index 2bf7836f79bb..c29ec72984b8 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>>>      * dpu_core_perf_crtc_update - update performance of the given crtc
>>>>>      * @crtc: Pointer to crtc
>>>>>      * @params_changed: true if crtc parameters are modified
>>>>> - * @stop_req: true if this is a stop request
>>>>>      * return: zero if success, or error code otherwise
>>>>>      */
>>>>>     int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>> -             int params_changed, bool stop_req);
>>>>> +                           int params_changed);
>>>>>
>>>>>     /**
>>>>>      * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> index ff5d306b95ed..214229d11e3e 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>>>>>     void dpu_crtc_complete_commit(struct drm_crtc *crtc)
>>>>>     {
>>>>>         trace_dpu_crtc_complete_commit(DRMID(crtc));
>>>>> -     dpu_core_perf_crtc_update(crtc, 0, false);
>>>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>>>         _dpu_crtc_complete_flip(crtc);
>>>>>     }
>>>>>
>>>>> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>>>>>                 return;
>>>>>
>>>>>         /* update performance setting before crtc kickoff */
>>>>> -     dpu_core_perf_crtc_update(crtc, 1, false);
>>>>> +     dpu_core_perf_crtc_update(crtc, 1);
>>>>>
>>>>>         /*
>>>>>          * Final plane updates: Give each plane a chance to complete all
>>>>> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>>>>>                 atomic_set(&dpu_crtc->frame_pending, 0);
>>>>>         }
>>>>>
>>>>> -     dpu_core_perf_crtc_update(crtc, 0, true);
>>>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>>>
>>>>>         drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>>>>>                 dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
>>>
>>>
>>>
> 
> 
> 

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 02:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/3/2023 4:01 PM, Dmitry Baryshkov wrote:
> > On Tue, 4 Jul 2023 at 01:55, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
> >>> On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> >>>>> The stop_req is true only in the dpu_crtc_disable() case, when
> >>>>> crtc->enable has already been set to false. This renders the stop_req
> >>>>> argument useless. Remove it completely.
> >>>>>
> >>>>
> >>>> What about the enable case?
> >>>>
> >>>> That time dpu_crtc->enabled  will be false but you need valid clock and
> >>>> BW that time when you want to enable.
> >>>
> >>> Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
> >>> atomic_enable, while dpu_core_perf_crtc_update() is called further,
> >>> during atomic_flush, or from the CRTC event handler. And both these
> >>> cases have stop_req as false.
> >>>
> >>
> >> But the first enable will happen with a commit too right?
> >
> > Which one? Could you please point to it?
> >
>
> My question was that
>
> drm_atomic_helper_commit_planes which calls
> drm_crtc_helper_funcs::atomic_flush (which calls
> dpu_core_perf_crtc_update) is called before
> drm_crtc_helper_funcs::atomic_enable which gets called only during
> drm_atomic_helper_commit_modeset_enables in msm_atomic_commit_tail
>
> Wouldnt that get broken now?

No, crtc->enabled is false, as you pointed out, so (crtc->enabled &&
!stop_req) will be false too.

>
>
> >>
> >> Thats the one I am referring to.
> >>
> >>>>
> >>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>>>> ---
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
> >>>>>     drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
> >>>>>     3 files changed, 10 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> index f8d5c87d0915..773e641eab28 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>>>> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
> >>>>>     }
> >>>>>
> >>>>>     int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>>> -             int params_changed, bool stop_req)
> >>>>> +                           int params_changed)
> >>>>>     {
> >>>>>         struct dpu_core_perf_params *new, *old;
> >>>>>         bool update_bus = false, update_clk = false;
> >>>>> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>>>         dpu_crtc = to_dpu_crtc(crtc);
> >>>>>         dpu_cstate = to_dpu_crtc_state(crtc->state);
> >>>>>
> >>>>> -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
> >>>>> -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
> >>>>> +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
> >>>>> +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
> >>>>>
> >>>>>         old = &dpu_crtc->cur_perf;
> >>>>>         new = &dpu_cstate->new_perf;
> >>>>>
> >>>>> -     if (crtc->enabled && !stop_req) {
> >>>>> +     if (crtc->enabled) {
> >>>>>                 /*
> >>>>>                  * cases for bus bandwidth update.
> >>>>>                  * 1. new bandwidth vote - "ab or ib vote" is higher
> >>>>> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>>>         }
> >>>>>
> >>>>>         trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
> >>>>> -             new->core_clk_rate, stop_req, update_bus, update_clk);
> >>>>> +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
> >>>>>
> >>>>>         if (update_bus) {
> >>>>>                 ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
> >>>>> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>>>         if (update_clk) {
> >>>>>                 clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
> >>>>>
> >>>>> -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
> >>>>> +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
> >>>>>
> >>>>>                 clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
> >>>>>                 ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> index 2bf7836f79bb..c29ec72984b8 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>>>> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
> >>>>>      * dpu_core_perf_crtc_update - update performance of the given crtc
> >>>>>      * @crtc: Pointer to crtc
> >>>>>      * @params_changed: true if crtc parameters are modified
> >>>>> - * @stop_req: true if this is a stop request
> >>>>>      * return: zero if success, or error code otherwise
> >>>>>      */
> >>>>>     int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>>> -             int params_changed, bool stop_req);
> >>>>> +                           int params_changed);
> >>>>>
> >>>>>     /**
> >>>>>      * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
> >>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> index ff5d306b95ed..214229d11e3e 100644
> >>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >>>>> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
> >>>>>     void dpu_crtc_complete_commit(struct drm_crtc *crtc)
> >>>>>     {
> >>>>>         trace_dpu_crtc_complete_commit(DRMID(crtc));
> >>>>> -     dpu_core_perf_crtc_update(crtc, 0, false);
> >>>>> +     dpu_core_perf_crtc_update(crtc, 0);
> >>>>>         _dpu_crtc_complete_flip(crtc);
> >>>>>     }
> >>>>>
> >>>>> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
> >>>>>                 return;
> >>>>>
> >>>>>         /* update performance setting before crtc kickoff */
> >>>>> -     dpu_core_perf_crtc_update(crtc, 1, false);
> >>>>> +     dpu_core_perf_crtc_update(crtc, 1);
> >>>>>
> >>>>>         /*
> >>>>>          * Final plane updates: Give each plane a chance to complete all
> >>>>> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >>>>>                 atomic_set(&dpu_crtc->frame_pending, 0);
> >>>>>         }
> >>>>>
> >>>>> -     dpu_core_perf_crtc_update(crtc, 0, true);
> >>>>> +     dpu_core_perf_crtc_update(crtc, 0);
> >>>>>
> >>>>>         drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> >>>>>                 dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
> >>>
> >>>
> >>>
> >
> >
> >



-- 
With best wishes
Dmitry

Re: [PATCH 5/8] drm/msm/dpu: drop the dpu_core_perf_crtc_update()'s stop_req param

[Abhinav Kumar]

On 7/3/2023 5:28 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 02:16, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 7/3/2023 4:01 PM, Dmitry Baryshkov wrote:
>>> On Tue, 4 Jul 2023 at 01:55, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On 7/3/2023 3:53 PM, Dmitry Baryshkov wrote:
>>>>> On Tue, 4 Jul 2023 at 01:37, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
>>>>>>> The stop_req is true only in the dpu_crtc_disable() case, when
>>>>>>> crtc->enable has already been set to false. This renders the stop_req
>>>>>>> argument useless. Remove it completely.
>>>>>>>
>>>>>>
>>>>>> What about the enable case?
>>>>>>
>>>>>> That time dpu_crtc->enabled  will be false but you need valid clock and
>>>>>> BW that time when you want to enable.
>>>>>
>>>>> Maybe I'm missing something here. The driver sets dpu_crtc->enabled in
>>>>> atomic_enable, while dpu_core_perf_crtc_update() is called further,
>>>>> during atomic_flush, or from the CRTC event handler. And both these
>>>>> cases have stop_req as false.
>>>>>
>>>>
>>>> But the first enable will happen with a commit too right?
>>>
>>> Which one? Could you please point to it?
>>>
>>
>> My question was that
>>
>> drm_atomic_helper_commit_planes which calls
>> drm_crtc_helper_funcs::atomic_flush (which calls
>> dpu_core_perf_crtc_update) is called before
>> drm_crtc_helper_funcs::atomic_enable which gets called only during
>> drm_atomic_helper_commit_modeset_enables in msm_atomic_commit_tail
>>
>> Wouldnt that get broken now?
> 
> No, crtc->enabled is false, as you pointed out, so (crtc->enabled &&
> !stop_req) will be false too.
> 

ah okay, no further concerns,


Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

>>
>>
>>>>
>>>> Thats the one I am referring to.
>>>>
>>>>>>
>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ++++++------
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  3 +--
>>>>>>>      drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  6 +++---
>>>>>>>      3 files changed, 10 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> index f8d5c87d0915..773e641eab28 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>>>>> @@ -277,7 +277,7 @@ static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
>>>>>>>      }
>>>>>>>
>>>>>>>      int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>> -             int params_changed, bool stop_req)
>>>>>>> +                           int params_changed)
>>>>>>>      {
>>>>>>>          struct dpu_core_perf_params *new, *old;
>>>>>>>          bool update_bus = false, update_clk = false;
>>>>>>> @@ -301,13 +301,13 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>>          dpu_crtc = to_dpu_crtc(crtc);
>>>>>>>          dpu_cstate = to_dpu_crtc_state(crtc->state);
>>>>>>>
>>>>>>> -     DRM_DEBUG_ATOMIC("crtc:%d stop_req:%d core_clk:%llu\n",
>>>>>>> -                     crtc->base.id, stop_req, kms->perf.core_clk_rate);
>>>>>>> +     DRM_DEBUG_ATOMIC("crtc:%d enabled:%d core_clk:%llu\n",
>>>>>>> +                     crtc->base.id, crtc->enabled, kms->perf.core_clk_rate);
>>>>>>>
>>>>>>>          old = &dpu_crtc->cur_perf;
>>>>>>>          new = &dpu_cstate->new_perf;
>>>>>>>
>>>>>>> -     if (crtc->enabled && !stop_req) {
>>>>>>> +     if (crtc->enabled) {
>>>>>>>                  /*
>>>>>>>                   * cases for bus bandwidth update.
>>>>>>>                   * 1. new bandwidth vote - "ab or ib vote" is higher
>>>>>>> @@ -337,7 +337,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>>          }
>>>>>>>
>>>>>>>          trace_dpu_perf_crtc_update(crtc->base.id, new->bw_ctl,
>>>>>>> -             new->core_clk_rate, stop_req, update_bus, update_clk);
>>>>>>> +             new->core_clk_rate, !crtc->enabled, update_bus, update_clk);
>>>>>>>
>>>>>>>          if (update_bus) {
>>>>>>>                  ret = _dpu_core_perf_crtc_update_bus(kms, crtc);
>>>>>>> @@ -355,7 +355,7 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>>          if (update_clk) {
>>>>>>>                  clk_rate = _dpu_core_perf_get_core_clk_rate(kms);
>>>>>>>
>>>>>>> -             trace_dpu_core_perf_update_clk(kms->dev, stop_req, clk_rate);
>>>>>>> +             trace_dpu_core_perf_update_clk(kms->dev, !crtc->enabled, clk_rate);
>>>>>>>
>>>>>>>                  clk_rate = min(clk_rate, kms->perf.max_core_clk_rate);
>>>>>>>                  ret = dev_pm_opp_set_rate(&kms->pdev->dev, clk_rate);
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> index 2bf7836f79bb..c29ec72984b8 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>>>>>> @@ -58,11 +58,10 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>>>>>>>       * dpu_core_perf_crtc_update - update performance of the given crtc
>>>>>>>       * @crtc: Pointer to crtc
>>>>>>>       * @params_changed: true if crtc parameters are modified
>>>>>>> - * @stop_req: true if this is a stop request
>>>>>>>       * return: zero if success, or error code otherwise
>>>>>>>       */
>>>>>>>      int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>>>>> -             int params_changed, bool stop_req);
>>>>>>> +                           int params_changed);
>>>>>>>
>>>>>>>      /**
>>>>>>>       * dpu_core_perf_crtc_release_bw - release bandwidth of the given crtc
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> index ff5d306b95ed..214229d11e3e 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>>>>> @@ -718,7 +718,7 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>>>>>>>      void dpu_crtc_complete_commit(struct drm_crtc *crtc)
>>>>>>>      {
>>>>>>>          trace_dpu_crtc_complete_commit(DRMID(crtc));
>>>>>>> -     dpu_core_perf_crtc_update(crtc, 0, false);
>>>>>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>>>>>          _dpu_crtc_complete_flip(crtc);
>>>>>>>      }
>>>>>>>
>>>>>>> @@ -884,7 +884,7 @@ static void dpu_crtc_atomic_flush(struct drm_crtc *crtc,
>>>>>>>                  return;
>>>>>>>
>>>>>>>          /* update performance setting before crtc kickoff */
>>>>>>> -     dpu_core_perf_crtc_update(crtc, 1, false);
>>>>>>> +     dpu_core_perf_crtc_update(crtc, 1);
>>>>>>>
>>>>>>>          /*
>>>>>>>           * Final plane updates: Give each plane a chance to complete all
>>>>>>> @@ -1100,7 +1100,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>>>>>>>                  atomic_set(&dpu_crtc->frame_pending, 0);
>>>>>>>          }
>>>>>>>
>>>>>>> -     dpu_core_perf_crtc_update(crtc, 0, true);
>>>>>>> +     dpu_core_perf_crtc_update(crtc, 0);
>>>>>>>
>>>>>>>          drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>>>>>>>                  dpu_encoder_register_frame_event_callback(encoder, NULL, NULL);
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
> 
> 
> 

[PATCH 6/8] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

[Dmitry Baryshkov]

Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
full-featured catalog data.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 52 ++++++++-----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
 3 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 773e641eab28..78a7e3ea27a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -19,11 +19,11 @@
 
 /**
  * _dpu_core_perf_calc_bw() - to calculate BW per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * Return: returns aggregated BW for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
 		struct drm_crtc *crtc)
 {
 	struct drm_plane *plane;
@@ -39,7 +39,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
 		crtc_plane_bw += pstate->plane_fetch_bw;
 	}
 
-	bw_factor = kms->catalog->perf->bw_inefficiency_factor;
+	bw_factor = perf_cfg->bw_inefficiency_factor;
 	if (bw_factor) {
 		crtc_plane_bw *= bw_factor;
 		do_div(crtc_plane_bw, 100);
@@ -50,12 +50,12 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
 
 /**
  * _dpu_core_perf_calc_clk() - to calculate clock per crtc
- * @kms:  pointer to the dpu_kms
+ * @perf_cfg: performance configuration
  * @crtc: pointer to a crtc
  * @state: pointer to a crtc state
  * Return: returns max clk for all planes in crtc.
  */
-static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
+static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg,
 		struct drm_crtc *crtc, struct drm_crtc_state *state)
 {
 	struct drm_plane *plane;
@@ -76,7 +76,7 @@ static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
 		crtc_clk = max(pstate->plane_clk, crtc_clk);
 	}
 
-	clk_factor = kms->catalog->perf->clk_inefficiency_factor;
+	clk_factor = perf_cfg->clk_inefficiency_factor;
 	if (clk_factor) {
 		crtc_clk *= clk_factor;
 		do_div(crtc_clk, 100);
@@ -92,20 +92,20 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
 	return to_dpu_kms(priv->kms);
 }
 
-static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
+static void _dpu_core_perf_calc_crtc(const struct dpu_perf_cfg *perf_cfg,
 		struct drm_crtc *crtc,
 		struct drm_crtc_state *state,
 		struct dpu_core_perf_params *perf)
 {
-	if (!kms || !kms->catalog || !crtc || !state || !perf) {
+	if (!perf_cfg || !crtc || !state || !perf) {
 		DPU_ERROR("invalid parameters\n");
 		return;
 	}
 
 	memset(perf, 0, sizeof(struct dpu_core_perf_params));
 
-	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
-	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
+	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
+	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
 
 	DRM_DEBUG_ATOMIC(
 		"crtc=%d clk_rate=%llu core_ab=%llu\n",
@@ -122,6 +122,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 	struct dpu_crtc_state *dpu_cstate;
 	struct drm_crtc *tmp_crtc;
 	struct dpu_kms *kms;
+	const struct dpu_perf_cfg *perf_cfg;
 
 	if (!crtc || !state) {
 		DPU_ERROR("invalid crtc\n");
@@ -129,10 +130,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 	}
 
 	kms = _dpu_crtc_get_kms(crtc);
-	if (!kms->catalog) {
-		DPU_ERROR("invalid parameters\n");
-		return 0;
-	}
+	perf_cfg = kms->perf.perf_cfg;
 
 	/* we only need bandwidth check on real-time clients (interfaces) */
 	if (dpu_crtc_get_client_type(crtc) == NRT_CLIENT)
@@ -141,7 +139,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 	dpu_cstate = to_dpu_crtc_state(state);
 
 	/* obtain new values */
-	_dpu_core_perf_calc_crtc(kms, crtc, state, &dpu_cstate->new_perf);
+	_dpu_core_perf_calc_crtc(perf_cfg, crtc, state, &dpu_cstate->new_perf);
 
 	bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl;
 	curr_client_type = dpu_crtc_get_client_type(crtc);
@@ -164,7 +162,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
 		bw = DIV_ROUND_UP_ULL(bw_sum_of_intfs, 1000);
 		DRM_DEBUG_ATOMIC("calculated bandwidth=%uk\n", bw);
 
-		threshold = kms->catalog->perf->max_bw_high;
+		threshold = perf_cfg->max_bw_high;
 
 		DRM_DEBUG_ATOMIC("final threshold bw limit = %d\n", threshold);
 
@@ -212,7 +210,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
 	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
 
 	for (i = 0; i < kms->num_paths; i++)
-		icc_set_bw(kms->path[i], avg_bw, kms->catalog->perf->min_dram_ib);
+		icc_set_bw(kms->path[i], avg_bw, kms->perf.perf_cfg->min_dram_ib);
 
 	return ret;
 }
@@ -293,10 +291,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 	}
 
 	kms = _dpu_crtc_get_kms(crtc);
-	if (!kms->catalog) {
-		DPU_ERROR("invalid kms\n");
-		return -EINVAL;
-	}
 
 	dpu_crtc = to_dpu_crtc(crtc);
 	dpu_cstate = to_dpu_crtc_state(crtc->state);
@@ -375,7 +369,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
 int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 {
 	struct dpu_core_perf *perf = &dpu_kms->perf;
-	const struct dpu_mdss_cfg *catalog = perf->catalog;
 	struct dentry *entry;
 
 	entry = debugfs_create_dir("core_perf", parent);
@@ -387,15 +380,15 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 	debugfs_create_u32("enable_bw_release", 0600, entry,
 			(u32 *)&perf->enable_bw_release);
 	debugfs_create_u32("threshold_low", 0600, entry,
-			(u32 *)&catalog->perf->max_bw_low);
+			(u32 *)&perf->perf_cfg->max_bw_low);
 	debugfs_create_u32("threshold_high", 0600, entry,
-			(u32 *)&catalog->perf->max_bw_high);
+			(u32 *)&perf->perf_cfg->max_bw_high);
 	debugfs_create_u32("min_core_ib", 0600, entry,
-			(u32 *)&catalog->perf->min_core_ib);
+			(u32 *)&perf->perf_cfg->min_core_ib);
 	debugfs_create_u32("min_llcc_ib", 0600, entry,
-			(u32 *)&catalog->perf->min_llcc_ib);
+			(u32 *)&perf->perf_cfg->min_llcc_ib);
 	debugfs_create_u32("min_dram_ib", 0600, entry,
-			(u32 *)&catalog->perf->min_dram_ib);
+			(u32 *)&perf->perf_cfg->min_dram_ib);
 
 	return 0;
 }
@@ -410,17 +403,16 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
 
 	perf->max_core_clk_rate = 0;
 	perf->core_clk = NULL;
-	perf->catalog = NULL;
 	perf->dev = NULL;
 }
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
 		struct drm_device *dev,
-		const struct dpu_mdss_cfg *catalog,
+		const struct dpu_perf_cfg *perf_cfg,
 		struct clk *core_clk)
 {
 	perf->dev = dev;
-	perf->catalog = catalog;
+	perf->perf_cfg = perf_cfg;
 	perf->core_clk = core_clk;
 
 	perf->max_core_clk_rate = clk_get_rate(core_clk);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index c29ec72984b8..e8a7916b6f71 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -28,7 +28,7 @@ struct dpu_core_perf_params {
  * struct dpu_core_perf - definition of core performance context
  * @dev: Pointer to drm device
  * @debugfs_root: top level debug folder
- * @catalog: Pointer to catalog configuration
+ * @perf_cfg: Platform-specific performance configuration
  * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
  * @max_core_clk_rate: maximum allowable core clock rate
@@ -38,7 +38,7 @@ struct dpu_core_perf_params {
 struct dpu_core_perf {
 	struct drm_device *dev;
 	struct dentry *debugfs_root;
-	const struct dpu_mdss_cfg *catalog;
+	const struct dpu_perf_cfg *perf_cfg;
 	struct clk *core_clk;
 	u64 core_clk_rate;
 	u64 max_core_clk_rate;
@@ -79,12 +79,12 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
  * @dev: Pointer to drm device
- * @catalog: Pointer to catalog
+ * @perf_cfg: Pointer to platform performance configuration
  * @core_clk: pointer to core clock
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
 		struct drm_device *dev,
-		const struct dpu_mdss_cfg *catalog,
+		const struct dpu_perf_cfg *perf_cfg,
 		struct clk *core_clk);
 
 struct dpu_kms;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..6e62606e32de 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1115,7 +1115,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 		dpu_kms->hw_vbif[vbif->id] = hw;
 	}
 
-	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
+	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog->perf,
 			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
 	if (rc) {
 		DPU_ERROR("failed to init perf %d\n", rc);
-- 
2.39.2

Re: [PATCH 6/8] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
> full-featured catalog data.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Check below.

>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 52 ++++++++-----------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
>  3 files changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 773e641eab28..78a7e3ea27a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -19,11 +19,11 @@
>  
>  /**
>   * _dpu_core_perf_calc_bw() - to calculate BW per crtc
> - * @kms:  pointer to the dpu_kms
> + * @perf_cfg: performance configuration
>   * @crtc: pointer to a crtc
>   * Return: returns aggregated BW for all planes in crtc.
>   */
> -static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
> +static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
>  		struct drm_crtc *crtc)
>  {
>  	struct drm_plane *plane;
> @@ -39,7 +39,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>  		crtc_plane_bw += pstate->plane_fetch_bw;
>  	}
>  
> -	bw_factor = kms->catalog->perf->bw_inefficiency_factor;
> +	bw_factor = perf_cfg->bw_inefficiency_factor;
It's set to 120 for all SoCs.. and it sounds very much like some kind of a
hack.

The 105 on the other inefficiency factor is easy to spot:

(1024/1000)^2 = 1.048576 =~= 1.05 = 105%

It comes from a MiB-MB-MHz conversion that Qcom splattered all over 
downstream as due to ancient tragical design decisions in msmbus
(which leak to the downstream interconnect a bit):

The logic needs to get some input that corresponds to a clock rate
of a bus clock (19.2, 200, 300 Mhz etc.) but the APIs expect a Kbps
value. So at one point they invented a MHZ_TO_MBPS macro which did this
conversion the other way around and probably had to account for it.

I think they tried to make it make more sense, but it ended up being
even more spaghetti :/

Not yet sure how it's done on RPMh icc, but with SMD RPM, passing e.g.

opp-peak-kBps = <(200 * 8 * 1000)>; # 200 MHz * 8-wide * KHz-to-MHz

results in a "correct" end rate.

Konrad
>  	if (bw_factor) {
>  		crtc_plane_bw *= bw_factor;
>  		do_div(crtc_plane_bw, 100);
> @@ -50,12 +50,12 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>  
>  /**
>   * _dpu_core_perf_calc_clk() - to calculate clock per crtc
> - * @kms:  pointer to the dpu_kms
> + * @perf_cfg: performance configuration
>   * @crtc: pointer to a crtc
>   * @state: pointer to a crtc state
>   * Return: returns max clk for all planes in crtc.
>   */
> -static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
> +static u64 _dpu_core_perf_calc_clk(const struct dpu_perf_cfg *perf_cfg,
>  		struct drm_crtc *crtc, struct drm_crtc_state *state)
>  {
>  	struct drm_plane *plane;
> @@ -76,7 +76,7 @@ static u64 _dpu_core_perf_calc_clk(struct dpu_kms *kms,
>  		crtc_clk = max(pstate->plane_clk, crtc_clk);
>  	}
>  
> -	clk_factor = kms->catalog->perf->clk_inefficiency_factor;
> +	clk_factor = perf_cfg->clk_inefficiency_factor;
>  	if (clk_factor) {
>  		crtc_clk *= clk_factor;
>  		do_div(crtc_clk, 100);
> @@ -92,20 +92,20 @@ static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>  	return to_dpu_kms(priv->kms);
>  }
>  
> -static void _dpu_core_perf_calc_crtc(struct dpu_kms *kms,
> +static void _dpu_core_perf_calc_crtc(const struct dpu_perf_cfg *perf_cfg,
>  		struct drm_crtc *crtc,
>  		struct drm_crtc_state *state,
>  		struct dpu_core_perf_params *perf)
>  {
> -	if (!kms || !kms->catalog || !crtc || !state || !perf) {
> +	if (!perf_cfg || !crtc || !state || !perf) {
>  		DPU_ERROR("invalid parameters\n");
>  		return;
>  	}
>  
>  	memset(perf, 0, sizeof(struct dpu_core_perf_params));
>  
> -	perf->bw_ctl = _dpu_core_perf_calc_bw(kms, crtc);
> -	perf->core_clk_rate = _dpu_core_perf_calc_clk(kms, crtc, state);
> +	perf->bw_ctl = _dpu_core_perf_calc_bw(perf_cfg, crtc);
> +	perf->core_clk_rate = _dpu_core_perf_calc_clk(perf_cfg, crtc, state);
>  
>  	DRM_DEBUG_ATOMIC(
>  		"crtc=%d clk_rate=%llu core_ab=%llu\n",
> @@ -122,6 +122,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  	struct dpu_crtc_state *dpu_cstate;
>  	struct drm_crtc *tmp_crtc;
>  	struct dpu_kms *kms;
> +	const struct dpu_perf_cfg *perf_cfg;
>  
>  	if (!crtc || !state) {
>  		DPU_ERROR("invalid crtc\n");
> @@ -129,10 +130,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  	}
>  
>  	kms = _dpu_crtc_get_kms(crtc);
> -	if (!kms->catalog) {
> -		DPU_ERROR("invalid parameters\n");
> -		return 0;
> -	}
> +	perf_cfg = kms->perf.perf_cfg;
>  
>  	/* we only need bandwidth check on real-time clients (interfaces) */
>  	if (dpu_crtc_get_client_type(crtc) == NRT_CLIENT)
> @@ -141,7 +139,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  	dpu_cstate = to_dpu_crtc_state(state);
>  
>  	/* obtain new values */
> -	_dpu_core_perf_calc_crtc(kms, crtc, state, &dpu_cstate->new_perf);
> +	_dpu_core_perf_calc_crtc(perf_cfg, crtc, state, &dpu_cstate->new_perf);
>  
>  	bw_sum_of_intfs = dpu_cstate->new_perf.bw_ctl;
>  	curr_client_type = dpu_crtc_get_client_type(crtc);
> @@ -164,7 +162,7 @@ int dpu_core_perf_crtc_check(struct drm_crtc *crtc,
>  		bw = DIV_ROUND_UP_ULL(bw_sum_of_intfs, 1000);
>  		DRM_DEBUG_ATOMIC("calculated bandwidth=%uk\n", bw);
>  
> -		threshold = kms->catalog->perf->max_bw_high;
> +		threshold = perf_cfg->max_bw_high;
>  
>  		DRM_DEBUG_ATOMIC("final threshold bw limit = %d\n", threshold);
>  
> @@ -212,7 +210,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>  	do_div(avg_bw, (kms->num_paths * 1000)); /*Bps_to_icc*/
>  
>  	for (i = 0; i < kms->num_paths; i++)
> -		icc_set_bw(kms->path[i], avg_bw, kms->catalog->perf->min_dram_ib);
> +		icc_set_bw(kms->path[i], avg_bw, kms->perf.perf_cfg->min_dram_ib);
>  
>  	return ret;
>  }
> @@ -293,10 +291,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  	}
>  
>  	kms = _dpu_crtc_get_kms(crtc);
> -	if (!kms->catalog) {
> -		DPU_ERROR("invalid kms\n");
> -		return -EINVAL;
> -	}
>  
>  	dpu_crtc = to_dpu_crtc(crtc);
>  	dpu_cstate = to_dpu_crtc_state(crtc->state);
> @@ -375,7 +369,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>  int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  {
>  	struct dpu_core_perf *perf = &dpu_kms->perf;
> -	const struct dpu_mdss_cfg *catalog = perf->catalog;
>  	struct dentry *entry;
>  
>  	entry = debugfs_create_dir("core_perf", parent);
> @@ -387,15 +380,15 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  	debugfs_create_u32("enable_bw_release", 0600, entry,
>  			(u32 *)&perf->enable_bw_release);
>  	debugfs_create_u32("threshold_low", 0600, entry,
> -			(u32 *)&catalog->perf->max_bw_low);
> +			(u32 *)&perf->perf_cfg->max_bw_low);
>  	debugfs_create_u32("threshold_high", 0600, entry,
> -			(u32 *)&catalog->perf->max_bw_high);
> +			(u32 *)&perf->perf_cfg->max_bw_high);
>  	debugfs_create_u32("min_core_ib", 0600, entry,
> -			(u32 *)&catalog->perf->min_core_ib);
> +			(u32 *)&perf->perf_cfg->min_core_ib);
>  	debugfs_create_u32("min_llcc_ib", 0600, entry,
> -			(u32 *)&catalog->perf->min_llcc_ib);
> +			(u32 *)&perf->perf_cfg->min_llcc_ib);
>  	debugfs_create_u32("min_dram_ib", 0600, entry,
> -			(u32 *)&catalog->perf->min_dram_ib);
> +			(u32 *)&perf->perf_cfg->min_dram_ib);
>  
>  	return 0;
>  }
> @@ -410,17 +403,16 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf)
>  
>  	perf->max_core_clk_rate = 0;
>  	perf->core_clk = NULL;
> -	perf->catalog = NULL;
>  	perf->dev = NULL;
>  }
>  
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
>  		struct drm_device *dev,
> -		const struct dpu_mdss_cfg *catalog,
> +		const struct dpu_perf_cfg *perf_cfg,
>  		struct clk *core_clk)
>  {
>  	perf->dev = dev;
> -	perf->catalog = catalog;
> +	perf->perf_cfg = perf_cfg;
>  	perf->core_clk = core_clk;
>  
>  	perf->max_core_clk_rate = clk_get_rate(core_clk);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index c29ec72984b8..e8a7916b6f71 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -28,7 +28,7 @@ struct dpu_core_perf_params {
>   * struct dpu_core_perf - definition of core performance context
>   * @dev: Pointer to drm device
>   * @debugfs_root: top level debug folder
> - * @catalog: Pointer to catalog configuration
> + * @perf_cfg: Platform-specific performance configuration
>   * @core_clk: Pointer to the core clock
>   * @core_clk_rate: current core clock rate
>   * @max_core_clk_rate: maximum allowable core clock rate
> @@ -38,7 +38,7 @@ struct dpu_core_perf_params {
>  struct dpu_core_perf {
>  	struct drm_device *dev;
>  	struct dentry *debugfs_root;
> -	const struct dpu_mdss_cfg *catalog;
> +	const struct dpu_perf_cfg *perf_cfg;
>  	struct clk *core_clk;
>  	u64 core_clk_rate;
>  	u64 max_core_clk_rate;
> @@ -79,12 +79,12 @@ void dpu_core_perf_destroy(struct dpu_core_perf *perf);
>   * dpu_core_perf_init - initialize the given core performance context
>   * @perf: Pointer to core performance context
>   * @dev: Pointer to drm device
> - * @catalog: Pointer to catalog
> + * @perf_cfg: Pointer to platform performance configuration
>   * @core_clk: pointer to core clock
>   */
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
>  		struct drm_device *dev,
> -		const struct dpu_mdss_cfg *catalog,
> +		const struct dpu_perf_cfg *perf_cfg,
>  		struct clk *core_clk);
>  
>  struct dpu_kms;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa8499de1b9f..6e62606e32de 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1115,7 +1115,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>  		dpu_kms->hw_vbif[vbif->id] = hw;
>  	}
>  
> -	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog,
> +	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog->perf,
>  			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
>  	if (rc) {
>  		DPU_ERROR("failed to init perf %d\n", rc);

Re: [PATCH 6/8] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

[Dmitry Baryshkov]

On 20/06/2023 13:55, Konrad Dybcio wrote:
> On 20.06.2023 02:08, Dmitry Baryshkov wrote:
>> Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
>> full-featured catalog data.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> Check below.
>
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 52 ++++++++-----------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
>>   3 files changed, 27 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> index 773e641eab28..78a7e3ea27a4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>> @@ -19,11 +19,11 @@
>>
>>   /**
>>    * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>> - * @kms:  pointer to the dpu_kms
>> + * @perf_cfg: performance configuration
>>    * @crtc: pointer to a crtc
>>    * Return: returns aggregated BW for all planes in crtc.
>>    */
>> -static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>> +static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
>>              struct drm_crtc *crtc)
>>   {
>>      struct drm_plane *plane;
>> @@ -39,7 +39,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>>              crtc_plane_bw += pstate->plane_fetch_bw;
>>      }
>>
>> -    bw_factor = kms->catalog->perf->bw_inefficiency_factor;
>> +    bw_factor = perf_cfg->bw_inefficiency_factor;
> It's set to 120 for all SoCs.. and it sounds very much like some kind of a
> hack.
>
> The 105 on the other inefficiency factor is easy to spot:
>
> (1024/1000)^2 = 1.048576 =~= 1.05 = 105%
>
> It comes from a MiB-MB-MHz conversion that Qcom splattered all over
> downstream as due to ancient tragical design decisions in msmbus
> (which leak to the downstream interconnect a bit):

This doesn't describe, why msm8226 and msm8974 had qcom,mdss-clk-factor
of 5/4. And 8084 got 1.05 as usual. I can only suppose that MDSS 1.0
(8974 v1) and 1.1 (8226) had some internal inefficiency / issues.

Also, this 1.05 is a clock inefficiency, so it should not be related
to msm bus client code.

>
> The logic needs to get some input that corresponds to a clock rate
> of a bus clock (19.2, 200, 300 Mhz etc.) but the APIs expect a Kbps
> value. So at one point they invented a MHZ_TO_MBPS macro which did this
> conversion the other way around and probably had to account for it.
>
> I think they tried to make it make more sense, but it ended up being
> even more spaghetti :/
>
> Not yet sure how it's done on RPMh icc, but with SMD RPM, passing e.g.
>
> opp-peak-kBps = <(200 * 8 * 1000)>; # 200 MHz * 8-wide * KHz-to-MHz
>
> results in a "correct" end rate.
>
> Konrad
>>      if (bw_factor) {
>>              crtc_plane_bw *= bw_factor;
>>              do_div(crtc_plane_bw, 100);


--
With best wishes
Dmitry

Re: [PATCH 6/8] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

[Konrad Dybcio]

On 20.06.2023 13:18, Dmitry Baryshkov wrote:
> On 20/06/2023 13:55, Konrad Dybcio wrote:
>> On 20.06.2023 02:08, Dmitry Baryshkov wrote:
>>> Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
>>> full-featured catalog data.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> Check below.
>>
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 52 ++++++++-----------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +--
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
>>>   3 files changed, 27 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index 773e641eab28..78a7e3ea27a4 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -19,11 +19,11 @@
>>>
>>>   /**
>>>    * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>>> - * @kms:  pointer to the dpu_kms
>>> + * @perf_cfg: performance configuration
>>>    * @crtc: pointer to a crtc
>>>    * Return: returns aggregated BW for all planes in crtc.
>>>    */
>>> -static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>>> +static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
>>>              struct drm_crtc *crtc)
>>>   {
>>>      struct drm_plane *plane;
>>> @@ -39,7 +39,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>>>              crtc_plane_bw += pstate->plane_fetch_bw;
>>>      }
>>>
>>> -    bw_factor = kms->catalog->perf->bw_inefficiency_factor;
>>> +    bw_factor = perf_cfg->bw_inefficiency_factor;
>> It's set to 120 for all SoCs.. and it sounds very much like some kind of a
>> hack.
>>
>> The 105 on the other inefficiency factor is easy to spot:
>>
>> (1024/1000)^2 = 1.048576 =~= 1.05 = 105%
>>
>> It comes from a MiB-MB-MHz conversion that Qcom splattered all over
>> downstream as due to ancient tragical design decisions in msmbus
>> (which leak to the downstream interconnect a bit):
> 
> This doesn't describe, why msm8226 and msm8974 had qcom,mdss-clk-factor
> of 5/4. And 8084 got 1.05 as usual. I can only suppose that MDSS 1.0
> (8974 v1) and 1.1 (8226) had some internal inefficiency / issues.
> 
> Also, this 1.05 is a clock inefficiency, so it should not be related
> to msm bus client code.
Right. Maybe Abhinav could shed some light on this.

Konrad
> 
>>
>> The logic needs to get some input that corresponds to a clock rate
>> of a bus clock (19.2, 200, 300 Mhz etc.) but the APIs expect a Kbps
>> value. So at one point they invented a MHZ_TO_MBPS macro which did this
>> conversion the other way around and probably had to account for it.
>>
>> I think they tried to make it make more sense, but it ended up being
>> even more spaghetti :/
>>
>> Not yet sure how it's done on RPMh icc, but with SMD RPM, passing e.g.
>>
>> opp-peak-kBps = <(200 * 8 * 1000)>; # 200 MHz * 8-wide * KHz-to-MHz
>>
>> results in a "correct" end rate.
>>
>> Konrad
>>>      if (bw_factor) {
>>>              crtc_plane_bw *= bw_factor;
>>>              do_div(crtc_plane_bw, 100);
> 
> 
> --
> With best wishes
> Dmitry

Re: [PATCH 6/8] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code

[Abhinav Kumar]

On 6/20/2023 4:31 AM, Konrad Dybcio wrote:
> On 20.06.2023 13:18, Dmitry Baryshkov wrote:
>> On 20/06/2023 13:55, Konrad Dybcio wrote:
>>> On 20.06.2023 02:08, Dmitry Baryshkov wrote:
>>>> Simplify dpu_core_perf code by using only dpu_perf_cfg instead of using
>>>> full-featured catalog data.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>
>>> Check below.
>>>
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 52 ++++++++-----------
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  8 +--
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  2 +-
>>>>    3 files changed, 27 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> index 773e641eab28..78a7e3ea27a4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>>> @@ -19,11 +19,11 @@
>>>>
>>>>    /**
>>>>     * _dpu_core_perf_calc_bw() - to calculate BW per crtc
>>>> - * @kms:  pointer to the dpu_kms
>>>> + * @perf_cfg: performance configuration
>>>>     * @crtc: pointer to a crtc
>>>>     * Return: returns aggregated BW for all planes in crtc.
>>>>     */
>>>> -static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>>>> +static u64 _dpu_core_perf_calc_bw(const struct dpu_perf_cfg *perf_cfg,
>>>>               struct drm_crtc *crtc)
>>>>    {
>>>>       struct drm_plane *plane;
>>>> @@ -39,7 +39,7 @@ static u64 _dpu_core_perf_calc_bw(struct dpu_kms *kms,
>>>>               crtc_plane_bw += pstate->plane_fetch_bw;
>>>>       }
>>>>
>>>> -    bw_factor = kms->catalog->perf->bw_inefficiency_factor;
>>>> +    bw_factor = perf_cfg->bw_inefficiency_factor;
>>> It's set to 120 for all SoCs.. and it sounds very much like some kind of a
>>> hack.
>>>
>>> The 105 on the other inefficiency factor is easy to spot:
>>>
>>> (1024/1000)^2 = 1.048576 =~= 1.05 = 105%
>>>
>>> It comes from a MiB-MB-MHz conversion that Qcom splattered all over
>>> downstream as due to ancient tragical design decisions in msmbus
>>> (which leak to the downstream interconnect a bit):
>>
>> This doesn't describe, why msm8226 and msm8974 had qcom,mdss-clk-factor
>> of 5/4. And 8084 got 1.05 as usual. I can only suppose that MDSS 1.0
>> (8974 v1) and 1.1 (8226) had some internal inefficiency / issues.
>>
>> Also, this 1.05 is a clock inefficiency, so it should not be related
>> to msm bus client code.
> Right. Maybe Abhinav could shed some light on this.
> 
> Konrad
>>

I will need to check with someone else about this as msm8974 and msm8226 
are quite old for me to remember.

That being said, I really dont think the explanation behind the number 
is going to be something which is going to be explained in detail here 
even if I did ask.

The name of the variable "clk_inefficiency_factor" says pretty much what 
has to be said for the purposes of this patch. I dont know if we will be 
able to go further into how that number came.

Coming to this patch itself, its not a major gain or major loss in my 
perspective.

Sure, we dont need to pass the full catalog today so we can just pass 
the perf_cfg. I cannot guarantee we wont need the full catalog later.


>>>
>>> The logic needs to get some input that corresponds to a clock rate
>>> of a bus clock (19.2, 200, 300 Mhz etc.) but the APIs expect a Kbps
>>> value. So at one point they invented a MHZ_TO_MBPS macro which did this
>>> conversion the other way around and probably had to account for it.
>>>
>>> I think they tried to make it make more sense, but it ended up being
>>> even more spaghetti :/
>>>
>>> Not yet sure how it's done on RPMh icc, but with SMD RPM, passing e.g.
>>>
>>> opp-peak-kBps = <(200 * 8 * 1000)>; # 200 MHz * 8-wide * KHz-to-MHz
>>>
>>> results in a "correct" end rate.
>>>
>>> Konrad
>>>>       if (bw_factor) {
>>>>               crtc_plane_bw *= bw_factor;
>>>>               do_div(crtc_plane_bw, 100);
>>
>>
>> --
>> With best wishes
>> Dmitry

[PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Dmitry Baryshkov]

This function does nothing, just clears several data pointers. Drop it
now.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
 3 files changed, 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index 78a7e3ea27a4..f779ad544347 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 }
 #endif
 
-void dpu_core_perf_destroy(struct dpu_core_perf *perf)
-{
-	if (!perf) {
-		DPU_ERROR("invalid parameters\n");
-		return;
-	}
-
-	perf->max_core_clk_rate = 0;
-	perf->core_clk = NULL;
-	perf->dev = NULL;
-}
-
 int dpu_core_perf_init(struct dpu_core_perf *perf,
 		struct drm_device *dev,
 		const struct dpu_perf_cfg *perf_cfg,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e8a7916b6f71..e1198c104b5e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
  */
 void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
 
-/**
- * dpu_core_perf_destroy - destroy the given core performance context
- * @perf: Pointer to core performance context
- */
-void dpu_core_perf_destroy(struct dpu_core_perf *perf);
-
 /**
  * dpu_core_perf_init - initialize the given core performance context
  * @perf: Pointer to core performance context
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 6e62606e32de..4439147d2c35 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 	return 0;
 
 drm_obj_init_err:
-	dpu_core_perf_destroy(&dpu_kms->perf);
 hw_intr_init_err:
 perf_err:
 power_error:
-- 
2.39.2

Re: [PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> This function does nothing, just clears several data pointers. Drop it
> now.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
>  3 files changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 78a7e3ea27a4..f779ad544347 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  }
>  #endif
>  
> -void dpu_core_perf_destroy(struct dpu_core_perf *perf)
> -{
> -	if (!perf) {
> -		DPU_ERROR("invalid parameters\n");
> -		return;
> -	}
> -
> -	perf->max_core_clk_rate = 0;
> -	perf->core_clk = NULL;
> -	perf->dev = NULL;
> -}
> -
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
>  		struct drm_device *dev,
>  		const struct dpu_perf_cfg *perf_cfg,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e8a7916b6f71..e1198c104b5e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>   */
>  void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
>  
> -/**
> - * dpu_core_perf_destroy - destroy the given core performance context
> - * @perf: Pointer to core performance context
> - */
> -void dpu_core_perf_destroy(struct dpu_core_perf *perf);
> -
>  /**
>   * dpu_core_perf_init - initialize the given core performance context
>   * @perf: Pointer to core performance context
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 6e62606e32de..4439147d2c35 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>  	return 0;
>  
>  drm_obj_init_err:
> -	dpu_core_perf_destroy(&dpu_kms->perf);
>  hw_intr_init_err:
>  perf_err:
>  power_error:
This screams for another cleanup ;)

Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> This function does nothing, just clears several data pointers. Drop it
> now.
> 

This will undo what dpu_core_perf_init() did when an error happens.

Why can we drop that?

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
>   3 files changed, 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index 78a7e3ea27a4..f779ad544347 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>   }
>   #endif
>   
> -void dpu_core_perf_destroy(struct dpu_core_perf *perf)
> -{
> -	if (!perf) {
> -		DPU_ERROR("invalid parameters\n");
> -		return;
> -	}
> -
> -	perf->max_core_clk_rate = 0;
> -	perf->core_clk = NULL;
> -	perf->dev = NULL;
> -}
> -
>   int dpu_core_perf_init(struct dpu_core_perf *perf,
>   		struct drm_device *dev,
>   		const struct dpu_perf_cfg *perf_cfg,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e8a7916b6f71..e1198c104b5e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>    */
>   void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
>   
> -/**
> - * dpu_core_perf_destroy - destroy the given core performance context
> - * @perf: Pointer to core performance context
> - */
> -void dpu_core_perf_destroy(struct dpu_core_perf *perf);
> -
>   /**
>    * dpu_core_perf_init - initialize the given core performance context
>    * @perf: Pointer to core performance context
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 6e62606e32de..4439147d2c35 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>   	return 0;
>   
>   drm_obj_init_err:
> -	dpu_core_perf_destroy(&dpu_kms->perf);
>   hw_intr_init_err:
>   perf_err:
>   power_error:

Re: [PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 01:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> > This function does nothing, just clears several data pointers. Drop it
> > now.
> >
>
> This will undo what dpu_core_perf_init() did when an error happens.
>
> Why can we drop that?

Because nothing will use this data in an error case. There is no need
to clean it.

>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
> >   3 files changed, 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > index 78a7e3ea27a4..f779ad544347 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> > @@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> >   }
> >   #endif
> >
> > -void dpu_core_perf_destroy(struct dpu_core_perf *perf)
> > -{
> > -     if (!perf) {
> > -             DPU_ERROR("invalid parameters\n");
> > -             return;
> > -     }
> > -
> > -     perf->max_core_clk_rate = 0;
> > -     perf->core_clk = NULL;
> > -     perf->dev = NULL;
> > -}
> > -
> >   int dpu_core_perf_init(struct dpu_core_perf *perf,
> >               struct drm_device *dev,
> >               const struct dpu_perf_cfg *perf_cfg,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > index e8a7916b6f71..e1198c104b5e 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> > @@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >    */
> >   void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
> >
> > -/**
> > - * dpu_core_perf_destroy - destroy the given core performance context
> > - * @perf: Pointer to core performance context
> > - */
> > -void dpu_core_perf_destroy(struct dpu_core_perf *perf);
> > -
> >   /**
> >    * dpu_core_perf_init - initialize the given core performance context
> >    * @perf: Pointer to core performance context
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 6e62606e32de..4439147d2c35 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> >       return 0;
> >
> >   drm_obj_init_err:
> > -     dpu_core_perf_destroy(&dpu_kms->perf);
> >   hw_intr_init_err:
> >   perf_err:
> >   power_error:



-- 
With best wishes
Dmitry

Re: [PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Abhinav Kumar]

On 7/3/2023 3:59 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Jul 2023 at 01:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>>
>>
>>
>> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
>>> This function does nothing, just clears several data pointers. Drop it
>>> now.
>>>
>>
>> This will undo what dpu_core_perf_init() did when an error happens.
>>
>> Why can we drop that?
> 
> Because nothing will use this data in an error case. There is no need
> to clean it.
> 

Usage is one thing for sure but I am still inclined to keep it symmetric 
with dpu_core_perf_init().

>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
>>>    3 files changed, 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> index 78a7e3ea27a4..f779ad544347 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
>>> @@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>>>    }
>>>    #endif
>>>
>>> -void dpu_core_perf_destroy(struct dpu_core_perf *perf)
>>> -{
>>> -     if (!perf) {
>>> -             DPU_ERROR("invalid parameters\n");
>>> -             return;
>>> -     }
>>> -
>>> -     perf->max_core_clk_rate = 0;
>>> -     perf->core_clk = NULL;
>>> -     perf->dev = NULL;
>>> -}
>>> -
>>>    int dpu_core_perf_init(struct dpu_core_perf *perf,
>>>                struct drm_device *dev,
>>>                const struct dpu_perf_cfg *perf_cfg,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> index e8a7916b6f71..e1198c104b5e 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
>>> @@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
>>>     */
>>>    void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
>>>
>>> -/**
>>> - * dpu_core_perf_destroy - destroy the given core performance context
>>> - * @perf: Pointer to core performance context
>>> - */
>>> -void dpu_core_perf_destroy(struct dpu_core_perf *perf);
>>> -
>>>    /**
>>>     * dpu_core_perf_init - initialize the given core performance context
>>>     * @perf: Pointer to core performance context
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 6e62606e32de..4439147d2c35 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>>>        return 0;
>>>
>>>    drm_obj_init_err:
>>> -     dpu_core_perf_destroy(&dpu_kms->perf);
>>>    hw_intr_init_err:
>>>    perf_err:
>>>    power_error:
> 
> 
> 

Re: [PATCH 7/8] drm/msm/dpu: drop dpu_core_perf_destroy()

[Dmitry Baryshkov]

On Tue, 4 Jul 2023 at 03:19, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
>
>
>
> On 7/3/2023 3:59 PM, Dmitry Baryshkov wrote:
> > On Tue, 4 Jul 2023 at 01:57, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> >>> This function does nothing, just clears several data pointers. Drop it
> >>> now.
> >>>
> >>
> >> This will undo what dpu_core_perf_init() did when an error happens.
> >>
> >> Why can we drop that?
> >
> > Because nothing will use this data in an error case. There is no need
> > to clean it.
> >
>
> Usage is one thing for sure but I am still inclined to keep it symmetric
> with dpu_core_perf_init().

Through the last several years the kernel was more and more inclined
to asymmetry here. For example, consider all the demv_ functions. We
have been removing cleanup and _remove functions all over the place.
So, I think, I will reiterate this patch.

>
> >>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 12 ------------
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  6 ------
> >>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  1 -
> >>>    3 files changed, 19 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> index 78a7e3ea27a4..f779ad544347 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> >>> @@ -394,18 +394,6 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
> >>>    }
> >>>    #endif
> >>>
> >>> -void dpu_core_perf_destroy(struct dpu_core_perf *perf)
> >>> -{
> >>> -     if (!perf) {
> >>> -             DPU_ERROR("invalid parameters\n");
> >>> -             return;
> >>> -     }
> >>> -
> >>> -     perf->max_core_clk_rate = 0;
> >>> -     perf->core_clk = NULL;
> >>> -     perf->dev = NULL;
> >>> -}
> >>> -
> >>>    int dpu_core_perf_init(struct dpu_core_perf *perf,
> >>>                struct drm_device *dev,
> >>>                const struct dpu_perf_cfg *perf_cfg,
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> index e8a7916b6f71..e1198c104b5e 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> >>> @@ -69,12 +69,6 @@ int dpu_core_perf_crtc_update(struct drm_crtc *crtc,
> >>>     */
> >>>    void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
> >>>
> >>> -/**
> >>> - * dpu_core_perf_destroy - destroy the given core performance context
> >>> - * @perf: Pointer to core performance context
> >>> - */
> >>> -void dpu_core_perf_destroy(struct dpu_core_perf *perf);
> >>> -
> >>>    /**
> >>>     * dpu_core_perf_init - initialize the given core performance context
> >>>     * @perf: Pointer to core performance context
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>> index 6e62606e32de..4439147d2c35 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >>> @@ -1162,7 +1162,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
> >>>        return 0;
> >>>
> >>>    drm_obj_init_err:
> >>> -     dpu_core_perf_destroy(&dpu_kms->perf);
> >>>    hw_intr_init_err:
> >>>    perf_err:
> >>>    power_error:
> >
> >
> >



-- 
With best wishes
Dmitry

[PATCH 8/8] drm/msm/dpu: remove unused fields from struct dpu_core_perf

[Dmitry Baryshkov]

Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
are not used by the code.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 ----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 2 +-
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
index f779ad544347..7f110d15b101 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
@@ -395,11 +395,9 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
 #endif
 
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-		struct drm_device *dev,
 		const struct dpu_perf_cfg *perf_cfg,
 		struct clk *core_clk)
 {
-	perf->dev = dev;
 	perf->perf_cfg = perf_cfg;
 	perf->core_clk = core_clk;
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
index e1198c104b5e..623e2d058695 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
@@ -27,7 +27,6 @@ struct dpu_core_perf_params {
 /**
  * struct dpu_core_perf - definition of core performance context
  * @dev: Pointer to drm device
- * @debugfs_root: top level debug folder
  * @perf_cfg: Platform-specific performance configuration
  * @core_clk: Pointer to the core clock
  * @core_clk_rate: current core clock rate
@@ -36,8 +35,6 @@ struct dpu_core_perf_params {
  * @enable_bw_release: debug control for bandwidth release
  */
 struct dpu_core_perf {
-	struct drm_device *dev;
-	struct dentry *debugfs_root;
 	const struct dpu_perf_cfg *perf_cfg;
 	struct clk *core_clk;
 	u64 core_clk_rate;
@@ -77,7 +74,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
  * @core_clk: pointer to core clock
  */
 int dpu_core_perf_init(struct dpu_core_perf *perf,
-		struct drm_device *dev,
 		const struct dpu_perf_cfg *perf_cfg,
 		struct clk *core_clk);
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 4439147d2c35..5297cec68c9c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -1115,7 +1115,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
 		dpu_kms->hw_vbif[vbif->id] = hw;
 	}
 
-	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog->perf,
+	rc = dpu_core_perf_init(&dpu_kms->perf, dpu_kms->catalog->perf,
 			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
 	if (rc) {
 		DPU_ERROR("failed to init perf %d\n", rc);
-- 
2.39.2

Re: [PATCH 8/8] drm/msm/dpu: remove unused fields from struct dpu_core_perf

[Konrad Dybcio]

On 20.06.2023 02:08, Dmitry Baryshkov wrote:
> Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
> are not used by the code.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 ----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 2 +-
>  3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index f779ad544347..7f110d15b101 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -395,11 +395,9 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>  #endif
>  
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
> -		struct drm_device *dev,
>  		const struct dpu_perf_cfg *perf_cfg,
>  		struct clk *core_clk)
>  {
> -	perf->dev = dev;
>  	perf->perf_cfg = perf_cfg;
>  	perf->core_clk = core_clk;
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e1198c104b5e..623e2d058695 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -27,7 +27,6 @@ struct dpu_core_perf_params {
>  /**
>   * struct dpu_core_perf - definition of core performance context
>   * @dev: Pointer to drm device
> - * @debugfs_root: top level debug folder
>   * @perf_cfg: Platform-specific performance configuration
>   * @core_clk: Pointer to the core clock
>   * @core_clk_rate: current core clock rate
> @@ -36,8 +35,6 @@ struct dpu_core_perf_params {
>   * @enable_bw_release: debug control for bandwidth release
>   */
>  struct dpu_core_perf {
> -	struct drm_device *dev;
> -	struct dentry *debugfs_root;
>  	const struct dpu_perf_cfg *perf_cfg;
>  	struct clk *core_clk;
>  	u64 core_clk_rate;
> @@ -77,7 +74,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
>   * @core_clk: pointer to core clock
>   */
>  int dpu_core_perf_init(struct dpu_core_perf *perf,
> -		struct drm_device *dev,
>  		const struct dpu_perf_cfg *perf_cfg,
>  		struct clk *core_clk);
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 4439147d2c35..5297cec68c9c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1115,7 +1115,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>  		dpu_kms->hw_vbif[vbif->id] = hw;
>  	}
>  
> -	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog->perf,
> +	rc = dpu_core_perf_init(&dpu_kms->perf, dpu_kms->catalog->perf,
>  			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
>  	if (rc) {
>  		DPU_ERROR("failed to init perf %d\n", rc);

Re: [PATCH 8/8] drm/msm/dpu: remove unused fields from struct dpu_core_perf

[Abhinav Kumar]

On 6/19/2023 5:08 PM, Dmitry Baryshkov wrote:
> Remove dpu_core_perf::dev and dpu_core_perf::debugfs_root fields, they
> are not used by the code.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 2 --
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h | 4 ----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       | 2 +-
>   3 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> index f779ad544347..7f110d15b101 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -395,11 +395,9 @@ int dpu_core_perf_debugfs_init(struct dpu_kms *dpu_kms, struct dentry *parent)
>   #endif
>   
>   int dpu_core_perf_init(struct dpu_core_perf *perf,
> -		struct drm_device *dev,
>   		const struct dpu_perf_cfg *perf_cfg,
>   		struct clk *core_clk)
>   {
> -	perf->dev = dev;
>   	perf->perf_cfg = perf_cfg;
>   	perf->core_clk = core_clk;
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> index e1198c104b5e..623e2d058695 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h
> @@ -27,7 +27,6 @@ struct dpu_core_perf_params {
>   /**
>    * struct dpu_core_perf - definition of core performance context
>    * @dev: Pointer to drm device

remove dev from here too

> - * @debugfs_root: top level debug folder
>    * @perf_cfg: Platform-specific performance configuration
>    * @core_clk: Pointer to the core clock
>    * @core_clk_rate: current core clock rate
> @@ -36,8 +35,6 @@ struct dpu_core_perf_params {
>    * @enable_bw_release: debug control for bandwidth release
>    */
>   struct dpu_core_perf {
> -	struct drm_device *dev;
> -	struct dentry *debugfs_root;
>   	const struct dpu_perf_cfg *perf_cfg;
>   	struct clk *core_clk;
>   	u64 core_clk_rate;
> @@ -77,7 +74,6 @@ void dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc);
>    * @core_clk: pointer to core clock
>    */
>   int dpu_core_perf_init(struct dpu_core_perf *perf,
> -		struct drm_device *dev,
>   		const struct dpu_perf_cfg *perf_cfg,
>   		struct clk *core_clk);
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 4439147d2c35..5297cec68c9c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -1115,7 +1115,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
>   		dpu_kms->hw_vbif[vbif->id] = hw;
>   	}
>   
> -	rc = dpu_core_perf_init(&dpu_kms->perf, dev, dpu_kms->catalog->perf,
> +	rc = dpu_core_perf_init(&dpu_kms->perf, dpu_kms->catalog->perf,
>   			msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks, "core"));
>   	if (rc) {
>   		DPU_ERROR("failed to init perf %d\n", rc);