[PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume

[PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume

[Gwan-gyeong Mun]

This patch series fix missed detection of changing of edid on between
suspend and resume.
First patch fixes drm_helper_hdp_irq_event() in order to fix a below use
case.

 Following scenario requires detection of changing of edid.
    
  1) plug display device to a connector
  2) system suspend
  3) unplug 1)'s display device and plug the other display device to a
     connector
  4) system resume

It adds edid check routine when a connector status still remains as
"connector_status_connected".

Second patch adds a missed update of edid property of drm connector on i915.
  
v2: Add NULL check before comparing of EDIDs.
v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika)
v4: Rebased
v5: Use a cached edid property blob data of connector instead of adding
    a new detected_edid variable. (Maarten)
    Add an using of reference count for getting a cached edid property
    blob data. (Maarten)

Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

v1, v2: https://patchwork.freedesktop.org/series/47680/
v3: https://patchwork.freedesktop.org/series/49298/
v4: https://patchwork.freedesktop.org/series/57397/

Gwan-gyeong Mun (2):
  drm: Add detection of changing of edid on between suspend and resume
  drm/i915: Add a missed update of edid property of drm connector

 drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_dp.c    |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c  |  1 +
 3 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.21.0

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

[PATCH v5 1/2] drm: Add detection of changing of edid on between suspend and resume

[Gwan-gyeong Mun]

The hotplug detection routine of drm_helper_hpd_irq_event() can detect
changing of status of connector, but it can not detect changing of edid.

Following scenario requires detection of changing of edid.

 1) plug display device to a connector
 2) system suspend
 3) unplug 1)'s display device and plug the other display device to a
    connector
 4) system resume

It adds edid check routine when a connector status still remains as
"connector_status_connected".

v2: Add NULL check before comparing of EDIDs.
v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika)
v4: Rebased
v5: Use a cached edid property blob data of connector instead of adding
    a new detected_edid variable. (Maarten)
    Add an using of reference count for getting a cached edid property
    blob data. (Maarten)

Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
---
 drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index 6fd08e04b323..27ad7f3dabb7 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
  * panels.
  *
  * This helper function is useful for drivers which can't or don't track hotplug
- * interrupts for each connector.
+ * interrupts for each connector. And it also supports a detection of changing
+ * of edid on between suspend and resume when a connector status still remains
+ * as "connector_status_connected".
+ *
+ * Following scenario requires detection of changing of edid.
+ *  1) plug display device to a connector
+ *  2) system suspend
+ *  3) unplug 1)'s display device and plug the other display device to a
+ *     connector
+ *  4) system resume
  *
  * Drivers which support hotplug interrupts for each connector individually and
  * which have a more fine-grained detect logic should bypass this code and
@@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	struct drm_connector *connector;
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;
+	struct drm_property_blob *old_edid_blob_ptr;
 	bool changed = false;
 
 	if (!dev->mode_config.poll_enabled)
@@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 
 		old_status = connector->status;
 
+		if (connector->edid_blob_ptr)
+			old_edid_blob_ptr = drm_property_blob_get(connector->edid_blob_ptr);
+		else
+			old_edid_blob_ptr = NULL;
+
 		connector->status = drm_helper_probe_detect(connector, NULL, false);
 		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
 			      connector->base.id,
@@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 			      drm_get_connector_status_name(connector->status));
 		if (old_status != connector->status)
 			changed = true;
+
+		/* Check changing of edid when a connector status still remains
+		 * as "connector_status_connected".
+		 */
+		if (old_edid_blob_ptr && connector->edid_blob_ptr &&
+		    old_status == connector->status &&
+		    old_status == connector_status_connected) {
+			if (memcmp(old_edid_blob_ptr->data,
+				connector->edid_blob_ptr->data,
+				old_edid_blob_ptr->length)) {
+				changed = true;
+				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid updated\n",
+					      connector->base.id,
+					      connector->name);
+			}
+		}
+		drm_property_blob_put(old_edid_blob_ptr);
 	}
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
-- 
2.21.0

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

[PATCH v5 2/2] drm/i915: Add a missed update of edid property of drm connector

[Gwan-gyeong Mun]

After suspend/resume process, hotplug detection is handled by
i915_hpd_poll_init_work() workqueue. While intel_hdmi_detect() or
intel_dp_detect() are called, intel_hdmi_set_edid() or intel_dp_set_edid()
only update an internal detect_edid variable of intel_connector.
A missed update of edid property of drm_connector leads incorrect behavior
of drm_helper_hpd_irq_event() on below testcases.
It adds a missed update of edid property of drm connector and updates
drm edid modes, while i915_hpd_poll_init_work() workqueue works.

Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
Testcase: igt/kms_chamelium/dp-edid-change-during-suspend

Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   | 1 +
 drivers/gpu/drm/i915/intel_hdmi.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c4e36759a756..0301e58495b4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5379,6 +5379,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
 	drm_dp_cec_set_edid(&intel_dp->aux, edid);
+	intel_connector_update_modes(&intel_connector->base, edid);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e1005d7b75fd..b53360c4d0ef 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2493,6 +2493,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	}
 
 	cec_notifier_set_phys_addr_from_edid(intel_hdmi->cec_notifier, edid);
+	intel_connector_update_modes(connector, edid);
 
 	return connected;
 }
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 1/2] drm: Add detection of changing of edid on between suspend and resume

[Lisovskiy, Stanislav]

On Thu, 2019-04-11 at 17:36 +0300, Gwan-gyeong Mun wrote:
> The hotplug detection routine of drm_helper_hpd_irq_event() can
> detect
> changing of status of connector, but it can not detect changing of
> edid.
> 
> Following scenario requires detection of changing of edid.
> 
>  1) plug display device to a connector
>  2) system suspend
>  3) unplug 1)'s display device and plug the other display device to a
>     connector
>  4) system resume
> 
> It adds edid check routine when a connector status still remains as
> "connector_status_connected".
> 
> v2: Add NULL check before comparing of EDIDs.
> v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,
> Mika)
> v4: Rebased
> v5: Use a cached edid property blob data of connector instead of
> adding
>     a new detected_edid variable. (Maarten)
>     Add an using of reference count for getting a cached edid
> property
>     blob data. (Maarten)
> 
> Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
> Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
> Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
> Testcase: igt/kms_chamelium/dp-edid-change-during-suspend
> 
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> ---
>  drivers/gpu/drm/drm_probe_helper.c | 34
> +++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 6fd08e04b323..27ad7f3dabb7 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
>   * panels.
>   *
>   * This helper function is useful for drivers which can't or don't
> track hotplug
> - * interrupts for each connector.
> + * interrupts for each connector. And it also supports a detection
> of changing
> + * of edid on between suspend and resume when a connector status
> still remains
> + * as "connector_status_connected".
> + *
> + * Following scenario requires detection of changing of edid.
> + *  1) plug display device to a connector
> + *  2) system suspend
> + *  3) unplug 1)'s display device and plug the other display device
> to a
> + *     connector
> + *  4) system resume
>   *
>   * Drivers which support hotplug interrupts for each connector
> individually and
>   * which have a more fine-grained detect logic should bypass this
> code and
> @@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device
> *dev)
>  	struct drm_connector *connector;
>  	struct drm_connector_list_iter conn_iter;
>  	enum drm_connector_status old_status;
> +	struct drm_property_blob *old_edid_blob_ptr;
>  	bool changed = false;
>  
>  	if (!dev->mode_config.poll_enabled)
> @@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct drm_device
> *dev)
>  
>  		old_status = connector->status;
>  
> +		if (connector->edid_blob_ptr)
> +			old_edid_blob_ptr =
> drm_property_blob_get(connector->edid_blob_ptr);
> +		else
> +			old_edid_blob_ptr = NULL;
> +
>  		connector->status = drm_helper_probe_detect(connector,
> NULL, false);
>  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s
> to %s\n",
>  			      connector->base.id,
> @@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct drm_device
> *dev)
>  			      drm_get_connector_status_name(connector-
> >status));
>  		if (old_status != connector->status)
>  			changed = true;
> +
> +		/* Check changing of edid when a connector status still
> remains
> +		 * as "connector_status_connected".
> +		 */
> +		if (old_edid_blob_ptr && connector->edid_blob_ptr &&

I guess you don't need to check both old_edid_blob_ptr && connector-
>edid_blob_ptr here. Because if old_edid_blob_ptr is not NULL - it
means that connector->edid_blob_ptr was not NULL for sure. See the 
condition you have added above.
I mean this one:

> +		if (connector->edid_blob_ptr)
> +			old_edid_blob_ptr =
> drm_property_blob_get(connector->edid_blob_ptr);
> +		else
> +			old_edid_blob_ptr = NULL;

So I would check only old_edid_blob_ptr for not being NULL here.



> +		    old_status == connector->status &&
> +		    old_status == connector_status_connected) {
> +			if (memcmp(old_edid_blob_ptr->data,
> +				connector->edid_blob_ptr->data,
> +				old_edid_blob_ptr->length)) {
> +				changed = true;
> +				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid
> updated\n",
> +					      connector->base.id,
> +					      connector->name);
> +			}
> +		}
> +		drm_property_blob_put(old_edid_blob_ptr);
>  	}
>  	drm_connector_list_iter_end(&conn_iter);
>  	mutex_unlock(&dev->mode_config.mutex);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm: Add detection of changing of edid on between suspend and resume

[Patchwork]

== Series Details ==

Series: drm: Add detection of changing of edid on between suspend and resume
URL   : https://patchwork.freedesktop.org/series/59352/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f8af584568d1 drm: Add detection of changing of edid on between suspend and resume
-:90: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#90: FILE: drivers/gpu/drm/drm_probe_helper.c:808:
+			if (memcmp(old_edid_blob_ptr->data,
+				connector->edid_blob_ptr->data,

total: 0 errors, 0 warnings, 1 checks, 58 lines checked
ccdf8a88a55c drm/i915: Add a missed update of edid property of drm connector

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

✗ Fi.CI.BAT: failure for drm: Add detection of changing of edid on between suspend and resume

[Patchwork]

== Series Details ==

Series: drm: Add detection of changing of edid on between suspend and resume
URL   : https://patchwork.freedesktop.org/series/59352/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5917 -> Patchwork_12767
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59352/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-icl-y:           NOTRUN -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-kbl-7500u:       NOTRUN -> SKIP [fdo#109271] +28

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +52

  * igt@gem_exec_basic@readonly-bsd1:
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] +52

  * igt@gem_exec_store@basic-bsd2:
    - fi-hsw-4770:        NOTRUN -> SKIP [fdo#109271] +41

  * igt@kms_busy@basic-flip-c:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-snb-2520m:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7500u:       NOTRUN -> DMESG-WARN [fdo#102505] / [fdo#103558] / [fdo#105079] / [fdo#105602]

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-byt-j1900:       PASS -> INCOMPLETE [fdo#102657]

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-byt-n2820:       PASS -> INCOMPLETE [fdo#102657]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-glk-dsi:         PASS -> FAIL [fdo#103191]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103191]

  
#### Possible fixes ####

  * igt@i915_selftest@live_contexts:
    - fi-bdw-gvtdvm:      DMESG-FAIL [fdo#110235 ] -> PASS

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       DMESG-WARN [fdo#103841] -> PASS

  
  [fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
  [fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110235 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110235 


Participating hosts (47 -> 44)
------------------------------

  Additional (3): fi-hsw-4770 fi-byt-clapper fi-snb-2520m 
  Missing    (6): fi-kbl-soraka fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


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

    * Linux: CI_DRM_5917 -> Patchwork_12767

  CI_DRM_5917: b01c0e68e8d1092c436dbba4d03b260c828f37c9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4944: 9b74b8226e8c108db091bd3b1d105a71dc0fb861 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12767: ccdf8a88a55c02a77ad81b97714fdf561a8dbbee @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

ccdf8a88a55c drm/i915: Add a missed update of edid property of drm connector
f8af584568d1 drm: Add detection of changing of edid on between suspend and resume

== Logs ==

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

✗ Fi.CI.CHECKPATCH: warning for drm: Add detection of changing of edid on between suspend and resume (rev2)

[Patchwork]

== Series Details ==

Series: drm: Add detection of changing of edid on between suspend and resume (rev2)
URL   : https://patchwork.freedesktop.org/series/59352/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
8cadfc318c07 drm: Add detection of changing of edid on between suspend and resume
-:90: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#90: FILE: drivers/gpu/drm/drm_probe_helper.c:808:
+			if (memcmp(old_edid_blob_ptr->data,
+				connector->edid_blob_ptr->data,

total: 0 errors, 0 warnings, 1 checks, 58 lines checked
10427a911a8a drm/i915: Add a missed update of edid property of drm connector

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

✗ Fi.CI.BAT: failure for drm: Add detection of changing of edid on between suspend and resume (rev2)

[Patchwork]

== Series Details ==

Series: drm: Add detection of changing of edid on between suspend and resume (rev2)
URL   : https://patchwork.freedesktop.org/series/59352/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5922 -> Patchwork_12774
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59352/revisions/2/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_hangcheck:
    - fi-icl-u3:          PASS -> DMESG-WARN

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-byt-n2820:       PASS -> DMESG-WARN

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@semaphore:
    - fi-kbl-7500u:       NOTRUN -> SKIP [fdo#109271] +28

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7500u:       NOTRUN -> DMESG-WARN [fdo#102505] / [fdo#103558] / [fdo#105079] / [fdo#105602]

  * igt@kms_flip@basic-flip-vs-dpms:
    - fi-byt-j1900:       PASS -> INCOMPLETE [fdo#102657]

  * igt@kms_flip@basic-flip-vs-modeset:
    - fi-bsw-n3050:       PASS -> INCOMPLETE [fdo#105876]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] +1

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      FAIL [fdo#108511] -> PASS

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       DMESG-WARN [fdo#103841] -> PASS

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-a:
    - fi-glk-dsi:         FAIL [fdo#103191] -> PASS

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

  [fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
  [fdo#102657]: https://bugs.freedesktop.org/show_bug.cgi?id=102657
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103841]: https://bugs.freedesktop.org/show_bug.cgi?id=103841
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#105876]: https://bugs.freedesktop.org/show_bug.cgi?id=105876
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109309]: https://bugs.freedesktop.org/show_bug.cgi?id=109309
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109316]: https://bugs.freedesktop.org/show_bug.cgi?id=109316


Participating hosts (49 -> 44)
------------------------------

  Additional (1): fi-icl-u2 
  Missing    (6): fi-kbl-soraka fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-gdg-551 


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

    * Linux: CI_DRM_5922 -> Patchwork_12774

  CI_DRM_5922: 849ac6dbff7f5073c3181c5eba07936fe3f576ec @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4944: 9b74b8226e8c108db091bd3b1d105a71dc0fb861 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12774: 10427a911a8af6236743d7d4cea2154e3fe46f08 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

10427a911a8a drm/i915: Add a missed update of edid property of drm connector
8cadfc318c07 drm: Add detection of changing of edid on between suspend and resume

== Logs ==

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

Re: [PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume

[Daniel Vetter]

On Thu, Apr 11, 2019 at 05:36:30PM +0300, Gwan-gyeong Mun wrote:
> This patch series fix missed detection of changing of edid on between
> suspend and resume.
> First patch fixes drm_helper_hdp_irq_event() in order to fix a below use
> case.
> 
>  Following scenario requires detection of changing of edid.
>     
>   1) plug display device to a connector
>   2) system suspend
>   3) unplug 1)'s display device and plug the other display device to a
>      connector
>   4) system resume
> 
> It adds edid check routine when a connector status still remains as
> "connector_status_connected".
> 
> Second patch adds a missed update of edid property of drm connector on i915.
>   
> v2: Add NULL check before comparing of EDIDs.
> v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan, Mika)
> v4: Rebased
> v5: Use a cached edid property blob data of connector instead of adding
>     a new detected_edid variable. (Maarten)
>     Add an using of reference count for getting a cached edid property
>     blob data. (Maarten)
> 
> Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
> Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
> Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
> Testcase: igt/kms_chamelium/dp-edid-change-during-suspend
> 
> v1, v2: https://patchwork.freedesktop.org/series/47680/
> v3: https://patchwork.freedesktop.org/series/49298/
> v4: https://patchwork.freedesktop.org/series/57397/

I guess I missed this, but there's a few fundamental things here first
imo:

- patch 2 is fallout from the fairly abitrary split between ->detect and
  ->get_modes. There's not really any good reason for that, I think we can
  just always call ->get_modes if ->detect says the connector is
  connected. There's more than just dp and hdmi (that's simply the 2
  things we test in CI using chamelium).

- the problem is bigger than just the edid changing (e.g. when repeaters
  change, or when something else changes aside from the edid, e.g. in dp
  aux, like how many lanes the dp cable has). Plus we have this long-term
  idea to give userspace a better idea about which connector exactly has
  changed. Therefor:

  1. add a new drm_connector->detect_epoque counter.
  2. Increment that counter every time something has changed, i.e. from
  probe helpers (we can push this down into the detect wrappers) when
  connector->status has changed, or when we update the edid blob.
  3. probe helpers look for changes in ->detect_epoque to decided whether
  to fire the uevent or not.
  4. long term we could expose the ->detect_epoque as a read-only
  property, or at least supply information about which connector caused
  the uevent using the new drm_sysfs_connector_status_event() (see the
  patch from Ram "[PATCH v3 09/10] drm: uevent for connector status
  change").

I think that gives us a much better long term foundation than adding lots
of hacks.

Cheers, Daniel

> 
> Gwan-gyeong Mun (2):
>   drm: Add detection of changing of edid on between suspend and resume
>   drm/i915: Add a missed update of edid property of drm connector
> 
>  drivers/gpu/drm/drm_probe_helper.c | 34 +++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_dp.c    |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c  |  1 +
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> -- 
> 2.21.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/2] drm: Add detection of changing of edid on between suspend and resume

[Mun, Gwan-gyeong]

On Thu, 2019-04-11 at 17:00 +0100, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-11 at 17:36 +0300, Gwan-gyeong Mun wrote:
> > The hotplug detection routine of drm_helper_hpd_irq_event() can
> > detect
> > changing of status of connector, but it can not detect changing of
> > edid.
> > 
> > Following scenario requires detection of changing of edid.
> > 
> >  1) plug display device to a connector
> >  2) system suspend
> >  3) unplug 1)'s display device and plug the other display device to
> > a
> >     connector
> >  4) system resume
> > 
> > It adds edid check routine when a connector status still remains as
> > "connector_status_connected".
> > 
> > v2: Add NULL check before comparing of EDIDs.
> > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,
> > Mika)
> > v4: Rebased
> > v5: Use a cached edid property blob data of connector instead of
> > adding
> >     a new detected_edid variable. (Maarten)
> >     Add an using of reference count for getting a cached edid
> > property
> >     blob data. (Maarten)
> > 
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
> > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend
> > 
> > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> > ---
> >  drivers/gpu/drm/drm_probe_helper.c | 34
> > +++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_probe_helper.c
> > b/drivers/gpu/drm/drm_probe_helper.c
> > index 6fd08e04b323..27ad7f3dabb7 100644
> > --- a/drivers/gpu/drm/drm_probe_helper.c
> > +++ b/drivers/gpu/drm/drm_probe_helper.c
> > @@ -742,7 +742,16 @@ EXPORT_SYMBOL(drm_kms_helper_poll_fini);
> >   * panels.
> >   *
> >   * This helper function is useful for drivers which can't or don't
> > track hotplug
> > - * interrupts for each connector.
> > + * interrupts for each connector. And it also supports a detection
> > of changing
> > + * of edid on between suspend and resume when a connector status
> > still remains
> > + * as "connector_status_connected".
> > + *
> > + * Following scenario requires detection of changing of edid.
> > + *  1) plug display device to a connector
> > + *  2) system suspend
> > + *  3) unplug 1)'s display device and plug the other display
> > device
> > to a
> > + *     connector
> > + *  4) system resume
> >   *
> >   * Drivers which support hotplug interrupts for each connector
> > individually and
> >   * which have a more fine-grained detect logic should bypass this
> > code and
> > @@ -760,6 +769,7 @@ bool drm_helper_hpd_irq_event(struct drm_device
> > *dev)
> >  	struct drm_connector *connector;
> >  	struct drm_connector_list_iter conn_iter;
> >  	enum drm_connector_status old_status;
> > +	struct drm_property_blob *old_edid_blob_ptr;
> >  	bool changed = false;
> >  
> >  	if (!dev->mode_config.poll_enabled)
> > @@ -774,6 +784,11 @@ bool drm_helper_hpd_irq_event(struct
> > drm_device
> > *dev)
> >  
> >  		old_status = connector->status;
> >  
> > +		if (connector->edid_blob_ptr)
> > +			old_edid_blob_ptr =
> > drm_property_blob_get(connector->edid_blob_ptr);
> > +		else
> > +			old_edid_blob_ptr = NULL;
> > +
> >  		connector->status = drm_helper_probe_detect(connector,
> > NULL, false);
> >  		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s
> > to %s\n",
> >  			      connector->base.id,
> > @@ -782,6 +797,23 @@ bool drm_helper_hpd_irq_event(struct
> > drm_device
> > *dev)
> >  			      drm_get_connector_status_name(connector-
> > > status));
> >  		if (old_status != connector->status)
> >  			changed = true;
> > +
> > +		/* Check changing of edid when a connector status still
> > remains
> > +		 * as "connector_status_connected".
> > +		 */
> > +		if (old_edid_blob_ptr && connector->edid_blob_ptr &&
> 
> I guess you don't need to check both old_edid_blob_ptr && connector-
> > edid_blob_ptr here. Because if old_edid_blob_ptr is not NULL - it
> means that connector->edid_blob_ptr was not NULL for sure. See the 
> condition you have added above.
> I mean this one:
> 
> > +		if (connector->edid_blob_ptr)
> > +			old_edid_blob_ptr =
> > drm_property_blob_get(connector->edid_blob_ptr);
> > +		else
> > +			old_edid_blob_ptr = NULL;
> 
> So I would check only old_edid_blob_ptr for not being NULL here.
> 
> 
Hi Stan,

Thank you for checking it.

First, drivers could set edid_blob_ptr to NULL with
drm_connector_update_edid_property().
And drm_helper_probe_detect() could lead updating of an internal edid
of driver and if driver can not get EDID from I2C, it could call
drm_connector_update_edid_property() with NULL.

therefore I think that we should check old and new edid_blob_ptr are
NULL or not.
> 
> > +		    old_status == connector->status &&
> > +		    old_status == connector_status_connected) {
> > +			if (memcmp(old_edid_blob_ptr->data,
> > +				connector->edid_blob_ptr->data,
> > +				old_edid_blob_ptr->length)) {
> > +				changed = true;
> > +				DRM_DEBUG_KMS("[CONNECTOR:%d:%s] edid
> > updated\n",
> > +					      connector->base.id,
> > +					      connector->name);
> > +			}
> > +		}
> > +		drm_property_blob_put(old_edid_blob_ptr);
> >  	}
> >  	drm_connector_list_iter_end(&conn_iter);
> >  	mutex_unlock(&dev->mode_config.mutex);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH v5 0/2] drm: Add detection of changing of edid on between suspend and resume

[Mun, Gwan-gyeong]

On Mon, 2019-04-15 at 18:32 +0200, Daniel Vetter wrote:
> On Thu, Apr 11, 2019 at 05:36:30PM +0300, Gwan-gyeong Mun wrote:
> > This patch series fix missed detection of changing of edid on
> > between
> > suspend and resume.
> > First patch fixes drm_helper_hdp_irq_event() in order to fix a
> > below use
> > case.
> > 
> >  Following scenario requires detection of changing of edid.
> >     
> >   1) plug display device to a connector
> >   2) system suspend
> >   3) unplug 1)'s display device and plug the other display device
> > to a
> >      connector
> >   4) system resume
> > 
> > It adds edid check routine when a connector status still remains as
> > "connector_status_connected".
> > 
> > Second patch adds a missed update of edid property of drm connector
> > on i915.
> >   
> > v2: Add NULL check before comparing of EDIDs.
> > v3: Make it as part of existing drm_helper_hpd_irq_event() (Stan,
> > Mika)
> > v4: Rebased
> > v5: Use a cached edid property blob data of connector instead of
> > adding
> >     a new detected_edid variable. (Maarten)
> >     Add an using of reference count for getting a cached edid
> > property
> >     blob data. (Maarten)
> > 
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/hdmi-edid-change-during-suspend
> > Testcase: igt/kms_chamelium/dp-edid-change-during-hibernate
> > Testcase: igt/kms_chamelium/dp-edid-change-during-suspend
> > 
> > v1, v2: https://patchwork.freedesktop.org/series/47680/
> > v3: https://patchwork.freedesktop.org/series/49298/
> > v4: https://patchwork.freedesktop.org/series/57397/
> 
> I guess I missed this, but there's a few fundamental things here
> first
> imo:
> 
> - patch 2 is fallout from the fairly abitrary split between ->detect
> and
>   ->get_modes. There's not really any good reason for that, I think
> we can
>   just always call ->get_modes if ->detect says the connector is
>   connected. There's more than just dp and hdmi (that's simply the 2
>   things we test in CI using chamelium).
> 
Thank you for checking it.

I agree. I'll modify first patch with your guide and will resend it.
> - the problem is bigger than just the edid changing (e.g. when
> repeaters
>   change, or when something else changes aside from the edid, e.g. in
> dp
>   aux, like how many lanes the dp cable has). Plus we have this long-
> term
>   idea to give userspace a better idea about which connector exactly
> has
>   changed. Therefor:
> 
Yes, you pointed an essential problem.

>   1. add a new drm_connector->detect_epoque counter.
>   2. Increment that counter every time something has changed, i.e.
> from
>   probe helpers (we can push this down into the detect wrappers) when
>   connector->status has changed, or when we update the edid blob.
>   3. probe helpers look for changes in ->detect_epoque to decided
> whether
>   to fire the uevent or not.
>   4. long term we could expose the ->detect_epoque as a read-only
>   property, or at least supply information about which connector
> caused
>   the uevent using the new drm_sysfs_connector_status_event() (see
> the
>   patch from Ram "[PATCH v3 09/10] drm: uevent for connector status
>   change").
> 
I'll make new patches which followes your guide.
Thanks for reviewing it and giving me a new approach.

Br,
G.G.

> I think that gives us a much better long term foundation than adding
> lots
> of hacks.
> 
> Cheers, Daniel
> 
> > Gwan-gyeong Mun (2):
> >   drm: Add detection of changing of edid on between suspend and
> > resume
> >   drm/i915: Add a missed update of edid property of drm connector
> > 
> >  drivers/gpu/drm/drm_probe_helper.c | 34
> > +++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_dp.c    |  1 +
> >  drivers/gpu/drm/i915/intel_hdmi.c  |  1 +
> >  3 files changed, 35 insertions(+), 1 deletion(-)
> > 
> > -- 
> > 2.21.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel