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

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

Posted by Devarsh Thakkar 4 months ago

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);
 
@@ -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;
 }
 
-- 
2.39.1

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

Posted by Tomi Valkeinen 3 months, 2 weeks ago

Hi,

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.

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.

 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;
>  }
>

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

Posted by Devarsh Thakkar 3 months, 2 weeks ago

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;
>>   }
>>   
>

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

Posted by Dmitry Baryshkov 4 months ago

On Tue, Oct 07, 2025 at 04:53:09PM +0530, 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);
>  
> @@ -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);
>  	}

The EDID read / free can also be dropped, they don't serve any purpose
now.

>  
> -	sii902x->sink_is_hdmi = connector->display_info.is_hdmi;
> -
>  	return num;
>  }
>  
> -- 
> 2.39.1
> 

-- 
With best wishes
Dmitry

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

Posted by Devarsh Thakkar 4 months ago

Hi Dmitry,

Thanks for the quick reviews.

On 09/10/25 01:48, Dmitry Baryshkov wrote:
> On Tue, Oct 07, 2025 at 04:53:09PM +0530, 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);
>>   
>> @@ -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);
>>   	}
> 
> The EDID read / free can also be dropped, they don't serve any purpose
> now.
> 

Sorry I did not understand this clearly.
I have already removed drm_edid_connector_update from sii902x_get_modes 
since now it is taken care inside sii902x_edid_read itself which is also 
called by .get_modes (sii902x_get_modes).

We still need to read edid inside sii902x_get_modes as for the legacy 
driver scenario which don't use DRM_BRIDGE_ATTACH_NO_CONNECTOR as in 
those cases .get_modes will get called but bridge function .edid_read 
won't be called. Also as the caller is supposed to free the edid 
allocated so I have preserved drm_edid_free call in .get_modes as 
drm_edid_connector_add_modes is allocating a new drm_edid pointer already.

Kindly let me know if there is a misunderstanding here.

Regards
Devarsh

>>   
>> -	sii902x->sink_is_hdmi = connector->display_info.is_hdmi;
>> -
>>   	return num;
>>   }
>>   
>> -- 
>> 2.39.1
>>
>

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

Posted by Devarsh Thakkar 3 months, 3 weeks ago

Hi Dmitry,

On 09/10/25 20:39, Devarsh Thakkar wrote:
> Hi Dmitry,
> 
> Thanks for the quick reviews.
> 
> On 09/10/25 01:48, Dmitry Baryshkov wrote:
>> On Tue, Oct 07, 2025 at 04:53:09PM +0530, 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);
>>> @@ -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);
>>>       }
>>
>> The EDID read / free can also be dropped, they don't serve any purpose
>> now.
>>
> 
> Sorry I did not understand this clearly.
> I have already removed drm_edid_connector_update from sii902x_get_modes 
> since now it is taken care inside sii902x_edid_read itself which is also 
> called by .get_modes (sii902x_get_modes).
> 
> We still need to read edid inside sii902x_get_modes as for the legacy 
> driver scenario which don't use DRM_BRIDGE_ATTACH_NO_CONNECTOR as in 
> those cases .get_modes will get called but bridge function .edid_read 
> won't be called. Also as the caller is supposed to free the edid 
> allocated so I have preserved drm_edid_free call in .get_modes as 
> drm_edid_connector_add_modes is allocating a new drm_edid pointer already.
> 
> Kindly let me know if there is a misunderstanding here.
> 

Just thought to check if we are good w.r.t above.
To summarize, as mentioned above, I think both sii902x_get_modes and 
sii902x_bridge_edid_read need to read edid. The former is called only 
for legacy driver scenario (and not for drivers using 
DRM_BRIDGE_ATTACH_NO_CONNECTOR) and the latter is called for the display 
drivers using DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Regards
Devarsh

> Regards
> Devarsh
> 
>>> -    sii902x->sink_is_hdmi = connector->display_info.is_hdmi;
>>> -
>>>       return num;
>>>   }
>>> -- 
>>> 2.39.1
>>>
>>
>