Re: [PATCH v2 02/10] drm: bridge: dw_hdmi: Only notify connected status on HPD interrupt

[Jonas Karlman]

Hi Neil,

On 2024-09-09 15:16, Neil Armstrong wrote:
> On 08/09/2024 15:28, Jonas Karlman wrote:
>> drm_helper_hpd_irq_event() and drm_bridge_hpd_notify() may incorrectly
>> be called with a connected status when HPD is high and RX sense is
>> changed. This typically happen when the HDMI cable is unplugged, shortly
>> before the HPD is changed to low.
>>
>> Fix this by only notify connected status on the HPD interrupt when HPD
>> is going high, not on the RX sense interrupt when RX sense is changed.
>>
>> Fixes: da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug event on link change")
>> Signed-off-by: Jonas Karlman <jonas@xxxxxxxxx>
>> ---
>> v2: New patch
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 9e7f86a0bf5c..055fc9848df4 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3123,7 +3123,8 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>>   			mutex_unlock(&hdmi->cec_notifier_mutex);
>>   		}
>>   
>> -		if (phy_stat & HDMI_PHY_HPD)
>> +		if ((intr_stat & HDMI_IH_PHY_STAT0_HPD) &&
>> +		    (phy_stat & HDMI_PHY_HPD))
>>   			status = connector_status_connected;
>>   
>>   		if (!(phy_stat & (HDMI_PHY_HPD | HDMI_PHY_RX_SENSE)))
> 
> Perhaps this should be also checked for the other lines checking HDMI_PHY_HPD ?

I think this is the only place we need to check to match the original
intent of da09daf88108 ("drm: bridge: dw_hdmi: only trigger hotplug
event on link change").

The interrupt pattern I can see when physically unplugging are:
- RX interrupt:  HPD=high RX=low  -> triggered connected event
- HPD interrupt: HPD=low  RX=low  -> trigger disconnected event

Based on the commit message the intent was to trigger hotplug event:
- when HPD goes high (plugin)
- when both HPD and RX sense has gone low (plugout)

For plugin we should only trigger when HPD=high at HPD interrupt, as is
done after this patch, to avoid unnecessary events when RX changes.

For plugout the event should be triggered when both HPD=low and RX=low,
that can happen at either HPD or RX interrupt.

This should probably be revisited in future, I think we should ignore
RX sense and instead use a work queue and a small debounce timeout after
HPD interrupt or similar, to better support EDID refresh. Something for
a future series.

Regards,
Jonas

> 
> Neil