[Intel-gfx] [PATCH v3 1/3] drm/i915: set proper N/M in modeset
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Aug 5 05:56:33 UTC 2016
On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> 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.
It's only OK for SST. We could of course delay dealing with MST, but
that seems like unnecessary churn. You're touching all these places in
this patch, so might as well use intel_dp_encoder() from the start. It
will make the MST patch smaller too, which will make it easier to
review.
>
> >
> > > + 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.
Yes, but we nee to make sure we clear whatever we programmed in for DP
previously.
>
> >
> > > + }
> > > + }
> > > +
> > > 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.
Yeah this part we can leave for the MST patch as it'll involve a bit
more than just adding another check to the if statement.
>
> 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
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list