On Mon, Dec 02, 2024 at 02:27:49PM +0100, Maxime Ripard wrote: > Hi, > > On Sun, Dec 01, 2024 at 02:44:13AM +0200, Dmitry Baryshkov wrote: > > Use the helper function to update the connector's information. This > > makes sure that HDMI-related events are handled in a generic way. > > Currently it is limited to the HDMI state reporting to the sound system. > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > --- > > drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c > > index d0a9aff7ad43016647493263c00d593296a1e3ad..d83f587ab69f4b8f7d5c37a00777f11da8301bc1 100644 > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > @@ -401,13 +401,16 @@ static void vc4_hdmi_handle_hotplug(struct vc4_hdmi *vc4_hdmi, > > */ > > > > if (status == connector_status_disconnected) { > > + drm_atomic_helper_connector_hdmi_update_edid(connector, NULL); > > cec_phys_addr_invalidate(vc4_hdmi->cec_adap); > > return; > > } > > > > drm_edid = drm_edid_read_ddc(connector, vc4_hdmi->ddc); > > > > - drm_edid_connector_update(connector, drm_edid); > > + // TODO: use drm_atomic_helper_connector_hdmi_update() once it gains > > + // CEC support > > + drm_atomic_helper_connector_hdmi_update_edid(connector, drm_edid); > > So, it's not just about EDID, and I think we shouldn't really focus on > that either. > > As that patch points out, even if we only consider EDID's, we have > different path depending on the connector status. It shouldn't be up to > the drivers to get this right. > > What I had in mind was something like a > drm_atomic_helper_connector_hdmi_hotplug function that would obviously > deal with EDID only here, but would expand to CEC, scrambling, etc. > later on. I thought about it, after our discussion, but in the end I had to implement the EDID-specific function, using edid == NULL as "disconnected" event. The issue is pretty simple: there is no standard way to get EDID from the connector. The devices can call drm_edid_read(), drm_edid_read_ddc(connector->ddc) or (especially embedded bridges) use drm_edid_read_custom(). Of course we can go with the functional way and add the .read_edid(drm_connector) callback to the HDMI funcs. Then the drm_atomic_helper_connector_hdmi_hotplug() function can read EDID on its own. Also the function that you proposed perfectly fits the HPD notification callbacks, but which function should be called from the .get_modes()? The _hdmi_hotplug() doesn't fit there. Do we still end up with both drm_atomic_helper_connector_hdmi_hotplug() and drm_atomic_helper_connector_hdmi_update_edid()? > > And would cover both the connected/disconnected cases. > > Maxime -- With best wishes Dmitry