diff mbox series

[PATCHv2,1/6] drm: drm_bridge: add connector_attach/detach bridge ops

Message ID 20210302162403.983585-2-hverkuil-cisco@xs4all.nl
State New
Headers show
Series drm/omap: hdmi: improve hdmi4 CEC, add CEC for hdmi5 | expand

Commit Message

Hans Verkuil March 2, 2021, 4:23 p.m. UTC
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(+)

Comments

Tomi Valkeinen April 16, 2021, 7:46 a.m. UTC | #1
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
Hans Verkuil April 28, 2021, 12:10 p.m. UTC | #2
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 mbox series

Patch

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);
 };
 
 /**