On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
HiSo what if you can probe DDC but can't actually read an EDID of any
Am 12.09.24 um 10:48 schrieb Jani Nikula:
On Thu, 12 Sep 2024, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:With drm_connector_helper_detect_from_ddc() there is already a helper
HiI guess I didn't get the memo on this one.
Am 11.09.24 um 20:06 schrieb Tejas Vipin:
Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi sinceThe problem is that the entire logic is outdated. The content
monitor HDMI information is available after EDID is parsed. Additionally
rewrite the code the code to have fewer indentation levels.
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.
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.
for detection. It makes sense to use it. And if we continue to update
the properties in detect (instead of get_modes), what is the correct
connector_status on errors? Right now and with the patch applied, detect
returns status_disconnected on errors. But this isn't correct if there
actually is a display. By separating detect and get_modes cleanly, we
can detect the display reliably, but also handle errors better than we
currently do in gma500. Get_modes is already expected to update the EDID
property, [1] for detect it's not so clear AFAICT. I think that from a
design perspective, it makes sense to have a read-only function that
only detects the physical state of the connector and a read-write
function that updates the connector's properties. Best regards Thomas
[1]
https://elixir.bootlin.com/linux/v6.10.9/source/include/drm/drm_modeset_helper_vtables.h#L865
kind? IMO that's a detect failure.
Or how about things like CEC attach? Seems natural to do it at
.detect(). Doing it at .get_modes() just seems wrong. However, it needs
the EDID for physical address.
I just don't think one size fits all here.
BR,
Jani.
I think it should be fine to do incremental refactors like the patch at
hand (modulo some issues I mention below).
BR,
Jani.
Do you have a device for testing such a change?Just drm_edid_read() is enough when you're using connector->ddc.
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);
This error path leaks the EDID.+ 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)
+ 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;
}