| Message ID | 20181108000554.18678-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915/icl: Release TC ports when unloading or suspending driver | expand |
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 >
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. > > - 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. > > 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. > > > > > 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 > >
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. > > > > > 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. > > > > > > > > > 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 > > >
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 > > > > > >
On Thu, Nov 29, 2018 at 10:18:40PM +0200, Souza, Jose wrote: > 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. Yes, there is: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_5129/fi-icl-u2/dmesg0.log : <7>[ 204.806237] [drm:intel_atomic_check [i915]] [CONNECTOR:209:HDMI-A-2] Limiting display bpp to 24 instead of EDID bpp 24, requested bpp 36, max platform bpp 36 ... <7>[ 204.806411] [drm:intel_dump_pipe_config [i915]] [CRTC:80:pipe A][modeset] ... <7>[ 204.809132] [drm:intel_enable_pipe [i915]] enabling pipe A ... <7>[ 204.909519] [IGT] kms_chamelium: executing ... <7>[ 204.947440] [IGT] kms_chamelium: starting subtest common-hpd-after-suspend ... <6>[ 205.936472] PM: suspend entry (deep) After which we will try to restore this active mode during resume. > > > > > > > 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. Not sure what you mean. From drm_kms_helper_poll_enable(): if (!dev->mode_config.poll_enabled || !drm_kms_helper_poll) return; so we won't do anything in this function if polling is globally disabled. if (connector->polled & (DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT)) poll = true; so we won't do anything if we haven't enabled polling for the connector. Generally we don't enable polling for connectors which provide HPD interrupts. There is actually a detection work scheduled from intel_hpd_init(), but again that will run only after we tried to restore the pre-suspend mode. Also note what I wrote about detection being not enough for us. The sink can be gone by the time we resume and we will still try to restore the mode. > 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 > > > > > > > > >
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); 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); +}
When suspending or unloading the driver, it needs to release the TC ports so HW can change it state without wait for driver handshake. 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(+)