[v6,2/4] drm/i915: implement sync_audio_rate callback
diff mbox

Message ID 1441174301-144177-2-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>

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 <libin.yang@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c    |   1 +
 drivers/gpu/drm/i915/i915_drv.h    |   5 ++
 drivers/gpu/drm/i915/intel_audio.c | 138 +++++++++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+)

Comments

Jani Nikula Sept. 2, 2015, 7:52 a.m. UTC | #1
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> 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 <libin.yang@intel.com>

Okay, so there are a number of questions and comments this patch still
raises. Please find them inline.

*However*, we should really get this finally moving forward, and I don't
think the issues are or need to be blockers, so this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

I think we (as in i915 folks) can clean the rest of the bikeshedding up,
and I don't think we should be NAKing this ad infinitum to teach you to
become an i915 developer... unless you want to be one. ;)

> ---
>  drivers/gpu/drm/i915/i915_dma.c    |   1 +
>  drivers/gpu/drm/i915/i915_drv.h    |   5 ++
>  drivers/gpu/drm/i915/intel_audio.c | 138 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 144 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 097d4ba..8ffb5dc 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	mutex_init(&dev_priv->sb_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
>  	mutex_init(&dev_priv->csr_lock);
> +	mutex_init(&dev_priv->av_mutex);
>  
>  	intel_pm_setup(dev);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 05ffd5a..2c6b76f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1882,6 +1882,11 @@ struct drm_i915_private {
>  
>  	/* hda/i915 audio component */
>  	bool audio_component_registered;
> +	/**
> +	 * av_mutex - mutex for audio/video sync
> +	 *
> +	 */
> +	struct mutex av_mutex;
>  
>  	uint32_t hw_context_size;
>  	struct list_head context_list;
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index dc32cf4..a021720 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 },
> +	{ 192000, TMDS_296M, 23296, 281250 },
> +	{ 192000, TMDS_297M, 20480, 247500 },
> +};

Okay, I admit, I did not look these up in the spec. *blush*. I'm sure
Ville has/will. ;)

> +
>  /* 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(const 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,
> @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder)
>  
>  	DRM_DEBUG_KMS("Disable audio codec on pipe %c\n", pipe_name(pipe));
>  
> +	mutex_lock(&dev_priv->av_mutex);
> +

Bikeshed. We should think about moving the locking to
intel_audio_codec_enable and intel_audio_codec_disable for
consistency. We'll be adding new platform hooks eventually anyway, no
need to duplicate the locking everywhere.

>  	/* Disable timestamps */
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder)
>  	tmp &= ~AUDIO_ELD_VALID(pipe);
>  	tmp &= ~AUDIO_OUTPUT_ENABLE(pipe);
>  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> +
> +	mutex_unlock(&dev_priv->av_mutex);
>  }
>  
>  static void hsw_audio_codec_enable(struct drm_connector *connector,
> @@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
>  		      pipe_name(pipe), drm_eld_size(eld));
>  
> +	mutex_lock(&dev_priv->av_mutex);
> +
>  	/* Enable audio presence detect, invalidate ELD */
>  	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>  	tmp |= AUDIO_OUTPUT_ENABLE(pipe);
> @@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(mode);
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> +
> +	mutex_unlock(&dev_priv->av_mutex);
>  }
>  
>  static void ilk_audio_codec_disable(struct intel_encoder *encoder)
> @@ -514,12 +572,92 @@ 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;

Nitpick. Initialize to INVALID_PIPE.

> +	u32 tmp;
> +	int n_low, n_up, n;
> +
> +	/* HSW, BDW SKL need this fix */
> +	if (!IS_SKYLAKE(dev_priv) &&
> +		!IS_BROADWELL(dev_priv) &&
> +		!IS_HASWELL(dev_priv))
> +		return 0;

Nitpick. We should elaborate that a bit more. Is it basically that only
the above platforms support HDMI mode and audio sample rate combinations
that the automatic mode in hardware can't support? In other words, would
audio_rate_need_prog() always return false on other platforms than
hsw/bdw/skl? And if so, why do we have this check? Perhaps we should
take care to make audio_rate_need_prog() do the right thing, and do that
as the first thing here.

> +
> +	mutex_lock(&dev_priv->av_mutex);
> +	/* 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__);

DRM_DEBUG_KMS includes __func__ already.

> +				continue;
> +			}
> +			pipe = crtc->pipe;
> +			break;
> +		}
> +	}
> +
> +	if (pipe == INVALID_PIPE) {
> +		DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));
> +		mutex_unlock(&dev_priv->av_mutex);
> +		return -ENODEV;

Nitpick. I am not fond of early returns when cleanup like mutex unlock
is required. The function should have goto labels at the end with
cleanup. (OTOH if we move locking to higher level this problem goes
away.)

> +	}
> +	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);
> +		mutex_unlock(&dev_priv->av_mutex);
> +		return 0;

Nitpick. Same here, you could have an auto: label at the end with the
auto programming...

> +	}
> +
> +	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);
> +		mutex_unlock(&dev_priv->av_mutex);
> +		return 0;

...especially because this bit is duplicated for no good reason.

Fallback to auto mode if the mode isn't in the list is, I think, a good
thing. Maybe it would deserve a DRM_ERROR, because clearly that
shouldn't happen.

But it's actually the !audio_rate_need_prog case that makes me wonder. I
think we've confirmed with the silicon folks that we can change N on the
fly... but I'm actually not sure if we've confirmed that we can switch
between automatic and manual modes on the fly. I think I've said it
before, but I think I'd prefer it if we always used the manual mode
anyway to reduce the complexity and have only one code path to debug,
and that would also cover the somewhat of a corner case (but still real
case) of switching between automatic/manual modes on the fly. And of
course, this conflicts with what I said about the platform support check
above, so needs thought.

> +	}
> +	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);
> +
> +	mutex_unlock(&dev_priv->av_mutex);
> +	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
>
Yang, Libin Sept. 2, 2015, 8:24 a.m. UTC | #2
Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Wednesday, September 02, 2015 3:52 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 2/4] drm/i915: implement sync_audio_rate
> callback
> 
> On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > 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 <libin.yang@intel.com>
> 
> Okay, so there are a number of questions and comments this patch still
> raises. Please find them inline.
> 
> *However*, we should really get this finally moving forward, and I
> don't
> think the issues are or need to be blockers, so this is
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> I think we (as in i915 folks) can clean the rest of the bikeshedding up,
> and I don't think we should be NAKing this ad infinitum to teach you to
> become an i915 developer... unless you want to be one. ;)
> 
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c    |   1 +
> >  drivers/gpu/drm/i915/i915_drv.h    |   5 ++
> >  drivers/gpu/drm/i915/intel_audio.c | 138
> +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 144 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c
> > index 097d4ba..8ffb5dc 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev,
> unsigned long flags)
> >  	mutex_init(&dev_priv->sb_lock);
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  	mutex_init(&dev_priv->csr_lock);
> > +	mutex_init(&dev_priv->av_mutex);
> >
> >  	intel_pm_setup(dev);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> > index 05ffd5a..2c6b76f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1882,6 +1882,11 @@ struct drm_i915_private {
> >
> >  	/* hda/i915 audio component */
> >  	bool audio_component_registered;
> > +	/**
> > +	 * av_mutex - mutex for audio/video sync
> > +	 *
> > +	 */
> > +	struct mutex av_mutex;
> >
> >  	uint32_t hw_context_size;
> >  	struct list_head context_list;
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index dc32cf4..a021720 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 },
> > +	{ 192000, TMDS_296M, 23296, 281250 },
> > +	{ 192000, TMDS_297M, 20480, 247500 },
> > +};
> 
> Okay, I admit, I did not look these up in the spec. *blush*. I'm sure
> Ville has/will. ;)
> 
> > +
> >  /* 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(const 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,
> > @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct
> intel_encoder *encoder)
> >
> >  	DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
> pipe_name(pipe));
> >
> > +	mutex_lock(&dev_priv->av_mutex);
> > +
> 
> Bikeshed. We should think about moving the locking to
> intel_audio_codec_enable and intel_audio_codec_disable for
> consistency. We'll be adding new platform hooks eventually anyway,
> no
> need to duplicate the locking everywhere.

Yes, at first, I want to add the mutex in intel_audio_codec_enable,
but after a second thought, we don't need it for ilk_xxx and g4x_xxx,
so I moved the lock in the hsw specific function for efficiency. 

I will move it to intel_audio_codec_enable in next version.

> 
> >  	/* Disable timestamps */
> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct
> intel_encoder *encoder)
> >  	tmp &= ~AUDIO_ELD_VALID(pipe);
> >  	tmp &= ~AUDIO_OUTPUT_ENABLE(pipe);
> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > +
> > +	mutex_unlock(&dev_priv->av_mutex);
> >  }
> >
> >  static void hsw_audio_codec_enable(struct drm_connector
> *connector,
> > @@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> ELD\n",
> >  		      pipe_name(pipe), drm_eld_size(eld));
> >
> > +	mutex_lock(&dev_priv->av_mutex);
> > +
> >  	/* Enable audio presence detect, invalidate ELD */
> >  	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> >  	tmp |= AUDIO_OUTPUT_ENABLE(pipe);
> > @@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > +
> > +	mutex_unlock(&dev_priv->av_mutex);
> >  }
> >
> >  static void ilk_audio_codec_disable(struct intel_encoder *encoder)
> > @@ -514,12 +572,92 @@ 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;
> 
> Nitpick. Initialize to INVALID_PIPE.

Oh, yes, I will change it.

> 
> > +	u32 tmp;
> > +	int n_low, n_up, n;
> > +
> > +	/* HSW, BDW SKL need this fix */
> > +	if (!IS_SKYLAKE(dev_priv) &&
> > +		!IS_BROADWELL(dev_priv) &&
> > +		!IS_HASWELL(dev_priv))
> > +		return 0;
> 
> Nitpick. We should elaborate that a bit more. Is it basically that only
> the above platforms support HDMI mode and audio sample rate
> combinations
> that the automatic mode in hardware can't support? In other words,
> would
> audio_rate_need_prog() always return false on other platforms than
> hsw/bdw/skl? And if so, why do we have this check? Perhaps we
> should
> take care to make audio_rate_need_prog() do the right thing, and do
> that
> as the first thing here.

I add this judgement because the platforms are filed in our
internal document. Besides, the next gen may have such
issue and may not (the doc said MAY have such issue).
For easily extension, I add such judgement.

audio_rate_need_prog() means the specified clock need
such setting. But I'm not sure whether other old platforms
support such clock. If yes, the audio_rate_need_prog()
may be not enough. Although I think doing the setting
should be OK even on the platforms not listed in the doc.
But I have no platform to do the test, so I didn't include
other platform this time.

> 
> > +
> > +	mutex_lock(&dev_priv->av_mutex);
> > +	/* 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__);
> 
> DRM_DEBUG_KMS includes __func__ already.

I will change it in next version. Thanks. 
> 
> > +				continue;
> > +			}
> > +			pipe = crtc->pipe;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (pipe == INVALID_PIPE) {
> > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> port_name(port));
> > +		mutex_unlock(&dev_priv->av_mutex);
> > +		return -ENODEV;
> 
> Nitpick. I am not fond of early returns when cleanup like mutex unlock
> is required. The function should have goto labels at the end with
> cleanup. (OTOH if we move locking to higher level this problem goes
> away.)

OK, so it seems to move the lock in upper function is an easy
way. I will move the lock in the upper function.

> 
> > +	}
> > +	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);
> > +		mutex_unlock(&dev_priv->av_mutex);
> > +		return 0;
> 
> Nitpick. Same here, you could have an auto: label at the end with the
> auto programming...
> 
> > +	}
> > +
> > +	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);
> > +		mutex_unlock(&dev_priv->av_mutex);
> > +		return 0;
> 
> ...especially because this bit is duplicated for no good reason.
> 
> Fallback to auto mode if the mode isn't in the list is, I think, a good
> thing. Maybe it would deserve a DRM_ERROR, because clearly that
> shouldn't happen.

OK, I will add auto label for it. And I will use DRM_ERROR in next
version.

> 
> But it's actually the !audio_rate_need_prog case that makes me
> wonder. I
> think we've confirmed with the silicon folks that we can change N on
> the
> fly... but I'm actually not sure if we've confirmed that we can switch
> between automatic and manual modes on the fly. I think I've said it
> before, but I think I'd prefer it if we always used the manual mode
> anyway to reduce the complexity and have only one code path to
> debug,
> and that would also cover the somewhat of a corner case (but still real
> case) of switching between automatic/manual modes on the fly. And
> of
> course, this conflicts with what I said about the platform support check
> above, so needs thought.

I will check with silicon team. Based on our stress test, it seems
to be OK change mode on the fly.

We don't set for all the frequencies because there are so many
frequencies and platforms to do the stress test. We may need
a lot time to do the test. If the silicon team says we can't
change mode on the fly, I will change it in next version. If silicon
team says we can do it, I will use the HW auto mode for other 
frequencies. What do you think?

Regards,
Libin

> 
> > +	}
> > +	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);
> > +
> > +	mutex_unlock(&dev_priv->av_mutex);
> > +	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
> >
> 
> --
> Jani Nikula, Intel Open Source Technology Center
Jani Nikula Sept. 2, 2015, 8:42 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 3:52 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 2/4] drm/i915: implement sync_audio_rate
>> callback
>> 
>> On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
>> > From: Libin Yang <libin.yang@intel.com>
>> >
>> > 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 <libin.yang@intel.com>
>> 
>> Okay, so there are a number of questions and comments this patch still
>> raises. Please find them inline.
>> 
>> *However*, we should really get this finally moving forward, and I
>> don't
>> think the issues are or need to be blockers, so this is
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> I think we (as in i915 folks) can clean the rest of the bikeshedding up,
>> and I don't think we should be NAKing this ad infinitum to teach you to
>> become an i915 developer... unless you want to be one. ;)

Libin, I suggest taking that R-b now instead of writing a new version of
this patch if you want to get things upstreamed!

Please focus on patch 4.

BR,
Jani.


>> 
>> > ---
>> >  drivers/gpu/drm/i915/i915_dma.c    |   1 +
>> >  drivers/gpu/drm/i915/i915_drv.h    |   5 ++
>> >  drivers/gpu/drm/i915/intel_audio.c | 138
>> +++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 144 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
>> b/drivers/gpu/drm/i915/i915_dma.c
>> > index 097d4ba..8ffb5dc 100644
>> > --- a/drivers/gpu/drm/i915/i915_dma.c
>> > +++ b/drivers/gpu/drm/i915/i915_dma.c
>> > @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev,
>> unsigned long flags)
>> >  	mutex_init(&dev_priv->sb_lock);
>> >  	mutex_init(&dev_priv->modeset_restore_lock);
>> >  	mutex_init(&dev_priv->csr_lock);
>> > +	mutex_init(&dev_priv->av_mutex);
>> >
>> >  	intel_pm_setup(dev);
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> > index 05ffd5a..2c6b76f 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -1882,6 +1882,11 @@ struct drm_i915_private {
>> >
>> >  	/* hda/i915 audio component */
>> >  	bool audio_component_registered;
>> > +	/**
>> > +	 * av_mutex - mutex for audio/video sync
>> > +	 *
>> > +	 */
>> > +	struct mutex av_mutex;
>> >
>> >  	uint32_t hw_context_size;
>> >  	struct list_head context_list;
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> > index dc32cf4..a021720 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 },
>> > +	{ 192000, TMDS_296M, 23296, 281250 },
>> > +	{ 192000, TMDS_297M, 20480, 247500 },
>> > +};
>> 
>> Okay, I admit, I did not look these up in the spec. *blush*. I'm sure
>> Ville has/will. ;)
>> 
>> > +
>> >  /* 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(const 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,
>> > @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct
>> intel_encoder *encoder)
>> >
>> >  	DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
>> pipe_name(pipe));
>> >
>> > +	mutex_lock(&dev_priv->av_mutex);
>> > +
>> 
>> Bikeshed. We should think about moving the locking to
>> intel_audio_codec_enable and intel_audio_codec_disable for
>> consistency. We'll be adding new platform hooks eventually anyway,
>> no
>> need to duplicate the locking everywhere.
>
> Yes, at first, I want to add the mutex in intel_audio_codec_enable,
> but after a second thought, we don't need it for ilk_xxx and g4x_xxx,
> so I moved the lock in the hsw specific function for efficiency. 
>
> I will move it to intel_audio_codec_enable in next version.
>
>> 
>> >  	/* Disable timestamps */
>> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> > @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct
>> intel_encoder *encoder)
>> >  	tmp &= ~AUDIO_ELD_VALID(pipe);
>> >  	tmp &= ~AUDIO_OUTPUT_ENABLE(pipe);
>> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> > +
>> > +	mutex_unlock(&dev_priv->av_mutex);
>> >  }
>> >
>> >  static void hsw_audio_codec_enable(struct drm_connector
>> *connector,
>> > @@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
>> ELD\n",
>> >  		      pipe_name(pipe), drm_eld_size(eld));
>> >
>> > +	mutex_lock(&dev_priv->av_mutex);
>> > +
>> >  	/* Enable audio presence detect, invalidate ELD */
>> >  	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>> >  	tmp |= AUDIO_OUTPUT_ENABLE(pipe);
>> > @@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  	else
>> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> > +
>> > +	mutex_unlock(&dev_priv->av_mutex);
>> >  }
>> >
>> >  static void ilk_audio_codec_disable(struct intel_encoder *encoder)
>> > @@ -514,12 +572,92 @@ 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;
>> 
>> Nitpick. Initialize to INVALID_PIPE.
>
> Oh, yes, I will change it.
>
>> 
>> > +	u32 tmp;
>> > +	int n_low, n_up, n;
>> > +
>> > +	/* HSW, BDW SKL need this fix */
>> > +	if (!IS_SKYLAKE(dev_priv) &&
>> > +		!IS_BROADWELL(dev_priv) &&
>> > +		!IS_HASWELL(dev_priv))
>> > +		return 0;
>> 
>> Nitpick. We should elaborate that a bit more. Is it basically that only
>> the above platforms support HDMI mode and audio sample rate
>> combinations
>> that the automatic mode in hardware can't support? In other words,
>> would
>> audio_rate_need_prog() always return false on other platforms than
>> hsw/bdw/skl? And if so, why do we have this check? Perhaps we
>> should
>> take care to make audio_rate_need_prog() do the right thing, and do
>> that
>> as the first thing here.
>
> I add this judgement because the platforms are filed in our
> internal document. Besides, the next gen may have such
> issue and may not (the doc said MAY have such issue).
> For easily extension, I add such judgement.
>
> audio_rate_need_prog() means the specified clock need
> such setting. But I'm not sure whether other old platforms
> support such clock. If yes, the audio_rate_need_prog()
> may be not enough. Although I think doing the setting
> should be OK even on the platforms not listed in the doc.
> But I have no platform to do the test, so I didn't include
> other platform this time.
>
>> 
>> > +
>> > +	mutex_lock(&dev_priv->av_mutex);
>> > +	/* 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__);
>> 
>> DRM_DEBUG_KMS includes __func__ already.
>
> I will change it in next version. Thanks. 
>> 
>> > +				continue;
>> > +			}
>> > +			pipe = crtc->pipe;
>> > +			break;
>> > +		}
>> > +	}
>> > +
>> > +	if (pipe == INVALID_PIPE) {
>> > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
>> port_name(port));
>> > +		mutex_unlock(&dev_priv->av_mutex);
>> > +		return -ENODEV;
>> 
>> Nitpick. I am not fond of early returns when cleanup like mutex unlock
>> is required. The function should have goto labels at the end with
>> cleanup. (OTOH if we move locking to higher level this problem goes
>> away.)
>
> OK, so it seems to move the lock in upper function is an easy
> way. I will move the lock in the upper function.
>
>> 
>> > +	}
>> > +	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);
>> > +		mutex_unlock(&dev_priv->av_mutex);
>> > +		return 0;
>> 
>> Nitpick. Same here, you could have an auto: label at the end with the
>> auto programming...
>> 
>> > +	}
>> > +
>> > +	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);
>> > +		mutex_unlock(&dev_priv->av_mutex);
>> > +		return 0;
>> 
>> ...especially because this bit is duplicated for no good reason.
>> 
>> Fallback to auto mode if the mode isn't in the list is, I think, a good
>> thing. Maybe it would deserve a DRM_ERROR, because clearly that
>> shouldn't happen.
>
> OK, I will add auto label for it. And I will use DRM_ERROR in next
> version.
>
>> 
>> But it's actually the !audio_rate_need_prog case that makes me
>> wonder. I
>> think we've confirmed with the silicon folks that we can change N on
>> the
>> fly... but I'm actually not sure if we've confirmed that we can switch
>> between automatic and manual modes on the fly. I think I've said it
>> before, but I think I'd prefer it if we always used the manual mode
>> anyway to reduce the complexity and have only one code path to
>> debug,
>> and that would also cover the somewhat of a corner case (but still real
>> case) of switching between automatic/manual modes on the fly. And
>> of
>> course, this conflicts with what I said about the platform support check
>> above, so needs thought.
>
> I will check with silicon team. Based on our stress test, it seems
> to be OK change mode on the fly.
>
> We don't set for all the frequencies because there are so many
> frequencies and platforms to do the stress test. We may need
> a lot time to do the test. If the silicon team says we can't
> change mode on the fly, I will change it in next version. If silicon
> team says we can do it, I will use the HW auto mode for other 
> frequencies. What do you think?
>
> Regards,
> Libin
>
>> 
>> > +	}
>> > +	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);
>> > +
>> > +	mutex_unlock(&dev_priv->av_mutex);
>> > +	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
>> >
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center
Yang, Libin Sept. 2, 2015, 8:47 a.m. UTC | #4
Hi Jani,



> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Wednesday, September 02, 2015 4:42 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
> Subject: RE: [PATCH v6 2/4] drm/i915: implement sync_audio_rate
> callback
> 
> 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 3:52 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 2/4] drm/i915: implement sync_audio_rate
> >> callback
> >>
> >> On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
> >> > From: Libin Yang <libin.yang@intel.com>
> >> >
> >> > 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 <libin.yang@intel.com>
> >>
> >> Okay, so there are a number of questions and comments this patch
> still
> >> raises. Please find them inline.
> >>
> >> *However*, we should really get this finally moving forward, and I
> >> don't
> >> think the issues are or need to be blockers, so this is
> >>
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> I think we (as in i915 folks) can clean the rest of the bikeshedding up,
> >> and I don't think we should be NAKing this ad infinitum to teach you
> to
> >> become an i915 developer... unless you want to be one. ;)
> 
> Libin, I suggest taking that R-b now instead of writing a new version of
> this patch if you want to get things upstreamed!
> 
> Please focus on patch 4.

Oh, I didn't realize you mean this. Thanks. :) 

Regards,
Libin
> 
> BR,
> Jani.
> 
> 
> >>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_dma.c    |   1 +
> >> >  drivers/gpu/drm/i915/i915_drv.h    |   5 ++
> >> >  drivers/gpu/drm/i915/intel_audio.c | 138
> >> +++++++++++++++++++++++++++++++++++++
> >> >  3 files changed, 144 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> >> b/drivers/gpu/drm/i915/i915_dma.c
> >> > index 097d4ba..8ffb5dc 100644
> >> > --- a/drivers/gpu/drm/i915/i915_dma.c
> >> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> >> > @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device
> *dev,
> >> unsigned long flags)
> >> >  	mutex_init(&dev_priv->sb_lock);
> >> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >> >  	mutex_init(&dev_priv->csr_lock);
> >> > +	mutex_init(&dev_priv->av_mutex);
> >> >
> >> >  	intel_pm_setup(dev);
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> b/drivers/gpu/drm/i915/i915_drv.h
> >> > index 05ffd5a..2c6b76f 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -1882,6 +1882,11 @@ struct drm_i915_private {
> >> >
> >> >  	/* hda/i915 audio component */
> >> >  	bool audio_component_registered;
> >> > +	/**
> >> > +	 * av_mutex - mutex for audio/video sync
> >> > +	 *
> >> > +	 */
> >> > +	struct mutex av_mutex;
> >> >
> >> >  	uint32_t hw_context_size;
> >> >  	struct list_head context_list;
> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> > index dc32cf4..a021720 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 },
> >> > +	{ 192000, TMDS_296M, 23296, 281250 },
> >> > +	{ 192000, TMDS_297M, 20480, 247500 },
> >> > +};
> >>
> >> Okay, I admit, I did not look these up in the spec. *blush*. I'm sure
> >> Ville has/will. ;)
> >>
> >> > +
> >> >  /* 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(const 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,
> >> > @@ -184,6 +234,8 @@ static void
> hsw_audio_codec_disable(struct
> >> intel_encoder *encoder)
> >> >
> >> >  	DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
> >> pipe_name(pipe));
> >> >
> >> > +	mutex_lock(&dev_priv->av_mutex);
> >> > +
> >>
> >> Bikeshed. We should think about moving the locking to
> >> intel_audio_codec_enable and intel_audio_codec_disable for
> >> consistency. We'll be adding new platform hooks eventually anyway,
> >> no
> >> need to duplicate the locking everywhere.
> >
> > Yes, at first, I want to add the mutex in intel_audio_codec_enable,
> > but after a second thought, we don't need it for ilk_xxx and g4x_xxx,
> > so I moved the lock in the hsw specific function for efficiency.
> >
> > I will move it to intel_audio_codec_enable in next version.
> >
> >>
> >> >  	/* Disable timestamps */
> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> > @@ -199,6 +251,8 @@ static void
> hsw_audio_codec_disable(struct
> >> intel_encoder *encoder)
> >> >  	tmp &= ~AUDIO_ELD_VALID(pipe);
> >> >  	tmp &= ~AUDIO_OUTPUT_ENABLE(pipe);
> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >> > +
> >> > +	mutex_unlock(&dev_priv->av_mutex);
> >> >  }
> >> >
> >> >  static void hsw_audio_codec_enable(struct drm_connector
> >> *connector,
> >> > @@ -215,6 +269,8 @@ static void
> hsw_audio_codec_enable(struct
> >> drm_connector *connector,
> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> >> ELD\n",
> >> >  		      pipe_name(pipe), drm_eld_size(eld));
> >> >
> >> > +	mutex_lock(&dev_priv->av_mutex);
> >> > +
> >> >  	/* Enable audio presence detect, invalidate ELD */
> >> >  	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> >> >  	tmp |= AUDIO_OUTPUT_ENABLE(pipe);
> >> > @@ -253,6 +309,8 @@ static void
> hsw_audio_codec_enable(struct
> >> drm_connector *connector,
> >> >  	else
> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> > +
> >> > +	mutex_unlock(&dev_priv->av_mutex);
> >> >  }
> >> >
> >> >  static void ilk_audio_codec_disable(struct intel_encoder
> *encoder)
> >> > @@ -514,12 +572,92 @@ 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;
> >>
> >> Nitpick. Initialize to INVALID_PIPE.
> >
> > Oh, yes, I will change it.
> >
> >>
> >> > +	u32 tmp;
> >> > +	int n_low, n_up, n;
> >> > +
> >> > +	/* HSW, BDW SKL need this fix */
> >> > +	if (!IS_SKYLAKE(dev_priv) &&
> >> > +		!IS_BROADWELL(dev_priv) &&
> >> > +		!IS_HASWELL(dev_priv))
> >> > +		return 0;
> >>
> >> Nitpick. We should elaborate that a bit more. Is it basically that only
> >> the above platforms support HDMI mode and audio sample rate
> >> combinations
> >> that the automatic mode in hardware can't support? In other words,
> >> would
> >> audio_rate_need_prog() always return false on other platforms
> than
> >> hsw/bdw/skl? And if so, why do we have this check? Perhaps we
> >> should
> >> take care to make audio_rate_need_prog() do the right thing, and
> do
> >> that
> >> as the first thing here.
> >
> > I add this judgement because the platforms are filed in our
> > internal document. Besides, the next gen may have such
> > issue and may not (the doc said MAY have such issue).
> > For easily extension, I add such judgement.
> >
> > audio_rate_need_prog() means the specified clock need
> > such setting. But I'm not sure whether other old platforms
> > support such clock. If yes, the audio_rate_need_prog()
> > may be not enough. Although I think doing the setting
> > should be OK even on the platforms not listed in the doc.
> > But I have no platform to do the test, so I didn't include
> > other platform this time.
> >
> >>
> >> > +
> >> > +	mutex_lock(&dev_priv->av_mutex);
> >> > +	/* 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__);
> >>
> >> DRM_DEBUG_KMS includes __func__ already.
> >
> > I will change it in next version. Thanks.
> >>
> >> > +				continue;
> >> > +			}
> >> > +			pipe = crtc->pipe;
> >> > +			break;
> >> > +		}
> >> > +	}
> >> > +
> >> > +	if (pipe == INVALID_PIPE) {
> >> > +		DRM_DEBUG_KMS("no pipe for the port %c\n",
> >> port_name(port));
> >> > +		mutex_unlock(&dev_priv->av_mutex);
> >> > +		return -ENODEV;
> >>
> >> Nitpick. I am not fond of early returns when cleanup like mutex
> unlock
> >> is required. The function should have goto labels at the end with
> >> cleanup. (OTOH if we move locking to higher level this problem
> goes
> >> away.)
> >
> > OK, so it seems to move the lock in upper function is an easy
> > way. I will move the lock in the upper function.
> >
> >>
> >> > +	}
> >> > +	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);
> >> > +		mutex_unlock(&dev_priv->av_mutex);
> >> > +		return 0;
> >>
> >> Nitpick. Same here, you could have an auto: label at the end with
> the
> >> auto programming...
> >>
> >> > +	}
> >> > +
> >> > +	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);
> >> > +		mutex_unlock(&dev_priv->av_mutex);
> >> > +		return 0;
> >>
> >> ...especially because this bit is duplicated for no good reason.
> >>
> >> Fallback to auto mode if the mode isn't in the list is, I think, a good
> >> thing. Maybe it would deserve a DRM_ERROR, because clearly that
> >> shouldn't happen.
> >
> > OK, I will add auto label for it. And I will use DRM_ERROR in next
> > version.
> >
> >>
> >> But it's actually the !audio_rate_need_prog case that makes me
> >> wonder. I
> >> think we've confirmed with the silicon folks that we can change N
> on
> >> the
> >> fly... but I'm actually not sure if we've confirmed that we can switch
> >> between automatic and manual modes on the fly. I think I've said it
> >> before, but I think I'd prefer it if we always used the manual mode
> >> anyway to reduce the complexity and have only one code path to
> >> debug,
> >> and that would also cover the somewhat of a corner case (but still
> real
> >> case) of switching between automatic/manual modes on the fly.
> And
> >> of
> >> course, this conflicts with what I said about the platform support
> check
> >> above, so needs thought.
> >
> > I will check with silicon team. Based on our stress test, it seems
> > to be OK change mode on the fly.
> >
> > We don't set for all the frequencies because there are so many
> > frequencies and platforms to do the stress test. We may need
> > a lot time to do the test. If the silicon team says we can't
> > change mode on the fly, I will change it in next version. If silicon
> > team says we can do it, I will use the HW auto mode for other
> > frequencies. What do you think?
> >
> > Regards,
> > Libin
> >
> >>
> >> > +	}
> >> > +	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);
> >> > +
> >> > +	mutex_unlock(&dev_priv->av_mutex);
> >> > +	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
> >> >
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> 
> --
> Jani Nikula, Intel Open Source Technology Center

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 097d4ba..8ffb5dc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -858,6 +858,7 @@  int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->sb_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 	mutex_init(&dev_priv->csr_lock);
+	mutex_init(&dev_priv->av_mutex);
 
 	intel_pm_setup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05ffd5a..2c6b76f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1882,6 +1882,11 @@  struct drm_i915_private {
 
 	/* hda/i915 audio component */
 	bool audio_component_registered;
+	/**
+	 * av_mutex - mutex for audio/video sync
+	 *
+	 */
+	struct mutex av_mutex;
 
 	uint32_t hw_context_size;
 	struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..a021720 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 },
+	{ 192000, TMDS_296M, 23296, 281250 },
+	{ 192000, 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(const 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,
@@ -184,6 +234,8 @@  static void hsw_audio_codec_disable(struct intel_encoder *encoder)
 
 	DRM_DEBUG_KMS("Disable audio codec on pipe %c\n", pipe_name(pipe));
 
+	mutex_lock(&dev_priv->av_mutex);
+
 	/* Disable timestamps */
 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -199,6 +251,8 @@  static void hsw_audio_codec_disable(struct intel_encoder *encoder)
 	tmp &= ~AUDIO_ELD_VALID(pipe);
 	tmp &= ~AUDIO_OUTPUT_ENABLE(pipe);
 	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+
+	mutex_unlock(&dev_priv->av_mutex);
 }
 
 static void hsw_audio_codec_enable(struct drm_connector *connector,
@@ -215,6 +269,8 @@  static void hsw_audio_codec_enable(struct drm_connector *connector,
 	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
 		      pipe_name(pipe), drm_eld_size(eld));
 
+	mutex_lock(&dev_priv->av_mutex);
+
 	/* Enable audio presence detect, invalidate ELD */
 	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
 	tmp |= AUDIO_OUTPUT_ENABLE(pipe);
@@ -253,6 +309,8 @@  static void hsw_audio_codec_enable(struct drm_connector *connector,
 	else
 		tmp |= audio_config_hdmi_pixel_clock(mode);
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	mutex_unlock(&dev_priv->av_mutex);
 }
 
 static void ilk_audio_codec_disable(struct intel_encoder *encoder)
@@ -514,12 +572,92 @@  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;
+
+	/* HSW, BDW SKL need this fix */
+	if (!IS_SKYLAKE(dev_priv) &&
+		!IS_BROADWELL(dev_priv) &&
+		!IS_HASWELL(dev_priv))
+		return 0;
+
+	mutex_lock(&dev_priv->av_mutex);
+	/* 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));
+		mutex_unlock(&dev_priv->av_mutex);
+		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);
+		mutex_unlock(&dev_priv->av_mutex);
+		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);
+		mutex_unlock(&dev_priv->av_mutex);
+		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);
+
+	mutex_unlock(&dev_priv->av_mutex);
+	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,