Re: [PATCH RESEND] drm: bridge: dw-mipi-dsi: Allow sync-pulses to override the burst vid-mode

[Liu Ying]

Hi Heiko,

On 06/26/2024, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>
> 
> The state right now is that if the panel has the burst-mode flag it
> will take precedence over the sync-pulses mode.

Yes.

> 
> While sync-pulses are only relevant for the video-mode, the burst-mode
> flag affects both the video-mode as well as the calculated lane_mbps.
> 
> Looking at drivers like the nwl-dsi [0] it only enables burst mode when
> the panel's flags do not contain the sync_pulse flag.
> 
> So handle things similar for dw-dsi in that it selects the video-mode

This doesn't seem to be necessary.  DSI host drivers may choose video
modes precedence.  DSI device drivers should set video modes in the
flag carefully, as any video mode set may be used by a certain DSI
host driver.

> with sync-pulses if that flag is set and only after that, checks for
> the burst-mode. So panels selecting a combination of both burst and
> sync-pulses get the sync-pulse mode.
> 
> The case this fixes can be found on the ltk050h3148w . It does need the
> lane-rate to be calculated according to burst formulas [1], but without
> sync-pulses we see the output shifted around 20 pixels to the right,
> meaning that the last 20 pixels from each line appear at the start of
> the next display line.

This sounds like ltk050h3148w abuses the video modes - it actually
uses sync pulse video mode, but needs lane-rate be calculated by
using lane_mbps instead of pixel clock which hints it uses burst
video mode.

Is it a panel initialization code issue or DSI host/PHY configuration
issue or clock issue?

commit e5f9d543419c78ac58f3b3557bc5a76b20ff600b says ltk050h3148w's
driving controller is a Himax HX8394-F.  Looking at
panel-himax-hx8394.c, "powkiddy,x55-panel" is also a 720x1280 panel
with only MIPI_DSI_MODE_VIDEO_BURST set and a different display
timing.  The panel is in rk3566-powkiddy-x55.dts, which means it also
connects with a Synopsys DSI host controller.  Maybe, you may try
that display timing with burst video mode?

> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/bridge/nwl-dsi.c#n301
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6c9dbee84cd005bed5f9d07b3a2797ae6414b435
> 
> Fixes: 93e82bb4de01 ("drm/bridge: synopsys: dw-mipi-dsi: Fix hcomponent lbcc for burst mode")

commit 93e82bb4de01 follows the current state to make sure this
driver chooses to take burst video mode as precedence over sync
pulse video mode.

> Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxxxxx>
> ---
> resend, because I messed up and somehow forgot to include _all_
> mailing lists.
> 
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 824fb3c65742e..28dd858a751bd 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -605,10 +605,10 @@ static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
>  	 */
>  	val = ENABLE_LOW_POWER;
>  
> -	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> -		val |= VID_MODE_TYPE_BURST;
> -	else if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
> +	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)
>  		val |= VID_MODE_TYPE_NON_BURST_SYNC_PULSES;
> +	else if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> +		val |= VID_MODE_TYPE_BURST;

If the precedence really needs to be swapped(which again doesn't
seem to be necessary), then change the precedence in
dw_mipi_dsi_get_hcomponent_lbcc() too, for the sake of consistency.

>  	else
>  		val |= VID_MODE_TYPE_NON_BURST_SYNC_EVENTS;
>  

-- 
Regards,
Liu Ying