Re: [PATCH v2 09/10] drm: bridge: dw_hdmi: Update EDID during hotplug processing

[neil.armstrong@linaro.org]

On 19/09/2024 22:34, Jonas Karlman wrote:
> Hi Neil,
> 
> On 2024-09-13 10:02, Neil Armstrong wrote:
>> On 08/09/2024 15:28, Jonas Karlman wrote:
>>> Update successfully read EDID during hotplug processing to ensure the
>>> connector diplay_info is always up-to-date.
>>>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> v2: No change
>>> ---
>>>    drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index c19307120909..7bd9f895f03f 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -2457,6 +2457,18 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
>>>    
>>>    	status = dw_hdmi_detect(hdmi);
>>>    
>>> +	/* Update EDID during hotplug processing (force=false) */
>>> +	if (status == connector_status_connected && !force) {
>>> +		const struct drm_edid *drm_edid;
>>> +
>>> +		drm_edid = dw_hdmi_edid_read(hdmi, connector);
>>> +		if (drm_edid)
>>> +			drm_edid_connector_update(connector, drm_edid);
>>> +		cec_notifier_set_phys_addr(hdmi->cec_notifier,
>>> +			connector->display_info.source_physical_address);
>>> +		drm_edid_free(drm_edid);
>>> +	}
>>> +
>>>    	if (status == connector_status_disconnected)
>>>    		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
>>>    
>>
>> I wonder why we should read edid at each dw_hdmi_connector_detect() call,
>> AFAIK it should only be when we have HPD pulses
> 
> That is what this change intends to help do.
> 
> As stated in the short comment EDID is only updated at HPD processing,
> i.e. when force=false. To be on the safe side EDID is also only updated
> here when connected and EDID could be read.
> 
> drm_helper_probe_detect() is called with force=true in the
> fill_modes/get_modes call path that is triggered by userspace
> or the kernel kms client.
> 
> After a HPD interrupt the call to drm_helper_hpd_irq_event() will call
> check_connector_changed() that in turn calls drm_helper_probe_detect()
> with force=false to check/detect if connector status has changed. It is
> in this call chain the EDID may be read and updated in this detect ops.
> 
> Reading EDID here at HPD processing may not be fully needed, however it
> help kernel keep the internal EDID state in better sync with sink when
> userspace does not act on the HOTPLUG=1 uevent.


I understand but if somehow a dw-hdmi integration fails to have HDP working
properly, EDID will be read continuously which is really not what we want.

HDMI 1.4b specifies in Section 8.5 and Appendix A:
============><==========================================
An HDMI Sink shall not assert high voltage level on its Hot Plug Detect pin when the E-EDID
is not available for reading.
An HDMI Sink shall indicate any change to the contents of the E-EDID by driving a low
voltage level pulse on the Hot Plug Detect pin. This pulse shall be at least 100 msec.
============><==========================================

So this is OK with the first sentence, and should also work with the second one because
right after the pulse we will read the EDID again, but I think we should have a much
more robust way to detect those 100ms pulses, no ?

Maxime, do you have an opinion on this ?

Neil

> 
> Regards,
> Jonas
> 
>>
>> Neil
>