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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Lyude <cpaul@redhat.com>
Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org,
	"open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow...,
	linux-kernel@vger.kernel.org open list"
	<dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/i915/vlv: Enable/disable VGA hotplugging properly
Date: Thu, 14 Apr 2016 20:59:22 +0300	[thread overview]
Message-ID: <20160414175922.GT4329@intel.com> (raw)
In-Reply-To: <1459284390-14485-1-git-send-email-cpaul@redhat.com>

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