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

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

[Abhinav Kumar]

Re-arrange the dsi_calc_pclk method to two helpers, one to
compute the DSI byte clk and the other to compute the pclk.

This makes the separation of the two clean and also allows
clients to compute and use the dsi byte clk separately.

Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
 drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
index 2a96b4fe7839..60ba8e67f550 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.h
+++ b/drivers/gpu/drm/msm/dsi/dsi.h
@@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
 int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
 void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
+unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
+		const struct drm_display_mode *mode);
 int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
 int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
 void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 57a4c0fa614b..32b35d4ac1d3 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
 	clk_disable_unprepare(msm_host->byte_clk);
 }
 
-static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
+static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
 {
-	struct drm_display_mode *mode = msm_host->mode;
 	unsigned long pclk_rate;
 
 	pclk_rate = mode->clock * 1000;
@@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bo
 	return pclk_rate;
 }
 
-static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
+unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
+		const struct drm_display_mode *mode)
 {
+	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
 	u8 lanes = msm_host->lanes;
 	u32 bpp = dsi_get_bpp(msm_host->format);
-	unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, is_bonded_dsi);
-	u64 pclk_bpp = (u64)pclk_rate * bpp;
+	unsigned long pclk_rate;
+	u64 pclk_bpp;
+
+	pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
+
+	pclk_bpp = (u64)pclk_rate * bpp;
 
 	if (lanes == 0) {
 		pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
@@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 	else
 		do_div(pclk_bpp, (8 * lanes));
 
-	msm_host->pixel_clk_rate = pclk_rate;
-	msm_host->byte_clk_rate = pclk_bpp;
+	return pclk_bpp;
+}
+
+static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
+{
+	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
+	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
+			msm_host->mode);
 
 	DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
 				msm_host->byte_clk_rate);
@@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
 
 	dsi_calc_pclk(msm_host, is_bonded_dsi);
 
-	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
+	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
 	do_div(pclk_bpp, 8);
 	msm_host->src_clk_rate = pclk_bpp;
 
-- 
2.7.4

[PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

Currently there is no protection against a user trying to set
an unsupported mode on DSI. Implement a check based on the opp
table whether the byte clock for the mode can be supported by
validating whether an opp table entry exists.

For devices which have not added opp table support yet, skip
this check otherwise it will break bootup on those devices.

Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
Reported-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 3a1417397283..87b518c42965 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -450,6 +450,29 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
 	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct mipi_dsi_host *host = msm_dsi->host;
+	struct platform_device *pdev = msm_dsi->pdev;
+	struct dev_pm_opp *opp;
+	struct opp_table *opp_tbl;
+	unsigned long byte_clk_rate;
+
+	byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
+
+	/*
+	 * first check if there is an opp table available for the calculated
+	 * byte clock and then check DSC related info. Some devices have not
+	 * added support for OPP table. Skip the check for those.
+	 */
+	opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
+	if (opp_tbl) {
+		opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
+		if (IS_ERR(opp)) {
+			pr_err("opp table not found for freq %lu err: %ld\n",
+					byte_clk_rate, PTR_ERR(opp));
+			return PTR_ERR(opp);
+		}
+		dev_pm_opp_put(opp);
+		dev_pm_opp_put_opp_table(opp_tbl);
+	}
 
 	return msm_dsi_host_check_dsc(host, mode);
 }
-- 
2.7.4

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

[Dmitry Baryshkov]

On 22/09/2022 03:49, Abhinav Kumar wrote:
> Re-arrange the dsi_calc_pclk method to two helpers, one to
> compute the DSI byte clk and the other to compute the pclk.
> 
> This makes the separation of the two clean and also allows
> clients to compute and use the dsi byte clk separately.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
>   2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 2a96b4fe7839..60ba8e67f550 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
>   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
> +		const struct drm_display_mode *mode);
>   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
>   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
>   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 57a4c0fa614b..32b35d4ac1d3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>   	clk_disable_unprepare(msm_host->byte_clk);
>   }
>   
> -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
>   {
> -	struct drm_display_mode *mode = msm_host->mode;
>   	unsigned long pclk_rate;
>   
>   	pclk_rate = mode->clock * 1000;
> @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bo
>   	return pclk_rate;
>   }
>   
> -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
> +		const struct drm_display_mode *mode)
>   {
> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>   	u8 lanes = msm_host->lanes;
>   	u32 bpp = dsi_get_bpp(msm_host->format);
> -	unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, is_bonded_dsi);
> -	u64 pclk_bpp = (u64)pclk_rate * bpp;
> +	unsigned long pclk_rate;
> +	u64 pclk_bpp;
> +
> +	pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> +
> +	pclk_bpp = (u64)pclk_rate * bpp;
>   
>   	if (lanes == 0) {
>   		pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
> @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   	else
>   		do_div(pclk_bpp, (8 * lanes));
>   
> -	msm_host->pixel_clk_rate = pclk_rate;
> -	msm_host->byte_clk_rate = pclk_bpp;
> +	return pclk_bpp;
> +}
> +
> +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +{
> +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> +	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
> +			msm_host->mode);

This way you are calling dsi_get_pclk_rate twice(), which is slightly 
inefficient. You can call it once (here) and then pass the resulting 
pclk_rate as an argument to dsi_byte_clk_get_rate().

>   
>   	DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
>   				msm_host->byte_clk_rate);
> @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   	dsi_calc_pclk(msm_host, is_bonded_dsi);
>   
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
> +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;

So... We have calculated all rates, stored the pclk_rate in 
msm_host->pixel_clk_rate. And now we are going to calculate it again. As 
you are touching this line of code, I'd suggest to just use 
msm_host->pixel_clk_rate instead of a function call.

>   	do_div(pclk_bpp, 8);
>   	msm_host->src_clk_rate = pclk_bpp;
>   

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Dmitry Baryshkov]

On 22/09/2022 03:49, Abhinav Kumar wrote:
> Currently there is no protection against a user trying to set
> an unsupported mode on DSI. Implement a check based on the opp
> table whether the byte clock for the mode can be supported by
> validating whether an opp table entry exists.
> 
> For devices which have not added opp table support yet, skip
> this check otherwise it will break bootup on those devices.
> 
> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
> Reported-by: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> index 3a1417397283..87b518c42965 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
> @@ -450,6 +450,29 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>   	int id = dsi_mgr_bridge_get_id(bridge);
>   	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>   	struct mipi_dsi_host *host = msm_dsi->host;
> +	struct platform_device *pdev = msm_dsi->pdev;
> +	struct dev_pm_opp *opp;
> +	struct opp_table *opp_tbl;
> +	unsigned long byte_clk_rate;
> +
> +	byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
> +
> +	/*
> +	 * first check if there is an opp table available for the calculated
> +	 * byte clock and then check DSC related info. Some devices have not
> +	 * added support for OPP table. Skip the check for those.
> +	 */
> +	opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);

Can we store the table inside the msm_dsi during the init? Then we won't 
have to get it again and again during each mode_valid call.

> +	if (opp_tbl) {
> +		opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
> +		if (IS_ERR(opp)) {
> +			pr_err("opp table not found for freq %lu err: %ld\n",
> +					byte_clk_rate, PTR_ERR(opp));
> +			return PTR_ERR(opp);
> +		}
> +		dev_pm_opp_put(opp);
> +		dev_pm_opp_put_opp_table(opp_tbl);
> +	}
>   
>   	return msm_dsi_host_check_dsc(host, mode);
>   }

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

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

So the goal was to have two independent APIs to calculate byte and pixel 
clk.

If we pass the output of one as the input to the other we are making 
them dependent.

Thats why i kept it separate.

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

Ack, I will fix this.

> 
>>       do_div(pclk_bpp, 8);
>>       msm_host->src_clk_rate = pclk_bpp;
> 

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

[Dmitry Baryshkov]

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

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

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

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

Hi Dmitry

Sorry for the late response.

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

Yes, calling one function from another means they are dependent and 
thats true in this case because byte clk computation depends on the pclk.

The reason behind this separation was that in the next patch, we need to 
compute only the byte clock and use that to validate in the mode_valid 
against the opp table limits.

The opp table for DSI is based on the byte clk.

If we were to pass pclk as a parameter, then we would have to explicitly 
compute the pclk just to pass the parameter to the method which computes 
the byte clk. As opposed to this approach, where it just happens under 
the hood.

So for the purpose of the next patch I kept it this way.

Let me know what you think.

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

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Dmitry Baryshkov]

On 27/10/2022 20:36, Dmitry Baryshkov wrote:
> On 22/09/2022 03:49, Abhinav Kumar wrote:
>> Currently there is no protection against a user trying to set
>> an unsupported mode on DSI. Implement a check based on the opp
>> table whether the byte clock for the mode can be supported by
>> validating whether an opp table entry exists.
>>
>> For devices which have not added opp table support yet, skip
>> this check otherwise it will break bootup on those devices.
>>
>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>> Reported-by: Rob Clark <robdclark@gmail.com>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> index 3a1417397283..87b518c42965 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>       int id = dsi_mgr_bridge_get_id(bridge);
>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>       struct mipi_dsi_host *host = msm_dsi->host;
>> +    struct platform_device *pdev = msm_dsi->pdev;
>> +    struct dev_pm_opp *opp;
>> +    struct opp_table *opp_tbl;
>> +    unsigned long byte_clk_rate;
>> +
>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
>> +
>> +    /*
>> +     * first check if there is an opp table available for the calculated
>> +     * byte clock and then check DSC related info. Some devices have not
>> +     * added support for OPP table. Skip the check for those.
>> +     */
>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
> 
> Can we store the table inside the msm_dsi during the init? Then we won't 
> have to get it again and again during each mode_valid call.

I checked other drivers. I think we can skip the get_opp_table 
completely, can we not? Just handle ENODEV returned from 
dev_pm_opp_find_freq_ceil().

> 
>> +    if (opp_tbl) {
>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
>> +        if (IS_ERR(opp)) {
>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>> +                    byte_clk_rate, PTR_ERR(opp));
>> +            return PTR_ERR(opp);
>> +        }
>> +        dev_pm_opp_put(opp);
>> +        dev_pm_opp_put_opp_table(opp_tbl);
>> +    }
>>       return msm_dsi_host_check_dsc(host, mode);
>>   }
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 22/09/2022 03:49, Abhinav Kumar wrote:
> Re-arrange the dsi_calc_pclk method to two helpers, one to
> compute the DSI byte clk and the other to compute the pclk.
> 
> This makes the separation of the two clean and also allows
> clients to compute and use the dsi byte clk separately.
> 
> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
>   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
>   2 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h
> index 2a96b4fe7839..60ba8e67f550 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi.h
> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
> @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host *msm_host);
>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
>   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
> +		const struct drm_display_mode *mode);
>   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
>   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
>   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 57a4c0fa614b..32b35d4ac1d3 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host)
>   	clk_disable_unprepare(msm_host->byte_clk);
>   }
>   
> -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode *mode, bool is_bonded_dsi)
>   {
> -	struct drm_display_mode *mode = msm_host->mode;
>   	unsigned long pclk_rate;
>   
>   	pclk_rate = mode->clock * 1000;
> @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, bool is_bo
>   	return pclk_rate;
>   }
>   
> -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool is_bonded_dsi,
> +		const struct drm_display_mode *mode)
>   {
> +	struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>   	u8 lanes = msm_host->lanes;
>   	u32 bpp = dsi_get_bpp(msm_host->format);
> -	unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, is_bonded_dsi);
> -	u64 pclk_bpp = (u64)pclk_rate * bpp;
> +	unsigned long pclk_rate;
> +	u64 pclk_bpp;
> +
> +	pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
> +
> +	pclk_bpp = (u64)pclk_rate * bpp;

Any particular reason for this? The following patch would be more obvious:

 > -	unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, is_bonded_dsi);
 > +	unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
 >  	u64 pclk_bpp = (u64)pclk_rate * bpp;


>   
>   	if (lanes == 0) {
>   		pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
> @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   	else
>   		do_div(pclk_bpp, (8 * lanes));
>   
> -	msm_host->pixel_clk_rate = pclk_rate;
> -	msm_host->byte_clk_rate = pclk_bpp;
> +	return pclk_bpp;
> +}
> +
> +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
> +{
> +	msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi);
> +	msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
> +			msm_host->mode);
>   
>   	DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
>   				msm_host->byte_clk_rate);
> @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi)
>   
>   	dsi_calc_pclk(msm_host, is_bonded_dsi);
>   
> -	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
> +	pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) * bpp;
>   	do_div(pclk_bpp, 8);
>   	msm_host->src_clk_rate = pclk_bpp;


Following my previous feedback:

I think at this moment msm_host->src_clk_rate = msm_host->byte_clk_rate 
* msm_host->lanes. If so, we can drop dsi_get_pclk_rate() call and the 
multiply/do_div calculation and use the above formula instead.

If this looks logical, could you please prepend a patch changing this?

LGTM otherwise.

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 1/9/2023 5:34 PM, Dmitry Baryshkov wrote:
> On 22/09/2022 03:49, Abhinav Kumar wrote:
>> Re-arrange the dsi_calc_pclk method to two helpers, one to
>> compute the DSI byte clk and the other to compute the pclk.
>>
>> This makes the separation of the two clean and also allows
>> clients to compute and use the dsi byte clk separately.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
>>   2 files changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>> b/drivers/gpu/drm/msm/dsi/dsi.h
>> index 2a96b4fe7839..60ba8e67f550 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>> @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host 
>> *msm_host);
>>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>>   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
>>   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
>> is_bonded_dsi,
>> +        const struct drm_display_mode *mode);
>>   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
>>   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
>>   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index 57a4c0fa614b..32b35d4ac1d3 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
>> *msm_host)
>>       clk_disable_unprepare(msm_host->byte_clk);
>>   }
>> -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host *msm_host, 
>> bool is_bonded_dsi)
>> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
>> *mode, bool is_bonded_dsi)
>>   {
>> -    struct drm_display_mode *mode = msm_host->mode;
>>       unsigned long pclk_rate;
>>       pclk_rate = mode->clock * 1000;
>> @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct 
>> msm_dsi_host *msm_host, bool is_bo
>>       return pclk_rate;
>>   }
>> -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
>> is_bonded_dsi,
>> +        const struct drm_display_mode *mode)
>>   {
>> +    struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>       u8 lanes = msm_host->lanes;
>>       u32 bpp = dsi_get_bpp(msm_host->format);
>> -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
>> is_bonded_dsi);
>> -    u64 pclk_bpp = (u64)pclk_rate * bpp;
>> +    unsigned long pclk_rate;
>> +    u64 pclk_bpp;
>> +
>> +    pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>> +
>> +    pclk_bpp = (u64)pclk_rate * bpp;
> 
> Any particular reason for this? The following patch would be more obvious:
> 
>  > -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
> is_bonded_dsi);
>  > +    unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>  >      u64 pclk_bpp = (u64)pclk_rate * bpp;
> 
> 
>>       if (lanes == 0) {
>>           pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
>> @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>       else
>>           do_div(pclk_bpp, (8 * lanes));
>> -    msm_host->pixel_clk_rate = pclk_rate;
>> -    msm_host->byte_clk_rate = pclk_bpp;
>> +    return pclk_bpp;
>> +}
>> +
>> +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>> is_bonded_dsi)
>> +{
>> +    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
>> is_bonded_dsi);
>> +    msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, 
>> is_bonded_dsi,
>> +            msm_host->mode);
>>       DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
>>                   msm_host->byte_clk_rate);
>> @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host 
>> *msm_host, bool is_bonded_dsi)
>>       dsi_calc_pclk(msm_host, is_bonded_dsi);
>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
>> +    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
>> * bpp;
>>       do_div(pclk_bpp, 8);
>>       msm_host->src_clk_rate = pclk_bpp;
> 
> 
> Following my previous feedback:
> 
> I think at this moment msm_host->src_clk_rate = msm_host->byte_clk_rate 
> * msm_host->lanes. If so, we can drop dsi_get_pclk_rate() call and the 
> multiply/do_div calculation and use the above formula instead.
> 

 From what I see msm_host->src_clk_rate = pixel_clk * bpp / 8;

 From where did you get the above formula?

I just felt that by having two APIs the next patch becomes easier 
because I need to just invoke the API which calculates byte clk.

> If this looks logical, could you please prepend a patch changing this?
> 
> LGTM otherwise.
> 

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

[Dmitry Baryshkov]

On 10/01/2023 04:21, Abhinav Kumar wrote:
> 
> 
> On 1/9/2023 5:34 PM, Dmitry Baryshkov wrote:
>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>> Re-arrange the dsi_calc_pclk method to two helpers, one to
>>> compute the DSI byte clk and the other to compute the pclk.
>>>
>>> This makes the separation of the two clean and also allows
>>> clients to compute and use the dsi byte clk separately.
>>>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
>>>   2 files changed, 21 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>> index 2a96b4fe7839..60ba8e67f550 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>> @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host 
>>> *msm_host);
>>>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>>>   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
>>>   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
>>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
>>> is_bonded_dsi,
>>> +        const struct drm_display_mode *mode);
>>>   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
>>>   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
>>>   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 57a4c0fa614b..32b35d4ac1d3 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
>>> *msm_host)
>>>       clk_disable_unprepare(msm_host->byte_clk);
>>>   }
>>> -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>> +static unsigned long dsi_get_pclk_rate(const struct drm_display_mode 
>>> *mode, bool is_bonded_dsi)
>>>   {
>>> -    struct drm_display_mode *mode = msm_host->mode;
>>>       unsigned long pclk_rate;
>>>       pclk_rate = mode->clock * 1000;
>>> @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct 
>>> msm_dsi_host *msm_host, bool is_bo
>>>       return pclk_rate;
>>>   }
>>> -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi)
>>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, bool 
>>> is_bonded_dsi,
>>> +        const struct drm_display_mode *mode)
>>>   {
>>> +    struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>>       u8 lanes = msm_host->lanes;
>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>> -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
>>> is_bonded_dsi);
>>> -    u64 pclk_bpp = (u64)pclk_rate * bpp;
>>> +    unsigned long pclk_rate;
>>> +    u64 pclk_bpp;
>>> +
>>> +    pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>>> +
>>> +    pclk_bpp = (u64)pclk_rate * bpp;
>>
>> Any particular reason for this? The following patch would be more 
>> obvious:
>>
>>  > -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
>> is_bonded_dsi);
>>  > +    unsigned long pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>>  >      u64 pclk_bpp = (u64)pclk_rate * bpp;
>>
>>
>>>       if (lanes == 0) {
>>>           pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
>>> @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>       else
>>>           do_div(pclk_bpp, (8 * lanes));
>>> -    msm_host->pixel_clk_rate = pclk_rate;
>>> -    msm_host->byte_clk_rate = pclk_bpp;
>>> +    return pclk_bpp;
>>> +}
>>> +
>>> +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>>> is_bonded_dsi)
>>> +{
>>> +    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
>>> is_bonded_dsi);
>>> +    msm_host->byte_clk_rate = dsi_byte_clk_get_rate(&msm_host->base, 
>>> is_bonded_dsi,
>>> +            msm_host->mode);
>>>       DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
>>>                   msm_host->byte_clk_rate);
>>> @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host 
>>> *msm_host, bool is_bonded_dsi)
>>>       dsi_calc_pclk(msm_host, is_bonded_dsi);
>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
>>> +    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, is_bonded_dsi) 
>>> * bpp;
>>>       do_div(pclk_bpp, 8);
>>>       msm_host->src_clk_rate = pclk_bpp;
>>
>>
>> Following my previous feedback:
>>
>> I think at this moment msm_host->src_clk_rate = 
>> msm_host->byte_clk_rate * msm_host->lanes. If so, we can drop 
>> dsi_get_pclk_rate() call and the multiply/do_div calculation and use 
>> the above formula instead.
>>
> 
>  From what I see msm_host->src_clk_rate = pixel_clk * bpp / 8;

and msm_host->byte_clk_rate = pixel_clk * bpp / (8 * nlanes);

> 
>  From where did you get the above formula?
> 
> I just felt that by having two APIs the next patch becomes easier 
> because I need to just invoke the API which calculates byte clk.

Yes, I see that, it looks like a correct approach. You know, let's 
ignore the dsi_calc_clk_rate_v2() for now, it is definitely a separate 
change.

Could you please drop the opp_table handling from patch 2/2, move 
assignments in dsi_byte_clk_get_rate() back to the definition lines and 
then send it as v2?

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>> Currently there is no protection against a user trying to set
>>> an unsupported mode on DSI. Implement a check based on the opp
>>> table whether the byte clock for the mode can be supported by
>>> validating whether an opp table entry exists.
>>>
>>> For devices which have not added opp table support yet, skip
>>> this check otherwise it will break bootup on those devices.
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>> Reported-by: Rob Clark <robdclark@gmail.com>
>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>   1 file changed, 23 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 3a1417397283..87b518c42965 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>> +    struct dev_pm_opp *opp;
>>> +    struct opp_table *opp_tbl;
>>> +    unsigned long byte_clk_rate;
>>> +
>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
>>> +
>>> +    /*
>>> +     * first check if there is an opp table available for the 
>>> calculated
>>> +     * byte clock and then check DSC related info. Some devices have 
>>> not
>>> +     * added support for OPP table. Skip the check for those.
>>> +     */
>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>
>> Can we store the table inside the msm_dsi during the init? Then we 
>> won't have to get it again and again during each mode_valid call.
> 
> I checked other drivers. I think we can skip the get_opp_table 
> completely, can we not? Just handle ENODEV returned from 
> dev_pm_opp_find_freq_ceil().
> 

Your point is valid but I had a doubt on that API.

As per the documentation of that API, it says

639  * Return: matching *opp and refreshes *freq accordingly, else returns
640  * ERR_PTR in case of error and should be handled using IS_ERR. 
Error return
641  * values can be:
642  * EINVAL:	for bad pointer
643  * ERANGE:	no match found for search
644  * ENODEV:	if device not found in list of registered devices
645  *
646  * The callers are required to call dev_pm_opp_put() for the 
returned OPP after
647  * use.
648  */
649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
650 					     unsigned long *freq)
651 {

So ideally yes, ENODEV means that table was not found but .... that API 
uses _find_opp_table under the hood.

which says

79  * Return: pointer to 'struct opp_table' if found, otherwise -ENODEV or
80  * -EINVAL based on type of error.
81  *
82  * The callers must call dev_pm_opp_put_opp_table() after the table 
is used.

Now, how would we know if the failure was due to table not found OR 
entry not found.

Table now found means that SOC has probably not started using OPP table 
which is alright and not an error.

But EINVAL could mean an entry not found which means it exceeds the opp 
table limits.

So there was some ambiguity on this. So I broke it down into two calls.

If my concern is invalid, let me know.

But I do agree with you that we can cache the opp table once rather than 
doing it in every mode_valid().

>>
>>> +    if (opp_tbl) {
>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
>>> +        if (IS_ERR(opp)) {
>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>> +                    byte_clk_rate, PTR_ERR(opp));
>>> +            return PTR_ERR(opp);
>>> +        }
>>> +        dev_pm_opp_put(opp);
>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>> +    }
>>>       return msm_dsi_host_check_dsc(host, mode);
>>>   }
>>
> 

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

[Abhinav Kumar]

On 1/9/2023 6:25 PM, Dmitry Baryshkov wrote:
> On 10/01/2023 04:21, Abhinav Kumar wrote:
>>
>>
>> On 1/9/2023 5:34 PM, Dmitry Baryshkov wrote:
>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>> Re-arrange the dsi_calc_pclk method to two helpers, one to
>>>> compute the DSI byte clk and the other to compute the pclk.
>>>>
>>>> This makes the separation of the two clean and also allows
>>>> clients to compute and use the dsi byte clk separately.
>>>>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi.h      |  2 ++
>>>>   drivers/gpu/drm/msm/dsi/dsi_host.c | 27 +++++++++++++++++++--------
>>>>   2 files changed, 21 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi.h 
>>>> b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> index 2a96b4fe7839..60ba8e67f550 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi.h
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi.h
>>>> @@ -118,6 +118,8 @@ int dsi_link_clk_enable_6g(struct msm_dsi_host 
>>>> *msm_host);
>>>>   int dsi_link_clk_enable_v2(struct msm_dsi_host *msm_host);
>>>>   void dsi_link_clk_disable_6g(struct msm_dsi_host *msm_host);
>>>>   void dsi_link_clk_disable_v2(struct msm_dsi_host *msm_host);
>>>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, 
>>>> bool is_bonded_dsi,
>>>> +        const struct drm_display_mode *mode);
>>>>   int dsi_tx_buf_alloc_6g(struct msm_dsi_host *msm_host, int size);
>>>>   int dsi_tx_buf_alloc_v2(struct msm_dsi_host *msm_host, int size);
>>>>   void *dsi_tx_buf_get_6g(struct msm_dsi_host *msm_host);
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> index 57a4c0fa614b..32b35d4ac1d3 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>>> @@ -569,9 +569,8 @@ void dsi_link_clk_disable_v2(struct msm_dsi_host 
>>>> *msm_host)
>>>>       clk_disable_unprepare(msm_host->byte_clk);
>>>>   }
>>>> -static unsigned long dsi_get_pclk_rate(struct msm_dsi_host 
>>>> *msm_host, bool is_bonded_dsi)
>>>> +static unsigned long dsi_get_pclk_rate(const struct 
>>>> drm_display_mode *mode, bool is_bonded_dsi)
>>>>   {
>>>> -    struct drm_display_mode *mode = msm_host->mode;
>>>>       unsigned long pclk_rate;
>>>>       pclk_rate = mode->clock * 1000;
>>>> @@ -588,12 +587,18 @@ static unsigned long dsi_get_pclk_rate(struct 
>>>> msm_dsi_host *msm_host, bool is_bo
>>>>       return pclk_rate;
>>>>   }
>>>> -static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi)
>>>> +unsigned long dsi_byte_clk_get_rate(struct mipi_dsi_host *host, 
>>>> bool is_bonded_dsi,
>>>> +        const struct drm_display_mode *mode)
>>>>   {
>>>> +    struct msm_dsi_host *msm_host = to_msm_dsi_host(host);
>>>>       u8 lanes = msm_host->lanes;
>>>>       u32 bpp = dsi_get_bpp(msm_host->format);
>>>> -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
>>>> is_bonded_dsi);
>>>> -    u64 pclk_bpp = (u64)pclk_rate * bpp;
>>>> +    unsigned long pclk_rate;
>>>> +    u64 pclk_bpp;
>>>> +
>>>> +    pclk_rate = dsi_get_pclk_rate(mode, is_bonded_dsi);
>>>> +
>>>> +    pclk_bpp = (u64)pclk_rate * bpp;
>>>
>>> Any particular reason for this? The following patch would be more 
>>> obvious:
>>>
>>>  > -    unsigned long pclk_rate = dsi_get_pclk_rate(msm_host, 
>>> is_bonded_dsi);
>>>  > +    unsigned long pclk_rate = dsi_get_pclk_rate(mode, 
>>> is_bonded_dsi);
>>>  >      u64 pclk_bpp = (u64)pclk_rate * bpp;
>>>
>>>
>>>>       if (lanes == 0) {
>>>>           pr_err("%s: forcing mdss_dsi lanes to 1\n", __func__);
>>>> @@ -606,8 +611,14 @@ static void dsi_calc_pclk(struct msm_dsi_host 
>>>> *msm_host, bool is_bonded_dsi)
>>>>       else
>>>>           do_div(pclk_bpp, (8 * lanes));
>>>> -    msm_host->pixel_clk_rate = pclk_rate;
>>>> -    msm_host->byte_clk_rate = pclk_bpp;
>>>> +    return pclk_bpp;
>>>> +}
>>>> +
>>>> +static void dsi_calc_pclk(struct msm_dsi_host *msm_host, bool 
>>>> is_bonded_dsi)
>>>> +{
>>>> +    msm_host->pixel_clk_rate = dsi_get_pclk_rate(msm_host->mode, 
>>>> is_bonded_dsi);
>>>> +    msm_host->byte_clk_rate = 
>>>> dsi_byte_clk_get_rate(&msm_host->base, is_bonded_dsi,
>>>> +            msm_host->mode);
>>>>       DBG("pclk=%lu, bclk=%lu", msm_host->pixel_clk_rate,
>>>>                   msm_host->byte_clk_rate);
>>>> @@ -635,7 +646,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host 
>>>> *msm_host, bool is_bonded_dsi)
>>>>       dsi_calc_pclk(msm_host, is_bonded_dsi);
>>>> -    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host, is_bonded_dsi) * bpp;
>>>> +    pclk_bpp = (u64)dsi_get_pclk_rate(msm_host->mode, 
>>>> is_bonded_dsi) * bpp;
>>>>       do_div(pclk_bpp, 8);
>>>>       msm_host->src_clk_rate = pclk_bpp;
>>>
>>>
>>> Following my previous feedback:
>>>
>>> I think at this moment msm_host->src_clk_rate = 
>>> msm_host->byte_clk_rate * msm_host->lanes. If so, we can drop 
>>> dsi_get_pclk_rate() call and the multiply/do_div calculation and use 
>>> the above formula instead.
>>>
>>
>>  From what I see msm_host->src_clk_rate = pixel_clk * bpp / 8;
> 
> and msm_host->byte_clk_rate = pixel_clk * bpp / (8 * nlanes);
> 
>>
>>  From where did you get the above formula?
>>
>> I just felt that by having two APIs the next patch becomes easier 
>> because I need to just invoke the API which calculates byte clk.
> 
> Yes, I see that, it looks like a correct approach. You know, let's 
> ignore the dsi_calc_clk_rate_v2() for now, it is definitely a separate 
> change.
> 
> Could you please drop the opp_table handling from patch 2/2, move 
> assignments in dsi_byte_clk_get_rate() back to the definition lines and 
> then send it as v2?
> 

Ack, will move the assignments to definition lines. Regarding the patch 
2/2, I have responded with my concerns.

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Dmitry Baryshkov]

On 10/01/2023 04:40, Abhinav Kumar wrote:
> 
> 
> On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
>> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>> Currently there is no protection against a user trying to set
>>>> an unsupported mode on DSI. Implement a check based on the opp
>>>> table whether the byte clock for the mode can be supported by
>>>> validating whether an opp table entry exists.
>>>>
>>>> For devices which have not added opp table support yet, skip
>>>> this check otherwise it will break bootup on those devices.
>>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>>> Reported-by: Rob Clark <robdclark@gmail.com>
>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>>   1 file changed, 23 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> index 3a1417397283..87b518c42965 100644
>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>>> +    struct dev_pm_opp *opp;
>>>> +    struct opp_table *opp_tbl;
>>>> +    unsigned long byte_clk_rate;
>>>> +
>>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), 
>>>> mode);
>>>> +
>>>> +    /*
>>>> +     * first check if there is an opp table available for the 
>>>> calculated
>>>> +     * byte clock and then check DSC related info. Some devices 
>>>> have not
>>>> +     * added support for OPP table. Skip the check for those.
>>>> +     */
>>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>>
>>> Can we store the table inside the msm_dsi during the init? Then we 
>>> won't have to get it again and again during each mode_valid call.
>>
>> I checked other drivers. I think we can skip the get_opp_table 
>> completely, can we not? Just handle ENODEV returned from 
>> dev_pm_opp_find_freq_ceil().
>>
> 
> Your point is valid but I had a doubt on that API.
> 
> As per the documentation of that API, it says
> 
> 639  * Return: matching *opp and refreshes *freq accordingly, else returns
> 640  * ERR_PTR in case of error and should be handled using IS_ERR. 
> Error return
> 641  * values can be:
> 642  * EINVAL:    for bad pointer
> 643  * ERANGE:    no match found for search
> 644  * ENODEV:    if device not found in list of registered devices
> 645  *
> 646  * The callers are required to call dev_pm_opp_put() for the 
> returned OPP after
> 647  * use.
> 648  */
> 649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
> 650                          unsigned long *freq)
> 651 {
> 
> So ideally yes, ENODEV means that table was not found but .... that API 
> uses _find_opp_table under the hood.
> 
> which says
> 
> 79  * Return: pointer to 'struct opp_table' if found, otherwise -ENODEV or
> 80  * -EINVAL based on type of error.
> 81  *
> 82  * The callers must call dev_pm_opp_put_opp_table() after the table 
> is used.
> 
> Now, how would we know if the failure was due to table not found OR 
> entry not found.
> 
> Table now found means that SOC has probably not started using OPP table 
> which is alright and not an error.
> 
> But EINVAL could mean an entry not found which means it exceeds the opp 
> table limits.

I think this would be -ERANGE as documented.

EINVAL means dev is null or something of the same kind.

> 
> So there was some ambiguity on this. So I broke it down into two calls.
> 
> If my concern is invalid, let me know.
> 
> But I do agree with you that we can cache the opp table once rather than 
> doing it in every mode_valid().
> 
>>>
>>>> +    if (opp_tbl) {
>>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
>>>> +        if (IS_ERR(opp)) {
>>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>>> +                    byte_clk_rate, PTR_ERR(opp));
>>>> +            return PTR_ERR(opp);
>>>> +        }
>>>> +        dev_pm_opp_put(opp);
>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>> +    }
>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>   }
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

On 1/9/2023 6:47 PM, Dmitry Baryshkov wrote:
> On 10/01/2023 04:40, Abhinav Kumar wrote:
>>
>>
>> On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
>>> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>>> Currently there is no protection against a user trying to set
>>>>> an unsupported mode on DSI. Implement a check based on the opp
>>>>> table whether the byte clock for the mode can be supported by
>>>>> validating whether an opp table entry exists.
>>>>>
>>>>> For devices which have not added opp table support yet, skip
>>>>> this check otherwise it will break bootup on those devices.
>>>>>
>>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>>>> Reported-by: Rob Clark <robdclark@gmail.com>
>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>>>   1 file changed, 23 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> index 3a1417397283..87b518c42965 100644
>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>>>> +    struct dev_pm_opp *opp;
>>>>> +    struct opp_table *opp_tbl;
>>>>> +    unsigned long byte_clk_rate;
>>>>> +
>>>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), 
>>>>> mode);
>>>>> +
>>>>> +    /*
>>>>> +     * first check if there is an opp table available for the 
>>>>> calculated
>>>>> +     * byte clock and then check DSC related info. Some devices 
>>>>> have not
>>>>> +     * added support for OPP table. Skip the check for those.
>>>>> +     */
>>>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>>>
>>>> Can we store the table inside the msm_dsi during the init? Then we 
>>>> won't have to get it again and again during each mode_valid call.
>>>
>>> I checked other drivers. I think we can skip the get_opp_table 
>>> completely, can we not? Just handle ENODEV returned from 
>>> dev_pm_opp_find_freq_ceil().
>>>
>>
>> Your point is valid but I had a doubt on that API.
>>
>> As per the documentation of that API, it says
>>
>> 639  * Return: matching *opp and refreshes *freq accordingly, else 
>> returns
>> 640  * ERR_PTR in case of error and should be handled using IS_ERR. 
>> Error return
>> 641  * values can be:
>> 642  * EINVAL:    for bad pointer
>> 643  * ERANGE:    no match found for search
>> 644  * ENODEV:    if device not found in list of registered devices
>> 645  *
>> 646  * The callers are required to call dev_pm_opp_put() for the 
>> returned OPP after
>> 647  * use.
>> 648  */
>> 649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>> 650                          unsigned long *freq)
>> 651 {
>>
>> So ideally yes, ENODEV means that table was not found but .... that 
>> API uses _find_opp_table under the hood.
>>
>> which says
>>
>> 79  * Return: pointer to 'struct opp_table' if found, otherwise 
>> -ENODEV or
>> 80  * -EINVAL based on type of error.
>> 81  *
>> 82  * The callers must call dev_pm_opp_put_opp_table() after the table 
>> is used.
>>
>> Now, how would we know if the failure was due to table not found OR 
>> entry not found.
>>
>> Table now found means that SOC has probably not started using OPP 
>> table which is alright and not an error.
>>
>> But EINVAL could mean an entry not found which means it exceeds the 
>> opp table limits.
> 
> I think this would be -ERANGE as documented.
> 
> EINVAL means dev is null or something of the same kind.
> 

Okay, so EINVAL and ERANGE are genuine errors but ENODEV is not in our 
case as all SOCs might not have opp table yet.

Do you still think selective handling of these two errors is better than 
the current implementation? That way its separation is clear.

>>
>> So there was some ambiguity on this. So I broke it down into two calls.
>>
>> If my concern is invalid, let me know.
>>
>> But I do agree with you that we can cache the opp table once rather 
>> than doing it in every mode_valid().
>>
>>>>
>>>>> +    if (opp_tbl) {
>>>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
>>>>> +        if (IS_ERR(opp)) {
>>>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>>>> +                    byte_clk_rate, PTR_ERR(opp));
>>>>> +            return PTR_ERR(opp);
>>>>> +        }
>>>>> +        dev_pm_opp_put(opp);
>>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>>> +    }
>>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>>   }
>>>>
>>>
> 

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Dmitry Baryshkov]

On 10/01/2023 04:56, Abhinav Kumar wrote:
> 
> 
> On 1/9/2023 6:47 PM, Dmitry Baryshkov wrote:
>> On 10/01/2023 04:40, Abhinav Kumar wrote:
>>>
>>>
>>> On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
>>>> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>>>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>>>> Currently there is no protection against a user trying to set
>>>>>> an unsupported mode on DSI. Implement a check based on the opp
>>>>>> table whether the byte clock for the mode can be supported by
>>>>>> validating whether an opp table entry exists.
>>>>>>
>>>>>> For devices which have not added opp table support yet, skip
>>>>>> this check otherwise it will break bootup on those devices.
>>>>>>
>>>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>>>>> Reported-by: Rob Clark <robdclark@gmail.com>
>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>>>>   1 file changed, 23 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>> index 3a1417397283..87b518c42965 100644
>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>>>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>>>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>>>>> +    struct dev_pm_opp *opp;
>>>>>> +    struct opp_table *opp_tbl;
>>>>>> +    unsigned long byte_clk_rate;
>>>>>> +
>>>>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), 
>>>>>> mode);
>>>>>> +
>>>>>> +    /*
>>>>>> +     * first check if there is an opp table available for the 
>>>>>> calculated
>>>>>> +     * byte clock and then check DSC related info. Some devices 
>>>>>> have not
>>>>>> +     * added support for OPP table. Skip the check for those.
>>>>>> +     */
>>>>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>>>>
>>>>> Can we store the table inside the msm_dsi during the init? Then we 
>>>>> won't have to get it again and again during each mode_valid call.
>>>>
>>>> I checked other drivers. I think we can skip the get_opp_table 
>>>> completely, can we not? Just handle ENODEV returned from 
>>>> dev_pm_opp_find_freq_ceil().
>>>>
>>>
>>> Your point is valid but I had a doubt on that API.
>>>
>>> As per the documentation of that API, it says
>>>
>>> 639  * Return: matching *opp and refreshes *freq accordingly, else 
>>> returns
>>> 640  * ERR_PTR in case of error and should be handled using IS_ERR. 
>>> Error return
>>> 641  * values can be:
>>> 642  * EINVAL:    for bad pointer
>>> 643  * ERANGE:    no match found for search
>>> 644  * ENODEV:    if device not found in list of registered devices
>>> 645  *
>>> 646  * The callers are required to call dev_pm_opp_put() for the 
>>> returned OPP after
>>> 647  * use.
>>> 648  */
>>> 649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>>> 650                          unsigned long *freq)
>>> 651 {
>>>
>>> So ideally yes, ENODEV means that table was not found but .... that 
>>> API uses _find_opp_table under the hood.
>>>
>>> which says
>>>
>>> 79  * Return: pointer to 'struct opp_table' if found, otherwise 
>>> -ENODEV or
>>> 80  * -EINVAL based on type of error.
>>> 81  *
>>> 82  * The callers must call dev_pm_opp_put_opp_table() after the 
>>> table is used.
>>>
>>> Now, how would we know if the failure was due to table not found OR 
>>> entry not found.
>>>
>>> Table now found means that SOC has probably not started using OPP 
>>> table which is alright and not an error.
>>>
>>> But EINVAL could mean an entry not found which means it exceeds the 
>>> opp table limits.
>>
>> I think this would be -ERANGE as documented.
>>
>> EINVAL means dev is null or something of the same kind.
>>
> 
> Okay, so EINVAL and ERANGE are genuine errors but ENODEV is not in our 
> case as all SOCs might not have opp table yet.
> 
> Do you still think selective handling of these two errors is better than 
> the current implementation? That way its separation is clear.

I think that we should drop the opp_table handling.
Then select basing on the returned error:
  ERANGE => MODE_CLOCK_HIGH
  EINVAL and any other error => MODE_ERROR
  ENODEV => skip the check, continue with dsc

See below.

> 
>>>
>>> So there was some ambiguity on this. So I broke it down into two calls.
>>>
>>> If my concern is invalid, let me know.
>>>
>>> But I do agree with you that we can cache the opp table once rather 
>>> than doing it in every mode_valid().
>>>
>>>>>
>>>>>> +    if (opp_tbl) {
>>>>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
>>>>>> +        if (IS_ERR(opp)) {
>>>>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>>>>> +                    byte_clk_rate, PTR_ERR(opp));
>>>>>> +            return PTR_ERR(opp);

Note: mode_valid returns `enum drm_mode_status`, so we can not return 
PTR_ERR here.

>>>>>> +        }
>>>>>> +        dev_pm_opp_put(opp);
>>>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>>>> +    }
>>>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>>>   }
>>>>>
>>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

On 1/9/2023 7:00 PM, Dmitry Baryshkov wrote:
> On 10/01/2023 04:56, Abhinav Kumar wrote:
>>
>>
>> On 1/9/2023 6:47 PM, Dmitry Baryshkov wrote:
>>> On 10/01/2023 04:40, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 1/9/2023 5:19 PM, Dmitry Baryshkov wrote:
>>>>> On 27/10/2022 20:36, Dmitry Baryshkov wrote:
>>>>>> On 22/09/2022 03:49, Abhinav Kumar wrote:
>>>>>>> Currently there is no protection against a user trying to set
>>>>>>> an unsupported mode on DSI. Implement a check based on the opp
>>>>>>> table whether the byte clock for the mode can be supported by
>>>>>>> validating whether an opp table entry exists.
>>>>>>>
>>>>>>> For devices which have not added opp table support yet, skip
>>>>>>> this check otherwise it will break bootup on those devices.
>>>>>>>
>>>>>>> Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
>>>>>>> Reported-by: Rob Clark <robdclark@gmail.com>
>>>>>>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/msm/dsi/dsi_manager.c | 23 +++++++++++++++++++++++
>>>>>>>   1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c 
>>>>>>> b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> index 3a1417397283..87b518c42965 100644
>>>>>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>>>>>> @@ -450,6 +450,29 @@ static enum drm_mode_status 
>>>>>>> dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>>>       int id = dsi_mgr_bridge_get_id(bridge);
>>>>>>>       struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>>>>>>       struct mipi_dsi_host *host = msm_dsi->host;
>>>>>>> +    struct platform_device *pdev = msm_dsi->pdev;
>>>>>>> +    struct dev_pm_opp *opp;
>>>>>>> +    struct opp_table *opp_tbl;
>>>>>>> +    unsigned long byte_clk_rate;
>>>>>>> +
>>>>>>> +    byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), 
>>>>>>> mode);
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * first check if there is an opp table available for the 
>>>>>>> calculated
>>>>>>> +     * byte clock and then check DSC related info. Some devices 
>>>>>>> have not
>>>>>>> +     * added support for OPP table. Skip the check for those.
>>>>>>> +     */
>>>>>>> +    opp_tbl = dev_pm_opp_get_opp_table(&pdev->dev);
>>>>>>
>>>>>> Can we store the table inside the msm_dsi during the init? Then we 
>>>>>> won't have to get it again and again during each mode_valid call.
>>>>>
>>>>> I checked other drivers. I think we can skip the get_opp_table 
>>>>> completely, can we not? Just handle ENODEV returned from 
>>>>> dev_pm_opp_find_freq_ceil().
>>>>>
>>>>
>>>> Your point is valid but I had a doubt on that API.
>>>>
>>>> As per the documentation of that API, it says
>>>>
>>>> 639  * Return: matching *opp and refreshes *freq accordingly, else 
>>>> returns
>>>> 640  * ERR_PTR in case of error and should be handled using IS_ERR. 
>>>> Error return
>>>> 641  * values can be:
>>>> 642  * EINVAL:    for bad pointer
>>>> 643  * ERANGE:    no match found for search
>>>> 644  * ENODEV:    if device not found in list of registered devices
>>>> 645  *
>>>> 646  * The callers are required to call dev_pm_opp_put() for the 
>>>> returned OPP after
>>>> 647  * use.
>>>> 648  */
>>>> 649 struct dev_pm_opp *dev_pm_opp_find_freq_ceil(struct device *dev,
>>>> 650                          unsigned long *freq)
>>>> 651 {
>>>>
>>>> So ideally yes, ENODEV means that table was not found but .... that 
>>>> API uses _find_opp_table under the hood.
>>>>
>>>> which says
>>>>
>>>> 79  * Return: pointer to 'struct opp_table' if found, otherwise 
>>>> -ENODEV or
>>>> 80  * -EINVAL based on type of error.
>>>> 81  *
>>>> 82  * The callers must call dev_pm_opp_put_opp_table() after the 
>>>> table is used.
>>>>
>>>> Now, how would we know if the failure was due to table not found OR 
>>>> entry not found.
>>>>
>>>> Table now found means that SOC has probably not started using OPP 
>>>> table which is alright and not an error.
>>>>
>>>> But EINVAL could mean an entry not found which means it exceeds the 
>>>> opp table limits.
>>>
>>> I think this would be -ERANGE as documented.
>>>
>>> EINVAL means dev is null or something of the same kind.
>>>
>>
>> Okay, so EINVAL and ERANGE are genuine errors but ENODEV is not in our 
>> case as all SOCs might not have opp table yet.
>>
>> Do you still think selective handling of these two errors is better 
>> than the current implementation? That way its separation is clear.
> 
> I think that we should drop the opp_table handling.
> Then select basing on the returned error:
>   ERANGE => MODE_CLOCK_HIGH
>   EINVAL and any other error => MODE_ERROR
>   ENODEV => skip the check, continue with dsc
> 
> See below.
> 

Ack, lets do it this way.

I am clear now, will spin a v2.

>>
>>>>
>>>> So there was some ambiguity on this. So I broke it down into two calls.
>>>>
>>>> If my concern is invalid, let me know.
>>>>
>>>> But I do agree with you that we can cache the opp table once rather 
>>>> than doing it in every mode_valid().
>>>>
>>>>>>
>>>>>>> +    if (opp_tbl) {
>>>>>>> +        opp = dev_pm_opp_find_freq_ceil(&pdev->dev, 
>>>>>>> &byte_clk_rate);
>>>>>>> +        if (IS_ERR(opp)) {
>>>>>>> +            pr_err("opp table not found for freq %lu err: %ld\n",
>>>>>>> +                    byte_clk_rate, PTR_ERR(opp));
>>>>>>> +            return PTR_ERR(opp);
> 
> Note: mode_valid returns `enum drm_mode_status`, so we can not return 
> PTR_ERR here.
> 
>>>>>>> +        }
>>>>>>> +        dev_pm_opp_put(opp);
>>>>>>> +        dev_pm_opp_put_opp_table(opp_tbl);
>>>>>>> +    }
>>>>>>>       return msm_dsi_host_check_dsc(host, mode);
>>>>>>>   }
>>>>>>
>>>>>
>>>
> 

[PATCH 2/2] drm/msm/dsi: implement opp table based check for dsi_mgr_bridge_mode_valid()

[Abhinav Kumar]

Currently there is no protection against a user trying to set
an unsupported mode on DSI. Implement a check based on the opp
table whether the byte clock for the mode can be supported by
validating whether an opp table entry exists.

For devices which have not added opp table support yet, skip
this check otherwise it will break bootup on those devices.

changes in v2:
	- drop dev_pm_opp_get_opp_table() usage

Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/15
Reported-by: Rob Clark <robdclark@gmail.com>
Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/dsi/dsi_manager.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
index 3a1417397283..c4c24dabfd6f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
@@ -450,6 +450,31 @@ static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
 	int id = dsi_mgr_bridge_get_id(bridge);
 	struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
 	struct mipi_dsi_host *host = msm_dsi->host;
+	struct platform_device *pdev = msm_dsi->pdev;
+	struct dev_pm_opp *opp;
+	unsigned long byte_clk_rate;
+
+	byte_clk_rate = dsi_byte_clk_get_rate(host, IS_BONDED_DSI(), mode);
+
+	/*
+	 * If dev_pm_opp_find_freq_ceil() returns -EINVAL, its a bad
+	 * pointer being passed, so treat as an error and return MODE_ERROR
+	 *
+	 * If dev_pm_opp_find_freq_ceil() returns -ERANGE, no clock
+	 * was found matching the byte_clk, so return MODE_CLOCK_RANGE
+	 *
+	 * If dev_pm_opp_find_freq_ceil() returns -ENODEV, don't treat
+	 * it as an error as it could mean opp table is not implemented
+	 */
+	opp = dev_pm_opp_find_freq_ceil(&pdev->dev, &byte_clk_rate);
+	if (IS_ERR(opp)) {
+		if (PTR_ERR(opp) == -EINVAL)
+			return MODE_ERROR;
+		else if (PTR_ERR(opp) == -ERANGE)
+			return MODE_CLOCK_RANGE;
+	} else {
+		dev_pm_opp_put(opp);
+	}
 
 	return msm_dsi_host_check_dsc(host, mode);
 }
-- 
2.39.0