[Freedreno] [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 at 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