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

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

[David Henningsson]

This patch set aims to resolve three problems:

 * The first - and most serious one - is that the audio driver is not woken up
   properly when in power save modes, especially not when the HDA controller is
   in D3. By having the i915 driver call directly into the hda driver, the HDA
   driver is always notified that an HDMI hotplug event has happened.

 * Second, there is currently no way for userspace to match an HDMI audio output
   with an HDMI video output. We fix this by sending connector_type and
   connector_type_id in the HDMI hotplug callback.

 * Third, writing ELD info to the hardware just so the HDA driver can read it
   from the hardware seems a bit inefficient. We could just pass that information
   in the callback, too.

The patch in its current form fixes the first of these problems and provides most
of the infrastructure for the second and third problem. 

The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has
been tested (and working) on one Skylake machine.

David Henningsson (4):
  drm/i915: Add audio hotplug info struct
  drm/i915: Call audio hotplug notify function
  ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
  ALSA: hda - Wake the codec up on hotplug notify events

 drivers/gpu/drm/i915/i915_drv.h    |    1 +
 drivers/gpu/drm/i915/intel_audio.c |   46 ++++++++++++++++++++++++++++++++++++
 include/drm/i915_component.h       |   19 +++++++++++++++
 include/sound/hdaudio.h            |    4 ++++
 sound/hda/hdac_i915.c              |   24 +++++++++++++++++++
 sound/pci/hda/patch_hdmi.c         |   22 ++++++++++++++++-
 6 files changed, 115 insertions(+), 1 deletion(-)

-- 
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 struct

[David Henningsson]

This struct will be used to transfer information from the i915
driver to the hda driver on HDMI hotplug events.

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

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..4fc0db3 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -24,8 +24,22 @@
 #ifndef _I915_COMPONENT_H_
 #define _I915_COMPONENT_H_
 
+struct hdac_bus;
+
+struct i915_audio_hotplug_info {
+	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
+	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
+	int port; /* Used for mapping to affected nid */
+	int port_multi_stream_device; /* For DP multi-streaming */
+
+	bool plugged_in;
+	uint8_t *eld;
+	int eld_size;
+};
+
 struct i915_audio_component {
 	struct device *dev;
+	struct hdac_bus *hdac_bus;
 
 	const struct i915_audio_component_ops {
 		struct module *owner;
@@ -34,6 +48,11 @@ struct i915_audio_component {
 		void (*codec_wake_override)(struct device *, bool enable);
 		int (*get_cdclk_freq)(struct device *);
 	} *ops;
+
+	const struct i915_audio_component_cb_ops {
+		struct module *owner;
+		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
+	} *cb_ops;
 };
 
 #endif /* _I915_COMPONENT_H_ */
-- 
1.7.9.5

_______________________________________________
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 information at all times (even when
audio is in different powersave states), and also without reading
it from the hardware.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 542fac6..696624c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1808,6 +1808,7 @@ struct drm_i915_private {
 	struct drm_property *force_audio_property;
 
 	/* hda/i915 audio component */
+	struct i915_audio_component *audio_component;
 	bool audio_component_registered;
 
 	uint32_t hw_context_size;
diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
index 3da9b84..eb29e1f 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
 	I915_WRITE(aud_config, tmp);
 }
 
+static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
+				 struct drm_connector *connector,
+				 struct intel_encoder *encoder,
+				 bool plugged_in)
+{
+	struct i915_audio_component *acomp = dev_priv->audio_component;
+	struct i915_audio_hotplug_info audio_info;
+	struct intel_digital_port *intel_dig_port =
+		enc_to_dig_port(&encoder->base);
+	enum port port = intel_dig_port->port;
+
+	if (!acomp || !acomp->cb_ops || !acomp->cb_ops->hotplug_notify)
+		return;
+
+	if (connector) {
+		audio_info.connector_type = connector->connector_type;
+		audio_info.connector_type_id = connector->connector_type_id;
+	} else {
+		audio_info.connector_type = -1;
+		audio_info.connector_type_id = -1;
+	}
+
+	audio_info.port = (int) port;
+	/* DP Mst is unsupported for now */
+	audio_info.port_multi_stream_device = 0;
+
+	audio_info.plugged_in = plugged_in;
+	if (plugged_in && connector) {
+		audio_info.eld = connector->eld;
+		audio_info.eld_size = drm_eld_size(audio_info.eld);
+	} else {
+		audio_info.eld = NULL;
+		audio_info.eld_size = 0;
+	}
+
+	acomp->cb_ops->hotplug_notify(acomp->hdac_bus, &audio_info);
+}
+
 /**
  * intel_audio_codec_enable - Enable the audio codec for HD audio
  * @intel_encoder: encoder on which to enable audio
@@ -419,6 +457,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
 
 	if (dev_priv->display.audio_codec_enable)
 		dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
+
+	audio_hotplug_notify(dev_priv, connector, intel_encoder, true);
 }
 
 /**
@@ -435,6 +475,8 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
 
 	if (dev_priv->display.audio_codec_disable)
 		dev_priv->display.audio_codec_disable(encoder);
+
+	audio_hotplug_notify(dev_priv, NULL, encoder, false);
 }
 
 /**
@@ -525,12 +567,14 @@ static int i915_audio_component_bind(struct device *i915_dev,
 				     struct device *hda_dev, void *data)
 {
 	struct i915_audio_component *acomp = data;
+	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
 
 	if (WARN_ON(acomp->ops || acomp->dev))
 		return -EEXIST;
 
 	acomp->ops = &i915_audio_component_ops;
 	acomp->dev = i915_dev;
+	dev_priv->audio_component = acomp;
 
 	return 0;
 }
@@ -539,9 +583,11 @@ static void i915_audio_component_unbind(struct device *i915_dev,
 					struct device *hda_dev, void *data)
 {
 	struct i915_audio_component *acomp = data;
+	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
 
 	acomp->ops = NULL;
 	acomp->dev = NULL;
+	dev_priv->audio_component = NULL;
 }
 
 static const struct component_ops i915_audio_component_bind_ops = {
-- 
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   |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 4caf1fd..ce34182 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -79,6 +79,10 @@ struct hdac_device {
 	int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
 			 unsigned int flags, unsigned int *res);
 
+	/* Used for hotplug notification from i915 driver */
+	void (*hotplug_notify)(struct hdac_device *,
+			       const struct i915_audio_hotplug_info *);
+
 	/* widgets */
 	unsigned int num_nodes;
 	hda_nid_t start_nid, end_nid;
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..134ef9c 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -118,6 +118,8 @@ static void hdac_component_master_unbind(struct device *dev)
 {
 	struct i915_audio_component *acomp = hdac_acomp;
 
+	acomp->cb_ops = NULL;
+	acomp->hdac_bus = NULL;
 	module_put(acomp->ops->owner);
 	component_unbind_all(dev, acomp);
 	WARN_ON(acomp->ops || acomp->dev);
@@ -128,6 +130,25 @@ static const struct component_master_ops hdac_component_master_ops = {
 	.unbind = hdac_component_master_unbind,
 };
 
+static void i915_audio_component_hotplug_notify(struct hdac_bus *bus,
+	const struct i915_audio_hotplug_info *info)
+{
+	struct hdac_device *d;
+
+	dev_dbg(bus->dev, "Received HDMI hotplug callback (connector = %d-%d, plugged in = %d)",
+		info->connector_type, info->connector_type_id, (int) info->plugged_in);
+
+	list_for_each_entry(d, &bus->codec_list, list) {
+		if (d->hotplug_notify)
+			d->hotplug_notify(d, info);
+	}
+}
+
+static const struct i915_audio_component_cb_ops i915_audio_component_cb_ops = {
+	.owner          = THIS_MODULE,
+	.hotplug_notify = i915_audio_component_hotplug_notify,
+};
+
 static int hdac_component_master_match(struct device *dev, void *data)
 {
 	/* i915 is the only supported component */
@@ -163,6 +184,9 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 		ret = -ENODEV;
 		goto out_master_del;
 	}
+	acomp->cb_ops = &i915_audio_component_cb_ops;
+	acomp->hdac_bus = bus;
+
 	dev_dbg(dev, "bound to i915 component master\n");
 
 	return 0;
-- 
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]

As a first cautious step, we're not going to trust the information
coming directly from the i915 driver, we're just going to use
the fact that there was an event to wakeup the codec and ask for
its status.

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

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

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 2f24338..230d83d 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -2316,6 +2316,24 @@ static void haswell_set_power_state(struct hda_codec *codec, hda_nid_t fg,
 	snd_hda_codec_set_power_to_all(codec, fg, power_state);
 }
 
+static void intel_haswell_hotplug_notify(struct hdac_device *dev,
+					 const struct i915_audio_hotplug_info *info)
+{
+	struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+	int pin_nid = info->port + 0x04;
+
+	check_presence_and_report(codec, pin_nid);
+}
+
+static void intel_valleyview_hotplug_notify(struct hdac_device *dev,
+					    const struct i915_audio_hotplug_info *info)
+{
+	struct hda_codec *codec = container_of(dev, struct hda_codec, core);
+	int pin_nid = 0x03; /* TODO: Ask Intel engineers to verify this */
+
+	check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
@@ -2331,7 +2349,9 @@ static int patch_generic_hdmi(struct hda_codec *codec)
 	if (is_haswell_plus(codec)) {
 		intel_haswell_enable_all_pins(codec, true);
 		intel_haswell_fixup_enable_dp12(codec);
-	}
+		codec->core.hotplug_notify = intel_haswell_hotplug_notify;
+	} else if (is_valleyview_plus(codec))
+		codec->core.hotplug_notify = intel_valleyview_hotplug_notify;
 
 	/* For Valleyview/Cherryview, only the display codec is in the display
 	 * power well and can use link_power ops to request/release the power.
-- 
1.7.9.5

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

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

[Daniel Vetter]

On Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
> On HDMI hotplug events, notify the audio driver. This will enable
> the audio driver to get the information at all times (even when
> audio is in different powersave states), and also without reading
> it from the hardware.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |    1 +
>  drivers/gpu/drm/i915/intel_audio.c |   46 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 542fac6..696624c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1808,6 +1808,7 @@ struct drm_i915_private {
>  	struct drm_property *force_audio_property;
>  
>  	/* hda/i915 audio component */
> +	struct i915_audio_component *audio_component;
>  	bool audio_component_registered;
>  
>  	uint32_t hw_context_size;
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 3da9b84..eb29e1f 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>  	I915_WRITE(aud_config, tmp);
>  }
>  
> +static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
> +				 struct drm_connector *connector,
> +				 struct intel_encoder *encoder,
> +				 bool plugged_in)

plugged_in is redundant, just check for NULL. Also just derive dev_priv
from intel_encoder imo. And finally we tend to put the object that a
function operates on first. Since this sends a notify out for the given
encoder (well dig port really) I'd put that first.

I also think that it would make sense to switch all the audio
enable/disable functions from intel_encoder to intel_dig_port. A lot need
to do upcasts anyway, and audio really only works on dig ports (which is
for hdmi/dp only).
-Daniel

> +{
> +	struct i915_audio_component *acomp = dev_priv->audio_component;
> +	struct i915_audio_hotplug_info audio_info;
> +	struct intel_digital_port *intel_dig_port =
> +		enc_to_dig_port(&encoder->base);
> +	enum port port = intel_dig_port->port;
> +
> +	if (!acomp || !acomp->cb_ops || !acomp->cb_ops->hotplug_notify)
> +		return;
> +
> +	if (connector) {
> +		audio_info.connector_type = connector->connector_type;
> +		audio_info.connector_type_id = connector->connector_type_id;
> +	} else {
> +		audio_info.connector_type = -1;
> +		audio_info.connector_type_id = -1;
> +	}
> +
> +	audio_info.port = (int) port;
> +	/* DP Mst is unsupported for now */
> +	audio_info.port_multi_stream_device = 0;
> +
> +	audio_info.plugged_in = plugged_in;
> +	if (plugged_in && connector) {
> +		audio_info.eld = connector->eld;
> +		audio_info.eld_size = drm_eld_size(audio_info.eld);
> +	} else {
> +		audio_info.eld = NULL;
> +		audio_info.eld_size = 0;
> +	}
> +
> +	acomp->cb_ops->hotplug_notify(acomp->hdac_bus, &audio_info);
> +}
> +
>  /**
>   * intel_audio_codec_enable - Enable the audio codec for HD audio
>   * @intel_encoder: encoder on which to enable audio
> @@ -419,6 +457,8 @@ void intel_audio_codec_enable(struct intel_encoder *intel_encoder)
>  
>  	if (dev_priv->display.audio_codec_enable)
>  		dev_priv->display.audio_codec_enable(connector, intel_encoder, mode);
> +
> +	audio_hotplug_notify(dev_priv, connector, intel_encoder, true);
>  }
>  
>  /**
> @@ -435,6 +475,8 @@ void intel_audio_codec_disable(struct intel_encoder *encoder)
>  
>  	if (dev_priv->display.audio_codec_disable)
>  		dev_priv->display.audio_codec_disable(encoder);
> +
> +	audio_hotplug_notify(dev_priv, NULL, encoder, false);
>  }
>  
>  /**
> @@ -525,12 +567,14 @@ static int i915_audio_component_bind(struct device *i915_dev,
>  				     struct device *hda_dev, void *data)
>  {
>  	struct i915_audio_component *acomp = data;
> +	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
>  
>  	if (WARN_ON(acomp->ops || acomp->dev))
>  		return -EEXIST;
>  
>  	acomp->ops = &i915_audio_component_ops;
>  	acomp->dev = i915_dev;
> +	dev_priv->audio_component = acomp;
>  
>  	return 0;
>  }
> @@ -539,9 +583,11 @@ static void i915_audio_component_unbind(struct device *i915_dev,
>  					struct device *hda_dev, void *data)
>  {
>  	struct i915_audio_component *acomp = data;
> +	struct drm_i915_private *dev_priv = dev_to_i915(i915_dev);
>  
>  	acomp->ops = NULL;
>  	acomp->dev = NULL;
> +	dev_priv->audio_component = NULL;
>  }
>  
>  static const struct component_ops i915_audio_component_bind_ops = {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[David Henningsson]

On 2015-07-21 11:14, Daniel Vetter wrote:
> On Tue, Jul 21, 2015 at 09:57:25AM +0200, David Henningsson wrote:
>> On HDMI hotplug events, notify the audio driver. This will enable
>> the audio driver to get the information at all times (even when
>> audio is in different powersave states), and also without reading
>> it from the hardware.
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h    |    1 +
>>   drivers/gpu/drm/i915/intel_audio.c |   46 ++++++++++++++++++++++++++++++++++++
>>   2 files changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 542fac6..696624c 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1808,6 +1808,7 @@ struct drm_i915_private {
>>   	struct drm_property *force_audio_property;
>>
>>   	/* hda/i915 audio component */
>> +	struct i915_audio_component *audio_component;
>>   	bool audio_component_registered;
>>
>>   	uint32_t hw_context_size;
>> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
>> index 3da9b84..eb29e1f 100644
>> --- a/drivers/gpu/drm/i915/intel_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_audio.c
>> @@ -384,6 +384,44 @@ static void ilk_audio_codec_enable(struct drm_connector *connector,
>>   	I915_WRITE(aud_config, tmp);
>>   }
>>
>> +static void audio_hotplug_notify(struct drm_i915_private *dev_priv,
>> +				 struct drm_connector *connector,
>> +				 struct intel_encoder *encoder,
>> +				 bool plugged_in)
>
> plugged_in is redundant, just check for NULL. Also just derive dev_priv
> from intel_encoder imo. And finally we tend to put the object that a
> function operates on first. Since this sends a notify out for the given
> encoder (well dig port really) I'd put that first.

Sure, will fix these and send out a v2 (after Takashi has reviewed, too).

> I also think that it would make sense to switch all the audio
> enable/disable functions from intel_encoder to intel_dig_port. A lot need
> to do upcasts anyway, and audio really only works on dig ports (which is
> for hdmi/dp only).

That's up to you - seems a bit out of scope for this patch set though, I 
assume.

Btw, there are two open questions though that you (or someone else) 
might be able to share some insight on:

  1) The mapping between ports and nid is fixed in patch 4/4, it would 
be good with an Ack from an Intel engineer on this mapping.

  2) Whether to use connector_type / connector_type_id combo or 
connector_name as identification - it seems like X has a different 
naming compared to the kernel ("HDMI2" in X vs "HDMI-A-2" in kernel), 
not sure what Wayland/Mir/others do here...

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[R, Durgadoss]

Hi David,

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson
>Sent: Tuesday, July 21, 2015 1:27 PM
>To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel;
>jani.nikula@linux.intel.com
>Cc: Koul, Vinod; David Henningsson
>Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
>
>This patch set aims to resolve three problems:
>
> * The first - and most serious one - is that the audio driver is not woken up
>   properly when in power save modes, especially not when the HDA controller is
>   in D3. By having the i915 driver call directly into the hda driver, the HDA
>   driver is always notified that an HDMI hotplug event has happened.
>
> * Second, there is currently no way for userspace to match an HDMI audio output
>   with an HDMI video output. We fix this by sending connector_type and
>   connector_type_id in the HDMI hotplug callback.
>
> * Third, writing ELD info to the hardware just so the HDA driver can read it
>   from the hardware seems a bit inefficient. We could just pass that information
>   in the callback, too.
>
>The patch in its current form fixes the first of these problems and provides most
>of the infrastructure for the second and third problem.
>
>The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has
>been tested (and working) on one Skylake machine.

I believe you tested these patches with hda driver after few cycles of D3.
By any chance, did you also try this once after i915 driver's D3 cycle also ?
In this case, can the check_presence_and_report() function get the
pin presence and ELD valid bits read out properly..?

Thanks,
Durga

>
>David Henningsson (4):
>  drm/i915: Add audio hotplug info struct
>  drm/i915: Call audio hotplug notify function
>  ALSA: hda - Dispatch incoming HDMI hotplug i915 callback
>  ALSA: hda - Wake the codec up on hotplug notify events
>
> drivers/gpu/drm/i915/i915_drv.h    |    1 +
> drivers/gpu/drm/i915/intel_audio.c |   46 ++++++++++++++++++++++++++++++++++++
> include/drm/i915_component.h       |   19 +++++++++++++++
> include/sound/hdaudio.h            |    4 ++++
> sound/hda/hdac_i915.c              |   24 +++++++++++++++++++
> sound/pci/hda/patch_hdmi.c         |   22 ++++++++++++++++-
> 6 files changed, 115 insertions(+), 1 deletion(-)
>
>--
>1.7.9.5
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
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 struct

[Takashi Iwai]

On Tue, 21 Jul 2015 09:57:24 +0200,
David Henningsson wrote:
> 
> This struct will be used to transfer information from the i915
> driver to the hda driver on HDMI hotplug events.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>

Looks good to me, just a few nitpicking:

> ---
>  include/drm/i915_component.h |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..4fc0db3 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -24,8 +24,22 @@
>  #ifndef _I915_COMPONENT_H_
>  #define _I915_COMPONENT_H_
>  
> +struct hdac_bus;
> +
> +struct i915_audio_hotplug_info {
> +	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
> +	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
> +	int port; /* Used for mapping to affected nid */
> +	int port_multi_stream_device; /* For DP multi-streaming */
> +
> +	bool plugged_in;
> +	uint8_t *eld;

Use u8 or just unsigned char as it's a in-kernel API.
Also, safer to add const, since this is read-only for audio side.

> +	int eld_size;
> +};
> +
>  struct i915_audio_component {
>  	struct device *dev;
> +	struct hdac_bus *hdac_bus;

If we want to be more generic, using a struct device would be better,
e.g.
	struct device *audio_dev;

>  	const struct i915_audio_component_ops {
>  		struct module *owner;
> @@ -34,6 +48,11 @@ struct i915_audio_component {
>  		void (*codec_wake_override)(struct device *, bool enable);
>  		int (*get_cdclk_freq)(struct device *);
>  	} *ops;
> +
> +	const struct i915_audio_component_cb_ops {
> +		struct module *owner;

Do we need the owner field at all?

> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> +	} *cb_ops;

cb_ops doesn't sound intuitive.  Any better name?


thanks,

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

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

[Takashi Iwai]

On Tue, 21 Jul 2015 09:57:26 +0200,
David Henningsson wrote:
> 
> This lets interested codec(s) be notified of HDMI hotplug
> events sent from the i915 driver.
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  include/sound/hdaudio.h |    4 ++++
>  sound/hda/hdac_i915.c   |   24 ++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index 4caf1fd..ce34182 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -79,6 +79,10 @@ struct hdac_device {
>  	int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
>  			 unsigned int flags, unsigned int *res);
>  
> +	/* Used for hotplug notification from i915 driver */
> +	void (*hotplug_notify)(struct hdac_device *,
> +			       const struct i915_audio_hotplug_info *);

Since this callback is specific to HDMI/DP, a more specific name would
be better.  Otherwise this sounds like a generic hotplug handler.

Or, we may make it really generic, e.g. something like
	void (*hotplug_notify)(struct hdac_device *, int event_type,
				const void *data);

and call it with a specific event_type value
	codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);


thanks,

Takashi
_______________________________________________
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 struct

[David Henningsson]

On 2015-07-22 10:22, Takashi Iwai wrote:
> On Tue, 21 Jul 2015 09:57:24 +0200,
> David Henningsson wrote:
>>
>> This struct will be used to transfer information from the i915
>> driver to the hda driver on HDMI hotplug events.
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>
> Looks good to me, just a few nitpicking:
>
>> ---
>>   include/drm/i915_component.h |   19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>> index c9a8b64..4fc0db3 100644
>> --- a/include/drm/i915_component.h
>> +++ b/include/drm/i915_component.h
>> @@ -24,8 +24,22 @@
>>   #ifndef _I915_COMPONENT_H_
>>   #define _I915_COMPONENT_H_
>>
>> +struct hdac_bus;
>> +
>> +struct i915_audio_hotplug_info {
>> +	int connector_type; /* DRM_MODE_CONNECTOR_*, meant for userspace */
>> +	int connector_type_id; /* Index within a DRM_MODE_CONNECTOR_* type, meant for userspace */
>> +	int port; /* Used for mapping to affected nid */
>> +	int port_multi_stream_device; /* For DP multi-streaming */
>> +
>> +	bool plugged_in;
>> +	uint8_t *eld;
>
> Use u8 or just unsigned char as it's a in-kernel API.
> Also, safer to add const, since this is read-only for audio side.

Ok.

>
>> +	int eld_size;
>> +};
>> +
>>   struct i915_audio_component {
>>   	struct device *dev;
>> +	struct hdac_bus *hdac_bus;
>
> If we want to be more generic, using a struct device would be better,
> e.g.
> 	struct device *audio_dev;

Does this work? If we want to have the hdac_bus.dev ptr instead of a 
hdac_bus ptr, there does not seem to be an obvious way to go from the 
audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
arbitrary dev pointer).

>>   	const struct i915_audio_component_ops {
>>   		struct module *owner;
>> @@ -34,6 +48,11 @@ struct i915_audio_component {
>>   		void (*codec_wake_override)(struct device *, bool enable);
>>   		int (*get_cdclk_freq)(struct device *);
>>   	} *ops;
>> +
>> +	const struct i915_audio_component_cb_ops {
>> +		struct module *owner;
>
> Do we need the owner field at all?

It was merely for symmetry. I'll remove it for v2.

>> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
>> +	} *cb_ops;
>
> cb_ops doesn't sound intuitive.  Any better name?

I was thinking of it as "callback ops", i e, calls that go in the 
reverse direction compared to the already existing "ops".

But if we call the device "audio_dev" as you suggested above, then maybe 
"audio_ops" would be nice and symmetric?

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 21 Jul 2015 19:37:17 +0200,
R, Durgadoss wrote:
> 
> Hi David,
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson
> >Sent: Tuesday, July 21, 2015 1:27 PM
> >To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel;
> >jani.nikula@linux.intel.com
> >Cc: Koul, Vinod; David Henningsson
> >Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
> >
> >This patch set aims to resolve three problems:
> >
> > * The first - and most serious one - is that the audio driver is not woken up
> >   properly when in power save modes, especially not when the HDA controller is
> >   in D3. By having the i915 driver call directly into the hda driver, the HDA
> >   driver is always notified that an HDMI hotplug event has happened.
> >
> > * Second, there is currently no way for userspace to match an HDMI audio output
> >   with an HDMI video output. We fix this by sending connector_type and
> >   connector_type_id in the HDMI hotplug callback.
> >
> > * Third, writing ELD info to the hardware just so the HDA driver can read it
> >   from the hardware seems a bit inefficient. We could just pass that information
> >   in the callback, too.
> >
> >The patch in its current form fixes the first of these problems and provides most
> >of the infrastructure for the second and third problem.
> >
> >The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has
> >been tested (and working) on one Skylake machine.
> 
> I believe you tested these patches with hda driver after few cycles of D3.
> By any chance, did you also try this once after i915 driver's D3 cycle also ?
> In this case, can the check_presence_and_report() function get the
> pin presence and ELD valid bits read out properly..?

The code path will call power well on, thus the i915 driver will be
woken up, in anyway.


thanks,

Takashi
_______________________________________________
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 struct

[Takashi Iwai]

On Wed, 22 Jul 2015 10:50:03 +0200,
David Henningsson wrote:
> 
> >>   struct i915_audio_component {
> >>   	struct device *dev;
> >> +	struct hdac_bus *hdac_bus;
> >
> > If we want to be more generic, using a struct device would be better,
> > e.g.
> > 	struct device *audio_dev;
> 
> Does this work? If we want to have the hdac_bus.dev ptr instead of a 
> hdac_bus ptr, there does not seem to be an obvious way to go from the 
> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
> arbitrary dev pointer).

Hrm, right, currently it's not straightforward.  Scratch the idea,
then.


> >> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> >> +	} *cb_ops;
> >
> > cb_ops doesn't sound intuitive.  Any better name?
> 
> I was thinking of it as "callback ops", i e, calls that go in the 
> reverse direction compared to the already existing "ops".
> 
> But if we call the device "audio_dev" as you suggested above, then maybe 
> "audio_ops" would be nice and symmetric?

Yes, it sounds better.


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

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

[Vinod Koul]

On Wed, Jul 22, 2015 at 10:30:57AM +0200, Takashi Iwai wrote:
> On Tue, 21 Jul 2015 09:57:26 +0200,
> David Henningsson wrote:
> > 
> > This lets interested codec(s) be notified of HDMI hotplug
> > events sent from the i915 driver.
> > 
> > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > ---
> >  include/sound/hdaudio.h |    4 ++++
> >  sound/hda/hdac_i915.c   |   24 ++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > index 4caf1fd..ce34182 100644
> > --- a/include/sound/hdaudio.h
> > +++ b/include/sound/hdaudio.h
> > @@ -79,6 +79,10 @@ struct hdac_device {
> >  	int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
> >  			 unsigned int flags, unsigned int *res);
> >  
> > +	/* Used for hotplug notification from i915 driver */
> > +	void (*hotplug_notify)(struct hdac_device *,
> > +			       const struct i915_audio_hotplug_info *);
> 
> Since this callback is specific to HDMI/DP, a more specific name would
> be better.  Otherwise this sounds like a generic hotplug handler.
> 
> Or, we may make it really generic, e.g. something like
> 	void (*hotplug_notify)(struct hdac_device *, int event_type,
> 				const void *data);
> 
> and call it with a specific event_type value
> 	codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
or should this be added to unsol_event handler for the device, we already
are have worker thread for that.
For device it will be single notify for both SW and HW events. Yes adding
event types will help

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

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

[Takashi Iwai]

On Wed, 22 Jul 2015 15:56:37 +0200,
Vinod Koul wrote:
> 
> On Wed, Jul 22, 2015 at 10:30:57AM +0200, Takashi Iwai wrote:
> > On Tue, 21 Jul 2015 09:57:26 +0200,
> > David Henningsson wrote:
> > > 
> > > This lets interested codec(s) be notified of HDMI hotplug
> > > events sent from the i915 driver.
> > > 
> > > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > > ---
> > >  include/sound/hdaudio.h |    4 ++++
> > >  sound/hda/hdac_i915.c   |   24 ++++++++++++++++++++++++
> > >  2 files changed, 28 insertions(+)
> > > 
> > > diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> > > index 4caf1fd..ce34182 100644
> > > --- a/include/sound/hdaudio.h
> > > +++ b/include/sound/hdaudio.h
> > > @@ -79,6 +79,10 @@ struct hdac_device {
> > >  	int (*exec_verb)(struct hdac_device *dev, unsigned int cmd,
> > >  			 unsigned int flags, unsigned int *res);
> > >  
> > > +	/* Used for hotplug notification from i915 driver */
> > > +	void (*hotplug_notify)(struct hdac_device *,
> > > +			       const struct i915_audio_hotplug_info *);
> > 
> > Since this callback is specific to HDMI/DP, a more specific name would
> > be better.  Otherwise this sounds like a generic hotplug handler.
> > 
> > Or, we may make it really generic, e.g. something like
> > 	void (*hotplug_notify)(struct hdac_device *, int event_type,
> > 				const void *data);
> > 
> > and call it with a specific event_type value
> > 	codec->hotplug_notify(codec, HDAC_NOTIFY_I915_DP, hotplug_info);
> or should this be added to unsol_event handler for the device, we already
> are have worker thread for that.
> For device it will be single notify for both SW and HW events. Yes adding
> event types will help

Well, the difference from the unsol event is that this gives the
certain amount of data together with the notification.  Of course, it
can be a two-step procedure: first notify via a faked unsol event,
then the driver fetches data actively.


Takashi
_______________________________________________
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 struct

[Vinod Koul]

On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
> On Wed, 22 Jul 2015 10:50:03 +0200,
> David Henningsson wrote:
> > 
> > >>   struct i915_audio_component {
> > >>   	struct device *dev;
> > >> +	struct hdac_bus *hdac_bus;
> > >
> > > If we want to be more generic, using a struct device would be better,
> > > e.g.
> > > 	struct device *audio_dev;
> > 
> > Does this work? If we want to have the hdac_bus.dev ptr instead of a 
> > hdac_bus ptr, there does not seem to be an obvious way to go from the 
> > audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an 
> > arbitrary dev pointer).
> 
> Hrm, right, currently it's not straightforward.  Scratch the idea,
> then.

That depends on the device we register this with. Actually this makes more
sense to me :)

If we register with struct device *audio_dev, which in this case would be
the codec device we create while probing the bus. This way you are linking i915
ops to the codec device. Ofcourse hdac_device has bus pointer but you can
invoke device callback without even searching for the device :)

-- 
~Vinod
> 
> 
> > >> +		void (*hotplug_notify)(struct hdac_bus *, const struct i915_audio_hotplug_info *);
> > >> +	} *cb_ops;
> > >
> > > cb_ops doesn't sound intuitive.  Any better name?
> > 
> > I was thinking of it as "callback ops", i e, calls that go in the 
> > reverse direction compared to the already existing "ops".
> > 
> > But if we call the device "audio_dev" as you suggested above, then maybe 
> > "audio_ops" would be nice and symmetric?
> 
> Yes, it sounds better.
> 
> 
> Takashi

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

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

[David Henningsson]

On 2015-07-21 19:37, R, Durgadoss wrote:
> Hi David,
>
>> -----Original Message-----
>> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of David Henningsson
>> Sent: Tuesday, July 21, 2015 1:27 PM
>> To: alsa-devel@alsa-project.org; intel-gfx@lists.freedesktop.org; tiwai@suse.de; Vetter, Daniel;
>> jani.nikula@linux.intel.com
>> Cc: Koul, Vinod; David Henningsson
>> Subject: [Intel-gfx] [PATCH 0/4] i915 to call hda driver on HDMI plug/unplug
>>
>> This patch set aims to resolve three problems:
>>
>> * The first - and most serious one - is that the audio driver is not woken up
>>    properly when in power save modes, especially not when the HDA controller is
>>    in D3. By having the i915 driver call directly into the hda driver, the HDA
>>    driver is always notified that an HDMI hotplug event has happened.
>>
>> * Second, there is currently no way for userspace to match an HDMI audio output
>>    with an HDMI video output. We fix this by sending connector_type and
>>    connector_type_id in the HDMI hotplug callback.
>>
>> * Third, writing ELD info to the hardware just so the HDA driver can read it
>>    from the hardware seems a bit inefficient. We could just pass that information
>>    in the callback, too.
>>
>> The patch in its current form fixes the first of these problems and provides most
>> of the infrastructure for the second and third problem.
>>
>> The patch set is based on 4.2rc2 + my recent codec wakeup patch. So far, it has
>> been tested (and working) on one Skylake machine.
>
> I believe you tested these patches with hda driver after few cycles of D3.
> By any chance, did you also try this once after i915 driver's D3 cycle also ?
> In this case, can the check_presence_and_report() function get the
> pin presence and ELD valid bits read out properly..?

When running this code with drm.debug=0xe, I can see a lot of calls to 
set different power wells on and off, to the extent that I don't keep 
track of them.

So I assume that means that the i915 driver goes into D3 as well during 
my tests.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
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 struct

[David Henningsson]

On 2015-07-22 16:13, Vinod Koul wrote:
> On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
>> On Wed, 22 Jul 2015 10:50:03 +0200,
>> David Henningsson wrote:
>>>
>>>>>    struct i915_audio_component {
>>>>>    	struct device *dev;
>>>>> +	struct hdac_bus *hdac_bus;
>>>>
>>>> If we want to be more generic, using a struct device would be better,
>>>> e.g.
>>>> 	struct device *audio_dev;
>>>
>>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
>>> hdac_bus ptr, there does not seem to be an obvious way to go from the
>>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
>>> arbitrary dev pointer).
>>
>> Hrm, right, currently it's not straightforward.  Scratch the idea,
>> then.
>
> That depends on the device we register this with. Actually this makes more
> sense to me :)
>
> If we register with struct device *audio_dev, which in this case would be
> the codec device we create while probing the bus. This way you are linking i915
> ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> invoke device callback without even searching for the device :)

It would require some extra setup, so I skipped it (at least for now).

I e, in order to detect codecs, we need to call hdac_i915 functions to 
turn the power well on. And in order to connect the codec to the 
i915_audio_component, we need to have detected a codec.

Which, now that I think of it, actually gives an interesting potential 
race condition, in case the following happens:

1) Monitor is plugged in at boot time
2) i915 initializes
3) hda starts initializing and sets up the audio component
4) i915 finishes initialization and as part of that, calls the hotplug 
notify to let hda know that the monitor is plugged in. However, at this 
point, hda has not finished initialization yet, so there are no codecs 
that listen for this information.

Anyhow, this is not a problem really yet, but it might be if we ever 
decide to not write the ELD to the hardware.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
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 struct

[Takashi Iwai]

On Wed, 22 Jul 2015 19:52:23 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-07-22 16:13, Vinod Koul wrote:
> > On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
> >> On Wed, 22 Jul 2015 10:50:03 +0200,
> >> David Henningsson wrote:
> >>>
> >>>>>    struct i915_audio_component {
> >>>>>    	struct device *dev;
> >>>>> +	struct hdac_bus *hdac_bus;
> >>>>
> >>>> If we want to be more generic, using a struct device would be better,
> >>>> e.g.
> >>>> 	struct device *audio_dev;
> >>>
> >>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
> >>> hdac_bus ptr, there does not seem to be an obvious way to go from the
> >>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
> >>> arbitrary dev pointer).
> >>
> >> Hrm, right, currently it's not straightforward.  Scratch the idea,
> >> then.
> >
> > That depends on the device we register this with. Actually this makes more
> > sense to me :)
> >
> > If we register with struct device *audio_dev, which in this case would be
> > the codec device we create while probing the bus. This way you are linking i915
> > ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> > invoke device callback without even searching for the device :)
> 
> It would require some extra setup, so I skipped it (at least for now).
> 
> I e, in order to detect codecs, we need to call hdac_i915 functions to 
> turn the power well on. And in order to connect the codec to the 
> i915_audio_component, we need to have detected a codec.
> 
> Which, now that I think of it, actually gives an interesting potential 
> race condition, in case the following happens:
> 
> 1) Monitor is plugged in at boot time
> 2) i915 initializes
> 3) hda starts initializing and sets up the audio component
> 4) i915 finishes initialization and as part of that, calls the hotplug 
> notify to let hda know that the monitor is plugged in. However, at this 
> point, hda has not finished initialization yet, so there are no codecs 
> that listen for this information.
> 
> Anyhow, this is not a problem really yet, but it might be if we ever 
> decide to not write the ELD to the hardware.

For avoid such a problem, maybe we need two ops, one for notification
and one for getting the assigned data.  At the initialization time,
the audio driver queries the assigned status and data.  When a hotplug
happens, it's just notified.  That is, it simply replaces the current
unsol event and the ELD data read via two ops.


Takashi
_______________________________________________
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 struct

[Vinod Koul]

On Wed, Jul 22, 2015 at 10:31:45PM +0200, Takashi Iwai wrote:
> > > That depends on the device we register this with. Actually this makes more
> > > sense to me :)
> > >
> > > If we register with struct device *audio_dev, which in this case would be
> > > the codec device we create while probing the bus. This way you are linking i915
> > > ops to the codec device. Ofcourse hdac_device has bus pointer but you can
> > > invoke device callback without even searching for the device :)
> > 
> > It would require some extra setup, so I skipped it (at least for now).
> > 
> > I e, in order to detect codecs, we need to call hdac_i915 functions to 
> > turn the power well on. And in order to connect the codec to the 
> > i915_audio_component, we need to have detected a codec.
Yes that is true but when driver registers the callback you can assign the
callback to i915 component, so afterwards the call from i915 lands up in
codec. If not registered we have default bus handler :)

> > Which, now that I think of it, actually gives an interesting potential 
> > race condition, in case the following happens:
> > 
> > 1) Monitor is plugged in at boot time
> > 2) i915 initializes
> > 3) hda starts initializing and sets up the audio component
> > 4) i915 finishes initialization and as part of that, calls the hotplug 
> > notify to let hda know that the monitor is plugged in. However, at this 
> > point, hda has not finished initialization yet, so there are no codecs 
> > that listen for this information.
> > 
> > Anyhow, this is not a problem really yet, but it might be if we ever 
> > decide to not write the ELD to the hardware.
> 
> For avoid such a problem, maybe we need two ops, one for notification
> and one for getting the assigned data.  At the initialization time,
> the audio driver queries the assigned status and data.  When a hotplug
> happens, it's just notified.  That is, it simply replaces the current
> unsol event and the ELD data read via two ops.
Yeah, I do favour adding new ops so that we can query the current values and
setup whenever driver probes

-- 
~Vinod

_______________________________________________
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 struct

[David Henningsson]

On 2015-07-22 22:31, Takashi Iwai wrote:
> On Wed, 22 Jul 2015 19:52:23 +0200,
> David Henningsson wrote:
>>
>>
>>
>> On 2015-07-22 16:13, Vinod Koul wrote:
>>> On Wed, Jul 22, 2015 at 10:55:55AM +0200, Takashi Iwai wrote:
>>>> On Wed, 22 Jul 2015 10:50:03 +0200,
>>>> David Henningsson wrote:
>>>>>
>>>>>>>     struct i915_audio_component {
>>>>>>>     	struct device *dev;
>>>>>>> +	struct hdac_bus *hdac_bus;
>>>>>>
>>>>>> If we want to be more generic, using a struct device would be better,
>>>>>> e.g.
>>>>>> 	struct device *audio_dev;
>>>>>
>>>>> Does this work? If we want to have the hdac_bus.dev ptr instead of a
>>>>> hdac_bus ptr, there does not seem to be an obvious way to go from the
>>>>> audio_dev back to the hdac_bus struct (as snd_hdac_bus_init takes an
>>>>> arbitrary dev pointer).
>>>>
>>>> Hrm, right, currently it's not straightforward.  Scratch the idea,
>>>> then.
>>>
>>> That depends on the device we register this with. Actually this makes more
>>> sense to me :)
>>>
>>> If we register with struct device *audio_dev, which in this case would be
>>> the codec device we create while probing the bus. This way you are linking i915
>>> ops to the codec device. Ofcourse hdac_device has bus pointer but you can
>>> invoke device callback without even searching for the device :)
>>
>> It would require some extra setup, so I skipped it (at least for now).
>>
>> I e, in order to detect codecs, we need to call hdac_i915 functions to
>> turn the power well on. And in order to connect the codec to the
>> i915_audio_component, we need to have detected a codec.
>>
>> Which, now that I think of it, actually gives an interesting potential
>> race condition, in case the following happens:
>>
>> 1) Monitor is plugged in at boot time
>> 2) i915 initializes
>> 3) hda starts initializing and sets up the audio component
>> 4) i915 finishes initialization and as part of that, calls the hotplug
>> notify to let hda know that the monitor is plugged in. However, at this
>> point, hda has not finished initialization yet, so there are no codecs
>> that listen for this information.
>>
>> Anyhow, this is not a problem really yet, but it might be if we ever
>> decide to not write the ELD to the hardware.
>
> For avoid such a problem, maybe we need two ops, one for notification
> and one for getting the assigned data.  At the initialization time,
> the audio driver queries the assigned status and data.  When a hotplug
> happens, it's just notified.  That is, it simply replaces the current
> unsol event and the ELD data read via two ops.

I'm about to go on vacation so it would be good to get some closure 
here. If you both prefer this setup, how about I remove "struct 
i915_audio_hotplug_info" for now? We will then have:

	const struct i915_audio_component_audio_ops {
		void (*hotplug_notify)(struct hdac_bus *);
	}

...which will make the hda driver read from the hardware. Asking the 
i915 driver for ELD, connector and hotplug status can then be done in a 
later patch.

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
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 struct

[David Henningsson]

On 2015-07-23 08:17, David Henningsson wrote:
> I'm about to go on vacation so it would be good to get some closure
> here. If you both prefer this setup, how about I remove "struct
> i915_audio_hotplug_info" for now? We will then have:
>
>      const struct i915_audio_component_audio_ops {
>          void (*hotplug_notify)(struct hdac_bus *);
>      }

Sorry, it would look like this:

       const struct i915_audio_component_audio_ops {
           void (*hotplug_notify)(struct hdac_bus *, int port, int 
port_mst_index);
       }

...to indicate what port needs updating.


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
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 struct

[Takashi Iwai]

On Thu, 23 Jul 2015 08:25:21 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-07-23 08:17, David Henningsson wrote:
> > I'm about to go on vacation so it would be good to get some closure
> > here. If you both prefer this setup, how about I remove "struct
> > i915_audio_hotplug_info" for now? We will then have:
> >
> >      const struct i915_audio_component_audio_ops {
> >          void (*hotplug_notify)(struct hdac_bus *);
> >      }
> 
> Sorry, it would look like this:
> 
>        const struct i915_audio_component_audio_ops {
>            void (*hotplug_notify)(struct hdac_bus *, int port, int 
> port_mst_index);
>        }
> 
> ...to indicate what port needs updating.

Yes, I think this is simpler.

A remaining question is whether it should be notified to bus or
codec.  In the latter case, we need to allow registering the audio
codec after binding the component.

I find the codec can be a bit better, as this is directly targeted
(while for bus you need to look through the codec list).  But it's
just a minor difference, and I don't mind so much about this, if there
is any other difficulty by that move.


thanks,

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