| Message ID | 20210428132545.1205162-2-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: hdmi: improve hdmi4 CEC, add CEC for hdmi5 | expand |
On 28/04/2021 16:25, 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 | 25 +++++++++++++++++++++++- > include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > index 791379816837..0676677badfe 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; > @@ -318,6 +323,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > struct i2c_adapter *ddc = NULL; > struct drm_bridge *bridge; > int connector_type; > + int ret; > > bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > if (!bridge_connector) > @@ -375,6 +381,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > connector->polled = DRM_CONNECTOR_POLL_CONNECT > | DRM_CONNECTOR_POLL_DISCONNECT; > > - return connector; > + ret = 0; > + /* call connector_attach for all bridges */ > + drm_for_each_bridge_in_chain(encoder, bridge) { > + if (!bridge->funcs->connector_attach) > + continue; > + ret = bridge->funcs->connector_attach(bridge, connector); > + if (ret) > + break; > + } > + if (!ret) > + return connector; > + > + /* on error, detach any previously successfully attached connectors */ > + list_for_each_entry_continue_reverse(bridge, &(encoder)->bridge_chain, No need for parenthesis in (encoder) here. > + chain_node) > + if (bridge->funcs->connector_detach) > + bridge->funcs->connector_detach(bridge, connector); > + return ERR_PTR(ret); > } > EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 2195daa289d2..333fbc3a03e9 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. > + * > + * The @connector_attach callback is optional. > + * > + * RETURNS: > + * > + * Zero on success, error code on failure. > + */ > + int (*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); > }; > > /** > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> I can take this series as it's mostly omapdrm, but we'll need a reviewed-by/acked-by from a maintainer for this patch. Tomi
On 04/05/2021 10:26, Tomi Valkeinen wrote: > On 28/04/2021 16:25, 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 | 25 +++++++++++++++++++++++- >> include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ >> 2 files changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c >> index 791379816837..0676677badfe 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; >> @@ -318,6 +323,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >> struct i2c_adapter *ddc = NULL; >> struct drm_bridge *bridge; >> int connector_type; >> + int ret; >> >> bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); >> if (!bridge_connector) >> @@ -375,6 +381,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, >> connector->polled = DRM_CONNECTOR_POLL_CONNECT >> | DRM_CONNECTOR_POLL_DISCONNECT; >> >> - return connector; >> + ret = 0; >> + /* call connector_attach for all bridges */ >> + drm_for_each_bridge_in_chain(encoder, bridge) { >> + if (!bridge->funcs->connector_attach) >> + continue; >> + ret = bridge->funcs->connector_attach(bridge, connector); >> + if (ret) >> + break; >> + } >> + if (!ret) >> + return connector; >> + >> + /* on error, detach any previously successfully attached connectors */ >> + list_for_each_entry_continue_reverse(bridge, &(encoder)->bridge_chain, > > No need for parenthesis in (encoder) here. > >> + chain_node) >> + if (bridge->funcs->connector_detach) >> + bridge->funcs->connector_detach(bridge, connector); >> + return ERR_PTR(ret); >> } >> EXPORT_SYMBOL_GPL(drm_bridge_connector_init); >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h >> index 2195daa289d2..333fbc3a03e9 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. >> + * >> + * The @connector_attach callback is optional. >> + * >> + * RETURNS: >> + * >> + * Zero on success, error code on failure. >> + */ >> + int (*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); >> }; >> >> /** >> > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > I can take this series as it's mostly omapdrm, but we'll need a > reviewed-by/acked-by from a maintainer for this patch. Laurent, aren't you the drm bridge maintainer? If so, can you review/ack this patch so Tomi can merge it? Regards, Hans > > Tomi >
Hi Hans, On Thu, May 27, 2021 at 09:22:12AM +0200, Hans Verkuil wrote: > On 04/05/2021 10:26, Tomi Valkeinen wrote: > > On 28/04/2021 16:25, 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 | 25 +++++++++++++++++++++++- > >> include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ > >> 2 files changed, 51 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c > >> index 791379816837..0676677badfe 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; > >> @@ -318,6 +323,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > >> struct i2c_adapter *ddc = NULL; > >> struct drm_bridge *bridge; > >> int connector_type; > >> + int ret; > >> > >> bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); > >> if (!bridge_connector) > >> @@ -375,6 +381,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, > >> connector->polled = DRM_CONNECTOR_POLL_CONNECT > >> | DRM_CONNECTOR_POLL_DISCONNECT; > >> > >> - return connector; > >> + ret = 0; > >> + /* call connector_attach for all bridges */ > >> + drm_for_each_bridge_in_chain(encoder, bridge) { > >> + if (!bridge->funcs->connector_attach) > >> + continue; > >> + ret = bridge->funcs->connector_attach(bridge, connector); > >> + if (ret) > >> + break; > >> + } > >> + if (!ret) > >> + return connector; > >> + > >> + /* on error, detach any previously successfully attached connectors */ > >> + list_for_each_entry_continue_reverse(bridge, &(encoder)->bridge_chain, > > > > No need for parenthesis in (encoder) here. > > > >> + chain_node) > >> + if (bridge->funcs->connector_detach) > >> + bridge->funcs->connector_detach(bridge, connector); > >> + return ERR_PTR(ret); > >> } > >> EXPORT_SYMBOL_GPL(drm_bridge_connector_init); > >> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > >> index 2195daa289d2..333fbc3a03e9 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. > >> + * > >> + * The @connector_attach callback is optional. > >> + * > >> + * RETURNS: > >> + * > >> + * Zero on success, error code on failure. > >> + */ > >> + int (*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); > >> }; > >> > >> /** > >> > > > > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > > > I can take this series as it's mostly omapdrm, but we'll need a > > reviewed-by/acked-by from a maintainer for this patch. > > Laurent, aren't you the drm bridge maintainer? I'm merely a reviewer, but one without much free time these days :-( > If so, can you review/ack this patch so Tomi can merge it? Overall, I'm not fond of giving access to the connector to individual bridges. It breaks the abstraction layers, and seems to be a bit of a hack. On the other hand, a proper solution would likely involve lots of work, so I'm not rejecting this, but I fear these operations will be abused and we'll regret it later.
diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c index 791379816837..0676677badfe 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; @@ -318,6 +323,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, struct i2c_adapter *ddc = NULL; struct drm_bridge *bridge; int connector_type; + int ret; bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL); if (!bridge_connector) @@ -375,6 +381,23 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm, connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT; - return connector; + ret = 0; + /* call connector_attach for all bridges */ + drm_for_each_bridge_in_chain(encoder, bridge) { + if (!bridge->funcs->connector_attach) + continue; + ret = bridge->funcs->connector_attach(bridge, connector); + if (ret) + break; + } + if (!ret) + return connector; + + /* on error, detach any previously successfully attached connectors */ + list_for_each_entry_continue_reverse(bridge, &(encoder)->bridge_chain, + chain_node) + if (bridge->funcs->connector_detach) + bridge->funcs->connector_detach(bridge, connector); + return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(drm_bridge_connector_init); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 2195daa289d2..333fbc3a03e9 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. + * + * The @connector_attach callback is optional. + * + * RETURNS: + * + * Zero on success, error code on failure. + */ + int (*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 | 25 +++++++++++++++++++++++- include/drm/drm_bridge.h | 27 ++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-)