[Intel-gfx] [PATCH] drm/i915: set proper N/M in modeset
Yang, Libin
libin.yang at intel.com
Thu Aug 4 06:04:10 UTC 2016
Hi Ville,
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, August 4, 2016 1:39 PM
> To: Yang, Libin <libin.yang at intel.com>
> Cc: 'libin.yang at linux.intel.com' <libin.yang at linux.intel.com>; 'intel-
> gfx at lists.freedesktop.org' <intel-gfx at lists.freedesktop.org>;
> 'jani.nikula at linux.intel.com' <jani.nikula at linux.intel.com>; Vetter, Daniel
> <daniel.vetter at intel.com>; 'tiwai at suse.de' <tiwai at suse.de>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
>
> On Thu, Aug 04, 2016 at 02:48:54AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Yang, Libin
> > > Sent: Tuesday, August 2, 2016 9:59 PM
> > > To: Ville Syrjälä <ville.syrjala at linux.intel.com>;
> > > libin.yang at linux.intel.com
> > > Cc: intel-gfx at lists.freedesktop.org; jani.nikula at linux.intel.com;
> > > Vetter, Daniel <daniel.vetter at intel.com>; tiwai at suse.de
> > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > Hi Ville
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > To: libin.yang at linux.intel.com
> > > > Cc: intel-gfx at lists.freedesktop.org; jani.nikula at linux.intel.com;
> > > > Vetter, Daniel <daniel.vetter at intel.com>; tiwai at suse.de; Yang,
> > > > Libin <libin.yang at intel.com>
> > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > >
> > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > libin.yang at linux.intel.com
> > > wrote:
> > > > > From: Libin Yang <libin.yang at linux.intel.com>
> > > > >
> > > > > 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 at linux.intel.com>
> > > > > ---
> > > > > 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.
> >
> > I will not add 192KHz, 96KHz and etc rate support as based on my test
> > these rate will be not used in DP. It will be converted to 48KHz.
>
> How about basing that decision on what's actually allowed by the driver?
> I can do 96kHz DP audio on my HSW just fine here. So clearly if the display
> supports it, there is nothing on the ALSA side that would prevent it from being
> used.
For 96KHz, audio will convert to 48KHz. You can try to print the sample rate
in i915_audio_component_sync_audio_rate(). So we can't test it for the real
96kHz.
In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.
However, the latter two sample rates is not supported in audio driver and
can't be tested so far. 88kHz, 96kHz and 192kHz cannot be tested, either.
Regards,
Libin
>
> >
> > >
> > > >
> > > > > +
> > > > > /* 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
>
> --
> Ville Syrjälä
> Intel OTC
More information about the Intel-gfx
mailing list