Re: [PATCH 1/2] drm/msm/dsi: add a helper method to compute the dsi byte clk

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 28/10/2022 01:22, Abhinav Kumar wrote:
> On 10/27/2022 10:35 AM, Dmitry Baryshkov wrote:
> > On 22/09/2022 03:49, Abhinav Kumar wrote:
> > > Re-arrange the dsi_calc_pclk method to two helpers, one to
> > > compute the DSI byte clk and the other to compute the pclk.
> > >
> > > This makes the separation of the two clean and also allows
> > > clients to compute and use the dsi byte clk separately.
> > >
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
> > >   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
> > >   2 files changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
> > > b/drivers/gpu/drm/msm/dsi/dsi.h
> > > index 2a96b4fe7839..60ba8e67f550 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi.h
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> > > @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host 
> > > *msm_host);
> > >   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
> > >   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
> > >   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
> > > +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
> > > is_bonded_dsi,
> > > +        const struct drm_display_mode *mode);
> > >   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
> > >   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
> > >   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> > > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > index 57a4c0fa614b..32b35d4ac1d3 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
> > > *msm_host)
> > >       clk_disable_unprepare(msm_host->byte_clk);
> > >   }
> > > -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host 
> > > *msm_host, bool is_bonded_dsi)
> > > +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
> > > *mode, bool is_bonded_dsi)
> > >   {
> > > -    struct drm_display_mode *mode = msm_host->mode;
> > >       unsigned long pclk_rate;
> > >       pclk_rate = mode->clock * 1000;
> > > @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct 
> > > msm_dsi_host *msm_host, bool is_bo
> > >       return pclk_rate;
> > >   }
> > > -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
> > > is_bonded_dsi)
> > > +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
> > > is_bonded_dsi,
> > > +        const struct drm_display_mode *mode)
> > >   {
> > > +    struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
> > >       u8 lanes = msm_host->lanes;
> > >       u32 bpp = dsi_get_bpp(msm_host->format);
> > > -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
> > > is_bonded_dsi);
> > > -    u64 pclk_bpp = (u64)pclk_rate * bpp;
> > > +    unsigned long pclk_rate;
> > > +    u64 pclk_bpp;
> > > +
> > > +    pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> > > +
> > > +    pclk_bpp = (u64)pclk_rate * bpp;
> > >       if (lanes == 0) {
> > >           pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
> > > @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host 
> > > *msm_host, bool is_bonded_dsi)
> > >       else
> > >           do_div(pclk_bpp, (8 * lanes));
> > > -    msm_host->pixel_clk_rate = pclk_rate;
> > > -    msm_host->byte_clk_rate = pclk_bpp;
> > > +    return pclk_bpp;
> > > +}
> > > +
> > > +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
> > > is_bonded_dsi)
> > > +{
> > > +    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
> > > is_bonded_dsi);
> > > +    msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, 
> > > is_bonded_dsi,
> > > +            msm_host->mode);
> >
> > This way you are calling dsi_get_pclk_rate twice(), which is slightly 
> > inefficient. You can call it once (here) and then pass the resulting 
> > pclk_rate as an argument to dsi_byte_clk_get_rate().
>
> So the goal was to have two independent APIs to calculate byte and pixel 
> clk.
>
> If we pass the output of one as the input to the other we are making 
> them dependent.
>
> Thats why i kept it separate.

Calling one function from another clearly points that they are not 
independent. And surely pixel and byte clocks can not be fully 
independent. I see your point about getting only the byte clock. But I 
think it would be easier to explicitly pass the pixel rate rather than 
calculating it again under the hood.

> > >       DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
> > >                   msm_host->byte_clk_rate);
> > > @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host 
> > > *msm_host, bool is_bonded_dsi)
> > >       dsi_calc_pclk(msm_host, is_bonded_dsi);
> > > -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
> > > +    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
> > > * bpp;
> >
> > So... We have calculated all rates, stored the pclk_rate in 
> > msm_host->pixel_clk_rate. And now we are going to calculate it again. 
> > As you are touching this line of code, I'd suggest to just use 
> > msm_host->pixel_clk_rate instead of a function call.
>
> Ack, I will fix this.
>
> > >       do_div(pclk_bpp, 8);
> > >       msm_host->src_clk_rate = pclk_bpp;

--
With best wishes
Dmitry