| Message ID | 20250213-bridge-connector-v3-31-e71598f49c8f@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: Various quality of life improvements | expand |
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. > > 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. The same comment as for v2: It's not very precise: connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); Is that unreliable somehow? > > 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 <mripard@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 >
On Thu, Feb 13, 2025 at 06:32:39PM +0200, Dmitry Baryshkov 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. > > > > 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. > > The same comment as for v2: > > It's not very precise: > > connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); > > Is that unreliable somehow? Then I'm not sure what you want here (and in v2) either. Do you want me to drop this patch because it's redundant, drop it, something else? Maxime
On Fri, Feb 14, 2025 at 02:06:34PM +0100, Maxime Ripard wrote: > On Thu, Feb 13, 2025 at 06:32:39PM +0200, Dmitry Baryshkov 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. > > > > > > 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. > > > > The same comment as for v2: > > > > It's not very precise: > > > > connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); > > > > Is that unreliable somehow? > > Then I'm not sure what you want here (and in v2) either. Do you want me > to drop this patch because it's redundant, drop it, something else? No, basically to expand the commit message. It states that there was no reliable way to get the connector, but there was one. I'd mention that getting CRTC and connector via the state results in a boilerplate code and complicates bridge drivers.
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. -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 <mripard@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 >
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 <mripard@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
Hi, Thanks for your answer 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. That work stemed from this series https://lore.kernel.org/all/20250210132620.42263-1-herve.codina@bootlin.com/ and in particular: https://lore.kernel.org/all/20250210132620.42263-5-herve.codina@bootlin.com/ Bridges, outside of the modesetting code path, don't have a way to access the drm_atomic_state since drm_bridge_state->state is typically cleared after swap_state. So accessing the connectors and CRTCs don't work anymore. In this particular case, we needed to access those from the bridge interrupt handler. Maxime
On Tue, Feb 18, 2025 at 11:23:00AM +0100, Maxime Ripard wrote: > Hi, > > Thanks for your answer > > 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. > > That work stemed from this series > https://lore.kernel.org/all/20250210132620.42263-1-herve.codina@bootlin.com/ > > and in particular: > https://lore.kernel.org/all/20250210132620.42263-5-herve.codina@bootlin.com/ > > Bridges, outside of the modesetting code path, don't have a way to > access the drm_atomic_state since drm_bridge_state->state is typically > cleared after swap_state. So accessing the connectors and CRTCs don't > work anymore. > > In this particular case, we needed to access those from the bridge > interrupt handler. Uh for interrupt handler you can't use anything stored in state objects anyway. So I'm even more confused. -Sima
On Mon, Feb 17, 2025 at 09:36:35PM +0200, Dmitry Baryshkov wrote: > 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. Yeah definitely don't replicate this across drivers, it needs to be a helper function. What I meant to say is that we have a way to do this already, and it also works for dp mst drivers. So full blast fun of dynamically selecting the right encoder _and_ hotplug/unplug of connectors. Unless bridge is very special, this should work for bridges too. -Sima
On Wed, Feb 19, 2025 at 02:35:25PM +0100, Simona Vetter wrote: > On Tue, Feb 18, 2025 at 11:23:00AM +0100, Maxime Ripard wrote: > > Hi, > > > > Thanks for your answer > > > > 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. > > > > That work stemed from this series > > https://lore.kernel.org/all/20250210132620.42263-1-herve.codina@bootlin.com/ > > > > and in particular: > > https://lore.kernel.org/all/20250210132620.42263-5-herve.codina@bootlin.com/ > > > > Bridges, outside of the modesetting code path, don't have a way to > > access the drm_atomic_state since drm_bridge_state->state is typically > > cleared after swap_state. So accessing the connectors and CRTCs don't > > work anymore. > > > > In this particular case, we needed to access those from the bridge > > interrupt handler. > > Uh for interrupt handler you can't use anything stored in state objects > anyway. So I'm even more confused. Why not? As long as we're in a threaded handler, and take the proper locks, what's wrong with it, and how is it fundamentally different than, idk, cec or audio hooks? Maxime
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;
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. 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 <mripard@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(+)