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

[Lyude Paul <cpaul@redhat.com>]

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