[Intel-gfx] [PATCH v3 1/3] drm/i915: set proper N/M in modeset

[Yang, Libin]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Friday, August 5, 2016 4:45 PM
> To: Yang, Libin <libin.yang at intel.com>
> Cc: libin.yang at linux.intel.com; intel-gfx at lists.freedesktop.org;
> jani.nikula at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>;
> tiwai at suse.de
> 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 at linux.intel.com]
> > > Sent: Friday, August 5, 2016 3:17 PM
> > > To: Yang, Libin <libin.yang at intel.com>
> > > Cc: libin.yang at linux.intel.com; intel-gfx at lists.freedesktop.org;
> > > jani.nikula at linux.intel.com; Vetter, Daniel
> > > <daniel.vetter at intel.com>; tiwai at suse.de
> > > 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