[alsa-devel] [PATCH v4 0/4] i915 to call hda driver on HDMI plug/unplug
It's been a while since the last patch set iteration, due to me being on vacation. But here's a new set, and I still hope that it can make it into the next merge window.
Changes since v3 (with the person suggesting that change within parantheses): * Valleyview now has three pins like all the others (Libin Yang) * Renamed a few references from "hotplug" to "pin_eld" to reduce confusion with hotplug code on the i915 side (Jani Nikula) * Rewrote the dispatching from hda core to codec on the hda side (Takashi Iwai)
This iteration has been tested and working on one Skylake machine.
David Henningsson (4): drm/i915: Add audio pin sense / ELD callback drm/i915: Call audio pin/ELD notify function ALSA: hda - allow codecs to access the i915 pin/ELD callback ALSA: hda - Wake the codec up on pin/ELD notify events
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++--- include/drm/i915_component.h | 12 ++++++++++++ include/sound/hda_i915.h | 7 +++++++ sound/hda/hdac_i915.c | 10 ++++++++++ sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 6 files changed, 71 insertions(+), 4 deletions(-)
This callback will be called by the i915 driver to notify the hda driver that its HDMI information needs to be refreshed, i e, that audio output is now available (or unavailable) - usually as a result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.henningsson@canonical.com --- include/drm/i915_component.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..ab5bde37 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -34,6 +34,18 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops; + + const struct i915_audio_component_audio_ops { + void *audio_ptr; + /** + * Call from i915 driver, notifying the HDA driver that + * pin sense and/or ELD information has changed. + * @audio_ptr: HDA driver object + * @port: Which port has changed (PORTA / PORTB / PORTC etc) + * @port_mst_index: Index within that port, for DisplayPort multistreaming + */ + void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index); + } *audio_ops; };
#endif /* _I915_COMPONENT_H_ */
On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
This callback will be called by the i915 driver to notify the hda driver that its HDMI information needs to be refreshed, i e, that audio output is now available (or unavailable) - usually as a result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/drm/i915_component.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..ab5bde37 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -34,6 +34,18 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_audio_ops {
void *audio_ptr;/*** Call from i915 driver, notifying the HDA driver that* pin sense and/or ELD information has changed.* @audio_ptr: HDA driver object* @port: Which port has changed (PORTA / PORTB / PORTC etc)* @port_mst_index: Index within that port, for DisplayPort multistreaming*/void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);- } *audio_ops;
This won't work as proper kerneldoc, but you get away with it since it's not pulled into the drm.tmpl. See my comments for the new set_audio_rate callback. -Daniel
};
#endif /* _I915_COMPONENT_H_ */
1.9.1
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2015-08-26 10:33, Daniel Vetter wrote:
On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
This callback will be called by the i915 driver to notify the hda driver that its HDMI information needs to be refreshed, i e, that audio output is now available (or unavailable) - usually as a result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/drm/i915_component.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..ab5bde37 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -34,6 +34,18 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_audio_ops {
void *audio_ptr;/*** Call from i915 driver, notifying the HDA driver that* pin sense and/or ELD information has changed.* @audio_ptr: HDA driver object* @port: Which port has changed (PORTA / PORTB / PORTC etc)* @port_mst_index: Index within that port, for DisplayPort multistreaming*/void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);- } *audio_ops;
This won't work as proper kerneldoc, but you get away with it since it's not pulled into the drm.tmpl. See my comments for the new set_audio_rate callback.
Sorry, my google failed me, so I can't find your comments for the set_audio_rate callback.
Apart from the kerneldoc issue, are you okay with acking the patch, at least the first two i915 ones, and agree with Takashi which tree this should go through?
On Wed, 26 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
On 2015-08-26 10:33, Daniel Vetter wrote:
On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
This callback will be called by the i915 driver to notify the hda driver that its HDMI information needs to be refreshed, i e, that audio output is now available (or unavailable) - usually as a result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/drm/i915_component.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..ab5bde37 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -34,6 +34,18 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_audio_ops {
void *audio_ptr;/*** Call from i915 driver, notifying the HDA driver that* pin sense and/or ELD information has changed.* @audio_ptr: HDA driver object* @port: Which port has changed (PORTA / PORTB / PORTC etc)* @port_mst_index: Index within that port, for DisplayPort multistreaming*/void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);- } *audio_ops;
This won't work as proper kerneldoc, but you get away with it since it's not pulled into the drm.tmpl. See my comments for the new set_audio_rate callback.
Sorry, my google failed me, so I can't find your comments for the set_audio_rate callback.
It's on the related thread [1], specifically the subthread starting at [2]. I guess there's no direct overlap between the two series, but it would be helpful if you can look at each other's work so there's no surprises.
BR, Jani.
[1] http://mid.gmane.org/1439880714-40931-1-git-send-email-libin.yang@intel.com [2] http://mid.gmane.org/20150826081735.GZ20434@phenom.ffwll.local
Apart from the kerneldoc issue, are you okay with acking the patch, at least the first two i915 ones, and agree with Takashi which tree this should go through?
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On Wed, 19 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
This callback will be called by the i915 driver to notify the hda driver that its HDMI information needs to be refreshed, i e, that audio output is now available (or unavailable) - usually as a result of a monitor being plugged in (or unplugged).
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/drm/i915_component.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..ab5bde37 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -34,6 +34,18 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_audio_ops {
void *audio_ptr;/*** Call from i915 driver, notifying the HDA driver that* pin sense and/or ELD information has changed.* @audio_ptr: HDA driver object* @port: Which port has changed (PORTA / PORTB / PORTC etc)* @port_mst_index: Index within that port, for DisplayPort multistreaming*/void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);
In the HDMI sample rate notification series [1] I explicitly asked Libin to drop a similar port_mst_index parameter since we don't use it yet, and we are not sure what to stick there yet. Indeed not having it now enforces us to change the API and check for each call site when enabling MST audio.
Other than that,
Reviewed-by: Jani Nikula jani.nikula@intel.com
[1] http://mid.gmane.org/1439880714-40931-1-git-send-email-libin.yang@intel.com
- } *audio_ops;
};
#endif /* _I915_COMPONENT_H_ */
1.9.1
When the audio codec is enabled or disabled, notify the audio driver. This will enable the audio driver to get the notification at all times (even when audio is in different powersave states).
Signed-off-by: David Henningsson david.henningsson@canonical.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fd1de45..1fc327d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1809,6 +1809,7 @@ struct drm_i915_private { struct drm_property *force_audio_property;
/* hda/i915 audio component */ + struct i915_audio_component *audio_component; bool audio_component_registered;
uint32_t hw_context_size; diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 3da9b84..969835d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) struct drm_connector *connector; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_audio_component *acomp = dev_priv->audio_component; + struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); + enum port port = intel_dig_port->port;
connector = drm_select_eld(encoder, mode); if (!connector) @@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, mode); + + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0); }
/** @@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) * The disable sequences must be performed before disabling the transcoder or * port. */ -void intel_audio_codec_disable(struct intel_encoder *encoder) +void intel_audio_codec_disable(struct intel_encoder *intel_encoder) { - struct drm_device *dev = encoder->base.dev; + struct drm_encoder *encoder = &intel_encoder->base; + struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; + struct i915_audio_component *acomp = dev_priv->audio_component; + struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder); + enum port port = intel_dig_port->port;
if (dev_priv->display.audio_codec_disable) - dev_priv->display.audio_codec_disable(encoder); + dev_priv->display.audio_codec_disable(intel_encoder); + + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify) + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0); }
/** @@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data; + struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp->ops || acomp->dev)) return -EEXIST;
acomp->ops = &i915_audio_component_ops; acomp->dev = i915_dev; + dev_priv->audio_component = acomp;
return 0; } @@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data; + struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp->ops = NULL; acomp->dev = NULL; + dev_priv->audio_component = NULL; }
static const struct component_ops i915_audio_component_bind_ops = {
On Wed, 19 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
When the audio codec is enabled or disabled, notify the audio driver. This will enable the audio driver to get the notification at all times (even when audio is in different powersave states).
Signed-off-by: David Henningsson david.henningsson@canonical.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fd1de45..1fc327d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1809,6 +1809,7 @@ struct drm_i915_private { struct drm_property *force_audio_property;
/* hda/i915 audio component */
struct i915_audio_component *audio_component; bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 3da9b84..969835d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) struct drm_connector *connector; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_audio_component *acomp = dev_priv->audio_component;
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
enum port port = intel_dig_port->port;
connector = drm_select_eld(encoder, mode); if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
- if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);}
/** @@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
- The disable sequences must be performed before disabling the transcoder or
- port.
*/ -void intel_audio_codec_disable(struct intel_encoder *encoder) +void intel_audio_codec_disable(struct intel_encoder *intel_encoder) {
- struct drm_device *dev = encoder->base.dev;
struct drm_encoder *encoder = &intel_encoder->base;
struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_audio_component *acomp = dev_priv->audio_component;
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
enum port port = intel_dig_port->port;
if (dev_priv->display.audio_codec_disable)
dev_priv->display.audio_codec_disable(encoder);
dev_priv->display.audio_codec_disable(intel_encoder);- if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);
AFAICT this has a race with i915_audio_component_unbind, i.e. modeset while hda is being unloaded might crash.
What do others think, is drm_modeset_lock_all in combonent bind/unbind overkill?
With that resolved one way or another (I'm not dismissing "we don't need to care") this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
}
/** @@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data;
struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp->ops || acomp->dev)) return -EEXIST;
acomp->ops = &i915_audio_component_ops; acomp->dev = i915_dev;
dev_priv->audio_component = acomp;
return 0;
} @@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data;
struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp->ops = NULL; acomp->dev = NULL;
dev_priv->audio_component = NULL;
}
static const struct component_ops i915_audio_component_bind_ops = {
1.9.1
On 2015-08-28 15:07, Jani Nikula wrote:
On Wed, 19 Aug 2015, David Henningsson david.henningsson@canonical.com wrote:
When the audio codec is enabled or disabled, notify the audio driver. This will enable the audio driver to get the notification at all times (even when audio is in different powersave states).
Signed-off-by: David Henningsson david.henningsson@canonical.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index fd1de45..1fc327d 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1809,6 +1809,7 @@ struct drm_i915_private { struct drm_property *force_audio_property;
/* hda/i915 audio component */
struct i915_audio_component *audio_component; bool audio_component_registered;
uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c index 3da9b84..969835d 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder) struct drm_connector *connector; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_audio_component *acomp = dev_priv->audio_component;
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
enum port port = intel_dig_port->port;
connector = drm_select_eld(encoder, mode); if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
if (dev_priv->display.audio_codec_enable) dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);}
/**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
- The disable sequences must be performed before disabling the transcoder or
- port.
*/ -void intel_audio_codec_disable(struct intel_encoder *encoder) +void intel_audio_codec_disable(struct intel_encoder *intel_encoder) {
- struct drm_device *dev = encoder->base.dev;
struct drm_encoder *encoder = &intel_encoder->base;
struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private;
struct i915_audio_component *acomp = dev_priv->audio_component;
struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
enum port port = intel_dig_port->port;
if (dev_priv->display.audio_codec_disable)
dev_priv->display.audio_codec_disable(encoder);
dev_priv->display.audio_codec_disable(intel_encoder);- if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);AFAICT this has a race with i915_audio_component_unbind, i.e. modeset while hda is being unloaded might crash.
What do others think, is drm_modeset_lock_all in combonent bind/unbind overkill?
Thanks for the review(s). I'm happy to add a drm_modeset_lock_all/drm_modeset_unlock_all - I'm not that familiar with the graphics side, really, so, is there another option...?
With that resolved one way or another (I'm not dismissing "we don't need to care") this is
Reviewed-by: Jani Nikula jani.nikula@intel.com
}
/** @@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data;
struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
if (WARN_ON(acomp->ops || acomp->dev)) return -EEXIST;
acomp->ops = &i915_audio_component_ops; acomp->dev = i915_dev;
dev_priv->audio_component = acomp;
return 0; }
@@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device *i915_dev, struct device *hda_dev, void *data) { struct i915_audio_component *acomp = data;
struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
acomp->ops = NULL; acomp->dev = NULL;
dev_priv->audio_component = NULL; }
static const struct component_ops i915_audio_component_bind_ops = {
-- 1.9.1
This lets the interested codec be notified when an i915 pin/ELD event happens.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- include/sound/hda_i915.h | 7 +++++++ sound/hda/hdac_i915.c | 10 ++++++++++ 2 files changed, 17 insertions(+)
diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h index adb5ba5..a0a1d67 100644 --- a/include/sound/hda_i915.h +++ b/include/sound/hda_i915.h @@ -4,12 +4,15 @@ #ifndef __SOUND_HDA_I915_H #define __SOUND_HDA_I915_H
+#include <drm/i915_component.h> + #ifdef CONFIG_SND_HDA_I915 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable); int snd_hdac_display_power(struct hdac_bus *bus, bool enable); int snd_hdac_get_display_clk(struct hdac_bus *bus); int snd_hdac_i915_init(struct hdac_bus *bus); int snd_hdac_i915_exit(struct hdac_bus *bus); +int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *); #else static int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable) { @@ -31,6 +34,10 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus) { return 0; } +static inline int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *) +{ + return -ENODEV; +} #endif
#endif /* __SOUND_HDA_I915_H */ diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 5676b84..55c3df4 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -134,6 +134,16 @@ static int hdac_component_master_match(struct device *dev, void *data) return !strcmp(dev->driver->name, "i915"); }
+int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *aops) +{ + if (WARN_ON(!hdac_acomp)) + return -ENODEV; + + hdac_acomp->audio_ops = aops; + return 0; +} +EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier); + int snd_hdac_i915_init(struct hdac_bus *bus) { struct component_match *match = NULL;
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback; + + /* i915/powerwell (Haswell+/Valleyview+) specific */ + struct i915_audio_component_audio_ops i915_audio_ops; };
@@ -2191,6 +2196,9 @@ 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)) + snd_hdac_i915_register_notifier(NULL); + for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); }
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{ + struct hda_codec *codec = audio_ptr; + int pin_nid = port + 0x04; + + check_presence_and_report(codec, pin_nid); +} + static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec; @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec)) + if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0; + spec->i915_audio_ops.audio_ptr = codec; + spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify; + snd_hdac_i915_register_notifier(&spec->i915_audio_ops); + }
if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL;
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback;
- /* i915/powerwell (Haswell+/Valleyview+) specific */
- struct i915_audio_component_audio_ops i915_audio_ops;
};
@@ -2191,6 +2196,9 @@ 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))
snd_hdac_i915_register_notifier(NULL);- for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); }
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{
- struct hda_codec *codec = audio_ptr;
- int pin_nid = port + 0x04;
- check_presence_and_report(codec, pin_nid);
+}
static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec; @@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0;
spec->i915_audio_ops.audio_ptr = codec;spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;snd_hdac_i915_register_notifier(&spec->i915_audio_ops);}
if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL;
-- 1.9.1
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback;
- /* i915/powerwell (Haswell+/Valleyview+) specific */
- struct i915_audio_component_audio_ops i915_audio_ops; };
@@ -2191,6 +2196,9 @@ 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))
snd_hdac_i915_register_notifier(NULL);- for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); }
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{
- struct hda_codec *codec = audio_ptr;
- int pin_nid = port + 0x04;
- check_presence_and_report(codec, pin_nid);
+}
- static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0;
spec->i915_audio_ops.audio_ptr = codec;spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;snd_hdac_i915_register_notifier(&spec->i915_audio_ops);}
if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL;
-- 1.9.1
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
In anyway, Reviewed-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback;
- /* i915/powerwell (Haswell+/Valleyview+) specific */
- struct i915_audio_component_audio_ops i915_audio_ops; };
@@ -2191,6 +2196,9 @@ 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))
snd_hdac_i915_register_notifier(NULL);- for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); }
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{
- struct hda_codec *codec = audio_ptr;
- int pin_nid = port + 0x04;
- check_presence_and_report(codec, pin_nid);
+}
- static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0;
spec->i915_audio_ops.audio_ptr = codec;spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;snd_hdac_i915_register_notifier(&spec->i915_audio_ops);}
if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL;
-- 1.9.1
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On Thu, 20 Aug 2015, Takashi Iwai tiwai@suse.de wrote:
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*.
BR, Jani.
In anyway, Reviewed-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
thanks,
Takashi
sound/pci/hda/patch_hdmi.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index a97db5f..932292c 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -37,6 +37,8 @@ #include <sound/jack.h> #include <sound/asoundef.h> #include <sound/tlv.h> +#include <sound/hdaudio.h> +#include <sound/hda_i915.h> #include "hda_codec.h" #include "hda_local.h" #include "hda_jack.h" @@ -144,6 +146,9 @@ struct hdmi_spec { */ struct hda_multi_out multiout; struct hda_pcm_stream pcm_playback;
- /* i915/powerwell (Haswell+/Valleyview+) specific */
- struct i915_audio_component_audio_ops i915_audio_ops; };
@@ -2191,6 +2196,9 @@ 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))
snd_hdac_i915_register_notifier(NULL);- for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
@@ -2316,6 +2324,14 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg, snd_hda_codec_set_power_to_all(codec, fg, power_state); }
+static void intel_pin_eld_notify(void *audio_ptr, int port, int port_mst_index) +{
- struct hda_codec *codec = audio_ptr;
- int pin_nid = port + 0x04;
- check_presence_and_report(codec, pin_nid);
+}
- static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_valleyview_plus(codec) || is_skylake(codec)) codec->core.link_power_control = 1;
- if (is_haswell_plus(codec) || is_valleyview_plus(codec))
if (is_haswell_plus(codec) || is_valleyview_plus(codec)) { codec->depop_delay = 0;
spec->i915_audio_ops.audio_ptr = codec;spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;snd_hdac_i915_register_notifier(&spec->i915_audio_ops);}
if (hdmi_parse_codec(codec) < 0) { codec->spec = NULL;
-- 1.9.1
-- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic
On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
On Thu, 20 Aug 2015, Takashi Iwai tiwai@suse.de wrote:
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*.
The only bit I want is some decent kerneldoc for the ad-hoc growing i915/hda-intel interfaces. But that can be done in follow-up patches and needs to be coordinated with the audio rate handling series anyway. So ack from my side for all getting merged through alsa trees too. -Daniel
On Wed, 02 Sep 2015 10:00:44 +0200, Daniel Vetter wrote:
On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
On Thu, 20 Aug 2015, Takashi Iwai tiwai@suse.de wrote:
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote:
Whenever there is an event from the i915 driver, wake the codec and recheck plug/unplug + ELD status.
This fixes the issue with lost unsol events in power save mode, the codec and controller can now sleep in D3 and still know when the HDMI monitor has been connected.
Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*.
The only bit I want is some decent kerneldoc for the ad-hoc growing i915/hda-intel interfaces. But that can be done in follow-up patches and needs to be coordinated with the audio rate handling series anyway. So ack from my side for all getting merged through alsa trees too.
Are you going to merge Libin's patchset? If yes, it's better to align both patchsets through a single tree. It'll make merges easier, as both touch the similar place.
thanks,
Takashi
On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 10:00:44 +0200, Daniel Vetter wrote:
On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
On Thu, 20 Aug 2015, Takashi Iwai tiwai@suse.de wrote:
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote:
On Wed, 19 Aug 2015 10:48:58 +0200, David Henningsson wrote: > > Whenever there is an event from the i915 driver, wake the codec > and recheck plug/unplug + ELD status. > > This fixes the issue with lost unsol events in power save mode, > the codec and controller can now sleep in D3 and still know when > the HDMI monitor has been connected. > > Signed-off-by: David Henningsson david.henningsson@canonical.com
This addition looks fine, but then we'll get double notification for the normal hotplug/unplug, one via component ops and another via unsol event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*.
The only bit I want is some decent kerneldoc for the ad-hoc growing i915/hda-intel interfaces. But that can be done in follow-up patches and needs to be coordinated with the audio rate handling series anyway. So ack from my side for all getting merged through alsa trees too.
Are you going to merge Libin's patchset? If yes, it's better to align both patchsets through a single tree. It'll make merges easier, as both touch the similar place.
Oh that was an ack for merging it all through your tree. If you punt it to 4.4 I might ask you for a stable tree to pull in in case I get conflicts. -Daniel
On Wed, 02 Sep 2015 10:32:34 +0200, Daniel Vetter wrote:
On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
On Wed, 02 Sep 2015 10:00:44 +0200, Daniel Vetter wrote:
On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
On Thu, 20 Aug 2015, Takashi Iwai tiwai@suse.de wrote:
On Thu, 20 Aug 2015 11:41:42 +0200, David Henningsson wrote:
On 2015-08-20 11:28, Takashi Iwai wrote: > On Wed, 19 Aug 2015 10:48:58 +0200, > David Henningsson wrote: >> >> Whenever there is an event from the i915 driver, wake the codec >> and recheck plug/unplug + ELD status. >> >> This fixes the issue with lost unsol events in power save mode, >> the codec and controller can now sleep in D3 and still know when >> the HDMI monitor has been connected. >> >> Signed-off-by: David Henningsson david.henningsson@canonical.com > > This addition looks fine, but then we'll get double notification for > the normal hotplug/unplug, one via component ops and another via unsol > event?
Right, in case the unsol event actually works...
I would argue that the normal case would be that the controller and codec is in D3 which means that the unsol event never gets through - due to hw limitations - which is what triggered this patch set in the first place.
But yes, in some case we might get double notification, but this should not cause any trouble in practice. The unsol event could be turned off, but would it be okay to save that for a later patch set (so I don't miss the upcoming merge window)?
In that case, it should be mentioned in the changelog at least.
This series came a bit too late for the merge window, so I'm not sure whether this can get in. I personally find it OK, so take my ack for ALSA parts (patch 3/4), but the rest need review and ack from i915 guys. And we don't know who to merge this, if any. The changes are almost even to i915 and hda. I don't mind either way, via drm or sound tree.
Personally I'm fine with this still going for v4.3 since to me it looks like any regressions this might cause will be on your side. *grin*.
The only bit I want is some decent kerneldoc for the ad-hoc growing i915/hda-intel interfaces. But that can be done in follow-up patches and needs to be coordinated with the audio rate handling series anyway. So ack from my side for all getting merged through alsa trees too.
Are you going to merge Libin's patchset? If yes, it's better to align both patchsets through a single tree. It'll make merges easier, as both touch the similar place.
Oh that was an ack for merging it all through your tree. If you punt it to 4.4 I might ask you for a stable tree to pull in in case I get conflicts.
OK, then I'll merge David's patchset now for the upcoming pull request for 4.3-rc1.
thanks,
Takashi
participants (4)
-
Daniel Vetter -
David Henningsson -
Jani Nikula -
Takashi Iwai