On Mon, Feb 17, 2025 at 05:41:24PM +0100, Simona Vetter wrote:
> On Thu, Feb 13, 2025 at 03:43:50PM +0100, Maxime Ripard wrote:
> > Now that connectors are no longer necessarily created by the bridges
> > drivers themselves but might be created by drm_bridge_connector, it's
> > pretty hard for bridge drivers to retrieve pointers to the connector and
> > CRTC they are attached to.
> >
> > Indeed, the only way to retrieve the CRTC is to follow the drm_bridge
> > encoder field, and then the drm_encoder crtc field, both of them being
> > deprecated.
>
> Eh, this isn't quite how this works. So unless bridges have become very
> dynamic and gained flexible routing the bridge(->bridge->...)->encoder
> chain is static. And the crtc for an encoder you find by walking the
> connector states in a drm_atomic_state, finding the right one that points
> at your encoder, and then return the ->crtc pointer from that connector
> state.
>
> It's a bit bonkers, but I think it's better to compute this than adding
> more pointers that potentially diverge. Unless there's a grand plan here,
> but then I think we want some safety checks that all the pointers never
> get out of sync here.
Luca is working on bridges hotplug, so connectors are dynamic now. And
at least my humble opinion is that we'd better provide the corresponding
pointers rather than having a lot of boilerplate code in the drivers.
(there are enough drivers which look up connector and/or CRTC) and there
are even more drivers (I think, I haven't actually checked the source
tree) that could have benefited from thaving the connector or CRTC in
the state. Instead they store a pointer to the connector or perform
other fancy tricks.
> -Sima
>
> >
> > And for the connector, since we can have multiple connectors attached to
> > a CRTC, we don't really have a reliable way to get it.
> >
> > Let's provide both pointers in the drm_bridge_state structure so we
> > don't have to follow deprecated, non-atomic, pointers, and be more
> > consistent with the other KMS entities.
> >
> > Signed-off-by: Maxime Ripard <mrip...@kernel.org>
> > ---
> > drivers/gpu/drm/drm_atomic_state_helper.c | 5 +++++
> > drivers/gpu/drm/drm_bridge.c | 5 +++++
> > include/drm/drm_atomic.h | 14 ++++++++++++++
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> > b/drivers/gpu/drm/drm_atomic_state_helper.c
> > index
> > 519228eb109533d2596e899a57b571fa0995824f..66661dca077215b78dffca7bc1712f56d35e3918
> > 100644
> > --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> > @@ -777,10 +777,15 @@
> > EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_state);
> > * that don't subclass the bridge state.
> > */
> > void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge,
> > struct drm_bridge_state *state)
> > {
> > + if (state->connector) {
> > + drm_connector_put(state->connector);
> > + state->connector = NULL;
> > + }
> > +
> > kfree(state);
> > }
> > EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state);
> >
> > /**
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index
> > b6d24092674c8fa33d9b6ebab9ece0f91fb8f8ea..db2e9834939217d65720ab7a2f82a9ca3db796b0
> > 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -812,10 +812,15 @@ static int drm_atomic_bridge_check(struct drm_bridge
> > *bridge,
> > bridge_state =
> > drm_atomic_get_new_bridge_state(crtc_state->state,
> > bridge);
> > if (WARN_ON(!bridge_state))
> > return -EINVAL;
> >
> > + bridge_state->crtc = crtc_state->crtc;
> > +
> > + drm_connector_get(conn_state->connector);
> > + bridge_state->connector = conn_state->connector;
> > +
> > if (bridge->funcs->atomic_check) {
> > ret = bridge->funcs->atomic_check(bridge, bridge_state,
> > crtc_state,
> > conn_state);
> > if (ret)
> > return ret;
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index
> > 4c673f0698fef6b60f77db980378d5e88e0e250e..293e2538a428bc14013d7fabea57a6b858ed7b47
> > 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -1216,10 +1216,24 @@ struct drm_bridge_state {
> > /**
> > * @bridge: the bridge this state refers to
> > */
> > struct drm_bridge *bridge;
> >
> > + /**
> > + * @crtc: CRTC the bridge is connected to, NULL if disabled.
> > + *
> > + * Do not change this directly.
> > + */
> > + struct drm_crtc *crtc;
> > +
> > + /**
> > + * @connector: The connector the bridge is connected to, NULL if
> > disabled.
> > + *
> > + * Do not change this directly.
> > + */
> > + struct drm_connector *connector;
> > +
> > /**
> > * @input_bus_cfg: input bus configuration
> > */
> > struct drm_bus_cfg input_bus_cfg;
> >
> >
> > --
> > 2.48.0
> >
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
With best wishes
Dmitry
