Re: [PATCH] drm/i915: set proper N/M in modeset

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

On Thu, Jul 14, 2016 at 03:06:21PM +0800, libin.yang@linux.intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
> 
> When modeset occurs and the LS_CLK is set to some
> special values in DP mode, the N/M need to be set
> manually if audio is playing.
> 
> Also, the patch applies
> commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset")
> to APL platform.
> 
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
>  drivers/gpu/drm/i915/intel_audio.c | 116 ++++++++++++++++++++++++++++++++-----
>  2 files changed, 108 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bfde75..2f9d00e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7351,6 +7351,12 @@ enum {
>  #define _HSW_AUD_CONFIG_B		0x65100
>  #define HSW_AUD_CFG(pipe)		_MMIO_PIPE(pipe, _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_B)
>  
> +#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
> +#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
> +#define HSW_AUD_M_CTS_ENABLE(pipe)		_MMIO_PIPE(pipe, _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
> +#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
> +#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
> +
>  #define _HSW_AUD_MISC_CTRL_A		0x65010
>  #define _HSW_AUD_MISC_CTRL_B		0x65110
>  #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe, _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 6700a7b..e2e4d4b 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -98,6 +98,22 @@ static const struct {
>  	{ 192000, TMDS_297M, 20480, 247500 },
>  };
>  
> +#define LC_533M 533250
> +#define LC_148M 148500
> +static const struct {
> +	int sample_rate;
> +	int clock;
> +	int n;
> +	int m;
> +} aud_nm[] = {
> +	{48000, LC_533M, 88875, 4096},
> +	{44100, LC_533M, 148125, 6272},
> +	{32000, LC_533M, 266625, 8192},
> +	{48000, LC_148M, 12375, 2048},
> +	{44100, LC_148M, 20625, 3136},
> +	{32000, LC_148M, 37125, 4096},
> +};
> +
>  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
>  static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted_mode)
>  {
> @@ -121,20 +137,50 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
>  	return hdmi_audio_clock[i].config;
>  }
>  
> -static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
> +static int audio_config_get_n(struct intel_crtc *crtc,
> +			      const struct drm_display_mode *mode, int rate)
> +{
> +	int i;
> +
> +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> +		for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> +			if ((rate == aud_ncts[i].sample_rate) &&
> +				(mode->clock == aud_ncts[i].clock)) {
> +				return aud_ncts[i].n;
> +			}
> +		}
> +	}
> +
> +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> +		for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> +			if ((rate == aud_nm[i].sample_rate) &&
> +				(mode->clock == aud_nm[i].clock)) {

So the spec says we should have "Maud / Naud = 512 * fs / f_LS_Clk",
where fs is the audio sample rate, and f_LS_CLK is the link symbol clock.
With that in mind this should not be looking at mode->clock but
crtc->config->port_clock.

So I don't actually understand why you say LS_CLK has "special" values.
It shouldn't. It's always either 162, 270, or 540 MHz.

Actually even the HDMI case is wrong in the code, it should be
looking at mode->crtc_clock instead of mode->clock. Or perhaps even
port_clock, if my reading of the HDMI spec is correct, but I never got
any sane answer from any hw folks to my questions about this :(

> +				return aud_nm[i].n;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int audio_config_get_m(struct intel_crtc *crtc,
> +			      const struct drm_display_mode *mode, int rate)
>  {
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> -		if ((rate == aud_ncts[i].sample_rate) &&
> -			(mode->clock == aud_ncts[i].clock)) {
> -			return aud_ncts[i].n;
> +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> +		for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> +			if ((rate == aud_nm[i].sample_rate) &&
> +				(mode->clock == aud_nm[i].clock)) {
> +				return aud_nm[i].m;
> +			}
>  		}
>  	}
> +
>  	return 0;
>  }
>  
> -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> +					 int n, uint32_t val)
>  {
>  	int n_low, n_up;
>  	uint32_t tmp = val;
> @@ -145,6 +191,23 @@ static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
>  	tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
>  			(n_low << AUD_CONFIG_LOWER_N_SHIFT) |
>  			AUD_CONFIG_N_PROG_ENABLE);
> +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> +		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> +	return tmp;
> +}
> +
> +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> +					 int m, uint32_t val)
> +{
> +	uint32_t tmp = val;
> +
> +	if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> +		return 0;
> +
> +	tmp |= m;
> +	tmp |= AUD_M_CTS_M_VALUE_INDEX;
> +	tmp |= AUD_M_CTS_M_PROG_ENABLE;
> +
>  	return tmp;
>  }
>  
> @@ -156,6 +219,10 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc,
>  		 (mode->clock == TMDS_296M)) &&
>  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
>  		return true;
> +	else if (((mode->clock == LC_533M) ||
> +		   (mode->clock == LC_148M)) &&
> +		  intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> +		return true;
>  	else
>  		return false;
>  }
> @@ -287,7 +354,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	struct intel_digital_port *intel_dig_port =
>  		enc_to_dig_port(&encoder->base);
>  	enum port port = intel_dig_port->port;
> -	uint32_t tmp;
> +	uint32_t tmp, m;
>  	int len, i;
>  	int n, rate;
>  
> @@ -343,15 +410,25 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  			DRM_ERROR("invalid port: %d\n", port);
>  			rate = 0;
>  		}
> -		n = audio_config_get_n(adjusted_mode, rate);
> +		n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
>  		if (n != 0)
> -			tmp = audio_config_setup_n_reg(n, tmp);
> +			tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
>  		else
>  			DRM_DEBUG_KMS("no suitable N value is found\n");
>  	}
>  
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
> +	/* setup m value for DP */
> +	if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
> +		m = audio_config_get_m(intel_crtc, adjusted_mode, rate);
> +		if (m != 0) {
> +			tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> +			tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> +			I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> +		}
> +	}
> +
>  	mutex_unlock(&dev_priv->av_mutex);
>  }
>  
> @@ -637,7 +714,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	struct drm_display_mode *mode;
>  	struct i915_audio_component *acomp = dev_priv->audio_component;
>  	enum pipe pipe = INVALID_PIPE;
> -	u32 tmp;
> +	u32 tmp, m;
>  	int n;
>  	int err = 0;
>  
> @@ -645,7 +722,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	if (!IS_SKYLAKE(dev_priv) &&
>  	    !IS_KABYLAKE(dev_priv) &&
>  	    !IS_BROADWELL(dev_priv) &&
> -	    !IS_HASWELL(dev_priv))
> +	    !IS_HASWELL(dev_priv) &&
> +	    !IS_BROXTON(dev_priv))
>  		return 0;
>  
>  	mutex_lock(&dev_priv->av_mutex);
> @@ -653,7 +731,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  	intel_encoder = dev_priv->dig_port_map[port];
>  	/* intel_encoder might be NULL for DP MST */
>  	if (!intel_encoder || !intel_encoder->base.crtc ||
> -	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
> +	    ((intel_encoder->type != INTEL_OUTPUT_HDMI) &&
> +	     (intel_encoder->type != INTEL_OUTPUT_DP))) {
>  		DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
>  		err = -ENODEV;
>  		goto unlock;
> @@ -681,7 +760,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  		goto unlock;
>  	}
>  
> -	n = audio_config_get_n(mode, rate);
> +	n = audio_config_get_n(crtc, mode, rate);
>  	if (n == 0) {
>  		DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
>  					  port_name(port));
> @@ -693,8 +772,17 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>  
>  	/* 3. set the N/CTS/M */
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> -	tmp = audio_config_setup_n_reg(n, tmp);
> +	tmp = audio_config_setup_n_reg(crtc, n, tmp);
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> +	/* setup m value for DP */
> +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> +		m = audio_config_get_m(crtc, mode, rate);
> +		if (m == 0)
> +			goto unlock;
> +		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> +		tmp = audio_config_setup_m_reg(crtc, m, tmp);
> +		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> +	}
>  
>   unlock:
>  	mutex_unlock(&dev_priv->av_mutex);
> -- 
> 1.9.1

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