diff mbox series

drm/msm/dsi: simplify pixel clk rate handling

Message ID 20230118130031.2345941-1-dmitry.baryshkov@linaro.org
State New
Headers show
Series drm/msm/dsi: simplify pixel clk rate handling | expand

Commit Message

Dmitry Baryshkov Jan. 18, 2023, 1 p.m. UTC
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(-)

Comments

Abhinav Kumar Jan. 26, 2023, 12:07 a.m. UTC | #1
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;
Dmitry Baryshkov March 28, 2023, 1:04 p.m. UTC | #2
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;
Marijn Suijten May 8, 2023, 9:10 p.m. UTC | #3
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
>
Dmitry Baryshkov May 19, 2023, 7:37 p.m. UTC | #4
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
>>>>
>>
diff mbox series

Patch

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;