Re: [PATCH v2 12/18] drm/bridge: cdns-dsi: Drop checks that shouldn't be in .mode_valid()

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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