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
