> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Friday, August 5, 2016 4:45 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 07:24:14AM +0000, Yang, Libin wrote: > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > > > Sent: Friday, August 5, 2016 3:17 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 > > > > > > > > > > > > > + 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? > > > > The register is used to set m/cts, which will impacts the audio clock > > sync. We didn't check the register reset or not. But the HDMI audio > > and DP will both works smoothly. And for your consideration, it is for > > HDMI. Let's make another patch for this issue if we really met the > > issue that hdmi can't sync the clock. What do you think? > > How about we just always write the register to make sure we won't get any > stupid bugs because of this. OK. Do you think it is OK I will write a separate patch for it, not included in the patch series? This is for HDMI N/CTS setting and this needs a lot of test on HDMI platforms. Regards, Libin > > > > > > > > > > > > > > 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 > > > > > > > > > crtc->config->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 > > -- > Ville Syrjälä > Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
