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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ville

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
> Sent: Tuesday, August 2, 2016 6:47 PM
> To: libin.yang@xxxxxxxxxxxxxxx
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; jani.nikula@xxxxxxxxxxxxxxx; Vetter, Daniel
> <daniel.vetter@xxxxxxxxx>; tiwai@xxxxxxx; Yang, Libin <libin.yang@xxxxxxxxx>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Tue, Aug 02, 2016 at 09:35:10AM +0800, libin.yang@xxxxxxxxxxxxxxx wrote:
> > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx>
> >
> > 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 <libin.yang@xxxxxxxxxxxxxxx>
> > ---
> >  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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux