Hi Ville,
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> Sent: Thursday, August 4, 2016 2:24 PM
> To: Yang, Libin <libin.y...@intel.com>
> Cc: 'libin.y...@linux.intel.com' <libin.y...@linux.intel.com>; 'intel-
> g...@lists.freedesktop.org' <intel-gfx@lists.freedesktop.org>;
> 'jani.nik...@linux.intel.com' <jani.nik...@linux.intel.com>; Vetter, Daniel
> <daniel.vet...@intel.com>; 'ti...@suse.de' <ti...@suse.de>
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
>
> On Thu, Aug 04, 2016 at 06:04:10AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > > Sent: Thursday, August 4, 2016 1:39 PM
> > > To: Yang, Libin <libin.y...@intel.com>
> > > Cc: 'libin.y...@linux.intel.com' <libin.y...@linux.intel.com>;
> > > 'intel- g...@lists.freedesktop.org'
> > > <intel-gfx@lists.freedesktop.org>;
> > > 'jani.nik...@linux.intel.com' <jani.nik...@linux.intel.com>; Vetter,
> > > Daniel <daniel.vet...@intel.com>; 'ti...@suse.de' <ti...@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.syrj...@linux.intel.com>;
> > > > > libin.y...@linux.intel.com
> > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > jani.nik...@linux.intel.com; Vetter, Daniel
> > > > > <daniel.vet...@intel.com>; ti...@suse.de
> > > > > Subject: RE: [PATCH] drm/i915: set proper N/M in modeset
> > > > >
> > > > > Hi Ville
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > > > > > Sent: Tuesday, August 2, 2016 6:47 PM
> > > > > > To: libin.y...@linux.intel.com
> > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > jani.nik...@linux.intel.com; Vetter, Daniel
> > > > > > <daniel.vet...@intel.com>; ti...@suse.de; Yang, Libin
> > > > > > <libin.y...@intel.com>
> > > > > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > > > > >
> > > > > > On Tue, Aug 02, 2016 at 09:35:10AM +0800,
> > > > > > libin.y...@linux.intel.com
> > > > > wrote:
> > > > > > > From: Libin Yang <libin.y...@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.y...@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.
>
> Wrong.
>
> # grep rate /proc/asound/HDMI/eld#0.0
> sad0_rates [0x4e0] 32000 44100 48000 96000
>
> # grep rate /proc/asound/HDMI/pcm3p/sub0/hw_params
> rate: 96000 (96000/1)
>
> [drm:i915_audio_component_sync_audio_rate] sample rate 96000 kHz
Interesting, maybe my monitor doesn't support. And my driver will convert
it to 48KHz.
>
> >
> > In DP spec, table 2-50 only says 48kHz, 32kHz, 44.1kHz, 384kHz and 768kHz.
>
> That table is just for informative purposes. The values are the ones you get
> from the formula. And the ELD can contain exactly the same SADs as with
> HDMI, so all the same sample rates can be used.
Yes. I will add the other sample rates support. But these date will not be
test as I can't play 96KHz audio with hardware support here.
Regards,
Libin
>
> > 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
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
