This adds a common drm helper to detect whether the EDID changed from
the last known cached one. This is useful help detect that a monitor was
changed during a suspend/resume cycle.
When that happens (a monitor is replaced by another one during suspend),
no hotplug event will be triggered so the change will not be caught at
resume time. Detecting that the EDID changed allows detecting it.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_edid.h | 3 +++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 6bb6337be920..f6ce8bc2907a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct drm_connector *connector,
}
return;
}
+
+/**
+ * drm_check_edid_changed - Check whether the EDID changed since the last update
+ * @connector: connector we're probing
+ * @adapter: I2C adapter to use for DDC
+ *
+ * Check whether the EDID changed since the last time it was updated in the
+ * drm blob cache.
+ *
+ * Return: A boolean indicating whether a change happened or not.
+ */
+bool drm_check_edid_changed(struct drm_connector *connector,
+ struct i2c_adapter *adapter)
+{
+ struct drm_property_blob *edid_blob;
+ struct edid *edid_stored;
+ struct edid *edid_read;
+ int ret = 0;
+
+ if (!connector->edid_blob_ptr)
+ return false;
+
+ edid_blob = drm_property_blob_get(connector->edid_blob_ptr);
+ if (!edid_blob)
+ return false;
+
+ if (!edid_blob->data || edid_blob->length != sizeof(struct edid))
+ goto out;
+
+ edid_stored = (struct edid *) edid_blob->data;
+
+ edid_read = drm_get_edid(connector, adapter);
+ if (!edid_read)
+ goto out;
+
+ ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
+
+ kfree(edid_read);
+
+out:
+ drm_property_blob_put(edid_blob);
+
+ return ret != 0;
+}
+EXPORT_SYMBOL_GPL(drm_check_edid_changed);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 1e1908a6b1d6..593a97b269c3 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
int hsize, int vsize, int fresh,
bool rb);
+bool drm_check_edid_changed(struct drm_connector *connector,
+ struct i2c_adapter *adapter);
+
#endif /* __DRM_EDID_H__ */
--
2.13.2
This introduces a dedicated work and related helpers to detect whether
a monitor was replaced by another one during suspend. This detection is
based on EDID change detection, using the associated generic drm helper.
This requires storing more information to the intel_connector structure,
such as the i2c adapter required to get the EDID.
Support for this is introduced for DP, HDMI and VGA.
Signed-off-by: Paul Kocialkowski <[email protected]>
---
drivers/gpu/drm/i915/i915_drv.c | 2 ++
drivers/gpu/drm/i915/i915_drv.h | 5 ++++
drivers/gpu/drm/i915/intel_crt.c | 3 +++
drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_dp.c | 3 +++
drivers/gpu/drm/i915/intel_drv.h | 6 +++++
drivers/gpu/drm/i915/intel_hdmi.c | 3 +++
7 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 3ac8215c0e36..b3cf4112fc65 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1702,6 +1702,8 @@ static int i915_drm_resume(struct drm_device *dev)
intel_display_resume(dev);
+ intel_edid_changes_detect(dev);
+
drm_kms_helper_poll_enable(dev);
/*
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b051122c960b..2c189a5b714e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3904,6 +3904,11 @@ intel_display_capture_error_state(struct drm_i915_private *dev_priv);
extern void intel_display_print_error_state(struct drm_i915_error_state_buf *e,
struct intel_display_error_state *error);
+/* edid change */
+void intel_edid_change_init(struct intel_connector *connector,
+ struct i2c_adapter *adapter);
+void intel_edid_changes_detect(struct drm_device *dev);
+
int sandybridge_pcode_read(struct drm_i915_private *dev_priv, u32 mbox, u32 *val);
int sandybridge_pcode_write(struct drm_i915_private *dev_priv, u32 mbox, u32 val);
int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 84a1f5e85153..63e2184a6ea4 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -925,6 +925,9 @@ void intel_crt_init(struct drm_i915_private *dev_priv)
*/
crt->force_hotplug_required = 0;
+ intel_edid_change_init(intel_connector,
+ intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin));
+
/*
* TODO: find a proper way to discover whether we need to set the the
* polarity and link reversal bits or not, instead of relying on the
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6c823cc02db3..69e306759a02 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15016,3 +15016,53 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
}
#endif
+
+static void intel_edid_change_work_func(struct work_struct *work)
+{
+ struct intel_connector *connector =
+ container_of(work, struct intel_connector, edid_change_work);
+ bool changed;
+
+ changed = drm_check_edid_changed(&connector->base, connector->adapter);
+ if (changed) {
+ DRM_DEBUG_KMS("[CONNECTOR:%d:%s] EDID change detected\n",
+ connector->base.base.id, connector->base.name);
+
+ drm_kms_helper_hotplug_event(connector->base.dev);
+ }
+}
+
+/**
+ * intel_edid_change_init - init the EDID change data
+ * @connector: DRM connector device to use
+ * @adapter: i2c adapter
+ *
+ * Store the i2c adapter and initialize the EDID change detection work.
+ */
+void intel_edid_change_init(struct intel_connector *connector,
+ struct i2c_adapter *adapter)
+{
+ connector->adapter = adapter;
+
+ INIT_WORK(&connector->edid_change_work, intel_edid_change_work_func);
+}
+
+/**
+ * intel_edid_changes_detect - detect EDID changes to connectors
+ * @dev: DRM device to get connectors from
+ *
+ * Schedule the EDID detection change work for all registered connectors.
+ */
+void intel_edid_changes_detect(struct drm_device *dev)
+{
+ struct intel_connector *connector;
+ struct drm_connector_list_iter conn_iter;
+
+ drm_connector_list_iter_begin(dev, &conn_iter);
+ for_each_intel_connector_iter(connector, &conn_iter) {
+ if (!connector->adapter)
+ continue;
+
+ schedule_work(&connector->edid_change_work);
+ }
+}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2d42d09428c9..bc4dbb076096 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -6063,6 +6063,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
goto fail;
}
+ if (!is_edp(intel_dp))
+ intel_edid_change_init(intel_connector, &intel_dp->aux.ddc);
+
intel_dp_add_properties(intel_dp, connector);
/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 021cc5487853..47d350d2f993 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -319,6 +319,12 @@ struct intel_connector {
struct edid *edid;
struct edid *detect_edid;
+ /* I2C adapter to retrieve the EDID. */
+ struct i2c_adapter *adapter;
+
+ /* Work to detect EDID change. */
+ struct work_struct edid_change_work;
+
/* since POLL and HPD connectors may use the same HPD line keep the native
state of connector->polled in case hotplug storm detection changes it */
u8 polled;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 2f831cfdd243..a6ce77f81668 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1909,6 +1909,9 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
else
intel_connector->get_hw_state = intel_connector_get_hw_state;
+ intel_edid_change_init(intel_connector,
+ intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus));
+
intel_hdmi_add_properties(intel_hdmi, connector);
intel_connector_attach_encoder(intel_connector, intel_encoder);
--
2.13.2
On 2017-07-24 10:54 AM, Paul Kocialkowski wrote:
> This adds a common drm helper to detect whether the EDID changed from
> the last known cached one. This is useful help detect that a monitor was
> changed during a suspend/resume cycle.
>
> When that happens (a monitor is replaced by another one during suspend),
> no hotplug event will be triggered so the change will not be caught at
> resume time. Detecting that the EDID changed allows detecting it.
>
This makes sense and could be used by other drivers.
Acked-by: Harry Wentland <[email protected]>
Harry
> Signed-off-by: Paul Kocialkowski <[email protected]>
> ---
> drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_edid.h | 3 +++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6bb6337be920..f6ce8bc2907a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct drm_connector *connector,
> }
> return;
> }
> +
> +/**
> + * drm_check_edid_changed - Check whether the EDID changed since the last update
> + * @connector: connector we're probing
> + * @adapter: I2C adapter to use for DDC
> + *
> + * Check whether the EDID changed since the last time it was updated in the
> + * drm blob cache.
> + *
> + * Return: A boolean indicating whether a change happened or not.
> + */
> +bool drm_check_edid_changed(struct drm_connector *connector,
> + struct i2c_adapter *adapter)
> +{
> + struct drm_property_blob *edid_blob;
> + struct edid *edid_stored;
> + struct edid *edid_read;
> + int ret = 0;
> +
> + if (!connector->edid_blob_ptr)
> + return false;
> +
> + edid_blob = drm_property_blob_get(connector->edid_blob_ptr);
> + if (!edid_blob)
> + return false;
> +
> + if (!edid_blob->data || edid_blob->length != sizeof(struct edid))
> + goto out;
> +
> + edid_stored = (struct edid *) edid_blob->data;
> +
> + edid_read = drm_get_edid(connector, adapter);
> + if (!edid_read)
> + goto out;
> +
> + ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
> +
> + kfree(edid_read);
> +
> +out:
> + drm_property_blob_put(edid_blob);
> +
> + return ret != 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 1e1908a6b1d6..593a97b269c3 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> int hsize, int vsize, int fresh,
> bool rb);
> +bool drm_check_edid_changed(struct drm_connector *connector,
> + struct i2c_adapter *adapter);
> +
> #endif /* __DRM_EDID_H__ */
>
On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
> This adds a common drm helper to detect whether the EDID changed from
> the last known cached one. This is useful help detect that a monitor was
> changed during a suspend/resume cycle.
>
> When that happens (a monitor is replaced by another one during suspend),
> no hotplug event will be triggered so the change will not be caught at
> resume time. Detecting that the EDID changed allows detecting it.
>
> Signed-off-by: Paul Kocialkowski <[email protected]>
I can't find the older mails I've typed about this, but the plan we've
discussed a while back was:
- Add a generational counter to each connector, maybe even expose it to
userspace.
- Increment that counter every time something changed, e.g.
connector->status in the propbe helpers, or when attaching a new edid
with the set_edid helper.
- Tada, no changes needed to drivers, and easily extensible to other
things than edid!
Cheers, Daniel
> ---
> drivers/gpu/drm/drm_edid.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_edid.h | 3 +++
> 2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 6bb6337be920..f6ce8bc2907a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct drm_connector *connector,
> }
> return;
> }
> +
> +/**
> + * drm_check_edid_changed - Check whether the EDID changed since the last update
> + * @connector: connector we're probing
> + * @adapter: I2C adapter to use for DDC
> + *
> + * Check whether the EDID changed since the last time it was updated in the
> + * drm blob cache.
> + *
> + * Return: A boolean indicating whether a change happened or not.
> + */
> +bool drm_check_edid_changed(struct drm_connector *connector,
> + struct i2c_adapter *adapter)
> +{
> + struct drm_property_blob *edid_blob;
> + struct edid *edid_stored;
> + struct edid *edid_read;
> + int ret = 0;
> +
> + if (!connector->edid_blob_ptr)
> + return false;
> +
> + edid_blob = drm_property_blob_get(connector->edid_blob_ptr);
> + if (!edid_blob)
> + return false;
> +
> + if (!edid_blob->data || edid_blob->length != sizeof(struct edid))
> + goto out;
> +
> + edid_stored = (struct edid *) edid_blob->data;
> +
> + edid_read = drm_get_edid(connector, adapter);
> + if (!edid_read)
> + goto out;
> +
> + ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
> +
> + kfree(edid_read);
> +
> +out:
> + drm_property_blob_put(edid_blob);
> +
> + return ret != 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 1e1908a6b1d6..593a97b269c3 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
> struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> int hsize, int vsize, int fresh,
> bool rb);
> +bool drm_check_edid_changed(struct drm_connector *connector,
> + struct i2c_adapter *adapter);
> +
> #endif /* __DRM_EDID_H__ */
> --
> 2.13.2
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
> > This adds a common drm helper to detect whether the EDID changed
> > from
> > the last known cached one. This is useful help detect that a monitor
> > was
> > changed during a suspend/resume cycle.
> >
> > When that happens (a monitor is replaced by another one during
> > suspend),
> > no hotplug event will be triggered so the change will not be caught
> > at
> > resume time. Detecting that the EDID changed allows detecting it.
> >
> > Signed-off-by: Paul Kocialkowski <[email protected]>
>
> I can't find the older mails I've typed about this, but the plan we've
> discussed a while back was:
> - Add a generational counter to each connector, maybe even expose it
> to
> userspace.
>
> - Increment that counter every time something changed, e.g.
> connector->status in the propbe helpers, or when attaching a new
> edid
> with the set_edid helper.
>
> - Tada, no changes needed to drivers, and easily extensible to other
> things than edid!
I don't see how it solves the problem here though. After a
suspend/resume cycle, there is simply no indication that anything has
changed when a monitor was replaced by another one, so I don't see how
adding a counter in the mix would help.
Could you provide more details about the reasoning? I feel like I'm
missing something here.
Cheers,
Paul
> > ---
> > drivers/gpu/drm/drm_edid.c | 45
> > +++++++++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_edid.h | 3 +++
> > 2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 6bb6337be920..f6ce8bc2907a 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct
> > drm_connector *connector,
> > }
> > return;
> > }
> > +
> > +/**
> > + * drm_check_edid_changed - Check whether the EDID changed since
> > the last update
> > + * @connector: connector we're probing
> > + * @adapter: I2C adapter to use for DDC
> > + *
> > + * Check whether the EDID changed since the last time it was
> > updated in the
> > + * drm blob cache.
> > + *
> > + * Return: A boolean indicating whether a change happened or not.
> > + */
> > +bool drm_check_edid_changed(struct drm_connector *connector,
> > + struct i2c_adapter *adapter)
> > +{
> > + struct drm_property_blob *edid_blob;
> > + struct edid *edid_stored;
> > + struct edid *edid_read;
> > + int ret = 0;
> > +
> > + if (!connector->edid_blob_ptr)
> > + return false;
> > +
> > + edid_blob = drm_property_blob_get(connector-
> > >edid_blob_ptr);
> > + if (!edid_blob)
> > + return false;
> > +
> > + if (!edid_blob->data || edid_blob->length != sizeof(struct
> > edid))
> > + goto out;
> > +
> > + edid_stored = (struct edid *) edid_blob->data;
> > +
> > + edid_read = drm_get_edid(connector, adapter);
> > + if (!edid_read)
> > + goto out;
> > +
> > + ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
> > +
> > + kfree(edid_read);
> > +
> > +out:
> > + drm_property_blob_put(edid_blob);
> > +
> > + return ret != 0;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 1e1908a6b1d6..593a97b269c3 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid
> > *edid, char *name,
> > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
> > int hsize, int vsize,
> > int fresh,
> > bool rb);
> > +bool drm_check_edid_changed(struct drm_connector *connector,
> > + struct i2c_adapter *adapter);
> > +
> > #endif /* __DRM_EDID_H__ */
> > --
> > 2.13.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
<[email protected]> wrote:
> On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
>> On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
>> > This adds a common drm helper to detect whether the EDID changed
>> > from
>> > the last known cached one. This is useful help detect that a monitor
>> > was
>> > changed during a suspend/resume cycle.
>> >
>> > When that happens (a monitor is replaced by another one during
>> > suspend),
>> > no hotplug event will be triggered so the change will not be caught
>> > at
>> > resume time. Detecting that the EDID changed allows detecting it.
>> >
>> > Signed-off-by: Paul Kocialkowski <[email protected]>
>>
>> I can't find the older mails I've typed about this, but the plan we've
>> discussed a while back was:
>> - Add a generational counter to each connector, maybe even expose it
>> to
>> userspace.
>>
>> - Increment that counter every time something changed, e.g.
>> connector->status in the propbe helpers, or when attaching a new
>> edid
>> with the set_edid helper.
>>
>> - Tada, no changes needed to drivers, and easily extensible to other
>> things than edid!
>
> I don't see how it solves the problem here though. After a
> suspend/resume cycle, there is simply no indication that anything has
> changed when a monitor was replaced by another one, so I don't see how
> adding a counter in the mix would help.
>
> Could you provide more details about the reasoning? I feel like I'm
> missing something here.
Your bug doesn't just exist over s/r, it's just much easier to observe
in s/r since users can take however long they want to with plugging in
a different monitor. But the same issue exists e.g. when we go from
hpd to polling because too much noise on the line.
Wrt the suspend/resume issue: What we need to do on resume is do a
full reprobe of all outputs, in an async worker. Telling userspace to
do this by sending an uevent was the cheapest way, but it'd be better
if the kernel could do that asynchronously and inform userspace about
the exact changes. And there's more to reprobe than just the edid, and
we don't want to re-invent a separate reprobe path just for resume
like you start in your patch series. So yeah my plan was missing:
- force a full async reprobe after resume (maybe we could reuse the
poll worker for that as a one-shot).
Cheers, Daniel
>
> Cheers,
>
> Paul
>
>> > ---
>> > drivers/gpu/drm/drm_edid.c | 45
>> > +++++++++++++++++++++++++++++++++++++++++++++
>> > include/drm/drm_edid.h | 3 +++
>> > 2 files changed, 48 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index 6bb6337be920..f6ce8bc2907a 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct
>> > drm_connector *connector,
>> > }
>> > return;
>> > }
>> > +
>> > +/**
>> > + * drm_check_edid_changed - Check whether the EDID changed since
>> > the last update
>> > + * @connector: connector we're probing
>> > + * @adapter: I2C adapter to use for DDC
>> > + *
>> > + * Check whether the EDID changed since the last time it was
>> > updated in the
>> > + * drm blob cache.
>> > + *
>> > + * Return: A boolean indicating whether a change happened or not.
>> > + */
>> > +bool drm_check_edid_changed(struct drm_connector *connector,
>> > + struct i2c_adapter *adapter)
>> > +{
>> > + struct drm_property_blob *edid_blob;
>> > + struct edid *edid_stored;
>> > + struct edid *edid_read;
>> > + int ret = 0;
>> > +
>> > + if (!connector->edid_blob_ptr)
>> > + return false;
>> > +
>> > + edid_blob = drm_property_blob_get(connector-
>> > >edid_blob_ptr);
>> > + if (!edid_blob)
>> > + return false;
>> > +
>> > + if (!edid_blob->data || edid_blob->length != sizeof(struct
>> > edid))
>> > + goto out;
>> > +
>> > + edid_stored = (struct edid *) edid_blob->data;
>> > +
>> > + edid_read = drm_get_edid(connector, adapter);
>> > + if (!edid_read)
>> > + goto out;
>> > +
>> > + ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
>> > +
>> > + kfree(edid_read);
>> > +
>> > +out:
>> > + drm_property_blob_put(edid_blob);
>> > +
>> > + return ret != 0;
>> > +}
>> > +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
>> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > index 1e1908a6b1d6..593a97b269c3 100644
>> > --- a/include/drm/drm_edid.h
>> > +++ b/include/drm/drm_edid.h
>> > @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid
>> > *edid, char *name,
>> > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>> > int hsize, int vsize,
>> > int fresh,
>> > bool rb);
>> > +bool drm_check_edid_changed(struct drm_connector *connector,
>> > + struct i2c_adapter *adapter);
>> > +
>> > #endif /* __DRM_EDID_H__ */
>> > --
>> > 2.13.2
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > [email protected]
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
> --
> Paul Kocialkowski <[email protected]>
> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
> <[email protected]> wrote:
> > On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> > > On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
> > > > This adds a common drm helper to detect whether the EDID changed
> > > > from
> > > > the last known cached one. This is useful help detect that a
> > > > monitor
> > > > was
> > > > changed during a suspend/resume cycle.
> > > >
> > > > When that happens (a monitor is replaced by another one during
> > > > suspend),
> > > > no hotplug event will be triggered so the change will not be
> > > > caught
> > > > at
> > > > resume time. Detecting that the EDID changed allows detecting
> > > > it.
> > > >
> > > > Signed-off-by: Paul Kocialkowski <[email protected].
> > > > com>
> > >
> > > I can't find the older mails I've typed about this, but the plan
> > > we've
> > > discussed a while back was:
> > > - Add a generational counter to each connector, maybe even expose
> > > it
> > > to
> > > userspace.
> > >
> > > - Increment that counter every time something changed, e.g.
> > > connector->status in the propbe helpers, or when attaching a new
> > > edid
> > > with the set_edid helper.
> > >
> > > - Tada, no changes needed to drivers, and easily extensible to
> > > other
> > > things than edid!
> >
> > I don't see how it solves the problem here though. After a
> > suspend/resume cycle, there is simply no indication that anything
> > has
> > changed when a monitor was replaced by another one, so I don't see
> > how
> > adding a counter in the mix would help.
> >
> > Could you provide more details about the reasoning? I feel like I'm
> > missing something here.
>
> Your bug doesn't just exist over s/r, it's just much easier to observe
> in s/r since users can take however long they want to with plugging in
> a different monitor. But the same issue exists e.g. when we go from
> hpd to polling because too much noise on the line.
>
> Wrt the suspend/resume issue: What we need to do on resume is do a
> full reprobe of all outputs, in an async worker. Telling userspace to
> do this by sending an uevent was the cheapest way, but it'd be better
> if the kernel could do that asynchronously and inform userspace about
> the exact changes. And there's more to reprobe than just the edid, and
> we don't want to re-invent a separate reprobe path just for resume
> like you start in your patch series. So yeah my plan was missing:
>
> - force a full async reprobe after resume (maybe we could reuse the
> poll worker for that as a one-shot).
First off, I definitely agree we need a way to tell userspace exactly
what has happened. I wanted to start a discussion about that in i-g-t
patch "Unrelated hotplug uevent masking out actual test result" but it
didn't get much traction. For testing purposes, it is unacceptable that
userspace only gets notified that "something happened".
Still, as far as I know, userspace is expected to ask for a full reprobe
when something has changed, and that is apparently part of the DRM spec,
so we can't expect that it could query for an update on "only the things
that changed".
However, one way to mitigate this is to make sure that the driver knows
what changed and only updates these things when a full reprobe is
requested. Is this the approach that you have in mind?
The methodology behind my series follows what is currently done: detect
change in whatever way necessary, inform userspace and let it trigger
full reprobe. If I'm understanding correctly, what you're suggesting is
instead to reprobe what is needed on the kernel side when an associated
change occurs instead of having userspace trigger it, and then let
userspace aware that something changed and return the "cached" updated
status when userspace asks for the subsequent reprobe. Is that correct?
Cheers,
Paul
> > > > ---
> > > > drivers/gpu/drm/drm_edid.c | 45
> > > > +++++++++++++++++++++++++++++++++++++++++++++
> > > > include/drm/drm_edid.h | 3 +++
> > > > 2 files changed, 48 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_edid.c
> > > > b/drivers/gpu/drm/drm_edid.c
> > > > index 6bb6337be920..f6ce8bc2907a 100644
> > > > --- a/drivers/gpu/drm/drm_edid.c
> > > > +++ b/drivers/gpu/drm/drm_edid.c
> > > > @@ -5036,3 +5036,48 @@ static void drm_get_displayid(struct
> > > > drm_connector *connector,
> > > > }
> > > > return;
> > > > }
> > > > +
> > > > +/**
> > > > + * drm_check_edid_changed - Check whether the EDID changed
> > > > since
> > > > the last update
> > > > + * @connector: connector we're probing
> > > > + * @adapter: I2C adapter to use for DDC
> > > > + *
> > > > + * Check whether the EDID changed since the last time it was
> > > > updated in the
> > > > + * drm blob cache.
> > > > + *
> > > > + * Return: A boolean indicating whether a change happened or
> > > > not.
> > > > + */
> > > > +bool drm_check_edid_changed(struct drm_connector *connector,
> > > > + struct i2c_adapter *adapter)
> > > > +{
> > > > + struct drm_property_blob *edid_blob;
> > > > + struct edid *edid_stored;
> > > > + struct edid *edid_read;
> > > > + int ret = 0;
> > > > +
> > > > + if (!connector->edid_blob_ptr)
> > > > + return false;
> > > > +
> > > > + edid_blob = drm_property_blob_get(connector-
> > > > > edid_blob_ptr);
> > > >
> > > > + if (!edid_blob)
> > > > + return false;
> > > > +
> > > > + if (!edid_blob->data || edid_blob->length != sizeof(struct
> > > > edid))
> > > > + goto out;
> > > > +
> > > > + edid_stored = (struct edid *) edid_blob->data;
> > > > +
> > > > + edid_read = drm_get_edid(connector, adapter);
> > > > + if (!edid_read)
> > > > + goto out;
> > > > +
> > > > + ret = memcmp(edid_stored, edid_read, sizeof(struct edid));
> > > > +
> > > > + kfree(edid_read);
> > > > +
> > > > +out:
> > > > + drm_property_blob_put(edid_blob);
> > > > +
> > > > + return ret != 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(drm_check_edid_changed);
> > > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > > > index 1e1908a6b1d6..593a97b269c3 100644
> > > > --- a/include/drm/drm_edid.h
> > > > +++ b/include/drm/drm_edid.h
> > > > @@ -485,4 +485,7 @@ void drm_edid_get_monitor_name(struct edid
> > > > *edid, char *name,
> > > > struct drm_display_mode *drm_mode_find_dmt(struct drm_device
> > > > *dev,
> > > > int hsize, int vsize,
> > > > int fresh,
> > > > bool rb);
> > > > +bool drm_check_edid_changed(struct drm_connector *connector,
> > > > + struct i2c_adapter *adapter);
> > > > +
> > > > #endif /* __DRM_EDID_H__ */
> > > > --
> > > > 2.13.2
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > [email protected]
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> >
> > --
> > Paul Kocialkowski <[email protected]>
> > Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo,
> > Finland
>
>
>
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
On Tue, Jul 25, 2017 at 10:58:55AM +0300, Paul Kocialkowski wrote:
> On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> > On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
> > <[email protected]> wrote:
> > > On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> > > > On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski wrote:
> > > > > This adds a common drm helper to detect whether the EDID changed
> > > > > from
> > > > > the last known cached one. This is useful help detect that a
> > > > > monitor
> > > > > was
> > > > > changed during a suspend/resume cycle.
> > > > >
> > > > > When that happens (a monitor is replaced by another one during
> > > > > suspend),
> > > > > no hotplug event will be triggered so the change will not be
> > > > > caught
> > > > > at
> > > > > resume time. Detecting that the EDID changed allows detecting
> > > > > it.
> > > > >
> > > > > Signed-off-by: Paul Kocialkowski <[email protected].
> > > > > com>
> > > >
> > > > I can't find the older mails I've typed about this, but the plan
> > > > we've
> > > > discussed a while back was:
> > > > - Add a generational counter to each connector, maybe even expose
> > > > it
> > > > to
> > > > userspace.
> > > >
> > > > - Increment that counter every time something changed, e.g.
> > > > connector->status in the propbe helpers, or when attaching a new
> > > > edid
> > > > with the set_edid helper.
> > > >
> > > > - Tada, no changes needed to drivers, and easily extensible to
> > > > other
> > > > things than edid!
> > >
> > > I don't see how it solves the problem here though. After a
> > > suspend/resume cycle, there is simply no indication that anything
> > > has
> > > changed when a monitor was replaced by another one, so I don't see
> > > how
> > > adding a counter in the mix would help.
> > >
> > > Could you provide more details about the reasoning? I feel like I'm
> > > missing something here.
> >
> > Your bug doesn't just exist over s/r, it's just much easier to observe
> > in s/r since users can take however long they want to with plugging in
> > a different monitor. But the same issue exists e.g. when we go from
> > hpd to polling because too much noise on the line.
> >
> > Wrt the suspend/resume issue: What we need to do on resume is do a
> > full reprobe of all outputs, in an async worker. Telling userspace to
> > do this by sending an uevent was the cheapest way, but it'd be better
> > if the kernel could do that asynchronously and inform userspace about
> > the exact changes. And there's more to reprobe than just the edid, and
> > we don't want to re-invent a separate reprobe path just for resume
> > like you start in your patch series. So yeah my plan was missing:
> >
> > - force a full async reprobe after resume (maybe we could reuse the
> > poll worker for that as a one-shot).
>
> First off, I definitely agree we need a way to tell userspace exactly
> what has happened. I wanted to start a discussion about that in i-g-t
> patch "Unrelated hotplug uevent masking out actual test result" but it
> didn't get much traction. For testing purposes, it is unacceptable that
> userspace only gets notified that "something happened".
>
> Still, as far as I know, userspace is expected to ask for a full reprobe
> when something has changed, and that is apparently part of the DRM spec,
> so we can't expect that it could query for an update on "only the things
> that changed".
We can update that spec in a backwards compatible way. E.g. we can ask for
the current properties without forcing a reprobe (won't even call down
into the driver), and userspace could use that to check which connector
has an incremented epoche counter since the last time it sampled things.
Then it can reprobe just that one.
Old userspace wouldn't know about this, and would keep working as-is.
> However, one way to mitigate this is to make sure that the driver knows
> what changed and only updates these things when a full reprobe is
> requested. Is this the approach that you have in mind?
>
> The methodology behind my series follows what is currently done: detect
> change in whatever way necessary, inform userspace and let it trigger
> full reprobe. If I'm understanding correctly, what you're suggesting is
> instead to reprobe what is needed on the kernel side when an associated
> change occurs instead of having userspace trigger it, and then let
> userspace aware that something changed and return the "cached" updated
> status when userspace asks for the subsequent reprobe. Is that correct?
There's two things: the uapi discussion and the internal implementation,
imo their separate (but somewhat connected) topics.
- For the internal implementation of detecting edid changes I don't like
your approach of rolling a completely new detect path just for resume.
I think we can very well integrate that into the existing probe code
using the approach I've laid out.
- There's more than just edid (e.g. hdcp status, various stuff that's
handled in dp aux for DP sinks), and I think a general mechanism for
tracking that something changed will be useful for the internal
implementation. The other plan would be that we have to wire a bool
changed through the entire probe stack, and make sure it's handled
correctly everywhere, which is a) a lot more work b) more fragile. Doing
a connector->status_epoch++ everywhere we detect a change is _much_
simpler.
- For the uapi change: We already support returning the cached stuff, the
only bit that's missing is the epoch counter to let userspace know where
it might need to do a full reprobe. Or maybe we'll just spec that a full
reprobe isn't necessary after a hpd event (but that's unlikely to work
out given how many bugs we'd need to fix first).
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, 2017-07-25 at 10:16 +0200, Daniel Vetter wrote:
> On Tue, Jul 25, 2017 at 10:58:55AM +0300, Paul Kocialkowski wrote:
> > On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> > > On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
> > > <[email protected]> wrote:
> > > > On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> > > > > On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski
> > > > > wrote:
> > > > > > This adds a common drm helper to detect whether the EDID
> > > > > > changed
> > > > > > from
> > > > > > the last known cached one. This is useful help detect that a
> > > > > > monitor
> > > > > > was
> > > > > > changed during a suspend/resume cycle.
> > > > > >
> > > > > > When that happens (a monitor is replaced by another one
> > > > > > during
> > > > > > suspend),
> > > > > > no hotplug event will be triggered so the change will not be
> > > > > > caught
> > > > > > at
> > > > > > resume time. Detecting that the EDID changed allows
> > > > > > detecting
> > > > > > it.
> > > > > >
> > > > > > Signed-off-by: Paul Kocialkowski <[email protected]
> > > > > > tel.
> > > > > > com>
> > > > >
> > > > > I can't find the older mails I've typed about this, but the
> > > > > plan
> > > > > we've
> > > > > discussed a while back was:
> > > > > - Add a generational counter to each connector, maybe even
> > > > > expose
> > > > > it
> > > > > to
> > > > > userspace.
> > > > >
> > > > > - Increment that counter every time something changed, e.g.
> > > > > connector->status in the propbe helpers, or when attaching a
> > > > > new
> > > > > edid
> > > > > with the set_edid helper.
> > > > >
> > > > > - Tada, no changes needed to drivers, and easily extensible to
> > > > > other
> > > > > things than edid!
> > > >
> > > > I don't see how it solves the problem here though. After a
> > > > suspend/resume cycle, there is simply no indication that
> > > > anything
> > > > has
> > > > changed when a monitor was replaced by another one, so I don't
> > > > see
> > > > how
> > > > adding a counter in the mix would help.
> > > >
> > > > Could you provide more details about the reasoning? I feel like
> > > > I'm
> > > > missing something here.
> > >
> > > Your bug doesn't just exist over s/r, it's just much easier to
> > > observe
> > > in s/r since users can take however long they want to with
> > > plugging in
> > > a different monitor. But the same issue exists e.g. when we go
> > > from
> > > hpd to polling because too much noise on the line.
> > >
> > > Wrt the suspend/resume issue: What we need to do on resume is do a
> > > full reprobe of all outputs, in an async worker. Telling userspace
> > > to
> > > do this by sending an uevent was the cheapest way, but it'd be
> > > better
> > > if the kernel could do that asynchronously and inform userspace
> > > about
> > > the exact changes. And there's more to reprobe than just the edid,
> > > and
> > > we don't want to re-invent a separate reprobe path just for resume
> > > like you start in your patch series. So yeah my plan was missing:
> > >
> > > - force a full async reprobe after resume (maybe we could reuse
> > > the
> > > poll worker for that as a one-shot).
> >
> > First off, I definitely agree we need a way to tell userspace
> > exactly
> > what has happened. I wanted to start a discussion about that in i-g-
> > t
> > patch "Unrelated hotplug uevent masking out actual test result" but
> > it
> > didn't get much traction. For testing purposes, it is unacceptable
> > that
> > userspace only gets notified that "something happened".
> >
> > Still, as far as I know, userspace is expected to ask for a full
> > reprobe
> > when something has changed, and that is apparently part of the DRM
> > spec,
> > so we can't expect that it could query for an update on "only the
> > things
> > that changed".
>
> We can update that spec in a backwards compatible way. E.g. we can ask
> for
> the current properties without forcing a reprobe (won't even call down
> into the driver), and userspace could use that to check which
> connector
> has an incremented epoche counter since the last time it sampled
> things.
> Then it can reprobe just that one.
>
> Old userspace wouldn't know about this, and would keep working as-is.
So the level of detail you're aiming at providing userspace is
"connector foo changed" then? I agree it is better than the current
"some connector(s) changed", but what I'd like to see for proper testing
is a way to find out "bar for connector foo changed".
> > However, one way to mitigate this is to make sure that the driver
> > knows
> > what changed and only updates these things when a full reprobe is
> > requested. Is this the approach that you have in mind?
> >
> > The methodology behind my series follows what is currently done:
> > detect
> > change in whatever way necessary, inform userspace and let it
> > trigger
> > full reprobe. If I'm understanding correctly, what you're suggesting
> > is
> > instead to reprobe what is needed on the kernel side when an
> > associated
> > change occurs instead of having userspace trigger it, and then let
> > userspace aware that something changed and return the "cached"
> > updated
> > status when userspace asks for the subsequent reprobe. Is that
> > correct?
>
> There's two things: the uapi discussion and the internal
> implementation,
> imo their separate (but somewhat connected) topics.
>
> - For the internal implementation of detecting edid changes I don't
> like
> your approach of rolling a completely new detect path just for
> resume.
> I think we can very well integrate that into the existing probe code
> using the approach I've laid out.
>
> - There's more than just edid (e.g. hdcp status, various stuff that's
> handled in dp aux for DP sinks), and I think a general mechanism for
> tracking that something changed will be useful for the internal
> implementation. The other plan would be that we have to wire a bool
> changed through the entire probe stack, and make sure it's handled
> correctly everywhere, which is a) a lot more work b) more fragile.
> Doing
> a connector->status_epoch++ everywhere we detect a change is _much_
> simpler.
So to summarize, the following would happen: an async worker would
detect whether something changed, then increase the counter for that
connector and notify userspace, which would trigger full reprobe of that
connector only. Legacy userspace would just trigger full reprobe for all
connectors.
I am still under the impression that you'd like the full reprobe to be
done on the kernel's async worker, to detect that e.g. EDID changed. But
then userspace is going to fully reprobe again, so it will be
duplicated. Unless the kernel also keeps a reference of the last time
the counter was read from userspace, to determine when to skip full
reprobe when it is asked from userspace? That feels pretty similar to
having a bool indicating change.
My approach here was to look specifically for the thing that can change
in the async worker (only EDID with this change, but it could be
extended for the other things you mentioned) as to reduce the
duplication as much as possible.
> - For the uapi change: We already support returning the cached stuff,
> the
> only bit that's missing is the epoch counter to let userspace know
> where
> it might need to do a full reprobe. Or maybe we'll just spec that a
> full
> reprobe isn't necessary after a hpd event (but that's unlikely to
> work
> out given how many bugs we'd need to fix first).
Okay, thanks for the additional explanation. I think I'm getting a
better grasp on your idea.
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
On Tue, Jul 25, 2017 at 03:18:04PM +0300, Paul Kocialkowski wrote:
> On Tue, 2017-07-25 at 10:16 +0200, Daniel Vetter wrote:
> > On Tue, Jul 25, 2017 at 10:58:55AM +0300, Paul Kocialkowski wrote:
> > > On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> > > > On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
> > > > <[email protected]> wrote:
> > > > > On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> > > > > > On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul Kocialkowski
> > > > > > wrote:
> > > > > > > This adds a common drm helper to detect whether the EDID
> > > > > > > changed
> > > > > > > from
> > > > > > > the last known cached one. This is useful help detect that a
> > > > > > > monitor
> > > > > > > was
> > > > > > > changed during a suspend/resume cycle.
> > > > > > >
> > > > > > > When that happens (a monitor is replaced by another one
> > > > > > > during
> > > > > > > suspend),
> > > > > > > no hotplug event will be triggered so the change will not be
> > > > > > > caught
> > > > > > > at
> > > > > > > resume time. Detecting that the EDID changed allows
> > > > > > > detecting
> > > > > > > it.
> > > > > > >
> > > > > > > Signed-off-by: Paul Kocialkowski <[email protected]
> > > > > > > tel.
> > > > > > > com>
> > > > > >
> > > > > > I can't find the older mails I've typed about this, but the
> > > > > > plan
> > > > > > we've
> > > > > > discussed a while back was:
> > > > > > - Add a generational counter to each connector, maybe even
> > > > > > expose
> > > > > > it
> > > > > > to
> > > > > > userspace.
> > > > > >
> > > > > > - Increment that counter every time something changed, e.g.
> > > > > > connector->status in the propbe helpers, or when attaching a
> > > > > > new
> > > > > > edid
> > > > > > with the set_edid helper.
> > > > > >
> > > > > > - Tada, no changes needed to drivers, and easily extensible to
> > > > > > other
> > > > > > things than edid!
> > > > >
> > > > > I don't see how it solves the problem here though. After a
> > > > > suspend/resume cycle, there is simply no indication that
> > > > > anything
> > > > > has
> > > > > changed when a monitor was replaced by another one, so I don't
> > > > > see
> > > > > how
> > > > > adding a counter in the mix would help.
> > > > >
> > > > > Could you provide more details about the reasoning? I feel like
> > > > > I'm
> > > > > missing something here.
> > > >
> > > > Your bug doesn't just exist over s/r, it's just much easier to
> > > > observe
> > > > in s/r since users can take however long they want to with
> > > > plugging in
> > > > a different monitor. But the same issue exists e.g. when we go
> > > > from
> > > > hpd to polling because too much noise on the line.
> > > >
> > > > Wrt the suspend/resume issue: What we need to do on resume is do a
> > > > full reprobe of all outputs, in an async worker. Telling userspace
> > > > to
> > > > do this by sending an uevent was the cheapest way, but it'd be
> > > > better
> > > > if the kernel could do that asynchronously and inform userspace
> > > > about
> > > > the exact changes. And there's more to reprobe than just the edid,
> > > > and
> > > > we don't want to re-invent a separate reprobe path just for resume
> > > > like you start in your patch series. So yeah my plan was missing:
> > > >
> > > > - force a full async reprobe after resume (maybe we could reuse
> > > > the
> > > > poll worker for that as a one-shot).
> > >
> > > First off, I definitely agree we need a way to tell userspace
> > > exactly
> > > what has happened. I wanted to start a discussion about that in i-g-
> > > t
> > > patch "Unrelated hotplug uevent masking out actual test result" but
> > > it
> > > didn't get much traction. For testing purposes, it is unacceptable
> > > that
> > > userspace only gets notified that "something happened".
> > >
> > > Still, as far as I know, userspace is expected to ask for a full
> > > reprobe
> > > when something has changed, and that is apparently part of the DRM
> > > spec,
> > > so we can't expect that it could query for an update on "only the
> > > things
> > > that changed".
> >
> > We can update that spec in a backwards compatible way. E.g. we can ask
> > for
> > the current properties without forcing a reprobe (won't even call down
> > into the driver), and userspace could use that to check which
> > connector
> > has an incremented epoche counter since the last time it sampled
> > things.
> > Then it can reprobe just that one.
> >
> > Old userspace wouldn't know about this, and would keep working as-is.
>
> So the level of detail you're aiming at providing userspace is
> "connector foo changed" then? I agree it is better than the current
> "some connector(s) changed", but what I'd like to see for proper testing
> is a way to find out "bar for connector foo changed".
If you want taht level of detail you need introspection in a in-kernel
selftest I think. We'd need to rather massively change/extend the uapi to
support that level of testing through the uapi, and thus far no one else
is asking for it with a real use-case.
> > > However, one way to mitigate this is to make sure that the driver
> > > knows
> > > what changed and only updates these things when a full reprobe is
> > > requested. Is this the approach that you have in mind?
> > >
> > > The methodology behind my series follows what is currently done:
> > > detect
> > > change in whatever way necessary, inform userspace and let it
> > > trigger
> > > full reprobe. If I'm understanding correctly, what you're suggesting
> > > is
> > > instead to reprobe what is needed on the kernel side when an
> > > associated
> > > change occurs instead of having userspace trigger it, and then let
> > > userspace aware that something changed and return the "cached"
> > > updated
> > > status when userspace asks for the subsequent reprobe. Is that
> > > correct?
> >
> > There's two things: the uapi discussion and the internal
> > implementation,
> > imo their separate (but somewhat connected) topics.
> >
> > - For the internal implementation of detecting edid changes I don't
> > like
> > your approach of rolling a completely new detect path just for
> > resume.
> > I think we can very well integrate that into the existing probe code
> > using the approach I've laid out.
> >
> > - There's more than just edid (e.g. hdcp status, various stuff that's
> > handled in dp aux for DP sinks), and I think a general mechanism for
> > tracking that something changed will be useful for the internal
> > implementation. The other plan would be that we have to wire a bool
> > changed through the entire probe stack, and make sure it's handled
> > correctly everywhere, which is a) a lot more work b) more fragile.
> > Doing
> > a connector->status_epoch++ everywhere we detect a change is _much_
> > simpler.
>
> So to summarize, the following would happen: an async worker would
> detect whether something changed, then increase the counter for that
> connector and notify userspace, which would trigger full reprobe of that
> connector only. Legacy userspace would just trigger full reprobe for all
> connectors.
>
> I am still under the impression that you'd like the full reprobe to be
> done on the kernel's async worker, to detect that e.g. EDID changed. But
> then userspace is going to fully reprobe again, so it will be
> duplicated. Unless the kernel also keeps a reference of the last time
> the counter was read from userspace, to determine when to skip full
> reprobe when it is asked from userspace? That feels pretty similar to
> having a bool indicating change.
>
> My approach here was to look specifically for the thing that can change
> in the async worker (only EDID with this change, but it could be
> extended for the other things you mentioned) as to reduce the
> duplication as much as possible.
>
> > - For the uapi change: We already support returning the cached stuff,
> > the
> > only bit that's missing is the epoch counter to let userspace know
> > where
> > it might need to do a full reprobe. Or maybe we'll just spec that a
> > full
> > reprobe isn't necessary after a hpd event (but that's unlikely to
> > work
> > out given how many bugs we'd need to fix first).
>
> Okay, thanks for the additional explanation. I think I'm getting a
> better grasp on your idea.
Yeah, the full reprobe isn't needed if you we spec that as the new fancy
behaviour. We already support getting the cached values through
drmModeGetConnectorCurrent(), without forcing a full reprobe.
Note that really the only thing the hpd handling doesn't do is fill and
filter the mode list. I guess as part of this work we should fix that,
that should take care of most of the needs for full reprobing.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, 2017-07-25 at 17:50 +0200, Daniel Vetter wrote:
> On Tue, Jul 25, 2017 at 03:18:04PM +0300, Paul Kocialkowski wrote:
> > On Tue, 2017-07-25 at 10:16 +0200, Daniel Vetter wrote:
> > > On Tue, Jul 25, 2017 at 10:58:55AM +0300, Paul Kocialkowski wrote:
> > > > On Tue, 2017-07-25 at 09:34 +0200, Daniel Vetter wrote:
> > > > > On Tue, Jul 25, 2017 at 9:25 AM, Paul Kocialkowski
> > > > > <[email protected]> wrote:
> > > > > > On Tue, 2017-07-25 at 08:53 +0200, Daniel Vetter wrote:
> > > > > > > On Mon, Jul 24, 2017 at 05:54:46PM +0300, Paul
> > > > > > > Kocialkowski
> > > > > > > wrote:
> > > > > > > > This adds a common drm helper to detect whether the EDID
> > > > > > > > changed
> > > > > > > > from
> > > > > > > > the last known cached one. This is useful help detect
> > > > > > > > that a
> > > > > > > > monitor
> > > > > > > > was
> > > > > > > > changed during a suspend/resume cycle.
> > > > > > > >
> > > > > > > > When that happens (a monitor is replaced by another one
> > > > > > > > during
> > > > > > > > suspend),
> > > > > > > > no hotplug event will be triggered so the change will
> > > > > > > > not be
> > > > > > > > caught
> > > > > > > > at
> > > > > > > > resume time. Detecting that the EDID changed allows
> > > > > > > > detecting
> > > > > > > > it.
> > > > > > > >
> > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linu
> > > > > > > > x.in
> > > > > > > > tel.
> > > > > > > > com>
> > > > > > >
> > > > > > > I can't find the older mails I've typed about this, but
> > > > > > > the
> > > > > > > plan
> > > > > > > we've
> > > > > > > discussed a while back was:
> > > > > > > - Add a generational counter to each connector, maybe even
> > > > > > > expose
> > > > > > > it
> > > > > > > to
> > > > > > > userspace.
> > > > > > >
> > > > > > > - Increment that counter every time something changed,
> > > > > > > e.g.
> > > > > > > connector->status in the propbe helpers, or when
> > > > > > > attaching a
> > > > > > > new
> > > > > > > edid
> > > > > > > with the set_edid helper.
> > > > > > >
> > > > > > > - Tada, no changes needed to drivers, and easily
> > > > > > > extensible to
> > > > > > > other
> > > > > > > things than edid!
> > > > > >
> > > > > > I don't see how it solves the problem here though. After a
> > > > > > suspend/resume cycle, there is simply no indication that
> > > > > > anything
> > > > > > has
> > > > > > changed when a monitor was replaced by another one, so I
> > > > > > don't
> > > > > > see
> > > > > > how
> > > > > > adding a counter in the mix would help.
> > > > > >
> > > > > > Could you provide more details about the reasoning? I feel
> > > > > > like
> > > > > > I'm
> > > > > > missing something here.
> > > > >
> > > > > Your bug doesn't just exist over s/r, it's just much easier to
> > > > > observe
> > > > > in s/r since users can take however long they want to with
> > > > > plugging in
> > > > > a different monitor. But the same issue exists e.g. when we go
> > > > > from
> > > > > hpd to polling because too much noise on the line.
> > > > >
> > > > > Wrt the suspend/resume issue: What we need to do on resume is
> > > > > do a
> > > > > full reprobe of all outputs, in an async worker. Telling
> > > > > userspace
> > > > > to
> > > > > do this by sending an uevent was the cheapest way, but it'd be
> > > > > better
> > > > > if the kernel could do that asynchronously and inform
> > > > > userspace
> > > > > about
> > > > > the exact changes. And there's more to reprobe than just the
> > > > > edid,
> > > > > and
> > > > > we don't want to re-invent a separate reprobe path just for
> > > > > resume
> > > > > like you start in your patch series. So yeah my plan was
> > > > > missing:
> > > > >
> > > > > - force a full async reprobe after resume (maybe we could
> > > > > reuse
> > > > > the
> > > > > poll worker for that as a one-shot).
> > > >
> > > > First off, I definitely agree we need a way to tell userspace
> > > > exactly
> > > > what has happened. I wanted to start a discussion about that in
> > > > i-g-
> > > > t
> > > > patch "Unrelated hotplug uevent masking out actual test result"
> > > > but
> > > > it
> > > > didn't get much traction. For testing purposes, it is
> > > > unacceptable
> > > > that
> > > > userspace only gets notified that "something happened".
> > > >
> > > > Still, as far as I know, userspace is expected to ask for a full
> > > > reprobe
> > > > when something has changed, and that is apparently part of the
> > > > DRM
> > > > spec,
> > > > so we can't expect that it could query for an update on "only
> > > > the
> > > > things
> > > > that changed".
> > >
> > > We can update that spec in a backwards compatible way. E.g. we can
> > > ask
> > > for
> > > the current properties without forcing a reprobe (won't even call
> > > down
> > > into the driver), and userspace could use that to check which
> > > connector
> > > has an incremented epoche counter since the last time it sampled
> > > things.
> > > Then it can reprobe just that one.
> > >
> > > Old userspace wouldn't know about this, and would keep working as-
> > > is.
> >
> > So the level of detail you're aiming at providing userspace is
> > "connector foo changed" then? I agree it is better than the current
> > "some connector(s) changed", but what I'd like to see for proper
> > testing
> > is a way to find out "bar for connector foo changed".
>
> If you want taht level of detail you need introspection in a in-kernel
> selftest I think. We'd need to rather massively change/extend the uapi
> to
> support that level of testing through the uapi, and thus far no one
> else
> is asking for it with a real use-case.
I'm frankly surprised that no one has complained about this yet!
The current level of (lack of) detail about what is happening when a
hotplug event is triggered makes testing for various things really hard.
But I suppose it makes sense with the "reprobe everything all the time"
approach ;)
For instance, it wouldn't hurt to add a REASON=foo field to the uevent,
in addition to HOTPLUG=1, which would most certainly maintain
compatibility with current userspace. I'm not sure that would work out
too well if uevents are to be stacked together though.
> > > > However, one way to mitigate this is to make sure that the
> > > > driver
> > > > knows
> > > > what changed and only updates these things when a full reprobe
> > > > is
> > > > requested. Is this the approach that you have in mind?
> > > >
> > > > The methodology behind my series follows what is currently done:
> > > > detect
> > > > change in whatever way necessary, inform userspace and let it
> > > > trigger
> > > > full reprobe. If I'm understanding correctly, what you're
> > > > suggesting
> > > > is
> > > > instead to reprobe what is needed on the kernel side when an
> > > > associated
> > > > change occurs instead of having userspace trigger it, and then
> > > > let
> > > > userspace aware that something changed and return the "cached"
> > > > updated
> > > > status when userspace asks for the subsequent reprobe. Is that
> > > > correct?
> > >
> > > There's two things: the uapi discussion and the internal
> > > implementation,
> > > imo their separate (but somewhat connected) topics.
> > >
> > > - For the internal implementation of detecting edid changes I
> > > don't
> > > like
> > > your approach of rolling a completely new detect path just for
> > > resume.
> > > I think we can very well integrate that into the existing probe
> > > code
> > > using the approach I've laid out.
> > >
> > > - There's more than just edid (e.g. hdcp status, various stuff
> > > that's
> > > handled in dp aux for DP sinks), and I think a general mechanism
> > > for
> > > tracking that something changed will be useful for the internal
> > > implementation. The other plan would be that we have to wire a
> > > bool
> > > changed through the entire probe stack, and make sure it's
> > > handled
> > > correctly everywhere, which is a) a lot more work b) more
> > > fragile.
> > > Doing
> > > a connector->status_epoch++ everywhere we detect a change is
> > > _much_
> > > simpler.
> >
> > So to summarize, the following would happen: an async worker would
> > detect whether something changed, then increase the counter for that
> > connector and notify userspace, which would trigger full reprobe of
> > that
> > connector only. Legacy userspace would just trigger full reprobe for
> > all
> > connectors.
> >
> > I am still under the impression that you'd like the full reprobe to
> > be
> > done on the kernel's async worker, to detect that e.g. EDID changed.
> > But
> > then userspace is going to fully reprobe again, so it will be
> > duplicated. Unless the kernel also keeps a reference of the last
> > time
> > the counter was read from userspace, to determine when to skip full
> > reprobe when it is asked from userspace? That feels pretty similar
> > to
> > having a bool indicating change.
> >
> > My approach here was to look specifically for the thing that can
> > change
> > in the async worker (only EDID with this change, but it could be
> > extended for the other things you mentioned) as to reduce the
> > duplication as much as possible.
> >
> > > - For the uapi change: We already support returning the cached
> > > stuff,
> > > the
> > > only bit that's missing is the epoch counter to let userspace
> > > know
> > > where
> > > it might need to do a full reprobe. Or maybe we'll just spec
> > > that a
> > > full
> > > reprobe isn't necessary after a hpd event (but that's unlikely
> > > to
> > > work
> > > out given how many bugs we'd need to fix first).
> >
> > Okay, thanks for the additional explanation. I think I'm getting a
> > better grasp on your idea.
>
> Yeah, the full reprobe isn't needed if you we spec that as the new
> fancy
> behaviour. We already support getting the cached values through
> drmModeGetConnectorCurrent(), without forcing a full reprobe.
So then userspace gets the cached values and considers them as "already
updated" if the counter has increased, and then stops there and doesn't
trigger a full reprobe (which was already done by the kernel's worker)?
So all in all, the win here is reprobing only 1 connector instead of
reprobing them all. The same could be achieved via a more detail hotplug
notification that also mentions which connector was concerned by the
event.
> Note that really the only thing the hpd handling doesn't do is fill
> and
> filter the mode list. I guess as part of this work we should fix that,
> that should take care of most of the needs for full reprobing.
Either way, I do not have enough time left in my internship to start
working on something as big as this change, but I think it's a step in
the right direction.
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland