| Message ID | 1459284390-14485-1-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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); + } + 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;
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(+)