[Intel-gfx] [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver
Souza, Jose
jose.souza at intel.com
Wed Nov 28 21:54:21 UTC 2018
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 at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20181128/9f9743c1/attachment.sig>
More information about the Intel-gfx
mailing list