| Message ID | 20241201-drm-bridge-hdmi-connector-v5-9-b5316e82f61a@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm: add DRM HDMI Codec framework | expand |
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@linaro.org> > --- > 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. And would cover both the connected/disconnected cases. Maxime
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@linaro.org> > > --- > > 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
On Tue, 03 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > 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@linaro.org> >> > --- >> > 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(). Lack of EDID may be a good approximation of disconnected for displays that should have one, but just a reminder that having an EDID should not be the only approximation for connected. This is relevant for the debugfs/firmware EDID case. You'll want to be able to have that, without it implying connected. BR, Jani. > > 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
On Tue, Dec 03, 2024 at 02:27:44PM +0200, Dmitry Baryshkov wrote: > 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@linaro.org> > > > --- > > > 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(). And that's fine, it's to be expected. > 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. Yep, that's definitely what we should do. And then we can make a get_modes helper too that would also use it. > 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()? I'd say both a get_modes helper and a hotplug helper, both using that read_edid hook under the hood. Maxime
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); cec_s_phys_addr(vc4_hdmi->cec_adap, connector->display_info.source_physical_address, false); @@ -487,7 +490,9 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) */ 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); cec_s_phys_addr(vc4_hdmi->cec_adap, connector->display_info.source_physical_address, false); if (!drm_edid)
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@linaro.org> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)