[PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[Lyude]

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);
+	}
+
 	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

Re: [PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[Ville Syrjälä]

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

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[Lyude Paul]

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

Re: [PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[Ville Syrjälä]

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

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly

[Lyude Paul]

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

[PATCH v2 1/2] drm/i915/vlv: Make intel_crt_reset() per-encoder

[Lyude]

This lets call intel_crt_reset() in contexts where IRQs are disabled and
as such, can't hold the locks required to work with the connectors.

CC: stable@vger.kernel.org
Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index a2a31fd..220ca91 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -707,11 +707,11 @@ static int intel_crt_set_property(struct drm_connector *connector,
 	return 0;
 }
 
-static void intel_crt_reset(struct drm_connector *connector)
+static void intel_crt_reset(struct drm_encoder *encoder)
 {
-	struct drm_device *dev = connector->dev;
+	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crt *crt = intel_attached_crt(connector);
+	struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder));
 
 	if (INTEL_INFO(dev)->gen >= 5) {
 		u32 adpa;
@@ -733,7 +733,6 @@ static void intel_crt_reset(struct drm_connector *connector)
  */
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
-	.reset = intel_crt_reset,
 	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -751,6 +750,7 @@ static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs
 };
 
 static const struct drm_encoder_funcs intel_crt_enc_funcs = {
+	.reset = intel_crt_reset,
 	.destroy = intel_encoder_destroy,
 };
 
@@ -896,5 +896,5 @@ void intel_crt_init(struct drm_device *dev)
 		dev_priv->fdi_rx_config = I915_READ(FDI_RX_CTL(PIPE_A)) & fdi_config;
 	}
 
-	intel_crt_reset(connector);
+	intel_crt_reset(&crt->base.base);
 }
-- 
2.5.5

[PATCH v2 2/2] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Lyude]

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);
 }
 
-- 
2.5.5

Re: [PATCH v2 1/2] drm/i915/vlv: Make intel_crt_reset() per-encoder

[Ville Syrjälä]

On Fri, Apr 15, 2016 at 03:40:09PM -0400, Lyude wrote:
> This lets call intel_crt_reset() in contexts where IRQs are disabled and
> as such, can't hold the locks required to work with the connectors.
> 
> CC: stable@vger.kernel.org
> Signed-off-by: Lyude <cpaul@redhat.com>

Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index a2a31fd..220ca91 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -707,11 +707,11 @@ static int intel_crt_set_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> -static void intel_crt_reset(struct drm_connector *connector)
> +static void intel_crt_reset(struct drm_encoder *encoder)
>  {
> -	struct drm_device *dev = connector->dev;
> +	struct drm_device *dev = encoder->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_crt *crt = intel_attached_crt(connector);
> +	struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder));
>  
>  	if (INTEL_INFO(dev)->gen >= 5) {
>  		u32 adpa;
> @@ -733,7 +733,6 @@ static void intel_crt_reset(struct drm_connector *connector)
>   */
>  
>  static const struct drm_connector_funcs intel_crt_connector_funcs = {
> -	.reset = intel_crt_reset,
>  	.dpms = drm_atomic_helper_connector_dpms,
>  	.detect = intel_crt_detect,
>  	.fill_modes = drm_helper_probe_single_connector_modes,
> @@ -751,6 +750,7 @@ static const struct drm_connector_helper_funcs intel_crt_connector_helper_funcs
>  };
>  
>  static const struct drm_encoder_funcs intel_crt_enc_funcs = {
> +	.reset = intel_crt_reset,
>  	.destroy = intel_encoder_destroy,
>  };
>  
> @@ -896,5 +896,5 @@ void intel_crt_init(struct drm_device *dev)
>  		dev_priv->fdi_rx_config = I915_READ(FDI_RX_CTL(PIPE_A)) & fdi_config;
>  	}
>  
> -	intel_crt_reset(connector);
> +	intel_crt_reset(&crt->base.base);
>  }
> -- 
> 2.5.5

-- 
Ville Syrj�l�
Intel OTC

Re: [PATCH v2 2/2] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Ville Syrjälä]

On Fri, Apr 15, 2016 at 03:40:10PM -0400, Lyude wrote:
> 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);
> +

Something like

struct intel_encoder *encoder;
...
for_each_intel_encoder(encoder) {
	if (encoder->type == ANALOG)
		intel_crt_reset(&encoder->base);
}

would be neater in my eyes.

>  	i915_redisable_vga_power_on(dev_priv->dev);
>  }
>  
> -- 
> 2.5.5

-- 
Ville Syrj�l�
Intel OTC

[PATCH v3 2/2] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Lyude]

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);
 }
 
-- 
2.5.5

Re: [PATCH v3 2/2] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Ville Syrjälä]

On Mon, Apr 18, 2016 at 10:00:36AM -0400, Lyude wrote:
> 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);

intel_crt_reset(&encoder->base) would be much easier to navigate.
Or if we want to be a bit more verbose we could call it
intel_crt_hpd_init() or somesuch.

With that 
Reviewed-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>

Before we push these, I think we'll need to do a bit more work to
make sure we don't end up in an infinite crt hpd loop (already
happend to me a few times). I'll polish up my hacks for that and
send them out...

> +	}
> +
>  	i915_redisable_vga_power_on(dev_priv->dev);
>  }
>  
> -- 
> 2.5.5

-- 
Ville Syrj�l�
Intel OTC

[PATCH v4 2/3] drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()

[Lyude]

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 v3:
 - Expose intel_crt_reset() through intel_drv.h and call that in
   vlv_display_power_well_init() instead of
   encoder->base.funcs->reset(&encoder->base);

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>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c        | 2 +-
 drivers/gpu/drm/i915/intel_drv.h        | 2 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 7 +++++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 220ca91..54db931 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -707,7 +707,7 @@ static int intel_crt_set_property(struct drm_connector *connector,
 	return 0;
 }
 
-static void intel_crt_reset(struct drm_encoder *encoder)
+void intel_crt_reset(struct drm_encoder *encoder)
 {
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e0fcfa1..4e96d5c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1044,7 +1044,7 @@ void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
 
 /* intel_crt.c */
 void intel_crt_init(struct drm_device *dev);
-
+void intel_crt_reset(struct drm_encoder *encoder);
 
 /* intel_ddi.c */
 void intel_ddi_clk_select(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 80e8bd4..b643533 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)
+			intel_crt_reset(&encoder->base);
+	}
+
 	i915_redisable_vga_power_on(dev_priv->dev);
 }
 
-- 
2.5.5