[v5,2/4] drm/i915: implement sync_audio_rate callback

Commit Message

Yang, Libin Aug. 18, 2015, 6:51 a.m. UTC

From: Libin Yang <[e-mail]>

HDMI audio may not work at some frequencies
with the HW provided N/CTS.

This patch sets the proper N value for the
given audio sample rate at the impacted frequencies.
At other frequencies, it will use the N/CTS value
which HW provides.

Signed-off-by: Libin Yang <[e-mail]>
---
 drivers/gpu/drm/i915/intel_audio.c | 119 +++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

Comments

Ville Syrjälä Aug. 21, 2015, 3:14 p.m. UTC | #1

On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail] wrote:
> From: Libin Yang <[e-mail]>
> 
> HDMI audio may not work at some frequencies
> with the HW provided N/CTS.
> 
> This patch sets the proper N value for the
> given audio sample rate at the impacted frequencies.
> At other frequencies, it will use the N/CTS value
> which HW provides.
> 
> Signed-off-by: Libin Yang <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_audio.c | 119 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index dc32cf4..96b97be 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -68,6 +68,31 @@ static const struct {
>  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
>  };
>  
> +/* HDMI N/CTS table */
> +#define TMDS_297M 297000
> +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)

I don't really like the defines.

> +static const struct {
> +	int sample_rate;
> +	int clock;
> +	int n;
> +	int cts;
> +} aud_ncts[] = {
> +	{ 44100, TMDS_296M, 4459, 234375 },
> +	{ 44100, TMDS_297M, 4704, 247500 },
> +	{ 48000, TMDS_296M, 5824, 281250 },
> +	{ 48000, TMDS_297M, 5120, 247500 },
> +	{ 32000, TMDS_296M, 5824, 421875 },
> +	{ 32000, TMDS_297M, 3072, 222750 },
> +	{ 88200, TMDS_296M, 8918, 234375 },
> +	{ 88200, TMDS_297M, 9408, 247500 },
> +	{ 96000, TMDS_296M, 11648, 281250 },
> +	{ 96000, TMDS_297M, 10240, 247500 },
> +	{ 176400, TMDS_296M, 17836, 234375 },
> +	{ 176400, TMDS_297M, 18816, 247500 },
> +	{ 44100, TMDS_296M, 23296, 281250 },
> +	{ 44100, TMDS_297M, 20480, 247500 },
> +};

Last two should be 192 kHz. All the other values seem to match the spec.

These do assume 8bpc, but as the spec doesn't even specify N/CTS
values for deep color modes @ 297MHz, so I suppose the expectations is
that the max TMDS clock will never be so high as to allow them.

If/when we start using manual programming for other TMDS clock rates
we'll need to consider 12bpc as well.

> +
>  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
>  static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
>  {
> @@ -90,6 +115,31 @@ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
>  	return hdmi_audio_clock[i].config;
>  }
>  
> +static int audio_config_get_n(struct drm_display_mode *mode, int rate)

mode can be const.

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> +		if ((rate == aud_ncts[i].sample_rate) &&
> +			(mode->clock == aud_ncts[i].clock)) {
> +			return aud_ncts[i].n;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/* check whether N/CTS/M need be set manually */
> +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> +					struct drm_display_mode *mode)
> +{
> +	if (((mode->clock == TMDS_297M) ||
> +		 (mode->clock == TMDS_296M)) &&
> +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> +		return true;
> +	else
> +		return false;
> +}
> +
>  static bool intel_eld_uptodate(struct drm_connector *connector,
>  			       int reg_eldv, uint32_t bits_eldv,
>  			       int reg_elda, uint32_t bits_elda,
> @@ -514,12 +564,81 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
>  	return ret;
>  }
>  
> +static int i915_audio_component_sync_audio_rate(struct device *dev,
> +						int port, int rate)
> +{
> +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> +	struct drm_device *drm_dev = dev_priv->dev;
> +	struct intel_encoder *intel_encoder;
> +	struct intel_digital_port *intel_dig_port;
> +	struct intel_crtc *crtc;
> +	struct drm_display_mode *mode;
> +	enum pipe pipe = -1;
> +	u32 tmp;
> +	int n_low, n_up, n;
> +
> +	/* 1. get the pipe */
> +	for_each_intel_encoder(drm_dev, intel_encoder) {
> +		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
> +			continue;
> +		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
> +		if (port == intel_dig_port->port) {
> +			crtc = to_intel_crtc(intel_encoder->base.crtc);

This sort of thing would need some locking to safely start digging at
the modeset state.

I would probably not do that, and instead add some new lock(s) for this.
The modeset code would then fill out some relevant information protected
by that same lock, and this function could then go look at it without
having to worry about the rest of modeset locking/state.

> +			if (!crtc) {
> +				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
> +				continue;
> +			}
> +			pipe = crtc->pipe;
> +			break;
> +		}
> +	}
> +
> +	if (pipe == INVALID_PIPE) {
> +		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> +		return -ENODEV;
> +	}
> +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> +				  pipe_name(pipe), port_name(port));
> +	mode = &crtc->config->base.adjusted_mode;
> +
> +	/* 2. check whether to set the N/CTS/M manually or not */
> +	if (!audio_rate_need_prog(crtc, mode)) {
> +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);

This stuff would need to be abstracted to work on pre-HSW too.

> +		return 0;
> +	}
> +
> +	n = audio_config_get_n(mode, rate);
> +	if (n == 0) {
> +		DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
> +					  port_name(port));
> +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> +		return 0;
> +	}
> +	n_low = n & 0xfff;
> +	n_up = (n >> 12) & 0xff;
> +
> +	/* 4. set the N/CTS/M */
> +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> +	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);
> +
> +	return 0;
> +}
> +
>  static const struct i915_audio_component_ops i915_audio_component_ops = {
>  	.owner		= THIS_MODULE,
>  	.get_power	= i915_audio_component_get_power,
>  	.put_power	= i915_audio_component_put_power,
>  	.codec_wake_override = i915_audio_component_codec_wake_override,
>  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> +	.sync_audio_rate = i915_audio_component_sync_audio_rate,
>  };
>  
>  static int i915_audio_component_bind(struct device *i915_dev,
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Yang, Libin Aug. 24, 2015, 2:38 a.m. UTC | #2

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Friday, August 21, 2015 11:14 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; intel-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> sync_audio_rate callback
> 
> On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail]
> wrote:
> > From: Libin Yang <[e-mail]>
> >
> > HDMI audio may not work at some frequencies
> > with the HW provided N/CTS.
> >
> > This patch sets the proper N value for the
> > given audio sample rate at the impacted frequencies.
> > At other frequencies, it will use the N/CTS value
> > which HW provides.
> >
> > Signed-off-by: Libin Yang <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_audio.c | 119
> +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 119 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index dc32cf4..96b97be 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -68,6 +68,31 @@ static const struct {
> >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> >  };
> >
> > +/* HDMI N/CTS table */
> > +#define TMDS_297M 297000
> > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> 
> I don't really like the defines.

I agree it's not a good name. Do you have some suggestions?

> 
> > +static const struct {
> > +	int sample_rate;
> > +	int clock;
> > +	int n;
> > +	int cts;
> > +} aud_ncts[] = {
> > +	{ 44100, TMDS_296M, 4459, 234375 },
> > +	{ 44100, TMDS_297M, 4704, 247500 },
> > +	{ 48000, TMDS_296M, 5824, 281250 },
> > +	{ 48000, TMDS_297M, 5120, 247500 },
> > +	{ 32000, TMDS_296M, 5824, 421875 },
> > +	{ 32000, TMDS_297M, 3072, 222750 },
> > +	{ 88200, TMDS_296M, 8918, 234375 },
> > +	{ 88200, TMDS_297M, 9408, 247500 },
> > +	{ 96000, TMDS_296M, 11648, 281250 },
> > +	{ 96000, TMDS_297M, 10240, 247500 },
> > +	{ 176400, TMDS_296M, 17836, 234375 },
> > +	{ 176400, TMDS_297M, 18816, 247500 },
> > +	{ 44100, TMDS_296M, 23296, 281250 },
> > +	{ 44100, TMDS_297M, 20480, 247500 },
> > +};
> 
> Last two should be 192 kHz. All the other values seem to match the
> spec.

Oh, my typo. Thanks for pointing it out.

> 
> These do assume 8bpc, but as the spec doesn't even specify N/CTS
> values for deep color modes @ 297MHz, so I suppose the expectations
> is
> that the max TMDS clock will never be so high as to allow them.
> 
> If/when we start using manual programming for other TMDS clock
> rates
> we'll need to consider 12bpc as well.

These values are recommended from HDMI spec. It's not related
to the bpp. Am I wrong? I'm find the value from HDMISpecification
1.4: table 7-1, table 7-2 and table 7-3.

> 
> > +
> >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> >  static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
> *mode)
> >  {
> > @@ -90,6 +115,31 @@ static u32
> audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
> >  	return hdmi_audio_clock[i].config;
> >  }
> >
> > +static int audio_config_get_n(struct drm_display_mode *mode, int
> rate)
> 
> mode can be const.

OK. I will change it.

> 
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > +		if ((rate == aud_ncts[i].sample_rate) &&
> > +			(mode->clock == aud_ncts[i].clock)) {
> > +			return aud_ncts[i].n;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +/* check whether N/CTS/M need be set manually */
> > +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> > +					struct drm_display_mode
> *mode)
> > +{
> > +	if (((mode->clock == TMDS_297M) ||
> > +		 (mode->clock == TMDS_296M)) &&
> > +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> > +		return true;
> > +	else
> > +		return false;
> > +}
> > +
> >  static bool intel_eld_uptodate(struct drm_connector *connector,
> >  			       int reg_eldv, uint32_t bits_eldv,
> >  			       int reg_elda, uint32_t bits_elda,
> > @@ -514,12 +564,81 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *dev)
> >  	return ret;
> >  }
> >
> > +static int i915_audio_component_sync_audio_rate(struct device
> *dev,
> > +						int port, int rate)
> > +{
> > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > +	struct drm_device *drm_dev = dev_priv->dev;
> > +	struct intel_encoder *intel_encoder;
> > +	struct intel_digital_port *intel_dig_port;
> > +	struct intel_crtc *crtc;
> > +	struct drm_display_mode *mode;
> > +	enum pipe pipe = -1;
> > +	u32 tmp;
> > +	int n_low, n_up, n;
> > +
> > +	/* 1. get the pipe */
> > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > +		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
> > +			continue;
> > +		intel_dig_port = enc_to_dig_port(&intel_encoder-
> >base);
> > +		if (port == intel_dig_port->port) {
> > +			crtc = to_intel_crtc(intel_encoder->base.crtc);
> 
> This sort of thing would need some locking to safely start digging at
> the modeset state.
> 
> I would probably not do that, and instead add some new lock(s) for this.
> The modeset code would then fill out some relevant information
> protected
> by that same lock, and this function could then go look at it without
> having to worry about the rest of modeset locking/state.

From audio side, there is no competition when calling the function,
I think.
For protecting the competition between audio and gfx driver,
it seems we need use the lock from gfx side. Do you have suggested
 lock I can use?

> 
> > +			if (!crtc) {
> > +				DRM_DEBUG_KMS("%s: crtc is
> NULL\n", __func__);
> > +				continue;
> > +			}
> > +			pipe = crtc->pipe;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (pipe == INVALID_PIPE) {
> > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> port_name(port));
> > +		return -ENODEV;
> > +	}
> > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > +				  pipe_name(pipe), port_name(port));
> > +	mode = &crtc->config->base.adjusted_mode;
> > +
> > +	/* 2. check whether to set the N/CTS/M manually or not */
> > +	if (!audio_rate_need_prog(crtc, mode)) {
> > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> 
> This stuff would need to be abstracted to work on pre-HSW too.

Right, I need judge the platform firstly.

> 
> > +		return 0;
> > +	}
> > +
> > +	n = audio_config_get_n(mode, rate);
> > +	if (n == 0) {
> > +		DRM_DEBUG_KMS("Using automatic mode for N value
> on port %c\n",
> > +					  port_name(port));
> > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > +		return 0;
> > +	}
> > +	n_low = n & 0xfff;
> > +	n_up = (n >> 12) & 0xff;
> > +
> > +	/* 4. set the N/CTS/M */
> > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +	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);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct i915_audio_component_ops
> i915_audio_component_ops = {
> >  	.owner		= THIS_MODULE,
> >  	.get_power	= i915_audio_component_get_power,
> >  	.put_power	= i915_audio_component_put_power,
> >  	.codec_wake_override =
> i915_audio_component_codec_wake_override,
> >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > +	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> >  };
> >
> >  static int i915_audio_component_bind(struct device *i915_dev,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > [e-mail]
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Aug. 24, 2015, 12:53 p.m. UTC | #3

On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Friday, August 21, 2015 11:14 PM
> > To: Yang, Libin
> > Cc: [e-mail]; [e-mail]; intel-
> > [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > sync_audio_rate callback
> > 
> > On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail]
> > wrote:
> > > From: Libin Yang <[e-mail]>
> > >
> > > HDMI audio may not work at some frequencies
> > > with the HW provided N/CTS.
> > >
> > > This patch sets the proper N value for the
> > > given audio sample rate at the impacted frequencies.
> > > At other frequencies, it will use the N/CTS value
> > > which HW provides.
> > >
> > > Signed-off-by: Libin Yang <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_audio.c | 119
> > +++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 119 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > > index dc32cf4..96b97be 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -68,6 +68,31 @@ static const struct {
> > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > >  };
> > >
> > > +/* HDMI N/CTS table */
> > > +#define TMDS_297M 297000
> > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > 
> > I don't really like the defines.
> 
> I agree it's not a good name. Do you have some suggestions?

I'd probably just have used raw numbers myself.

> 
> > 
> > > +static const struct {
> > > +	int sample_rate;
> > > +	int clock;
> > > +	int n;
> > > +	int cts;
> > > +} aud_ncts[] = {
> > > +	{ 44100, TMDS_296M, 4459, 234375 },
> > > +	{ 44100, TMDS_297M, 4704, 247500 },
> > > +	{ 48000, TMDS_296M, 5824, 281250 },
> > > +	{ 48000, TMDS_297M, 5120, 247500 },
> > > +	{ 32000, TMDS_296M, 5824, 421875 },
> > > +	{ 32000, TMDS_297M, 3072, 222750 },
> > > +	{ 88200, TMDS_296M, 8918, 234375 },
> > > +	{ 88200, TMDS_297M, 9408, 247500 },
> > > +	{ 96000, TMDS_296M, 11648, 281250 },
> > > +	{ 96000, TMDS_297M, 10240, 247500 },
> > > +	{ 176400, TMDS_296M, 17836, 234375 },
> > > +	{ 176400, TMDS_297M, 18816, 247500 },
> > > +	{ 44100, TMDS_296M, 23296, 281250 },
> > > +	{ 44100, TMDS_297M, 20480, 247500 },
> > > +};
> > 
> > Last two should be 192 kHz. All the other values seem to match the
> > spec.
> 
> Oh, my typo. Thanks for pointing it out.
> 
> > 
> > These do assume 8bpc, but as the spec doesn't even specify N/CTS
> > values for deep color modes @ 297MHz, so I suppose the expectations
> > is
> > that the max TMDS clock will never be so high as to allow them.
> > 
> > If/when we start using manual programming for other TMDS clock
> > rates
> > we'll need to consider 12bpc as well.
> 
> These values are recommended from HDMI spec. It's not related
> to the bpp. Am I wrong? I'm find the value from HDMISpecification
> 1.4: table 7-1, table 7-2 and table 7-3.

There are separate tables for deep color modes in appendix D. Tables
D-1 through D-3 are of interest to us since we can only do the 12bpc
deep color mode.

> 
> > 
> > > +
> > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > >  static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
> > *mode)
> > >  {
> > > @@ -90,6 +115,31 @@ static u32
> > audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
> > >  	return hdmi_audio_clock[i].config;
> > >  }
> > >
> > > +static int audio_config_get_n(struct drm_display_mode *mode, int
> > rate)
> > 
> > mode can be const.
> 
> OK. I will change it.
> 
> > 
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > +		if ((rate == aud_ncts[i].sample_rate) &&
> > > +			(mode->clock == aud_ncts[i].clock)) {
> > > +			return aud_ncts[i].n;
> > > +		}
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > > +/* check whether N/CTS/M need be set manually */
> > > +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > +					struct drm_display_mode
> > *mode)
> > > +{
> > > +	if (((mode->clock == TMDS_297M) ||
> > > +		 (mode->clock == TMDS_296M)) &&
> > > +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> > > +		return true;
> > > +	else
> > > +		return false;
> > > +}
> > > +
> > >  static bool intel_eld_uptodate(struct drm_connector *connector,
> > >  			       int reg_eldv, uint32_t bits_eldv,
> > >  			       int reg_elda, uint32_t bits_elda,
> > > @@ -514,12 +564,81 @@ static int
> > i915_audio_component_get_cdclk_freq(struct device *dev)
> > >  	return ret;
> > >  }
> > >
> > > +static int i915_audio_component_sync_audio_rate(struct device
> > *dev,
> > > +						int port, int rate)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > +	struct intel_encoder *intel_encoder;
> > > +	struct intel_digital_port *intel_dig_port;
> > > +	struct intel_crtc *crtc;
> > > +	struct drm_display_mode *mode;
> > > +	enum pipe pipe = -1;
> > > +	u32 tmp;
> > > +	int n_low, n_up, n;
> > > +
> > > +	/* 1. get the pipe */
> > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > +		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
> > > +			continue;
> > > +		intel_dig_port = enc_to_dig_port(&intel_encoder-
> > >base);
> > > +		if (port == intel_dig_port->port) {
> > > +			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > 
> > This sort of thing would need some locking to safely start digging at
> > the modeset state.
> > 
> > I would probably not do that, and instead add some new lock(s) for this.
> > The modeset code would then fill out some relevant information
> > protected
> > by that same lock, and this function could then go look at it without
> > having to worry about the rest of modeset locking/state.
> 
> >From audio side, there is no competition when calling the function,
> I think.
> For protecting the competition between audio and gfx driver,
> it seems we need use the lock from gfx side. Do you have suggested
>  lock I can use?

To do it like this we'd need pretty much all the modeset locks, which
to me feels a bit troublesome if the audio driver can call this at
any point. So I suggested that we may want to add some kind of new
audio lock to i915.

> 
> > 
> > > +			if (!crtc) {
> > > +				DRM_DEBUG_KMS("%s: crtc is
> > NULL\n", __func__);
> > > +				continue;
> > > +			}
> > > +			pipe = crtc->pipe;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (pipe == INVALID_PIPE) {
> > > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> > port_name(port));
> > > +		return -ENODEV;
> > > +	}
> > > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > > +				  pipe_name(pipe), port_name(port));
> > > +	mode = &crtc->config->base.adjusted_mode;
> > > +
> > > +	/* 2. check whether to set the N/CTS/M manually or not */
> > > +	if (!audio_rate_need_prog(crtc, mode)) {
> > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > 
> > This stuff would need to be abstracted to work on pre-HSW too.
> 
> Right, I need judge the platform firstly.
> 
> > 
> > > +		return 0;
> > > +	}
> > > +
> > > +	n = audio_config_get_n(mode, rate);
> > > +	if (n == 0) {
> > > +		DRM_DEBUG_KMS("Using automatic mode for N value
> > on port %c\n",
> > > +					  port_name(port));
> > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > +		return 0;
> > > +	}
> > > +	n_low = n & 0xfff;
> > > +	n_up = (n >> 12) & 0xff;
> > > +
> > > +	/* 4. set the N/CTS/M */
> > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > +	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);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct i915_audio_component_ops
> > i915_audio_component_ops = {
> > >  	.owner		= THIS_MODULE,
> > >  	.get_power	= i915_audio_component_get_power,
> > >  	.put_power	= i915_audio_component_put_power,
> > >  	.codec_wake_override =
> > i915_audio_component_codec_wake_override,
> > >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > > +	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> > >  };
> > >
> > >  static int i915_audio_component_bind(struct device *i915_dev,
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [e-mail]
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

Takashi Iwai Aug. 24, 2015, 12:58 p.m. UTC | #4

On Mon, 24 Aug 2015 14:53:19 +0200,
Ville Syrjälä wrote:
> 
> On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Friday, August 21, 2015 11:14 PM
> > > To: Yang, Libin
> > > Cc: [e-mail]; [e-mail]; intel-
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > sync_audio_rate callback
> > > 
> > > On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail]
> > > wrote:
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > HDMI audio may not work at some frequencies
> > > > with the HW provided N/CTS.
> > > >
> > > > This patch sets the proper N value for the
> > > > given audio sample rate at the impacted frequencies.
> > > > At other frequencies, it will use the N/CTS value
> > > > which HW provides.
> > > >
> > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_audio.c | 119
> > > +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 119 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > index dc32cf4..96b97be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -68,6 +68,31 @@ static const struct {
> > > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > > >  };
> > > >
> > > > +/* HDMI N/CTS table */
> > > > +#define TMDS_297M 297000
> > > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > > 
> > > I don't really like the defines.
> > 
> > I agree it's not a good name. Do you have some suggestions?
> 
> I'd probably just have used raw numbers myself.

It's quite unsafe.  You can easily overlook, e.g. 29700 instead of
297000.


Takashi

Yang, Libin Aug. 24, 2015, 3:35 p.m. UTC | #5

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Monday, August 24, 2015 8:53 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; intel-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> sync_audio_rate callback
> 
> On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Friday, August 21, 2015 11:14 PM
> > > To: Yang, Libin
> > > Cc: [e-mail]; [e-mail]; intel-
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > sync_audio_rate callback
> > >
> > > On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail]
> > > wrote:
> > > > From: Libin Yang <[e-mail]>
> > > >
> > > > HDMI audio may not work at some frequencies
> > > > with the HW provided N/CTS.
> > > >
> > > > This patch sets the proper N value for the
> > > > given audio sample rate at the impacted frequencies.
> > > > At other frequencies, it will use the N/CTS value
> > > > which HW provides.
> > > >
> > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_audio.c | 119
> > > +++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 119 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > index dc32cf4..96b97be 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -68,6 +68,31 @@ static const struct {
> > > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > > >  };
> > > >
> > > > +/* HDMI N/CTS table */
> > > > +#define TMDS_297M 297000
> > > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > >
> > > I don't really like the defines.
> >
> > I agree it's not a good name. Do you have some suggestions?
> 
> I'd probably just have used raw numbers myself.
> 
> >
> > >
> > > > +static const struct {
> > > > +	int sample_rate;
> > > > +	int clock;
> > > > +	int n;
> > > > +	int cts;
> > > > +} aud_ncts[] = {
> > > > +	{ 44100, TMDS_296M, 4459, 234375 },
> > > > +	{ 44100, TMDS_297M, 4704, 247500 },
> > > > +	{ 48000, TMDS_296M, 5824, 281250 },
> > > > +	{ 48000, TMDS_297M, 5120, 247500 },
> > > > +	{ 32000, TMDS_296M, 5824, 421875 },
> > > > +	{ 32000, TMDS_297M, 3072, 222750 },
> > > > +	{ 88200, TMDS_296M, 8918, 234375 },
> > > > +	{ 88200, TMDS_297M, 9408, 247500 },
> > > > +	{ 96000, TMDS_296M, 11648, 281250 },
> > > > +	{ 96000, TMDS_297M, 10240, 247500 },
> > > > +	{ 176400, TMDS_296M, 17836, 234375 },
> > > > +	{ 176400, TMDS_297M, 18816, 247500 },
> > > > +	{ 44100, TMDS_296M, 23296, 281250 },
> > > > +	{ 44100, TMDS_297M, 20480, 247500 },
> > > > +};
> > >
> > > Last two should be 192 kHz. All the other values seem to match the
> > > spec.
> >
> > Oh, my typo. Thanks for pointing it out.
> >
> > >
> > > These do assume 8bpc, but as the spec doesn't even specify N/CTS
> > > values for deep color modes @ 297MHz, so I suppose the
> expectations
> > > is
> > > that the max TMDS clock will never be so high as to allow them.
> > >
> > > If/when we start using manual programming for other TMDS clock
> > > rates
> > > we'll need to consider 12bpc as well.
> >
> > These values are recommended from HDMI spec. It's not related
> > to the bpp. Am I wrong? I'm find the value from HDMISpecification
> > 1.4: table 7-1, table 7-2 and table 7-3.
> 
> There are separate tables for deep color modes in appendix D. Tables
> D-1 through D-3 are of interest to us since we can only do the 12bpc
> deep color mode.

Yes, we found the D tables in the spec before, and it seems a little
Complicated. And the table 7-x seems to be more general. This is
the reason we use table 7-x.

OK. We will use D-1, D-2 and D-3 table for N/CTS.

> 
> >
> > >
> > > > +
> > > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > >  static u32 audio_config_hdmi_pixel_clock(struct
> drm_display_mode
> > > *mode)
> > > >  {
> > > > @@ -90,6 +115,31 @@ static u32
> > > audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
> > > >  	return hdmi_audio_clock[i].config;
> > > >  }
> > > >
> > > > +static int audio_config_get_n(struct drm_display_mode *mode,
> int
> > > rate)
> > >
> > > mode can be const.
> >
> > OK. I will change it.
> >
> > >
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > +		if ((rate == aud_ncts[i].sample_rate) &&
> > > > +			(mode->clock == aud_ncts[i].clock)) {
> > > > +			return aud_ncts[i].n;
> > > > +		}
> > > > +	}
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/* check whether N/CTS/M need be set manually */
> > > > +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > > +					struct drm_display_mode
> > > *mode)
> > > > +{
> > > > +	if (((mode->clock == TMDS_297M) ||
> > > > +		 (mode->clock == TMDS_296M)) &&
> > > > +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> > > > +		return true;
> > > > +	else
> > > > +		return false;
> > > > +}
> > > > +
> > > >  static bool intel_eld_uptodate(struct drm_connector *connector,
> > > >  			       int reg_eldv, uint32_t bits_eldv,
> > > >  			       int reg_elda, uint32_t bits_elda,
> > > > @@ -514,12 +564,81 @@ static int
> > > i915_audio_component_get_cdclk_freq(struct device *dev)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +static int i915_audio_component_sync_audio_rate(struct device
> > > *dev,
> > > > +						int port, int rate)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > > +	struct intel_encoder *intel_encoder;
> > > > +	struct intel_digital_port *intel_dig_port;
> > > > +	struct intel_crtc *crtc;
> > > > +	struct drm_display_mode *mode;
> > > > +	enum pipe pipe = -1;
> > > > +	u32 tmp;
> > > > +	int n_low, n_up, n;
> > > > +
> > > > +	/* 1. get the pipe */
> > > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > > +		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
> > > > +			continue;
> > > > +		intel_dig_port = enc_to_dig_port(&intel_encoder-
> > > >base);
> > > > +		if (port == intel_dig_port->port) {
> > > > +			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > >
> > > This sort of thing would need some locking to safely start digging
> at
> > > the modeset state.
> > >
> > > I would probably not do that, and instead add some new lock(s) for
> this.
> > > The modeset code would then fill out some relevant information
> > > protected
> > > by that same lock, and this function could then go look at it
> without
> > > having to worry about the rest of modeset locking/state.
> >
> > >From audio side, there is no competition when calling the function,
> > I think.
> > For protecting the competition between audio and gfx driver,
> > it seems we need use the lock from gfx side. Do you have suggested
> >  lock I can use?
> 
> To do it like this we'd need pretty much all the modeset locks, which
> to me feels a bit troublesome if the audio driver can call this at
> any point. So I suggested that we may want to add some kind of new
> audio lock to i915.

If we are using a new audio lock, I believe we still need use the
lock when gfx is doing the modeset. Only adding the new lock 
in the function i915_audio_component_sync_audio_rate() 
is not enough because gfx driver will still modify it without
locking.

I'm not sure where to add the lock in the gfx driver. 

Regards,
Libin
> 
> >
> > >
> > > > +			if (!crtc) {
> > > > +				DRM_DEBUG_KMS("%s: crtc is
> > > NULL\n", __func__);
> > > > +				continue;
> > > > +			}
> > > > +			pipe = crtc->pipe;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (pipe == INVALID_PIPE) {
> > > > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> > > port_name(port));
> > > > +		return -ENODEV;
> > > > +	}
> > > > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > > > +				  pipe_name(pipe), port_name(port));
> > > > +	mode = &crtc->config->base.adjusted_mode;
> > > > +
> > > > +	/* 2. check whether to set the N/CTS/M manually or not */
> > > > +	if (!audio_rate_need_prog(crtc, mode)) {
> > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >
> > > This stuff would need to be abstracted to work on pre-HSW too.
> >
> > Right, I need judge the platform firstly.
> >
> > >
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	n = audio_config_get_n(mode, rate);
> > > > +	if (n == 0) {
> > > > +		DRM_DEBUG_KMS("Using automatic mode for N value
> > > on port %c\n",
> > > > +					  port_name(port));
> > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > +		return 0;
> > > > +	}
> > > > +	n_low = n & 0xfff;
> > > > +	n_up = (n >> 12) & 0xff;
> > > > +
> > > > +	/* 4. set the N/CTS/M */
> > > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > +	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);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static const struct i915_audio_component_ops
> > > i915_audio_component_ops = {
> > > >  	.owner		= THIS_MODULE,
> > > >  	.get_power	= i915_audio_component_get_power,
> > > >  	.put_power	= i915_audio_component_put_power,
> > > >  	.codec_wake_override =
> > > i915_audio_component_codec_wake_override,
> > > >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > > > +	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> > > >  };
> > > >
> > > >  static int i915_audio_component_bind(struct device *i915_dev,
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > [e-mail]
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Aug. 24, 2015, 4:27 p.m. UTC | #6

On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Monday, August 24, 2015 8:53 PM
> > To: Yang, Libin
> > Cc: [e-mail]; [e-mail]; intel-
> > [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > sync_audio_rate callback
> > 
> > On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > Sent: Friday, August 21, 2015 11:14 PM
> > > > To: Yang, Libin
> > > > Cc: [e-mail]; [e-mail]; intel-
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > sync_audio_rate callback
> > > >
> > > > On Tue, Aug 18, 2015 at 02:51:52PM +0800, [e-mail]
> > > > wrote:
> > > > > From: Libin Yang <[e-mail]>
> > > > >
> > > > > HDMI audio may not work at some frequencies
> > > > > with the HW provided N/CTS.
> > > > >
> > > > > This patch sets the proper N value for the
> > > > > given audio sample rate at the impacted frequencies.
> > > > > At other frequencies, it will use the N/CTS value
> > > > > which HW provides.
> > > > >
> > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_audio.c | 119
> > > > +++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 119 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index dc32cf4..96b97be 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -68,6 +68,31 @@ static const struct {
> > > > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > > > >  };
> > > > >
> > > > > +/* HDMI N/CTS table */
> > > > > +#define TMDS_297M 297000
> > > > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > > >
> > > > I don't really like the defines.
> > >
> > > I agree it's not a good name. Do you have some suggestions?
> > 
> > I'd probably just have used raw numbers myself.
> > 
> > >
> > > >
> > > > > +static const struct {
> > > > > +	int sample_rate;
> > > > > +	int clock;
> > > > > +	int n;
> > > > > +	int cts;
> > > > > +} aud_ncts[] = {
> > > > > +	{ 44100, TMDS_296M, 4459, 234375 },
> > > > > +	{ 44100, TMDS_297M, 4704, 247500 },
> > > > > +	{ 48000, TMDS_296M, 5824, 281250 },
> > > > > +	{ 48000, TMDS_297M, 5120, 247500 },
> > > > > +	{ 32000, TMDS_296M, 5824, 421875 },
> > > > > +	{ 32000, TMDS_297M, 3072, 222750 },
> > > > > +	{ 88200, TMDS_296M, 8918, 234375 },
> > > > > +	{ 88200, TMDS_297M, 9408, 247500 },
> > > > > +	{ 96000, TMDS_296M, 11648, 281250 },
> > > > > +	{ 96000, TMDS_297M, 10240, 247500 },
> > > > > +	{ 176400, TMDS_296M, 17836, 234375 },
> > > > > +	{ 176400, TMDS_297M, 18816, 247500 },
> > > > > +	{ 44100, TMDS_296M, 23296, 281250 },
> > > > > +	{ 44100, TMDS_297M, 20480, 247500 },
> > > > > +};
> > > >
> > > > Last two should be 192 kHz. All the other values seem to match the
> > > > spec.
> > >
> > > Oh, my typo. Thanks for pointing it out.
> > >
> > > >
> > > > These do assume 8bpc, but as the spec doesn't even specify N/CTS
> > > > values for deep color modes @ 297MHz, so I suppose the
> > expectations
> > > > is
> > > > that the max TMDS clock will never be so high as to allow them.
> > > >
> > > > If/when we start using manual programming for other TMDS clock
> > > > rates
> > > > we'll need to consider 12bpc as well.
> > >
> > > These values are recommended from HDMI spec. It's not related
> > > to the bpp. Am I wrong? I'm find the value from HDMISpecification
> > > 1.4: table 7-1, table 7-2 and table 7-3.
> > 
> > There are separate tables for deep color modes in appendix D. Tables
> > D-1 through D-3 are of interest to us since we can only do the 12bpc
> > deep color mode.
> 
> Yes, we found the D tables in the spec before, and it seems a little
> Complicated. And the table 7-x seems to be more general. This is
> the reason we use table 7-x.

What's complicated? With 8bpc you use 7-x, with 12bpc you use D-x. But
as stated there are no 297MHz values in the D-x tables so the assumption
seems to be that the max TMDS clock will be ~300Mhz and so there's no
way to to get a 297Mhz pixel clock with deep color modes.

The fact that the D-x tables refer to the pixel clock instead of TMDS
clock is also a bit confusing. Seems they forgot about pixel
replication there. But I believe we should just consider the pixel
replicated pixel clock when consulting these tables.

> 
> OK. We will use D-1, D-2 and D-3 table for N/CTS.

If you're only worried about 297Mhz modes, then there should be no need
to consult the D-x tables at this time.

> 
> > 
> > >
> > > >
> > > > > +
> > > > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > > >  static u32 audio_config_hdmi_pixel_clock(struct
> > drm_display_mode
> > > > *mode)
> > > > >  {
> > > > > @@ -90,6 +115,31 @@ static u32
> > > > audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
> > > > >  	return hdmi_audio_clock[i].config;
> > > > >  }
> > > > >
> > > > > +static int audio_config_get_n(struct drm_display_mode *mode,
> > int
> > > > rate)
> > > >
> > > > mode can be const.
> > >
> > > OK. I will change it.
> > >
> > > >
> > > > > +{
> > > > > +	int i;
> > > > > +
> > > > > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > +		if ((rate == aud_ncts[i].sample_rate) &&
> > > > > +			(mode->clock == aud_ncts[i].clock)) {
> > > > > +			return aud_ncts[i].n;
> > > > > +		}
> > > > > +	}
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +/* check whether N/CTS/M need be set manually */
> > > > > +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > +					struct drm_display_mode
> > > > *mode)
> > > > > +{
> > > > > +	if (((mode->clock == TMDS_297M) ||
> > > > > +		 (mode->clock == TMDS_296M)) &&
> > > > > +		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
> > > > > +		return true;
> > > > > +	else
> > > > > +		return false;
> > > > > +}
> > > > > +
> > > > >  static bool intel_eld_uptodate(struct drm_connector *connector,
> > > > >  			       int reg_eldv, uint32_t bits_eldv,
> > > > >  			       int reg_elda, uint32_t bits_elda,
> > > > > @@ -514,12 +564,81 @@ static int
> > > > i915_audio_component_get_cdclk_freq(struct device *dev)
> > > > >  	return ret;
> > > > >  }
> > > > >
> > > > > +static int i915_audio_component_sync_audio_rate(struct device
> > > > *dev,
> > > > > +						int port, int rate)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > > > +	struct intel_encoder *intel_encoder;
> > > > > +	struct intel_digital_port *intel_dig_port;
> > > > > +	struct intel_crtc *crtc;
> > > > > +	struct drm_display_mode *mode;
> > > > > +	enum pipe pipe = -1;
> > > > > +	u32 tmp;
> > > > > +	int n_low, n_up, n;
> > > > > +
> > > > > +	/* 1. get the pipe */
> > > > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > > > +		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
> > > > > +			continue;
> > > > > +		intel_dig_port = enc_to_dig_port(&intel_encoder-
> > > > >base);
> > > > > +		if (port == intel_dig_port->port) {
> > > > > +			crtc = to_intel_crtc(intel_encoder->base.crtc);
> > > >
> > > > This sort of thing would need some locking to safely start digging
> > at
> > > > the modeset state.
> > > >
> > > > I would probably not do that, and instead add some new lock(s) for
> > this.
> > > > The modeset code would then fill out some relevant information
> > > > protected
> > > > by that same lock, and this function could then go look at it
> > without
> > > > having to worry about the rest of modeset locking/state.
> > >
> > > >From audio side, there is no competition when calling the function,
> > > I think.
> > > For protecting the competition between audio and gfx driver,
> > > it seems we need use the lock from gfx side. Do you have suggested
> > >  lock I can use?
> > 
> > To do it like this we'd need pretty much all the modeset locks, which
> > to me feels a bit troublesome if the audio driver can call this at
> > any point. So I suggested that we may want to add some kind of new
> > audio lock to i915.
> 
> If we are using a new audio lock, I believe we still need use the
> lock when gfx is doing the modeset. Only adding the new lock 
> in the function i915_audio_component_sync_audio_rate() 
> is not enough because gfx driver will still modify it without
> locking.
> 
> I'm not sure where to add the lock in the gfx driver. 

The audio enable/disable during modeset would also grab the lock, and
consult whatever information the audio driver provided. I would also
suggest renaming the .sync_audio_rate() to something more clear like
.set_sample_rate()

Anyway, I was thinking of something like this:

intel_audio_codec_enable()
{
	lock();
	.. configure n/cts
	encoder->audio.enabled = true;
	unlock();
}

intel_audio_codec_disable()
{
	lock();
	encoder->audio.enabled = false;
	unlock();
}

set_sample_rate()
{
	lock();
	encoder->audio.sample_rate = rate;
	if (encoder->audio.enabled) {
		... reconfigure n/cts
	}
	unlock();
}

Something like that should protect sample rate changes from racing
with an ongoing modeset.

> 
> Regards,
> Libin
> > 
> > >
> > > >
> > > > > +			if (!crtc) {
> > > > > +				DRM_DEBUG_KMS("%s: crtc is
> > > > NULL\n", __func__);
> > > > > +				continue;
> > > > > +			}
> > > > > +			pipe = crtc->pipe;
> > > > > +			break;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (pipe == INVALID_PIPE) {
> > > > > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> > > > port_name(port));
> > > > > +		return -ENODEV;
> > > > > +	}
> > > > > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > > > > +				  pipe_name(pipe), port_name(port));
> > > > > +	mode = &crtc->config->base.adjusted_mode;
> > > > > +
> > > > > +	/* 2. check whether to set the N/CTS/M manually or not */
> > > > > +	if (!audio_rate_need_prog(crtc, mode)) {
> > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > >
> > > > This stuff would need to be abstracted to work on pre-HSW too.
> > >
> > > Right, I need judge the platform firstly.
> > >
> > > >
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	n = audio_config_get_n(mode, rate);
> > > > > +	if (n == 0) {
> > > > > +		DRM_DEBUG_KMS("Using automatic mode for N value
> > > > on port %c\n",
> > > > > +					  port_name(port));
> > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > +		return 0;
> > > > > +	}
> > > > > +	n_low = n & 0xfff;
> > > > > +	n_up = (n >> 12) & 0xff;
> > > > > +
> > > > > +	/* 4. set the N/CTS/M */
> > > > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > +	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);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static const struct i915_audio_component_ops
> > > > i915_audio_component_ops = {
> > > > >  	.owner		= THIS_MODULE,
> > > > >  	.get_power	= i915_audio_component_get_power,
> > > > >  	.put_power	= i915_audio_component_put_power,
> > > > >  	.codec_wake_override =
> > > > i915_audio_component_codec_wake_override,
> > > > >  	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
> > > > > +	.sync_audio_rate = i915_audio_component_sync_audio_rate,
> > > > >  };
> > > > >
> > > > >  static int i915_audio_component_bind(struct device *i915_dev,
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > [e-mail]
> > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

Yang, Libin Aug. 25, 2015, 1:48 a.m. UTC | #7

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Tuesday, August 25, 2015 12:27 AM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; intel-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> sync_audio_rate callback
> 
> On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Monday, August 24, 2015 8:53 PM
> > > To: Yang, Libin
> > > Cc: [e-mail]; [e-mail]; intel-
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > sync_audio_rate callback
> > >
> > > On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > Sent: Friday, August 21, 2015 11:14 PM
> > > > > To: Yang, Libin
> > > > > Cc: [e-mail]; [e-mail]; intel-
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > > sync_audio_rate callback
> > > > >
> > > > > On Tue, Aug 18, 2015 at 02:51:52PM +0800,
> [e-mail]
> > > > > wrote:
> > > > > > From: Libin Yang <[e-mail]>
> > > > > >
> > > > > > HDMI audio may not work at some frequencies
> > > > > > with the HW provided N/CTS.
> > > > > >
> > > > > > This patch sets the proper N value for the
> > > > > > given audio sample rate at the impacted frequencies.
> > > > > > At other frequencies, it will use the N/CTS value
> > > > > > which HW provides.
> > > > > >
> > > > > > Signed-off-by: Libin Yang <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_audio.c | 119
> > > > > +++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 119 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > index dc32cf4..96b97be 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > @@ -68,6 +68,31 @@ static const struct {
> > > > > >  	{ 148500,
> AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > > > > >  };
> > > > > >
> > > > > > +/* HDMI N/CTS table */
> > > > > > +#define TMDS_297M 297000
> > > > > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > > > >
> > > > > I don't really like the defines.
> > > >
> > > > I agree it's not a good name. Do you have some suggestions?
> > >
> > > I'd probably just have used raw numbers myself.
> > >
> > > >
> > > > >
> > > > > > +static const struct {
> > > > > > +	int sample_rate;
> > > > > > +	int clock;
> > > > > > +	int n;
> > > > > > +	int cts;
> > > > > > +} aud_ncts[] = {
> > > > > > +	{ 44100, TMDS_296M, 4459, 234375 },
> > > > > > +	{ 44100, TMDS_297M, 4704, 247500 },
> > > > > > +	{ 48000, TMDS_296M, 5824, 281250 },
> > > > > > +	{ 48000, TMDS_297M, 5120, 247500 },
> > > > > > +	{ 32000, TMDS_296M, 5824, 421875 },
> > > > > > +	{ 32000, TMDS_297M, 3072, 222750 },
> > > > > > +	{ 88200, TMDS_296M, 8918, 234375 },
> > > > > > +	{ 88200, TMDS_297M, 9408, 247500 },
> > > > > > +	{ 96000, TMDS_296M, 11648, 281250 },
> > > > > > +	{ 96000, TMDS_297M, 10240, 247500 },
> > > > > > +	{ 176400, TMDS_296M, 17836, 234375 },
> > > > > > +	{ 176400, TMDS_297M, 18816, 247500 },
> > > > > > +	{ 44100, TMDS_296M, 23296, 281250 },
> > > > > > +	{ 44100, TMDS_297M, 20480, 247500 },
> > > > > > +};
> > > > >
> > > > > Last two should be 192 kHz. All the other values seem to match
> the
> > > > > spec.
> > > >
> > > > Oh, my typo. Thanks for pointing it out.
> > > >
> > > > >
> > > > > These do assume 8bpc, but as the spec doesn't even specify
> N/CTS
> > > > > values for deep color modes @ 297MHz, so I suppose the
> > > expectations
> > > > > is
> > > > > that the max TMDS clock will never be so high as to allow them.
> > > > >
> > > > > If/when we start using manual programming for other TMDS
> clock
> > > > > rates
> > > > > we'll need to consider 12bpc as well.
> > > >
> > > > These values are recommended from HDMI spec. It's not related
> > > > to the bpp. Am I wrong? I'm find the value from
> HDMISpecification
> > > > 1.4: table 7-1, table 7-2 and table 7-3.
> > >
> > > There are separate tables for deep color modes in appendix D.
> Tables
> > > D-1 through D-3 are of interest to us since we can only do the
> 12bpc
> > > deep color mode.
> >
> > Yes, we found the D tables in the spec before, and it seems a little
> > Complicated. And the table 7-x seems to be more general. This is
> > the reason we use table 7-x.
> 
> What's complicated? With 8bpc you use 7-x, with 12bpc you use D-x.
> But
> as stated there are no 297MHz values in the D-x tables so the
> assumption
> seems to be that the max TMDS clock will be ~300Mhz and so there's
> no
> way to to get a 297Mhz pixel clock with deep color modes.
> 
> The fact that the D-x tables refer to the pixel clock instead of TMDS
> clock is also a bit confusing. Seems they forgot about pixel
> replication there. But I believe we should just consider the pixel
> replicated pixel clock when consulting these tables.
> 
> >
> > OK. We will use D-1, D-2 and D-3 table for N/CTS.
> 
> If you're only worried about 297Mhz modes, then there should be no
> need
> to consult the D-x tables at this time.

OK, so I will still use the 7-x table.

> 
> >
> > >
> > > >
> > > > >
> > > > > > +
> > > > > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode
> */
> > > > > >  static u32 audio_config_hdmi_pixel_clock(struct
> > > drm_display_mode
> > > > > *mode)
> > > > > >  {
> > > > > > @@ -90,6 +115,31 @@ static u32
> > > > > audio_config_hdmi_pixel_clock(struct drm_display_mode
> *mode)
> > > > > >  	return hdmi_audio_clock[i].config;
> > > > > >  }
> > > > > >
> > > > > > +static int audio_config_get_n(struct drm_display_mode
> *mode,
> > > int
> > > > > rate)
> > > > >
> > > > > mode can be const.
> > > >
> > > > OK. I will change it.
> > > >
> > > > >
> > > > > > +{
> > > > > > +	int i;
> > > > > > +
> > > > > > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > > +		if ((rate == aud_ncts[i].sample_rate) &&
> > > > > > +			(mode->clock == aud_ncts[i].clock)) {
> > > > > > +			return aud_ncts[i].n;
> > > > > > +		}
> > > > > > +	}
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +/* check whether N/CTS/M need be set manually */
> > > > > > +static bool audio_rate_need_prog(struct intel_crtc *crtc,
> > > > > > +					struct
> drm_display_mode
> > > > > *mode)
> > > > > > +{
> > > > > > +	if (((mode->clock == TMDS_297M) ||
> > > > > > +		 (mode->clock == TMDS_296M)) &&
> > > > > > +		intel_pipe_has_type(crtc,
> INTEL_OUTPUT_HDMI))
> > > > > > +		return true;
> > > > > > +	else
> > > > > > +		return false;
> > > > > > +}
> > > > > > +
> > > > > >  static bool intel_eld_uptodate(struct drm_connector
> *connector,
> > > > > >  			       int reg_eldv, uint32_t bits_eldv,
> > > > > >  			       int reg_elda, uint32_t bits_elda,
> > > > > > @@ -514,12 +564,81 @@ static int
> > > > > i915_audio_component_get_cdclk_freq(struct device *dev)
> > > > > >  	return ret;
> > > > > >  }
> > > > > >
> > > > > > +static int i915_audio_component_sync_audio_rate(struct
> device
> > > > > *dev,
> > > > > > +						int port, int
> rate)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > > > > +	struct intel_encoder *intel_encoder;
> > > > > > +	struct intel_digital_port *intel_dig_port;
> > > > > > +	struct intel_crtc *crtc;
> > > > > > +	struct drm_display_mode *mode;
> > > > > > +	enum pipe pipe = -1;
> > > > > > +	u32 tmp;
> > > > > > +	int n_low, n_up, n;
> > > > > > +
> > > > > > +	/* 1. get the pipe */
> > > > > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > > > > +		if (intel_encoder->type !=
> INTEL_OUTPUT_HDMI)
> > > > > > +			continue;
> > > > > > +		intel_dig_port =
> enc_to_dig_port(&intel_encoder-
> > > > > >base);
> > > > > > +		if (port == intel_dig_port->port) {
> > > > > > +			crtc = to_intel_crtc(intel_encoder-
> >base.crtc);
> > > > >
> > > > > This sort of thing would need some locking to safely start
> digging
> > > at
> > > > > the modeset state.
> > > > >
> > > > > I would probably not do that, and instead add some new lock(s)
> for
> > > this.
> > > > > The modeset code would then fill out some relevant
> information
> > > > > protected
> > > > > by that same lock, and this function could then go look at it
> > > without
> > > > > having to worry about the rest of modeset locking/state.
> > > >
> > > > >From audio side, there is no competition when calling the
> function,
> > > > I think.
> > > > For protecting the competition between audio and gfx driver,
> > > > it seems we need use the lock from gfx side. Do you have
> suggested
> > > >  lock I can use?
> > >
> > > To do it like this we'd need pretty much all the modeset locks,
> which
> > > to me feels a bit troublesome if the audio driver can call this at
> > > any point. So I suggested that we may want to add some kind of
> new
> > > audio lock to i915.
> >
> > If we are using a new audio lock, I believe we still need use the
> > lock when gfx is doing the modeset. Only adding the new lock
> > in the function i915_audio_component_sync_audio_rate()
> > is not enough because gfx driver will still modify it without
> > locking.
> >
> > I'm not sure where to add the lock in the gfx driver.
> 
> The audio enable/disable during modeset would also grab the lock,
> and
> consult whatever information the audio driver provided. I would also
> suggest renaming the .sync_audio_rate() to something more clear like
> .set_sample_rate()

The sample rate in audio means the how often the audio data
is sampled. Actually this function is none of setting sample rate.
The sample rate setting is done in audio driver. What the function
does is only setting the n/cts based on the audio sample rate.

> 
> Anyway, I was thinking of something like this:
> 
> intel_audio_codec_enable()
> {
> 	lock();
> 	.. configure n/cts
> 	encoder->audio.enabled = true;
> 	unlock();
> }
> 
> intel_audio_codec_disable()
> {
> 	lock();
> 	encoder->audio.enabled = false;
> 	unlock();
> }
> 
> set_sample_rate()
> {
> 	lock();
> 	encoder->audio.sample_rate = rate;
> 	if (encoder->audio.enabled) {
> 		... reconfigure n/cts
> 	}
> 	unlock();
> }

I'm still not sure whether it is safe when gfx driver is doing the modeset.
Suppose gfx driver will handle the protection, right?

If so, I will add spin_lock_irqsave as you suggested. 

> 
> Something like that should protect sample rate changes from racing
> with an ongoing modeset.

Btw, it is not changing the sample rate. Audio sample rate will never
be changed in these functions.

Regards,
Libin
> 
> >
> > Regards,
> > Libin
> > >
> > > >
> > > > >
> > > > > > +			if (!crtc) {
> > > > > > +				DRM_DEBUG_KMS("%s: crtc is
> > > > > NULL\n", __func__);
> > > > > > +				continue;
> > > > > > +			}
> > > > > > +			pipe = crtc->pipe;
> > > > > > +			break;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	if (pipe == INVALID_PIPE) {
> > > > > > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> > > > > port_name(port));
> > > > > > +		return -ENODEV;
> > > > > > +	}
> > > > > > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > > > > > +				  pipe_name(pipe),
> port_name(port));
> > > > > > +	mode = &crtc->config->base.adjusted_mode;
> > > > > > +
> > > > > > +	/* 2. check whether to set the N/CTS/M manually or
> not */
> > > > > > +	if (!audio_rate_need_prog(crtc, mode)) {
> > > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > >
> > > > > This stuff would need to be abstracted to work on pre-HSW too.
> > > >
> > > > Right, I need judge the platform firstly.
> > > >
> > > > >
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +
> > > > > > +	n = audio_config_get_n(mode, rate);
> > > > > > +	if (n == 0) {
> > > > > > +		DRM_DEBUG_KMS("Using automatic mode for
> N value
> > > > > on port %c\n",
> > > > > > +					  port_name(port));
> > > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > +		return 0;
> > > > > > +	}
> > > > > > +	n_low = n & 0xfff;
> > > > > > +	n_up = (n >> 12) & 0xff;
> > > > > > +
> > > > > > +	/* 4. set the N/CTS/M */
> > > > > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > +	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);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static const struct i915_audio_component_ops
> > > > > i915_audio_component_ops = {
> > > > > >  	.owner		= THIS_MODULE,
> > > > > >  	.get_power	= i915_audio_component_get_power,
> > > > > >  	.put_power	= i915_audio_component_put_power,
> > > > > >  	.codec_wake_override =
> > > > > i915_audio_component_codec_wake_override,
> > > > > >  	.get_cdclk_freq	=
> i915_audio_component_get_cdclk_freq,
> > > > > > +	.sync_audio_rate =
> i915_audio_component_sync_audio_rate,
> > > > > >  };
> > > > > >
> > > > > >  static int i915_audio_component_bind(struct device
> *i915_dev,
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Intel-gfx mailing list
> > > > > > [e-mail]
> > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> --
> Ville Syrjälä
> Intel OTC

Ville Syrjälä Aug. 26, 2015, 12:27 p.m. UTC | #8

On Tue, Aug 25, 2015 at 01:48:13AM +0000, Yang, Libin wrote:
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:[e-mail]]
> > Sent: Tuesday, August 25, 2015 12:27 AM
> > To: Yang, Libin
> > Cc: [e-mail]; [e-mail]; intel-
> > [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > sync_audio_rate callback
> > 
> > On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > Sent: Monday, August 24, 2015 8:53 PM
> > > > To: Yang, Libin
> > > > Cc: [e-mail]; [e-mail]; intel-
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > sync_audio_rate callback
> > > >
> > > > On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > > Sent: Friday, August 21, 2015 11:14 PM
> > > > > > To: Yang, Libin
> > > > > > Cc: [e-mail]; [e-mail]; intel-
> > > > > > [e-mail]; [e-mail];
> > > > > > [e-mail]
> > > > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > > > sync_audio_rate callback
> > > > > >
> > > > > > On Tue, Aug 18, 2015 at 02:51:52PM +0800,
> > [e-mail]
> > > > > > wrote:
> > > > > > > +static int i915_audio_component_sync_audio_rate(struct
> > device
> > > > > > *dev,
> > > > > > > +						int port, int
> > rate)
> > > > > > > +{
> > > > > > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > > > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > > > > > +	struct intel_encoder *intel_encoder;
> > > > > > > +	struct intel_digital_port *intel_dig_port;
> > > > > > > +	struct intel_crtc *crtc;
> > > > > > > +	struct drm_display_mode *mode;
> > > > > > > +	enum pipe pipe = -1;
> > > > > > > +	u32 tmp;
> > > > > > > +	int n_low, n_up, n;
> > > > > > > +
> > > > > > > +	/* 1. get the pipe */
> > > > > > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > > > > > +		if (intel_encoder->type !=
> > INTEL_OUTPUT_HDMI)
> > > > > > > +			continue;
> > > > > > > +		intel_dig_port =
> > enc_to_dig_port(&intel_encoder-
> > > > > > >base);
> > > > > > > +		if (port == intel_dig_port->port) {
> > > > > > > +			crtc = to_intel_crtc(intel_encoder-
> > >base.crtc);
> > > > > >
> > > > > > This sort of thing would need some locking to safely start
> > digging
> > > > at
> > > > > > the modeset state.
> > > > > >
> > > > > > I would probably not do that, and instead add some new lock(s)
> > for
> > > > this.
> > > > > > The modeset code would then fill out some relevant
> > information
> > > > > > protected
> > > > > > by that same lock, and this function could then go look at it
> > > > without
> > > > > > having to worry about the rest of modeset locking/state.
> > > > >
> > > > > >From audio side, there is no competition when calling the
> > function,
> > > > > I think.
> > > > > For protecting the competition between audio and gfx driver,
> > > > > it seems we need use the lock from gfx side. Do you have
> > suggested
> > > > >  lock I can use?
> > > >
> > > > To do it like this we'd need pretty much all the modeset locks,
> > which
> > > > to me feels a bit troublesome if the audio driver can call this at
> > > > any point. So I suggested that we may want to add some kind of
> > new
> > > > audio lock to i915.
> > >
> > > If we are using a new audio lock, I believe we still need use the
> > > lock when gfx is doing the modeset. Only adding the new lock
> > > in the function i915_audio_component_sync_audio_rate()
> > > is not enough because gfx driver will still modify it without
> > > locking.
> > >
> > > I'm not sure where to add the lock in the gfx driver.
> > 
> > The audio enable/disable during modeset would also grab the lock,
> > and
> > consult whatever information the audio driver provided. I would also
> > suggest renaming the .sync_audio_rate() to something more clear like
> > .set_sample_rate()
> 
> The sample rate in audio means the how often the audio data
> is sampled. Actually this function is none of setting sample rate.

Well it's more of sample_rate_change_notify() or something like that.
But I don't mind too much what it's called. We can go with your original
name if you feel it's more clear.

> The sample rate setting is done in audio driver. What the function
> does is only setting the n/cts based on the audio sample rate.
> 
> > 
> > Anyway, I was thinking of something like this:
> > 
> > intel_audio_codec_enable()
> > {
> > 	lock();
> > 	.. configure n/cts
> > 	encoder->audio.enabled = true;
> > 	unlock();
> > }
> > 
> > intel_audio_codec_disable()
> > {
> > 	lock();
> > 	encoder->audio.enabled = false;
> > 	unlock();
> > }
> > 
> > set_sample_rate()
> > {
> > 	lock();
> > 	encoder->audio.sample_rate = rate;
> > 	if (encoder->audio.enabled) {
> > 		... reconfigure n/cts
> > 	}
> > 	unlock();
> > }
> 
> I'm still not sure whether it is safe when gfx driver is doing the modeset.
> Suppose gfx driver will handle the protection, right?

intel_audio_codec_enable/disable are called during the modeset. With my
idea as long as you're holding the lock and audio.enabled==true you
could be sure there's nothing bad happening at the same time in i915.

> 
> If so, I will add spin_lock_irqsave as you suggested. 

Could be a mutex I think.

> 
> > 
> > Something like that should protect sample rate changes from racing
> > with an ongoing modeset.
> 
> Btw, it is not changing the sample rate. Audio sample rate will never
> be changed in these functions.

The audio driver can change the sample rate at any point, at which point
it'll call into i915 to reconfigure n/cts, no? So we just need to make
sure things can't blow up if it does that while there's a modeset
happening at the same time.

> 
> Regards,
> Libin
> > 
> > >
> > > Regards,
> > > Libin
> > > >
> > > > >
> > > > > >
> > > > > > > +			if (!crtc) {
> > > > > > > +				DRM_DEBUG_KMS("%s: crtc is
> > > > > > NULL\n", __func__);
> > > > > > > +				continue;
> > > > > > > +			}
> > > > > > > +			pipe = crtc->pipe;
> > > > > > > +			break;
> > > > > > > +		}
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (pipe == INVALID_PIPE) {
> > > > > > > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> > > > > > port_name(port));
> > > > > > > +		return -ENODEV;
> > > > > > > +	}
> > > > > > > +	DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > > > > > > +				  pipe_name(pipe),
> > port_name(port));
> > > > > > > +	mode = &crtc->config->base.adjusted_mode;
> > > > > > > +
> > > > > > > +	/* 2. check whether to set the N/CTS/M manually or
> > not */
> > > > > > > +	if (!audio_rate_need_prog(crtc, mode)) {
> > > > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > >
> > > > > > This stuff would need to be abstracted to work on pre-HSW too.
> > > > >
> > > > > Right, I need judge the platform firstly.
> > > > >
> > > > > >
> > > > > > > +		return 0;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	n = audio_config_get_n(mode, rate);
> > > > > > > +	if (n == 0) {
> > > > > > > +		DRM_DEBUG_KMS("Using automatic mode for
> > N value
> > > > > > on port %c\n",
> > > > > > > +					  port_name(port));
> > > > > > > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > > > > > > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > +		return 0;
> > > > > > > +	}
> > > > > > > +	n_low = n & 0xfff;
> > > > > > > +	n_up = (n >> 12) & 0xff;
> > > > > > > +
> > > > > > > +	/* 4. set the N/CTS/M */
> > > > > > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > +	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);
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > >  static const struct i915_audio_component_ops
> > > > > > i915_audio_component_ops = {
> > > > > > >  	.owner		= THIS_MODULE,
> > > > > > >  	.get_power	= i915_audio_component_get_power,
> > > > > > >  	.put_power	= i915_audio_component_put_power,
> > > > > > >  	.codec_wake_override =
> > > > > > i915_audio_component_codec_wake_override,
> > > > > > >  	.get_cdclk_freq	=
> > i915_audio_component_get_cdclk_freq,
> > > > > > > +	.sync_audio_rate =
> > i915_audio_component_sync_audio_rate,
> > > > > > >  };
> > > > > > >
> > > > > > >  static int i915_audio_component_bind(struct device
> > *i915_dev,
> > > > > > > --
> > > > > > > 1.9.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Intel-gfx mailing list
> > > > > > > [e-mail]
> > > > > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > > > >
> > > > > > --
> > > > > > Ville Syrjälä
> > > > > > Intel OTC
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> > 
> > --
> > Ville Syrjälä
> > Intel OTC

Yang, Libin Aug. 27, 2015, 2:06 a.m. UTC | #9

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:[e-mail]]
> Sent: Wednesday, August 26, 2015 8:27 PM
> To: Yang, Libin
> Cc: [e-mail]; [e-mail]; intel-
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> sync_audio_rate callback
> 
> On Tue, Aug 25, 2015 at 01:48:13AM +0000, Yang, Libin wrote:
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:[e-mail]]
> > > Sent: Tuesday, August 25, 2015 12:27 AM
> > > To: Yang, Libin
> > > Cc: [e-mail]; [e-mail]; intel-
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > sync_audio_rate callback
> > >
> > > On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > Sent: Monday, August 24, 2015 8:53 PM
> > > > > To: Yang, Libin
> > > > > Cc: [e-mail]; [e-mail]; intel-
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > > sync_audio_rate callback
> > > > >
> > > > > On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Ville Syrjälä [mailto:[e-mail]]
> > > > > > > Sent: Friday, August 21, 2015 11:14 PM
> > > > > > > To: Yang, Libin
> > > > > > > Cc: [e-mail]; [e-mail]; intel-
> > > > > > > [e-mail]; [e-mail];
> > > > > > > [e-mail]
> > > > > > > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement
> > > > > > > sync_audio_rate callback
> > > > > > >
> > > > > > > On Tue, Aug 18, 2015 at 02:51:52PM +0800,
> > > [e-mail]
> > > > > > > wrote:
> > > > > > > > +static int i915_audio_component_sync_audio_rate(struct
> > > device
> > > > > > > *dev,
> > > > > > > > +						int port, int
> > > rate)
> > > > > > > > +{
> > > > > > > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > > > > > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > > > > > > +	struct intel_encoder *intel_encoder;
> > > > > > > > +	struct intel_digital_port *intel_dig_port;
> > > > > > > > +	struct intel_crtc *crtc;
> > > > > > > > +	struct drm_display_mode *mode;
> > > > > > > > +	enum pipe pipe = -1;
> > > > > > > > +	u32 tmp;
> > > > > > > > +	int n_low, n_up, n;
> > > > > > > > +
> > > > > > > > +	/* 1. get the pipe */
> > > > > > > > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > > > > > > > +		if (intel_encoder->type !=
> > > INTEL_OUTPUT_HDMI)
> > > > > > > > +			continue;
> > > > > > > > +		intel_dig_port =
> > > enc_to_dig_port(&intel_encoder-
> > > > > > > >base);
> > > > > > > > +		if (port == intel_dig_port->port) {
> > > > > > > > +			crtc = to_intel_crtc(intel_encoder-
> > > >base.crtc);
> > > > > > >
> > > > > > > This sort of thing would need some locking to safely start
> > > digging
> > > > > at
> > > > > > > the modeset state.
> > > > > > >
> > > > > > > I would probably not do that, and instead add some new
> lock(s)
> > > for
> > > > > this.
> > > > > > > The modeset code would then fill out some relevant
> > > information
> > > > > > > protected
> > > > > > > by that same lock, and this function could then go look at it
> > > > > without
> > > > > > > having to worry about the rest of modeset locking/state.
> > > > > >
> > > > > > >From audio side, there is no competition when calling the
> > > function,
> > > > > > I think.
> > > > > > For protecting the competition between audio and gfx driver,
> > > > > > it seems we need use the lock from gfx side. Do you have
> > > suggested
> > > > > >  lock I can use?
> > > > >
> > > > > To do it like this we'd need pretty much all the modeset locks,
> > > which
> > > > > to me feels a bit troublesome if the audio driver can call this at
> > > > > any point. So I suggested that we may want to add some kind
> of
> > > new
> > > > > audio lock to i915.
> > > >
> > > > If we are using a new audio lock, I believe we still need use the
> > > > lock when gfx is doing the modeset. Only adding the new lock
> > > > in the function i915_audio_component_sync_audio_rate()
> > > > is not enough because gfx driver will still modify it without
> > > > locking.
> > > >
> > > > I'm not sure where to add the lock in the gfx driver.
> > >
> > > The audio enable/disable during modeset would also grab the lock,
> > > and
> > > consult whatever information the audio driver provided. I would
> also
> > > suggest renaming the .sync_audio_rate() to something more clear
> like
> > > .set_sample_rate()
> >
> > The sample rate in audio means the how often the audio data
> > is sampled. Actually this function is none of setting sample rate.
> 
> Well it's more of sample_rate_change_notify() or something like that.
> But I don't mind too much what it's called. We can go with your
> original
> name if you feel it's more clear.

OK, I will keep the original name as it will be more clear for 
audio driver developer.

> 
> > The sample rate setting is done in audio driver. What the function
> > does is only setting the n/cts based on the audio sample rate.
> >
> > >
> > > Anyway, I was thinking of something like this:
> > >
> > > intel_audio_codec_enable()
> > > {
> > > 	lock();
> > > 	.. configure n/cts
> > > 	encoder->audio.enabled = true;
> > > 	unlock();
> > > }
> > >
> > > intel_audio_codec_disable()
> > > {
> > > 	lock();
> > > 	encoder->audio.enabled = false;
> > > 	unlock();
> > > }
> > >
> > > set_sample_rate()
> > > {
> > > 	lock();
> > > 	encoder->audio.sample_rate = rate;
> > > 	if (encoder->audio.enabled) {
> > > 		... reconfigure n/cts
> > > 	}
> > > 	unlock();
> > > }
> >
> > I'm still not sure whether it is safe when gfx driver is doing the
> modeset.
> > Suppose gfx driver will handle the protection, right?
> 
> intel_audio_codec_enable/disable are called during the modeset. With
> my
> idea as long as you're holding the lock and audio.enabled==true you
> could be sure there's nothing bad happening at the same time in i915.
> 
> >
> > If so, I will add spin_lock_irqsave as you suggested.
> 
> Could be a mutex I think.

OK, mutex is OK for audio driver, too.

> 
> >
> > >
> > > Something like that should protect sample rate changes from
> racing
> > > with an ongoing modeset.
> >
> > Btw, it is not changing the sample rate. Audio sample rate will never
> > be changed in these functions.
> 
> The audio driver can change the sample rate at any point, at which
> point
> it'll call into i915 to reconfigure n/cts, no? So we just need to make
> sure things can't blow up if it does that while there's a modeset
> happening at the same time.

The function is called before playing audio.


Regards,
Libin

Patch

diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -68,6 +68,31 @@  static const struct {
 	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
 };
 
+/* HDMI N/CTS table */
+#define TMDS_297M 297000
+#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
+static const struct {
+	int sample_rate;
+	int clock;
+	int n;
+	int cts;
+} aud_ncts[] = {
+	{ 44100, TMDS_296M, 4459, 234375 },
+	{ 44100, TMDS_297M, 4704, 247500 },
+	{ 48000, TMDS_296M, 5824, 281250 },
+	{ 48000, TMDS_297M, 5120, 247500 },
+	{ 32000, TMDS_296M, 5824, 421875 },
+	{ 32000, TMDS_297M, 3072, 222750 },
+	{ 88200, TMDS_296M, 8918, 234375 },
+	{ 88200, TMDS_297M, 9408, 247500 },
+	{ 96000, TMDS_296M, 11648, 281250 },
+	{ 96000, TMDS_297M, 10240, 247500 },
+	{ 176400, TMDS_296M, 17836, 234375 },
+	{ 176400, TMDS_297M, 18816, 247500 },
+	{ 44100, TMDS_296M, 23296, 281250 },
+	{ 44100, TMDS_297M, 20480, 247500 },
+};
+
 /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
 static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
 {
@@ -90,6 +115,31 @@  static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
 	return hdmi_audio_clock[i].config;
 }
 
+static int audio_config_get_n(struct drm_display_mode *mode, int rate)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
+		if ((rate == aud_ncts[i].sample_rate) &&
+			(mode->clock == aud_ncts[i].clock)) {
+			return aud_ncts[i].n;
+		}
+	}
+	return 0;
+}
+
+/* check whether N/CTS/M need be set manually */
+static bool audio_rate_need_prog(struct intel_crtc *crtc,
+					struct drm_display_mode *mode)
+{
+	if (((mode->clock == TMDS_297M) ||
+		 (mode->clock == TMDS_296M)) &&
+		intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))
+		return true;
+	else
+		return false;
+}
+
 static bool intel_eld_uptodate(struct drm_connector *connector,
 			       int reg_eldv, uint32_t bits_eldv,
 			       int reg_elda, uint32_t bits_elda,
@@ -514,12 +564,81 @@  static int i915_audio_component_get_cdclk_freq(struct device *dev)
 	return ret;
 }
 
+static int i915_audio_component_sync_audio_rate(struct device *dev,
+						int port, int rate)
+{
+	struct drm_i915_private *dev_priv = dev_to_i915(dev);
+	struct drm_device *drm_dev = dev_priv->dev;
+	struct intel_encoder *intel_encoder;
+	struct intel_digital_port *intel_dig_port;
+	struct intel_crtc *crtc;
+	struct drm_display_mode *mode;
+	enum pipe pipe = -1;
+	u32 tmp;
+	int n_low, n_up, n;
+
+	/* 1. get the pipe */
+	for_each_intel_encoder(drm_dev, intel_encoder) {
+		if (intel_encoder->type != INTEL_OUTPUT_HDMI)
+			continue;
+		intel_dig_port = enc_to_dig_port(&intel_encoder->base);
+		if (port == intel_dig_port->port) {
+			crtc = to_intel_crtc(intel_encoder->base.crtc);
+			if (!crtc) {
+				DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);
+				continue;
+			}
+			pipe = crtc->pipe;
+			break;
+		}
+	}
+
+	if (pipe == INVALID_PIPE) {
+		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
+		return -ENODEV;
+	}
+	DRM_DEBUG_KMS("pipe %c connects port %c\n",
+				  pipe_name(pipe), port_name(port));
+	mode = &crtc->config->base.adjusted_mode;
+
+	/* 2. check whether to set the N/CTS/M manually or not */
+	if (!audio_rate_need_prog(crtc, mode)) {
+		tmp = I915_READ(HSW_AUD_CFG(pipe));
+		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+		return 0;
+	}
+
+	n = audio_config_get_n(mode, rate);
+	if (n == 0) {
+		DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
+					  port_name(port));
+		tmp = I915_READ(HSW_AUD_CFG(pipe));
+		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+		return 0;
+	}
+	n_low = n & 0xfff;
+	n_up = (n >> 12) & 0xff;
+
+	/* 4. set the N/CTS/M */
+	tmp = I915_READ(HSW_AUD_CFG(pipe));
+	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);
+
+	return 0;
+}
+
 static const struct i915_audio_component_ops i915_audio_component_ops = {
 	.owner		= THIS_MODULE,
 	.get_power	= i915_audio_component_get_power,
 	.put_power	= i915_audio_component_put_power,
 	.codec_wake_override = i915_audio_component_codec_wake_override,
 	.get_cdclk_freq	= i915_audio_component_get_cdclk_freq,
+	.sync_audio_rate = i915_audio_component_sync_audio_rate,
 };
 
 static int i915_audio_component_bind(struct device *i915_dev,