[PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi_mode2cfg()

[Tomi Valkeinen]

Hi,

On 20/04/2025 21:10, Aradhya Bhatia wrote:
> Hi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver does all the calculations and programming with video timings
>> (hftp, hbp, etc.) instead of the modeline values (hsync_start, ...).
>> Thus it makes sense to use struct videomode instead of struct
>> drm_display_mode internally.
>>
>> Switch to videomode and do some cleanups in cdns_dsi_mode2cfg() along
>> the way.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 45 ++++++++++++++------------
>>   1 file changed, 24 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index fb0623d3f854..a55f851711f0 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -9,6 +9,7 @@
>>   #include <drm/drm_drv.h>
>>   #include <drm/drm_probe_helper.h>
>>   #include <video/mipi_display.h>
>> +#include <video/videomode.h>
>>   
>>   #include <linux/clk.h>
>>   #include <linux/interrupt.h>
>> @@ -467,36 +468,35 @@ 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,
>> +			     const struct videomode *vm,
>>   			     struct cdns_dsi_cfg *dsi_cfg)
>>   {
>>   	struct cdns_dsi_output *output = &dsi->output;
>> -	unsigned int tmp;
>> -	bool sync_pulse = false;
>> +	u32 dpi_hsa, dpi_hbp, dpi_hfp, dpi_hact;
>> +	bool sync_pulse;
>>   	int bpp;
>>   
>> +	dpi_hsa = vm->hsync_len;
>> +	dpi_hbp = vm->hback_porch;
>> +	dpi_hfp = vm->hfront_porch;
>> +	dpi_hact = vm->hactive;
>> +
>>   	memset(dsi_cfg, 0, sizeof(*dsi_cfg));
>>   
>> -	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
>> -		sync_pulse = true;
>> +	sync_pulse = output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
>>   
>>   	bpp = mipi_dsi_pixel_format_to_bpp(output->dev->format);
>>   
>> -	tmp = mode->htotal -
>> -		(sync_pulse ? mode->hsync_end : mode->hsync_start);
>> +	dsi_cfg->hbp = dpi_to_dsi_timing(dpi_hbp + (sync_pulse ? 0 : dpi_hsa),
>> +					 bpp, DSI_HBP_FRAME_OVERHEAD);
>>   
>> -	dsi_cfg->hbp = dpi_to_dsi_timing(tmp, bpp, DSI_HBP_FRAME_OVERHEAD);
>> +	if (sync_pulse)
>> +		dsi_cfg->hsa =
>> +			dpi_to_dsi_timing(dpi_hsa, bpp, DSI_HSA_FRAME_OVERHEAD);
>>   
>> -	if (sync_pulse) {
>> -		tmp = mode->hsync_end - mode->hsync_start;
>> +	dsi_cfg->hact = dpi_to_dsi_timing(dpi_hact, bpp, 0);
>>   
>> -		dsi_cfg->hsa = dpi_to_dsi_timing(tmp, bpp,
>> -						 DSI_HSA_FRAME_OVERHEAD);
>> -	}
>> -
>> -	dsi_cfg->hact = dpi_to_dsi_timing(mode->hdisplay, bpp, 0);
>> -	dsi_cfg->hfp = dpi_to_dsi_timing(mode->hsync_start - mode->hdisplay,
>> -					 bpp, DSI_HFP_FRAME_OVERHEAD);
>> +	dsi_cfg->hfp = dpi_to_dsi_timing(dpi_hfp, bpp, DSI_HFP_FRAME_OVERHEAD);
>>   
>>   	dsi_cfg->htotal = dsi_cfg->hbp + DSI_HBP_FRAME_OVERHEAD;
>>   	if (output->dev->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> 
> I think at this stage, the dsi_cfg->htotal will always come out to be
> 
> ((dpi_htotal * bitspp) / 8),
> 
> no matter whether the sync_pulse or the event_mode is set or not.
> 
> Whatever the overheads are there, they get cancelled out. So, it doesn't
> need to be individually tracked.

dpi_to_dsi_timing() doesn't return the DPI timing converted _exactly_ to 
DSI. It uses DIV_ROUND_UP() and handles the case where the DPI timing is 
too small for DSI with the overhead.

And I'd rather separate DPI and DSI timings as much as possible, even if 
it is a bit more verbose. Here we want to calculate DSI htotal (i.e. the 
total of DSI horizontal ticks), so I'd rather construct it from the DSI 
timings, instead of making shortcuts and trusting that the DPI timings 
match exactly (even if they would). Calculating these is rather error 
prone already.

At some point we want to adjust the DSI timings (at least if/when 
implementing burst mode). While even then we'll aim to match the exact 
DPI time, I think it's better to calculate the DSI htotal from the 
adjusted DSI timings. If the DSI timings are not right, then the htotal 
will also match that "wrongness", instead of just showing the DPI htotal.

  Tomi