[alsa-devel] [PATCH 0/7] Add get_eld audio component for i915/HD-audio
Hi,
this is a patchset to add get_eld op to audio component for communicating more directly between i915 and HD-audio. Currently, the HDMI/DP audio status and ELD are notified and obtained via the hardware-level communication over HD-audio unsolicited event and verbs although the graphics driver holds the exactly same information. As we already have a notification via audio component, this is another step forward; the audio driver fetches directly the audio status and ELD via the new component op.
The initial draft was posted to alsa-devel as an RFC some weeks ago, and this is a revised version, posted to both i915 and alsa-devel for review.
The current patchset is found in sound git tree test/hdmi-jack branch.
Takashi
Takashi Iwai (7): drm/i915: Remove superfluous NULL check drm/i915: Add get_eld audio component drm/i915: refactoring audio component functions ALSA: hda - Split ELD update code from hdmi_present_sense() ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling ALSA: hda - Do zero-clear in snd_hdmi_parse_eld() itself ALSA: hda - Skip ELD notification during PM process
drivers/gpu/drm/i915/intel_audio.c | 75 ++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 + sound/pci/hda/hda_eld.c | 1 + sound/pci/hda/patch_hdmi.c | 195 ++++++++++++++++++++++++++----------- 5 files changed, 200 insertions(+), 75 deletions(-)
to_intel_crtc() always returns a non-NULL pointer.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/i915/intel_audio.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 4dccd9b003a1..0c38cc6c82ae 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -656,10 +656,6 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, 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; }
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
For achieving this, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 +++ 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..6b318a8d5dc9 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
+ intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode); @@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;
+ intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);
@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }
+static int i915_audio_component_get_eld(struct device *dev, int port, + bool *enabled, + unsigned char *buf, int max_bytes) +{ + 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 drm_connector *connector; + unsigned char *eld; + int ret = -EINVAL; + + mutex_lock(&dev_priv->av_mutex); + for_each_intel_encoder(drm_dev, intel_encoder) { + if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT && + intel_encoder->type != INTEL_OUTPUT_HDMI) + continue; + intel_dig_port = enc_to_dig_port(&intel_encoder->base); + if (port == intel_dig_port->port) { + ret = 0; + *enabled = intel_dig_port->audio_enabled; + if (!*enabled) + break; + connector = drm_select_eld(&intel_encoder->base); + if (!connector) + break; + eld = connector->eld; + ret = min(max_bytes, drm_eld_size(eld)); + memcpy(buf, eld, ret); + break; + } + } + + mutex_unlock(&dev_priv->av_mutex); + return ret; +} + static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .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, + .get_eld = i915_audio_component_get_eld, };
static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi; + bool audio_enabled; enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override; }; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@ * @codec_wake_override: Enable/Disable generating the codec wake signal * @get_cdclk_freq: get the Core Display Clock in KHz * @sync_audio_rate: set n/cts based on the sample rate + * @get_eld: fill the audio state and ELD bytes for the given port */ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate); + int (*get_eld)(struct device *, int port, bool *enabled, + unsigned char *buf, int max_bytes); };
struct i915_audio_component_audio_ops {
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
For achieving this, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 +++ 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..6b318a8d5dc9 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);
@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;
- intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);
@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }
+static int i915_audio_component_get_eld(struct device *dev, int port,
bool *enabled,unsigned char *buf, int max_bytes)+{
- 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 drm_connector *connector;
- unsigned char *eld;
- int ret = -EINVAL;
Locking is a bit in-flux atm with atomic, but needs dev_priv->dev->mode_config->mutex here to protect against the eld changing.
- mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)
MST? Not that I have a clue how that should work ;-)
continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;connector = drm_select_eld(&intel_encoder->base);if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));
How can the caller figure out what the eld size is if you use min here? At least in drm we just return the size we want if it's too small.
memcpy(buf, eld, ret);break;}- }
- mutex_unlock(&dev_priv->av_mutex);
- return ret;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .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,
- .get_eld = i915_audio_component_get_eld,
};
static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi;
- bool audio_enabled;
Needs a comment that this is protected by dev_priv->av_mutex.
enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override; }; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
- @get_eld: fill the audio state and ELD bytes for the given port
In 4.4 kerneldoc supports extended in-line comments for struct members like this:
*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
/** * @get_eld: * * lots of blabla, possibly in multiple paragraphs. */
Please use that layout and put a copy of the more detailed description you put into the commit message of how ->get_eld exactly works.
I did ask for this to get done as part of some of the previous, and it was partially done but not exactly how kerneldoc wants it :( But I think we need to start somewhere ...
Cheers, Daniel
- int (*get_eld)(struct device *, int port, bool *enabled,
unsigned char *buf, int max_bytes);};
struct i915_audio_component_audio_ops {
2.6.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
- @get_eld: fill the audio state and ELD bytes for the given port
In 4.4 kerneldoc supports extended in-line comments for struct members like this:
*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
/** * @get_eld: * * lots of blabla, possibly in multiple paragraphs. */
Please use that layout and put a copy of the more detailed description you put into the commit message of how ->get_eld exactly works.
I did ask for this to get done as part of some of the previous, and it was partially done but not exactly how kerneldoc wants it :( But I think we need to start somewhere ...
Strike that, I looked at the wrong tree ;-) linux-next should have all the patches using the new extended style. -Daniel
On Mon, 30 Nov 2015 15:17:05 +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
- @get_eld: fill the audio state and ELD bytes for the given port
In 4.4 kerneldoc supports extended in-line comments for struct members like this:
*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
/** * @get_eld: * * lots of blabla, possibly in multiple paragraphs. */
Please use that layout and put a copy of the more detailed description you put into the commit message of how ->get_eld exactly works.
I did ask for this to get done as part of some of the previous, and it was partially done but not exactly how kerneldoc wants it :( But I think we need to start somewhere ...
Strike that, I looked at the wrong tree ;-) linux-next should have all the patches using the new extended style.
OK, so this is a post-4.4 change. I can rebase on it. Could you point a steady branch suitable for it?
thanks,
Takashi
On Mon, Nov 30, 2015 at 04:55:50PM +0100, Takashi Iwai wrote:
On Mon, 30 Nov 2015 15:17:05 +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 03:11:16PM +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
- @get_eld: fill the audio state and ELD bytes for the given port
In 4.4 kerneldoc supports extended in-line comments for struct members like this:
*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
/** * @get_eld: * * lots of blabla, possibly in multiple paragraphs. */
Please use that layout and put a copy of the more detailed description you put into the commit message of how ->get_eld exactly works.
I did ask for this to get done as part of some of the previous, and it was partially done but not exactly how kerneldoc wants it :( But I think we need to start somewhere ...
Strike that, I looked at the wrong tree ;-) linux-next should have all the patches using the new extended style.
OK, so this is a post-4.4 change. I can rebase on it. Could you point a steady branch suitable for it?
Dave's drm-next would be it, if Dave opens it up and pulls in the 2 pull requests I've sent out earlier. Dave? -Daniel
On Mon, 30 Nov 2015 15:11:16 +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
For achieving this, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 +++ 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..6b318a8d5dc9 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);
@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;
- intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);
@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }
+static int i915_audio_component_get_eld(struct device *dev, int port,
bool *enabled,unsigned char *buf, int max_bytes)+{
- 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 drm_connector *connector;
- unsigned char *eld;
- int ret = -EINVAL;
Locking is a bit in-flux atm with atomic, but needs dev_priv->dev->mode_config->mutex here to protect against the eld changing.
Noted, I'll add it in the next respin.
- mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)MST? Not that I have a clue how that should work ;-)
Well, I supposed that MST entry doesn't actually serve for the digital port, but I'm not entirely sure. In anyway, the whole MST audio support shall be added / handled by other Intel people soon later, so let's keep this as is -- which is the same condition as the current i915 code.
continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;connector = drm_select_eld(&intel_encoder->base);if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));How can the caller figure out what the eld size is if you use min here? At least in drm we just return the size we want if it's too small.
A good point. The function should return the size "to be written all", indeed.
memcpy(buf, eld, ret);break;}- }
- mutex_unlock(&dev_priv->av_mutex);
- return ret;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .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,
- .get_eld = i915_audio_component_get_eld,
};
static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi;
- bool audio_enabled;
Needs a comment that this is protected by dev_priv->av_mutex.
OK.
enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override; }; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
- @get_eld: fill the audio state and ELD bytes for the given port
In 4.4 kerneldoc supports extended in-line comments for struct members like this:
*/ struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
/** * @get_eld: * * lots of blabla, possibly in multiple paragraphs. */
Please use that layout and put a copy of the more detailed description you put into the commit message of how ->get_eld exactly works.
I did ask for this to get done as part of some of the previous, and it was partially done but not exactly how kerneldoc wants it :( But I think we need to start somewhere ...
Yeah, it looks like that I missed the back-merge of 4.4-rc although I thought I did. Will rebase and rewrite that.
thanks,
Takashi
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
For achieving this, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 +++ 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..6b318a8d5dc9 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);
@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;
- intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);
@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }
+static int i915_audio_component_get_eld(struct device *dev, int port,
bool *enabled,unsigned char *buf, int max_bytes)+{
- 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 drm_connector *connector;
- unsigned char *eld;
- int ret = -EINVAL;
- mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;connector = drm_select_eld(&intel_encoder->base);if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);break;}- }
- mutex_unlock(&dev_priv->av_mutex);
- return ret;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .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,
- .get_eld = i915_audio_component_get_eld,
};
static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi;
- bool audio_enabled; enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override;
}; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
*/
- @get_eld: fill the audio state and ELD bytes for the given port
struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
- int (*get_eld)(struct device *, int port, bool *enabled,
unsigned char *buf, int max_bytes);};
struct i915_audio_component_audio_ops {
2.6.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 30 Nov 2015 16:24:41 +0100, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
We need this at least in two situations: - when the audio driver is probed - when the audio driver is resumed
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
OK, then we may change the eld_notify to pass the values in the arguments, and also add the new op with a lock to be called from other places from other places.
Takashi
On 2015-11-30 16:29, Takashi Iwai wrote:
On Mon, 30 Nov 2015 16:24:41 +0100, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
We need this at least in two situations:
- when the audio driver is probed
- when the audio driver is resumed
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
OK, then we may change the eld_notify to pass the values in the arguments, and also add the new op with a lock to be called from other places from other places.
Maybe we have to make eld_notify not do anything except to call schedule_work then? And that work in turn will ask for updated eld from the i915 driver, and handle the result.
On Mon, 30 Nov 2015 16:34:22 +0100, David Henningsson wrote:
On 2015-11-30 16:29, Takashi Iwai wrote:
On Mon, 30 Nov 2015 16:24:41 +0100, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
We need this at least in two situations:
- when the audio driver is probed
- when the audio driver is resumed
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
OK, then we may change the eld_notify to pass the values in the arguments, and also add the new op with a lock to be called from other places from other places.
Maybe we have to make eld_notify not do anything except to call schedule_work then? And that work in turn will ask for updated eld from the i915 driver, and handle the result.
Yes, it would work, too -- though, this would need a bit more code reorganization.
Takashi
On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
Hmm. Actually the locking isn't perhaps quite like that atm. But I guess the mode_config.mutex will make it so.
Apart from that it seesm to me that you should pull the av_mutex lock/unlock from the .audio_code_eanble/disable hooks into intel_audio_codec_enable/disable, so that it protects the audio_enabled flag as well. Not sure if the eld_notify should be called while holding that lock or not. If we need to avoid calling it from the eld_notify anywya due to other locks then maybe it can be under av_mutex as well.
For achieving this, a new field audio_enabled is added to struct intel_digital_port. This is set/reset at each audio enable/disable call in intel_audio.c.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 40 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + include/drm/i915_component.h | 3 +++ 3 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 0c38cc6c82ae..6b318a8d5dc9 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -521,6 +521,7 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
connector->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
- intel_dig_port->audio_enabled = true; if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, adjusted_mode);
@@ -545,6 +546,7 @@ void intel_audio_codec_disable(struct intel_encoder *intel_encoder) struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); enum port port = intel_dig_port->port;
- intel_dig_port->audio_enabled = false; if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(intel_encoder);
@@ -702,6 +704,43 @@ static int i915_audio_component_sync_audio_rate(struct device *dev, return 0; }
+static int i915_audio_component_get_eld(struct device *dev, int port,
bool *enabled,unsigned char *buf, int max_bytes)+{
- 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 drm_connector *connector;
- unsigned char *eld;
- int ret = -EINVAL;
- mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;connector = drm_select_eld(&intel_encoder->base);if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);break;}- }
- mutex_unlock(&dev_priv->av_mutex);
- return ret;
+}
static const struct i915_audio_component_ops i915_audio_component_ops = { .owner = THIS_MODULE, .get_power = i915_audio_component_get_power, @@ -709,6 +748,7 @@ static const struct i915_audio_component_ops i915_audio_component_ops = { .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,
- .get_eld = i915_audio_component_get_eld,
};
static int i915_audio_component_bind(struct device *i915_dev, diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0598932ce623..4afc7560be04 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,6 +798,7 @@ struct intel_digital_port { u32 saved_port_bits; struct intel_dp dp; struct intel_hdmi hdmi;
- bool audio_enabled; enum irqreturn (*hpd_pulse)(struct intel_digital_port *, bool); bool release_cl2_override;
}; diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index 30d89e0da2c6..058d39e8d57f 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -38,6 +38,7 @@
- @codec_wake_override: Enable/Disable generating the codec wake signal
- @get_cdclk_freq: get the Core Display Clock in KHz
- @sync_audio_rate: set n/cts based on the sample rate
*/
- @get_eld: fill the audio state and ELD bytes for the given port
struct i915_audio_component_ops { struct module *owner; @@ -46,6 +47,8 @@ struct i915_audio_component_ops { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); int (*sync_audio_rate)(struct device *, int port, int rate);
- int (*get_eld)(struct device *, int port, bool *enabled,
unsigned char *buf, int max_bytes);};
struct i915_audio_component_audio_ops {
2.6.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
Hmm. Actually the locking isn't perhaps quite like that atm. But I guess the mode_config.mutex will make it so.
If we need this we could create a substruct in dev_priv with copies of everything, which would only be protected by av_mutex. That's the usual approach we use when faced with this kind of locking inversion: Copy/update relevant data in the modeset ->enable/disable hooks, then just use these local locks to access those copies. Usually that's enough to untangle things, with no need to resort to workers. -Daniel
On Mon, Nov 30, 2015 at 05:34:45PM +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 06:09:33PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
Hmm. Actually the locking isn't perhaps quite like that atm. But I guess the mode_config.mutex will make it so.
If we need this we could create a substruct in dev_priv with copies of everything, which would only be protected by av_mutex. That's the usual approach we use when faced with this kind of locking inversion: Copy/update relevant data in the modeset ->enable/disable hooks, then just use these local locks to access those copies. Usually that's enough to untangle things, with no need to resort to workers.
Yeah, IIRC I suggested just that originally.
On Mon, 30 Nov 2015 17:09:33 +0100, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 05:24:41PM +0200, Ville Syrjälä wrote:
On Mon, Nov 30, 2015 at 02:37:46PM +0100, Takashi Iwai wrote:
Implement a new i915_audio_component_ops, get_eld(). It's called by the audio driver to fetch the current ELD of the given HDMI/DP port. It returns the size of ELD bytes if it's valid, or zero if the audio is disabled or unplugged, or a negative error code.
Why do we need this? Isn't it something the eld notify hook should pass from i915 to the audio driver?
At least with the locking you have for this, the audio driver can not call this from the eld notify hook since it would deadlock.
Hmm. Actually the locking isn't perhaps quite like that atm. But I guess the mode_config.mutex will make it so.
Apart from that it seesm to me that you should pull the av_mutex lock/unlock from the .audio_code_eanble/disable hooks into intel_audio_codec_enable/disable, so that it protects the audio_enabled flag as well. Not sure if the eld_notify should be called while holding that lock or not. If we need to avoid calling it from the eld_notify anywya due to other locks then maybe it can be under av_mutex as well.
Currently I'm thinking of:
- not allow to call get_eld directly from eld_notify; I found that the existing eld_repoll work in the HDA can be reused easily, so let's follow Daniel's advice.
- drm_select_eld() seems requring mode_config.mutex and connection_mutex modeset lock in anyway; so let get_eld taking both.
Is it OK to call drm_modeset_lock_all() for simplicity?
- Get rid of av_mutex from get_eld instead; get_eld doesn't conflict with other ops
In that way, audio_enabled flag is protected in both places via modeset lock, I suppose.
thanks,
Takashi
We have a common loop of encoder to look for the given audio port in two audio component functions. Split out a local helper function to do it for the code simplification.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/i915/intel_audio.c | 61 ++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 6b318a8d5dc9..024e88ae6305 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -630,17 +630,33 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static struct intel_encoder *audio_port_to_encoder(struct drm_device *drm_dev, + int port) +{ + struct intel_encoder *intel_encoder; + struct intel_digital_port *intel_dig_port; + + for_each_intel_encoder(drm_dev, intel_encoder) { + if (intel_encoder->type != INTEL_OUTPUT_HDMI && + intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT) + continue; + intel_dig_port = enc_to_dig_port(&intel_encoder->base); + if (port == intel_dig_port->port) + return intel_encoder; + } + return NULL; +} + 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; struct i915_audio_component *acomp = dev_priv->audio_component; - enum pipe pipe = -1; + enum pipe pipe; u32 tmp; int n;
@@ -652,22 +668,14 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
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); - pipe = crtc->pipe; - break; - } - } - - if (pipe == INVALID_PIPE) { + intel_encoder = audio_port_to_encoder(drm_dev, port); + if (!intel_encoder || intel_encoder->type != INTEL_OUTPUT_HDMI) { DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); mutex_unlock(&dev_priv->av_mutex); return -ENODEV; } + crtc = to_intel_crtc(intel_encoder->base.crtc); + pipe = crtc->pipe; DRM_DEBUG_KMS("pipe %c connects port %c\n", pipe_name(pipe), port_name(port)); mode = &crtc->config->base.adjusted_mode; @@ -717,23 +725,18 @@ static int i915_audio_component_get_eld(struct device *dev, int port, int ret = -EINVAL;
mutex_lock(&dev_priv->av_mutex); - for_each_intel_encoder(drm_dev, intel_encoder) { - if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT && - intel_encoder->type != INTEL_OUTPUT_HDMI) - continue; + intel_encoder = audio_port_to_encoder(drm_dev, port); + if (intel_encoder) { + ret = 0; intel_dig_port = enc_to_dig_port(&intel_encoder->base); - if (port == intel_dig_port->port) { - ret = 0; - *enabled = intel_dig_port->audio_enabled; - if (!*enabled) - break; + *enabled = intel_dig_port->audio_enabled; + if (*enabled) { connector = drm_select_eld(&intel_encoder->base); - if (!connector) - break; - eld = connector->eld; - ret = min(max_bytes, drm_eld_size(eld)); - memcpy(buf, eld, ret); - break; + if (connector) { + eld = connector->eld; + ret = min(max_bytes, drm_eld_size(eld)); + memcpy(buf, eld, ret); + } } }
On Mon, Nov 30, 2015 at 02:37:47PM +0100, Takashi Iwai wrote:
We have a common loop of encoder to look for the given audio port in two audio component functions. Split out a local helper function to do it for the code simplification.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 61 ++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 6b318a8d5dc9..024e88ae6305 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -630,17 +630,33 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static struct intel_encoder *audio_port_to_encoder(struct drm_device *drm_dev,
int port)+{
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI &&intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port)
If this is static, maybe just maintain a snd_pin_to_port mapping in dev_priv? That's the approach we're usually taking, and it has the benefit of making it clear(er) that the binding is static ... Or is it not?
Makes sense otherwise. -Daniel
return intel_encoder;- }
- return NULL;
+}
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; struct i915_audio_component *acomp = dev_priv->audio_component;
- enum pipe pipe = -1;
- enum pipe pipe; u32 tmp; int n;
@@ -652,22 +668,14 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
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);pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
- intel_encoder = audio_port_to_encoder(drm_dev, port);
- if (!intel_encoder || intel_encoder->type != INTEL_OUTPUT_HDMI) { DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); mutex_unlock(&dev_priv->av_mutex); return -ENODEV; }
- crtc = to_intel_crtc(intel_encoder->base.crtc);
- pipe = crtc->pipe; DRM_DEBUG_KMS("pipe %c connects port %c\n", pipe_name(pipe), port_name(port)); mode = &crtc->config->base.adjusted_mode;
@@ -717,23 +725,18 @@ static int i915_audio_component_get_eld(struct device *dev, int port, int ret = -EINVAL;
mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)continue;
- intel_encoder = audio_port_to_encoder(drm_dev, port);
- if (intel_encoder) {
ret = 0;
if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;
*enabled = intel_dig_port->audio_enabled;if (*enabled) { connector = drm_select_eld(&intel_encoder->base);
if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);break;
if (connector) {eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);}-- 2.6.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 30 Nov 2015 15:14:03 +0100, Daniel Vetter wrote:
On Mon, Nov 30, 2015 at 02:37:47PM +0100, Takashi Iwai wrote:
We have a common loop of encoder to look for the given audio port in two audio component functions. Split out a local helper function to do it for the code simplification.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/i915/intel_audio.c | 61 ++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 6b318a8d5dc9..024e88ae6305 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -630,17 +630,33 @@ static int i915_audio_component_get_cdclk_freq(struct device *dev) return ret; }
+static struct intel_encoder *audio_port_to_encoder(struct drm_device *drm_dev,
int port)+{
- struct intel_encoder *intel_encoder;
- struct intel_digital_port *intel_dig_port;
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_HDMI &&intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT)continue;intel_dig_port = enc_to_dig_port(&intel_encoder->base);if (port == intel_dig_port->port)If this is static, maybe just maintain a snd_pin_to_port mapping in dev_priv? That's the approach we're usually taking, and it has the benefit of making it clear(er) that the binding is static ... Or is it not?
Yes, I *guess* this is static, but need a double-check. The current patchset just derives from the existing code.
thanks,
Takashi
Makes sense otherwise. -Daniel
return intel_encoder;- }
- return NULL;
+}
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; struct i915_audio_component *acomp = dev_priv->audio_component;
- enum pipe pipe = -1;
- enum pipe pipe; u32 tmp; int n;
@@ -652,22 +668,14 @@ static int i915_audio_component_sync_audio_rate(struct device *dev,
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);pipe = crtc->pipe;break;}- }
- if (pipe == INVALID_PIPE) {
- intel_encoder = audio_port_to_encoder(drm_dev, port);
- if (!intel_encoder || intel_encoder->type != INTEL_OUTPUT_HDMI) { DRM_DEBUG_KMS("no pipe for the port %c\n", port_name(port)); mutex_unlock(&dev_priv->av_mutex); return -ENODEV; }
- crtc = to_intel_crtc(intel_encoder->base.crtc);
- pipe = crtc->pipe; DRM_DEBUG_KMS("pipe %c connects port %c\n", pipe_name(pipe), port_name(port)); mode = &crtc->config->base.adjusted_mode;
@@ -717,23 +725,18 @@ static int i915_audio_component_get_eld(struct device *dev, int port, int ret = -EINVAL;
mutex_lock(&dev_priv->av_mutex);
- for_each_intel_encoder(drm_dev, intel_encoder) {
if (intel_encoder->type != INTEL_OUTPUT_DISPLAYPORT &&intel_encoder->type != INTEL_OUTPUT_HDMI)continue;
- intel_encoder = audio_port_to_encoder(drm_dev, port);
- if (intel_encoder) {
ret = 0;
if (port == intel_dig_port->port) {ret = 0;*enabled = intel_dig_port->audio_enabled;if (!*enabled)break;
*enabled = intel_dig_port->audio_enabled;if (*enabled) { connector = drm_select_eld(&intel_encoder->base);
if (!connector)break;eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);break;
if (connector) {eld = connector->eld;ret = min(max_bytes, drm_eld_size(eld));memcpy(buf, eld, ret);}-- 2.6.3
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
This is a preliminary patch for the later change to support ELD/jack handling with i915 audio component. This splits the ELD update code from hdmi_present_sense() so that it can be called from other places.
Just a code refactoring, no functional change.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 108 ++++++++++++++++++++++----------------------- 1 file changed, 54 insertions(+), 54 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 4b6fb668c91c..28684aa86408 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1530,6 +1530,56 @@ static int hdmi_read_pin_conn(struct hda_codec *codec, int pin_idx) return 0; }
+/* update per_pin ELD from the given new ELD; + * setup info frame and notification accordingly + */ +static void update_eld(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin, + struct hdmi_eld *eld) +{ + struct hdmi_eld *pin_eld = &per_pin->sink_eld; + bool old_eld_valid = pin_eld->eld_valid; + bool eld_changed; + + if (eld->eld_valid) + snd_hdmi_show_eld(codec, &eld->info); + + eld_changed = (pin_eld->eld_valid != eld->eld_valid); + if (eld->eld_valid && pin_eld->eld_valid) + if (pin_eld->eld_size != eld->eld_size || + memcmp(pin_eld->eld_buffer, eld->eld_buffer, + eld->eld_size) != 0) + eld_changed = true; + + pin_eld->eld_valid = eld->eld_valid; + pin_eld->eld_size = eld->eld_size; + if (eld->eld_valid) + memcpy(pin_eld->eld_buffer, eld->eld_buffer, eld->eld_size); + pin_eld->info = eld->info; + + /* + * Re-setup pin and infoframe. This is needed e.g. when + * - sink is first plugged-in + * - transcoder can change during stream playback on Haswell + * and this can make HW reset converter selection on a pin. + */ + if (eld->eld_valid && !old_eld_valid && per_pin->setup) { + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { + intel_verify_pin_cvt_connect(codec, per_pin); + intel_not_share_assigned_cvt(codec, per_pin->pin_nid, + per_pin->mux_idx); + } + + hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm); + } + + if (eld_changed) + snd_ctl_notify(codec->card, + SNDRV_CTL_EVENT_MASK_VALUE | + SNDRV_CTL_EVENT_MASK_INFO, + &per_pin->eld_ctl->id); +} + static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_jack_tbl *jack; @@ -1547,8 +1597,6 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) * the unsolicited response to avoid custom WARs. */ int present; - bool update_eld = false; - bool eld_changed = false; bool ret;
snd_hda_power_up_pm(codec); @@ -1575,61 +1623,13 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) eld->eld_size) < 0) eld->eld_valid = false; } - - if (eld->eld_valid) { - snd_hdmi_show_eld(codec, &eld->info); - update_eld = true; - } - else if (repoll) { - schedule_delayed_work(&per_pin->work, - msecs_to_jiffies(300)); - goto unlock; - } }
- if (pin_eld->eld_valid != eld->eld_valid) - eld_changed = true; - - if (pin_eld->eld_valid && !eld->eld_valid) - update_eld = true; - - if (update_eld) { - bool old_eld_valid = pin_eld->eld_valid; - pin_eld->eld_valid = eld->eld_valid; - if (pin_eld->eld_size != eld->eld_size || - memcmp(pin_eld->eld_buffer, eld->eld_buffer, - eld->eld_size) != 0) { - memcpy(pin_eld->eld_buffer, eld->eld_buffer, - eld->eld_size); - eld_changed = true; - } - pin_eld->eld_size = eld->eld_size; - pin_eld->info = eld->info; - - /* - * Re-setup pin and infoframe. This is needed e.g. when - * - sink is first plugged-in (infoframe is not set up if !monitor_present) - * - transcoder can change during stream playback on Haswell - * and this can make HW reset converter selection on a pin. - */ - if (eld->eld_valid && !old_eld_valid && per_pin->setup) { - if (is_haswell_plus(codec) || - is_valleyview_plus(codec)) { - intel_verify_pin_cvt_connect(codec, per_pin); - intel_not_share_assigned_cvt(codec, pin_nid, - per_pin->mux_idx); - } - - hdmi_setup_audio_infoframe(codec, per_pin, - per_pin->non_pcm); - } - } + if (!eld->eld_valid && repoll) + schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300)); + else + update_eld(codec, per_pin, eld);
- if (eld_changed) - snd_ctl_notify(codec->card, - SNDRV_CTL_EVENT_MASK_VALUE | SNDRV_CTL_EVENT_MASK_INFO, - &per_pin->eld_ctl->id); - unlock: ret = !repoll || !pin_eld->monitor_present || pin_eld->eld_valid;
jack = snd_hda_jack_tbl_get(codec, pin_nid);
On Mon, Nov 30, 2015 at 02:37:48PM +0100, Takashi Iwai wrote:
+/* update per_pin ELD from the given new ELD;
- setup info frame and notification accordingly
- */
nitpick, comment style
On Mon, 30 Nov 2015 17:00:33 +0100, Vinod Koul wrote:
On Mon, Nov 30, 2015 at 02:37:48PM +0100, Takashi Iwai wrote:
+/* update per_pin ELD from the given new ELD;
- setup info frame and notification accordingly
- */
nitpick, comment style
There is no preference in sound/* about this, and I myself prefer without the extra line :)
Takashi
Since we have a new audio component ops to fetch the current ELD and state now, we can reduce the usage of unsol event of HDMI/DP pins. The unsol event isn't only unreliable, but it also needs the power up/down of the codec and link at each time, which is a significant power and time loss.
In this patch, the jack creation and unsol/jack event handling are modified to use the audio component for the dedicated Intel chips.
The jack handling got slightly more codes than a simple usage of hda_jack layer since we need to deal directly with snd_jack object; the hda_jack layer is basically designed for the pin sense read and unsol events, both of which aren't used any longer in our case.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 28684aa86408..8378c31e0b4f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin { struct mutex lock; struct delayed_work work; struct snd_kcontrol *eld_ctl; + struct snd_jack *acomp_jack; /* jack via audio component */ int repoll_count; bool setup; /* the stream has been set up by prepare callback */ int channels; /* current number of channels */ @@ -141,6 +142,7 @@ struct hdmi_spec { struct hdmi_ops ops;
bool dyn_pin_out; + bool use_acomp; /* use audio component for ELD notify/update */
/* * Non-generic VIA/NVIDIA specific @@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec, &per_pin->eld_ctl->id); }
+static void sync_eld_via_acomp(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin); + static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_jack_tbl *jack; @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) int present; bool ret;
+ if (spec->use_acomp) { + sync_eld_via_acomp(codec, per_pin); + return false; /* don't call snd_hda_jack_report_sync() */ + } + snd_hda_power_up_pm(codec); present = snd_hda_pin_sense(codec, pin_nid);
@@ -2091,6 +2101,68 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec) return 0; }
+/* update ELD and jack state via audio component */ +static void sync_eld_via_acomp(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin) +{ + struct i915_audio_component *acomp = codec->bus->core.audio_component; + struct hdmi_spec *spec = codec->spec; + struct hdmi_eld *eld = &spec->temp_eld; + int size; + + if (acomp && acomp->ops && acomp->ops->get_eld) { + mutex_lock(&per_pin->lock); + size = acomp->ops->get_eld(acomp->dev, + intel_pin2port(per_pin->pin_nid), + &eld->monitor_present, + eld->eld_buffer, + ELD_MAX_SIZE); + if (size > 0) { + memset(&eld->info, 0, sizeof(eld->info)); + if (snd_hdmi_parse_eld(codec, &eld->info, + eld->eld_buffer, size) < 0) + size = -EINVAL; + } + + if (size > 0) { + eld->eld_valid = true; + eld->eld_size = size; + } else { + eld->eld_valid = false; + eld->eld_size = 0; + } + + update_eld(codec, per_pin, eld); + snd_jack_report(per_pin->acomp_jack, + eld->monitor_present ? SND_JACK_AVOUT : 0); + mutex_unlock(&per_pin->lock); + } +} + +static void free_acomp_jack_priv(struct snd_jack *jack) +{ + struct hdmi_spec_per_pin *per_pin = jack->private_data; + + per_pin->acomp_jack = NULL; +} + +static int add_acomp_jack_kctl(struct hda_codec *codec, + struct hdmi_spec_per_pin *per_pin, + const char *name) +{ + struct snd_jack *jack; + int err; + + err = snd_jack_new(codec->card, name, SND_JACK_AVOUT, &jack, + true, false); + if (err < 0) + return err; + per_pin->acomp_jack = jack; + jack->private_data = per_pin; + jack->private_free = free_acomp_jack_priv; + return 0; +} + static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx) { char hdmi_str[32] = "HDMI/DP"; @@ -2101,6 +2173,8 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pin_idx)
if (pcmdev > 0) sprintf(hdmi_str + strlen(hdmi_str), ",pcm=%d", pcmdev); + if (spec->use_acomp) + return add_acomp_jack_kctl(codec, per_pin, hdmi_str); phantom_jack = !is_jack_detectable(codec, per_pin->pin_nid); if (phantom_jack) strncat(hdmi_str, " Phantom", @@ -2196,6 +2270,8 @@ static int generic_hdmi_init(struct hda_codec *codec) hda_nid_t pin_nid = per_pin->pin_nid;
hdmi_init_pin(codec, pin_nid); + if (spec->use_acomp) + continue; snd_hda_jack_detect_enable_callback(codec, pin_nid, codec->jackpoll_interval > 0 ? jack_callback : NULL); } @@ -2219,7 +2295,7 @@ static void generic_hdmi_free(struct hda_codec *codec) struct hdmi_spec *spec = codec->spec; int pin_idx;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec)) + if (spec->use_acomp) snd_hdac_i915_register_notifier(NULL);
for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { @@ -2227,6 +2303,8 @@ static void generic_hdmi_free(struct hda_codec *codec)
cancel_delayed_work_sync(&per_pin->work); eld_proc_free(per_pin); + if (per_pin->acomp_jack) + snd_device_free(codec->card, per_pin->acomp_jack); }
hdmi_array_free(spec); @@ -2388,7 +2466,9 @@ static int patch_generic_hdmi(struct hda_codec *codec) is_broxton(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { + spec->use_acomp = + is_haswell_plus(codec) || is_valleyview_plus(codec); + if (spec->use_acomp) { codec->depop_delay = 0; spec->i915_audio_ops.audio_ptr = codec; spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
On Mon, Nov 30, 2015 at 02:37:49PM +0100, Takashi Iwai wrote:
Since we have a new audio component ops to fetch the current ELD and state now, we can reduce the usage of unsol event of HDMI/DP pins. The unsol event isn't only unreliable, but it also needs the power up/down of the codec and link at each time, which is a significant power and time loss.
In this patch, the jack creation and unsol/jack event handling are modified to use the audio component for the dedicated Intel chips.
The jack handling got slightly more codes than a simple usage of hda_jack layer since we need to deal directly with snd_jack object; the hda_jack layer is basically designed for the pin sense read and unsol events, both of which aren't used any longer in our case.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/patch_hdmi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 28684aa86408..8378c31e0b4f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin { struct mutex lock; struct delayed_work work; struct snd_kcontrol *eld_ctl;
- struct snd_jack *acomp_jack; /* jack via audio component */ int repoll_count; bool setup; /* the stream has been set up by prepare callback */ int channels; /* current number of channels */
@@ -141,6 +142,7 @@ struct hdmi_spec { struct hdmi_ops ops;
bool dyn_pin_out;
bool use_acomp; /* use audio component for ELD notify/update */
/*
- Non-generic VIA/NVIDIA specific
@@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec, &per_pin->eld_ctl->id); }
+static void sync_eld_via_acomp(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin);static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_jack_tbl *jack; @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) int present; bool ret;
- if (spec->use_acomp) {
sync_eld_via_acomp(codec, per_pin);return false; /* don't call snd_hda_jack_report_sync() */- }
- snd_hda_power_up_pm(codec); present = snd_hda_pin_sense(codec, pin_nid);
@@ -2091,6 +2101,68 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec) return 0; }
+/* update ELD and jack state via audio component */ +static void sync_eld_via_acomp(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin)+{
- struct i915_audio_component *acomp = codec->bus->core.audio_component;
- struct hdmi_spec *spec = codec->spec;
- struct hdmi_eld *eld = &spec->temp_eld;
- int size;
- if (acomp && acomp->ops && acomp->ops->get_eld) {
mutex_lock(&per_pin->lock);size = acomp->ops->get_eld(acomp->dev,intel_pin2port(per_pin->pin_nid),&eld->monitor_present,eld->eld_buffer,ELD_MAX_SIZE);if (size > 0) {memset(&eld->info, 0, sizeof(eld->info));if (snd_hdmi_parse_eld(codec, &eld->info,eld->eld_buffer, size) < 0)size = -EINVAL;}if (size > 0) {eld->eld_valid = true;eld->eld_size = size;} else {eld->eld_valid = false;eld->eld_size = 0;}update_eld(codec, per_pin, eld);snd_jack_report(per_pin->acomp_jack,eld->monitor_present ? SND_JACK_AVOUT : 0);mutex_unlock(&per_pin->lock);- }
+}
IMO This and the rest can be moved to sound/hda/ so that other can reuse this code, as the code will be same for other users too..
On Mon, 30 Nov 2015 17:42:33 +0100, Vinod Koul wrote:
On Mon, Nov 30, 2015 at 02:37:49PM +0100, Takashi Iwai wrote:
Since we have a new audio component ops to fetch the current ELD and state now, we can reduce the usage of unsol event of HDMI/DP pins. The unsol event isn't only unreliable, but it also needs the power up/down of the codec and link at each time, which is a significant power and time loss.
In this patch, the jack creation and unsol/jack event handling are modified to use the audio component for the dedicated Intel chips.
The jack handling got slightly more codes than a simple usage of hda_jack layer since we need to deal directly with snd_jack object; the hda_jack layer is basically designed for the pin sense read and unsol events, both of which aren't used any longer in our case.
Signed-off-by: Takashi Iwai tiwai@suse.de
sound/pci/hda/patch_hdmi.c | 84 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 82 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 28684aa86408..8378c31e0b4f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin { struct mutex lock; struct delayed_work work; struct snd_kcontrol *eld_ctl;
- struct snd_jack *acomp_jack; /* jack via audio component */ int repoll_count; bool setup; /* the stream has been set up by prepare callback */ int channels; /* current number of channels */
@@ -141,6 +142,7 @@ struct hdmi_spec { struct hdmi_ops ops;
bool dyn_pin_out;
bool use_acomp; /* use audio component for ELD notify/update */
/*
- Non-generic VIA/NVIDIA specific
@@ -1580,6 +1582,9 @@ static void update_eld(struct hda_codec *codec, &per_pin->eld_ctl->id); }
+static void sync_eld_via_acomp(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin);static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) { struct hda_jack_tbl *jack; @@ -1599,6 +1604,11 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) int present; bool ret;
- if (spec->use_acomp) {
sync_eld_via_acomp(codec, per_pin);return false; /* don't call snd_hda_jack_report_sync() */- }
- snd_hda_power_up_pm(codec); present = snd_hda_pin_sense(codec, pin_nid);
@@ -2091,6 +2101,68 @@ static int generic_hdmi_build_pcms(struct hda_codec *codec) return 0; }
+/* update ELD and jack state via audio component */ +static void sync_eld_via_acomp(struct hda_codec *codec,
struct hdmi_spec_per_pin *per_pin)+{
- struct i915_audio_component *acomp = codec->bus->core.audio_component;
- struct hdmi_spec *spec = codec->spec;
- struct hdmi_eld *eld = &spec->temp_eld;
- int size;
- if (acomp && acomp->ops && acomp->ops->get_eld) {
mutex_lock(&per_pin->lock);size = acomp->ops->get_eld(acomp->dev,intel_pin2port(per_pin->pin_nid),&eld->monitor_present,eld->eld_buffer,ELD_MAX_SIZE);if (size > 0) {memset(&eld->info, 0, sizeof(eld->info));if (snd_hdmi_parse_eld(codec, &eld->info,eld->eld_buffer, size) < 0)size = -EINVAL;}if (size > 0) {eld->eld_valid = true;eld->eld_size = size;} else {eld->eld_valid = false;eld->eld_size = 0;}update_eld(codec, per_pin, eld);snd_jack_report(per_pin->acomp_jack,eld->monitor_present ? SND_JACK_AVOUT : 0);mutex_unlock(&per_pin->lock);- }
+}
IMO This and the rest can be moved to sound/hda/ so that other can reuse this code, as the code will be same for other users too..
We have no per_pin or HDMI specific stuff there yet. What we can reuse at most is only the acomp->ops... call itself, so far. hdac_i915.c is a thin wrapper, and it doesn't handle anything deep wrt HDMI/DP by itself.
Takashi
Instead of doing in each caller side, snd_hdmi_parse_eld() does zero-clear of the parsed data by itself. This is safer and simplifies the code.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_eld.c | 1 + sound/pci/hda/patch_hdmi.c | 2 -- 2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/pci/hda/hda_eld.c b/sound/pci/hda/hda_eld.c index 563984dd2562..bc2e08257c2e 100644 --- a/sound/pci/hda/hda_eld.c +++ b/sound/pci/hda/hda_eld.c @@ -253,6 +253,7 @@ int snd_hdmi_parse_eld(struct hda_codec *codec, struct parsed_hdmi_eld *e, int mnl; int i;
+ memset(e, 0, sizeof(*e)); e->eld_ver = GRAB_BITS(buf, 0, 3, 5); if (e->eld_ver != ELD_VER_CEA_861D && e->eld_ver != ELD_VER_PARTIAL) { diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 8378c31e0b4f..23a4292c355f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1628,7 +1628,6 @@ static bool hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) &eld->eld_size) < 0) eld->eld_valid = false; else { - memset(&eld->info, 0, sizeof(struct parsed_hdmi_eld)); if (snd_hdmi_parse_eld(codec, &eld->info, eld->eld_buffer, eld->eld_size) < 0) eld->eld_valid = false; @@ -2118,7 +2117,6 @@ static void sync_eld_via_acomp(struct hda_codec *codec, eld->eld_buffer, ELD_MAX_SIZE); if (size > 0) { - memset(&eld->info, 0, sizeof(eld->info)); if (snd_hdmi_parse_eld(codec, &eld->info, eld->eld_buffer, size) < 0) size = -EINVAL;
The ELD notification can be received asynchronously from the graphics side, and this may happen just at the moment the sound driver is processing the suspend or the resume, and it would confuse the whole procedure. Since the ELD and connection states are updated in anyway at the end of the resume, we can skip it when received during PM process.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 23a4292c355f..0492f3cf744e 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2433,6 +2433,9 @@ static void intel_pin_eld_notify(void *audio_ptr, int port) */ if (snd_power_get_state(codec->card) != SNDRV_CTL_POWER_D0) return; + /* ditto during suspend/resume process itself */ + if (atomic_read(&(codec)->core.in_pm)) + return;
check_presence_and_report(codec, pin_nid); }
participants (5)
-
Daniel Vetter -
David Henningsson -
Takashi Iwai -
Ville Syrjälä -
Vinod Koul