[alsa-devel] [PATCH v5 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 | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..aabebcb 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,7 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); + int (*sync_audio_rate)(struct device *, int port, int rate); } *ops; };
From: Libin Yang libin.yang@intel.com
HDMI audio may not work at some frequencies with the HW provided N/CTS.
This patch sets the proper N value for the given audio sample rate at the impacted frequencies. At other frequencies, it will use the N/CTS value which HW provides.
Signed-off-by: Libin Yang libin.yang@intel.com --- drivers/gpu/drm/i915/intel_audio.c | 119 +++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..96b97be 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -68,6 +68,31 @@ static const struct { { 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 }, };
+/* HDMI N/CTS table */ +#define TMDS_297M 297000 +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001) +static const struct { + int sample_rate; + int clock; + int n; + int cts; +} aud_ncts[] = { + { 44100, TMDS_296M, 4459, 234375 }, + { 44100, TMDS_297M, 4704, 247500 }, + { 48000, TMDS_296M, 5824, 281250 }, + { 48000, TMDS_297M, 5120, 247500 }, + { 32000, TMDS_296M, 5824, 421875 }, + { 32000, TMDS_297M, 3072, 222750 }, + { 88200, TMDS_296M, 8918, 234375 }, + { 88200, TMDS_297M, 9408, 247500 }, + { 96000, TMDS_296M, 11648, 281250 }, + { 96000, TMDS_297M, 10240, 247500 }, + { 176400, TMDS_296M, 17836, 234375 }, + { 176400, TMDS_297M, 18816, 247500 }, + { 44100, TMDS_296M, 23296, 281250 }, + { 44100, TMDS_297M, 20480, 247500 }, +}; + /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -90,6 +115,31 @@ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode, int rate) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) { + if ((rate == aud_ncts[i].sample_rate) && + (mode->clock == aud_ncts[i].clock)) { + return aud_ncts[i].n; + } + } + return 0; +} + +/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc, + struct drm_display_mode *mode) +{ + if (((mode->clock == TMDS_297M) || + (mode->clock == TMDS_296M)) && + intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) + return true; + else + return false; +} + static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_sync_audio_rate(struct device *dev, + int port, int rate) +{ + struct drm_i915_private *dev_priv = dev_to_i915(dev); + struct drm_device *drm_dev = dev_priv->dev; + struct intel_encoder *intel_encoder; + struct intel_digital_port *intel_dig_port; + struct intel_crtc *crtc; + struct drm_display_mode *mode; + enum pipe pipe = -1; + u32 tmp; + int n_low, n_up, n; + + /* 1. get the pipe */ + for_each_intel_encoder(drm_dev, intel_encoder) { + 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)); + return -ENODEV; + } + DRM_DEBUG_KMS("pipe %c connects port %c\n", + pipe_name(pipe), port_name(port)); + mode = &crtc->config->base.adjusted_mode; + + /* 2. check whether to set the N/CTS/M manually or not */ + if (!audio_rate_need_prog(crtc, mode)) { + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + return 0; + } + + n = audio_config_get_n(mode, rate); + if (n == 0) { + DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n", + port_name(port)); + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + return 0; + } + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + + /* 4. set the N/CTS/M */ + tmp = I915_READ(HSW_AUD_CFG(pipe)); + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK); + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | + (n_low << AUD_CONFIG_LOWER_N_SHIFT) | + AUD_CONFIG_N_PROG_ENABLE); + I915_WRITE(HSW_AUD_CFG(pipe), tmp); + + return 0; +} + static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq, + .sync_audio_rate = i915_audio_component_sync_audio_rate, };
static int i915_audio_component_bind(struct device *i915_dev,
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119 +++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index dc32cf4..96b97be 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)
I don't really like the defines.
+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 },
+};
Last two should be 192 kHz. All the other values seem to match the spec.
These do assume 8bpc, but as the spec doesn't even specify N/CTS values for deep color modes @ 297MHz, so I suppose the expectations is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS clock rates we'll need to consider 12bpc as well.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) { @@ -90,6 +115,31 @@ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode *mode) return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode, int rate)
mode can be const.
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode *mode)+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static int i915_audio_component_sync_audio_rate(struct device *dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);
This sort of thing would need some locking to safely start digging at the modeset state.
I would probably not do that, and instead add some new lock(s) for this. The modeset code would then fill out some relevant information protected by that same lock, and this function could then go look at it without having to worry about the rest of modeset locking/state.
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));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);
This stuff would need to be abstracted to work on pre-HSW too.
return 0;- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode for N value on port %c\n",port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override = i915_audio_component_codec_wake_override, .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
+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 },
+};
Last two should be 192 kHz. All the other values seem to match the spec.
Oh, my typo. Thanks for pointing it out.
These do assume 8bpc, but as the spec doesn't even specify N/CTS values for deep color modes @ 297MHz, so I suppose the expectations is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS clock rates we'll need to consider 12bpc as well.
These values are recommended from HDMI spec. It's not related to the bpp. Am I wrong? I'm find the value from HDMISpecification 1.4: table 7-1, table 7-2 and table 7-3.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode, int
rate)
mode can be const.
OK. I will change it.
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);This sort of thing would need some locking to safely start digging at the modeset state.
I would probably not do that, and instead add some new lock(s) for this. The modeset code would then fill out some relevant information protected by that same lock, and this function could then go look at it without having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have suggested lock I can use?
if (!crtc) {DRM_DEBUG_KMS("%s: crtc isNULL\n", __func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
return 0;- }
- 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);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
I'd probably just have used raw numbers myself.
+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 },
+};
Last two should be 192 kHz. All the other values seem to match the spec.
Oh, my typo. Thanks for pointing it out.
These do assume 8bpc, but as the spec doesn't even specify N/CTS values for deep color modes @ 297MHz, so I suppose the expectations is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS clock rates we'll need to consider 12bpc as well.
These values are recommended from HDMI spec. It's not related to the bpp. Am I wrong? I'm find the value from HDMISpecification 1.4: table 7-1, table 7-2 and table 7-3.
There are separate tables for deep color modes in appendix D. Tables D-1 through D-3 are of interest to us since we can only do the 12bpc deep color mode.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode, int
rate)
mode can be const.
OK. I will change it.
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);This sort of thing would need some locking to safely start digging at the modeset state.
I would probably not do that, and instead add some new lock(s) for this. The modeset code would then fill out some relevant information protected by that same lock, and this function could then go look at it without having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have suggested lock I can use?
To do it like this we'd need pretty much all the modeset locks, which to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of new audio lock to i915.
if (!crtc) {DRM_DEBUG_KMS("%s: crtc isNULL\n", __func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
return 0;- }
- 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);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
On Mon, 24 Aug 2015 14:53:19 +0200, Ville Syrjälä wrote:
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
I'd probably just have used raw numbers myself.
It's quite unsafe. You can easily overlook, e.g. 29700 instead of 297000.
Takashi
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
I'd probably just have used raw numbers myself.
+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 },
+};
Last two should be 192 kHz. All the other values seem to match the spec.
Oh, my typo. Thanks for pointing it out.
These do assume 8bpc, but as the spec doesn't even specify N/CTS values for deep color modes @ 297MHz, so I suppose the
expectations
is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS clock rates we'll need to consider 12bpc as well.
These values are recommended from HDMI spec. It's not related to the bpp. Am I wrong? I'm find the value from HDMISpecification 1.4: table 7-1, table 7-2 and table 7-3.
There are separate tables for deep color modes in appendix D. Tables D-1 through D-3 are of interest to us since we can only do the 12bpc deep color mode.
Yes, we found the D tables in the spec before, and it seems a little Complicated. And the table 7-x seems to be more general. This is the reason we use table 7-x.
OK. We will use D-1, D-2 and D-3 table for N/CTS.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode,
int
rate)
mode can be const.
OK. I will change it.
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);This sort of thing would need some locking to safely start digging
at
the modeset state.
I would probably not do that, and instead add some new lock(s) for
this.
The modeset code would then fill out some relevant information protected by that same lock, and this function could then go look at it
without
having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have suggested lock I can use?
To do it like this we'd need pretty much all the modeset locks, which to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of new audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
Regards, Libin
if (!crtc) {DRM_DEBUG_KMS("%s: crtc isNULL\n", __func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
return 0;- }
- 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);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800, 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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
I'd probably just have used raw numbers myself.
+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 },
+};
Last two should be 192 kHz. All the other values seem to match the spec.
Oh, my typo. Thanks for pointing it out.
These do assume 8bpc, but as the spec doesn't even specify N/CTS values for deep color modes @ 297MHz, so I suppose the
expectations
is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS clock rates we'll need to consider 12bpc as well.
These values are recommended from HDMI spec. It's not related to the bpp. Am I wrong? I'm find the value from HDMISpecification 1.4: table 7-1, table 7-2 and table 7-3.
There are separate tables for deep color modes in appendix D. Tables D-1 through D-3 are of interest to us since we can only do the 12bpc deep color mode.
Yes, we found the D tables in the spec before, and it seems a little Complicated. And the table 7-x seems to be more general. This is the reason we use table 7-x.
What's complicated? With 8bpc you use 7-x, with 12bpc you use D-x. But as stated there are no 297MHz values in the D-x tables so the assumption seems to be that the max TMDS clock will be ~300Mhz and so there's no way to to get a 297Mhz pixel clock with deep color modes.
The fact that the D-x tables refer to the pixel clock instead of TMDS clock is also a bit confusing. Seems they forgot about pixel replication there. But I believe we should just consider the pixel replicated pixel clock when consulting these tables.
OK. We will use D-1, D-2 and D-3 table for N/CTS.
If you're only worried about 297Mhz modes, then there should be no need to consult the D-x tables at this time.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */ static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode *mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode *mode,
int
rate)
mode can be const.
OK. I will change it.
+{
- int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
if ((rate == aud_ncts[i].sample_rate) &&(mode->clock == aud_ncts[i].clock)) {return aud_ncts[i].n;}- }
- return 0;
+}
+/* check whether N/CTS/M need be set manually */ +static bool audio_rate_need_prog(struct intel_crtc *crtc,
struct drm_display_mode*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI))return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -514,12 +564,81 @@ static int
i915_audio_component_get_cdclk_freq(struct device *dev)
return ret; }
+static int i915_audio_component_sync_audio_rate(struct device
*dev,
int port, int rate)+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);This sort of thing would need some locking to safely start digging
at
the modeset state.
I would probably not do that, and instead add some new lock(s) for
this.
The modeset code would then fill out some relevant information protected by that same lock, and this function could then go look at it
without
having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have suggested lock I can use?
To do it like this we'd need pretty much all the modeset locks, which to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of new audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
The audio enable/disable during modeset would also grab the lock, and consult whatever information the audio driver provided. I would also suggest renaming the .sync_audio_rate() to something more clear like .set_sample_rate()
Anyway, I was thinking of something like this:
intel_audio_codec_enable() { lock(); .. configure n/cts encoder->audio.enabled = true; unlock(); }
intel_audio_codec_disable() { lock(); encoder->audio.enabled = false; unlock(); }
set_sample_rate() { lock(); encoder->audio.sample_rate = rate; if (encoder->audio.enabled) { ... reconfigure n/cts } unlock(); }
Something like that should protect sample rate changes from racing with an ongoing modeset.
Regards, Libin
if (!crtc) {DRM_DEBUG_KMS("%s: crtc isNULL\n", __func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe), port_name(port));- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
return 0;- }
- 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);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq = i915_audio_component_get_cdclk_freq,
- .sync_audio_rate = i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device *i915_dev,
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Tuesday, August 25, 2015 12:27 AM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800,
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
drivers/gpu/drm/i915/intel_audio.c | 119
+++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index dc32cf4..96b97be 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)
I don't really like the defines.
I agree it's not a good name. Do you have some suggestions?
I'd probably just have used raw numbers myself.
+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 },
+};
Last two should be 192 kHz. All the other values seem to match
the
spec.
Oh, my typo. Thanks for pointing it out.
These do assume 8bpc, but as the spec doesn't even specify
N/CTS
values for deep color modes @ 297MHz, so I suppose the
expectations
is that the max TMDS clock will never be so high as to allow them.
If/when we start using manual programming for other TMDS
clock
rates we'll need to consider 12bpc as well.
These values are recommended from HDMI spec. It's not related to the bpp. Am I wrong? I'm find the value from
HDMISpecification
1.4: table 7-1, table 7-2 and table 7-3.
There are separate tables for deep color modes in appendix D.
Tables
D-1 through D-3 are of interest to us since we can only do the
12bpc
deep color mode.
Yes, we found the D tables in the spec before, and it seems a little Complicated. And the table 7-x seems to be more general. This is the reason we use table 7-x.
What's complicated? With 8bpc you use 7-x, with 12bpc you use D-x. But as stated there are no 297MHz values in the D-x tables so the assumption seems to be that the max TMDS clock will be ~300Mhz and so there's no way to to get a 297Mhz pixel clock with deep color modes.
The fact that the D-x tables refer to the pixel clock instead of TMDS clock is also a bit confusing. Seems they forgot about pixel replication there. But I believe we should just consider the pixel replicated pixel clock when consulting these tables.
OK. We will use D-1, D-2 and D-3 table for N/CTS.
If you're only worried about 297Mhz modes, then there should be no need to consult the D-x tables at this time.
OK, so I will still use the 7-x table.
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode
*/
static u32 audio_config_hdmi_pixel_clock(struct
drm_display_mode
*mode)
{ @@ -90,6 +115,31 @@ static u32
audio_config_hdmi_pixel_clock(struct drm_display_mode
*mode)
return hdmi_audio_clock[i].config; }
+static int audio_config_get_n(struct drm_display_mode
*mode,
int
rate)
mode can be const.
OK. I will change it.
+{
- 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,
structdrm_display_mode
*mode)
+{
- if (((mode->clock == TMDS_297M) ||
(mode->clock == TMDS_296M)) &&intel_pipe_has_type(crtc,INTEL_OUTPUT_HDMI))
return true;- else
return false;+}
static bool intel_eld_uptodate(struct drm_connector
*connector,
int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda,@@ -514,12 +564,81 @@ 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, intrate)
+{
- struct drm_i915_private *dev_priv = dev_to_i915(dev);
- struct drm_device *drm_dev = dev_priv->dev;
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- struct intel_crtc *crtc;
- struct drm_display_mode *mode;
- enum pipe pipe = -1;
- u32 tmp;
- int n_low, n_up, n;
- /* 1. get the pipe */
- for_each_intel_encoder(drm_dev, intel_encoder) {
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);
This sort of thing would need some locking to safely start
digging
at
the modeset state.
I would probably not do that, and instead add some new lock(s)
for
this.
The modeset code would then fill out some relevant
information
protected by that same lock, and this function could then go look at it
without
having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the
function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have
suggested
lock I can use?
To do it like this we'd need pretty much all the modeset locks,
which
to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of
new
audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
The audio enable/disable during modeset would also grab the lock, and consult whatever information the audio driver provided. I would also suggest renaming the .sync_audio_rate() to something more clear like .set_sample_rate()
The sample rate in audio means the how often the audio data is sampled. Actually this function is none of setting sample rate. The sample rate setting is done in audio driver. What the function does is only setting the n/cts based on the audio sample rate.
Anyway, I was thinking of something like this:
intel_audio_codec_enable() { lock(); .. configure n/cts encoder->audio.enabled = true; unlock(); }
intel_audio_codec_disable() { lock(); encoder->audio.enabled = false; unlock(); }
set_sample_rate() { lock(); encoder->audio.sample_rate = rate; if (encoder->audio.enabled) { ... reconfigure n/cts } unlock(); }
I'm still not sure whether it is safe when gfx driver is doing the modeset. Suppose gfx driver will handle the protection, right?
If so, I will add spin_lock_irqsave as you suggested.
Something like that should protect sample rate changes from racing with an ongoing modeset.
Btw, it is not changing the sample rate. Audio sample rate will never be changed in these functions.
Regards, Libin
Regards, Libin
if (!crtc) {DRM_DEBUG_KMS("%s: crtc isNULL\n", __func__);
continue;}pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
DRM_DEBUG_KMS("no pipe for the port %c\n",port_name(port));
return -ENODEV;- }
- DRM_DEBUG_KMS("pipe %c connects port %c\n",
pipe_name(pipe),port_name(port));
- mode = &crtc->config->base.adjusted_mode;
- /* 2. check whether to set the N/CTS/M manually or
not */
- if (!audio_rate_need_prog(crtc, mode)) {
tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
return 0;- }
- n = audio_config_get_n(mode, rate);
- if (n == 0) {
DRM_DEBUG_KMS("Using automatic mode forN value
on port %c\n",
port_name(port));tmp = I915_READ(HSW_AUD_CFG(pipe));tmp &= ~AUD_CONFIG_N_PROG_ENABLE;I915_WRITE(HSW_AUD_CFG(pipe), tmp);return 0;- }
- n_low = n & 0xfff;
- n_up = (n >> 12) & 0xff;
- /* 4. set the N/CTS/M */
- tmp = I915_READ(HSW_AUD_CFG(pipe));
- tmp &= ~(AUD_CONFIG_UPPER_N_MASK |
AUD_CONFIG_LOWER_N_MASK);
- tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low <<AUD_CONFIG_LOWER_N_SHIFT) |
AUD_CONFIG_N_PROG_ENABLE);- I915_WRITE(HSW_AUD_CFG(pipe), tmp);
- return 0;
+}
static const struct i915_audio_component_ops
i915_audio_component_ops = {
.owner = THIS_MODULE, .get_power = i915_audio_component_get_power, .put_power = i915_audio_component_put_power, .codec_wake_override =
i915_audio_component_codec_wake_override,
.get_cdclk_freq =
i915_audio_component_get_cdclk_freq,
- .sync_audio_rate =
i915_audio_component_sync_audio_rate,
};
static int i915_audio_component_bind(struct device
*i915_dev,
-- 1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
On Tue, Aug 25, 2015 at 01:48:13AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Tuesday, August 25, 2015 12:27 AM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:14 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:52PM +0800,
libin.yang@intel.com
wrote: > +static int i915_audio_component_sync_audio_rate(struct
device
*dev, > + int port, int
rate)
> +{ > + struct drm_i915_private *dev_priv = dev_to_i915(dev); > + struct drm_device *drm_dev = dev_priv->dev; > + struct intel_encoder *intel_encoder; > + struct intel_digital_port *intel_dig_port; > + struct intel_crtc *crtc; > + struct drm_display_mode *mode; > + enum pipe pipe = -1; > + u32 tmp; > + int n_low, n_up, n; > + > + /* 1. get the pipe */ > + for_each_intel_encoder(drm_dev, intel_encoder) { > + 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);
This sort of thing would need some locking to safely start
digging
at
the modeset state.
I would probably not do that, and instead add some new lock(s)
for
this.
The modeset code would then fill out some relevant
information
protected by that same lock, and this function could then go look at it
without
having to worry about the rest of modeset locking/state.
From audio side, there is no competition when calling the
function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have
suggested
lock I can use?
To do it like this we'd need pretty much all the modeset locks,
which
to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind of
new
audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
The audio enable/disable during modeset would also grab the lock, and consult whatever information the audio driver provided. I would also suggest renaming the .sync_audio_rate() to something more clear like .set_sample_rate()
The sample rate in audio means the how often the audio data is sampled. Actually this function is none of setting sample rate.
Well it's more of sample_rate_change_notify() or something like that. But I don't mind too much what it's called. We can go with your original name if you feel it's more clear.
The sample rate setting is done in audio driver. What the function does is only setting the n/cts based on the audio sample rate.
Anyway, I was thinking of something like this:
intel_audio_codec_enable() { lock(); .. configure n/cts encoder->audio.enabled = true; unlock(); }
intel_audio_codec_disable() { lock(); encoder->audio.enabled = false; unlock(); }
set_sample_rate() { lock(); encoder->audio.sample_rate = rate; if (encoder->audio.enabled) { ... reconfigure n/cts } unlock(); }
I'm still not sure whether it is safe when gfx driver is doing the modeset. Suppose gfx driver will handle the protection, right?
intel_audio_codec_enable/disable are called during the modeset. With my idea as long as you're holding the lock and audio.enabled==true you could be sure there's nothing bad happening at the same time in i915.
If so, I will add spin_lock_irqsave as you suggested.
Could be a mutex I think.
Something like that should protect sample rate changes from racing with an ongoing modeset.
Btw, it is not changing the sample rate. Audio sample rate will never be changed in these functions.
The audio driver can change the sample rate at any point, at which point it'll call into i915 to reconfigure n/cts, no? So we just need to make sure things can't blow up if it does that while there's a modeset happening at the same time.
Regards, Libin
Regards, Libin
> + 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)); > + 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);
This stuff would need to be abstracted to work on pre-HSW too.
Right, I need judge the platform firstly.
> + return 0; > + } > + > + n = audio_config_get_n(mode, rate); > + if (n == 0) { > + DRM_DEBUG_KMS("Using automatic mode for
N value
on port %c\n", > + port_name(port)); > + tmp = I915_READ(HSW_AUD_CFG(pipe)); > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; > + I915_WRITE(HSW_AUD_CFG(pipe), tmp); > + return 0; > + } > + n_low = n & 0xfff; > + n_up = (n >> 12) & 0xff; > + > + /* 4. set the N/CTS/M */ > + tmp = I915_READ(HSW_AUD_CFG(pipe)); > + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | AUD_CONFIG_LOWER_N_MASK); > + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | > + (n_low <<
AUD_CONFIG_LOWER_N_SHIFT) |
> + AUD_CONFIG_N_PROG_ENABLE); > + I915_WRITE(HSW_AUD_CFG(pipe), tmp); > + > + return 0; > +} > + > static const struct i915_audio_component_ops i915_audio_component_ops = { > .owner = THIS_MODULE, > .get_power = i915_audio_component_get_power, > .put_power = i915_audio_component_put_power, > .codec_wake_override = i915_audio_component_codec_wake_override, > .get_cdclk_freq =
i915_audio_component_get_cdclk_freq,
> + .sync_audio_rate =
i915_audio_component_sync_audio_rate,
> }; > > static int i915_audio_component_bind(struct device
*i915_dev,
> -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
-- Ville Syrjälä Intel OTC
Hi Ville,
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Wednesday, August 26, 2015 8:27 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Tue, Aug 25, 2015 at 01:48:13AM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Tuesday, August 25, 2015 12:27 AM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 03:35:33PM +0000, Yang, Libin wrote:
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Monday, August 24, 2015 8:53 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement sync_audio_rate callback
On Mon, Aug 24, 2015 at 02:38:14AM +0000, Yang, Libin wrote:
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, August 21, 2015 11:14 PM > To: Yang, Libin > Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- > gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; > jani.nikula@linux.intel.com > Subject: Re: [Intel-gfx] [PATCH v5 2/4] drm/i915: implement > sync_audio_rate callback > > On Tue, Aug 18, 2015 at 02:51:52PM +0800,
libin.yang@intel.com
> wrote: > > +static int i915_audio_component_sync_audio_rate(struct
device
> *dev, > > + int port, int
rate)
> > +{ > > + struct drm_i915_private *dev_priv = dev_to_i915(dev); > > + struct drm_device *drm_dev = dev_priv->dev; > > + struct intel_encoder *intel_encoder; > > + struct intel_digital_port *intel_dig_port; > > + struct intel_crtc *crtc; > > + struct drm_display_mode *mode; > > + enum pipe pipe = -1; > > + u32 tmp; > > + int n_low, n_up, n; > > + > > + /* 1. get the pipe */ > > + for_each_intel_encoder(drm_dev, intel_encoder) { > > + 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);
> > This sort of thing would need some locking to safely start
digging
at
> the modeset state. > > I would probably not do that, and instead add some new
lock(s)
for
this.
> The modeset code would then fill out some relevant
information
> protected > by that same lock, and this function could then go look at it
without
> having to worry about the rest of modeset locking/state.
>From audio side, there is no competition when calling the
function,
I think. For protecting the competition between audio and gfx driver, it seems we need use the lock from gfx side. Do you have
suggested
lock I can use?
To do it like this we'd need pretty much all the modeset locks,
which
to me feels a bit troublesome if the audio driver can call this at any point. So I suggested that we may want to add some kind
of
new
audio lock to i915.
If we are using a new audio lock, I believe we still need use the lock when gfx is doing the modeset. Only adding the new lock in the function i915_audio_component_sync_audio_rate() is not enough because gfx driver will still modify it without locking.
I'm not sure where to add the lock in the gfx driver.
The audio enable/disable during modeset would also grab the lock, and consult whatever information the audio driver provided. I would
also
suggest renaming the .sync_audio_rate() to something more clear
like
.set_sample_rate()
The sample rate in audio means the how often the audio data is sampled. Actually this function is none of setting sample rate.
Well it's more of sample_rate_change_notify() or something like that. But I don't mind too much what it's called. We can go with your original name if you feel it's more clear.
OK, I will keep the original name as it will be more clear for audio driver developer.
The sample rate setting is done in audio driver. What the function does is only setting the n/cts based on the audio sample rate.
Anyway, I was thinking of something like this:
intel_audio_codec_enable() { lock(); .. configure n/cts encoder->audio.enabled = true; unlock(); }
intel_audio_codec_disable() { lock(); encoder->audio.enabled = false; unlock(); }
set_sample_rate() { lock(); encoder->audio.sample_rate = rate; if (encoder->audio.enabled) { ... reconfigure n/cts } unlock(); }
I'm still not sure whether it is safe when gfx driver is doing the
modeset.
Suppose gfx driver will handle the protection, right?
intel_audio_codec_enable/disable are called during the modeset. With my idea as long as you're holding the lock and audio.enabled==true you could be sure there's nothing bad happening at the same time in i915.
If so, I will add spin_lock_irqsave as you suggested.
Could be a mutex I think.
OK, mutex is OK for audio driver, too.
Something like that should protect sample rate changes from
racing
with an ongoing modeset.
Btw, it is not changing the sample rate. Audio sample rate will never be changed in these functions.
The audio driver can change the sample rate at any point, at which point it'll call into i915 to reconfigure n/cts, no? So we just need to make sure things can't blow up if it does that while there's a modeset happening at the same time.
The function is called before playing audio.
Regards, Libin
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 6786e94..122b5bd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7035,6 +7035,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac + #define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ @@ -7049,6 +7051,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \ + _HSW_AUD_STR_DESC_1, \ + _HSW_AUD_STR_DESC_2) + #define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 96b97be..0dfdc77 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -140,6 +140,27 @@ static bool audio_rate_need_prog(struct intel_crtc *crtc, return false; }
+static int audio_config_get_rate(struct drm_i915_private *dev_priv, + enum pipe pipe) +{ + uint32_t tmp; + int cvt_idx; + int base_rate, mul, div, rate; + + tmp = I915_READ(HSW_AUD_PIPE_CONN_SEL_CTRL); + cvt_idx = (tmp >> (pipe * 8)) & 0xff; + tmp = I915_READ(AUD_STR_DESC(cvt_idx)); + base_rate = tmp & (1 << 14); + if (base_rate == 0) + rate = 48000; + else + rate = 44100; + mul = (tmp & (0x7 << 11)) + 1; + div = (tmp & (0x7 << 8)) + 1; + rate = rate * mul / div; + return rate; +} + static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -261,6 +282,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i; + int n_low, n_up, n; + int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld)); @@ -296,12 +319,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX; - tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode); + + tmp &= ~AUD_CONFIG_N_PROG_ENABLE; + if (audio_rate_need_prog(intel_crtc, mode)) { + rate = audio_config_get_rate(dev_priv, pipe); + n = audio_config_get_n(mode, rate); + if (n != 0) { + n_low = n & 0xfff; + n_up = (n >> 12) & 0xff; + tmp &= ~(AUD_CONFIG_UPPER_N_MASK | + AUD_CONFIG_LOWER_N_MASK); + tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) | + (n_low << AUD_CONFIG_LOWER_N_SHIFT) | + AUD_CONFIG_N_PROG_ENABLE); + } + } + I915_WRITE(HSW_AUD_CFG(pipe), tmp); }
On Tue, Aug 18, 2015 at 02:51:54PM +0800, 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.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40 +++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6786e94..122b5bd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7035,6 +7035,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ @@ -7049,6 +7051,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1, \_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 96b97be..0dfdc77 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;
+}
Given your new .sync_audio_rate() callback, the audio driver should have already told us what the current sample rate is, and so we shouldn't have to go dig it up from registers.
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -261,6 +282,8 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes ELD\n", pipe_name(pipe), drm_eld_size(eld));
@@ -296,12 +319,27 @@ static void hsw_audio_codec_enable(struct drm_connector *connector, /* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DISPLAYPORT)) tmp |= AUD_CONFIG_N_VALUE_INDEX; else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_eed_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);}- }
We should share the code to set this up with .sync_audio_rate() rather than having two copies of essentically the same code.
- 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
-----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Friday, August 21, 2015 11:26 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [Intel-gfx] [PATCH v5 4/4] drm/i915: set proper N/CTS in modeset
On Tue, Aug 18, 2015 at 02:51:54PM +0800, 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.
Signed-off-by: Libin Yang libin.yang@intel.com
drivers/gpu/drm/i915/i915_reg.h | 8 ++++++++ drivers/gpu/drm/i915/intel_audio.c | 40
+++++++++++++++++++++++++++++++++++++-
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 6786e94..122b5bd 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7035,6 +7035,8 @@ enum skl_disp_power_wells { _HSW_AUD_MISC_CTRL_A, \ _HSW_AUD_MISC_CTRL_B)
+#define HSW_AUD_PIPE_CONN_SEL_CTRL 0x650ac
#define _HSW_AUD_DIP_ELD_CTRL_ST_A 0x650b4 #define _HSW_AUD_DIP_ELD_CTRL_ST_B 0x651b4 #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \ @@ -7049,6 +7051,12 @@ enum skl_disp_power_wells { _HSW_AUD_DIG_CNVT_2) #define DIP_PORT_SEL_MASK 0x3
+#define _HSW_AUD_STR_DESC_1 0x65084 +#define _HSW_AUD_STR_DESC_2 0x65184 +#define AUD_STR_DESC(pipe) _PIPE(pipe, \
_HSW_AUD_STR_DESC_1,\
_HSW_AUD_STR_DESC_2)#define _HSW_AUD_EDID_DATA_A 0x65050 #define _HSW_AUD_EDID_DATA_B 0x65150 #define HSW_AUD_EDID_DATA(pipe) _PIPE(pipe, \ diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 96b97be..0dfdc77 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;
+}
Given your new .sync_audio_rate() callback, the audio driver should have already told us what the current sample rate is, and so we shouldn't have to go dig it up from registers.
This patch is not calling sync_audio_rate() from audio driver. It happens when gfx doing the modeset.
static bool intel_eld_uptodate(struct drm_connector *connector, int reg_eldv, uint32_t bits_eldv, int reg_elda, uint32_t bits_elda, @@ -261,6 +282,8 @@ static void hsw_audio_codec_enable(struct
drm_connector *connector,
const uint8_t *eld = connector->eld; uint32_t tmp; int len, i;
int n_low, n_up, n;
int rate;
DRM_DEBUG_KMS("Enable audio codec on pipe %c, %u bytes
ELD\n",
pipe_name(pipe), drm_eld_size(eld));@@ -296,12 +319,27 @@ static void
hsw_audio_codec_enable(struct drm_connector *connector,
/* Enable timestamps */ tmp = I915_READ(HSW_AUD_CFG(pipe)); tmp &= ~AUD_CONFIG_N_VALUE_INDEX;
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE; tmp &= ~AUD_CONFIG_PIXEL_CLOCK_HDMI_MASK; if (intel_pipe_has_type(intel_crtc,
INTEL_OUTPUT_DISPLAYPORT))
tmp |= AUD_CONFIG_N_VALUE_INDEX;else tmp |= audio_config_hdmi_pixel_clock(mode);
- tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
- if (audio_rate_eed_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);
}- }
We should share the code to set this up with .sync_audio_rate() rather than having two copies of essentically the same code.
It's only setting a variable tmp, not sure we really need a separate function to do the setting?
- 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
-- Ville Syrjälä Intel OTC
On Tue, Aug 18, 2015 at 02:51:51PM +0800, 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
include/drm/i915_component.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..aabebcb 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,7 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *);
int (*sync_audio_rate)(struct device *, int port, int rate);
We're missing kerneldoc for this entire struct here, which isn't great. This needs to be fixed. Please - pull out the ops structure so it's not inlined (kerneldoc can't handle nested ops structures). - please document all the callbacks with kerneldoc. In 4.3 we can have kerneldoc in-line in structures right above each member like this
/** * @put_power: * * Longer text explaining this hook. */ void (*put_power)(struct device *);
For each hook please explain at least who calls it (gfx or audio) and where exactly it's called in the overall flow. - Extended the overview doc section with references to the component/ops structure would be needed too.
And please make sure it all looks pretty with make htmldocs.
Thanks, Daniel
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 4:18 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:51PM +0800, 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
include/drm/i915_component.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..aabebcb 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,7 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*sync_audio_rate)(struct device *, int port, intrate);
We're missing kerneldoc for this entire struct here, which isn't great. This needs to be fixed. Please
pull out the ops structure so it's not inlined (kerneldoc can't handle nested ops structures).
please document all the callbacks with kerneldoc. In 4.3 we can have kerneldoc in-line in structures right above each member like this
/**
- @put_power:
- Longer text explaining this hook.
*/ void (*put_power)(struct device *);
For each hook please explain at least who calls it (gfx or audio) and where exactly it's called in the overall flow.
Extended the overview doc section with references to the
component/ops structure would be needed too.
And please make sure it all looks pretty with make htmldocs.
Could we start another patch session for this task because, as you know, this is a little independent on these patches? What do you think?
Regards, Libin
Thanks, Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Wed, Aug 26, 2015 at 08:29:09AM +0000, Yang, Libin wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 4:18 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:51PM +0800, 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
include/drm/i915_component.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..aabebcb 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,7 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*sync_audio_rate)(struct device *, int port, intrate);
We're missing kerneldoc for this entire struct here, which isn't great. This needs to be fixed. Please
pull out the ops structure so it's not inlined (kerneldoc can't handle nested ops structures).
please document all the callbacks with kerneldoc. In 4.3 we can have kerneldoc in-line in structures right above each member like this
/**
- @put_power:
- Longer text explaining this hook.
*/ void (*put_power)(struct device *);
For each hook please explain at least who calls it (gfx or audio) and where exactly it's called in the overall flow.
Extended the overview doc section with references to the
component/ops structure would be needed too.
And please make sure it all looks pretty with make htmldocs.
Could we start another patch session for this task because, as you know, this is a little independent on these patches? What do you think?
Nowadays I want proper docs for new features, and for places where we missed them thus far they need to be backfilled. Also there's some good confusion in the review about how this all works together, so better docs seem called for no matter what to get this in. Instead of just adding all the explanations to commit messages only I figured it's better to do them as kerneldoc. -Daniel
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 5:08 PM To: Yang, Libin Cc: Daniel Vetter; alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
On Wed, Aug 26, 2015 at 08:29:09AM +0000, Yang, Libin wrote:
Hi Daniel,
-----Original Message----- From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter Sent: Wednesday, August 26, 2015 4:18 PM To: Yang, Libin Cc: alsa-devel@alsa-project.org; tiwai@suse.de; intel- gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; jani.nikula@linux.intel.com Subject: Re: [PATCH v5 1/4] drm/i915: Add audio sync_audio_rate callback
On Tue, Aug 18, 2015 at 02:51:51PM +0800, 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
include/drm/i915_component.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/drm/i915_component.h
b/include/drm/i915_component.h
index c9a8b64..aabebcb 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -33,6 +33,7 @@ struct i915_audio_component { void (*put_power)(struct device *); void (*codec_wake_override)(struct device *, bool
enable);
int (*get_cdclk_freq)(struct device *);
int (*sync_audio_rate)(struct device *, int port, intrate);
We're missing kerneldoc for this entire struct here, which isn't
great.
This needs to be fixed. Please
- pull out the ops structure so it's not inlined (kerneldoc can't
handle
nested ops structures).
- please document all the callbacks with kerneldoc. In 4.3 we can
have
kerneldoc in-line in structures right above each member like this
/**
- @put_power:
- Longer text explaining this hook.
*/ void (*put_power)(struct device *);
For each hook please explain at least who calls it (gfx or audio)
and
where exactly it's called in the overall flow.
- Extended the overview doc section with references to the
component/ops structure would be needed too.
And please make sure it all looks pretty with make htmldocs.
Could we start another patch session for this task because, as you know, this is a little independent on these patches? What do you think?
Nowadays I want proper docs for new features, and for places where we missed them thus far they need to be backfilled. Also there's some good confusion in the review about how this all works together, so better docs seem called for no matter what to get this in. Instead of just adding all the explanations to commit messages only I figured it's better to do them as kerneldoc.
Yes, I agree and I will add it for the sync_audio_rate function in the next version.
Regards, Libin
-Daniel
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
participants (5)
-
Daniel Vetter -
libin.yang＠intel.com -
Takashi Iwai -
Ville Syrjälä -
Yang, Libin