| Message ID | 20250115-bridge-connector-v1-25-9a2fecd886a6@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: Various quality of life improvements | expand |
On Wed, Jan 15, 2025 at 10:05:32PM +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. > > 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 | 21 +++++++++++++-------- > include/drm/drm_atomic.h | 14 ++++++++++++++ > 3 files changed, 32 insertions(+), 8 deletions(-) > > 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 c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > static int drm_atomic_bridge_check(struct drm_bridge *bridge, > struct drm_crtc_state *crtc_state, > struct drm_connector_state *conn_state) > { > + struct drm_bridge_state *bridge_state; > + int ret; > + > + bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > + bridge); It felt like an error to me to call this function for a non-atomic bridges, until I fully followed the code path to find that it will return NULL if the bridge isn't registered as a private object. BTW: if my grep-foo isn't deceiving me, we currently have 34 non-atomic bridges out of 90. Should we start forcebly updating them to use atomic interface in attempt to drop the mode_fixup() and other non-atomic callbacks? > + 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) { > - struct drm_bridge_state *bridge_state; > - int ret; > - > - bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > - bridge); > - if (WARN_ON(!bridge_state)) > - return -EINVAL; > - > ret = bridge->funcs->atomic_check(bridge, bridge_state, > crtc_state, conn_state); > if (ret) > return ret; > } else if (bridge->funcs->mode_fixup) {
Hi, On Thu, Jan 16, 2025 at 03:04:19AM +0200, Dmitry Baryshkov wrote: > On Wed, Jan 15, 2025 at 10:05:32PM +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. > > > > 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 | 21 +++++++++++++-------- > > include/drm/drm_atomic.h | 14 ++++++++++++++ > > 3 files changed, 32 insertions(+), 8 deletions(-) > > > > 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 c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > > > static int drm_atomic_bridge_check(struct drm_bridge *bridge, > > struct drm_crtc_state *crtc_state, > > struct drm_connector_state *conn_state) > > { > > + struct drm_bridge_state *bridge_state; > > + int ret; > > + > > + bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > > + bridge); > > It felt like an error to me to call this function for a non-atomic > bridges, until I fully followed the code path to find that it will > return NULL if the bridge isn't registered as a private object. Yeah.. I wasn't too sure what to do about this one either. I think it would be more consistent to always have a state properly filled, even if we have !atomic drivers. It's what happens with the rest of the framework. But also, I have no idea what the side-effects might be. One thing though: a driver having an atomic_check callback is not an indication of whether it supports atomic mode-setting or not. atomic_check is optional, so we can have atomic drivers without atomic_check. > BTW: if my grep-foo isn't deceiving me, we currently have 34 non-atomic > bridges out of 90. Should we start forcebly updating them to use atomic > interface in attempt to drop the mode_fixup() and other non-atomic > callbacks? Maybe? I'm not sure forcing anyone to anything really helps. sii8620 for example is going to be a fun one, and I'd rather stay away from it :) Maxime
On Thu, Jan 16, 2025 at 09:42:54AM +0100, Maxime Ripard wrote: > Hi, > > On Thu, Jan 16, 2025 at 03:04:19AM +0200, Dmitry Baryshkov wrote: > > On Wed, Jan 15, 2025 at 10:05:32PM +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. > > > > > > 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 | 21 +++++++++++++-------- > > > include/drm/drm_atomic.h | 14 ++++++++++++++ > > > 3 files changed, 32 insertions(+), 8 deletions(-) > > > > > > 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 c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644 > > > --- a/drivers/gpu/drm/drm_bridge.c > > > +++ b/drivers/gpu/drm/drm_bridge.c > > > @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > > > > > static int drm_atomic_bridge_check(struct drm_bridge *bridge, > > > struct drm_crtc_state *crtc_state, > > > struct drm_connector_state *conn_state) > > > { > > > + struct drm_bridge_state *bridge_state; > > > + int ret; > > > + > > > + bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, > > > + bridge); > > > > It felt like an error to me to call this function for a non-atomic > > bridges, until I fully followed the code path to find that it will > > return NULL if the bridge isn't registered as a private object. > > Yeah.. I wasn't too sure what to do about this one either. I think it > would be more consistent to always have a state properly filled, even if > we have !atomic drivers. It's what happens with the rest of the > framework. Well... Unlike other parts of the framework there is no state for non-atomic bridges. Of course we can probably fix that by using default helpers if the callbacks are not provided. > But also, I have no idea what the side-effects might be. > > One thing though: a driver having an atomic_check callback is not an > indication of whether it supports atomic mode-setting or not. > atomic_check is optional, so we can have atomic drivers without > atomic_check. Yeah. The framework uses the presence of the .atomic_reset() callback in order to register the bridge as a private object, maning state management & co.
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 c937980d6591fd98e33e37d799ebf84e7e6c5529..069c105aa59636c64caffbefcf482133b0db97d9 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -829,19 +829,24 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); static int drm_atomic_bridge_check(struct drm_bridge *bridge, struct drm_crtc_state *crtc_state, struct drm_connector_state *conn_state) { + struct drm_bridge_state *bridge_state; + int ret; + + 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) { - struct drm_bridge_state *bridge_state; - int ret; - - bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, - bridge); - if (WARN_ON(!bridge_state)) - return -EINVAL; - ret = bridge->funcs->atomic_check(bridge, bridge_state, crtc_state, conn_state); if (ret) return ret; } else if (bridge->funcs->mode_fixup) { diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index bd7959ae312c99c0a0034d36378ae44f04f6a374..b2c5868a3a66280ffc7437fa7a8613079402facd 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -1166,10 +1166,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 | 21 +++++++++++++-------- include/drm/drm_atomic.h | 14 ++++++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-)