On Fri, Jul 29, 2016 at 05:54:23AM +0000, Yang, Libin wrote:
> Hi Ville,
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[email protected]]
> > Sent: Thursday, July 28, 2016 3:42 PM
> > To: [email protected]
> > Cc: [email protected]; [email protected]; Vetter,
> > Daniel <[email protected]>; [email protected]; Yang, Libin
> > <[email protected]>
> > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> >
> > On Thu, Jul 14, 2016 at 03:06:21PM +0800, [email protected] wrote:
> > > From: Libin Yang <[email protected]>
> > >
> > > When modeset occurs and the LS_CLK is set to some special values in DP
> > > mode, the N/M need to be set manually if audio is playing.
> > >
> > > Also, the patch applies
> > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to APL
> > > platform.
> > >
> > > Signed-off-by: Libin Yang <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > drivers/gpu/drm/i915/intel_audio.c | 116
> > > ++++++++++++++++++++++++++++++++-----
> > > 2 files changed, 108 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7351,6 +7351,12 @@ enum {
> > > #define _HSW_AUD_CONFIG_B 0x65100
> > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_B)
> > >
> > > +#define _HSW_AUD_M_CTS_ENABLE_A 0x65028
> > > +#define _HSW_AUD_M_CTS_ENABLE_B 0x65128
> > > +#define HSW_AUD_M_CTS_ENABLE(pipe) _MMIO_PIPE(pipe,
> > _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
> > > +#define AUD_M_CTS_M_VALUE_INDEX (1 << 21)
> > > +#define AUD_M_CTS_M_PROG_ENABLE (1 << 20)
> > > +
> > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > index 6700a7b..e2e4d4b 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -98,6 +98,22 @@ static const struct {
> > > { 192000, TMDS_297M, 20480, 247500 }, };
> > >
> > > +#define LC_533M 533250
> > > +#define LC_148M 148500
> > > +static const struct {
> > > + int sample_rate;
> > > + int clock;
> > > + int n;
> > > + int m;
> > > +} aud_nm[] = {
> > > + {48000, LC_533M, 88875, 4096},
> > > + {44100, LC_533M, 148125, 6272},
> > > + {32000, LC_533M, 266625, 8192},
> > > + {48000, LC_148M, 12375, 2048},
> > > + {44100, LC_148M, 20625, 3136},
> > > + {32000, LC_148M, 37125, 4096},
> > > +};
> > > +
> > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32
> > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > *adjusted_mode) { @@ -121,20 +137,50 @@ static u32
> > > audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
> > > return hdmi_audio_clock[i].config;
> > > }
> > >
> > > -static int audio_config_get_n(const struct drm_display_mode *mode,
> > > int rate)
> > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > + const struct drm_display_mode *mode, int rate) {
> > > + int i;
> > > +
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > + (mode->clock == aud_ncts[i].clock)) {
> > > + return aud_ncts[i].n;
> > > + }
> > > + }
> > > + }
> > > +
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > + if ((rate == aud_nm[i].sample_rate) &&
> > > + (mode->clock == aud_nm[i].clock)) {
> >
> > So the spec says we should have "Maud / Naud = 512 * fs / f_LS_Clk", where
> > fs is the audio sample rate, and f_LS_CLK is the link symbol clock.
> > With that in mind this should not be looking at mode->clock but
> > crtc->config->port_clock.
> >
> > So I don't actually understand why you say LS_CLK has "special" values.
> > It shouldn't. It's always either 162, 270, or 540 MHz.
>
> Thanks for the correction. I will use crtc->config->port_clock.
But why do you need to do it at all? The hardware can't deal with one of
the standard link rates on its own?
>
> >
> > Actually even the HDMI case is wrong in the code, it should be looking at
> > mode->crtc_clock instead of mode->clock. Or perhaps even port_clock, if my
> > reading of the HDMI spec is correct, but I never got any sane answer from
> > any hw folks to my questions about this :(
>
> I will try mode->crtc_clock later for HDMI. Thanks.
While you're at it, can you pls do s/mode/adjusted_mode/ ? A while back
I unified our naming convention for these things, but looks like the
audio code has since gone back to the inconsisten old way.
>
> Regards,
> Libin
>
> >
> > > + return aud_nm[i].n;
> > > + }
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int audio_config_get_m(struct intel_crtc *crtc,
> > > + const struct drm_display_mode *mode, int rate)
> > > {
> > > int i;
> > >
> > > - for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > - if ((rate == aud_ncts[i].sample_rate) &&
> > > - (mode->clock == aud_ncts[i].clock)) {
> > > - return aud_ncts[i].n;
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > + if ((rate == aud_nm[i].sample_rate) &&
> > > + (mode->clock == aud_nm[i].clock)) {
> > > + return aud_nm[i].m;
> > > + }
> > > }
> > > }
> > > +
> > > return 0;
> > > }
> > >
> > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > + int n, uint32_t val)
> > > {
> > > int n_low, n_up;
> > > uint32_t tmp = val;
> > > @@ -145,6 +191,23 @@ static uint32_t audio_config_setup_n_reg(int n,
> > uint32_t val)
> > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > AUD_CONFIG_N_PROG_ENABLE);
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > + return tmp;
> > > +}
> > > +
> > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > + int m, uint32_t val)
> > > +{
> > > + uint32_t tmp = val;
> > > +
> > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > + return 0;
> > > +
> > > + tmp |= m;
> > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > +
> > > return tmp;
> > > }
> > >
> > > @@ -156,6 +219,10 @@ static bool audio_rate_need_prog(struct intel_crtc
> > *crtc,
> > > (mode->clock == TMDS_296M)) &&
> > > intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > return true;
> > > + else if (((mode->clock == LC_533M) ||
> > > + (mode->clock == LC_148M)) &&
> > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > + return true;
> > > else
> > > return false;
> > > }
> > > @@ -287,7 +354,7 @@ static void hsw_audio_codec_enable(struct
> > drm_connector *connector,
> > > struct intel_digital_port *intel_dig_port =
> > > enc_to_dig_port(&encoder->base);
> > > enum port port = intel_dig_port->port;
> > > - uint32_t tmp;
> > > + uint32_t tmp, m;
> > > int len, i;
> > > int n, rate;
> > >
> > > @@ -343,15 +410,25 @@ static void hsw_audio_codec_enable(struct
> > drm_connector *connector,
> > > DRM_ERROR("invalid port: %d\n", port);
> > > rate = 0;
> > > }
> > > - n = audio_config_get_n(adjusted_mode, rate);
> > > + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> > > if (n != 0)
> > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> > > else
> > > DRM_DEBUG_KMS("no suitable N value is found\n");
> > > }
> > >
> > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >
> > > + /* setup m value for DP */
> > > + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
> > > + m = audio_config_get_m(intel_crtc, adjusted_mode, rate);
> > > + if (m != 0) {
> > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > + }
> > > + }
> > > +
> > > mutex_unlock(&dev_priv->av_mutex);
> > > }
> > >
> > > @@ -637,7 +714,7 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > struct drm_display_mode *mode;
> > > struct i915_audio_component *acomp = dev_priv-
> > >audio_component;
> > > enum pipe pipe = INVALID_PIPE;
> > > - u32 tmp;
> > > + u32 tmp, m;
> > > int n;
> > > int err = 0;
> > >
> > > @@ -645,7 +722,8 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > if (!IS_SKYLAKE(dev_priv) &&
> > > !IS_KABYLAKE(dev_priv) &&
> > > !IS_BROADWELL(dev_priv) &&
> > > - !IS_HASWELL(dev_priv))
> > > + !IS_HASWELL(dev_priv) &&
> > > + !IS_BROXTON(dev_priv))
> > > return 0;
> > >
> > > mutex_lock(&dev_priv->av_mutex);
> > > @@ -653,7 +731,8 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > intel_encoder = dev_priv->dig_port_map[port];
> > > /* intel_encoder might be NULL for DP MST */
> > > if (!intel_encoder || !intel_encoder->base.crtc ||
> > > - intel_encoder->type != INTEL_OUTPUT_HDMI) {
> > > + ((intel_encoder->type != INTEL_OUTPUT_HDMI) &&
> > > + (intel_encoder->type != INTEL_OUTPUT_DP))) {
> > > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > err = -ENODEV;
> > > goto unlock;
> > > @@ -681,7 +760,7 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > goto unlock;
> > > }
> > >
> > > - n = audio_config_get_n(mode, rate);
> > > + n = audio_config_get_n(crtc, mode, rate);
> > > if (n == 0) {
> > > DRM_DEBUG_KMS("Using automatic mode for N value on
> > port %c\n",
> > > port_name(port));
> > > @@ -693,8 +772,17 @@ static int
> > > i915_audio_component_sync_audio_rate(struct device *dev,
> > >
> > > /* 3. set the N/CTS/M */
> > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > + /* setup m value for DP */
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > + m = audio_config_get_m(crtc, mode, rate);
> > > + if (m == 0)
> > > + goto unlock;
> > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > + }
> > >
> > > unlock:
> > > mutex_unlock(&dev_priv->av_mutex);
> > > --
> > > 1.9.1
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
