Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver

[Imre Deak <imre.deak@intel.com>]

On Wed, Nov 07, 2018 at 04:05:52PM -0800, José Roberto de Souza wrote:
> When suspending or unloading the driver, it needs to release the
> TC ports so HW can change it state without wait for driver handshake.

According to 
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c26

this patch should fix the bug reported at
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c17

but, I can't see how the change here would fix the corresponding problem
described in
https://bugs.freedesktop.org/show_bug.cgi?id=108070#c18

Would you explain?

I think there are more fundamental problems in TypeC HPD handling as we
discussed earlier here on the list, which should be fixed first:

- Switching to/from type C mode from the interrupt handler without
  considering if we have an active mode or an ongoing AUX transfer.

- Not having any way for handling the case where we'd do a modeset after
  an HPD disconnect interrupt, like in the case of
  common-hpd-after-suspend in the bug report.

More comments below:

> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  1 +
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/i915_irq.c      |  1 +
>  drivers/gpu/drm/i915/intel_dp.c      | 19 +++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_hotplug.c |  9 +++++++++
>  6 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index acb516308262..14331c396278 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1911,6 +1911,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);

What about runtime suspend? We won't receive HPD interrupts after that
either, so we'd need to disconnect in that case too according to the
spec.

And how are we handling resume then where we'll be in a disconnected
state due to the change in this patch and would continue to do a
modeset (to restore the mode we had before suspend).

>  
>  	intel_suspend_encoders(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0c8438de3c1b..96d5ddc36f4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2792,6 +2792,7 @@ enum hpd_pin intel_hpd_pin_default(struct drm_i915_private *dev_priv,
>  				   enum port port);
>  bool intel_hpd_disable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
>  void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin);
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv);
>  
>  /* i915_irq.c */
>  static inline void i915_queue_hangcheck(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index d7e47d6082de..23084d227e2a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4996,6 +4996,7 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv)
>  {
>  	drm_irq_uninstall(&dev_priv->drm);
>  	intel_hpd_cancel_work(dev_priv);
> +	intel_hpd_suspend(dev_priv);
>  	dev_priv->runtime_pm.irqs_enabled = false;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5258c9d654f4..27c6163426d6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5043,6 +5043,25 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
>  	return false;
>  }
>  
> +/*
> + * intel_digital_port_force_disconnection - Used to force port disconnection or
> + * release any control from display driver before going to a state that driver
> + * will not handle interruptions.
> + */
> +void intel_digital_port_force_disconnection(struct intel_encoder *encoder)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port;
> +
> +	dig_port = enc_to_dig_port(&encoder->base);
> +	/* Skip fake MST ports */
> +	if (!dig_port)
> +		return;
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icl_tc_phy_disconnect(dev_priv, dig_port);
> +}
> +
>  static struct edid *
>  intel_dp_get_edid(struct intel_dp *intel_dp)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6772e9974751..bc30f107b430 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1851,6 +1851,7 @@ bool intel_dp_read_dpcd(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> +void intel_digital_port_force_disconnection(struct intel_encoder *encoder);
>  
>  /* intel_dp_aux_backlight.c */
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index 42e61e10f517..d6745b7b79d5 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -655,3 +655,12 @@ void intel_hpd_enable(struct drm_i915_private *dev_priv, enum hpd_pin pin)
>  	dev_priv->hotplug.stats[pin].state = HPD_ENABLED;
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
> +
> +void intel_hpd_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +	struct intel_encoder *encoder;
> +
> +	for_each_intel_encoder(dev, encoder)
> +		intel_digital_port_force_disconnection(encoder);
> +}
> -- 
> 2.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx