Re: [PATCH] drm/panel: move some dsi commands from unprepare to disable

[Jakob Hauser <jahau@rocketmail.com>]

Hi all,

On 16.06.23 01:36, Doug Anderson wrote:
...
> I guess the tl;dr (summary of my summary) is:
> 
> a) Moving panels like this to "pre_enable_prev_first" seems like a
> reasonable idea anyway and (presumably) works around the issue.
> 
> b) Moving some commands between disable() / post_diable() or
> pre_enable() / enable() can also make sense and isn't terrible. As
> people have pointed out, there is some vagueness on exactly what
> belongs in each.
> 
> c) Ideally someone could fix the MSM DSI driver to behave as Dave documented.
...
Actually I don't want to disturb the discussion here. Still I would like 
to point out that option a) "pre_enable_prev_first" doesn't seem to work 
well yet. On my device samsung-serranove with panel 
samsung-s6e88a0-ams427ap24 (not in mainline, [1]), when applying 
"pre_enable_prev_first" I notice two issues.

1) According to commig 4fb912e5e190 ("drm/bridge: Introduce 
pre_enable_prev_first to alter bridge init order") [2] it's supposed to 
reverse the order in "post_disable" as well. That doesn't work on my 
device, the "post_disable" order doesn't get changed by 
"pre_enable_prev_first".

2) When enabling the panel, for each mipi_dsi_dcs_... command there is 
an error in dmesg "msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] 
*ERROR* wait for video done timed out". The panel works well 
nonetheless. However, before commit 007ac0262b0d ("drm/msm/dsi: switch 
to DRM_PANEL_BRIDGE") these errors didn't show up.

I tried to get more insight in the order of issue 1). I added additional 
debug markers in drivers/gpu/drm/drm_bridge.c (original state linked in 
[3]) and got the following behavior. To me it's rather hard to 
understand the decision-making, to be honest. Both in "pre_enable" as 
well as in "post_disable" first the host and then the panel are 
processed. At "post_disable" it should be the other way around.

I'm currently on kernel 6.4-rc4 with cherry-picked commits from 
linux-next 9e15123eca79 ("drm/msm/dsi: Stop unconditionally powering up 
DSI hosts at modeset") and d8dd416cb420 ("drm/msm/dsi: More properly 
handle errors in regards to dsi_mgr_bridge_power_on()") and added 
"pre_enable_prev_first" to the panel driver. Device is samsung-serranove 
in X11 environment.

At boot:
--------
chain_pre_enable: bridge at beginning: 'host'
chain_pre_enable: iter in the list loop at the beginning: 'panel'
chain_pre_enable: next if iter prev_first: 'panel'
chain_pre_enable: limit if iter prev_first: 'host'
chain_pre_enable: next in list loop from_reverse: 'panel'
chain_pre_enable: next in list loop from_reverse: 'host'
chain_pre_enable: break because 'next == bridge' 'host'
chain_pre_enable: marker at end of first list loop block
chain_pre_enable: iter at end of first list loop block: 'panel'
chain_pre_enable: next at end of first list loop block: 'host'
chain_pre_enable: limit at end of first list loop block: 'host'
chain_pre_enable: next in 2nd list loop block: 'host'
chain_pre_enable: next is not iter, call pre_enable within 2nd list loop
                   block: 'host'
call_pre_enable: pre_enable bridge 'host'
call_pre_enable: inside 'else if', do pre_enable
chain_pre_enable: next in 2nd list loop block: 'panel'
chain_pre_enable: break because 'next == iter': 'panel'
chain_pre_enable: marker at end of 2nd list loop block
chain_pre_enable: iter after 2nd list loop block, call pre_enable:
                  'panel'
call_pre_enable: pre_enable bridge 'panel'
call_pre_enable: inside 'if'
call_pre_enable: passed second 'if', do atomic_pre_enable
msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] *ERROR* wait for video
                      done timed out
msm_dsi 1a98000.dsi: [drm:dsi_cmds2buf_tx [msm]] *ERROR* wait for video
                      done timed out
...
chain_pre_enable: if iter is prev_first...: 'panel'
chain_pre_enable: ... change iter to 'limit': 'host'
chain_pre_enable: iter at the end of function: 'host'
chain_pre_enable: bridge at the end of function: 'host'
chain_pre_enable: break if iter is bridge at the end of function: 'host'

Then the panel get's turned off:
--------------------------------
chain_post_disable: bridge at beginning: 'host'
chain_post_disable: bridge of the list loop at the beginning: 'host'
chain_post_disable: if entry is not last, set 'next' to next entry:
                     'panel'
chain_post_disable: if 'next' is prev_first, set 'limit' to next:
                     'panel'
chain_post_disable: loop through the list of 'next': 'panel'
chain_post_disable: if 'next' is prev_first, change 'next' to: 'host'
chain_post_disable: ... and 'limit' to the same: 'host'
chain_post_disable: new loop through list of 'next' in reverse order:
                     'host'
chain_post_disable: break because 'next == bridge' 'host'
chain_post_disable: after !list_is_last block, call post_disable for
                     bridge: 'host'
call_post_disable: post_disable bridge: 'host'
call_post_disable: inside 'else if', do post_disable
chain_post_disable: if limit available, set 'bridge = limit': 'host'
chain_post_disable: bridge of the list loop at the beginning: 'panel'
chain_post_disable: after !list_is_last block, call post_disable for
                     bridge: 'panel'
call_post_disable: post_disable bridge: 'panel'
call_post_disable: inside 'if'
call_post_disable: passed second 'if', do atomic_post_disable
panel-s6e88a0-ams427ap24 1a98000.dsi.0: Failed to set display off: -22
panel-s6e88a0-ams427ap24 1a98000.dsi.0: Failed to un-initialize panel:
                                         -22

It's not my idea to go into detailed debugging. I just wanted to say 
that option a) "pre_enable_prev_first" currently doesn't work well, at 
least on my device. I would assume it affects other devices too. Also I 
didn't want to delay Caleb's patch review. However, it might be related 
if the "pre_enable_prev_first" approach doesn't work on disable.

[1] 
https://github.com/msm8916-mainline/linux/blob/msm8916/6.4-rc4/drivers/gpu/drm/panel/msm8916-generated/panel-samsung-s6e88a0-ams427ap24.c
[2] 
https://github.com/torvalds/linux/commit/4fb912e5e19075874379cfcf074d90bd51ebf8ea
[3] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_bridge.c?h=v6.4-rc4

Kind regards,
Jakob