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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 10468 bytes --]

On Thu, 2018-11-29 at 15:52 +0200, Imre Deak wrote:
> On Wed, Nov 28, 2018 at 11:54:21PM +0200, Souza, Jose wrote:
> > On Wed, 2018-11-28 at 13:34 +0200, Imre Deak wrote:
> > > 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.
> > 
> > Yes that is still missing, I plan to work on that after I deliver a
> > few
> > tasks. Also I read the emails in discussion that you started about
> > that.
> 
> The point is that imo those fundamental problems should be fixed
> first.
> 
> > > - 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.
> > 
> > For what I understood about this test it will suspend, disconnect
> > chamelium board from source, wakeup and check if connector state
> > match,
> > as we did not marked port as safe before suspending we are left in
> > a
> > invalid state where PD firmware do not trigger a interruption after
> > wakeup.
> 
> No, it's not about the lack of HPD interrupts. What happens is that
> the
> sink disappears during system suspend/resume, but we still have an
> active mode enabled on the corresponding port. We try to restore this
> mode during resume but that modeset will fail.

Before suspend, all display output is disabled so there is no active
modeset.

> 
> > > 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.
> > 
> > Oh my bad I was thinking that the regular suspend would handle this
> > too
> > but I was wrong.
> > 
> > So I will add a intel_hpd_suspend() call to intel_runtime_suspend()
> > leaving the i915_drv.c changes as this.
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index b1d23c73c147..948914a79c67 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1905,6 +1905,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);
> > 
> >         intel_suspend_encoders(dev_priv);
> > 
> > @@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct
> > device
> > *kdev)
> >         intel_uc_suspend(dev_priv);
> > 
> >         intel_runtime_pm_disable_interrupts(dev_priv);
> > +       intel_hpd_suspend(dev_priv);
> > 
> >         intel_uncore_suspend(dev_priv);
> > 
> > Also I will drop the next patch as we don't call
> > intel_hpd_cancel_work() in intel_runtime_suspend() path.
> > 
> > 
> > > 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).
> > 
> > The drm_kms_helper_poll_enable() calls will trigger the call to the
> > detection function of the connectors that will call
> > intel_digital_port_connected() thal will call ICL TC functions that
> > will take care of update everything.
> 
> No detection will be triggered, since we're not polling this port
> (and
> polling can be disabled as a whole). In any case that would be too
> late,
> since we're trying to restore the mode before we call
> drm_kms_helper_poll_enable().  Also doing a detection is not enough
> here, since if detection fails (as it would in our case) we still
> have a
> mode that we need to restore. The corresponding modeset would fail,
> but
> we don't have a way to fail it gracefully as I wrote above.


For the regular suspend it will run the connector detection functions
for sure when trying to restore the previous modeset.
About the runtime resume I need to check if with the polling disabled
it will still update the connector state.


> 
> > > >  
> > > >  	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
> > > > 
> 
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx