On Thu, 12 Sep 2024, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote: >> Hi >> >> Am 11.09.24 um 20:06 schrieb Tejas Vipin: >>> Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi since >>> monitor HDMI information is available after EDID is parsed. Additionally >>> rewrite the code the code to have fewer indentation levels. >> >> The problem is that the entire logic is outdated. The content >> of cdv_hdmi_detect() should go into cdv_hdmi_get_modes(), the detect_ctx >> callback should be set to drm_connector_helper_detect_from_ddc() and >> cdv_hdmi_detect() should be deleted. The result is that ->detect_ctx >> will detect the presence of a display and ->get_modes will update EDID >> and other properties. > > I guess I didn't get the memo on this one. > > What's the problem with reading the EDID at detect? The subsequent > drm_edid_connector_add_modes() called from .get_modes() does not need to > read the EDID again. Moreover, in this case .detect() only detects digital displays as reported by EDID. If you postpone that to .get_modes(), the probe helper will still report connected, and invent non-EDID fallback modes. The behaviour changes. > I think it should be fine to do incremental refactors like the patch at > hand (modulo some issues I mention below). Another issue in the patch is that it should also change .get_modes() to not read the EDID again, but just call drm_edid_connector_add_modes(). BR, Jani. > > BR, > Jani. > > >> >> Do you have a device for testing such a change? >> >> Best regards >> Thomas >> >>> >>> Signed-off-by: Tejas Vipin <tejasvipin76@xxxxxxxxx> >>> --- >>> Changes in v2: >>> - Use drm_edid instead of edid >>> >>> Link to v1: https://lore.kernel.org/all/20240910051856.700210-1-tejasvipin76@xxxxxxxxx/ >>> --- >>> drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 24 +++++++++++++----------- >>> 1 file changed, 13 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> index 2d95e0471291..701f8bbd5f2b 100644 >>> --- a/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> +++ b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c >>> @@ -128,23 +128,25 @@ static enum drm_connector_status cdv_hdmi_detect( >>> { >>> struct gma_encoder *gma_encoder = gma_attached_encoder(connector); >>> struct mid_intel_hdmi_priv *hdmi_priv = gma_encoder->dev_priv; >>> - struct edid *edid = NULL; >>> + const struct drm_edid *drm_edid; >>> + int ret; >>> enum drm_connector_status status = connector_status_disconnected; >>> >>> - edid = drm_get_edid(connector, connector->ddc); >>> + drm_edid = drm_edid_read_ddc(connector, connector->ddc); > > Just drm_edid_read() is enough when you're using connector->ddc. > >>> + ret = drm_edid_connector_update(connector, drm_edid); >>> >>> hdmi_priv->has_hdmi_sink = false; >>> hdmi_priv->has_hdmi_audio = false; >>> - if (edid) { >>> - if (edid->input & DRM_EDID_INPUT_DIGITAL) { >>> - status = connector_status_connected; >>> - hdmi_priv->has_hdmi_sink = >>> - drm_detect_hdmi_monitor(edid); >>> - hdmi_priv->has_hdmi_audio = >>> - drm_detect_monitor_audio(edid); >>> - } >>> - kfree(edid); >>> + if (ret) > > This error path leaks the EDID. > >>> + return status; >>> + >>> + if (drm_edid_is_digital(drm_edid)) { >>> + status = connector_status_connected; >>> + hdmi_priv->has_hdmi_sink = connector->display_info.is_hdmi; >>> + hdmi_priv->has_hdmi_audio = connector->display_info.has_audio; >>> } >>> + drm_edid_free(drm_edid); >>> + >>> return status; >>> } >>> -- Jani Nikula, Intel
