Re: [PATCH 09/23] drm/i915: Factor out common parts from TypeC port handling functions

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Jun 04, 2019 at 05:58:12PM +0300, Imre Deak wrote:
> Factor out helpers reading/parsing the TypeC specific registers, making
> current users of them clearer and letting us use them later.
> 
> While at it also:
> - Simplify icl_tc_phy_connect() with an early return in legacy mode.
> - Simplify the live status check using one bitmask for all HPD bits.
> - Remove a micro-optimisation of the repeated safe-mode clearing.
> - Make sure we fix the legacy port flag in all cases.
> 
> Except for the last two, no functional changes.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |   5 +-
>  drivers/gpu/drm/i915/intel_tc.c  | 166 +++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_tc.h  |   1 +
>  3 files changed, 103 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 8f223d48d562..d236839bee19 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2983,7 +2983,6 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(intel_dig_port->base.base.dev);
>  	enum port port = intel_dig_port->base.port;
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
>  	u32 ln0, ln1, lane_info;
>  
>  	if (intel_dig_port->tc_mode == TC_PORT_TBT_ALT)
> @@ -2997,9 +2996,7 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port)
>  		ln0 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE);
>  		ln1 &= ~(MG_DP_MODE_CFG_DP_X1_MODE | MG_DP_MODE_CFG_DP_X2_MODE);
>  
> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> -			    DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> +		lane_info = intel_tc_port_get_lane_info(intel_dig_port);
>  
>  		switch (lane_info) {
>  		case 0x1:
> diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
> index 07488235b67a..3fdcfa2bbaee 100644
> --- a/drivers/gpu/drm/i915/intel_tc.c
> +++ b/drivers/gpu/drm/i915/intel_tc.c
> @@ -43,10 +43,19 @@ static const char *tc_port_mode_name(enum tc_port_mode mode)
>  	return names[mode];
>  }
>  
> -int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> +u32 intel_tc_port_get_lane_info(struct intel_digital_port *dig_port)

get_lane_mask() or something?

>  {
>  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	u32 lane_info = I915_READ(PORT_TX_DFLEXDPSP);
> +
> +	return (lane_info & DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> +	       DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> +}
> +
> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>  	intel_wakeref_t wakeref;
>  	u32 lane_info;
>  
> @@ -55,9 +64,7 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
>  
>  	lane_info = 0;
>  	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> +		lane_info = intel_tc_port_get_lane_info(dig_port);
>  
>  	switch (lane_info) {
>  	default:
> @@ -75,6 +82,69 @@ int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
>  	}
>  }
>  
> +static void tc_port_fixup_legacy_flag(struct intel_digital_port *dig_port,
> +				      u32 live_status_mask)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	u32 valid_hpd_mask = dig_port->tc_legacy_port ? BIT(TC_PORT_LEGACY) :
> +							~BIT(TC_PORT_LEGACY);
> +
> +	if (!(live_status_mask & ~valid_hpd_mask))
> +		return;

A bit too many nots for me to follow. I guess what it's doing is
checking if any of the other bits are set, and if so it assumes the
mode was misdeclared in vbt? 

Actually, won't this end up ping-ponging back and forth if the
hw really reports both legacy and non-legacy bits as set?

> +
> +	/* If live status mismatches the VBT flag, trust the live status. */
> +	DRM_ERROR("Port %s: live status %08x mismatches the legacy port flag, fix flag\n",
> +		  tc_port_name(dev_priv, tc_port), live_status_mask);
> +
> +	dig_port->tc_legacy_port = !dig_port->tc_legacy_port;
> +}
> +

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx