drm/i915/audio: audio cleanups, 4k fixes

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 1/9] drm/i915/audio: abstract audio config update

Prepare for using the same code for updating HSW_AUD_CFG register. No functional changes.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 68 ++++++++++++++++++++++------------
----
 1 file changed, 40 insertions(+), 28 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +38 -2

index 9583f432e02e..0a54f7cdce37 100644
@@ -245,6 +245,45 @@ static void g4x_audio_codec_enable(struct
 drm_connector *connector,
 	I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 }
 
+static void hsw_audio_config_update(struct intel_crtc *intel_crtc,
+				    enum port port,
+				    const struct drm_display_mode
 *adjusted_mode) {
+	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	enum pipe pipe = intel_crtc->pipe;
+	int n, rate;
+	u32 tmp;
+
+	tmp = I915_READ(HSW_AUD_CFG(pipe));
+	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
+	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
+	if (intel_crtc_has_dp_encoder(intel_crtc->config))
+		tmp |= AUD_CONFIG_N_VALUE_INDEX;
+	else
+		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
+
+	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+	if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
+		if (!acomp)
+			rate = 0;
+		else if (port >= PORT_A && port <= PORT_E)
+			rate = acomp->aud_sample_rate[port];
+		else {
+			DRM_ERROR("invalid port: %d\n", port);
+			rate = 0;
+		}
+
+		n = audio_config_get_n(adjusted_mode, rate);
+		if (n != 0)
+			tmp = audio_config_setup_n_reg(n, tmp);
+		else
+			DRM_DEBUG_KMS("no suitable N value is found\n");
+	}
+
+	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+}
+
 static void hsw_audio_codec_disable(struct intel_encoder *encoder)  {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

@@ -283,11 +322,9 @@ static void hsw_audio_codec_enable(struct
 drm_connector *connector,
 	struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
 	enum pipe pipe = intel_crtc->pipe;
 	enum port port = intel_encoder->port;
-	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
 	const uint8_t *eld = connector->eld;
 	uint32_t tmp;
 	int len, i;
-	int n, rate;
 

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n",
 		      pipe_name(pipe), drm_eld_size(eld)); @@ -323,32 +360,7

@@ static void hsw_audio_codec_enable(struct drm_connector *connector,

drivers/gpu/drm/i915/intel_audio.c +1 -27

 	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
 
 	/* Enable timestamps */
-	tmp = I915_READ(HSW_AUD_CFG(pipe));
-	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
-	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
-	if (intel_crtc_has_dp_encoder(intel_crtc->config))
-		tmp |= AUD_CONFIG_N_VALUE_INDEX;
-	else
-		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
-
-	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
-	if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
-		if (!acomp)
-			rate = 0;
-		else if (port >= PORT_A && port <= PORT_E)
-			rate = acomp->aud_sample_rate[port];
-		else {
-			DRM_ERROR("invalid port: %d\n", port);
-			rate = 0;
-		}
-		n = audio_config_get_n(adjusted_mode, rate);
-		if (n != 0)
-			tmp = audio_config_setup_n_reg(n, tmp);
-		else
-			DRM_DEBUG_KMS("no suitable N value is found\n");
-	}
-
-	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+	hsw_audio_config_update(intel_crtc, port, adjusted_mode);
 
 	mutex_unlock(&dev_priv->av_mutex);
 }
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 2/9] drm/i915/audio: port is going to be just fine, simplify checks

If it was wrong, we'd be screwed already.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +2 -1

index 0a54f7cdce37..5d0bd07afa51 100644
@@ -251,8 +251,9 @@ static void hsw_audio_config_update(struct intel_crtc
 *intel_crtc,  {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
 	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	int n, rate;
+	int n;
 	u32 tmp;
 

drivers/gpu/drm/i915/intel_audio.c +0 -9

 	tmp = I915_READ(HSW_AUD_CFG(pipe));

@@ -265,15 +266,6 @@ static void hsw_audio_config_update(struct
 intel_crtc *intel_crtc,
 
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
-		if (!acomp)
-			rate = 0;
-		else if (port >= PORT_A && port <= PORT_E)
-			rate = acomp->aud_sample_rate[port];
-		else {
-			DRM_ERROR("invalid port: %d\n", port);
-			rate = 0;
-		}
-
 		n = audio_config_get_n(adjusted_mode, rate);
 		if (n != 0)

tmp = audio_config_setup_n_reg(n, tmp);
--
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 3/9] drm/i915/audio: use the same code for updating audio config

It gets fragile to duplicate the code for updating HSW_AUD_CFG. The only change should be that the hdmi pixel clock is also updated in i915_audio_component_sync_audio_rate(), but it should not be any different.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 29 +++--------------------------
 1 file changed, 3 insertions(+), 26 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +1 -3

index 5d0bd07afa51..4d62b3e8ac19 100644
@@ -671,10 +671,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
 	struct intel_encoder *intel_encoder;
 	struct intel_crtc *crtc;
-	struct drm_display_mode *mode;
+	struct drm_display_mode *adjusted_mode;
 	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
-	u32 tmp;
-	int n;
 	int err = 0;

/* HSW, BDW, SKL, KBL need this fix */ @@ -700,33 +698,12 @@
static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,

drivers/gpu/drm/i915/intel_audio.c +1 -12

 	crtc = to_intel_crtc(intel_encoder->base.crtc);
 	pipe = crtc->pipe;
 
-	mode = &crtc->config->base.adjusted_mode;
+	adjusted_mode = &crtc->config->base.adjusted_mode;
 
 	/* port must be valid now, otherwise the pipe will be invalid */
 	acomp->aud_sample_rate[port] = rate;
 
-	/* 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);
-		goto unlock;
-	}
-
-	n = audio_config_get_n(mode, rate);
-	if (n == 0) {
-		DRM_DEBUG_KMS("Using automatic mode for N value on

port %c\n",

drivers/gpu/drm/i915/intel_audio.c +1 -12

-					  port_name(port));
-		tmp = I915_READ(HSW_AUD_CFG(pipe));
-		tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
-		I915_WRITE(HSW_AUD_CFG(pipe), tmp);
-		goto unlock;
-	}
-
-	/* 3. set the N/CTS/M */
-	tmp = I915_READ(HSW_AUD_CFG(pipe));
-	tmp = audio_config_setup_n_reg(n, tmp);
-	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+	hsw_audio_config_update(crtc, port, adjusted_mode);
 
  unlock:
 	mutex_unlock(&dev_priv->av_mutex);
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 4/9] drm/i915/audio: split dp and hdmi audio config update

The code for dp and hdmi are already different, and they're about to diverge even more. Split them for clarity in future work. No functional changes.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 55 +++++++++++++++++++++++----------
-----
 1 file changed, 34 insertions(+), 21 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +0 -11

index 4d62b3e8ac19..db7fd6d8333f 100644
@@ -148,18 +148,6 @@ static uint32_t audio_config_setup_n_reg(int n,
 uint32_t val)
 	return tmp;
 }
 
-/* check whether N/CTS/M need be set manually */ -static bool
 audio_rate_need_prog(struct intel_crtc *crtc,
-				 const struct drm_display_mode *mode)
-{
-	if (((mode->clock == TMDS_297M) ||
-		 (mode->clock == TMDS_296M)) &&
-		intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
-		return true;
-	else
-		return false;
-}
-
 static bool intel_eld_uptodate(struct drm_connector *connector,
 			       i915_reg_t reg_eldv, uint32_t bits_eldv,
 			       i915_reg_t reg_elda, uint32_t bits_elda, @@ -

245,9 +233,26 @@ static void g4x_audio_codec_enable(struct drm_connector *connector,

drivers/gpu/drm/i915/intel_audio.c +0 -3

 	I915_WRITE(G4X_AUD_CNTL_ST, tmp);
 }
 
-static void hsw_audio_config_update(struct intel_crtc *intel_crtc,
-				    enum port port,
-				    const struct drm_display_mode

*adjusted_mode)

drivers/gpu/drm/i915/intel_audio.c +18 -0

+static void
+hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
+			   const struct drm_display_mode *adjusted_mode) {
+	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	enum pipe pipe = intel_crtc->pipe;
+	u32 tmp;
+
+	tmp = I915_READ(HSW_AUD_CFG(pipe));
+	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
+	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
+	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
+	tmp |= AUD_CONFIG_N_VALUE_INDEX;
+
+	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+}
+
+static void
+hsw_hdmi_audio_config_update(struct intel_crtc *intel_crtc, enum port

port,

drivers/gpu/drm/i915/intel_audio.c +1 -0

+			     const struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
 	struct i915_audio_component *acomp = dev_priv-

Quoted 1 line

audio_component; @@ -259,13 +264,11 @@ static void

hsw_audio_config_update(struct intel_crtc *intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +5 -6

 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
 	tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK;
-	if (intel_crtc_has_dp_encoder(intel_crtc->config))
-		tmp |= AUD_CONFIG_N_VALUE_INDEX;
-	else
-		tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
-
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
-	if (audio_rate_need_prog(intel_crtc, adjusted_mode)) {
+	tmp |= audio_config_hdmi_pixel_clock(adjusted_mode);
+
+	if (adjusted_mode->clock == TMDS_296M ||
+	    adjusted_mode->clock == TMDS_297M) {
 		n = audio_config_get_n(adjusted_mode, rate);
 		if (n != 0)
 			tmp = audio_config_setup_n_reg(n, tmp); @@ -276,6
+279,16 @@ static void hsw_audio_config_update(struct intel_crtc

*intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +5 -0

 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
 }
 
+static void
+hsw_audio_config_update(struct intel_crtc *intel_crtc, enum port port,
+			const struct drm_display_mode *adjusted_mode) {
+	if (intel_crtc_has_dp_encoder(intel_crtc->config))
+		hsw_dp_audio_config_update(intel_crtc, port,

adjusted_mode);

drivers/gpu/drm/i915/intel_audio.c +2 -0

+	else
+		hsw_hdmi_audio_config_update(intel_crtc, port,

adjusted_mode); }

drivers/gpu/drm/i915/intel_audio.c +1 -1

+
 static void hsw_audio_codec_disable(struct intel_encoder *encoder)  {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 7/9] drm/i915/audio: add register macros for audio config N value

Have generic macros in line with the rest of the register bit definition macros instead of a dedicated function in intel_audio.c, and use them.
No functional changes.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |  4 ++++
 drivers/gpu/drm/i915/intel_audio.c | 23 ++++++-----------------
 2 files changed, 10 insertions(+), 17 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index acc767a52d8e..595d196f753f 100644

drivers/gpu/drm/i915/i915_reg.h +4 -0

@@ -7332,6 +7332,10 @@ enum {
 #define   AUD_CONFIG_UPPER_N_MASK		(0xff << 20)
 #define   AUD_CONFIG_LOWER_N_SHIFT		4
 #define   AUD_CONFIG_LOWER_N_MASK		(0xfff << 4)
+#define   AUD_CONFIG_N_MASK
 	(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK)
+#define   AUD_CONFIG_N(n) \
+	(((((n) >> 12) & 0xff) << AUD_CONFIG_UPPER_N_SHIFT) |	\
+	 (((n) & 0xfff) << AUD_CONFIG_LOWER_N_SHIFT))
 #define   AUD_CONFIG_PIXEL_CLOCK_HDMI_SHIFT	16
 #define   AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK	(0xf << 16)

#define AUD_CONFIG_PIXEL_CLOCK_HDMI_25175 (0 << 16)

drivers/gpu/drm/i915/intel_audio.c +0 -13

index 6aa619a84439..d2c6227f72b8 100644
@@ -135,20 +135,6 @@ static int audio_config_get_n(const struct
 drm_display_mode *adjusted_mode,
 	return 0;
 }
 
-static uint32_t audio_config_setup_n_reg(int n, uint32_t val) -{
-	int n_low, n_up;
-	uint32_t tmp = val;
-
-	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);
-	return tmp;
-}
-
 static bool intel_eld_uptodate(struct drm_connector *connector,
 			       i915_reg_t reg_eldv, uint32_t bits_eldv,
 			       i915_reg_t reg_elda, uint32_t bits_elda, @@ -

271,10 +257,13 @@ hsw_hdmi_audio_config_update(struct intel_crtc *intel_crtc, enum port port,

drivers/gpu/drm/i915/intel_audio.c +6 -4

 	if (adjusted_mode->crtc_clock == TMDS_296M ||
 	    adjusted_mode->crtc_clock == TMDS_297M) {
 		n = audio_config_get_n(adjusted_mode, rate);
-		if (n != 0)
-			tmp = audio_config_setup_n_reg(n, tmp);
-		else
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
 			DRM_DEBUG_KMS("no suitable N value is found\n");
+		}
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 8/9] drm/i915/audio: rename N value getter to emphasize it's for hdmi

We'll be getting a function and a table for dp parameters soon enough, so rename the function and table for hdmi. No functional changes.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +1 -1

index d2c6227f72b8..81df29ca4947 100644
@@ -81,7 +81,7 @@ static const struct {
 	int clock;
 	int n;
 	int cts;
-} aud_ncts[] = {
+} hdmi_aud_ncts[] = {
 	{ 44100, TMDS_296M, 4459, 234375 },
 	{ 44100, TMDS_297M, 4704, 247500 },
 	{ 48000, TMDS_296M, 5824, 281250 },

@@ -121,15 +121,15 @@ static u32 audio_config_hdmi_pixel_clock(const

struct drm_display_mode *adjusted

drivers/gpu/drm/i915/intel_audio.c +0 -1

 	return hdmi_audio_clock[i].config;
 }
 
-static int audio_config_get_n(const struct drm_display_mode

*adjusted_mode,

drivers/gpu/drm/i915/intel_audio.c +1 -1

-			      int rate)
+static int audio_config_hdmi_get_n(const struct drm_display_mode

*adjusted_mode,

drivers/gpu/drm/i915/intel_audio.c +6 -5

+				   int rate)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
-		if ((rate == aud_ncts[i].sample_rate) &&
-			(adjusted_mode->crtc_clock == aud_ncts[i].clock)) {
-			return aud_ncts[i].n;
+	for (i = 0; i < ARRAY_SIZE(hdmi_aud_ncts); i++) {
+		if (rate == hdmi_aud_ncts[i].sample_rate &&
+		    adjusted_mode->crtc_clock == hdmi_aud_ncts[i].clock) {
+			return hdmi_aud_ncts[i].n;
 		}
 	}
 	return 0;

@@ -256,7 +256,7 @@ hsw_hdmi_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 
 	if (adjusted_mode->crtc_clock == TMDS_296M ||
 	    adjusted_mode->crtc_clock == TMDS_297M) {
-		n = audio_config_get_n(adjusted_mode, rate);
+		n = audio_config_hdmi_get_n(adjusted_mode, rate);
 		if (n != 0) {
 			tmp &= ~AUD_CONFIG_N_MASK;

tmp |= AUD_CONFIG_N(n);
--
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reviewed-by Libin Yang <[email protected]>

Regards,
Libin

Quoted 75 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 8/9] drm/i915/audio: rename N value getter to emphasize it's for hdmi

We'll be getting a function and a table for dp parameters soon enough, so rename the function and table for hdmi. No functional changes.

Cc Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_audio.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

drivers/gpu/drm/i915/intel_audio.c +1 -1

index d2c6227f72b8..81df29ca4947 100644
@@ -81,7 +81,7 @@ static const struct {
 	int clock;
 	int n;
 	int cts;
-} aud_ncts[] = {
+} hdmi_aud_ncts[] = {
 	{ 44100, TMDS_296M, 4459, 234375 },
 	{ 44100, TMDS_297M, 4704, 247500 },
 	{ 48000, TMDS_296M, 5824, 281250 },

@@ -121,15 +121,15 @@ static u32 audio_config_hdmi_pixel_clock(const

struct drm_display_mode *adjusted

drivers/gpu/drm/i915/intel_audio.c +0 -1

 	return hdmi_audio_clock[i].config;
 }
 
-static int audio_config_get_n(const struct drm_display_mode

*adjusted_mode,

drivers/gpu/drm/i915/intel_audio.c +1 -1

-			      int rate)
+static int audio_config_hdmi_get_n(const struct drm_display_mode

*adjusted_mode,

drivers/gpu/drm/i915/intel_audio.c +6 -5

+				   int rate)
 {
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
-		if ((rate == aud_ncts[i].sample_rate) &&
-			(adjusted_mode->crtc_clock == aud_ncts[i].clock)) {
-			return aud_ncts[i].n;
+	for (i = 0; i < ARRAY_SIZE(hdmi_aud_ncts); i++) {
+		if (rate == hdmi_aud_ncts[i].sample_rate &&
+		    adjusted_mode->crtc_clock == hdmi_aud_ncts[i].clock) {
+			return hdmi_aud_ncts[i].n;
 		}
 	}
 	return 0;

@@ -256,7 +256,7 @@ hsw_hdmi_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 
 	if (adjusted_mode->crtc_clock == TMDS_296M ||
 	    adjusted_mode->crtc_clock == TMDS_297M) {
-		n = audio_config_get_n(adjusted_mode, rate);
+		n = audio_config_hdmi_get_n(adjusted_mode, rate);
 		if (n != 0) {
 			tmp &= ~AUD_CONFIG_N_MASK;

tmp |= AUD_CONFIG_N(n);
--
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following equation:
Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B)

drivers/gpu/drm/i915/intel_audio.c +0 -0

index 81df29ca4947..0bc2701b6c9c 100644
@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called from

i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +52 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+
+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },
+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },
+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },
+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },
+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },
+};
+
+static const struct dp_aud_n_m *
+audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -1

+rate);
+
+	return nm ? nm->n : 0;
+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 			   const struct drm_display_mode *adjusted_mode)  {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 
 	tmp = I915_READ(HSW_AUD_CFG(pipe));

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;

@@ -234,7 +300,30 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;
 

drivers/gpu/drm/i915/intel_audio.c +6 -0

 static void

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

static void

drivers/gpu/drm/i915/intel_audio.c +2 -1

@@ -687,7 +782,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",
 port_name(port));

drivers/gpu/drm/i915/intel_audio.c +0 -1

 		err = -ENODEV;
 		goto unlock;
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Lin, Mengdong
Sent: Wednesday, October 12, 2016 10:46 AM
To: Nikula, Jani <[email protected]>; [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 34 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following equation:
Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h enum { · line 7369

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +6 -0

 #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
 _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
 
+#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
 _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
+#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
+#define   AUD_CONFIG_M_MASK		0xfffff

The last line cause misalignment after applying the patch.

Quoted 5 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B) diff --

git

Quoted 98 lines

a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 81df29ca4947..0bc2701b6c9c 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called from

i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +52 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+
+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },
+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },
+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },
+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },
+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },
+};
+
+static const struct dp_aud_n_m *
+audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -1

+rate);
+
+	return nm ? nm->n : 0;
+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 			   const struct drm_display_mode *adjusted_mode)  {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 
 	tmp = I915_READ(HSW_AUD_CFG(pipe));

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;

@@ -234,7 +300,30 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;
 

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 312

Quoted 10 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+	if (intel_crtc->config->port_clock == LC_540M ||
+	    intel_crtc->config->port_clock == LC_270M ||
+	    intel_crtc->config->port_clock == LC_162M) {
+		n = audio_config_dp_get_n(intel_crtc, rate);
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
+			DRM_DEBUG_KMS("no suitable N value is found\n");

Could some comments be added here why we don't return if no valid N value is found?

Do we still need to program write the register to let HW calculate N by itself?

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 316

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

And if no valid N value is found, need we go ahead to check M value?

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 327

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+
+	m = audio_config_dp_get_m(intel_crtc, rate);
+	if (m) {
+		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+		tmp &= ~AUD_CONFIG_M_MASK;
+		tmp |= m;
+		tmp |= AUD_M_CTS_M_VALUE_INDEX;
+		tmp |= AUD_M_CTS_M_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+	}
 }

Thanks
Mengdong

Quoted 16 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

 static void

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

static void

drivers/gpu/drm/i915/intel_audio.c static int · line 788

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +2 -1

@@ -687,7 +782,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",

port_name(port));

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +0 -0

 		err = -ENODEV;

goto unlock;
--
2.1.4

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 87 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Lin, Mengdong
Sent: Wednesday, October 12, 2016 10:46 AM
To: Nikula, Jani <[email protected]>; [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 34 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following equation:
Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h enum { · line 7369

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +6 -0

 #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
 _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
 
+#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
 _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
+#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
+#define   AUD_CONFIG_M_MASK		0xfffff

The last line cause misalignment after applying the patch.

Quoted 5 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B) diff --

git

Quoted 22 lines

a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 81df29ca4947..0bc2701b6c9c 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called from

i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +12 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c line 79

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +8 -0

+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },
+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },

Quoted 6 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },
+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c line 93

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +8 -0

+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },

Quoted 54 lines

Quoted 54 lines

drivers/gpu/drm/i915/intel_audio.c +18 -0

+};
+
+static const struct dp_aud_n_m *
+audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc, int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -1

+rate);
+
+	return nm ? nm->n : 0;
+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 			   const struct drm_display_mode *adjusted_mode)  {
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 
 	tmp = I915_READ(HSW_AUD_CFG(pipe));

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX;

@@ -234,7 +300,30 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;
 

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 305

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+	if (intel_crtc->config->port_clock == LC_540M ||
+	    intel_crtc->config->port_clock == LC_270M ||
+	    intel_crtc->config->port_clock == LC_162M) {

Quoted 10 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 312

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c +7 -0

+		n = audio_config_dp_get_n(intel_crtc, rate);
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
+			DRM_DEBUG_KMS("no suitable N value is found\n");

Could some comments be added here why we don't return if no valid N value is found?

This is because although we don't need set N value here, but there are some other bits to be set. So you can see we call I915_WRITE(HSW_AUD_CFG(pipe), tmp);
Later.

Quoted 1 line

Do we still need to program write the register to let HW calculate N by itself?

Ideally, all the frequency should be in the table. But if the frequency is not in our table, we will let HW calculate N by itself.

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 316

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

And if no valid N value is found, need we go ahead to check M value?

If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate); will also return 0 and skip the m setting.

Regards,
Libin

Quoted 54 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 327

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+
+	m = audio_config_dp_get_m(intel_crtc, rate);
+	if (m) {
+		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+		tmp &= ~AUD_CONFIG_M_MASK;
+		tmp |= m;
+		tmp |= AUD_M_CTS_M_VALUE_INDEX;
+		tmp |= AUD_M_CTS_M_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+	}
 }

Thanks
Mengdong

Quoted 16 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

 static void

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

static void

drivers/gpu/drm/i915/intel_audio.c static int · line 788

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +2 -1

@@ -687,7 +782,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",

port_name(port));

Quoted 9 lines

drivers/gpu/drm/i915/intel_audio.c +0 -0

 		err = -ENODEV;

goto unlock;
--
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-----Original Message-----
From: Nikula, Jani
Sent: Wednesday, October 19, 2016 11:09 PM
To: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]
Cc: [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhang, Keqiao <[email protected]>; Zhao, Juan J <[email protected]>
Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Sorry it's taken me forever to get back to this. Some comments inline.

BR,
Jani.

Quoted 24 lines · Libin

Quoted 24 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Lin, Mengdong
Sent: Wednesday, October 12, 2016 10:46 AM
To: Nikula, Jani <[email protected]>; [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 11 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following

equation:

Quoted 32 lines

Quoted 32 lines

Quoted 19 lines

Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h enum { · line 7369

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +6 -0

 #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
 _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
 
+#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
 _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
+#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
+#define   AUD_CONFIG_M_MASK		0xfffff

The last line cause misalignment after applying the patch.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B)

diff --

Quoted 27 lines

Quoted 27 lines

git

Quoted 26 lines

a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 81df29ca4947..0bc2701b6c9c 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called

from
i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +15 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+
+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c line 79

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +5 -0

+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },

Any particular reason these M/N values are half of what they're in table 2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

Quoted 15 lines

Quoted 15 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +4 -0

+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },

drivers/gpu/drm/i915/intel_audio.c line 93

Quoted 10 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },
+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },

Do we not support 128 or 64 kHz audio?

Quoted 58 lines

Quoted 58 lines

Quoted 58 lines

drivers/gpu/drm/i915/intel_audio.c +19 -0

+};
+
+static const struct dp_aud_n_m *
+audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +9 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -1

+rate);
+
+	return nm ? nm->n : 0;
+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 			   const struct drm_display_mode *adjusted_mode)  {
 	struct drm_i915_private *dev_priv =
 to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -234,7 +300,30 @@

hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 305

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+	if (intel_crtc->config->port_clock == LC_540M ||
+	    intel_crtc->config->port_clock == LC_270M ||
+	    intel_crtc->config->port_clock == LC_162M) {

Actually, this check could be removed. audio_config_dp_get_n_m will always return 0 if this doesn't hold. But not a big deal.

Quoted 12 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 312

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +7 -0

+		n = audio_config_dp_get_n(intel_crtc, rate);
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
+			DRM_DEBUG_KMS("no suitable N value is found\n");

Could some comments be added here why we don't return if no valid N value is found?

Quoted 5 lines

This is because although we don't need set N value here, but there are some other bits to be set. So you can see we call I915_WRITE(HSW_AUD_CFG(pipe), tmp); Later.

Quoted 1 line

Do we still need to program write the register to let HW calculate N by

itself?

Quoted 12 lines

Ideally, all the frequency should be in the table. But if the frequency is not in our table, we will let HW calculate N by itself.

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 316

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

And if no valid N value is found, need we go ahead to check M value?

Quoted 2 lines

If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate); will also return 0 and skip the m setting.

If we can't find M/N values, the HSW_AUD_CFG register is set to use automatic N value. However, the HSW_AUD_M_CTS_ENABLE register is not updated at all, and could potentially have stale stuff. Do we need to clear the AUD_M_CTS_M_VALUE_INDEX and/or AUD_M_CTS_M_PROG_ENABLE bits?

We only do this stuff in hsw_audio_codec_enable now. Do we need to do something in hsw_audio_codec_disable too?

BR,
Jani.

Quoted 20 lines

Regards,
Libin

Quoted 16 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 327

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+
+	m = audio_config_dp_get_m(intel_crtc, rate);
+	if (m) {
+		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+		tmp &= ~AUD_CONFIG_M_MASK;
+		tmp |= m;
+		tmp |= AUD_M_CTS_M_VALUE_INDEX;
+		tmp |= AUD_M_CTS_M_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+	}
 }

Thanks
Mengdong

Quoted 2 lines

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c hsw_hdmi_audio_config_update(struct

Quoted 2 lines

static void

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct

intel_crtc

Quoted 32 lines

Quoted 32 lines

Quoted 14 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

static void

drivers/gpu/drm/i915/intel_audio.c static int · line 788

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +2 -1

@@ -687,7 +782,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",

port_name(port));

Quoted 9 lines

drivers/gpu/drm/i915/intel_audio.c +0 -0

 		err = -ENODEV;

goto unlock;
--
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 4 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Technology Center

Quoted 113 lines

-----Original Message-----
From: Nikula, Jani
Sent: Wednesday, October 19, 2016 11:09 PM
To: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]
Cc: [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhang, Keqiao <[email protected]>; Zhao, Juan J <[email protected]>
Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Sorry it's taken me forever to get back to this. Some comments inline.

BR,
Jani.

Quoted 24 lines · Libin

Quoted 24 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Lin, Mengdong
Sent: Wednesday, October 12, 2016 10:46 AM
To: Nikula, Jani <[email protected]>; [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 11 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following

equation:

Quoted 32 lines

Quoted 32 lines

Quoted 19 lines

Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index 595d196f753f..8d9dbc7d5b32 100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h enum { · line 7369

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +6 -0

 #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
 _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
 
+#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
 _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
+#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
+#define   AUD_CONFIG_M_MASK		0xfffff

The last line cause misalignment after applying the patch.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B)

diff --

Quoted 27 lines

Quoted 27 lines

git

Quoted 26 lines

a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 81df29ca4947..0bc2701b6c9c 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called

from
i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +15 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+
+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c line 79

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +5 -0

+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },

Any particular reason these M/N values are half of what they're in table 2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

For HDMI, we found only set N is enough. HW then can handle the remaining.

Quoted 18 lines

Quoted 16 lines

Quoted 16 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_audio.c +4 -0

+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },

drivers/gpu/drm/i915/intel_audio.c line 93

Quoted 10 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },
+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },

Do we not support 128 or 64 kHz audio?

No, we don't support HBR audio.

Quoted 58 lines

Quoted 58 lines

Quoted 58 lines

Quoted 58 lines

drivers/gpu/drm/i915/intel_audio.c +19 -0

+};
+
+static const struct dp_aud_n_m *
+audio_config_dp_get_n_m(struct intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock == dp_aud_n_m[i].clock)
+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +9 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +8 -1

+rate);
+
+	return nm ? nm->n : 0;
+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct intel_crtc
 *intel_crtc, enum port port,
 			   const struct drm_display_mode *adjusted_mode)  {
 	struct drm_i915_private *dev_priv =
 to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -234,7 +300,30 @@

hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;

Quoted 6 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 305

Quoted 3 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+	if (intel_crtc->config->port_clock == LC_540M ||
+	    intel_crtc->config->port_clock == LC_270M ||
+	    intel_crtc->config->port_clock == LC_162M) {

Actually, this check could be removed. audio_config_dp_get_n_m will always return 0 if this doesn't hold. But not a big deal.

Yes, we can remove it.

Quoted 33 lines

Quoted 12 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 312

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +7 -0

+		n = audio_config_dp_get_n(intel_crtc, rate);
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
+			DRM_DEBUG_KMS("no suitable N value is found\n");

Could some comments be added here why we don't return if no valid N value is found?

Quoted 5 lines

This is because although we don't need set N value here, but there are some other bits to be set. So you can see we call I915_WRITE(HSW_AUD_CFG(pipe), tmp); Later.

Quoted 1 line

Do we still need to program write the register to let HW calculate N by

itself?

Quoted 12 lines

Ideally, all the frequency should be in the table. But if the frequency is not in our table, we will let HW calculate N by itself.

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 316

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

And if no valid N value is found, need we go ahead to check M value?

Quoted 2 lines

If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate); will also return 0 and skip the m setting.

Quoted 7 lines

If we can't find M/N values, the HSW_AUD_CFG register is set to use automatic N value. However, the HSW_AUD_M_CTS_ENABLE register is not updated at all, and could potentially have stale stuff. Do we need to clear the AUD_M_CTS_M_VALUE_INDEX and/or AUD_M_CTS_M_PROG_ENABLE bits?

We only do this stuff in hsw_audio_codec_enable now. Do we need to do something in hsw_audio_codec_disable too?

In _disable(), the audio doesn't work. So the value will not impact audio. It will not be worse that we do not reset the value in _disable(). Without reset, the value is "cached". Next time, it may be a little quicker for monitor response if we are using the same parameter, I think.

Regards
Libin

Quoted 68 lines

BR,
Jani.

Quoted 20 lines

Regards,
Libin

Quoted 16 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 327

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+
+	m = audio_config_dp_get_m(intel_crtc, rate);
+	if (m) {
+		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+		tmp &= ~AUD_CONFIG_M_MASK;
+		tmp |= m;
+		tmp |= AUD_M_CTS_M_VALUE_INDEX;
+		tmp |= AUD_M_CTS_M_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+	}
 }

Thanks
Mengdong

Quoted 2 lines

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c hsw_hdmi_audio_config_update(struct

Quoted 2 lines

static void

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct

intel_crtc

Quoted 28 lines

Quoted 28 lines

Quoted 14 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

static void

drivers/gpu/drm/i915/intel_audio.c static int · line 788

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +2 -1

@@ -687,7 +782,8 @@ static int
 i915_audio_component_sync_audio_rate(struct device *kdev, int port,
 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",

port_name(port));

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +0 -0

 		err = -ENODEV;

goto unlock;
--
2.1.4

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Jani Nikula, Intel Open Source Technology Center

-----Original Message-----
From: Nikula, Jani
Sent: Thursday, October 20, 2016 4:34 PM
To: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]
Cc: [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhang, Keqiao <[email protected]>; Zhao, Juan J <[email protected]>
Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 126 lines · Libin

Quoted 125 lines

-----Original Message-----
From: Nikula, Jani
Sent: Wednesday, October 19, 2016 11:09 PM
To: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]
Cc: [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhang, Keqiao <[email protected]>; Zhao, Juan J <[email protected]>
Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Sorry it's taken me forever to get back to this. Some comments inline.

BR,
Jani.

Quoted 25 lines · Libin

Quoted 25 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Lin, Mengdong
Sent: Wednesday, October 12, 2016 10:46 AM
To: Nikula, Jani <[email protected]>; [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 11 lines

-----Original Message-----
From: Intel-gfx [mailto:[email protected]] On Behalf Of Jani Nikula
Sent: Monday, October 10, 2016 11:04 PM
To: [email protected]
Cc: Nikula, Jani <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>
Subject: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

When modeset occurs and the LS_CLK is set to some special values in DP mode, the N/M need to be set manually if audio is playing. Otherwise the first several seconds may be silent in audio playback.

The relationship of Maud and Naud is expressed in the following

equation:

Quoted 33 lines

Quoted 33 lines

Quoted 20 lines

Maud/Naud = 512 * fs / f_LS_Clk

Please refer VESA DisplayPort Standard spec for details.

Signed-off-by Libin Yang <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/i915_reg.h    |   7 +++
 drivers/gpu/drm/i915/intel_audio.c | 100
++++++++++++++++++++++++++++++++++++-
 2 files changed, 105 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/i915/i915_reg.h index
595d196f753f..8d9dbc7d5b32
100644

drivers/gpu/drm/i915/i915_reg.h +0 -0

@@ -7359,6 +7359,13 @@ enum {
 #define _HSW_AUD_MISC_CTRL_B		0x65110

drivers/gpu/drm/i915/i915_reg.h enum { · line 7369

Quoted 10 lines

drivers/gpu/drm/i915/i915_reg.h +6 -0

 #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
 _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
 
+#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe)	_MMIO_PIPE(pipe,
 _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
+#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
+#define   AUD_CONFIG_M_MASK		0xfffff

The last line cause misalignment after applying the patch.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i915/i915_reg.h +1 -0

+
 #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) _MMIO_PIPE(pipe,
_HSW_AUD_DIP_ELD_CTRL_ST_A, _HSW_AUD_DIP_ELD_CTRL_ST_B)

diff --

Quoted 24 lines

Quoted 24 lines

git

Quoted 23 lines

a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 81df29ca4947..0bc2701b6c9c 100644

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -57,6 +57,70 @@
  * struct &i915_audio_component_audio_ops @audio_ops is called

from
i915 driver.
*/

drivers/gpu/drm/i915/intel_audio.c +12 -0

+/* DP N/M table */
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+
+struct dp_aud_n_m {
+	int sample_rate;
+	int clock;
+	u16 n;
+	u16 m;
+};
+

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c line 79

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +8 -0

+static const struct dp_aud_n_m dp_aud_n_m[] = {
+	{ 192000, LC_540M, 5625, 1024 },
+	{ 176400, LC_540M, 9375, 1568 },
+	{ 96000, LC_540M, 5625, 512 },
+	{ 88200, LC_540M, 9375, 784 },
+	{ 48000, LC_540M, 5625, 256 },
+	{ 44100, LC_540M, 9375, 392 },
+	{ 32000, LC_540M, 16875, 512 },

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

Quoted 1 line

For HDMI, we found only set N is enough. HW then can handle the

remaining.

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why?

Jani.

Quoted 22 lines

Quoted 18 lines

Quoted 16 lines

Quoted 16 lines

Quoted 6 lines

drivers/gpu/drm/i915/intel_audio.c +4 -0

+	{ 192000, LC_270M, 5625, 2048 },
+	{ 176400, LC_270M, 9375, 3136 },
+	{ 96000, LC_270M, 5625, 1024 },
+	{ 88200, LC_270M, 9375, 1568 },

drivers/gpu/drm/i915/intel_audio.c line 93

Quoted 10 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+	{ 48000, LC_270M, 5625, 512 },
+	{ 44100, LC_270M, 9375, 784 },
+	{ 32000, LC_270M, 16875, 1024 },
+	{ 192000, LC_162M, 3375, 2048 },
+	{ 176400, LC_162M, 5625, 3136 },
+	{ 96000, LC_162M, 3375, 1024 },
+	{ 88200, LC_162M, 5625, 1568 },
+	{ 48000, LC_162M, 3375, 512 },
+	{ 44100, LC_162M, 5625, 784 },
+	{ 32000, LC_162M, 10125, 1024 },

Do we not support 128 or 64 kHz audio?

No, we don't support HBR audio.

Quoted 1 line

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/gpu/drm/i915/intel_audio.c

Quoted 9 lines

drivers/gpu/drm/i915/intel_audio.c +9 -0

+};
+
+static const struct dp_aud_n_m * audio_config_dp_get_n_m(struct
+intel_crtc *intel_crtc, int rate) {
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dp_aud_n_m); i++) {
+		if (rate == dp_aud_n_m[i].sample_rate &&
+		    intel_crtc->config->port_clock ==

dp_aud_n_m[i].clock)

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+			return &dp_aud_n_m[i];
+	}
+
+	return NULL;
+}
+
+static int audio_config_dp_get_m(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +9 -0

+rate);
+
+	return nm ? nm->m : 0;
+}
+
+static int audio_config_dp_get_n(struct intel_crtc *intel_crtc,
+int
+rate) {
+	const struct dp_aud_n_m *nm =

audio_config_dp_get_n_m(intel_crtc,

drivers/gpu/drm/i915/intel_audio.c +3 -0

+rate);
+
+	return nm ? nm->n : 0;

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct · line 290

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c +2 -0

+}
+
 static const struct {
 	int clock;
 	u32 config;

@@ -225,8 +289,10 @@ hsw_dp_audio_config_update(struct
 intel_crtc *intel_crtc, enum port port,
 			   const struct drm_display_mode

*adjusted_mode) {

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/gpu/drm/i915/intel_audio.c

Quoted 11 lines

drivers/gpu/drm/i915/intel_audio.c +3 -1

 	struct drm_i915_private *dev_priv =
 to_i915(intel_crtc->base.dev);
+	struct i915_audio_component *acomp = dev_priv-
 >audio_component;
+	int rate = acomp ? acomp->aud_sample_rate[port] : 0;
 	enum pipe pipe = intel_crtc->pipe;
-	u32 tmp;
+	u32 tmp, n, m;
 

drivers/gpu/drm/i915/intel_audio.c +0 -0

 	tmp = I915_READ(HSW_AUD_CFG(pipe));
 	tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -234,7 +300,30

@@

Quoted 15 lines

Quoted 11 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 305

Quoted 8 lines

hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,

drivers/gpu/drm/i915/intel_audio.c +3 -0

 	tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
 	tmp |= AUD_CONFIG_N_VALUE_INDEX;
 
+	if (intel_crtc->config->port_clock == LC_540M ||
+	    intel_crtc->config->port_clock == LC_270M ||
+	    intel_crtc->config->port_clock == LC_162M) {

Actually, this check could be removed. audio_config_dp_get_n_m will always return 0 if this doesn't hold. But not a big deal.

Yes, we can remove it.

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port,

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c +7 -0

+		n = audio_config_dp_get_n(intel_crtc, rate);
+		if (n != 0) {
+			tmp &= ~AUD_CONFIG_N_MASK;
+			tmp |= AUD_CONFIG_N(n);
+			tmp |= AUD_CONFIG_N_PROG_ENABLE;
+		} else {
+			DRM_DEBUG_KMS("no suitable N value is

found\n");

Quoted 23 lines

Quoted 23 lines

Quoted 10 lines

Quoted 3 lines

Could some comments be added here why we don't return if no valid N value is found?

This is because although we don't need set N value here, but there are some other bits to be set. So you can see we call I915_WRITE(HSW_AUD_CFG(pipe), tmp); Later.

Quoted 2 lines

Do we still need to program write the register to let HW calculate N by

itself?

Quoted 12 lines

Ideally, all the frequency should be in the table. But if the frequency is not in our table, we will let HW calculate N by itself.

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 316

Quoted 5 lines

drivers/gpu/drm/i915/intel_audio.c +3 -0

+		}
+	}
+
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);

And if no valid N value is found, need we go ahead to check M value?

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

If there is no valid N value, audio_config_dp_get_m(intel_crtc, rate); will also return 0 and skip the m setting.

If we can't find M/N values, the HSW_AUD_CFG register is set to use automatic N value. However, the HSW_AUD_M_CTS_ENABLE register is

not

Quoted 2 lines

Quoted 2 lines

updated at all, and could potentially have stale stuff. Do we need to clear the AUD_M_CTS_M_VALUE_INDEX and/or

AUD_M_CTS_M_PROG_ENABLE bits?

Quoted 53 lines

Quoted 3 lines

We only do this stuff in hsw_audio_codec_enable now. Do we need to do something in hsw_audio_codec_disable too?

In _disable(), the audio doesn't work. So the value will not impact audio. It will not be worse that we do not reset the value in _disable(). Without reset, the value is "cached". Next time, it may be a little quicker for monitor response if we are using the same parameter, I think.

Regards
Libin

Quoted 41 lines

BR,
Jani.

Quoted 20 lines

Regards,
Libin

Quoted 16 lines

drivers/gpu/drm/i915/intel_audio.c hsw_dp_audio_config_update(struct intel_crtc *intel_crtc, enum port port, · line 327

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +10 -0

+
+	m = audio_config_dp_get_m(intel_crtc, rate);
+	if (m) {
+		tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+		tmp &= ~AUD_CONFIG_M_MASK;
+		tmp |= m;
+		tmp |= AUD_M_CTS_M_VALUE_INDEX;
+		tmp |= AUD_M_CTS_M_PROG_ENABLE;
+		I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+	}
 }

Thanks
Mengdong

Quoted 2 lines

Quoted 2 lines

drivers/gpu/drm/i915/intel_audio.c hsw_hdmi_audio_config_update(struct

Quoted 2 lines

static void

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -267,6 +356,12 @@ hsw_hdmi_audio_config_update(struct

intel_crtc

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/i915/intel_audio.c +6 -0

 *intel_crtc, enum port port,
 	}
 
 	I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+
+	tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+	tmp &= ~AUD_CONFIG_M_MASK;
+	tmp &= ~AUD_M_CTS_M_VALUE_INDEX;
+	tmp |= AUD_M_CTS_M_PROG_ENABLE;
+	I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
 }
 

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

static void

drivers/gpu/drm/i915/intel_audio.c +0 -0

@@ -687,7 +782,8 @@ static int

i915_audio_component_sync_audio_rate(struct device *kdev, int

port,

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

Quoted 21 lines

drivers/gpu/drm/i915/intel_audio.c static int i915_audio_component_sync_audio_rate(struct device *kdev, int port,

Quoted 7 lines

drivers/gpu/drm/i915/intel_audio.c +2 -1

 	/* 1. get the pipe */
 	intel_encoder = get_saved_enc(dev_priv, port, pipe);
 	if (!intel_encoder || !intel_encoder->base.crtc ||
-	    intel_encoder->type != INTEL_OUTPUT_HDMI) {
+	    (intel_encoder->type != INTEL_OUTPUT_HDMI &&
+	     intel_encoder->type != INTEL_OUTPUT_DP)) {
 		DRM_DEBUG_KMS("Not valid for port %c\n",

port_name(port));

Quoted 9 lines

drivers/gpu/drm/i915/intel_audio.c +0 -1

 		err = -ENODEV;
 		goto unlock;
--

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 2 lines

Quoted 2 lines

--
Jani Nikula, Intel Open Source Technology Center

--
Jani Nikula, Intel Open Source Technology Center

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

For HDMI, we found only set N is enough. HW then can handle the

Quoted 4 lines

remaining.

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why?

Which table are you meaning? I calculate the values myself. I didn't find the full table in DP spec. I only find the table for 270MHz and 162MHz in Table 2-50: Examples of Maud and Naud Values

Quoted 11 lines · Libin

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

For HDMI, we found only set N is enough. HW then can handle the

remaining.

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why?

Quoted 3 lines

Which table are you meaning? I calculate the values myself. I didn't find the full table in DP spec. I only find the table for 270MHz and 162MHz in Table 2-50: Examples of Maud and Naud Values

Table 2-104 in DP 1.4. Maybe you're looking at an older version of the spec.

-----Original Message-----
From: Ville Syrjälä [mailto:[email protected]]
Sent: Thursday, October 20, 2016 8:03 PM
To: Nikula, Jani <[email protected]>
Cc: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]; Zhang, Keqiao <[email protected]>; [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhao, Juan J <[email protected]>
Subject: Re: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 16 lines · Jani Nikula

Quoted 14 lines · Libin

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

For HDMI, we found only set N is enough. HW then can handle the

remaining.

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why?

Which table are you meaning? I calculate the values myself. I didn't find the full table in DP spec. I only find the table for 270MHz and 162MHz in Table 2-50: Examples of Maud and Naud Values

Quoted 2 lines

Table 2-104 in DP 1.4. Maybe you're looking at an older version of the spec.

So it looks like they used the same M value for all link frequencies in that example, which for 540M ends up being double what the minimal accurate value is. But as both M and N are doubled the ratio is still exactly the same.

And there is indeed a 64k and 128k bitrates added as well. Those are not classified under HBR audio. But as those can't be expressed via short audio descriptors in the EDID, I don't think they would be accepted by ALSA. Or am I wrong? I do wonder how they are supposed to be used though since there must be some way for the sink to advertise them, otherwise I can't see the point in adding them.

--
Ville Syrjälä
Intel OTC

-----Original Message-----
From: Nikula, Jani
Sent: Thursday, October 20, 2016 7:35 PM
To: Yang, Libin <[email protected]>; Lin, Mengdong <[email protected]>; [email protected]
Cc: [email protected]; Pandiyan, Dhinakaran <[email protected]>; Zhang, Keqiao <[email protected]>; Zhao, Juan J <[email protected]>
Subject: RE: [Intel-gfx] [PATCH RESEND 9/9] drm/i915: set proper N/M in modeset

Quoted 8 lines · Libin

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Any particular reason these M/N values are half of what they're in table
2-104 of DP 1.4 spec? (Admittedly the table is an informative example.)

For HDMI, we found only set N is enough. HW then can handle the

remaining.

Quoted 6 lines

Quoted 2 lines

I meant, the M and N values in this part of the dp_aud_n_m table are 1/2 of what they are in the DP spec table. Why?

Which table are you meaning? I calculate the values myself. I didn't find the full table in DP spec. I only find the table for 270MHz and 162MHz in Table 2-50: Examples of Maud and Naud Values

Table 2-104 in DP 1.4. Maybe you're looking at an older version of the spec.

BR,
Jani.

--
Jani Nikula, Intel Open Source Technology Center