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);
 
-- 
2.46.0

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

Neil

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.

Regards,
Jonas

> 
> Neil

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
>

On 2024-09-20 09:04, neil.armstrong@linaro.org wrote:
> 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.

I do not fully understand when or how that could happen.

The dw_hdmi_detect() -> phy.ops->read_hpd() call chain only return
connected status when HPD is high, for what I can see from all current
integrations.

The default dw_hdmi_phy_read_hpd() used by all but meson should only
return connected status when the HPD is high:

	return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
		connector_status_connected : connector_status_disconnected;

DRM_CONNECTOR_POLL_HPD is also used for all integrations so there should
not be any polling happening that would trigger multiple detect calls.

I guess this could/should be improved to also check for the polled type
to not cause multiple unnecessary EDID reads, in case a future
integration need to poll connector status.

> 
> 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 ?

Using a work queue to debounce reacting on HPD event for >100 ms when
HPD goes from high to low and a few ms when it goes from low to high
could probably further prevent unnecessary detect calls, and also help
avoid a possible unnecessary disable/enable cycle.

I have not seen anything other in core that handles the EDID refresh in
any special way, but I may have missed something?

Will try to use a debounce work queue to delay any calls to
helper_hpd_irq_event() and drm_bridge_hpd_notify() and see if that could
improve the HPD handling.

Regards,
Jonas

> 
> Maxime, do you have an opinion on this ?
> 
> Neil
> 
>>
>> Regards,
>> Jonas
>>
>>>
>>> Neil
>>
>