| Message ID | 20230118130031.2345941-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| 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 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. Thanks, Jessica Zhang > >> >>> if (ret) { >>> pr_err("%s: unable to calc clk rate, %d\n", __func__, ret); >>> return; > > -- > With best wishes > Dmitry >
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()? 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 >>
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. > 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 >>> >
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 >>>> >>
On 2023-05-19 22:37:34, Dmitry Baryshkov wrote: <snip> > >>>>>> + 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. Unfortunate, but okay. Then don't forget to send the following hunk of this patch in isolation: - pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp; + pclk_bpp = msm_host->pixel_clk_rate * bpp; - Marijn > >> 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 >
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(-)