[alsa-devel] [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Wednesday, September 02, 2015 4:20 PM
> To: Yang, Libin; alsa-devel at alsa-project.org; tiwai at suse.de; intel-
> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch;
> ville.syrjala at linux.intel.com
> Cc: Yang, Libin
> Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
> 
> On Wed, 02 Sep 2015, libin.yang at intel.com wrote:
> > From: Libin Yang <libin.yang at intel.com>
> >
> > When modeset occurs and the TMDS frequency is set to some
> > speical values, the N/CTS need to be set manually if audio
> > is playing.
> 
> Do we still need this patch after David Henningsson's series [1]? IIUC
> you will now always get the callback on modesets, so you should be
> able
> to, uh, callback on the callback to set audio rate. Granted, with this I
> suppose you reduce the time there might be wrong N/CTS after enable.

Yes, we need. David's patch will not trigger the sync_audio_rate
callback.

> 
> Some other comments inline.
> 
> [1] http://mid.gmane.org/1440781350-12053-1-git-send-email-
> david.henningsson at canonical.com
> 
> >
> > Signed-off-by: Libin Yang <libin.yang at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |  8 ++++++++
> >  drivers/gpu/drm/i915/intel_audio.c | 40
> +++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index ae7fa3e..5bdee36 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_MISC_CTRL_A, \
> >  					_HSW_AUD_MISC_CTRL_B)
> >
> > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> 
> Nitpick. The bit fields are not defined.

OK, I will add the bit definition. 

> 
> > +
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > @@ -7072,6 +7074,12 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_DIG_CNVT_2)
> >  #define DIP_PORT_SEL_MASK		0x3
> >
> > +#define _HSW_AUD_STR_DESC_1		0x65084
> > +#define _HSW_AUD_STR_DESC_2		0x65184
> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> > +					 _HSW_AUD_STR_DESC_1,
> 	\
> > +					 _HSW_AUD_STR_DESC_2)
> 
> Nitpick. The bit fields are not defined. I think it's fine to use _PIPE
> macro, but you should probably use "converter" or "cvt" or something
> for
> the parameter name to not mislead people into thinking this is pipe
> based.

Do you mean it should be like:
_PIPE(cvt, _HSW_AUD_STR_DESC_1, ...) ?

> 
> > +
> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index a021720..acdb68c 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct
> intel_crtc *crtc,
> >  		return false;
> >  }
> >
> > +static int audio_config_get_rate(struct drm_i915_private *dev_priv,
> > +						enum pipe pipe)
> > +{
> > +	uint32_t tmp;
> > +	int cvt_idx;
> > +	int base_rate, mul, div, rate;
> > +
> > +	tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> > +	cvt_idx = (tmp >> (pipe * 8)) & 0xff;
> 
> This one needs to be addressed. Are you sure it's indexed by pipe? The
> spec is vague.

Yes, I did lot of test to confirm it.

> 
> Bits 7:0 are "control B", 15:8 are "control C", and so on, while your
> (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C,
> etc. This would speak for port, not pipe, as pipe A should be valid
> while port A not.

We found there is something wrong/or vague in the spec.

> 
> > +	tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> > +	base_rate = tmp & (1 << 14);
> 
> Nitpick. We prefer (tmp & MASK) >> SHIFT.

OK, I will change it.

> 
> > +	if (base_rate == 0)
> > +		rate = 48000;
> > +	else
> > +		rate = 44100;
> > +	mul = (tmp & (0x7 << 11)) + 1;
> > +	div = (tmp & (0x7 << 8)) + 1;
> 
> This one needs to be addressed. This is bogus. The +1 would work if
> you
> actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted
> value. Your multipliers and divisors are *way* off.

OK, I will check it and change the patch.
> 
> NAK on this patch.
> 
> > +	rate = rate * mul / div;
> > +	return rate;
> > +}
> > +
> >  static bool intel_eld_uptodate(struct drm_connector *connector,
> >  			       int reg_eldv, uint32_t bits_eldv,
> >  			       int reg_elda, uint32_t bits_elda,
> > @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	const uint8_t *eld = connector->eld;
> >  	uint32_t tmp;
> >  	int len, i;
> > +	int n_low, n_up, n;
> > +	int rate;
> >
> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> ELD\n",
> >  		      pipe_name(pipe), drm_eld_size(eld));
> > @@ -302,12 +325,27 @@ static void
> hsw_audio_codec_enable(struct drm_connector *connector,
> >  	/* Enable timestamps */
> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > -	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >  	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
> >  	if (intel_pipe_has_type(intel_crtc,
> INTEL_OUTPUT_DISPLAYPORT))
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> > +
> > +	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +	if (audio_rate_need_prog(intel_crtc, mode)) {
> > +		rate = audio_config_get_rate(dev_priv, pipe);
> > +		n = audio_config_get_n(mode, rate);
> > +		if (n != 0) {
> > +			n_low = n & 0xfff;
> > +			n_up = (n >> 12) & 0xff;
> > +			tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
> > +
> AUD_CONFIG_LOWER_N_MASK);
> > +			tmp |= ((n_up <<
> AUD_CONFIG_UPPER_N_SHIFT) |
> > +					(n_low <<
> AUD_CONFIG_LOWER_N_SHIFT) |
> > +
> 	AUD_CONFIG_N_PROG_ENABLE);
> 
> Nitpick. I'd prefer some sharing with the similar blocks from the
> earlier patch. Also a debug message on n == 0 would be nice; you
> probably didn't notice your audio_config_get_rate() wasn't working
> right
> because this silently fell back to the automatic mode here.

OK, I will add the msg. As you and Ville are insisting on
sharing code, I will do it in next version.

Regards,
Libin
> 
> > +		}
> > +	}
> > +
> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >
> >  	mutex_unlock(&dev_priv->av_mutex);
> > --
> > 1.9.1
> >
> 
> --
> Jani Nikula, Intel Open Source Technology Center