[PATCH 1/2] drm/edid: abstract override/firmware EDID retrieval

[PATCH 1/2] drm/edid: abstract override/firmware EDID retrieval

[Jani Nikula]

Abstract the debugfs override and the firmware EDID retrieval
function. We'll be needing it in the follow-up. No functional changes.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Harish Chegondi <harish.chegondi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c3296a72fff9..c59a1e8c5ada 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1573,6 +1573,20 @@ static void connector_bad_edid(struct drm_connector *connector,
 	}
 }
 
+/* Get override or firmware EDID */
+static struct edid *drm_get_override_edid(struct drm_connector *connector)
+{
+	struct edid *override = NULL;
+
+	if (connector->override_edid)
+		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
+
+	if (!override)
+		override = drm_load_edid_firmware(connector);
+
+	return IS_ERR(override) ? NULL : override;
+}
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
@@ -1600,15 +1614,10 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
 {
 	int i, j = 0, valid_extensions = 0;
 	u8 *edid, *new;
-	struct edid *override = NULL;
-
-	if (connector->override_edid)
-		override = drm_edid_duplicate(connector->edid_blob_ptr->data);
-
-	if (!override)
-		override = drm_load_edid_firmware(connector);
+	struct edid *override;
 
-	if (!IS_ERR_OR_NULL(override))
+	override = drm_get_override_edid(connector);
+	if (override)
 		return override;
 
 	if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL)
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Jani Nikula]

We've moved the override and firmware EDID (simply "override EDID" from
now on) handling to the low level drm_do_get_edid() function in order to
transparently use the override throughout the stack. The idea is that
you get the override EDID via the ->get_modes() hook.

Unfortunately, there are scenarios where the DDC probe in drm_get_edid()
called via ->get_modes() fails, although the preceding ->detect()
succeeds.

In the case reported by Paul Wise, the ->detect() hook,
intel_crt_detect(), relies on hotplug detect, bypassing the DDC. In the
case reported by Ilpo Järvinen, there is no ->detect() hook, which is
interpreted as connected. The subsequent DDC probe reached via
->get_modes() fails, and we don't even look at the override EDID,
resulting in no modes being added.

Because drm_get_edid() is used via ->detect() all over the place, we
can't trivially remove the DDC probe, as it leads to override EDID
effectively meaning connector forcing. The goal is that connector
forcing and override EDID remain orthogonal.

Generally, the underlying problem here is the conflation of ->detect()
and ->get_modes() via drm_get_edid(). The former should just detect, and
the latter should just get the modes, typically via reading the EDID. As
long as drm_get_edid() is used in ->detect(), it needs to retain the DDC
probe. Or such users need to have a separate DDC probe step first.

Work around the regression by falling back to a separate attempt at
getting the override EDID at drm_helper_probe_single_connector_modes()
level. With a working DDC and override EDID, it'll never be called; the
override EDID will come via ->get_modes(). There will still be a failing
DDC probe attempt in the cases that require the fallback.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107583
Reported-by: Paul Wise <pabs3@bonedaddy.net>
Cc: Paul Wise <pabs3@bonedaddy.net>
References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
Reported-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
Cc: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")
Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at drm_do_get_edid() level")
Cc: <stable@vger.kernel.org> # v4.15+
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harish Chegondi <harish.chegondi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c         | 29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c |  7 +++++++
 include/drm/drm_edid.h             |  1 +
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c59a1e8c5ada..780146bfc225 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1587,6 +1587,35 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector)
 	return IS_ERR(override) ? NULL : override;
 }
 
+/**
+ * drm_add_override_edid_modes - add modes from override/firmware EDID
+ * @connector: connector we're probing
+ *
+ * Add modes from the override/firmware EDID, if available. Only to be used from
+ * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe
+ * failed during drm_get_edid() and caused the override/firmware EDID to be
+ * skipped.
+ *
+ * Return: The number of modes added or 0 if we couldn't find any.
+ */
+int drm_add_override_edid_modes(struct drm_connector *connector)
+{
+	struct edid *override;
+	int num_modes = 0;
+
+	override = drm_get_override_edid(connector);
+	if (override) {
+		num_modes = drm_add_edid_modes(connector, override);
+		kfree(override);
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
+			      connector->base.id, connector->name, num_modes);
+	}
+
+	return num_modes;
+}
+EXPORT_SYMBOL(drm_add_override_edid_modes);
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 01e243f1ea94..ef2c468205a2 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -480,6 +480,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	count = (*connector_funcs->get_modes)(connector);
 
+	/*
+	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
+	 * override/firmware EDID.
+	 */
+	if (count == 0 && connector->status == connector_status_connected)
+		count = drm_add_override_edid_modes(connector);
+
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
 	count += drm_helper_probe_add_cmdline_mode(connector);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 88b63801f9db..b9719418c3d2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -478,6 +478,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
+int drm_add_override_edid_modes(struct drm_connector *connector);
 
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
-- 
2.20.1

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval
URL   : https://patchwork.freedesktop.org/series/61764/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b8887b8817bc drm/edid: abstract override/firmware EDID retrieval
b02d6f0631c2 drm: add fallback override/firmware EDID modes workaround
-:45: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#45: 
References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi

-:48: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")'
#48: 
References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")

total: 1 errors, 1 warnings, 0 checks, 55 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval
URL   : https://patchwork.freedesktop.org/series/61764/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6216 -> Patchwork_13203
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/

Known issues
------------

  Here are the changes found in Patchwork_13203 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-icl-u3/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-dsi:         [PASS][3] -> [FAIL][4] ([fdo#103167])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-dsi/igt@kms_frontbuffer_tracking@basic.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-icl-dsi/igt@kms_frontbuffer_tracking@basic.html

  
#### Possible fixes ####

  * igt@gem_close_race@basic-process:
    - fi-icl-u3:          [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u3/igt@gem_close_race@basic-process.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-icl-u3/igt@gem_close_race@basic-process.html

  * igt@gem_ctx_create@basic-files:
    - fi-icl-u2:          [INCOMPLETE][7] ([fdo#107713] / [fdo#109100]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-u2/igt@gem_ctx_create@basic-files.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-icl-u2/igt@gem_ctx_create@basic-files.html
    - fi-icl-y:           [INCOMPLETE][9] ([fdo#107713] / [fdo#109100]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-icl-y/igt@gem_ctx_create@basic-files.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-icl-y/igt@gem_ctx_create@basic-files.html

  * igt@gem_exec_basic@basic-all:
    - fi-cml-u2:          [INCOMPLETE][11] ([fdo#110566]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-cml-u2/igt@gem_exec_basic@basic-all.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-cml-u2/igt@gem_exec_basic@basic-all.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [DMESG-WARN][13] ([fdo#102614]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#110566]: https://bugs.freedesktop.org/show_bug.cgi?id=110566


Participating hosts (54 -> 47)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7560u fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6216 -> Patchwork_13203

  CI_DRM_6216: e58a2b1f565ec5d77c69724d2f43a7de6f7953b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5047: 3f4b1a49ed5e1c77ea42970d4d3156c997f66050 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13203: b02d6f0631c274c4a6d66fee16390f6078030080 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b02d6f0631c2 drm: add fallback override/firmware EDID modes workaround
b8887b8817bc drm/edid: abstract override/firmware EDID retrieval

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Daniel Vetter]

On Fri, Jun 07, 2019 at 02:05:13PM +0300, Jani Nikula wrote:
> We've moved the override and firmware EDID (simply "override EDID" from
> now on) handling to the low level drm_do_get_edid() function in order to
> transparently use the override throughout the stack. The idea is that
> you get the override EDID via the ->get_modes() hook.
> 
> Unfortunately, there are scenarios where the DDC probe in drm_get_edid()
> called via ->get_modes() fails, although the preceding ->detect()
> succeeds.
> 
> In the case reported by Paul Wise, the ->detect() hook,
> intel_crt_detect(), relies on hotplug detect, bypassing the DDC. In the
> case reported by Ilpo Järvinen, there is no ->detect() hook, which is
> interpreted as connected. The subsequent DDC probe reached via
> ->get_modes() fails, and we don't even look at the override EDID,
> resulting in no modes being added.
> 
> Because drm_get_edid() is used via ->detect() all over the place, we
> can't trivially remove the DDC probe, as it leads to override EDID
> effectively meaning connector forcing. The goal is that connector
> forcing and override EDID remain orthogonal.
> 
> Generally, the underlying problem here is the conflation of ->detect()
> and ->get_modes() via drm_get_edid(). The former should just detect, and
> the latter should just get the modes, typically via reading the EDID. As
> long as drm_get_edid() is used in ->detect(), it needs to retain the DDC
> probe. Or such users need to have a separate DDC probe step first.
> 
> Work around the regression by falling back to a separate attempt at
> getting the override EDID at drm_helper_probe_single_connector_modes()
> level. With a working DDC and override EDID, it'll never be called; the
> override EDID will come via ->get_modes(). There will still be a failing
> DDC probe attempt in the cases that require the fallback.

I think we should also highlight here that EDID caching between ->detect
and ->get_modes is a further complicating concern, which is why making
drm_do_get_edid magically dtrt between ->detect and ->get_modes doesn't
work either.

Aside from that nit I think this covers our lengthy discussion completely.

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107583
> Reported-by: Paul Wise <pabs3@bonedaddy.net>
> Cc: Paul Wise <pabs3@bonedaddy.net>
> References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
> Reported-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> Cc: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")
> Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at drm_do_get_edid() level")
> Cc: <stable@vger.kernel.org> # v4.15+
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harish Chegondi <harish.chegondi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

As discussed on irc, we need tested-by here from the reporters since
there's way too many losing and frustrangingly few winning moves here.

With all that, on the series:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks a lot for slogging through all this and pondering all the options
and implications!

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_edid.c         | 29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c |  7 +++++++
>  include/drm/drm_edid.h             |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c59a1e8c5ada..780146bfc225 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1587,6 +1587,35 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector)
>  	return IS_ERR(override) ? NULL : override;
>  }
>  
> +/**
> + * drm_add_override_edid_modes - add modes from override/firmware EDID
> + * @connector: connector we're probing
> + *
> + * Add modes from the override/firmware EDID, if available. Only to be used from
> + * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe
> + * failed during drm_get_edid() and caused the override/firmware EDID to be
> + * skipped.
> + *
> + * Return: The number of modes added or 0 if we couldn't find any.
> + */
> +int drm_add_override_edid_modes(struct drm_connector *connector)
> +{
> +	struct edid *override;
> +	int num_modes = 0;
> +
> +	override = drm_get_override_edid(connector);
> +	if (override) {
> +		num_modes = drm_add_edid_modes(connector, override);
> +		kfree(override);
> +
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> +			      connector->base.id, connector->name, num_modes);
> +	}
> +
> +	return num_modes;
> +}
> +EXPORT_SYMBOL(drm_add_override_edid_modes);
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 01e243f1ea94..ef2c468205a2 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -480,6 +480,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  
>  	count = (*connector_funcs->get_modes)(connector);
>  
> +	/*
> +	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
> +	 * override/firmware EDID.
> +	 */
> +	if (count == 0 && connector->status == connector_status_connected)
> +		count = drm_add_override_edid_modes(connector);
> +
>  	if (count == 0 && connector->status == connector_status_connected)
>  		count = drm_add_modes_noedid(connector, 1024, 768);
>  	count += drm_helper_probe_add_cmdline_mode(connector);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 88b63801f9db..b9719418c3d2 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -478,6 +478,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);
>  struct edid *drm_edid_duplicate(const struct edid *edid);
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> +int drm_add_override_edid_modes(struct drm_connector *connector);
>  
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
> -- 
> 2.20.1
> 

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

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On Fri, 2019-06-07 at 17:10 +0200, Daniel Vetter wrote:

> As discussed on irc, we need tested-by here from the reporters since
> there's way too many losing and frustrangingly few winning moves here.

I'm building it now, hopefully will be done today.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 635 bytes --]

On Fri, 2019-06-07 at 17:10 +0200, Daniel Vetter wrote:

> As discussed on irc, we need tested-by here from the reporters since
> there's way too many losing and frustrangingly few winning moves here.

Tested-by: Paul Wise <pabs3@bonedaddy.net>

I've tested these two patches on top of Linux v5.2-rc3 and the EDID
override works correctly on an Intel Ironlake GPU with a monitor that
lost its EDID a while ago.

I'll test that it also works with an nVidia GPU & noveau drivers later
today once that system is available.

https://patchwork.freedesktop.org/series/61764/

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On Sat, 2019-06-08 at 13:10 +0800, Paul Wise wrote:

> I've tested these two patches on top of Linux v5.2-rc3 and the EDID
> override works correctly on an Intel Ironlake GPU with a monitor that
> lost its EDID a while ago.

While testing I noticed a couple of things:

While everything the GUI is the correct resolution, GNOME is unable to
identify the monitor vendor or model. This is a regression from the
previous edid override functionality. It looks like this is because the
edid file in /sys is not populated with the EDID override data.

I got a crash due to null pointer dereference at one point, I'll try to
track down when this happens.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Sat, 2019-06-08 at 13:10 +0800, Paul Wise wrote:

> I'll test that it also works with an nVidia GPU & noveau drivers
> later today once that system is available.

Same results as with the Intel GPU:

Correct screen resolution but missing EDID override data.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

✗ Fi.CI.IGT: failure for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval
URL   : https://patchwork.freedesktop.org/series/61764/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6216_full -> Patchwork_13203_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13203_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13203_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13203_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_ctx_engines@execute-one:
    - shard-glk:          NOTRUN -> [DMESG-WARN][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-glk6/igt@gem_ctx_engines@execute-one.html

  
Known issues
------------

  Here are the changes found in Patchwork_13203_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-apl:          [PASS][2] -> [DMESG-WARN][3] ([fdo#108686])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl8/igt@gem_tiled_swapping@non-threaded.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-apl8/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [PASS][4] -> [DMESG-WARN][5] ([fdo#108566]) +3 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl4/igt@i915_suspend@debugfs-reader.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-apl8/igt@i915_suspend@debugfs-reader.html

  * igt@kms_flip@dpms-vs-vblank-race-interruptible:
    - shard-glk:          [PASS][6] -> [FAIL][7] ([fdo#103060])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-glk3/igt@kms_flip@dpms-vs-vblank-race-interruptible.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-glk9/igt@kms_flip@dpms-vs-vblank-race-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-kbl:          [PASS][8] -> [INCOMPLETE][9] ([fdo#103665])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-kbl3/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw:
    - shard-iclb:         [PASS][10] -> [FAIL][11] ([fdo#103167]) +4 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb5/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb6/igt@kms_frontbuffer_tracking@fbcpsr-1p-pri-indfb-multidraw.html

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc:
    - shard-iclb:         [PASS][12] -> [INCOMPLETE][13] ([fdo#106978] / [fdo#107713])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb3/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb7/igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [PASS][14] -> [FAIL][15] ([fdo#108145])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-skl2/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][16] -> [FAIL][17] ([fdo#108145] / [fdo#110403])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-skl5/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@perf@blocking:
    - shard-skl:          [PASS][18] -> [FAIL][19] ([fdo#110728])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl8/igt@perf@blocking.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-skl7/igt@perf@blocking.html

  
#### Possible fixes ####

  * igt@gem_ctx_engines@execute-one:
    - shard-skl:          [DMESG-WARN][20] -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@gem_ctx_engines@execute-one.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-skl10/igt@gem_ctx_engines@execute-one.html

  * igt@gem_ctx_isolation@rcs0-s3:
    - shard-iclb:         [INCOMPLETE][22] ([fdo#107713] / [fdo#109100]) -> [PASS][23]
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb7/igt@gem_ctx_isolation@rcs0-s3.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb8/igt@gem_ctx_isolation@rcs0-s3.html

  * igt@gem_workarounds@suspend-resume-fd:
    - shard-skl:          [INCOMPLETE][24] ([fdo#104108]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-skl1/igt@gem_workarounds@suspend-resume-fd.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-skl10/igt@gem_workarounds@suspend-resume-fd.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt:
    - shard-iclb:         [FAIL][26] ([fdo#103167]) -> [PASS][27] +6 similar issues
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb1/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][28] ([fdo#108566]) -> [PASS][29] +3 similar issues
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-apl1/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-apl5/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_vblank@pipe-c-query-idle-hang:
    - shard-iclb:         [INCOMPLETE][30] ([fdo#107713]) -> [PASS][31]
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb1/igt@kms_vblank@pipe-c-query-idle-hang.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb7/igt@kms_vblank@pipe-c-query-idle-hang.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         [INCOMPLETE][32] ([fdo#107713] / [fdo#109100]) -> [TIMEOUT][33] ([fdo#109673])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6216/shard-iclb8/igt@gem_mmap_gtt@forked-big-copy-xy.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/shard-iclb1/igt@gem_mmap_gtt@forked-big-copy-xy.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110728]: https://bugs.freedesktop.org/show_bug.cgi?id=110728
  [fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854


Participating hosts (9 -> 9)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6216 -> Patchwork_13203

  CI_DRM_6216: e58a2b1f565ec5d77c69724d2f43a7de6f7953b3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5047: 3f4b1a49ed5e1c77ea42970d4d3156c997f66050 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13203: b02d6f0631c274c4a6d66fee16390f6078030080 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13203/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm: add fallback override/firmware EDID modes workaround

[Jani Nikula]

We've moved the override and firmware EDID (simply "override EDID" from
now on) handling to the low level drm_do_get_edid() function in order to
transparently use the override throughout the stack. The idea is that
you get the override EDID via the ->get_modes() hook.

Unfortunately, there are scenarios where the DDC probe in drm_get_edid()
called via ->get_modes() fails, although the preceding ->detect()
succeeds.

In the case reported by Paul Wise, the ->detect() hook,
intel_crt_detect(), relies on hotplug detect, bypassing the DDC. In the
case reported by Ilpo Järvinen, there is no ->detect() hook, which is
interpreted as connected. The subsequent DDC probe reached via
->get_modes() fails, and we don't even look at the override EDID,
resulting in no modes being added.

Because drm_get_edid() is used via ->detect() all over the place, we
can't trivially remove the DDC probe, as it leads to override EDID
effectively meaning connector forcing. The goal is that connector
forcing and override EDID remain orthogonal.

Generally, the underlying problem here is the conflation of ->detect()
and ->get_modes() via drm_get_edid(). The former should just detect, and
the latter should just get the modes, typically via reading the EDID. As
long as drm_get_edid() is used in ->detect(), it needs to retain the DDC
probe. Or such users need to have a separate DDC probe step first.

The EDID caching between ->detect() and ->get_modes() done by some
drivers is a further complication that prevents us from making
drm_do_get_edid() adapt to the two cases.

Work around the regression by falling back to a separate attempt at
getting the override EDID at drm_helper_probe_single_connector_modes()
level. With a working DDC and override EDID, it'll never be called; the
override EDID will come via ->get_modes(). There will still be a failing
DDC probe attempt in the cases that require the fallback.

v2:
- Call drm_connector_update_edid_property (Paul)
- Update commit message about EDID caching (Daniel)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107583
Reported-by: Paul Wise <pabs3@bonedaddy.net>
Cc: Paul Wise <pabs3@bonedaddy.net>
References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
Reported-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
Cc: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")
Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at drm_do_get_edid() level")
Cc: <stable@vger.kernel.org> # v4.15+
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Harish Chegondi <harish.chegondi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c         | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c |  7 +++++++
 include/drm/drm_edid.h             |  1 +
 3 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index c59a1e8c5ada..9d8f2b952004 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1587,6 +1587,36 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector)
 	return IS_ERR(override) ? NULL : override;
 }
 
+/**
+ * drm_add_override_edid_modes - add modes from override/firmware EDID
+ * @connector: connector we're probing
+ *
+ * Add modes from the override/firmware EDID, if available. Only to be used from
+ * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe
+ * failed during drm_get_edid() and caused the override/firmware EDID to be
+ * skipped.
+ *
+ * Return: The number of modes added or 0 if we couldn't find any.
+ */
+int drm_add_override_edid_modes(struct drm_connector *connector)
+{
+	struct edid *override;
+	int num_modes = 0;
+
+	override = drm_get_override_edid(connector);
+	if (override) {
+		drm_connector_update_edid_property(connector, override);
+		num_modes = drm_add_edid_modes(connector, override);
+		kfree(override);
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
+			      connector->base.id, connector->name, num_modes);
+	}
+
+	return num_modes;
+}
+EXPORT_SYMBOL(drm_add_override_edid_modes);
+
 /**
  * drm_do_get_edid - get EDID data using a custom EDID block read function
  * @connector: connector we're probing
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 01e243f1ea94..ef2c468205a2 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -480,6 +480,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
 
 	count = (*connector_funcs->get_modes)(connector);
 
+	/*
+	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
+	 * override/firmware EDID.
+	 */
+	if (count == 0 && connector->status == connector_status_connected)
+		count = drm_add_override_edid_modes(connector);
+
 	if (count == 0 && connector->status == connector_status_connected)
 		count = drm_add_modes_noedid(connector, 1024, 768);
 	count += drm_helper_probe_add_cmdline_mode(connector);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 88b63801f9db..b9719418c3d2 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -478,6 +478,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);
 struct edid *drm_edid_duplicate(const struct edid *edid);
 int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
+int drm_add_override_edid_modes(struct drm_connector *connector);
 
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
-- 
2.20.1

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Jani Nikula]

On Sat, 08 Jun 2019, Paul Wise <pabs3@bonedaddy.net> wrote:
> On Sat, 2019-06-08 at 13:10 +0800, Paul Wise wrote:
>
>> I've tested these two patches on top of Linux v5.2-rc3 and the EDID
>> override works correctly on an Intel Ironlake GPU with a monitor that
>> lost its EDID a while ago.
>
> While testing I noticed a couple of things:
>
> While everything the GUI is the correct resolution, GNOME is unable to
> identify the monitor vendor or model. This is a regression from the
> previous edid override functionality. It looks like this is because the
> edid file in /sys is not populated with the EDID override data.

Right, I've added a call to drm_connector_update_edid_property() in v2
to address this issue.

> I got a crash due to null pointer dereference at one point, I'll try to
> track down when this happens.

Can't think of why this would happen; the backtrace might offer clues.

Thanks for testing!

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)
URL   : https://patchwork.freedesktop.org/series/61764/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d5896a186b31 drm/edid: abstract override/firmware EDID retrieval
745bb8191a6d drm: add fallback override/firmware EDID modes workaround
-:53: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#53: 
References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi

-:57: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")'
#57: 
References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")

total: 1 errors, 1 warnings, 0 checks, 56 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)
URL   : https://patchwork.freedesktop.org/series/61764/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6225 -> Patchwork_13217
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/

Known issues
------------

  Here are the changes found in Patchwork_13217 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic-small-bo:
    - fi-icl-u3:          [PASS][1] -> [DMESG-WARN][2] ([fdo#107724])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/fi-icl-u3/igt@gem_mmap@basic-small-bo.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/fi-icl-u3/igt@gem_mmap@basic-small-bo.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         [PASS][3] -> [DMESG-WARN][4] ([fdo#106387]) +2 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/fi-ilk-650/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@gem_render_linear_blits@basic:
    - fi-icl-u3:          [DMESG-WARN][5] ([fdo#107724]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/fi-icl-u3/igt@gem_render_linear_blits@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/fi-icl-u3/igt@gem_render_linear_blits@basic.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][7] ([fdo#109485]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_frontbuffer_tracking@basic:
    - fi-hsw-peppy:       [DMESG-WARN][9] ([fdo#102614]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/fi-hsw-peppy/igt@kms_frontbuffer_tracking@basic.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102614]: https://bugs.freedesktop.org/show_bug.cgi?id=102614
  [fdo#106387]: https://bugs.freedesktop.org/show_bug.cgi?id=106387
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485


Participating hosts (54 -> 47)
------------------------------

  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * Linux: CI_DRM_6225 -> Patchwork_13217

  CI_DRM_6225: 39bb7459567aada2e706e4da4a650dc4f7c41abf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5049: db51cbba5a8f4856d6f56a61aa51fda6e239fa44 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13217: 745bb8191a6de49c7aed831251ea790e37342d33 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

745bb8191a6d drm: add fallback override/firmware EDID modes workaround
d5896a186b31 drm/edid: abstract override/firmware EDID retrieval

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/edid: abstract override/firmware EDID retrieval (rev2)
URL   : https://patchwork.freedesktop.org/series/61764/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6225_full -> Patchwork_13217_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13217_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@gem_ctx_engines@independent}:
    - shard-hsw:          [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-hsw1/igt@gem_ctx_engines@independent.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-hsw4/igt@gem_ctx_engines@independent.html
    - shard-snb:          [PASS][3] -> [DMESG-WARN][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-snb4/igt@gem_ctx_engines@independent.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-snb5/igt@gem_ctx_engines@independent.html

  
Known issues
------------

  Here are the changes found in Patchwork_13217_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_workarounds@suspend-resume:
    - shard-skl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#104108])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl4/igt@gem_workarounds@suspend-resume.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl5/igt@gem_workarounds@suspend-resume.html

  * igt@i915_suspend@fence-restore-tiled2untiled:
    - shard-apl:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108566]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-apl8/igt@i915_suspend@fence-restore-tiled2untiled.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [PASS][9] -> [FAIL][10] ([fdo#105363])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl2/igt@kms_flip@flip-vs-expired-vblank.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl4/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip_tiling@flip-changes-tiling:
    - shard-skl:          [PASS][11] -> [FAIL][12] ([fdo#108303])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl8/igt@kms_flip_tiling@flip-changes-tiling.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl8/igt@kms_flip_tiling@flip-changes-tiling.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
    - shard-iclb:         [PASS][13] -> [FAIL][14] ([fdo#103167]) +4 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-pwrite:
    - shard-hsw:          [PASS][15] -> [SKIP][16] ([fdo#109271]) +9 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-hsw8/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-pwrite.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-hsw1/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-cur-indfb-draw-pwrite.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#108145])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_setmode@basic:
    - shard-apl:          [PASS][19] -> [FAIL][20] ([fdo#99912])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-apl4/igt@kms_setmode@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-apl7/igt@kms_setmode@basic.html

  
#### Possible fixes ####

  * {igt@gem_ctx_engines@execute-oneforall}:
    - shard-snb:          [DMESG-WARN][21] ([fdo#110869]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-snb4/igt@gem_ctx_engines@execute-oneforall.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-snb5/igt@gem_ctx_engines@execute-oneforall.html

  * igt@gem_eio@in-flight-suspend:
    - shard-skl:          [INCOMPLETE][23] ([fdo#104108]) -> [PASS][24] +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl4/igt@gem_eio@in-flight-suspend.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl9/igt@gem_eio@in-flight-suspend.html

  * igt@kms_cursor_crc@pipe-b-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][25] ([fdo#108566]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-kbl6/igt@kms_cursor_crc@pipe-b-cursor-suspend.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-kbl4/igt@kms_cursor_crc@pipe-b-cursor-suspend.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          [FAIL][27] ([fdo#105363]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-glk1/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible.html

  * igt@kms_flip@2x-plain-flip-ts-check-interruptible:
    - shard-hsw:          [SKIP][29] ([fdo#109271]) -> [PASS][30] +22 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-hsw1/igt@kms_flip@2x-plain-flip-ts-check-interruptible.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-hsw4/igt@kms_flip@2x-plain-flip-ts-check-interruptible.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-skl:          [FAIL][31] ([fdo#105363]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-skl9/igt@kms_flip@flip-vs-expired-vblank-interruptible.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-skl6/igt@kms_flip@flip-vs-expired-vblank-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw:
    - shard-iclb:         [FAIL][33] ([fdo#103167]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-iclb6/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-pri-indfb-multidraw.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-apl:          [DMESG-WARN][35] ([fdo#108566]) -> [PASS][36] +5 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-apl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][37] ([fdo#99912]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-kbl2/igt@kms_setmode@basic.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-kbl3/igt@kms_setmode@basic.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [FAIL][39] ([fdo#100047]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6225/shard-iclb3/igt@kms_sysfs_edid_timing.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/shard-iclb5/igt@kms_sysfs_edid_timing.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110869]: https://bugs.freedesktop.org/show_bug.cgi?id=110869
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_6225 -> Patchwork_13217

  CI_DRM_6225: 39bb7459567aada2e706e4da4a650dc4f7c41abf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5049: db51cbba5a8f4856d6f56a61aa51fda6e239fa44 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13217: 745bb8191a6de49c7aed831251ea790e37342d33 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13217/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 6525 bytes --]

On Mon, 2019-06-10 at 12:30 +0300, Jani Nikula wrote:
> We've moved the override and firmware EDID (simply "override EDID" from
> now on) handling to the low level drm_do_get_edid() function in order to
> transparently use the override throughout the stack. The idea is that
> you get the override EDID via the ->get_modes() hook.
> 
> Unfortunately, there are scenarios where the DDC probe in drm_get_edid()
> called via ->get_modes() fails, although the preceding ->detect()
> succeeds.
> 
> In the case reported by Paul Wise, the ->detect() hook,
> intel_crt_detect(), relies on hotplug detect, bypassing the DDC. In the
> case reported by Ilpo Järvinen, there is no ->detect() hook, which is
> interpreted as connected. The subsequent DDC probe reached via
> ->get_modes() fails, and we don't even look at the override EDID,
> resulting in no modes being added.
> 
> Because drm_get_edid() is used via ->detect() all over the place, we
> can't trivially remove the DDC probe, as it leads to override EDID
> effectively meaning connector forcing. The goal is that connector
> forcing and override EDID remain orthogonal.
> 
> Generally, the underlying problem here is the conflation of ->detect()
> and ->get_modes() via drm_get_edid(). The former should just detect, and
> the latter should just get the modes, typically via reading the EDID. As
> long as drm_get_edid() is used in ->detect(), it needs to retain the DDC
> probe. Or such users need to have a separate DDC probe step first.
> 
> The EDID caching between ->detect() and ->get_modes() done by some
> drivers is a further complication that prevents us from making
> drm_do_get_edid() adapt to the two cases.
> 
> Work around the regression by falling back to a separate attempt at
> getting the override EDID at drm_helper_probe_single_connector_modes()
> level. With a working DDC and override EDID, it'll never be called; the
> override EDID will come via ->get_modes(). There will still be a failing
> DDC probe attempt in the cases that require the fallback.
> 
> v2:
> - Call drm_connector_update_edid_property (Paul)
> - Update commit message about EDID caching (Daniel)
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107583
> Reported-by: Paul Wise <pabs3@bonedaddy.net>
> Cc: Paul Wise <pabs3@bonedaddy.net>
> References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
> Reported-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> Cc: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")
> Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at drm_do_get_edid() level")
> Cc: <stable@vger.kernel.org> # v4.15+
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Harish Chegondi <harish.chegondi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c         | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c |  7 +++++++
>  include/drm/drm_edid.h             |  1 +
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index c59a1e8c5ada..9d8f2b952004 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1587,6 +1587,36 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector)
>  	return IS_ERR(override) ? NULL : override;
>  }
>  
> +/**
> + * drm_add_override_edid_modes - add modes from override/firmware EDID
> + * @connector: connector we're probing
> + *
> + * Add modes from the override/firmware EDID, if available. Only to be used from
> + * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe
> + * failed during drm_get_edid() and caused the override/firmware EDID to be
> + * skipped.
> + *
> + * Return: The number of modes added or 0 if we couldn't find any.
> + */
> +int drm_add_override_edid_modes(struct drm_connector *connector)
> +{
> +	struct edid *override;
> +	int num_modes = 0;
> +
> +	override = drm_get_override_edid(connector);
> +	if (override) {
> +		drm_connector_update_edid_property(connector, override);
> +		num_modes = drm_add_edid_modes(connector, override);
> +		kfree(override);
> +
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
> +			      connector->base.id, connector->name, num_modes);
> +	}
> +
> +	return num_modes;
> +}
> +EXPORT_SYMBOL(drm_add_override_edid_modes);
> +
>  /**
>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>   * @connector: connector we're probing
> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
> index 01e243f1ea94..ef2c468205a2 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -480,6 +480,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>  
>  	count = (*connector_funcs->get_modes)(connector);
>  
> +	/*
> +	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
> +	 * override/firmware EDID.
> +	 */
> +	if (count == 0 && connector->status == connector_status_connected)
> +		count = drm_add_override_edid_modes(connector);
> +
>  	if (count == 0 && connector->status == connector_status_connected)
>  		count = drm_add_modes_noedid(connector, 1024, 768);
>  	count += drm_helper_probe_add_cmdline_mode(connector);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 88b63801f9db..b9719418c3d2 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -478,6 +478,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>  				     struct i2c_adapter *adapter);
>  struct edid *drm_edid_duplicate(const struct edid *edid);
>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
> +int drm_add_override_edid_modes(struct drm_connector *connector);
>  
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);

Tested-by: Paul Wise <pabs3@bonedaddy.net>

This version looks good to me.

Both the EDID override data and the resolution are fixed.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] drm: add fallback override/firmware EDID modes workaround

[Paul Wise]

[-- Attachment #1: Type: text/plain, Size: 389 bytes --]

On Mon, 2019-06-10 at 12:32 +0300, Jani Nikula wrote:

> Right, I've added a call to drm_connector_update_edid_property() in v2
> to address this issue.

Confirmed this fixed the EDID override data.

> Can't think of why this would happen; the backtrace might offer clues.

Unfortunately I wasn't able to capture the backtrace.

-- 
bye,
pabs

https://bonedaddy.net/pabs3/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] drm: add fallback override/firmware EDID modes workaround

[Jani Nikula]

On Tue, 11 Jun 2019, Paul Wise <pabs3@bonedaddy.net> wrote:
> On Mon, 2019-06-10 at 12:30 +0300, Jani Nikula wrote:
>> We've moved the override and firmware EDID (simply "override EDID" from
>> now on) handling to the low level drm_do_get_edid() function in order to
>> transparently use the override throughout the stack. The idea is that
>> you get the override EDID via the ->get_modes() hook.
>> 
>> Unfortunately, there are scenarios where the DDC probe in drm_get_edid()
>> called via ->get_modes() fails, although the preceding ->detect()
>> succeeds.
>> 
>> In the case reported by Paul Wise, the ->detect() hook,
>> intel_crt_detect(), relies on hotplug detect, bypassing the DDC. In the
>> case reported by Ilpo Järvinen, there is no ->detect() hook, which is
>> interpreted as connected. The subsequent DDC probe reached via
>> ->get_modes() fails, and we don't even look at the override EDID,
>> resulting in no modes being added.
>> 
>> Because drm_get_edid() is used via ->detect() all over the place, we
>> can't trivially remove the DDC probe, as it leads to override EDID
>> effectively meaning connector forcing. The goal is that connector
>> forcing and override EDID remain orthogonal.
>> 
>> Generally, the underlying problem here is the conflation of ->detect()
>> and ->get_modes() via drm_get_edid(). The former should just detect, and
>> the latter should just get the modes, typically via reading the EDID. As
>> long as drm_get_edid() is used in ->detect(), it needs to retain the DDC
>> probe. Or such users need to have a separate DDC probe step first.
>> 
>> The EDID caching between ->detect() and ->get_modes() done by some
>> drivers is a further complication that prevents us from making
>> drm_do_get_edid() adapt to the two cases.
>> 
>> Work around the regression by falling back to a separate attempt at
>> getting the override EDID at drm_helper_probe_single_connector_modes()
>> level. With a working DDC and override EDID, it'll never be called; the
>> override EDID will come via ->get_modes(). There will still be a failing
>> DDC probe attempt in the cases that require the fallback.
>> 
>> v2:
>> - Call drm_connector_update_edid_property (Paul)
>> - Update commit message about EDID caching (Daniel)
>> 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107583
>> Reported-by: Paul Wise <pabs3@bonedaddy.net>
>> Cc: Paul Wise <pabs3@bonedaddy.net>
>> References: http://mid.mail-archive.com/alpine.DEB.2.20.1905262211270.24390@whs-18.cs.helsinki.fi
>> Reported-by: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
>> Cc: Ilpo Järvinen <ilpo.jarvinen@cs.helsinki.fi>
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> References: 15f080f08d48 ("drm/edid: respect connector force for drm_get_edid ddc probe")
>> Fixes: 53fd40a90f3c ("drm: handle override and firmware EDID at drm_do_get_edid() level")
>> Cc: <stable@vger.kernel.org> # v4.15+
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Harish Chegondi <harish.chegondi@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c         | 30 ++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/drm_probe_helper.c |  7 +++++++
>>  include/drm/drm_edid.h             |  1 +
>>  3 files changed, 38 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index c59a1e8c5ada..9d8f2b952004 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1587,6 +1587,36 @@ static struct edid *drm_get_override_edid(struct drm_connector *connector)
>>  	return IS_ERR(override) ? NULL : override;
>>  }
>>  
>> +/**
>> + * drm_add_override_edid_modes - add modes from override/firmware EDID
>> + * @connector: connector we're probing
>> + *
>> + * Add modes from the override/firmware EDID, if available. Only to be used from
>> + * drm_helper_probe_single_connector_modes() as a fallback for when DDC probe
>> + * failed during drm_get_edid() and caused the override/firmware EDID to be
>> + * skipped.
>> + *
>> + * Return: The number of modes added or 0 if we couldn't find any.
>> + */
>> +int drm_add_override_edid_modes(struct drm_connector *connector)
>> +{
>> +	struct edid *override;
>> +	int num_modes = 0;
>> +
>> +	override = drm_get_override_edid(connector);
>> +	if (override) {
>> +		drm_connector_update_edid_property(connector, override);
>> +		num_modes = drm_add_edid_modes(connector, override);
>> +		kfree(override);
>> +
>> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] adding %d modes via fallback override/firmware EDID\n",
>> +			      connector->base.id, connector->name, num_modes);
>> +	}
>> +
>> +	return num_modes;
>> +}
>> +EXPORT_SYMBOL(drm_add_override_edid_modes);
>> +
>>  /**
>>   * drm_do_get_edid - get EDID data using a custom EDID block read function
>>   * @connector: connector we're probing
>> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
>> index 01e243f1ea94..ef2c468205a2 100644
>> --- a/drivers/gpu/drm/drm_probe_helper.c
>> +++ b/drivers/gpu/drm/drm_probe_helper.c
>> @@ -480,6 +480,13 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector,
>>  
>>  	count = (*connector_funcs->get_modes)(connector);
>>  
>> +	/*
>> +	 * Fallback for when DDC probe failed in drm_get_edid() and thus skipped
>> +	 * override/firmware EDID.
>> +	 */
>> +	if (count == 0 && connector->status == connector_status_connected)
>> +		count = drm_add_override_edid_modes(connector);
>> +
>>  	if (count == 0 && connector->status == connector_status_connected)
>>  		count = drm_add_modes_noedid(connector, 1024, 768);
>>  	count += drm_helper_probe_add_cmdline_mode(connector);
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 88b63801f9db..b9719418c3d2 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -478,6 +478,7 @@ struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>>  				     struct i2c_adapter *adapter);
>>  struct edid *drm_edid_duplicate(const struct edid *edid);
>>  int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
>> +int drm_add_override_edid_modes(struct drm_connector *connector);
>>  
>>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
>>  enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
>
> Tested-by: Paul Wise <pabs3@bonedaddy.net>
>
> This version looks good to me.
>
> Both the EDID override data and the resolution are fixed.

Thanks for testing and review, pushed both to drm-misc-fixes, cc: stable
v4.15.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center