From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Souza, Jose" Subject: Re: [PATCH 1/3] drm/i915/icl: Release TC ports when unloading or suspending driver Date: Thu, 29 Nov 2018 20:18:40 +0000 Message-ID: <01a08d916e25943e80839a765f5302bbe0961bb0.camel@intel.com> References: <20181108000554.18678-1-jose.souza@intel.com> <20181128113423.GC3349@ideak-desk.fi.intel.com> <3e58c36a1a75edd4aaa48037d92d5c9750b86dd2.camel@intel.com> <20181129135229.GD3349@ideak-desk.fi.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0762926540==" Return-path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 169236E3EF for ; Thu, 29 Nov 2018 20:18:43 +0000 (UTC) In-Reply-To: <20181129135229.GD3349@ideak-desk.fi.intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Deak, Imre" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org --===============0762926540== Content-Language: en-US Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="=-aClpSQXEYPIoEsUCy3ex" --=-aClpSQXEYPIoEsUCy3ex Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=C3=A9 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. > > >=20 > > > According to=20 > > > https://bugs.freedesktop.org/show_bug.cgi?id=3D108070#c26 > > >=20 > > > this patch should fix the bug reported at > > > https://bugs.freedesktop.org/show_bug.cgi?id=3D108070#c17 > > >=20 > > > but, I can't see how the change here would fix the corresponding > > > problem > > > described in > > > https://bugs.freedesktop.org/show_bug.cgi?id=3D108070#c18 > > >=20 > > > Would you explain? > > >=20 > > > I think there are more fundamental problems in TypeC HPD handling > > > as > > > we > > > discussed earlier here on the list, which should be fixed first: > > >=20 > > > - Switching to/from type C mode from the interrupt handler > > > without > > > considering if we have an active mode or an ongoing AUX > > > transfer. > >=20 > > 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. >=20 > The point is that imo those fundamental problems should be fixed > first. >=20 > > > - 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. > >=20 > > 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. >=20 > 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. >=20 > > > More comments below: > > >=20 > > > > Cc: Imre Deak > > > > Cc: Ville Syrj=C3=A4l=C3=A4 > > > > Signed-off-by: Jos=C3=A9 Roberto de Souza > > > > --- > > > > 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(+) > > > >=20 > > > > 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) > > > > =20 > > > > intel_runtime_pm_disable_interrupts(dev_priv); > > > > intel_hpd_cancel_work(dev_priv); > > > > + intel_hpd_suspend(dev_priv); > > >=20 > > > 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. > >=20 > > Oh my bad I was thinking that the regular suspend would handle this > > too > > but I was wrong. > >=20 > > So I will add a intel_hpd_suspend() call to intel_runtime_suspend() > > leaving the i915_drv.c changes as this. > >=20 > > 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) > >=20 > > intel_runtime_pm_disable_interrupts(dev_priv); > > intel_hpd_cancel_work(dev_priv); > > + intel_hpd_suspend(dev_priv); > >=20 > > intel_suspend_encoders(dev_priv); > >=20 > > @@ -2909,6 +2910,7 @@ static int intel_runtime_suspend(struct > > device > > *kdev) > > intel_uc_suspend(dev_priv); > >=20 > > intel_runtime_pm_disable_interrupts(dev_priv); > > + intel_hpd_suspend(dev_priv); > >=20 > > intel_uncore_suspend(dev_priv); > >=20 > > Also I will drop the next patch as we don't call > > intel_hpd_cancel_work() in intel_runtime_suspend() path. > >=20 > >=20 > > > 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). > >=20 > > 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. >=20 > 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. >=20 > > > > =20 > > > > intel_suspend_encoders(dev_priv); > > > > =20 > > > > 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); > > > > =20 > > > > /* 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 =3D false; > > > > } > > > > =20 > > > > 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; > > > > } > > > > =20 > > > > +/* > > > > + * 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 =3D to_i915(encoder- > > > > >base.dev); > > > > + struct intel_digital_port *dig_port; > > > > + > > > > + dig_port =3D enc_to_dig_port(&encoder->base); > > > > + /* Skip fake MST ports */ > > > > + if (!dig_port) > > > > + return; > > > > + > > > > + if (INTEL_GEN(dev_priv) >=3D 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); > > > > =20 > > > > /* 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 =3D HPD_ENABLED; > > > > spin_unlock_irq(&dev_priv->irq_lock); > > > > } > > > > + > > > > +void intel_hpd_suspend(struct drm_i915_private *dev_priv) > > > > +{ > > > > + struct drm_device *dev =3D &dev_priv->drm; > > > > + struct intel_encoder *encoder; > > > > + > > > > + for_each_intel_encoder(dev, encoder) > > > > + intel_digital_port_force_disconnection(encoder) > > > > ; > > > > +} > > > > --=20 > > > > 2.19.1 > > > >=20 >=20 >=20 --=-aClpSQXEYPIoEsUCy3ex Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEVNG051EijGa0MiaQVenbO/mOWkkFAlwASZ4ACgkQVenbO/mO Wklzkwf/aP1RWT9PaE6+iEKLirbLaxSgxspYbQWeBZF49hvWFNCsYWLbzEMEt6gx 5pKiPRjlk3MD3FpnDZAjxHFCUaJE/OBjENnize0SUaqsxZlrsWIB6a2HRvNehAg8 ChFBUk5b83FzwEwA8KwPHnFExV8a7VZcJEnhcyzsztw0EgsQ1Xq3gAo7Xe+JDgtk pm4sppjBtJSOIvhi21VQnEDRqn/70Y6ugX8S527TIt5gw93J2q98jHpYOg+wtVh5 pfI2d1RlLAUXrBMc5r26W0F012BGbgV4+zosw1WLDNtW8ntSlbwsLU6XR2M4tfxW z16FLoHwIkzUf5+HSaeWQadYBBSPjg== =DLv+ -----END PGP SIGNATURE----- --=-aClpSQXEYPIoEsUCy3ex-- --===============0762926540== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0762926540==--