[alsa-devel] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
This patch set aims to resolve three problems:
* The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
* Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback.
* Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too.
The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem.
The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine.
David Henningsson (4): drm/i915: Add audio hotplug info struct drm/i915: Call audio hotplug notify function ALSA: hda - Dispatch incoming HDMI hotplug i915 callback ALSA: hda - Wake the codec up on hotplug notify events
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 46 ++++++++++++++++++++++++++++++++++++ include/drm/i915_component.h | 19 +++++++++++++++ include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 +++++++++++++++++++ sound/pci/hda/patch_hdmi.c | 22 ++++++++++++++++- 6 files changed, 115 insertions(+), 1 deletion(-)
This struct will be used to transfer information from the i915 driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- include/drm/i915_component.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..4fc0db3 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -24,8 +24,22 @@ #ifndef _I915_COMPONENT_H_ #define _I915_COMPONENT_H_
+struct hdac_bus; + +struct i915_audio_hotplug_info { + int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */ + int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */ + int port; /* Used for mapping to affected nid */ + int port_multi_stream_device; /* For DP multi-streaming */ + + bool plugged_in; + uint8_t *eld; + int eld_size; +}; + struct i915_audio_component { struct device *dev; + struct hdac_bus *hdac_bus;
const struct i915_audio_component_ops { struct module *owner; @@ -34,6 +48,11 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops; + + const struct i915_audio_component_cb_ops { + struct module *owner; + void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *); + } *cb_ops; };
#endif /* _I915_COMPONENT_H_ */
On Tue, 21 Jul 2015 09:57:24 +0200, David Henningsson wrote:
This struct will be used to transfer information from the i915 driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.henningsson@canonical.com
Looks good to me, just a few nitpicking:
include/drm/i915_component.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..4fc0db3 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -24,8 +24,22 @@ #ifndef _I915_COMPONENT_H_ #define _I915_COMPONENT_H_
+struct hdac_bus;
+struct i915_audio_hotplug_info {
- int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
- int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
- int port; /* Used for mapping to affected nid */
- int port_multi_stream_device; /* For DP multi-streaming */
- bool plugged_in;
- uint8_t *eld;
Use u8 or just unsigned char as it's a in-kernel API. Also, safer to add const, since this is read-only for audio side.
- int eld_size;
+};
struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
const struct i915_audio_component_ops { struct module *owner; @@ -34,6 +48,11 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_cb_ops {
struct module *owner;
Do we need the owner field at all?
void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);- } *cb_ops;
cb_ops doesn't sound intuitive. Any better name?
thanks,
Takashi
On 2015-07-22 10:22, Takashi Iwai wrote:
On Tue, 21 Jul 2015 09:57:24 +0200, David Henningsson wrote:
This struct will be used to transfer information from the i915 driver to the hda driver on HDMI hotplug events.
Signed-off-by: David Henningsson david.henningsson@canonical.com
Looks good to me, just a few nitpicking:
include/drm/i915_component.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h index c9a8b64..4fc0db3 100644 --- a/include/drm/i915_component.h +++ b/include/drm/i915_component.h @@ -24,8 +24,22 @@ #ifndef _I915_COMPONENT_H_ #define _I915_COMPONENT_H_
+struct hdac_bus;
+struct i915_audio_hotplug_info {
- int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
- int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
- int port; /* Used for mapping to affected nid */
- int port_multi_stream_device; /* For DP multi-streaming */
- bool plugged_in;
- uint8_t *eld;
Use u8 or just unsigned char as it's a in-kernel API. Also, safer to add const, since this is read-only for audio side.
Ok.
- int eld_size;
+};
- struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
const struct i915_audio_component_ops { struct module *owner; @@ -34,6 +48,11 @@ struct i915_audio_component { void (*codec_wake_override)(struct device *, bool enable); int (*get_cdclk_freq)(struct device *); } *ops;
- const struct i915_audio_component_cb_ops {
struct module *owner;Do we need the owner field at all?
It was merely for symmetry. I'll remove it for v2.
void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);- } *cb_ops;
cb_ops doesn't sound intuitive. Any better name?
I was thinking of it as "callback ops", i e, calls that go in the reverse direction compared to the already existing "ops".
But if we call the device "audio_dev" as you suggested above, then maybe "audio_ops" would be nice and symmetric?
On Wed, 22 Jul 2015 10:50:03 +0200, David Henningsson wrote:
struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea, then.
void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);- } *cb_ops;
cb_ops doesn't sound intuitive. Any better name?
I was thinking of it as "callback ops", i e, calls that go in the reverse direction compared to the already existing "ops".
But if we call the device "audio_dev" as you suggested above, then maybe "audio_ops" would be nice and symmetric?
Yes, it sounds better.
Takashi
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200, David Henningsson wrote:
struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea, then.
That depends on the device we register this with. Actually this makes more sense to me :)
If we register with struct device *audio_dev, which in this case would be the codec device we create while probing the bus. This way you are linking i915 ops to the codec device. Ofcourse hdac_device has bus pointer but you can invoke device callback without even searching for the device :)
On 2015-07-22 16:13, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200, David Henningsson wrote:
struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea, then.
That depends on the device we register this with. Actually this makes more sense to me :)
If we register with struct device *audio_dev, which in this case would be the codec device we create while probing the bus. This way you are linking i915 ops to the codec device. Ofcourse hdac_device has bus pointer but you can invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to turn the power well on. And in order to connect the codec to the i915_audio_component, we need to have detected a codec.
Which, now that I think of it, actually gives an interesting potential race condition, in case the following happens:
1) Monitor is plugged in at boot time 2) i915 initializes 3) hda starts initializing and sets up the audio component 4) i915 finishes initialization and as part of that, calls the hotplug notify to let hda know that the monitor is plugged in. However, at this point, hda has not finished initialization yet, so there are no codecs that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever decide to not write the ELD to the hardware.
On Wed, 22 Jul 2015 19:52:23 +0200, David Henningsson wrote:
On 2015-07-22 16:13, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200, David Henningsson wrote:
struct i915_audio_component { struct device *dev;
- struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea, then.
That depends on the device we register this with. Actually this makes more sense to me :)
If we register with struct device *audio_dev, which in this case would be the codec device we create while probing the bus. This way you are linking i915 ops to the codec device. Ofcourse hdac_device has bus pointer but you can invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to turn the power well on. And in order to connect the codec to the i915_audio_component, we need to have detected a codec.
Which, now that I think of it, actually gives an interesting potential race condition, in case the following happens:
- Monitor is plugged in at boot time
- i915 initializes
- hda starts initializing and sets up the audio component
- i915 finishes initialization and as part of that, calls the hotplug
notify to let hda know that the monitor is plugged in. However, at this point, hda has not finished initialization yet, so there are no codecs that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever decide to not write the ELD to the hardware.
For avoid such a problem, maybe we need two ops, one for notification and one for getting the assigned data. At the initialization time, the audio driver queries the assigned status and data. When a hotplug happens, it's just notified. That is, it simply replaces the current unsol event and the ELD data read via two ops.
Takashi
On Wed, Jul 22, 2015 at 10:31:45PM +0200, Takashi Iwai wrote:
That depends on the device we register this with. Actually this makes more sense to me :)
If we register with struct device *audio_dev, which in this case would be the codec device we create while probing the bus. This way you are linking i915 ops to the codec device. Ofcourse hdac_device has bus pointer but you can invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to turn the power well on. And in order to connect the codec to the i915_audio_component, we need to have detected a codec.
Yes that is true but when driver registers the callback you can assign the callback to i915 component, so afterwards the call from i915 lands up in codec. If not registered we have default bus handler :)
Which, now that I think of it, actually gives an interesting potential race condition, in case the following happens:
- Monitor is plugged in at boot time
- i915 initializes
- hda starts initializing and sets up the audio component
- i915 finishes initialization and as part of that, calls the hotplug
notify to let hda know that the monitor is plugged in. However, at this point, hda has not finished initialization yet, so there are no codecs that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever decide to not write the ELD to the hardware.
For avoid such a problem, maybe we need two ops, one for notification and one for getting the assigned data. At the initialization time, the audio driver queries the assigned status and data. When a hotplug happens, it's just notified. That is, it simply replaces the current unsol event and the ELD data read via two ops.
Yeah, I do favour adding new ops so that we can query the current values and setup whenever driver probes
On 2015-07-22 22:31, Takashi Iwai wrote:
On Wed, 22 Jul 2015 19:52:23 +0200, David Henningsson wrote:
On 2015-07-22 16:13, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
On Wed, 22 Jul 2015 10:50:03 +0200, David Henningsson wrote:
> struct i915_audio_component { > struct device *dev; > + struct hdac_bus *hdac_bus;
If we want to be more generic, using a struct device would be better, e.g. struct device *audio_dev;
Does this work? If we want to have the hdac_bus.dev ptr instead of a hdac_bus ptr, there does not seem to be an obvious way to go from the audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an arbitrary dev pointer).
Hrm, right, currently it's not straightforward. Scratch the idea, then.
That depends on the device we register this with. Actually this makes more sense to me :)
If we register with struct device *audio_dev, which in this case would be the codec device we create while probing the bus. This way you are linking i915 ops to the codec device. Ofcourse hdac_device has bus pointer but you can invoke device callback without even searching for the device :)
It would require some extra setup, so I skipped it (at least for now).
I e, in order to detect codecs, we need to call hdac_i915 functions to turn the power well on. And in order to connect the codec to the i915_audio_component, we need to have detected a codec.
Which, now that I think of it, actually gives an interesting potential race condition, in case the following happens:
- Monitor is plugged in at boot time
- i915 initializes
- hda starts initializing and sets up the audio component
- i915 finishes initialization and as part of that, calls the hotplug
notify to let hda know that the monitor is plugged in. However, at this point, hda has not finished initialization yet, so there are no codecs that listen for this information.
Anyhow, this is not a problem really yet, but it might be if we ever decide to not write the ELD to the hardware.
For avoid such a problem, maybe we need two ops, one for notification and one for getting the assigned data. At the initialization time, the audio driver queries the assigned status and data. When a hotplug happens, it's just notified. That is, it simply replaces the current unsol event and the ELD data read via two ops.
I'm about to go on vacation so it would be good to get some closure here. If you both prefer this setup, how about I remove "struct i915_audio_hotplug_info" for now? We will then have:
const struct i915_audio_component_audio_ops { void (*hotplug_notify)(struct hdac_bus *); }
...which will make the hda driver read from the hardware. Asking the i915 driver for ELD, connector and hotplug status can then be done in a later patch.
On 2015-07-23 08:17, David Henningsson wrote:
I'm about to go on vacation so it would be good to get some closure here. If you both prefer this setup, how about I remove "struct i915_audio_hotplug_info" for now? We will then have:
const struct i915_audio_component_audio_ops { void (*hotplug_notify)(struct hdac_bus *); }
Sorry, it would look like this:
const struct i915_audio_component_audio_ops { void (*hotplug_notify)(struct hdac_bus *, int port, int port_mst_index); }
...to indicate what port needs updating.
On Thu, 23 Jul 2015 08:25:21 +0200, David Henningsson wrote:
On 2015-07-23 08:17, David Henningsson wrote:
I'm about to go on vacation so it would be good to get some closure here. If you both prefer this setup, how about I remove "struct i915_audio_hotplug_info" for now? We will then have:
const struct i915_audio_component_audio_ops { void (*hotplug_notify)(struct hdac_bus *); }Sorry, it would look like this:
const struct i915_audio_component_audio_ops { void (*hotplug_notify)(struct hdac_bus *, int port, intport_mst_index); }
...to indicate what port needs updating.
Yes, I think this is simpler.
A remaining question is whether it should be notified to bus or codec. In the latter case, we need to allow registering the audio codec after binding the component.
I find the codec can be a bit better, as this is directly targeted (while for bus you need to look through the codec list). But it's just a minor difference, and I don't mind so much about this, if there is any other difficulty by that move.
thanks,
Takashi
On HDMI hotplug events, notify the audio driver. This will enable the audio driver to get the information at all times (even when audio is in different powersave states), and also without reading it from the hardware.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 542fac6..696624c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1808,6 +1808,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..eb29e1f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector, I915_WRITE(aud_config, tmp); }
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv, + struct drm_connector *connector, + struct intel_encoder *encoder, + bool plugged_in) +{ + struct i915_audio_component *acomp = dev_priv->audio_component; + struct i915_audio_hotplug_info audio_info; + struct intel_digital_port *intel_dig_port = + enc_to_dig_port(&encoder->base); + enum port port = intel_dig_port->port; + + if (!acomp || !acomp->cb_ops || !acomp->cb_ops->hotplug_notify) + return; + + if (connector) { + audio_info.connector_type = connector->connector_type; + audio_info.connector_type_id = connector->connector_type_id; + } else { + audio_info.connector_type = -1; + audio_info.connector_type_id = -1; + } + + audio_info.port = (int) port; + /* DP Mst is unsupported for now */ + audio_info.port_multi_stream_device = 0; + + audio_info.plugged_in = plugged_in; + if (plugged_in && connector) { + audio_info.eld = connector->eld; + audio_info.eld_size = drm_eld_size(audio_info.eld); + } else { + audio_info.eld = NULL; + audio_info.eld_size = 0; + } + + acomp->cb_ops->hotplug_notify(acomp->hdac_bus, &audio_info); +} + /** * intel_audio_codec_enable - Enable the audio codec for HD audio * @intel_encoder: encoder on which to enable audio @@ -419,6 +457,8 @@ 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); + + audio_hotplug_notify(dev_priv, connector, intel_encoder, true); }
/** @@ -435,6 +475,8 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(encoder); + + audio_hotplug_notify(dev_priv, NULL, encoder, false); }
/** @@ -525,12 +567,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 +583,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 Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
On HDMI hotplug events, notify the audio driver. This will enable the audio driver to get the information at all times (even when audio is in different powersave states), and also without reading it from the hardware.
Signed-off-by: David Henningsson david.henningsson@canonical.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 542fac6..696624c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1808,6 +1808,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..eb29e1f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector, I915_WRITE(aud_config, tmp); }
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
struct drm_connector *connector,struct intel_encoder *encoder,bool plugged_in)
plugged_in is redundant, just check for NULL. Also just derive dev_priv from intel_encoder imo. And finally we tend to put the object that a function operates on first. Since this sends a notify out for the given encoder (well dig port really) I'd put that first.
I also think that it would make sense to switch all the audio enable/disable functions from intel_encoder to intel_dig_port. A lot need to do upcasts anyway, and audio really only works on dig ports (which is for hdmi/dp only). -Daniel
+{
- struct i915_audio_component *acomp = dev_priv->audio_component;
- struct i915_audio_hotplug_info audio_info;
- struct intel_digital_port *intel_dig_port =
enc_to_dig_port(&encoder->base);- enum port port = intel_dig_port->port;
- if (!acomp || !acomp->cb_ops || !acomp->cb_ops->hotplug_notify)
return;- if (connector) {
audio_info.connector_type = connector->connector_type;audio_info.connector_type_id = connector->connector_type_id;- } else {
audio_info.connector_type = -1;audio_info.connector_type_id = -1;- }
- audio_info.port = (int) port;
- /* DP Mst is unsupported for now */
- audio_info.port_multi_stream_device = 0;
- audio_info.plugged_in = plugged_in;
- if (plugged_in && connector) {
audio_info.eld = connector->eld;audio_info.eld_size = drm_eld_size(audio_info.eld);- } else {
audio_info.eld = NULL;audio_info.eld_size = 0;- }
- acomp->cb_ops->hotplug_notify(acomp->hdac_bus, &audio_info);
+}
/**
- intel_audio_codec_enable - Enable the audio codec for HD audio
- @intel_encoder: encoder on which to enable audio
@@ -419,6 +457,8 @@ 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);
- audio_hotplug_notify(dev_priv, connector, intel_encoder, true);
}
/** @@ -435,6 +475,8 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
if (dev_priv->display.audio_codec_disable) dev_priv->display.audio_codec_disable(encoder);
- audio_hotplug_notify(dev_priv, NULL, encoder, false);
}
/** @@ -525,12 +567,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 +583,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.7.9.5
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 2015-07-21 11:14, Daniel Vetter wrote:
On Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
On HDMI hotplug events, notify the audio driver. This will enable the audio driver to get the information at all times (even when audio is in different powersave states), and also without reading it from the hardware.
Signed-off-by: David Henningsson david.henningsson@canonical.com
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 542fac6..696624c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1808,6 +1808,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..eb29e1f 100644 --- a/drivers/gpu/drm/i915/intel_audio.c +++ b/drivers/gpu/drm/i915/intel_audio.c @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector, I915_WRITE(aud_config, tmp); }
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
struct drm_connector *connector,struct intel_encoder *encoder,bool plugged_in)plugged_in is redundant, just check for NULL. Also just derive dev_priv from intel_encoder imo. And finally we tend to put the object that a function operates on first. Since this sends a notify out for the given encoder (well dig port really) I'd put that first.
Sure, will fix these and send out a v2 (after Takashi has reviewed, too).
I also think that it would make sense to switch all the audio enable/disable functions from intel_encoder to intel_dig_port. A lot need to do upcasts anyway, and audio really only works on dig ports (which is for hdmi/dp only).
That's up to you - seems a bit out of scope for this patch set though, I assume.
Btw, there are two open questions though that you (or someone else) might be able to share some insight on:
1) The mapping between ports and nid is fixed in patch 4/4, it would be good with an Ack from an Intel engineer on this mapping.
2) Whether to use connector_type / connector_type_id combo or connector_name as identification - it seems like X has a different naming compared to the kernel ("HDMI2" in X vs "HDMI-A-2" in kernel), not sure what Wayland/Mir/others do here...
This lets interested codec(s) be notified of HDMI hotplug events sent from the i915 driver.
Signed-off-by: David Henningsson david.henningsson@canonical.com --- include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 4caf1fd..ce34182 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -79,6 +79,10 @@ struct hdac_device { int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, unsigned int *res);
+ /* Used for hotplug notification from i915 driver */ + void (*hotplug_notify)(struct hdac_device *, + const struct i915_audio_hotplug_info *); + /* widgets */ unsigned int num_nodes; hda_nid_t start_nid, end_nid; diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c index 5676b84..134ef9c 100644 --- a/sound/hda/hdac_i915.c +++ b/sound/hda/hdac_i915.c @@ -118,6 +118,8 @@ static void hdac_component_master_unbind(struct device *dev) { struct i915_audio_component *acomp = hdac_acomp;
+ acomp->cb_ops = NULL; + acomp->hdac_bus = NULL; module_put(acomp->ops->owner); component_unbind_all(dev, acomp); WARN_ON(acomp->ops || acomp->dev); @@ -128,6 +130,25 @@ static const struct component_master_ops hdac_component_master_ops = { .unbind = hdac_component_master_unbind, };
+static void i915_audio_component_hotplug_notify(struct hdac_bus *bus, + const struct i915_audio_hotplug_info *info) +{ + struct hdac_device *d; + + dev_dbg(bus->dev, "Received HDMI hotplug callback (connector = %d-%d, plugged in = %d)", + info->connector_type, info->connector_type_id, (int) info->plugged_in); + + list_for_each_entry(d, &bus->codec_list, list) { + if (d->hotplug_notify) + d->hotplug_notify(d, info); + } +} + +static const struct i915_audio_component_cb_ops i915_audio_component_cb_ops = { + .owner = THIS_MODULE, + .hotplug_notify = i915_audio_component_hotplug_notify, +}; + static int hdac_component_master_match(struct device *dev, void *data) { /* i915 is the only supported component */ @@ -163,6 +184,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus) ret = -ENODEV; goto out_master_del; } + acomp->cb_ops = &i915_audio_component_cb_ops; + acomp->hdac_bus = bus; + dev_dbg(dev, "bound to i915 component master\n");
return 0;
On Tue, 21 Jul 2015 09:57:26 +0200, David Henningsson wrote:
This lets interested codec(s) be notified of HDMI hotplug events sent from the i915 driver.
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 4caf1fd..ce34182 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -79,6 +79,10 @@ struct hdac_device { int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, unsigned int *res);
- /* Used for hotplug notification from i915 driver */
- void (*hotplug_notify)(struct hdac_device *,
const struct i915_audio_hotplug_info *);
Since this callback is specific to HDMI/DP, a more specific name would be better. Otherwise this sounds like a generic hotplug handler.
Or, we may make it really generic, e.g. something like void (*hotplug_notify)(struct hdac_device *, int event_type, const void *data);
and call it with a specific event_type value codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
thanks,
Takashi
On Wed, Jul 22, 2015 at 10:30:57AM +0200, Takashi Iwai wrote:
On Tue, 21 Jul 2015 09:57:26 +0200, David Henningsson wrote:
This lets interested codec(s) be notified of HDMI hotplug events sent from the i915 driver.
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 4caf1fd..ce34182 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -79,6 +79,10 @@ struct hdac_device { int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, unsigned int *res);
- /* Used for hotplug notification from i915 driver */
- void (*hotplug_notify)(struct hdac_device *,
const struct i915_audio_hotplug_info *);Since this callback is specific to HDMI/DP, a more specific name would be better. Otherwise this sounds like a generic hotplug handler.
Or, we may make it really generic, e.g. something like void (*hotplug_notify)(struct hdac_device *, int event_type, const void *data);
and call it with a specific event_type value codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
or should this be added to unsol_event handler for the device, we already are have worker thread for that. For device it will be single notify for both SW and HW events. Yes adding event types will help
On Wed, 22 Jul 2015 15:56:37 +0200, Vinod Koul wrote:
On Wed, Jul 22, 2015 at 10:30:57AM +0200, Takashi Iwai wrote:
On Tue, 21 Jul 2015 09:57:26 +0200, David Henningsson wrote:
This lets interested codec(s) be notified of HDMI hotplug events sent from the i915 driver.
Signed-off-by: David Henningsson david.henningsson@canonical.com
include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 ++++++++++++++++++++++++ 2 files changed, 28 insertions(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 4caf1fd..ce34182 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -79,6 +79,10 @@ struct hdac_device { int (*exec_verb)(struct hdac_device *dev, unsigned int cmd, unsigned int flags, unsigned int *res);
- /* Used for hotplug notification from i915 driver */
- void (*hotplug_notify)(struct hdac_device *,
const struct i915_audio_hotplug_info *);Since this callback is specific to HDMI/DP, a more specific name would be better. Otherwise this sounds like a generic hotplug handler.
Or, we may make it really generic, e.g. something like void (*hotplug_notify)(struct hdac_device *, int event_type, const void *data);
and call it with a specific event_type value codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
or should this be added to unsol_event handler for the device, we already are have worker thread for that. For device it will be single notify for both SW and HW events. Yes adding event types will help
Well, the difference from the unsol event is that this gives the certain amount of data together with the notification. Of course, it can be a two-step procedure: first notify via a faked unsol event, then the driver fetches data actively.
Takashi
As a first cautious step, we're not going to trust the information coming directly from the i915 driver, we're just going to use the fact that there was an event to wakeup the codec and ask for its 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 2f24338..230d83d 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2316,6 +2316,24 @@ 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_haswell_hotplug_notify(struct hdac_device *dev, + const struct i915_audio_hotplug_info *info) +{ + struct hda_codec *codec = container_of(dev, struct hda_codec, core); + int pin_nid = info->port + 0x04; + + check_presence_and_report(codec, pin_nid); +} + +static void intel_valleyview_hotplug_notify(struct hdac_device *dev, + const struct i915_audio_hotplug_info *info) +{ + struct hda_codec *codec = container_of(dev, struct hda_codec, core); + int pin_nid = 0x03; /* TODO: Ask Intel engineers to verify this */ + + check_presence_and_report(codec, pin_nid); +} + static int patch_generic_hdmi(struct hda_codec *codec) { struct hdmi_spec *spec; @@ -2331,7 +2349,9 @@ static int patch_generic_hdmi(struct hda_codec *codec) if (is_haswell_plus(codec)) { intel_haswell_enable_all_pins(codec, true); intel_haswell_fixup_enable_dp12(codec); - } + codec->core.hotplug_notify = intel_haswell_hotplug_notify; + } else if (is_valleyview_plus(codec)) + codec->core.hotplug_notify = intel_valleyview_hotplug_notify;
/* For Valleyview/Cherryview, only the display codec is in the display * power well and can use link_power ops to request/release the power.
Hi David,
-----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson Sent: Tuesday, July 21, 2015 1:27 PM To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel; jani.nikula@linux.intel.com Cc: Koul, Vinod; David Henningsson Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
This patch set aims to resolve three problems:
The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback.
Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too.
The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem.
The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine.
I believe you tested these patches with hda driver after few cycles of D3. By any chance, did you also try this once after i915 driver's D3 cycle also ? In this case, can the check_presence_and_report() function get the pin presence and ELD valid bits read out properly..?
Thanks, Durga
David Henningsson (4): drm/i915: Add audio hotplug info struct drm/i915: Call audio hotplug notify function ALSA: hda - Dispatch incoming HDMI hotplug i915 callback ALSA: hda - Wake the codec up on hotplug notify events
drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_audio.c | 46 ++++++++++++++++++++++++++++++++++++ include/drm/i915_component.h | 19 +++++++++++++++ include/sound/hdaudio.h | 4 ++++ sound/hda/hdac_i915.c | 24 +++++++++++++++++++ sound/pci/hda/patch_hdmi.c | 22 ++++++++++++++++- 6 files changed, 115 insertions(+), 1 deletion(-)
-- 1.7.9.5
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 21 Jul 2015 19:37:17 +0200, R, Durgadoss wrote:
Hi David,
-----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson Sent: Tuesday, July 21, 2015 1:27 PM To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel; jani.nikula@linux.intel.com Cc: Koul, Vinod; David Henningsson Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
This patch set aims to resolve three problems:
The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback.
Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too.
The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem.
The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine.
I believe you tested these patches with hda driver after few cycles of D3. By any chance, did you also try this once after i915 driver's D3 cycle also ? In this case, can the check_presence_and_report() function get the pin presence and ELD valid bits read out properly..?
The code path will call power well on, thus the i915 driver will be woken up, in anyway.
thanks,
Takashi
On 2015-07-21 19:37, R, Durgadoss wrote:
Hi David,
-----Original Message----- From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson Sent: Tuesday, July 21, 2015 1:27 PM To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel; jani.nikula@linux.intel.com Cc: Koul, Vinod; David Henningsson Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
This patch set aims to resolve three problems:
The first - and most serious one - is that the audio driver is not woken up properly when in power save modes, especially not when the HDA controller is in D3. By having the i915 driver call directly into the hda driver, the HDA driver is always notified that an HDMI hotplug event has happened.
Second, there is currently no way for userspace to match an HDMI audio output with an HDMI video output. We fix this by sending connector_type and connector_type_id in the HDMI hotplug callback.
Third, writing ELD info to the hardware just so the HDA driver can read it from the hardware seems a bit inefficient. We could just pass that information in the callback, too.
The patch in its current form fixes the first of these problems and provides most of the infrastructure for the second and third problem.
The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has been tested (and working) on one Skylake machine.
I believe you tested these patches with hda driver after few cycles of D3. By any chance, did you also try this once after i915 driver's D3 cycle also ? In this case, can the check_presence_and_report() function get the pin presence and ELD valid bits read out properly..?
When running this code with drm.debug=0xe, I can see a lot of calls to set different power wells on and off, to the extent that I don't keep track of them.
So I assume that means that the i915 driver goes into D3 as well during my tests.
participants (5)
-
Daniel Vetter -
David Henningsson -
R, Durgadoss -
Takashi Iwai -
Vinod Koul