[alsa-devel] [PATCH v6 1/4] drm/i915: Add audio sync_audio_rate callback
From: Libin Yang libin.yang@intel.com
Add the sync_audio_rate callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS or N/M based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com --- include/drm/i915_component.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..8ad6f1b 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,13 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); + /** + * @sync_audio_rate: set n/cts based on the sample rate + * + * Called from audio driver. After audio driver sets the + * sample rate, it will call this function to set n/cts + */ + int (*sync_audio_rate)(struct device *, int port, int rate); } *ops; };
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com --- drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_audio.c | 138 +++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 097d4ba..8ffb5dc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); mutex_init(&dev_priv->csr_lock); + mutex_init(&dev_priv->av_mutex);
intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 05ffd5a..2c6b76f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,11 @@ struct drm_i915_private {
/* hda/i915 audio component */ bool audio_component_registered; + /** + * av_mutex - mutex for audio/video sync + * + */ + struct mutex av_mutex;
uint32_t hw_context_size; struct list_head context_list; diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..a021720 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct { + int sample_rate; + int clock; + int n; + int cts; +} aud_ncts[] = { + { 44100, TMDS_296M, 4459, 234375 }, + { 44100, TMDS_297M, 4704, 247500 }, + { 48000, TMDS_296M, 5824, 281250 }, + { 48000, TMDS_297M, 5120, 247500 }, + { 32000, TMDS_296M, 5824, 421875 }, + { 32000, TMDS_297M, 3072, 222750 }, + { 88200, TMDS_296M, 8918, 234375 }, + { 88200, TMDS_297M, 9408, 247500 }, + { 96000, TMDS_296M, 11648, 281250 }, + { 96000, TMDS_297M, 10240, 247500 }, + { 176400, TMDS_296M, 17836, 234375 }, + { 176400, TMDS_297M, 18816, 247500 }, + { 192000, TMDS_296M, 23296, 281250 }, + { 192000, TMDS_297M, 20480, 247500 }, +}; + /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -90,6 +115,31 @@ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(const struct drm_display_mode *mode, int rate) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { + if ((rate == aud_ncts[i].sample_rate) && + (mode->clock == aud_ncts[i].clock)) { + return aud_ncts[i].n; + } + } + return 0; +} + +/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc, + struct drm_display_mode *mode) +{ + if (((mode->clock == TMDS_297M) || + (mode->clock == TMDS_296M)) && + intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) + return true; + else + return false; +} + static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder)
DRM_DEBUG_KMS("Disable audio codec on pipe %c\n", pipe_name(pipe));
+ mutex_lock(&dev_priv->av_mutex); + /* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) tmp &= ~AUDIO_ELD_VALID(pipe); tmp &= ~AUDIO_OUTPUT_ENABLE(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); + + mutex_unlock(&dev_priv->av_mutex); }
static void hsw_audio_codec_enable(struct drm_connector *connector, @@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
+ mutex_lock(&dev_priv->av_mutex); + /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe); @@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, else tmp |= audio_config_hdmi_pixel_clock(mode); I915_WRITE(HSW_AUD_CFG(pipe), tmp); + + mutex_unlock(&dev_priv->av_mutex); }
static void ilk_audio_codec_disable(struct intel_encoder *encoder) @@ -514,12 +572,92 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_sync_audio_rate(struct device *dev, + int port, int rate) +{ + struct drm_i915_private *dev_priv = dev_to_i915(dev); + struct drm_device *drm_dev = dev_priv->dev; + struct intel_encoder *intel_encoder; + struct intel_digital_port *intel_dig_port; + struct intel_crtc *crtc; + struct drm_display_mode *mode; + enum pipe pipe = -1; + u32 tmp; + int n_low, n_up, n; + + /* HSW, BDW SKL need this fix */ + if (!IS_SKYLAKE(dev_priv) && + !IS_BROADWELL(dev_priv) && + !IS_HASWELL(dev_priv)) + return 0; + + mutex_lock(&dev_priv->av_mutex); + /* 1. get the pipe */ + for_each_intel_encoder(drm_dev, intel_encoder) { + if (intel_encoder->type != INTEL_OUTPUT_HDMI) + continue; + 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 == INVALID_PIPE) { + DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); + mutex_unlock(&dev_priv->av_mutex); + return -ENODEV; + } + DRM_DEBUG_KMS("pipe %c connects port %c\n", + pipe_name(pipe), port_name(port)); + mode = &crtc->config->base.adjusted_mode; + + /* 2. check whether to set the N/CTS/M manually or not */ + if (!audio_rate_need_prog(crtc, mode)) { + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + mutex_unlock(&dev_priv->av_mutex); + return 0; + } + + n = audio_config_get_n(mode, rate); + if (n == 0) { + DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n", + port_name(port)); + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + mutex_unlock(&dev_priv->av_mutex); + return 0; + } + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + + /* 4. set the N/CTS/M */ + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK); + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | + (n_low << AUD_CONFIG_LOWER_N_SHIFT) | + AUD_CONFIG_N_PROG_ENABLE); + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + + mutex_unlock(&dev_priv->av_mutex); + return 0; +} + static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq, + .sync_audio_rate = i915_audio_component_sync_audio_rate, };
static int i915_audio_component_bind(struct device *i915_dev,
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
Okay, so there are a number of questions and comments this patch still raises. Please find them inline.
*However*, we should really get this finally moving forward, and I don't think the issues are or need to be blockers, so this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
I think we (as in i915 folks) can clean the rest of the bikeshedding up, and I don't think we should be NAKing this ad infinitum to teach you to become an i915 developer... unless you want to be one. ;)
drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_audio.c | 138 +++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 097d4ba..8ffb5dc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); mutex_init(&dev_priv->csr_lock);
mutex_init(&dev_priv->av_mutex);
intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 05ffd5a..2c6b76f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,11 @@ struct drm_i915_private {
/* hda/i915 audio component */ bool audio_component_registered;
/**
* av_mutex - mutex for audio/video sync**/struct mutex av_mutex;
uint32_t hw_context_size; struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..a021720 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 192000, TMDS_296M, 23296, 281250 },
- { 192000, TMDS_297M, 20480, 247500 },
+};
Okay, I admit, I did not look these up in the spec. *blush*. I'm sure Ville has/will. ;)
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -90,6 +115,31 @@ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(const struct drm_display_mode *mode, int rate) +{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode *mode)+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder)
DRM_DEBUG_KMS("Disable audio codec on pipe %c\n", pipe_name(pipe));
- mutex_lock(&dev_priv->av_mutex);
Bikeshed. We should think about moving the locking to intel_audio_codec_enable and intel_audio_codec_disable for consistency. We'll be adding new platform hooks eventually anyway, no need to duplicate the locking everywhere.
/* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder) tmp &= ~AUDIO_ELD_VALID(pipe); tmp &= ~AUDIO_OUTPUT_ENABLE(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void hsw_audio_codec_enable(struct drm_connector *connector, @@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
- mutex_lock(&dev_priv->av_mutex);
- /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe);
@@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, else tmp |= audio_config_hdmi_pixel_clock(mode); I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void ilk_audio_codec_disable(struct intel_encoder *encoder) @@ -514,12 +572,92 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_sync_audio_rate(struct device *dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
Nitpick. Initialize to INVALID_PIPE.
- u32 tmp;
- int n_low, n_up, n;
- /* HSW, BDW SKL need this fix */
- if (!IS_SKYLAKE(dev_priv) &&
!IS_BROADWELL(dev_priv) &&!IS_HASWELL(dev_priv))return 0;
Nitpick. We should elaborate that a bit more. Is it basically that only the above platforms support HDMI mode and audio sample rate combinations that the automatic mode in hardware can't support? In other words, would audio_rate_need_prog() always return false on other platforms than hsw/bdw/skl? And if so, why do we have this check? Perhaps we should take care to make audio_rate_need_prog() do the right thing, and do that as the first thing here.
- mutex_lock(&dev_priv->av_mutex);
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI)continue;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__);
DRM_DEBUG_KMS includes __func__ already.
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port));mutex_unlock(&dev_priv->av_mutex);return -ENODEV;
Nitpick. I am not fond of early returns when cleanup like mutex unlock is required. The function should have goto labels at the end with cleanup. (OTOH if we move locking to higher level this problem goes away.)
- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;
Nitpick. Same here, you could have an auto: label at the end with the auto programming...
- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;
...especially because this bit is duplicated for no good reason.
Fallback to auto mode if the mode isn't in the list is, I think, a good thing. Maybe it would deserve a DRM_ERROR, because clearly that shouldn't happen.
But it's actually the !audio_rate_need_prog case that makes me wonder. I think we've confirmed with the silicon folks that we can change N on the fly... but I'm actually not sure if we've confirmed that we can switch between automatic and manual modes on the fly. I think I've said it before, but I think I'd prefer it if we always used the manual mode anyway to reduce the complexity and have only one code path to debug, and that would also cover the somewhat of a corner case (but still real case) of switching between automatic/manual modes on the fly. And of course, this conflicts with what I said about the platform support check above, so needs thought.
- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
- return 0;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 3:52 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 2/4] drm/i915: implement sync_audio_rate callback
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
Okay, so there are a number of questions and comments this patch still raises. Please find them inline.
*However*, we should really get this finally moving forward, and I don't think the issues are or need to be blockers, so this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
I think we (as in i915 folks) can clean the rest of the bikeshedding up, and I don't think we should be NAKing this ad infinitum to teach you to become an i915 developer... unless you want to be one. ;)
drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_audio.c | 138
+++++++++++++++++++++++++++++++++++++
3 files changed, 144 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 097d4ba..8ffb5dc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev,
unsigned long flags)
mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); mutex_init(&dev_priv->csr_lock);
mutex_init(&dev_priv->av_mutex);
intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 05ffd5a..2c6b76f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,11 @@ struct drm_i915_private {
/* hda/i915 audio component */ bool audio_component_registered;
/**
* av_mutex - mutex for audio/video sync**/struct mutex av_mutex;
uint32_t hw_context_size; struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..a021720 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 192000, TMDS_296M, 23296, 281250 },
- { 192000, TMDS_297M, 20480, 247500 },
+};
Okay, I admit, I did not look these up in the spec. *blush*. I'm sure Ville has/will. ;)
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(const struct drm_display_mode
*mode, int rate)
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct
intel_encoder *encoder)
DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
pipe_name(pipe));
- mutex_lock(&dev_priv->av_mutex);
Bikeshed. We should think about moving the locking to intel_audio_codec_enable and intel_audio_codec_disable for consistency. We'll be adding new platform hooks eventually anyway, no need to duplicate the locking everywhere.
Yes, at first, I want to add the mutex in intel_audio_codec_enable, but after a second thought, we don't need it for ilk_xxx and g4x_xxx, so I moved the lock in the hsw specific function for efficiency.
I will move it to intel_audio_codec_enable in next version.
/* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct
intel_encoder *encoder)
tmp &= ~AUDIO_ELD_VALID(pipe); tmp &= ~AUDIO_OUTPUT_ENABLE(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void hsw_audio_codec_enable(struct drm_connector
*connector,
@@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));
- mutex_lock(&dev_priv->av_mutex);
- /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe);
@@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
else tmp |= audio_config_hdmi_pixel_clock(mode); I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void ilk_audio_codec_disable(struct intel_encoder *encoder) @@ -514,12 +572,92 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
Nitpick. Initialize to INVALID_PIPE.
Oh, yes, I will change it.
- u32 tmp;
- int n_low, n_up, n;
- /* HSW, BDW SKL need this fix */
- if (!IS_SKYLAKE(dev_priv) &&
!IS_BROADWELL(dev_priv) &&!IS_HASWELL(dev_priv))return 0;Nitpick. We should elaborate that a bit more. Is it basically that only the above platforms support HDMI mode and audio sample rate combinations that the automatic mode in hardware can't support? In other words, would audio_rate_need_prog() always return false on other platforms than hsw/bdw/skl? And if so, why do we have this check? Perhaps we should take care to make audio_rate_need_prog() do the right thing, and do that as the first thing here.
I add this judgement because the platforms are filed in our internal document. Besides, the next gen may have such issue and may not (the doc said MAY have such issue). For easily extension, I add such judgement.
audio_rate_need_prog() means the specified clock need such setting. But I'm not sure whether other old platforms support such clock. If yes, the audio_rate_need_prog() may be not enough. Although I think doing the setting should be OK even on the platforms not listed in the doc. But I have no platform to do the test, so I didn't include other platform this time.
- mutex_lock(&dev_priv->av_mutex);
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI)continue;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 isNULL\n", __func__);
DRM_DEBUG_KMS includes __func__ already.
I will change it in next version. Thanks.
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
mutex_unlock(&dev_priv->av_mutex);return -ENODEV;Nitpick. I am not fond of early returns when cleanup like mutex unlock is required. The function should have goto labels at the end with cleanup. (OTOH if we move locking to higher level this problem goes away.)
OK, so it seems to move the lock in upper function is an easy way. I will move the lock in the upper function.
- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;Nitpick. Same here, you could have an auto: label at the end with the auto programming...
- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N valueon port %c\n",
port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;...especially because this bit is duplicated for no good reason.
Fallback to auto mode if the mode isn't in the list is, I think, a good thing. Maybe it would deserve a DRM_ERROR, because clearly that shouldn't happen.
OK, I will add auto label for it. And I will use DRM_ERROR in next version.
But it's actually the !audio_rate_need_prog case that makes me wonder. I think we've confirmed with the silicon folks that we can change N on the fly... but I'm actually not sure if we've confirmed that we can switch between automatic and manual modes on the fly. I think I've said it before, but I think I'd prefer it if we always used the manual mode anyway to reduce the complexity and have only one code path to debug, and that would also cover the somewhat of a corner case (but still real case) of switching between automatic/manual modes on the fly. And of course, this conflicts with what I said about the platform support check above, so needs thought.
I will check with silicon team. Based on our stress test, it seems to be OK change mode on the fly.
We don't set for all the frequencies because there are so many frequencies and platforms to do the stress test. We may need a lot time to do the test. If the silicon team says we can't change mode on the fly, I will change it in next version. If silicon team says we can do it, I will use the HW auto mode for other frequencies. What do you think?
Regards, Libin
- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 02 Sep 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 3:52 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 2/4] drm/i915: implement sync_audio_rate callback
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
Okay, so there are a number of questions and comments this patch still raises. Please find them inline.
*However*, we should really get this finally moving forward, and I don't think the issues are or need to be blockers, so this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
I think we (as in i915 folks) can clean the rest of the bikeshedding up, and I don't think we should be NAKing this ad infinitum to teach you to become an i915 developer... unless you want to be one. ;)
Libin, I suggest taking that R-b now instead of writing a new version of this patch if you want to get things upstreamed!
Please focus on patch 4.
BR, Jani.
drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_audio.c | 138
+++++++++++++++++++++++++++++++++++++
3 files changed, 144 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 097d4ba..8ffb5dc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device *dev,
unsigned long flags)
mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); mutex_init(&dev_priv->csr_lock);
mutex_init(&dev_priv->av_mutex);
intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 05ffd5a..2c6b76f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,11 @@ struct drm_i915_private {
/* hda/i915 audio component */ bool audio_component_registered;
/**
* av_mutex - mutex for audio/video sync**/struct mutex av_mutex;
uint32_t hw_context_size; struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..a021720 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 192000, TMDS_296M, 23296, 281250 },
- { 192000, TMDS_297M, 20480, 247500 },
+};
Okay, I admit, I did not look these up in the spec. *blush*. I'm sure Ville has/will. ;)
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(const struct drm_display_mode
*mode, int rate)
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -184,6 +234,8 @@ static void hsw_audio_codec_disable(struct
intel_encoder *encoder)
DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
pipe_name(pipe));
- mutex_lock(&dev_priv->av_mutex);
Bikeshed. We should think about moving the locking to intel_audio_codec_enable and intel_audio_codec_disable for consistency. We'll be adding new platform hooks eventually anyway, no need to duplicate the locking everywhere.
Yes, at first, I want to add the mutex in intel_audio_codec_enable, but after a second thought, we don't need it for ilk_xxx and g4x_xxx, so I moved the lock in the hsw specific function for efficiency.
I will move it to intel_audio_codec_enable in next version.
/* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -199,6 +251,8 @@ static void hsw_audio_codec_disable(struct
intel_encoder *encoder)
tmp &= ~AUDIO_ELD_VALID(pipe); tmp &= ~AUDIO_OUTPUT_ENABLE(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void hsw_audio_codec_enable(struct drm_connector
*connector,
@@ -215,6 +269,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));
- mutex_lock(&dev_priv->av_mutex);
- /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe);
@@ -253,6 +309,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
else tmp |= audio_config_hdmi_pixel_clock(mode); I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void ilk_audio_codec_disable(struct intel_encoder *encoder) @@ -514,12 +572,92 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
Nitpick. Initialize to INVALID_PIPE.
Oh, yes, I will change it.
- u32 tmp;
- int n_low, n_up, n;
- /* HSW, BDW SKL need this fix */
- if (!IS_SKYLAKE(dev_priv) &&
!IS_BROADWELL(dev_priv) &&!IS_HASWELL(dev_priv))return 0;Nitpick. We should elaborate that a bit more. Is it basically that only the above platforms support HDMI mode and audio sample rate combinations that the automatic mode in hardware can't support? In other words, would audio_rate_need_prog() always return false on other platforms than hsw/bdw/skl? And if so, why do we have this check? Perhaps we should take care to make audio_rate_need_prog() do the right thing, and do that as the first thing here.
I add this judgement because the platforms are filed in our internal document. Besides, the next gen may have such issue and may not (the doc said MAY have such issue). For easily extension, I add such judgement.
audio_rate_need_prog() means the specified clock need such setting. But I'm not sure whether other old platforms support such clock. If yes, the audio_rate_need_prog() may be not enough. Although I think doing the setting should be OK even on the platforms not listed in the doc. But I have no platform to do the test, so I didn't include other platform this time.
- mutex_lock(&dev_priv->av_mutex);
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI)continue;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 isNULL\n", __func__);
DRM_DEBUG_KMS includes __func__ already.
I will change it in next version. Thanks.
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
mutex_unlock(&dev_priv->av_mutex);return -ENODEV;Nitpick. I am not fond of early returns when cleanup like mutex unlock is required. The function should have goto labels at the end with cleanup. (OTOH if we move locking to higher level this problem goes away.)
OK, so it seems to move the lock in upper function is an easy way. I will move the lock in the upper function.
- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;Nitpick. Same here, you could have an auto: label at the end with the auto programming...
- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N valueon port %c\n",
port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;...especially because this bit is duplicated for no good reason.
Fallback to auto mode if the mode isn't in the list is, I think, a good thing. Maybe it would deserve a DRM_ERROR, because clearly that shouldn't happen.
OK, I will add auto label for it. And I will use DRM_ERROR in next version.
But it's actually the !audio_rate_need_prog case that makes me wonder. I think we've confirmed with the silicon folks that we can change N on the fly... but I'm actually not sure if we've confirmed that we can switch between automatic and manual modes on the fly. I think I've said it before, but I think I'd prefer it if we always used the manual mode anyway to reduce the complexity and have only one code path to debug, and that would also cover the somewhat of a corner case (but still real case) of switching between automatic/manual modes on the fly. And of course, this conflicts with what I said about the platform support check above, so needs thought.
I will check with silicon team. Based on our stress test, it seems to be OK change mode on the fly.
We don't set for all the frequencies because there are so many frequencies and platforms to do the stress test. We may need a lot time to do the test. If the silicon team says we can't change mode on the fly, I will change it in next version. If silicon team says we can do it, I will use the HW auto mode for other frequencies. What do you think?
Regards, Libin
- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
-- Jani Nikula, Intel Open Source Technology Center
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 4:42 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Subject: RE: [PATCH v6 2/4] drm/i915: implement sync_audio_rate callback
On Wed, 02 Sep 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 3:52 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 2/4] drm/i915: implement sync_audio_rate callback
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com
Okay, so there are a number of questions and comments this patch
still
raises. Please find them inline.
*However*, we should really get this finally moving forward, and I don't think the issues are or need to be blockers, so this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
I think we (as in i915 folks) can clean the rest of the bikeshedding up, and I don't think we should be NAKing this ad infinitum to teach you
to
become an i915 developer... unless you want to be one. ;)
Libin, I suggest taking that R-b now instead of writing a new version of this patch if you want to get things upstreamed!
Please focus on patch 4.
Oh, I didn't realize you mean this. Thanks. :)
Regards, Libin
BR, Jani.
drivers/gpu/drm/i915/i915_dma.c | 1 + drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_audio.c | 138
+++++++++++++++++++++++++++++++++++++
3 files changed, 144 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 097d4ba..8ffb5dc 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -858,6 +858,7 @@ int i915_driver_load(struct drm_device
*dev,
unsigned long flags)
mutex_init(&dev_priv->sb_lock); mutex_init(&dev_priv->modeset_restore_lock); mutex_init(&dev_priv->csr_lock);
mutex_init(&dev_priv->av_mutex);
intel_pm_setup(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index 05ffd5a..2c6b76f 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1882,6 +1882,11 @@ struct drm_i915_private {
/* hda/i915 audio component */ bool audio_component_registered;
/**
* av_mutex - mutex for audio/video sync**/struct mutex av_mutex;
uint32_t hw_context_size; struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..a021720 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct {
- int sample_rate;
- int clock;
- int n;
- int cts;
+} aud_ncts[] = {
- { 44100, TMDS_296M, 4459, 234375 },
- { 44100, TMDS_297M, 4704, 247500 },
- { 48000, TMDS_296M, 5824, 281250 },
- { 48000, TMDS_297M, 5120, 247500 },
- { 32000, TMDS_296M, 5824, 421875 },
- { 32000, TMDS_297M, 3072, 222750 },
- { 88200, TMDS_296M, 8918, 234375 },
- { 88200, TMDS_297M, 9408, 247500 },
- { 96000, TMDS_296M, 11648, 281250 },
- { 96000, TMDS_297M, 10240, 247500 },
- { 176400, TMDS_296M, 17836, 234375 },
- { 176400, TMDS_297M, 18816, 247500 },
- { 192000, TMDS_296M, 23296, 281250 },
- { 192000, TMDS_297M, 20480, 247500 },
+};
Okay, I admit, I did not look these up in the spec. *blush*. I'm sure Ville has/will. ;)
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(const struct drm_display_mode
*mode, int rate)
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -184,6 +234,8 @@ static void
hsw_audio_codec_disable(struct
intel_encoder *encoder)
DRM_DEBUG_KMS("Disable audio codec on pipe %c\n",
pipe_name(pipe));
- mutex_lock(&dev_priv->av_mutex);
Bikeshed. We should think about moving the locking to intel_audio_codec_enable and intel_audio_codec_disable for consistency. We'll be adding new platform hooks eventually anyway, no need to duplicate the locking everywhere.
Yes, at first, I want to add the mutex in intel_audio_codec_enable, but after a second thought, we don't need it for ilk_xxx and g4x_xxx, so I moved the lock in the hsw specific function for efficiency.
I will move it to intel_audio_codec_enable in next version.
/* Disable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; @@ -199,6 +251,8 @@ static void
hsw_audio_codec_disable(struct
intel_encoder *encoder)
tmp &= ~AUDIO_ELD_VALID(pipe); tmp &= ~AUDIO_OUTPUT_ENABLE(pipe); I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void hsw_audio_codec_enable(struct drm_connector
*connector,
@@ -215,6 +269,8 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));
- mutex_lock(&dev_priv->av_mutex);
- /* Enable audio presence detect, invalidate ELD */ tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD); tmp |= AUDIO_OUTPUT_ENABLE(pipe);
@@ -253,6 +309,8 @@ static void
hsw_audio_codec_enable(struct
drm_connector *connector,
else tmp |= audio_config_hdmi_pixel_clock(mode); I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
}
static void ilk_audio_codec_disable(struct intel_encoder
*encoder)
@@ -514,12 +572,92 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
Nitpick. Initialize to INVALID_PIPE.
Oh, yes, I will change it.
- u32 tmp;
- int n_low, n_up, n;
- /* HSW, BDW SKL need this fix */
- if (!IS_SKYLAKE(dev_priv) &&
!IS_BROADWELL(dev_priv) &&!IS_HASWELL(dev_priv))return 0;Nitpick. We should elaborate that a bit more. Is it basically that only the above platforms support HDMI mode and audio sample rate combinations that the automatic mode in hardware can't support? In other words, would audio_rate_need_prog() always return false on other platforms
than
hsw/bdw/skl? And if so, why do we have this check? Perhaps we should take care to make audio_rate_need_prog() do the right thing, and
do
that as the first thing here.
I add this judgement because the platforms are filed in our internal document. Besides, the next gen may have such issue and may not (the doc said MAY have such issue). For easily extension, I add such judgement.
audio_rate_need_prog() means the specified clock need such setting. But I'm not sure whether other old platforms support such clock. If yes, the audio_rate_need_prog() may be not enough. Although I think doing the setting should be OK even on the platforms not listed in the doc. But I have no platform to do the test, so I didn't include other platform this time.
- mutex_lock(&dev_priv->av_mutex);
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI)continue;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 isNULL\n", __func__);
DRM_DEBUG_KMS includes __func__ already.
I will change it in next version. Thanks.
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
mutex_unlock(&dev_priv->av_mutex);return -ENODEV;Nitpick. I am not fond of early returns when cleanup like mutex
unlock
is required. The function should have goto labels at the end with cleanup. (OTOH if we move locking to higher level this problem
goes
away.)
OK, so it seems to move the lock in upper function is an easy way. I will move the lock in the upper function.
- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;Nitpick. Same here, you could have an auto: label at the end with
the
auto programming...
- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N valueon port %c\n",
port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);mutex_unlock(&dev_priv->av_mutex);return 0;...especially because this bit is duplicated for no good reason.
Fallback to auto mode if the mode isn't in the list is, I think, a good thing. Maybe it would deserve a DRM_ERROR, because clearly that shouldn't happen.
OK, I will add auto label for it. And I will use DRM_ERROR in next version.
But it's actually the !audio_rate_need_prog case that makes me wonder. I think we've confirmed with the silicon folks that we can change N
on
the fly... but I'm actually not sure if we've confirmed that we can switch between automatic and manual modes on the fly. I think I've said it before, but I think I'd prefer it if we always used the manual mode anyway to reduce the complexity and have only one code path to debug, and that would also cover the somewhat of a corner case (but still
real
case) of switching between automatic/manual modes on the fly.
And
of course, this conflicts with what I said about the platform support
check
above, so needs thought.
I will check with silicon team. Based on our stress test, it seems to be OK change mode on the fly.
We don't set for all the frequencies because there are so many frequencies and platforms to do the stress test. We may need a lot time to do the test. If the silicon team says we can't change mode on the fly, I will change it in next version. If silicon team says we can do it, I will use the HW auto mode for other frequencies. What do you think?
Regards, Libin
- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- mutex_unlock(&dev_priv->av_mutex);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
-- Jani Nikula, Intel Open Source Technology Center
-- Jani Nikula, Intel Open Source Technology Center
From: Libin Yang libin.yang@intel.com
For display audio, call the sync_audio_rate callback function to do the synchronization between gfx driver and audio driver.
Signed-off-by: Libin Yang libin.yang@intel.com Reviewed-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..1668868 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1770,6 +1770,16 @@ static bool check_non_pcm_per_cvt(struct hda_codec *codec, hda_nid_t cvt_nid) return non_pcm; }
+/* There is a fixed mapping between audio pin node and display port + * on current Intel platforms: + * Pin Widget 5 - PORT B (port = 1 in i915 driver) + * Pin Widget 6 - PORT C (port = 2 in i915 driver) + * Pin Widget 7 - PORT D (port = 3 in i915 driver) + */ +static int intel_pin2port(hda_nid_t pin_nid) +{ + return pin_nid - 4; +}
/* * HDMI callbacks @@ -1786,6 +1796,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, int pin_idx = hinfo_to_pin_index(codec, hinfo); struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); hda_nid_t pin_nid = per_pin->pin_nid; + struct snd_pcm_runtime *runtime = substream->runtime; + struct i915_audio_component *acomp = codec->bus->core.audio_component; bool non_pcm; int pinctl;
@@ -1802,6 +1814,13 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo, intel_not_share_assigned_cvt(codec, pin_nid, per_pin->mux_idx); }
+ /* Call sync_audio_rate to set the N/CTS/M manually if necessary */ + /* Todo: add DP1.2 MST audio support later */ + if (acomp && acomp->ops && acomp->ops->sync_audio_rate) + acomp->ops->sync_audio_rate(acomp->dev, + intel_pin2port(pin_nid), + runtime->rate); + non_pcm = check_non_pcm_per_cvt(codec, cvt_nid); mutex_lock(&per_pin->lock); per_pin->channels = substream->runtime->channels;
From: Libin Yang libin.yang@intel.com
When modeset occurs and the TMDS frequency is set to some speical values, the N/CTS need to be set manually if audio is playing.
Signed-off-by: Libin Yang libin.yang@intel.com --- drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ae7fa3e..5bdee36 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7058,6 +7058,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, \ @@ -7072,6 +7074,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 a021720..acdb68c 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc, return false; }
+static int audio_config_get_rate(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + uint32_t tmp; + int cvt_idx; + int base_rate, mul, div, rate; + + tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL); + cvt_idx = (tmp >> (pipe * 8)) & 0xff; + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); + base_rate = tmp & (1 << 14); + if (base_rate == 0) + rate = 48000; + else + rate = 44100; + mul = (tmp & (0x7 << 11)) + 1; + div = (tmp & (0x7 << 8)) + 1; + rate = rate * mul / div; + return rate; +} + static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i; + int n_low, n_up, n; + int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld)); @@ -302,12 +325,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; - tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode); + + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + if (audio_rate_need_prog(intel_crtc, mode)) { + rate = audio_config_get_rate(dev_priv, pipe); + n = audio_config_get_n(mode, rate); + if (n != 0) { + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | + AUD_CONFIG_LOWER_N_MASK); + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | + (n_low << AUD_CONFIG_LOWER_N_SHIFT) | + AUD_CONFIG_N_PROG_ENABLE); + } + } + I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
On Wed, 02 Sep 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 values, the N/CTS need to be set manually if audio is playing.
Do we still need this patch after David Henningsson's series [1]? IIUC you will now always get the callback on modesets, so you should be able to, uh, callback on the callback to set audio rate. Granted, with this I suppose you reduce the time there might be wrong N/CTS after enable.
Some other comments inline.
[1] http://mid.gmane.org/1440781350-12053-1-git-send-email-david.henningsson@can...
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index ae7fa3e..5bdee36 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
Nitpick. The bit fields are not defined.
#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, \ @@ -7072,6 +7074,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)
Nitpick. The bit fields are not defined. I think it's fine to use _PIPE macro, but you should probably use "converter" or "cvt" or something for the parameter name to not mislead people into thinking this is pipe based.
#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 a021720..acdb68c 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc, return false; }
+static int audio_config_get_rate(struct drm_i915_private *dev_priv,
enum pipe pipe)+{
- uint32_t tmp;
- int cvt_idx;
- int base_rate, mul, div, rate;
- tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
- cvt_idx = (tmp >> (pipe * 8)) & 0xff;
This one needs to be addressed. Are you sure it's indexed by pipe? The spec is vague.
Bits 7:0 are "control B", 15:8 are "control C", and so on, while your (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C, etc. This would speak for port, not pipe, as pipe A should be valid while port A not.
- tmp = I915_READ(AUD_STR_DESC(cvt_idx));
- base_rate = tmp & (1 << 14);
Nitpick. We prefer (tmp & MASK) >> SHIFT.
- if (base_rate == 0)
rate = 48000;- else
rate = 44100;- mul = (tmp & (0x7 << 11)) + 1;
- div = (tmp & (0x7 << 8)) + 1;
This one needs to be addressed. This is bogus. The +1 would work if you actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted value. Your multipliers and divisors are *way* off.
NAK on this patch.
- rate = rate * mul / div;
- return rate;
+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
@@ -302,12 +325,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_need_prog(intel_crtc, mode)) {
rate = audio_config_get_rate(dev_priv, pipe);n = audio_config_get_n(mode, rate);if (n != 0) {n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp &= ~(AUD_CONFIG_UPPER_N_MASK |AUD_CONFIG_LOWER_N_MASK);tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
}}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
-- 1.9.1
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 4:20 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 02 Sep 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 values, the N/CTS need to be set manually if audio is playing.
Do we still need this patch after David Henningsson's series [1]? IIUC you will now always get the callback on modesets, so you should be able to, uh, callback on the callback to set audio rate. Granted, with this I suppose you reduce the time there might be wrong N/CTS after enable.
Yes, we need. David's patch will not trigger the sync_audio_rate callback.
Some other comments inline.
[1] http://mid.gmane.org/1440781350-12053-1-git-send-email- david.henningsson@canonical.com
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40
+++++++++++++++++++++++++++++++++++++-
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ae7fa3e..5bdee36 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
Nitpick. The bit fields are not defined.
OK, I will add the bit definition.
#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, \ @@ -7072,6 +7074,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)Nitpick. The bit fields are not defined. I think it's fine to use _PIPE macro, but you should probably use "converter" or "cvt" or something for the parameter name to not mislead people into thinking this is pipe based.
Do you mean it should be like: _PIPE(cvt, _HSW_AUD_STR_DESC_1, ...) ?
#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 a021720..acdb68c 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct
intel_crtc *crtc,
return false;}
+static int audio_config_get_rate(struct drm_i915_private *dev_priv,
enum pipe pipe)+{
- uint32_t tmp;
- int cvt_idx;
- int base_rate, mul, div, rate;
- tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
- cvt_idx = (tmp >> (pipe * 8)) & 0xff;
This one needs to be addressed. Are you sure it's indexed by pipe? The spec is vague.
Yes, I did lot of test to confirm it.
Bits 7:0 are "control B", 15:8 are "control C", and so on, while your (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C, etc. This would speak for port, not pipe, as pipe A should be valid while port A not.
We found there is something wrong/or vague in the spec.
- tmp = I915_READ(AUD_STR_DESC(cvt_idx));
- base_rate = tmp & (1 << 14);
Nitpick. We prefer (tmp & MASK) >> SHIFT.
OK, I will change it.
- if (base_rate == 0)
rate = 48000;- else
rate = 44100;- mul = (tmp & (0x7 << 11)) + 1;
- div = (tmp & (0x7 << 8)) + 1;
This one needs to be addressed. This is bogus. The +1 would work if you actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted value. Your multipliers and divisors are *way* off.
OK, I will check it and change the patch.
NAK on this patch.
- rate = rate * mul / div;
- return rate;
+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -302,12 +325,27 @@ static void
hsw_audio_codec_enable(struct drm_connector *connector,
/* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc,
INTEL_OUTPUT_DISPLAYPORT))
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_need_prog(intel_crtc, mode)) {
rate = audio_config_get_rate(dev_priv, pipe);n = audio_config_get_n(mode, rate);if (n != 0) {n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp &= ~(AUD_CONFIG_UPPER_N_MASK |AUD_CONFIG_LOWER_N_MASK);
tmp |= ((n_up <<AUD_CONFIG_UPPER_N_SHIFT) |
(n_low <<AUD_CONFIG_LOWER_N_SHIFT) |
AUD_CONFIG_N_PROG_ENABLE);
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Regards, Libin
}}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
-- 1.9.1
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 02 Sep 2015, "Yang, Libin" libin.yang@intel.com wrote:
Hi Jani,
-----Original Message----- From: Jani Nikula [mailto:jani.nikula@linux.intel.com] Sent: Wednesday, September 02, 2015 4:20 PM To: Yang, Libin; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Cc: Yang, Libin Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 02 Sep 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 values, the N/CTS need to be set manually if audio is playing.
Do we still need this patch after David Henningsson's series [1]? IIUC you will now always get the callback on modesets, so you should be able to, uh, callback on the callback to set audio rate. Granted, with this I suppose you reduce the time there might be wrong N/CTS after enable.
Yes, we need. David's patch will not trigger the sync_audio_rate callback.
Okay.
Some other comments inline.
[1] http://mid.gmane.org/1440781350-12053-1-git-send-email- david.henningsson@canonical.com
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40
+++++++++++++++++++++++++++++++++++++-
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index ae7fa3e..5bdee36 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7058,6 +7058,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
Nitpick. The bit fields are not defined.
OK, I will add the bit definition.
#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, \ @@ -7072,6 +7074,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)Nitpick. The bit fields are not defined. I think it's fine to use _PIPE macro, but you should probably use "converter" or "cvt" or something for the parameter name to not mislead people into thinking this is pipe based.
Do you mean it should be like: _PIPE(cvt, _HSW_AUD_STR_DESC_1, ...) ?
Yes.
#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 a021720..acdb68c 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct
intel_crtc *crtc,
return false;}
+static int audio_config_get_rate(struct drm_i915_private *dev_priv,
enum pipe pipe)+{
- uint32_t tmp;
- int cvt_idx;
- int base_rate, mul, div, rate;
- tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL);
- cvt_idx = (tmp >> (pipe * 8)) & 0xff;
This one needs to be addressed. Are you sure it's indexed by pipe? The spec is vague.
Yes, I did lot of test to confirm it.
Bits 7:0 are "control B", 15:8 are "control C", and so on, while your (tmp >> (pipe * 8)) maps pipe A to control B, pipe B to control C, etc. This would speak for port, not pipe, as pipe A should be valid while port A not.
We found there is something wrong/or vague in the spec.
Indeed.
- tmp = I915_READ(AUD_STR_DESC(cvt_idx));
- base_rate = tmp & (1 << 14);
Nitpick. We prefer (tmp & MASK) >> SHIFT.
OK, I will change it.
- if (base_rate == 0)
rate = 48000;- else
rate = 44100;- mul = (tmp & (0x7 << 11)) + 1;
- div = (tmp & (0x7 << 8)) + 1;
This one needs to be addressed. This is bogus. The +1 would work if you actually did (tmp & MASK) >> SHIFT, but now you add it to the shifted value. Your multipliers and divisors are *way* off.
OK, I will check it and change the patch.
NAK on this patch.
- rate = rate * mul / div;
- return rate;
+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -265,6 +286,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -302,12 +325,27 @@ static void
hsw_audio_codec_enable(struct drm_connector *connector,
/* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc,
INTEL_OUTPUT_DISPLAYPORT))
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_need_prog(intel_crtc, mode)) {
rate = audio_config_get_rate(dev_priv, pipe);n = audio_config_get_n(mode, rate);if (n != 0) {n_low = n & 0xfff;n_up = (n >> 12) & 0xff;tmp &= ~(AUD_CONFIG_UPPER_N_MASK |AUD_CONFIG_LOWER_N_MASK);
tmp |= ((n_up <<AUD_CONFIG_UPPER_N_SHIFT) |
(n_low <<AUD_CONFIG_LOWER_N_SHIFT) |
AUD_CONFIG_N_PROG_ENABLE);
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
BR, Jani.
Regards, Libin
}}
I915_WRITE(HSW_AUD_CFG(pipe), tmp);
mutex_unlock(&dev_priv->av_mutex);
-- 1.9.1
-- Jani Nikula, Intel Open Source Technology Center
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
thanks,
Takashi
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a fixed version of patch 4 could follow. Just a thought.
BR, Jani.
thanks,
Takashi
On Wed, 02 Sep 2015 15:44:34 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a fixed version of patch 4 could follow. Just a thought.
That's good, then I can take the first three patches.
Daniel, would you like to review these three before I merge them? I assumed your previous mail as a kind of ack to this series, too.
thanks,
Takashi
On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 15:44:34 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a fixed version of patch 4 could follow. Just a thought.
That's good, then I can take the first three patches.
Daniel, would you like to review these three before I merge them? I assumed your previous mail as a kind of ack to this series, too.
fwiw ack on that plan, but given how much I made a mess out of these two series and mixed them up I wouldn't trust me anyway ;-)
Once the code settles I'll rebase/backmerge so that I can apply the follow-up documentation/kerneldoc work that's still getting done.
Cheers, Daniel
On Wed, 02 Sep 2015 17:22:01 +0200, Daniel Vetter wrote:
On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 15:44:34 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
> Nitpick. I'd prefer some sharing with the similar blocks from the > earlier patch. Also a debug message on n == 0 would be nice; you > probably didn't notice your audio_config_get_rate() wasn't working > right > because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a fixed version of patch 4 could follow. Just a thought.
That's good, then I can take the first three patches.
Daniel, would you like to review these three before I merge them? I assumed your previous mail as a kind of ack to this series, too.
fwiw ack on that plan, but given how much I made a mess out of these two series and mixed them up I wouldn't trust me anyway ;-)
Once the code settles I'll rebase/backmerge so that I can apply the follow-up documentation/kerneldoc work that's still getting done.
FYI, I've pushed a branch including Libin's patchset (but only patches 1-3) just for checking. Let me know if this looks OK.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, September 02, 2015 11:36 PM To: Daniel Vetter Cc: Jani Nikula; Yang, Libin; alsa-devel@alsa-project.org; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 02 Sep 2015 17:22:01 +0200, Daniel Vetter wrote:
On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 15:44:34 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
>> Nitpick. I'd prefer some sharing with the similar blocks from
the
>> earlier patch. Also a debug message on n == 0 would be
nice; you
>> probably didn't notice your audio_config_get_rate() wasn't
working
>> right >> because this silently fell back to the automatic mode here. > > OK, I will add the msg. As you and Ville are insisting on > sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for
now,
we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to
Takashi I
guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if
Libin
can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a
fixed
version of patch 4 could follow. Just a thought.
That's good, then I can take the first three patches.
Daniel, would you like to review these three before I merge them? I assumed your previous mail as a kind of ack to this series, too.
fwiw ack on that plan, but given how much I made a mess out of
these two
series and mixed them up I wouldn't trust me anyway ;-)
Once the code settles I'll rebase/backmerge so that I can apply the follow-up documentation/kerneldoc work that's still getting done.
FYI, I've pushed a branch including Libin's patchset (but only patches 1-3) just for checking. Let me know if this looks OK.
Thanks, Takashi, Daniel, Jani and Ville.
So I need submit the 4th patch only next time, right?
Takashi
On Fri, 04 Sep 2015 03:56:26 +0200, Yang, Libin wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, September 02, 2015 11:36 PM To: Daniel Vetter Cc: Jani Nikula; Yang, Libin; alsa-devel@alsa-project.org; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; ville.syrjala@linux.intel.com Subject: Re: [PATCH v6 4/4] drm/i915: set proper N/CTS in modeset
On Wed, 02 Sep 2015 17:22:01 +0200, Daniel Vetter wrote:
On Wed, Sep 02, 2015 at 03:46:40PM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 15:44:34 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote: > > >> Nitpick. I'd prefer some sharing with the similar blocks from
the
> >> earlier patch. Also a debug message on n == 0 would be
nice; you
> >> probably didn't notice your audio_config_get_rate() wasn't
working
> >> right > >> because this silently fell back to the automatic mode here. > > > > OK, I will add the msg. As you and Ville are insisting on > > sharing code, I will do it in next version. > > Well, really, I'm fine with having that part duplicated as-is for
now,
> we can fix it later. More important to focus on getting > audio_config_get_rate() right. > > I don't know if you're still targeting v4.3 with this (up to
Takashi I
> guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if
Libin
can prepare the fixed version soonish, indeed.
IIUC patches 1-3 would be useful on their own already, and a
fixed
version of patch 4 could follow. Just a thought.
That's good, then I can take the first three patches.
Daniel, would you like to review these three before I merge them? I assumed your previous mail as a kind of ack to this series, too.
fwiw ack on that plan, but given how much I made a mess out of
these two
series and mixed them up I wouldn't trust me anyway ;-)
Once the code settles I'll rebase/backmerge so that I can apply the follow-up documentation/kerneldoc work that's still getting done.
FYI, I've pushed a branch including Libin's patchset (but only patches 1-3) just for checking. Let me know if this looks OK.
Thanks, Takashi, Daniel, Jani and Ville.
So I need submit the 4th patch only next time, right?
Yes.
Takashi
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
Takashi, just to double check, none of this made it into v4.3 after all?
BR, Jani.
thanks,
Takashi
On Mon, 19 Oct 2015 10:25:22 +0200, Jani Nikula wrote:
On Wed, 02 Sep 2015, Takashi Iwai tiwai@suse.de wrote:
On Wed, 02 Sep 2015 11:02:42 +0200, Jani Nikula wrote:
Nitpick. I'd prefer some sharing with the similar blocks from the earlier patch. Also a debug message on n == 0 would be nice; you probably didn't notice your audio_config_get_rate() wasn't working right because this silently fell back to the automatic mode here.
OK, I will add the msg. As you and Ville are insisting on sharing code, I will do it in next version.
Well, really, I'm fine with having that part duplicated as-is for now, we can fix it later. More important to focus on getting audio_config_get_rate() right.
I don't know if you're still targeting v4.3 with this (up to Takashi I guess) we'll really need to wrap this up soon.
I'm in favor of merging this into 4.3, so it'd be appreciated if Libin can prepare the fixed version soonish, indeed.
Takashi, just to double check, none of this made it into v4.3 after all?
Right, sync_audio_rate was slipped from 4.3 merge window, unfortunately.
Takashi
On Wed, 02 Sep 2015, libin.yang@intel.com wrote:
From: Libin Yang libin.yang@intel.com
Add the sync_audio_rate callback.
With the callback, audio driver can trigger i915 driver to set the proper N/CTS or N/M based on different sample rates.
Signed-off-by: Libin Yang libin.yang@intel.com
Reviewed-by: Jani Nikula jani.nikula@intel.com
include/drm/i915_component.h | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..8ad6f1b 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,13 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *);
/*** @sync_audio_rate: set n/cts based on the sample rate** Called from audio driver. After audio driver sets the* sample rate, it will call this function to set n/cts*/int (*sync_audio_rate)(struct device *, int port, int rate);};
-- 1.9.1
participants (5)
-
Daniel Vetter -
Jani Nikula -
libin.yang＠intel.com -
Takashi Iwai -
Yang, Libin