Re: [Intel-gfx] [PATCH 01/14] drm/i915/tc: Abort DP AUX transfer on a disconnected TC port

From: "Kahola, Mika" <mika.kahola@intel.com>
To: "Deak, Imre" <imre.deak@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: Chris Chiu <chris.chiu@canonical.com>
Subject: Re: [Intel-gfx] [PATCH 01/14] drm/i915/tc: Abort DP AUX transfer on a disconnected TC port
Date: Tue, 21 Mar 2023 11:09:41 +0000	[thread overview]
Message-ID: <MW4PR11MB70547A0C5A6E323F4DC67826EF819@MW4PR11MB7054.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20230316131724.359612-2-imre.deak@intel.com>

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Thursday, March 16, 2023 3:17 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Chris Chiu <chris.chiu@canonical.com>
> Subject: [Intel-gfx] [PATCH 01/14] drm/i915/tc: Abort DP AUX transfer on a
> disconnected TC port
> 
> On TC ports the 4ms AUX timeout combined with the 5 * 32 retry attempts
> during DPCD accesses adds a 640ms delay to each access if the sink is
> disconnected. This in turn slows down a modeset during which the sink is
> disconnected (for instance a disabling modeset).
> 
> Prevent the above delay by aborting AUX transfers on a TC port with a
> disconnected sink.
> 
> The DP 1.4a link CTS (4.2.1.5 Source Device Inactive HPD / Inactive AUX
> Test") also requires not to initiate AUX transfers on disconnected DP ports in
> general, however this patch doesn't change the behavior on non-TC ports,
> leaving that for a follow-up.
> 
> Reported-and-tested-by: Chris Chiu <chris.chiu@canonical.com>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8279
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_aux.c | 15 +++++++++++++--
>  drivers/gpu/drm/i915/display/intel_tc.c     | 15 +++++++++++----
>  drivers/gpu/drm/i915/display/intel_tc.h     |  1 +
>  3 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> index 96967e21c94c2..eb07dc5d87099 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> @@ -205,8 +205,19 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
>  	for (i = 0; i < ARRAY_SIZE(ch_data); i++)
>  		ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i);
> 
> -	if (is_tc_port)
> +	if (is_tc_port) {
>  		intel_tc_port_lock(dig_port);
> +		/*
> +		 * Abort transfers on a disconnected port as required by
> +		 * DP 1.4a link CTS 4.2.1.5, also avoiding the long AUX
> +		 * timeouts that would otherwise happen.
> +		 * TODO: abort the transfer on non-TC ports as well.
> +		 */
> +		if (!intel_tc_port_connected_locked(&dig_port->base)) {
> +			ret = -ENXIO;

TC port being a physical object this error code could be ENODEV as well, right?

Anyway, patch looks ok to me.

Reviewed-by: Mika Kahola <mika.kahola@intel.com>

> +			goto out_unlock;
> +		}
> +	}
> 
>  	aux_domain = intel_aux_power_domain(dig_port);
> 
> @@ -367,7 +378,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> 
>  	intel_pps_unlock(intel_dp, pps_wakeref);
>  	intel_display_power_put_async(i915, aux_domain, aux_wakeref);
> -
> +out_unlock:
>  	if (is_tc_port)
>  		intel_tc_port_unlock(dig_port);
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c
> b/drivers/gpu/drm/i915/display/intel_tc.c
> index f45328712bff1..050f998284592 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.c
> +++ b/drivers/gpu/drm/i915/display/intel_tc.c
> @@ -768,16 +768,23 @@ void intel_tc_port_sanitize_mode(struct
> intel_digital_port *dig_port)
>   * connected ports are usable, and avoids exposing to the users objects they
>   * can't really use.
>   */
> +bool intel_tc_port_connected_locked(struct intel_encoder *encoder) {
> +	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
> +
> +	drm_WARN_ON(&i915->drm, !intel_tc_port_ref_held(dig_port));
> +
> +	return tc_port_live_status_mask(dig_port) & BIT(dig_port->tc_mode); }
> +
>  bool intel_tc_port_connected(struct intel_encoder *encoder)  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	bool is_connected;
> 
>  	intel_tc_port_lock(dig_port);
> -
> -	is_connected = tc_port_live_status_mask(dig_port) &
> -		       BIT(dig_port->tc_mode);
> -
> +	is_connected = intel_tc_port_connected_locked(encoder);
>  	intel_tc_port_unlock(dig_port);
> 
>  	return is_connected;
> diff --git a/drivers/gpu/drm/i915/display/intel_tc.h
> b/drivers/gpu/drm/i915/display/intel_tc.h
> index d54082e2d5e8d..93813056043a5 100644
> --- a/drivers/gpu/drm/i915/display/intel_tc.h
> +++ b/drivers/gpu/drm/i915/display/intel_tc.h
> @@ -17,6 +17,7 @@ bool intel_tc_port_in_dp_alt_mode(struct
> intel_digital_port *dig_port);  bool intel_tc_port_in_legacy_mode(struct
> intel_digital_port *dig_port);
> 
>  bool intel_tc_port_connected(struct intel_encoder *encoder);
> +bool intel_tc_port_connected_locked(struct intel_encoder *encoder);
> 
>  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);
> --
> 2.37.1