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

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

On Thu, 20 Oct 2016, "Yang, Libin" <libin.yang@intel.com> wrote:
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Wednesday, October 19, 2016 11:09 PM
>> To: Yang, Libin <libin.yang@intel.com>; Lin, Mengdong
>> <mengdong.lin@intel.com>; intel-gfx@lists.freedesktop.org
>> Cc: libin.yang@linux.intel.com; Pandiyan, Dhinakaran
>> <dhinakaran.pandiyan@intel.com>; Zhang, Keqiao
>> <keqiao.zhang@intel.com>; Zhao, Juan J <juan.j.zhao@intel.com>
>> Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in
>> modeset
>> 
>> 
>> 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.)
>
> For HDMI, we found only set N is enough. HW then can handle the remaining.

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2
of what they are in the DP spec table. Why?

Jani.


>
>> 
>> >> > +	{ 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?
>
> No, we don't support HBR 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.
>
> Yes, we can remove it.
>
>> 
>> >> > +		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?
>
> In _disable(), the audio doesn't work. So the value will not impact audio.
> It will not be worse that we do not reset the value in _disable().
> Without reset, the value is "cached". Next time, it may be a little quicker
> for monitor response if we are using the same parameter, I think.
>
> Regards
> Libin
>
>> 
>> 
>> 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

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