drm/i915: set proper N/M in modeset

Commit Message

[e-mail] Aug. 2, 2016, 1:35 a.m. UTC

From: Libin Yang <[e-mail]>

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.

The relationship of Maud and Naud is expressed in
the following equation:
Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Also, the patch applies
commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset")
to APL platform.

Signed-off-by: Libin Yang <[e-mail]>
---
 drivers/gpu/drm/i915/i915_reg.h    |   6 ++
 drivers/gpu/drm/i915/intel_audio.c | 122 +++++++++++++++++++++++++++++++------
 2 files changed, 111 insertions(+), 17 deletions(-)

Comments

Ville Syrjälä Aug. 2, 2016, 10:47 a.m. UTC | #1

On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
> 
> 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.
> 
> The relationship of Maud and Naud is expressed in
> the following equation:
> Maud/Naud = 512 * fs / f_LS_Clk
> 
> Please refer VESA DisplayPort Standard spec for details.
> 
> Also, the patch applies
> commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset")
> to APL platform.
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
>  drivers/gpu/drm/i915/intel_audio.c | 122 +++++++++++++++++++++++++++++++------
>  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> +#define LC_162M 162000

Do we have some explanation why 2.7 doesn't need M/N programming,
but 1.62 and 5.4 do?

And I see you're only doing this on HSW+. Earlier platforms don't need
this?

> +static const struct {
> +	int sample_rate;
> +	int clock;
> +	int n;
> +	int m;

Can save a bit of space by using u16 for m and n.

> +} aud_nm[] = {
> +	{48000, LC_540M, 5625, 256},`
> +	{44100, LC_540M, 9375, 392},
> +	{32000, LC_540M, 16875, 512},
> +	{48000, LC_162M, 3375, 512},
> +	{44100, LC_162M, 5625, 784},
> +	{32000, LC_162M, 10125, 1024
> +};

The numbers look good, but what about other sample rates? For HDMI we go
up to 192kHz, why not for DP?

> +
>  /* 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 *adjusted_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) &&
> +				(adjusted_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) &&
> +				(crtc->config->port_clock == aud_nm[i].clock)) {
> +				return aud_nm[i].n;
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> +				(crtc->config->port_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,17 +191,38 @@ 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;
>  }
>  
>  /* check whether N/CTS/M need be set manually */
>  static bool audio_rate_need_prog(struct intel_crtc *crtc,
> -				 const struct drm_display_mode *mode)
> +				 const struct drm_display_mode *adjusted_mode)
>  {
> -	if (((mode->clock == TMDS_297M) ||
> -		 (mode->clock == TMDS_296M)) &&
> +	if (((adjusted_mode->clock == TMDS_297M) ||
> +		 (adjusted_mode->clock == TMDS_296M)) &&
>  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
>  		return true;
> +	else if (((crtc->config->port_clock == LC_540M) ||
> +		   (crtc->config->port_clock == LC_162M)) &&
> +		  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, 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;

HAS_DDI perhaps?

>  
>  	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, 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

Jani Nikula Aug. 2, 2016, 10:52 a.m. UTC | #2

On Tue, 02 Aug 2016, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
>
> 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.
>
> The relationship of Maud and Naud is expressed in
> the following equation:
> Maud/Naud = 512 * fs / f_LS_Clk
>
> Please refer VESA DisplayPort Standard spec for details.
>
> Also, the patch applies
> commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset")
> to APL platform.

Whenever a commit message says "also", it makes me think the patch
should probably be split to several patches. And that is certainly true
here.

Yang, Libin Aug. 2, 2016, 1:58 p.m. UTC | #3

Hi Ville

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Tuesday, August 2, 2016 6:47 PM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; Vetter, Daniel
> <[e-mail]>; [e-mail]; Yang, Libin <[e-mail]>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail] wrote:
> > From: Libin Yang <[e-mail]>
> >
> > 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.
> >
> > The relationship of Maud and Naud is expressed in the following
> > equation:
> > Maud/Naud = 512 * fs / f_LS_Clk
> >
> > Please refer VESA DisplayPort Standard spec for details.
> >
> > Also, the patch applies
> > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > platform.
> >
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 122
> > +++++++++++++++++++++++++++++++------
> >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > +#define LC_162M 162000
> 
> Do we have some explanation why 2.7 doesn't need M/N programming, but
> 1.62 and 5.4 do?

I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the test.
5.4 is for 4K and 1.62 is for 1080p.

> 
> And I see you're only doing this on HSW+. Earlier platforms don't need this?

We are not supporting earlier platforms and I'm not sure whether the
old platforms supports 4K DP or not.

> 
> > +static const struct {
> > +	int sample_rate;
> > +	int clock;
> > +	int n;
> > +	int m;
> 
> Can save a bit of space by using u16 for m and n.

OK, I will do it in next version.

> 
> > +} aud_nm[] = {
> > +	{48000, LC_540M, 5625, 256},`
> > +	{44100, LC_540M, 9375, 392},
> > +	{32000, LC_540M, 16875, 512},
> > +	{48000, LC_162M, 3375, 512},
> > +	{44100, LC_162M, 5625, 784},
> > +	{32000, LC_162M, 10125, 1024
> > +};
> 
> The numbers look good, but what about other sample rates? For HDMI we go
> up to 192kHz, why not for DP?

Our test only includes 32K, 44.1K and 48K :)
I will add the support if you think we should.

> 
> > +
> >  /* 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 *adjusted_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) &&
> > +				(adjusted_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) &&
> > +				(crtc->config->port_clock == aud_nm[i].clock))
> {
> > +				return aud_nm[i].n;
> > +			}
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > +				(crtc->config->port_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,17 +191,38 @@ 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;
> >  }
> >
> >  /* check whether N/CTS/M need be set manually */  static bool
> > audio_rate_need_prog(struct intel_crtc *crtc,
> > -				 const struct drm_display_mode *mode)
> > +				 const struct drm_display_mode
> *adjusted_mode)
> >  {
> > -	if (((mode->clock == TMDS_297M) ||
> > -		 (mode->clock == TMDS_296M)) &&
> > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > +		 (adjusted_mode->clock == TMDS_296M)) &&
> >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> >  		return true;
> > +	else if (((crtc->config->port_clock == LC_540M) ||
> > +		   (crtc->config->port_clock == LC_162M)) &&
> > +		  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, 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;
> 
> HAS_DDI perhaps?

Do you mean we should add "&& HAS_DDI"? 
Could you please give me more details?

Regards,
Libin

> 
> >
> >  	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, 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

Yang, Libin Aug. 2, 2016, 2 p.m. UTC | #4

Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:[e-mail]]
> Sent: Tuesday, August 2, 2016 6:53 PM
> To: [e-mail]; [e-mail];
> [e-mail]; Vetter, Daniel <[e-mail]>;
> [e-mail]
> Cc: Yang, Libin <[e-mail]>; Libin Yang
> <[e-mail]>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Tue, 02 Aug 2016, [e-mail] wrote:
> > From: Libin Yang <[e-mail]>
> >
> > 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.
> >
> > The relationship of Maud and Naud is expressed in the following
> > equation:
> > Maud/Naud = 512 * fs / f_LS_Clk
> >
> > Please refer VESA DisplayPort Standard spec for details.
> >
> > Also, the patch applies
> > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > platform.
> 
> Whenever a commit message says "also", it makes me think the patch should
> probably be split to several patches. And that is certainly true here.

I will split the patch in next version.

Regards,
Libin

> 
> 
> --
> Jani Nikula, Intel Open Source Technology Center

Ville Syrjälä Aug. 2, 2016, 4:59 p.m. UTC | #5

On Tue, Aug 02, 2016 at 01:58:51PM +0000, Yang, Libin wrote:
> Hi Ville
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Tuesday, August 2, 2016 6:47 PM
> > To: [e-mail]
> > Cc: [e-mail]; [e-mail]; Vetter, Daniel
> > <[e-mail]>; [e-mail]; Yang, Libin <[e-mail]>
> > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > 
> > On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail] wrote:
> > > From: Libin Yang <[e-mail]>
> > >
> > > 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.
> > >
> > > The relationship of Maud and Naud is expressed in the following
> > > equation:
> > > Maud/Naud = 512 * fs / f_LS_Clk
> > >
> > > Please refer VESA DisplayPort Standard spec for details.
> > >
> > > Also, the patch applies
> > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > > platform.
> > >
> > > Signed-off-by: Libin Yang <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > +++++++++++++++++++++++++++++++------
> > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > +#define LC_162M 162000
> > 
> > Do we have some explanation why 2.7 doesn't need M/N programming, but
> > 1.62 and 5.4 do?
> 
> I didn't use 2.7 because I can't find a mode using 2.7.

Hmm. Maybe we should add some knobs to force a specific bpc/link rate/number
of lanes to help with this kind of testing. Currently you just get what
you get, which isn't so nice when you want to test all variations.
...
OK, so I just went ahead and did that. Here's a branch:

git://github.com/vsyrjala/linux.git modparam_clock_bpp_limit

For your DP testing just setting
i915.max_port_clock=162000 or i915.max_port_clock=270000 and then
forcing a modeset should do the trick.

> So I can't do the test.
> 5.4 is for 4K and 1.62 is for 1080p.
> 
> > 
> > And I see you're only doing this on HSW+. Earlier platforms don't need this?
> 
> We are not supporting earlier platforms and I'm not sure whether the
> old platforms supports 4K DP or not.

SNB-IVB dotclock can go up to 360Mhz, ILK up to 405 Mhz. At least in
theory. The DP link is limited to 4 x 2.7 for all. From the those the
dotclock limit is the one you should hit first since DP can always fall
back to 6bpc and that should be correspond to a dotclock of 480 MHz.
Anyways, 360MHz is plenty for 4k@30.


The question really is why we need to do this in the first place.
There's nothing in the spec telling is it's really required. All I can
find in the DP spec is "Maud value is set to 2^15 (=32,768) when the
audio clock is asynchronous to the LS_Clk.", and then 

Thinking about it a bit more, on HSW+ we do drive DP ports with the
LCPLL, which is also reponsible for cdclk, and there are some vague
hints that audio may be clocked via cdclk. So if the DDI clock and the
audio clock are coming from the same reference, I suppose they are
considered synchronous, which may explain why this is needed. It's all
very poorly documented though, so I can't be sure.

As for the older platforms, the clocks are even less well documented.
The audio stuff is in the PCH, which is also where the DPLLs live, but
I have no idea where any audio clocks come from.

> 
> > 
> > > +static const struct {
> > > +	int sample_rate;
> > > +	int clock;
> > > +	int n;
> > > +	int m;
> > 
> > Can save a bit of space by using u16 for m and n.
> 
> OK, I will do it in next version.
> 
> > 
> > > +} aud_nm[] = {
> > > +	{48000, LC_540M, 5625, 256},`
> > > +	{44100, LC_540M, 9375, 392},
> > > +	{32000, LC_540M, 16875, 512},
> > > +	{48000, LC_162M, 3375, 512},
> > > +	{44100, LC_162M, 5625, 784},
> > > +	{32000, LC_162M, 10125, 1024
> > > +};
> > 
> > The numbers look good, but what about other sample rates? For HDMI we go
> > up to 192kHz, why not for DP?
> 
> Our test only includes 32K, 44.1K and 48K :)
> I will add the support if you think we should.

What sample rates can the user select? That should be the only
relevant question here.

> 
> > 
> > > +
> > >  /* 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 *adjusted_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) &&
> > > +				(adjusted_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) &&
> > > +				(crtc->config->port_clock == aud_nm[i].clock))
> > {
> > > +				return aud_nm[i].n;
> > > +			}
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > >  }
> > >
> > >  /* check whether N/CTS/M need be set manually */  static bool
> > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > -				 const struct drm_display_mode *mode)
> > > +				 const struct drm_display_mode
> > *adjusted_mode)
> > >  {
> > > -	if (((mode->clock == TMDS_297M) ||
> > > -		 (mode->clock == TMDS_296M)) &&
> > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > >  		return true;
> > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > +		  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, 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;
> > 
> > HAS_DDI perhaps?
> 
> Do you mean we should add "&& HAS_DDI"? 
> Could you please give me more details?

HAS_DDI alone should be enough.

> 
> Regards,
> Libin
> 
> > 
> > >
> > >  	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, 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

Yang, Libin Aug. 4, 2016, 2:48 a.m. UTC | #6

Hi Ville,

> -----Original Message-----
> From: Yang, Libin
> Sent: Tuesday, August 2, 2016 9:59 PM
> To: Ville Syrjälä <[e-mail]>; [e-mail]
> Cc: [e-mail]; [e-mail]; Vetter, Daniel
> <[e-mail]>; [e-mail]
> Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> 
> Hi Ville
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Tuesday, August 2, 2016 6:47 PM
> > To: [e-mail]
> > Cc: [e-mail]; [e-mail];
> > Vetter, Daniel <[e-mail]>; [e-mail]; Yang, Libin
> > <[e-mail]>
> > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> >
> > On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail]
> wrote:
> > > From: Libin Yang <[e-mail]>
> > >
> > > 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.
> > >
> > > The relationship of Maud and Naud is expressed in the following
> > > equation:
> > > Maud/Naud = 512 * fs / f_LS_Clk
> > >
> > > Please refer VESA DisplayPort Standard spec for details.
> > >
> > > Also, the patch applies
> > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > > platform.
> > >
> > > Signed-off-by: Libin Yang <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > +++++++++++++++++++++++++++++++------
> > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > +#define LC_162M 162000
> >
> > Do we have some explanation why 2.7 doesn't need M/N programming, but
> > 1.62 and 5.4 do?
> 
> I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the test.
> 5.4 is for 4K and 1.62 is for 1080p.
> 
> >
> > And I see you're only doing this on HSW+. Earlier platforms don't need this?
> 
> We are not supporting earlier platforms and I'm not sure whether the old
> platforms supports 4K DP or not.
> 
> >
> > > +static const struct {
> > > +	int sample_rate;
> > > +	int clock;
> > > +	int n;
> > > +	int m;
> >
> > Can save a bit of space by using u16 for m and n.
> 
> OK, I will do it in next version.
> 
> >
> > > +} aud_nm[] = {
> > > +	{48000, LC_540M, 5625, 256},`
> > > +	{44100, LC_540M, 9375, 392},
> > > +	{32000, LC_540M, 16875, 512},
> > > +	{48000, LC_162M, 3375, 512},
> > > +	{44100, LC_162M, 5625, 784},
> > > +	{32000, LC_162M, 10125, 1024
> > > +};
> >
> > The numbers look good, but what about other sample rates? For HDMI we
> > go up to 192kHz, why not for DP?
> 
> Our test only includes 32K, 44.1K and 48K :) I will add the support if you think
> we should.

I will not add 192KHz, 96KHz and etc rate support as based on my test
these rate will be not used in DP. It will be converted to 48KHz.

> 
> >
> > > +
> > >  /* 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 *adjusted_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) &&
> > > +				(adjusted_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) &&
> > > +				(crtc->config->port_clock == aud_nm[i].clock))
> > {
> > > +				return aud_nm[i].n;
> > > +			}
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > >  }
> > >
> > >  /* check whether N/CTS/M need be set manually */  static bool
> > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > -				 const struct drm_display_mode *mode)
> > > +				 const struct drm_display_mode
> > *adjusted_mode)
> > >  {
> > > -	if (((mode->clock == TMDS_297M) ||
> > > -		 (mode->clock == TMDS_296M)) &&
> > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > >  		return true;
> > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > +		  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, 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;
> >
> > HAS_DDI perhaps?
> 
> Do you mean we should add "&& HAS_DDI"?
> Could you please give me more details?
> 
> Regards,
> Libin
> 
> >
> > >
> > >  	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, 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

Ville Syrjälä Aug. 4, 2016, 5:39 a.m. UTC | #7

On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> Hi Ville,
> 
> > -----Original Message-----
> > From: Yang, Libin
> > Sent: Tuesday, August 2, 2016 9:59 PM
> > To: Ville Syrjälä <[e-mail]>; [e-mail]
> > Cc: [e-mail]; [e-mail]; Vetter, Daniel
> > <[e-mail]>; [e-mail]
> > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > 
> > Hi Ville
> > 
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > To: [e-mail]
> > > Cc: [e-mail]; [e-mail];
> > > Vetter, Daniel <[e-mail]>; [e-mail]; Yang, Libin
> > > <[e-mail]>
> > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail]
> > wrote:
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > 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.
> > > >
> > > > The relationship of Maud and Naud is expressed in the following
> > > > equation:
> > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > >
> > > > Please refer VESA DisplayPort Standard spec for details.
> > > >
> > > > Also, the patch applies
> > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > > > platform.
> > > >
> > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > +++++++++++++++++++++++++++++++------
> > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > +#define LC_162M 162000
> > >
> > > Do we have some explanation why 2.7 doesn't need M/N programming, but
> > > 1.62 and 5.4 do?
> > 
> > I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the test.
> > 5.4 is for 4K and 1.62 is for 1080p.
> > 
> > >
> > > And I see you're only doing this on HSW+. Earlier platforms don't need this?
> > 
> > We are not supporting earlier platforms and I'm not sure whether the old
> > platforms supports 4K DP or not.
> > 
> > >
> > > > +static const struct {
> > > > +	int sample_rate;
> > > > +	int clock;
> > > > +	int n;
> > > > +	int m;
> > >
> > > Can save a bit of space by using u16 for m and n.
> > 
> > OK, I will do it in next version.
> > 
> > >
> > > > +} aud_nm[] = {
> > > > +	{48000, LC_540M, 5625, 256},`
> > > > +	{44100, LC_540M, 9375, 392},
> > > > +	{32000, LC_540M, 16875, 512},
> > > > +	{48000, LC_162M, 3375, 512},
> > > > +	{44100, LC_162M, 5625, 784},
> > > > +	{32000, LC_162M, 10125, 1024
> > > > +};
> > >
> > > The numbers look good, but what about other sample rates? For HDMI we
> > > go up to 192kHz, why not for DP?
> > 
> > Our test only includes 32K, 44.1K and 48K :) I will add the support if you think
> > we should.
> 
> I will not add 192KHz, 96KHz and etc rate support as based on my test
> these rate will be not used in DP. It will be converted to 48KHz.

How about basing that decision on what's actually allowed by the driver?
I can do 96kHz DP audio on my HSW just fine here. So clearly if the
display supports it, there is nothing on the ALSA side that would prevent
it from being used.

> 
> > 
> > >
> > > > +
> > > >  /* 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 *adjusted_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) &&
> > > > +				(adjusted_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) &&
> > > > +				(crtc->config->port_clock == aud_nm[i].clock))
> > > {
> > > > +				return aud_nm[i].n;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > >  }
> > > >
> > > >  /* check whether N/CTS/M need be set manually */  static bool
> > > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > > -				 const struct drm_display_mode *mode)
> > > > +				 const struct drm_display_mode
> > > *adjusted_mode)
> > > >  {
> > > > -	if (((mode->clock == TMDS_297M) ||
> > > > -		 (mode->clock == TMDS_296M)) &&
> > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > >  		return true;
> > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > +		  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, 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;
> > >
> > > HAS_DDI perhaps?
> > 
> > Do you mean we should add "&& HAS_DDI"?
> > Could you please give me more details?
> > 
> > Regards,
> > Libin
> > 
> > >
> > > >
> > > >  	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, 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

Yang, Libin Aug. 4, 2016, 5:46 a.m. UTC | #8

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Wednesday, August 3, 2016 12:59 AM
> To: Yang, Libin <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Vetter, Daniel <[e-mail]>;
> [e-mail]
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Tue, Aug 02, 2016 at 01:58:51PM +0000, Yang, Libin wrote:
> > Hi Ville
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > To: [e-mail]
> > > Cc: [e-mail]; [e-mail];
> > > Vetter, Daniel <[e-mail]>; [e-mail]; Yang, Libin
> > > <[e-mail]>
> > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > On Tue, Aug 02, 2016 at 09:35:10AM +0800, [e-mail]
> wrote:
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > 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.
> > > >
> > > > The relationship of Maud and Naud is expressed in the following
> > > > equation:
> > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > >
> > > > Please refer VESA DisplayPort Standard spec for details.
> > > >
> > > > Also, the patch applies
> > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to
> > > > APL platform.
> > > >
> > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > +++++++++++++++++++++++++++++++------
> > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > +#define LC_162M 162000
> > >
> > > Do we have some explanation why 2.7 doesn't need M/N programming,
> > > but
> > > 1.62 and 5.4 do?
> >
> > I didn't use 2.7 because I can't find a mode using 2.7.
> 
> Hmm. Maybe we should add some knobs to force a specific bpc/link
> rate/number of lanes to help with this kind of testing. Currently you just get
> what you get, which isn't so nice when you want to test all variations.
> ...
> OK, so I just went ahead and did that. Here's a branch:
> 
> git://github.com/vsyrjala/linux.git modparam_clock_bpp_limit
> 
> For your DP testing just setting
> i915.max_port_clock=162000 or i915.max_port_clock=270000 and then
> forcing a modeset should do the trick.

Thanks for the new branch. It seems download is very slow, less than
10Kib/s. So I will submit the new patches firstly and then do the test.
Fortunately, I found there is recommended data for 340MHz in the spec.
I copied the data to the patch and suppose the data should be accurate. 

> 
> > So I can't do the test.
> > 5.4 is for 4K and 1.62 is for 1080p.
> >
> > >
> > > And I see you're only doing this on HSW+. Earlier platforms don't need this?
> >
> > We are not supporting earlier platforms and I'm not sure whether the
> > old platforms supports 4K DP or not.
> 
> SNB-IVB dotclock can go up to 360Mhz, ILK up to 405 Mhz. At least in theory.
> The DP link is limited to 4 x 2.7 for all. From the those the dotclock limit is the
> one you should hit first since DP can always fall back to 6bpc and that should
> be correspond to a dotclock of 480 MHz.
> Anyways, 360MHz is plenty for 4k@30.
> 
> 
> The question really is why we need to do this in the first place.
> There's nothing in the spec telling is it's really required. All I can find in the DP
> spec is "Maud value is set to 2^15 (=32,768) when the audio clock is
> asynchronous to the LS_Clk.", and then

We made the patch because we found the HW can't calculate the value this
will cause there is several seconds silence at the beginning of audio playback.
With this patch, the silence is much shorter than before and is acceptable. 

> 
> Thinking about it a bit more, on HSW+ we do drive DP ports with the LCPLL,
> which is also reponsible for cdclk, and there are some vague hints that audio
> may be clocked via cdclk. So if the DDI clock and the audio clock are coming
> from the same reference, I suppose they are considered synchronous, which
> may explain why this is needed. It's all very poorly documented though, so I
> can't be sure.

Yes, audio is using cdclk. Changing the cdclk will impact audio function.
However N/M, N/CTS is used to recover the audio clock in monitor. 
Monitor's PLL will use these values, together with the clock transferred
by the lanes, to recover the clock for audio.

> 
> As for the older platforms, the clocks are even less well documented.
> The audio stuff is in the PCH, which is also where the DPLLs live, but I have no
> idea where any audio clocks come from.
> 
> >
> > >
> > > > +static const struct {
> > > > +	int sample_rate;
> > > > +	int clock;
> > > > +	int n;
> > > > +	int m;
> > >
> > > Can save a bit of space by using u16 for m and n.
> >
> > OK, I will do it in next version.
> >
> > >
> > > > +} aud_nm[] = {
> > > > +	{48000, LC_540M, 5625, 256},`
> > > > +	{44100, LC_540M, 9375, 392},
> > > > +	{32000, LC_540M, 16875, 512},
> > > > +	{48000, LC_162M, 3375, 512},
> > > > +	{44100, LC_162M, 5625, 784},
> > > > +	{32000, LC_162M, 10125, 1024
> > > > +};
> > >
> > > The numbers look good, but what about other sample rates? For HDMI
> > > we go up to 192kHz, why not for DP?
> >
> > Our test only includes 32K, 44.1K and 48K :) I will add the support if
> > you think we should.
> 
> What sample rates can the user select? That should be the only relevant
> question here.

I have tested, other sample rates is not valid on DP.

> 
> >
> > >
> > > > +
> > > >  /* 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 *adjusted_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) &&
> > > > +				(adjusted_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) &&
> > > > +				(crtc->config->port_clock == aud_nm[i].clock))
> > > {
> > > > +				return aud_nm[i].n;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > >  }
> > > >
> > > >  /* check whether N/CTS/M need be set manually */  static bool
> > > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > > -				 const struct drm_display_mode *mode)
> > > > +				 const struct drm_display_mode
> > > *adjusted_mode)
> > > >  {
> > > > -	if (((mode->clock == TMDS_297M) ||
> > > > -		 (mode->clock == TMDS_296M)) &&
> > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > >  		return true;
> > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > +		  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, 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;
> > >
> > > HAS_DDI perhaps?
> >
> > Do you mean we should add "&& HAS_DDI"?
> > Could you please give me more details?
> 
> HAS_DDI alone should be enough.

Get it. Thanks.

Regards,
Libin

> 
> --
> Ville Syrjälä
> Intel OTC

Yang, Libin Aug. 4, 2016, 6:04 a.m. UTC | #9

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Thursday, August 4, 2016 1:39 PM
> To: Yang, Libin <[e-mail]>
> Cc: '[e-mail]' <[e-mail]>; 'intel-
> [e-mail]' <[e-mail]>;
> '[e-mail]' <[e-mail]>; Vetter, Daniel
> <[e-mail]>; '[e-mail]' <[e-mail]>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Yang, Libin
> > > Sent: Tuesday, August 2, 2016 9:59 PM
> > > To: Ville Syrjälä <[e-mail]>;
> > > [e-mail]
> > > Cc: [e-mail]; [e-mail];
> > > Vetter, Daniel <[e-mail]>; [e-mail]
> > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > Hi Ville
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > To: [e-mail]
> > > > Cc: [e-mail]; [e-mail];
> > > > Vetter, Daniel <[e-mail]>; [e-mail]; Yang,
> > > > Libin <[e-mail]>
> > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > >
> > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > [e-mail]
> > > wrote:
> > > > > From: Libin Yang <[e-mail]>
> > > > >
> > > > > 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.
> > > > >
> > > > > The relationship of Maud and Naud is expressed in the following
> > > > > equation:
> > > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > > >
> > > > > Please refer VESA DisplayPort Standard spec for details.
> > > > >
> > > > > Also, the patch applies
> > > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to
> > > > > APL platform.
> > > > >
> > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > > +++++++++++++++++++++++++++++++------
> > > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > > +#define LC_162M 162000
> > > >
> > > > Do we have some explanation why 2.7 doesn't need M/N programming,
> > > > but
> > > > 1.62 and 5.4 do?
> > >
> > > I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the test.
> > > 5.4 is for 4K and 1.62 is for 1080p.
> > >
> > > >
> > > > And I see you're only doing this on HSW+. Earlier platforms don't need
> this?
> > >
> > > We are not supporting earlier platforms and I'm not sure whether the
> > > old platforms supports 4K DP or not.
> > >
> > > >
> > > > > +static const struct {
> > > > > +	int sample_rate;
> > > > > +	int clock;
> > > > > +	int n;
> > > > > +	int m;
> > > >
> > > > Can save a bit of space by using u16 for m and n.
> > >
> > > OK, I will do it in next version.
> > >
> > > >
> > > > > +} aud_nm[] = {
> > > > > +	{48000, LC_540M, 5625, 256},`
> > > > > +	{44100, LC_540M, 9375, 392},
> > > > > +	{32000, LC_540M, 16875, 512},
> > > > > +	{48000, LC_162M, 3375, 512},
> > > > > +	{44100, LC_162M, 5625, 784},
> > > > > +	{32000, LC_162M, 10125, 1024
> > > > > +};
> > > >
> > > > The numbers look good, but what about other sample rates? For HDMI
> > > > we go up to 192kHz, why not for DP?
> > >
> > > Our test only includes 32K, 44.1K and 48K :) I will add the support
> > > if you think we should.
> >
> > I will not add 192KHz, 96KHz and etc rate support as based on my test
> > these rate will be not used in DP. It will be converted to 48KHz.
> 
> How about basing that decision on what's actually allowed by the driver?
> I can do 96kHz DP audio on my HSW just fine here. So clearly if the display
> supports it, there is nothing on the ALSA side that would prevent it from being
> used.

For 96KHz, audio will convert to 48KHz. You can try to print the sample rate
in i915_audio_component_sync_audio_rate(). So we can't test it for the real
96kHz.

In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.
However, the latter two sample rates is not supported in audio driver and
can't be tested so far. 88kHz, 96kHz and 192kHz cannot be tested, either.

Regards,
Libin

> 
> >
> > >
> > > >
> > > > > +
> > > > >  /* 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
> *adjusted_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) &&
> > > > > +				(adjusted_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) &&
> > > > > +				(crtc->config->port_clock ==
> aud_nm[i].clock))
> > > > {
> > > > > +				return aud_nm[i].n;
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > > >  }
> > > > >
> > > > >  /* check whether N/CTS/M need be set manually */  static bool
> > > > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > -				 const struct drm_display_mode *mode)
> > > > > +				 const struct drm_display_mode
> > > > *adjusted_mode)
> > > > >  {
> > > > > -	if (((mode->clock == TMDS_297M) ||
> > > > > -		 (mode->clock == TMDS_296M)) &&
> > > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > > >  		return true;
> > > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > > +		  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, 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;
> > > >
> > > > HAS_DDI perhaps?
> > >
> > > Do you mean we should add "&& HAS_DDI"?
> > > Could you please give me more details?
> > >
> > > Regards,
> > > Libin
> > >
> > > >
> > > > >
> > > > >  	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, 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
> 
> --
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Aug. 4, 2016, 6:23 a.m. UTC | #10

On Thu, Aug 04, 2016 at 06:04:10AM +0000, Yang, Libin wrote:
> Hi Ville,
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Thursday, August 4, 2016 1:39 PM
> > To: Yang, Libin <[e-mail]>
> > Cc: '[e-mail]' <[e-mail]>; 'intel-
> > [e-mail]' <[e-mail]>;
> > '[e-mail]' <[e-mail]>; Vetter, Daniel
> > <[e-mail]>; '[e-mail]' <[e-mail]>
> > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > 
> > On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> > > Hi Ville,
> > >
> > > > -----Original Message-----
> > > > From: Yang, Libin
> > > > Sent: Tuesday, August 2, 2016 9:59 PM
> > > > To: Ville Syrjälä <[e-mail]>;
> > > > [e-mail]
> > > > Cc: [e-mail]; [e-mail];
> > > > Vetter, Daniel <[e-mail]>; [e-mail]
> > > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > > >
> > > > Hi Ville
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > > To: [e-mail]
> > > > > Cc: [e-mail]; [e-mail];
> > > > > Vetter, Daniel <[e-mail]>; [e-mail]; Yang,
> > > > > Libin <[e-mail]>
> > > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > > >
> > > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > > [e-mail]
> > > > wrote:
> > > > > > From: Libin Yang <[e-mail]>
> > > > > >
> > > > > > 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.
> > > > > >
> > > > > > The relationship of Maud and Naud is expressed in the following
> > > > > > equation:
> > > > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > > > >
> > > > > > Please refer VESA DisplayPort Standard spec for details.
> > > > > >
> > > > > > Also, the patch applies
> > > > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to
> > > > > > APL platform.
> > > > > >
> > > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > > > +++++++++++++++++++++++++++++++------
> > > > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > > > +#define LC_162M 162000
> > > > >
> > > > > Do we have some explanation why 2.7 doesn't need M/N programming,
> > > > > but
> > > > > 1.62 and 5.4 do?
> > > >
> > > > I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the test.
> > > > 5.4 is for 4K and 1.62 is for 1080p.
> > > >
> > > > >
> > > > > And I see you're only doing this on HSW+. Earlier platforms don't need
> > this?
> > > >
> > > > We are not supporting earlier platforms and I'm not sure whether the
> > > > old platforms supports 4K DP or not.
> > > >
> > > > >
> > > > > > +static const struct {
> > > > > > +	int sample_rate;
> > > > > > +	int clock;
> > > > > > +	int n;
> > > > > > +	int m;
> > > > >
> > > > > Can save a bit of space by using u16 for m and n.
> > > >
> > > > OK, I will do it in next version.
> > > >
> > > > >
> > > > > > +} aud_nm[] = {
> > > > > > +	{48000, LC_540M, 5625, 256},`
> > > > > > +	{44100, LC_540M, 9375, 392},
> > > > > > +	{32000, LC_540M, 16875, 512},
> > > > > > +	{48000, LC_162M, 3375, 512},
> > > > > > +	{44100, LC_162M, 5625, 784},
> > > > > > +	{32000, LC_162M, 10125, 1024
> > > > > > +};
> > > > >
> > > > > The numbers look good, but what about other sample rates? For HDMI
> > > > > we go up to 192kHz, why not for DP?
> > > >
> > > > Our test only includes 32K, 44.1K and 48K :) I will add the support
> > > > if you think we should.
> > >
> > > I will not add 192KHz, 96KHz and etc rate support as based on my test
> > > these rate will be not used in DP. It will be converted to 48KHz.
> > 
> > How about basing that decision on what's actually allowed by the driver?
> > I can do 96kHz DP audio on my HSW just fine here. So clearly if the display
> > supports it, there is nothing on the ALSA side that would prevent it from being
> > used.
> 
> For 96KHz, audio will convert to 48KHz. You can try to print the sample rate
> in i915_audio_component_sync_audio_rate(). So we can't test it for the real
> 96kHz.

Wrong.

# grep rate /proc/asound/HDMI/eld#0.0 
sad0_rates		[0x4e0] 32000 44100 48000 96000

# grep rate /proc/asound/HDMI/pcm3p/sub0/hw_params 
rate: 96000 (96000/1)

[drm:i915_audio_component_sync_audio_rate] sample rate 96000 kHz

> 
> In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.

That table is just for informative purposes. The values are the ones you
get from the formula. And the ELD can contain exactly the same SADs as
with HDMI, so all the same sample rates can be used.

> However, the latter two sample rates is not supported in audio driver and
> can't be tested so far. 88kHz, 96kHz and 192kHz cannot be tested, either.
> 
> Regards,
> Libin
> 
> > 
> > >
> > > >
> > > > >
> > > > > > +
> > > > > >  /* 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
> > *adjusted_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) &&
> > > > > > +				(adjusted_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) &&
> > > > > > +				(crtc->config->port_clock ==
> > aud_nm[i].clock))
> > > > > {
> > > > > > +				return aud_nm[i].n;
> > > > > > +			}
> > > > > > +		}
> > > > > > +	}
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > > > >  }
> > > > > >
> > > > > >  /* check whether N/CTS/M need be set manually */  static bool
> > > > > > audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > > -				 const struct drm_display_mode *mode)
> > > > > > +				 const struct drm_display_mode
> > > > > *adjusted_mode)
> > > > > >  {
> > > > > > -	if (((mode->clock == TMDS_297M) ||
> > > > > > -		 (mode->clock == TMDS_296M)) &&
> > > > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > > > >  		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > > > >  		return true;
> > > > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > > > +		  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, 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;
> > > > >
> > > > > HAS_DDI perhaps?
> > > >
> > > > Do you mean we should add "&& HAS_DDI"?
> > > > Could you please give me more details?
> > > >
> > > > Regards,
> > > > Libin
> > > >
> > > > >
> > > > > >
> > > > > >  	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, 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
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

Yang, Libin Aug. 4, 2016, 6:31 a.m. UTC | #11

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Thursday, August 4, 2016 2:24 PM
> To: Yang, Libin <[e-mail]>
> Cc: '[e-mail]' <[e-mail]>; 'intel-
> [e-mail]' <[e-mail]>;
> '[e-mail]' <[e-mail]>; Vetter, Daniel
> <[e-mail]>; '[e-mail]' <[e-mail]>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Thu, Aug 04, 2016 at 06:04:10AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Thursday, August 4, 2016 1:39 PM
> > > To: Yang, Libin <[e-mail]>
> > > Cc: '[e-mail]' <[e-mail]>;
> > > 'intel- [e-mail]'
> > > <[e-mail]>;
> > > '[e-mail]' <[e-mail]>; Vetter,
> > > Daniel <[e-mail]>; '[e-mail]' <[e-mail]>
> > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> > > > Hi Ville,
> > > >
> > > > > -----Original Message-----
> > > > > From: Yang, Libin
> > > > > Sent: Tuesday, August 2, 2016 9:59 PM
> > > > > To: Ville Syrjälä <[e-mail]>;
> > > > > [e-mail]
> > > > > Cc: [e-mail];
> > > > > [e-mail]; Vetter, Daniel
> > > > > <[e-mail]>; [e-mail]
> > > > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > > > >
> > > > > Hi Ville
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > > > To: [e-mail]
> > > > > > Cc: [e-mail];
> > > > > > [e-mail]; Vetter, Daniel
> > > > > > <[e-mail]>; [e-mail]; Yang, Libin
> > > > > > <[e-mail]>
> > > > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > > > >
> > > > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > > > [e-mail]
> > > > > wrote:
> > > > > > > From: Libin Yang <[e-mail]>
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > The relationship of Maud and Naud is expressed in the
> > > > > > > following
> > > > > > > equation:
> > > > > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > > > > >
> > > > > > > Please refer VESA DisplayPort Standard spec for details.
> > > > > > >
> > > > > > > Also, the patch applies
> > > > > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in
> > > > > > > modeset") to APL platform.
> > > > > > >
> > > > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > > > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > > > > +++++++++++++++++++++++++++++++------
> > > > > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > > > > +#define LC_162M 162000
> > > > > >
> > > > > > Do we have some explanation why 2.7 doesn't need M/N
> > > > > > programming, but
> > > > > > 1.62 and 5.4 do?
> > > > >
> > > > > I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the
> test.
> > > > > 5.4 is for 4K and 1.62 is for 1080p.
> > > > >
> > > > > >
> > > > > > And I see you're only doing this on HSW+. Earlier platforms
> > > > > > don't need
> > > this?
> > > > >
> > > > > We are not supporting earlier platforms and I'm not sure whether
> > > > > the old platforms supports 4K DP or not.
> > > > >
> > > > > >
> > > > > > > +static const struct {
> > > > > > > +	int sample_rate;
> > > > > > > +	int clock;
> > > > > > > +	int n;
> > > > > > > +	int m;
> > > > > >
> > > > > > Can save a bit of space by using u16 for m and n.
> > > > >
> > > > > OK, I will do it in next version.
> > > > >
> > > > > >
> > > > > > > +} aud_nm[] = {
> > > > > > > +	{48000, LC_540M, 5625, 256},`
> > > > > > > +	{44100, LC_540M, 9375, 392},
> > > > > > > +	{32000, LC_540M, 16875, 512},
> > > > > > > +	{48000, LC_162M, 3375, 512},
> > > > > > > +	{44100, LC_162M, 5625, 784},
> > > > > > > +	{32000, LC_162M, 10125, 1024 };
> > > > > >
> > > > > > The numbers look good, but what about other sample rates? For
> > > > > > HDMI we go up to 192kHz, why not for DP?
> > > > >
> > > > > Our test only includes 32K, 44.1K and 48K :) I will add the
> > > > > support if you think we should.
> > > >
> > > > I will not add 192KHz, 96KHz and etc rate support as based on my
> > > > test these rate will be not used in DP. It will be converted to 48KHz.
> > >
> > > How about basing that decision on what's actually allowed by the driver?
> > > I can do 96kHz DP audio on my HSW just fine here. So clearly if the
> > > display supports it, there is nothing on the ALSA side that would
> > > prevent it from being used.
> >
> > For 96KHz, audio will convert to 48KHz. You can try to print the
> > sample rate in i915_audio_component_sync_audio_rate(). So we can't
> > test it for the real 96kHz.
> 
> Wrong.
> 
> # grep rate /proc/asound/HDMI/eld#0.0
> sad0_rates		[0x4e0] 32000 44100 48000 96000
> 
> # grep rate /proc/asound/HDMI/pcm3p/sub0/hw_params
> rate: 96000 (96000/1)
> 
> [drm:i915_audio_component_sync_audio_rate] sample rate 96000 kHz

Interesting, maybe my monitor doesn't support. And my driver will convert
it to 48KHz.

> 
> >
> > In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.
> 
> That table is just for informative purposes. The values are the ones you get
> from the formula. And the ELD can contain exactly the same SADs as with
> HDMI, so all the same sample rates can be used.

Yes. I will add the other sample rates support. But these date will not be 
test as I can't play 96KHz audio with hardware support here.

Regards,
Libin

> 
> > However, the latter two sample rates is not supported in audio driver
> > and can't be tested so far. 88kHz, 96kHz and 192kHz cannot be tested, either.
> >
> > Regards,
> > Libin
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > +
> > > > > > >  /* 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
> > > *adjusted_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) &&
> > > > > > > +				(adjusted_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) &&
> > > > > > > +				(crtc->config->port_clock ==
> > > aud_nm[i].clock))
> > > > > > {
> > > > > > > +				return aud_nm[i].n;
> > > > > > > +			}
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > > > > >  }
> > > > > > >
> > > > > > >  /* check whether N/CTS/M need be set manually */  static
> > > > > > > bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > > > -				 const struct drm_display_mode
> *mode)
> > > > > > > +				 const struct drm_display_mode
> > > > > > *adjusted_mode)
> > > > > > >  {
> > > > > > > -	if (((mode->clock == TMDS_297M) ||
> > > > > > > -		 (mode->clock == TMDS_296M)) &&
> > > > > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > >  		intel_crtc_has_type(crtc->config,
> INTEL_OUTPUT_HDMI))
> > > > > > >  		return true;
> > > > > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > > > > +		  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, 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;
> > > > > >
> > > > > > HAS_DDI perhaps?
> > > > >
> > > > > Do you mean we should add "&& HAS_DDI"?
> > > > > Could you please give me more details?
> > > > >
> > > > > Regards,
> > > > > Libin
> > > > >
> > > > > >
> > > > > > >
> > > > > > >  	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, 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
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Aug. 4, 2016, 6:36 a.m. UTC | #12

On Thu, Aug 04, 2016 at 06:31:19AM +0000, Yang, Libin wrote:
> Hi Ville,
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Thursday, August 4, 2016 2:24 PM
> > To: Yang, Libin <[e-mail]>
> > Cc: '[e-mail]' <[e-mail]>; 'intel-
> > [e-mail]' <[e-mail]>;
> > '[e-mail]' <[e-mail]>; Vetter, Daniel
> > <[e-mail]>; '[e-mail]' <[e-mail]>
> > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > 
> > On Thu, Aug 04, 2016 at 06:04:10AM +0000, Yang, Libin wrote:
> > > Hi Ville,
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > Sent: Thursday, August 4, 2016 1:39 PM
> > > > To: Yang, Libin <[e-mail]>
> > > > Cc: '[e-mail]' <[e-mail]>;
> > > > 'intel- [e-mail]'
> > > > <[e-mail]>;
> > > > '[e-mail]' <[e-mail]>; Vetter,
> > > > Daniel <[e-mail]>; '[e-mail]' <[e-mail]>
> > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > >
> > > > On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> > > > > Hi Ville,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Yang, Libin
> > > > > > Sent: Tuesday, August 2, 2016 9:59 PM
> > > > > > To: Ville Syrjälä <[e-mail]>;
> > > > > > [e-mail]
> > > > > > Cc: [e-mail];
> > > > > > [e-mail]; Vetter, Daniel
> > > > > > <[e-mail]>; [e-mail]
> > > > > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > > > > >
> > > > > > Hi Ville
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > > > > To: [e-mail]
> > > > > > > Cc: [e-mail];
> > > > > > > [e-mail]; Vetter, Daniel
> > > > > > > <[e-mail]>; [e-mail]; Yang, Libin
> > > > > > > <[e-mail]>
> > > > > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > > > > >
> > > > > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > > > > [e-mail]
> > > > > > wrote:
> > > > > > > > From: Libin Yang <[e-mail]>
> > > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > The relationship of Maud and Naud is expressed in the
> > > > > > > > following
> > > > > > > > equation:
> > > > > > > > Maud/Naud = 512 * fs / f_LS_Clk
> > > > > > > >
> > > > > > > > Please refer VESA DisplayPort Standard spec for details.
> > > > > > > >
> > > > > > > > Also, the patch applies
> > > > > > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in
> > > > > > > > modeset") to APL platform.
> > > > > > > >
> > > > > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > > > > > >  drivers/gpu/drm/i915/intel_audio.c | 122
> > > > > > > > +++++++++++++++++++++++++++++++------
> > > > > > > >  2 files changed, 111 insertions(+), 17 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..de55ecf 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_540M 540000
> > > > > > > > +#define LC_162M 162000
> > > > > > >
> > > > > > > Do we have some explanation why 2.7 doesn't need M/N
> > > > > > > programming, but
> > > > > > > 1.62 and 5.4 do?
> > > > > >
> > > > > > I didn't use 2.7 because I can't find a mode using 2.7. So I can't do the
> > test.
> > > > > > 5.4 is for 4K and 1.62 is for 1080p.
> > > > > >
> > > > > > >
> > > > > > > And I see you're only doing this on HSW+. Earlier platforms
> > > > > > > don't need
> > > > this?
> > > > > >
> > > > > > We are not supporting earlier platforms and I'm not sure whether
> > > > > > the old platforms supports 4K DP or not.
> > > > > >
> > > > > > >
> > > > > > > > +static const struct {
> > > > > > > > +	int sample_rate;
> > > > > > > > +	int clock;
> > > > > > > > +	int n;
> > > > > > > > +	int m;
> > > > > > >
> > > > > > > Can save a bit of space by using u16 for m and n.
> > > > > >
> > > > > > OK, I will do it in next version.
> > > > > >
> > > > > > >
> > > > > > > > +} aud_nm[] = {
> > > > > > > > +	{48000, LC_540M, 5625, 256},`
> > > > > > > > +	{44100, LC_540M, 9375, 392},
> > > > > > > > +	{32000, LC_540M, 16875, 512},
> > > > > > > > +	{48000, LC_162M, 3375, 512},
> > > > > > > > +	{44100, LC_162M, 5625, 784},
> > > > > > > > +	{32000, LC_162M, 10125, 1024 };
> > > > > > >
> > > > > > > The numbers look good, but what about other sample rates? For
> > > > > > > HDMI we go up to 192kHz, why not for DP?
> > > > > >
> > > > > > Our test only includes 32K, 44.1K and 48K :) I will add the
> > > > > > support if you think we should.
> > > > >
> > > > > I will not add 192KHz, 96KHz and etc rate support as based on my
> > > > > test these rate will be not used in DP. It will be converted to 48KHz.
> > > >
> > > > How about basing that decision on what's actually allowed by the driver?
> > > > I can do 96kHz DP audio on my HSW just fine here. So clearly if the
> > > > display supports it, there is nothing on the ALSA side that would
> > > > prevent it from being used.
> > >
> > > For 96KHz, audio will convert to 48KHz. You can try to print the
> > > sample rate in i915_audio_component_sync_audio_rate(). So we can't
> > > test it for the real 96kHz.
> > 
> > Wrong.
> > 
> > # grep rate /proc/asound/HDMI/eld#0.0
> > sad0_rates		[0x4e0] 32000 44100 48000 96000
> > 
> > # grep rate /proc/asound/HDMI/pcm3p/sub0/hw_params
> > rate: 96000 (96000/1)
> > 
> > [drm:i915_audio_component_sync_audio_rate] sample rate 96000 kHz
> 
> Interesting, maybe my monitor doesn't support. And my driver will convert
> it to 48KHz.
> 
> > 
> > >
> > > In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.
> > 
> > That table is just for informative purposes. The values are the ones you get
> > from the formula. And the ELD can contain exactly the same SADs as with
> > HDMI, so all the same sample rates can be used.
> 
> Yes. I will add the other sample rates support. But these date will not be 
> test as I can't play 96KHz audio with hardware support here.

Yeah, we can't always test all the combinations. Just have to go on
faith that it works out ;)

> 
> Regards,
> Libin
> 
> > 
> > > However, the latter two sample rates is not supported in audio driver
> > > and can't be tested so far. 88kHz, 96kHz and 192kHz cannot be tested, either.
> > >
> > > Regards,
> > > Libin
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > >  /* 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
> > > > *adjusted_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) &&
> > > > > > > > +				(adjusted_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) &&
> > > > > > > > +				(crtc->config->port_clock ==
> > > > aud_nm[i].clock))
> > > > > > > {
> > > > > > > > +				return aud_nm[i].n;
> > > > > > > > +			}
> > > > > > > > +		}
> > > > > > > > +	}
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
> > > > > > > > +				(crtc->config->port_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,17 +191,38 @@ 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;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  /* check whether N/CTS/M need be set manually */  static
> > > > > > > > bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > > > > -				 const struct drm_display_mode
> > *mode)
> > > > > > > > +				 const struct drm_display_mode
> > > > > > > *adjusted_mode)
> > > > > > > >  {
> > > > > > > > -	if (((mode->clock == TMDS_297M) ||
> > > > > > > > -		 (mode->clock == TMDS_296M)) &&
> > > > > > > > +	if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > > > +		 (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > > >  		intel_crtc_has_type(crtc->config,
> > INTEL_OUTPUT_HDMI))
> > > > > > > >  		return true;
> > > > > > > > +	else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > > > +		   (crtc->config->port_clock == LC_162M)) &&
> > > > > > > > +		  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, 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;
> > > > > > >
> > > > > > > HAS_DDI perhaps?
> > > > > >
> > > > > > Do you mean we should add "&& HAS_DDI"?
> > > > > > Could you please give me more details?
> > > > > >
> > > > > > Regards,
> > > > > > Libin
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >  	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, 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
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

Patch

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..de55ecf 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_540M 540000
+#define LC_162M 162000
+static const struct {
+	int sample_rate;
+	int clock;
+	int n;
+	int m;
+} aud_nm[] = {
+	{48000, LC_540M, 5625, 256},
+	{44100, LC_540M, 9375, 392},
+	{32000, LC_540M, 16875, 512},
+	{48000, LC_162M, 3375, 512},
+	{44100, LC_162M, 5625, 784},
+	{32000, LC_162M, 10125, 1024},
+};
+
 /* 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 *adjusted_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) &&
+				(adjusted_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) &&
+				(crtc->config->port_clock == aud_nm[i].clock)) {
+				return aud_nm[i].n;
+			}
+		}
+	}
+	return 0;
+}
+
+static int audio_config_get_m(struct intel_crtc *crtc, 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) &&
+				(crtc->config->port_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,17 +191,38 @@  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;
 }
 
 /* check whether N/CTS/M need be set manually */
 static bool audio_rate_need_prog(struct intel_crtc *crtc,
-				 const struct drm_display_mode *mode)
+				 const struct drm_display_mode *adjusted_mode)
 {
-	if (((mode->clock == TMDS_297M) ||
-		 (mode->clock == TMDS_296M)) &&
+	if (((adjusted_mode->clock == TMDS_297M) ||
+		 (adjusted_mode->clock == TMDS_296M)) &&
 		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
 		return true;
+	else if (((crtc->config->port_clock == LC_540M) ||
+		   (crtc->config->port_clock == LC_162M)) &&
+		  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, 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, 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);