| Message ID | 20250204-bridge-connector-v2-34-35dd6c834e08@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: Various quality of life improvements | expand |
On Tue, Feb 04, 2025 at 03:58:02PM +0100, Maxime Ripard wrote: > The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c > index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 > --- a/drivers/gpu/drm/bridge/tc358768.c > +++ b/drivers/gpu/drm/bridge/tc358768.c > @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) > ret = tc358768_clear_error(priv); > if (ret) > dev_warn(priv->dev, "Software disable failed: %d\n", ret); > } > > +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, > + struct drm_atomic_state *state) > +{ > + tc358768_bridge_disable(bridge); > +} > + Please change corresponding functions into atomic_disable() and atomic_post_disable(). Calling sites have access to the atomic state, so there is no need to have yet another wrapper. > static void tc358768_bridge_post_disable(struct drm_bridge *bridge) > { > struct tc358768_priv *priv = bridge_to_tc358768(bridge); > > tc358768_hw_disable(priv); > } > > +static void tc358768_bridge_atomic_post_disable(struct drm_bridge *bridge, > + struct drm_atomic_state *state) > +{ > + tc358768_bridge_post_disable(bridge); > +} > +
On Sun, Feb 09, 2025 at 09:13:36AM +0200, Dmitry Baryshkov wrote: > On Tue, Feb 04, 2025 at 03:58:02PM +0100, Maxime Ripard wrote: > > The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c > > index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 > > --- a/drivers/gpu/drm/bridge/tc358768.c > > +++ b/drivers/gpu/drm/bridge/tc358768.c > > @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) > > ret = tc358768_clear_error(priv); > > if (ret) > > dev_warn(priv->dev, "Software disable failed: %d\n", ret); > > } > > > > +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state) > > +{ > > + tc358768_bridge_disable(bridge); > > +} > > + > > Please change corresponding functions into atomic_disable() and > atomic_post_disable(). Calling sites have access to the atomic state, so > there is no need to have yet another wrapper. It's pretty hard to do (at least without the hardware), both tc358768_bridge_disable() and tc358768_bridge_post_disable() have multiple call sites in the driver, and passing a state enabling the bridge doesn't make sense for those. I guess I can still drop that patch. Maxime
On Tue, Feb 11, 2025 at 03:33:58PM +0100, Maxime Ripard wrote: > On Sun, Feb 09, 2025 at 09:13:36AM +0200, Dmitry Baryshkov wrote: > > On Tue, Feb 04, 2025 at 03:58:02PM +0100, Maxime Ripard wrote: > > > The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c > > > index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 > > > --- a/drivers/gpu/drm/bridge/tc358768.c > > > +++ b/drivers/gpu/drm/bridge/tc358768.c > > > @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) > > > ret = tc358768_clear_error(priv); > > > if (ret) > > > dev_warn(priv->dev, "Software disable failed: %d\n", ret); > > > } > > > > > > +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, > > > + struct drm_atomic_state *state) > > > +{ > > > + tc358768_bridge_disable(bridge); > > > +} > > > + > > > > Please change corresponding functions into atomic_disable() and > > atomic_post_disable(). Calling sites have access to the atomic state, so > > there is no need to have yet another wrapper. > > It's pretty hard to do (at least without the hardware), both > tc358768_bridge_disable() and tc358768_bridge_post_disable() have > multiple call sites in the driver, and passing a state enabling the > bridge doesn't make sense for those. I think it makes sense. The function knows that the bridge needs to be disabled. The state is totally unused (or it can be used to get connectors / CRTC / etc). > > I guess I can still drop that patch. > > Maxime
On Wed, Feb 12, 2025 at 02:38:52AM +0200, Dmitry Baryshkov wrote: > On Tue, Feb 11, 2025 at 03:33:58PM +0100, Maxime Ripard wrote: > > On Sun, Feb 09, 2025 at 09:13:36AM +0200, Dmitry Baryshkov wrote: > > > On Tue, Feb 04, 2025 at 03:58:02PM +0100, Maxime Ripard wrote: > > > > The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- > > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c > > > > index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 > > > > --- a/drivers/gpu/drm/bridge/tc358768.c > > > > +++ b/drivers/gpu/drm/bridge/tc358768.c > > > > @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) > > > > ret = tc358768_clear_error(priv); > > > > if (ret) > > > > dev_warn(priv->dev, "Software disable failed: %d\n", ret); > > > > } > > > > > > > > +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, > > > > + struct drm_atomic_state *state) > > > > +{ > > > > + tc358768_bridge_disable(bridge); > > > > +} > > > > + > > > > > > Please change corresponding functions into atomic_disable() and > > > atomic_post_disable(). Calling sites have access to the atomic state, so > > > there is no need to have yet another wrapper. > > > > It's pretty hard to do (at least without the hardware), both > > tc358768_bridge_disable() and tc358768_bridge_post_disable() have > > multiple call sites in the driver, and passing a state enabling the > > bridge doesn't make sense for those. > > I think it makes sense. The function knows that the bridge needs to be > disabled. The state is totally unused (or it can be used to get > connectors / CRTC / etc). That's the thing though, if we were to pass the state, it would be a state where the bridge is enabled, like, it would have an active CRTC. In a disable path, you wouldn't have it. Another idea would be to just drop the call to disable the bridge, the assumption is that we can't fail in atomic_enable, so no driver actually tries to mitigate a failure. I'm not sure why this one would need to. Maxime
On Wed, Feb 12, 2025 at 09:24:21AM +0100, Maxime Ripard wrote: > On Wed, Feb 12, 2025 at 02:38:52AM +0200, Dmitry Baryshkov wrote: > > On Tue, Feb 11, 2025 at 03:33:58PM +0100, Maxime Ripard wrote: > > > On Sun, Feb 09, 2025 at 09:13:36AM +0200, Dmitry Baryshkov wrote: > > > > On Tue, Feb 04, 2025 at 03:58:02PM +0100, Maxime Ripard wrote: > > > > > The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- > > > > > 1 file changed, 23 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c > > > > > index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 > > > > > --- a/drivers/gpu/drm/bridge/tc358768.c > > > > > +++ b/drivers/gpu/drm/bridge/tc358768.c > > > > > @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) > > > > > ret = tc358768_clear_error(priv); > > > > > if (ret) > > > > > dev_warn(priv->dev, "Software disable failed: %d\n", ret); > > > > > } > > > > > > > > > > +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, > > > > > + struct drm_atomic_state *state) > > > > > +{ > > > > > + tc358768_bridge_disable(bridge); > > > > > +} > > > > > + > > > > > > > > Please change corresponding functions into atomic_disable() and > > > > atomic_post_disable(). Calling sites have access to the atomic state, so > > > > there is no need to have yet another wrapper. > > > > > > It's pretty hard to do (at least without the hardware), both > > > tc358768_bridge_disable() and tc358768_bridge_post_disable() have > > > multiple call sites in the driver, and passing a state enabling the > > > bridge doesn't make sense for those. > > > > I think it makes sense. The function knows that the bridge needs to be > > disabled. The state is totally unused (or it can be used to get > > connectors / CRTC / etc). > > That's the thing though, if we were to pass the state, it would be a > state where the bridge is enabled, like, it would have an active CRTC. > In a disable path, you wouldn't have it. > > Another idea would be to just drop the call to disable the bridge, the > assumption is that we can't fail in atomic_enable, so no driver actually > tries to mitigate a failure. I'm not sure why this one would need to. SGTM too.
diff --git a/drivers/gpu/drm/bridge/tc358768.c b/drivers/gpu/drm/bridge/tc358768.c index 6db18d1e8824dd7d387211d6d1e668645cf88bbe..6ff6b29e8075d7c6fa0b74b4fec83c5230512d96 100644 --- a/drivers/gpu/drm/bridge/tc358768.c +++ b/drivers/gpu/drm/bridge/tc358768.c @@ -601,17 +601,29 @@ static void tc358768_bridge_disable(struct drm_bridge *bridge) ret = tc358768_clear_error(priv); if (ret) dev_warn(priv->dev, "Software disable failed: %d\n", ret); } +static void tc358768_bridge_atomic_disable(struct drm_bridge *bridge, + struct drm_atomic_state *state) +{ + tc358768_bridge_disable(bridge); +} + static void tc358768_bridge_post_disable(struct drm_bridge *bridge) { struct tc358768_priv *priv = bridge_to_tc358768(bridge); tc358768_hw_disable(priv); } +static void tc358768_bridge_atomic_post_disable(struct drm_bridge *bridge, + struct drm_atomic_state *state) +{ + tc358768_bridge_post_disable(bridge); +} + static int tc358768_setup_pll(struct tc358768_priv *priv, const struct drm_display_mode *mode) { u32 fbd, prd, frs; int ret; @@ -681,17 +693,19 @@ static u32 tc358768_dsi_bytes_to_ns(struct tc358768_priv *priv, u32 val) u64 n = priv->dsiclk / 4 * priv->dsi_lanes; return (u32)div_u64(m, n); } -static void tc358768_bridge_pre_enable(struct drm_bridge *bridge) +static void tc358768_bridge_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state) { struct tc358768_priv *priv = bridge_to_tc358768(bridge); struct mipi_dsi_device *dsi_dev = priv->output.dev; unsigned long mode_flags = dsi_dev->mode_flags; u32 val, val2, lptxcnt, hact, data_type; s32 raw_val; + struct drm_bridge_state *bridge_state; const struct drm_display_mode *mode; u32 hsbyteclk_ps, dsiclk_ps, ui_ps; u32 dsiclk, hsbyteclk; int ret, i; struct videomode vm; @@ -718,11 +732,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge) dev_err(dev, "Software reset failed: %d\n", ret); tc358768_hw_disable(priv); return; } - mode = &bridge->encoder->crtc->state->adjusted_mode; + bridge_state = drm_atomic_get_new_bridge_state(state, bridge); + mode = &bridge_state->crtc->state->adjusted_mode; ret = tc358768_setup_pll(priv, mode); if (ret) { dev_err(dev, "PLL setup failed: %d\n", ret); tc358768_hw_disable(priv); return; @@ -1082,11 +1097,12 @@ static void tc358768_bridge_pre_enable(struct drm_bridge *bridge) tc358768_bridge_disable(bridge); tc358768_bridge_post_disable(bridge); } } -static void tc358768_bridge_enable(struct drm_bridge *bridge) +static void tc358768_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_atomic_state *state) { struct tc358768_priv *priv = bridge_to_tc358768(bridge); int ret; if (!priv->enabled) { @@ -1165,14 +1181,14 @@ static bool tc358768_mode_fixup(struct drm_bridge *bridge, static const struct drm_bridge_funcs tc358768_bridge_funcs = { .attach = tc358768_bridge_attach, .mode_valid = tc358768_bridge_mode_valid, .mode_fixup = tc358768_mode_fixup, - .pre_enable = tc358768_bridge_pre_enable, - .enable = tc358768_bridge_enable, - .disable = tc358768_bridge_disable, - .post_disable = tc358768_bridge_post_disable, + .atomic_pre_enable = tc358768_bridge_atomic_pre_enable, + .atomic_enable = tc358768_bridge_atomic_enable, + .atomic_disable = tc358768_bridge_atomic_disable, + .atomic_post_disable = tc358768_bridge_atomic_post_disable, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_get_input_bus_fmts = tc358768_atomic_get_input_bus_fmts,
The tc358768 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/tc358768.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-)