Message-ID: <CAD=FV=WriQU4bGrPrrZDncKS0HmL4sfTguGBs9DbVx6yg2ezXw@mail.gmail.com>
Date: Tue, 11 Feb 2025 09:52:55 -0800
From: Doug Anderson <dianders@...omium.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>, 
	Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>, 
	Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Stephen Boyd <swboyd@...omium.org>
Subject: Re: [PATCH v2 35/35] drm/bridge: ti-sn65dsi86: Use bridge_state crtc pointer

Hi,

On Tue, Feb 11, 2025 at 5:14 AM Maxime Ripard <mripard@...nel.org> 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@...nel.org> 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@...nel.org>
> > > ---
> > >  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

Powered by blists - more mailing lists