HDMI hotplug on Skylake when power well is off

HDMI hotplug on Skylake when power well is off

[David Henningsson]

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?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

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.

IMO, a cleaner solution would be rather the notifier implementation in
software, e.g. extend the i915 component to pass the audio side ops
for notification.


Takashi

Re: HDMI hotplug on Skylake when power well is off

[Vinod Koul]

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

> 
> IMO, a cleaner solution would be rather the notifier implementation in
> software, e.g. extend the i915 component to pass the audio side ops
> for notification.
Yes that should be added but I would prefer we have hw do that as well

-- 
~Vinod

Re: HDMI hotplug on Skylake when power well is off

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]



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?

>> IMO, a cleaner solution would be rather the notifier implementation in
>> software, e.g. extend the i915 component to pass the audio side ops
>> for notification.
> Yes that should be added but I would prefer we have hw do that as well

So I took a quick stab at this and tried to write down a draft, but I 
got stuck trying to figure out how to wake up the audio codecs from the 
hdac_i915.c file. I'm not sure how to do this with the recent reorg as I 
don't want to break the ASoC version of the driver by including the 
wrong header files.

See attached patch (which is a very rough draft, not even compile 
tested), maybe you or Takashi could offer some insight w r t whether I'm 
on the right track, and how to proceed?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

[-- Attachment #2: i915_audio_component_cb_ops.patch --]
[-- Type: text/x-diff, Size: 4985 bytes --]

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..3799d88 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -424,6 +424,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->cb_ops && acomp->cb_ops->hotplug_notify)
+		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true);
 }
 
 /**
@@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_audio_component *acomp = dev_priv->audio_component;
 
 	if (dev_priv->display.audio_codec_disable)
 		dev_priv->display.audio_codec_disable(encoder);
+
+	if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify)
+		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false);
 }
 
 /**
@@ -529,12 +536,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 +552,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..3feab48 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,11 @@
 #ifndef _I915_COMPONENT_H_
 #define _I915_COMPONENT_H_
 
+struct hdac_bus;
+
 struct i915_audio_component {
 	struct device *dev;
+	struct hdac_bus *hdac_bus;
 
 	const struct i915_audio_component_ops {
 		struct module *owner;
@@ -34,6 +37,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 *, bool enable);
+	} *cb_ops;
 };
 
 #endif /* _I915_COMPONENT_H_ */
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 442500e..0ec49a6 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,31 @@ static const struct component_master_ops hdac_component_master_ops = {
 	.unbind = hdac_component_master_unbind,
 };
 
+static const struct i915_ hdac_component_master_ops = {
+	.bind = hdac_component_master_bind,
+	.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, bool enable)
+{
+	struct hda_codec *codec;
+
+	codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable);
+
+	// TODO: Not sure about this part - with the new reorg, maybe I can't access
+	// codec->jackpoll_work from this file?
+	list_for_each_codec(codec, bus)
+		schedule_delayed_work(&codec->jackpoll_work,
+			codec->jackpoll_interval);
+
+}
+
 static int hdac_component_master_match(struct device *dev, void *data)
 {
 	/* i915 is the only supported component */
@@ -160,6 +187,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;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: HDMI hotplug on Skylake when power well is off

[Vinod Koul]

On Thu, Jul 16, 2015 at 11:34:02AM +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?
Hmmm that is intresting question, Takashi, Mengdong do you ahve ay idea on
this one?

> 
> >>IMO, a cleaner solution would be rather the notifier implementation in
> >>software, e.g. extend the i915 component to pass the audio side ops
> >>for notification.
> >Yes that should be added but I would prefer we have hw do that as well
> 
> So I took a quick stab at this and tried to write down a draft, but
> I got stuck trying to figure out how to wake up the audio codecs
> from the hdac_i915.c file. I'm not sure how to do this with the
> recent reorg as I don't want to break the ASoC version of the driver
> by including the wrong header files.
> 
> See attached patch (which is a very rough draft, not even compile
> tested), maybe you or Takashi could offer some insight w r t whether
> I'm on the right track, and how to proceed?
> 
> -- 
> 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..3799d88 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -424,6 +424,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->cb_ops && acomp->cb_ops->hotplug_notify)
> +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true);
>  }
>  
>  /**
> @@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_audio_component *acomp = dev_priv->audio_component;
>  
>  	if (dev_priv->display.audio_codec_disable)
>  		dev_priv->display.audio_codec_disable(encoder);
> +
> +	if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify)
> +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false);
>  }
>  
>  /**
> @@ -529,12 +536,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 +552,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..3feab48 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,11 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +struct hdac_bus;
> +
>  struct i915_audio_component {
>  	struct device *dev;
> +	struct hdac_bus *hdac_bus;
>  
>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +37,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 *, bool enable);
> +	} *cb_ops;
>  };
>  
>  #endif /* _I915_COMPONENT_H_ */
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 442500e..0ec49a6 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,31 @@ static const struct component_master_ops hdac_component_master_ops = {
>  	.unbind = hdac_component_master_unbind,
>  };
>  
> +static const struct i915_ hdac_component_master_ops = {
> +	.bind = hdac_component_master_bind,
> +	.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,
> +};
do we need these two, why not add .hotplug_notify in the
i915_audio_component. During bind the hdac_i915 sets this up and the display
can invoke this callback (assuming here the binding takes care of passing
this value from audio to display component)

> +
> +
> +static void i915_audio_component_hotplug_notify(struct hdac_bus *bus, bool enable)
> +{
> +	struct hda_codec *codec;
> +
> +	codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable);
> +
> +	// TODO: Not sure about this part - with the new reorg, maybe I can't access
> +	// codec->jackpoll_work from this file?
Here i feel that we should add notify in hdac_device. Driver can set that up
and bus can invoke

-- 
~Vinod
> +	list_for_each_codec(codec, bus)
> +		schedule_delayed_work(&codec->jackpoll_work,
> +			codec->jackpoll_interval);
> +
> +}
> +
>  static int hdac_component_master_match(struct device *dev, void *data)
>  {
>  	/* i915 is the only supported component */
> @@ -160,6 +187,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;


-- 

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

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.

> >> IMO, a cleaner solution would be rather the notifier implementation in
> >> software, e.g. extend the i915 component to pass the audio side ops
> >> for notification.
> > Yes that should be added but I would prefer we have hw do that as well
> 
> So I took a quick stab at this and tried to write down a draft, but I 
> got stuck trying to figure out how to wake up the audio codecs from the 
> hdac_i915.c file. I'm not sure how to do this with the recent reorg as I 
> don't want to break the ASoC version of the driver by including the 
> wrong header files.
> 
> See attached patch (which is a very rough draft, not even compile 
> tested), maybe you or Takashi could offer some insight w r t whether I'm 
> on the right track, and how to proceed?

Well, at this time, it's not that easy, unfortunately.  We need to
propagate the notification to a specific codec, not to (only) a bus.
And there will be multiple outputs.  So it'll be an array or list.

Also, ELD and some other bits can be stored there to share between
audio and gfx, too.  This is a big bonus as it allows us to avoid the
read via the often unstable hardware access.

An unclear thing so far is the mapping between the audio codec and the
gfx output.  And this is also the missing piece as of now.  User can't
know which audio output corresponds to which gfx output.  So this is
the first question: is there any good way to know this mapping?

If the mapping is static and can be known at the probe time, we can
register the notifier and assign ELD record and gfx mapping for each
HDMI/DP codec.


thanks,

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..3799d88 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -424,6 +424,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->cb_ops && acomp->cb_ops->hotplug_notify)
> +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true);
>  }
>  
>  /**
> @@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
>  {
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_audio_component *acomp = dev_priv->audio_component;
>  
>  	if (dev_priv->display.audio_codec_disable)
>  		dev_priv->display.audio_codec_disable(encoder);
> +
> +	if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify)
> +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false);
>  }
>  
>  /**
> @@ -529,12 +536,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 +552,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..3feab48 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,11 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +struct hdac_bus;
> +
>  struct i915_audio_component {
>  	struct device *dev;
> +	struct hdac_bus *hdac_bus;
>  
>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +37,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 *, bool enable);
> +	} *cb_ops;
>  };
>  
>  #endif /* _I915_COMPONENT_H_ */
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index 442500e..0ec49a6 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,31 @@ static const struct component_master_ops hdac_component_master_ops = {
>  	.unbind = hdac_component_master_unbind,
>  };
>  
> +static const struct i915_ hdac_component_master_ops = {
> +	.bind = hdac_component_master_bind,
> +	.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, bool enable)
> +{
> +	struct hda_codec *codec;
> +
> +	codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable);
> +
> +	// TODO: Not sure about this part - with the new reorg, maybe I can't access
> +	// codec->jackpoll_work from this file?
> +	list_for_each_codec(codec, bus)
> +		schedule_delayed_work(&codec->jackpoll_work,
> +			codec->jackpoll_interval);
> +
> +}
> +
>  static int hdac_component_master_match(struct device *dev, void *data)
>  {
>  	/* i915 is the only supported component */
> @@ -160,6 +187,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;

Re: HDMI hotplug on Skylake when power well is off

[Vinod Koul]

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.
> 
> > >> IMO, a cleaner solution would be rather the notifier implementation in
> > >> software, e.g. extend the i915 component to pass the audio side ops
> > >> for notification.
> > > Yes that should be added but I would prefer we have hw do that as well
> > 
> > So I took a quick stab at this and tried to write down a draft, but I 
> > got stuck trying to figure out how to wake up the audio codecs from the 
> > hdac_i915.c file. I'm not sure how to do this with the recent reorg as I 
> > don't want to break the ASoC version of the driver by including the 
> > wrong header files.
> > 
> > See attached patch (which is a very rough draft, not even compile 
> > tested), maybe you or Takashi could offer some insight w r t whether I'm 
> > on the right track, and how to proceed?
> 
> Well, at this time, it's not that easy, unfortunately.  We need to
> propagate the notification to a specific codec, not to (only) a bus.
> And there will be multiple outputs.  So it'll be an array or list.
Shouldn't the codec say that notify me to bus and thus completing the link?
Anyway only one codec per bus would have done that

> 
> Also, ELD and some other bits can be stored there to share between
> audio and gfx, too.  This is a big bonus as it allows us to avoid the
> read via the often unstable hardware access.
Yes I do like that idea..

> 
> An unclear thing so far is the mapping between the audio codec and the
> gfx output.  And this is also the missing piece as of now.  User can't
> know which audio output corresponds to which gfx output.  So this is
> the first question: is there any good way to know this mapping?
Yes, so why not add get monitor name as well with this as we discussed
sometime back. Then for the PCM user can read the name from control/sysfs

-- 
~Vinod

> 
> If the mapping is static and can be known at the probe time, we can
> register the notifier and assign ELD record and gfx mapping for each
> HDMI/DP codec.
> 
> 
> thanks,
> 
> 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..3799d88 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -424,6 +424,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->cb_ops && acomp->cb_ops->hotplug_notify)
> > +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true);
> >  }
> >  
> >  /**
> > @@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
> >  {
> >  	struct drm_device *dev = encoder->base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct i915_audio_component *acomp = dev_priv->audio_component;
> >  
> >  	if (dev_priv->display.audio_codec_disable)
> >  		dev_priv->display.audio_codec_disable(encoder);
> > +
> > +	if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify)
> > +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false);
> >  }
> >  
> >  /**
> > @@ -529,12 +536,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 +552,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..3feab48 100644
> > --- a/include/drm/i915_component.h
> > +++ b/include/drm/i915_component.h
> > @@ -24,8 +24,11 @@
> >  #ifndef _I915_COMPONENT_H_
> >  #define _I915_COMPONENT_H_
> >  
> > +struct hdac_bus;
> > +
> >  struct i915_audio_component {
> >  	struct device *dev;
> > +	struct hdac_bus *hdac_bus;
> >  
> >  	const struct i915_audio_component_ops {
> >  		struct module *owner;
> > @@ -34,6 +37,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 *, bool enable);
> > +	} *cb_ops;
> >  };
> >  
> >  #endif /* _I915_COMPONENT_H_ */
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index 442500e..0ec49a6 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,31 @@ static const struct component_master_ops hdac_component_master_ops = {
> >  	.unbind = hdac_component_master_unbind,
> >  };
> >  
> > +static const struct i915_ hdac_component_master_ops = {
> > +	.bind = hdac_component_master_bind,
> > +	.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, bool enable)
> > +{
> > +	struct hda_codec *codec;
> > +
> > +	codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable);
> > +
> > +	// TODO: Not sure about this part - with the new reorg, maybe I can't access
> > +	// codec->jackpoll_work from this file?
> > +	list_for_each_codec(codec, bus)
> > +		schedule_delayed_work(&codec->jackpoll_work,
> > +			codec->jackpoll_interval);
> > +
> > +}
> > +
> >  static int hdac_component_master_match(struct device *dev, void *data)
> >  {
> >  	/* i915 is the only supported component */
> > @@ -160,6 +187,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;

-- 

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

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.
> > 
> > > >> IMO, a cleaner solution would be rather the notifier implementation in
> > > >> software, e.g. extend the i915 component to pass the audio side ops
> > > >> for notification.
> > > > Yes that should be added but I would prefer we have hw do that as well
> > > 
> > > So I took a quick stab at this and tried to write down a draft, but I 
> > > got stuck trying to figure out how to wake up the audio codecs from the 
> > > hdac_i915.c file. I'm not sure how to do this with the recent reorg as I 
> > > don't want to break the ASoC version of the driver by including the 
> > > wrong header files.
> > > 
> > > See attached patch (which is a very rough draft, not even compile 
> > > tested), maybe you or Takashi could offer some insight w r t whether I'm 
> > > on the right track, and how to proceed?
> > 
> > Well, at this time, it's not that easy, unfortunately.  We need to
> > propagate the notification to a specific codec, not to (only) a bus.
> > And there will be multiple outputs.  So it'll be an array or list.
> Shouldn't the codec say that notify me to bus and thus completing the link?
> Anyway only one codec per bus would have done that

If it's 1:1, then no big problem.  But it's always many codecs on a
bus on modern systems.  Then we don't want to broadcast to all HDMI
codecs at each time a connection happens to a single output.

> > Also, ELD and some other bits can be stored there to share between
> > audio and gfx, too.  This is a big bonus as it allows us to avoid the
> > read via the often unstable hardware access.
> Yes I do like that idea..
> 
> > 
> > An unclear thing so far is the mapping between the audio codec and the
> > gfx output.  And this is also the missing piece as of now.  User can't
> > know which audio output corresponds to which gfx output.  So this is
> > the first question: is there any good way to know this mapping?
> Yes, so why not add get monitor name as well with this as we discussed
> sometime back. Then for the PCM user can read the name from control/sysfs

Yes, that would be good.  But the question is which component is
responsible for it, how it's detected and at which timing.  This is
the missing piece (information) for the further implementation.
And I belive Intel guys know this at best.


thanks,

Takashi

> 
> -- 
> ~Vinod
> 
> > 
> > If the mapping is static and can be known at the probe time, we can
> > register the notifier and assign ELD record and gfx mapping for each
> > HDMI/DP codec.
> > 
> > 
> > thanks,
> > 
> > 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..3799d88 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -424,6 +424,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->cb_ops && acomp->cb_ops->hotplug_notify)
> > > +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, true);
> > >  }
> > >  
> > >  /**
> > > @@ -437,9 +440,13 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
> > >  {
> > >  	struct drm_device *dev = encoder->base.dev;
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct i915_audio_component *acomp = dev_priv->audio_component;
> > >  
> > >  	if (dev_priv->display.audio_codec_disable)
> > >  		dev_priv->display.audio_codec_disable(encoder);
> > > +
> > > +	if (acomp && acomp->cb_ops && acomp->cb_ops->hotplug_notify)
> > > +		acomp->cb_ops->hotplug_notify(acomp->hdac_bus, false);
> > >  }
> > >  
> > >  /**
> > > @@ -529,12 +536,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 +552,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..3feab48 100644
> > > --- a/include/drm/i915_component.h
> > > +++ b/include/drm/i915_component.h
> > > @@ -24,8 +24,11 @@
> > >  #ifndef _I915_COMPONENT_H_
> > >  #define _I915_COMPONENT_H_
> > >  
> > > +struct hdac_bus;
> > > +
> > >  struct i915_audio_component {
> > >  	struct device *dev;
> > > +	struct hdac_bus *hdac_bus;
> > >  
> > >  	const struct i915_audio_component_ops {
> > >  		struct module *owner;
> > > @@ -34,6 +37,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 *, bool enable);
> > > +	} *cb_ops;
> > >  };
> > >  
> > >  #endif /* _I915_COMPONENT_H_ */
> > > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > > index 442500e..0ec49a6 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,31 @@ static const struct component_master_ops hdac_component_master_ops = {
> > >  	.unbind = hdac_component_master_unbind,
> > >  };
> > >  
> > > +static const struct i915_ hdac_component_master_ops = {
> > > +	.bind = hdac_component_master_bind,
> > > +	.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, bool enable)
> > > +{
> > > +	struct hda_codec *codec;
> > > +
> > > +	codec_dbg("Received HDMI hotplug callback (enable = %d)", (int) enable);
> > > +
> > > +	// TODO: Not sure about this part - with the new reorg, maybe I can't access
> > > +	// codec->jackpoll_work from this file?
> > > +	list_for_each_codec(codec, bus)
> > > +		schedule_delayed_work(&codec->jackpoll_work,
> > > +			codec->jackpoll_interval);
> > > +
> > > +}
> > > +
> > >  static int hdac_component_master_match(struct device *dev, void *data)
> > >  {
> > >  	/* i915 is the only supported component */
> > > @@ -160,6 +187,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;
> 
> -- 
> 

Re: HDMI hotplug on Skylake when power well is off

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]



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?

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.

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).

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

[-- Attachment #2: hdmi_hotplug_notify2.patch --]
[-- Type: text/x-diff, Size: 7527 bytes --]

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);

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

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);

Re: HDMI hotplug on Skylake when power well is off

[Vinod Koul]

On Thu, Jul 16, 2015 at 03:57:03PM +0200, Takashi Iwai wrote:
> > >>> 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.
So essentialy both for analog and hdmi this never works reliably ?
I was under the impression taht this was working okay

> > 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.
Are you talking about nid to connctor name mapping or something else?

-- 
~Vinod

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

On Fri, 17 Jul 2015 13:17:24 +0200,
Vinod Koul wrote:
> 
> On Thu, Jul 16, 2015 at 03:57:03PM +0200, Takashi Iwai wrote:
> > > >>> 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.
> So essentialy both for analog and hdmi this never works reliably ?

No machines worked on my test machines, so far.

> I was under the impression taht this was working okay
> 
> > > 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.
> Are you talking about nid to connctor name mapping or something else?

Yes.


Takashi

Re: HDMI hotplug on Skylake when power well is off

[Vinod Koul]

On Thu, Jul 16, 2015 at 03:37:07PM +0200, David Henningsson wrote:
> +static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
> +	const struct i915_audio_hotplug_info *info)
> +{
> +	struct hda_codec *codec;
this should be hdac_device as core doesnt know hda_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);
> +}

Rest looks good :)

-- 
~Vinod

Re: HDMI hotplug on Skylake when power well is off

[Takashi Iwai]

On Fri, 17 Jul 2015 13:14:29 +0200,
Vinod Koul wrote:
> 
> On Thu, Jul 16, 2015 at 03:37:07PM +0200, David Henningsson wrote:
> > +static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
> > +	const struct i915_audio_hotplug_info *info)
> > +{
> > +	struct hda_codec *codec;
> this should be hdac_device as core doesnt know hda_codec

Yes, hdac_device is the core part of the codec device struct.

> > +
> > +	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);
> > +}
> 
> Rest looks good :)

I prefer traversing over the list like:

	struct hdac_device *codec;

	list_for_each_entry(codec, &bus->list, list) {
		if (codec->hotplug_notify)
			codec->hotplug_notify(codec, info);
	}


thanks,

Takashi