| Message ID | 20240908132823.3308029-10-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: bridge: dw_hdmi: Misc enable/disable, CEC and EDID cleanup | expand |
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 >> >
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);
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(+)