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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC