Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Gabriel Krisman Bertazi]

Ville Syrjälä  writes:


>>  mutex_lock(>mode_config.mutex);
>>  DRM_DEBUG_KMS("running encoder hotplug functions\n");
>> @@ -339,13 +340,18 @@ static void i915_hotplug_work_func(struct work_struct 
>> *work)
>>  if (!intel_connector->encoder)
>>  continue;
>>  intel_encoder = intel_connector->encoder;
>> -if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
>> +enc_hpd_pin_mask = (1 << intel_encoder->hpd_pin);
>> +if (hpd_event_bits & enc_hpd_pin_mask) {
>>  DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug 
>> event.\n",
>>connector->name, intel_encoder->hpd_pin);
>>  if (intel_encoder->hot_plug)
>>  intel_encoder->hot_plug(intel_encoder);
>> -if (intel_hpd_irq_event(dev, connector))
>> +if (intel_hpd_irq_event(dev, connector)) {
>>  changed = true;
>> +if (connector->status ==
>> +connector_status_connected)
>> +hpd_event_bits &= ~(enc_hpd_pin_mask);
>
> This won't work correctly because now it will fail to update
> connector->status for the other connector. I admit that being fast
> enough to switch between DP<->HDMI directly while the HPD interrupt
> is enabled is perhaps a little unlikely, but it can certainly happen
> very easily if the interrupt has been disabled.

Hi Ville,

I see.  So, still racy when dealing with multiple connectors events on
the same HPD line.  Nice catch. I didn't see that.

> We alrady have other bugs in that area. Eg. if you manage to switch to a
> totally different display without noticing the disconnected state in
> between we'll skip sending the uevent. We should really be checking if
> the sink changed somehow (eg. EDID changed) and in that case send the
> uevent anyway.
>
> I also don't think this can have anything to do with vblank tests
> failing, unless there's some other problem involved that somehow
> triggers HPDs while the test is doing its measurements.

I thought about (blindly) reducing the overhead of
i915_hotplug_work_func, as mentioned by Chris in his second comment in
the report for bug 100215.  But I understand that the latest
reproduction of 100215 triggered a lot of timeouts directly from
userspace, and I didn't find any HPD occurrences there.

Anyway, correct me if I'm wrong, but I still understand that the amount
of time the DP detection takes is a little too much, and the fact that
it triggers whenever we connect anything to a shared port is very odd
behavior.  There are a few bugs reported where the DP detection timeouts
was pointed as the probable root cause.

I came up with two patches to workaround the issue but they both had
issues of their own.  I understand the first version I submitted is a
better approach than the second, and I think we could explore something
in that direction, once we address the race condition first pointed out
by Chris.  He suggested on an earlier email that we leverage the
knowledge about digital ports to improve the overall detection, but I'm
not sure how to start that.  I've been checking the specs and I don't
see how we could decide whether we have a DP device without doing the
dpcp dance.  Do you have any suggestions on that?  I'm new to i915, so
I'm not sure if I'm missing something, if that is the case, please point
it out. :)

-- 
Gabriel Krisman Bertazi
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Ville Syrjälä]

On Sun, May 28, 2017 at 09:53:19PM -0300, Gabriel Krisman Bertazi wrote:
> Chris Wilson  writes:
> 
> > The key problem here is say a race between DP unplug and HDMI plug, and
> > users are evil enough (or common enough) for it to happen.
> >
> > I thought the idea was reasonable though, and perhaps we could make
> > more use of the knowlege of the shared ports to improve detection of
> > common DP/HDMI DDIs (i.e. run detection once for a ddi and have it
> > decide whether it is hdmi or dp).
> 
> Hi Chris and Manasi, thanks for the feedback and sorry for the late
> response.
> 
> I see your point about being racy.  I think this is not an issue in my
> system since it appears that the disconnect of the first connector
> triggers another scheduling of the hotplug_work, that further detects
> the other connector.
> 
> I'm still working on this, but can you provide more details on your
> suggestion about using more knowledge of the shared ports to improve the
> detection of DDIs?  I still don't see how we could prevent this race
> once and for all.
> 
> In the mean time, I wrote another patch with a different way to reduce
> the overhead of attempting to detect connectors of shared ports already
> in use.  My understanding is that this version is no longer racy but it
> doesn't catch every condition where we could avoid detection.  Though,
> it feels like something we want to get upstream.  Can you please take a
> look and let me know?
> 
> I think this also addresses the cloned encoders issue raised by Manasi,
> if I understood it correctly.
> 
> >8
> Subject: [PATCH] drm: i915: Stop further detection on shared pins if a sink is
>  connected
> 
> HPD pins may be shared among connectors but, according to documentation,
> only one connector can be enebled at any given time.  This avoids
> waisting time with attempting to detect further connectors on the same
> pin if the first one was already detected.
> 
> Since this reduces the overhead of the i915_hotplug_work_func, it may
> address the following vblank misses detected by the CI:
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100215
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100558
> 
> Signed-off-by: Gabriel Krisman Bertazi 
> ---
>  drivers/gpu/drm/i915/intel_hotplug.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
> b/drivers/gpu/drm/i915/intel_hotplug.c
> index f1200272a699..913e8523b89d 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -319,6 +319,7 @@ static void i915_hotplug_work_func(struct work_struct 
> *work)
>   struct drm_connector_list_iter conn_iter;
>   bool changed = false;
>   u32 hpd_event_bits;
> + u32 enc_hpd_pin_mask;
>  
>   mutex_lock(>mode_config.mutex);
>   DRM_DEBUG_KMS("running encoder hotplug functions\n");
> @@ -339,13 +340,18 @@ static void i915_hotplug_work_func(struct work_struct 
> *work)
>   if (!intel_connector->encoder)
>   continue;
>   intel_encoder = intel_connector->encoder;
> - if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
> + enc_hpd_pin_mask = (1 << intel_encoder->hpd_pin);
> + if (hpd_event_bits & enc_hpd_pin_mask) {
>   DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug 
> event.\n",
> connector->name, intel_encoder->hpd_pin);
>   if (intel_encoder->hot_plug)
>   intel_encoder->hot_plug(intel_encoder);
> - if (intel_hpd_irq_event(dev, connector))
> + if (intel_hpd_irq_event(dev, connector)) {
>   changed = true;
> + if (connector->status ==
> + connector_status_connected)
> + hpd_event_bits &= ~(enc_hpd_pin_mask);

This won't work correctly because now it will fail to update
connector->status for the other connector. I admit that being fast
enough to switch between DP<->HDMI directly while the HPD interrupt
is enabled is perhaps a little unlikely, but it can certainly happen
very easily if the interrupt has been disabled.

We alrady have other bugs in that area. Eg. if you manage to switch to a
totally different display without noticing the disconnected state in
between we'll skip sending the uevent. We should really be checking if
the sink changed somehow (eg. EDID changed) and in that case send the
uevent anyway.

I also don't think this can have anything to do with vblank tests
failing, unless there's some other problem involved that somehow
triggers HPDs while the test is doing its measurements.

> + }
>   }
>   }
>   drm_connector_list_iter_end(_iter);
> -- 
> 2.11.0
> 
> 

Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Gabriel Krisman Bertazi]

Chris Wilson  writes:

> The key problem here is say a race between DP unplug and HDMI plug, and
> users are evil enough (or common enough) for it to happen.
>
> I thought the idea was reasonable though, and perhaps we could make
> more use of the knowlege of the shared ports to improve detection of
> common DP/HDMI DDIs (i.e. run detection once for a ddi and have it
> decide whether it is hdmi or dp).

Hi Chris and Manasi, thanks for the feedback and sorry for the late
response.

I see your point about being racy.  I think this is not an issue in my
system since it appears that the disconnect of the first connector
triggers another scheduling of the hotplug_work, that further detects
the other connector.

I'm still working on this, but can you provide more details on your
suggestion about using more knowledge of the shared ports to improve the
detection of DDIs?  I still don't see how we could prevent this race
once and for all.

In the mean time, I wrote another patch with a different way to reduce
the overhead of attempting to detect connectors of shared ports already
in use.  My understanding is that this version is no longer racy but it
doesn't catch every condition where we could avoid detection.  Though,
it feels like something we want to get upstream.  Can you please take a
look and let me know?

I think this also addresses the cloned encoders issue raised by Manasi,
if I understood it correctly.

>8
Subject: [PATCH] drm: i915: Stop further detection on shared pins if a sink is
 connected

HPD pins may be shared among connectors but, according to documentation,
only one connector can be enebled at any given time.  This avoids
waisting time with attempting to detect further connectors on the same
pin if the first one was already detected.

Since this reduces the overhead of the i915_hotplug_work_func, it may
address the following vblank misses detected by the CI:

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100215
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100558

Signed-off-by: Gabriel Krisman Bertazi 
---
 drivers/gpu/drm/i915/intel_hotplug.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hotplug.c 
b/drivers/gpu/drm/i915/intel_hotplug.c
index f1200272a699..913e8523b89d 100644
--- a/drivers/gpu/drm/i915/intel_hotplug.c
+++ b/drivers/gpu/drm/i915/intel_hotplug.c
@@ -319,6 +319,7 @@ static void i915_hotplug_work_func(struct work_struct *work)
struct drm_connector_list_iter conn_iter;
bool changed = false;
u32 hpd_event_bits;
+   u32 enc_hpd_pin_mask;
 
mutex_lock(>mode_config.mutex);
DRM_DEBUG_KMS("running encoder hotplug functions\n");
@@ -339,13 +340,18 @@ static void i915_hotplug_work_func(struct work_struct 
*work)
if (!intel_connector->encoder)
continue;
intel_encoder = intel_connector->encoder;
-   if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) {
+   enc_hpd_pin_mask = (1 << intel_encoder->hpd_pin);
+   if (hpd_event_bits & enc_hpd_pin_mask) {
DRM_DEBUG_KMS("Connector %s (pin %i) received hotplug 
event.\n",
  connector->name, intel_encoder->hpd_pin);
if (intel_encoder->hot_plug)
intel_encoder->hot_plug(intel_encoder);
-   if (intel_hpd_irq_event(dev, connector))
+   if (intel_hpd_irq_event(dev, connector)) {
changed = true;
+   if (connector->status ==
+   connector_status_connected)
+   hpd_event_bits &= ~(enc_hpd_pin_mask);
+   }
}
}
drm_connector_list_iter_end(_iter);
-- 
2.11.0

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

Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Manasi Navare]

On Thu, May 04, 2017 at 05:36:49PM -0300, Gabriel Krisman Bertazi wrote:
> On systems where more than one connector is attached to the same port,
> the HPD pin is also shared, and attaching one connector will trigger a
> hotplug on every other connector on that port.  But, according to the
> documentation, connectors sharing the port cannot be enabled
> simultaneously, such that we can abort the detection process early if
> another connector was detected succesfully.
> 
> This has the good side effect of preventing DP timeouts whenever
> something else is connected to the shared port, like below:
> 
> [drm:intel_dp_aux_ch [i915]] dp_aux_ch timeout status 0x7143003f
> [drm:drm_dp_dpcd_access] Too many retries, giving up. First error: -110
> 
> Since this reduces the overhead of the i915_hotplug_work_func, it may
> address the following vblank misses detected by the CI:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=100215
> https://bugs.freedesktop.org/show_bug.cgi?id=100558
> 
> Signed-off-by: Gabriel Krisman Bertazi 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 26 ++
>  drivers/gpu/drm/i915/intel_dp.c  |  3 +++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_hdmi.c|  3 +++
>  4 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 85b9e2f521a0..618b5138c0c7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11270,6 +11270,32 @@ static bool check_digital_port_conflicts(struct 
> drm_atomic_state *state)
>   return true;
>  }
>  
> +bool intel_shared_digital_port_in_use(struct drm_connector *conn)
> +{
> + struct drm_connector *peer;
> + struct drm_connector_list_iter iter;
> + struct intel_encoder *enc = to_intel_connector(conn)->encoder;
> + int ret = false;
> +
> + drm_connector_list_iter_begin(conn->dev, );
> + drm_for_each_connector_iter(peer, ) {
> + struct intel_encoder *peer_enc;
> +
> + if (peer == conn ||
> + peer->status != connector_status_connected)
> + continue;

So here, you are trying to find another connector in the list
of connectors that is not same as the passed connector but it is connected, 
right?


> +
> + peer_enc = to_intel_connector(peer)->encoder;
> + if (peer_enc->port == enc->port) {
> + ret = true;
> + break;

And the intention for this check is to see the connector that just
got hotplugged is connected to the same port as the port that already
has a connector connected? 

How does this handle the case where displays are cloned? Guess it would be same 
encoder
but still different ports?

Manasi


> + }
> + }
> + drm_connector_list_iter_end();
> +
> + return ret;
> +}
> +
>  static void
>  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 08834f74d396..0823c588575f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4628,6 +4628,9 @@ intel_dp_long_pulse(struct intel_connector 
> *intel_connector)
>  
>   
> WARN_ON(!drm_modeset_is_locked(>dev->mode_config.connection_mutex));
>  
> + if (intel_shared_digital_port_in_use(connector))
> + return connector_status_disconnected;
> +
>   intel_display_power_get(to_i915(dev), intel_dp->aux_power_domain);
>  
>   /* Can't disconnect eDP, but you can close the lid... */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 54f3ff840812..fd5f2517bede 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1411,6 +1411,7 @@ int vlv_force_pll_on(struct drm_i915_private *dev_priv, 
> enum pipe pipe,
>const struct dpll *dpll);
>  void vlv_force_pll_off(struct drm_i915_private *dev_priv, enum pipe pipe);
>  int lpt_get_iclkip(struct drm_i915_private *dev_priv);
> +bool intel_shared_digital_port_in_use(struct drm_connector *conn);
>  
>  /* modesetting asserts */
>  void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 52f0b2d5fad2..9ce0b1f45cde 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1532,6 +1532,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool 
> force)
>   DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> connector->base.id, connector->name);
>  
> + if (intel_shared_digital_port_in_use(connector))
> + return connector_status_disconnected;;
> +
>   intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  
>   intel_hdmi_unset_edid(connector);
> -- 
> 2.11.0
> 

Re: [Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Chris Wilson]

On Thu, May 04, 2017 at 05:36:49PM -0300, Gabriel Krisman Bertazi wrote:
> On systems where more than one connector is attached to the same port,
> the HPD pin is also shared, and attaching one connector will trigger a
> hotplug on every other connector on that port.  But, according to the
> documentation, connectors sharing the port cannot be enabled
> simultaneously, such that we can abort the detection process early if
> another connector was detected succesfully.
> 
> This has the good side effect of preventing DP timeouts whenever
> something else is connected to the shared port, like below:
> 
> [drm:intel_dp_aux_ch [i915]] dp_aux_ch timeout status 0x7143003f
> [drm:drm_dp_dpcd_access] Too many retries, giving up. First error: -110
> 
> Since this reduces the overhead of the i915_hotplug_work_func, it may
> address the following vblank misses detected by the CI:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=100215
> https://bugs.freedesktop.org/show_bug.cgi?id=100558
> 
> Signed-off-by: Gabriel Krisman Bertazi 

The key problem here is say a race between DP unplug and HDMI plug, and
users are evil enough (or common enough) for it to happen.

I thought the idea was reasonable though, and perhaps we could make
more use of the knowlege of the shared ports to improve detection of
common DP/HDMI DDIs (i.e. run detection once for a ddi and have it
decide whether it is hdmi or dp).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH RESEND] drm: i915: Don't try detecting sinks on ports already in use

[Gabriel Krisman Bertazi]

On systems where more than one connector is attached to the same port,
the HPD pin is also shared, and attaching one connector will trigger a
hotplug on every other connector on that port.  But, according to the
documentation, connectors sharing the port cannot be enabled
simultaneously, such that we can abort the detection process early if
another connector was detected succesfully.

This has the good side effect of preventing DP timeouts whenever
something else is connected to the shared port, like below:

[drm:intel_dp_aux_ch [i915]] dp_aux_ch timeout status 0x7143003f
[drm:drm_dp_dpcd_access] Too many retries, giving up. First error: -110

Since this reduces the overhead of the i915_hotplug_work_func, it may
address the following vblank misses detected by the CI:

https://bugs.freedesktop.org/show_bug.cgi?id=100215
https://bugs.freedesktop.org/show_bug.cgi?id=100558

Signed-off-by: Gabriel Krisman Bertazi 
---
 drivers/gpu/drm/i915/intel_display.c | 26 ++
 drivers/gpu/drm/i915/intel_dp.c  |  3 +++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c|  3 +++
 4 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 85b9e2f521a0..618b5138c0c7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11270,6 +11270,32 @@ static bool check_digital_port_conflicts(struct 
drm_atomic_state *state)
return true;
 }
 
+bool intel_shared_digital_port_in_use(struct drm_connector *conn)
+{
+   struct drm_connector *peer;
+   struct drm_connector_list_iter iter;
+   struct intel_encoder *enc = to_intel_connector(conn)->encoder;
+   int ret = false;
+
+   drm_connector_list_iter_begin(conn->dev, );
+   drm_for_each_connector_iter(peer, ) {
+   struct intel_encoder *peer_enc;
+
+   if (peer == conn ||
+   peer->status != connector_status_connected)
+   continue;
+
+   peer_enc = to_intel_connector(peer)->encoder;
+   if (peer_enc->port == enc->port) {
+   ret = true;
+   break;
+   }
+   }
+   drm_connector_list_iter_end();
+
+   return ret;
+}
+
 static void
 clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 08834f74d396..0823c588575f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4628,6 +4628,9 @@ intel_dp_long_pulse(struct intel_connector 
*intel_connector)
 

WARN_ON(!drm_modeset_is_locked(>dev->mode_config.connection_mutex));
 
+   if (intel_shared_digital_port_in_use(connector))
+   return connector_status_disconnected;
+
intel_display_power_get(to_i915(dev), intel_dp->aux_power_domain);
 
/* Can't disconnect eDP, but you can close the lid... */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 54f3ff840812..fd5f2517bede 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1411,6 +1411,7 @@ int vlv_force_pll_on(struct drm_i915_private *dev_priv, 
enum pipe pipe,
 const struct dpll *dpll);
 void vlv_force_pll_off(struct drm_i915_private *dev_priv, enum pipe pipe);
 int lpt_get_iclkip(struct drm_i915_private *dev_priv);
+bool intel_shared_digital_port_in_use(struct drm_connector *conn);
 
 /* modesetting asserts */
 void assert_panel_unlocked(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index 52f0b2d5fad2..9ce0b1f45cde 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1532,6 +1532,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool 
force)
DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
  connector->base.id, connector->name);
 
+   if (intel_shared_digital_port_in_use(connector))
+   return connector_status_disconnected;;
+
intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
intel_hdmi_unset_edid(connector);
-- 
2.11.0

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