[Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset
Yang, Libin
libin.yang at intel.com
Thu Oct 20 08:42:27 UTC 2016
> -----Original Message-----
> From: Nikula, Jani
> Sent: Thursday, October 20, 2016 4:34 PM
> To: Yang, Libin <libin.yang at intel.com>; Lin, Mengdong
> <mengdong.lin at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: libin.yang at linux.intel.com; Pandiyan, Dhinakaran
> <dhinakaran.pandiyan at intel.com>; Zhang, Keqiao
> <keqiao.zhang at intel.com>; Zhao, Juan J <juan.j.zhao at intel.com>
> Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in
> modeset
>
> On Thu, 20 Oct 2016, "Yang, Libin" <libin.yang at intel.com> wrote:
> >> -----Original Message-----
> >> From: Nikula, Jani
> >> Sent: Wednesday, October 19, 2016 11:09 PM
> >> To: Yang, Libin <libin.yang at intel.com>; Lin, Mengdong
> >> <mengdong.lin at intel.com>; intel-gfx at lists.freedesktop.org
> >> Cc: libin.yang at linux.intel.com; Pandiyan, Dhinakaran
> >> <dhinakaran.pandiyan at intel.com>; Zhang, Keqiao
> >> <keqiao.zhang at intel.com>; Zhao, Juan J <juan.j.zhao at intel.com>
> >> Subject: RE: [Intel-gfx] [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 at intel.com> wrote:
> >> >> -----Original Message-----
> >> >> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org]
> >> >> On Behalf Of Lin, Mengdong
> >> >> Sent: Wednesday, October 12, 2016 10:46 AM
> >> >> To: Nikula, Jani <jani.nikula at intel.com>;
> >> >> intel-gfx at lists.freedesktop.org
> >> >> Cc: Nikula, Jani <jani.nikula at intel.com>;
> >> >> libin.yang at linux.intel.com; Pandiyan, Dhinakaran
> >> >> <dhinakaran.pandiyan at intel.com>
> >> >> Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper
> >> >> N/M in modeset
> >> >>
> >> >>
> >> >>
> >> >> > -----Original Message-----
> >> >> > From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org]
> >> >> > On Behalf Of Jani Nikula
> >> >> > Sent: Monday, October 10, 2016 11:04 PM
> >> >> > To: intel-gfx at lists.freedesktop.org
> >> >> > Cc: Nikula, Jani <jani.nikula at intel.com>;
> >> >> > libin.yang at linux.intel.com; Pandiyan, Dhinakaran
> >> >> > <dhinakaran.pandiyan at intel.com>
> >> >> > Subject: [Intel-gfx] [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 at linux.intel.com>
> >> >> > Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> >> > ---
> >> >> > 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?
Which table are you meaning? I calculate the values myself. I didn't find the
full table in DP spec. I only find the table for 270MHz and 162MHz in
Table 2-50: Examples of Maud and Naud Values
Regards,
Libin
>
> 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 at lists.freedesktop.org
> >> >> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx at lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list