drm: i915: Improve behavior in case of broken HDMI EDID

Submitted by Ezequiel Garcia on April 19, 2016, 5:31 p.m.

Details

Message ID 1461087073-14903-1-git-send-email-ezequiel@vanguardiasur.com.ar
State New
Headers show
Series "drm: i915: Improve behavior in case of broken HDMI EDID" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Ezequiel Garcia April 19, 2016, 5:31 p.m.
Currently, our implementation of drm_connector_funcs.detect is
based on getting a valid EDID.

This requirement makes the driver fail to detect connected
connectors in case of EDID corruption, which in turn prevents
from falling back to modes provided by builtin or user-provided
EDIDs.

Let's fix this by calling drm_probe_ddc in drm_connector_funcs.detect,
and do the EDID full reading and parsing in
drm_connector_helper_funcs.get_modes, when it's actually needed.

This patch allows i915 to take advantage of the DRM_LOAD_EDID_FIRMWARE
infrastructure.

Without this patch, any device that fails to provide a valid
EDID will be reported as disconnected (unless the state is forced)
and thus the kernel won't allow to use such device with any mode,
either builtin, user-provided, or the 1024x768 noedid fallback.

Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
This patch supersedes: "drm/i915/hdmi: Fix weak connector detection",
https://patchwork.freedesktop.org/patch/79098/.

 drivers/gpu/drm/i915/intel_hdmi.c | 59 ++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 23 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 616108c4bc3e..aa2f2271394a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1392,36 +1392,17 @@  intel_hdmi_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
-	bool live_status = false;
-	unsigned int try;
-
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
+	struct i2c_adapter *adap;
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
-	for (try = 0; !live_status && try < 9; try++) {
-		if (try)
-			msleep(10);
-		live_status = intel_digital_port_connected(dev_priv,
-				hdmi_to_dig_port(intel_hdmi));
-	}
-
-	if (!live_status)
-		DRM_DEBUG_KMS("Live status not up!");
-
-	intel_hdmi_unset_edid(connector);
-
-	if (intel_hdmi_set_edid(connector, live_status)) {
-		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-
-		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+	adap = intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
+	if (drm_probe_ddc(adap))
 		status = connector_status_connected;
-	} else
+	else
 		status = connector_status_disconnected;
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
-
 	return status;
 }
 
@@ -1442,10 +1423,42 @@  intel_hdmi_force(struct drm_connector *connector)
 	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 }
 
+static void intel_hdmi_detect_edid(struct drm_connector *connector)
+{
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	bool live_status = false;
+	unsigned int try;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
+
+	for (try = 0; !live_status && try < 9; try++) {
+		if (try)
+			msleep(10);
+		live_status = intel_digital_port_connected(dev_priv,
+				hdmi_to_dig_port(intel_hdmi));
+	}
+
+	if (!live_status)
+		DRM_DEBUG_KMS("Live status not up!");
+
+	intel_hdmi_unset_edid(connector);
+	if (intel_hdmi_set_edid(connector, live_status))
+		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
+}
+
 static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
 	struct edid *edid;
 
+	if (!to_intel_connector(connector)->detect_edid)
+		intel_hdmi_detect_edid(connector);
+
 	edid = to_intel_connector(connector)->detect_edid;
 	if (edid == NULL)
 		return 0;

Comments

Daniel,

Thanks a lot for the quick reply!

On 20 Apr 01:34 PM, Daniel Vetter wrote:
> On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
> > Currently, our implementation of drm_connector_funcs.detect is
> > based on getting a valid EDID.
> > 
> > This requirement makes the driver fail to detect connected
> > connectors in case of EDID corruption, which in turn prevents
> > from falling back to modes provided by builtin or user-provided
> > EDIDs.
> 
> Imo, this should be fixed in the probe helpers. Something like the below
> might make sense:
> 
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index e714b5a7955f..d3b9dc7535da 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  		else
>  			connector->status = connector_status_disconnected;
>  		if (connector->funcs->force)
> -			connector->funcs->force(connector);
> +		connector->funcs->force(connector);
> +	} else if (connector->override_edid){
> +		connector->status = connector_status_connected;
> +		connector->funcs->force(connector);
>  	} else {
>  		connector->status = connector->funcs->detect(connector, true);
>  	}
> 
> 
> It should do what you want it to do, still allow us to override force
> state manually and also fix things up for every, not just i915-hdmi. Also,
> much smaller patch.
> 

The patch you are proposing doesn't seem to be related
to the issue I want to fix, so maybe my explanation is still
unclear. After re-reading my commit log, I came to realize
I'm still not explaining the issue properly.

Let me try again :-)

A user can connect any kind of HDMI monitor to the box, and
the kernel should be able to output some video, even when the
HDMI monitor is a piece of crap and sends completely broken EDID.

Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
to set the connector status. IOW, the connector status is set to "connected"
*only* if the EDID is correct, and is left as "disconnected" if the EDID
is corrupt.

However, the connector status can be detected by just probing
the I2C bus (drm_probe_ddc).

The DRM probe helper relies on the .detect callback to decide if
the modes' fallbacks, EDID provided modes, etc are going to be set.

It only set those modes if the .detect callback handler returns
a "connected" status and does nothing if .detect returns
"disconnected".

If the connector is reported as "disconnected" it will skip it and
the user won't be able to use it (except if the state is forced
with a parameter).

I am currently shipping intel boxes without monitors, and the
user can connect its own monitor. I can't know before hand
what device is going to be plugged (neither on which output
connector, HDMI, DVI, etc)... so I am not able to force any state.

The patch I am proposing makes the kernel work without any
user intervention, in the face of corrupted EDID coming out of
a monitor. This works because the .detect logic after my patch
just checks if a I2C device is present in the bus to mark the
connector as "connected" and does not use the EDID parsing for that.

In fact, the EDID parsing is moved to .get_modes() since they're
not really used before. This at the very least, is consistent with
how other drivers work (I'm not inventing anything).

Maybe the following commit log is better. How does it look now?

----------8<----------
drm: i915: Fix HDMI connector status detection in case of broken EDID

The i915 DRM driver attempts to parse the EDID in the HDMI connector
.detect callback and use the return status of intel_hdmi_set_edid()
to decide if the connector status is connected or disconnected.

The problem is that intel_hdmi_set_edid() fails if the EDID is not
correct (i.e: corrupted) and so .detect will wrongly report to the
DRM core that the connector is disconnected.

It's totally acceptable to use a HDMI connector even in the case of
a broken EDID, by using the CONFIG_DRM_LOAD_EDID_FIRMWARE
and noedid fallback options. The only thing that .detect should
check is that there is a device answering in the correct I2C address
and bus.

So this patch changes the .detect logic to just check the DDC presence
to decide the connector status, so the core can set the EDID fallbacks.

Also, checking for the EDID in the .connect callback doesn't seem to be
correct since that should only check that the connector has been hooked
so this patch also moves the EDID parsing logic to the .get_modes handler
when the modes are actually needed and filled to report to user-space.
On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
> Daniel,
> 
> Thanks a lot for the quick reply!
> 
> On 20 Apr 01:34 PM, Daniel Vetter wrote:
> > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
> > > Currently, our implementation of drm_connector_funcs.detect is
> > > based on getting a valid EDID.
> > > 
> > > This requirement makes the driver fail to detect connected
> > > connectors in case of EDID corruption, which in turn prevents
> > > from falling back to modes provided by builtin or user-provided
> > > EDIDs.
> > 
> > Imo, this should be fixed in the probe helpers. Something like the below
> > might make sense:
> > 
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > index e714b5a7955f..d3b9dc7535da 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> >  		else
> >  			connector->status = connector_status_disconnected;
> >  		if (connector->funcs->force)
> > -			connector->funcs->force(connector);
> > +		connector->funcs->force(connector);
> > +	} else if (connector->override_edid){
> > +		connector->status = connector_status_connected;
> > +		connector->funcs->force(connector);
> >  	} else {
> >  		connector->status = connector->funcs->detect(connector, true);
> >  	}
> > 
> > 
> > It should do what you want it to do, still allow us to override force
> > state manually and also fix things up for every, not just i915-hdmi. Also,
> > much smaller patch.
> > 
> 
> The patch you are proposing doesn't seem to be related
> to the issue I want to fix, so maybe my explanation is still
> unclear. After re-reading my commit log, I came to realize
> I'm still not explaining the issue properly.
> 
> Let me try again :-)
> 
> A user can connect any kind of HDMI monitor to the box, and
> the kernel should be able to output some video, even when the
> HDMI monitor is a piece of crap and sends completely broken EDID.
> 
> Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
> to set the connector status. IOW, the connector status is set to "connected"
> *only* if the EDID is correct, and is left as "disconnected" if the EDID
> is corrupt.
> 
> However, the connector status can be detected by just probing
> the I2C bus (drm_probe_ddc).
> 
> The DRM probe helper relies on the .detect callback to decide if
> the modes' fallbacks, EDID provided modes, etc are going to be set.
> 
> It only set those modes if the .detect callback handler returns
> a "connected" status and does nothing if .detect returns
> "disconnected".
> 
> If the connector is reported as "disconnected" it will skip it and
> the user won't be able to use it (except if the state is forced
> with a parameter).
> 
> I am currently shipping intel boxes without monitors, and the
> user can connect its own monitor. I can't know before hand
> what device is going to be plugged (neither on which output
> connector, HDMI, DVI, etc)... so I am not able to force any state.
> 
> The patch I am proposing makes the kernel work without any
> user intervention, in the face of corrupted EDID coming out of
> a monitor. This works because the .detect logic after my patch
> just checks if a I2C device is present in the bus to mark the
> connector as "connected" and does not use the EDID parsing for that.
> 
> In fact, the EDID parsing is moved to .get_modes() since they're
> not really used before. This at the very least, is consistent with
> how other drivers work (I'm not inventing anything).
> 
> Maybe the following commit log is better. How does it look now?

But in that case the only thing you get is the 1024x756 fallback mode.
You're users are happy with that? I thought your use-case was that you
need to overwrite the edid anyway, and that doing the edid override alone
doesn't work. Hence my patch to make stuff work directly with just the
edid override.

At least I myself wouldn't consider 1024x756 "working" ...
-Daniel

> 
> ----------8<----------
> drm: i915: Fix HDMI connector status detection in case of broken EDID
> 
> The i915 DRM driver attempts to parse the EDID in the HDMI connector
> .detect callback and use the return status of intel_hdmi_set_edid()
> to decide if the connector status is connected or disconnected.
> 
> The problem is that intel_hdmi_set_edid() fails if the EDID is not
> correct (i.e: corrupted) and so .detect will wrongly report to the
> DRM core that the connector is disconnected.
> 
> It's totally acceptable to use a HDMI connector even in the case of
> a broken EDID, by using the CONFIG_DRM_LOAD_EDID_FIRMWARE
> and noedid fallback options. The only thing that .detect should
> check is that there is a device answering in the correct I2C address
> and bus.
> 
> So this patch changes the .detect logic to just check the DDC presence
> to decide the connector status, so the core can set the EDID fallbacks.
> 
> Also, checking for the EDID in the .connect callback doesn't seem to be
> correct since that should only check that the connector has been hooked
> so this patch also moves the EDID parsing logic to the .get_modes handler
> when the modes are actually needed and filled to report to user-space.
> -- 
> Ezequiel Garcia, VanguardiaSur
> www.vanguardiasur.com.ar
On Fri, Apr 22, 2016 at 10:15:20AM +0200, Daniel Vetter wrote:
> On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
> > Daniel,
> > 
> > Thanks a lot for the quick reply!
> > 
> > On 20 Apr 01:34 PM, Daniel Vetter wrote:
> > > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
> > > > Currently, our implementation of drm_connector_funcs.detect is
> > > > based on getting a valid EDID.
> > > > 
> > > > This requirement makes the driver fail to detect connected
> > > > connectors in case of EDID corruption, which in turn prevents
> > > > from falling back to modes provided by builtin or user-provided
> > > > EDIDs.
> > > 
> > > Imo, this should be fixed in the probe helpers. Something like the below
> > > might make sense:
> > > 
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index e714b5a7955f..d3b9dc7535da 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > >  		else
> > >  			connector->status = connector_status_disconnected;
> > >  		if (connector->funcs->force)
> > > -			connector->funcs->force(connector);
> > > +		connector->funcs->force(connector);
> > > +	} else if (connector->override_edid){
> > > +		connector->status = connector_status_connected;
> > > +		connector->funcs->force(connector);
> > >  	} else {
> > >  		connector->status = connector->funcs->detect(connector, true);
> > >  	}
> > > 
> > > 
> > > It should do what you want it to do, still allow us to override force
> > > state manually and also fix things up for every, not just i915-hdmi. Also,
> > > much smaller patch.
> > > 
> > 
> > The patch you are proposing doesn't seem to be related
> > to the issue I want to fix, so maybe my explanation is still
> > unclear. After re-reading my commit log, I came to realize
> > I'm still not explaining the issue properly.
> > 
> > Let me try again :-)
> > 
> > A user can connect any kind of HDMI monitor to the box, and
> > the kernel should be able to output some video, even when the
> > HDMI monitor is a piece of crap and sends completely broken EDID.
> > 
> > Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
> > to set the connector status. IOW, the connector status is set to "connected"
> > *only* if the EDID is correct, and is left as "disconnected" if the EDID
> > is corrupt.
> > 
> > However, the connector status can be detected by just probing
> > the I2C bus (drm_probe_ddc).
> > 
> > The DRM probe helper relies on the .detect callback to decide if
> > the modes' fallbacks, EDID provided modes, etc are going to be set.
> > 
> > It only set those modes if the .detect callback handler returns
> > a "connected" status and does nothing if .detect returns
> > "disconnected".
> > 
> > If the connector is reported as "disconnected" it will skip it and
> > the user won't be able to use it (except if the state is forced
> > with a parameter).
> > 
> > I am currently shipping intel boxes without monitors, and the
> > user can connect its own monitor. I can't know before hand
> > what device is going to be plugged (neither on which output
> > connector, HDMI, DVI, etc)... so I am not able to force any state.
> > 
> > The patch I am proposing makes the kernel work without any
> > user intervention, in the face of corrupted EDID coming out of
> > a monitor. This works because the .detect logic after my patch
> > just checks if a I2C device is present in the bus to mark the
> > connector as "connected" and does not use the EDID parsing for that.
> > 
> > In fact, the EDID parsing is moved to .get_modes() since they're
> > not really used before. This at the very least, is consistent with
> > how other drivers work (I'm not inventing anything).
> > 
> > Maybe the following commit log is better. How does it look now?
> 
> But in that case the only thing you get is the 1024x756 fallback mode.
> You're users are happy with that? I thought your use-case was that you
> need to overwrite the edid anyway, and that doing the edid override alone
> doesn't work. Hence my patch to make stuff work directly with just the
> edid override.
> 
> At least I myself wouldn't consider 1024x756 "working" ...

Not sure it's even guaranteed to work with all displays. I can't
remember if HDMI specifies any fallback mode. DP does, but it's
640x480 which isn't entirely useful these days.
On 22 Apr 10:15 AM, Daniel Vetter wrote:
> On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
> > Daniel,
> > 
> > Thanks a lot for the quick reply!
> > 
> > On 20 Apr 01:34 PM, Daniel Vetter wrote:
> > > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
> > > > Currently, our implementation of drm_connector_funcs.detect is
> > > > based on getting a valid EDID.
> > > > 
> > > > This requirement makes the driver fail to detect connected
> > > > connectors in case of EDID corruption, which in turn prevents
> > > > from falling back to modes provided by builtin or user-provided
> > > > EDIDs.
> > > 
> > > Imo, this should be fixed in the probe helpers. Something like the below
> > > might make sense:
> > > 
> > > 
> > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > index e714b5a7955f..d3b9dc7535da 100644
> > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > >  		else
> > >  			connector->status = connector_status_disconnected;
> > >  		if (connector->funcs->force)
> > > -			connector->funcs->force(connector);
> > > +		connector->funcs->force(connector);
> > > +	} else if (connector->override_edid){
> > > +		connector->status = connector_status_connected;
> > > +		connector->funcs->force(connector);
> > >  	} else {
> > >  		connector->status = connector->funcs->detect(connector, true);
> > >  	}
> > > 
> > > 
> > > It should do what you want it to do, still allow us to override force
> > > state manually and also fix things up for every, not just i915-hdmi. Also,
> > > much smaller patch.
> > > 
> > 
> > The patch you are proposing doesn't seem to be related
> > to the issue I want to fix, so maybe my explanation is still
> > unclear. After re-reading my commit log, I came to realize
> > I'm still not explaining the issue properly.
> > 
> > Let me try again :-)
> > 
> > A user can connect any kind of HDMI monitor to the box, and
> > the kernel should be able to output some video, even when the
> > HDMI monitor is a piece of crap and sends completely broken EDID.
> > 
> > Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
> > to set the connector status. IOW, the connector status is set to "connected"
> > *only* if the EDID is correct, and is left as "disconnected" if the EDID
> > is corrupt.
> > 
> > However, the connector status can be detected by just probing
> > the I2C bus (drm_probe_ddc).
> > 
> > The DRM probe helper relies on the .detect callback to decide if
> > the modes' fallbacks, EDID provided modes, etc are going to be set.
> > 
> > It only set those modes if the .detect callback handler returns
> > a "connected" status and does nothing if .detect returns
> > "disconnected".
> > 
> > If the connector is reported as "disconnected" it will skip it and
> > the user won't be able to use it (except if the state is forced
> > with a parameter).
> > 
> > I am currently shipping intel boxes without monitors, and the
> > user can connect its own monitor. I can't know before hand
> > what device is going to be plugged (neither on which output
> > connector, HDMI, DVI, etc)... so I am not able to force any state.
> > 
> > The patch I am proposing makes the kernel work without any
> > user intervention, in the face of corrupted EDID coming out of
> > a monitor. This works because the .detect logic after my patch
> > just checks if a I2C device is present in the bus to mark the
> > connector as "connected" and does not use the EDID parsing for that.
> > 
> > In fact, the EDID parsing is moved to .get_modes() since they're
> > not really used before. This at the very least, is consistent with
> > how other drivers work (I'm not inventing anything).
> > 
> > Maybe the following commit log is better. How does it look now?
> 
> But in that case the only thing you get is the 1024x756 fallback mode.
> You're users are happy with that?

Well, users are happy when things Just Work :-)

> I thought your use-case was that you
> need to overwrite the edid anyway, and that doing the edid override alone
> doesn't work. Hence my patch to make stuff work directly with just the
> edid override.
> 

I don't think the edid override does what you think it does. If you take
a look at the sources, you'll find it's only a debugfs interface to
inject EDID. I'm sure it's probably useful for debugging and development,
but it won't help at all in this case.

So, your patch doesn't improve the situation I have described in any way.
The patch you proposed only does one thing: if you inject some EDID using
debugfs edid_override file, and *then* you probe the driver, it will force
the state to connected, and use that EDID.

However, I'm trying to fix a completely different issue: the i915 driver
is unable to detect a connected HDMI monitor, unless the monitor sends
valid EDID data. I would like i915 to simply detect the device as
"connected" when the device is connected.

That's why I'm insisting this needs fixing at the driver level: the DRM core
excepts the driver's .detect hook to do the Right Thing and tell if the
connector is connected or disconnected.

Once the connector status is detected, we can add modes with the builtin/user
EDID firmware loading, or ultimately fallback to 1024x768 noedid.

And 1024x768 is just the noedid fallback. I can't know if every device
will work with it: that's why it's the last fallback, in case the other
methods to add modes have failed.

What I'm really expecting is the connector to be detected as "connected"
when it *is* "connected".

> At least I myself wouldn't consider 1024x756 "working" ...
>
> -Daniel
> 
> > 
> > ----------8<----------
> > drm: i915: Fix HDMI connector status detection in case of broken EDID
> > 
> > The i915 DRM driver attempts to parse the EDID in the HDMI connector
> > .detect callback and use the return status of intel_hdmi_set_edid()
> > to decide if the connector status is connected or disconnected.
> > 
> > The problem is that intel_hdmi_set_edid() fails if the EDID is not
> > correct (i.e: corrupted) and so .detect will wrongly report to the
> > DRM core that the connector is disconnected.
> > 
> > It's totally acceptable to use a HDMI connector even in the case of
> > a broken EDID, by using the CONFIG_DRM_LOAD_EDID_FIRMWARE
> > and noedid fallback options. The only thing that .detect should
> > check is that there is a device answering in the correct I2C address
> > and bus.
> > 
> > So this patch changes the .detect logic to just check the DDC presence
> > to decide the connector status, so the core can set the EDID fallbacks.
> > 
> > Also, checking for the EDID in the .connect callback doesn't seem to be
> > correct since that should only check that the connector has been hooked
> > so this patch also moves the EDID parsing logic to the .get_modes handler
> > when the modes are actually needed and filled to report to user-space.
> > -- 
> > Ezequiel Garcia, VanguardiaSur
> > www.vanguardiasur.com.ar
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
On Fri, Apr 22, 2016 at 12:18:07PM -0300, Ezequiel Garcia wrote:
> On 22 Apr 10:15 AM, Daniel Vetter wrote:
> > On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
> > > Daniel,
> > > 
> > > Thanks a lot for the quick reply!
> > > 
> > > On 20 Apr 01:34 PM, Daniel Vetter wrote:
> > > > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
> > > > > Currently, our implementation of drm_connector_funcs.detect is
> > > > > based on getting a valid EDID.
> > > > > 
> > > > > This requirement makes the driver fail to detect connected
> > > > > connectors in case of EDID corruption, which in turn prevents
> > > > > from falling back to modes provided by builtin or user-provided
> > > > > EDIDs.
> > > > 
> > > > Imo, this should be fixed in the probe helpers. Something like the below
> > > > might make sense:
> > > > 
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> > > > index e714b5a7955f..d3b9dc7535da 100644
> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > > > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
> > > >  		else
> > > >  			connector->status = connector_status_disconnected;
> > > >  		if (connector->funcs->force)
> > > > -			connector->funcs->force(connector);
> > > > +		connector->funcs->force(connector);
> > > > +	} else if (connector->override_edid){
> > > > +		connector->status = connector_status_connected;
> > > > +		connector->funcs->force(connector);
> > > >  	} else {
> > > >  		connector->status = connector->funcs->detect(connector, true);
> > > >  	}
> > > > 
> > > > 
> > > > It should do what you want it to do, still allow us to override force
> > > > state manually and also fix things up for every, not just i915-hdmi. Also,
> > > > much smaller patch.
> > > > 
> > > 
> > > The patch you are proposing doesn't seem to be related
> > > to the issue I want to fix, so maybe my explanation is still
> > > unclear. After re-reading my commit log, I came to realize
> > > I'm still not explaining the issue properly.
> > > 
> > > Let me try again :-)
> > > 
> > > A user can connect any kind of HDMI monitor to the box, and
> > > the kernel should be able to output some video, even when the
> > > HDMI monitor is a piece of crap and sends completely broken EDID.
> > > 
> > > Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
> > > to set the connector status. IOW, the connector status is set to "connected"
> > > *only* if the EDID is correct, and is left as "disconnected" if the EDID
> > > is corrupt.
> > > 
> > > However, the connector status can be detected by just probing
> > > the I2C bus (drm_probe_ddc).
> > > 
> > > The DRM probe helper relies on the .detect callback to decide if
> > > the modes' fallbacks, EDID provided modes, etc are going to be set.
> > > 
> > > It only set those modes if the .detect callback handler returns
> > > a "connected" status and does nothing if .detect returns
> > > "disconnected".
> > > 
> > > If the connector is reported as "disconnected" it will skip it and
> > > the user won't be able to use it (except if the state is forced
> > > with a parameter).
> > > 
> > > I am currently shipping intel boxes without monitors, and the
> > > user can connect its own monitor. I can't know before hand
> > > what device is going to be plugged (neither on which output
> > > connector, HDMI, DVI, etc)... so I am not able to force any state.
> > > 
> > > The patch I am proposing makes the kernel work without any
> > > user intervention, in the face of corrupted EDID coming out of
> > > a monitor. This works because the .detect logic after my patch
> > > just checks if a I2C device is present in the bus to mark the
> > > connector as "connected" and does not use the EDID parsing for that.
> > > 
> > > In fact, the EDID parsing is moved to .get_modes() since they're
> > > not really used before. This at the very least, is consistent with
> > > how other drivers work (I'm not inventing anything).
> > > 
> > > Maybe the following commit log is better. How does it look now?
> > 
> > But in that case the only thing you get is the 1024x756 fallback mode.
> > You're users are happy with that?
> 
> Well, users are happy when things Just Work :-)
> 
> > I thought your use-case was that you
> > need to overwrite the edid anyway, and that doing the edid override alone
> > doesn't work. Hence my patch to make stuff work directly with just the
> > edid override.
> > 
> 
> I don't think the edid override does what you think it does. If you take
> a look at the sources, you'll find it's only a debugfs interface to
> inject EDID. I'm sure it's probably useful for debugging and development,
> but it won't help at all in this case.

We can lift that to a sysfs interface easily if there's demand. And this
is the same injection thing as the firmware loader. I'm just not convinced
that the 1024x768 fallback you get without an injected edid is useful for
anyone, and on top of that you can force the connector state easily.

So not yet convinced.
-Daniel

> So, your patch doesn't improve the situation I have described in any way.
> The patch you proposed only does one thing: if you inject some EDID using
> debugfs edid_override file, and *then* you probe the driver, it will force
> the state to connected, and use that EDID.
> 
> However, I'm trying to fix a completely different issue: the i915 driver
> is unable to detect a connected HDMI monitor, unless the monitor sends
> valid EDID data. I would like i915 to simply detect the device as
> "connected" when the device is connected.
> 
> That's why I'm insisting this needs fixing at the driver level: the DRM core
> excepts the driver's .detect hook to do the Right Thing and tell if the
> connector is connected or disconnected.
> 
> Once the connector status is detected, we can add modes with the builtin/user
> EDID firmware loading, or ultimately fallback to 1024x768 noedid.
> 
> And 1024x768 is just the noedid fallback. I can't know if every device
> will work with it: that's why it's the last fallback, in case the other
> methods to add modes have failed.
> 
> What I'm really expecting is the connector to be detected as "connected"
> when it *is* "connected".
> 
> > At least I myself wouldn't consider 1024x756 "working" ...
> >
> > -Daniel
> > 
> > > 
> > > ----------8<----------
> > > drm: i915: Fix HDMI connector status detection in case of broken EDID
> > > 
> > > The i915 DRM driver attempts to parse the EDID in the HDMI connector
> > > .detect callback and use the return status of intel_hdmi_set_edid()
> > > to decide if the connector status is connected or disconnected.
> > > 
> > > The problem is that intel_hdmi_set_edid() fails if the EDID is not
> > > correct (i.e: corrupted) and so .detect will wrongly report to the
> > > DRM core that the connector is disconnected.
> > > 
> > > It's totally acceptable to use a HDMI connector even in the case of
> > > a broken EDID, by using the CONFIG_DRM_LOAD_EDID_FIRMWARE
> > > and noedid fallback options. The only thing that .detect should
> > > check is that there is a device answering in the correct I2C address
> > > and bus.
> > > 
> > > So this patch changes the .detect logic to just check the DDC presence
> > > to decide the connector status, so the core can set the EDID fallbacks.
> > > 
> > > Also, checking for the EDID in the .connect callback doesn't seem to be
> > > correct since that should only check that the connector has been hooked
> > > so this patch also moves the EDID parsing logic to the .get_modes handler
> > > when the modes are actually needed and filled to report to user-space.
> > > -- 
> > > Ezequiel Garcia, VanguardiaSur
> > > www.vanguardiasur.com.ar
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ezequiel Garcia, VanguardiaSur
> www.vanguardiasur.com.ar
On 22 April 2016 at 14:02, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Apr 22, 2016 at 12:18:07PM -0300, Ezequiel Garcia wrote:
>> On 22 Apr 10:15 AM, Daniel Vetter wrote:
>> > On Thu, Apr 21, 2016 at 03:13:51PM -0300, Ezequiel Garcia wrote:
>> > > Daniel,
>> > >
>> > > Thanks a lot for the quick reply!
>> > >
>> > > On 20 Apr 01:34 PM, Daniel Vetter wrote:
>> > > > On Tue, Apr 19, 2016 at 02:31:13PM -0300, Ezequiel Garcia wrote:
>> > > > > Currently, our implementation of drm_connector_funcs.detect is
>> > > > > based on getting a valid EDID.
>> > > > >
>> > > > > This requirement makes the driver fail to detect connected
>> > > > > connectors in case of EDID corruption, which in turn prevents
>> > > > > from falling back to modes provided by builtin or user-provided
>> > > > > EDIDs.
>> > > >
>> > > > Imo, this should be fixed in the probe helpers. Something like the below
>> > > > might make sense:
>> > > >
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> > > > index e714b5a7955f..d3b9dc7535da 100644
>> > > > --- a/drivers/gpu/drm/drm_probe_helper.c
>> > > > +++ b/drivers/gpu/drm/drm_probe_helper.c
>> > > > @@ -214,7 +214,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>> > > >                 else
>> > > >                         connector->status = connector_status_disconnected;
>> > > >                 if (connector->funcs->force)
>> > > > -                       connector->funcs->force(connector);
>> > > > +               connector->funcs->force(connector);
>> > > > +       } else if (connector->override_edid){
>> > > > +               connector->status = connector_status_connected;
>> > > > +               connector->funcs->force(connector);
>> > > >         } else {
>> > > >                 connector->status = connector->funcs->detect(connector, true);
>> > > >         }
>> > > >
>> > > >
>> > > > It should do what you want it to do, still allow us to override force
>> > > > state manually and also fix things up for every, not just i915-hdmi. Also,
>> > > > much smaller patch.
>> > > >
>> > >
>> > > The patch you are proposing doesn't seem to be related
>> > > to the issue I want to fix, so maybe my explanation is still
>> > > unclear. After re-reading my commit log, I came to realize
>> > > I'm still not explaining the issue properly.
>> > >
>> > > Let me try again :-)
>> > >
>> > > A user can connect any kind of HDMI monitor to the box, and
>> > > the kernel should be able to output some video, even when the
>> > > HDMI monitor is a piece of crap and sends completely broken EDID.
>> > >
>> > > Currently, the i915 driver uses the return value of intel_hdmi_set_edid()
>> > > to set the connector status. IOW, the connector status is set to "connected"
>> > > *only* if the EDID is correct, and is left as "disconnected" if the EDID
>> > > is corrupt.
>> > >
>> > > However, the connector status can be detected by just probing
>> > > the I2C bus (drm_probe_ddc).
>> > >
>> > > The DRM probe helper relies on the .detect callback to decide if
>> > > the modes' fallbacks, EDID provided modes, etc are going to be set.
>> > >
>> > > It only set those modes if the .detect callback handler returns
>> > > a "connected" status and does nothing if .detect returns
>> > > "disconnected".
>> > >
>> > > If the connector is reported as "disconnected" it will skip it and
>> > > the user won't be able to use it (except if the state is forced
>> > > with a parameter).
>> > >
>> > > I am currently shipping intel boxes without monitors, and the
>> > > user can connect its own monitor. I can't know before hand
>> > > what device is going to be plugged (neither on which output
>> > > connector, HDMI, DVI, etc)... so I am not able to force any state.
>> > >
>> > > The patch I am proposing makes the kernel work without any
>> > > user intervention, in the face of corrupted EDID coming out of
>> > > a monitor. This works because the .detect logic after my patch
>> > > just checks if a I2C device is present in the bus to mark the
>> > > connector as "connected" and does not use the EDID parsing for that.
>> > >
>> > > In fact, the EDID parsing is moved to .get_modes() since they're
>> > > not really used before. This at the very least, is consistent with
>> > > how other drivers work (I'm not inventing anything).
>> > >
>> > > Maybe the following commit log is better. How does it look now?
>> >
>> > But in that case the only thing you get is the 1024x756 fallback mode.
>> > You're users are happy with that?
>>
>> Well, users are happy when things Just Work :-)
>>
>> > I thought your use-case was that you
>> > need to overwrite the edid anyway, and that doing the edid override alone
>> > doesn't work. Hence my patch to make stuff work directly with just the
>> > edid override.
>> >
>>
>> I don't think the edid override does what you think it does. If you take
>> a look at the sources, you'll find it's only a debugfs interface to
>> inject EDID. I'm sure it's probably useful for debugging and development,
>> but it won't help at all in this case.
>
> We can lift that to a sysfs interface easily if there's demand. And this
> is the same injection thing as the firmware loader.

No, it's not. The firmware loader has some very useful builtin modes.

And there's no demand for this: the sysfs interface is already there,
assuming drivers detect the connector state properly. No need to force
anything.

> I'm just not convinced
> that the 1024x768 fallback you get without an injected edid is useful for
> anyone,

Well, it was useful for one of my users in the field. So, this *is* useful.

> and on top of that you can force the connector state easily.
>

No, I can't force the connector state easily. I have to add a kernel
parameter, and that requires a kernel reboot or a driver re-probe,
so it's not something easy.

But moreover, let's suppose my box has four connectors: two HDMIs,
and two DPs. Which one is the user using? I have no idea about which
connector is being connected, if the i915 driver refuses to detect the device.

(Well, I guess I can probe the I2C bus... which is what the driver should do :-)

> So not yet convinced.

I'm having a very hard time understanding why you want to have
a driver that doesn't work.

If you can't see why EDID builtins and fallback noedid modes are useful
for me, I wonder if I can at least convince you of having the .detect hook
doing what the rest of the DRM drivers do: limit itself to connector
state detection.