| Message ID | 20230118130031.2345941-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm/dsi: simplify pixel clk rate handling | expand |
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;
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;
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 >
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 --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;
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(-)