Looks like we might not need to worry about this patch anymore actually, looks like this problem got fixed by accident by one of the other vlv fixes you pushed. Now it's not always modesetting on hotplug when it was before though :(, so I'll get to work on bisecting that.

On Thu, 2016-04-14 at 20:59 +0300, Ville Syrjälä wrote:

On Tue, Mar 29, 2016 at 04:46:30PM -0400, Lyude wrote:

...

On Valleyview, VGA hotplugging is controlled through a seperate register than everything else, VLV_ADPA, which must be explicitly set.

While VGA hotplugging worked(ish) before, it looks like that was mainly because we'd unintentionally enable it in valleyview_crt_detect_hotplug() when we did a force trigger. This doesn't work reliably enough because whenever the display powerwell on vlv gets disabled, the values set in VLV_ADPA get cleared and consequently VGA hotplugging gets disabled. This causes bugs such as one we found on an Intel NUC, where doing the following sequence of hotplugs:

• Disconnect all monitors
• Connect VGA
• Disconnect VGA
• Connect HDMI

Would result in hotplugging getting disabled, due to the display powerwells getting toggled in the process of connecting HDMI.

CC: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com

---

drivers/gpu/drm/i915/i915_irq.c | 14 ++++++++++++++  1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 5aa4239..60592a4 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3611,6 +3611,7 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)  {   u32 pipestat_mask;   u32 iir_mask;

• u32 adpa_reg;

enum pipe pipe;     pipestat_mask = PIPESTAT_INT_STATUS_MASK | @@ -3627,6 +3628,12 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)   for_each_pipe(dev_priv, pipe)         i915_enable_pipestat(dev_priv, pipe, pipestat_mask);

• if (IS_VALLEYVIEW(dev_priv)) {

• adpa_reg = I915_READ(VLV_ADPA);
  

• adpa_reg |= ADPA_CRT_HOTPLUG_ENABLE;
  

• I915_WRITE(VLV_ADPA, adpa_reg);
  

• }

We might not want to enable that when there's no VGA connector.

Seems like we should just be calling intel_crt_reset() here. We definitely don't want to call the reset for hooks for all the other connectors so drm_mode_config_reset() is out. Also the connector locking might be problematic here, so I might suggest adjusting intel_crt_reset() to take an encoder instead of connector, and then we should be able to walk the encoder list without any troubles.

...

iir_mask = I915_DISPLAY_PORT_INTERRUPT |      I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |      I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; @@ -3645,8 +3652,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)  {   u32 pipestat_mask;   u32 iir_mask;

• u32 adpa_reg;

enum pipe pipe;

• if (IS_VALLEYVIEW(dev_priv)) {

• adpa_reg = I915_READ(VLV_ADPA);
  

• adpa_reg &= ~ADPA_CRT_HOTPLUG_ENABLE;
  

• I915_WRITE(VLV_ADPA, adpa_reg);
  

• }

iir_mask = I915_DISPLAY_PORT_INTERRUPT |      I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |      I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; --  2.5.5

---

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

Huh, neither am I now. I seem to be able to reproduce the problem just fine again. Anyway I'll send the new versions of the patches in a little bit

On Fri, 2016-04-15 at 18:49 +0300, Ville Syrjälä wrote:

On Fri, Apr 15, 2016 at 09:47:51AM -0400, Lyude Paul wrote:

...

Looks like we might not need to worry about this patch anymore actually, looks like this problem got fixed by accident by one of the other vlv fixes you pushed.

Not sure what exactly changed for you, but we definitely need to reinitialize ADPA when re-enabling the power well.

...

Now it's not always modesetting on hotplug when it was before though :(, so I'll get to work on bisecting that.

On Thu, 2016-04-14 at 20:59 +0300, Ville Syrjälä wrote:

...

On Tue, Mar 29, 2016 at 04:46:30PM -0400, Lyude wrote:

...

On Valleyview, VGA hotplugging is controlled through a seperate register than everything else, VLV_ADPA, which must be explicitly set.

While VGA hotplugging worked(ish) before, it looks like that was mainly because we'd unintentionally enable it in valleyview_crt_detect_hotplug() when we did a force trigger. This doesn't work reliably enough because whenever the display powerwell on vlv gets disabled, the values set in VLV_ADPA get cleared and consequently VGA hotplugging gets disabled. This causes bugs such as one we found on an Intel NUC, where doing the following sequence of hotplugs:

• Disconnect all monitors
• Connect VGA
• Disconnect VGA
• Connect HDMI

Would result in hotplugging getting disabled, due to the display powerwells getting toggled in the process of connecting HDMI.

CC: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com

---

drivers/gpu/drm/i915/i915_irq.c | 14 ++++++++++++++  1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 5aa4239..60592a4 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3611,6 +3611,7 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)  {   u32 pipestat_mask;   u32 iir_mask;

• u32 adpa_reg;

enum pipe pipe;     pipestat_mask = PIPESTAT_INT_STATUS_MASK | @@ -3627,6 +3628,12 @@ static void valleyview_display_irqs_install(struct drm_i915_private *dev_priv)   for_each_pipe(dev_priv, pipe)         i915_enable_pipestat(dev_priv, pipe, pipestat_mask);

• if (IS_VALLEYVIEW(dev_priv)) {

• adpa_reg = I915_READ(VLV_ADPA);
  

• adpa_reg |= ADPA_CRT_HOTPLUG_ENABLE;
  

• I915_WRITE(VLV_ADPA, adpa_reg);
  

• }

We might not want to enable that when there's no VGA connector.

Seems like we should just be calling intel_crt_reset() here. We definitely don't want to call the reset for hooks for all the other connectors so drm_mode_config_reset() is out. Also the connector locking might be problematic here, so I might suggest adjusting intel_crt_reset() to take an encoder instead of connector, and then we should be able to walk the encoder list without any troubles.

...

iir_mask = I915_DISPLAY_PORT_INTERRUPT |      I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |      I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; @@ -3645,8 +3652,15 @@ static void valleyview_display_irqs_uninstall(struct drm_i915_private *dev_priv)  {   u32 pipestat_mask;   u32 iir_mask;

• u32 adpa_reg;

enum pipe pipe;

• if (IS_VALLEYVIEW(dev_priv)) {

• adpa_reg = I915_READ(VLV_ADPA);
  

• adpa_reg &= ~ADPA_CRT_HOTPLUG_ENABLE;
  

• I915_WRITE(VLV_ADPA, adpa_reg);
  

• }

iir_mask = I915_DISPLAY_PORT_INTERRUPT |      I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |      I915_DISPLAY_PIPE_B_EVENT_INTERRUPT; --  2.5.5

---

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

While VGA hotplugging worked(ish) before, it looks like that was mainly because we'd unintentionally enable it in valleyview_crt_detect_hotplug() when we did a force trigger. This doesn't work reliably enough because whenever the display powerwell on vlv gets disabled, the values set in VLV_ADPA get cleared and consequently VGA hotplugging gets disabled. This causes bugs such as one we found on an Intel NUC, where doing the following sequence of hotplugs:

- Disconnect all monitors - Connect VGA - Disconnect VGA - Connect HDMI

Would result in VGA hotplugging becoming disabled, due to the powerwells getting toggled in the process of connecting HDMI.

Changes since v1: - Instead of handling the register writes ourself, we just reuse intel_crt_detect() - Instead of resetting the ADPA during display IRQ installation, we now reset them in vlv_display_power_well_init()

CC: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/i915/intel_runtime_pm.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 80e8bd4..c7d195f 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -902,6 +902,7 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,

static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) { + struct drm_encoder *encoder, *vga_encoder = NULL; enum pipe pipe;

/* @@ -935,6 +936,17 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)

intel_hpd_init(dev_priv);

+ /* Re-enable the ADPA, if we have one */ + drm_for_each_encoder(encoder, dev_priv->dev) { + if (encoder->encoder_type == DRM_MODE_ENCODER_DAC) { + vga_encoder = encoder; + break; + } + } + + if (vga_encoder && vga_encoder->funcs->reset) + vga_encoder->funcs->reset(vga_encoder); + i915_redisable_vga_power_on(dev_priv->dev); }

While VGA hotplugging worked(ish) before, it looks like that was mainly because we'd unintentionally enable it in valleyview_crt_detect_hotplug() when we did a force trigger. This doesn't work reliably enough because whenever the display powerwell on vlv gets disabled, the values set in VLV_ADPA get cleared and consequently VGA hotplugging gets disabled. This causes bugs such as one we found on an Intel NUC, where doing the following sequence of hotplugs:

- Disconnect all monitors - Connect VGA - Disconnect VGA - Connect HDMI

Would result in VGA hotplugging becoming disabled, due to the powerwells getting toggled in the process of connecting HDMI.

Changes since v2: - Use intel_encoder structs instead of drm_encoder structs

Changes since v1: - Instead of handling the register writes ourself, we just reuse intel_crt_detect() - Instead of resetting the ADPA during display IRQ installation, we now reset them in vlv_display_power_well_init()

CC: stable@vger.kernel.org Signed-off-by: Lyude cpaul@redhat.com --- drivers/gpu/drm/i915/intel_runtime_pm.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 80e8bd4..0eae08a 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -902,6 +902,7 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,

static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) { + struct intel_encoder *encoder; enum pipe pipe;

/* @@ -935,6 +936,12 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv)

intel_hpd_init(dev_priv);

+ /* Re-enable the ADPA, if we have one */ + for_each_intel_encoder(dev_priv->dev, encoder) { + if (encoder->type == INTEL_OUTPUT_ANALOG) + encoder->base.funcs->reset(&encoder->base); + } + i915_redisable_vga_power_on(dev_priv->dev); }