On Thu, 20 Oct 2016, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Nikula, Jani >> Sent: Wednesday, October 19, 2016 11:09 PM >> To: Yang, Libin <libin.yang@xxxxxxxxx>; Lin, Mengdong >> <mengdong.lin@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Cc: libin.yang@xxxxxxxxxxxxxxx; Pandiyan, Dhinakaran >> <dhinakaran.pandiyan@xxxxxxxxx>; Zhang, Keqiao >> <keqiao.zhang@xxxxxxxxx>; Zhao, Juan J <juan.j.zhao@xxxxxxxxx> >> Subject: RE: [PATCH RESEND 9/9] drm/i915: set proper N/M in >> modeset >> >> >> Sorry it's taken me forever to get back to this. Some comments inline. >> >> BR, >> Jani. >> >> On Wed, 12 Oct 2016, "Yang, Libin" <libin.yang@xxxxxxxxx> wrote: >> >> -----Original Message----- >> >> From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On >> >> Behalf Of Lin, Mengdong >> >> Sent: Wednesday, October 12, 2016 10:46 AM >> >> To: Nikula, Jani <jani.nikula@xxxxxxxxx>; >> >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; libin.yang@xxxxxxxxxxxxxxx; >> >> Pandiyan, Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx> >> >> Subject: Re: [PATCH RESEND 9/9] drm/i915: set proper N/M >> >> in modeset >> >> >> >> >> >> >> >> > -----Original Message----- >> >> > From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On >> >> > Behalf Of Jani Nikula >> >> > Sent: Monday, October 10, 2016 11:04 PM >> >> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >> > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; >> >> > libin.yang@xxxxxxxxxxxxxxx; Pandiyan, Dhinakaran >> >> > <dhinakaran.pandiyan@xxxxxxxxx> >> >> > Subject: [PATCH RESEND 9/9] drm/i915: set proper N/M in >> >> > modeset >> >> > >> >> > 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. >> >> > Otherwise the first several seconds may be silent in audio playback. >> >> > >> >> > 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. >> >> > >> >> > Signed-off-by: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> >> >> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx> >> >> > --- >> >> > drivers/gpu/drm/i915/i915_reg.h | 7 +++ >> >> > drivers/gpu/drm/i915/intel_audio.c | 100 >> >> > ++++++++++++++++++++++++++++++++++++- >> >> > 2 files changed, 105 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h >> >> > b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 >> >> > 100644 >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> >> > @@ -7359,6 +7359,13 @@ enum { >> >> > #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) >> >> > >> >> > +#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 AUD_CONFIG_M_MASK 0xfffff >> >> >> >> The last line cause misalignment after applying the patch. >> >> >> >> > + >> >> > #define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 >> >> > #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 >> >> > #define HSW_AUD_DIP_ELD_CTRL(pipe) _MMIO_PIPE(pipe, >> >> > _HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B) >> diff -- >> >> git >> >> > a/drivers/gpu/drm/i915/intel_audio.c >> >> > b/drivers/gpu/drm/i915/intel_audio.c >> >> > index 81df29ca4947..0bc2701b6c9c 100644 >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c >> >> > @@ -57,6 +57,70 @@ >> >> > * struct &i915_audio_component_audio_ops @audio_ops is called >> >> > from >> >> > i915 driver. >> >> > */ >> >> > >> >> > +/* DP N/M table */ >> >> > +#define LC_540M 540000 >> >> > +#define LC_270M 270000 >> >> > +#define LC_162M 162000 >> >> > + >> >> > +struct dp_aud_n_m { >> >> > + int sample_rate; >> >> > + int clock; >> >> > + u16 n; >> >> > + u16 m; >> >> > +}; >> >> > + >> >> > +static const struct dp_aud_n_m dp_aud_n_m[] = { >> >> > + { 192000, LC_540M, 5625, 1024 }, >> >> > + { 176400, LC_540M, 9375, 1568 }, >> >> > + { 96000, LC_540M, 5625, 512 }, >> >> > + { 88200, LC_540M, 9375, 784 }, >> >> > + { 48000, LC_540M, 5625, 256 }, >> >> > + { 44100, LC_540M, 9375, 392 }, >> >> > + { 32000, LC_540M, 16875, 512 }, >> >> Any particular reason these M/N values are half of what they're in table >> 2-104 of DP 1.4 spec? (Admittedly the table is an informative example.) > > For HDMI, we found only set N is enough. HW then can handle the remaining. I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why? Jani. > >> >> >> > + { 192000, LC_270M, 5625, 2048 }, >> >> > + { 176400, LC_270M, 9375, 3136 }, >> >> > + { 96000, LC_270M, 5625, 1024 }, >> >> > + { 88200, LC_270M, 9375, 1568 }, >> >> > + { 48000, LC_270M, 5625, 512 }, >> >> > + { 44100, LC_270M, 9375, 784 }, >> >> > + { 32000, LC_270M, 16875, 1024 }, >> >> > + { 192000, LC_162M, 3375, 2048 }, >> >> > + { 176400, LC_162M, 5625, 3136 }, >> >> > + { 96000, LC_162M, 3375, 1024 }, >> >> > + { 88200, LC_162M, 5625, 1568 }, >> >> > + { 48000, LC_162M, 3375, 512 }, >> >> > + { 44100, LC_162M, 5625, 784 }, >> >> > + { 32000, LC_162M, 10125, 1024 }, >> >> Do we not support 128 or 64 kHz audio? > > No, we don't support HBR audio. > >> >> >> > +}; >> >> > + >> >> > +static const struct dp_aud_n_m * >> >> > +audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) { >> >> > + int i; >> >> > + >> >> > + for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) { >> >> > + if (rate == dp_aud_n_m[i].sample_rate && >> >> > + intel_crtc->config->port_clock == dp_aud_n_m[i].clock) >> >> > + return &dp_aud_n_m[i]; >> >> > + } >> >> > + >> >> > + return NULL; >> >> > +} >> >> > + >> >> > +static int audio_config_dp_get_m(struct intel_crtc *intel_crtc, >> >> > +int >> >> > +rate) { >> >> > + const struct dp_aud_n_m *nm = >> >> > audio_config_dp_get_n_m(intel_crtc, >> >> > +rate); >> >> > + >> >> > + return nm ? nm->m : 0; >> >> > +} >> >> > + >> >> > +static int audio_config_dp_get_n(struct intel_crtc *intel_crtc, >> >> > +int >> >> > +rate) { >> >> > + const struct dp_aud_n_m *nm = >> >> > audio_config_dp_get_n_m(intel_crtc, >> >> > +rate); >> >> > + >> >> > + return nm ? nm->n : 0; >> >> > +} >> >> > + >> >> > static const struct { >> >> > int clock; >> >> > u32 config; >> >> > @@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc >> >> > *intel_crtc, enum port port, >> >> > const struct drm_display_mode *adjusted_mode) { >> >> > struct drm_i915_private *dev_priv = >> >> > to_i915(intel_crtc->base.dev); >> >> > + struct i915_audio_component *acomp = dev_priv- >> >> > >audio_component; >> >> > + int rate = acomp ? acomp->aud_sample_rate[port] : 0; >> >> > enum pipe pipe = intel_crtc->pipe; >> >> > - u32 tmp; >> >> > + u32 tmp, n, m; >> >> > >> >> > tmp = I915_READ(HSW_AUD_CFG(pipe)); >> >> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -234,7 +300,30 @@ >> >> > hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port >> >> > port, >> >> > tmp &= ~AUD_CONFIG_N_PROG_ENABLE; >> >> > tmp |= AUD_CONFIG_N_VALUE_INDEX; >> >> > >> >> > + if (intel_crtc->config->port_clock == LC_540M || >> >> > + intel_crtc->config->port_clock == LC_270M || >> >> > + intel_crtc->config->port_clock == LC_162M) { >> >> Actually, this check could be removed. audio_config_dp_get_n_m will always >> return 0 if this doesn't hold. But not a big deal. > > Yes, we can remove it. > >> >> >> > + n = audio_config_dp_get_n(intel_crtc, rate); >> >> > + if (n != 0) { >> >> > + tmp &= ~AUD_CONFIG_N_MASK; >> >> > + tmp |= AUD_CONFIG_N(n); >> >> > + tmp |= AUD_CONFIG_N_PROG_ENABLE; >> >> > + } else { >> >> > + DRM_DEBUG_KMS("no suitable N value is found\n"); >> >> >> >> Could some comments be added here why we don't return if no valid N >> >> value is found? >> > >> > This is because although we don't need set N value here, but there are >> > some other bits to be set. So you can see we call >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); Later. >> > >> >> Do we still need to program write the register to let HW calculate N by >> itself? >> > >> > Ideally, all the frequency should be in the table. But if the >> > frequency is not in our table, we will let HW calculate N by itself. >> > >> >> >> >> > + } >> >> > + } >> >> > + >> >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> >> >> >> And if no valid N value is found, need we go ahead to check M value? >> > >> > If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate); >> > will also return 0 and skip the m setting. >> >> If we can't find M/N values, the HSW_AUD_CFG register is set to use >> automatic N value. However, the HSW_AUD_M_CTS_ENABLE register is not >> updated at all, and could potentially have stale stuff. Do we need to clear the >> AUD_M_CTS_M_VALUE_INDEX and/or AUD_M_CTS_M_PROG_ENABLE bits? >> >> We only do this stuff in hsw_audio_codec_enable now. Do we need to do >> something in hsw_audio_codec_disable too? > > In _disable(), the audio doesn't work. So the value will not impact audio. > It will not be worse that we do not reset the value in _disable(). > Without reset, the value is "cached". Next time, it may be a little quicker > for monitor response if we are using the same parameter, I think. > > Regards > Libin > >> >> >> BR, >> Jani. >> >> >> > >> > Regards, >> > Libin >> > >> >> >> >> > + >> >> > + m = audio_config_dp_get_m(intel_crtc, rate); >> >> > + if (m) { >> >> > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe)); >> >> > + tmp &= ~AUD_CONFIG_M_MASK; >> >> > + tmp |= m; >> >> > + tmp |= AUD_M_CTS_M_VALUE_INDEX; >> >> > + tmp |= AUD_M_CTS_M_PROG_ENABLE; >> >> > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp); >> >> > + } >> >> > } >> >> >> >> Thanks >> >> Mengdong >> >> >> >> > static void >> >> > @@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct >> intel_crtc >> >> > *intel_crtc, enum port port, >> >> > } >> >> > >> >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> >> > + >> >> > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe)); >> >> > + tmp &= ~AUD_CONFIG_M_MASK; >> >> > + tmp &= ~AUD_M_CTS_M_VALUE_INDEX; >> >> > + tmp |= AUD_M_CTS_M_PROG_ENABLE; >> >> > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp); >> >> > } >> >> > >> >> > static void >> >> > @@ -687,7 +782,8 @@ static int >> >> > i915_audio_component_sync_audio_rate(struct device *kdev, int port, >> >> > /* 1. get the pipe */ >> >> > intel_encoder = get_saved_enc(dev_priv, port, pipe); >> >> > 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("Not valid for port %c\n", >> >> port_name(port)); >> >> > err = -ENODEV; >> >> > goto unlock; >> >> > -- >> >> > 2.1.4 >> >> > >> >> > _______________________________________________ >> >> > Intel-gfx mailing list >> >> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> _______________________________________________ >> >> Intel-gfx mailing list >> >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
