| Message ID | 1459457703-687-1-git-send-email-ezequiel@vanguardiasur.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling > back to modes provided by builtin or user-provided EDIDs. So why are you getting corrupted EDIDs? > > Let's fix this by improving the detection, with a DDC probe, > if the current EDID-based detection failed. > > Note that a better way of dealing with this could calling > drm_probe_ddc in drm_connector_funcs.detect, and do the > EDID full reading and parsing in drm_connector_helper_funcs.get_modes, > when it's actually needed. > > However, this would be more invasive and thus more error-prone. > The current commit is an attempt to get some uninvasive fix, > and allow for easier backporting. > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index a0d8daed2470..c079206e6681 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1428,6 +1428,20 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > } else > status = connector_status_disconnected; > > + /* > + * The above call to intel_hdmi_set_edid() checked for a valid EDID. > + * However, the EDID can get corrupted for several reasons, resulting > + * in a disconnected status despite the connector being connected. > + * Hence, let's try one more time, by only probing the DDC. > + * > + * This allows the DRM core to fallback to builtin or user-provided > + * EDID firmware, e.g. in drm_helper_probe_single_connector_modes. > + */ > + if (status == connector_status_disconnected) > + if (drm_probe_ddc(intel_gmbus_get_adapter(dev_priv, > + intel_hdmi->ddc_bus))) > + status = connector_status_connected; > + > intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > return status; > -- > 2.7.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
El abr. 1, 2016 11:47 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> escribió: > > On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling > > back to modes provided by builtin or user-provided EDIDs. > > So why are you getting corrupted EDIDs? > Does it matter? > > > > Let's fix this by improving the detection, with a DDC probe, > > if the current EDID-based detection failed. > > > > Note that a better way of dealing with this could calling > > drm_probe_ddc in drm_connector_funcs.detect, and do the > > EDID full reading and parsing in drm_connector_helper_funcs.get_modes, > > when it's actually needed. > > > > However, this would be more invasive and thus more error-prone. > > The current commit is an attempt to get some uninvasive fix, > > and allow for easier backporting. > > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index a0d8daed2470..c079206e6681 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1428,6 +1428,20 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > > } else > > status = connector_status_disconnected; > > > > + /* > > + * The above call to intel_hdmi_set_edid() checked for a valid EDID. > > + * However, the EDID can get corrupted for several reasons, resulting > > + * in a disconnected status despite the connector being connected. > > + * Hence, let's try one more time, by only probing the DDC. > > + * > > + * This allows the DRM core to fallback to builtin or user-provided > > + * EDID firmware, e.g. in drm_helper_probe_single_connector_modes. > > + */ > > + if (status == connector_status_disconnected) > > + if (drm_probe_ddc(intel_gmbus_get_adapter(dev_priv, > > + intel_hdmi->ddc_bus))) > > + status = connector_status_connected; > > + > > intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > > > return status; > > -- > > 2.7.0 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC
On Fri, Apr 01, 2016 at 12:38:11PM -0300, Ezequiel Garcia wrote: > El abr. 1, 2016 11:47 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> > escribió: > > > > On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling > > > back to modes provided by builtin or user-provided EDIDs. > > > > So why are you getting corrupted EDIDs? > > > > Does it matter? Yes. We should fix the real cause (if possible) instead of adding more duct tape. > > > > > > > Let's fix this by improving the detection, with a DDC probe, > > > if the current EDID-based detection failed. > > > > > > Note that a better way of dealing with this could calling > > > drm_probe_ddc in drm_connector_funcs.detect, and do the > > > EDID full reading and parsing in drm_connector_helper_funcs.get_modes, > > > when it's actually needed. > > > > > > However, this would be more invasive and thus more error-prone. > > > The current commit is an attempt to get some uninvasive fix, > > > and allow for easier backporting. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> > > > --- > > > drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++++++ > > > 1 file changed, 14 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > > > index a0d8daed2470..c079206e6681 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -1428,6 +1428,20 @@ intel_hdmi_detect(struct drm_connector > *connector, bool force) > > > } else > > > status = connector_status_disconnected; > > > > > > + /* > > > + * The above call to intel_hdmi_set_edid() checked for a valid > EDID. > > > + * However, the EDID can get corrupted for several reasons, > resulting > > > + * in a disconnected status despite the connector being connected. > > > + * Hence, let's try one more time, by only probing the DDC. > > > + * > > > + * This allows the DRM core to fallback to builtin or > user-provided > > > + * EDID firmware, e.g. in drm_helper_probe_single_connector_modes. > > > + */ > > > + if (status == connector_status_disconnected) > > > + if (drm_probe_ddc(intel_gmbus_get_adapter(dev_priv, > > > + intel_hdmi->ddc_bus))) > > > + status = connector_status_connected; > > > + > > > intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); > > > > > > return status; > > > -- > > > 2.7.0 > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Ville Syrjälä > > Intel OTC
On 01 Apr 06:46 PM, Ville Syrjälä wrote: > On Fri, Apr 01, 2016 at 12:38:11PM -0300, Ezequiel Garcia wrote: > > El abr. 1, 2016 11:47 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> > > escribió: > > > > > > On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling > > > > back to modes provided by builtin or user-provided EDIDs. > > > > > > So why are you getting corrupted EDIDs? > > > > > > > Does it matter? > > Yes. We should fix the real cause (if possible) instead of adding > more duct tape. > So, there are two things involved in this patch: 1. There are several reasons why EDID can get screwed, this is documented at length [1], and it's the motivation for CONFIG_DRM_LOAD_EDID_FIRMWARE to exist. You can find lots of reports on the internet of people getting corrupt EDID from their monitors. For instance, here's one [2]. And even if no firmware is provided using CONFIG_DRM_LOAD_EDID_FIRMWARE, the DRM core will provide a 1024x768 fallback mode: int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY) { [..] if (count == 0 && connector->status == connector_status_connected) count = drm_add_modes_noedid(connector, 1024, 768); But, this only works if the connector is detected. Since I'm interested in backporting this patch to apply it on the kernels I maintain (which are currently deployed on hundreds of machines), I tried to find a simple solution. Hence, this patch. There's no issue to fix here, because broken hardware is a fact of life, and not something we can fix or ignore [3]. 2. On the other side, the i915 implementation looks suspicious. IMHO, drm_connector_funcs.detect should not try to read a valid EDID, and just try to detect if the connector is connected or disconnected. The EDID can be read in drm_connector_helper_funcs.get_modes, as other drm/connector drivers are doing (tda998x, tfp410, tegra). However, I think it's safer to get a simple fix now, and do this as follow-up patches. How does it sound? [1] Documentation/EDID/HOWTO.txt [2] http://www.blaicher.com/2012/06/howto-fixing-a-broken-edid-eeprom-with-a-bus-pirate-v4/ [3] https://marc.info/?l=linux-kernel&m=112838038415265&w=4
(Adding Jani again, who got dropped for some reason) On 1 April 2016 at 16:50, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > On 01 Apr 06:46 PM, Ville Syrjälä wrote: >> On Fri, Apr 01, 2016 at 12:38:11PM -0300, Ezequiel Garcia wrote: >> > El abr. 1, 2016 11:47 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> >> > escribió: >> > > >> > > On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling >> > > > back to modes provided by builtin or user-provided EDIDs. >> > > >> > > So why are you getting corrupted EDIDs? >> > > >> > >> > Does it matter? >> >> Yes. We should fix the real cause (if possible) instead of adding >> more duct tape. >> > > So, there are two things involved in this patch: > > 1. > There are several reasons why EDID can get screwed, this is > documented at length [1], and it's the motivation for > CONFIG_DRM_LOAD_EDID_FIRMWARE to exist. > > You can find lots of reports on the internet of people getting > corrupt EDID from their monitors. For instance, here's one [2]. > > And even if no firmware is provided using CONFIG_DRM_LOAD_EDID_FIRMWARE, > the DRM core will provide a 1024x768 fallback mode: > > int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > uint32_t maxX, uint32_t maxY) > { > [..] > if (count == 0 && connector->status == connector_status_connected) > count = drm_add_modes_noedid(connector, 1024, 768); > > But, this only works if the connector is detected. > > Since I'm interested in backporting this patch to apply it on the kernels > I maintain (which are currently deployed on hundreds of machines), I tried > to find a simple solution. Hence, this patch. > > There's no issue to fix here, because broken hardware is a fact of life, > and not something we can fix or ignore [3]. > > 2. > On the other side, the i915 implementation looks suspicious. IMHO, > drm_connector_funcs.detect should not try to read a valid EDID, > and just try to detect if the connector is connected or disconnected. > > The EDID can be read in drm_connector_helper_funcs.get_modes, as other > drm/connector drivers are doing (tda998x, tfp410, tegra). > > However, I think it's safer to get a simple fix now, and do this > as follow-up patches. > > How does it sound? > > [1] Documentation/EDID/HOWTO.txt > [2] http://www.blaicher.com/2012/06/howto-fixing-a-broken-edid-eeprom-with-a-bus-pirate-v4/ > [3] https://marc.info/?l=linux-kernel&m=112838038415265&w=4 > -- > Ezequiel Garcia, VanguardiaSur > www.vanguardiasur.com.ar
On 5 April 2016 at 11:54, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: > (Adding Jani again, who got dropped for some reason) > > On 1 April 2016 at 16:50, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote: >> On 01 Apr 06:46 PM, Ville Syrjälä wrote: >>> On Fri, Apr 01, 2016 at 12:38:11PM -0300, Ezequiel Garcia wrote: >>> > El abr. 1, 2016 11:47 AM, "Ville Syrjälä" <ville.syrjala@linux.intel.com> >>> > escribió: >>> > > >>> > > On Thu, Mar 31, 2016 at 05:55:03PM -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 prevents from falling >>> > > > back to modes provided by builtin or user-provided EDIDs. >>> > > >>> > > So why are you getting corrupted EDIDs? >>> > > >>> > >>> > Does it matter? >>> >>> Yes. We should fix the real cause (if possible) instead of adding >>> more duct tape. >>> >> >> So, there are two things involved in this patch: >> >> 1. >> There are several reasons why EDID can get screwed, this is >> documented at length [1], and it's the motivation for >> CONFIG_DRM_LOAD_EDID_FIRMWARE to exist. >> >> You can find lots of reports on the internet of people getting >> corrupt EDID from their monitors. For instance, here's one [2]. >> >> And even if no firmware is provided using CONFIG_DRM_LOAD_EDID_FIRMWARE, >> the DRM core will provide a 1024x768 fallback mode: >> >> int drm_helper_probe_single_connector_modes(struct drm_connector *connector, >> uint32_t maxX, uint32_t maxY) >> { >> [..] >> if (count == 0 && connector->status == connector_status_connected) >> count = drm_add_modes_noedid(connector, 1024, 768); >> >> But, this only works if the connector is detected. >> >> Since I'm interested in backporting this patch to apply it on the kernels >> I maintain (which are currently deployed on hundreds of machines), I tried >> to find a simple solution. Hence, this patch. >> >> There's no issue to fix here, because broken hardware is a fact of life, >> and not something we can fix or ignore [3]. >> >> 2. >> On the other side, the i915 implementation looks suspicious. IMHO, >> drm_connector_funcs.detect should not try to read a valid EDID, >> and just try to detect if the connector is connected or disconnected. >> >> The EDID can be read in drm_connector_helper_funcs.get_modes, as other >> drm/connector drivers are doing (tda998x, tfp410, tegra). >> >> However, I think it's safer to get a simple fix now, and do this >> as follow-up patches. >> >> How does it sound? >> Are there any other comments regarding this patch? If at all possible, I'd like to see this merged, or otherwise a proposal for an alternative solution. >> [1] Documentation/EDID/HOWTO.txt >> [2] http://www.blaicher.com/2012/06/howto-fixing-a-broken-edid-eeprom-with-a-bus-pirate-v4/ >> [3] https://marc.info/?l=linux-kernel&m=112838038415265&w=4 Thanks,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index a0d8daed2470..c079206e6681 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1428,6 +1428,20 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) } else status = connector_status_disconnected; + /* + * The above call to intel_hdmi_set_edid() checked for a valid EDID. + * However, the EDID can get corrupted for several reasons, resulting + * in a disconnected status despite the connector being connected. + * Hence, let's try one more time, by only probing the DDC. + * + * This allows the DRM core to fallback to builtin or user-provided + * EDID firmware, e.g. in drm_helper_probe_single_connector_modes. + */ + if (status == connector_status_disconnected) + if (drm_probe_ddc(intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus))) + status = connector_status_connected; + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); return status;
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 prevents from falling back to modes provided by builtin or user-provided EDIDs. Let's fix this by improving the detection, with a DDC probe, if the current EDID-based detection failed. Note that a better way of dealing with this could calling drm_probe_ddc in drm_connector_funcs.detect, and do the EDID full reading and parsing in drm_connector_helper_funcs.get_modes, when it's actually needed. However, this would be more invasive and thus more error-prone. The current commit is an attempt to get some uninvasive fix, and allow for easier backporting. Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> --- drivers/gpu/drm/i915/intel_hdmi.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)