Re: [PATCH 5/8] drm/i915/cnl: Move voltage check into ddi buf trans functions.

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

On Tue, Aug 29, 2017 at 04:22:27PM -0700, Rodrigo Vivi wrote:
> Let's start converging CNL buf translations to same style
> used on previous platforms. So first thing is to use the
> standard signature so we don't need to propagate the voltage
> check into other parts of the code, but only on the parts
> that it is really useful.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 48 ++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 506782c1a62a..7b547a7f6c2b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1830,9 +1830,10 @@ u8 intel_ddi_dp_voltage_max(struct intel_encoder *encoder)
>  }
>  
>  static const struct cnl_ddi_buf_trans *
> -cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv,
> -		       u32 voltage, int *n_entries)
> +cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> +	u32 voltage = I915_READ(CNL_PORT_COMP_DW3) & VOLTAGE_INFO_MASK;

I wonder if we should just cache that somewhere. My main worry is
whether the device is always awake when we call this code.

> +
>  	if (voltage == VOLTAGE_INFO_0_85V) {
>  		*n_entries = ARRAY_SIZE(cnl_ddi_translations_hdmi_0_85V);
>  		return cnl_ddi_translations_hdmi_0_85V;
> @@ -1842,14 +1843,16 @@ cnl_get_buf_trans_hdmi(struct drm_i915_private *dev_priv,
>  	} else if (voltage == VOLTAGE_INFO_1_05V) {
>  		*n_entries = ARRAY_SIZE(cnl_ddi_translations_hdmi_1_05V);
>  		return cnl_ddi_translations_hdmi_1_05V;
> -	}
> +	} else
> +		MISSING_CASE(voltage);

nit: Looks like these if ladders could be turned into switch statements.


Anyways, patch lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	return NULL;
>  }
>  
>  static const struct cnl_ddi_buf_trans *
> -cnl_get_buf_trans_dp(struct drm_i915_private *dev_priv,
> -		     u32 voltage, int *n_entries)
> +cnl_get_buf_trans_dp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> +	u32 voltage = I915_READ(CNL_PORT_COMP_DW3) & VOLTAGE_INFO_MASK;
> +
>  	if (voltage == VOLTAGE_INFO_0_85V) {
>  		*n_entries = ARRAY_SIZE(cnl_ddi_translations_dp_0_85V);
>  		return cnl_ddi_translations_dp_0_85V;
> @@ -1859,14 +1862,16 @@ cnl_get_buf_trans_dp(struct drm_i915_private *dev_priv,
>  	} else if (voltage == VOLTAGE_INFO_1_05V) {
>  		*n_entries = ARRAY_SIZE(cnl_ddi_translations_dp_1_05V);
>  		return cnl_ddi_translations_dp_1_05V;
> -	}
> +	} else
> +		MISSING_CASE(voltage);
>  	return NULL;
>  }
>  
>  static const struct cnl_ddi_buf_trans *
> -cnl_get_buf_trans_edp(struct drm_i915_private *dev_priv,
> -		      u32 voltage, int *n_entries)
> +cnl_get_buf_trans_edp(struct drm_i915_private *dev_priv, int *n_entries)
>  {
> +	u32 voltage = I915_READ(CNL_PORT_COMP_DW3) & VOLTAGE_INFO_MASK;
> +
>  	if (dev_priv->vbt.edp.low_vswing) {
>  		if (voltage == VOLTAGE_INFO_0_85V) {
>  			*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_0_85V);
> @@ -1877,10 +1882,11 @@ cnl_get_buf_trans_edp(struct drm_i915_private *dev_priv,
>  		} else if (voltage == VOLTAGE_INFO_1_05V) {
>  			*n_entries = ARRAY_SIZE(cnl_ddi_translations_edp_1_05V);
>  			return cnl_ddi_translations_edp_1_05V;
> -		}
> +		} else
> +			MISSING_CASE(voltage);
>  		return NULL;
>  	} else {
> -		return cnl_get_buf_trans_dp(dev_priv, voltage, n_entries);
> +		return cnl_get_buf_trans_dp(dev_priv, n_entries);
>  	}
>  }
>  
> @@ -1888,31 +1894,19 @@ static void cnl_ddi_vswing_program(struct drm_i915_private *dev_priv,
>  				    u32 level, enum port port, int type)
>  {
>  	const struct cnl_ddi_buf_trans *ddi_translations = NULL;
> -	u32 n_entries, val, voltage;
> +	u32 n_entries, val;
>  	int ln;
>  
> -	/*
> -	 * Values for each port type are listed in
> -	 * voltage swing programming tables.
> -	 * Vccio voltage found in PORT_COMP_DW3.
> -	 */
> -	voltage = I915_READ(CNL_PORT_COMP_DW3) & VOLTAGE_INFO_MASK;
> -
>  	if (type == INTEL_OUTPUT_HDMI) {
> -		ddi_translations = cnl_get_buf_trans_hdmi(dev_priv,
> -							  voltage, &n_entries);
> +		ddi_translations = cnl_get_buf_trans_hdmi(dev_priv, &n_entries);
>  	} else if (type == INTEL_OUTPUT_DP) {
> -		ddi_translations = cnl_get_buf_trans_dp(dev_priv,
> -							voltage, &n_entries);
> +		ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries);
>  	} else if (type == INTEL_OUTPUT_EDP) {
> -		ddi_translations = cnl_get_buf_trans_edp(dev_priv,
> -							 voltage, &n_entries);
> +		ddi_translations = cnl_get_buf_trans_edp(dev_priv, &n_entries);
>  	}
>  
> -	if (ddi_translations == NULL) {
> -		MISSING_CASE(voltage);
> +	if (WARN_ON(ddi_translations == NULL))
>  		return;
> -	}
>  
>  	if (level >= n_entries) {
>  		DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1);
> -- 
> 2.13.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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