diff mbox series

[v2,2/2] drm/bridge: ti-sn65dsi86: Fix ti_sn_bridge_set_dsi_rate function

Message ID 20240618081418.250953-3-j-choudhary@ti.com (mailing list archive)
State New, archived
Headers show
Series SN65DSI86 minor fixes | expand

Commit Message

Jayesh Choudhary June 18, 2024, 8:14 a.m. UTC 
During code inspection, it was found that due to integer calculations,
the rounding off can cause errors in the final value propagated in the
registers.
Considering the example of 1080p (very common resolution), the mode->clock
is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
clock frequency would come as 444 when we are expecting the value 445.5
which would reflect in SN_DSIA_CLK_FREQ_REG.
So move the division to be the last operation where rounding off will not
impact the register value.

Fixes: a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver")
Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Dmitry Baryshkov June 18, 2024, 9:03 a.m. UTC | #1 
On Tue, Jun 18, 2024 at 01:44:18PM GMT, Jayesh Choudhary wrote:
> During code inspection, it was found that due to integer calculations,
> the rounding off can cause errors in the final value propagated in the
> registers.
> Considering the example of 1080p (very common resolution), the mode->clock
> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> clock frequency would come as 444 when we are expecting the value 445.5
> which would reflect in SN_DSIA_CLK_FREQ_REG.
> So move the division to be the last operation where rounding off will not
> impact the register value.

Should this division use DIV_ROUND_UP instead? DIV_ROUND_CLOSEST?

> 
> Fixes: a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver")
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>

Fixes should go before feature patches. Please change the order of you
patches for the next submission.

> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index d13b42d7c512..5bf12af6b657 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -111,8 +111,6 @@
>  #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
>  #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
>  
> -#define MIN_DSI_CLK_FREQ_MHZ	40
> -
>  /*
>   * NOTE: DSI clock frequency range: [40MHz,755MHz)
>   * DSI clock frequency range is in 5-MHz increments
> @@ -1219,19 +1217,21 @@ static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
>  {
>  	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>  	struct drm_display_mode *mode = &crtc_state->mode;
> -	unsigned int bit_rate_mhz, clk_freq_mhz;
> +	unsigned int bit_rate_khz;
>  
>  	/* Pixel clock check */
>  	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
>  		return -EINVAL;
>  
> -	bit_rate_mhz = (mode->clock / 1000) *
> +	bit_rate_khz = mode->clock *
>  			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> -	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>  
> -	/* for each increment in dsi_clk_range, frequency increases by 5MHz */
> -	pdata->dsi_clk_range = (MIN_DSI_CLK_FREQ_MHZ / 5) +
> -		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
> +	/*
> +	 * For each increment in dsi_clk_range, frequency increases by 5MHz
> +	 * and the factor of 1000 comes from kHz to MHz conversion
> +	 */
> +	pdata->dsi_clk_range = (bit_rate_khz /
> +				(pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
>  
>  	/* SN_DSIA_CLK_FREQ_REG check */
>  	if (pdata->dsi_clk_range > MAX_DSI_CLK_RANGE ||
> -- 
> 2.25.1
>
Jayesh Choudhary June 18, 2024, 10:04 a.m. UTC | #2 
Hello Dmitry,

On 18/06/24 14:33, Dmitry Baryshkov wrote:
> On Tue, Jun 18, 2024 at 01:44:18PM GMT, Jayesh Choudhary wrote:
>> During code inspection, it was found that due to integer calculations,
>> the rounding off can cause errors in the final value propagated in the
>> registers.
>> Considering the example of 1080p (very common resolution), the mode->clock
>> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
>> clock frequency would come as 444 when we are expecting the value 445.5
>> which would reflect in SN_DSIA_CLK_FREQ_REG.
>> So move the division to be the last operation where rounding off will not
>> impact the register value.
> 
> Should this division use DIV_ROUND_UP instead? DIV_ROUND_CLOSEST?
> 

Floor of the final value is expected according to datasheet.
The error was due to taking floor earlier and then error propagation
due to multiplication later on.
I think we can come up with a case when DIV_ROUND_UP can also give this
error. So this particular approach seemed okay to me.

>>
>> Fixes: a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver")
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> 
> Fixes should go before feature patches. Please change the order of you
> patches for the next submission.

Okay. this was supposed to be code snippet movement in the first patch 
and fix in the second patch as suggested in v1:
https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801

I can fix it in next revision.

Thanks,
Jayesh

> 
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index d13b42d7c512..5bf12af6b657 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -111,8 +111,6 @@
>>   #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
>>   #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
>>   
>> -#define MIN_DSI_CLK_FREQ_MHZ	40
>> -
>>   /*
>>    * NOTE: DSI clock frequency range: [40MHz,755MHz)
>>    * DSI clock frequency range is in 5-MHz increments
>> @@ -1219,19 +1217,21 @@ static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
>>   {
>>   	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
>>   	struct drm_display_mode *mode = &crtc_state->mode;
>> -	unsigned int bit_rate_mhz, clk_freq_mhz;
>> +	unsigned int bit_rate_khz;
>>   
>>   	/* Pixel clock check */
>>   	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
>>   		return -EINVAL;
>>   
>> -	bit_rate_mhz = (mode->clock / 1000) *
>> +	bit_rate_khz = mode->clock *
>>   			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> -	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>>   
>> -	/* for each increment in dsi_clk_range, frequency increases by 5MHz */
>> -	pdata->dsi_clk_range = (MIN_DSI_CLK_FREQ_MHZ / 5) +
>> -		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
>> +	/*
>> +	 * For each increment in dsi_clk_range, frequency increases by 5MHz
>> +	 * and the factor of 1000 comes from kHz to MHz conversion
>> +	 */
>> +	pdata->dsi_clk_range = (bit_rate_khz /
>> +				(pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
>>   
>>   	/* SN_DSIA_CLK_FREQ_REG check */
>>   	if (pdata->dsi_clk_range > MAX_DSI_CLK_RANGE ||
>> -- 
>> 2.25.1
>>
>
Dmitry Baryshkov June 18, 2024, 10:09 a.m. UTC | #3 
On Tue, 18 Jun 2024 at 13:05, Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Hello Dmitry,
>
> On 18/06/24 14:33, Dmitry Baryshkov wrote:
> > On Tue, Jun 18, 2024 at 01:44:18PM GMT, Jayesh Choudhary wrote:
> >> During code inspection, it was found that due to integer calculations,
> >> the rounding off can cause errors in the final value propagated in the
> >> registers.
> >> Considering the example of 1080p (very common resolution), the mode->clock
> >> is 148500, dsi->lanes = 4, and bpp = 24, with the previous logic, the DSI
> >> clock frequency would come as 444 when we are expecting the value 445.5
> >> which would reflect in SN_DSIA_CLK_FREQ_REG.
> >> So move the division to be the last operation where rounding off will not
> >> impact the register value.
> >
> > Should this division use DIV_ROUND_UP instead? DIV_ROUND_CLOSEST?
> >
>
> Floor of the final value is expected according to datasheet.
> The error was due to taking floor earlier and then error propagation
> due to multiplication later on.
> I think we can come up with a case when DIV_ROUND_UP can also give this
> error. So this particular approach seemed okay to me.

Ack

>
> >>
> >> Fixes: a095f15c00e2 ("drm/bridge: add support for sn65dsi86 bridge driver")
> >> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> >
> > Fixes should go before feature patches. Please change the order of you
> > patches for the next submission.
>
> Okay. this was supposed to be code snippet movement in the first patch
> and fix in the second patch as suggested in v1:
> https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801

My point is pretty simple: fixes are backported to the earlier
kernels. non-fixing commits are not. In your patchset you have added a
dependency from the fix onto a non-fix (and
not-selected-for-backporting) patch, which is not so good.

>
> I can fix it in next revision.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index d13b42d7c512..5bf12af6b657 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -111,8 +111,6 @@ 
 #define  AUX_IRQ_STATUS_AUX_SHORT		BIT(5)
 #define  AUX_IRQ_STATUS_NAT_I2C_FAIL		BIT(6)
 
-#define MIN_DSI_CLK_FREQ_MHZ	40
-
 /*
  * NOTE: DSI clock frequency range: [40MHz,755MHz)
  * DSI clock frequency range is in 5-MHz increments
@@ -1219,19 +1217,21 @@  static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 	struct drm_display_mode *mode = &crtc_state->mode;
-	unsigned int bit_rate_mhz, clk_freq_mhz;
+	unsigned int bit_rate_khz;
 
 	/* Pixel clock check */
 	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
 		return -EINVAL;
 
-	bit_rate_mhz = (mode->clock / 1000) *
+	bit_rate_khz = mode->clock *
 			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
-	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
 
-	/* for each increment in dsi_clk_range, frequency increases by 5MHz */
-	pdata->dsi_clk_range = (MIN_DSI_CLK_FREQ_MHZ / 5) +
-		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
+	/*
+	 * For each increment in dsi_clk_range, frequency increases by 5MHz
+	 * and the factor of 1000 comes from kHz to MHz conversion
+	 */
+	pdata->dsi_clk_range = (bit_rate_khz /
+				(pdata->dsi->lanes * 2 * 1000 * 5)) & 0xFF;
 
 	/* SN_DSIA_CLK_FREQ_REG check */
 	if (pdata->dsi_clk_range > MAX_DSI_CLK_RANGE ||