| Message ID | 20210302162403.983585-2-hverkuil-cisco@xs4all.nl |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2,1/6] drm: drm_bridge: add connector_attach/detach bridge ops | expand |
Hi Hans, On 02/03/2021 18:23, Hans Verkuil wrote: > Add bridge connector_attach/detach ops. These ops are called when a > bridge is attached or detached to a drm_connector. These ops can be > used to register and unregister an HDMI CEC adapter for a bridge that > supports CEC. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/gpu/drm/drm_bridge_connector.c | 9 +++++++++ > include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > index 791379816837..07db71d4f5b3 100644 > --- a/drivers/gpu/drm/drm_bridge_connector.c > +++ b/drivers/gpu/drm/drm_bridge_connector.c > @@ -203,6 +203,11 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector) > { > struct drm_bridge_connector *bridge_connector = > to_drm_bridge_connector(connector); > + struct drm_bridge *bridge; > + > + drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge) > + if (bridge->funcs->connector_detach) > + bridge->funcs->connector_detach(bridge, connector); > > if (bridge_connector->bridge_hpd) { > struct drm_bridge *hpd = bridge_connector->bridge_hpd; > @@ -375,6 +380,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > connector->polled = DRM_CONNECTOR_POLL_CONNECT > | DRM_CONNECTOR_POLL_DISCONNECT; > > + drm_for_each_bridge_in_chain(encoder, bridge) > + if (bridge->funcs->connector_attach) > + bridge->funcs->connector_attach(bridge, connector); > + > return connector; > } > EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 2195daa289d2..3320a6ebd253 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -629,6 +629,33 @@ struct drm_bridge_funcs { > * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops. > */ > void (*hpd_disable)(struct drm_bridge *bridge); > + > + /** > + * @connector_attach: > + * > + * This callback is invoked whenever our bridge is being attached to a > + * &drm_connector. This is where an HDMI CEC adapter can be registered. > + * Note that this callback expects that this op always succeeds. Since > + * HDMI CEC support is an optional feature, any failure to register a > + * CEC adapter must be ignored since video output will still work > + * without CEC. > + * Even if CEC support is optional, the callback itself is generic. Wouldn't it be better to make this function return an error, and for CEC, just return 0 if CEC won't get registered correctly? Also, I personally like things to fail if something doesn't go right, instead of continuing, if that thing is never supposed to happen in normal situations. E.g. if CEC registration fails because we're out of memory, I think the op should fail too. Tomi
On 16/04/2021 09:46, Tomi Valkeinen wrote: > Hi Hans, > > On 02/03/2021 18:23, Hans Verkuil wrote: >> Add bridge connector_attach/detach ops. These ops are called when a >> bridge is attached or detached to a drm_connector. These ops can be >> used to register and unregister an HDMI CEC adapter for a bridge that >> supports CEC. >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> drivers/gpu/drm/drm_bridge_connector.c | 9 +++++++++ >> include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c >> index 791379816837..07db71d4f5b3 100644 >> --- a/drivers/gpu/drm/drm_bridge_connector.c >> +++ b/drivers/gpu/drm/drm_bridge_connector.c >> @@ -203,6 +203,11 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector) >> { >> struct drm_bridge_connector *bridge_connector = >> to_drm_bridge_connector(connector); >> + struct drm_bridge *bridge; >> + >> + drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge) >> + if (bridge->funcs->connector_detach) >> + bridge->funcs->connector_detach(bridge, connector); >> >> if (bridge_connector->bridge_hpd) { >> struct drm_bridge *hpd = bridge_connector->bridge_hpd; >> @@ -375,6 +380,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >> connector->polled = DRM_CONNECTOR_POLL_CONNECT >> | DRM_CONNECTOR_POLL_DISCONNECT; >> >> + drm_for_each_bridge_in_chain(encoder, bridge) >> + if (bridge->funcs->connector_attach) >> + bridge->funcs->connector_attach(bridge, connector); >> + >> return connector; >> } >> EXPORT_SYMBOL_GPL(drm_bridge_connector_init); >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index 2195daa289d2..3320a6ebd253 100644 >> --- a/include/drm/drm_bridge.h >> +++ b/include/drm/drm_bridge.h >> @@ -629,6 +629,33 @@ struct drm_bridge_funcs { >> * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops. >> */ >> void (*hpd_disable)(struct drm_bridge *bridge); >> + >> + /** >> + * @connector_attach: >> + * >> + * This callback is invoked whenever our bridge is being attached to a >> + * &drm_connector. This is where an HDMI CEC adapter can be registered. >> + * Note that this callback expects that this op always succeeds. Since >> + * HDMI CEC support is an optional feature, any failure to register a >> + * CEC adapter must be ignored since video output will still work >> + * without CEC. >> + * > > Even if CEC support is optional, the callback itself is generic. > Wouldn't it be better to make this function return an error, and for > CEC, just return 0 if CEC won't get registered correctly? I'll do that. > > Also, I personally like things to fail if something doesn't go right, > instead of continuing, if that thing is never supposed to happen in > normal situations. E.g. if CEC registration fails because we're out of > memory, I think the op should fail too. If that happens you have no video output. And that's a lot more important than CEC! As you suggested, I'll have the cec connector_attach just return 0. Regards, Hans > > Tomi >
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 791379816837..07db71d4f5b3 100644 --- a/drivers/gpu/drm/drm_bridge_connector.c +++ b/drivers/gpu/drm/drm_bridge_connector.c @@ -203,6 +203,11 @@ static void drm_bridge_connector_destroy(struct drm_connector *connector) { struct drm_bridge_connector *bridge_connector = to_drm_bridge_connector(connector); + struct drm_bridge *bridge; + + drm_for_each_bridge_in_chain(bridge_connector->encoder, bridge) + if (bridge->funcs->connector_detach) + bridge->funcs->connector_detach(bridge, connector); if (bridge_connector->bridge_hpd) { struct drm_bridge *hpd = bridge_connector->bridge_hpd; @@ -375,6 +380,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT; + drm_for_each_bridge_in_chain(encoder, bridge) + if (bridge->funcs->connector_attach) + bridge->funcs->connector_attach(bridge, connector); + return connector; } EXPORT_SYMBOL_GPL(drm_bridge_connector_init); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 2195daa289d2..3320a6ebd253 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -629,6 +629,33 @@ struct drm_bridge_funcs { * the DRM_BRIDGE_OP_HPD flag in their &drm_bridge->ops. */ void (*hpd_disable)(struct drm_bridge *bridge); + + /** + * @connector_attach: + * + * This callback is invoked whenever our bridge is being attached to a + * &drm_connector. This is where an HDMI CEC adapter can be registered. + * Note that this callback expects that this op always succeeds. Since + * HDMI CEC support is an optional feature, any failure to register a + * CEC adapter must be ignored since video output will still work + * without CEC. + * + * The @connector_attach callback is optional. + */ + void (*connector_attach)(struct drm_bridge *bridge, + struct drm_connector *conn); + + /** + * @connector_detach: + * + * This callback is invoked whenever our bridge is being detached from a + * &drm_connector. This is where an HDMI CEC adapter can be + * unregistered. + * + * The @connector_detach callback is optional. + */ + void (*connector_detach)(struct drm_bridge *bridge, + struct drm_connector *conn); }; /**
Add bridge connector_attach/detach ops. These ops are called when a bridge is attached or detached to a drm_connector. These ops can be used to register and unregister an HDMI CEC adapter for a bridge that supports CEC. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/gpu/drm/drm_bridge_connector.c | 9 +++++++++ include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)