| Message ID | 20160420113438.GQ2510@phenom.ffwll.local (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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); }