Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@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.

Some other comments inline.

[1] http://mid.gmane.org/1440781350-12053-1-git-send-email-david.henningsson@canonical.com

>
> Signed-off-by: Libin Yang <libin.yang@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.

> +
>  #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.

> +
>  #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.

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.

> +	tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> +	base_rate = tmp & (1 << 14);

Nitpick. We prefer (tmp & MASK) >> SHIFT.

> +	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.

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.

> +		}
> +	}
> +
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  
>  	mutex_unlock(&dev_priv->av_mutex);
> -- 
> 1.9.1
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx