| Message ID | 20250204-bridge-connector-v2-35-35dd6c834e08@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Various quality of life improvements | expand |
Hi, On Tue, Feb 4, 2025 at 7:01 AM Maxime Ripard <mripard@kernel.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@kernel.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. :( > @@ -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.
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@kernel.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@kernel.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? > > @@ -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. Maxime
Hi, On Tue, Feb 11, 2025 at 5:14 AM Maxime Ripard <mripard@kernel.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@kernel.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@kernel.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
Hi, On Tue, Feb 4, 2025 at 7:01 AM Maxime Ripard <mripard@kernel.org> wrote: > > @@ -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); Oh. I haven't tested yet (my device is at home, but I think there is an easy solution to my deadlock problems. Drop the modeset locks here and just pass NULL for the state. We only end up here if "pdata->refclk" is not NULL. Then we only use the passed state if "pdata->refclk" _is_ NULL.
Hi, On Tue, Feb 11, 2025 at 2:16 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Feb 4, 2025 at 7:01 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > @@ -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); > > Oh. I haven't tested yet (my device is at home, but I think there is > an easy solution to my deadlock problems. Drop the modeset locks here > and just pass NULL for the state. We only end up here if > "pdata->refclk" is not NULL. Then we only use the passed state if > "pdata->refclk" _is_ NULL. I can confirm this works. At the very least I was able to boot up both with a hardcoded panel and with "edp-panel" and both worked. Seems like a good solution for your patch. Long term we should probably get rid of the support for working without a "refclk". It's theoretically possible to use the hardware that way and some super early prototypes I worked on used that. ...but it's a bad idea and I'm fairly certain nobody is _actually_ using it. That means that the code for handling it is likely not tested and may have bugs. -Doug
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 497e7212d7719a03045d055b7069f9a10c7f3877..590504bd2746fd1f9b78a8203e762d154dfb0bb2 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -242,15 +242,16 @@ static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, u8 buf[2] = { val & 0xff, val >> 8 }; regmap_bulk_write(pdata->regmap, reg, buf, ARRAY_SIZE(buf)); } -static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) +static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state) { u32 bit_rate_khz, clk_freq_khz; struct drm_display_mode *mode = - &pdata->bridge.encoder->crtc->state->adjusted_mode; + &bridge_state->crtc->state->adjusted_mode; bit_rate_khz = mode->clock * mipi_dsi_pixel_format_to_bpp(pdata->dsi->format); clk_freq_khz = bit_rate_khz / (pdata->dsi->lanes * 2); @@ -273,11 +274,12 @@ static const u32 ti_sn_bridge_dsiclk_lut[] = { 416000000, 486000000, 460800000, }; -static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) +static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state) { int i; u32 refclk_rate; const u32 *refclk_lut; size_t refclk_lut_size; @@ -286,11 +288,11 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) refclk_rate = clk_get_rate(pdata->refclk); refclk_lut = ti_sn_bridge_refclk_lut; refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_refclk_lut); clk_prepare_enable(pdata->refclk); } else { - refclk_rate = ti_sn_bridge_get_dsi_freq(pdata) * 1000; + refclk_rate = ti_sn_bridge_get_dsi_freq(pdata, bridge_state) * 1000; refclk_lut = ti_sn_bridge_dsiclk_lut; refclk_lut_size = ARRAY_SIZE(ti_sn_bridge_dsiclk_lut); } /* for i equals to refclk_lut_size means default frequency */ @@ -310,16 +312,17 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn65dsi86 *pdata) * regardless of its actual sourcing. */ pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; } -static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) +static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state) { mutex_lock(&pdata->comms_mutex); /* configure bridge ref_clk */ - ti_sn_bridge_set_refclk_freq(pdata); + ti_sn_bridge_set_refclk_freq(pdata, bridge_state); /* * HPD on this bridge chip is a bit useless. This is an eDP bridge * so the HPD is an internal signal that's only there to signal that * the panel is done powering up. ...but the bridge chip debounces @@ -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); + } return ret; } static int __maybe_unused ti_sn65dsi86_suspend(struct device *dev) @@ -821,16 +827,17 @@ static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge, /* disable video stream */ regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0); } -static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) +static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state) { unsigned int bit_rate_mhz, clk_freq_mhz; unsigned int val; struct drm_display_mode *mode = - &pdata->bridge.encoder->crtc->state->adjusted_mode; + &bridge_state->crtc->state->adjusted_mode; /* set DSIA clk frequency */ bit_rate_mhz = (mode->clock / 1000) * mipi_dsi_pixel_format_to_bpp(pdata->dsi->format); clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2); @@ -856,16 +863,18 @@ static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) */ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp) +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state, + unsigned int bpp) { unsigned int bit_rate_khz, dp_rate_mhz; unsigned int i; struct drm_display_mode *mode = - &pdata->bridge.encoder->crtc->state->adjusted_mode; + &bridge_state->crtc->state->adjusted_mode; /* Calculate minimum bit rate based on our pixel clock. */ bit_rate_khz = mode->clock * bpp; /* Calculate minimum DP data rate, taking 80% as per DP spec */ @@ -960,14 +969,15 @@ static unsigned int ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata) } return valid_rates; } -static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata) +static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata, + struct drm_bridge_state *bridge_state) { struct drm_display_mode *mode = - &pdata->bridge.encoder->crtc->state->adjusted_mode; + &bridge_state->crtc->state->adjusted_mode; u8 hsync_polarity = 0, vsync_polarity = 0; if (mode->flags & DRM_MODE_FLAG_NHSYNC) hsync_polarity = CHA_HSYNC_POLARITY; if (mode->flags & DRM_MODE_FLAG_NVSYNC) @@ -1076,19 +1086,21 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx, static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_atomic_state *state) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct drm_bridge_state *bridge_state; struct drm_connector *connector; const char *last_err_str = "No supported DP rate"; unsigned int valid_rates; int dp_rate_idx; unsigned int val; int ret = -EINVAL; int max_dp_lanes; unsigned int bpp; + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); if (!connector) { dev_err_ratelimited(pdata->dev, "Could not get the connector\n"); return; @@ -1105,11 +1117,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, regmap_write(pdata->regmap, SN_LN_ASSIGN_REG, pdata->ln_assign); regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, LN_POLRS_MASK, pdata->ln_polrs << LN_POLRS_OFFSET); /* set dsi clk frequency value */ - ti_sn_bridge_set_dsi_rate(pdata); + ti_sn_bridge_set_dsi_rate(pdata, bridge_state); /* * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled for eDP panels. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel @@ -1140,11 +1152,11 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, val); valid_rates = ti_sn_bridge_read_valid_rates(pdata); /* Train until we run out of rates */ - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bridge_state, bpp); dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); dp_rate_idx++) { if (!(valid_rates & BIT(dp_rate_idx))) continue; @@ -1156,26 +1168,28 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, DRM_DEV_ERROR(pdata->dev, "%s (%d)\n", last_err_str, ret); return; } /* config video parameters */ - ti_sn_bridge_set_video_timings(pdata); + ti_sn_bridge_set_video_timings(pdata, bridge_state); /* enable video stream */ regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, VSTREAM_ENABLE); } static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *state) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct drm_bridge_state *bridge_state = + drm_atomic_get_new_bridge_state(state, bridge); pm_runtime_get_sync(pdata->dev); if (!pdata->refclk) - ti_sn65dsi86_enable_comms(pdata); + ti_sn65dsi86_enable_comms(pdata, bridge_state); /* td7: min 100 us after enable before DSI data */ usleep_range(100, 110); }
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@kernel.org> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 50 ++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 18 deletions(-)