[Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri May 19 19:37:34 UTC 2023
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 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.
>>>
>>> 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
More information about the dri-devel
mailing list