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

[Jonas Karlman <jonas@kwiboo.se>]

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