linux-kernel - Re: [PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean up cdns_dsi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6d053590-7390-4d4c-98b7-32a02b5bf561@ideasonboard.com>
Date: Fri, 25 Apr 2025 14:57:54 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, Francesco Dolcini <francesco@...cini.it>,
 Devarsh Thakkar <devarsht@...com>, Jyri Sarha <jyri.sarha@....fi>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Andrzej Hajda <andrzej.hajda@...el.com>,
 Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
 Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
 Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Jayesh Choudhary <j-choudhary@...com>
Subject: Re: [PATCH v3 14/17] drm/bridge: cdns-dsi: Use video mode and clean
 up cdns_dsi_mode2cfg()

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@...asonboard.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