[alsa-devel] HDMI hotplug on Skylake when power well is off
Takashi Iwai
tiwai at suse.de
Thu Jul 16 15:57:03 CEST 2015
On Thu, 16 Jul 2015 15:37:07 +0200,
David Henningsson wrote:
>
>
>
> On 2015-07-16 14:21, Takashi Iwai wrote:
> > On Thu, 16 Jul 2015 13:35:14 +0200,
> > Vinod Koul wrote:
> >>
> >> On Thu, Jul 16, 2015 at 12:06:51PM +0200, Takashi Iwai wrote:
> >>> On Thu, 16 Jul 2015 11:34:02 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2015-07-16 10:39, Vinod Koul wrote:
> >>>>> On Wed, Jul 15, 2015 at 12:59:02PM +0200, Takashi Iwai wrote:
> >>>>>> On Wed, 15 Jul 2015 11:05:22 +0200,
> >>>>>> David Henningsson wrote:
> >>>>>>>
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> I'm trying to debug an issue here where the HDMI hotplug events are not
> >>>>>>> delivered to the audio side when the power well is off. This is on a
> >>>>>>> Skylake machine (running in HDA mode).
> >>>>>>>
> >>>>>>> I'm not sure whether the problem is upstream or due to my own patches
> >>>>>>> while testing, so I was wondering how this is supposed to be working, so
> >>>>>>> I can troubleshoot further?
> >>>>>>>
> >>>>>>> Should there be an IRQ on the HDA controller even if the power well is
> >>>>>>> off, and if not, how should the audio driver be notified that an HDMI
> >>>>>>> hotplug event has happened?
> >>>>>>
> >>>>>> I thought this has been always a problem when the runtime PM is
> >>>>>> enabled, no matter whether the power well state is.
> >>>>> Shouldn't the hotplug action turn on the power well? Then notification for
> >>>>> audio side should get propagated as power well is On
> >>>>
> >>>> While the video side can turn the power well on, maybe there are other
> >>>> things that needs to be turned on from the audio driver?
> >>>
> >>> Yes. This was supposed to work with WAKEEN bits, but this didn't work
> >>> as expected, after all.
>
> But this applies only to HDMI codecs and the power well, right? Analog
> controllers could go into D3 and WAKEEN would wake them up? Or did we
> never get that working either?
Well, the analog codec wakeup never worked in my tests. Maybe some
hardware works, who knows.
> Anyhow, I went searching in the video drivers for things to match on,
> inspired by your thoughts about submitting information from the graphics
> driver to the audio driver. See new attached patch (still writing blind
> drafts). Let me know what you think.
Thanks, this looks promising!
> I think "connector->name" seems what the most reasonable thing to export
> to userspace to indicate what gfx output is related to which audio nid,
> and the port being what maps 1-to-1 to a nid - we'll use that for our
> own lookup (this lookup might differ between hardware generations).
Yeah, this mapping is the unresolved mystery. This mythos has to be
revealed by Intel people. Also, the actual work in audio side might
be better in a work struct so that the notifier call won't be blocked
too long.
Once when we get a reliably notification via component ops, we can get
rid of the unsolicited event and generates the jack event manually.
Takashi
>
> --
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8ae6f7f..0eaac41 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -45,6 +45,7 @@
> #include <drm/intel-gtt.h>
> #include <drm/drm_legacy.h> /* for struct drm_dma_handle */
> #include <drm/drm_gem.h>
> +#include <drm/i915_component.h>
> #include <linux/backlight.h>
> #include <linux/hashtable.h>
> #include <linux/intel-iommu.h>
> @@ -1752,6 +1753,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 ef34257..0f30513 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -389,6 +389,37 @@ 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;
> +
> + audio_info.connector_name = connector ? connector->name : NULL;
> + audio_info.port = (int) port;
> + audio_info.multi_stream_device = 0; // TODO
> +
> + audio_info.plugged_in = plugged_in;
> + if (plugged_in && connector) {
> + audio_info.eld = connector->eld;
> + audio_info.eld_size = ELD_MAX_BYTES; // TODO
> + }
> + 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
> @@ -424,6 +455,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, encoder, true);
> }
>
> /**
> @@ -440,6 +473,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);
> }
>
> /**
> @@ -529,12 +564,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;
> }
> @@ -543,9 +580,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 = {
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..7b418f6 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,21 @@
> #ifndef _I915_COMPONENT_H_
> #define _I915_COMPONENT_H_
>
> +struct hdac_bus;
> +
> +struct i915_audio_hotplug_info {
> + const char *connector_name; /* Used to coordinate with gfx out for userspace */
> + int port; /* Used for mapping to affected Nid */
> + int 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 +47,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_ */
> 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 442500e..3abdc2f 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -115,6 +115,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);
> @@ -125,6 +127,24 @@ static const struct component_master_ops hdac_component_master_ops = {
> .unbind = hdac_component_master_unbind,
> };
>
> +static const struct i915_audio_component_cb_ops i915_audio_component_cb_ops = {
> + .owner = THIS_MODULE,
> + .hotplug_notify = i915_audio_component_hotplug_notify,
> +};
> +
> +static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
> + const struct i915_audio_hotplug_info *info)
> +{
> + struct hda_codec *codec;
> +
> + codec_dbg("Received HDMI hotplug callback (connector = %s, plugged in = %d)",
> + info->connector_name, (int) info->plugged_in);
> +
> + for (i = 0; i <= MAX_CODEC_ADDRESS; i++)
> + if (bus->caddr_tbl[i] && bus->caddr_tbl[i]->hotplug_notify)
> + bus->caddr_tbl[i]->hotplug_notify(bus->caddr_tbl[i], info);
> +}
> +
> static int hdac_component_master_match(struct device *dev, void *data)
> {
> /* i915 is the only supported component */
> @@ -160,6 +180,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;
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 9515891..8151651 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -2316,6 +2316,13 @@ 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 hdmi_hotplug_notify(struct hdac_device *dev,
> + const struct i915_audio_hotplug_info *info)
> +{
> + struct hda_codec *codec = container_of(dev, hda_codec, core);
> + // TODO: Do something fun with this information
> +}
> +
> static int patch_generic_hdmi(struct hda_codec *codec)
> {
> struct hdmi_spec *spec;
> @@ -2345,6 +2352,8 @@ static int patch_generic_hdmi(struct hda_codec *codec)
> if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> codec->depop_delay = 0;
>
> + codec->core.hotplug_notify = hdmi_hotplug_notify;
> +
> if (hdmi_parse_codec(codec) < 0) {
> codec->spec = NULL;
> kfree(spec);
More information about the Alsa-devel
mailing list