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

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

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@xxxxxxxxxx>
> > > > > ---
> > > > >   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