Re: [PATCH v3] drm/bridge: sii902x: Fix HDMI detection with DRM_BRIDGE_ATTACH_NO_CONNECTOR

[Devarsh Thakkar <devarsht@ti.com>]

Hi Tomi,

On 22/10/25 12:19, Tomi Valkeinen wrote:
> Hi,
> 

Thanks for the review.

> On 07/10/2025 14:23, Devarsh Thakkar wrote:
>> The SII902x HDMI bridge driver wasn't working properly with drivers that
>> use the newer bridge connector architecture with the
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR flag, like TIDSS.  This caused HDMI audio to
>> fail since the driver wasn't properly setting the sink_is_hdmi flag when
>> the bridge was attached without a connector since .get_modes() is never
>> called in this case. Fix it by setting sink_is_hdmi flag when reading
>> the EDID block itself.
>>
>> Fixes: 3de47e1309c2 ("drm/bridge: sii902x: use display info is_hdmi")
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V3: Use drm_edid_connector_update without edid NULL check
>> V2: Use drm_edid_connector_update to detect HDMI
>>
>> Link to V2:
>> https://lore.kernel.org/all/20251006150714.3144368-1-devarsht@ti.com/
>> Link to V1:
>> https://lore.kernel.org/all/20251003143642.4072918-1-devarsht@ti.com/
>>
>>   drivers/gpu/drm/bridge/sii902x.c | 5 ++---
>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
>> index d537b1d036fb..bb613d4c281f 100644
>> --- a/drivers/gpu/drm/bridge/sii902x.c
>> +++ b/drivers/gpu/drm/bridge/sii902x.c
>> @@ -296,6 +296,8 @@ static const struct drm_edid *sii902x_edid_read(struct sii902x *sii902x,
>>   	mutex_lock(&sii902x->mutex);
>>   
>>   	drm_edid = drm_edid_read_ddc(connector, sii902x->i2cmux->adapter[0]);
>> +	drm_edid_connector_update(connector, drm_edid);
>> +	sii902x->sink_is_hdmi = connector->display_info.is_hdmi;
>>   
>>   	mutex_unlock(&sii902x->mutex);
> 
> I'm not sure if this is right. If I'm not mistaken, the
> drm_bridge_connector.c calls sii902x_edid_read, and then does a
> drm_edid_connector_update(). So drm_edid_connector_update() will be
> called twice, and also, I don't think the bridge should be updating
> connector's data when using DRM_BRIDGE_ATTACH_NO_CONNECTOR.
> 

Thanks for catching this!. I missed out to catch this while addressing 
previous review comment.

> Maybe either drop sink_is_hdmi, and just look at
> connector->display_info.is_hdmi, or if that doesn't work for some
> reason, just get the "is-hdmi" flag from the edid data directly.
> 

Yes I plan to drop sink_is_hdmi flag altogether and directly use 
connector->display_info.is_hdmi in v4.

I think we should be able to use 
drm_atomic_get_new_connector_for_encoder in .atomic_enable to get the 
connector->display_info.is_hdmi and this should work for both the legacy 
and DISPLAY_BRIDGE_ATTACH_NO_CONNECTOR scenarios and set the output mode 
accordingly. I will try out with this approach.

Kindly let me know if any concerns with above approach.

Regards
Devarsh

>   Tomi
> 
>> @@ -309,14 +311,11 @@ static int sii902x_get_modes(struct drm_connector *connector)
>>   	int num = 0;
>>   
>>   	drm_edid = sii902x_edid_read(sii902x, connector);
>> -	drm_edid_connector_update(connector, drm_edid);
>>   	if (drm_edid) {
>>   		num = drm_edid_connector_add_modes(connector);
>>   		drm_edid_free(drm_edid);
>>   	}
>>   
>> -	sii902x->sink_is_hdmi = connector->display_info.is_hdmi;
>> -
>>   	return num;
>>   }
>>   
>