NACK/Cmnt: [PATCH v2 1/1][SRU][H] drm/i915: Force a TypeC PHY disconnect during suspend/shutdown
Stefan Bader
stefan.bader at canonical.com
Fri Jul 16 05:53:53 UTC 2021
On 15.07.21 14:39, Chris Chiu wrote:
> On Wed, Jul 14, 2021 at 3:33 PM Stefan Bader <stefan.bader at canonical.com> wrote:
>>
>> On 01.07.21 12:53, 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
>>> https://cgit.freedesktop.org/drm-intel/?h=drm-intel-next)
>>> Signed-off-by: Chris Chiu <chris.chiu at canonical.com>
>>> ---
>>
>> This patch appears to be on linux-next now. However there is a major difference.
>> In intel_ddi.c the linux-next version actually adds the suspend/resume
>> functions. And looking into our Hirsute base it seems those must be re-added as
>> they got removed earlier.
>>
>> commit a171f8e7f466345a2a0c7bacdc7992777e1c1f37
>> Author: Imre Deak <imre.deak at intel.com>
>> Date: Fri Jun 28 17:36:34 2019 +0300
>>
>> drm/i915: Remove unneeded disconnect in TypeC legacy port mode
>>
>> -Stefan
>>
>
> After checking linux-next and Hirsute base, this upstream commit
> 151ec347b06a ("drm/i915: Force a TypeC PHY disconnect during
> suspend/shutdown") will add the intel_ddi_encoder_suspend() back which
> is removed by the commit in Hirsute you mentioned, and
> intel_ddi_encoder_suspend() in addition. It makes sense to me and I
> didn't see there's anything else needed to be done for this patch.
> Anything I missed which might cause a problem?
>
> Chris
>
>>> drivers/gpu/drm/i915/display/intel_ddi.c | 4 +--
>>> drivers/gpu/drm/i915/display/intel_tc.c | 34 +++++++++++++++++++-----
>>> drivers/gpu/drm/i915/display/intel_tc.h | 2 ++
>>> 3 files changed, 31 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..731e716b6382 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
>>> @@ -5432,8 +5432,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;
Are you not ^here attempt to set suspend/resume functions to something that does
not exist? Does this even compile? At least there should be some backport note
explaining why that section of the upstream patch was dropped.
-Stefan
>>> 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);
>>>
>>
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20210716/16fb0e80/attachment.sig>
More information about the kernel-team
mailing list