* [PATCH] drm/msm/dsi: simplify pixel clk rate handling
@ 2023-01-18 13:00 Dmitry Baryshkov
2023-01-26 0:07 ` Abhinav Kumar
2023-05-08 21:10 ` Marijn Suijten
0 siblings, 2 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2023-01-18 13:00 UTC (permalink / raw)
To: Rob Clark, Sean Paul, Abhinav Kumar
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
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);
if (ret) {
pr_err("%s: unable to calc clk rate, %d\n", __func__, ret);
return;
--
2.39.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-01-18 13:00 [PATCH] drm/msm/dsi: simplify pixel clk rate handling Dmitry Baryshkov
@ 2023-01-26 0:07 ` Abhinav Kumar
2023-03-28 13:04 ` Dmitry Baryshkov
2023-05-08 21:10 ` Marijn Suijten
1 sibling, 1 reply; 9+ messages in thread
From: Abhinav Kumar @ 2023-01-26 0:07 UTC (permalink / raw)
To: Dmitry Baryshkov, Rob Clark, Sean Paul
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-01-26 0:07 ` Abhinav Kumar
@ 2023-03-28 13:04 ` Dmitry Baryshkov
2023-05-19 18:54 ` [Freedreno] " Jessica Zhang
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2023-03-28 13:04 UTC (permalink / raw)
To: Abhinav Kumar, Rob Clark, Sean Paul
Cc: freedreno, linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd
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;
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-01-18 13:00 [PATCH] drm/msm/dsi: simplify pixel clk rate handling Dmitry Baryshkov
2023-01-26 0:07 ` Abhinav Kumar
@ 2023-05-08 21:10 ` Marijn Suijten
1 sibling, 0 replies; 9+ messages in thread
From: Marijn Suijten @ 2023-05-08 21:10 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, Sean Paul, Bjorn Andersson, Abhinav Kumar, dri-devel,
Stephen Boyd, linux-arm-msm
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-03-28 13:04 ` Dmitry Baryshkov
@ 2023-05-19 18:54 ` Jessica Zhang
2023-05-19 19:33 ` Dmitry Baryshkov
0 siblings, 1 reply; 9+ messages in thread
From: Jessica Zhang @ 2023-05-19 18:54 UTC (permalink / raw)
To: Dmitry Baryshkov, Abhinav Kumar, Rob Clark, Sean Paul
Cc: linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd, freedreno
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-05-19 18:54 ` [Freedreno] " Jessica Zhang
@ 2023-05-19 19:33 ` Dmitry Baryshkov
2023-05-19 19:36 ` Abhinav Kumar
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2023-05-19 19:33 UTC (permalink / raw)
To: Jessica Zhang, Abhinav Kumar, Rob Clark, Sean Paul
Cc: linux-arm-msm, Bjorn Andersson, dri-devel, Stephen Boyd, freedreno
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
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-05-19 19:33 ` Dmitry Baryshkov
@ 2023-05-19 19:36 ` Abhinav Kumar
2023-05-19 19:37 ` Dmitry Baryshkov
0 siblings, 1 reply; 9+ messages in thread
From: Abhinav Kumar @ 2023-05-19 19:36 UTC (permalink / raw)
To: Dmitry Baryshkov, Jessica Zhang, Rob Clark, Sean Paul
Cc: linux-arm-msm, Bjorn Andersson, freedreno, dri-devel, Stephen Boyd
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
>>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-05-19 19:36 ` Abhinav Kumar
@ 2023-05-19 19:37 ` Dmitry Baryshkov
2023-05-19 21:14 ` Marijn Suijten
0 siblings, 1 reply; 9+ messages in thread
From: Dmitry Baryshkov @ 2023-05-19 19:37 UTC (permalink / raw)
To: Abhinav Kumar, Jessica Zhang, Rob Clark, Sean Paul
Cc: linux-arm-msm, Bjorn Andersson, freedreno, dri-devel, Stephen Boyd
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
>>>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Freedreno] [PATCH] drm/msm/dsi: simplify pixel clk rate handling
2023-05-19 19:37 ` Dmitry Baryshkov
@ 2023-05-19 21:14 ` Marijn Suijten
0 siblings, 0 replies; 9+ messages in thread
From: Marijn Suijten @ 2023-05-19 21:14 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: freedreno, linux-arm-msm, Bjorn Andersson, Abhinav Kumar,
dri-devel, Stephen Boyd, Jessica Zhang, Sean Paul
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-05-19 21:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 13:00 [PATCH] drm/msm/dsi: simplify pixel clk rate handling Dmitry Baryshkov
2023-01-26 0:07 ` Abhinav Kumar
2023-03-28 13:04 ` Dmitry Baryshkov
2023-05-19 18:54 ` [Freedreno] " Jessica Zhang
2023-05-19 19:33 ` Dmitry Baryshkov
2023-05-19 19:36 ` Abhinav Kumar
2023-05-19 19:37 ` Dmitry Baryshkov
2023-05-19 21:14 ` Marijn Suijten
2023-05-08 21:10 ` Marijn Suijten
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).