[PATCH v3 0/4] i915 to call hda driver on HDMI plug/unplug

[PATCH v3 0/4] i915 to call hda driver on HDMI plug/unplug

[David Henningsson]

Changes since v2 is that the patch set has diminished to not transfer
connector/eld information, since it might be better that such information
to be transferred by a separate call in the ordinary direction. That
could be added in a later patch set.

The audio_ptr is now a void* (instead of a hdac_bus*) so that the audio
side can easily refactor the ptr to point to the codec instead of the bus
if we would find that beneficial.

David Henningsson (4):
  drm/i915: Add audio hotplug info callback
  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 |   23 ++++++++++++++++++++---
 include/drm/i915_component.h       |    5 +++++
 include/sound/hdaudio.h            |    4 ++++
 sound/hda/hdac_i915.c              |   23 +++++++++++++++++++++++
 sound/pci/hda/patch_hdmi.c         |   13 ++++++++++++-
 6 files changed, 65 insertions(+), 4 deletions(-)

-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Add audio hotplug info callback

[David Henningsson]

This callback will be called by the i915 driver to notify the hda
driver that HDMI has been hotplugged.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 include/drm/i915_component.h |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..d053008 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -26,6 +26,7 @@
 
 struct i915_audio_component {
 	struct device *dev;
+	void *audio_ptr;
 
 	const struct i915_audio_component_ops {
 		struct module *owner;
@@ -34,6 +35,10 @@ struct i915_audio_component {
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
 	} *ops;
+
+	const struct i915_audio_component_audio_ops {
+		void (*hotplug_notify)(void *audio_ptr, int port, int port_mst_index);
+	} *audio_ops;
 };
 
 #endif /* _I915_COMPONENT_H_ */
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Add audio hotplug info callback

[Jani Nikula]

On Thu, 23 Jul 2015, David Henningsson <david.henningsson@canonical.com> wrote:
> This callback will be called by the i915 driver to notify the hda
> driver that HDMI has been hotplugged.
>
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  include/drm/i915_component.h |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..d053008 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -26,6 +26,7 @@
>  
>  struct i915_audio_component {
>  	struct device *dev;
> +	void *audio_ptr;
>  
>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +35,10 @@ struct i915_audio_component {
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
>  	} *ops;
> +
> +	const struct i915_audio_component_audio_ops {
> +		void (*hotplug_notify)(void *audio_ptr, int port, int port_mst_index);
> +	} *audio_ops;

I'd appreciate kernel-doc for the callback, in particular with the
preconditions for making the call clarified. When I first saw the
series, I thought "oh no you can't make the call from hotplug
code". Which you don't, but all your commit messages and comments and
naming refer to hotplug.

The main point is that you can't tell the audio it's good to go before
we have pretty much the whole display pipeline enabled, and there's a
long way from hotplug to that point.

BR,
Jani.



>  };
>  
>  #endif /* _I915_COMPONENT_H_ */
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Call audio hotplug notify function

[David Henningsson]

On HDMI hotplug events, notify the audio driver. This will enable
the audio driver to get the notification at all times (even when
audio is in different powersave states).

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |    1 +
 drivers/gpu/drm/i915/intel_audio.c |   23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 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..3cc8849 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -399,6 +399,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 	struct drm_connector *connector;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_audio_component *acomp = dev_priv->audio_component;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	enum port port = intel_dig_port->port;
 
 	connector = drm_select_eld(encoder, mode);
 	if (!connector)
@@ -419,6 +422,9 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 
 	if (dev_priv->display.audio_codec_enable)
 		dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
+
+	if (acomp && acomp->audio_ops && acomp->audio_ops->hotplug_notify)
+		acomp->audio_ops->hotplug_notify(acomp->audio_ptr, (int) port, 0);
 }
 
 /**
@@ -428,13 +434,20 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
  * The disable sequences must be performed before disabling the transcoder or
  * port.
  */
-void intel_audio_codec_disable(struct intel_encoder *encoder)
+void intel_audio_codec_disable(struct intel_encoder *intel_encoder)
 {
-	struct drm_device *dev = encoder->base.dev;
+	struct drm_encoder *encoder = &intel_encoder->base;
+	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_audio_component *acomp = dev_priv->audio_component;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
+	enum port port = intel_dig_port->port;
 
 	if (dev_priv->display.audio_codec_disable)
-		dev_priv->display.audio_codec_disable(encoder);
+		dev_priv->display.audio_codec_disable(intel_encoder);
+
+	if (acomp && acomp->audio_ops && acomp->audio_ops->hotplug_notify)
+		acomp->audio_ops->hotplug_notify(acomp->audio_ptr, (int) port, 0);
 }
 
 /**
@@ -525,12 +538,14 @@ static int i915_audio_component_bind(struct device *i915_dev,
 				     struct device *hda_dev, void *data)
 {
 	struct i915_audio_component *acomp = data;
+	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
 
 	if (WARN_ON(acomp->ops || acomp->dev))
 		return -EEXIST;
 
 	acomp->ops = &i915_audio_component_ops;
 	acomp->dev = i915_dev;
+	dev_priv->audio_component = acomp;
 
 	return 0;
 }
@@ -539,9 +554,11 @@ static void i915_audio_component_unbind(struct device *i915_dev,
 					struct device *hda_dev, void *data)
 {
 	struct i915_audio_component *acomp = data;
+	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
 
 	acomp->ops = NULL;
 	acomp->dev = NULL;
+	dev_priv->audio_component = NULL;
 }
 
 static const struct component_ops i915_audio_component_bind_ops = {
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] ALSA: hda - Dispatch incoming HDMI hotplug i915 callback

[David Henningsson]

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   |   23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4caf1fd..8142d03 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 (*i915_hotplug_notify)(struct hdac_device *, int port,
+				    int port_mst_index);
+
 	/* 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..17295c2 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->audio_ops = NULL;
+	acomp->audio_ptr = NULL;
 	module_put(acomp->ops->owner);
 	component_unbind_all(dev, acomp);
 	WARN_ON(acomp->ops || acomp->dev);
@@ -128,6 +130,24 @@ static const struct component_master_ops hdac_component_master_ops = {
 	.unbind = hdac_component_master_unbind,
 };
 
+static void i915_audio_component_hotplug_notify(void *audio_ptr,
+						int port, int port_mst_index)
+{
+	struct hdac_device *d;
+	struct hdac_bus *bus = audio_ptr;
+
+	dev_dbg(bus->dev, "i915 hotplug event (port = %d:%d)",
+		port, port_mst_index);
+
+	list_for_each_entry(d, &bus->codec_list, list)
+		if (d->i915_hotplug_notify)
+			d->i915_hotplug_notify(d, port, port_mst_index);
+}
+
+static const struct i915_audio_component_audio_ops i915_audio_component_audio_ops = {
+	.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 +183,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 		ret = -ENODEV;
 		goto out_master_del;
 	}
+	acomp->audio_ops = &i915_audio_component_audio_ops;
+	acomp->audio_ptr = bus;
+
 	dev_dbg(dev, "bound to i915 component master\n");
 
 	return 0;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] ALSA: hda - Wake the codec up on hotplug notify events

[David Henningsson]

Whenever there is an event from the i915 driver, wake the codec
and recheck plug/unplug + ELD status.

This fixes the issue with lost unsol events in power save mode,
the codec and controller can now sleep in D3 and still know when
the HDMI monitor has been connected.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/patch_hdmi.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2f24338..6cc1524 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2316,6 +2316,15 @@ 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_hotplug_notify(struct hdac_device *dev, int port,
+				 int port_mst_index)
+{
+	struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+	int pin_nid = is_valleyview(codec) ? 0x03 : port + 0x04;
+
+	check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
@@ -2342,8 +2351,10 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 	if (is_valleyview_plus(codec) || is_skylake(codec))
 		codec->core.link_power_control = 1;
 
-	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
 		codec->depop_delay = 0;
+		codec->core.i915_hotplug_notify = intel_hotplug_notify;
+	}
 
 	if (hdmi_parse_codec(codec) < 0) {
 		codec->spec = NULL;
-- 
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] ALSA: hda - Wake the codec up on hotplug notify events

[Yang, Libin]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On
> Behalf Of David Henningsson
> Sent: Thursday, July 23, 2015 11:26 PM
> To: Koul, Vinod; jani.nikula@linux.intel.com; Vetter, Daniel;
> tiwai@suse.de; intel-gfx@lists.freedesktop.org; alsa-devel@alsa-
> project.org
> Cc: David Henningsson
> Subject: [Intel-gfx] [PATCH 4/4] ALSA: hda - Wake the codec up on
> hotplug notify events
> 
> Whenever there is an event from the i915 driver, wake the codec
> and recheck plug/unplug + ELD status.
> 
> This fixes the issue with lost unsol events in power save mode,
> the codec and controller can now sleep in D3 and still know when
> the HDMI monitor has been connected.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/patch_hdmi.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c
> b/sound/pci/hda/patch_hdmi.c
> index 2f24338..6cc1524 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -2316,6 +2316,15 @@ 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_hotplug_notify(struct hdac_device *dev, int port,
> +				 int port_mst_index)
> +{
> +	struct hda_codec *codec = container_of(dev, struct
> hda_codec, core);
> +	int pin_nid = is_valleyview(codec) ? 0x03 : port + 0x04;

Valleyview should be the same as others.

Regards,
Libin

> +
> +	check_presence_and_report(codec, pin_nid);
> +}
> +
>  static int patch_generic_hdmi(struct hda_codec *codec)
>  {
>  	struct hdmi_spec *spec;
> @@ -2342,8 +2351,10 @@ static int patch_generic_hdmi(struct
> hda_codec *codec)
>  	if (is_valleyview_plus(codec) || is_skylake(codec))
>  		codec->core.link_power_control = 1;
> 
> -	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec)) {
>  		codec->depop_delay = 0;
> +		codec->core.i915_hotplug_notify =
> intel_hotplug_notify;
> +	}
> 
>  	if (hdmi_parse_codec(codec) < 0) {
>  		codec->spec = NULL;
> --
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 0/4] i915 to call hda driver on HDMI plug/unplug

[Takashi Iwai]

On Thu, 23 Jul 2015 17:26:15 +0200,
David Henningsson wrote:
> 
> Changes since v2 is that the patch set has diminished to not transfer
> connector/eld information, since it might be better that such information
> to be transferred by a separate call in the ordinary direction. That
> could be added in a later patch set.
> 
> The audio_ptr is now a void* (instead of a hdac_bus*) so that the audio
> side can easily refactor the ptr to point to the codec instead of the bus
> if we would find that beneficial.

OK, that leaves some room if we want to change in future.

I still consider whether it's better to register from the codec driver
or from the bus driver.  The codec driver might be a bit cleaner, but
then it'd be a call like
	snd_hda_i915_register_notifier()
instead of adding a callback in the codec ops.

What I see a bit ugly in the current patchset is the addition of
i915 specific callback to a generic hda codec ops.  If the codec
registers the callback dynamically like above, the pointer will be
stored in component directly, so we don't need it in codec ops.
(But we'd need to unregister at codec removal, too.)

Or, if we keep it as a codec ops callback, as already mentioned, it
might make more sense to define it as a generic notifier, i.e. drop
i915 prefix and pass some event type to identify the event.


thanks,

Takashi

> David Henningsson (4):
>   drm/i915: Add audio hotplug info callback
>   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 |   23 ++++++++++++++++++++---
>  include/drm/i915_component.h       |    5 +++++
>  include/sound/hdaudio.h            |    4 ++++
>  sound/hda/hdac_i915.c              |   23 +++++++++++++++++++++++
>  sound/pci/hda/patch_hdmi.c         |   13 ++++++++++++-
>  6 files changed, 65 insertions(+), 4 deletions(-)
> 
> -- 
> 1.7.9.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx