Re: [PATCH v2 35/35] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

[Doug Anderson]

Hi,

On Tue, Feb 4, 2025 at 7:01 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> The TI sn65dsi86 driver follows the drm_encoder->crtc pointer that is
> deprecated and shouldn't be used by atomic drivers.
>
> This was due to the fact that we did't have any other alternative to
> retrieve the CRTC pointer. Fortunately, the crtc pointer is now provided
> in the bridge state, so we can move to atomic callbacks and drop that
> deprecated pointer usage.
>
> Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 50 ++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 18 deletions(-)

I'm about out of time for now, but I finally managed to at least test
this and can confirm it _doesn't_ work. If I take the rest of the
series without this patch then things seem OK. When I add this patch
then the splash screen on my Chromebook comes up but the browser never
boots. :(


> @@ -374,12 +377,15 @@ static int __maybe_unused ti_sn65dsi86_resume(struct device *dev)
>          * panel (including the aux channel) w/out any need for an input clock
>          * so we can do it in resume which lets us read the EDID before
>          * pre_enable(). Without a reference clock we need the MIPI reference
>          * clock so reading early doesn't work.
>          */
> -       if (pdata->refclk)
> -               ti_sn65dsi86_enable_comms(pdata);
> +       if (pdata->refclk) {
> +               drm_modeset_lock(&pdata->bridge.base.lock, NULL);
> +               ti_sn65dsi86_enable_comms(pdata, drm_bridge_get_current_state(&pdata->bridge));
> +               drm_modeset_unlock(&pdata->bridge.base.lock);
> +       }

I believe grabbing the locks here is the problem. Sure enough,
commenting that out fixes things. Also, if I wait long enough:

[  247.151951] INFO: task DrmThread:1838 blocked for more than 122 seconds.
[  247.158862]       Tainted: G        W
6.14.0-rc1-00226-g4144859f9421 #1
[  247.166474] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs"
disables this message.
[  247.174541] task:DrmThread       state:D stack:0     pid:1838
tgid:1756  ppid:1      task_flags:0x400040 flags:0x00000a0d
[  247.185904] Call trace:
[  247.188450]  __switch_to+0x12c/0x1e0 (T)
[  247.192520]  __schedule+0x2d0/0x4a0
[  247.196132]  schedule_preempt_disabled+0x50/0x88
[  247.200904]  __ww_mutex_lock+0x3d8/0xa68
[  247.204970]  __ww_mutex_lock_slowpath+0x24/0x38
[  247.209653]  ww_mutex_lock+0x7c/0x140
[  247.213441]  drm_modeset_lock+0xd4/0x110
[  247.217493]  ti_sn65dsi86_resume+0x78/0xe0
[  247.221730]  __rpm_callback+0x84/0x148
[  247.225619]  rpm_callback+0x34/0x98
[  247.229232]  rpm_resume+0x320/0x488
[  247.232842]  __pm_runtime_resume+0x54/0xa8
[  247.237073]  ti_sn_bridge_gpio_get+0x48/0xb8
[  247.241486]  gpiod_get_raw_value_commit+0x70/0x178
[  247.246436]  gpiod_get_value_cansleep+0x34/0x88
[  247.251122]  panel_edp_resume+0xf0/0x270
[  247.255187]  __rpm_callback+0x84/0x148
[  247.259072]  rpm_callback+0x34/0x98
[  247.262685]  rpm_resume+0x320/0x488
[  247.266293]  __pm_runtime_resume+0x54/0xa8
[  247.270536]  panel_edp_prepare+0x2c/0x68
[  247.274591]  drm_panel_prepare+0x54/0x118
[  247.278743]  panel_bridge_atomic_pre_enable+0x60/0x78
[  247.283965]  drm_atomic_bridge_chain_pre_enable+0x110/0x168
[  247.289723]  drm_atomic_helper_commit_modeset_enables+0x204/0x288
[  247.296005]  msm_atomic_commit_tail+0x1b4/0x510
[  247.300690]  commit_tail+0xa8/0x178
[  247.304298]  drm_atomic_helper_commit+0xec/0x180
[  247.309066]  drm_atomic_commit+0xa8/0xf8
[  247.313125]  drm_mode_atomic_ioctl+0x718/0xcd8
[  247.317717]  drm_ioctl+0x1ec/0x450
[  247.321248]  __arm64_sys_ioctl+0x3e4/0x4d8
[  247.325494]  invoke_syscall+0x4c/0xf0
[  247.329284]  do_el0_svc+0x70/0xf8
[  247.332717]  el0_svc+0x38/0x68
[  247.335886]  el0t_64_sync_handler+0x20/0x128
[  247.340296]  el0t_64_sync+0x1b0/0x1b8

I guess the problem is that the HPD gpio (which is given to the panel)
is implemented by ti-sn65dsi86. It's been a long time, but probably we
don't need to "enable comms" just to access a GPIO, but there's only
one level of runtime PM. Maybe the fix would be to separately enable
pm_runtime for the various sub-devices and the GPIO? ...and then the
"aux" channel enables comms and the bridge one also grabs a PM runtime
reference to the aux sub-device? Not sure I have time to dig into that
myself now.