[v6,4/4] drm/i915: set proper N/CTS in modeset
diff mbox

Message ID 1441174301-144177-4-git-send-email-libin.yang@intel.com
State New
Headers show

Commit Message

Yang, Libin Sept. 2, 2015, 6:11 a.m. UTC
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.

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(-)

Comments

Jani Nikula Sept. 2, 2015, 8:20 a.m. UTC | #1
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
>
Yang, Libin Sept. 2, 2015, 8:42 a.m. UTC | #2
Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Wednesday, September 02, 2015 4:20 PM
> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch;
> ville.syrjala@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@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.

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

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
Jani Nikula Sept. 2, 2015, 9:02 a.m. UTC | #3
On Wed, 02 Sep 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Wednesday, September 02, 2015 4:20 PM
>> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch;
>> ville.syrjala@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@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.
>
> Yes, we need. David's patch will not trigger the sync_audio_rate
> callback.

Okay.

>
>> 
>> 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.
>
> 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, ...) ?

Yes.

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

Indeed.

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

Well, really, I'm fine with having that part duplicated as-is for now,
we can fix it later. More important to focus on getting
audio_config_get_rate() right.

I don't know if you're still targeting v4.3 with this (up to Takashi I
guess) we'll really need to wrap this up soon.

BR,
Jani.

>
> 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
Takashi Iwai Sept. 2, 2015, 1:34 p.m. UTC | #4
On Wed, 02 Sep 2015 11:02:42 +0200,
Jani Nikula wrote:
> 
> >> 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.
> 
> Well, really, I'm fine with having that part duplicated as-is for now,
> we can fix it later. More important to focus on getting
> audio_config_get_rate() right.
> 
> I don't know if you're still targeting v4.3 with this (up to Takashi I
> guess) we'll really need to wrap this up soon.

I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
can prepare the fixed version soonish, indeed.


thanks,

Takashi
Jani Nikula Sept. 2, 2015, 1:44 p.m. UTC | #5
On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 02 Sep 2015 11:02:42 +0200,
> Jani Nikula wrote:
>> 
>> >> 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.
>> 
>> Well, really, I'm fine with having that part duplicated as-is for now,
>> we can fix it later. More important to focus on getting
>> audio_config_get_rate() right.
>> 
>> I don't know if you're still targeting v4.3 with this (up to Takashi I
>> guess) we'll really need to wrap this up soon.
>
> I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> can prepare the fixed version soonish, indeed.

IIUC patches 1-3 would be useful on their own already, and a fixed
version of patch 4 could follow. Just a thought.

BR,
Jani.

>
>
> thanks,
>
> Takashi
Takashi Iwai Sept. 2, 2015, 1:46 p.m. UTC | #6
On Wed, 02 Sep 2015 15:44:34 +0200,
Jani Nikula wrote:
> 
> On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 02 Sep 2015 11:02:42 +0200,
> > Jani Nikula wrote:
> >> 
> >> >> 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.
> >> 
> >> Well, really, I'm fine with having that part duplicated as-is for now,
> >> we can fix it later. More important to focus on getting
> >> audio_config_get_rate() right.
> >> 
> >> I don't know if you're still targeting v4.3 with this (up to Takashi I
> >> guess) we'll really need to wrap this up soon.
> >
> > I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> > can prepare the fixed version soonish, indeed.
> 
> IIUC patches 1-3 would be useful on their own already, and a fixed
> version of patch 4 could follow. Just a thought.

That's good, then I can take the first three patches.

Daniel, would you like to review these three before I merge them?
I assumed your previous mail as a kind of ack to this series, too.


thanks,

Takashi
Daniel Vetter Sept. 2, 2015, 3:22 p.m. UTC | #7
On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
> On Wed, 02 Sep 2015 15:44:34 +0200,
> Jani Nikula wrote:
> > 
> > On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > On Wed, 02 Sep 2015 11:02:42 +0200,
> > > Jani Nikula wrote:
> > >> 
> > >> >> 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.
> > >> 
> > >> Well, really, I'm fine with having that part duplicated as-is for now,
> > >> we can fix it later. More important to focus on getting
> > >> audio_config_get_rate() right.
> > >> 
> > >> I don't know if you're still targeting v4.3 with this (up to Takashi I
> > >> guess) we'll really need to wrap this up soon.
> > >
> > > I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> > > can prepare the fixed version soonish, indeed.
> > 
> > IIUC patches 1-3 would be useful on their own already, and a fixed
> > version of patch 4 could follow. Just a thought.
> 
> That's good, then I can take the first three patches.
> 
> Daniel, would you like to review these three before I merge them?
> I assumed your previous mail as a kind of ack to this series, too.

fwiw ack on that plan, but given how much I made a mess out of these two
series and mixed them up I wouldn't trust me anyway ;-)

Once the code settles I'll rebase/backmerge so that I can apply the
follow-up documentation/kerneldoc work that's still getting done.

Cheers, Daniel
Takashi Iwai Sept. 2, 2015, 3:36 p.m. UTC | #8
On Wed, 02 Sep 2015 17:22:01 +0200,
Daniel Vetter wrote:
> 
> On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
> > On Wed, 02 Sep 2015 15:44:34 +0200,
> > Jani Nikula wrote:
> > > 
> > > On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > > On Wed, 02 Sep 2015 11:02:42 +0200,
> > > > Jani Nikula wrote:
> > > >> 
> > > >> >> 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.
> > > >> 
> > > >> Well, really, I'm fine with having that part duplicated as-is for now,
> > > >> we can fix it later. More important to focus on getting
> > > >> audio_config_get_rate() right.
> > > >> 
> > > >> I don't know if you're still targeting v4.3 with this (up to Takashi I
> > > >> guess) we'll really need to wrap this up soon.
> > > >
> > > > I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> > > > can prepare the fixed version soonish, indeed.
> > > 
> > > IIUC patches 1-3 would be useful on their own already, and a fixed
> > > version of patch 4 could follow. Just a thought.
> > 
> > That's good, then I can take the first three patches.
> > 
> > Daniel, would you like to review these three before I merge them?
> > I assumed your previous mail as a kind of ack to this series, too.
> 
> fwiw ack on that plan, but given how much I made a mess out of these two
> series and mixed them up I wouldn't trust me anyway ;-)
> 
> Once the code settles I'll rebase/backmerge so that I can apply the
> follow-up documentation/kerneldoc work that's still getting done.

FYI, I've pushed a branch including Libin's patchset (but only patches
1-3) just for checking.  Let me know if this looks OK.


Takashi
Yang, Libin Sept. 4, 2015, 1:56 a.m. UTC | #9
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, September 02, 2015 11:36 PM
> To: Daniel Vetter
> Cc: Jani Nikula; Yang, Libin; alsa-devel@alsa-project.org; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch;
> ville.syrjala@linux.intel.com
> Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
> 
> On Wed, 02 Sep 2015 17:22:01 +0200,
> Daniel Vetter wrote:
> >
> > On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
> > > On Wed, 02 Sep 2015 15:44:34 +0200,
> > > Jani Nikula wrote:
> > > >
> > > > On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > > > On Wed, 02 Sep 2015 11:02:42 +0200,
> > > > > Jani Nikula wrote:
> > > > >>
> > > > >> >> 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.
> > > > >>
> > > > >> Well, really, I'm fine with having that part duplicated as-is for
> now,
> > > > >> we can fix it later. More important to focus on getting
> > > > >> audio_config_get_rate() right.
> > > > >>
> > > > >> I don't know if you're still targeting v4.3 with this (up to
> Takashi I
> > > > >> guess) we'll really need to wrap this up soon.
> > > > >
> > > > > I'm in favor of merging this into 4.3, so it'd be appreciated if
> Libin
> > > > > can prepare the fixed version soonish, indeed.
> > > >
> > > > IIUC patches 1-3 would be useful on their own already, and a
> fixed
> > > > version of patch 4 could follow. Just a thought.
> > >
> > > That's good, then I can take the first three patches.
> > >
> > > Daniel, would you like to review these three before I merge them?
> > > I assumed your previous mail as a kind of ack to this series, too.
> >
> > fwiw ack on that plan, but given how much I made a mess out of
> these two
> > series and mixed them up I wouldn't trust me anyway ;-)
> >
> > Once the code settles I'll rebase/backmerge so that I can apply the
> > follow-up documentation/kerneldoc work that's still getting done.
> 
> FYI, I've pushed a branch including Libin's patchset (but only patches
> 1-3) just for checking.  Let me know if this looks OK.

Thanks, Takashi, Daniel, Jani and Ville.

So I need submit the 4th patch only next time, right?

> 
> 
> Takashi
Takashi Iwai Sept. 4, 2015, 4:50 a.m. UTC | #10
On Fri, 04 Sep 2015 03:56:26 +0200,
Yang, Libin wrote:
> 
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai@suse.de]
> > Sent: Wednesday, September 02, 2015 11:36 PM
> > To: Daniel Vetter
> > Cc: Jani Nikula; Yang, Libin; alsa-devel@alsa-project.org; intel-
> > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch;
> > ville.syrjala@linux.intel.com
> > Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
> > 
> > On Wed, 02 Sep 2015 17:22:01 +0200,
> > Daniel Vetter wrote:
> > >
> > > On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
> > > > On Wed, 02 Sep 2015 15:44:34 +0200,
> > > > Jani Nikula wrote:
> > > > >
> > > > > On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > > > > On Wed, 02 Sep 2015 11:02:42 +0200,
> > > > > > Jani Nikula wrote:
> > > > > >>
> > > > > >> >> 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.
> > > > > >>
> > > > > >> Well, really, I'm fine with having that part duplicated as-is for
> > now,
> > > > > >> we can fix it later. More important to focus on getting
> > > > > >> audio_config_get_rate() right.
> > > > > >>
> > > > > >> I don't know if you're still targeting v4.3 with this (up to
> > Takashi I
> > > > > >> guess) we'll really need to wrap this up soon.
> > > > > >
> > > > > > I'm in favor of merging this into 4.3, so it'd be appreciated if
> > Libin
> > > > > > can prepare the fixed version soonish, indeed.
> > > > >
> > > > > IIUC patches 1-3 would be useful on their own already, and a
> > fixed
> > > > > version of patch 4 could follow. Just a thought.
> > > >
> > > > That's good, then I can take the first three patches.
> > > >
> > > > Daniel, would you like to review these three before I merge them?
> > > > I assumed your previous mail as a kind of ack to this series, too.
> > >
> > > fwiw ack on that plan, but given how much I made a mess out of
> > these two
> > > series and mixed them up I wouldn't trust me anyway ;-)
> > >
> > > Once the code settles I'll rebase/backmerge so that I can apply the
> > > follow-up documentation/kerneldoc work that's still getting done.
> > 
> > FYI, I've pushed a branch including Libin's patchset (but only patches
> > 1-3) just for checking.  Let me know if this looks OK.
> 
> Thanks, Takashi, Daniel, Jani and Ville.
> 
> So I need submit the 4th patch only next time, right?

Yes.


Takashi
Jani Nikula Oct. 19, 2015, 8:25 a.m. UTC | #11
On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> On Wed, 02 Sep 2015 11:02:42 +0200,
> Jani Nikula wrote:
>> 
>> >> 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.
>> 
>> Well, really, I'm fine with having that part duplicated as-is for now,
>> we can fix it later. More important to focus on getting
>> audio_config_get_rate() right.
>> 
>> I don't know if you're still targeting v4.3 with this (up to Takashi I
>> guess) we'll really need to wrap this up soon.
>
> I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> can prepare the fixed version soonish, indeed.

Takashi, just to double check, none of this made it into v4.3 after all?

BR,
Jani.



>
>
> thanks,
>
> Takashi
Takashi Iwai Oct. 19, 2015, 8:27 a.m. UTC | #12
On Mon, 19 Oct 2015 10:25:22 +0200,
Jani Nikula wrote:
> 
> On Wed, 02 Sep 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > On Wed, 02 Sep 2015 11:02:42 +0200,
> > Jani Nikula wrote:
> >> 
> >> >> 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.
> >> 
> >> Well, really, I'm fine with having that part duplicated as-is for now,
> >> we can fix it later. More important to focus on getting
> >> audio_config_get_rate() right.
> >> 
> >> I don't know if you're still targeting v4.3 with this (up to Takashi I
> >> guess) we'll really need to wrap this up soon.
> >
> > I'm in favor of merging this into 4.3, so it'd be appreciated if Libin
> > can prepare the fixed version soonish, indeed.
> 
> Takashi, just to double check, none of this made it into v4.3 after all?

Right, sync_audio_rate was slipped from 4.3 merge window,
unfortunately.


Takashi

Patch
diff mbox

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
+
 #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)
+
 #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;
+	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;
+	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);
+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
 
 	mutex_unlock(&dev_priv->av_mutex);