[v2,2/4] drm/i915: implement set_ncts callback
diff mbox

Message ID 1439191931-25705-2-git-send-email-libin.yang@intel.com
State New
Headers show

Commit Message

Yang, Libin Aug. 10, 2015, 7:32 a.m. UTC
From: Libin Yang <libin.yang@intel.com>

Display 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_reg.h    |  2 +
 drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)

Comments

Jani Nikula Aug. 10, 2015, noon UTC | #1
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> Display 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_reg.h    |  2 +
>  drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ea46d68..da2d128 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
>  					_HSW_AUD_MISC_CTRL_A, \
>  					_HSW_AUD_MISC_CTRL_B)
>  
> +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac

You're not using this register in this patch.

> +
>  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
>  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
>  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index dc32cf4..eddf37f 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -68,6 +68,30 @@ static const struct {
>  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
>  };
>  
> +#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)
>  {
> @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
>  	return ret;
>  }
>  
> +static int i915_audio_component_set_ncts(struct device *dev, int port,
> +			int dev_entry, 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 i;
> +	int n_low, n_up, n = 0;
> +
> +	/* 1. get the pipe */
> +	for_each_intel_encoder(drm_dev, intel_encoder) {
> +		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 == -1) {

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. Need set the N/CTS manually at some frequencies */
> +	if ((mode->clock != TMDS_297M) &&
> +		(mode->clock != TMDS_296M)) {
> +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> +		return 0;
> +	}

I'm thinking it would be better to always use the manual setting. There
may be obscure bugs due to *sometimes* using automatic mode and some
other times not. Or maybe we could use automatic mode in error
scenarios?

> +
> +	/* 3. calculate the N/CTS */
> +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> +		if ((rate == aud_ncts[i].sample_rate) &&
> +			(mode->clock == aud_ncts[i].clock)) {
> +			n = aud_ncts[i].n;
> +			break;
> +		}
> +	}

This part looks like a function to be abstracted (see
e.g. audio_config_hdmi_pixel_clock).

> +	if (n == 0)
> +		return 0;

This needs at least a debug warning and perhaps reverting to the
automatic mode.

> +	n_low = n & 0xfff;
> +	n_up = (n >> 12) & 0xff;
> +
> +	/* 4. set the N/CTS */
> +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);

You could squash the ors and ands together.

> +	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,
> +	.set_ncts = i915_audio_component_set_ncts,
>  };
>  
>  static int i915_audio_component_bind(struct device *i915_dev,
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Aug. 10, 2015, 12:16 p.m. UTC | #2
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> Display 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_reg.h    |  2 +
>  drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 97 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ea46d68..da2d128 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
>  					_HSW_AUD_MISC_CTRL_A, \
>  					_HSW_AUD_MISC_CTRL_B)
>  
> +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> +
>  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
>  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
>  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index dc32cf4..eddf37f 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -68,6 +68,30 @@ static const struct {
>  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
>  };
>  
> +#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)
>  {
> @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
>  	return ret;
>  }
>  
> +static int i915_audio_component_set_ncts(struct device *dev, int port,
> +			int dev_entry, 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 i;
> +	int n_low, n_up, n = 0;

I think you'll need the power well get here, and put at the end. Or
check that we it.

> +
> +	/* 1. get the pipe */
> +	for_each_intel_encoder(drm_dev, intel_encoder) {
> +		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 == -1) {
> +		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. Need set the N/CTS manually at some frequencies */
> +	if ((mode->clock != TMDS_297M) &&
> +		(mode->clock != TMDS_296M)) {
> +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> +		return 0;
> +	}
> +
> +	/* 3. calculate the N/CTS */
> +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> +		if ((rate == aud_ncts[i].sample_rate) &&
> +			(mode->clock == aud_ncts[i].clock)) {
> +			n = aud_ncts[i].n;
> +			break;
> +		}
> +	}
> +	if (n == 0)
> +		return 0;
> +	n_low = n & 0xfff;
> +	n_up = (n >> 12) & 0xff;
> +
> +	/* 4. set the N/CTS */
> +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> +	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,
> +	.set_ncts = i915_audio_component_set_ncts,
>  };
>  
>  static int i915_audio_component_bind(struct device *i915_dev,
> -- 
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Yang, Libin Aug. 11, 2015, 2:49 a.m. UTC | #3
Hi Jani,


> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, August 10, 2015 8:00 PM
> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Display 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_reg.h    |  2 +
> >  drivers/gpu/drm/i915/intel_audio.c | 95
> ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index ea46d68..da2d128 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_MISC_CTRL_A, \
> >  					_HSW_AUD_MISC_CTRL_B)
> >
> > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> 
> You're not using this register in this patch.

Oh, yes. In my first inside version, I use it. But remove using it
later. I will remove it. Thanks. 

> 
> > +
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index dc32cf4..eddf37f 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -68,6 +68,30 @@ static const struct {
> >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> >  };
> >
> > +#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)
> >  {
> > @@ -514,12 +538,83 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *dev)
> >  	return ret;
> >  }
> >
> > +static int i915_audio_component_set_ncts(struct device *dev, int
> port,
> > +			int dev_entry, 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 i;
> > +	int n_low, n_up, n = 0;
> > +
> > +	/* 1. get the pipe */
> > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > +		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 == -1) {
> 
> INVALID_PIPE

Got it. Thanks.

> 
> > +		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. Need set the N/CTS manually at some frequencies */
> > +	if ((mode->clock != TMDS_297M) &&
> > +		(mode->clock != TMDS_296M)) {
> > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > +		return 0;
> > +	}
> 
> I'm thinking it would be better to always use the manual setting. There
> may be obscure bugs due to *sometimes* using automatic mode and
> some
> other times not. Or maybe we could use automatic mode in error
> scenarios?

We have done a lot test using automatic mode in other 
TMDS (resolution). If we always use the  manual setting,
we may need too much testing on other TMDS on different platforms.
It seems using automatic mode in error scenarios will cause no sound.

> 
> > +
> > +	/* 3. calculate the N/CTS */
> > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > +		if ((rate == aud_ncts[i].sample_rate) &&
> > +			(mode->clock == aud_ncts[i].clock)) {
> > +			n = aud_ncts[i].n;
> > +			break;
> > +		}
> > +	}
> 
> This part looks like a function to be abstracted (see
> e.g. audio_config_hdmi_pixel_clock).

OK. I will use a function for it.

> 
> > +	if (n == 0)
> > +		return 0;
> 
> This needs at least a debug warning and perhaps reverting to the
> automatic mode.

OK. I see.

> 
> > +	n_low = n & 0xfff;
> > +	n_up = (n >> 12) & 0xff;
> > +
> > +	/* 4. set the N/CTS */
> > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> 
> You could squash the ors and ands together.

OK.

Regards,
Libin

> 
> > +	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,
> > +	.set_ncts = i915_audio_component_set_ncts,
> >  };
> >
> >  static int i915_audio_component_bind(struct device *i915_dev,
> > --
> > 1.9.1
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Technology Center
Yang, Libin Aug. 11, 2015, 2:55 a.m. UTC | #4
Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, August 10, 2015 8:17 PM
> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Display 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_reg.h    |  2 +
> >  drivers/gpu/drm/i915/intel_audio.c | 95
> ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index ea46d68..da2d128 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_MISC_CTRL_A, \
> >  					_HSW_AUD_MISC_CTRL_B)
> >
> > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> > +
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index dc32cf4..eddf37f 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -68,6 +68,30 @@ static const struct {
> >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> >  };
> >
> > +#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)
> >  {
> > @@ -514,12 +538,83 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *dev)
> >  	return ret;
> >  }
> >
> > +static int i915_audio_component_set_ncts(struct device *dev, int
> port,
> > +			int dev_entry, 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 i;
> > +	int n_low, n_up, n = 0;
> 
> I think you'll need the power well get here, and put at the end. Or
> check that we it.

As we only need set the n/cts when playing the audio,
this can make sure that audio driver has already required 
the power before we call the callback function.

Regards,
Libin

> 
> > +
> > +	/* 1. get the pipe */
> > +	for_each_intel_encoder(drm_dev, intel_encoder) {
> > +		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 == -1) {
> > +		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. Need set the N/CTS manually at some frequencies */
> > +	if ((mode->clock != TMDS_297M) &&
> > +		(mode->clock != TMDS_296M)) {
> > +		tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > +		return 0;
> > +	}
> > +
> > +	/* 3. calculate the N/CTS */
> > +	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > +		if ((rate == aud_ncts[i].sample_rate) &&
> > +			(mode->clock == aud_ncts[i].clock)) {
> > +			n = aud_ncts[i].n;
> > +			break;
> > +		}
> > +	}
> > +	if (n == 0)
> > +		return 0;
> > +	n_low = n & 0xfff;
> > +	n_up = (n >> 12) & 0xff;
> > +
> > +	/* 4. set the N/CTS */
> > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> > +	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,
> > +	.set_ncts = i915_audio_component_set_ncts,
> >  };
> >
> >  static int i915_audio_component_bind(struct device *i915_dev,
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Jani Nikula, Intel Open Source Technology Center
Daniel Vetter Aug. 12, 2015, 1:04 p.m. UTC | #5
On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > +	n_low = n & 0xfff;
> > +	n_up = (n >> 12) & 0xff;
> > +
> > +	/* 4. set the N/CTS */
> > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> 
> You could squash the ors and ands together.

Also read-modify-write considered evil. If you need to save a few bits
please be explicit about it like

	tmp &= BITS_WE_WANT_TO_KEEP;

if there's no bits to keep just start out with 0. If we have too much rwm
to hw registers then probably something with the overall design is
botched, e.g. with the pipe config precomputation we managed to remove
almost all the rwm code for modesets.
-Daniel

> 
> > +	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,
> > +	.set_ncts = i915_audio_component_set_ncts,
> >  };
> >  
> >  static int i915_audio_component_bind(struct device *i915_dev,
> > -- 
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
Daniel Vetter Aug. 12, 2015, 1:06 p.m. UTC | #6
On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > Display 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_reg.h    |  2 +
> >  drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 97 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index ea46d68..da2d128 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_MISC_CTRL_A, \
> >  					_HSW_AUD_MISC_CTRL_B)
> >  
> > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> > +
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > index dc32cf4..eddf37f 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -68,6 +68,30 @@ static const struct {
> >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> >  };
> >  
> > +#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)
> >  {
> > @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev)
> >  	return ret;
> >  }
> >  
> > +static int i915_audio_component_set_ncts(struct device *dev, int port,
> > +			int dev_entry, 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 i;
> > +	int n_low, n_up, n = 0;
> 
> I think you'll need the power well get here, and put at the end. Or
> check that we it.

If we call this and end up actually dropping the power well then the
register writes will go exactly nowhere at all. Which indicates a bug in
how this is orchestrated. There is probably one ...
-Daniel
Yang, Libin Aug. 12, 2015, 2:31 p.m. UTC | #7
Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:05 PM
> To: Jani Nikula
> Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > > +	n_low = n & 0xfff;
> > > +	n_up = (n >> 12) & 0xff;
> > > +
> > > +	/* 4. set the N/CTS */
> > > +	tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > +	tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > > +	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > > +	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > > +	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > > +	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
> >
> > You could squash the ors and ands together.
> 
> Also read-modify-write considered evil. If you need to save a few bits
> please be explicit about it like
> 
> 	tmp &= BITS_WE_WANT_TO_KEEP;

OK, I will try to use this format.

> 
> if there's no bits to keep just start out with 0. If we have too much rwm
> to hw registers then probably something with the overall design is
> botched, e.g. with the pipe config precomputation we managed to
> remove
> almost all the rwm code for modesets.

Yes, if we can set the register by starting out with 0,
it will be better. But in this case, there are some reserved
bits. We should reserve its bits for compatibility, I think.

> -Daniel
> 
> >
> > > +	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,
> > > +	.set_ncts = i915_audio_component_set_ncts,
> > >  };
> > >
> > >  static int i915_audio_component_bind(struct device *i915_dev,
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Jani Nikula, Intel Open Source Technology Center
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Yang, Libin Aug. 12, 2015, 2:36 p.m. UTC | #8
Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:06 PM
> To: Jani Nikula
> Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > > From: Libin Yang <libin.yang@intel.com>
> > >
> > > Display 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_reg.h    |  2 +
> > >  drivers/gpu/drm/i915/intel_audio.c | 95
> ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 97 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > > index ea46d68..da2d128 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> > >  					_HSW_AUD_MISC_CTRL_A, \
> > >  					_HSW_AUD_MISC_CTRL_B)
> > >
> > > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> > > +
> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> > >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > > index dc32cf4..eddf37f 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -68,6 +68,30 @@ static const struct {
> > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > >  };
> > >
> > > +#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)
> > >  {
> > > @@ -514,12 +538,83 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *dev)
> > >  	return ret;
> > >  }
> > >
> > > +static int i915_audio_component_set_ncts(struct device *dev, int
> port,
> > > +			int dev_entry, 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 i;
> > > +	int n_low, n_up, n = 0;
> >
> > I think you'll need the power well get here, and put at the end. Or
> > check that we it.
> 
> If we call this and end up actually dropping the power well then the
> register writes will go exactly nowhere at all. Which indicates a bug in
> how this is orchestrated. There is probably one ...

Sorry, I'm not understanding your meaning clearly. 
Do you mean if we put the power well, the register's value will
be invalid? Actually, we found another issue for audio power
management (this is another bug, not related to this feature),
and I'm thinking to add get_power/put_power as Jani said
in another patch.

Regards,
Libin

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Jani Nikula Aug. 12, 2015, 2:45 p.m. UTC | #9
On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Daniel,
>
>> -----Original Message-----
>> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
>> Daniel Vetter
>> Sent: Wednesday, August 12, 2015 9:06 PM
>> To: Jani Nikula
>> Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
>> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
>> callback
>> 
>> On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
>> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> > > From: Libin Yang <libin.yang@intel.com>
>> > >
>> > > Display 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_reg.h    |  2 +
>> > >  drivers/gpu/drm/i915/intel_audio.c | 95
>> ++++++++++++++++++++++++++++++++++++++
>> > >  2 files changed, 97 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> > > index ea46d68..da2d128 100644
>> > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
>> > >  					_HSW_AUD_MISC_CTRL_A, \
>> > >  					_HSW_AUD_MISC_CTRL_B)
>> > >
>> > > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
>> > > +
>> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
>> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
>> > >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
>> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> > > index dc32cf4..eddf37f 100644
>> > > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > > @@ -68,6 +68,30 @@ static const struct {
>> > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
>> > >  };
>> > >
>> > > +#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)
>> > >  {
>> > > @@ -514,12 +538,83 @@ static int
>> i915_audio_component_get_cdclk_freq(struct device *dev)
>> > >  	return ret;
>> > >  }
>> > >
>> > > +static int i915_audio_component_set_ncts(struct device *dev, int
>> port,
>> > > +			int dev_entry, 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 i;
>> > > +	int n_low, n_up, n = 0;
>> >
>> > I think you'll need the power well get here, and put at the end. Or
>> > check that we it.
>> 
>> If we call this and end up actually dropping the power well then the
>> register writes will go exactly nowhere at all. Which indicates a bug in
>> how this is orchestrated. There is probably one ...
>
> Sorry, I'm not understanding your meaning clearly. 
> Do you mean if we put the power well, the register's value will
> be invalid? Actually, we found another issue for audio power
> management (this is another bug, not related to this feature),
> and I'm thinking to add get_power/put_power as Jani said
> in another patch.

No, the point was that you should rather check that we have power. If
there isn't, and you get/put, you'll lose the values at the put.

BR,
Jani.



>
> Regards,
> Libin
>
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
Yang, Libin Aug. 12, 2015, 2:48 p.m. UTC | #10
Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Wednesday, August 12, 2015 10:45 PM
> To: Yang, Libin; Daniel Vetter
> Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: RE: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > Hi Daniel,
> >
> >> -----Original Message-----
> >> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> >> Daniel Vetter
> >> Sent: Wednesday, August 12, 2015 9:06 PM
> >> To: Jani Nikula
> >> Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-
> >> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> >> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement
> set_ncts
> >> callback
> >>
> >> On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
> >> > On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> > > From: Libin Yang <libin.yang@intel.com>
> >> > >
> >> > > Display 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_reg.h    |  2 +
> >> > >  drivers/gpu/drm/i915/intel_audio.c | 95
> >> ++++++++++++++++++++++++++++++++++++++
> >> > >  2 files changed, 97 insertions(+)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> > > index ea46d68..da2d128 100644
> >> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> >> > >  					_HSW_AUD_MISC_CTRL_A, \
> >> > >  					_HSW_AUD_MISC_CTRL_B)
> >> > >
> >> > > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> >> > > +
> >> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> >> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> >> > >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> >> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> > > index dc32cf4..eddf37f 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> > > @@ -68,6 +68,30 @@ static const struct {
> >> > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> >> > >  };
> >> > >
> >> > > +#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)
> >> > >  {
> >> > > @@ -514,12 +538,83 @@ static int
> >> i915_audio_component_get_cdclk_freq(struct device *dev)
> >> > >  	return ret;
> >> > >  }
> >> > >
> >> > > +static int i915_audio_component_set_ncts(struct device *dev,
> int
> >> port,
> >> > > +			int dev_entry, 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 i;
> >> > > +	int n_low, n_up, n = 0;
> >> >
> >> > I think you'll need the power well get here, and put at the end. Or
> >> > check that we it.
> >>
> >> If we call this and end up actually dropping the power well then the
> >> register writes will go exactly nowhere at all. Which indicates a bug
> in
> >> how this is orchestrated. There is probably one ...
> >
> > Sorry, I'm not understanding your meaning clearly.
> > Do you mean if we put the power well, the register's value will
> > be invalid? Actually, we found another issue for audio power
> > management (this is another bug, not related to this feature),
> > and I'm thinking to add get_power/put_power as Jani said
> > in another patch.
> 
> No, the point was that you should rather check that we have power. If
> there isn't, and you get/put, you'll lose the values at the put.

In another patch, we may not care the register value,
we need get_power to trigger an interrupt to audio
for the hotplug. Otherwise audio driver will never
receive the interrupt. After the interrupt is triggered,
we may not care the registers. This is a draft thought,
I need do more testing for that bug. Let's discuss about
it in another patch thread :)

Regards,
Libin

> 
> BR,
> Jani.
> 
> 
> 
> >
> > Regards,
> > Libin
> >
> >> -Daniel
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> 
> --
> Jani Nikula, Intel Open Source Technology Center

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7014,6 +7014,8 @@  enum skl_disp_power_wells {
 					_HSW_AUD_MISC_CTRL_A, \
 					_HSW_AUD_MISC_CTRL_B)
 
+#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
+
 #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
 #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -68,6 +68,30 @@  static const struct {
 	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
 };
 
+#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)
 {
@@ -514,12 +538,83 @@  static int i915_audio_component_get_cdclk_freq(struct device *dev)
 	return ret;
 }
 
+static int i915_audio_component_set_ncts(struct device *dev, int port,
+			int dev_entry, 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 i;
+	int n_low, n_up, n = 0;
+
+	/* 1. get the pipe */
+	for_each_intel_encoder(drm_dev, intel_encoder) {
+		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 == -1) {
+		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. Need set the N/CTS manually at some frequencies */
+	if ((mode->clock != TMDS_297M) &&
+		(mode->clock != TMDS_296M)) {
+		tmp = I915_READ(HSW_AUD_CFG(pipe));
+		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+		return 0;
+	}
+
+	/* 3. calculate the N/CTS */
+	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
+		if ((rate == aud_ncts[i].sample_rate) &&
+			(mode->clock == aud_ncts[i].clock)) {
+			n = aud_ncts[i].n;
+			break;
+		}
+	}
+	if (n == 0)
+		return 0;
+	n_low = n & 0xfff;
+	n_up = (n >> 12) & 0xff;
+
+	/* 4. set the N/CTS */
+	tmp = I915_READ(HSW_AUD_CFG(pipe));
+	tmp |= AUD_CONFIG_N_PROG_ENABLE;
+	tmp &= ~AUD_CONFIG_UPPER_N_MASK;
+	tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
+	tmp &= ~AUD_CONFIG_LOWER_N_MASK;
+	tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
+	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,
+	.set_ncts = i915_audio_component_set_ncts,
 };
 
 static int i915_audio_component_bind(struct device *i915_dev,