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

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

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
>