Date: Tue, 18 Jun 2024 11:59:42 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jayesh Choudhary <j-choudhary@...com>
Cc: dianders@...omium.org, andrzej.hajda@...el.com, 
	neil.armstrong@...aro.org, rfoss@...nel.org, Laurent.pinchart@...asonboard.com, 
	mripard@...nel.org, linux-kernel@...r.kernel.org, jonas@...boo.se, 
	jernej.skrabec@...il.com, maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de, 
	airlied@...il.com, daniel@...ll.ch, spanda@...eaurora.org, a-bhatia1@...com, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi86: Add atomic_check hook
 for the bridge

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@...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@...z) 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

Powered by blists - more mailing lists