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

[Jayesh Choudhary <j-choudhary@ti.com>]

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
>>
>