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

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

On Mon, 10 Aug 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 value, the N/CTS need to be set manually if audio
> is playing.

When modeset occurs, we disable everything, and then re-enable
everything, and notify the audio driver every step of the way. IIUC
there should be no audio playing across the modeset, and when the
modeset is complete and we've set the valid ELD, the audio driver should
call the callback from the earlier patches to set the correct audio
rate.

Why is this patch needed? Please enlighten me.

Also some comments in-line, provided you've convinced me first. ;)

> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>  drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da2d128..85f3beb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7030,6 +7030,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)
> +
>  #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 eddf37f..082f96d 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	const uint8_t *eld = connector->eld;
>  	uint32_t tmp;
>  	int len, i;
> +	int cvt_idx;
> +	int n_low, n_up, n;
> +	int base_rate, mul, div, rate;
>  
>  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
>  		      pipe_name(pipe), drm_eld_size(eld));
> @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	tmp |= AUDIO_ELD_VALID(pipe);
>  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>  
> +	if ((mode->clock == 297000) ||
> +		(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {
> +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> +		cvt_idx = (tmp >> pipe*8) & 0xff;
> +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> +		base_rate = tmp & (1 << 14);
> +		if (base_rate == 0)
> +			rate = 48000;
> +		else
> +			rate = 44100;
> +		mul = (tmp & (0x7 << 11)) + 1;
> +		div = (tmp & (0x7 << 8)) + 1;
> +		rate = rate * mul / div;
> +	}

This should be abstracted to a separate function.

> +
>  	/* Enable timestamps */
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(mode);
> +
> +	if ((mode->clock != 297000) &&
> +		(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> +			if ((rate == aud_ncts[i].sample_rate) &&
> +				(mode->clock == aud_ncts[i].clock)) {
> +				n = aud_ncts[i].n;
> +				break;
> +			}
> +		}
> +		if (n != 0) {
> +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +			n_low = n & 0xfff;
> +			n_up = (n >> 12) & 0xff;
> +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +			tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> +			tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> +			tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> +			tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> +		}
> +	}

Please de-duplicate the copy-paste from patch 2. At the very least you
should use a helper for finding the parameters from the table.

> +
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  }
>  
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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