Re: [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

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

Sorry it's taken me forever to get back to this. Some comments inline.

BR,
Jani.

On Wed, 12 Oct 2016, "Yang, Libin" <libin.yang@intel.com> wrote:
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
>> Lin, Mengdong
>> Sent: Wednesday, October 12, 2016 10:46 AM
>> To: Nikula, Jani <jani.nikula@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: Nikula, Jani <jani.nikula@intel.com>; libin.yang@linux.intel.com;
>> Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
>> Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in
>> modeset
>> 
>> 
>> 
>> > -----Original Message-----
>> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
>> > Behalf Of Jani Nikula
>> > Sent: Monday, October 10, 2016 11:04 PM
>> > To: intel-gfx@lists.freedesktop.org
>> > Cc: Nikula, Jani <jani.nikula@intel.com>; libin.yang@linux.intel.com;
>> > Pandiyan, Dhinakaran <dhinakaran.pandiyan@intel.com>
>> > Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in
>> > modeset
>> >
>> > When modeset occurs and the LS_CLK is set to some special values in DP
>> > mode, the N/M need to be set manually if audio is playing. Otherwise
>> > the first several seconds may be silent in audio playback.
>> >
>> > The relationship of Maud and Naud is expressed in the following equation:
>> > Maud/Naud = 512 * fs / f_LS_Clk
>> >
>> > Please refer VESA DisplayPort Standard spec for details.
>> >
>> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h    |   7 +++
>> >  drivers/gpu/drm/i915/intel_audio.c | 100
>> > ++++++++++++++++++++++++++++++++++++-
>> >  2 files changed, 105 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32
>> > 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -7359,6 +7359,13 @@ enum {
>> >  #define _HSW_AUD_MISC_CTRL_B		0x65110
>> >  #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
>> > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
>> >
>> > +#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
>> > +#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
>> > +#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
>> > _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
>> > +#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
>> > +#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
>> > +#define   AUD_CONFIG_M_MASK		0xfffff
>> 
>> The last line cause misalignment after applying the patch.
>> 
>> > +
>> >  #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)	_MMIO_PIPE(pipe,
>> > _HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B) diff --
>> git
>> > a/drivers/gpu/drm/i915/intel_audio.c
>> > b/drivers/gpu/drm/i915/intel_audio.c
>> > index 81df29ca4947..0bc2701b6c9c 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -57,6 +57,70 @@
>> >   * struct &i915_audio_component_audio_ops @audio_ops is called from
>> > i915 driver.
>> >   */
>> >
>> > +/* DP N/M table */
>> > +#define LC_540M 540000
>> > +#define LC_270M 270000
>> > +#define LC_162M 162000
>> > +
>> > +struct dp_aud_n_m {
>> > +	int sample_rate;
>> > +	int clock;
>> > +	u16 n;
>> > +	u16 m;
>> > +};
>> > +
>> > +static const struct dp_aud_n_m dp_aud_n_m[] = {
>> > +	{ 192000, LC_540M, 5625, 1024 },
>> > +	{ 176400, LC_540M, 9375, 1568 },
>> > +	{ 96000, LC_540M, 5625, 512 },
>> > +	{ 88200, LC_540M, 9375, 784 },
>> > +	{ 48000, LC_540M, 5625, 256 },
>> > +	{ 44100, LC_540M, 9375, 392 },
>> > +	{ 32000, LC_540M, 16875, 512 },

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

>> > +	{ 192000, LC_270M, 5625, 2048 },
>> > +	{ 176400, LC_270M, 9375, 3136 },
>> > +	{ 96000, LC_270M, 5625, 1024 },
>> > +	{ 88200, LC_270M, 9375, 1568 },
>> > +	{ 48000, LC_270M, 5625, 512 },
>> > +	{ 44100, LC_270M, 9375, 784 },
>> > +	{ 32000, LC_270M, 16875, 1024 },
>> > +	{ 192000, LC_162M, 3375, 2048 },
>> > +	{ 176400, LC_162M, 5625, 3136 },
>> > +	{ 96000, LC_162M, 3375, 1024 },
>> > +	{ 88200, LC_162M, 5625, 1568 },
>> > +	{ 48000, LC_162M, 3375, 512 },
>> > +	{ 44100, LC_162M, 5625, 784 },
>> > +	{ 32000, LC_162M, 10125, 1024 },

Do we not support 128 or 64 kHz audio?

>> > +};
>> > +
>> > +static const struct dp_aud_n_m *
>> > +audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
>> > +	int i;
>> > +
>> > +	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
>> > +		if (rate == dp_aud_n_m[i].sample_rate &&
>> > +		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
>> > +			return &dp_aud_n_m[i];
>> > +	}
>> > +
>> > +	return NULL;
>> > +}
>> > +
>> > +static int audio_config_dp_get_m(struct intel_crtc *intel_crtc, int
>> > +rate) {
>> > +	const struct dp_aud_n_m *nm =
>> > audio_config_dp_get_n_m(intel_crtc,
>> > +rate);
>> > +
>> > +	return nm ? nm->m : 0;
>> > +}
>> > +
>> > +static int audio_config_dp_get_n(struct intel_crtc *intel_crtc, int
>> > +rate) {
>> > +	const struct dp_aud_n_m *nm =
>> > audio_config_dp_get_n_m(intel_crtc,
>> > +rate);
>> > +
>> > +	return nm ? nm->n : 0;
>> > +}
>> > +
>> >  static const struct {
>> >  	int clock;
>> >  	u32 config;
>> > @@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
>> > *intel_crtc, enum port port,
>> >  			   const struct drm_display_mode *adjusted_mode)  {
>> >  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>> > +	struct i915_audio_component *acomp = dev_priv-
>> > >audio_component;
>> > +	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
>> >  	enum pipe pipe = intel_crtc->pipe;
>> > -	u32 tmp;
>> > +	u32 tmp, n, m;
>> >
>> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> > @@ -234,7 +300,30 @@ hsw_dp_audio_config_update(struct intel_crtc
>> > *intel_crtc, enum port port,
>> >  	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >  	tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >
>> > +	if (intel_crtc->config->port_clock == LC_540M ||
>> > +	    intel_crtc->config->port_clock == LC_270M ||
>> > +	    intel_crtc->config->port_clock == LC_162M) {

Actually, this check could be removed. audio_config_dp_get_n_m will
always return 0 if this doesn't hold. But not a big deal.

>> > +		n = audio_config_dp_get_n(intel_crtc, rate);
>> > +		if (n != 0) {
>> > +			tmp &= ~AUD_CONFIG_N_MASK;
>> > +			tmp |= AUD_CONFIG_N(n);
>> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> > +		} else {
>> > +			DRM_DEBUG_KMS("no suitable N value is found\n");
>> 
>> Could some comments be added here why we don't return if no valid N value
>> is found?
>
> This is because although we don't need set N value here, but there are some
> other bits to be set. So you can see we call 
> I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> Later. 
>
>> Do we still need to program write the register to let HW calculate N by itself?
>
> Ideally, all the frequency should be in the table. But if the frequency is not
> in our table, we will let HW calculate N by itself.
>
>> 
>> > +		}
>> > +	}
>> > +
>> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> 
>> And if no valid N value is found, need we go ahead to check M value?
>
> If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate);
> will also return 0 and skip the m setting.

If we can't find M/N values, the HSW_AUD_CFG register is set to use
automatic N value. However, the HSW_AUD_M_CTS_ENABLE register is not
updated at all, and could potentially have stale stuff. Do we need to
clear the AUD_M_CTS_M_VALUE_INDEX and/or AUD_M_CTS_M_PROG_ENABLE bits?

We only do this stuff in hsw_audio_codec_enable now. Do we need to do
something in hsw_audio_codec_disable too?


BR,
Jani.


>
> Regards,
> Libin
>
>> 
>> > +
>> > +	m = audio_config_dp_get_m(intel_crtc, rate);
>> > +	if (m) {
>> > +		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
>> > +		tmp &= ~AUD_CONFIG_M_MASK;
>> > +		tmp |= m;
>> > +		tmp |= AUD_M_CTS_M_VALUE_INDEX;
>> > +		tmp |= AUD_M_CTS_M_PROG_ENABLE;
>> > +		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
>> > +	}
>> >  }
>> 
>> Thanks
>> Mengdong
>> 
>> >  static void
>> > @@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct intel_crtc
>> > *intel_crtc, enum port port,
>> >  	}
>> >
>> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> > +
>> > +	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
>> > +	tmp &= ~AUD_CONFIG_M_MASK;
>> > +	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
>> > +	tmp |= AUD_M_CTS_M_PROG_ENABLE;
>> > +	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
>> >  }
>> >
>> >  static void
>> > @@ -687,7 +782,8 @@ static int
>> > i915_audio_component_sync_audio_rate(struct device *kdev, int port,
>> >  	/* 1. get the pipe */
>> >  	intel_encoder = get_saved_enc(dev_priv, port, pipe);
>> >  	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)) {
>> >  		DRM_DEBUG_KMS("Not valid for port %c\n",
>> port_name(port));
>> >  		err = -ENODEV;
>> >  		goto unlock;
>> > --
>> > 2.1.4
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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