Re: [PATCH v3 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides

From: Abhinav Kumar <quic_abhinavk@quicinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
	Marijn Suijten <marijn.suijten@somainline.org>
Cc: Stephen Boyd <swboyd@chromium.org>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Andersson <andersson@kernel.org>,
	<linux-arm-msm@vger.kernel.org>,
	<dri-devel@lists.freedesktop.org>,
	<freedreno@lists.freedesktop.org>
Subject: Re: [PATCH v3 04/11] drm/msm/dpu: drop separate dpu_core_perf_tune overrides
Date: Mon, 10 Jul 2023 19:31:49 -0700	[thread overview]
Message-ID: <729de13d-6fb7-ff1c-8660-4710d914258b@quicinc.com> (raw)
In-Reply-To: <20230707193942.3806526-5-dmitry.baryshkov@linaro.org>

On 7/7/2023 12:39 PM, Dmitry Baryshkov wrote:
> The values in struct dpu_core_perf_tune are fixed per the core perf
> mode. Drop the 'tune' values and substitute them with known values when
> performing perf management.
> 
> Note: min_bus_vote was not used at all, so it is just silently dropped.
> 

Interesting ..... should bring this back properly. Will take it up.

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c | 29 ++++++++-----------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.h |  4 ---
>   2 files changed, 12 insertions(+), 21 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 05d340aa18c5..348550ac7e51 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
> @@ -235,7 +235,7 @@ static int _dpu_core_perf_crtc_update_bus(struct dpu_kms *kms,
>   {
>   	struct dpu_core_perf_params perf = { 0 };
>   	int i, ret = 0;
> -	u64 avg_bw;
> +	u32 avg_bw;
>   

avg_bw seems unused in this patch, so unrelated change?

>   	if (!kms->num_paths)
>   		return 0;
> @@ -291,10 +291,16 @@ 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;
>   	struct drm_crtc *crtc;
>   	struct dpu_crtc_state *dpu_cstate;
>   
> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
> +		return kms->perf.fix_core_clk_rate;
> +
> +	if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
> +		return kms->perf.max_core_clk_rate;
> +
>   	drm_for_each_crtc(crtc, kms->dev) {
>   		if (crtc->enabled) {
>   			dpu_cstate = to_dpu_crtc_state(crtc->state);
> @@ -305,11 +311,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);
> -

Why dont you move both FIXED and MINIMUM handling below instead of above.

So that they will just override the clk_rate and you can keep this 
useful log here and it matches where the function is.

This chunk looks better that way.

<skipping the rest as it LGTM>