[PATCH v2 0/2] SN65DSI86 minor fixes

[PATCH v2 0/2] SN65DSI86 minor fixes

[Jayesh Choudhary]

Hello All,

These 2 patches add the atomic check hook for sn65dsi86 bridge and
does a minor math fix for dsi rate calculation.

According to the datasheet[0], for the max resolution, it says:
  Suitable for 60 fps 4K 4096 x 2304 resolution at 18
  bpp color, and WUXGA 1920 x 1200 resolution
  with 3D graphics at 60 fps (120 fps equivalent)

A very usual clock frequency for 4K@60fps resolution is 594MHz.
So keeping the max value supported by the bridge as 600MHz for
safe check.

DSI clock frequency range check are as per datasheet[0].

Changelog v1->v2:
- Check the value in atomic_check hook
- Fix the "Fixes" tag
- Fix MAX_DSI_CLK_RANGE to reflect actual supported value
- Add mode_clock check to ensure that the bit_rate_khz variable
  does not overflow instead of justifying by reverse calculation
  in comments.
- Fix commit message to show that the math uissue was found during
  code inspection.

v1 patch:
<https://lore.kernel.org/all/20240408073623.186489-1-j-choudhary@ti.com/>

[0]: <https://www.ti.com/lit/gpn/sn65dsi86>

Jayesh Choudhary (2):
  drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge
  drm/bridge: ti-sn65dsi86: Fix ti_sn_bridge_set_dsi_rate function

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 67 +++++++++++++++++++--------
 1 file changed, 47 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Jayesh Choudhary]

Add the atomic_check hook to ensure that the parameters are within the
valid range.
As of now, dsi clock freqency is being calculated in bridge_enable but
this needs to be checked in atomic_check which is called before
bridge_enable so move this calculation to atomic_check and write the
register value in bridge_enable as it is.

For now, add mode clock check for the max resolution supported by the
bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
check for SN_DSIA_CLK_FREQ_REG.
According to the datasheet[0], the minimum value for that reg is 0x08
and the maximum value is 0x96. So add check for that.

[0]: <https://www.ti.com/lit/gpn/sn65dsi86>

Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
 1 file changed, 46 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 84698a0b27a8..d13b42d7c512 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -113,6 +113,20 @@
 
 #define MIN_DSI_CLK_FREQ_MHZ	40
 
+/*
+ * NOTE: DSI clock frequency range: [40MHz,755MHz)
+ * DSI clock frequency range is in 5-MHz increments
+ * So [40MHz,45MHz) translates to 0x08 (min value)
+ * And [750MHz,755MHz) translates to 0x96 (max value)
+ */
+#define MIN_DSI_CLK_RANGE	0x8
+#define MAX_DSI_CLK_RANGE	0x96
+
+/* Pixel clock to support max resolution (4K@60Hz) supported
+ * by the bridge.
+ */
+#define SN65DSI86_MAX_PIXEL_CLOCK_KHZ 600000
+
 /* fudge factor required to account for 8b/10b encoding */
 #define DP_CLK_FUDGE_NUM	10
 #define DP_CLK_FUDGE_DEN	8
@@ -191,6 +205,7 @@ struct ti_sn65dsi86 {
 	u8				ln_polrs;
 	bool				comms_enabled;
 	struct mutex			comms_mutex;
+	u32				dsi_clk_range;
 
 #if defined(CONFIG_OF_GPIO)
 	struct gpio_chip		gchip;
@@ -820,24 +835,6 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 }
 
-static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
-{
-	unsigned int bit_rate_mhz, clk_freq_mhz;
-	unsigned int val;
-	struct drm_display_mode *mode =
-		&pdata->bridge.encoder->crtc->state->adjusted_mode;
-
-	/* set DSIA clk frequency */
-	bit_rate_mhz = (mode->clock / 1000) *
-			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
-	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
-
-	/* for each increment in val, frequency increases by 5MHz */
-	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
-		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
-	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
-}
-
 static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
 {
 	if (connector->display_info.bpc <= 6)
@@ -1104,7 +1101,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
 			   pdata->ln_polrs << LN_POLRS_OFFSET);
 
 	/* set dsi clk frequency value */
-	ti_sn_bridge_set_dsi_rate(pdata);
+	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, pdata->dsi_clk_range);
 
 	/*
 	 * The SN65DSI86 only supports ASSR Display Authentication method and
@@ -1215,6 +1212,35 @@ static const struct drm_edid *ti_sn_bridge_edid_read(struct drm_bridge *bridge,
 	return drm_edid_read_ddc(connector, &pdata->aux.ddc);
 }
 
+static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
+				     struct drm_bridge_state *bridge_state,
+				     struct drm_crtc_state *crtc_state,
+				     struct drm_connector_state *conn_state)
+{
+	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;
+
+	/* Pixel clock check */
+	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
+		return -EINVAL;
+
+	bit_rate_mhz = (mode->clock / 1000) *
+			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);
+
+	/* SN_DSIA_CLK_FREQ_REG check */
+	if (pdata->dsi_clk_range > MAX_DSI_CLK_RANGE ||
+	    pdata->dsi_clk_range < MIN_DSI_CLK_RANGE)
+		return -EINVAL;
+
+	return 0;
+}
+
 static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.attach = ti_sn_bridge_attach,
 	.detach = ti_sn_bridge_detach,
@@ -1228,6 +1254,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_check = ti_sn_bridge_atomic_check,
 };
 
 static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
-- 
2.25.1

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

[Jayesh Choudhary]

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

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

Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Dmitry Baryshkov]

On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
> Add the atomic_check hook to ensure that the parameters are within the
> valid range.
> As of now, dsi clock freqency is being calculated in bridge_enable but
> this needs to be checked in atomic_check which is called before
> bridge_enable so move this calculation to atomic_check and write the
> register value in bridge_enable as it is.
> 
> For now, add mode clock check for the max resolution supported by the
> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
> check for SN_DSIA_CLK_FREQ_REG.
> According to the datasheet[0], the minimum value for that reg is 0x08
> and the maximum value is 0x96. So add check for that.
> 
> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
> 
> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
>  1 file changed, 46 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 84698a0b27a8..d13b42d7c512 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -113,6 +113,20 @@
>  
>  #define MIN_DSI_CLK_FREQ_MHZ	40
>  
> +/*
> + * NOTE: DSI clock frequency range: [40MHz,755MHz)
> + * DSI clock frequency range is in 5-MHz increments
> + * So [40MHz,45MHz) translates to 0x08 (min value)
> + * And [750MHz,755MHz) translates to 0x96 (max value)
> + */
> +#define MIN_DSI_CLK_RANGE	0x8
> +#define MAX_DSI_CLK_RANGE	0x96
> +
> +/* Pixel clock to support max resolution (4K@60Hz) supported
> + * by the bridge.
> + */
> +#define SN65DSI86_MAX_PIXEL_CLOCK_KHZ 600000
> +
>  /* fudge factor required to account for 8b/10b encoding */
>  #define DP_CLK_FUDGE_NUM	10
>  #define DP_CLK_FUDGE_DEN	8
> @@ -191,6 +205,7 @@ struct ti_sn65dsi86 {
>  	u8				ln_polrs;
>  	bool				comms_enabled;
>  	struct mutex			comms_mutex;
> +	u32				dsi_clk_range;
>  
>  #if defined(CONFIG_OF_GPIO)
>  	struct gpio_chip		gchip;
> @@ -820,24 +835,6 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge,
>  	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
>  }
>  
> -static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata)
> -{
> -	unsigned int bit_rate_mhz, clk_freq_mhz;
> -	unsigned int val;
> -	struct drm_display_mode *mode =
> -		&pdata->bridge.encoder->crtc->state->adjusted_mode;
> -
> -	/* set DSIA clk frequency */
> -	bit_rate_mhz = (mode->clock / 1000) *
> -			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> -	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> -
> -	/* for each increment in val, frequency increases by 5MHz */
> -	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
> -		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
> -	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> -}
> -
>  static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector)
>  {
>  	if (connector->display_info.bpc <= 6)
> @@ -1104,7 +1101,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge,
>  			   pdata->ln_polrs << LN_POLRS_OFFSET);
>  
>  	/* set dsi clk frequency value */
> -	ti_sn_bridge_set_dsi_rate(pdata);
> +	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, pdata->dsi_clk_range);
>  
>  	/*
>  	 * The SN65DSI86 only supports ASSR Display Authentication method and
> @@ -1215,6 +1212,35 @@ static const struct drm_edid *ti_sn_bridge_edid_read(struct drm_bridge *bridge,
>  	return drm_edid_read_ddc(connector, &pdata->aux.ddc);
>  }
>  
> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *bridge_state,
> +				     struct drm_crtc_state *crtc_state,
> +				     struct drm_connector_state *conn_state)
> +{
> +	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;
> +
> +	/* Pixel clock check */
> +	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
> +		return -EINVAL;
> +
> +	bit_rate_mhz = (mode->clock / 1000) *
> +			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);

atomic_check might be called several times, it might be called to test
the state. As such, it should not modify anything outside of the
state variables.

> +
> +	/* SN_DSIA_CLK_FREQ_REG check */
> +	if (pdata->dsi_clk_range > MAX_DSI_CLK_RANGE ||
> +	    pdata->dsi_clk_range < MIN_DSI_CLK_RANGE)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.attach = ti_sn_bridge_attach,
>  	.detach = ti_sn_bridge_detach,
> @@ -1228,6 +1254,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_check = ti_sn_bridge_atomic_check,
>  };
>  
>  static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
> -- 
> 2.25.1
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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
> 

-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Jayesh Choudhary]

Hello Dmitry,

Thanks for the review.

On 18/06/24 14:29, Dmitry Baryshkov wrote:
> On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
>> Add the atomic_check hook to ensure that the parameters are within the
>> valid range.
>> As of now, dsi clock freqency is being calculated in bridge_enable but
>> this needs to be checked in atomic_check which is called before
>> bridge_enable so move this calculation to atomic_check and write the
>> register value in bridge_enable as it is.
>>
>> For now, add mode clock check for the max resolution supported by the
>> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
>> check for SN_DSIA_CLK_FREQ_REG.
>> According to the datasheet[0], the minimum value for that reg is 0x08
>> and the maximum value is 0x96. So add check for that.
>>
>> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
>>
>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>> ---
>>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
>>   1 file changed, 46 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> index 84698a0b27a8..d13b42d7c512 100644
>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -113,6 +113,20 @@
>>   

[...]

>>   
>> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
>> +				     struct drm_bridge_state *bridge_state,
>> +				     struct drm_crtc_state *crtc_state,
>> +				     struct drm_connector_state *conn_state)
>> +{
>> +	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;
>> +
>> +	/* Pixel clock check */
>> +	if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
>> +		return -EINVAL;
>> +
>> +	bit_rate_mhz = (mode->clock / 1000) *
>> +			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);
> 
> atomic_check might be called several times, it might be called to test
> the state. As such, it should not modify anything outside of the
> state variables.
> 

If not in atomic_check, then where should I move this calculation and check?
mode_valid with returning MODE_BAD in case of failure?

I had to move it from bridge_enable based on the comments on v1:
https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801

Warm Regards,
Jayesh

[...]

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

[Jayesh Choudhary]

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

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

[Dmitry Baryshkov]

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.


-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Dmitry Baryshkov]

On Tue, 18 Jun 2024 at 12:56, Jayesh Choudhary <j-choudhary@ti.com> wrote:
>
> Hello Dmitry,
>
> Thanks for the review.
>
> On 18/06/24 14:29, Dmitry Baryshkov wrote:
> > On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
> >> Add the atomic_check hook to ensure that the parameters are within the
> >> valid range.
> >> As of now, dsi clock freqency is being calculated in bridge_enable but
> >> this needs to be checked in atomic_check which is called before
> >> bridge_enable so move this calculation to atomic_check and write the
> >> register value in bridge_enable as it is.
> >>
> >> For now, add mode clock check for the max resolution supported by the
> >> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
> >> check for SN_DSIA_CLK_FREQ_REG.
> >> According to the datasheet[0], the minimum value for that reg is 0x08
> >> and the maximum value is 0x96. So add check for that.
> >>
> >> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
> >>
> >> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
> >> ---
> >>   drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
> >>   1 file changed, 46 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> index 84698a0b27a8..d13b42d7c512 100644
> >> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> >> @@ -113,6 +113,20 @@
> >>
>
> [...]
>
> >>
> >> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
> >> +                                 struct drm_bridge_state *bridge_state,
> >> +                                 struct drm_crtc_state *crtc_state,
> >> +                                 struct drm_connector_state *conn_state)
> >> +{
> >> +    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;
> >> +
> >> +    /* Pixel clock check */
> >> +    if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
> >> +            return -EINVAL;
> >> +
> >> +    bit_rate_mhz = (mode->clock / 1000) *
> >> +                    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);
> >
> > atomic_check might be called several times, it might be called to test
> > the state. As such, it should not modify anything outside of the
> > state variables.
> >
>
> If not in atomic_check, then where should I move this calculation and check?
> mode_valid with returning MODE_BAD in case of failure?

I didn't write that it's the wrong place for math. I wrote that you
should not be modifying global structure.

So you have to subclass drm_bridge_state for the driver and store the
value there. Or just add a helper function and call it from
atomic_check(), mode_valid() and set_dsi_rate(). It really looks like
a simpler solution here.

Note, there is a significant difference between mode_valid() and
atomic_check(). The former function is used for filtering the modes,
while the latter one is used for actually checking that the parameters
passed from the client are correct.

>
> I had to move it from bridge_enable based on the comments on v1:
> https://patchwork.kernel.org/project/dri-devel/patch/20240408073623.186489-1-j-choudhary@ti.com/#25801801
>
> Warm Regards,
> Jayesh
>
> [...]



-- 
With best wishes
Dmitry

Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook for the bridge

[Jayesh Choudhary]

Hello Dmitry,

On 18/06/24 15:45, Dmitry Baryshkov wrote:
> On Tue, 18 Jun 2024 at 12:56, Jayesh Choudhary <j-choudhary@ti.com> wrote:
>>
>> Hello Dmitry,
>>
>> Thanks for the review.
>>
>> On 18/06/24 14:29, Dmitry Baryshkov wrote:
>>> On Tue, Jun 18, 2024 at 01:44:17PM GMT, Jayesh Choudhary wrote:
>>>> Add the atomic_check hook to ensure that the parameters are within the
>>>> valid range.
>>>> As of now, dsi clock freqency is being calculated in bridge_enable but
>>>> this needs to be checked in atomic_check which is called before
>>>> bridge_enable so move this calculation to atomic_check and write the
>>>> register value in bridge_enable as it is.
>>>>
>>>> For now, add mode clock check for the max resolution supported by the
>>>> bridge as mentioned in the SN65DSI86 datasheet[0] and dsi clock range
>>>> check for SN_DSIA_CLK_FREQ_REG.
>>>> According to the datasheet[0], the minimum value for that reg is 0x08
>>>> and the maximum value is 0x96. So add check for that.
>>>>
>>>> [0]: <https://www.ti.com/lit/gpn/sn65dsi86>
>>>>
>>>> Signed-off-by: Jayesh Choudhary <j-choudhary@ti.com>
>>>> ---
>>>>    drivers/gpu/drm/bridge/ti-sn65dsi86.c | 65 +++++++++++++++++++--------
>>>>    1 file changed, 46 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>>> index 84698a0b27a8..d13b42d7c512 100644
>>>> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>>>> @@ -113,6 +113,20 @@
>>>>
>>
>> [...]
>>
>>>>
>>>> +static int ti_sn_bridge_atomic_check(struct drm_bridge *bridge,
>>>> +                                 struct drm_bridge_state *bridge_state,
>>>> +                                 struct drm_crtc_state *crtc_state,
>>>> +                                 struct drm_connector_state *conn_state)
>>>> +{
>>>> +    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;
>>>> +
>>>> +    /* Pixel clock check */
>>>> +    if (mode->clock > SN65DSI86_MAX_PIXEL_CLOCK_KHZ)
>>>> +            return -EINVAL;
>>>> +
>>>> +    bit_rate_mhz = (mode->clock / 1000) *
>>>> +                    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);
>>>
>>> atomic_check might be called several times, it might be called to test
>>> the state. As such, it should not modify anything outside of the
>>> state variables.
>>>
>>
>> If not in atomic_check, then where should I move this calculation and check?
>> mode_valid with returning MODE_BAD in case of failure?
> 
> I didn't write that it's the wrong place for math. I wrote that you
> should not be modifying global structure.
> 
> So you have to subclass drm_bridge_state for the driver and store the
> value there. Or just add a helper function and call it from
> atomic_check(), mode_valid() and set_dsi_rate(). It really looks like
> a simpler solution here.
> 

Okay, instead of moving the set_dsi_rate, I will rename it to
calc_dsi_rate with integer return value which I would use in both
bridge enable to write the register value and atomic_check to check the
parameters eliminating the need to modify the pdata structure/ adding
new variable to the structure.

(Earlier I was trying to avoid calculation in both calls so I added
another variable to the structure and used that. But I get your point
now!)

I will re-order the patches to have an independent fix patch
addressing your concern in [2/2] patch
https://lore.kernel.org/all/CAA8EJpq2UkMn9ArSNaJcOyw28H4uUcRwvUqfUBBqSCALmozBrg@mail.gmail.com/

Also in the code they have been using 594MHz as mode clock limit.
I was using more relaxed value (600MHz) in atomic check but I will
switch to 594MHz to be in sync with the value that is used in the
driver.

> Note, there is a significant difference between mode_valid() and
> atomic_check(). The former function is used for filtering the modes,
> while the latter one is used for actually checking that the parameters
> passed from the client are correct.

[...]

Warm Regards,
Jayesh