Re: [PATCH v3 12/13] drm: bridge: dw_hdmi: Use dw_hdmi_connector_status_update()

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Tuesday, 28 April 2026 23:47:40 Central European Summer Time Jonas Karlman wrote:
> Hi Nicolas,
> 
> On 4/28/2026 1:53 PM, Nicolas Frattaroli wrote:
> > On Friday, 3 April 2026 20:52:58 Central European Summer Time Jonas Karlman wrote:
> >> Update connector EDID and CEC phys addr from detect and force funcs to
> >> ensure that userspace always have access to latest read EDID after a
> >> sink use a HPD low voltage pulse to indicate that EDID has changed.
> >>
> >> With EDID being updated in detect and force funcs, there should no
> >> longer be a need to re-read EDID in get_modes funcs, so drop it.
> >>
> >> This change make the dw-hdmi connector work more closely like the bridge
> >> connector does with a hdmi bridge.
> >>
> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> >> ---
> >> v3: Reworked 'Update EDID during hotplug processing' patch
> >> ---
> >>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 24 +++++++++++++----------
> >>  1 file changed, 14 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >> index 0d42fdf9a386..5b5654ef6015 100644
> >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> >> @@ -2474,33 +2474,36 @@ dw_hdmi_connector_status_update(struct drm_connector *connector,
> >>  	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
> >>  	const struct drm_edid *drm_edid;
> >>  
> >> +	if (status == connector_status_disconnected) {
> >> +		drm_edid_connector_update(connector, NULL);
> >> +		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> >> +		return;
> >> +	}
> >> +
> >>  	drm_edid = dw_hdmi_edid_read(hdmi, connector);
> >>  	drm_edid_connector_update(connector, drm_edid);
> >>  	drm_edid_free(drm_edid);
> >>  
> >> -	cec_notifier_set_phys_addr(hdmi->cec_notifier,
> >> -				   connector->display_info.source_physical_address);
> >> +	if (status == connector_status_connected)
> >> +		cec_notifier_set_phys_addr(hdmi->cec_notifier,
> >> +				connector->display_info.source_physical_address);
> >>  }
> >>  
> >>  static enum drm_connector_status
> >>  dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
> >>  {
> >> -	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> >> -					     connector);
> >> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
> >>  	enum drm_connector_status status;
> >>  
> >>  	status = dw_hdmi_detect(hdmi);
> >>  
> >> -	if (status == connector_status_disconnected)
> >> -		cec_notifier_phys_addr_invalidate(hdmi->cec_notifier);
> >> +	dw_hdmi_connector_status_update(connector, status);
> >>  
> >>  	return status;
> >>  }
> >>  
> >>  static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
> >>  {
> >> -	dw_hdmi_connector_status_update(connector, connector->status);
> >> -
> >>  	return drm_edid_connector_add_modes(connector);
> >>  }
> > 
> > Are we absolutely sure status_update is called before this, so that the
> > EDID doesn't need to be re-read? E.g. drm_connector_helper_get_modes
> > does do an EDID read beforehand.
> 
> With the change to re-read EDID at .detect() we should not need to
> re-read EDID at .get_modes(). This also closely match how the bridge
> connector handle EDID update at .detect() and not .get_modes() for hdmi
> bridges.
> 
> At least following call paths should ensure EDID have been re-read:
> 
> drm_helper_probe_single_connector_modes():
>  -> connector->funcs->force()
>     -> dw_hdmi_connector_force()
>        -> dw_hdmi_connector_status_update()
>           -> drm_edid_connector_update()             <<-- here
>  -> drm_helper_probe_detect()
>     -> detect_connector_status()
>        -> connector->funcs->detect();
>           -> dw_hdmi_connector_detect()
>              -> dw_hdmi_connector_status_update()
>                 -> drm_edid_connector_update()       <<-- or here
>  -> drm_helper_probe_get_modes()
>     -> connector_funcs->get_modes()
>        -> dw_hdmi_connector_get_modes()              <<-- now updated
> 
> drm_helper_hpd_irq_event():
>  -> detect_connector_status()
>     -> connector->funcs->detect();
>        -> dw_hdmi_connector_detect()
>           -> dw_hdmi_connector_status_update()
>              -> drm_edid_connector_update()          <<-- also after HPD
> 
> > Speaking of which: if we had connector->ddc set, we could just use
> > drm_connector_helper_get_modes for this callback. But maybe it's not
> > worth spending the time refactoring the resource handling around
> > hdmi->ddc when the eventual goal is to refactor this all into using
> > a bridge connector anyway.
> 
> Correct, hopefully all connector related functions can be dropped from
> dw-hdmi in a near future when dw-hdmi is converted to a hdmi bridge.
> 
> I currently have a work-in-progress snapshot at [1] where dw-hdmi have
> been converted to a hdmi bridge and a bridge connector is used instead.
> 
> The changes in this series only aim to align dw-hdmi to the hdmi bridge
> where possible to reduce any functional change in such future series.
> 
> I will likely keep this as-is in a v4 unless there is some objections.

With that explanation, I'm on board with this. So:

Reviewed-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

Kind regards,
Nicolas Frattaroli

> 
> [1] https://github.com/Kwiboo/linux-rockchip/commits/next-20260427-rk-hdmi-v2/
> 
> Regards,
> Jonas
> 
> >>  
> >> @@ -2530,14 +2533,15 @@ static int dw_hdmi_connector_atomic_check(struct drm_connector *connector,
> >>  
> >>  static void dw_hdmi_connector_force(struct drm_connector *connector)
> >>  {
> >> -	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
> >> -					     connector);
> >> +	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, connector);
> >>  
> >>  	mutex_lock(&hdmi->mutex);
> >>  	hdmi->force = connector->force;
> >>  	hdmi->last_connector_result = connector->status;
> >>  	dw_hdmi_update_phy_mask(hdmi);
> >>  	mutex_unlock(&hdmi->mutex);
> >> +
> >> +	dw_hdmi_connector_status_update(connector, connector->status);
> >>  }
> >>  
> >>  static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
> >>
> > 
> > 
> > 
> > 
> 
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip