On Fri, Aug 05, 2016 at 06:40:45AM +0000, Yang, Libin wrote: > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > > Sent: Friday, August 5, 2016 2:36 PM > > To: Yang, Libin <libin.yang@xxxxxxxxx> > > Cc: libin.yang@xxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > > jani.nikula@xxxxxxxxxxxxxxx; Vetter, Daniel <daniel.vetter@xxxxxxxxx>; > > tiwai@xxxxxxx > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset > > > > On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote: > > > Hi Ville, > > > > > > > -----Original Message----- > > > > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > > > > Sent: Friday, August 5, 2016 1:57 PM > > > > To: Yang, Libin <libin.yang@xxxxxxxxx> > > > > Cc: libin.yang@xxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > > > > jani.nikula@xxxxxxxxxxxxxxx; Vetter, Daniel > > > > <daniel.vetter@xxxxxxxxx>; tiwai@xxxxxxx > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset > > > > > > > > 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@xxxxxxxxxxxxxxx] > > > > > > Sent: Thursday, August 4, 2016 4:53 PM > > > > > > To: libin.yang@xxxxxxxxxxxxxxx > > > > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; > > > > > > jani.nikula@xxxxxxxxxxxxxxx; Vetter, Daniel > > > > > > <daniel.vetter@xxxxxxxxx>; tiwai@xxxxxxx; Yang, Libin > > > > > > <libin.yang@xxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx > > > > wrote: > > > > > > > From: Libin Yang <libin.yang@xxxxxxxxxxxxxxx> > > > > > > > > > > > > > > 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@xxxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > 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. > > > > > > OK, I will use intel_crtc_has_dp_encoder(). Hope it still works 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. > > > > > > > > Yes, but we nee to make sure we clear whatever we programmed in for > > > > DP previously. > > > > > > The silicon seems to be able to handle the situation of DP -> HDMI and > > > HDMI -> DP. > > > > Did you make sure eg. that the power well didn't get toggled in between? > > That would reset the register anyway. > > We have done the test for this case. So far it is OK. Test what? Checking that the register gets reset w/o any power well toggling and the like? So what event does reset that register? > > Regards, > Libin > > > > > > What's more it can handle sample rate/tmds changes. > > > > > > Regards, > > > Libin > > > > > > > > > > > > > > > > > > > > > > > > > + } > > > > > > > + } > > > > > > > + > > > > > > > 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 > > > > -- > > Ville Syrjälä > > Intel OTC -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
