Hi,
On Tue, Feb 11, 2025 at 5:14 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> On Fri, Feb 07, 2025 at 05:44:38PM -0800, Doug Anderson wrote:
> > 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. :(
>
> Thanks for testing still :)
>
> Could you add your tested-by on the previous patches if you found that
> they were working?
Two of the previous patches didn't compile (which I replied about). I
was going to wait till v3 and then reply with Tested-by on any patches
that were at least exercised on my basic test.
> > > @@ -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.
>
> I don't know the hardware, so I can't really comment, unfortunately.
> I'll drop it if it's broken.
Though it's unsafe, you could drop the locks and replace them with a
comment saying that they should be grabbed here if we can figure out
the deadlock. I don't think the newer code is any less safe without
the locks than the existing code, right?
-Doug
