[alsa-devel] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com --- include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); + int (*set_ncts)(struct device *, int port, int dev_entry, + int rate); } *ops; };
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com --- drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac + #define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct { + int sample_rate; + int clock; + int n; + int cts; +} aud_ncts[] = { + { 44100, TMDS_296M, 4459, 234375 }, + { 44100, TMDS_297M, 4704, 247500 }, + { 48000, TMDS_296M, 5824, 281250 }, + { 48000, TMDS_297M, 5120, 247500 }, + { 32000, TMDS_296M, 5824, 421875 }, + { 32000, TMDS_297M, 3072, 222750 }, + { 88200, TMDS_296M, 8918, 234375 }, + { 88200, TMDS_297M, 9408, 247500 }, + { 96000, TMDS_296M, 11648, 281250 }, + { 96000, TMDS_297M, 10240, 247500 }, + { 176400, TMDS_296M, 17836, 234375 }, + { 176400, TMDS_297M, 18816, 247500 }, + { 44100, TMDS_296M, 23296, 281250 }, + { 44100, TMDS_297M, 20480, 247500 }, +}; + /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int port, + int dev_entry, int rate) +{ + struct drm_i915_private *dev_priv = dev_to_i915(dev); + struct drm_device *drm_dev = dev_priv->dev; + struct intel_encoder *intel_encoder; + struct intel_digital_port *intel_dig_port; + struct intel_crtc *crtc; + struct drm_display_mode *mode; + enum pipe pipe = -1; + u32 tmp; + int i; + int n_low, n_up, n = 0; + + /* 1. get the pipe */ + for_each_intel_encoder(drm_dev, intel_encoder) { + intel_dig_port = enc_to_dig_port(&intel_encoder->base); + if (port == intel_dig_port->port) { + crtc = to_intel_crtc(intel_encoder->base.crtc); + if (!crtc) { + DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__); + continue; + } + pipe = crtc->pipe; + break; + } + } + + if (pipe == -1) { + DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); + return -ENODEV; + } + DRM_DEBUG_KMS("pipe %c connects port %c\n", + pipe_name(pipe), port_name(port)); + mode = &crtc->config->base.adjusted_mode; + + /* 2. Need set the N/CTS manually at some frequencies */ + if ((mode->clock != TMDS_297M) && + (mode->clock != TMDS_296M)) { + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + return 0; + } + + /* 3. calculate the N/CTS */ + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { + if ((rate == aud_ncts[i].sample_rate) && + (mode->clock == aud_ncts[i].clock)) { + n = aud_ncts[i].n; + break; + } + } + if (n == 0) + return 0; + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + + /* 4. set the N/CTS */ + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp |= AUD_CONFIG_N_PROG_ENABLE; + tmp &= ~AUD_CONFIG_UPPER_N_MASK; + tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT); + tmp &= ~AUD_CONFIG_LOWER_N_MASK; + tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT); + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + + return 0; +} + static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq, + .set_ncts = i915_audio_component_set_ncts, };
static int i915_audio_component_bind(struct device *i915_dev,
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
You're not using this register in this patch.
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {crtc = to_intel_crtc(intel_encoder->base.crtc);if (!crtc) {DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);continue;}pipe = crtc->pipe;break;}- }
- if (pipe == -1) {
INVALID_PIPE
DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. Need set the N/CTS manually at some frequencies */
- if ((mode->clock != TMDS_297M) &&
(mode->clock != TMDS_296M)) {tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
I'm thinking it would be better to always use the manual setting. There may be obscure bugs due to *sometimes* using automatic mode and some other times not. Or maybe we could use automatic mode in error scenarios?
- /* 3. calculate the N/CTS */
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}- }
This part looks like a function to be abstracted (see e.g. audio_config_hdmi_pixel_clock).
- if (n == 0)
return 0;
This needs at least a debug warning and perhaps reverting to the automatic mode.
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
You could squash the ors and ands together.
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:00 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95
++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
You're not using this register in this patch.
Oh, yes. In my first inside version, I use it. But remove using it later. I will remove it. Thanks.
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -514,12 +538,83 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int
port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
intel_dig_port = enc_to_dig_port(&intel_encoder-base);
if (port == intel_dig_port->port) {crtc = to_intel_crtc(intel_encoder->base.crtc);if (!crtc) {DRM_DEBUG_KMS("%s: crtc is NULL\n",__func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == -1) {
INVALID_PIPE
Got it. Thanks.
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. Need set the N/CTS manually at some frequencies */
- if ((mode->clock != TMDS_297M) &&
(mode->clock != TMDS_296M)) {tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
I'm thinking it would be better to always use the manual setting. There may be obscure bugs due to *sometimes* using automatic mode and some other times not. Or maybe we could use automatic mode in error scenarios?
We have done a lot test using automatic mode in other TMDS (resolution). If we always use the manual setting, we may need too much testing on other TMDS on different platforms. It seems using automatic mode in error scenarios will cause no sound.
- /* 3. calculate the N/CTS */
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}- }
This part looks like a function to be abstracted (see e.g. audio_config_hdmi_pixel_clock).
OK. I will use a function for it.
- if (n == 0)
return 0;This needs at least a debug warning and perhaps reverting to the automatic mode.
OK. I see.
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
You could squash the ors and ands together.
OK.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
You could squash the ors and ands together.
Also read-modify-write considered evil. If you need to save a few bits please be explicit about it like
tmp &= BITS_WE_WANT_TO_KEEP;
if there's no bits to keep just start out with 0. If we have too much rwm to hw registers then probably something with the overall design is botched, e.g. with the pipe config precomputation we managed to remove almost all the rwm code for modesets. -Daniel
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 12, 2015 9:05 PM To: Jani Nikula Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Mon, Aug 10, 2015 at 03:00:13PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
You could squash the ors and ands together.
Also read-modify-write considered evil. If you need to save a few bits please be explicit about it like
tmp &= BITS_WE_WANT_TO_KEEP;
OK, I will try to use this format.
if there's no bits to keep just start out with 0. If we have too much rwm to hw registers then probably something with the overall design is botched, e.g. with the pipe config precomputation we managed to remove almost all the rwm code for modesets.
Yes, if we can set the register by starting out with 0, it will be better. But in this case, there are some reserved bits. We should reserve its bits for compatibility, I think.
-Daniel
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {crtc = to_intel_crtc(intel_encoder->base.crtc);if (!crtc) {DRM_DEBUG_KMS("%s: crtc is NULL\n", __func__);continue;}pipe = crtc->pipe;break;}- }
- if (pipe == -1) {
DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. Need set the N/CTS manually at some frequencies */
- if ((mode->clock != TMDS_297M) &&
(mode->clock != TMDS_296M)) {tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
- /* 3. calculate the N/CTS */
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}- }
- if (n == 0)
return 0;- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:17 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95
++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -514,12 +538,83 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int
port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
As we only need set the n/cts when playing the audio, this can make sure that audio driver has already required the power before we call the callback function.
Regards, Libin
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
intel_dig_port = enc_to_dig_port(&intel_encoder-base);
if (port == intel_dig_port->port) {crtc = to_intel_crtc(intel_encoder->base.crtc);if (!crtc) {DRM_DEBUG_KMS("%s: crtc is NULL\n",__func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == -1) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. Need set the N/CTS manually at some frequencies */
- if ((mode->clock != TMDS_297M) &&
(mode->clock != TMDS_296M)) {tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
- /* 3. calculate the N/CTS */
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}- }
- if (n == 0)
return 0;- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp |= AUD_CONFIG_N_PROG_ENABLE;
- tmp &= ~AUD_CONFIG_UPPER_N_MASK;
- tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
- tmp &= ~AUD_CONFIG_LOWER_N_MASK;
- tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .set_ncts = i915_audio_component_set_ncts,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -514,12 +538,83 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
If we call this and end up actually dropping the power well then the register writes will go exactly nowhere at all. Which indicates a bug in how this is orchestrated. There is probably one ... -Daniel
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 12, 2015 9:06 PM To: Jani Nikula Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95
++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode *mode)
{ @@ -514,12 +538,83 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int
port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
If we call this and end up actually dropping the power well then the register writes will go exactly nowhere at all. Which indicates a bug in how this is orchestrated. There is probably one ...
Sorry, I'm not understanding your meaning clearly. Do you mean if we put the power well, the register's value will be invalid? Actually, we found another issue for audio power management (this is another bug, not related to this feature), and I'm thinking to add get_power/put_power as Jani said in another patch.
Regards, Libin
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 12, 2015 9:06 PM To: Jani Nikula Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95
++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode *mode)
{ @@ -514,12 +538,83 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_set_ncts(struct device *dev, int
port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
If we call this and end up actually dropping the power well then the register writes will go exactly nowhere at all. Which indicates a bug in how this is orchestrated. There is probably one ...
Sorry, I'm not understanding your meaning clearly. Do you mean if we put the power well, the register's value will be invalid? Actually, we found another issue for audio power management (this is another bug, not related to this feature), and I'm thinking to add get_power/put_power as Jani said in another patch.
No, the point was that you should rather check that we have power. If there isn't, and you get/put, you'll lose the values at the put.
BR, Jani.
Regards, Libin
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, August 12, 2015 10:45 PM To: Yang, Libin; Daniel Vetter Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 12, 2015 9:06 PM To: Jani Nikula Cc: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement
set_ncts
callback
On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Display audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 2 + drivers/gpu/drm/i915/intel_audio.c | 95
++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ea46d68..da2d128 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..eddf37f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,30 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 44100, TMDS_296M, 23296, 281250 },
- { 44100, TMDS_297M, 20480, 247500 },
+};
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode *mode)
{ @@ -514,12 +538,83 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_set_ncts(struct device *dev,
int
port,
int dev_entry, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int i;
- int n_low, n_up, n = 0;
I think you'll need the power well get here, and put at the end. Or check that we it.
If we call this and end up actually dropping the power well then the register writes will go exactly nowhere at all. Which indicates a bug
in
how this is orchestrated. There is probably one ...
Sorry, I'm not understanding your meaning clearly. Do you mean if we put the power well, the register's value will be invalid? Actually, we found another issue for audio power management (this is another bug, not related to this feature), and I'm thinking to add get_power/put_power as Jani said in another patch.
No, the point was that you should rather check that we have power. If there isn't, and you get/put, you'll lose the values at the put.
In another patch, we may not care the register value, we need get_power to trigger an interrupt to audio for the hotplug. Otherwise audio driver will never receive the interrupt. After the interrupt is triggered, we may not care the registers. This is a draft thought, I need do more testing for that bug. Let's discuss about it in another patch thread :)
Regards, Libin
BR, Jani.
Regards, Libin
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Jani Nikula, Intel Open Source Technology Center
From: Libin Yang libin.yang@intel.com
On some Intel platforms, display audio need set N/CTS manually at some TMDS frequencies.
Signed-off-by: Libin Yang libin.yang@intel.com --- sound/pci/hda/patch_hdmi.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..918435e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1770,6 +1770,16 @@ static bool check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid) return non_pcm; }
+/* There is a fixed mapping between audio pin node and display port + * on current Intel platforms: + * Pin Widget 5 - PORT B (port = 1 in i915 driver) + * Pin Widget 6 - PORT C (port = 2 in i915 driver) + * Pin Widget 7 - PORT D (port = 3 in i915 driver) + */ +static int intel_pin2port(hda_nid_t pin_nid) +{ + return pin_nid - 4; +}
/* * HDMI callbacks @@ -1786,6 +1796,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, int pin_idx = hinfo_to_pin_index(codec, hinfo); struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); hda_nid_t pin_nid = per_pin->pin_nid; + struct snd_pcm_runtime *runtime = substream->runtime; + struct i915_audio_component *acomp = codec->bus->core.audio_component; bool non_pcm; int pinctl;
@@ -1802,6 +1814,17 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx); }
+ /* Set N/CTS for HSW, BDW and SKL platforms. + * These platforms need call set_ncts to set the N/CTS manually, + * otherwise there is no sound in some sample rates. + */ + if (is_haswell_plus(codec)) { + /* Todo: add DP1.2 MST audio support later */ + if (acomp && acomp->ops && acomp->ops->set_ncts) + acomp->ops->set_ncts(acomp->dev, + intel_pin2port(pin_nid), + 0, runtime->rate); + } non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
On some Intel platforms, display audio need set N/CTS manually at some TMDS frequencies.
Signed-off-by: Libin Yang libin.yang@intel.com
sound/pci/hda/patch_hdmi.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..918435e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1770,6 +1770,16 @@ static bool check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid) return non_pcm; }
+/* There is a fixed mapping between audio pin node and display port
- on current Intel platforms:
- Pin Widget 5 - PORT B (port = 1 in i915 driver)
- Pin Widget 6 - PORT C (port = 2 in i915 driver)
- Pin Widget 7 - PORT D (port = 3 in i915 driver)
- */
+static int intel_pin2port(hda_nid_t pin_nid) +{
- return pin_nid - 4;
+}
/*
- HDMI callbacks
@@ -1786,6 +1796,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, int pin_idx = hinfo_to_pin_index(codec, hinfo); struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); hda_nid_t pin_nid = per_pin->pin_nid;
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct i915_audio_component *acomp = codec->bus->core.audio_component; bool non_pcm; int pinctl;
@@ -1802,6 +1814,17 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx); }
- /* Set N/CTS for HSW, BDW and SKL platforms.
* These platforms need call set_ncts to set the N/CTS manually,* otherwise there is no sound in some sample rates.*/- if (is_haswell_plus(codec)) {
Maybe you should move this check to the graphics side, and do the call whenever the callback is non-NULL, and you can be sure of the pin->port mapping?
/* Todo: add DP1.2 MST audio support later */if (acomp && acomp->ops && acomp->ops->set_ncts)acomp->ops->set_ncts(acomp->dev,intel_pin2port(pin_nid),0, runtime->rate);- } non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:03 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v3 3/4] ALSA: hda - display audio call ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
On some Intel platforms, display audio need set N/CTS manually at some TMDS frequencies.
Signed-off-by: Libin Yang libin.yang@intel.com
sound/pci/hda/patch_hdmi.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c
b/sound/pci/hda/patch_hdmi.c
index a97db5f..918435e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1770,6 +1770,16 @@ static bool
check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
return non_pcm; }
+/* There is a fixed mapping between audio pin node and display
port
- on current Intel platforms:
- Pin Widget 5 - PORT B (port = 1 in i915 driver)
- Pin Widget 6 - PORT C (port = 2 in i915 driver)
- Pin Widget 7 - PORT D (port = 3 in i915 driver)
- */
+static int intel_pin2port(hda_nid_t pin_nid) +{
- return pin_nid - 4;
+}
/*
- HDMI callbacks
@@ -1786,6 +1796,8 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
int pin_idx = hinfo_to_pin_index(codec, hinfo); struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); hda_nid_t pin_nid = per_pin->pin_nid;
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct i915_audio_component *acomp = codec->bus-
core.audio_component; bool non_pcm; int pinctl;
@@ -1802,6 +1814,17 @@ static int
generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
intel_not_share_assigned_cvt(codec, pin_nid, per_pin-mux_idx); }
- /* Set N/CTS for HSW, BDW and SKL platforms.
* These platforms need call set_ncts to set the N/CTS manually,* otherwise there is no sound in some sample rates.*/- if (is_haswell_plus(codec)) {
Maybe you should move this check to the graphics side, and do the call whenever the callback is non-NULL, and you can be sure of the pin-
port
mapping?
OK, I will remove this check here.
We have checked with silicon team for the mapping.
/* Todo: add DP1.2 MST audio support later */if (acomp && acomp->ops && acomp->ops->set_ncts)acomp->ops->set_ncts(acomp->dev,intel_pin2port(pin_nid),0, runtime->rate);- } non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
Signed-off-by: Libin Yang libin.yang@intel.com --- drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ + _HSW_AUD_STR_DESC_1, \ + _HSW_AUD_STR_DESC_2) + #define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i; + int cvt_idx; + int n_low, n_up, n; + int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld)); @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+ if ((mode->clock == 297000) || + (mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) { + tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL); + cvt_idx = (tmp >> pipe*8) & 0xff; + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); + base_rate = tmp & (1 << 14); + if (base_rate == 0) + rate = 48000; + else + rate = 44100; + mul = (tmp & (0x7 << 11)) + 1; + div = (tmp & (0x7 << 8)) + 1; + rate = rate * mul / div; + } + /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode); + + if ((mode->clock != 297000) && + (mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) { + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + } else { + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { + if ((rate == aud_ncts[i].sample_rate) && + (mode->clock == aud_ncts[i].clock)) { + n = aud_ncts[i].n; + break; + } + } + if (n != 0) { + tmp |= AUD_CONFIG_N_PROG_ENABLE; + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + tmp |= AUD_CONFIG_N_PROG_ENABLE; + tmp &= ~AUD_CONFIG_UPPER_N_MASK; + tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT); + tmp &= ~AUD_CONFIG_LOWER_N_MASK; + tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT); + } + } + I915_WRITE(HSW_AUD_CFG(pipe), tmp); }
On Mon, 10 Aug 2015 09:32:11 +0200, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1, \_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {
TMDS_297M and TMDS_296M can be used here?
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}
Missing initialization of n?
if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;
You don't need to set it twice.
tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);}- }
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Monday, August 10, 2015 4:04 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Mon, 10 Aug 2015 09:32:11 +0200, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))){
TMDS_297M and TMDS_296M can be used here?
Oh, sorry, I forgot to use it here.
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))){
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}Missing initialization of n?
Oh, yes, I will fix it.
if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;You don't need to set it twice.
Yes.
tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
thanks,
Takashi
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when the modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1, \_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) {tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);}- }
Please de-duplicate the copy-paste from patch 2. At the very least you should use a helper for finding the parameters from the table.
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when the modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000,1001))) {
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
Yes. I will do it.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000,1001))) {
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up <<AUD_CONFIG_UPPER_N_SHIFT);
tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
Please de-duplicate the copy-paste from patch 2. At the very least you should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when the modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's a modeset, the graphics driver will disable audio and invalidate the ELD, which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug or not when there's a codec disable/enable.
BR, Jani.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000,1001))) {
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
Yes. I will do it.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000,1001))) {
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up <<AUD_CONFIG_UPPER_N_SHIFT);
tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
Please de-duplicate the copy-paste from patch 2. At the very least you should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when the modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's a modeset, the graphics driver will disable audio and invalidate the ELD, which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug or not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
BR, Jani.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000,1001))) {
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
Yes. I will do it.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000,1001))) {
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up <<AUD_CONFIG_UPPER_N_SHIFT);
tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
Please de-duplicate the copy-paste from patch 2. At the very least
you
should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when the modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's a modeset, the graphics driver will disable audio and invalidate the ELD, which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug or not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
Timeout.
BR, Jani.
BR, Jani.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000,1001))) {
tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
Yes. I will do it.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000,1001))) {
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up <<AUD_CONFIG_UPPER_N_SHIFT);
tmp &= ~AUD_CONFIG_LOWER_N_MASK;tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
Please de-duplicate the copy-paste from patch 2. At the very least
you
should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical value, the N/CTS need to be set manually if audio is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when
the
modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's
a
modeset, the graphics driver will disable audio and invalidate the
ELD,
which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug
or
not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Timeout.
BR, Jani.
BR, Jani.
Also some comments in-line, provided you've convinced me first. ;)
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ drivers/gpu/drm/i915/intel_audio.c | 42
++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index da2d128..85f3beb 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells {
_HSW_AUD_DIG_CNVT_2)
#define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,
\
_HSW_AUD_STR_DESC_2)
#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index eddf37f..082f96d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int cvt_idx;
int n_low, n_up, n;
int base_rate, mul, div, rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUDIO_ELD_VALID(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- if ((mode->clock == 297000) ||
(mode->clock == DIV_ROUND_UP(297000 * 1000,1001))) {
tmp =I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
cvt_idx = (tmp >> pipe*8) & 0xff;tmp = I915_READ(AUD_STR_DESC(cvt_idx));base_rate = tmp & (1 << 14);if (base_rate == 0)rate = 48000;elserate = 44100;mul = (tmp & (0x7 << 11)) + 1;div = (tmp & (0x7 << 8)) + 1;rate = rate * mul / div;- }
This should be abstracted to a separate function.
Yes. I will do it.
- /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
@@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- if ((mode->clock != 297000) &&
(mode->clock != DIV_ROUND_UP(297000 * 1000,1001))) {
tmp &= ~AUD_CONFIG_N_PROG_ENABLE;- } else {
for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {if ((rate == aud_ncts[i].sample_rate) &&(mode->clock ==aud_ncts[i].clock)) {
n = aud_ncts[i].n;break;}}if (n != 0) {tmp |= AUD_CONFIG_N_PROG_ENABLE;n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp |= AUD_CONFIG_N_PROG_ENABLE;tmp &= ~AUD_CONFIG_UPPER_N_MASK;tmp |= (n_up <<AUD_CONFIG_UPPER_N_SHIFT);
tmp &=~AUD_CONFIG_LOWER_N_MASK;
tmp |= (n_low <<AUD_CONFIG_LOWER_N_SHIFT);
}- }
Please de-duplicate the copy-paste from patch 2. At the very least
you
should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
}
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 8:10 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
modeset
On Mon, 10 Aug 2015, libin.yang@intel.com wrote: > From: Libin Yang libin.yang@intel.com > > When modeset occurs and the TMDS frequency is set to some > speical value, the N/CTS need to be set manually if audio > is playing.
When modeset occurs, we disable everything, and then re-enable everything, and notify the audio driver every step of the way. IIUC there should be no audio playing across the modeset, and when
the
modeset is complete and we've set the valid ELD, the audio driver should call the callback from the earlier patches to set the correct audio rate.
Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's
a
modeset, the graphics driver will disable audio and invalidate the
ELD,
which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug
or
not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
Please find one more code comment below.
Timeout.
BR, Jani.
BR, Jani.
Also some comments in-line, provided you've convinced me first. ;)
> Signed-off-by: Libin Yang libin.yang@intel.com > --- > drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ > drivers/gpu/drm/i915/intel_audio.c | 42 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index da2d128..85f3beb 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { >
_HSW_AUD_DIG_CNVT_2)
> #define DIP_PORT_SEL_MASK 0x3 > > +#define _HSW_AUD_STR_DESC_1 0x65084 > +#define _HSW_AUD_STR_DESC_2 0x65184 > +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ > +
_HSW_AUD_STR_DESC_1,
\ > +
_HSW_AUD_STR_DESC_2)
> + > #define _HSW_AUD_EDID_DATA_A 0x65050 > #define _HSW_AUD_EDID_DATA_B 0x65150 > #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c > index eddf37f..082f96d 100644 > --- a/drivers/gpu/drm/i915/intel_audio.c > +++ b/drivers/gpu/drm/i915/intel_audio.c > @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector, > const uint8_t *eld = connector->eld; > uint32_t tmp; > int len, i; > + int cvt_idx; > + int n_low, n_up, n; > + int base_rate, mul, div, rate; > > DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
bytes
ELD\n", > pipe_name(pipe), drm_eld_size(eld)); > @@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector, > tmp |= AUDIO_ELD_VALID(pipe); > I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); > > + if ((mode->clock == 297000) || > + (mode->clock == DIV_ROUND_UP(297000 * 1000, 1001))) {
Please abstract that condition into a helper and give it a helpful name. That's repeated many times now, in this and negated forms, and I want the compiler to ensure it's the same in each place.
Thanks, Jani.
> + tmp =
I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> + cvt_idx = (tmp >> pipe*8) & 0xff; > + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); > + base_rate = tmp & (1 << 14); > + if (base_rate == 0) > + rate = 48000; > + else > + rate = 44100; > + mul = (tmp & (0x7 << 11)) + 1; > + div = (tmp & (0x7 << 8)) + 1; > + rate = rate * mul / div; > + }
This should be abstracted to a separate function.
Yes. I will do it.
> + > /* Enable timestamps */ > tmp = I915_READ(HSW_AUD_CFG(pipe)); > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > @@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector, > tmp |= AUD_CONFIG_N_VALUE_INDEX; > else > tmp |= audio_config_hdmi_pixel_clock(mode); > + > + if ((mode->clock != 297000) && > + (mode->clock != DIV_ROUND_UP(297000 * 1000, 1001))) { > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > + } else { > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { > + if ((rate == aud_ncts[i].sample_rate) && > + (mode->clock ==
aud_ncts[i].clock)) {
> + n = aud_ncts[i].n; > + break; > + } > + } > + if (n != 0) { > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > + n_low = n & 0xfff; > + n_up = (n >> 12) & 0xff; > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > + tmp &= ~AUD_CONFIG_UPPER_N_MASK; > + tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT); > + tmp &=
~AUD_CONFIG_LOWER_N_MASK;
> + tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT); > + } > + }
Please de-duplicate the copy-paste from patch 2. At the very least
you
should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
> + > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > } > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, August 12, 2015 2:20 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Monday, August 10, 2015 8:10 PM > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de;
intel-
> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch > Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
N/CTS
in
> modeset > > On Mon, 10 Aug 2015, libin.yang@intel.com wrote: > > From: Libin Yang libin.yang@intel.com > > > > When modeset occurs and the TMDS frequency is set to
some
> > speical value, the N/CTS need to be set manually if audio > > is playing. > > When modeset occurs, we disable everything, and then re-
enable
> everything, and notify the audio driver every step of the way.
IIUC
> there should be no audio playing across the modeset, and
when
the
> modeset is complete and we've set the valid ELD, the audio
driver
> should > call the callback from the earlier patches to set the correct
audio
> rate. > > Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not
call
set_ncts.
Why does the audio driver not do anything here? Whenever
there's
a
modeset, the graphics driver will disable audio and invalidate the
ELD,
which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a
hotplug
or
not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on
previous
state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for
all
we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
OK. Thanks.
Please find one more code comment below.
Timeout.
BR, Jani.
BR, Jani.
> > Also some comments in-line, provided you've convinced me
first. ;)
> > > Signed-off-by: Libin Yang libin.yang@intel.com > > --- > > drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ > > drivers/gpu/drm/i915/intel_audio.c | 42 > ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > > index da2d128..85f3beb 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { > >
_HSW_AUD_DIG_CNVT_2)
> > #define DIP_PORT_SEL_MASK 0x3 > > > > +#define _HSW_AUD_STR_DESC_1 0x65084 > > +#define _HSW_AUD_STR_DESC_2 0x65184 > > +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ > > +
_HSW_AUD_STR_DESC_1,
> \ > > +
_HSW_AUD_STR_DESC_2)
> > + > > #define _HSW_AUD_EDID_DATA_A 0x65050 > > #define _HSW_AUD_EDID_DATA_B 0x65150 > > #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > > index eddf37f..082f96d 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > const uint8_t *eld = connector->eld; > > uint32_t tmp; > > int len, i; > > + int cvt_idx; > > + int n_low, n_up, n; > > + int base_rate, mul, div, rate; > > > > DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
bytes
> ELD\n", > > pipe_name(pipe), drm_eld_size(eld)); > > @@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > tmp |= AUDIO_ELD_VALID(pipe); > > I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); > > > > + if ((mode->clock == 297000) || > > + (mode->clock == DIV_ROUND_UP(297000 * 1000, > 1001))) {
Please abstract that condition into a helper and give it a helpful name. That's repeated many times now, in this and negated forms, and I want the compiler to ensure it's the same in each place.
OK. I will do it.
Thanks, Jani.
> > + tmp =
I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> > + cvt_idx = (tmp >> pipe*8) & 0xff; > > + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); > > + base_rate = tmp & (1 << 14); > > + if (base_rate == 0) > > + rate = 48000; > > + else > > + rate = 44100; > > + mul = (tmp & (0x7 << 11)) + 1; > > + div = (tmp & (0x7 << 8)) + 1; > > + rate = rate * mul / div; > > + } > > This should be abstracted to a separate function.
Yes. I will do it.
> > > + > > /* Enable timestamps */ > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > @@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > else > > tmp |= audio_config_hdmi_pixel_clock(mode); > > + > > + if ((mode->clock != 297000) && > > + (mode->clock != DIV_ROUND_UP(297000 * 1000, > 1001))) { > > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > > + } else { > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { > > + if ((rate == aud_ncts[i].sample_rate) && > > + (mode->clock ==
aud_ncts[i].clock)) {
> > + n = aud_ncts[i].n; > > + break; > > + } > > + } > > + if (n != 0) { > > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > > + n_low = n & 0xfff; > > + n_up = (n >> 12) & 0xff; > > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > > + tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > + tmp |= (n_up << > AUD_CONFIG_UPPER_N_SHIFT); > > + tmp &=
~AUD_CONFIG_LOWER_N_MASK;
> > + tmp |= (n_low << > AUD_CONFIG_LOWER_N_SHIFT); > > + } > > + } > > Please de-duplicate the copy-paste from patch 2. At the very
least
you
> should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
> > > + > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > } > > > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
Thanks for careful reviewing for all the patches, please see my comments.
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Monday, August 10, 2015 8:10 PM > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch > Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
> modeset > > On Mon, 10 Aug 2015, libin.yang@intel.com wrote: > > From: Libin Yang libin.yang@intel.com > > > > When modeset occurs and the TMDS frequency is set to some > > speical value, the N/CTS need to be set manually if audio > > is playing. > > When modeset occurs, we disable everything, and then re-enable > everything, and notify the audio driver every step of the way. IIUC > there should be no audio playing across the modeset, and when
the
> modeset is complete and we've set the valid ELD, the audio driver > should > call the callback from the earlier patches to set the correct audio > rate. > > Why is this patch needed? Please enlighten me.
Please image this scenario: when audio is playing, the gfx driver does the modeset. In this situation, after modeset, audio driver will do nothing and continue playing. And audio driver will not call set_ncts.
Why does the audio driver not do anything here? Whenever there's
a
modeset, the graphics driver will disable audio and invalidate the
ELD,
which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug
or
not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
It sounds like we actually need to ping the audio side _before_ we do the hw modeset while computing the pipe_config. That would probably take care of my rwm concerns, give this thing proper structure and also make sure N/CTS never get out of sync. Also then we can track this stuff in the pipe_config and throw the state checker at it to make sure it's all fine. -Daniel
Please find one more code comment below.
Timeout.
BR, Jani.
BR, Jani.
> > Also some comments in-line, provided you've convinced me first. ;) > > > Signed-off-by: Libin Yang libin.yang@intel.com > > --- > > drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ > > drivers/gpu/drm/i915/intel_audio.c | 42 > ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > > index da2d128..85f3beb 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { > >
_HSW_AUD_DIG_CNVT_2)
> > #define DIP_PORT_SEL_MASK 0x3 > > > > +#define _HSW_AUD_STR_DESC_1 0x65084 > > +#define _HSW_AUD_STR_DESC_2 0x65184 > > +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ > > +
_HSW_AUD_STR_DESC_1,
> \ > > +
_HSW_AUD_STR_DESC_2)
> > + > > #define _HSW_AUD_EDID_DATA_A 0x65050 > > #define _HSW_AUD_EDID_DATA_B 0x65150 > > #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ > > diff --git a/drivers/gpu/drm/i915/intel_audio.c > b/drivers/gpu/drm/i915/intel_audio.c > > index eddf37f..082f96d 100644 > > --- a/drivers/gpu/drm/i915/intel_audio.c > > +++ b/drivers/gpu/drm/i915/intel_audio.c > > @@ -235,6 +235,9 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > const uint8_t *eld = connector->eld; > > uint32_t tmp; > > int len, i; > > + int cvt_idx; > > + int n_low, n_up, n; > > + int base_rate, mul, div, rate; > > > > DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
bytes
> ELD\n", > > pipe_name(pipe), drm_eld_size(eld)); > > @@ -267,6 +270,21 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > tmp |= AUDIO_ELD_VALID(pipe); > > I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); > > > > + if ((mode->clock == 297000) || > > + (mode->clock == DIV_ROUND_UP(297000 * 1000, > 1001))) {
Please abstract that condition into a helper and give it a helpful name. That's repeated many times now, in this and negated forms, and I want the compiler to ensure it's the same in each place.
Thanks, Jani.
> > + tmp =
I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
> > + cvt_idx = (tmp >> pipe*8) & 0xff; > > + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); > > + base_rate = tmp & (1 << 14); > > + if (base_rate == 0) > > + rate = 48000; > > + else > > + rate = 44100; > > + mul = (tmp & (0x7 << 11)) + 1; > > + div = (tmp & (0x7 << 8)) + 1; > > + rate = rate * mul / div; > > + } > > This should be abstracted to a separate function.
Yes. I will do it.
> > > + > > /* Enable timestamps */ > > tmp = I915_READ(HSW_AUD_CFG(pipe)); > > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; > > @@ -276,6 +294,30 @@ static void
hsw_audio_codec_enable(struct
> drm_connector *connector, > > tmp |= AUD_CONFIG_N_VALUE_INDEX; > > else > > tmp |= audio_config_hdmi_pixel_clock(mode); > > + > > + if ((mode->clock != 297000) && > > + (mode->clock != DIV_ROUND_UP(297000 * 1000, > 1001))) { > > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > > + } else { > > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { > > + if ((rate == aud_ncts[i].sample_rate) && > > + (mode->clock ==
aud_ncts[i].clock)) {
> > + n = aud_ncts[i].n; > > + break; > > + } > > + } > > + if (n != 0) { > > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > > + n_low = n & 0xfff; > > + n_up = (n >> 12) & 0xff; > > + tmp |= AUD_CONFIG_N_PROG_ENABLE; > > + tmp &= ~AUD_CONFIG_UPPER_N_MASK; > > + tmp |= (n_up << > AUD_CONFIG_UPPER_N_SHIFT); > > + tmp &=
~AUD_CONFIG_LOWER_N_MASK;
> > + tmp |= (n_low << > AUD_CONFIG_LOWER_N_SHIFT); > > + } > > + } > > Please de-duplicate the copy-paste from patch 2. At the very least
you
> should use a helper for finding the parameters from the table.
OK. I see.
Regards, Libin
> > > + > > I915_WRITE(HSW_AUD_CFG(pipe), tmp); > > } > > > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 12 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 3:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote: > Hi Jani, > > Thanks for careful reviewing for all the patches, please > see my comments. > >> -----Original Message----- >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >> Sent: Monday, August 10, 2015 8:10 PM >> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- >> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch >> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
>> modeset >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote: >> > From: Libin Yang libin.yang@intel.com >> > >> > When modeset occurs and the TMDS frequency is set to some >> > speical value, the N/CTS need to be set manually if audio >> > is playing. >> >> When modeset occurs, we disable everything, and then re-enable >> everything, and notify the audio driver every step of the way. IIUC >> there should be no audio playing across the modeset, and when
the
>> modeset is complete and we've set the valid ELD, the audio driver >> should >> call the callback from the earlier patches to set the correct audio >> rate. >> >> Why is this patch needed? Please enlighten me. > > Please image this scenario: when audio is playing, the gfx driver > does the modeset. In this situation, after modeset, audio driver > will do nothing and continue playing. And audio driver will not call > set_ncts.
Why does the audio driver not do anything here? Whenever there's
a
modeset, the graphics driver will disable audio and invalidate the
ELD,
which should cause an interrupt to notify the audio driver about this. This is no different from a hot-unplug, in fact I can't see how the audio driver could even detect whether there's been a hotplug
or
not when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
It sounds like we actually need to ping the audio side _before_ we do the
Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?
Jani.
hw modeset while computing the pipe_config. That would probably take care of my rwm concerns, give this thing proper structure and also make sure N/CTS never get out of sync. Also then we can track this stuff in the pipe_config and throw the state checker at it to make sure it's all fine. -Daniel
Please find one more code comment below.
Timeout.
BR, Jani.
BR, Jani.
> >> >> Also some comments in-line, provided you've convinced me first. ;) >> >> > Signed-off-by: Libin Yang libin.yang@intel.com >> > --- >> > drivers/gpu/drm/i915/i915_reg.h | 6 ++++++ >> > drivers/gpu/drm/i915/intel_audio.c | 42 >> ++++++++++++++++++++++++++++++++++++++ >> > 2 files changed, 48 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> > index da2d128..85f3beb 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -7030,6 +7030,12 @@ enum skl_disp_power_wells { >> >
_HSW_AUD_DIG_CNVT_2)
>> > #define DIP_PORT_SEL_MASK 0x3 >> > >> > +#define _HSW_AUD_STR_DESC_1 0x65084 >> > +#define _HSW_AUD_STR_DESC_2 0x65184 >> > +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ >> > +
_HSW_AUD_STR_DESC_1,
>> \ >> > +
_HSW_AUD_STR_DESC_2)
>> > + >> > #define _HSW_AUD_EDID_DATA_A 0x65050 >> > #define _HSW_AUD_EDID_DATA_B 0x65150 >> > #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ >> > diff --git a/drivers/gpu/drm/i915/intel_audio.c >> b/drivers/gpu/drm/i915/intel_audio.c >> > index eddf37f..082f96d 100644 >> > --- a/drivers/gpu/drm/i915/intel_audio.c >> > +++ b/drivers/gpu/drm/i915/intel_audio.c >> > @@ -235,6 +235,9 @@ static void hsw_audio_codec_enable(struct >> drm_connector *connector, >> > const uint8_t *eld = connector->eld; >> > uint32_t tmp; >> > int len, i; >> > + int cvt_idx; >> > + int n_low, n_up, n; >> > + int base_rate, mul, div, rate; >> > >> > DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u
bytes
>> ELD\n", >> > pipe_name(pipe), drm_eld_size(eld)); >> > @@ -267,6 +270,21 @@ static void hsw_audio_codec_enable(struct >> drm_connector *connector, >> > tmp |= AUDIO_ELD_VALID(pipe); >> > I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); >> > >> > + if ((mode->clock == 297000) || >> > + (mode->clock == DIV_ROUND_UP(297000 * 1000, >> 1001))) {
Please abstract that condition into a helper and give it a helpful name. That's repeated many times now, in this and negated forms, and I want the compiler to ensure it's the same in each place.
Thanks, Jani.
>> > + tmp =
I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
>> > + cvt_idx = (tmp >> pipe*8) & 0xff; >> > + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); >> > + base_rate = tmp & (1 << 14); >> > + if (base_rate == 0) >> > + rate = 48000; >> > + else >> > + rate = 44100; >> > + mul = (tmp & (0x7 << 11)) + 1; >> > + div = (tmp & (0x7 << 8)) + 1; >> > + rate = rate * mul / div; >> > + } >> >> This should be abstracted to a separate function. > > Yes. I will do it. > >> >> > + >> > /* Enable timestamps */ >> > tmp = I915_READ(HSW_AUD_CFG(pipe)); >> > tmp &= ~AUD_CONFIG_N_VALUE_INDEX; >> > @@ -276,6 +294,30 @@ static void hsw_audio_codec_enable(struct >> drm_connector *connector, >> > tmp |= AUD_CONFIG_N_VALUE_INDEX; >> > else >> > tmp |= audio_config_hdmi_pixel_clock(mode); >> > + >> > + if ((mode->clock != 297000) && >> > + (mode->clock != DIV_ROUND_UP(297000 * 1000, >> 1001))) { >> > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; >> > + } else { >> > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { >> > + if ((rate == aud_ncts[i].sample_rate) && >> > + (mode->clock ==
aud_ncts[i].clock)) {
>> > + n = aud_ncts[i].n; >> > + break; >> > + } >> > + } >> > + if (n != 0) { >> > + tmp |= AUD_CONFIG_N_PROG_ENABLE; >> > + n_low = n & 0xfff; >> > + n_up = (n >> 12) & 0xff; >> > + tmp |= AUD_CONFIG_N_PROG_ENABLE; >> > + tmp &= ~AUD_CONFIG_UPPER_N_MASK; >> > + tmp |= (n_up << >> AUD_CONFIG_UPPER_N_SHIFT); >> > + tmp &=
~AUD_CONFIG_LOWER_N_MASK;
>> > + tmp |= (n_low << >> AUD_CONFIG_LOWER_N_SHIFT); >> > + } >> > + } >> >> Please de-duplicate the copy-paste from patch 2. At the very least you >> should use a helper for finding the parameters from the table. > > OK. I see. > > Regards, > Libin > >> >> > + >> > I915_WRITE(HSW_AUD_CFG(pipe), tmp); >> > } >> > >> > -- >> > 1.9.1 >> > >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Tuesday, August 11, 2015 3:14 PM > To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch > Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in > modeset > > On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote: > > Hi Jani, > > > > Thanks for careful reviewing for all the patches, please > > see my comments. > > > >> -----Original Message----- > >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > >> Sent: Monday, August 10, 2015 8:10 PM > >> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- > >> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch > >> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS
in
> >> modeset > >> > >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote: > >> > From: Libin Yang libin.yang@intel.com > >> > > >> > When modeset occurs and the TMDS frequency is set to some > >> > speical value, the N/CTS need to be set manually if audio > >> > is playing. > >> > >> When modeset occurs, we disable everything, and then re-enable > >> everything, and notify the audio driver every step of the way. IIUC > >> there should be no audio playing across the modeset, and when
the
> >> modeset is complete and we've set the valid ELD, the audio driver > >> should > >> call the callback from the earlier patches to set the correct audio > >> rate. > >> > >> Why is this patch needed? Please enlighten me. > > > > Please image this scenario: when audio is playing, the gfx driver > > does the modeset. In this situation, after modeset, audio driver > > will do nothing and continue playing. And audio driver will not call > > set_ncts. > > Why does the audio driver not do anything here? Whenever there's
a
> modeset, the graphics driver will disable audio and invalidate the
ELD,
> which should cause an interrupt to notify the audio driver about > this. This is no different from a hot-unplug, in fact I can't see how > the audio driver could even detect whether there's been a hotplug
or
> not > when there's a codec disable/enable.
Yes, it will trigger an interrupt to audio driver when unplug and another interrupt for hotplug. But from audio driver, we will not stop the playing and resume the playing based on the hotplug or modeset. The audio is always playing during the hotplug/modeset.
But the whole display, and consequently ELD, might have changed during the hotplug, why do you assume you can just keep playing?! The display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on previous state when enabling the outputs, for more reliability. We generally don't do partial changes, we disable everything and then enable again. And now you're suggesting we look at some register which for all we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
It sounds like we actually need to ping the audio side _before_ we do the
Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?
No, I mean call into snd-hda-intel before we do the modeset to ask it for what it actually wants to have set up. At least it feels a bit like only reacting to a modeset after it's done leads to confusion. Or maybe we just need to update the register values we want at compute_time instead of doing rwm fun ...
Pure gut feeling comment, I didn't look into details ;-) -Daniel
On Wed, 12 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
It sounds like we actually need to ping the audio side _before_ we do the
Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio side" above?
No, I mean call into snd-hda-intel before we do the modeset to ask it for what it actually wants to have set up. At least it feels a bit like only reacting to a modeset after it's done leads to confusion. Or maybe we just need to update the register values we want at compute_time instead of doing rwm fun ...
Pure gut feeling comment, I didn't look into details ;-)
The audio driver calls us, not vice versa...
Jani.
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 12, 2015 9:43 PM To: Jani Nikula Cc: Daniel Vetter; Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper N/CTS in modeset
On Wed, Aug 12, 2015 at 04:23:12PM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 12, 2015 at 09:20:17AM +0300, Jani Nikula wrote:
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Tuesday, August 11, 2015 4:14 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de;
intel-
gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
N/CTS in
modeset
On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com
wrote:
> Hi Jani, > >> -----Original Message----- >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >> Sent: Tuesday, August 11, 2015 3:14 PM >> To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de;
intel-
>> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch >> Subject: RE: [Intel-gfx] [PATCH v4 4/4] drm/i915: set proper
N/CTS in
>> modeset >> >> On Tue, 11 Aug 2015, "Yang, Libin" libin.yang@intel.com
wrote:
>> > Hi Jani, >> > >> > Thanks for careful reviewing for all the patches, please >> > see my comments. >> > >> >> -----Original Message----- >> >> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] >> >> Sent: Monday, August 10, 2015 8:10 PM >> >> To: Yang, Libin; alsa-devel@alsa-project.org;
tiwai@suse.de; intel-
>> >> gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch >> >> Subject: Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: set
proper N/CTS
in >> >> modeset >> >> >> >> On Mon, 10 Aug 2015, libin.yang@intel.com wrote: >> >> > From: Libin Yang libin.yang@intel.com >> >> > >> >> > When modeset occurs and the TMDS frequency is set
to some
>> >> > speical value, the N/CTS need to be set manually if
audio
>> >> > is playing. >> >> >> >> When modeset occurs, we disable everything, and then
re-enable
>> >> everything, and notify the audio driver every step of the
way. IIUC
>> >> there should be no audio playing across the modeset,
and when
the >> >> modeset is complete and we've set the valid ELD, the
audio driver
>> >> should >> >> call the callback from the earlier patches to set the
correct audio
>> >> rate. >> >> >> >> Why is this patch needed? Please enlighten me. >> > >> > Please image this scenario: when audio is playing, the gfx
driver
>> > does the modeset. In this situation, after modeset, audio
driver
>> > will do nothing and continue playing. And audio driver will
not call
>> > set_ncts. >> >> Why does the audio driver not do anything here? Whenever
there's
a >> modeset, the graphics driver will disable audio and
invalidate the
ELD, >> which should cause an interrupt to notify the audio driver
about
>> this. This is no different from a hot-unplug, in fact I can't see
how
>> the audio driver could even detect whether there's been a
hotplug
or >> not >> when there's a codec disable/enable. > > Yes, it will trigger an interrupt to audio driver when unplug > and another interrupt for hotplug. But from audio driver, > we will not stop the playing and resume the playing based > on the hotplug or modeset. The audio is always playing
during
> the hotplug/modeset.
But the whole display, and consequently ELD, might have
changed
during the hotplug, why do you assume you can just keep playing?!
The
display might even have changed from HDMI to DP or vice versa.
The eld info changing will not impact the audio playing. Actually, you can image the monitor as a digital speaker, just like the headphone to the analog audio. Unplug the speaker will not impact on the audio playing. Of course , there is a little difference, when monitor hotplug, in the interrupt, we may change the codec configuration dynamically.
And audio driver supply the mechanism to stop the audio playing when hotplug if the HW requires.
We've also been putting a lot of effort into not depending on
previous
state when enabling the outputs, for more reliability. We
generally
don't do partial changes, we disable everything and then
enable
again. And now you're suggesting we look at some register
which for all
we know may have been valid for some other display?
In the patch, it will not depend on previous state, I think. We read the register value which is based on the state after
modeset.
Okay, let's have the patches cleaned up and see. It'll be easier and quicker to solicit more review on that than this expanding thread.
It sounds like we actually need to ping the audio side _before_ we
do the
Do you mean "read _HSW_AUD_STR_DESC_*" by "ping the audio
side" above?
No, I mean call into snd-hda-intel before we do the modeset to ask it for what it actually wants to have set up. At least it feels a bit like only reacting to a modeset after it's done leads to confusion. Or maybe we just need to update the register values we want at compute_time instead of doing rwm fun ...
Actually we read the _HSW_AUD_STR_DESC_* register to get what value we are going to set. This is similar with calling audio driver to ask for the parameters. The _HSW_AUD_STR_DESC_* will tell us what audio sample rates it's using. :-)
Regards, Libin
Pure gut feeling comment, I didn't look into details ;-)
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *);
int (*set_ncts)(struct device *, int port, int dev_entry,int rate);
I'd rather call this set_audio_rate or similar, and dropping the references to N and CTS. The caller does not need to know.
I'm also not fond of adding a dev_entry parameter and leaving it unused. I do not think we know specifically how we're going to identify MST sinks, and the interface may need to be changed anyway. Let's force an update in the caller side as well when there's actually sensible support in our side.
BR, Jani.
} *ops; };
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Jani,
Thanks for reviewing, please see my comments
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 7:46 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*set_ncts)(struct device *, int port, int dev_entry,int rate);I'd rather call this set_audio_rate or similar, and dropping the references to N and CTS. The caller does not need to know.
But it seems the set_audio_rate will confuse the user. From the literal meaning, it is setting the rate of audio, such as sample rate, which will make audio driver developers confused. And the function is just setting N/CTS which is defined from HDMI SPEC.
BTW: your comment reminds me that get_cdclk_freq() seems to need change the name as cdclk is not in the open spec.
I'm also not fond of adding a dev_entry parameter and leaving it unused. I do not think we know specifically how we're going to identify MST sinks, and the interface may need to be changed anyway. Let's force an update in the caller side as well when there's actually sensible support in our side.
The device entry is a concept in HDA spec. For driver implementation, I think we can use an int variable or a struct device to represent it. A int variable is an easy way. There is some place in audio driver using int variable to represent the deivce entry. And audio driver will not care how gfx driver supports the DP1.2 MST, I mean audio driver will not know the structures inside gfx driver.
I will remove this parameter in this version if you are thinking the MST interface is indeterminate.
BTW: do you know when gfx will normally support MST?
Best Regards, Libin
BR, Jani.
} *ops; };
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
Hi,
-----Original Message----- From: Yang, Libin Sent: Tuesday, August 11, 2015 10:41 AM To: Jani Nikula; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
Hi Jani,
Thanks for reviewing, please see my comments
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 7:46 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*set_ncts)(struct device *, int port, int dev_entry,int rate);I'd rather call this set_audio_rate or similar, and dropping the references to N and CTS. The caller does not need to know.
But it seems the set_audio_rate will confuse the user. From the literal meaning, it is setting the rate of audio, such as sample rate, which will make audio driver developers confused. And the function is just setting N/CTS which is defined from HDMI SPEC.
What about the name sync_audio_rate()?
BTW: your comment reminds me that get_cdclk_freq() seems to need change the name as cdclk is not in the open spec.
I'm also not fond of adding a dev_entry parameter and leaving it unused. I do not think we know specifically how we're going to
identify
MST sinks, and the interface may need to be changed anyway. Let's force an update in the caller side as well when there's actually sensible support in our side.
The device entry is a concept in HDA spec. For driver implementation, I think we can use an int variable or a struct device to represent it. A int variable is an easy way. There is some place in audio driver using int variable to represent the deivce entry. And audio driver will not care how gfx driver supports the DP1.2 MST, I mean audio driver will not know the structures inside gfx driver.
I will remove this parameter in this version if you are thinking the MST interface is indeterminate.
BTW: do you know when gfx will normally support MST?
Best Regards, Libin
BR, Jani.
} *ops; };
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 12 Aug 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi,
-----Original Message----- From: Yang, Libin Sent: Tuesday, August 11, 2015 10:41 AM To: Jani Nikula; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
Hi Jani,
Thanks for reviewing, please see my comments
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 7:46 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*set_ncts)(struct device *, int port, int dev_entry,int rate);I'd rather call this set_audio_rate or similar, and dropping the references to N and CTS. The caller does not need to know.
But it seems the set_audio_rate will confuse the user. From the literal meaning, it is setting the rate of audio, such as sample rate, which will make audio driver developers confused. And the function is just setting N/CTS which is defined from HDMI SPEC.
What about the name sync_audio_rate()?
I'm fine with that.
BR, Jani.
BTW: your comment reminds me that get_cdclk_freq() seems to need change the name as cdclk is not in the open spec.
I'm also not fond of adding a dev_entry parameter and leaving it unused. I do not think we know specifically how we're going to
identify
MST sinks, and the interface may need to be changed anyway. Let's force an update in the caller side as well when there's actually sensible support in our side.
The device entry is a concept in HDA spec. For driver implementation, I think we can use an int variable or a struct device to represent it. A int variable is an easy way. There is some place in audio driver using int variable to represent the deivce entry. And audio driver will not care how gfx driver supports the DP1.2 MST, I mean audio driver will not know the structures inside gfx driver.
I will remove this parameter in this version if you are thinking the MST interface is indeterminate.
BTW: do you know when gfx will normally support MST?
Best Regards, Libin
BR, Jani.
} *ops; };
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
-----Original Message----- From: Yang, Libin Sent: Tuesday, August 11, 2015 10:41 AM To: 'Jani Nikula'; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: RE: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
Hi Jani,
Thanks for reviewing, please see my comments
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Monday, August 10, 2015 7:46 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch Subject: Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Add audio set_ncts callback
On Mon, 10 Aug 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the set_ncts callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
include/drm/i915_component.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..7305881 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,8 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*set_ncts)(struct device *, int port, int dev_entry,int rate);I'd rather call this set_audio_rate or similar, and dropping the references to N and CTS. The caller does not need to know.
But it seems the set_audio_rate will confuse the user. From the literal meaning, it is setting the rate of audio, such as sample rate, which will make audio driver developers confused. And the function is just setting N/CTS which is defined from HDMI SPEC.
After a second thought, set_ncts is not very good, we should consider DP's naming and handling DP mode together.
BTW: your comment reminds me that get_cdclk_freq() seems to need change the name as cdclk is not in the open spec.
I'm also not fond of adding a dev_entry parameter and leaving it unused. I do not think we know specifically how we're going to
identify
MST sinks, and the interface may need to be changed anyway. Let's force an update in the caller side as well when there's actually sensible support in our side.
The device entry is a concept in HDA spec. For driver implementation, I think we can use an int variable or a struct device to represent it. A int variable is an easy way. There is some place in audio driver using int variable to represent the deivce entry. And audio driver will not care how gfx driver supports the DP1.2 MST, I mean audio driver will not know the structures inside gfx driver.
I will remove this parameter in this version if you are thinking the MST interface is indeterminate.
BTW: do you know when gfx will normally support MST?
Best Regards, Libin
BR, Jani.
} *ops; };
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Jani Nikula, Intel Open Source Technology Center
participants (5)
-
Daniel Vetter -
Jani Nikula -
libin.yang＠intel.com -
Takashi Iwai -
Yang, Libin