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
