Re: [PATCH 2/5] host1x: hdmi: Detect whether display is connected with HDMI or DVI

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]

On Wed, Aug 28, 2013 at 03:34:53PM +0300, Mikko Perttunen wrote:
> On 08/28/2013 03:07 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Wed, Aug 28, 2013 at 01:40:56PM +0300, Mikko Perttunen wrote:
> >>Use EDID data to determine whether the display supports HDMI or just DVI.
> >>This used to be hardcoded to be HDMI, which broke support for DVI displays
> >>that couldn't understand the interspersed audio/other data.
> >>
> >>If the EDID data isn't available, default to DVI, which should be a safer
> >>choice.
> >>
> >>Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >>---
> >>  drivers/gpu/host1x/drm/hdmi.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >>diff --git a/drivers/gpu/host1x/drm/hdmi.c b/drivers/gpu/host1x/drm/hdmi.c
> >>index d81fac8..140339b 100644
> >>--- a/drivers/gpu/host1x/drm/hdmi.c
> >>+++ b/drivers/gpu/host1x/drm/hdmi.c
> >>@@ -702,6 +702,14 @@ static int tegra_output_hdmi_enable(struct tegra_output *output)
> >>  	unsigned long value;
> >>  	int retries = 1000;
> >>  	int err;
> >>+	struct drm_property_blob *edid_blob = output->connector.edid_blob_ptr;
> >>+
> >>+	if (edid_blob && edid_blob->data &&
> >>+		drm_detect_hdmi_monitor((struct edid *)edid_blob->data)) {
> >>+		hdmi->dvi = false;
> >>+	} else {
> >>+		hdmi->dvi = true;
> >>+	}
> >>
> >>  	pclk = mode->clock * 1000;
> >>  	h_sync_width = mode->hsync_end - mode->hsync_start;
> >
> >Odd, now that I see that code I remember that there was a similar patch
> >a few months back, but it was never applied for some reason:
> >
> >	http://lists.freedesktop.org/archives/dri-devel/2013-January/033509.html
> >
> >That was already reviewed by me and Jon Mayo, so I'll go ahead and apply
> >that one instead.
> >
> >Thierry
> >
> >* Unknown Key
> >* 0x7F3EB3A1
> >
> 
> That patch seems to cause a warning for me:
> drivers/gpu/host1x/drm/hdmi.c: In function ‘tegra_output_hdmi_enable’:
> drivers/gpu/host1x/drm/hdmi.c:706:2: warning: passing argument 1 of
> ‘drm_detect_hdmi_monitor’ discards ‘const’ qualifier from pointer
> target type [enabled by default]
> include/drm/drm_crtc.h:1037:13: note: expected ‘struct edid *’ but
> argument is of type ‘const struct edid *’

Given the discussion about this back in January, I think the best way,
at least for now, is to keep a copy of the EDID data so that it can
actually be modified.

But looking at the problem more closely, I don't think the patch from
January is quite correct. The problem is that it uses the EDID stored
within the struct tegra_output. That's problematic because that field
is not updated when EDID is probed via DDC. That would make the patch
that you proposed more correct.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]