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@xxxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > --- > > 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 > >
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
