[PATCH v3 0/3] Send a hotplug when edid changes

[PATCH v3 0/3] Send a hotplug when edid changes

[Stanislav Lisovskiy]

This series introduce to drm a way to determine if something else
except connection_status had changed during probing, which
can be used by other drivers as well. Another i915 specific part
uses this approach to determine if edid had changed without
changing the connection status and send a hotplug event.

Stanislav Lisovskiy (3):
  drm: Add helper to compare edids.
  drm: Introduce change counter to drm_connector
  drm/i915: Send hotplug event if edid had changed.

 drivers/gpu/drm/drm_connector.c              |  1 +
 drivers/gpu/drm/drm_edid.c                   | 33 ++++++++++++++++++++
 drivers/gpu/drm/drm_probe_helper.c           | 29 +++++++++++++++--
 drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
 drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++---
 include/drm/drm_connector.h                  |  3 ++
 include/drm/drm_edid.h                       |  9 ++++++
 8 files changed, 117 insertions(+), 11 deletions(-)

-- 
2.17.1

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

[PATCH v3 1/3] drm: Add helper to compare edids.

[Stanislav Lisovskiy]

Many drivers would benefit from using
drm helper to compare edid, rather
than bothering with own implementation.

v2: Added documentation for this function.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  9 +++++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 82a4ceed3fcf..6cd086ea6253 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1362,6 +1362,39 @@ static bool drm_edid_is_zero(const u8 *in_edid, int length)
 	return true;
 }
 
+/**
+ * drm_edid_are_equal - compare two edid blobs.
+ * @edid1: pointer to first blob
+ * @edid2: pointer to second blob
+ * This helper can be used during probing to determine if
+ * edid had changed.
+ */
+bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2)
+{
+	int edid1_len, edid2_len;
+	bool edid1_present = edid1 != NULL;
+	bool edid2_present = edid2 != NULL;
+
+	if (edid1_present != edid2_present)
+		return false;
+
+	if (edid1) {
+
+		edid1_len = EDID_LENGTH * (1 + edid1->extensions);
+		edid2_len = EDID_LENGTH * (1 + edid2->extensions);
+
+		if (edid1_len != edid2_len)
+			return false;
+
+		if (memcmp(edid1, edid2, edid1_len))
+			return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_edid_are_equal);
+
+
 /**
  * drm_edid_block_valid - Sanity check the EDID block (base or extension)
  * @raw_edid: pointer to raw EDID block
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index b9719418c3d2..716964f63312 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -354,6 +354,15 @@ drm_load_edid_firmware(struct drm_connector *connector)
 }
 #endif
 
+/**
+ * drm_edid_are_equal - compare two edid blobs.
+ * @edid1: pointer to first blob
+ * @edid2: pointer to second blob
+ * This helper can be used during probing to determine if
+ * edid had changed.
+ */
+bool drm_edid_are_equal(struct edid *edid1, struct edid *edid2);
+
 int
 drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
 					 struct drm_connector *connector,
-- 
2.17.1

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

[PATCH v3 2/3] drm: Introduce change counter to drm_connector

[Stanislav Lisovskiy]

This counter will be used by drm_helper_probe_detect caller to determine
if something else had changed except connection status,
like for example edid. Hardware specific drivers are responsible
for updating this counter when some change is detected to notify
the drm part, which can trigger for example hotplug event.

Currently there is no way to propagate that to a calling layer,
as we send only connection_status update, however as we see with
edid the changes can be broader.

v2: Added documentation for the new counter. Rename change_counter to
    epoch_counter.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/drm_connector.c    |  1 +
 drivers/gpu/drm/drm_probe_helper.c | 29 +++++++++++++++++++++++++++--
 include/drm/drm_connector.h        |  3 +++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index d49e19f3de3a..9c5e63b31527 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -246,6 +246,7 @@ int drm_connector_init(struct drm_device *dev,
 	INIT_LIST_HEAD(&connector->modes);
 	mutex_init(&connector->mutex);
 	connector->edid_blob_ptr = NULL;
+	connector->epoch_counter = 0;
 	connector->tile_blob_ptr = NULL;
 	connector->status = connector_status_unknown;
 	connector->display_info.panel_orientation =
diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c
index ef2c468205a2..5857053174da 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -776,6 +776,7 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 	struct drm_connector_list_iter conn_iter;
 	enum drm_connector_status old_status;
 	bool changed = false;
+	uint64_t old_epoch_counter;
 
 	if (!dev->mode_config.poll_enabled)
 		return false;
@@ -789,20 +790,44 @@ bool drm_helper_hpd_irq_event(struct drm_device *dev)
 
 		old_status = connector->status;
 
+		old_epoch_counter = connector->epoch_counter;
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Old change counter %llu\n", connector->base.id,
+			      connector->name,
+			      old_epoch_counter);
+
 		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,
 			      connector->name,
 			      drm_get_connector_status_name(old_status),
 			      drm_get_connector_status_name(connector->status));
-		if (old_status != connector->status)
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] New change counter %llu\n",
+			      connector->base.id,
+			      connector->name,
+			      connector->epoch_counter);
+
+		if (old_status != connector->status) {
 			changed = true;
+		}
+
+		/* Check changing of edid when a connector status still remains
+		 * as "connector_status_connected".
+		 */
+		if (connector->status == connector_status_connected &&
+		    old_status == connector_status_connected &&
+		    old_epoch_counter != connector->epoch_counter) {
+			changed = true;
+		}
 	}
 	drm_connector_list_iter_end(&conn_iter);
 	mutex_unlock(&dev->mode_config.mutex);
 
-	if (changed)
+	if (changed) {
 		drm_kms_helper_hotplug_event(dev);
+		DRM_DEBUG_KMS("Sent hotplug event\n");
+	}
 
 	return changed;
 }
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index fc5d08438333..58b24d44de9c 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1282,6 +1282,9 @@ struct drm_connector {
 	/** @override_edid: has the EDID been overwritten through debugfs for testing? */
 	bool override_edid;
 
+	/** @epoch_counter: used to detect any other changes in connector, besides status */
+	uint64_t epoch_counter;
+
 #define DRM_CONNECTOR_MAX_ENCODER 3
 	/**
 	 * @encoder_ids: Valid encoders for this connector. Please only use
-- 
2.17.1

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

[PATCH v3 3/3] drm/i915: Send hotplug event if edid had changed.

[Stanislav Lisovskiy]

Added edid checking to dp and hdmi edid setting functions, which
are called from detect hooks. The result currently is propagated
to calling layer using drm_connector->change_counter(proposed by Daniel Vetter).
drm_helper_hpd_irq_event and intel_encoder_hotplug are currently both
responsible for checking if this counter or connection status is changed.

There are conflicting parts in drm and i915 which attempt
to do the same job - drm_helper_hpd_irq_event attempts to
check connector status changes and then call hotplug,
just as i915_hotplug_work_func, which calls encoder->hotplug
hook which in turn calls generic intel_encoder_hotplug function
which attempts to probe if output has changed.
Looks like both needs to be changed, so added edid checking
also to intel_encoder_hotplug function which is called both
for hdmi and dp.

v2: Renamed change counter to epoch counter. Fixed type name.

v3: Fixed rebase conflict

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105540
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c      | 16 ++++++++++++++-
 drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++++++---
 drivers/gpu/drm/i915/display/intel_hotplug.c | 21 +++++++++++++++-----
 3 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 0eb5d66f87a7..e4eb7e97a556 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5316,10 +5316,24 @@ static void
 intel_dp_set_edid(struct intel_dp *intel_dp)
 {
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct drm_connector *connector = &intel_connector->base;
 	struct edid *edid;
+	struct edid *old_edid;
 
-	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
+	old_edid = intel_connector->detect_edid;
+
+	if (!drm_edid_are_equal(edid, old_edid)) {
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
+		    connector->base.id, connector->name);
+
+		connector->epoch_counter += 1;
+		DRM_DEBUG_KMS("Updating change counter to %llu\n", connector->epoch_counter);
+
+		intel_connector_update_modes(&intel_connector->base, edid);
+	}
+
+	intel_dp_unset_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
 	intel_dp->has_audio = drm_detect_monitor_audio(edid);
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 9bf28de10401..72b3f2135228 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2503,7 +2503,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	intel_wakeref_t wakeref;
-	struct edid *edid;
+	struct edid *edid, *old_edid;
 	bool connected = false;
 	struct i2c_adapter *i2c;
 
@@ -2524,11 +2524,22 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS, wakeref);
 
+	old_edid = to_intel_connector(connector)->detect_edid;
+
+	if (!drm_edid_are_equal(edid, old_edid)) {
+		intel_connector_update_modes(connector, edid);
+		DRM_DEBUG_KMS("Updating change counter to %llu\n", connector->epoch_counter);
+		connector->epoch_counter += 1;
+
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
+		    connector->base.id, connector->name);
+	}
+	intel_hdmi_unset_edid(connector);
 	to_intel_connector(connector)->detect_edid = edid;
+
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
 		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 		intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
-
 		connected = true;
 	}
 
@@ -2555,7 +2566,6 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	    !intel_digital_port_connected(encoder))
 		goto out;
 
-	intel_hdmi_unset_edid(connector);
 
 	if (intel_hdmi_set_edid(connector))
 		status = connector_status_connected;
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c
index c844ae4480af..dda07c9ab2cb 100644
--- a/drivers/gpu/drm/i915/display/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
@@ -280,23 +280,34 @@ intel_encoder_hotplug(struct intel_encoder *encoder,
 {
 	struct drm_device *dev = connector->base.dev;
 	enum drm_connector_status old_status;
+	u64 old_epoch_counter;
+	bool ret = false;
 
 	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
 	old_status = connector->base.status;
 
+	old_epoch_counter = connector->base.epoch_counter;
+
 	connector->base.status =
 		drm_helper_probe_detect(&connector->base, NULL, false);
 
-	if (old_status == connector->base.status)
-		return INTEL_HOTPLUG_UNCHANGED;
+	if (old_status != connector->base.status)
+		ret = true;
+
+	if (old_epoch_counter != connector->base.epoch_counter)
+		ret = true;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n",
+	if (ret) {
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s(change counter %llu)\n",
 		      connector->base.base.id,
 		      connector->base.name,
 		      drm_get_connector_status_name(old_status),
-		      drm_get_connector_status_name(connector->base.status));
+		      drm_get_connector_status_name(connector->base.status),
+		      connector->base.epoch_counter);
+		return INTEL_HOTPLUG_CHANGED;
+	}
 
-	return INTEL_HOTPLUG_CHANGED;
+	return INTEL_HOTPLUG_UNCHANGED;
 }
 
 static bool intel_encoder_has_hpd_pulse(struct intel_encoder *encoder)
-- 
2.17.1

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy wrote:
> This series introduce to drm a way to determine if something else
> except connection_status had changed during probing, which
> can be used by other drivers as well. Another i915 specific part
> uses this approach to determine if edid had changed without
> changing the connection status and send a hotplug event.

Did you read through the entire huge thread on all this (with I think
Paul, Pekka, Ram and some others)? I feel like this is mostly taking that
idea, but not taking a lot of the details we've discussed there.
Specifically I'm not sure how userspace should handle this without also
exposing the epoch counter, or at least generating a hotplug event for the
specific connector. All that and more we discussed there.

And then there's the follow-up question: What's the userspace for this?
Existing expectations are a minefield here, so even if you don't plan to
change userspace we need to know what this is aimed for, and see above I
don't think this is possible to use without userspace changes ...
-Daniel

> 
> Stanislav Lisovskiy (3):
>   drm: Add helper to compare edids.
>   drm: Introduce change counter to drm_connector
>   drm/i915: Send hotplug event if edid had changed.
> 
>  drivers/gpu/drm/drm_connector.c              |  1 +
>  drivers/gpu/drm/drm_edid.c                   | 33 ++++++++++++++++++++
>  drivers/gpu/drm/drm_probe_helper.c           | 29 +++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
>  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++---
>  include/drm/drm_connector.h                  |  3 ++
>  include/drm/drm_edid.h                       |  9 ++++++
>  8 files changed, 117 insertions(+), 11 deletions(-)
> 
> -- 
> 2.17.1
> 

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

✗ Fi.CI.CHECKPATCH: warning for Send a hotplug when edid changes (rev4)

[Patchwork]

== Series Details ==

Series: Send a hotplug when edid changes (rev4)
URL   : https://patchwork.freedesktop.org/series/62816/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
36f82de41a75 drm: Add helper to compare edids.
-:32: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "edid1"
#32: FILE: drivers/gpu/drm/drm_edid.c:1375:
+	bool edid1_present = edid1 != NULL;

-:33: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "edid2"
#33: FILE: drivers/gpu/drm/drm_edid.c:1376:
+	bool edid2_present = edid2 != NULL;

-:39: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#39: FILE: drivers/gpu/drm/drm_edid.c:1382:
+	if (edid1) {
+

-:54: CHECK:LINE_SPACING: Please don't use multiple blank lines
#54: FILE: drivers/gpu/drm/drm_edid.c:1397:
+
+

total: 0 errors, 0 warnings, 4 checks, 54 lines checked
489410ace4ab drm: Introduce change counter to drm_connector
-:42: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u64' over 'uint64_t'
#42: FILE: drivers/gpu/drm/drm_probe_helper.c:779:
+	uint64_t old_epoch_counter;

-:69: WARNING:BRACES: braces {} are not necessary for single statement blocks
#69: FILE: drivers/gpu/drm/drm_probe_helper.c:811:
+		if (old_status != connector->status) {
 			changed = true;
+		}

-:102: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u64' over 'uint64_t'
#102: FILE: include/drm/drm_connector.h:1286:
+	uint64_t epoch_counter;

total: 0 errors, 1 warnings, 2 checks, 69 lines checked
96448743d614 drm/i915: Send hotplug event if edid had changed.
-:8: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#8: 
to calling layer using drm_connector->change_counter(proposed by Daniel Vetter).

-:47: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#47: FILE: drivers/gpu/drm/i915/display/intel_dp.c:5328:
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
+		    connector->base.id, connector->name);

-:84: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#84: FILE: drivers/gpu/drm/i915/display/intel_hdmi.c:2535:
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Edid was changed! Updating blob property.\n",
+		    connector->base.id, connector->name);

-:134: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#134: FILE: drivers/gpu/drm/i915/display/intel_hotplug.c:302:
+		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s(change counter %llu)\n",
 		      connector->base.base.id,

total: 0 errors, 1 warnings, 3 checks, 102 lines checked

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy wrote:
> > This series introduce to drm a way to determine if something else
> > except connection_status had changed during probing, which
> > can be used by other drivers as well. Another i915 specific part
> > uses this approach to determine if edid had changed without
> > changing the connection status and send a hotplug event.
> 
> Did you read through the entire huge thread on all this (with I think
> Paul, Pekka, Ram and some others)? I feel like this is mostly taking
> that
> idea, but not taking a lot of the details we've discussed there.
> Specifically I'm not sure how userspace should handle this without
> also
> exposing the epoch counter, or at least generating a hotplug event
> for the
> specific connector. All that and more we discussed there.
> 
> And then there's the follow-up question: What's the userspace for
> this?
> Existing expectations are a minefield here, so even if you don't plan
> to
> change userspace we need to know what this is aimed for, and see
> above I
> don't think this is possible to use without userspace changes ...

Yes, sure I agree about userspace. However I guess we must start from
something at least.
I think I have seen some discussion regarding exposing this epoch
counter as a property. Was even implementing this at some point but
didn't dare to send to mailing list :)

My idea was just to expose this epoch counter as a drm property, to let
userspace then to figure out, what has happened, as imho adding
different events for this and that is a bit of an overkill and brings
additional complexity..

However, please let me know, what do you think we should do for
userspace.


-Stanislav


> -Daniel
> 
> > 
> > Stanislav Lisovskiy (3):
> >   drm: Add helper to compare edids.
> >   drm: Introduce change counter to drm_connector
> >   drm/i915: Send hotplug event if edid had changed.
> > 
> >  drivers/gpu/drm/drm_connector.c              |  1 +
> >  drivers/gpu/drm/drm_edid.c                   | 33
> > ++++++++++++++++++++
> >  drivers/gpu/drm/drm_probe_helper.c           | 29 +++++++++++++++-
> > -
> >  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
> >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
> >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++---
> >  include/drm/drm_connector.h                  |  3 ++
> >  include/drm/drm_edid.h                       |  9 ++++++
> >  8 files changed, 117 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.17.1
> > 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✓ Fi.CI.BAT: success for Send a hotplug when edid changes (rev4)

[Patchwork]

== Series Details ==

Series: Send a hotplug when edid changes (rev4)
URL   : https://patchwork.freedesktop.org/series/62816/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6639 -> Patchwork_13889
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_busy@basic-flip-a:
    - fi-kbl-7567u:       [PASS][1] -> [SKIP][2] ([fdo#109271] / [fdo#109278]) +2 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-kbl-7567u/igt@kms_busy@basic-flip-a.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [PASS][3] -> [FAIL][4] ([fdo#109483])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-icl-u3:          [PASS][5] -> [DMESG-WARN][6] ([fdo#107724]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-icl-u3/igt@prime_vgem@basic-fence-flip.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-icl-u3/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-blb-e6850:       [INCOMPLETE][7] ([fdo#107718]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-blb-e6850/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@kms_busy@basic-flip-c:
    - fi-kbl-7500u:       [SKIP][9] ([fdo#109271] / [fdo#109278]) -> [PASS][10] +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-kbl-7500u/igt@kms_busy@basic-flip-c.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - {fi-icl-u4}:        [FAIL][11] ([fdo#111045] / [fdo#111046 ]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-icl-u4/igt@kms_chamelium@hdmi-crc-fast.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-icl-u4/igt@kms_chamelium@hdmi-crc-fast.html

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

  * igt@prime_vgem@basic-fence-flip:
    - fi-kbl-7500u:       [SKIP][15] ([fdo#109271]) -> [PASS][16] +23 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-kbl-7500u/igt@prime_vgem@basic-fence-flip.html

  * igt@vgem_basic@sysfs:
    - fi-icl-u3:          [DMESG-WARN][17] ([fdo#107724]) -> [PASS][18] +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/fi-icl-u3/igt@vgem_basic@sysfs.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/fi-icl-u3/igt@vgem_basic@sysfs.html

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

  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109483]: https://bugs.freedesktop.org/show_bug.cgi?id=109483
  [fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
  [fdo#111045]: https://bugs.freedesktop.org/show_bug.cgi?id=111045
  [fdo#111046 ]: https://bugs.freedesktop.org/show_bug.cgi?id=111046 


Participating hosts (54 -> 46)
------------------------------

  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-cfl-guc fi-icl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6639 -> Patchwork_13889

  CI-20190529: 20190529
  CI_DRM_6639: 474a7391a134134b2ddba7c7e89fb3bfa7b5a068 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5121: 242cb6f2149cb9699ba9b316e5f60b756260e829 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13889: 96448743d61441f0bec684bb45bf05c46efe97e7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

96448743d614 drm/i915: Send hotplug event if edid had changed.
489410ace4ab drm: Introduce change counter to drm_connector
36f82de41a75 drm: Add helper to compare edids.

== Logs ==

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
<stanislav.lisovskiy@intel.com> wrote:
>
> On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> > On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy wrote:
> > > This series introduce to drm a way to determine if something else
> > > except connection_status had changed during probing, which
> > > can be used by other drivers as well. Another i915 specific part
> > > uses this approach to determine if edid had changed without
> > > changing the connection status and send a hotplug event.
> >
> > Did you read through the entire huge thread on all this (with I think
> > Paul, Pekka, Ram and some others)? I feel like this is mostly taking
> > that
> > idea, but not taking a lot of the details we've discussed there.
> > Specifically I'm not sure how userspace should handle this without
> > also
> > exposing the epoch counter, or at least generating a hotplug event
> > for the
> > specific connector. All that and more we discussed there.
> >
> > And then there's the follow-up question: What's the userspace for
> > this?
> > Existing expectations are a minefield here, so even if you don't plan
> > to
> > change userspace we need to know what this is aimed for, and see
> > above I
> > don't think this is possible to use without userspace changes ...
>
> Yes, sure I agree about userspace. However I guess we must start from
> something at least.
> I think I have seen some discussion regarding exposing this epoch
> counter as a property. Was even implementing this at some point but
> didn't dare to send to mailing list :)
>
> My idea was just to expose this epoch counter as a drm property, to let
> userspace then to figure out, what has happened, as imho adding
> different events for this and that is a bit of an overkill and brings
> additional complexity..

Adding Ram and link to the (warning, huge!) thread:

https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9

> However, please let me know, what do you think we should do for
> userspace.

That seems backwards, since this is an uapi change I'd expect you're
solving some specific issue in some specific userspace? If we're doing
this just because I'm not really following.

Cheers, Daniel

>
>
> -Stanislav
>
>
> > -Daniel
> >
> > >
> > > Stanislav Lisovskiy (3):
> > >   drm: Add helper to compare edids.
> > >   drm: Introduce change counter to drm_connector
> > >   drm/i915: Send hotplug event if edid had changed.
> > >
> > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > ++++++++++++++++++++
> > >  drivers/gpu/drm/drm_probe_helper.c           | 29 +++++++++++++++-
> > > -
> > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
> > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
> > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++---
> > >  include/drm/drm_connector.h                  |  3 ++
> > >  include/drm/drm_edid.h                       |  9 ++++++
> > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
> >
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for Send a hotplug when edid changes (rev4)

[Patchwork]

== Series Details ==

Series: Send a hotplug when edid changes (rev4)
URL   : https://patchwork.freedesktop.org/series/62816/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6639_full -> Patchwork_13889_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_softpin@noreloc-s3:
    - shard-apl:          [PASS][1] -> [DMESG-WARN][2] ([fdo#108566]) +4 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-apl8/igt@gem_softpin@noreloc-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-apl4/igt@gem_softpin@noreloc-s3.html

  * igt@i915_suspend@sysfs-reader:
    - shard-kbl:          [PASS][3] -> [DMESG-WARN][4] ([fdo#108566]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-kbl6/igt@i915_suspend@sysfs-reader.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-kbl3/igt@i915_suspend@sysfs-reader.html

  * igt@kms_cursor_crc@pipe-a-cursor-suspend:
    - shard-skl:          [PASS][5] -> [INCOMPLETE][6] ([fdo#110741])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-skl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-skl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html

  * igt@kms_cursor_crc@pipe-b-cursor-128x42-random:
    - shard-skl:          [PASS][7] -> [FAIL][8] ([fdo#103232])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-skl4/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-skl10/igt@kms_cursor_crc@pipe-b-cursor-128x42-random.html

  * igt@kms_flip@2x-plain-flip-ts-check:
    - shard-glk:          [PASS][9] -> [FAIL][10] ([fdo#100368])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-glk2/igt@kms_flip@2x-plain-flip-ts-check.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-glk1/igt@kms_flip@2x-plain-flip-ts-check.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103167]) +4 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb4/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-iclb:         [PASS][13] -> [INCOMPLETE][14] ([fdo#107713] / [fdo#110036 ])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb1/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb7/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([fdo#108145] / [fdo#110403])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-skl4/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-skl10/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [PASS][17] -> [SKIP][18] ([fdo#109441]) +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb4/igt@kms_psr@psr2_cursor_blt.html

  * igt@kms_rotation_crc@primary-rotation-180:
    - shard-apl:          [PASS][19] -> [INCOMPLETE][20] ([fdo#103927]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-apl8/igt@kms_rotation_crc@primary-rotation-180.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-apl8/igt@kms_rotation_crc@primary-rotation-180.html

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-skl:          [PASS][21] -> [INCOMPLETE][22] ([fdo#104108])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-skl8/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-skl7/igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend.html

  
#### Possible fixes ####

  * igt@kms_flip@flip-vs-dpms-off-vs-modeset:
    - shard-hsw:          [INCOMPLETE][23] ([fdo#103540]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-hsw8/igt@kms_flip@flip-vs-dpms-off-vs-modeset.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-hsw8/igt@kms_flip@flip-vs-dpms-off-vs-modeset.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-kbl:          [DMESG-WARN][25] ([fdo#108566]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-kbl4/igt@kms_flip@flip-vs-suspend-interruptible.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-kbl7/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-iclb:         [FAIL][27] ([fdo#103167]) -> [PASS][28] +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb7/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb5/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-pgflip-blt:
    - shard-iclb:         [INCOMPLETE][29] ([fdo#106978] / [fdo#107713]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb7/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-pgflip-blt.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb3/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-pgflip-blt.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-kbl:          [INCOMPLETE][31] ([fdo#103665]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-kbl3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
    - shard-apl:          [DMESG-WARN][33] ([fdo#108566]) -> [PASS][34] +2 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-apl5/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-apl7/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [FAIL][35] ([fdo#103166]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-iclb4/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@tools_test@tools_test:
    - shard-skl:          [SKIP][37] ([fdo#109271]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6639/shard-skl2/igt@tools_test@tools_test.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13889/shard-skl1/igt@tools_test@tools_test.html

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

  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [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#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#110036 ]: https://bugs.freedesktop.org/show_bug.cgi?id=110036 
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110741]: https://bugs.freedesktop.org/show_bug.cgi?id=110741


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

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_6639 -> Patchwork_13889

  CI-20190529: 20190529
  CI_DRM_6639: 474a7391a134134b2ddba7c7e89fb3bfa7b5a068 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5121: 242cb6f2149cb9699ba9b316e5f60b756260e829 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13889: 96448743d61441f0bec684bb45bf05c46efe97e7 @ 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_13889/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote:
> On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
> <stanislav.lisovskiy@intel.com> wrote:
> > 
> > On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> > > On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy
> > > wrote:
> > > > This series introduce to drm a way to determine if something
> > > > else
> > > > except connection_status had changed during probing, which
> > > > can be used by other drivers as well. Another i915 specific
> > > > part
> > > > uses this approach to determine if edid had changed without
> > > > changing the connection status and send a hotplug event.
> > > 
> > > Did you read through the entire huge thread on all this (with I
> > > think
> > > Paul, Pekka, Ram and some others)? I feel like this is mostly
> > > taking
> > > that
> > > idea, but not taking a lot of the details we've discussed there.
> > > Specifically I'm not sure how userspace should handle this
> > > without
> > > also
> > > exposing the epoch counter, or at least generating a hotplug
> > > event
> > > for the
> > > specific connector. All that and more we discussed there.
> > > 
> > > And then there's the follow-up question: What's the userspace for
> > > this?
> > > Existing expectations are a minefield here, so even if you don't
> > > plan
> > > to
> > > change userspace we need to know what this is aimed for, and see
> > > above I
> > > don't think this is possible to use without userspace changes ...
> > 
> > Yes, sure I agree about userspace. However I guess we must start
> > from
> > something at least.
> > I think I have seen some discussion regarding exposing this epoch
> > counter as a property. Was even implementing this at some point but
> > didn't dare to send to mailing list :)
> > 
> > My idea was just to expose this epoch counter as a drm property, to
> > let
> > userspace then to figure out, what has happened, as imho adding
> > different events for this and that is a bit of an overkill and
> > brings
> > additional complexity..
> 
> Adding Ram and link to the (warning, huge!) thread:
> 
> https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9
> 
> > However, please let me know, what do you think we should do for
> > userspace.
> 
> That seems backwards, since this is an uapi change I'd expect you're
> solving some specific issue in some specific userspace? If we're
> doing
> this just because I'm not really following.

Specifically, I'm solving an issue of changed edid during suspend, like
we for example have in kms_chamelium hotplug tests(some of which now
fail because of that). 
So even if connection status hasn't changed, we still need to send
hotplug event and userspace needs to be able to understand that
something had changed and whether we need to do a full reprobe or not.
Epoch counter property would be responsible for this.
As I understand in general there might be other connector updates,
except edid which we need propagate in a similar way.

-Stanislav

> 
> Cheers, Daniel
> 
> > 
> > 
> > -Stanislav
> > 
> > 
> > > -Daniel
> > > 
> > > > 
> > > > Stanislav Lisovskiy (3):
> > > >   drm: Add helper to compare edids.
> > > >   drm: Introduce change counter to drm_connector
> > > >   drm/i915: Send hotplug event if edid had changed.
> > > > 
> > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > ++++++++++++++++++++
> > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > +++++++++++++++-
> > > > -
> > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
> > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
> > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++
> > > > ---
> > > >  include/drm/drm_connector.h                  |  3 ++
> > > >  include/drm/drm_edid.h                       |  9 ++++++
> > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > 
> > > > --
> > > > 2.17.1
> > > > 
> > > 
> > > 
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Wed, Aug 07, 2019 at 07:43:18AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote:
> > On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
> > <stanislav.lisovskiy@intel.com> wrote:
> > > 
> > > On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> > > > On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy
> > > > wrote:
> > > > > This series introduce to drm a way to determine if something
> > > > > else
> > > > > except connection_status had changed during probing, which
> > > > > can be used by other drivers as well. Another i915 specific
> > > > > part
> > > > > uses this approach to determine if edid had changed without
> > > > > changing the connection status and send a hotplug event.
> > > > 
> > > > Did you read through the entire huge thread on all this (with I
> > > > think
> > > > Paul, Pekka, Ram and some others)? I feel like this is mostly
> > > > taking
> > > > that
> > > > idea, but not taking a lot of the details we've discussed there.
> > > > Specifically I'm not sure how userspace should handle this
> > > > without
> > > > also
> > > > exposing the epoch counter, or at least generating a hotplug
> > > > event
> > > > for the
> > > > specific connector. All that and more we discussed there.
> > > > 
> > > > And then there's the follow-up question: What's the userspace for
> > > > this?
> > > > Existing expectations are a minefield here, so even if you don't
> > > > plan
> > > > to
> > > > change userspace we need to know what this is aimed for, and see
> > > > above I
> > > > don't think this is possible to use without userspace changes ...
> > > 
> > > Yes, sure I agree about userspace. However I guess we must start
> > > from
> > > something at least.
> > > I think I have seen some discussion regarding exposing this epoch
> > > counter as a property. Was even implementing this at some point but
> > > didn't dare to send to mailing list :)
> > > 
> > > My idea was just to expose this epoch counter as a drm property, to
> > > let
> > > userspace then to figure out, what has happened, as imho adding
> > > different events for this and that is a bit of an overkill and
> > > brings
> > > additional complexity..
> > 
> > Adding Ram and link to the (warning, huge!) thread:
> > 
> > https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9
> > 
> > > However, please let me know, what do you think we should do for
> > > userspace.
> > 
> > That seems backwards, since this is an uapi change I'd expect you're
> > solving some specific issue in some specific userspace? If we're
> > doing
> > this just because I'm not really following.
> 
> Specifically, I'm solving an issue of changed edid during suspend, like
> we for example have in kms_chamelium hotplug tests(some of which now
> fail because of that). 
> So even if connection status hasn't changed, we still need to send
> hotplug event and userspace needs to be able to understand that
> something had changed and whether we need to do a full reprobe or not.
> Epoch counter property would be responsible for this.
> As I understand in general there might be other connector updates,
> except edid which we need propagate in a similar way.

So igt isn't valid userspace (it's just good testcases). Can we repro the
same on real userspace? Does this work with real userspace? We've had
userspace which tries to be clever and filters out what looks like
redundant hotplug events. And then gets it wrong in cases like this.

Also, we've had forever an unconditional uevent on resume, exactly because
anything could have changed. Did we loose this one on the way somewhere?
Or maybe I misremember ...

If all we care about is resume re-adding that uncondtional uevent on
resume is going to be a lot easier than this here.
-Daniel


> 
> -Stanislav
> 
> > 
> > Cheers, Daniel
> > 
> > > 
> > > 
> > > -Stanislav
> > > 
> > > 
> > > > -Daniel
> > > > 
> > > > > 
> > > > > Stanislav Lisovskiy (3):
> > > > >   drm: Add helper to compare edids.
> > > > >   drm: Introduce change counter to drm_connector
> > > > >   drm/i915: Send hotplug event if edid had changed.
> > > > > 
> > > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > > ++++++++++++++++++++
> > > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > > +++++++++++++++-
> > > > > -
> > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16 +++++++++-
> > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16 ++++++++--
> > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21 ++++++++++
> > > > > ---
> > > > >  include/drm/drm_connector.h                  |  3 ++
> > > > >  include/drm/drm_edid.h                       |  9 ++++++
> > > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > > 
> > > > > --
> > > > > 2.17.1
> > > > > 
> > > > 
> > > > 
> > 
> > 
> > 

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> On Wed, Aug 07, 2019 at 07:43:18AM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote:
> > > On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
> > > <stanislav.lisovskiy@intel.com> wrote:
> > > > 
> > > > On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
> > > > > On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy
> > > > > wrote:
> > > > > > This series introduce to drm a way to determine if
> > > > > > something
> > > > > > else
> > > > > > except connection_status had changed during probing, which
> > > > > > can be used by other drivers as well. Another i915 specific
> > > > > > part
> > > > > > uses this approach to determine if edid had changed without
> > > > > > changing the connection status and send a hotplug event.
> > > > > 
> > > > > Did you read through the entire huge thread on all this (with
> > > > > I
> > > > > think
> > > > > Paul, Pekka, Ram and some others)? I feel like this is mostly
> > > > > taking
> > > > > that
> > > > > idea, but not taking a lot of the details we've discussed
> > > > > there.
> > > > > Specifically I'm not sure how userspace should handle this
> > > > > without
> > > > > also
> > > > > exposing the epoch counter, or at least generating a hotplug
> > > > > event
> > > > > for the
> > > > > specific connector. All that and more we discussed there.
> > > > > 
> > > > > And then there's the follow-up question: What's the userspace
> > > > > for
> > > > > this?
> > > > > Existing expectations are a minefield here, so even if you
> > > > > don't
> > > > > plan
> > > > > to
> > > > > change userspace we need to know what this is aimed for, and
> > > > > see
> > > > > above I
> > > > > don't think this is possible to use without userspace changes
> > > > > ...
> > > > 
> > > > Yes, sure I agree about userspace. However I guess we must
> > > > start
> > > > from
> > > > something at least.
> > > > I think I have seen some discussion regarding exposing this
> > > > epoch
> > > > counter as a property. Was even implementing this at some point
> > > > but
> > > > didn't dare to send to mailing list :)
> > > > 
> > > > My idea was just to expose this epoch counter as a drm
> > > > property, to
> > > > let
> > > > userspace then to figure out, what has happened, as imho adding
> > > > different events for this and that is a bit of an overkill and
> > > > brings
> > > > additional complexity..
> > > 
> > > Adding Ram and link to the (warning, huge!) thread:
> > > 
> > > 
https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9
> > > 
> > > > However, please let me know, what do you think we should do for
> > > > userspace.
> > > 
> > > That seems backwards, since this is an uapi change I'd expect
> > > you're
> > > solving some specific issue in some specific userspace? If we're
> > > doing
> > > this just because I'm not really following.
> > 
> > Specifically, I'm solving an issue of changed edid during suspend,
> > like
> > we for example have in kms_chamelium hotplug tests(some of which
> > now
> > fail because of that). 
> > So even if connection status hasn't changed, we still need to send
> > hotplug event and userspace needs to be able to understand that
> > something had changed and whether we need to do a full reprobe or
> > not.
> > Epoch counter property would be responsible for this.
> > As I understand in general there might be other connector updates,
> > except edid which we need propagate in a similar way.
> 
> So igt isn't valid userspace (it's just good testcases). Can we repro
> the
> same on real userspace? Does this work with real userspace? We've had
> userspace which tries to be clever and filters out what looks like
> redundant hotplug events. And then gets it wrong in cases like this.
> 
> Also, we've had forever an unconditional uevent on resume, exactly
> because
> anything could have changed. Did we loose this one on the way
> somewhere?
> Or maybe I misremember ...
> 
> If all we care about is resume re-adding that uncondtional uevent on
> resume is going to be a lot easier than this here.
> -Daniel

Sorry for long reply(was on vacation), that is a good question
regarding reproducing this in real life scenario. My obvious guess
was to suspend the machine and meanwhile change connected display to
another one. However this situation seems to be already handled by
kernel nicely(tried few times and we always get a hotplug event). So
that edid change during suspend chamelium test case seems to be
a bit different. I will talk to our guys who wrote this about what is
the real life scenario for this, because I'm now curious as well.

- Stanislav

> 
> 
> > 
> > -Stanislav
> > 
> > > 
> > > Cheers, Daniel
> > > 
> > > > 
> > > > 
> > > > -Stanislav
> > > > 
> > > > 
> > > > > -Daniel
> > > > > 
> > > > > > 
> > > > > > Stanislav Lisovskiy (3):
> > > > > >   drm: Add helper to compare edids.
> > > > > >   drm: Introduce change counter to drm_connector
> > > > > >   drm/i915: Send hotplug event if edid had changed.
> > > > > > 
> > > > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > > > ++++++++++++++++++++
> > > > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > > > +++++++++++++++-
> > > > > > -
> > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16
> > > > > > +++++++++-
> > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16
> > > > > > ++++++++--
> > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21
> > > > > > ++++++++++
> > > > > > ---
> > > > > >  include/drm/drm_connector.h                  |  3 ++
> > > > > >  include/drm/drm_edid.h                       |  9 ++++++
> > > > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > > > 
> > > > > > --
> > > > > > 2.17.1
> > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Peres, Martin]

On 19/08/2019 10:23, Lisovskiy, Stanislav wrote:
> On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
>> On Wed, Aug 07, 2019 at 07:43:18AM +0000, Lisovskiy, Stanislav wrote:
>>> On Tue, 2019-08-06 at 19:43 +0200, Daniel Vetter wrote:
>>>> On Tue, Aug 6, 2019 at 4:06 PM Lisovskiy, Stanislav
>>>> <stanislav.lisovskiy@intel.com> wrote:
>>>>>
>>>>> On Tue, 2019-08-06 at 15:51 +0200, Daniel Vetter wrote:
>>>>>> On Tue, Aug 06, 2019 at 03:55:48PM +0300, Stanislav Lisovskiy
>>>>>> wrote:
>>>>>>> This series introduce to drm a way to determine if
>>>>>>> something
>>>>>>> else
>>>>>>> except connection_status had changed during probing, which
>>>>>>> can be used by other drivers as well. Another i915 specific
>>>>>>> part
>>>>>>> uses this approach to determine if edid had changed without
>>>>>>> changing the connection status and send a hotplug event.
>>>>>>
>>>>>> Did you read through the entire huge thread on all this (with
>>>>>> I
>>>>>> think
>>>>>> Paul, Pekka, Ram and some others)? I feel like this is mostly
>>>>>> taking
>>>>>> that
>>>>>> idea, but not taking a lot of the details we've discussed
>>>>>> there.
>>>>>> Specifically I'm not sure how userspace should handle this
>>>>>> without
>>>>>> also
>>>>>> exposing the epoch counter, or at least generating a hotplug
>>>>>> event
>>>>>> for the
>>>>>> specific connector. All that and more we discussed there.
>>>>>>
>>>>>> And then there's the follow-up question: What's the userspace
>>>>>> for
>>>>>> this?
>>>>>> Existing expectations are a minefield here, so even if you
>>>>>> don't
>>>>>> plan
>>>>>> to
>>>>>> change userspace we need to know what this is aimed for, and
>>>>>> see
>>>>>> above I
>>>>>> don't think this is possible to use without userspace changes
>>>>>> ...
>>>>>
>>>>> Yes, sure I agree about userspace. However I guess we must
>>>>> start
>>>>> from
>>>>> something at least.
>>>>> I think I have seen some discussion regarding exposing this
>>>>> epoch
>>>>> counter as a property. Was even implementing this at some point
>>>>> but
>>>>> didn't dare to send to mailing list :)
>>>>>
>>>>> My idea was just to expose this epoch counter as a drm
>>>>> property, to
>>>>> let
>>>>> userspace then to figure out, what has happened, as imho adding
>>>>> different events for this and that is a bit of an overkill and
>>>>> brings
>>>>> additional complexity..
>>>>
>>>> Adding Ram and link to the (warning, huge!) thread:
>>>>
>>>>
> https://patchwork.freedesktop.org/patch/303905/?series=57232&rev=9
>>>>
>>>>> However, please let me know, what do you think we should do for
>>>>> userspace.
>>>>
>>>> That seems backwards, since this is an uapi change I'd expect
>>>> you're
>>>> solving some specific issue in some specific userspace? If we're
>>>> doing
>>>> this just because I'm not really following.
>>>
>>> Specifically, I'm solving an issue of changed edid during suspend,
>>> like
>>> we for example have in kms_chamelium hotplug tests(some of which
>>> now
>>> fail because of that). 
>>> So even if connection status hasn't changed, we still need to send
>>> hotplug event and userspace needs to be able to understand that
>>> something had changed and whether we need to do a full reprobe or
>>> not.
>>> Epoch counter property would be responsible for this.
>>> As I understand in general there might be other connector updates,
>>> except edid which we need propagate in a similar way.
>>
>> So igt isn't valid userspace (it's just good testcases). Can we repro
>> the
>> same on real userspace? Does this work with real userspace? We've had
>> userspace which tries to be clever and filters out what looks like
>> redundant hotplug events. And then gets it wrong in cases like this.
>>
>> Also, we've had forever an unconditional uevent on resume, exactly
>> because
>> anything could have changed. Did we loose this one on the way
>> somewhere?
>> Or maybe I misremember ...
>>
>> If all we care about is resume re-adding that uncondtional uevent on
>> resume is going to be a lot easier than this here.
>> -Daniel
> 
> Sorry for long reply(was on vacation), that is a good question
> regarding reproducing this in real life scenario. My obvious guess
> was to suspend the machine and meanwhile change connected display to
> another one. However this situation seems to be already handled by
> kernel nicely(tried few times and we always get a hotplug event). So
> that edid change during suspend chamelium test case seems to be
> a bit different. I will talk to our guys who wrote this about what is
> the real life scenario for this, because I'm now curious as well.

Thanks Daniel for the feedback.

I also now wonder why our IGT test (chamelium-based) does not pass if a
uevent is sent on resume automatically and all the test is expecting is
a uevent...

Martin

> 
> - Stanislav
> 
>>
>>
>>>
>>> -Stanislav
>>>
>>>>
>>>> Cheers, Daniel
>>>>
>>>>>
>>>>>
>>>>> -Stanislav
>>>>>
>>>>>
>>>>>> -Daniel
>>>>>>
>>>>>>>
>>>>>>> Stanislav Lisovskiy (3):
>>>>>>>   drm: Add helper to compare edids.
>>>>>>>   drm: Introduce change counter to drm_connector
>>>>>>>   drm/i915: Send hotplug event if edid had changed.
>>>>>>>
>>>>>>>  drivers/gpu/drm/drm_connector.c              |  1 +
>>>>>>>  drivers/gpu/drm/drm_edid.c                   | 33
>>>>>>> ++++++++++++++++++++
>>>>>>>  drivers/gpu/drm/drm_probe_helper.c           | 29
>>>>>>> +++++++++++++++-
>>>>>>> -
>>>>>>>  drivers/gpu/drm/i915/display/intel_dp.c      | 16
>>>>>>> +++++++++-
>>>>>>>  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16
>>>>>>> ++++++++--
>>>>>>>  drivers/gpu/drm/i915/display/intel_hotplug.c | 21
>>>>>>> ++++++++++
>>>>>>> ---
>>>>>>>  include/drm/drm_connector.h                  |  3 ++
>>>>>>>  include/drm/drm_edid.h                       |  9 ++++++
>>>>>>>  8 files changed, 117 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> --
>>>>>>> 2.17.1
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>
> 

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote:
> On 19/08/2019 10:23, Lisovskiy, Stanislav wrote:
> > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> > > 
> > > 
> > > So igt isn't valid userspace (it's just good testcases). Can we
> > > repro
> > > the
> > > same on real userspace? Does this work with real userspace? We've
> > > had
> > > userspace which tries to be clever and filters out what looks
> > > like
> > > redundant hotplug events. And then gets it wrong in cases like
> > > this.
> > > 
> > > Also, we've had forever an unconditional uevent on resume,
> > > exactly
> > > because
> > > anything could have changed. Did we loose this one on the way
> > > somewhere?
> > > Or maybe I misremember ...
> > > 
> > > If all we care about is resume re-adding that uncondtional uevent
> > > on
> > > resume is going to be a lot easier than this here.
> > > -Daniel
> > 
> > Sorry for long reply(was on vacation), that is a good question
> > regarding reproducing this in real life scenario. My obvious guess
> > was to suspend the machine and meanwhile change connected display
> > to
> > another one. However this situation seems to be already handled by
> > kernel nicely(tried few times and we always get a hotplug event).
> > So
> > that edid change during suspend chamelium test case seems to be
> > a bit different. I will talk to our guys who wrote this about what
> > is
> > the real life scenario for this, because I'm now curious as well.
> 
> Thanks Daniel for the feedback.
> 
> I also now wonder why our IGT test (chamelium-based) does not pass if
> a
> uevent is sent on resume automatically and all the test is expecting
> is
> a uevent...
> 
> Martin

In fact I was wrong - when it worked, it was using exactly those
patches :). With clean drm-tip - it seems to work ocassionally and it
doesn't update the actual display edid and other stuff, so even when
displays are changed we still see the old info/edid from userspace.

We always get a hpd irq when suspend/resume however it doesn't always
result in uevent being sent. So there is a real need in those patches.

> 
> > 
> > - Stanislav
> > 
> > > 
> > > 
> > > > 
> > > > -Stanislav
> > > > 
> > > > > 
> > > > > Cheers, Daniel
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > -Stanislav
> > > > > > 
> > > > > > 
> > > > > > > -Daniel
> > > > > > > 
> > > > > > > > 
> > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > >   drm: Add helper to compare edids.
> > > > > > > >   drm: Introduce change counter to drm_connector
> > > > > > > >   drm/i915: Send hotplug event if edid had changed.
> > > > > > > > 
> > > > > > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > > > > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > > > > > ++++++++++++++++++++
> > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > > > > > +++++++++++++++-
> > > > > > > > -
> > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16
> > > > > > > > +++++++++-
> > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16
> > > > > > > > ++++++++--
> > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21
> > > > > > > > ++++++++++
> > > > > > > > ---
> > > > > > > >  include/drm/drm_connector.h                  |  3 ++
> > > > > > > >  include/drm/drm_edid.h                       |  9
> > > > > > > > ++++++
> > > > > > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > > > > > 
> > > > > > > > --
> > > > > > > > 2.17.1
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Mon, 2019-08-19 at 09:05 +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote:
> > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote:
> > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> > > > 
> > > > 
> > > > So igt isn't valid userspace (it's just good testcases). Can we
> > > > repro
> > > > the
> > > > same on real userspace? Does this work with real userspace?
> > > > We've
> > > > had
> > > > userspace which tries to be clever and filters out what looks
> > > > like
> > > > redundant hotplug events. And then gets it wrong in cases like
> > > > this.
> > > > 
> > > > Also, we've had forever an unconditional uevent on resume,
> > > > exactly
> > > > because
> > > > anything could have changed. Did we loose this one on the way
> > > > somewhere?
> > > > Or maybe I misremember ...
> > > > 
> > > > If all we care about is resume re-adding that uncondtional
> > > > uevent
> > > > on
> > > > resume is going to be a lot easier than this here.
> > > > -Daniel
> > > 
> > > Sorry for long reply(was on vacation), that is a good question
> > > regarding reproducing this in real life scenario. My obvious
> > > guess
> > > was to suspend the machine and meanwhile change connected display
> > > to
> > > another one. However this situation seems to be already handled
> > > by
> > > kernel nicely(tried few times and we always get a hotplug event).
> > > So
> > > that edid change during suspend chamelium test case seems to be
> > > a bit different. I will talk to our guys who wrote this about
> > > what
> > > is
> > > the real life scenario for this, because I'm now curious as well.
> > 
> > Thanks Daniel for the feedback.
> > 
> > I also now wonder why our IGT test (chamelium-based) does not pass
> > if
> > a
> > uevent is sent on resume automatically and all the test is
> > expecting
> > is
> > a uevent...
> > 
> > Martin
> 
> In fact I was wrong - when it worked, it was using exactly those
> patches :). With clean drm-tip - it seems to work ocassionally and it
> doesn't update the actual display edid and other stuff, so even when
> displays are changed we still see the old info/edid from userspace.
> 
> We always get a hpd irq when suspend/resume however it doesn't always
> result in uevent being sent. So there is a real need in those
> patches.
> 

Just decided to "ping" this discussion again. The issue is already some
years old and still nothing is fixed. I do agree that may be something
needs to be fixed/changed here in those patches, but something must be
agreed at least I guess, as discussions themself do not fix bugs.
Currently those patches address a particular issue which occurs, if
display is changed during suspend. 
On ocassional basis, userspace might not get a hotplug event at all,
causing different kind of problems(like wrong mode set on display or
diaply not working at all). Also some kms_chamelium hotplug tests fail
because of that. 

> > 
> > > 
> > > - Stanislav
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > -Stanislav
> > > > > 
> > > > > > 
> > > > > > Cheers, Daniel
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > -Stanislav
> > > > > > > 
> > > > > > > 
> > > > > > > > -Daniel
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > >   drm: Introduce change counter to drm_connector
> > > > > > > > >   drm/i915: Send hotplug event if edid had changed.
> > > > > > > > > 
> > > > > > > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > > > > > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > > > > > > ++++++++++++++++++++
> > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > > > > > > +++++++++++++++-
> > > > > > > > > -
> > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16
> > > > > > > > > +++++++++-
> > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16
> > > > > > > > > ++++++++--
> > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21
> > > > > > > > > ++++++++++
> > > > > > > > > ---
> > > > > > > > >  include/drm/drm_connector.h                  |  3 ++
> > > > > > > > >  include/drm/drm_edid.h                       |  9
> > > > > > > > > ++++++
> > > > > > > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > > > > > > 
> > > > > > > > > --
> > > > > > > > > 2.17.1
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Tue, Sep 03, 2019 at 09:17:49AM +0000, Lisovskiy, Stanislav wrote:
> On Mon, 2019-08-19 at 09:05 +0000, Lisovskiy, Stanislav wrote:
> > On Mon, 2019-08-19 at 08:35 +0100, Peres, Martin wrote:
> > > On 19/08/2019 10:23, Lisovskiy, Stanislav wrote:
> > > > On Wed, 2019-08-07 at 23:07 +0200, Daniel Vetter wrote:
> > > > > 
> > > > > 
> > > > > So igt isn't valid userspace (it's just good testcases). Can we
> > > > > repro
> > > > > the
> > > > > same on real userspace? Does this work with real userspace?
> > > > > We've
> > > > > had
> > > > > userspace which tries to be clever and filters out what looks
> > > > > like
> > > > > redundant hotplug events. And then gets it wrong in cases like
> > > > > this.
> > > > > 
> > > > > Also, we've had forever an unconditional uevent on resume,
> > > > > exactly
> > > > > because
> > > > > anything could have changed. Did we loose this one on the way
> > > > > somewhere?
> > > > > Or maybe I misremember ...
> > > > > 
> > > > > If all we care about is resume re-adding that uncondtional
> > > > > uevent
> > > > > on
> > > > > resume is going to be a lot easier than this here.
> > > > > -Daniel
> > > > 
> > > > Sorry for long reply(was on vacation), that is a good question
> > > > regarding reproducing this in real life scenario. My obvious
> > > > guess
> > > > was to suspend the machine and meanwhile change connected display
> > > > to
> > > > another one. However this situation seems to be already handled
> > > > by
> > > > kernel nicely(tried few times and we always get a hotplug event).
> > > > So
> > > > that edid change during suspend chamelium test case seems to be
> > > > a bit different. I will talk to our guys who wrote this about
> > > > what
> > > > is
> > > > the real life scenario for this, because I'm now curious as well.
> > > 
> > > Thanks Daniel for the feedback.
> > > 
> > > I also now wonder why our IGT test (chamelium-based) does not pass
> > > if
> > > a
> > > uevent is sent on resume automatically and all the test is
> > > expecting
> > > is
> > > a uevent...
> > > 
> > > Martin
> > 
> > In fact I was wrong - when it worked, it was using exactly those
> > patches :). With clean drm-tip - it seems to work ocassionally and it
> > doesn't update the actual display edid and other stuff, so even when
> > displays are changed we still see the old info/edid from userspace.
> > 
> > We always get a hpd irq when suspend/resume however it doesn't always
> > result in uevent being sent. So there is a real need in those
> > patches.
> > 
> 
> Just decided to "ping" this discussion again. The issue is already some
> years old and still nothing is fixed. I do agree that may be something
> needs to be fixed/changed here in those patches, but something must be
> agreed at least I guess, as discussions themself do not fix bugs.
> Currently those patches address a particular issue which occurs, if
> display is changed during suspend. 
> On ocassional basis, userspace might not get a hotplug event at all,
> causing different kind of problems(like wrong mode set on display or
> diaply not working at all). Also some kms_chamelium hotplug tests fail
> because of that. 

I still think we'll long-term regret this if we just duct-tape more stuff
on top, instead of giving userspace a more informative uevent. This will
send more uevents to userspace, so maybe then userspace tries to filter
more and be clever, which never works, and we're back to tears.

Anyway, on the approach itself: It's extremely i915 specific, and it
requires that all drivers roll out drm_edid_equal checks and not forget to
increment the epoch counter.

What I had in mind is that when we set the edid for a connector with
drm_connector_update_edid_property() or whatever, then the epoch counter
would auto-increment if anything has changed. Similarly (long-term idea at
least) if anything important with DP registers has changed.

Can't we do that, instead of this sub-optimal solution of requiring all
drivers to roll out lots of code?
-Daniel

> 
> > > 
> > > > 
> > > > - Stanislav
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > -Stanislav
> > > > > > 
> > > > > > > 
> > > > > > > Cheers, Daniel
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -Stanislav
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > -Daniel
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > >   drm: Introduce change counter to drm_connector
> > > > > > > > > >   drm/i915: Send hotplug event if edid had changed.
> > > > > > > > > > 
> > > > > > > > > >  drivers/gpu/drm/drm_connector.c              |  1 +
> > > > > > > > > >  drivers/gpu/drm/drm_edid.c                   | 33
> > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           | 29
> > > > > > > > > > +++++++++++++++-
> > > > > > > > > > -
> > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      | 16
> > > > > > > > > > +++++++++-
> > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    | 16
> > > > > > > > > > ++++++++--
> > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c | 21
> > > > > > > > > > ++++++++++
> > > > > > > > > > ---
> > > > > > > > > >  include/drm/drm_connector.h                  |  3 ++
> > > > > > > > > >  include/drm/drm_edid.h                       |  9
> > > > > > > > > > ++++++
> > > > > > > > > >  8 files changed, 117 insertions(+), 11 deletions(-)
> > > > > > > > > > 
> > > > > > > > > > --
> > > > > > > > > > 2.17.1
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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 v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> 
> > > In fact I was wrong - when it worked, it was using exactly those
> > > patches :). With clean drm-tip - it seems to work ocassionally
> > > and it
> > > doesn't update the actual display edid and other stuff, so even
> > > when
> > > displays are changed we still see the old info/edid from
> > > userspace.
> > > 
> > > We always get a hpd irq when suspend/resume however it doesn't
> > > always
> > > result in uevent being sent. So there is a real need in those
> > > patches.
> > > 
> > 
> > Just decided to "ping" this discussion again. The issue is already
> > some
> > years old and still nothing is fixed. I do agree that may be
> > something
> > needs to be fixed/changed here in those patches, but something must
> > be
> > agreed at least I guess, as discussions themself do not fix bugs.
> > Currently those patches address a particular issue which occurs, if
> > display is changed during suspend. 
> > On ocassional basis, userspace might not get a hotplug event at
> > all,
> > causing different kind of problems(like wrong mode set on display
> > or
> > diaply not working at all). Also some kms_chamelium hotplug tests
> > fail
> > because of that. 
> 
> I still think we'll long-term regret this if we just duct-tape more
> stuff
> on top, instead of giving userspace a more informative uevent. This
> will
> send more uevents to userspace, so maybe then userspace tries to
> filter
> more and be clever, which never works, and we're back to tears.

But here we actually do need a uevent as currently we don't get any at
all, if edid changes during suspend. If userspace will try to filter
this out - it's just stupid, however we still need to do things
correctly.

> 
> Anyway, on the approach itself: It's extremely i915 specific, and it
> requires that all drivers roll out drm_edid_equal checks and not
> forget to
> increment the epoch counter.

> 
> What I had in mind is that when we set the edid for a connector with
> drm_connector_update_edid_property() or whatever, then the epoch
> counter
> would auto-increment if anything has changed. Similarly (long-term
> idea at
> least) if anything important with DP registers has changed.
> 
> Can't we do that, instead of this sub-optimal solution of requiring
> all
> drivers to roll out lots of code?

1) We update edid in intel_dp_set_edid, which is called from
intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which is
called from drm_helper_probe_detect. That one is called either from
specific intel_encoder->hotplug hook in i915_hotplug_work_func or by
userspace request during reprobe.

2) Previously we were simply updating edid in intel_dp_set_edid without
caring if it is the same or not and hotplug event was sent only once
connection_status had changed. 

3) drm_connector_update_edid_property is called from connector-
>get_modes hook(lets say intel_dp_get_modes fo dp) however it simply
uses results of
drm_helper_probe_detect so without actual comparison it would not be
able to detect if we really need to update epoch_counter or not.

Because as I said currently intel_dp_set_edid simply assigns it without
checking, so that way you will get epoch_counter updated every time,
i.e exactly what you wanted to avoid here.

So we really need someway to determine if edid had changed, instead of
simply assigning it all the time - that is why I had to make this
function.

Cheers,

Stanislav


> -Daniel
> 
> > 
> > > > 
> > > > > 
> > > > > - Stanislav
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > -Stanislav
> > > > > > > 
> > > > > > > > 
> > > > > > > > Cheers, Daniel
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > -Stanislav
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > -Daniel
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > >   drm: Introduce change counter to drm_connector
> > > > > > > > > > >   drm/i915: Send hotplug event if edid had
> > > > > > > > > > > changed.
> > > > > > > > > > > 
> > > > > > > > > > >  drivers/gpu/drm/drm_connector.c              |  
> > > > > > > > > > > 1 +
> > > > > > > > > > >  drivers/gpu/drm/drm_edid.c                   |
> > > > > > > > > > > 33
> > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           |
> > > > > > > > > > > 29
> > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > -
> > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      |
> > > > > > > > > > > 16
> > > > > > > > > > > +++++++++-
> > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    |
> > > > > > > > > > > 16
> > > > > > > > > > > ++++++++--
> > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c |
> > > > > > > > > > > 21
> > > > > > > > > > > ++++++++++
> > > > > > > > > > > ---
> > > > > > > > > > >  include/drm/drm_connector.h                  |  
> > > > > > > > > > > 3 ++
> > > > > > > > > > >  include/drm/drm_edid.h                       |  
> > > > > > > > > > > 9
> > > > > > > > > > > ++++++
> > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > deletions(-)
> > > > > > > > > > > 
> > > > > > > > > > > --
> > > > > > > > > > > 2.17.1
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Tue, 2019-09-03 at 11:49 +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> > 
> > > > In fact I was wrong - when it worked, it was using exactly
> > > > those
> > > > patches :). With clean drm-tip - it seems to work ocassionally
> > > > and it
> > > > doesn't update the actual display edid and other stuff, so even
> > > > when
> > > > displays are changed we still see the old info/edid from
> > > > userspace.
> > > > 
> > > > We always get a hpd irq when suspend/resume however it doesn't
> > > > always
> > > > result in uevent being sent. So there is a real need in those
> > > > patches.
> > > > 
> > > 
> > > Just decided to "ping" this discussion again. The issue is
> > > already
> > > some
> > > years old and still nothing is fixed. I do agree that may be
> > > something
> > > needs to be fixed/changed here in those patches, but something
> > > must
> > > be
> > > agreed at least I guess, as discussions themself do not fix bugs.
> > > Currently those patches address a particular issue which occurs,
> > > if
> > > display is changed during suspend. 
> > > On ocassional basis, userspace might not get a hotplug event at
> > > all,
> > > causing different kind of problems(like wrong mode set on display
> > > or
> > > diaply not working at all). Also some kms_chamelium hotplug tests
> > > fail
> > > because of that. 
> > 
> > I still think we'll long-term regret this if we just duct-tape more
> > stuff
> > on top, instead of giving userspace a more informative uevent. This
> > will
> > send more uevents to userspace, so maybe then userspace tries to
> > filter
> > more and be clever, which never works, and we're back to tears.
> 
> But here we actually do need a uevent as currently we don't get any
> at
> all, if edid changes during suspend. If userspace will try to filter
> this out - it's just stupid, however we still need to do things
> correctly.
> 
> > 
> > Anyway, on the approach itself: It's extremely i915 specific, and
> > it
> > requires that all drivers roll out drm_edid_equal checks and not
> > forget to
> > increment the epoch counter.
> > 
> > What I had in mind is that when we set the edid for a connector
> > with
> > drm_connector_update_edid_property() or whatever, then the epoch
> > counter
> > would auto-increment if anything has changed. Similarly (long-term
> > idea at
> > least) if anything important with DP registers has changed.
> > 
> > Can't we do that, instead of this sub-optimal solution of requiring
> > all
> > drivers to roll out lots of code?

So once again, just to summarize things:

1) We update edid in intel_dp_set_edid, which is called from
intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which is
called from drm_helper_probe_detect. That one is called either from
specific intel_encoder->hotplug hook in i915_hotplug_work_func or by
userspace request during reprobe.

2) Currently we are simply updating edid in intel_dp_set_edid without
caring if it is the same or not and hotplug event is sent only once
connection_status had changed. 

3) drm_connector_update_edid_property is called from connector-
>get_modes hook(lets say intel_dp_get_modes fo dp) however it simply
uses results of
drm_helper_probe_detect so without actual comparison it would not be
able to detect if we really need to update epoch_counter or not.

Because as I said currently intel_dp_set_edid simply assigns it without
checking, so that way you will get epoch_counter updated every time,
i.e exactly what you wanted to avoid here.

So we really need someway to determine if edid had changed, instead of
simply assigning it all the time - that is why I had to implement
drm_edid_equal function.


- Stanislav


> 
> 
> > -Daniel
> > 
> > > 
> > > > > 
> > > > > > 
> > > > > > - Stanislav
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > -Stanislav
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Cheers, Daniel
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > -Stanislav
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > -Daniel
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > > >   drm: Introduce change counter to
> > > > > > > > > > > > drm_connector
> > > > > > > > > > > >   drm/i915: Send hotplug event if edid had
> > > > > > > > > > > > changed.
> > > > > > > > > > > > 
> > > > > > > > > > > >  drivers/gpu/drm/drm_connector.c              |
> > > > > > > > > > > >   
> > > > > > > > > > > > 1 +
> > > > > > > > > > > >  drivers/gpu/drm/drm_edid.c                   |
> > > > > > > > > > > > 33
> > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           |
> > > > > > > > > > > > 29
> > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > -
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      |
> > > > > > > > > > > > 16
> > > > > > > > > > > > +++++++++-
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    |
> > > > > > > > > > > > 16
> > > > > > > > > > > > ++++++++--
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c |
> > > > > > > > > > > > 21
> > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > ---
> > > > > > > > > > > >  include/drm/drm_connector.h                  |
> > > > > > > > > > > >   
> > > > > > > > > > > > 3 ++
> > > > > > > > > > > >  include/drm/drm_edid.h                       |
> > > > > > > > > > > >   
> > > > > > > > > > > > 9
> > > > > > > > > > > > ++++++
> > > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Tue, Sep 03, 2019 at 11:49:23AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> > 
> > > > In fact I was wrong - when it worked, it was using exactly those
> > > > patches :). With clean drm-tip - it seems to work ocassionally
> > > > and it
> > > > doesn't update the actual display edid and other stuff, so even
> > > > when
> > > > displays are changed we still see the old info/edid from
> > > > userspace.
> > > > 
> > > > We always get a hpd irq when suspend/resume however it doesn't
> > > > always
> > > > result in uevent being sent. So there is a real need in those
> > > > patches.
> > > > 
> > > 
> > > Just decided to "ping" this discussion again. The issue is already
> > > some
> > > years old and still nothing is fixed. I do agree that may be
> > > something
> > > needs to be fixed/changed here in those patches, but something must
> > > be
> > > agreed at least I guess, as discussions themself do not fix bugs.
> > > Currently those patches address a particular issue which occurs, if
> > > display is changed during suspend. 
> > > On ocassional basis, userspace might not get a hotplug event at
> > > all,
> > > causing different kind of problems(like wrong mode set on display
> > > or
> > > diaply not working at all). Also some kms_chamelium hotplug tests
> > > fail
> > > because of that. 
> > 
> > I still think we'll long-term regret this if we just duct-tape more
> > stuff
> > on top, instead of giving userspace a more informative uevent. This
> > will
> > send more uevents to userspace, so maybe then userspace tries to
> > filter
> > more and be clever, which never works, and we're back to tears.
> 
> But here we actually do need a uevent as currently we don't get any at
> all, if edid changes during suspend. If userspace will try to filter
> this out - it's just stupid, however we still need to do things
> correctly.
> 
> > 
> > Anyway, on the approach itself: It's extremely i915 specific, and it
> > requires that all drivers roll out drm_edid_equal checks and not
> > forget to
> > increment the epoch counter.
> 
> > 
> > What I had in mind is that when we set the edid for a connector with
> > drm_connector_update_edid_property() or whatever, then the epoch
> > counter
> > would auto-increment if anything has changed. Similarly (long-term
> > idea at
> > least) if anything important with DP registers has changed.
> > 
> > Can't we do that, instead of this sub-optimal solution of requiring
> > all
> > drivers to roll out lots of code?
> 
> 1) We update edid in intel_dp_set_edid, which is called from
> intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which is
> called from drm_helper_probe_detect. That one is called either from
> specific intel_encoder->hotplug hook in i915_hotplug_work_func or by
> userspace request during reprobe.
> 
> 2) Previously we were simply updating edid in intel_dp_set_edid without
> caring if it is the same or not and hotplug event was sent only once
> connection_status had changed. 
> 
> 3) drm_connector_update_edid_property is called from connector-
> >get_modes hook(lets say intel_dp_get_modes fo dp) however it simply
> uses results of
> drm_helper_probe_detect so without actual comparison it would not be
> able to detect if we really need to update epoch_counter or not.
> 
> Because as I said currently intel_dp_set_edid simply assigns it without
> checking, so that way you will get epoch_counter updated every time,
> i.e exactly what you wanted to avoid here.
> 
> So we really need someway to determine if edid had changed, instead of
> simply assigning it all the time - that is why I had to make this
> function.

update_edid is the thing which changes the userspace visible edid. We can
check there with the previous userspace visible edid, and if it's
different, increase the epoch counter. If we never call update_edid then
userspace won't see the changed edid (it might see the changed mode list
or whatever), so doing that is pretty much a requirement for correctness.

The higher levels should notice the epoch counter change (you might not
have captured all of them, there's a bunch between ioctl, poll worker and
sysfs iirc), and send out the uevent.

Why does that not work?
-Daniel

> 
> Cheers,
> 
> Stanislav
> 
> 
> > -Daniel
> > 
> > > 
> > > > > 
> > > > > > 
> > > > > > - Stanislav
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > -Stanislav
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Cheers, Daniel
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > -Stanislav
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > -Daniel
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > > >   drm: Introduce change counter to drm_connector
> > > > > > > > > > > >   drm/i915: Send hotplug event if edid had
> > > > > > > > > > > > changed.
> > > > > > > > > > > > 
> > > > > > > > > > > >  drivers/gpu/drm/drm_connector.c              |  
> > > > > > > > > > > > 1 +
> > > > > > > > > > > >  drivers/gpu/drm/drm_edid.c                   |
> > > > > > > > > > > > 33
> > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c           |
> > > > > > > > > > > > 29
> > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > -
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c      |
> > > > > > > > > > > > 16
> > > > > > > > > > > > +++++++++-
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c    |
> > > > > > > > > > > > 16
> > > > > > > > > > > > ++++++++--
> > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c |
> > > > > > > > > > > > 21
> > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > ---
> > > > > > > > > > > >  include/drm/drm_connector.h                  |  
> > > > > > > > > > > > 3 ++
> > > > > > > > > > > >  include/drm/drm_edid.h                       |  
> > > > > > > > > > > > 9
> > > > > > > > > > > > ++++++
> > > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > --
> > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 

-- 
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 v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Tue, 2019-09-03 at 17:49 +0200, Daniel Vetter wrote:
> On Tue, Sep 03, 2019 at 11:49:23AM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> > > 
> > > > > In fact I was wrong - when it worked, it was using exactly
> > > > > those
> > > > > patches :). With clean drm-tip - it seems to work
> > > > > ocassionally
> > > > > and it
> > > > > doesn't update the actual display edid and other stuff, so
> > > > > even
> > > > > when
> > > > > displays are changed we still see the old info/edid from
> > > > > userspace.
> > > > > 
> > > > > We always get a hpd irq when suspend/resume however it
> > > > > doesn't
> > > > > always
> > > > > result in uevent being sent. So there is a real need in those
> > > > > patches.
> > > > > 
> > > > 
> > > > Just decided to "ping" this discussion again. The issue is
> > > > already
> > > > some
> > > > years old and still nothing is fixed. I do agree that may be
> > > > something
> > > > needs to be fixed/changed here in those patches, but something
> > > > must
> > > > be
> > > > agreed at least I guess, as discussions themself do not fix
> > > > bugs.
> > > > Currently those patches address a particular issue which
> > > > occurs, if
> > > > display is changed during suspend. 
> > > > On ocassional basis, userspace might not get a hotplug event at
> > > > all,
> > > > causing different kind of problems(like wrong mode set on
> > > > display
> > > > or
> > > > diaply not working at all). Also some kms_chamelium hotplug
> > > > tests
> > > > fail
> > > > because of that. 
> > > 
> > > I still think we'll long-term regret this if we just duct-tape
> > > more
> > > stuff
> > > on top, instead of giving userspace a more informative uevent.
> > > This
> > > will
> > > send more uevents to userspace, so maybe then userspace tries to
> > > filter
> > > more and be clever, which never works, and we're back to tears.
> > 
> > But here we actually do need a uevent as currently we don't get any
> > at
> > all, if edid changes during suspend. If userspace will try to
> > filter
> > this out - it's just stupid, however we still need to do things
> > correctly.
> > 
> > > 
> > > Anyway, on the approach itself: It's extremely i915 specific, and
> > > it
> > > requires that all drivers roll out drm_edid_equal checks and not
> > > forget to
> > > increment the epoch counter.
> > > 
> > > What I had in mind is that when we set the edid for a connector
> > > with
> > > drm_connector_update_edid_property() or whatever, then the epoch
> > > counter
> > > would auto-increment if anything has changed. Similarly (long-
> > > term
> > > idea at
> > > least) if anything important with DP registers has changed.
> > > 
> > > Can't we do that, instead of this sub-optimal solution of
> > > requiring
> > > all
> > > drivers to roll out lots of code?
> > 
> > 1) We update edid in intel_dp_set_edid, which is called from
> > intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which
> > is
> > called from drm_helper_probe_detect. That one is called either from
> > specific intel_encoder->hotplug hook in i915_hotplug_work_func or
> > by
> > userspace request during reprobe.
> > 
> > 2) Previously we were simply updating edid in intel_dp_set_edid
> > without
> > caring if it is the same or not and hotplug event was sent only
> > once
> > connection_status had changed. 
> > 
> > 3) drm_connector_update_edid_property is called from connector-
> > > get_modes hook(lets say intel_dp_get_modes fo dp) however it
> > > simply
> > 
> > uses results of
> > drm_helper_probe_detect so without actual comparison it would not
> > be
> > able to detect if we really need to update epoch_counter or not.
> > 
> > Because as I said currently intel_dp_set_edid simply assigns it
> > without
> > checking, so that way you will get epoch_counter updated every
> > time,
> > i.e exactly what you wanted to avoid here.
> > 
> > So we really need someway to determine if edid had changed, instead
> > of
> > simply assigning it all the time - that is why I had to make this
> > function.
> 
> update_edid is the thing which changes the userspace visible edid. We
> can
> check there with the previous userspace visible edid, and if it's
> different, increase the epoch counter. If we never call update_edid
> then
> userspace won't see the changed edid (it might see the changed mode
> list
> or whatever), so doing that is pretty much a requirement for
> correctness.
> 
> The higher levels should notice the epoch counter change (you might
> not
> have captured all of them, there's a bunch between ioctl, poll worker
> and
> sysfs iirc), and send out the uevent.
> 
> Why does that not work?

Sure this will work, but still we need somehow to be able to determine
this "if it's different" state. In your solution we just move that
comparison to drm_connector_update_edid_property, which is quite fine
for me.

I would say that yes, this idea may be is even better because 
drivers won't need to implement this comparison in encoder->hotplug in
each driver.
However: 
we still need a comparison in
drm_connector_update_edid_property(drm_edid_equal) and also I'm not
sure we can send a hotplug event based on that as
drm_connector_update_edid_property seems
to get called only during connector init or during reprobe from
userspace from connector->get_modes hook.
Also it is called from drm_kms_helper_hotplug_event from, but this one
is called from i915 only if connection status had changed.

- Stanislav


> -Daniel
> 
> > 
> > Cheers,
> > 
> > Stanislav
> > 
> > 
> > > -Daniel
> > > 
> > > > 
> > > > > > 
> > > > > > > 
> > > > > > > - Stanislav
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > -Stanislav
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Cheers, Daniel
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > -Stanislav
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > > -Daniel
> > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > > > >   drm: Introduce change counter to
> > > > > > > > > > > > > drm_connector
> > > > > > > > > > > > >   drm/i915: Send hotplug event if edid had
> > > > > > > > > > > > > changed.
> > > > > > > > > > > > > 
> > > > > > > > > > > > >  drivers/gpu/drm/drm_connector.c             
> > > > > > > > > > > > >  |  
> > > > > > > > > > > > > 1 +
> > > > > > > > > > > > >  drivers/gpu/drm/drm_edid.c                  
> > > > > > > > > > > > >  |
> > > > > > > > > > > > > 33
> > > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c          
> > > > > > > > > > > > >  |
> > > > > > > > > > > > > 29
> > > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > > -
> > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c     
> > > > > > > > > > > > >  |
> > > > > > > > > > > > > 16
> > > > > > > > > > > > > +++++++++-
> > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c   
> > > > > > > > > > > > >  |
> > > > > > > > > > > > > 16
> > > > > > > > > > > > > ++++++++--
> > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > > > > > > > > > |
> > > > > > > > > > > > > 21
> > > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  include/drm/drm_connector.h                 
> > > > > > > > > > > > >  |  
> > > > > > > > > > > > > 3 ++
> > > > > > > > > > > > >  include/drm/drm_edid.h                      
> > > > > > > > > > > > >  |  
> > > > > > > > > > > > > 9
> > > > > > > > > > > > > ++++++
> > > > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > > > 
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Daniel Vetter]

On Wed, Sep 04, 2019 at 08:36:46AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-09-03 at 17:49 +0200, Daniel Vetter wrote:
> > On Tue, Sep 03, 2019 at 11:49:23AM +0000, Lisovskiy, Stanislav wrote:
> > > On Tue, 2019-09-03 at 11:40 +0200, Daniel Vetter wrote:
> > > > 
> > > > > > In fact I was wrong - when it worked, it was using exactly
> > > > > > those
> > > > > > patches :). With clean drm-tip - it seems to work
> > > > > > ocassionally
> > > > > > and it
> > > > > > doesn't update the actual display edid and other stuff, so
> > > > > > even
> > > > > > when
> > > > > > displays are changed we still see the old info/edid from
> > > > > > userspace.
> > > > > > 
> > > > > > We always get a hpd irq when suspend/resume however it
> > > > > > doesn't
> > > > > > always
> > > > > > result in uevent being sent. So there is a real need in those
> > > > > > patches.
> > > > > > 
> > > > > 
> > > > > Just decided to "ping" this discussion again. The issue is
> > > > > already
> > > > > some
> > > > > years old and still nothing is fixed. I do agree that may be
> > > > > something
> > > > > needs to be fixed/changed here in those patches, but something
> > > > > must
> > > > > be
> > > > > agreed at least I guess, as discussions themself do not fix
> > > > > bugs.
> > > > > Currently those patches address a particular issue which
> > > > > occurs, if
> > > > > display is changed during suspend. 
> > > > > On ocassional basis, userspace might not get a hotplug event at
> > > > > all,
> > > > > causing different kind of problems(like wrong mode set on
> > > > > display
> > > > > or
> > > > > diaply not working at all). Also some kms_chamelium hotplug
> > > > > tests
> > > > > fail
> > > > > because of that. 
> > > > 
> > > > I still think we'll long-term regret this if we just duct-tape
> > > > more
> > > > stuff
> > > > on top, instead of giving userspace a more informative uevent.
> > > > This
> > > > will
> > > > send more uevents to userspace, so maybe then userspace tries to
> > > > filter
> > > > more and be clever, which never works, and we're back to tears.
> > > 
> > > But here we actually do need a uevent as currently we don't get any
> > > at
> > > all, if edid changes during suspend. If userspace will try to
> > > filter
> > > this out - it's just stupid, however we still need to do things
> > > correctly.
> > > 
> > > > 
> > > > Anyway, on the approach itself: It's extremely i915 specific, and
> > > > it
> > > > requires that all drivers roll out drm_edid_equal checks and not
> > > > forget to
> > > > increment the epoch counter.
> > > > 
> > > > What I had in mind is that when we set the edid for a connector
> > > > with
> > > > drm_connector_update_edid_property() or whatever, then the epoch
> > > > counter
> > > > would auto-increment if anything has changed. Similarly (long-
> > > > term
> > > > idea at
> > > > least) if anything important with DP registers has changed.
> > > > 
> > > > Can't we do that, instead of this sub-optimal solution of
> > > > requiring
> > > > all
> > > > drivers to roll out lots of code?
> > > 
> > > 1) We update edid in intel_dp_set_edid, which is called from
> > > intel_dp_detect(drm_connector_helper_funcs->detect_ctx hook) which
> > > is
> > > called from drm_helper_probe_detect. That one is called either from
> > > specific intel_encoder->hotplug hook in i915_hotplug_work_func or
> > > by
> > > userspace request during reprobe.
> > > 
> > > 2) Previously we were simply updating edid in intel_dp_set_edid
> > > without
> > > caring if it is the same or not and hotplug event was sent only
> > > once
> > > connection_status had changed. 
> > > 
> > > 3) drm_connector_update_edid_property is called from connector-
> > > > get_modes hook(lets say intel_dp_get_modes fo dp) however it
> > > > simply
> > > 
> > > uses results of
> > > drm_helper_probe_detect so without actual comparison it would not
> > > be
> > > able to detect if we really need to update epoch_counter or not.
> > > 
> > > Because as I said currently intel_dp_set_edid simply assigns it
> > > without
> > > checking, so that way you will get epoch_counter updated every
> > > time,
> > > i.e exactly what you wanted to avoid here.
> > > 
> > > So we really need someway to determine if edid had changed, instead
> > > of
> > > simply assigning it all the time - that is why I had to make this
> > > function.
> > 
> > update_edid is the thing which changes the userspace visible edid. We
> > can
> > check there with the previous userspace visible edid, and if it's
> > different, increase the epoch counter. If we never call update_edid
> > then
> > userspace won't see the changed edid (it might see the changed mode
> > list
> > or whatever), so doing that is pretty much a requirement for
> > correctness.
> > 
> > The higher levels should notice the epoch counter change (you might
> > not
> > have captured all of them, there's a bunch between ioctl, poll worker
> > and
> > sysfs iirc), and send out the uevent.
> > 
> > Why does that not work?
> 
> Sure this will work, but still we need somehow to be able to determine
> this "if it's different" state. In your solution we just move that
> comparison to drm_connector_update_edid_property, which is quite fine
> for me.

Yes we need to compare edid somewhere, that much is clear. I'm not
disputing that. I just want something where we don't have to roll this out
over all the drivers, because that's a hopeless endeavour.

> I would say that yes, this idea may be is even better because 
> drivers won't need to implement this comparison in encoder->hotplug in
> each driver.
> However: 
> we still need a comparison in
> drm_connector_update_edid_property(drm_edid_equal) and also I'm not
> sure we can send a hotplug event based on that as
> drm_connector_update_edid_property seems
> to get called only during connector init or during reprobe from
> userspace from connector->get_modes hook.
> Also it is called from drm_kms_helper_hotplug_event from, but this one
> is called from i915 only if connection status had changed.

So ditch the optimization to only call ->get_modes when called from
userspace? We've been talking about this one too in the past ...

I'd really like a solution where it will work for most drivers out of the
box.
-Daniel

> 
> - Stanislav
> 
> 
> > -Daniel
> > 
> > > 
> > > Cheers,
> > > 
> > > Stanislav
> > > 
> > > 
> > > > -Daniel
> > > > 
> > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > - Stanislav
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > -Stanislav
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Cheers, Daniel
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > -Stanislav
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > > -Daniel
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > > > > >   drm: Introduce change counter to
> > > > > > > > > > > > > > drm_connector
> > > > > > > > > > > > > >   drm/i915: Send hotplug event if edid had
> > > > > > > > > > > > > > changed.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >  drivers/gpu/drm/drm_connector.c             
> > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > 1 +
> > > > > > > > > > > > > >  drivers/gpu/drm/drm_edid.c                  
> > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > 33
> > > > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c          
> > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > 29
> > > > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > > > -
> > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c     
> > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > +++++++++-
> > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.c   
> > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > ++++++++--
> > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotplug.c
> > > > > > > > > > > > > > |
> > > > > > > > > > > > > > 21
> > > > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >  include/drm/drm_connector.h                 
> > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > 3 ++
> > > > > > > > > > > > > >  include/drm/drm_edid.h                      
> > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > 9
> > > > > > > > > > > > > > ++++++
> > > > > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > --
> > > > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > 
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > 
> > > > 
> > 
> > 

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

Re: [PATCH v3 0/3] Send a hotplug when edid changes

[Lisovskiy, Stanislav]

On Wed, 2019-09-04 at 11:23 +0200, Daniel Vetter wrote:
> 
> > Sure this will work, but still we need somehow to be able to
> > determine
> > this "if it's different" state. In your solution we just move that
> > comparison to drm_connector_update_edid_property, which is quite
> > fine
> > for me.
> 
> Yes we need to compare edid somewhere, that much is clear. I'm not
> disputing that. I just want something where we don't have to roll
> this out
> over all the drivers, because that's a hopeless endeavour.
> 
> > I would say that yes, this idea may be is even better because 
> > drivers won't need to implement this comparison in encoder->hotplug 
> > in
> > each driver.
> > However: 
> > we still need a comparison in
> > drm_connector_update_edid_property(drm_edid_equal) and also I'm not
> > sure we can send a hotplug event based on that as
> > drm_connector_update_edid_property seems
> > to get called only during connector init or during reprobe from
> > userspace from connector->get_modes hook.
> > Also it is called from drm_kms_helper_hotplug_event from, but this
> > one
> > is called from i915 only if connection status had changed.
> 
> So ditch the optimization to only call ->get_modes when called from
> userspace? We've been talking about this one too in the past ...
> 
> I'd really like a solution where it will work for most drivers out of
> the
> box.

So I guess the conclusion would be to try to use
drm_connector_update_edid_property that way we will avoid duplicating
drm_edid_equal code in all drivers. However this might
require ensuring that drm_connector_update_edid_property is always
called when we get a hotplug, so there we can check if edid had changed
and send uevent, if needed.

- Stanislav


> -Daniel
> 
> > 
> > - Stanislav
> > 
> > 
> > > -Daniel
> > > 
> > > > 
> > > > Cheers,
> > > > 
> > > > Stanislav
> > > > 
> > > > 
> > > > > -Daniel
> > > > > 
> > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > - Stanislav
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > -Stanislav
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Cheers, Daniel
> > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > -Stanislav
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > -Daniel
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Stanislav Lisovskiy (3):
> > > > > > > > > > > > > > >   drm: Add helper to compare edids.
> > > > > > > > > > > > > > >   drm: Introduce change counter to
> > > > > > > > > > > > > > > drm_connector
> > > > > > > > > > > > > > >   drm/i915: Send hotplug event if edid
> > > > > > > > > > > > > > > had
> > > > > > > > > > > > > > > changed.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >  drivers/gpu/drm/drm_connector.c         
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > > 1 +
> > > > > > > > > > > > > > >  drivers/gpu/drm/drm_edid.c              
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > > 33
> > > > > > > > > > > > > > > ++++++++++++++++++++
> > > > > > > > > > > > > > >  drivers/gpu/drm/drm_probe_helper.c      
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > > 29
> > > > > > > > > > > > > > > +++++++++++++++-
> > > > > > > > > > > > > > > -
> > > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c 
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > > +++++++++-
> > > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hdmi.
> > > > > > > > > > > > > > > c   
> > > > > > > > > > > > > > >  |
> > > > > > > > > > > > > > > 16
> > > > > > > > > > > > > > > ++++++++--
> > > > > > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_hotpl
> > > > > > > > > > > > > > > ug.c
> > > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 21
> > > > > > > > > > > > > > > ++++++++++
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > >  include/drm/drm_connector.h             
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > > 3 ++
> > > > > > > > > > > > > > >  include/drm/drm_edid.h                  
> > > > > > > > > > > > > > >     
> > > > > > > > > > > > > > >  |  
> > > > > > > > > > > > > > > 9
> > > > > > > > > > > > > > > ++++++
> > > > > > > > > > > > > > >  8 files changed, 117 insertions(+), 11
> > > > > > > > > > > > > > > deletions(-)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > --
> > > > > > > > > > > > > > > 2.17.1
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx