[PATCH] drm/msm/dsi: simplify pixel clk rate handling

[PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Dmitry Baryshkov]

Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.

Also, while we are at it, replace another dsi_get_pclk_rate() invocation
with using the stored value at msm_host->pixel_clk_rate.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
 drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
 drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index bd3763a5d723..93ec54478eb6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *iova);
 int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
 int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
 int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
-int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
-int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
+int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
+int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
 void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
 void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
 struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
index 44be4a88aa83..5106e66846c3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
+++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
@@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
 	void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
 	void (*tx_buf_put)(struct msm_dsi_host *msm_host);
 	int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t *iova);
-	int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
+	int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
 };
 
 struct msm_dsi_cfg_handler {
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 18fa30e1e858..7d99a108bff6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 }
 
-int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
+int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
 {
-	if (!msm_host->mode) {
-		pr_err("%s: mode not set\n", __func__);
-		return -EINVAL;
-	}
-
-	dsi_calc_pclk(msm_host, is_bonded_dsi);
 	msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
+
 	return 0;
 }
 
-int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
+int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
 {
 	u32 bpp = dsi_get_bpp(msm_host->format);
 	u64 pclk_bpp;
 	unsigned int esc_mhz, esc_div;
 	unsigned long byte_mhz;
 
-	dsi_calc_pclk(msm_host, is_bonded_dsi);
-
-	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
+	pclk_bpp = msm_host->pixel_clk_rate * bpp;
 	do_div(pclk_bpp, 8);
 	msm_host->src_clk_rate = pclk_bpp;
 
@@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
 	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
 	int ret;
 
-	ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
+	if (!msm_host->mode) {
+		pr_err("%s: mode not set\n", __func__);
+		return;
+	}
+
+	dsi_calc_pclk(msm_host, is_bonded_dsi);
+
+	ret = cfg_hnd->ops->calc_clk_rate(msm_host);
 	if (ret) {
 		pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
 		return;
-- 
2.39.0

Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Abhinav Kumar]

On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
> 
> Also, while we are at it, replace another dsi_get_pclk_rate() invocation
> with using the stored value at msm_host->pixel_clk_rate.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>   3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index bd3763a5d723..93ec54478eb6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *iova);
>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index 44be4a88aa83..5106e66846c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>   	void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>   	void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>   	int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t *iova);
> -	int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> +	int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>   };
>   
>   struct msm_dsi_cfg_handler {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 18fa30e1e858..7d99a108bff6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   }
>   
> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>   {
> -	if (!msm_host->mode) {
> -		pr_err("%s: mode not set\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	dsi_calc_pclk(msm_host, is_bonded_dsi);
>   	msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
> +
>   	return 0;
>   }
>   
> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>   {
>   	u32 bpp = dsi_get_bpp(msm_host->format);
>   	u64 pclk_bpp;
>   	unsigned int esc_mhz, esc_div;
>   	unsigned long byte_mhz;
>   
> -	dsi_calc_pclk(msm_host, is_bonded_dsi);
> -
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> +	pclk_bpp = msm_host->pixel_clk_rate * bpp;
>   	do_div(pclk_bpp, 8);
>   	msm_host->src_clk_rate = pclk_bpp;
>   
> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>   	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>   	int ret;
>   
> -	ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
> +	if (!msm_host->mode) {
> +		pr_err("%s: mode not set\n", __func__);
> +		return;
> +	}
> +
> +	dsi_calc_pclk(msm_host, is_bonded_dsi);
> +
> +	ret = cfg_hnd->ops->calc_clk_rate(msm_host);

I am not too sure what we are gaining by this.

Its not that we are replacing dsi_get_pclk_rate().

We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to the 
msm_dsi_host_get_phy_clk_req().

Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty to 
stand on its own.

The original intention of the calc_clk_rate() op seems to be calculate 
and store all the clocks (byte, pixel and esc).

Why change that behavior by breaking it up?

>   	if (ret) {
>   		pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>   		return;

Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Dmitry Baryshkov]

On 26/01/2023 02:07, Abhinav Kumar wrote:
> 
> 
> On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
>> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
>> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
>>
>> Also, while we are at it, replace another dsi_get_pclk_rate() invocation
>> with using the stored value at msm_host->pixel_clk_rate.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>>   3 files changed, 15 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.h
>> index bd3763a5d723..93ec54478eb6 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host 
>> *msm_host, uint64_t *iova);
>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi);
>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi);
>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct 
>> mipi_dsi_host *host);
>>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct 
>> mipi_dsi_host *host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> index 44be4a88aa83..5106e66846c3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>>       void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>>       void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>>       int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t *iova);
>> -    int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi);
>> +    int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>>   };
>>   struct msm_dsi_cfg_handler {
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 18fa30e1e858..7d99a108bff6 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>   }
>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>>   {
>> -    if (!msm_host->mode) {
>> -        pr_err("%s: mode not set\n", __func__);
>> -        return -EINVAL;
>> -    }
>> -
>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>       msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>> +
>>       return 0;
>>   }
>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>>   {
>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>       u64 pclk_bpp;
>>       unsigned int esc_mhz, esc_div;
>>       unsigned long byte_mhz;
>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>> -
>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
>> * bpp;
>> +    pclk_bpp = msm_host->pixel_clk_rate * bpp;
>>       do_div(pclk_bpp, 8);
>>       msm_host->src_clk_rate = pclk_bpp;
>> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct 
>> mipi_dsi_host *host,
>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>>       int ret;
>> -    ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
>> +    if (!msm_host->mode) {
>> +        pr_err("%s: mode not set\n", __func__);
>> +        return;
>> +    }
>> +
>> +    dsi_calc_pclk(msm_host, is_bonded_dsi);
>> +
>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
> 
> I am not too sure what we are gaining by this.
> 
> Its not that we are replacing dsi_get_pclk_rate().
> 
> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to the 
> msm_dsi_host_get_phy_clk_req().
> 
> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty to 
> stand on its own.
> 
> The original intention of the calc_clk_rate() op seems to be calculate 
> and store all the clocks (byte, pixel and esc).
> 
> Why change that behavior by breaking it up?

Unification between platforms. Both v2 and 6g platforms call 
dsi_calc_pclk(). Let's just move it to a common code path.

> 
>>       if (ret) {
>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>>           return;

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Jessica Zhang]

On 3/28/2023 6:04 AM, Dmitry Baryshkov wrote:
> On 26/01/2023 02:07, Abhinav Kumar wrote:
>>
>>
>> On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
>>> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
>>> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
>>>
>>> Also, while we are at it, replace another dsi_get_pclk_rate() invocation
>>> with using the stored value at msm_host->pixel_clk_rate.
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>>>   3 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>> index bd3763a5d723..93ec54478eb6 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host 
>>> *msm_host, uint64_t *iova);
>>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t 
>>> *iova);
>>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi);
>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi);
>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, 
>>> struct mipi_dsi_host *host);
>>>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct 
>>> mipi_dsi_host *host);
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
>>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>> index 44be4a88aa83..5106e66846c3 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>>>       void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>>>       void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>>>       int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t 
>>> *iova);
>>> -    int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi);
>>> +    int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>>>   };
>>>   struct msm_dsi_cfg_handler {
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 18fa30e1e858..7d99a108bff6 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>   }
>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi)
>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>>>   {
>>> -    if (!msm_host->mode) {
>>> -        pr_err("%s: mode not set\n", __func__);
>>> -        return -EINVAL;
>>> -    }
>>> -
>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>       msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>> +
>>>       return 0;
>>>   }
>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi)
>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>>>   {
>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>>       u64 pclk_bpp;
>>>       unsigned int esc_mhz, esc_div;
>>>       unsigned long byte_mhz;
>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>> -
>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
>>> * bpp;
>>> +    pclk_bpp = msm_host->pixel_clk_rate * bpp;
>>>       do_div(pclk_bpp, 8);
>>>       msm_host->src_clk_rate = pclk_bpp;
>>> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct 
>>> mipi_dsi_host *host,
>>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>>>       int ret;
>>> -    ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
>>> +    if (!msm_host->mode) {
>>> +        pr_err("%s: mode not set\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>> +
>>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
>>
>> I am not too sure what we are gaining by this.
>>
>> Its not that we are replacing dsi_get_pclk_rate().
>>
>> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to the 
>> msm_dsi_host_get_phy_clk_req().
>>
>> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty to 
>> stand on its own.
>>
>> The original intention of the calc_clk_rate() op seems to be calculate 
>> and store all the clocks (byte, pixel and esc).
>>
>> Why change that behavior by breaking it up?
> 
> Unification between platforms. Both v2 and 6g platforms call 
> dsi_calc_pclk(). Let's just move it to a common code path.

Hi Dmitry,

I think what Abhinav means here is that the meaning and functionality of 
calc_clk_rate() changes with this patch.

Before, calc_clk_rate() does *all* the clk_rate calculations and 
assignments. But after this change, it will only calculate and assign 
the escape clk rate.

I agree with Abhinav that this change renders the calc_clk_rate() op 
misleading as it will not calculate all of the clock rates anymore.

Thanks,

Jessica Zhang

> 
>>
>>>       if (ret) {
>>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>>>           return;
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Dmitry Baryshkov]

On 19/05/2023 21:54, Jessica Zhang wrote:
> 
> 
> On 3/28/2023 6:04 AM, Dmitry Baryshkov wrote:
>> On 26/01/2023 02:07, Abhinav Kumar wrote:
>>>
>>>
>>> On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
>>>> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
>>>> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
>>>>
>>>> Also, while we are at it, replace another dsi_get_pclk_rate() 
>>>> invocation
>>>> with using the stored value at msm_host->pixel_clk_rate.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>>>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>>>>   3 files changed, 15 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> index bd3763a5d723..93ec54478eb6 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host 
>>>> *msm_host, uint64_t *iova);
>>>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t 
>>>> *iova);
>>>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi);
>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi);
>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>>>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, 
>>>> struct mipi_dsi_host *host);
>>>>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>>>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct 
>>>> mipi_dsi_host *host);
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>> index 44be4a88aa83..5106e66846c3 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>>>>       void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>>>>       void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>>>>       int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t 
>>>> *iova);
>>>> -    int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi);
>>>> +    int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>>>>   };
>>>>   struct msm_dsi_cfg_handler {
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 18fa30e1e858..7d99a108bff6 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>>>> *msm_host, bool is_bonded_dsi)
>>>>   }
>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi)
>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>>>>   {
>>>> -    if (!msm_host->mode) {
>>>> -        pr_err("%s: mode not set\n", __func__);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>       msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>> +
>>>>       return 0;
>>>>   }
>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi)
>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>>>>   {
>>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>>>       u64 pclk_bpp;
>>>>       unsigned int esc_mhz, esc_div;
>>>>       unsigned long byte_mhz;
>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>> -
>>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, 
>>>> is_bonded_dsi) * bpp;
>>>> +    pclk_bpp = msm_host->pixel_clk_rate * bpp;
>>>>       do_div(pclk_bpp, 8);
>>>>       msm_host->src_clk_rate = pclk_bpp;
>>>> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct 
>>>> mipi_dsi_host *host,
>>>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>>>>       int ret;
>>>> -    ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
>>>> +    if (!msm_host->mode) {
>>>> +        pr_err("%s: mode not set\n", __func__);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>> +
>>>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
>>>
>>> I am not too sure what we are gaining by this.
>>>
>>> Its not that we are replacing dsi_get_pclk_rate().
>>>
>>> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to the 
>>> msm_dsi_host_get_phy_clk_req().
>>>
>>> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty to 
>>> stand on its own.
>>>
>>> The original intention of the calc_clk_rate() op seems to be 
>>> calculate and store all the clocks (byte, pixel and esc).
>>>
>>> Why change that behavior by breaking it up?
>>
>> Unification between platforms. Both v2 and 6g platforms call 
>> dsi_calc_pclk(). Let's just move it to a common code path.
> 
> Hi Dmitry,
> 
> I think what Abhinav means here is that the meaning and functionality of 
> calc_clk_rate() changes with this patch.
> 
> Before, calc_clk_rate() does *all* the clk_rate calculations and 
> assignments. But after this change, it will only calculate and assign 
> the escape clk rate.
> 
> I agree with Abhinav that this change renders the calc_clk_rate() op 
> misleading as it will not calculate all of the clock rates anymore.

Would it make sense if I rename it to calc_other_clock_rates()?

Moving pclk calculation to the core code emphasises that pclk 
calculation is common between v2 and 6g hosts.

> 
> Thanks,
> 
> Jessica Zhang
> 
>>
>>>
>>>>       if (ret) {
>>>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>>>>           return;
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Abhinav Kumar]

On 5/19/2023 12:33 PM, Dmitry Baryshkov wrote:
> On 19/05/2023 21:54, Jessica Zhang wrote:
>>
>>
>> On 3/28/2023 6:04 AM, Dmitry Baryshkov wrote:
>>> On 26/01/2023 02:07, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
>>>>> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
>>>>> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
>>>>>
>>>>> Also, while we are at it, replace another dsi_get_pclk_rate() 
>>>>> invocation
>>>>> with using the stored value at msm_host->pixel_clk_rate.
>>>>>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>>>>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>>>>>   3 files changed, 15 insertions(+), 15 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> index bd3763a5d723..93ec54478eb6 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host 
>>>>> *msm_host, uint64_t *iova);
>>>>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t 
>>>>> *iova);
>>>>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>>>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>>> is_bonded_dsi);
>>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>>> is_bonded_dsi);
>>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
>>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>>>>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, 
>>>>> struct mipi_dsi_host *host);
>>>>>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>>>>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct 
>>>>> mipi_dsi_host *host);
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>> index 44be4a88aa83..5106e66846c3 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>>>>>       void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>>>>>       void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>>>>>       int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t 
>>>>> *iova);
>>>>> -    int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool 
>>>>> is_bonded_dsi);
>>>>> +    int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>>>>>   };
>>>>>   struct msm_dsi_cfg_handler {
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> index 18fa30e1e858..7d99a108bff6 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>>>>> *msm_host, bool is_bonded_dsi)
>>>>>   }
>>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>>> is_bonded_dsi)
>>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>>>>>   {
>>>>> -    if (!msm_host->mode) {
>>>>> -        pr_err("%s: mode not set\n", __func__);
>>>>> -        return -EINVAL;
>>>>> -    }
>>>>> -
>>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>>       msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>>> +
>>>>>       return 0;
>>>>>   }
>>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>>> is_bonded_dsi)
>>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>>>>>   {
>>>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>>>>       u64 pclk_bpp;
>>>>>       unsigned int esc_mhz, esc_div;
>>>>>       unsigned long byte_mhz;
>>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>> -
>>>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, 
>>>>> is_bonded_dsi) * bpp;
>>>>> +    pclk_bpp = msm_host->pixel_clk_rate * bpp;
>>>>>       do_div(pclk_bpp, 8);
>>>>>       msm_host->src_clk_rate = pclk_bpp;
>>>>> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct 
>>>>> mipi_dsi_host *host,
>>>>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>>>>>       int ret;
>>>>> -    ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
>>>>> +    if (!msm_host->mode) {
>>>>> +        pr_err("%s: mode not set\n", __func__);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>> +
>>>>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
>>>>
>>>> I am not too sure what we are gaining by this.
>>>>
>>>> Its not that we are replacing dsi_get_pclk_rate().
>>>>
>>>> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to 
>>>> the msm_dsi_host_get_phy_clk_req().
>>>>
>>>> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty 
>>>> to stand on its own.
>>>>
>>>> The original intention of the calc_clk_rate() op seems to be 
>>>> calculate and store all the clocks (byte, pixel and esc).
>>>>
>>>> Why change that behavior by breaking it up?
>>>
>>> Unification between platforms. Both v2 and 6g platforms call 
>>> dsi_calc_pclk(). Let's just move it to a common code path.
>>
>> Hi Dmitry,
>>
>> I think what Abhinav means here is that the meaning and functionality 
>> of calc_clk_rate() changes with this patch.
>>
>> Before, calc_clk_rate() does *all* the clk_rate calculations and 
>> assignments. But after this change, it will only calculate and assign 
>> the escape clk rate.
>>
>> I agree with Abhinav that this change renders the calc_clk_rate() op 
>> misleading as it will not calculate all of the clock rates anymore.
> 
> Would it make sense if I rename it to calc_other_clock_rates()?
> 

Not really. I would rather still have it separate and drop this patch.

So even if pixel clock calculation looks common today between v2 and 6g, 
lets say tomorrow there is a 7g or 8g which needs some other math there, 
I think this is the right place where it should stay so that we 
calculate all clocks together.

> Moving pclk calculation to the core code emphasises that pclk 
> calculation is common between v2 and 6g hosts.
> 
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>
>>>>>       if (ret) {
>>>>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>>>>>           return;
>>>
>>> -- 
>>> With best wishes
>>> Dmitry
>>>
> 

Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Dmitry Baryshkov]

On 19/05/2023 22:36, Abhinav Kumar wrote:
> 
> 
> On 5/19/2023 12:33 PM, Dmitry Baryshkov wrote:
>> On 19/05/2023 21:54, Jessica Zhang wrote:
>>>
>>>
>>> On 3/28/2023 6:04 AM, Dmitry Baryshkov wrote:
>>>> On 26/01/2023 02:07, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 1/18/2023 5:00 AM, Dmitry Baryshkov wrote:
>>>>>> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
>>>>>> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 
>>>>>> hosts.
>>>>>>
>>>>>> Also, while we are at it, replace another dsi_get_pclk_rate() 
>>>>>> invocation
>>>>>> with using the stored value at msm_host->pixel_clk_rate.
>>>>>>
>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>>>>>>   drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>>>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>>>>>>   3 files changed, 15 insertions(+), 15 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>>> index bd3763a5d723..93ec54478eb6 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>>>> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host 
>>>>>> *msm_host, uint64_t *iova);
>>>>>>   int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t 
>>>>>> *iova);
>>>>>>   int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>>>>>>   int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
>>>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>>>> is_bonded_dsi);
>>>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>>>> is_bonded_dsi);
>>>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
>>>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>>>>>>   void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, 
>>>>>> struct mipi_dsi_host *host);
>>>>>>   void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>>>>>>   struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct 
>>>>>> mipi_dsi_host *host);
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h 
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>>> index 44be4a88aa83..5106e66846c3 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
>>>>>> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>>>>>>       void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>>>>>>       void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>>>>>>       int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t 
>>>>>> *iova);
>>>>>> -    int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool 
>>>>>> is_bonded_dsi);
>>>>>> +    int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>>>>>>   };
>>>>>>   struct msm_dsi_cfg_handler {
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> index 18fa30e1e858..7d99a108bff6 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>>>> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct 
>>>>>> msm_dsi_host *msm_host, bool is_bonded_dsi)
>>>>>>   }
>>>>>> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool 
>>>>>> is_bonded_dsi)
>>>>>> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>>>>>>   {
>>>>>> -    if (!msm_host->mode) {
>>>>>> -        pr_err("%s: mode not set\n", __func__);
>>>>>> -        return -EINVAL;
>>>>>> -    }
>>>>>> -
>>>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>>>       msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
>>>>>> +
>>>>>>       return 0;
>>>>>>   }
>>>>>> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool 
>>>>>> is_bonded_dsi)
>>>>>> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>>>>>>   {
>>>>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>>>>>       u64 pclk_bpp;
>>>>>>       unsigned int esc_mhz, esc_div;
>>>>>>       unsigned long byte_mhz;
>>>>>> -    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>>> -
>>>>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, 
>>>>>> is_bonded_dsi) * bpp;
>>>>>> +    pclk_bpp = msm_host->pixel_clk_rate * bpp;
>>>>>>       do_div(pclk_bpp, 8);
>>>>>>       msm_host->src_clk_rate = pclk_bpp;
>>>>>> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct 
>>>>>> mipi_dsi_host *host,
>>>>>>       const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>>>>>>       int ret;
>>>>>> -    ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
>>>>>> +    if (!msm_host->mode) {
>>>>>> +        pr_err("%s: mode not set\n", __func__);
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>>>> +
>>>>>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
>>>>>
>>>>> I am not too sure what we are gaining by this.
>>>>>
>>>>> Its not that we are replacing dsi_get_pclk_rate().
>>>>>
>>>>> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to 
>>>>> the msm_dsi_host_get_phy_clk_req().
>>>>>
>>>>> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty 
>>>>> to stand on its own.
>>>>>
>>>>> The original intention of the calc_clk_rate() op seems to be 
>>>>> calculate and store all the clocks (byte, pixel and esc).
>>>>>
>>>>> Why change that behavior by breaking it up?
>>>>
>>>> Unification between platforms. Both v2 and 6g platforms call 
>>>> dsi_calc_pclk(). Let's just move it to a common code path.
>>>
>>> Hi Dmitry,
>>>
>>> I think what Abhinav means here is that the meaning and functionality 
>>> of calc_clk_rate() changes with this patch.
>>>
>>> Before, calc_clk_rate() does *all* the clk_rate calculations and 
>>> assignments. But after this change, it will only calculate and assign 
>>> the escape clk rate.
>>>
>>> I agree with Abhinav that this change renders the calc_clk_rate() op 
>>> misleading as it will not calculate all of the clock rates anymore.
>>
>> Would it make sense if I rename it to calc_other_clock_rates()?
>>
> 
> Not really. I would rather still have it separate and drop this patch.
> 
> So even if pixel clock calculation looks common today between v2 and 6g, 
> lets say tomorrow there is a 7g or 8g which needs some other math there, 
> I think this is the right place where it should stay so that we 
> calculate all clocks together.

Ack.

> 
>> Moving pclk calculation to the core code emphasises that pclk 
>> calculation is common between v2 and 6g hosts.
>>
>>>
>>> Thanks,
>>>
>>> Jessica Zhang
>>>
>>>>
>>>>>
>>>>>>       if (ret) {
>>>>>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>>>>>>           return;
>>>>
>>>> -- 
>>>> With best wishes
>>>> Dmitry
>>>>
>>

-- 
With best wishes
Dmitry

Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Marijn Suijten]

On 2023-05-19 22:37:34, Dmitry Baryshkov wrote:
<snip>
> >>>>>> +    ret = cfg_hnd->ops->calc_clk_rate(msm_host);
> >>>>>
> >>>>> I am not too sure what we are gaining by this.
> >>>>>
> >>>>> Its not that we are replacing dsi_get_pclk_rate().
> >>>>>
> >>>>> We are moving the dsi_get_pclk_rate() from the calc_clk_rate() to 
> >>>>> the msm_dsi_host_get_phy_clk_req().
> >>>>>
> >>>>> Also, with this change, dsi_calc_clk_rate_6g() looks kind of empty 
> >>>>> to stand on its own.
> >>>>>
> >>>>> The original intention of the calc_clk_rate() op seems to be 
> >>>>> calculate and store all the clocks (byte, pixel and esc).
> >>>>>
> >>>>> Why change that behavior by breaking it up?
> >>>>
> >>>> Unification between platforms. Both v2 and 6g platforms call 
> >>>> dsi_calc_pclk(). Let's just move it to a common code path.
> >>>
> >>> Hi Dmitry,
> >>>
> >>> I think what Abhinav means here is that the meaning and functionality 
> >>> of calc_clk_rate() changes with this patch.
> >>>
> >>> Before, calc_clk_rate() does *all* the clk_rate calculations and 
> >>> assignments. But after this change, it will only calculate and assign 
> >>> the escape clk rate.
> >>>
> >>> I agree with Abhinav that this change renders the calc_clk_rate() op 
> >>> misleading as it will not calculate all of the clock rates anymore.
> >>
> >> Would it make sense if I rename it to calc_other_clock_rates()?
> >>
> > 
> > Not really. I would rather still have it separate and drop this patch.
> > 
> > So even if pixel clock calculation looks common today between v2 and 6g, 
> > lets say tomorrow there is a 7g or 8g which needs some other math there, 
> > I think this is the right place where it should stay so that we 
> > calculate all clocks together.
> 
> Ack.

Unfortunate, but okay.  Then don't forget to send the following hunk of
this patch in isolation:

    -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
    +	pclk_bpp = msm_host->pixel_clk_rate * bpp;

- Marijn

> >> Moving pclk calculation to the core code emphasises that pclk 
> >> calculation is common between v2 and 6g hosts.
> >>
> >>>
> >>> Thanks,
> >>>
> >>> Jessica Zhang
> >>>
> >>>>
> >>>>>
> >>>>>>       if (ret) {
> >>>>>>           pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
> >>>>>>           return;
> >>>>
> >>>> -- 
> >>>> With best wishes
> >>>> Dmitry
> >>>>
> >>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling

[Marijn Suijten]

On 2023-01-18 15:00:31, Dmitry Baryshkov wrote:
> Move a call to dsi_calc_pclk() out of calc_clk_rate directly towards
> msm_dsi_host_get_phy_clk_req(). It is called for both 6g and v2 hosts.
> 
> Also, while we are at it, replace another dsi_get_pclk_rate() invocation
> with using the stored value at msm_host->pixel_clk_rate.

Yes please, this was annoying and confusing to read in one of the recent
patches to that stray pclk_bpp assignment, thanks for cleaning it up.

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

For the rest of the cleanup, also totally happy to see the duplication
moved out of the callback.  As Abhinav notes it does make the functions
a bit lighter, but that's exactly the purpose to make the differences
more obvious.

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  drivers/gpu/drm/msm/dsi/dsi.h      |  4 ++--
>  drivers/gpu/drm/msm/dsi/dsi_cfg.h  |  2 +-
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 24 ++++++++++++------------
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index bd3763a5d723..93ec54478eb6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -129,8 +129,8 @@ int dsi_dma_base_get_6g(struct msm_dsi_host *msm_host, uint64_t *iova);
>  int dsi_dma_base_get_v2(struct msm_dsi_host *msm_host, uint64_t *iova);
>  int dsi_clk_init_v2(struct msm_dsi_host *msm_host);
>  int dsi_clk_init_6g_v2(struct msm_dsi_host *msm_host);
> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host);
> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host);
>  void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host);
>  void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host);
>  struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_cfg.h b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> index 44be4a88aa83..5106e66846c3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi_cfg.h
> @@ -51,7 +51,7 @@ struct msm_dsi_host_cfg_ops {
>  	void* (*tx_buf_get)(struct msm_dsi_host *msm_host);
>  	void (*tx_buf_put)(struct msm_dsi_host *msm_host);
>  	int (*dma_base_get)(struct msm_dsi_host *msm_host, uint64_t *iova);
> -	int (*calc_clk_rate)(struct msm_dsi_host *msm_host, bool is_bonded_dsi);
> +	int (*calc_clk_rate)(struct msm_dsi_host *msm_host);
>  };
>  
>  struct msm_dsi_cfg_handler {
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 18fa30e1e858..7d99a108bff6 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -616,28 +616,21 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>  
>  }
>  
> -int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host)
>  {
> -	if (!msm_host->mode) {
> -		pr_err("%s: mode not set\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	dsi_calc_pclk(msm_host, is_bonded_dsi);
>  	msm_host->esc_clk_rate = clk_get_rate(msm_host->esc_clk);
> +
>  	return 0;
>  }
>  
> -int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host)
>  {
>  	u32 bpp = dsi_get_bpp(msm_host->format);
>  	u64 pclk_bpp;
>  	unsigned int esc_mhz, esc_div;
>  	unsigned long byte_mhz;
>  
> -	dsi_calc_pclk(msm_host, is_bonded_dsi);
> -
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
> +	pclk_bpp = msm_host->pixel_clk_rate * bpp;
>  	do_div(pclk_bpp, 8);
>  	msm_host->src_clk_rate = pclk_bpp;
>  
> @@ -2292,7 +2285,14 @@ void msm_dsi_host_get_phy_clk_req(struct mipi_dsi_host *host,
>  	const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd;
>  	int ret;
>  
> -	ret = cfg_hnd->ops->calc_clk_rate(msm_host, is_bonded_dsi);
> +	if (!msm_host->mode) {
> +		pr_err("%s: mode not set\n", __func__);
> +		return;
> +	}
> +
> +	dsi_calc_pclk(msm_host, is_bonded_dsi);
> +
> +	ret = cfg_hnd->ops->calc_clk_rate(msm_host);
>  	if (ret) {
>  		pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
>  		return;
> -- 
> 2.39.0
>