The docs say about mode_valid():

"it is not allowed to look at anything else but the passed-in mode, and
validate it against configuration-invariant hardware constraints"

We're doing a lot more than just looking at the mode. The main issue
here is that we're doing checks based on the pixel clock, before we know
what the pixel clock from the crtc actually is.

So, drop the checks from .mode_valid(). This also allows us to remove
the 'mode_valid_check' parameter from internal functions, and the
related code.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index e85c8652c96e..cf783680b1b4 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
 
 static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 			     const struct drm_display_mode *mode,
-			     struct cdns_dsi_cfg *dsi_cfg,
-			     bool mode_valid_check)
+			     struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
 	bool sync_pulse;
 	int bpp;
 
-	if (mode_valid_check) {
-		dpi_hsa = mode->hsync_end - mode->hsync_start;
-		dpi_hbp = mode->htotal - mode->hsync_end;
-		dpi_hfp = mode->hsync_start - mode->hdisplay;
-		dpi_hact = mode->hdisplay;
-	} else {
-		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
-		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
-		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
-		dpi_hact = mode->crtc_hdisplay;
-	}
+	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
+	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
+	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
+	dpi_hact = mode->crtc_hdisplay;
 
 	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
 
@@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
 static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 			      struct cdns_dsi_cfg *dsi_cfg,
 			      struct phy_configure_opts_mipi_dphy *phy_cfg,
-			      const struct drm_display_mode *mode,
-			      bool mode_valid_check)
+			      const struct drm_display_mode *mode)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	unsigned long long dlane_bps;
@@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 	if (dsi_htotal % lanes)
 		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
 
-	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
+	dpi_hz = mode->crtc_clock * 1000;
 	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
 
 	/* data rate in bytes/sec is not an integer, refuse the mode. */
-	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
+	dpi_htotal = mode->crtc_htotal;
 	if (do_div(dlane_bps, lanes * dpi_htotal))
 		return -EINVAL;
 
@@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
 
 static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 			       const struct drm_display_mode *mode,
-			       struct cdns_dsi_cfg *dsi_cfg,
-			       bool mode_valid_check)
+			       struct cdns_dsi_cfg *dsi_cfg)
 {
 	struct cdns_dsi_output *output = &dsi->output;
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned int nlanes = output->dev->lanes;
-	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
 	unsigned long req_hs_clk_rate;
 	int ret;
 
-	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
+	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
 	if (ret)
 		return ret;
 
-	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
+	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
 					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					       nlanes, phy_cfg);
 	if (ret)
 		return ret;
 
-	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
+	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
 	if (ret)
 		return ret;
 
@@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	struct cdns_dsi_output *output = &dsi->output;
-	struct cdns_dsi_cfg dsi_cfg;
-	int bpp, ret;
+	int bpp;
 
 	/*
 	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
@@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	if ((mode->hdisplay * bpp) % 32)
 		return MODE_H_ILLEGAL;
 
-	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
-	if (ret)
-		return MODE_BAD;
-
 	return MODE_OK;
 }
 
@@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
 
-	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
+	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
 }
 
 static struct drm_bridge_state *

-- 
2.43.0

Hi Tomi,

On 02/04/25 19:00, Tomi Valkeinen wrote:
> The docs say about mode_valid():
> 
> "it is not allowed to look at anything else but the passed-in mode, and
> validate it against configuration-invariant hardware constraints"
> 
> We're doing a lot more than just looking at the mode. The main issue
> here is that we're doing checks based on the pixel clock, before we know
> what the pixel clock from the crtc actually is.
> 
> So, drop the checks from .mode_valid(). This also allows us to remove
> the 'mode_valid_check' parameter from internal functions, and the
> related code.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
>  1 file changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index e85c8652c96e..cf783680b1b4 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>  
>  static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>  			     const struct drm_display_mode *mode,
> -			     struct cdns_dsi_cfg *dsi_cfg,
> -			     bool mode_valid_check)
> +			     struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>  	bool sync_pulse;
>  	int bpp;
>  
> -	if (mode_valid_check) {
> -		dpi_hsa = mode->hsync_end - mode->hsync_start;
> -		dpi_hbp = mode->htotal - mode->hsync_end;
> -		dpi_hfp = mode->hsync_start - mode->hdisplay;
> -		dpi_hact = mode->hdisplay;
> -	} else {
> -		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
> -		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
> -		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
> -		dpi_hact = mode->crtc_hdisplay;
> -	}
> +	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
> +	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
> +	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
> +	dpi_hact = mode->crtc_hdisplay;
>  
>  	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>  
> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>  static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  			      struct cdns_dsi_cfg *dsi_cfg,
>  			      struct phy_configure_opts_mipi_dphy *phy_cfg,
> -			      const struct drm_display_mode *mode,
> -			      bool mode_valid_check)
> +			      const struct drm_display_mode *mode)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	unsigned long long dlane_bps;
> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  	if (dsi_htotal % lanes)
>  		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>  
> -	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
> +	dpi_hz = mode->crtc_clock * 1000;
>  	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>  
>  	/* data rate in bytes/sec is not an integer, refuse the mode. */
> -	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
> +	dpi_htotal = mode->crtc_htotal;
>  	if (do_div(dlane_bps, lanes * dpi_htotal))
>  		return -EINVAL;
>  
> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>  
>  static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>  			       const struct drm_display_mode *mode,
> -			       struct cdns_dsi_cfg *dsi_cfg,
> -			       bool mode_valid_check)
> +			       struct cdns_dsi_cfg *dsi_cfg)
>  {
>  	struct cdns_dsi_output *output = &dsi->output;
>  	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>  	unsigned int nlanes = output->dev->lanes;
> -	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
>  	unsigned long req_hs_clk_rate;
>  	int ret;
>  
> -	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
> +	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>  	if (ret)
>  		return ret;
>  
> -	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
> +	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>  					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
>  					       nlanes, phy_cfg);
>  	if (ret)
>  		return ret;
>  
> -	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
> +	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>  	if (ret)
>  		return ret;
>  
> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>  	struct cdns_dsi *dsi = input_to_dsi(input);
>  	struct cdns_dsi_output *output = &dsi->output;
> -	struct cdns_dsi_cfg dsi_cfg;
> -	int bpp, ret;
> +	int bpp;
>  
>  	/*
>  	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>  	if ((mode->hdisplay * bpp) % 32)
>  		return MODE_H_ILLEGAL;
>  
> -	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
> -	if (ret)
> -		return MODE_BAD;
> -
>  	return MODE_OK;
>  }
>  
> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>  	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>  	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>  
> -	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);

With this patch, the driver shifts to using the crtc_* values during the
check phase, and then, it is brought back to using non crtc_* values in
the next patch.

Should this patch just drop the cdns_dsi_check_conf() check from
.mode_valid() instead, and let the next patch phase out the
mdoe_valid_check parameter as the driver simultaneously shifts to using
the non crtc_* values throughout?


-- 
Regards
Aradhya

Hi,

On 07/04/2025 20:59, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 02/04/25 19:00, Tomi Valkeinen wrote:
>> The docs say about mode_valid():
>>
>> "it is not allowed to look at anything else but the passed-in mode, and
>> validate it against configuration-invariant hardware constraints"
>>
>> We're doing a lot more than just looking at the mode. The main issue
>> here is that we're doing checks based on the pixel clock, before we know
>> what the pixel clock from the crtc actually is.
>>
>> So, drop the checks from .mode_valid(). This also allows us to remove
>> the 'mode_valid_check' parameter from internal functions, and the
>> related code.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 ++++++++------------------
>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index e85c8652c96e..cf783680b1b4 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned int dpi_timing,
>>   
>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>   			     const struct drm_display_mode *mode,
>> -			     struct cdns_dsi_cfg *dsi_cfg,
>> -			     bool mode_valid_check)
>> +			     struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>   	bool sync_pulse;
>>   	int bpp;
>>   
>> -	if (mode_valid_check) {
>> -		dpi_hsa = mode->hsync_end - mode->hsync_start;
>> -		dpi_hbp = mode->htotal - mode->hsync_end;
>> -		dpi_hfp = mode->hsync_start - mode->hdisplay;
>> -		dpi_hact = mode->hdisplay;
>> -	} else {
>> -		dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>> -		dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>> -		dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>> -		dpi_hact = mode->crtc_hdisplay;
>> -	}
>> +	dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>> +	dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>> +	dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>> +	dpi_hact = mode->crtc_hdisplay;
>>   
>>   	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>   
>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   			      struct cdns_dsi_cfg *dsi_cfg,
>>   			      struct phy_configure_opts_mipi_dphy *phy_cfg,
>> -			      const struct drm_display_mode *mode,
>> -			      bool mode_valid_check)
>> +			      const struct drm_display_mode *mode)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	unsigned long long dlane_bps;
>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   	if (dsi_htotal % lanes)
>>   		adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>   
>> -	dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 1000;
>> +	dpi_hz = mode->crtc_clock * 1000;
>>   	dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>   
>>   	/* data rate in bytes/sec is not an integer, refuse the mode. */
>> -	dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>> +	dpi_htotal = mode->crtc_htotal;
>>   	if (do_div(dlane_bps, lanes * dpi_htotal))
>>   		return -EINVAL;
>>   
>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>   
>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>   			       const struct drm_display_mode *mode,
>> -			       struct cdns_dsi_cfg *dsi_cfg,
>> -			       bool mode_valid_check)
>> +			       struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>>   	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
>>   	unsigned int nlanes = output->dev->lanes;
>> -	int mode_clock = (mode_valid_check ? mode->clock : mode->crtc_clock);
>>   	unsigned long req_hs_clk_rate;
>>   	int ret;
>>   
>> -	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>> +	ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>> +	ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>   					       mipi_dsi_pixel_format_to_bpp(output->dev->format),
>>   					       nlanes, phy_cfg);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, mode_valid_check);
>> +	ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>>   	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>   	struct cdns_dsi *dsi = input_to_dsi(input);
>>   	struct cdns_dsi_output *output = &dsi->output;
>> -	struct cdns_dsi_cfg dsi_cfg;
>> -	int bpp, ret;
>> +	int bpp;
>>   
>>   	/*
>>   	 * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
>>   	if ((mode->hdisplay * bpp) % 32)
>>   		return MODE_H_ILLEGAL;
>>   
>> -	ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>> -	if (ret)
>> -		return MODE_BAD;
>> -
>>   	return MODE_OK;
>>   }
>>   
>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>   	adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>>   	adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC;
>>   
>> -	return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>> +	return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
> 
> With this patch, the driver shifts to using the crtc_* values during the
> check phase, and then, it is brought back to using non crtc_* values in
> the next patch.
> 
> Should this patch just drop the cdns_dsi_check_conf() check from
> .mode_valid() instead, and let the next patch phase out the
> mdoe_valid_check parameter as the driver simultaneously shifts to using
> the non crtc_* values throughout?

Yes.

  Tomi

Hi,

On 08/04/2025 09:09, Tomi Valkeinen wrote:
> Hi,
> 
> On 07/04/2025 20:59, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 02/04/25 19:00, Tomi Valkeinen wrote:
>>> The docs say about mode_valid():
>>>
>>> "it is not allowed to look at anything else but the passed-in mode, and
>>> validate it against configuration-invariant hardware constraints"
>>>
>>> We're doing a lot more than just looking at the mode. The main issue
>>> here is that we're doing checks based on the pixel clock, before we know
>>> what the pixel clock from the crtc actually is.
>>>
>>> So, drop the checks from .mode_valid(). This also allows us to remove
>>> the 'mode_valid_check' parameter from internal functions, and the
>>> related code.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> ---
>>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 +++++++ 
>>> +------------------
>>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/ 
>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> index e85c8652c96e..cf783680b1b4 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned 
>>> int dpi_timing,
>>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>                    const struct drm_display_mode *mode,
>>> -                 struct cdns_dsi_cfg *dsi_cfg,
>>> -                 bool mode_valid_check)
>>> +                 struct cdns_dsi_cfg *dsi_cfg)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>>       bool sync_pulse;
>>>       int bpp;
>>> -    if (mode_valid_check) {
>>> -        dpi_hsa = mode->hsync_end - mode->hsync_start;
>>> -        dpi_hbp = mode->htotal - mode->hsync_end;
>>> -        dpi_hfp = mode->hsync_start - mode->hdisplay;
>>> -        dpi_hact = mode->hdisplay;
>>> -    } else {
>>> -        dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> -        dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> -        dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> -        dpi_hact = mode->crtc_hdisplay;
>>> -    }
>>> +    dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>> +    dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>> +    dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>> +    dpi_hact = mode->crtc_hdisplay;
>>>       memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>>                     struct cdns_dsi_cfg *dsi_cfg,
>>>                     struct phy_configure_opts_mipi_dphy *phy_cfg,
>>> -                  const struct drm_display_mode *mode,
>>> -                  bool mode_valid_check)
>>> +                  const struct drm_display_mode *mode)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       unsigned long long dlane_bps;
>>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct 
>>> cdns_dsi *dsi,
>>>       if (dsi_htotal % lanes)
>>>           adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>> -    dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) * 
>>> 1000;
>>> +    dpi_hz = mode->crtc_clock * 1000;
>>>       dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>>       /* data rate in bytes/sec is not an integer, refuse the mode. */
>>> -    dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>>> +    dpi_htotal = mode->crtc_htotal;
>>>       if (do_div(dlane_bps, lanes * dpi_htotal))
>>>           return -EINVAL;
>>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct 
>>> cdns_dsi *dsi,
>>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>>                      const struct drm_display_mode *mode,
>>> -                   struct cdns_dsi_cfg *dsi_cfg,
>>> -                   bool mode_valid_check)
>>> +                   struct cdns_dsi_cfg *dsi_cfg)
>>>   {
>>>       struct cdns_dsi_output *output = &dsi->output;
>>>       struct phy_configure_opts_mipi_dphy *phy_cfg = &output- 
>>> >phy_opts.mipi_dphy;
>>>       unsigned int nlanes = output->dev->lanes;
>>> -    int mode_clock = (mode_valid_check ? mode->clock : mode- 
>>> >crtc_clock);
>>>       unsigned long req_hs_clk_rate;
>>>       int ret;
>>> -    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>>> +    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>>       if (ret)
>>>           return ret;
>>> -    ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>>> +    ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>>                              mipi_dsi_pixel_format_to_bpp(output- 
>>> >dev->format),
>>>                              nlanes, phy_cfg);
>>>       if (ret)
>>>           return ret;
>>> -    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode, 
>>> mode_valid_check);
>>> +    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>>       if (ret)
>>>           return ret;
>>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge 
>>> *bridge,
>>>       struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>>       struct cdns_dsi *dsi = input_to_dsi(input);
>>>       struct cdns_dsi_output *output = &dsi->output;
>>> -    struct cdns_dsi_cfg dsi_cfg;
>>> -    int bpp, ret;
>>> +    int bpp;
>>>       /*
>>>        * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge 
>>> *bridge,
>>>       if ((mode->hdisplay * bpp) % 32)
>>>           return MODE_H_ILLEGAL;
>>> -    ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>>> -    if (ret)
>>> -        return MODE_BAD;
>>> -
>>>       return MODE_OK;
>>>   }
>>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct 
>>> drm_bridge *bridge,
>>>       adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | 
>>> DRM_MODE_FLAG_PVSYNC);
>>>       adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC | 
>>> DRM_MODE_FLAG_NVSYNC;
>>> -    return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>>> +    return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
>>
>> With this patch, the driver shifts to using the crtc_* values during the
>> check phase, and then, it is brought back to using non crtc_* values in
>> the next patch.
>>
>> Should this patch just drop the cdns_dsi_check_conf() check from
>> .mode_valid() instead, and let the next patch phase out the
>> mdoe_valid_check parameter as the driver simultaneously shifts to using
>> the non crtc_* values throughout?
> 
> Yes.

Actually, this patch doesn't change the crtc_* vs non-crtc_* behavior. 
After dropping the cdns_dsi_check_conf() call in mode_valid(), the 
'mode_valid_check' is always false. So this patch removes the parameter, 
and any code paths for the true-case.

Should the atomic_check() have been using 'true' for the 
'mode_valid_check'? The atomic_check code was added in 
a53d987756eab40678f241d7cd0eb7e1ca42bba7.

  Tomi

Hi Tomi,

On 08/04/25 12:39, Tomi Valkeinen wrote:
> Hi,
> 
> On 08/04/2025 09:09, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 07/04/2025 20:59, Aradhya Bhatia wrote:
>>> Hi Tomi,
>>>
>>> On 02/04/25 19:00, Tomi Valkeinen wrote:
>>>> The docs say about mode_valid():
>>>>
>>>> "it is not allowed to look at anything else but the passed-in mode, and
>>>> validate it against configuration-invariant hardware constraints"
>>>>
>>>> We're doing a lot more than just looking at the mode. The main issue
>>>> here is that we're doing checks based on the pixel clock, before we
>>>> know
>>>> what the pixel clock from the crtc actually is.
>>>>
>>>> So, drop the checks from .mode_valid(). This also allows us to remove
>>>> the 'mode_valid_check' parameter from internal functions, and the
>>>> related code.
>>>>
>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> ---
>>>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 44 +++++++
>>>> +------------------
>>>>   1 file changed, 14 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/
>>>> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> index e85c8652c96e..cf783680b1b4 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>>>> @@ -469,25 +469,17 @@ static unsigned int dpi_to_dsi_timing(unsigned
>>>> int dpi_timing,
>>>>   static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>>                    const struct drm_display_mode *mode,
>>>> -                 struct cdns_dsi_cfg *dsi_cfg,
>>>> -                 bool mode_valid_check)
>>>> +                 struct cdns_dsi_cfg *dsi_cfg)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>>>>       bool sync_pulse;
>>>>       int bpp;
>>>> -    if (mode_valid_check) {
>>>> -        dpi_hsa = mode->hsync_end - mode->hsync_start;
>>>> -        dpi_hbp = mode->htotal - mode->hsync_end;
>>>> -        dpi_hfp = mode->hsync_start - mode->hdisplay;
>>>> -        dpi_hact = mode->hdisplay;
>>>> -    } else {
>>>> -        dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>>> -        dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>>> -        dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>>> -        dpi_hact = mode->crtc_hdisplay;
>>>> -    }
>>>> +    dpi_hsa = mode->crtc_hsync_end - mode->crtc_hsync_start;
>>>> +    dpi_hbp = mode->crtc_htotal - mode->crtc_hsync_end;
>>>> +    dpi_hfp =  mode->crtc_hsync_start - mode->crtc_hdisplay;
>>>> +    dpi_hact = mode->crtc_hdisplay;
>>>>       memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>>> @@ -518,8 +510,7 @@ static int cdns_dsi_mode2cfg(struct cdns_dsi *dsi,
>>>>   static int cdns_dsi_adjust_phy_config(struct cdns_dsi *dsi,
>>>>                     struct cdns_dsi_cfg *dsi_cfg,
>>>>                     struct phy_configure_opts_mipi_dphy *phy_cfg,
>>>> -                  const struct drm_display_mode *mode,
>>>> -                  bool mode_valid_check)
>>>> +                  const struct drm_display_mode *mode)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       unsigned long long dlane_bps;
>>>> @@ -549,11 +540,11 @@ static int cdns_dsi_adjust_phy_config(struct
>>>> cdns_dsi *dsi,
>>>>       if (dsi_htotal % lanes)
>>>>           adj_dsi_htotal += lanes - (dsi_htotal % lanes);
>>>> -    dpi_hz = (mode_valid_check ? mode->clock : mode->crtc_clock) *
>>>> 1000;
>>>> +    dpi_hz = mode->crtc_clock * 1000;
>>>>       dlane_bps = (unsigned long long)dpi_hz * adj_dsi_htotal;
>>>>       /* data rate in bytes/sec is not an integer, refuse the mode. */
>>>> -    dpi_htotal = mode_valid_check ? mode->htotal : mode->crtc_htotal;
>>>> +    dpi_htotal = mode->crtc_htotal;
>>>>       if (do_div(dlane_bps, lanes * dpi_htotal))
>>>>           return -EINVAL;
>>>> @@ -569,27 +560,25 @@ static int cdns_dsi_adjust_phy_config(struct
>>>> cdns_dsi *dsi,
>>>>   static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
>>>>                      const struct drm_display_mode *mode,
>>>> -                   struct cdns_dsi_cfg *dsi_cfg,
>>>> -                   bool mode_valid_check)
>>>> +                   struct cdns_dsi_cfg *dsi_cfg)
>>>>   {
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>>       struct phy_configure_opts_mipi_dphy *phy_cfg = &output-
>>>> >phy_opts.mipi_dphy;
>>>>       unsigned int nlanes = output->dev->lanes;
>>>> -    int mode_clock = (mode_valid_check ? mode->clock : mode-
>>>> >crtc_clock);
>>>>       unsigned long req_hs_clk_rate;
>>>>       int ret;
>>>> -    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg, mode_valid_check);
>>>> +    ret = cdns_dsi_mode2cfg(dsi, mode, dsi_cfg);
>>>>       if (ret)
>>>>           return ret;
>>>> -    ret = phy_mipi_dphy_get_default_config(mode_clock * 1000,
>>>> +    ret = phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
>>>>                              mipi_dsi_pixel_format_to_bpp(output-
>>>> >dev->format),
>>>>                              nlanes, phy_cfg);
>>>>       if (ret)
>>>>           return ret;
>>>> -    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode,
>>>> mode_valid_check);
>>>> +    ret = cdns_dsi_adjust_phy_config(dsi, dsi_cfg, phy_cfg, mode);
>>>>       if (ret)
>>>>           return ret;
>>>> @@ -635,8 +624,7 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge
>>>> *bridge,
>>>>       struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
>>>>       struct cdns_dsi *dsi = input_to_dsi(input);
>>>>       struct cdns_dsi_output *output = &dsi->output;
>>>> -    struct cdns_dsi_cfg dsi_cfg;
>>>> -    int bpp, ret;
>>>> +    int bpp;
>>>>       /*
>>>>        * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at
>>>> @@ -654,10 +642,6 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge
>>>> *bridge,
>>>>       if ((mode->hdisplay * bpp) % 32)
>>>>           return MODE_H_ILLEGAL;
>>>> -    ret = cdns_dsi_check_conf(dsi, mode, &dsi_cfg, true);
>>>> -    if (ret)
>>>> -        return MODE_BAD;
>>>> -
>>>>       return MODE_OK;
>>>>   }
>>>> @@ -996,7 +980,7 @@ static int cdns_dsi_bridge_atomic_check(struct
>>>> drm_bridge *bridge,
>>>>       adjusted_crtc_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC |
>>>> DRM_MODE_FLAG_PVSYNC);
>>>>       adjusted_crtc_mode->flags |= DRM_MODE_FLAG_NHSYNC |
>>>> DRM_MODE_FLAG_NVSYNC;
>>>> -    return cdns_dsi_check_conf(dsi, mode, dsi_cfg, false);
>>>> +    return cdns_dsi_check_conf(dsi, mode, dsi_cfg);
>>>
>>> With this patch, the driver shifts to using the crtc_* values during the
>>> check phase, and then, it is brought back to using non crtc_* values in
>>> the next patch.
>>>
>>> Should this patch just drop the cdns_dsi_check_conf() check from
>>> .mode_valid() instead, and let the next patch phase out the
>>> mdoe_valid_check parameter as the driver simultaneously shifts to using
>>> the non crtc_* values throughout?
>>
>> Yes.
> 
> Actually, this patch doesn't change the crtc_* vs non-crtc_* behavior.
> After dropping the cdns_dsi_check_conf() call in mode_valid(), the
> 'mode_valid_check' is always false. So this patch removes the parameter,
> and any code paths for the true-case.
> 
> Should the atomic_check() have been using 'true' for the
> 'mode_valid_check'? The atomic_check code was added in
> a53d987756eab40678f241d7cd0eb7e1ca42bba7.
> 

You are right. I had lost a bit of context there.

Upon seeing the change logs, and history for my DSI patches, it seems
that the _atomic_check() was added as a replacement for the check that
was taking place in the _(atomic)_enable(), because the enable-path was
not the right place to do so.

Since the enable-path worked on crtc_* values, the check used to happen
with crtc_* values. And then the _atomic_check() continued to use the
crtc_* values after the patch.

But, since the crtc_* values don't get populated before the bridge's
check-phase, the crtc_* values shouldn't be used at this stage for any
checks.
They are getting populated in this case, via the fbdev_client_setup, if
I understand that right. But it's not right to depend on fbdev as it can
be disabled.

So, it would make sense to use 'true' for the `mode_valid_check`
parameter here. And, I will post a fix for this.


However, we have another question. How would the driver verify the
crtc_* values then, if not during the enable-path? Effectively, it might
not matter for this driver, but there should be a general guideline.


--
Regards
Aradhya