NACK/cmt: [PATCH v3 1/1][SRU][H] drm/i915: Force a TypeC PHY disconnect during suspend/shutdown
Chris Chiu
chris.chiu at canonical.com
Tue Jul 20 09:32:28 UTC 2021
On Mon, Jul 19, 2021 at 10:29 PM Tim Gardner <tim.gardner at canonical.com>
wrote:
> Backports of more then simple complexity generally deserve some
> explanation. In this case it looks like you've dropped a couple of
> functions.
>
>
In the v2 version, I did miss the code of intel_ddi_encoder_suspend and
shutdown, so I added them back in v3. In the v3 version, the dropped
function which causes unclean cherry-pick is skl_hpd_pin() which is handled
by the following commit which does not affect the TGL models.
commit c8455098c67914c59d07f01819469e2e6f76f358
Author: Lyude Paul <lyude at redhat.com>
Date: Tue Feb 9 14:16:28 2021 -0500
drm/i915/gen9_bc: Introduce HPD pin mappings for TGP PCH + CML combos
and intel_ddi_is_tc() from the following commit which could cause lots of
conflicts but we don't really need it for TGL model.
commit 36ecb0ec105412aa7e7c89991a7cff90bf90b2f1
Author: Ville Syrjälä <ville.syrjala at linux.intel.com>
Date: Fri Feb 5 23:46:26 2021 +0200
drm/i915: Extract icl+ .{enable,disable}_clock() vfuncs
Should I explain in the cover letter? Which part would be appropriate?
[Fix]? I'll propose a v4 with explanations. Thanks
On 7/16/21 2:48 AM, chris.chiu at canonical.com wrote:
> > From: Imre Deak <imre.deak at intel.com>
> >
> > BugLink: https://bugs.launchpad.net/bugs/1931072
> >
> > Disconnect TypeC PHYs during system suspend and shutdown, even with the
> > corresponding TypeC sink still plugged to its connector, since leaving
> > the PHY connected causes havoc at least during system resume in the
> > presence of an Nvidia card.
> >
> > Note that this will only make a difference in the TypeC DP alternate
> > mode, since in Thunderbolt alternate mode the PHY is never owned by the
> > display engine and there is no notion of PHY ownership in legacy mode
> > (the display engine being the only possible owner in that mode and the
> > TypeC subsystem not having anything to do with the port in that case).
> >
> > Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3500
> > Reported-and-tested-by: Chris Chiu <chris.chiu at canonical.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > Reviewed-by: Uma Shankar <uma.shankar at intel.com>
> > Link:
> https://patchwork.freedesktop.org/patch/msgid/20210610174223.605904-1-imre.deak@intel.com
> > (backported from commit 151ec347b06a2fb6ecd2922475dca71a7af827a5
> > linux-next)
> > Signed-off-by: Chris Chiu <chris.chiu at canonical.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_ddi.c | 34 ++++++++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_tc.c | 34 +++++++++++++++++++-----
> > drivers/gpu/drm/i915/display/intel_tc.h | 2 ++
> > 3 files changed, 61 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index cf665636408b..b272da304795 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -5338,6 +5338,36 @@ static enum hpd_pin cnl_hpd_pin(struct
> drm_i915_private *dev_priv,
> > return HPD_PORT_A + port - PORT_A;
> > }
> >
> > +static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
> > +{
> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > + enum phy phy = intel_port_to_phy(i915, encoder->port);
> > +
> > + intel_dp_encoder_suspend(encoder);
> > +
> > + if (!intel_phy_is_tc(i915, phy))
> > + return;
> > +
> > + intel_tc_port_disconnect_phy(dig_port);
> > +}
> > +
> > +static void intel_ddi_encoder_shutdown(struct intel_encoder *encoder)
> > +{
> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > + enum phy phy = intel_port_to_phy(i915, encoder->port);
> > +
> > + intel_dp_encoder_shutdown(encoder);
> > +
> > + if (!intel_phy_is_tc(i915, phy))
> > + return;
> > +
> > + intel_tc_port_disconnect_phy(dig_port);
> > +}
> > +
> > #define port_tc_name(port) ((port) - PORT_TC1 + '1')
> > #define tc_port_name(tc_port) ((tc_port) - TC_PORT_1 + '1')
> >
> > @@ -5432,8 +5462,8 @@ void intel_ddi_init(struct drm_i915_private
> *dev_priv, enum port port)
> > encoder->get_config = intel_ddi_get_config;
> > encoder->sync_state = intel_ddi_sync_state;
> > encoder->initial_fastset_check = intel_ddi_initial_fastset_check;
> > - encoder->suspend = intel_dp_encoder_suspend;
> > - encoder->shutdown = intel_dp_encoder_shutdown;
> > + encoder->suspend = intel_ddi_encoder_suspend;
> > + encoder->shutdown = intel_ddi_encoder_shutdown;
> > encoder->get_power_domains = intel_ddi_get_power_domains;
> >
> > encoder->type = INTEL_OUTPUT_DDI;
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> > index 8b6f16f9d0d1..2f4d7336d76e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> > @@ -449,7 +449,7 @@ intel_tc_port_get_target_mode(struct
> intel_digital_port *dig_port)
> > }
> >
> > static void intel_tc_port_reset_mode(struct intel_digital_port
> *dig_port,
> > - int required_lanes)
> > + int required_lanes, bool
> force_disconnect)
> > {
> > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > enum tc_port_mode old_tc_mode = dig_port->tc_mode;
> > @@ -465,7 +465,8 @@ static void intel_tc_port_reset_mode(struct
> intel_digital_port *dig_port,
> > }
> >
> > icl_tc_phy_disconnect(dig_port);
> > - icl_tc_phy_connect(dig_port, required_lanes);
> > + if (!force_disconnect)
> > + icl_tc_phy_connect(dig_port, required_lanes);
> >
> > drm_dbg_kms(&i915->drm, "Port %s: TC port mode reset (%s -> %s)\n",
> > dig_port->tc_port_name,
> > @@ -555,7 +556,7 @@ bool intel_tc_port_connected(struct intel_encoder
> *encoder)
> > }
> >
> > static void __intel_tc_port_lock(struct intel_digital_port *dig_port,
> > - int required_lanes)
> > + int required_lanes, bool force_disconnect)
> > {
> > struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> > intel_wakeref_t wakeref;
> > @@ -569,8 +570,9 @@ static void __intel_tc_port_lock(struct
> intel_digital_port *dig_port,
> >
> > tc_cold_wref = tc_cold_block(dig_port);
> >
> > - if (intel_tc_port_needs_reset(dig_port))
> > - intel_tc_port_reset_mode(dig_port, required_lanes);
> > + if (force_disconnect ||
> intel_tc_port_needs_reset(dig_port))
> > + intel_tc_port_reset_mode(dig_port, required_lanes,
> > + force_disconnect);
> >
> > tc_cold_unblock(dig_port, tc_cold_wref);
> > }
> > @@ -581,7 +583,7 @@ static void __intel_tc_port_lock(struct
> intel_digital_port *dig_port,
> >
> > void intel_tc_port_lock(struct intel_digital_port *dig_port)
> > {
> > - __intel_tc_port_lock(dig_port, 1);
> > + __intel_tc_port_lock(dig_port, 1, false);
> > }
> >
> > void intel_tc_port_unlock(struct intel_digital_port *dig_port)
> > @@ -595,6 +597,24 @@ void intel_tc_port_unlock(struct intel_digital_port
> *dig_port)
> > wakeref);
> > }
> >
> > +/**
> > + * intel_tc_port_disconnect_phy: disconnect TypeC PHY from display port
> > + * @dig_port: digital port
> > + *
> > + * Disconnect the given digital port from its TypeC PHY (handing back
> the
> > + * control of the PHY to the TypeC subsystem). The only purpose of this
> > + * function is to force the disconnect even with a TypeC display output
> still
> > + * plugged to the TypeC connector, which is required by the TypeC
> firmwares
> > + * during system suspend and shutdown. Otherwise - during the unplug
> event
> > + * handling - the PHY ownership is released automatically by
> > + * intel_tc_port_reset_mode(), when calling this function is not
> required.
> > + */
> > +void intel_tc_port_disconnect_phy(struct intel_digital_port *dig_port)
> > +{
> > + __intel_tc_port_lock(dig_port, 1, true);
> > + intel_tc_port_unlock(dig_port);
> > +}
> > +
> > bool intel_tc_port_ref_held(struct intel_digital_port *dig_port)
> > {
> > return mutex_is_locked(&dig_port->tc_lock) ||
> > @@ -604,7 +624,7 @@ bool intel_tc_port_ref_held(struct
> intel_digital_port *dig_port)
> > void intel_tc_port_get_link(struct intel_digital_port *dig_port,
> > int required_lanes)
> > {
> > - __intel_tc_port_lock(dig_port, required_lanes);
> > + __intel_tc_port_lock(dig_port, required_lanes, false);
> > dig_port->tc_link_refcount++;
> > intel_tc_port_unlock(dig_port);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> b/drivers/gpu/drm/i915/display/intel_tc.h
> > index b619e4736f85..57d157fc822f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_tc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> > @@ -13,6 +13,8 @@ struct intel_digital_port;
> > struct intel_encoder;
> >
> > bool intel_tc_port_connected(struct intel_encoder *encoder);
> > +void intel_tc_port_disconnect_phy(struct intel_digital_port *dig_port);
> > +
> > u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port);
> > u32 intel_tc_port_get_pin_assignment_mask(struct intel_digital_port
> *dig_port);
> > int intel_tc_port_fia_max_lane_count(struct intel_digital_port
> *dig_port);
> >
>
> --
> -----------
> Tim Gardner
> Canonical, Inc
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210720/0f47151d/attachment-0001.html>
More information about the kernel-team
mailing list