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.