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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux