[Intel-gfx] [PATCH v3 1/3] drm/i915: set proper N/M in modeset
Yang, Libin
libin.yang at intel.com
Fri Aug 5 05:39:19 UTC 2016
Hi Ville,
Sorry for delay reply. Please see my comments below.
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, August 4, 2016 4:53 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 v3 1/3] drm/i915: set proper N/M in modeset
>
> On Thu, Aug 04, 2016 at 03:58:02PM +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. 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.
>
> Pleas include a small changelog here so that I know what has changed with
> each revision of the patch. Eg:
>
> v2: blah
> v3: whatever
>
> >
> > 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 | 133
> > ++++++++++++++++++++++++++++++++-----
> > 2 files changed, 123 insertions(+), 16 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)
>
> Please try to keep the register defines in order by the register offset.
> Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
>
> > +
> > #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..04e7358 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -98,6 +98,35 @@ static const struct {
> > { 192000, TMDS_297M, 20480, 247500 }, };
> >
> > +#define LC_540M 540000
> > +#define LC_270M 270000
> > +#define LC_162M 162000
> > +static const struct {
> > + int sample_rate;
> > + int clock;
> > + u16 n;
> > + u16 m;
> > +} aud_nm[] = {
> > + {192000, LC_540M, 5625, 1024},
> ^ ^
>
> Style nit: please add spaces here
>
> > + {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},
> > + {192000, LC_270M, 5625, 2048},
> > + {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},
> > + {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},
>
> The numbers looks good. But looks like you forgot about 176.4 kHz?
>
> > +};
> > +
> > /* 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 +150,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;
> >
> > - 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_HDMI)) {
> > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > + if ((rate == aud_ncts[i].sample_rate) &&
> > + (adjusted_mode->clock == aud_ncts[i].clock)) {
>
> Indent fail, and a bit too many parens for my taste. Should look like:
>
> 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)) {
>
> intel_has_dp_encoder() will make this work for MST as well.
>
> "In MST mode, the Sink device must ignore the Maud and Mvid values sent by
> an MST Source device unless the MST Sink device is directly connected to the
> MST Source device via a single link as the Source device generates those values
> based on the LS_Clk of the link it is driving. An MST Sink device must be able to
> regenerate a stream clock without depending on Mvid and Mvid time stamp
> values because the MST Sink device might be plugged into an MST Branch
> device, not an MST Source device."
>
> So if I'm reading that right, an MST sink can still use the M/N values if it's
> direclty hooked up to a MST source. Hence we probably want to fill out the
> M/N even for MST.
My understanding is this is the requirement for sink device.
Non-mst and MST, source will send the N/M value, and if it is
Non-mst/single link, sink device should use these data to generate
the clock. If it is in MST, sink device should ignore the data and regenerate
the clock without depending on N/M value.
So this code should OK be for both non-mst and MST. If not, I think we
can make a separate patch for DP MST.
>
> > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > + if ((rate == aud_nm[i].sample_rate) &&
> > + (crtc->config->port_clock == aud_nm[i].clock))
> {
>
> indent + parens
>
> > + return aud_nm[i].n;
> > + }
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > + int i;
> > +
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
>
> intel_has_dp_encoder()
>
> > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > + if ((rate == aud_nm[i].sample_rate) &&
> > + (crtc->config->port_clock == aud_nm[i].clock))
> {
>
> indent + parens
>
> > + 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 +204,39 @@ 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))
>
> intel_has_dp_encoder()
>
> > + 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;
>
> The caller already checked that.
>
> > +
> > + 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_270M) ||
> > + (crtc->config->port_clock == LC_162M)) &&
> > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
>
> intel_has_dp_encoder()
>
> > + return true;
> > else
> > return false;
> > }
> > @@ -287,7 +368,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 +424,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)) {
>
> intel_has_dp_encoder()
>
> > + 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);
>
> We should program this register even for HDMI, so that we don't leak invalid
> register values eg. when changing from DP->HDMI.
HDMI doesn't need set these values based on our test. It seems silicon can
handle smoothly for HDMI.
>
> > + }
> > + }
> > +
> > mutex_unlock(&dev_priv->av_mutex);
> > }
> >
> > @@ -637,7 +728,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;
> >
> > @@ -653,7 +744,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))) {
>
> Hmm. Should probablt move this stuff later so that we can use check
> crtc->config->output_types instead. But I guess that's more
> appropriately left to any MST patch.
The same reason as intel_has_dp_encoder(). And if it is wrong
for DP MST, let's have a separate patch when we support DP MST.
At that time, we can do a full test on DP MST.
Regards,
Libin
>
> > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > err = -ENODEV;
> > goto unlock;
> > @@ -681,7 +773,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 +785,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)) {
>
> intel_has_dp_encoder()
>
> > + 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
More information about the Intel-gfx
mailing list