[PATCH v4 1/4] drm/i915: Add audio sync_audio_rate callback

[PATCH v4 1/4] drm/i915: Add audio sync_audio_rate callback

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

Add the sync_audio_rate callback.

With the callback, audio driver can trigger
i915 driver to set the proper N/CTS or N/M
based on different sample rates.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 include/drm/i915_component.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..aabebcb 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -33,6 +33,7 @@ struct i915_audio_component {
 		void (*put_power)(struct device *);
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
+		int (*sync_audio_rate)(struct device *, int port, int rate);
 	} *ops;
 };
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 2/4] drm/i915: implement sync_audio_rate callback

[libin.yang]

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/intel_audio.c | 116 +++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 3/4] ALSA: hda - display audio call sync_audio_rate callback

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

For display audio, call the sync_audio_rate callback function
to do the synchronization between gfx driver and audio driver.

Signed-off-by: Libin Yang <libin.yang@intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/patch_hdmi.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index a97db5f..1668868 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1770,6 +1770,16 @@ static bool check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
 	return non_pcm;
 }
 
+/* There is a fixed mapping between audio pin node and display port
+ * on current Intel platforms:
+ * Pin Widget 5 - PORT B (port = 1 in i915 driver)
+ * Pin Widget 6 - PORT C (port = 2 in i915 driver)
+ * Pin Widget 7 - PORT D (port = 3 in i915 driver)
+ */
+static int intel_pin2port(hda_nid_t pin_nid)
+{
+	return pin_nid - 4;
+}
 
 /*
  * HDMI callbacks
@@ -1786,6 +1796,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 	int pin_idx = hinfo_to_pin_index(codec, hinfo);
 	struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 	hda_nid_t pin_nid = per_pin->pin_nid;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct i915_audio_component *acomp = codec->bus->core.audio_component;
 	bool non_pcm;
 	int pinctl;
 
@@ -1802,6 +1814,13 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
 		intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx);
 	}
 
+	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
+	/* Todo: add DP1.2 MST audio support later */
+	if (acomp && acomp->ops && acomp->ops->sync_audio_rate)
+		acomp->ops->sync_audio_rate(acomp->dev,
+				intel_pin2port(pin_nid),
+				runtime->rate);
+
 	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
 	mutex_lock(&per_pin->lock);
 	per_pin->channels = substream->runtime->channels;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

When modeset occurs and the TMDS frequency is set to some
speical values, the N/CTS need to be set manually if audio
is playing.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  8 ++++++++
 drivers/gpu/drm/i915/intel_audio.c | 40 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6786e94..122b5bd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7035,6 +7035,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, \
@@ -7049,6 +7051,12 @@ enum skl_disp_power_wells {
 					_HSW_AUD_DIG_CNVT_2)
 #define DIP_PORT_SEL_MASK		0x3
 
+#define _HSW_AUD_STR_DESC_1		0x65084
+#define _HSW_AUD_STR_DESC_2		0x65184
+#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
+					 _HSW_AUD_STR_DESC_1,	\
+					 _HSW_AUD_STR_DESC_2)
+
 #define _HSW_AUD_EDID_DATA_A		0x65050
 #define _HSW_AUD_EDID_DATA_B		0x65150
 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 9d6ba84..b756309 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc,
 		return false;
 }
 
+static int audio_config_get_rate(struct drm_i915_private *dev_priv,
+						enum pipe pipe)
+{
+	uint32_t tmp;
+	int cvt_idx;
+	int base_rate, mul, div, rate;
+
+	tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
+	cvt_idx = (tmp >> (pipe * 8)) & 0xff;
+	tmp = I915_READ(AUD_STR_DESC(cvt_idx));
+	base_rate = tmp & (1 << 14);
+	if (base_rate == 0)
+		rate = 48000;
+	else
+		rate = 44100;
+	mul = (tmp & (0x7 << 11)) + 1;
+	div = (tmp & (0x7 << 8)) + 1;
+	rate = rate * mul / div;
+	return rate;
+}
+
 static bool intel_eld_uptodate(struct drm_connector *connector,
 			       int reg_eldv, uint32_t bits_eldv,
 			       int reg_elda, uint32_t bits_elda,
@@ -261,6 +282,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 	const uint8_t *eld = connector->eld;
 	uint32_t tmp;
 	int len, i;
+	int n_low, n_up, n;
+	int rate;
 
 	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
 		      pipe_name(pipe), drm_eld_size(eld));
@@ -296,12 +319,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 	/* Enable timestamps */
 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
-	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
 	if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT))
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
 		tmp |= audio_config_hdmi_pixel_clock(mode);
+
+	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+	if (audio_rate_need_prog(intel_crtc, mode)) {
+		rate = audio_config_get_rate(dev_priv, pipe);
+		n = audio_config_get_n(mode, rate);
+		if (n != 0) {
+			n_low = n & 0xfff;
+			n_up = (n >> 12) & 0xff;
+			tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
+					 AUD_CONFIG_LOWER_N_MASK);
+			tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
+					(n_low << AUD_CONFIG_LOWER_N_SHIFT) |
+					AUD_CONFIG_N_PROG_ENABLE);
+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/4] drm/i915: Add audio set_ncts callback

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

Add the set_ncts callback.

With the callback, audio driver can trigger
i915 driver to set the proper N/CTS
based on different sample rates.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 include/drm/i915_component.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..7305881 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -33,6 +33,8 @@ struct i915_audio_component {
 		void (*put_power)(struct device *);
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
+		int (*set_ncts)(struct device *, int port, int dev_entry,
+					int rate);
 	} *ops;
 };
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[libin.yang]

From: Libin Yang <libin.yang@intel.com>

When modeset occurs and the TMDS frequency is set to some
speical value, the N/CTS need to be set manually if audio
is playing.

Signed-off-by: Libin Yang <libin.yang@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
 drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
 					_HSW_AUD_DIG_CNVT_2)
 #define DIP_PORT_SEL_MASK		0x3
 
+#define _HSW_AUD_STR_DESC_1		0x65084
+#define _HSW_AUD_STR_DESC_2		0x65184
+#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
+					 _HSW_AUD_STR_DESC_1,	\
+					 _HSW_AUD_STR_DESC_2)
+
 #define _HSW_AUD_EDID_DATA_A		0x65050
 #define _HSW_AUD_EDID_DATA_B		0x65150
 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 	const uint8_t *eld = connector->eld;
 	uint32_t tmp;
 	int len, i;
+	int cvt_idx;
+	int n_low, n_up, n;
+	int base_rate, mul, div, rate;
 
 	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
 		      pipe_name(pipe), drm_eld_size(eld));
@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 	tmp |= AUDIO_ELD_VALID(pipe);
 	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
 
+	if ((mode->clock == 297000) ||
+		(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {
+		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
+		cvt_idx = (tmp >> pipe*8) & 0xff;
+		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
+		base_rate = tmp & (1 << 14);
+		if (base_rate == 0)
+			rate = 48000;
+		else
+			rate = 44100;
+		mul = (tmp & (0x7 << 11)) + 1;
+		div = (tmp & (0x7 << 8)) + 1;
+		rate = rate * mul / div;
+	}
+
 	/* Enable timestamps */
 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
 		tmp |= AUD_CONFIG_N_VALUE_INDEX;
 	else
 		tmp |= audio_config_hdmi_pixel_clock(mode);
+
+	if ((mode->clock != 297000) &&
+		(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {
+		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+	} else {
+		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) {
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+			n_low = n & 0xfff;
+			n_up = (n >> 12) & 0xff;
+			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);
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Takashi Iwai]

On Mon, 10 Aug 2015 09:32:11 +0200,
libin.yang@intel.com wrote:
> 
> From: Libin Yang <libin.yang@intel.com>
> 
> When modeset occurs and the TMDS frequency is set to some
> speical value, the N/CTS need to be set manually if audio
> is playing.
> 
> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>  drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da2d128..85f3beb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>  					_HSW_AUD_DIG_CNVT_2)
>  #define DIP_PORT_SEL_MASK		0x3
>  
> +#define _HSW_AUD_STR_DESC_1		0x65084
> +#define _HSW_AUD_STR_DESC_2		0x65184
> +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> +					 _HSW_AUD_STR_DESC_1,	\
> +					 _HSW_AUD_STR_DESC_2)
> +
>  #define _HSW_AUD_EDID_DATA_A		0x65050
>  #define _HSW_AUD_EDID_DATA_B		0x65150
>  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index eddf37f..082f96d 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	const uint8_t *eld = connector->eld;
>  	uint32_t tmp;
>  	int len, i;
> +	int cvt_idx;
> +	int n_low, n_up, n;
> +	int base_rate, mul, div, rate;
>  
>  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
>  		      pipe_name(pipe), drm_eld_size(eld));
> @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	tmp |= AUDIO_ELD_VALID(pipe);
>  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>  
> +	if ((mode->clock == 297000) ||
> +		(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {

TMDS_297M and TMDS_296M can be used here?


> +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> +		cvt_idx = (tmp >> pipe*8) & 0xff;
> +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> +		base_rate = tmp & (1 << 14);
> +		if (base_rate == 0)
> +			rate = 48000;
> +		else
> +			rate = 44100;
> +		mul = (tmp & (0x7 << 11)) + 1;
> +		div = (tmp & (0x7 << 8)) + 1;
> +		rate = rate * mul / div;
> +	}
> +
>  	/* Enable timestamps */
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(mode);
> +
> +	if ((mode->clock != 297000) &&
> +		(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +	} else {
> +		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;
> +			}
> +		}

Missing initialization of n?

> +		if (n != 0) {
> +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +			n_low = n & 0xfff;
> +			n_up = (n >> 12) & 0xff;
> +			tmp |= AUD_CONFIG_N_PROG_ENABLE;

You don't need to set it twice.

> +			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);
>  }
>  
> -- 
> 1.9.1
> 


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Monday, August 10, 2015 4:04 PM
> To: Yang, Libin
> Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org;
> daniel.vetter@ffwll.ch
> Subject: Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
> 
> On Mon, 10 Aug 2015 09:32:11 +0200,
> libin.yang@intel.com wrote:
> >
> > From: Libin Yang <libin.yang@intel.com>
> >
> > When modeset occurs and the TMDS frequency is set to some
> > speical value, the N/CTS need to be set manually if audio
> > is playing.
> >
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >  drivers/gpu/drm/i915/intel_audio.c | 42
> ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index da2d128..85f3beb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_DIG_CNVT_2)
> >  #define DIP_PORT_SEL_MASK		0x3
> >
> > +#define _HSW_AUD_STR_DESC_1		0x65084
> > +#define _HSW_AUD_STR_DESC_2		0x65184
> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> > +					 _HSW_AUD_STR_DESC_1,
> 	\
> > +					 _HSW_AUD_STR_DESC_2)
> > +
> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index eddf37f..082f96d 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	const uint8_t *eld = connector->eld;
> >  	uint32_t tmp;
> >  	int len, i;
> > +	int cvt_idx;
> > +	int n_low, n_up, n;
> > +	int base_rate, mul, div, rate;
> >
> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> ELD\n",
> >  		      pipe_name(pipe), drm_eld_size(eld));
> > @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >
> > +	if ((mode->clock == 297000) ||
> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001)))
> {
> 
> TMDS_297M and TMDS_296M can be used here?

Oh, sorry, I forgot to use it here.

> 
> 
> > +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> > +		base_rate = tmp & (1 << 14);
> > +		if (base_rate == 0)
> > +			rate = 48000;
> > +		else
> > +			rate = 44100;
> > +		mul = (tmp & (0x7 << 11)) + 1;
> > +		div = (tmp & (0x7 << 8)) + 1;
> > +		rate = rate * mul / div;
> > +	}
> > +
> >  	/* Enable timestamps */
> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> > +
> > +	if ((mode->clock != 297000) &&
> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001)))
> {
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +	} else {
> > +		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;
> > +			}
> > +		}
> 
> Missing initialization of n?

Oh, yes, I will fix it.

> 
> > +		if (n != 0) {
> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > +			n_low = n & 0xfff;
> > +			n_up = (n >> 12) & 0xff;
> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> 
> You don't need to set it twice.

Yes.

> 
> > +			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);
> >  }
> >
> > --
> > 1.9.1
> >
> 
> 
> thanks,
> 
> Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> From: Libin Yang <libin.yang@intel.com>
>
> When modeset occurs and the TMDS frequency is set to some
> speical value, the N/CTS need to be set manually if audio
> is playing.

When modeset occurs, we disable everything, and then re-enable
everything, and notify the audio driver every step of the way. IIUC
there should be no audio playing across the modeset, and when the
modeset is complete and we've set the valid ELD, the audio driver should
call the callback from the earlier patches to set the correct audio
rate.

Why is this patch needed? Please enlighten me.

Also some comments in-line, provided you've convinced me first. ;)

> Signed-off-by: Libin Yang <libin.yang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>  drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index da2d128..85f3beb 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>  					_HSW_AUD_DIG_CNVT_2)
>  #define DIP_PORT_SEL_MASK		0x3
>  
> +#define _HSW_AUD_STR_DESC_1		0x65084
> +#define _HSW_AUD_STR_DESC_2		0x65184
> +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> +					 _HSW_AUD_STR_DESC_1,	\
> +					 _HSW_AUD_STR_DESC_2)
> +
>  #define _HSW_AUD_EDID_DATA_A		0x65050
>  #define _HSW_AUD_EDID_DATA_B		0x65150
>  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index eddf37f..082f96d 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	const uint8_t *eld = connector->eld;
>  	uint32_t tmp;
>  	int len, i;
> +	int cvt_idx;
> +	int n_low, n_up, n;
> +	int base_rate, mul, div, rate;
>  
>  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
>  		      pipe_name(pipe), drm_eld_size(eld));
> @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  	tmp |= AUDIO_ELD_VALID(pipe);
>  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>  
> +	if ((mode->clock == 297000) ||
> +		(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {
> +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> +		cvt_idx = (tmp >> pipe*8) & 0xff;
> +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> +		base_rate = tmp & (1 << 14);
> +		if (base_rate == 0)
> +			rate = 48000;
> +		else
> +			rate = 44100;
> +		mul = (tmp & (0x7 << 11)) + 1;
> +		div = (tmp & (0x7 << 8)) + 1;
> +		rate = rate * mul / div;
> +	}

This should be abstracted to a separate function.

> +
>  	/* Enable timestamps */
>  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
>  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>  	else
>  		tmp |= audio_config_hdmi_pixel_clock(mode);
> +
> +	if ((mode->clock != 297000) &&
> +		(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {
> +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> +	} else {
> +		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) {
> +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> +			n_low = n & 0xfff;
> +			n_up = (n >> 12) & 0xff;
> +			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);
> +		}
> +	}

Please de-duplicate the copy-paste from patch 2. At the very least you
should use a helper for finding the parameters from the table.

> +
>  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>  }
>  
> -- 
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Jani,

Thanks for careful reviewing for all the patches, please
see my comments.

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > From: Libin Yang <libin.yang@intel.com>
> >
> > When modeset occurs and the TMDS frequency is set to some
> > speical value, the N/CTS need to be set manually if audio
> > is playing.
> 
> When modeset occurs, we disable everything, and then re-enable
> everything, and notify the audio driver every step of the way. IIUC
> there should be no audio playing across the modeset, and when the
> modeset is complete and we've set the valid ELD, the audio driver
> should
> call the callback from the earlier patches to set the correct audio
> rate.
> 
> Why is this patch needed? Please enlighten me.

Please image this scenario: when audio is playing, the gfx driver
does the modeset. In this situation, after modeset, audio driver
will do nothing and continue playing. And audio driver will not call
set_ncts. 

> 
> Also some comments in-line, provided you've convinced me first. ;)
> 
> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >  drivers/gpu/drm/i915/intel_audio.c | 42
> ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 48 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index da2d128..85f3beb 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >  					_HSW_AUD_DIG_CNVT_2)
> >  #define DIP_PORT_SEL_MASK		0x3
> >
> > +#define _HSW_AUD_STR_DESC_1		0x65084
> > +#define _HSW_AUD_STR_DESC_2		0x65184
> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> > +					 _HSW_AUD_STR_DESC_1,
> 	\
> > +					 _HSW_AUD_STR_DESC_2)
> > +
> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > index eddf37f..082f96d 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	const uint8_t *eld = connector->eld;
> >  	uint32_t tmp;
> >  	int len, i;
> > +	int cvt_idx;
> > +	int n_low, n_up, n;
> > +	int base_rate, mul, div, rate;
> >
> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> ELD\n",
> >  		      pipe_name(pipe), drm_eld_size(eld));
> > @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >
> > +	if ((mode->clock == 297000) ||
> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
> 1001))) {
> > +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> > +		base_rate = tmp & (1 << 14);
> > +		if (base_rate == 0)
> > +			rate = 48000;
> > +		else
> > +			rate = 44100;
> > +		mul = (tmp & (0x7 << 11)) + 1;
> > +		div = (tmp & (0x7 << 8)) + 1;
> > +		rate = rate * mul / div;
> > +	}
> 
> This should be abstracted to a separate function.

Yes. I will do it.

> 
> > +
> >  	/* Enable timestamps */
> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >  	else
> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> > +
> > +	if ((mode->clock != 297000) &&
> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
> 1001))) {
> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > +	} else {
> > +		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) {
> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > +			n_low = n & 0xfff;
> > +			n_up = (n >> 12) & 0xff;
> > +			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);
> > +		}
> > +	}
> 
> Please de-duplicate the copy-paste from patch 2. At the very least you
> should use a helper for finding the parameters from the table.

OK. I see.

Regards,
Libin

> 
> > +
> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >  }
> >
> > --
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Jani,
>
> Thanks for careful reviewing for all the patches, please
> see my comments.
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS in
>> modeset
>> 
>> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> > From: Libin Yang <libin.yang@intel.com>
>> >
>> > When modeset occurs and the TMDS frequency is set to some
>> > speical value, the N/CTS need to be set manually if audio
>> > is playing.
>> 
>> When modeset occurs, we disable everything, and then re-enable
>> everything, and notify the audio driver every step of the way. IIUC
>> there should be no audio playing across the modeset, and when the
>> modeset is complete and we've set the valid ELD, the audio driver
>> should
>> call the callback from the earlier patches to set the correct audio
>> rate.
>> 
>> Why is this patch needed? Please enlighten me.
>
> Please image this scenario: when audio is playing, the gfx driver
> does the modeset. In this situation, after modeset, audio driver
> will do nothing and continue playing. And audio driver will not call
> set_ncts.

Why does the audio driver not do anything here? Whenever there's a
modeset, the graphics driver will disable audio and invalidate the ELD,
which should cause an interrupt to notify the audio driver about
this. This is no different from a hot-unplug, in fact I can't see how
the audio driver could even detect whether there's been a hotplug or not
when there's a codec disable/enable.

BR,
Jani.


>
>> 
>> Also some comments in-line, provided you've convinced me first. ;)
>> 
>> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>> >  drivers/gpu/drm/i915/intel_audio.c | 42
>> ++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 48 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> > index da2d128..85f3beb 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>> >  					_HSW_AUD_DIG_CNVT_2)
>> >  #define DIP_PORT_SEL_MASK		0x3
>> >
>> > +#define _HSW_AUD_STR_DESC_1		0x65084
>> > +#define _HSW_AUD_STR_DESC_2		0x65184
>> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
>> > +					 _HSW_AUD_STR_DESC_1,
>> 	\
>> > +					 _HSW_AUD_STR_DESC_2)
>> > +
>> >  #define _HSW_AUD_EDID_DATA_A		0x65050
>> >  #define _HSW_AUD_EDID_DATA_B		0x65150
>> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
>> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> b/drivers/gpu/drm/i915/intel_audio.c
>> > index eddf37f..082f96d 100644
>> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> > @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  	const uint8_t *eld = connector->eld;
>> >  	uint32_t tmp;
>> >  	int len, i;
>> > +	int cvt_idx;
>> > +	int n_low, n_up, n;
>> > +	int base_rate, mul, div, rate;
>> >
>> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
>> ELD\n",
>> >  		      pipe_name(pipe), drm_eld_size(eld));
>> > @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  	tmp |= AUDIO_ELD_VALID(pipe);
>> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> >
>> > +	if ((mode->clock == 297000) ||
>> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
>> 1001))) {
>> > +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
>> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> > +		base_rate = tmp & (1 << 14);
>> > +		if (base_rate == 0)
>> > +			rate = 48000;
>> > +		else
>> > +			rate = 44100;
>> > +		mul = (tmp & (0x7 << 11)) + 1;
>> > +		div = (tmp & (0x7 << 8)) + 1;
>> > +		rate = rate * mul / div;
>> > +	}
>> 
>> This should be abstracted to a separate function.
>
> Yes. I will do it.
>
>> 
>> > +
>> >  	/* Enable timestamps */
>> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> > @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
>> drm_connector *connector,
>> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >  	else
>> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> > +
>> > +	if ((mode->clock != 297000) &&
>> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
>> 1001))) {
>> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> > +	} else {
>> > +		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) {
>> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> > +			n_low = n & 0xfff;
>> > +			n_up = (n >> 12) & 0xff;
>> > +			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);
>> > +		}
>> > +	}
>> 
>> Please de-duplicate the copy-paste from patch 2. At the very least you
>> should use a helper for finding the parameters from the table.
>
> OK. I see.
>
> Regards,
> Libin
>
>> 
>> > +
>> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >  }
>> >
>> > --
>> > 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

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

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > Hi Jani,
> >
> > Thanks for careful reviewing for all the patches, please
> > see my comments.
> >
> >> -----Original Message-----
> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS in
> >> modeset
> >>
> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> > From: Libin Yang <libin.yang@intel.com>
> >> >
> >> > When modeset occurs and the TMDS frequency is set to some
> >> > speical value, the N/CTS need to be set manually if audio
> >> > is playing.
> >>
> >> When modeset occurs, we disable everything, and then re-enable
> >> everything, and notify the audio driver every step of the way. IIUC
> >> there should be no audio playing across the modeset, and when the
> >> modeset is complete and we've set the valid ELD, the audio driver
> >> should
> >> call the callback from the earlier patches to set the correct audio
> >> rate.
> >>
> >> Why is this patch needed? Please enlighten me.
> >
> > Please image this scenario: when audio is playing, the gfx driver
> > does the modeset. In this situation, after modeset, audio driver
> > will do nothing and continue playing. And audio driver will not call
> > set_ncts.
> 
> Why does the audio driver not do anything here? Whenever there's a
> modeset, the graphics driver will disable audio and invalidate the ELD,
> which should cause an interrupt to notify the audio driver about
> this. This is no different from a hot-unplug, in fact I can't see how
> the audio driver could even detect whether there's been a hotplug or
> not
> when there's a codec disable/enable.

Yes, it will trigger an interrupt to audio driver when unplug
and another interrupt for hotplug. But from audio driver,
we will not stop the playing and resume the playing based
on the hotplug or modeset. The audio is always playing during
the hotplug/modeset.

> 
> BR,
> Jani.
> 
> 
> >
> >>
> >> Also some comments in-line, provided you've convinced me first. ;)
> >>
> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
> >> ++++++++++++++++++++++++++++++++++++++
> >> >  2 files changed, 48 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> > index da2d128..85f3beb 100644
> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >> >  					_HSW_AUD_DIG_CNVT_2)
> >> >  #define DIP_PORT_SEL_MASK		0x3
> >> >
> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> >> > +					 _HSW_AUD_STR_DESC_1,
> >> 	\
> >> > +					 _HSW_AUD_STR_DESC_2)
> >> > +
> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> > index eddf37f..082f96d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> > @@ -235,6 +235,9 @@ static void
> hsw_audio_codec_enable(struct
> >> drm_connector *connector,
> >> >  	const uint8_t *eld = connector->eld;
> >> >  	uint32_t tmp;
> >> >  	int len, i;
> >> > +	int cvt_idx;
> >> > +	int n_low, n_up, n;
> >> > +	int base_rate, mul, div, rate;
> >> >
> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
> >> ELD\n",
> >> >  		      pipe_name(pipe), drm_eld_size(eld));
> >> > @@ -267,6 +270,21 @@ static void
> hsw_audio_codec_enable(struct
> >> drm_connector *connector,
> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >> >
> >> > +	if ((mode->clock == 297000) ||
> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
> >> 1001))) {
> >> > +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> >> > +		base_rate = tmp & (1 << 14);
> >> > +		if (base_rate == 0)
> >> > +			rate = 48000;
> >> > +		else
> >> > +			rate = 44100;
> >> > +		mul = (tmp & (0x7 << 11)) + 1;
> >> > +		div = (tmp & (0x7 << 8)) + 1;
> >> > +		rate = rate * mul / div;
> >> > +	}
> >>
> >> This should be abstracted to a separate function.
> >
> > Yes. I will do it.
> >
> >>
> >> > +
> >> >  	/* Enable timestamps */
> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> > @@ -276,6 +294,30 @@ static void
> hsw_audio_codec_enable(struct
> >> drm_connector *connector,
> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >  	else
> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >> > +
> >> > +	if ((mode->clock != 297000) &&
> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
> >> 1001))) {
> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >> > +	} else {
> >> > +		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) {
> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> > +			n_low = n & 0xfff;
> >> > +			n_up = (n >> 12) & 0xff;
> >> > +			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);
> >> > +		}
> >> > +	}
> >>
> >> Please de-duplicate the copy-paste from patch 2. At the very least
> you
> >> should use a helper for finding the parameters from the table.
> >
> > OK. I see.
> >
> > Regards,
> > Libin
> >
> >>
> >> > +
> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >  }
> >> >
> >> > --
> >> > 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
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
>> modeset
>> 
>> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> > Hi Jani,
>> >
>> > Thanks for careful reviewing for all the patches, please
>> > see my comments.
>> >
>> >> -----Original Message-----
>> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS in
>> >> modeset
>> >>
>> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> >> > From: Libin Yang <libin.yang@intel.com>
>> >> >
>> >> > When modeset occurs and the TMDS frequency is set to some
>> >> > speical value, the N/CTS need to be set manually if audio
>> >> > is playing.
>> >>
>> >> When modeset occurs, we disable everything, and then re-enable
>> >> everything, and notify the audio driver every step of the way. IIUC
>> >> there should be no audio playing across the modeset, and when the
>> >> modeset is complete and we've set the valid ELD, the audio driver
>> >> should
>> >> call the callback from the earlier patches to set the correct audio
>> >> rate.
>> >>
>> >> Why is this patch needed? Please enlighten me.
>> >
>> > Please image this scenario: when audio is playing, the gfx driver
>> > does the modeset. In this situation, after modeset, audio driver
>> > will do nothing and continue playing. And audio driver will not call
>> > set_ncts.
>> 
>> Why does the audio driver not do anything here? Whenever there's a
>> modeset, the graphics driver will disable audio and invalidate the ELD,
>> which should cause an interrupt to notify the audio driver about
>> this. This is no different from a hot-unplug, in fact I can't see how
>> the audio driver could even detect whether there's been a hotplug or
>> not
>> when there's a codec disable/enable.
>
> Yes, it will trigger an interrupt to audio driver when unplug
> and another interrupt for hotplug. But from audio driver,
> we will not stop the playing and resume the playing based
> on the hotplug or modeset. The audio is always playing during
> the hotplug/modeset.

But the whole display, and consequently ELD, might have changed during
the hotplug, why do you assume you can just keep playing?! The display
might even have changed from HDMI to DP or vice versa.

We've also been putting a lot of effort into not depending on previous
state when enabling the outputs, for more reliability. We generally
don't do partial changes, we disable everything and then enable
again. And now you're suggesting we look at some register which for all
we know may have been valid for some other display?

Timeout.


BR,
Jani.


>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >>
>> >> Also some comments in-line, provided you've convinced me first. ;)
>> >>
>> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> >> > ---
>> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
>> >> ++++++++++++++++++++++++++++++++++++++
>> >> >  2 files changed, 48 insertions(+)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> b/drivers/gpu/drm/i915/i915_reg.h
>> >> > index da2d128..85f3beb 100644
>> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>> >> >  					_HSW_AUD_DIG_CNVT_2)
>> >> >  #define DIP_PORT_SEL_MASK		0x3
>> >> >
>> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
>> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
>> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
>> >> > +					 _HSW_AUD_STR_DESC_1,
>> >> 	\
>> >> > +					 _HSW_AUD_STR_DESC_2)
>> >> > +
>> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
>> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
>> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
>> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> >> b/drivers/gpu/drm/i915/intel_audio.c
>> >> > index eddf37f..082f96d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> >> > @@ -235,6 +235,9 @@ static void
>> hsw_audio_codec_enable(struct
>> >> drm_connector *connector,
>> >> >  	const uint8_t *eld = connector->eld;
>> >> >  	uint32_t tmp;
>> >> >  	int len, i;
>> >> > +	int cvt_idx;
>> >> > +	int n_low, n_up, n;
>> >> > +	int base_rate, mul, div, rate;
>> >> >
>> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
>> >> ELD\n",
>> >> >  		      pipe_name(pipe), drm_eld_size(eld));
>> >> > @@ -267,6 +270,21 @@ static void
>> hsw_audio_codec_enable(struct
>> >> drm_connector *connector,
>> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
>> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> >> >
>> >> > +	if ((mode->clock == 297000) ||
>> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
>> >> 1001))) {
>> >> > +		tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
>> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> >> > +		base_rate = tmp & (1 << 14);
>> >> > +		if (base_rate == 0)
>> >> > +			rate = 48000;
>> >> > +		else
>> >> > +			rate = 44100;
>> >> > +		mul = (tmp & (0x7 << 11)) + 1;
>> >> > +		div = (tmp & (0x7 << 8)) + 1;
>> >> > +		rate = rate * mul / div;
>> >> > +	}
>> >>
>> >> This should be abstracted to a separate function.
>> >
>> > Yes. I will do it.
>> >
>> >>
>> >> > +
>> >> >  	/* Enable timestamps */
>> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >> > @@ -276,6 +294,30 @@ static void
>> hsw_audio_codec_enable(struct
>> >> drm_connector *connector,
>> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >> >  	else
>> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> >> > +
>> >> > +	if ((mode->clock != 297000) &&
>> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
>> >> 1001))) {
>> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >> > +	} else {
>> >> > +		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) {
>> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >> > +			n_low = n & 0xfff;
>> >> > +			n_up = (n >> 12) & 0xff;
>> >> > +			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);
>> >> > +		}
>> >> > +	}
>> >>
>> >> Please de-duplicate the copy-paste from patch 2. At the very least
>> you
>> >> should use a helper for finding the parameters from the table.
>> >
>> > OK. I see.
>> >
>> > Regards,
>> > Libin
>> >
>> >>
>> >> > +
>> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >> >  }
>> >> >
>> >> > --
>> >> > 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
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > Hi Jani,
> >
> >> -----Original Message-----
> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> modeset
> >>
> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> > Hi Jani,
> >> >
> >> > Thanks for careful reviewing for all the patches, please
> >> > see my comments.
> >> >
> >> >> -----Original Message-----
> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS
> in
> >> >> modeset
> >> >>
> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> >> > From: Libin Yang <libin.yang@intel.com>
> >> >> >
> >> >> > When modeset occurs and the TMDS frequency is set to some
> >> >> > speical value, the N/CTS need to be set manually if audio
> >> >> > is playing.
> >> >>
> >> >> When modeset occurs, we disable everything, and then re-enable
> >> >> everything, and notify the audio driver every step of the way. IIUC
> >> >> there should be no audio playing across the modeset, and when
> the
> >> >> modeset is complete and we've set the valid ELD, the audio driver
> >> >> should
> >> >> call the callback from the earlier patches to set the correct audio
> >> >> rate.
> >> >>
> >> >> Why is this patch needed? Please enlighten me.
> >> >
> >> > Please image this scenario: when audio is playing, the gfx driver
> >> > does the modeset. In this situation, after modeset, audio driver
> >> > will do nothing and continue playing. And audio driver will not call
> >> > set_ncts.
> >>
> >> Why does the audio driver not do anything here? Whenever there's
> a
> >> modeset, the graphics driver will disable audio and invalidate the
> ELD,
> >> which should cause an interrupt to notify the audio driver about
> >> this. This is no different from a hot-unplug, in fact I can't see how
> >> the audio driver could even detect whether there's been a hotplug
> or
> >> not
> >> when there's a codec disable/enable.
> >
> > Yes, it will trigger an interrupt to audio driver when unplug
> > and another interrupt for hotplug. But from audio driver,
> > we will not stop the playing and resume the playing based
> > on the hotplug or modeset. The audio is always playing during
> > the hotplug/modeset.
> 
> But the whole display, and consequently ELD, might have changed
> during
> the hotplug, why do you assume you can just keep playing?! The
> display
> might even have changed from HDMI to DP or vice versa.

The eld info changing will not impact the audio playing.
Actually, you can image the monitor as a digital speaker, just
like the headphone to the analog audio. Unplug the speaker
will not impact on the audio playing. Of course , there is a
little difference, when monitor hotplug, in the interrupt,
we may change the codec configuration dynamically. 

And audio driver supply the mechanism to stop the
audio playing when hotplug if the HW requires.

> 
> We've also been putting a lot of effort into not depending on previous
> state when enabling the outputs, for more reliability. We generally
> don't do partial changes, we disable everything and then enable
> again. And now you're suggesting we look at some register which for all
> we know may have been valid for some other display?

In the patch, it will not depend on previous state, I think. We
read the register value which is based on the state after modeset.

> 
> Timeout.
> 
> 
> BR,
> Jani.
> 
> 
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> >>
> >> >> Also some comments in-line, provided you've convinced me first. ;)
> >> >>
> >> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> >> >> > ---
> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
> >> >> ++++++++++++++++++++++++++++++++++++++
> >> >> >  2 files changed, 48 insertions(+)
> >> >> >
> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> >> > index da2d128..85f3beb 100644
> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >> >> >
> 	_HSW_AUD_DIG_CNVT_2)
> >> >> >  #define DIP_PORT_SEL_MASK		0x3
> >> >> >
> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> >> >> > +
> _HSW_AUD_STR_DESC_1,
> >> >> 	\
> >> >> > +
> _HSW_AUD_STR_DESC_2)
> >> >> > +
> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> >> > index eddf37f..082f96d 100644
> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> >> > @@ -235,6 +235,9 @@ static void
> >> hsw_audio_codec_enable(struct
> >> >> drm_connector *connector,
> >> >> >  	const uint8_t *eld = connector->eld;
> >> >> >  	uint32_t tmp;
> >> >> >  	int len, i;
> >> >> > +	int cvt_idx;
> >> >> > +	int n_low, n_up, n;
> >> >> > +	int base_rate, mul, div, rate;
> >> >> >
> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
> bytes
> >> >> ELD\n",
> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
> >> >> > @@ -267,6 +270,21 @@ static void
> >> hsw_audio_codec_enable(struct
> >> >> drm_connector *connector,
> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >> >> >
> >> >> > +	if ((mode->clock == 297000) ||
> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
> >> >> 1001))) {
> >> >> > +		tmp =
> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> >> >> > +		base_rate = tmp & (1 << 14);
> >> >> > +		if (base_rate == 0)
> >> >> > +			rate = 48000;
> >> >> > +		else
> >> >> > +			rate = 44100;
> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
> >> >> > +		rate = rate * mul / div;
> >> >> > +	}
> >> >>
> >> >> This should be abstracted to a separate function.
> >> >
> >> > Yes. I will do it.
> >> >
> >> >>
> >> >> > +
> >> >> >  	/* Enable timestamps */
> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> >> > @@ -276,6 +294,30 @@ static void
> >> hsw_audio_codec_enable(struct
> >> >> drm_connector *connector,
> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >> >  	else
> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >> >> > +
> >> >> > +	if ((mode->clock != 297000) &&
> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
> >> >> 1001))) {
> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >> >> > +	} else {
> >> >> > +		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) {
> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> >> > +			n_low = n & 0xfff;
> >> >> > +			n_up = (n >> 12) & 0xff;
> >> >> > +			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);
> >> >> > +		}
> >> >> > +	}
> >> >>
> >> >> Please de-duplicate the copy-paste from patch 2. At the very least
> >> you
> >> >> should use a helper for finding the parameters from the table.
> >> >
> >> > OK. I see.
> >> >
> >> > Regards,
> >> > Libin
> >> >
> >> >>
> >> >> > +
> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >> >  }
> >> >> >
> >> >> > --
> >> >> > 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
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
>> modeset
>> 
>> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> > Hi Jani,
>> >
>> >> -----Original Message-----
>> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
>> >> modeset
>> >>
>> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> >> > Hi Jani,
>> >> >
>> >> > Thanks for careful reviewing for all the patches, please
>> >> > see my comments.
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS
>> in
>> >> >> modeset
>> >> >>
>> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> >> >> > From: Libin Yang <libin.yang@intel.com>
>> >> >> >
>> >> >> > When modeset occurs and the TMDS frequency is set to some
>> >> >> > speical value, the N/CTS need to be set manually if audio
>> >> >> > is playing.
>> >> >>
>> >> >> When modeset occurs, we disable everything, and then re-enable
>> >> >> everything, and notify the audio driver every step of the way. IIUC
>> >> >> there should be no audio playing across the modeset, and when
>> the
>> >> >> modeset is complete and we've set the valid ELD, the audio driver
>> >> >> should
>> >> >> call the callback from the earlier patches to set the correct audio
>> >> >> rate.
>> >> >>
>> >> >> Why is this patch needed? Please enlighten me.
>> >> >
>> >> > Please image this scenario: when audio is playing, the gfx driver
>> >> > does the modeset. In this situation, after modeset, audio driver
>> >> > will do nothing and continue playing. And audio driver will not call
>> >> > set_ncts.
>> >>
>> >> Why does the audio driver not do anything here? Whenever there's
>> a
>> >> modeset, the graphics driver will disable audio and invalidate the
>> ELD,
>> >> which should cause an interrupt to notify the audio driver about
>> >> this. This is no different from a hot-unplug, in fact I can't see how
>> >> the audio driver could even detect whether there's been a hotplug
>> or
>> >> not
>> >> when there's a codec disable/enable.
>> >
>> > Yes, it will trigger an interrupt to audio driver when unplug
>> > and another interrupt for hotplug. But from audio driver,
>> > we will not stop the playing and resume the playing based
>> > on the hotplug or modeset. The audio is always playing during
>> > the hotplug/modeset.
>> 
>> But the whole display, and consequently ELD, might have changed
>> during
>> the hotplug, why do you assume you can just keep playing?! The
>> display
>> might even have changed from HDMI to DP or vice versa.
>
> The eld info changing will not impact the audio playing.
> Actually, you can image the monitor as a digital speaker, just
> like the headphone to the analog audio. Unplug the speaker
> will not impact on the audio playing. Of course , there is a
> little difference, when monitor hotplug, in the interrupt,
> we may change the codec configuration dynamically. 
>
> And audio driver supply the mechanism to stop the
> audio playing when hotplug if the HW requires.
>
>> 
>> We've also been putting a lot of effort into not depending on previous
>> state when enabling the outputs, for more reliability. We generally
>> don't do partial changes, we disable everything and then enable
>> again. And now you're suggesting we look at some register which for all
>> we know may have been valid for some other display?
>
> In the patch, it will not depend on previous state, I think. We
> read the register value which is based on the state after modeset.

Okay, let's have the patches cleaned up and see. It'll be easier and
quicker to solicit more review on that than this expanding thread.

Please find one more code comment below.

>
>> 
>> Timeout.
>> 
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >> >
>> >> >>
>> >> >> Also some comments in-line, provided you've convinced me first. ;)
>> >> >>
>> >> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> >> >> > ---
>> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
>> >> >> ++++++++++++++++++++++++++++++++++++++
>> >> >> >  2 files changed, 48 insertions(+)
>> >> >> >
>> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> > index da2d128..85f3beb 100644
>> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>> >> >> >
>> 	_HSW_AUD_DIG_CNVT_2)
>> >> >> >  #define DIP_PORT_SEL_MASK		0x3
>> >> >> >
>> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
>> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
>> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
>> >> >> > +
>> _HSW_AUD_STR_DESC_1,
>> >> >> 	\
>> >> >> > +
>> _HSW_AUD_STR_DESC_2)
>> >> >> > +
>> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
>> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
>> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
>> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> > index eddf37f..082f96d 100644
>> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> > @@ -235,6 +235,9 @@ static void
>> >> hsw_audio_codec_enable(struct
>> >> >> drm_connector *connector,
>> >> >> >  	const uint8_t *eld = connector->eld;
>> >> >> >  	uint32_t tmp;
>> >> >> >  	int len, i;
>> >> >> > +	int cvt_idx;
>> >> >> > +	int n_low, n_up, n;
>> >> >> > +	int base_rate, mul, div, rate;
>> >> >> >
>> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
>> bytes
>> >> >> ELD\n",
>> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
>> >> >> > @@ -267,6 +270,21 @@ static void
>> >> hsw_audio_codec_enable(struct
>> >> >> drm_connector *connector,
>> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
>> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> >> >> >
>> >> >> > +	if ((mode->clock == 297000) ||
>> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
>> >> >> 1001))) {

Please abstract that condition into a helper and give it a helpful
name. That's repeated many times now, in this and negated forms, and I
want the compiler to ensure it's the same in each place.

Thanks,
Jani.

>> >> >> > +		tmp =
>> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
>> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> >> >> > +		base_rate = tmp & (1 << 14);
>> >> >> > +		if (base_rate == 0)
>> >> >> > +			rate = 48000;
>> >> >> > +		else
>> >> >> > +			rate = 44100;
>> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
>> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
>> >> >> > +		rate = rate * mul / div;
>> >> >> > +	}
>> >> >>
>> >> >> This should be abstracted to a separate function.
>> >> >
>> >> > Yes. I will do it.
>> >> >
>> >> >>
>> >> >> > +
>> >> >> >  	/* Enable timestamps */
>> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >> >> > @@ -276,6 +294,30 @@ static void
>> >> hsw_audio_codec_enable(struct
>> >> >> drm_connector *connector,
>> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >> >> >  	else
>> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> >> >> > +
>> >> >> > +	if ((mode->clock != 297000) &&
>> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
>> >> >> 1001))) {
>> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >> >> > +	} else {
>> >> >> > +		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) {
>> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >> >> > +			n_low = n & 0xfff;
>> >> >> > +			n_up = (n >> 12) & 0xff;
>> >> >> > +			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);
>> >> >> > +		}
>> >> >> > +	}
>> >> >>
>> >> >> Please de-duplicate the copy-paste from patch 2. At the very least
>> >> you
>> >> >> should use a helper for finding the parameters from the table.
>> >> >
>> >> > OK. I see.
>> >> >
>> >> > Regards,
>> >> > Libin
>> >> >
>> >> >>
>> >> >> > +
>> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >> >> >  }
>> >> >> >
>> >> >> > --
>> >> >> > 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
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> 
>> --
>> Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Jani,

> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Wednesday, August 12, 2015 2:20 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 v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > Hi Jani,
> >
> >> -----Original Message-----
> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> modeset
> >>
> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> > Hi Jani,
> >> >
> >> >> -----Original Message-----
> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS
> in
> >> >> modeset
> >> >>
> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> >> > Hi Jani,
> >> >> >
> >> >> > Thanks for careful reviewing for all the patches, please
> >> >> > see my comments.
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper
> N/CTS
> >> in
> >> >> >> modeset
> >> >> >>
> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> >> >> > From: Libin Yang <libin.yang@intel.com>
> >> >> >> >
> >> >> >> > When modeset occurs and the TMDS frequency is set to
> some
> >> >> >> > speical value, the N/CTS need to be set manually if audio
> >> >> >> > is playing.
> >> >> >>
> >> >> >> When modeset occurs, we disable everything, and then re-
> enable
> >> >> >> everything, and notify the audio driver every step of the way.
> IIUC
> >> >> >> there should be no audio playing across the modeset, and
> when
> >> the
> >> >> >> modeset is complete and we've set the valid ELD, the audio
> driver
> >> >> >> should
> >> >> >> call the callback from the earlier patches to set the correct
> audio
> >> >> >> rate.
> >> >> >>
> >> >> >> Why is this patch needed? Please enlighten me.
> >> >> >
> >> >> > Please image this scenario: when audio is playing, the gfx driver
> >> >> > does the modeset. In this situation, after modeset, audio driver
> >> >> > will do nothing and continue playing. And audio driver will not
> call
> >> >> > set_ncts.
> >> >>
> >> >> Why does the audio driver not do anything here? Whenever
> there's
> >> a
> >> >> modeset, the graphics driver will disable audio and invalidate the
> >> ELD,
> >> >> which should cause an interrupt to notify the audio driver about
> >> >> this. This is no different from a hot-unplug, in fact I can't see how
> >> >> the audio driver could even detect whether there's been a
> hotplug
> >> or
> >> >> not
> >> >> when there's a codec disable/enable.
> >> >
> >> > Yes, it will trigger an interrupt to audio driver when unplug
> >> > and another interrupt for hotplug. But from audio driver,
> >> > we will not stop the playing and resume the playing based
> >> > on the hotplug or modeset. The audio is always playing during
> >> > the hotplug/modeset.
> >>
> >> But the whole display, and consequently ELD, might have changed
> >> during
> >> the hotplug, why do you assume you can just keep playing?! The
> >> display
> >> might even have changed from HDMI to DP or vice versa.
> >
> > The eld info changing will not impact the audio playing.
> > Actually, you can image the monitor as a digital speaker, just
> > like the headphone to the analog audio. Unplug the speaker
> > will not impact on the audio playing. Of course , there is a
> > little difference, when monitor hotplug, in the interrupt,
> > we may change the codec configuration dynamically.
> >
> > And audio driver supply the mechanism to stop the
> > audio playing when hotplug if the HW requires.
> >
> >>
> >> We've also been putting a lot of effort into not depending on
> previous
> >> state when enabling the outputs, for more reliability. We generally
> >> don't do partial changes, we disable everything and then enable
> >> again. And now you're suggesting we look at some register which for
> all
> >> we know may have been valid for some other display?
> >
> > In the patch, it will not depend on previous state, I think. We
> > read the register value which is based on the state after modeset.
> 
> Okay, let's have the patches cleaned up and see. It'll be easier and
> quicker to solicit more review on that than this expanding thread.

OK. Thanks.

> 
> Please find one more code comment below.
> 
> >
> >>
> >> Timeout.
> >>
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> >
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >> >
> >> >> >>
> >> >> >> Also some comments in-line, provided you've convinced me
> first. ;)
> >> >> >>
> >> >> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> >> >> >> > ---
> >> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
> >> >> >> ++++++++++++++++++++++++++++++++++++++
> >> >> >> >  2 files changed, 48 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > index da2d128..85f3beb 100644
> >> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >> >> >> >
> >> 	_HSW_AUD_DIG_CNVT_2)
> >> >> >> >  #define DIP_PORT_SEL_MASK		0x3
> >> >> >> >
> >> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
> >> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
> >> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> >> >> >> > +
> >> _HSW_AUD_STR_DESC_1,
> >> >> >> 	\
> >> >> >> > +
> >> _HSW_AUD_STR_DESC_2)
> >> >> >> > +
> >> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> >> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > index eddf37f..082f96d 100644
> >> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > @@ -235,6 +235,9 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  	const uint8_t *eld = connector->eld;
> >> >> >> >  	uint32_t tmp;
> >> >> >> >  	int len, i;
> >> >> >> > +	int cvt_idx;
> >> >> >> > +	int n_low, n_up, n;
> >> >> >> > +	int base_rate, mul, div, rate;
> >> >> >> >
> >> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
> >> bytes
> >> >> >> ELD\n",
> >> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
> >> >> >> > @@ -267,6 +270,21 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >> >> >> >
> >> >> >> > +	if ((mode->clock == 297000) ||
> >> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
> >> >> >> 1001))) {
> 
> Please abstract that condition into a helper and give it a helpful
> name. That's repeated many times now, in this and negated forms, and
> I
> want the compiler to ensure it's the same in each place.

OK. I will do it.

> 
> Thanks,
> Jani.
> 
> >> >> >> > +		tmp =
> >> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> >> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> >> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> >> >> >> > +		base_rate = tmp & (1 << 14);
> >> >> >> > +		if (base_rate == 0)
> >> >> >> > +			rate = 48000;
> >> >> >> > +		else
> >> >> >> > +			rate = 44100;
> >> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
> >> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
> >> >> >> > +		rate = rate * mul / div;
> >> >> >> > +	}
> >> >> >>
> >> >> >> This should be abstracted to a separate function.
> >> >> >
> >> >> > Yes. I will do it.
> >> >> >
> >> >> >>
> >> >> >> > +
> >> >> >> >  	/* Enable timestamps */
> >> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> >> >> > @@ -276,6 +294,30 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >> >> >  	else
> >> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >> >> >> > +
> >> >> >> > +	if ((mode->clock != 297000) &&
> >> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
> >> >> >> 1001))) {
> >> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >> >> >> > +	} else {
> >> >> >> > +		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) {
> >> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> >> >> > +			n_low = n & 0xfff;
> >> >> >> > +			n_up = (n >> 12) & 0xff;
> >> >> >> > +			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);
> >> >> >> > +		}
> >> >> >> > +	}
> >> >> >>
> >> >> >> Please de-duplicate the copy-paste from patch 2. At the very
> least
> >> >> you
> >> >> >> should use a helper for finding the parameters from the table.
> >> >> >
> >> >> > OK. I see.
> >> >> >
> >> >> > Regards,
> >> >> > Libin
> >> >> >
> >> >> >>
> >> >> >> > +
> >> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >> >> >  }
> >> >> >> >
> >> >> >> > --
> >> >> >> > 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
> >> >>
> >> >> --
> >> >> Jani Nikula, Intel Open Source Technology Center
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> 
> --
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Daniel Vetter]

On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > Hi Jani,
> >
> >> -----Original Message-----
> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> modeset
> >> 
> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> > Hi Jani,
> >> >
> >> >> -----Original Message-----
> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> >> modeset
> >> >>
> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> >> > Hi Jani,
> >> >> >
> >> >> > Thanks for careful reviewing for all the patches, please
> >> >> > see my comments.
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS
> >> in
> >> >> >> modeset
> >> >> >>
> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> >> >> > From: Libin Yang <libin.yang@intel.com>
> >> >> >> >
> >> >> >> > When modeset occurs and the TMDS frequency is set to some
> >> >> >> > speical value, the N/CTS need to be set manually if audio
> >> >> >> > is playing.
> >> >> >>
> >> >> >> When modeset occurs, we disable everything, and then re-enable
> >> >> >> everything, and notify the audio driver every step of the way. IIUC
> >> >> >> there should be no audio playing across the modeset, and when
> >> the
> >> >> >> modeset is complete and we've set the valid ELD, the audio driver
> >> >> >> should
> >> >> >> call the callback from the earlier patches to set the correct audio
> >> >> >> rate.
> >> >> >>
> >> >> >> Why is this patch needed? Please enlighten me.
> >> >> >
> >> >> > Please image this scenario: when audio is playing, the gfx driver
> >> >> > does the modeset. In this situation, after modeset, audio driver
> >> >> > will do nothing and continue playing. And audio driver will not call
> >> >> > set_ncts.
> >> >>
> >> >> Why does the audio driver not do anything here? Whenever there's
> >> a
> >> >> modeset, the graphics driver will disable audio and invalidate the
> >> ELD,
> >> >> which should cause an interrupt to notify the audio driver about
> >> >> this. This is no different from a hot-unplug, in fact I can't see how
> >> >> the audio driver could even detect whether there's been a hotplug
> >> or
> >> >> not
> >> >> when there's a codec disable/enable.
> >> >
> >> > Yes, it will trigger an interrupt to audio driver when unplug
> >> > and another interrupt for hotplug. But from audio driver,
> >> > we will not stop the playing and resume the playing based
> >> > on the hotplug or modeset. The audio is always playing during
> >> > the hotplug/modeset.
> >> 
> >> But the whole display, and consequently ELD, might have changed
> >> during
> >> the hotplug, why do you assume you can just keep playing?! The
> >> display
> >> might even have changed from HDMI to DP or vice versa.
> >
> > The eld info changing will not impact the audio playing.
> > Actually, you can image the monitor as a digital speaker, just
> > like the headphone to the analog audio. Unplug the speaker
> > will not impact on the audio playing. Of course , there is a
> > little difference, when monitor hotplug, in the interrupt,
> > we may change the codec configuration dynamically. 
> >
> > And audio driver supply the mechanism to stop the
> > audio playing when hotplug if the HW requires.
> >
> >> 
> >> We've also been putting a lot of effort into not depending on previous
> >> state when enabling the outputs, for more reliability. We generally
> >> don't do partial changes, we disable everything and then enable
> >> again. And now you're suggesting we look at some register which for all
> >> we know may have been valid for some other display?
> >
> > In the patch, it will not depend on previous state, I think. We
> > read the register value which is based on the state after modeset.
> 
> Okay, let's have the patches cleaned up and see. It'll be easier and
> quicker to solicit more review on that than this expanding thread.

It sounds like we actually need to ping the audio side _before_ we do the
hw modeset while computing the pipe_config. That would probably take care
of my rwm concerns, give this thing proper structure and also make sure
N/CTS never get out of sync. Also then we can track this stuff in the
pipe_config and throw the state checker at it to make sure it's all fine.
-Daniel

> 
> Please find one more code comment below.
> 
> >
> >> 
> >> Timeout.
> >> 
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >> >
> >> >> >>
> >> >> >> Also some comments in-line, provided you've convinced me first. ;)
> >> >> >>
> >> >> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
> >> >> >> > ---
> >> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
> >> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
> >> >> >> ++++++++++++++++++++++++++++++++++++++
> >> >> >> >  2 files changed, 48 insertions(+)
> >> >> >> >
> >> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> b/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > index da2d128..85f3beb 100644
> >> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
> >> >> >> >
> >> 	_HSW_AUD_DIG_CNVT_2)
> >> >> >> >  #define DIP_PORT_SEL_MASK		0x3
> >> >> >> >
> >> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
> >> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
> >> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
> >> >> >> > +
> >> _HSW_AUD_STR_DESC_1,
> >> >> >> 	\
> >> >> >> > +
> >> _HSW_AUD_STR_DESC_2)
> >> >> >> > +
> >> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
> >> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
> >> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
> >> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> b/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > index eddf37f..082f96d 100644
> >> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> >> >> >> > @@ -235,6 +235,9 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  	const uint8_t *eld = connector->eld;
> >> >> >> >  	uint32_t tmp;
> >> >> >> >  	int len, i;
> >> >> >> > +	int cvt_idx;
> >> >> >> > +	int n_low, n_up, n;
> >> >> >> > +	int base_rate, mul, div, rate;
> >> >> >> >
> >> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
> >> bytes
> >> >> >> ELD\n",
> >> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
> >> >> >> > @@ -267,6 +270,21 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
> >> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> >> >> >> >
> >> >> >> > +	if ((mode->clock == 297000) ||
> >> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
> >> >> >> 1001))) {
> 
> Please abstract that condition into a helper and give it a helpful
> name. That's repeated many times now, in this and negated forms, and I
> want the compiler to ensure it's the same in each place.
> 
> Thanks,
> Jani.
> 
> >> >> >> > +		tmp =
> >> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> >> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
> >> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
> >> >> >> > +		base_rate = tmp & (1 << 14);
> >> >> >> > +		if (base_rate == 0)
> >> >> >> > +			rate = 48000;
> >> >> >> > +		else
> >> >> >> > +			rate = 44100;
> >> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
> >> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
> >> >> >> > +		rate = rate * mul / div;
> >> >> >> > +	}
> >> >> >>
> >> >> >> This should be abstracted to a separate function.
> >> >> >
> >> >> > Yes. I will do it.
> >> >> >
> >> >> >>
> >> >> >> > +
> >> >> >> >  	/* Enable timestamps */
> >> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
> >> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
> >> >> >> > @@ -276,6 +294,30 @@ static void
> >> >> hsw_audio_codec_enable(struct
> >> >> >> drm_connector *connector,
> >> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
> >> >> >> >  	else
> >> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
> >> >> >> > +
> >> >> >> > +	if ((mode->clock != 297000) &&
> >> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
> >> >> >> 1001))) {
> >> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> >> >> >> > +	} else {
> >> >> >> > +		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) {
> >> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
> >> >> >> > +			n_low = n & 0xfff;
> >> >> >> > +			n_up = (n >> 12) & 0xff;
> >> >> >> > +			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);
> >> >> >> > +		}
> >> >> >> > +	}
> >> >> >>
> >> >> >> Please de-duplicate the copy-paste from patch 2. At the very least
> >> >> you
> >> >> >> should use a helper for finding the parameters from the table.
> >> >> >
> >> >> > OK. I see.
> >> >> >
> >> >> > Regards,
> >> >> > Libin
> >> >> >
> >> >> >>
> >> >> >> > +
> >> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >> >> >> >  }
> >> >> >> >
> >> >> >> > --
> >> >> >> > 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
> >> >>
> >> >> --
> >> >> Jani Nikula, Intel Open Source Technology Center
> >> 
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Wed, 12 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
>> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> > Hi Jani,
>> >
>> >> -----Original Message-----
>> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
>> >> modeset
>> >> 
>> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> >> > Hi Jani,
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
>> >> >> modeset
>> >> >>
>> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
>> >> >> > Hi Jani,
>> >> >> >
>> >> >> > Thanks for careful reviewing for all the patches, please
>> >> >> > see my comments.
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
>> >> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS
>> >> in
>> >> >> >> modeset
>> >> >> >>
>> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
>> >> >> >> > From: Libin Yang <libin.yang@intel.com>
>> >> >> >> >
>> >> >> >> > When modeset occurs and the TMDS frequency is set to some
>> >> >> >> > speical value, the N/CTS need to be set manually if audio
>> >> >> >> > is playing.
>> >> >> >>
>> >> >> >> When modeset occurs, we disable everything, and then re-enable
>> >> >> >> everything, and notify the audio driver every step of the way. IIUC
>> >> >> >> there should be no audio playing across the modeset, and when
>> >> the
>> >> >> >> modeset is complete and we've set the valid ELD, the audio driver
>> >> >> >> should
>> >> >> >> call the callback from the earlier patches to set the correct audio
>> >> >> >> rate.
>> >> >> >>
>> >> >> >> Why is this patch needed? Please enlighten me.
>> >> >> >
>> >> >> > Please image this scenario: when audio is playing, the gfx driver
>> >> >> > does the modeset. In this situation, after modeset, audio driver
>> >> >> > will do nothing and continue playing. And audio driver will not call
>> >> >> > set_ncts.
>> >> >>
>> >> >> Why does the audio driver not do anything here? Whenever there's
>> >> a
>> >> >> modeset, the graphics driver will disable audio and invalidate the
>> >> ELD,
>> >> >> which should cause an interrupt to notify the audio driver about
>> >> >> this. This is no different from a hot-unplug, in fact I can't see how
>> >> >> the audio driver could even detect whether there's been a hotplug
>> >> or
>> >> >> not
>> >> >> when there's a codec disable/enable.
>> >> >
>> >> > Yes, it will trigger an interrupt to audio driver when unplug
>> >> > and another interrupt for hotplug. But from audio driver,
>> >> > we will not stop the playing and resume the playing based
>> >> > on the hotplug or modeset. The audio is always playing during
>> >> > the hotplug/modeset.
>> >> 
>> >> But the whole display, and consequently ELD, might have changed
>> >> during
>> >> the hotplug, why do you assume you can just keep playing?! The
>> >> display
>> >> might even have changed from HDMI to DP or vice versa.
>> >
>> > The eld info changing will not impact the audio playing.
>> > Actually, you can image the monitor as a digital speaker, just
>> > like the headphone to the analog audio. Unplug the speaker
>> > will not impact on the audio playing. Of course , there is a
>> > little difference, when monitor hotplug, in the interrupt,
>> > we may change the codec configuration dynamically. 
>> >
>> > And audio driver supply the mechanism to stop the
>> > audio playing when hotplug if the HW requires.
>> >
>> >> 
>> >> We've also been putting a lot of effort into not depending on previous
>> >> state when enabling the outputs, for more reliability. We generally
>> >> don't do partial changes, we disable everything and then enable
>> >> again. And now you're suggesting we look at some register which for all
>> >> we know may have been valid for some other display?
>> >
>> > In the patch, it will not depend on previous state, I think. We
>> > read the register value which is based on the state after modeset.
>> 
>> Okay, let's have the patches cleaned up and see. It'll be easier and
>> quicker to solicit more review on that than this expanding thread.
>
> It sounds like we actually need to ping the audio side _before_ we do the

Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?

Jani.

> hw modeset while computing the pipe_config. That would probably take care
> of my rwm concerns, give this thing proper structure and also make sure
> N/CTS never get out of sync. Also then we can track this stuff in the
> pipe_config and throw the state checker at it to make sure it's all fine.
> -Daniel
>
>> 
>> Please find one more code comment below.
>> 
>> >
>> >> 
>> >> Timeout.
>> >> 
>> >> 
>> >> BR,
>> >> Jani.
>> >> 
>> >> 
>> >> >
>> >> >>
>> >> >> BR,
>> >> >> Jani.
>> >> >>
>> >> >>
>> >> >> >
>> >> >> >>
>> >> >> >> Also some comments in-line, provided you've convinced me first. ;)
>> >> >> >>
>> >> >> >> > Signed-off-by: Libin Yang <libin.yang@intel.com>
>> >> >> >> > ---
>> >> >> >> >  drivers/gpu/drm/i915/i915_reg.h    |  6 ++++++
>> >> >> >> >  drivers/gpu/drm/i915/intel_audio.c | 42
>> >> >> >> ++++++++++++++++++++++++++++++++++++++
>> >> >> >> >  2 files changed, 48 insertions(+)
>> >> >> >> >
>> >> >> >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > index da2d128..85f3beb 100644
>> >> >> >> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> >> >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
>> >> >> >> >
>> >> 	_HSW_AUD_DIG_CNVT_2)
>> >> >> >> >  #define DIP_PORT_SEL_MASK		0x3
>> >> >> >> >
>> >> >> >> > +#define _HSW_AUD_STR_DESC_1		0x65084
>> >> >> >> > +#define _HSW_AUD_STR_DESC_2		0x65184
>> >> >> >> > +#define AUD_STR_DESC(pipe)	_PIPE(pipe, \
>> >> >> >> > +
>> >> _HSW_AUD_STR_DESC_1,
>> >> >> >> 	\
>> >> >> >> > +
>> >> _HSW_AUD_STR_DESC_2)
>> >> >> >> > +
>> >> >> >> >  #define _HSW_AUD_EDID_DATA_A		0x65050
>> >> >> >> >  #define _HSW_AUD_EDID_DATA_B		0x65150
>> >> >> >> >  #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \
>> >> >> >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > index eddf37f..082f96d 100644
>> >> >> >> > --- a/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > +++ b/drivers/gpu/drm/i915/intel_audio.c
>> >> >> >> > @@ -235,6 +235,9 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  	const uint8_t *eld = connector->eld;
>> >> >> >> >  	uint32_t tmp;
>> >> >> >> >  	int len, i;
>> >> >> >> > +	int cvt_idx;
>> >> >> >> > +	int n_low, n_up, n;
>> >> >> >> > +	int base_rate, mul, div, rate;
>> >> >> >> >
>> >> >> >> >  	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
>> >> bytes
>> >> >> >> ELD\n",
>> >> >> >> >  		      pipe_name(pipe), drm_eld_size(eld));
>> >> >> >> > @@ -267,6 +270,21 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  	tmp |= AUDIO_ELD_VALID(pipe);
>> >> >> >> >  	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
>> >> >> >> >
>> >> >> >> > +	if ((mode->clock == 297000) ||
>> >> >> >> > +		(mode->clock == DIV_ROUND_UP(297000 * 1000,
>> >> >> >> 1001))) {
>> 
>> Please abstract that condition into a helper and give it a helpful
>> name. That's repeated many times now, in this and negated forms, and I
>> want the compiler to ensure it's the same in each place.
>> 
>> Thanks,
>> Jani.
>> 
>> >> >> >> > +		tmp =
>> >> I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> >> >> >> > +		cvt_idx = (tmp >> pipe*8) & 0xff;
>> >> >> >> > +		tmp = I915_READ(AUD_STR_DESC(cvt_idx));
>> >> >> >> > +		base_rate = tmp & (1 << 14);
>> >> >> >> > +		if (base_rate == 0)
>> >> >> >> > +			rate = 48000;
>> >> >> >> > +		else
>> >> >> >> > +			rate = 44100;
>> >> >> >> > +		mul = (tmp & (0x7 << 11)) + 1;
>> >> >> >> > +		div = (tmp & (0x7 << 8)) + 1;
>> >> >> >> > +		rate = rate * mul / div;
>> >> >> >> > +	}
>> >> >> >>
>> >> >> >> This should be abstracted to a separate function.
>> >> >> >
>> >> >> > Yes. I will do it.
>> >> >> >
>> >> >> >>
>> >> >> >> > +
>> >> >> >> >  	/* Enable timestamps */
>> >> >> >> >  	tmp = I915_READ(HSW_AUD_CFG(pipe));
>> >> >> >> >  	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
>> >> >> >> > @@ -276,6 +294,30 @@ static void
>> >> >> hsw_audio_codec_enable(struct
>> >> >> >> drm_connector *connector,
>> >> >> >> >  		tmp |= AUD_CONFIG_N_VALUE_INDEX;
>> >> >> >> >  	else
>> >> >> >> >  		tmp |= audio_config_hdmi_pixel_clock(mode);
>> >> >> >> > +
>> >> >> >> > +	if ((mode->clock != 297000) &&
>> >> >> >> > +		(mode->clock != DIV_ROUND_UP(297000 * 1000,
>> >> >> >> 1001))) {
>> >> >> >> > +		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
>> >> >> >> > +	} else {
>> >> >> >> > +		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) {
>> >> >> >> > +			tmp |= AUD_CONFIG_N_PROG_ENABLE;
>> >> >> >> > +			n_low = n & 0xfff;
>> >> >> >> > +			n_up = (n >> 12) & 0xff;
>> >> >> >> > +			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);
>> >> >> >> > +		}
>> >> >> >> > +	}
>> >> >> >>
>> >> >> >> Please de-duplicate the copy-paste from patch 2. At the very least
>> >> >> you
>> >> >> >> should use a helper for finding the parameters from the table.
>> >> >> >
>> >> >> > OK. I see.
>> >> >> >
>> >> >> > Regards,
>> >> >> > Libin
>> >> >> >
>> >> >> >>
>> >> >> >> > +
>> >> >> >> >  	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>> >> >> >> >  }
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > 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
>> >> >>
>> >> >> --
>> >> >> Jani Nikula, Intel Open Source Technology Center
>> >> 
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Daniel Vetter]

On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
> On Wed, 12 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
> >> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> > Hi Jani,
> >> >
> >> >> -----Original Message-----
> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> >> modeset
> >> >> 
> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> >> > Hi Jani,
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper N/CTS in
> >> >> >> modeset
> >> >> >>
> >> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> >> >> >> > Hi Jani,
> >> >> >> >
> >> >> >> > Thanks for careful reviewing for all the patches, please
> >> >> >> > see my comments.
> >> >> >> >
> >> >> >> >> -----Original Message-----
> >> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> >> >> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set proper N/CTS
> >> >> in
> >> >> >> >> modeset
> >> >> >> >>
> >> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> >> >> >> >> > From: Libin Yang <libin.yang@intel.com>
> >> >> >> >> >
> >> >> >> >> > When modeset occurs and the TMDS frequency is set to some
> >> >> >> >> > speical value, the N/CTS need to be set manually if audio
> >> >> >> >> > is playing.
> >> >> >> >>
> >> >> >> >> When modeset occurs, we disable everything, and then re-enable
> >> >> >> >> everything, and notify the audio driver every step of the way. IIUC
> >> >> >> >> there should be no audio playing across the modeset, and when
> >> >> the
> >> >> >> >> modeset is complete and we've set the valid ELD, the audio driver
> >> >> >> >> should
> >> >> >> >> call the callback from the earlier patches to set the correct audio
> >> >> >> >> rate.
> >> >> >> >>
> >> >> >> >> Why is this patch needed? Please enlighten me.
> >> >> >> >
> >> >> >> > Please image this scenario: when audio is playing, the gfx driver
> >> >> >> > does the modeset. In this situation, after modeset, audio driver
> >> >> >> > will do nothing and continue playing. And audio driver will not call
> >> >> >> > set_ncts.
> >> >> >>
> >> >> >> Why does the audio driver not do anything here? Whenever there's
> >> >> a
> >> >> >> modeset, the graphics driver will disable audio and invalidate the
> >> >> ELD,
> >> >> >> which should cause an interrupt to notify the audio driver about
> >> >> >> this. This is no different from a hot-unplug, in fact I can't see how
> >> >> >> the audio driver could even detect whether there's been a hotplug
> >> >> or
> >> >> >> not
> >> >> >> when there's a codec disable/enable.
> >> >> >
> >> >> > Yes, it will trigger an interrupt to audio driver when unplug
> >> >> > and another interrupt for hotplug. But from audio driver,
> >> >> > we will not stop the playing and resume the playing based
> >> >> > on the hotplug or modeset. The audio is always playing during
> >> >> > the hotplug/modeset.
> >> >> 
> >> >> But the whole display, and consequently ELD, might have changed
> >> >> during
> >> >> the hotplug, why do you assume you can just keep playing?! The
> >> >> display
> >> >> might even have changed from HDMI to DP or vice versa.
> >> >
> >> > The eld info changing will not impact the audio playing.
> >> > Actually, you can image the monitor as a digital speaker, just
> >> > like the headphone to the analog audio. Unplug the speaker
> >> > will not impact on the audio playing. Of course , there is a
> >> > little difference, when monitor hotplug, in the interrupt,
> >> > we may change the codec configuration dynamically. 
> >> >
> >> > And audio driver supply the mechanism to stop the
> >> > audio playing when hotplug if the HW requires.
> >> >
> >> >> 
> >> >> We've also been putting a lot of effort into not depending on previous
> >> >> state when enabling the outputs, for more reliability. We generally
> >> >> don't do partial changes, we disable everything and then enable
> >> >> again. And now you're suggesting we look at some register which for all
> >> >> we know may have been valid for some other display?
> >> >
> >> > In the patch, it will not depend on previous state, I think. We
> >> > read the register value which is based on the state after modeset.
> >> 
> >> Okay, let's have the patches cleaned up and see. It'll be easier and
> >> quicker to solicit more review on that than this expanding thread.
> >
> > It sounds like we actually need to ping the audio side _before_ we do the
> 
> Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?

No, I mean call into snd-hda-intel before we do the modeset to ask it for
what it actually wants to have set up. At least it feels a bit like only
reacting to a modeset after it's done leads to confusion. Or maybe we just
need to update the register values we want at compute_time instead of
doing rwm fun ...

Pure gut feeling comment, I didn't look into details ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Jani Nikula]

On Wed, 12 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
>> On Wed, 12 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > It sounds like we actually need to ping the audio side _before_ we do the
>> 
>> Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?
>
> No, I mean call into snd-hda-intel before we do the modeset to ask it for
> what it actually wants to have set up. At least it feels a bit like only
> reacting to a modeset after it's done leads to confusion. Or maybe we just
> need to update the register values we want at compute_time instead of
> doing rwm fun ...
>
> Pure gut feeling comment, I didn't look into details ;-)

The audio driver calls us, not vice versa...

Jani.



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

Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset

[Yang, Libin]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:43 PM
> To: Jani Nikula
> Cc: Daniel Vetter; Yang, Libin; alsa-devel@alsa-project.org;
> tiwai@suse.de; intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in
> modeset
> 
> On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
> > On Wed, 12 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
> > >> On Wed, 12 Aug 2015, "Yang, Libin" <libin.yang@intel.com> wrote:
> > >> > Hi Jani,
> > >> >
> > >> >> -----Original Message-----
> > >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > >> >> Sent: Tuesday, August 11, 2015 4:14 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 v4 4/4] drm/i915: set proper
> N/CTS in
> > >> >> modeset
> > >> >>
> > >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com>
> wrote:
> > >> >> > Hi Jani,
> > >> >> >
> > >> >> >> -----Original Message-----
> > >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > >> >> >> Sent: Tuesday, August 11, 2015 3:14 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 v4 4/4] drm/i915: set proper
> N/CTS in
> > >> >> >> modeset
> > >> >> >>
> > >> >> >> On Tue, 11 Aug 2015, "Yang, Libin" <libin.yang@intel.com>
> wrote:
> > >> >> >> > Hi Jani,
> > >> >> >> >
> > >> >> >> > Thanks for careful reviewing for all the patches, please
> > >> >> >> > see my comments.
> > >> >> >> >
> > >> >> >> >> -----Original Message-----
> > >> >> >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> > >> >> >> >> Sent: Monday, August 10, 2015 8:10 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 v4 4/4] drm/i915: set
> proper N/CTS
> > >> >> in
> > >> >> >> >> modeset
> > >> >> >> >>
> > >> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
> > >> >> >> >> > From: Libin Yang <libin.yang@intel.com>
> > >> >> >> >> >
> > >> >> >> >> > When modeset occurs and the TMDS frequency is set
> to some
> > >> >> >> >> > speical value, the N/CTS need to be set manually if
> audio
> > >> >> >> >> > is playing.
> > >> >> >> >>
> > >> >> >> >> When modeset occurs, we disable everything, and then
> re-enable
> > >> >> >> >> everything, and notify the audio driver every step of the
> way. IIUC
> > >> >> >> >> there should be no audio playing across the modeset,
> and when
> > >> >> the
> > >> >> >> >> modeset is complete and we've set the valid ELD, the
> audio driver
> > >> >> >> >> should
> > >> >> >> >> call the callback from the earlier patches to set the
> correct audio
> > >> >> >> >> rate.
> > >> >> >> >>
> > >> >> >> >> Why is this patch needed? Please enlighten me.
> > >> >> >> >
> > >> >> >> > Please image this scenario: when audio is playing, the gfx
> driver
> > >> >> >> > does the modeset. In this situation, after modeset, audio
> driver
> > >> >> >> > will do nothing and continue playing. And audio driver will
> not call
> > >> >> >> > set_ncts.
> > >> >> >>
> > >> >> >> Why does the audio driver not do anything here? Whenever
> there's
> > >> >> a
> > >> >> >> modeset, the graphics driver will disable audio and
> invalidate the
> > >> >> ELD,
> > >> >> >> which should cause an interrupt to notify the audio driver
> about
> > >> >> >> this. This is no different from a hot-unplug, in fact I can't see
> how
> > >> >> >> the audio driver could even detect whether there's been a
> hotplug
> > >> >> or
> > >> >> >> not
> > >> >> >> when there's a codec disable/enable.
> > >> >> >
> > >> >> > Yes, it will trigger an interrupt to audio driver when unplug
> > >> >> > and another interrupt for hotplug. But from audio driver,
> > >> >> > we will not stop the playing and resume the playing based
> > >> >> > on the hotplug or modeset. The audio is always playing
> during
> > >> >> > the hotplug/modeset.
> > >> >>
> > >> >> But the whole display, and consequently ELD, might have
> changed
> > >> >> during
> > >> >> the hotplug, why do you assume you can just keep playing?!
> The
> > >> >> display
> > >> >> might even have changed from HDMI to DP or vice versa.
> > >> >
> > >> > The eld info changing will not impact the audio playing.
> > >> > Actually, you can image the monitor as a digital speaker, just
> > >> > like the headphone to the analog audio. Unplug the speaker
> > >> > will not impact on the audio playing. Of course , there is a
> > >> > little difference, when monitor hotplug, in the interrupt,
> > >> > we may change the codec configuration dynamically.
> > >> >
> > >> > And audio driver supply the mechanism to stop the
> > >> > audio playing when hotplug if the HW requires.
> > >> >
> > >> >>
> > >> >> We've also been putting a lot of effort into not depending on
> previous
> > >> >> state when enabling the outputs, for more reliability. We
> generally
> > >> >> don't do partial changes, we disable everything and then
> enable
> > >> >> again. And now you're suggesting we look at some register
> which for all
> > >> >> we know may have been valid for some other display?
> > >> >
> > >> > In the patch, it will not depend on previous state, I think. We
> > >> > read the register value which is based on the state after
> modeset.
> > >>
> > >> Okay, let's have the patches cleaned up and see. It'll be easier and
> > >> quicker to solicit more review on that than this expanding thread.
> > >
> > > It sounds like we actually need to ping the audio side _before_ we
> do the
> >
> > Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio
> side" above?
> 
> No, I mean call into snd-hda-intel before we do the modeset to ask it
> for
> what it actually wants to have set up. At least it feels a bit like only
> reacting to a modeset after it's done leads to confusion. Or maybe we
> just
> need to update the register values we want at compute_time instead
> of
> doing rwm fun ...

Actually we read the _HSW_AUD_STR_DESC_* register to get
what value we are going to set. This is similar with calling audio
driver to ask for the parameters. The _HSW_AUD_STR_DESC_*
will tell us what audio sample rates it's using. :-)

Regards,
Libin
> 
> Pure gut feeling comment, I didn't look into details ;-)
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx