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
