[Intel-gfx] [PATCH] drm: i915: Improve behavior in case of broken HDMI EDID

[Ezequiel Garcia]

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.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar