[Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset
Jani Nikula
jani.nikula at intel.com
Wed Oct 19 15:08:37 UTC 2016
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.)
>> > + { 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?
>> > +};
>> > +
>> > +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.
>> > + 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?
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
More information about the Intel-gfx
mailing list