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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch