* [PATCH v3 1/3] drm/i915: set proper N/M in modeset
@ 2016-08-04 7:58 libin.yang
2016-08-04 7:58 ` [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms libin.yang
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: libin.yang @ 2016-08-04 7:58 UTC (permalink / raw)
To: intel-gfx, jani.nikula, ville.syrjala, daniel.vetter, tiwai; +Cc: Libin Yang
From: Libin Yang <libin.yang@linux.intel.com>
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 <libin.yang@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 6 ++
drivers/gpu/drm/i915/intel_audio.c | 133 ++++++++++++++++++++++++++++++++-----
2 files changed, 123 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..2f9d00e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7351,6 +7351,12 @@ enum {
#define _HSW_AUD_CONFIG_B 0x65100
#define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe, _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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 _HSW_AUD_MISC_CTRL_A 0x65010
#define _HSW_AUD_MISC_CTRL_B 0x65110
#define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe, _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 6700a7b..04e7358 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -98,6 +98,35 @@ static const struct {
{ 192000, TMDS_297M, 20480, 247500 },
};
+#define LC_540M 540000
+#define LC_270M 270000
+#define LC_162M 162000
+static const struct {
+ int sample_rate;
+ int clock;
+ u16 n;
+ u16 m;
+} aud_nm[] = {
+ {192000, LC_540M, 5625, 1024},
+ {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},
+ {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},
+ {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},
+};
+
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted_mode)
{
@@ -121,20 +150,50 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
return hdmi_audio_clock[i].config;
}
-static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
+static int audio_config_get_n(struct intel_crtc *crtc,
+ const struct drm_display_mode *adjusted_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;
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
+ for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
+ if ((rate == aud_ncts[i].sample_rate) &&
+ (adjusted_mode->clock == aud_ncts[i].clock)) {
+ return aud_ncts[i].n;
+ }
}
}
+
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
+ for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
+ if ((rate == aud_nm[i].sample_rate) &&
+ (crtc->config->port_clock == aud_nm[i].clock)) {
+ return aud_nm[i].n;
+ }
+ }
+ }
+ return 0;
+}
+
+static int audio_config_get_m(struct intel_crtc *crtc, int rate)
+{
+ int i;
+
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
+ for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
+ if ((rate == aud_nm[i].sample_rate) &&
+ (crtc->config->port_clock == aud_nm[i].clock)) {
+ return aud_nm[i].m;
+ }
+ }
+ }
+
return 0;
}
-static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
+static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
+ int n, uint32_t val)
{
int n_low, n_up;
uint32_t tmp = val;
@@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |
AUD_CONFIG_N_PROG_ENABLE);
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
+ tmp |= AUD_CONFIG_N_VALUE_INDEX;
+ return tmp;
+}
+
+static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
+ int m, uint32_t val)
+{
+ uint32_t tmp = val;
+
+ if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
+ return 0;
+
+ tmp |= m;
+ tmp |= AUD_M_CTS_M_VALUE_INDEX;
+ tmp |= AUD_M_CTS_M_PROG_ENABLE;
+
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)
+ const struct drm_display_mode *adjusted_mode)
{
- if (((mode->clock == TMDS_297M) ||
- (mode->clock == TMDS_296M)) &&
+ if (((adjusted_mode->clock == TMDS_297M) ||
+ (adjusted_mode->clock == TMDS_296M)) &&
intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
return true;
+ else if (((crtc->config->port_clock == LC_540M) ||
+ (crtc->config->port_clock == LC_270M) ||
+ (crtc->config->port_clock == LC_162M)) &&
+ intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
+ return true;
else
return false;
}
@@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
struct intel_digital_port *intel_dig_port =
enc_to_dig_port(&encoder->base);
enum port port = intel_dig_port->port;
- uint32_t tmp;
+ uint32_t tmp, m;
int len, i;
int n, rate;
@@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
DRM_ERROR("invalid port: %d\n", port);
rate = 0;
}
- n = audio_config_get_n(adjusted_mode, rate);
+ n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
if (n != 0)
- tmp = audio_config_setup_n_reg(n, tmp);
+ tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
else
DRM_DEBUG_KMS("no suitable N value is found\n");
}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+ /* setup m value for DP */
+ if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
+ m = audio_config_get_m(intel_crtc, rate);
+ if (m != 0) {
+ tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+ tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
+ I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+ }
+ }
+
mutex_unlock(&dev_priv->av_mutex);
}
@@ -637,7 +728,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
struct drm_display_mode *mode;
struct i915_audio_component *acomp = dev_priv->audio_component;
enum pipe pipe = INVALID_PIPE;
- u32 tmp;
+ u32 tmp, m;
int n;
int err = 0;
@@ -653,7 +744,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
intel_encoder = dev_priv->dig_port_map[port];
/* intel_encoder might be NULL for DP MST */
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("no valid port %c\n", port_name(port));
err = -ENODEV;
goto unlock;
@@ -681,7 +773,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
goto unlock;
}
- n = audio_config_get_n(mode, rate);
+ n = audio_config_get_n(crtc, mode, rate);
if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
port_name(port));
@@ -693,8 +785,17 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
/* 3. set the N/CTS/M */
tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp = audio_config_setup_n_reg(n, tmp);
+ tmp = audio_config_setup_n_reg(crtc, n, tmp);
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
+ /* setup m value for DP */
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
+ m = audio_config_get_m(crtc, rate);
+ if (m == 0)
+ goto unlock;
+ tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
+ tmp = audio_config_setup_m_reg(crtc, m, tmp);
+ I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
+ }
unlock:
mutex_unlock(&dev_priv->av_mutex);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms
2016-08-04 7:58 [PATCH v3 1/3] drm/i915: set proper N/M in modeset libin.yang
@ 2016-08-04 7:58 ` libin.yang
2016-08-04 8:54 ` Ville Syrjälä
2016-08-04 7:58 ` [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock libin.yang
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: libin.yang @ 2016-08-04 7:58 UTC (permalink / raw)
To: intel-gfx, jani.nikula, ville.syrjala, daniel.vetter, tiwai; +Cc: Libin Yang
From: Libin Yang <libin.yang@linux.intel.com>
This patch applies setting proper N/M, N/CTS on more platforms.
Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
drivers/gpu/drm/i915/intel_audio.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 04e7358..7aa0203 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -732,11 +732,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
int n;
int err = 0;
- /* HSW, BDW, SKL, KBL need this fix */
- if (!IS_SKYLAKE(dev_priv) &&
- !IS_KABYLAKE(dev_priv) &&
- !IS_BROADWELL(dev_priv) &&
- !IS_HASWELL(dev_priv))
+ if (!HAS_DDI(dev_priv))
return 0;
mutex_lock(&dev_priv->av_mutex);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock
2016-08-04 7:58 [PATCH v3 1/3] drm/i915: set proper N/M in modeset libin.yang
2016-08-04 7:58 ` [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms libin.yang
@ 2016-08-04 7:58 ` libin.yang
2016-08-04 8:57 ` Ville Syrjälä
2016-08-04 8:52 ` [PATCH v3 1/3] drm/i915: set proper N/M in modeset Ville Syrjälä
2016-08-04 9:32 ` ✗ Ro.CI.BAT: failure for series starting with [v3,1/3] " Patchwork
3 siblings, 1 reply; 19+ messages in thread
From: libin.yang @ 2016-08-04 7:58 UTC (permalink / raw)
To: intel-gfx, jani.nikula, ville.syrjala, daniel.vetter, tiwai; +Cc: Libin Yang
From: Libin Yang <libin.yang@linux.intel.com>
HDMI audio should use crtc_clock to get the TMDS clock.
Besides, this patch renames mode to adjusted_mode to unify the name.
Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
---
drivers/gpu/drm/i915/intel_audio.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 7aa0203..bf02645 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -159,7 +159,7 @@ static int audio_config_get_n(struct intel_crtc *crtc,
if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&
- (adjusted_mode->clock == aud_ncts[i].clock)) {
+ (adjusted_mode->crtc_clock == aud_ncts[i].clock)) {
return aud_ncts[i].n;
}
}
@@ -228,8 +228,8 @@ static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
static bool audio_rate_need_prog(struct intel_crtc *crtc,
const struct drm_display_mode *adjusted_mode)
{
- if (((adjusted_mode->clock == TMDS_297M) ||
- (adjusted_mode->clock == TMDS_296M)) &&
+ if (((adjusted_mode->crtc_clock == TMDS_297M) ||
+ (adjusted_mode->crtc_clock == TMDS_296M)) &&
intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
return true;
else if (((crtc->config->port_clock == LC_540M) ||
@@ -725,7 +725,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
struct drm_i915_private *dev_priv = dev_to_i915(dev);
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;
enum pipe pipe = INVALID_PIPE;
u32 tmp, m;
@@ -756,20 +756,20 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));
- 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)) {
+ if (!audio_rate_need_prog(crtc, adjusted_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(crtc, mode, rate);
+ n = audio_config_get_n(crtc, adjusted_mode, rate);
if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
port_name(port));
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-04 7:58 [PATCH v3 1/3] drm/i915: set proper N/M in modeset libin.yang
2016-08-04 7:58 ` [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms libin.yang
2016-08-04 7:58 ` [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock libin.yang
@ 2016-08-04 8:52 ` Ville Syrjälä
2016-08-05 5:39 ` Yang, Libin
2016-08-04 9:32 ` ✗ Ro.CI.BAT: failure for series starting with [v3,1/3] " Patchwork
3 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-04 8:52 UTC (permalink / raw)
To: libin.yang; +Cc: tiwai, daniel.vetter, intel-gfx
On Thu, Aug 04, 2016 at 03:58:02PM +0800, libin.yang@linux.intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
>
> 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.
Pleas include a small changelog here so that I know what has
changed with each revision of the patch. Eg:
v2: blah
v3: whatever
>
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 6 ++
> drivers/gpu/drm/i915/intel_audio.c | 133 ++++++++++++++++++++++++++++++++-----
> 2 files changed, 123 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bfde75..2f9d00e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7351,6 +7351,12 @@ enum {
> #define _HSW_AUD_CONFIG_B 0x65100
> #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe, _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
Please try to keep the register defines in order by the register offset.
Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> +
> #define _HSW_AUD_MISC_CTRL_A 0x65010
> #define _HSW_AUD_MISC_CTRL_B 0x65110
> #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe, _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 6700a7b..04e7358 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -98,6 +98,35 @@ static const struct {
> { 192000, TMDS_297M, 20480, 247500 },
> };
>
> +#define LC_540M 540000
> +#define LC_270M 270000
> +#define LC_162M 162000
> +static const struct {
> + int sample_rate;
> + int clock;
> + u16 n;
> + u16 m;
> +} aud_nm[] = {
> + {192000, LC_540M, 5625, 1024},
^ ^
Style nit: please add spaces here
> + {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},
> + {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},
> + {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},
The numbers looks good. But looks like you forgot about 176.4 kHz?
> +};
> +
> /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted_mode)
> {
> @@ -121,20 +150,50 @@ static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
> return hdmi_audio_clock[i].config;
> }
>
> -static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
> +static int audio_config_get_n(struct intel_crtc *crtc,
> + const struct drm_display_mode *adjusted_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;
> + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> + if ((rate == aud_ncts[i].sample_rate) &&
> + (adjusted_mode->clock == aud_ncts[i].clock)) {
Indent fail, and a bit too many parens for my taste. Should look like:
if (rate == aud_ncts[i].sample_rate &&
adjusted_mode->clock == aud_ncts[i].clock) {
> + return aud_ncts[i].n;
> + }
> }
> }
> +
> + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
intel_has_dp_encoder() will make this work for MST as well.
"In MST mode, the Sink device must ignore the Maud and Mvid values sent
by an MST Source device unless the MST Sink device is directly connected
to the MST Source device via a single link as the Source device generates
those values based on the LS_Clk of the link it is driving. An MST Sink
device must be able to regenerate a stream clock without depending on Mvid
and Mvid time stamp values because the MST Sink device might be plugged
into an MST Branch device, not an MST Source device."
So if I'm reading that right, an MST sink can still use the M/N values
if it's direclty hooked up to a MST source. Hence we probably want to
fill out the M/N even for MST.
> + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> + if ((rate == aud_nm[i].sample_rate) &&
> + (crtc->config->port_clock == aud_nm[i].clock)) {
indent + parens
> + return aud_nm[i].n;
> + }
> + }
> + }
> + return 0;
> +}
> +
> +static int audio_config_get_m(struct intel_crtc *crtc, int rate)
> +{
> + int i;
> +
> + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
intel_has_dp_encoder()
> + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> + if ((rate == aud_nm[i].sample_rate) &&
> + (crtc->config->port_clock == aud_nm[i].clock)) {
indent + parens
> + return aud_nm[i].m;
> + }
> + }
> + }
> +
> return 0;
> }
>
> -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> + int n, uint32_t val)
> {
> int n_low, n_up;
> uint32_t tmp = val;
> @@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> AUD_CONFIG_N_PROG_ENABLE);
> + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
intel_has_dp_encoder()
> + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> + return tmp;
> +}
> +
> +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> + int m, uint32_t val)
> +{
> + uint32_t tmp = val;
> +
> + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> + return 0;
The caller already checked that.
> +
> + tmp |= m;
> + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> +
> 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)
> + const struct drm_display_mode *adjusted_mode)
> {
> - if (((mode->clock == TMDS_297M) ||
> - (mode->clock == TMDS_296M)) &&
> + if (((adjusted_mode->clock == TMDS_297M) ||
> + (adjusted_mode->clock == TMDS_296M)) &&
> intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> return true;
> + else if (((crtc->config->port_clock == LC_540M) ||
> + (crtc->config->port_clock == LC_270M) ||
> + (crtc->config->port_clock == LC_162M)) &&
> + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
intel_has_dp_encoder()
> + return true;
> else
> return false;
> }
> @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> struct intel_digital_port *intel_dig_port =
> enc_to_dig_port(&encoder->base);
> enum port port = intel_dig_port->port;
> - uint32_t tmp;
> + uint32_t tmp, m;
> int len, i;
> int n, rate;
>
> @@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct drm_connector *connector,
> DRM_ERROR("invalid port: %d\n", port);
> rate = 0;
> }
> - n = audio_config_get_n(adjusted_mode, rate);
> + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> if (n != 0)
> - tmp = audio_config_setup_n_reg(n, tmp);
> + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> else
> DRM_DEBUG_KMS("no suitable N value is found\n");
> }
>
> I915_WRITE(HSW_AUD_CFG(pipe), tmp);
>
> + /* setup m value for DP */
> + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
intel_has_dp_encoder()
> + m = audio_config_get_m(intel_crtc, rate);
> + if (m != 0) {
> + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
We should program this register even for HDMI, so that we don't leak
invalid register values eg. when changing from DP->HDMI.
> + }
> + }
> +
> mutex_unlock(&dev_priv->av_mutex);
> }
>
> @@ -637,7 +728,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> struct drm_display_mode *mode;
> struct i915_audio_component *acomp = dev_priv->audio_component;
> enum pipe pipe = INVALID_PIPE;
> - u32 tmp;
> + u32 tmp, m;
> int n;
> int err = 0;
>
> @@ -653,7 +744,8 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> intel_encoder = dev_priv->dig_port_map[port];
> /* intel_encoder might be NULL for DP MST */
> 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))) {
Hmm. Should probablt move this stuff later so that we can use check
crtc->config->output_types instead. But I guess that's more
appropriately left to any MST patch.
> DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> err = -ENODEV;
> goto unlock;
> @@ -681,7 +773,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> goto unlock;
> }
>
> - n = audio_config_get_n(mode, rate);
> + n = audio_config_get_n(crtc, mode, rate);
> if (n == 0) {
> DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
> port_name(port));
> @@ -693,8 +785,17 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>
> /* 3. set the N/CTS/M */
> tmp = I915_READ(HSW_AUD_CFG(pipe));
> - tmp = audio_config_setup_n_reg(n, tmp);
> + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> + /* setup m value for DP */
> + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
intel_has_dp_encoder()
> + m = audio_config_get_m(crtc, rate);
> + if (m == 0)
> + goto unlock;
> + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> + }
>
> unlock:
> mutex_unlock(&dev_priv->av_mutex);
> --
> 1.9.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms
2016-08-04 7:58 ` [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms libin.yang
@ 2016-08-04 8:54 ` Ville Syrjälä
2016-08-05 5:41 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-04 8:54 UTC (permalink / raw)
To: libin.yang; +Cc: tiwai, daniel.vetter, intel-gfx
On Thu, Aug 04, 2016 at 03:58:03PM +0800, libin.yang@linux.intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
>
> This patch applies setting proper N/M, N/CTS on more platforms.
>
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Not that I particularly like this special casing of platforms. We should
really try to unify this across the board, and I thought we already
agreed that that was going to happen?
> ---
> drivers/gpu/drm/i915/intel_audio.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 04e7358..7aa0203 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -732,11 +732,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> int n;
> int err = 0;
>
> - /* HSW, BDW, SKL, KBL need this fix */
> - if (!IS_SKYLAKE(dev_priv) &&
> - !IS_KABYLAKE(dev_priv) &&
> - !IS_BROADWELL(dev_priv) &&
> - !IS_HASWELL(dev_priv))
> + if (!HAS_DDI(dev_priv))
> return 0;
>
> mutex_lock(&dev_priv->av_mutex);
> --
> 1.9.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock
2016-08-04 7:58 ` [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock libin.yang
@ 2016-08-04 8:57 ` Ville Syrjälä
0 siblings, 0 replies; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-04 8:57 UTC (permalink / raw)
To: libin.yang; +Cc: tiwai, daniel.vetter, intel-gfx
On Thu, Aug 04, 2016 at 03:58:04PM +0800, libin.yang@linux.intel.com wrote:
> From: Libin Yang <libin.yang@linux.intel.com>
>
> HDMI audio should use crtc_clock to get the TMDS clock.
>
> Besides, this patch renames mode to adjusted_mode to unify the name.
>
> Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_audio.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 7aa0203..bf02645 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -159,7 +159,7 @@ static int audio_config_get_n(struct intel_crtc *crtc,
> if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> if ((rate == aud_ncts[i].sample_rate) &&
> - (adjusted_mode->clock == aud_ncts[i].clock)) {
> + (adjusted_mode->crtc_clock == aud_ncts[i].clock)) {
> return aud_ncts[i].n;
> }
> }
> @@ -228,8 +228,8 @@ static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> static bool audio_rate_need_prog(struct intel_crtc *crtc,
> const struct drm_display_mode *adjusted_mode)
> {
> - if (((adjusted_mode->clock == TMDS_297M) ||
> - (adjusted_mode->clock == TMDS_296M)) &&
> + if (((adjusted_mode->crtc_clock == TMDS_297M) ||
> + (adjusted_mode->crtc_clock == TMDS_296M)) &&
Indentation is still a bit off here, but that looks like a more systemic
problem for intel_audio.c. Probably better to clean it all up
separately.
So yeah, this patch looks all right
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> return true;
> else if (((crtc->config->port_clock == LC_540M) ||
> @@ -725,7 +725,7 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
> struct drm_i915_private *dev_priv = dev_to_i915(dev);
> 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;
> enum pipe pipe = INVALID_PIPE;
> u32 tmp, m;
> @@ -756,20 +756,20 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
>
> DRM_DEBUG_KMS("pipe %c connects port %c\n",
> pipe_name(pipe), port_name(port));
> - 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)) {
> + if (!audio_rate_need_prog(crtc, adjusted_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(crtc, mode, rate);
> + n = audio_config_get_n(crtc, adjusted_mode, rate);
> if (n == 0) {
> DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",
> port_name(port));
> --
> 1.9.1
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* ✗ Ro.CI.BAT: failure for series starting with [v3,1/3] drm/i915: set proper N/M in modeset
2016-08-04 7:58 [PATCH v3 1/3] drm/i915: set proper N/M in modeset libin.yang
` (2 preceding siblings ...)
2016-08-04 8:52 ` [PATCH v3 1/3] drm/i915: set proper N/M in modeset Ville Syrjälä
@ 2016-08-04 9:32 ` Patchwork
3 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2016-08-04 9:32 UTC (permalink / raw)
To: libin.yang; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v3,1/3] drm/i915: set proper N/M in modeset
URL : https://patchwork.freedesktop.org/series/10645/
State : failure
== Summary ==
Series 10645v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/10645/revisions/1/mbox
Test kms_cursor_legacy:
Subgroup basic-cursor-vs-flip-varying-size:
fail -> PASS (ro-ilk1-i5-650)
Subgroup basic-flip-vs-cursor-legacy:
fail -> PASS (ro-bdw-i7-5600u)
fail -> PASS (ro-bdw-i5-5250u)
pass -> FAIL (ro-skl3-i5-6260u)
fi-kbl-qkkr total:240 pass:181 dwarn:29 dfail:0 fail:3 skip:27
fi-skl-i5-6260u total:240 pass:224 dwarn:0 dfail:0 fail:2 skip:14
fi-skl-i7-6700k total:240 pass:208 dwarn:0 dfail:0 fail:4 skip:28
fi-snb-i7-2600 total:240 pass:198 dwarn:0 dfail:0 fail:0 skip:42
ro-bdw-i5-5250u total:240 pass:219 dwarn:4 dfail:0 fail:1 skip:16
ro-bdw-i7-5557U total:240 pass:224 dwarn:0 dfail:0 fail:0 skip:16
ro-bdw-i7-5600u total:240 pass:207 dwarn:0 dfail:0 fail:1 skip:32
ro-bsw-n3050 total:240 pass:194 dwarn:0 dfail:0 fail:4 skip:42
ro-byt-n2820 total:240 pass:197 dwarn:0 dfail:0 fail:3 skip:40
ro-hsw-i3-4010u total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-hsw-i7-4770r total:240 pass:214 dwarn:0 dfail:0 fail:0 skip:26
ro-ilk-i7-620lm total:240 pass:173 dwarn:1 dfail:0 fail:1 skip:65
ro-ilk1-i5-650 total:235 pass:174 dwarn:0 dfail:0 fail:1 skip:60
ro-ivb-i7-3770 total:240 pass:205 dwarn:0 dfail:0 fail:0 skip:35
ro-ivb2-i7-3770 total:240 pass:209 dwarn:0 dfail:0 fail:0 skip:31
ro-skl3-i5-6260u total:240 pass:222 dwarn:0 dfail:0 fail:4 skip:14
ro-snb-i7-2620M total:240 pass:198 dwarn:0 dfail:0 fail:1 skip:41
fi-hsw-i7-4770k failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1701/
4a66cc6 drm-intel-nightly: 2016y-08m-04d-08h-20m-44s UTC integration manifest
e795b85 drm/i915: HDMI audio gets the TMDS clock by crtc_clock
683ab01 drm/i915: set proper N/MCTS on more platforms
fc34824 drm/i915: set proper N/M in modeset
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-04 8:52 ` [PATCH v3 1/3] drm/i915: set proper N/M in modeset Ville Syrjälä
@ 2016-08-05 5:39 ` Yang, Libin
2016-08-05 5:56 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 5:39 UTC (permalink / raw)
To: Ville Syrjälä, libin.yang; +Cc: tiwai, Vetter, Daniel, intel-gfx
Hi Ville,
Sorry for delay reply. Please see my comments below.
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, August 4, 2016 4:53 PM
> To: libin.yang@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Vetter, Daniel
> <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin <libin.yang@intel.com>
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Thu, Aug 04, 2016 at 03:58:02PM +0800, libin.yang@linux.intel.com wrote:
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > 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.
>
> Pleas include a small changelog here so that I know what has changed with
> each revision of the patch. Eg:
>
> v2: blah
> v3: whatever
>
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > drivers/gpu/drm/i915/intel_audio.c | 133
> > ++++++++++++++++++++++++++++++++-----
> > 2 files changed, 123 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7351,6 +7351,12 @@ enum {
> > #define _HSW_AUD_CONFIG_B 0x65100
> > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
>
> Please try to keep the register defines in order by the register offset.
> Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
>
> > +
> > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 6700a7b..04e7358 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -98,6 +98,35 @@ static const struct {
> > { 192000, TMDS_297M, 20480, 247500 }, };
> >
> > +#define LC_540M 540000
> > +#define LC_270M 270000
> > +#define LC_162M 162000
> > +static const struct {
> > + int sample_rate;
> > + int clock;
> > + u16 n;
> > + u16 m;
> > +} aud_nm[] = {
> > + {192000, LC_540M, 5625, 1024},
> ^ ^
>
> Style nit: please add spaces here
>
> > + {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},
> > + {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},
> > + {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},
>
> The numbers looks good. But looks like you forgot about 176.4 kHz?
>
> > +};
> > +
> > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32
> > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
> > return hdmi_audio_clock[i].config;
> > }
> >
> > -static int audio_config_get_n(const struct drm_display_mode *mode,
> > int rate)
> > +static int audio_config_get_n(struct intel_crtc *crtc,
> > + const struct drm_display_mode *adjusted_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;
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > + if ((rate == aud_ncts[i].sample_rate) &&
> > + (adjusted_mode->clock == aud_ncts[i].clock)) {
>
> Indent fail, and a bit too many parens for my taste. Should look like:
>
> if (rate == aud_ncts[i].sample_rate &&
> adjusted_mode->clock == aud_ncts[i].clock) {
>
>
> > + return aud_ncts[i].n;
> > + }
> > }
> > }
> > +
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
>
> intel_has_dp_encoder() will make this work for MST as well.
>
> "In MST mode, the Sink device must ignore the Maud and Mvid values sent by
> an MST Source device unless the MST Sink device is directly connected to the
> MST Source device via a single link as the Source device generates those values
> based on the LS_Clk of the link it is driving. An MST Sink device must be able to
> regenerate a stream clock without depending on Mvid and Mvid time stamp
> values because the MST Sink device might be plugged into an MST Branch
> device, not an MST Source device."
>
> So if I'm reading that right, an MST sink can still use the M/N values if it's
> direclty hooked up to a MST source. Hence we probably want to fill out the
> M/N even for MST.
My understanding is this is the requirement for sink device.
Non-mst and MST, source will send the N/M value, and if it is
Non-mst/single link, sink device should use these data to generate
the clock. If it is in MST, sink device should ignore the data and regenerate
the clock without depending on N/M value.
So this code should OK be for both non-mst and MST. If not, I think we
can make a separate patch for DP MST.
>
> > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > + if ((rate == aud_nm[i].sample_rate) &&
> > + (crtc->config->port_clock == aud_nm[i].clock))
> {
>
> indent + parens
>
> > + return aud_nm[i].n;
> > + }
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > + int i;
> > +
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
>
> intel_has_dp_encoder()
>
> > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > + if ((rate == aud_nm[i].sample_rate) &&
> > + (crtc->config->port_clock == aud_nm[i].clock))
> {
>
> indent + parens
>
> > + return aud_nm[i].m;
> > + }
> > + }
> > + }
> > +
> > return 0;
> > }
> >
> > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > + int n, uint32_t val)
> > {
> > int n_low, n_up;
> > uint32_t tmp = val;
> > @@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int n,
> uint32_t val)
> > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > AUD_CONFIG_N_PROG_ENABLE);
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
>
> intel_has_dp_encoder()
>
> > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > + return tmp;
> > +}
> > +
> > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > + int m, uint32_t val)
> > +{
> > + uint32_t tmp = val;
> > +
> > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > + return 0;
>
> The caller already checked that.
>
> > +
> > + tmp |= m;
> > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > +
> > 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)
> > + const struct drm_display_mode
> *adjusted_mode)
> > {
> > - if (((mode->clock == TMDS_297M) ||
> > - (mode->clock == TMDS_296M)) &&
> > + if (((adjusted_mode->clock == TMDS_297M) ||
> > + (adjusted_mode->clock == TMDS_296M)) &&
> > intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > return true;
> > + else if (((crtc->config->port_clock == LC_540M) ||
> > + (crtc->config->port_clock == LC_270M) ||
> > + (crtc->config->port_clock == LC_162M)) &&
> > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
>
> intel_has_dp_encoder()
>
> > + return true;
> > else
> > return false;
> > }
> > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> > struct intel_digital_port *intel_dig_port =
> > enc_to_dig_port(&encoder->base);
> > enum port port = intel_dig_port->port;
> > - uint32_t tmp;
> > + uint32_t tmp, m;
> > int len, i;
> > int n, rate;
> >
> > @@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct
> drm_connector *connector,
> > DRM_ERROR("invalid port: %d\n", port);
> > rate = 0;
> > }
> > - n = audio_config_get_n(adjusted_mode, rate);
> > + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> > if (n != 0)
> > - tmp = audio_config_setup_n_reg(n, tmp);
> > + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> > else
> > DRM_DEBUG_KMS("no suitable N value is found\n");
> > }
> >
> > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> >
> > + /* setup m value for DP */
> > + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
>
> intel_has_dp_encoder()
>
> > + m = audio_config_get_m(intel_crtc, rate);
> > + if (m != 0) {
> > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
>
> We should program this register even for HDMI, so that we don't leak invalid
> register values eg. when changing from DP->HDMI.
HDMI doesn't need set these values based on our test. It seems silicon can
handle smoothly for HDMI.
>
> > + }
> > + }
> > +
> > mutex_unlock(&dev_priv->av_mutex);
> > }
> >
> > @@ -637,7 +728,7 @@ static int
> i915_audio_component_sync_audio_rate(struct device *dev,
> > struct drm_display_mode *mode;
> > struct i915_audio_component *acomp = dev_priv->audio_component;
> > enum pipe pipe = INVALID_PIPE;
> > - u32 tmp;
> > + u32 tmp, m;
> > int n;
> > int err = 0;
> >
> > @@ -653,7 +744,8 @@ static int
> i915_audio_component_sync_audio_rate(struct device *dev,
> > intel_encoder = dev_priv->dig_port_map[port];
> > /* intel_encoder might be NULL for DP MST */
> > 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))) {
>
> Hmm. Should probablt move this stuff later so that we can use check
> crtc->config->output_types instead. But I guess that's more
> appropriately left to any MST patch.
The same reason as intel_has_dp_encoder(). And if it is wrong
for DP MST, let's have a separate patch when we support DP MST.
At that time, we can do a full test on DP MST.
Regards,
Libin
>
> > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > err = -ENODEV;
> > goto unlock;
> > @@ -681,7 +773,7 @@ static int
> i915_audio_component_sync_audio_rate(struct device *dev,
> > goto unlock;
> > }
> >
> > - n = audio_config_get_n(mode, rate);
> > + n = audio_config_get_n(crtc, mode, rate);
> > if (n == 0) {
> > DRM_DEBUG_KMS("Using automatic mode for N value on
> port %c\n",
> > port_name(port));
> > @@ -693,8 +785,17 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> >
> > /* 3. set the N/CTS/M */
> > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > - tmp = audio_config_setup_n_reg(n, tmp);
> > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > + /* setup m value for DP */
> > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
>
> intel_has_dp_encoder()
>
> > + m = audio_config_get_m(crtc, rate);
> > + if (m == 0)
> > + goto unlock;
> > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > + }
> >
> > unlock:
> > mutex_unlock(&dev_priv->av_mutex);
> > --
> > 1.9.1
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms
2016-08-04 8:54 ` Ville Syrjälä
@ 2016-08-05 5:41 ` Yang, Libin
0 siblings, 0 replies; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 5:41 UTC (permalink / raw)
To: Ville Syrjälä, libin.yang; +Cc: tiwai, Vetter, Daniel, intel-gfx
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, August 4, 2016 4:54 PM
> To: libin.yang@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Vetter, Daniel
> <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin <libin.yang@intel.com>
> Subject: Re: [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms
>
> On Thu, Aug 04, 2016 at 03:58:03PM +0800, libin.yang@linux.intel.com wrote:
> > From: Libin Yang <libin.yang@linux.intel.com>
> >
> > This patch applies setting proper N/M, N/CTS on more platforms.
> >
> > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Not that I particularly like this special casing of platforms. We should really try
> to unify this across the board, and I thought we already agreed that that was
> going to happen?
Yes, and based on the spec, even it is not supporting 4K resolution,
the n/mcts should also be OK. It is for clock, not for resolution.
So I agree to apply the patches to other platforms.
Regards,
Libin
>
> > ---
> > drivers/gpu/drm/i915/intel_audio.c | 6 +-----
> > 1 file changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > b/drivers/gpu/drm/i915/intel_audio.c
> > index 04e7358..7aa0203 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -732,11 +732,7 @@ static int
> i915_audio_component_sync_audio_rate(struct device *dev,
> > int n;
> > int err = 0;
> >
> > - /* HSW, BDW, SKL, KBL need this fix */
> > - if (!IS_SKYLAKE(dev_priv) &&
> > - !IS_KABYLAKE(dev_priv) &&
> > - !IS_BROADWELL(dev_priv) &&
> > - !IS_HASWELL(dev_priv))
> > + if (!HAS_DDI(dev_priv))
> > return 0;
> >
> > mutex_lock(&dev_priv->av_mutex);
> > --
> > 1.9.1
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 5:39 ` Yang, Libin
@ 2016-08-05 5:56 ` Ville Syrjälä
2016-08-05 6:16 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-05 5:56 UTC (permalink / raw)
To: Yang, Libin; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> Hi Ville,
>
> Sorry for delay reply. Please see my comments below.
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, August 4, 2016 4:53 PM
> > To: libin.yang@linux.intel.com
> > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com; Vetter, Daniel
> > <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin <libin.yang@intel.com>
> > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> >
> > On Thu, Aug 04, 2016 at 03:58:02PM +0800, libin.yang@linux.intel.com wrote:
> > > From: Libin Yang <libin.yang@linux.intel.com>
> > >
> > > 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.
> >
> > Pleas include a small changelog here so that I know what has changed with
> > each revision of the patch. Eg:
> >
> > v2: blah
> > v3: whatever
> >
> > >
> > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > ++++++++++++++++++++++++++++++++-----
> > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7351,6 +7351,12 @@ enum {
> > > #define _HSW_AUD_CONFIG_B 0x65100
> > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> >
> > Please try to keep the register defines in order by the register offset.
> > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> >
> > > +
> > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > b/drivers/gpu/drm/i915/intel_audio.c
> > > index 6700a7b..04e7358 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -98,6 +98,35 @@ static const struct {
> > > { 192000, TMDS_297M, 20480, 247500 }, };
> > >
> > > +#define LC_540M 540000
> > > +#define LC_270M 270000
> > > +#define LC_162M 162000
> > > +static const struct {
> > > + int sample_rate;
> > > + int clock;
> > > + u16 n;
> > > + u16 m;
> > > +} aud_nm[] = {
> > > + {192000, LC_540M, 5625, 1024},
> > ^ ^
> >
> > Style nit: please add spaces here
> >
> > > + {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},
> > > + {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},
> > > + {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},
> >
> > The numbers looks good. But looks like you forgot about 176.4 kHz?
> >
> > > +};
> > > +
> > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32
> > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > audio_config_hdmi_pixel_clock(const struct drm_display_mode *adjusted
> > > return hdmi_audio_clock[i].config;
> > > }
> > >
> > > -static int audio_config_get_n(const struct drm_display_mode *mode,
> > > int rate)
> > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > + const struct drm_display_mode *adjusted_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;
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > + (adjusted_mode->clock == aud_ncts[i].clock)) {
> >
> > Indent fail, and a bit too many parens for my taste. Should look like:
> >
> > if (rate == aud_ncts[i].sample_rate &&
> > adjusted_mode->clock == aud_ncts[i].clock) {
> >
> >
> > > + return aud_ncts[i].n;
> > > + }
> > > }
> > > }
> > > +
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> >
> > intel_has_dp_encoder() will make this work for MST as well.
> >
> > "In MST mode, the Sink device must ignore the Maud and Mvid values sent by
> > an MST Source device unless the MST Sink device is directly connected to the
> > MST Source device via a single link as the Source device generates those values
> > based on the LS_Clk of the link it is driving. An MST Sink device must be able to
> > regenerate a stream clock without depending on Mvid and Mvid time stamp
> > values because the MST Sink device might be plugged into an MST Branch
> > device, not an MST Source device."
> >
> > So if I'm reading that right, an MST sink can still use the M/N values if it's
> > direclty hooked up to a MST source. Hence we probably want to fill out the
> > M/N even for MST.
>
> My understanding is this is the requirement for sink device.
> Non-mst and MST, source will send the N/M value, and if it is
> Non-mst/single link, sink device should use these data to generate
> the clock. If it is in MST, sink device should ignore the data and regenerate
> the clock without depending on N/M value.
>
> So this code should OK be for both non-mst and MST. If not, I think we
> can make a separate patch for DP MST.
It's only OK for SST. We could of course delay dealing with MST, but
that seems like unnecessary churn. You're touching all these places in
this patch, so might as well use intel_dp_encoder() from the start. It
will make the MST patch smaller too, which will make it easier to
review.
>
> >
> > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > + if ((rate == aud_nm[i].sample_rate) &&
> > > + (crtc->config->port_clock == aud_nm[i].clock))
> > {
> >
> > indent + parens
> >
> > > + return aud_nm[i].n;
> > > + }
> > > + }
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > + int i;
> > > +
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> >
> > intel_has_dp_encoder()
> >
> > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > + if ((rate == aud_nm[i].sample_rate) &&
> > > + (crtc->config->port_clock == aud_nm[i].clock))
> > {
> >
> > indent + parens
> >
> > > + return aud_nm[i].m;
> > > + }
> > > + }
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > + int n, uint32_t val)
> > > {
> > > int n_low, n_up;
> > > uint32_t tmp = val;
> > > @@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int n,
> > uint32_t val)
> > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > AUD_CONFIG_N_PROG_ENABLE);
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> >
> > intel_has_dp_encoder()
> >
> > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > + return tmp;
> > > +}
> > > +
> > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > + int m, uint32_t val)
> > > +{
> > > + uint32_t tmp = val;
> > > +
> > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > + return 0;
> >
> > The caller already checked that.
> >
> > > +
> > > + tmp |= m;
> > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > +
> > > 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)
> > > + const struct drm_display_mode
> > *adjusted_mode)
> > > {
> > > - if (((mode->clock == TMDS_297M) ||
> > > - (mode->clock == TMDS_296M)) &&
> > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > return true;
> > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > + (crtc->config->port_clock == LC_270M) ||
> > > + (crtc->config->port_clock == LC_162M)) &&
> > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> >
> > intel_has_dp_encoder()
> >
> > > + return true;
> > > else
> > > return false;
> > > }
> > > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> > drm_connector *connector,
> > > struct intel_digital_port *intel_dig_port =
> > > enc_to_dig_port(&encoder->base);
> > > enum port port = intel_dig_port->port;
> > > - uint32_t tmp;
> > > + uint32_t tmp, m;
> > > int len, i;
> > > int n, rate;
> > >
> > > @@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct
> > drm_connector *connector,
> > > DRM_ERROR("invalid port: %d\n", port);
> > > rate = 0;
> > > }
> > > - n = audio_config_get_n(adjusted_mode, rate);
> > > + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> > > if (n != 0)
> > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> > > else
> > > DRM_DEBUG_KMS("no suitable N value is found\n");
> > > }
> > >
> > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > >
> > > + /* setup m value for DP */
> > > + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
> >
> > intel_has_dp_encoder()
> >
> > > + m = audio_config_get_m(intel_crtc, rate);
> > > + if (m != 0) {
> > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> >
> > We should program this register even for HDMI, so that we don't leak invalid
> > register values eg. when changing from DP->HDMI.
>
> HDMI doesn't need set these values based on our test. It seems silicon can
> handle smoothly for HDMI.
Yes, but we nee to make sure we clear whatever we programmed in for DP
previously.
>
> >
> > > + }
> > > + }
> > > +
> > > mutex_unlock(&dev_priv->av_mutex);
> > > }
> > >
> > > @@ -637,7 +728,7 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > struct drm_display_mode *mode;
> > > struct i915_audio_component *acomp = dev_priv->audio_component;
> > > enum pipe pipe = INVALID_PIPE;
> > > - u32 tmp;
> > > + u32 tmp, m;
> > > int n;
> > > int err = 0;
> > >
> > > @@ -653,7 +744,8 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > intel_encoder = dev_priv->dig_port_map[port];
> > > /* intel_encoder might be NULL for DP MST */
> > > 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))) {
> >
> > Hmm. Should probablt move this stuff later so that we can use check
> > crtc->config->output_types instead. But I guess that's more
> > appropriately left to any MST patch.
>
> The same reason as intel_has_dp_encoder(). And if it is wrong
> for DP MST, let's have a separate patch when we support DP MST.
> At that time, we can do a full test on DP MST.
Yeah this part we can leave for the MST patch as it'll involve a bit
more than just adding another check to the if statement.
>
> Regards,
> Libin
>
> >
> > > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > err = -ENODEV;
> > > goto unlock;
> > > @@ -681,7 +773,7 @@ static int
> > i915_audio_component_sync_audio_rate(struct device *dev,
> > > goto unlock;
> > > }
> > >
> > > - n = audio_config_get_n(mode, rate);
> > > + n = audio_config_get_n(crtc, mode, rate);
> > > if (n == 0) {
> > > DRM_DEBUG_KMS("Using automatic mode for N value on
> > port %c\n",
> > > port_name(port));
> > > @@ -693,8 +785,17 @@ static int
> > > i915_audio_component_sync_audio_rate(struct device *dev,
> > >
> > > /* 3. set the N/CTS/M */
> > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > + /* setup m value for DP */
> > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> >
> > intel_has_dp_encoder()
> >
> > > + m = audio_config_get_m(crtc, rate);
> > > + if (m == 0)
> > > + goto unlock;
> > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > + }
> > >
> > > unlock:
> > > mutex_unlock(&dev_priv->av_mutex);
> > > --
> > > 1.9.1
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 5:56 ` Ville Syrjälä
@ 2016-08-05 6:16 ` Yang, Libin
2016-08-05 6:36 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 6:16 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
Hi Ville,
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, August 5, 2016 1:57 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> tiwai@suse.de
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > Sorry for delay reply. Please see my comments below.
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Thursday, August 4, 2016 4:53 PM
> > > To: libin.yang@linux.intel.com
> > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com;
> > > Vetter, Daniel <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > <libin.yang@intel.com>
> > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > >
> > > On Thu, Aug 04, 2016 at 03:58:02PM +0800, libin.yang@linux.intel.com
> wrote:
> > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > >
> > > > 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.
> > >
> > > Pleas include a small changelog here so that I know what has changed
> > > with each revision of the patch. Eg:
> > >
> > > v2: blah
> > > v3: whatever
> > >
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > ++++++++++++++++++++++++++++++++-----
> > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -7351,6 +7351,12 @@ enum {
> > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > >
> > > Please try to keep the register defines in order by the register offset.
> > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > >
> > > > +
> > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 6700a7b..04e7358 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -98,6 +98,35 @@ static const struct {
> > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > >
> > > > +#define LC_540M 540000
> > > > +#define LC_270M 270000
> > > > +#define LC_162M 162000
> > > > +static const struct {
> > > > + int sample_rate;
> > > > + int clock;
> > > > + u16 n;
> > > > + u16 m;
> > > > +} aud_nm[] = {
> > > > + {192000, LC_540M, 5625, 1024},
> > > ^ ^
> > >
> > > Style nit: please add spaces here
> > >
> > > > + {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},
> > > > + {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},
> > > > + {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},
> > >
> > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > >
> > > > +};
> > > > +
> > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static
> > > > u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> *adjusted
> > > > return hdmi_audio_clock[i].config; }
> > > >
> > > > -static int audio_config_get_n(const struct drm_display_mode
> > > > *mode, int rate)
> > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > + const struct drm_display_mode *adjusted_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;
> > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > + (adjusted_mode->clock == aud_ncts[i].clock)) {
> > >
> > > Indent fail, and a bit too many parens for my taste. Should look like:
> > >
> > > if (rate == aud_ncts[i].sample_rate &&
> > > adjusted_mode->clock == aud_ncts[i].clock) {
> > >
> > >
> > > > + return aud_ncts[i].n;
> > > > + }
> > > > }
> > > > }
> > > > +
> > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > >
> > > intel_has_dp_encoder() will make this work for MST as well.
> > >
> > > "In MST mode, the Sink device must ignore the Maud and Mvid values
> > > sent by an MST Source device unless the MST Sink device is directly
> > > connected to the MST Source device via a single link as the Source
> > > device generates those values based on the LS_Clk of the link it is
> > > driving. An MST Sink device must be able to regenerate a stream
> > > clock without depending on Mvid and Mvid time stamp values because
> > > the MST Sink device might be plugged into an MST Branch device, not an
> MST Source device."
> > >
> > > So if I'm reading that right, an MST sink can still use the M/N
> > > values if it's direclty hooked up to a MST source. Hence we probably
> > > want to fill out the M/N even for MST.
> >
> > My understanding is this is the requirement for sink device.
> > Non-mst and MST, source will send the N/M value, and if it is
> > Non-mst/single link, sink device should use these data to generate the
> > clock. If it is in MST, sink device should ignore the data and
> > regenerate the clock without depending on N/M value.
> >
> > So this code should OK be for both non-mst and MST. If not, I think we
> > can make a separate patch for DP MST.
>
> It's only OK for SST. We could of course delay dealing with MST, but that seems
> like unnecessary churn. You're touching all these places in this patch, so might
> as well use intel_dp_encoder() from the start. It will make the MST patch
> smaller too, which will make it easier to review.
OK, I will use intel_crtc_has_dp_encoder(). Hope it still works for DP MST :)
>
> >
> > >
> > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > + (crtc->config->port_clock == aud_nm[i].clock))
> > > {
> > >
> > > indent + parens
> > >
> > > > + return aud_nm[i].n;
> > > > + }
> > > > + }
> > > > + }
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > + int i;
> > > > +
> > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > >
> > > intel_has_dp_encoder()
> > >
> > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > + (crtc->config->port_clock == aud_nm[i].clock))
> > > {
> > >
> > > indent + parens
> > >
> > > > + return aud_nm[i].m;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > + int n, uint32_t val)
> > > > {
> > > > int n_low, n_up;
> > > > uint32_t tmp = val;
> > > > @@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int
> > > > n,
> > > uint32_t val)
> > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > >
> > > intel_has_dp_encoder()
> > >
> > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > + return tmp;
> > > > +}
> > > > +
> > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > + int m, uint32_t val)
> > > > +{
> > > > + uint32_t tmp = val;
> > > > +
> > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > + return 0;
> > >
> > > The caller already checked that.
> > >
> > > > +
> > > > + tmp |= m;
> > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > +
> > > > 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)
> > > > + const struct drm_display_mode
> > > *adjusted_mode)
> > > > {
> > > > - if (((mode->clock == TMDS_297M) ||
> > > > - (mode->clock == TMDS_296M)) &&
> > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > > return true;
> > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > + (crtc->config->port_clock == LC_270M) ||
> > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > >
> > > intel_has_dp_encoder()
> > >
> > > > + return true;
> > > > else
> > > > return false;
> > > > }
> > > > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> > > drm_connector *connector,
> > > > struct intel_digital_port *intel_dig_port =
> > > > enc_to_dig_port(&encoder->base);
> > > > enum port port = intel_dig_port->port;
> > > > - uint32_t tmp;
> > > > + uint32_t tmp, m;
> > > > int len, i;
> > > > int n, rate;
> > > >
> > > > @@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct
> > > drm_connector *connector,
> > > > DRM_ERROR("invalid port: %d\n", port);
> > > > rate = 0;
> > > > }
> > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> > > > if (n != 0)
> > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> > > > else
> > > > DRM_DEBUG_KMS("no suitable N value is found\n");
> > > > }
> > > >
> > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > >
> > > > + /* setup m value for DP */
> > > > + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
> > >
> > > intel_has_dp_encoder()
> > >
> > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > + if (m != 0) {
> > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > >
> > > We should program this register even for HDMI, so that we don't leak
> > > invalid register values eg. when changing from DP->HDMI.
> >
> > HDMI doesn't need set these values based on our test. It seems silicon
> > can handle smoothly for HDMI.
>
> Yes, but we nee to make sure we clear whatever we programmed in for DP
> previously.
The silicon seems to be able to handle the situation of DP -> HDMI and HDMI ->
DP. What's more it can handle sample rate/tmds changes.
Regards,
Libin
>
> >
> > >
> > > > + }
> > > > + }
> > > > +
> > > > mutex_unlock(&dev_priv->av_mutex);
> > > > }
> > > >
> > > > @@ -637,7 +728,7 @@ static int
> > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > struct drm_display_mode *mode;
> > > > struct i915_audio_component *acomp = dev_priv->audio_component;
> > > > enum pipe pipe = INVALID_PIPE;
> > > > - u32 tmp;
> > > > + u32 tmp, m;
> > > > int n;
> > > > int err = 0;
> > > >
> > > > @@ -653,7 +744,8 @@ static int
> > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > /* intel_encoder might be NULL for DP MST */
> > > > 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))) {
> > >
> > > Hmm. Should probablt move this stuff later so that we can use check
> > > crtc->config->output_types instead. But I guess that's more
> > > appropriately left to any MST patch.
> >
> > The same reason as intel_has_dp_encoder(). And if it is wrong for DP
> > MST, let's have a separate patch when we support DP MST.
> > At that time, we can do a full test on DP MST.
>
> Yeah this part we can leave for the MST patch as it'll involve a bit more than
> just adding another check to the if statement.
>
> >
> > Regards,
> > Libin
> >
> > >
> > > > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > > err = -ENODEV;
> > > > goto unlock;
> > > > @@ -681,7 +773,7 @@ static int
> > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > goto unlock;
> > > > }
> > > >
> > > > - n = audio_config_get_n(mode, rate);
> > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > if (n == 0) {
> > > > DRM_DEBUG_KMS("Using automatic mode for N value on
> > > port %c\n",
> > > > port_name(port));
> > > > @@ -693,8 +785,17 @@ static int
> > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > >
> > > > /* 3. set the N/CTS/M */
> > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > + /* setup m value for DP */
> > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > >
> > > intel_has_dp_encoder()
> > >
> > > > + m = audio_config_get_m(crtc, rate);
> > > > + if (m == 0)
> > > > + goto unlock;
> > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > + }
> > > >
> > > > unlock:
> > > > mutex_unlock(&dev_priv->av_mutex);
> > > > --
> > > > 1.9.1
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 6:16 ` Yang, Libin
@ 2016-08-05 6:36 ` Ville Syrjälä
2016-08-05 6:40 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-05 6:36 UTC (permalink / raw)
To: Yang, Libin; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote:
> Hi Ville,
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, August 5, 2016 1:57 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> > tiwai@suse.de
> > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> >
> > On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > > Hi Ville,
> > >
> > > Sorry for delay reply. Please see my comments below.
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Thursday, August 4, 2016 4:53 PM
> > > > To: libin.yang@linux.intel.com
> > > > Cc: intel-gfx@lists.freedesktop.org; jani.nikula@linux.intel.com;
> > > > Vetter, Daniel <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > > <libin.yang@intel.com>
> > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > >
> > > > On Thu, Aug 04, 2016 at 03:58:02PM +0800, libin.yang@linux.intel.com
> > wrote:
> > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > >
> > > > > 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.
> > > >
> > > > Pleas include a small changelog here so that I know what has changed
> > > > with each revision of the patch. Eg:
> > > >
> > > > v2: blah
> > > > v3: whatever
> > > >
> > > > >
> > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > > ++++++++++++++++++++++++++++++++-----
> > > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -7351,6 +7351,12 @@ enum {
> > > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > > >
> > > > Please try to keep the register defines in order by the register offset.
> > > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > > >
> > > > > +
> > > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > index 6700a7b..04e7358 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > @@ -98,6 +98,35 @@ static const struct {
> > > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > > >
> > > > > +#define LC_540M 540000
> > > > > +#define LC_270M 270000
> > > > > +#define LC_162M 162000
> > > > > +static const struct {
> > > > > + int sample_rate;
> > > > > + int clock;
> > > > > + u16 n;
> > > > > + u16 m;
> > > > > +} aud_nm[] = {
> > > > > + {192000, LC_540M, 5625, 1024},
> > > > ^ ^
> > > >
> > > > Style nit: please add spaces here
> > > >
> > > > > + {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},
> > > > > + {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},
> > > > > + {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},
> > > >
> > > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > > >
> > > > > +};
> > > > > +
> > > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static
> > > > > u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > *adjusted
> > > > > return hdmi_audio_clock[i].config; }
> > > > >
> > > > > -static int audio_config_get_n(const struct drm_display_mode
> > > > > *mode, int rate)
> > > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > > + const struct drm_display_mode *adjusted_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;
> > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > > + (adjusted_mode->clock == aud_ncts[i].clock)) {
> > > >
> > > > Indent fail, and a bit too many parens for my taste. Should look like:
> > > >
> > > > if (rate == aud_ncts[i].sample_rate &&
> > > > adjusted_mode->clock == aud_ncts[i].clock) {
> > > >
> > > >
> > > > > + return aud_ncts[i].n;
> > > > > + }
> > > > > }
> > > > > }
> > > > > +
> > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > >
> > > > intel_has_dp_encoder() will make this work for MST as well.
> > > >
> > > > "In MST mode, the Sink device must ignore the Maud and Mvid values
> > > > sent by an MST Source device unless the MST Sink device is directly
> > > > connected to the MST Source device via a single link as the Source
> > > > device generates those values based on the LS_Clk of the link it is
> > > > driving. An MST Sink device must be able to regenerate a stream
> > > > clock without depending on Mvid and Mvid time stamp values because
> > > > the MST Sink device might be plugged into an MST Branch device, not an
> > MST Source device."
> > > >
> > > > So if I'm reading that right, an MST sink can still use the M/N
> > > > values if it's direclty hooked up to a MST source. Hence we probably
> > > > want to fill out the M/N even for MST.
> > >
> > > My understanding is this is the requirement for sink device.
> > > Non-mst and MST, source will send the N/M value, and if it is
> > > Non-mst/single link, sink device should use these data to generate the
> > > clock. If it is in MST, sink device should ignore the data and
> > > regenerate the clock without depending on N/M value.
> > >
> > > So this code should OK be for both non-mst and MST. If not, I think we
> > > can make a separate patch for DP MST.
> >
> > It's only OK for SST. We could of course delay dealing with MST, but that seems
> > like unnecessary churn. You're touching all these places in this patch, so might
> > as well use intel_dp_encoder() from the start. It will make the MST patch
> > smaller too, which will make it easier to review.
>
> OK, I will use intel_crtc_has_dp_encoder(). Hope it still works for DP MST :)
>
> >
> > >
> > > >
> > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > + (crtc->config->port_clock == aud_nm[i].clock))
> > > > {
> > > >
> > > > indent + parens
> > > >
> > > > > + return aud_nm[i].n;
> > > > > + }
> > > > > + }
> > > > > + }
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > > + int i;
> > > > > +
> > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > >
> > > > intel_has_dp_encoder()
> > > >
> > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > + (crtc->config->port_clock == aud_nm[i].clock))
> > > > {
> > > >
> > > > indent + parens
> > > >
> > > > > + return aud_nm[i].m;
> > > > > + }
> > > > > + }
> > > > > + }
> > > > > +
> > > > > return 0;
> > > > > }
> > > > >
> > > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > > + int n, uint32_t val)
> > > > > {
> > > > > int n_low, n_up;
> > > > > uint32_t tmp = val;
> > > > > @@ -145,17 +204,39 @@ static uint32_t audio_config_setup_n_reg(int
> > > > > n,
> > > > uint32_t val)
> > > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > >
> > > > intel_has_dp_encoder()
> > > >
> > > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > > + return tmp;
> > > > > +}
> > > > > +
> > > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > > + int m, uint32_t val)
> > > > > +{
> > > > > + uint32_t tmp = val;
> > > > > +
> > > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > + return 0;
> > > >
> > > > The caller already checked that.
> > > >
> > > > > +
> > > > > + tmp |= m;
> > > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > > +
> > > > > 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)
> > > > > + const struct drm_display_mode
> > > > *adjusted_mode)
> > > > > {
> > > > > - if (((mode->clock == TMDS_297M) ||
> > > > > - (mode->clock == TMDS_296M)) &&
> > > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > > intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI))
> > > > > return true;
> > > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > > + (crtc->config->port_clock == LC_270M) ||
> > > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > >
> > > > intel_has_dp_encoder()
> > > >
> > > > > + return true;
> > > > > else
> > > > > return false;
> > > > > }
> > > > > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> > > > drm_connector *connector,
> > > > > struct intel_digital_port *intel_dig_port =
> > > > > enc_to_dig_port(&encoder->base);
> > > > > enum port port = intel_dig_port->port;
> > > > > - uint32_t tmp;
> > > > > + uint32_t tmp, m;
> > > > > int len, i;
> > > > > int n, rate;
> > > > >
> > > > > @@ -343,15 +424,25 @@ static void hsw_audio_codec_enable(struct
> > > > drm_connector *connector,
> > > > > DRM_ERROR("invalid port: %d\n", port);
> > > > > rate = 0;
> > > > > }
> > > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > > + n = audio_config_get_n(intel_crtc, adjusted_mode, rate);
> > > > > if (n != 0)
> > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > + tmp = audio_config_setup_n_reg(intel_crtc, n, tmp);
> > > > > else
> > > > > DRM_DEBUG_KMS("no suitable N value is found\n");
> > > > > }
> > > > >
> > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > >
> > > > > + /* setup m value for DP */
> > > > > + if (intel_crtc_has_type(intel_crtc->config, INTEL_OUTPUT_DP)) {
> > > >
> > > > intel_has_dp_encoder()
> > > >
> > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > + if (m != 0) {
> > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > + tmp = audio_config_setup_m_reg(intel_crtc, m, tmp);
> > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > >
> > > > We should program this register even for HDMI, so that we don't leak
> > > > invalid register values eg. when changing from DP->HDMI.
> > >
> > > HDMI doesn't need set these values based on our test. It seems silicon
> > > can handle smoothly for HDMI.
> >
> > Yes, but we nee to make sure we clear whatever we programmed in for DP
> > previously.
>
> The silicon seems to be able to handle the situation of DP -> HDMI and HDMI ->
> DP.
Did you make sure eg. that the power well didn't get toggled in between?
That would reset the register anyway.
> What's more it can handle sample rate/tmds changes.
>
> Regards,
> Libin
>
> >
> > >
> > > >
> > > > > + }
> > > > > + }
> > > > > +
> > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > }
> > > > >
> > > > > @@ -637,7 +728,7 @@ static int
> > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > struct drm_display_mode *mode;
> > > > > struct i915_audio_component *acomp = dev_priv->audio_component;
> > > > > enum pipe pipe = INVALID_PIPE;
> > > > > - u32 tmp;
> > > > > + u32 tmp, m;
> > > > > int n;
> > > > > int err = 0;
> > > > >
> > > > > @@ -653,7 +744,8 @@ static int
> > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > /* intel_encoder might be NULL for DP MST */
> > > > > 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))) {
> > > >
> > > > Hmm. Should probablt move this stuff later so that we can use check
> > > > crtc->config->output_types instead. But I guess that's more
> > > > appropriately left to any MST patch.
> > >
> > > The same reason as intel_has_dp_encoder(). And if it is wrong for DP
> > > MST, let's have a separate patch when we support DP MST.
> > > At that time, we can do a full test on DP MST.
> >
> > Yeah this part we can leave for the MST patch as it'll involve a bit more than
> > just adding another check to the if statement.
> >
> > >
> > > Regards,
> > > Libin
> > >
> > > >
> > > > > DRM_DEBUG_KMS("no valid port %c\n", port_name(port));
> > > > > err = -ENODEV;
> > > > > goto unlock;
> > > > > @@ -681,7 +773,7 @@ static int
> > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > goto unlock;
> > > > > }
> > > > >
> > > > > - n = audio_config_get_n(mode, rate);
> > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > if (n == 0) {
> > > > > DRM_DEBUG_KMS("Using automatic mode for N value on
> > > > port %c\n",
> > > > > port_name(port));
> > > > > @@ -693,8 +785,17 @@ static int
> > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > >
> > > > > /* 3. set the N/CTS/M */
> > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > + /* setup m value for DP */
> > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > >
> > > > intel_has_dp_encoder()
> > > >
> > > > > + m = audio_config_get_m(crtc, rate);
> > > > > + if (m == 0)
> > > > > + goto unlock;
> > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > > + }
> > > > >
> > > > > unlock:
> > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > --
> > > > > 1.9.1
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 6:36 ` Ville Syrjälä
@ 2016-08-05 6:40 ` Yang, Libin
2016-08-05 7:16 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 6:40 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, August 5, 2016 2:36 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> tiwai@suse.de
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, August 5, 2016 1:57 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > >
> > > On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > > > Hi Ville,
> > > >
> > > > Sorry for delay reply. Please see my comments below.
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > Sent: Thursday, August 4, 2016 4:53 PM
> > > > > To: libin.yang@linux.intel.com
> > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > > > <libin.yang@intel.com>
> > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > > >
> > > > > On Thu, Aug 04, 2016 at 03:58:02PM +0800,
> > > > > libin.yang@linux.intel.com
> > > wrote:
> > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > >
> > > > > > 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.
> > > > >
> > > > > Pleas include a small changelog here so that I know what has
> > > > > changed with each revision of the patch. Eg:
> > > > >
> > > > > v2: blah
> > > > > v3: whatever
> > > > >
> > > > > >
> > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > > > ++++++++++++++++++++++++++++++++-----
> > > > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e
> > > > > > 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -7351,6 +7351,12 @@ enum {
> > > > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > > > >
> > > > > Please try to keep the register defines in order by the register offset.
> > > > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > > > >
> > > > > > +
> > > > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > > > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > index 6700a7b..04e7358 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > @@ -98,6 +98,35 @@ static const struct {
> > > > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > > > >
> > > > > > +#define LC_540M 540000
> > > > > > +#define LC_270M 270000
> > > > > > +#define LC_162M 162000
> > > > > > +static const struct {
> > > > > > + int sample_rate;
> > > > > > + int clock;
> > > > > > + u16 n;
> > > > > > + u16 m;
> > > > > > +} aud_nm[] = {
> > > > > > + {192000, LC_540M, 5625, 1024},
> > > > > ^ ^
> > > > >
> > > > > Style nit: please add spaces here
> > > > >
> > > > > > + {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},
> > > > > > + {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},
> > > > > > + {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},
> > > > >
> > > > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > > > >
> > > > > > +};
> > > > > > +
> > > > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > > > > static
> > > > > > u32 audio_config_hdmi_pixel_clock(const struct
> > > > > > drm_display_mode
> > > > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > *adjusted
> > > > > > return hdmi_audio_clock[i].config; }
> > > > > >
> > > > > > -static int audio_config_get_n(const struct drm_display_mode
> > > > > > *mode, int rate)
> > > > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > > > + const struct drm_display_mode
> *adjusted_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;
> > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > > > + (adjusted_mode->clock ==
> aud_ncts[i].clock)) {
> > > > >
> > > > > Indent fail, and a bit too many parens for my taste. Should look like:
> > > > >
> > > > > if (rate == aud_ncts[i].sample_rate &&
> > > > > adjusted_mode->clock == aud_ncts[i].clock) {
> > > > >
> > > > >
> > > > > > + return aud_ncts[i].n;
> > > > > > + }
> > > > > > }
> > > > > > }
> > > > > > +
> > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > >
> > > > > intel_has_dp_encoder() will make this work for MST as well.
> > > > >
> > > > > "In MST mode, the Sink device must ignore the Maud and Mvid
> > > > > values sent by an MST Source device unless the MST Sink device
> > > > > is directly connected to the MST Source device via a single link
> > > > > as the Source device generates those values based on the LS_Clk
> > > > > of the link it is driving. An MST Sink device must be able to
> > > > > regenerate a stream clock without depending on Mvid and Mvid
> > > > > time stamp values because the MST Sink device might be plugged
> > > > > into an MST Branch device, not an
> > > MST Source device."
> > > > >
> > > > > So if I'm reading that right, an MST sink can still use the M/N
> > > > > values if it's direclty hooked up to a MST source. Hence we
> > > > > probably want to fill out the M/N even for MST.
> > > >
> > > > My understanding is this is the requirement for sink device.
> > > > Non-mst and MST, source will send the N/M value, and if it is
> > > > Non-mst/single link, sink device should use these data to generate
> > > > the clock. If it is in MST, sink device should ignore the data and
> > > > regenerate the clock without depending on N/M value.
> > > >
> > > > So this code should OK be for both non-mst and MST. If not, I
> > > > think we can make a separate patch for DP MST.
> > >
> > > It's only OK for SST. We could of course delay dealing with MST, but
> > > that seems like unnecessary churn. You're touching all these places
> > > in this patch, so might as well use intel_dp_encoder() from the
> > > start. It will make the MST patch smaller too, which will make it easier to
> review.
> >
> > OK, I will use intel_crtc_has_dp_encoder(). Hope it still works for DP
> > MST :)
> >
> > >
> > > >
> > > > >
> > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > + (crtc->config->port_clock ==
> aud_nm[i].clock))
> > > > > {
> > > > >
> > > > > indent + parens
> > > > >
> > > > > > + return aud_nm[i].n;
> > > > > > + }
> > > > > > + }
> > > > > > + }
> > > > > > + return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > > > + int i;
> > > > > > +
> > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > >
> > > > > intel_has_dp_encoder()
> > > > >
> > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > + (crtc->config->port_clock ==
> aud_nm[i].clock))
> > > > > {
> > > > >
> > > > > indent + parens
> > > > >
> > > > > > + return aud_nm[i].m;
> > > > > > + }
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > > > + int n, uint32_t val)
> > > > > > {
> > > > > > int n_low, n_up;
> > > > > > uint32_t tmp = val;
> > > > > > @@ -145,17 +204,39 @@ static uint32_t
> > > > > > audio_config_setup_n_reg(int n,
> > > > > uint32_t val)
> > > > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > >
> > > > > intel_has_dp_encoder()
> > > > >
> > > > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > > > + return tmp;
> > > > > > +}
> > > > > > +
> > > > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > > > + int m, uint32_t val)
> > > > > > +{
> > > > > > + uint32_t tmp = val;
> > > > > > +
> > > > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > + return 0;
> > > > >
> > > > > The caller already checked that.
> > > > >
> > > > > > +
> > > > > > + tmp |= m;
> > > > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > > > +
> > > > > > 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)
> > > > > > + const struct drm_display_mode
> > > > > *adjusted_mode)
> > > > > > {
> > > > > > - if (((mode->clock == TMDS_297M) ||
> > > > > > - (mode->clock == TMDS_296M)) &&
> > > > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > intel_crtc_has_type(crtc->config,
> INTEL_OUTPUT_HDMI))
> > > > > > return true;
> > > > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > + (crtc->config->port_clock == LC_270M) ||
> > > > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > >
> > > > > intel_has_dp_encoder()
> > > > >
> > > > > > + return true;
> > > > > > else
> > > > > > return false;
> > > > > > }
> > > > > > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> > > > > drm_connector *connector,
> > > > > > struct intel_digital_port *intel_dig_port =
> > > > > > enc_to_dig_port(&encoder->base);
> > > > > > enum port port = intel_dig_port->port;
> > > > > > - uint32_t tmp;
> > > > > > + uint32_t tmp, m;
> > > > > > int len, i;
> > > > > > int n, rate;
> > > > > >
> > > > > > @@ -343,15 +424,25 @@ static void
> > > > > > hsw_audio_codec_enable(struct
> > > > > drm_connector *connector,
> > > > > > DRM_ERROR("invalid port: %d\n", port);
> > > > > > rate = 0;
> > > > > > }
> > > > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > > > + n = audio_config_get_n(intel_crtc, adjusted_mode,
> rate);
> > > > > > if (n != 0)
> > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > + tmp = audio_config_setup_n_reg(intel_crtc, n,
> tmp);
> > > > > > else
> > > > > > DRM_DEBUG_KMS("no suitable N value is
> found\n");
> > > > > > }
> > > > > >
> > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > >
> > > > > > + /* setup m value for DP */
> > > > > > + if (intel_crtc_has_type(intel_crtc->config,
> > > > > > +INTEL_OUTPUT_DP)) {
> > > > >
> > > > > intel_has_dp_encoder()
> > > > >
> > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > + if (m != 0) {
> > > > > > + tmp =
> I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > + tmp = audio_config_setup_m_reg(intel_crtc,
> m, tmp);
> > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> tmp);
> > > > >
> > > > > We should program this register even for HDMI, so that we don't
> > > > > leak invalid register values eg. when changing from DP->HDMI.
> > > >
> > > > HDMI doesn't need set these values based on our test. It seems
> > > > silicon can handle smoothly for HDMI.
> > >
> > > Yes, but we nee to make sure we clear whatever we programmed in for
> > > DP previously.
> >
> > The silicon seems to be able to handle the situation of DP -> HDMI and
> > HDMI -> DP.
>
> Did you make sure eg. that the power well didn't get toggled in between?
> That would reset the register anyway.
We have done the test for this case. So far it is OK.
Regards,
Libin
>
> > What's more it can handle sample rate/tmds changes.
> >
> > Regards,
> > Libin
> >
> > >
> > > >
> > > > >
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > }
> > > > > >
> > > > > > @@ -637,7 +728,7 @@ static int
> > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > struct drm_display_mode *mode;
> > > > > > struct i915_audio_component *acomp = dev_priv-
> >audio_component;
> > > > > > enum pipe pipe = INVALID_PIPE;
> > > > > > - u32 tmp;
> > > > > > + u32 tmp, m;
> > > > > > int n;
> > > > > > int err = 0;
> > > > > >
> > > > > > @@ -653,7 +744,8 @@ static int
> > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > > /* intel_encoder might be NULL for DP MST */
> > > > > > 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))) {
> > > > >
> > > > > Hmm. Should probablt move this stuff later so that we can use
> > > > > check
> > > > > crtc->config->output_types instead. But I guess that's more
> > > > > appropriately left to any MST patch.
> > > >
> > > > The same reason as intel_has_dp_encoder(). And if it is wrong for
> > > > DP MST, let's have a separate patch when we support DP MST.
> > > > At that time, we can do a full test on DP MST.
> > >
> > > Yeah this part we can leave for the MST patch as it'll involve a bit
> > > more than just adding another check to the if statement.
> > >
> > > >
> > > > Regards,
> > > > Libin
> > > >
> > > > >
> > > > > > DRM_DEBUG_KMS("no valid port %c\n",
> port_name(port));
> > > > > > err = -ENODEV;
> > > > > > goto unlock;
> > > > > > @@ -681,7 +773,7 @@ static int
> > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > goto unlock;
> > > > > > }
> > > > > >
> > > > > > - n = audio_config_get_n(mode, rate);
> > > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > > if (n == 0) {
> > > > > > DRM_DEBUG_KMS("Using automatic mode for N
> value on
> > > > > port %c\n",
> > > > > > port_name(port));
> > > > > > @@ -693,8 +785,17 @@ static int
> > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > >
> > > > > > /* 3. set the N/CTS/M */
> > > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > + /* setup m value for DP */
> > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > >
> > > > > intel_has_dp_encoder()
> > > > >
> > > > > > + m = audio_config_get_m(crtc, rate);
> > > > > > + if (m == 0)
> > > > > > + goto unlock;
> > > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > > > + }
> > > > > >
> > > > > > unlock:
> > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > --
> > > > > > 1.9.1
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 6:40 ` Yang, Libin
@ 2016-08-05 7:16 ` Ville Syrjälä
2016-08-05 7:24 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-05 7:16 UTC (permalink / raw)
To: Yang, Libin; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
On Fri, Aug 05, 2016 at 06:40:45AM +0000, Yang, Libin wrote:
>
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, August 5, 2016 2:36 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> > tiwai@suse.de
> > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> >
> > On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote:
> > > Hi Ville,
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Friday, August 5, 2016 1:57 PM
> > > > To: Yang, Libin <libin.yang@intel.com>
> > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > >
> > > > On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > > > > Hi Ville,
> > > > >
> > > > > Sorry for delay reply. Please see my comments below.
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > > Sent: Thursday, August 4, 2016 4:53 PM
> > > > > > To: libin.yang@linux.intel.com
> > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > > <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > > > > <libin.yang@intel.com>
> > > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > > > >
> > > > > > On Thu, Aug 04, 2016 at 03:58:02PM +0800,
> > > > > > libin.yang@linux.intel.com
> > > > wrote:
> > > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Pleas include a small changelog here so that I know what has
> > > > > > changed with each revision of the patch. Eg:
> > > > > >
> > > > > > v2: blah
> > > > > > v3: whatever
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > > > > ++++++++++++++++++++++++++++++++-----
> > > > > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e
> > > > > > > 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > @@ -7351,6 +7351,12 @@ enum {
> > > > > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > > > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > > > > >
> > > > > > Please try to keep the register defines in order by the register offset.
> > > > > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > > > > >
> > > > > > > +
> > > > > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > > > > #define HSW_AUD_MISC_CTRL(pipe) _MMIO_PIPE(pipe,
> > > > > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > index 6700a7b..04e7358 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > @@ -98,6 +98,35 @@ static const struct {
> > > > > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > > > > >
> > > > > > > +#define LC_540M 540000
> > > > > > > +#define LC_270M 270000
> > > > > > > +#define LC_162M 162000
> > > > > > > +static const struct {
> > > > > > > + int sample_rate;
> > > > > > > + int clock;
> > > > > > > + u16 n;
> > > > > > > + u16 m;
> > > > > > > +} aud_nm[] = {
> > > > > > > + {192000, LC_540M, 5625, 1024},
> > > > > > ^ ^
> > > > > >
> > > > > > Style nit: please add spaces here
> > > > > >
> > > > > > > + {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},
> > > > > > > + {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},
> > > > > > > + {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},
> > > > > >
> > > > > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > > > > >
> > > > > > > +};
> > > > > > > +
> > > > > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > > > > > static
> > > > > > > u32 audio_config_hdmi_pixel_clock(const struct
> > > > > > > drm_display_mode
> > > > > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > > > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > > *adjusted
> > > > > > > return hdmi_audio_clock[i].config; }
> > > > > > >
> > > > > > > -static int audio_config_get_n(const struct drm_display_mode
> > > > > > > *mode, int rate)
> > > > > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > > > > + const struct drm_display_mode
> > *adjusted_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;
> > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > > > > + (adjusted_mode->clock ==
> > aud_ncts[i].clock)) {
> > > > > >
> > > > > > Indent fail, and a bit too many parens for my taste. Should look like:
> > > > > >
> > > > > > if (rate == aud_ncts[i].sample_rate &&
> > > > > > adjusted_mode->clock == aud_ncts[i].clock) {
> > > > > >
> > > > > >
> > > > > > > + return aud_ncts[i].n;
> > > > > > > + }
> > > > > > > }
> > > > > > > }
> > > > > > > +
> > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > > >
> > > > > > intel_has_dp_encoder() will make this work for MST as well.
> > > > > >
> > > > > > "In MST mode, the Sink device must ignore the Maud and Mvid
> > > > > > values sent by an MST Source device unless the MST Sink device
> > > > > > is directly connected to the MST Source device via a single link
> > > > > > as the Source device generates those values based on the LS_Clk
> > > > > > of the link it is driving. An MST Sink device must be able to
> > > > > > regenerate a stream clock without depending on Mvid and Mvid
> > > > > > time stamp values because the MST Sink device might be plugged
> > > > > > into an MST Branch device, not an
> > > > MST Source device."
> > > > > >
> > > > > > So if I'm reading that right, an MST sink can still use the M/N
> > > > > > values if it's direclty hooked up to a MST source. Hence we
> > > > > > probably want to fill out the M/N even for MST.
> > > > >
> > > > > My understanding is this is the requirement for sink device.
> > > > > Non-mst and MST, source will send the N/M value, and if it is
> > > > > Non-mst/single link, sink device should use these data to generate
> > > > > the clock. If it is in MST, sink device should ignore the data and
> > > > > regenerate the clock without depending on N/M value.
> > > > >
> > > > > So this code should OK be for both non-mst and MST. If not, I
> > > > > think we can make a separate patch for DP MST.
> > > >
> > > > It's only OK for SST. We could of course delay dealing with MST, but
> > > > that seems like unnecessary churn. You're touching all these places
> > > > in this patch, so might as well use intel_dp_encoder() from the
> > > > start. It will make the MST patch smaller too, which will make it easier to
> > review.
> > >
> > > OK, I will use intel_crtc_has_dp_encoder(). Hope it still works for DP
> > > MST :)
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > + (crtc->config->port_clock ==
> > aud_nm[i].clock))
> > > > > > {
> > > > > >
> > > > > > indent + parens
> > > > > >
> > > > > > > + return aud_nm[i].n;
> > > > > > > + }
> > > > > > > + }
> > > > > > > + }
> > > > > > > + return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > > > > + int i;
> > > > > > > +
> > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > > >
> > > > > > intel_has_dp_encoder()
> > > > > >
> > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > + (crtc->config->port_clock ==
> > aud_nm[i].clock))
> > > > > > {
> > > > > >
> > > > > > indent + parens
> > > > > >
> > > > > > > + return aud_nm[i].m;
> > > > > > > + }
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > return 0;
> > > > > > > }
> > > > > > >
> > > > > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
> > > > > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > > > > + int n, uint32_t val)
> > > > > > > {
> > > > > > > int n_low, n_up;
> > > > > > > uint32_t tmp = val;
> > > > > > > @@ -145,17 +204,39 @@ static uint32_t
> > > > > > > audio_config_setup_n_reg(int n,
> > > > > > uint32_t val)
> > > > > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > >
> > > > > > intel_has_dp_encoder()
> > > > > >
> > > > > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > > > > + return tmp;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > > > > + int m, uint32_t val)
> > > > > > > +{
> > > > > > > + uint32_t tmp = val;
> > > > > > > +
> > > > > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > + return 0;
> > > > > >
> > > > > > The caller already checked that.
> > > > > >
> > > > > > > +
> > > > > > > + tmp |= m;
> > > > > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > > > > +
> > > > > > > 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)
> > > > > > > + const struct drm_display_mode
> > > > > > *adjusted_mode)
> > > > > > > {
> > > > > > > - if (((mode->clock == TMDS_297M) ||
> > > > > > > - (mode->clock == TMDS_296M)) &&
> > > > > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > > intel_crtc_has_type(crtc->config,
> > INTEL_OUTPUT_HDMI))
> > > > > > > return true;
> > > > > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > > + (crtc->config->port_clock == LC_270M) ||
> > > > > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > >
> > > > > > intel_has_dp_encoder()
> > > > > >
> > > > > > > + return true;
> > > > > > > else
> > > > > > > return false;
> > > > > > > }
> > > > > > > @@ -287,7 +368,7 @@ static void hsw_audio_codec_enable(struct
> > > > > > drm_connector *connector,
> > > > > > > struct intel_digital_port *intel_dig_port =
> > > > > > > enc_to_dig_port(&encoder->base);
> > > > > > > enum port port = intel_dig_port->port;
> > > > > > > - uint32_t tmp;
> > > > > > > + uint32_t tmp, m;
> > > > > > > int len, i;
> > > > > > > int n, rate;
> > > > > > >
> > > > > > > @@ -343,15 +424,25 @@ static void
> > > > > > > hsw_audio_codec_enable(struct
> > > > > > drm_connector *connector,
> > > > > > > DRM_ERROR("invalid port: %d\n", port);
> > > > > > > rate = 0;
> > > > > > > }
> > > > > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > > > > + n = audio_config_get_n(intel_crtc, adjusted_mode,
> > rate);
> > > > > > > if (n != 0)
> > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > + tmp = audio_config_setup_n_reg(intel_crtc, n,
> > tmp);
> > > > > > > else
> > > > > > > DRM_DEBUG_KMS("no suitable N value is
> > found\n");
> > > > > > > }
> > > > > > >
> > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > >
> > > > > > > + /* setup m value for DP */
> > > > > > > + if (intel_crtc_has_type(intel_crtc->config,
> > > > > > > +INTEL_OUTPUT_DP)) {
> > > > > >
> > > > > > intel_has_dp_encoder()
> > > > > >
> > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > + if (m != 0) {
> > > > > > > + tmp =
> > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > + tmp = audio_config_setup_m_reg(intel_crtc,
> > m, tmp);
> > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > tmp);
> > > > > >
> > > > > > We should program this register even for HDMI, so that we don't
> > > > > > leak invalid register values eg. when changing from DP->HDMI.
> > > > >
> > > > > HDMI doesn't need set these values based on our test. It seems
> > > > > silicon can handle smoothly for HDMI.
> > > >
> > > > Yes, but we nee to make sure we clear whatever we programmed in for
> > > > DP previously.
> > >
> > > The silicon seems to be able to handle the situation of DP -> HDMI and
> > > HDMI -> DP.
> >
> > Did you make sure eg. that the power well didn't get toggled in between?
> > That would reset the register anyway.
>
> We have done the test for this case. So far it is OK.
Test what? Checking that the register gets reset w/o any power well
toggling and the like? So what event does reset that register?
>
> Regards,
> Libin
>
> >
> > > What's more it can handle sample rate/tmds changes.
> > >
> > > Regards,
> > > Libin
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > }
> > > > > > >
> > > > > > > @@ -637,7 +728,7 @@ static int
> > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > struct drm_display_mode *mode;
> > > > > > > struct i915_audio_component *acomp = dev_priv-
> > >audio_component;
> > > > > > > enum pipe pipe = INVALID_PIPE;
> > > > > > > - u32 tmp;
> > > > > > > + u32 tmp, m;
> > > > > > > int n;
> > > > > > > int err = 0;
> > > > > > >
> > > > > > > @@ -653,7 +744,8 @@ static int
> > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > > > /* intel_encoder might be NULL for DP MST */
> > > > > > > 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))) {
> > > > > >
> > > > > > Hmm. Should probablt move this stuff later so that we can use
> > > > > > check
> > > > > > crtc->config->output_types instead. But I guess that's more
> > > > > > appropriately left to any MST patch.
> > > > >
> > > > > The same reason as intel_has_dp_encoder(). And if it is wrong for
> > > > > DP MST, let's have a separate patch when we support DP MST.
> > > > > At that time, we can do a full test on DP MST.
> > > >
> > > > Yeah this part we can leave for the MST patch as it'll involve a bit
> > > > more than just adding another check to the if statement.
> > > >
> > > > >
> > > > > Regards,
> > > > > Libin
> > > > >
> > > > > >
> > > > > > > DRM_DEBUG_KMS("no valid port %c\n",
> > port_name(port));
> > > > > > > err = -ENODEV;
> > > > > > > goto unlock;
> > > > > > > @@ -681,7 +773,7 @@ static int
> > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > goto unlock;
> > > > > > > }
> > > > > > >
> > > > > > > - n = audio_config_get_n(mode, rate);
> > > > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > > > if (n == 0) {
> > > > > > > DRM_DEBUG_KMS("Using automatic mode for N
> > value on
> > > > > > port %c\n",
> > > > > > > port_name(port));
> > > > > > > @@ -693,8 +785,17 @@ static int
> > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > >
> > > > > > > /* 3. set the N/CTS/M */
> > > > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > + /* setup m value for DP */
> > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > > >
> > > > > > intel_has_dp_encoder()
> > > > > >
> > > > > > > + m = audio_config_get_m(crtc, rate);
> > > > > > > + if (m == 0)
> > > > > > > + goto unlock;
> > > > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > > > > + }
> > > > > > >
> > > > > > > unlock:
> > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > --
> > > > > > > 1.9.1
> > > > > >
> > > > > > --
> > > > > > Ville Syrjälä
> > > > > > Intel OTC
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 7:16 ` Ville Syrjälä
@ 2016-08-05 7:24 ` Yang, Libin
2016-08-05 8:45 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 7:24 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, August 5, 2016 3:17 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> tiwai@suse.de
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Fri, Aug 05, 2016 at 06:40:45AM +0000, Yang, Libin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, August 5, 2016 2:36 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > >
> > > On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote:
> > > > Hi Ville,
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > Sent: Friday, August 5, 2016 1:57 PM
> > > > > To: Yang, Libin <libin.yang@intel.com>
> > > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > > >
> > > > > On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > > > > > Hi Ville,
> > > > > >
> > > > > > Sorry for delay reply. Please see my comments below.
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > > > Sent: Thursday, August 4, 2016 4:53 PM
> > > > > > > To: libin.yang@linux.intel.com
> > > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > > > <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > > > > > <libin.yang@intel.com>
> > > > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in
> > > > > > > modeset
> > > > > > >
> > > > > > > On Thu, Aug 04, 2016 at 03:58:02PM +0800,
> > > > > > > libin.yang@linux.intel.com
> > > > > wrote:
> > > > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > Pleas include a small changelog here so that I know what has
> > > > > > > changed with each revision of the patch. Eg:
> > > > > > >
> > > > > > > v2: blah
> > > > > > > v3: whatever
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > > > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > > > > > ++++++++++++++++++++++++++++++++-----
> > > > > > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e
> > > > > > > > 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > @@ -7351,6 +7351,12 @@ enum {
> > > > > > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > > > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > > > > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > > > > > >
> > > > > > > Please try to keep the register defines in order by the register offset.
> > > > > > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > > > > > >
> > > > > > > > +
> > > > > > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > > > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > > > > > #define HSW_AUD_MISC_CTRL(pipe)
> _MMIO_PIPE(pipe,
> > > > > > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > index 6700a7b..04e7358 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > @@ -98,6 +98,35 @@ static const struct {
> > > > > > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > > > > > >
> > > > > > > > +#define LC_540M 540000
> > > > > > > > +#define LC_270M 270000
> > > > > > > > +#define LC_162M 162000
> > > > > > > > +static const struct {
> > > > > > > > + int sample_rate;
> > > > > > > > + int clock;
> > > > > > > > + u16 n;
> > > > > > > > + u16 m;
> > > > > > > > +} aud_nm[] = {
> > > > > > > > + {192000, LC_540M, 5625, 1024},
> > > > > > > ^ ^
> > > > > > >
> > > > > > > Style nit: please add spaces here
> > > > > > >
> > > > > > > > + {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},
> > > > > > > > + {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},
> > > > > > > > + {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},
> > > > > > >
> > > > > > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > > > > > >
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > > > > > > static
> > > > > > > > u32 audio_config_hdmi_pixel_clock(const struct
> > > > > > > > drm_display_mode
> > > > > > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > > > > > audio_config_hdmi_pixel_clock(const struct
> > > > > > > > drm_display_mode
> > > > > *adjusted
> > > > > > > > return hdmi_audio_clock[i].config; }
> > > > > > > >
> > > > > > > > -static int audio_config_get_n(const struct
> > > > > > > > drm_display_mode *mode, int rate)
> > > > > > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > > > > > + const struct drm_display_mode
> > > *adjusted_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;
> > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > > > > > + (adjusted_mode->clock ==
> > > aud_ncts[i].clock)) {
> > > > > > >
> > > > > > > Indent fail, and a bit too many parens for my taste. Should look like:
> > > > > > >
> > > > > > > if (rate == aud_ncts[i].sample_rate &&
> > > > > > > adjusted_mode->clock == aud_ncts[i].clock) {
> > > > > > >
> > > > > > >
> > > > > > > > + return aud_ncts[i].n;
> > > > > > > > + }
> > > > > > > > }
> > > > > > > > }
> > > > > > > > +
> > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > +{
> > > > > > >
> > > > > > > intel_has_dp_encoder() will make this work for MST as well.
> > > > > > >
> > > > > > > "In MST mode, the Sink device must ignore the Maud and Mvid
> > > > > > > values sent by an MST Source device unless the MST Sink
> > > > > > > device is directly connected to the MST Source device via a
> > > > > > > single link as the Source device generates those values
> > > > > > > based on the LS_Clk of the link it is driving. An MST Sink
> > > > > > > device must be able to regenerate a stream clock without
> > > > > > > depending on Mvid and Mvid time stamp values because the MST
> > > > > > > Sink device might be plugged into an MST Branch device, not
> > > > > > > an
> > > > > MST Source device."
> > > > > > >
> > > > > > > So if I'm reading that right, an MST sink can still use the
> > > > > > > M/N values if it's direclty hooked up to a MST source. Hence
> > > > > > > we probably want to fill out the M/N even for MST.
> > > > > >
> > > > > > My understanding is this is the requirement for sink device.
> > > > > > Non-mst and MST, source will send the N/M value, and if it is
> > > > > > Non-mst/single link, sink device should use these data to
> > > > > > generate the clock. If it is in MST, sink device should ignore
> > > > > > the data and regenerate the clock without depending on N/M value.
> > > > > >
> > > > > > So this code should OK be for both non-mst and MST. If not, I
> > > > > > think we can make a separate patch for DP MST.
> > > > >
> > > > > It's only OK for SST. We could of course delay dealing with MST,
> > > > > but that seems like unnecessary churn. You're touching all these
> > > > > places in this patch, so might as well use intel_dp_encoder()
> > > > > from the start. It will make the MST patch smaller too, which
> > > > > will make it easier to
> > > review.
> > > >
> > > > OK, I will use intel_crtc_has_dp_encoder(). Hope it still works
> > > > for DP MST :)
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > > + (crtc->config->port_clock ==
> > > aud_nm[i].clock))
> > > > > > > {
> > > > > > >
> > > > > > > indent + parens
> > > > > > >
> > > > > > > > + return aud_nm[i].n;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > + return 0;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > > > > > + int i;
> > > > > > > > +
> > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > +{
> > > > > > >
> > > > > > > intel_has_dp_encoder()
> > > > > > >
> > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > > + (crtc->config->port_clock ==
> > > aud_nm[i].clock))
> > > > > > > {
> > > > > > >
> > > > > > > indent + parens
> > > > > > >
> > > > > > > > + return aud_nm[i].m;
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > return 0;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t
> > > > > > > > val)
> > > > > > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > > > > > + int n, uint32_t val)
> > > > > > > > {
> > > > > > > > int n_low, n_up;
> > > > > > > > uint32_t tmp = val;
> > > > > > > > @@ -145,17 +204,39 @@ static uint32_t
> > > > > > > > audio_config_setup_n_reg(int n,
> > > > > > > uint32_t val)
> > > > > > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > > > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > > > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > >
> > > > > > > intel_has_dp_encoder()
> > > > > > >
> > > > > > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > > > > > + return tmp;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > > > > > + int m, uint32_t val) {
> > > > > > > > + uint32_t tmp = val;
> > > > > > > > +
> > > > > > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > + return 0;
> > > > > > >
> > > > > > > The caller already checked that.
> > > > > > >
> > > > > > > > +
> > > > > > > > + tmp |= m;
> > > > > > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > > > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > > > > > +
> > > > > > > > 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)
> > > > > > > > + const struct drm_display_mode
> > > > > > > *adjusted_mode)
> > > > > > > > {
> > > > > > > > - if (((mode->clock == TMDS_297M) ||
> > > > > > > > - (mode->clock == TMDS_296M)) &&
> > > > > > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > > > intel_crtc_has_type(crtc->config,
> > > INTEL_OUTPUT_HDMI))
> > > > > > > > return true;
> > > > > > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > > > + (crtc->config->port_clock == LC_270M) ||
> > > > > > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > > > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > >
> > > > > > > intel_has_dp_encoder()
> > > > > > >
> > > > > > > > + return true;
> > > > > > > > else
> > > > > > > > return false;
> > > > > > > > }
> > > > > > > > @@ -287,7 +368,7 @@ static void
> > > > > > > > hsw_audio_codec_enable(struct
> > > > > > > drm_connector *connector,
> > > > > > > > struct intel_digital_port *intel_dig_port =
> > > > > > > > enc_to_dig_port(&encoder->base);
> > > > > > > > enum port port = intel_dig_port->port;
> > > > > > > > - uint32_t tmp;
> > > > > > > > + uint32_t tmp, m;
> > > > > > > > int len, i;
> > > > > > > > int n, rate;
> > > > > > > >
> > > > > > > > @@ -343,15 +424,25 @@ static void
> > > > > > > > hsw_audio_codec_enable(struct
> > > > > > > drm_connector *connector,
> > > > > > > > DRM_ERROR("invalid port: %d\n", port);
> > > > > > > > rate = 0;
> > > > > > > > }
> > > > > > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > > > > > + n = audio_config_get_n(intel_crtc, adjusted_mode,
> > > rate);
> > > > > > > > if (n != 0)
> > > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > > + tmp = audio_config_setup_n_reg(intel_crtc, n,
> > > tmp);
> > > > > > > > else
> > > > > > > > DRM_DEBUG_KMS("no suitable N value is
> > > found\n");
> > > > > > > > }
> > > > > > > >
> > > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > >
> > > > > > > > + /* setup m value for DP */
> > > > > > > > + if (intel_crtc_has_type(intel_crtc->config,
> > > > > > > > +INTEL_OUTPUT_DP)) {
> > > > > > >
> > > > > > > intel_has_dp_encoder()
> > > > > > >
> > > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > > + if (m != 0) {
> > > > > > > > + tmp =
> > > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > + tmp = audio_config_setup_m_reg(intel_crtc,
> > > m, tmp);
> > > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > > tmp);
> > > > > > >
> > > > > > > We should program this register even for HDMI, so that we
> > > > > > > don't leak invalid register values eg. when changing from DP->HDMI.
> > > > > >
> > > > > > HDMI doesn't need set these values based on our test. It seems
> > > > > > silicon can handle smoothly for HDMI.
> > > > >
> > > > > Yes, but we nee to make sure we clear whatever we programmed in
> > > > > for DP previously.
> > > >
> > > > The silicon seems to be able to handle the situation of DP -> HDMI
> > > > and HDMI -> DP.
> > >
> > > Did you make sure eg. that the power well didn't get toggled in between?
> > > That would reset the register anyway.
> >
> > We have done the test for this case. So far it is OK.
>
> Test what? Checking that the register gets reset w/o any power well toggling
> and the like? So what event does reset that register?
The register is used to set m/cts, which will impacts the audio clock sync. We
didn't check the register reset or not. But the HDMI audio and DP will both
works smoothly. And for your consideration, it is for HDMI. Let's make
another patch for this issue if we really met the issue that hdmi can't
sync the clock. What do you think?
>
> >
> > Regards,
> > Libin
> >
> > >
> > > > What's more it can handle sample rate/tmds changes.
> > > >
> > > > Regards,
> > > > Libin
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > + }
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -637,7 +728,7 @@ static int
> > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > struct drm_display_mode *mode;
> > > > > > > > struct i915_audio_component *acomp = dev_priv-
> > > >audio_component;
> > > > > > > > enum pipe pipe = INVALID_PIPE;
> > > > > > > > - u32 tmp;
> > > > > > > > + u32 tmp, m;
> > > > > > > > int n;
> > > > > > > > int err = 0;
> > > > > > > >
> > > > > > > > @@ -653,7 +744,8 @@ static int
> > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > > > > /* intel_encoder might be NULL for DP MST */
> > > > > > > > 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))) {
> > > > > > >
> > > > > > > Hmm. Should probablt move this stuff later so that we can
> > > > > > > use check
> > > > > > > crtc->config->output_types instead. But I guess that's more
> > > > > > > appropriately left to any MST patch.
> > > > > >
> > > > > > The same reason as intel_has_dp_encoder(). And if it is wrong
> > > > > > for DP MST, let's have a separate patch when we support DP MST.
> > > > > > At that time, we can do a full test on DP MST.
> > > > >
> > > > > Yeah this part we can leave for the MST patch as it'll involve a
> > > > > bit more than just adding another check to the if statement.
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Libin
> > > > > >
> > > > > > >
> > > > > > > > DRM_DEBUG_KMS("no valid port %c\n",
> > > port_name(port));
> > > > > > > > err = -ENODEV;
> > > > > > > > goto unlock;
> > > > > > > > @@ -681,7 +773,7 @@ static int
> > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > goto unlock;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - n = audio_config_get_n(mode, rate);
> > > > > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > > > > if (n == 0) {
> > > > > > > > DRM_DEBUG_KMS("Using automatic mode for N
> > > value on
> > > > > > > port %c\n",
> > > > > > > > port_name(port));
> > > > > > > > @@ -693,8 +785,17 @@ static int
> > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > >
> > > > > > > > /* 3. set the N/CTS/M */
> > > > > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > > + /* setup m value for DP */
> > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > +{
> > > > > > >
> > > > > > > intel_has_dp_encoder()
> > > > > > >
> > > > > > > > + m = audio_config_get_m(crtc, rate);
> > > > > > > > + if (m == 0)
> > > > > > > > + goto unlock;
> > > > > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > > > > > + }
> > > > > > > >
> > > > > > > > unlock:
> > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > --
> > > > > > > > 1.9.1
> > > > > > >
> > > > > > > --
> > > > > > > Ville Syrjälä
> > > > > > > Intel OTC
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 7:24 ` Yang, Libin
@ 2016-08-05 8:45 ` Ville Syrjälä
2016-08-05 8:55 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-05 8:45 UTC (permalink / raw)
To: Yang, Libin; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
On Fri, Aug 05, 2016 at 07:24:14AM +0000, Yang, Libin wrote:
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, August 5, 2016 3:17 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> > tiwai@suse.de
> > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> >
> > On Fri, Aug 05, 2016 at 06:40:45AM +0000, Yang, Libin wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Friday, August 5, 2016 2:36 PM
> > > > To: Yang, Libin <libin.yang@intel.com>
> > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > >
> > > > On Fri, Aug 05, 2016 at 06:16:59AM +0000, Yang, Libin wrote:
> > > > > Hi Ville,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > > Sent: Friday, August 5, 2016 1:57 PM
> > > > > > To: Yang, Libin <libin.yang@intel.com>
> > > > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > > > >
> > > > > > On Fri, Aug 05, 2016 at 05:39:19AM +0000, Yang, Libin wrote:
> > > > > > > Hi Ville,
> > > > > > >
> > > > > > > Sorry for delay reply. Please see my comments below.
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > > > > Sent: Thursday, August 4, 2016 4:53 PM
> > > > > > > > To: libin.yang@linux.intel.com
> > > > > > > > Cc: intel-gfx@lists.freedesktop.org;
> > > > > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > > > > <daniel.vetter@intel.com>; tiwai@suse.de; Yang, Libin
> > > > > > > > <libin.yang@intel.com>
> > > > > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in
> > > > > > > > modeset
> > > > > > > >
> > > > > > > > On Thu, Aug 04, 2016 at 03:58:02PM +0800,
> > > > > > > > libin.yang@linux.intel.com
> > > > > > wrote:
> > > > > > > > > From: Libin Yang <libin.yang@linux.intel.com>
> > > > > > > > >
> > > > > > > > > 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.
> > > > > > > >
> > > > > > > > Pleas include a small changelog here so that I know what has
> > > > > > > > changed with each revision of the patch. Eg:
> > > > > > > >
> > > > > > > > v2: blah
> > > > > > > > v3: whatever
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Libin Yang <libin.yang@linux.intel.com>
> > > > > > > > > ---
> > > > > > > > > drivers/gpu/drm/i915/i915_reg.h | 6 ++
> > > > > > > > > drivers/gpu/drm/i915/intel_audio.c | 133
> > > > > > > > > ++++++++++++++++++++++++++++++++-----
> > > > > > > > > 2 files changed, 123 insertions(+), 16 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e
> > > > > > > > > 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > > > > @@ -7351,6 +7351,12 @@ enum {
> > > > > > > > > #define _HSW_AUD_CONFIG_B 0x65100
> > > > > > > > > #define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe,
> > > > > > > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_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)
> > > > > > > >
> > > > > > > > Please try to keep the register defines in order by the register offset.
> > > > > > > > Ie. these registers should be placed just after HSW_AUD_MISC_CTRL.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > #define _HSW_AUD_MISC_CTRL_A 0x65010
> > > > > > > > > #define _HSW_AUD_MISC_CTRL_B 0x65110
> > > > > > > > > #define HSW_AUD_MISC_CTRL(pipe)
> > _MMIO_PIPE(pipe,
> > > > > > > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > > index 6700a7b..04e7358 100644
> > > > > > > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > > > > > > @@ -98,6 +98,35 @@ static const struct {
> > > > > > > > > { 192000, TMDS_297M, 20480, 247500 }, };
> > > > > > > > >
> > > > > > > > > +#define LC_540M 540000
> > > > > > > > > +#define LC_270M 270000
> > > > > > > > > +#define LC_162M 162000
> > > > > > > > > +static const struct {
> > > > > > > > > + int sample_rate;
> > > > > > > > > + int clock;
> > > > > > > > > + u16 n;
> > > > > > > > > + u16 m;
> > > > > > > > > +} aud_nm[] = {
> > > > > > > > > + {192000, LC_540M, 5625, 1024},
> > > > > > > > ^ ^
> > > > > > > >
> > > > > > > > Style nit: please add spaces here
> > > > > > > >
> > > > > > > > > + {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},
> > > > > > > > > + {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},
> > > > > > > > > + {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},
> > > > > > > >
> > > > > > > > The numbers looks good. But looks like you forgot about 176.4 kHz?
> > > > > > > >
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > > > > > > > > static
> > > > > > > > > u32 audio_config_hdmi_pixel_clock(const struct
> > > > > > > > > drm_display_mode
> > > > > > > > > *adjusted_mode) { @@ -121,20 +150,50 @@ static u32
> > > > > > > > > audio_config_hdmi_pixel_clock(const struct
> > > > > > > > > drm_display_mode
> > > > > > *adjusted
> > > > > > > > > return hdmi_audio_clock[i].config; }
> > > > > > > > >
> > > > > > > > > -static int audio_config_get_n(const struct
> > > > > > > > > drm_display_mode *mode, int rate)
> > > > > > > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > > > > > > + const struct drm_display_mode
> > > > *adjusted_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;
> > > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > > > > > > + if ((rate == aud_ncts[i].sample_rate) &&
> > > > > > > > > + (adjusted_mode->clock ==
> > > > aud_ncts[i].clock)) {
> > > > > > > >
> > > > > > > > Indent fail, and a bit too many parens for my taste. Should look like:
> > > > > > > >
> > > > > > > > if (rate == aud_ncts[i].sample_rate &&
> > > > > > > > adjusted_mode->clock == aud_ncts[i].clock) {
> > > > > > > >
> > > > > > > >
> > > > > > > > > + return aud_ncts[i].n;
> > > > > > > > > + }
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > +
> > > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > > +{
> > > > > > > >
> > > > > > > > intel_has_dp_encoder() will make this work for MST as well.
> > > > > > > >
> > > > > > > > "In MST mode, the Sink device must ignore the Maud and Mvid
> > > > > > > > values sent by an MST Source device unless the MST Sink
> > > > > > > > device is directly connected to the MST Source device via a
> > > > > > > > single link as the Source device generates those values
> > > > > > > > based on the LS_Clk of the link it is driving. An MST Sink
> > > > > > > > device must be able to regenerate a stream clock without
> > > > > > > > depending on Mvid and Mvid time stamp values because the MST
> > > > > > > > Sink device might be plugged into an MST Branch device, not
> > > > > > > > an
> > > > > > MST Source device."
> > > > > > > >
> > > > > > > > So if I'm reading that right, an MST sink can still use the
> > > > > > > > M/N values if it's direclty hooked up to a MST source. Hence
> > > > > > > > we probably want to fill out the M/N even for MST.
> > > > > > >
> > > > > > > My understanding is this is the requirement for sink device.
> > > > > > > Non-mst and MST, source will send the N/M value, and if it is
> > > > > > > Non-mst/single link, sink device should use these data to
> > > > > > > generate the clock. If it is in MST, sink device should ignore
> > > > > > > the data and regenerate the clock without depending on N/M value.
> > > > > > >
> > > > > > > So this code should OK be for both non-mst and MST. If not, I
> > > > > > > think we can make a separate patch for DP MST.
> > > > > >
> > > > > > It's only OK for SST. We could of course delay dealing with MST,
> > > > > > but that seems like unnecessary churn. You're touching all these
> > > > > > places in this patch, so might as well use intel_dp_encoder()
> > > > > > from the start. It will make the MST patch smaller too, which
> > > > > > will make it easier to
> > > > review.
> > > > >
> > > > > OK, I will use intel_crtc_has_dp_encoder(). Hope it still works
> > > > > for DP MST :)
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > > > + (crtc->config->port_clock ==
> > > > aud_nm[i].clock))
> > > > > > > > {
> > > > > > > >
> > > > > > > > indent + parens
> > > > > > > >
> > > > > > > > > + return aud_nm[i].n;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > + return 0;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int audio_config_get_m(struct intel_crtc *crtc, int rate) {
> > > > > > > > > + int i;
> > > > > > > > > +
> > > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > > +{
> > > > > > > >
> > > > > > > > intel_has_dp_encoder()
> > > > > > > >
> > > > > > > > > + for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > > > > > > + if ((rate == aud_nm[i].sample_rate) &&
> > > > > > > > > + (crtc->config->port_clock ==
> > > > aud_nm[i].clock))
> > > > > > > > {
> > > > > > > >
> > > > > > > > indent + parens
> > > > > > > >
> > > > > > > > > + return aud_nm[i].m;
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > return 0;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -static uint32_t audio_config_setup_n_reg(int n, uint32_t
> > > > > > > > > val)
> > > > > > > > > +static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
> > > > > > > > > + int n, uint32_t val)
> > > > > > > > > {
> > > > > > > > > int n_low, n_up;
> > > > > > > > > uint32_t tmp = val;
> > > > > > > > > @@ -145,17 +204,39 @@ static uint32_t
> > > > > > > > > audio_config_setup_n_reg(int n,
> > > > > > > > uint32_t val)
> > > > > > > > > tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
> > > > > > > > > (n_low << AUD_CONFIG_LOWER_N_SHIFT) |
> > > > > > > > > AUD_CONFIG_N_PROG_ENABLE);
> > > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > >
> > > > > > > > intel_has_dp_encoder()
> > > > > > > >
> > > > > > > > > + tmp |= AUD_CONFIG_N_VALUE_INDEX;
> > > > > > > > > + return tmp;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
> > > > > > > > > + int m, uint32_t val) {
> > > > > > > > > + uint32_t tmp = val;
> > > > > > > > > +
> > > > > > > > > + if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > > + return 0;
> > > > > > > >
> > > > > > > > The caller already checked that.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > + tmp |= m;
> > > > > > > > > + tmp |= AUD_M_CTS_M_VALUE_INDEX;
> > > > > > > > > + tmp |= AUD_M_CTS_M_PROG_ENABLE;
> > > > > > > > > +
> > > > > > > > > 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)
> > > > > > > > > + const struct drm_display_mode
> > > > > > > > *adjusted_mode)
> > > > > > > > > {
> > > > > > > > > - if (((mode->clock == TMDS_297M) ||
> > > > > > > > > - (mode->clock == TMDS_296M)) &&
> > > > > > > > > + if (((adjusted_mode->clock == TMDS_297M) ||
> > > > > > > > > + (adjusted_mode->clock == TMDS_296M)) &&
> > > > > > > > > intel_crtc_has_type(crtc->config,
> > > > INTEL_OUTPUT_HDMI))
> > > > > > > > > return true;
> > > > > > > > > + else if (((crtc->config->port_clock == LC_540M) ||
> > > > > > > > > + (crtc->config->port_clock == LC_270M) ||
> > > > > > > > > + (crtc->config->port_clock == LC_162M)) &&
> > > > > > > > > + intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > >
> > > > > > > > intel_has_dp_encoder()
> > > > > > > >
> > > > > > > > > + return true;
> > > > > > > > > else
> > > > > > > > > return false;
> > > > > > > > > }
> > > > > > > > > @@ -287,7 +368,7 @@ static void
> > > > > > > > > hsw_audio_codec_enable(struct
> > > > > > > > drm_connector *connector,
> > > > > > > > > struct intel_digital_port *intel_dig_port =
> > > > > > > > > enc_to_dig_port(&encoder->base);
> > > > > > > > > enum port port = intel_dig_port->port;
> > > > > > > > > - uint32_t tmp;
> > > > > > > > > + uint32_t tmp, m;
> > > > > > > > > int len, i;
> > > > > > > > > int n, rate;
> > > > > > > > >
> > > > > > > > > @@ -343,15 +424,25 @@ static void
> > > > > > > > > hsw_audio_codec_enable(struct
> > > > > > > > drm_connector *connector,
> > > > > > > > > DRM_ERROR("invalid port: %d\n", port);
> > > > > > > > > rate = 0;
> > > > > > > > > }
> > > > > > > > > - n = audio_config_get_n(adjusted_mode, rate);
> > > > > > > > > + n = audio_config_get_n(intel_crtc, adjusted_mode,
> > > > rate);
> > > > > > > > > if (n != 0)
> > > > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > > > + tmp = audio_config_setup_n_reg(intel_crtc, n,
> > > > tmp);
> > > > > > > > > else
> > > > > > > > > DRM_DEBUG_KMS("no suitable N value is
> > > > found\n");
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > > >
> > > > > > > > > + /* setup m value for DP */
> > > > > > > > > + if (intel_crtc_has_type(intel_crtc->config,
> > > > > > > > > +INTEL_OUTPUT_DP)) {
> > > > > > > >
> > > > > > > > intel_has_dp_encoder()
> > > > > > > >
> > > > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > > > + if (m != 0) {
> > > > > > > > > + tmp =
> > > > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > + tmp = audio_config_setup_m_reg(intel_crtc,
> > > > m, tmp);
> > > > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > > > tmp);
> > > > > > > >
> > > > > > > > We should program this register even for HDMI, so that we
> > > > > > > > don't leak invalid register values eg. when changing from DP->HDMI.
> > > > > > >
> > > > > > > HDMI doesn't need set these values based on our test. It seems
> > > > > > > silicon can handle smoothly for HDMI.
> > > > > >
> > > > > > Yes, but we nee to make sure we clear whatever we programmed in
> > > > > > for DP previously.
> > > > >
> > > > > The silicon seems to be able to handle the situation of DP -> HDMI > > > > > and HDMI -> DP.
> > > >
> > > > Did you make sure eg. that the power well didn't get toggled in between?
> > > > That would reset the register anyway.
> > >
> > > We have done the test for this case. So far it is OK.
> >
> > Test what? Checking that the register gets reset w/o any power well toggling
> > and the like? So what event does reset that register?
>
> The register is used to set m/cts, which will impacts the audio clock sync. We
> didn't check the register reset or not. But the HDMI audio and DP will both
> works smoothly. And for your consideration, it is for HDMI. Let's make
> another patch for this issue if we really met the issue that hdmi can't
> sync the clock. What do you think?
How about we just always write the register to make sure we won't
get any stupid bugs because of this.
>
> >
> > >
> > > Regards,
> > > Libin
> > >
> > > >
> > > > > What's more it can handle sample rate/tmds changes.
> > > > >
> > > > > Regards,
> > > > > Libin
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > + }
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > @@ -637,7 +728,7 @@ static int
> > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > struct drm_display_mode *mode;
> > > > > > > > > struct i915_audio_component *acomp = dev_priv-
> > > > >audio_component;
> > > > > > > > > enum pipe pipe = INVALID_PIPE;
> > > > > > > > > - u32 tmp;
> > > > > > > > > + u32 tmp, m;
> > > > > > > > > int n;
> > > > > > > > > int err = 0;
> > > > > > > > >
> > > > > > > > > @@ -653,7 +744,8 @@ static int
> > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > > > > > /* intel_encoder might be NULL for DP MST */
> > > > > > > > > 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))) {
> > > > > > > >
> > > > > > > > Hmm. Should probablt move this stuff later so that we can
> > > > > > > > use check
> > > > > > > > crtc->config->output_types instead. But I guess that's more
> > > > > > > > appropriately left to any MST patch.
> > > > > > >
> > > > > > > The same reason as intel_has_dp_encoder(). And if it is wrong
> > > > > > > for DP MST, let's have a separate patch when we support DP MST.
> > > > > > > At that time, we can do a full test on DP MST.
> > > > > >
> > > > > > Yeah this part we can leave for the MST patch as it'll involve a
> > > > > > bit more than just adding another check to the if statement.
> > > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > > Libin
> > > > > > >
> > > > > > > >
> > > > > > > > > DRM_DEBUG_KMS("no valid port %c\n",
> > > > port_name(port));
> > > > > > > > > err = -ENODEV;
> > > > > > > > > goto unlock;
> > > > > > > > > @@ -681,7 +773,7 @@ static int
> > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > goto unlock;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > - n = audio_config_get_n(mode, rate);
> > > > > > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > > > > > if (n == 0) {
> > > > > > > > > DRM_DEBUG_KMS("Using automatic mode for N
> > > > value on
> > > > > > > > port %c\n",
> > > > > > > > > port_name(port));
> > > > > > > > > @@ -693,8 +785,17 @@ static int
> > > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > >
> > > > > > > > > /* 3. set the N/CTS/M */
> > > > > > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > > > + /* setup m value for DP */
> > > > > > > > > + if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
> > > > > > > > > +{
> > > > > > > >
> > > > > > > > intel_has_dp_encoder()
> > > > > > > >
> > > > > > > > > + m = audio_config_get_m(crtc, rate);
> > > > > > > > > + if (m == 0)
> > > > > > > > > + goto unlock;
> > > > > > > > > + tmp = I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > + tmp = audio_config_setup_m_reg(crtc, m, tmp);
> > > > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe), tmp);
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > unlock:
> > > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > > --
> > > > > > > > > 1.9.1
> > > > > > > >
> > > > > > > > --
> > > > > > > > Ville Syrjälä
> > > > > > > > Intel OTC
> > > > > >
> > > > > > --
> > > > > > Ville Syrjälä
> > > > > > Intel OTC
> > > >
> > > > --
> > > > Ville Syrjälä
> > > > Intel OTC
> >
> > --
> > Ville Syrjälä
> > Intel OTC
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 8:45 ` Ville Syrjälä
@ 2016-08-05 8:55 ` Yang, Libin
2016-08-05 10:54 ` Ville Syrjälä
0 siblings, 1 reply; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 8:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, August 5, 2016 4:45 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> tiwai@suse.de
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Fri, Aug 05, 2016 at 07:24:14AM +0000, Yang, Libin wrote:
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, August 5, 2016 3:17 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > >
> > > > > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > > > > + if (m != 0) {
> > > > > > > > > > + tmp =
> > > > > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > > + tmp =
> audio_config_setup_m_reg(intel_crtc,
> > > > > m, tmp);
> > > > > > > > > > +
> I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > > > > tmp);
> > > > > > > > >
> > > > > > > > > We should program this register even for HDMI, so that
> > > > > > > > > we don't leak invalid register values eg. when changing from DP-
> >HDMI.
> > > > > > > >
> > > > > > > > HDMI doesn't need set these values based on our test. It
> > > > > > > > seems silicon can handle smoothly for HDMI.
> > > > > > >
> > > > > > > Yes, but we nee to make sure we clear whatever we programmed
> > > > > > > in for DP previously.
> > > > > >
> > > > > > The silicon seems to be able to handle the situation of DP ->
> HDMI > > > > > and HDMI -> DP.
> > > > >
> > > > > Did you make sure eg. that the power well didn't get toggled in
> between?
> > > > > That would reset the register anyway.
> > > >
> > > > We have done the test for this case. So far it is OK.
> > >
> > > Test what? Checking that the register gets reset w/o any power well
> > > toggling and the like? So what event does reset that register?
> >
> > The register is used to set m/cts, which will impacts the audio clock
> > sync. We didn't check the register reset or not. But the HDMI audio
> > and DP will both works smoothly. And for your consideration, it is for
> > HDMI. Let's make another patch for this issue if we really met the
> > issue that hdmi can't sync the clock. What do you think?
>
> How about we just always write the register to make sure we won't get any
> stupid bugs because of this.
OK. Do you think it is OK I will write a separate patch for it, not included
in the patch series?
This is for HDMI N/CTS setting and this needs a lot of test on HDMI platforms.
Regards,
Libin
>
> >
> > >
> > > >
> > > > Regards,
> > > > Libin
> > > >
> > > > >
> > > > > > What's more it can handle sample rate/tmds changes.
> > > > > >
> > > > > > Regards,
> > > > > > Libin
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > + }
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > @@ -637,7 +728,7 @@ static int
> > > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > > struct drm_display_mode *mode;
> > > > > > > > > > struct i915_audio_component *acomp = dev_priv-
> > > > > >audio_component;
> > > > > > > > > > enum pipe pipe = INVALID_PIPE;
> > > > > > > > > > - u32 tmp;
> > > > > > > > > > + u32 tmp, m;
> > > > > > > > > > int n;
> > > > > > > > > > int err = 0;
> > > > > > > > > >
> > > > > > > > > > @@ -653,7 +744,8 @@ static int
> > > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > > intel_encoder = dev_priv->dig_port_map[port];
> > > > > > > > > > /* intel_encoder might be NULL for DP MST */
> > > > > > > > > > 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))) {
> > > > > > > > >
> > > > > > > > > Hmm. Should probablt move this stuff later so that we
> > > > > > > > > can use check
> > > > > > > > > crtc->config->output_types instead. But I guess that's
> > > > > > > > > crtc->config->more
> > > > > > > > > appropriately left to any MST patch.
> > > > > > > >
> > > > > > > > The same reason as intel_has_dp_encoder(). And if it is
> > > > > > > > wrong for DP MST, let's have a separate patch when we support
> DP MST.
> > > > > > > > At that time, we can do a full test on DP MST.
> > > > > > >
> > > > > > > Yeah this part we can leave for the MST patch as it'll
> > > > > > > involve a bit more than just adding another check to the if
> statement.
> > > > > > >
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Libin
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > DRM_DEBUG_KMS("no valid port %c\n",
> > > > > port_name(port));
> > > > > > > > > > err = -ENODEV;
> > > > > > > > > > goto unlock;
> > > > > > > > > > @@ -681,7 +773,7 @@ static int
> > > > > > > > > i915_audio_component_sync_audio_rate(struct device *dev,
> > > > > > > > > > goto unlock;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > - n = audio_config_get_n(mode, rate);
> > > > > > > > > > + n = audio_config_get_n(crtc, mode, rate);
> > > > > > > > > > if (n == 0) {
> > > > > > > > > > DRM_DEBUG_KMS("Using automatic mode
> for N
> > > > > value on
> > > > > > > > > port %c\n",
> > > > > > > > > > port_name(port));
> @@ -693,8 +785,17 @@ static
> > > > > > > > > > int i915_audio_component_sync_audio_rate(struct device
> > > > > > > > > > *dev,
> > > > > > > > > >
> > > > > > > > > > /* 3. set the N/CTS/M */
> > > > > > > > > > tmp = I915_READ(HSW_AUD_CFG(pipe));
> > > > > > > > > > - tmp = audio_config_setup_n_reg(n, tmp);
> > > > > > > > > > + tmp = audio_config_setup_n_reg(crtc, n, tmp);
> > > > > > > > > > I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > > > > > > > > > + /* setup m value for DP */
> > > > > > > > > > + if (intel_crtc_has_type(crtc->config,
> > > > > > > > > > +INTEL_OUTPUT_DP)) {
> > > > > > > > >
> > > > > > > > > intel_has_dp_encoder()
> > > > > > > > >
> > > > > > > > > > + m = audio_config_get_m(crtc, rate);
> > > > > > > > > > + if (m == 0)
> > > > > > > > > > + goto unlock;
> > > > > > > > > > + tmp =
> I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > > + tmp = audio_config_setup_m_reg(crtc, m,
> tmp);
> > > > > > > > > > + I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> tmp);
> > > > > > > > > > + }
> > > > > > > > > >
> > > > > > > > > > unlock:
> > > > > > > > > > mutex_unlock(&dev_priv->av_mutex);
> > > > > > > > > > --
> > > > > > > > > > 1.9.1
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Ville Syrjälä
> > > > > > > > > Intel OTC
> > > > > > >
> > > > > > > --
> > > > > > > Ville Syrjälä
> > > > > > > Intel OTC
> > > > >
> > > > > --
> > > > > Ville Syrjälä
> > > > > Intel OTC
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 8:55 ` Yang, Libin
@ 2016-08-05 10:54 ` Ville Syrjälä
2016-08-05 12:32 ` Yang, Libin
0 siblings, 1 reply; 19+ messages in thread
From: Ville Syrjälä @ 2016-08-05 10:54 UTC (permalink / raw)
To: Yang, Libin; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
On Fri, Aug 05, 2016 at 08:55:27AM +0000, Yang, Libin wrote:
>
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, August 5, 2016 4:45 PM
> > To: Yang, Libin <libin.yang@intel.com>
> > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> > tiwai@suse.de
> > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> >
> > On Fri, Aug 05, 2016 at 07:24:14AM +0000, Yang, Libin wrote:
> > >
> > > > -----Original Message-----
> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > Sent: Friday, August 5, 2016 3:17 PM
> > > > To: Yang, Libin <libin.yang@intel.com>
> > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > >
> > > > > > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > > > > > + if (m != 0) {
> > > > > > > > > > > + tmp =
> > > > > > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > > > + tmp =
> > audio_config_setup_m_reg(intel_crtc,
> > > > > > m, tmp);
> > > > > > > > > > > +
> > I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > > > > > tmp);
> > > > > > > > > >
> > > > > > > > > > We should program this register even for HDMI, so that
> > > > > > > > > > we don't leak invalid register values eg. when changing from DP-
> > >HDMI.
> > > > > > > > >
> > > > > > > > > HDMI doesn't need set these values based on our test. It
> > > > > > > > > seems silicon can handle smoothly for HDMI.
> > > > > > > >
> > > > > > > > Yes, but we nee to make sure we clear whatever we programmed
> > > > > > > > in for DP previously.
> > > > > > >
> > > > > > > The silicon seems to be able to handle the situation of DP ->
> > HDMI > > > > > and HDMI -> DP.
> > > > > >
> > > > > > Did you make sure eg. that the power well didn't get toggled in
> > between?
> > > > > > That would reset the register anyway.
> > > > >
> > > > > We have done the test for this case. So far it is OK.
> > > >
> > > > Test what? Checking that the register gets reset w/o any power well
> > > > toggling and the like? So what event does reset that register?
> > >
> > > The register is used to set m/cts, which will impacts the audio clock
> > > sync. We didn't check the register reset or not. But the HDMI audio
> > > and DP will both works smoothly. And for your consideration, it is for
> > > HDMI. Let's make another patch for this issue if we really met the
> > > issue that hdmi can't sync the clock. What do you think?
> >
> > How about we just always write the register to make sure we won't get any
> > stupid bugs because of this.
>
> OK. Do you think it is OK I will write a separate patch for it, not included
> in the patch series?
>
> This is for HDMI N/CTS setting and this needs a lot of test on HDMI platforms.
I don't mean you should write the CTS value there, that would change
the behaviour. Just make sure the manual programming bit is cleared
etc. like it was before this DP patch. Ie. probably just overwrite the
register with 0, or something.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
2016-08-05 10:54 ` Ville Syrjälä
@ 2016-08-05 12:32 ` Yang, Libin
0 siblings, 0 replies; 19+ messages in thread
From: Yang, Libin @ 2016-08-05 12:32 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: tiwai, Vetter, Daniel, libin.yang, intel-gfx
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, August 5, 2016 6:54 PM
> To: Yang, Libin <libin.yang@intel.com>
> Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> jani.nikula@linux.intel.com; Vetter, Daniel <daniel.vetter@intel.com>;
> tiwai@suse.de
> Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
>
> On Fri, Aug 05, 2016 at 08:55:27AM +0000, Yang, Libin wrote:
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Friday, August 5, 2016 4:45 PM
> > > To: Yang, Libin <libin.yang@intel.com>
> > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > >
> > > On Fri, Aug 05, 2016 at 07:24:14AM +0000, Yang, Libin wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > > > Sent: Friday, August 5, 2016 3:17 PM
> > > > > To: Yang, Libin <libin.yang@intel.com>
> > > > > Cc: libin.yang@linux.intel.com; intel-gfx@lists.freedesktop.org;
> > > > > jani.nikula@linux.intel.com; Vetter, Daniel
> > > > > <daniel.vetter@intel.com>; tiwai@suse.de
> > > > > Subject: Re: [PATCH v3 1/3] drm/i915: set proper N/M in modeset
> > > > >
> > > > > > > > > > > > + m = audio_config_get_m(intel_crtc, rate);
> > > > > > > > > > > > + if (m != 0) {
> > > > > > > > > > > > + tmp =
> > > > > > > I915_READ(HSW_AUD_M_CTS_ENABLE(pipe));
> > > > > > > > > > > > + tmp =
> > > audio_config_setup_m_reg(intel_crtc,
> > > > > > > m, tmp);
> > > > > > > > > > > > +
> > > I915_WRITE(HSW_AUD_M_CTS_ENABLE(pipe),
> > > > > > > tmp);
> > > > > > > > > > >
> > > > > > > > > > > We should program this register even for HDMI, so
> > > > > > > > > > > that we don't leak invalid register values eg. when
> > > > > > > > > > > changing from DP-
> > > >HDMI.
> > > > > > > > > >
> > > > > > > > > > HDMI doesn't need set these values based on our test.
> > > > > > > > > > It seems silicon can handle smoothly for HDMI.
> > > > > > > > >
> > > > > > > > > Yes, but we nee to make sure we clear whatever we
> > > > > > > > > programmed in for DP previously.
> > > > > > > >
> > > > > > > > The silicon seems to be able to handle the situation of DP
> > > > > > > > ->
> > > HDMI > > > > > and HDMI -> DP.
> > > > > > >
> > > > > > > Did you make sure eg. that the power well didn't get toggled
> > > > > > > in
> > > between?
> > > > > > > That would reset the register anyway.
> > > > > >
> > > > > > We have done the test for this case. So far it is OK.
> > > > >
> > > > > Test what? Checking that the register gets reset w/o any power
> > > > > well toggling and the like? So what event does reset that register?
> > > >
> > > > The register is used to set m/cts, which will impacts the audio
> > > > clock sync. We didn't check the register reset or not. But the
> > > > HDMI audio and DP will both works smoothly. And for your
> > > > consideration, it is for HDMI. Let's make another patch for this
> > > > issue if we really met the issue that hdmi can't sync the clock. What do
> you think?
> > >
> > > How about we just always write the register to make sure we won't
> > > get any stupid bugs because of this.
> >
> > OK. Do you think it is OK I will write a separate patch for it, not
> > included in the patch series?
> >
> > This is for HDMI N/CTS setting and this needs a lot of test on HDMI platforms.
>
> I don't mean you should write the CTS value there, that would change the
> behaviour. Just make sure the manual programming bit is cleared etc. like it
> was before this DP patch. Ie. probably just overwrite the register with 0, or
> something.
Get it. I was thinking you are saying setting the CTS.
Regards,
Libin
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-08-05 12:32 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-04 7:58 [PATCH v3 1/3] drm/i915: set proper N/M in modeset libin.yang
2016-08-04 7:58 ` [PATCH v3 2/3] drm/i915: set proper N/MCTS on more platforms libin.yang
2016-08-04 8:54 ` Ville Syrjälä
2016-08-05 5:41 ` Yang, Libin
2016-08-04 7:58 ` [PATCH v3 3/3] drm/i915: HDMI audio gets the TMDS clock by crtc_clock libin.yang
2016-08-04 8:57 ` Ville Syrjälä
2016-08-04 8:52 ` [PATCH v3 1/3] drm/i915: set proper N/M in modeset Ville Syrjälä
2016-08-05 5:39 ` Yang, Libin
2016-08-05 5:56 ` Ville Syrjälä
2016-08-05 6:16 ` Yang, Libin
2016-08-05 6:36 ` Ville Syrjälä
2016-08-05 6:40 ` Yang, Libin
2016-08-05 7:16 ` Ville Syrjälä
2016-08-05 7:24 ` Yang, Libin
2016-08-05 8:45 ` Ville Syrjälä
2016-08-05 8:55 ` Yang, Libin
2016-08-05 10:54 ` Ville Syrjälä
2016-08-05 12:32 ` Yang, Libin
2016-08-04 9:32 ` ✗ Ro.CI.BAT: failure for series starting with [v3,1/3] " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.