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

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

[David Henningsson]

It's been a while since the last patch set iteration, due to me
being on vacation. But here's a new set, and I still hope that it can
make it into the next merge window.

Changes since v3 (with the person suggesting that change within parantheses):
 * Valleyview now has three pins like all the others (Libin Yang)
 * Renamed a few references from "hotplug" to "pin_eld" to reduce 
   confusion with hotplug code on the i915 side (Jani Nikula)
 * Rewrote the dispatching from hda core to codec on the hda side (Takashi Iwai)

This iteration has been tested and working on one Skylake machine.

David Henningsson (4):
  drm/i915: Add audio pin sense / ELD callback
  drm/i915: Call audio pin/ELD notify function
  ALSA: hda - allow codecs to access the i915 pin/ELD callback
  ALSA: hda - Wake the codec up on pin/ELD notify events

 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_audio.c | 23 ++++++++++++++++++++---
 include/drm/i915_component.h       | 12 ++++++++++++
 include/sound/hda_i915.h           |  7 +++++++
 sound/hda/hdac_i915.c              | 10 ++++++++++
 sound/pci/hda/patch_hdmi.c         | 22 +++++++++++++++++++++-
 6 files changed, 71 insertions(+), 4 deletions(-)

-- 
1.9.1

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

[PATCH 1/4] drm/i915: Add audio pin sense / ELD callback

[David Henningsson]

This callback will be called by the i915 driver to notify the hda
driver that its HDMI information needs to be refreshed, i e,
that audio output is now available (or unavailable) - usually as a
result of a monitor being plugged in (or unplugged).

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

diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index c9a8b64..ab5bde37 100644
--- a/include/drm/i915_component.h
+++ b/include/drm/i915_component.h
@@ -34,6 +34,18 @@ 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 *audio_ptr;
+		/**
+		 * Call from i915 driver, notifying the HDA driver that
+		 * pin sense and/or ELD information has changed.
+		 * @audio_ptr:		HDA driver object
+		 * @port:		Which port has changed (PORTA / PORTB / PORTC etc)
+		 * @port_mst_index:	Index within that port, for DisplayPort multistreaming
+		 */
+		void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);
+	} *audio_ops;
 };
 
 #endif /* _I915_COMPONENT_H_ */
-- 
1.9.1

_______________________________________________
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 pin sense / ELD callback

[Daniel Vetter]

On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
> This callback will be called by the i915 driver to notify the hda
> driver that its HDMI information needs to be refreshed, i e,
> that audio output is now available (or unavailable) - usually as a
> result of a monitor being plugged in (or unplugged).
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  include/drm/i915_component.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..ab5bde37 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -34,6 +34,18 @@ 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 *audio_ptr;
> +		/**
> +		 * Call from i915 driver, notifying the HDA driver that
> +		 * pin sense and/or ELD information has changed.
> +		 * @audio_ptr:		HDA driver object
> +		 * @port:		Which port has changed (PORTA / PORTB / PORTC etc)
> +		 * @port_mst_index:	Index within that port, for DisplayPort multistreaming
> +		 */
> +		void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);
> +	} *audio_ops;

This won't work as proper kerneldoc, but you get away with it since it's
not pulled into the drm.tmpl. See my comments for the new set_audio_rate
callback.
-Daniel

>  };
>  
>  #endif /* _I915_COMPONENT_H_ */
> -- 
> 1.9.1
> 
> _______________________________________________
> 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: [alsa-devel] [PATCH 1/4] drm/i915: Add audio pin sense / ELD callback

[David Henningsson]

On 2015-08-26 10:33, Daniel Vetter wrote:
> On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
>> This callback will be called by the i915 driver to notify the hda
>> driver that its HDMI information needs to be refreshed, i e,
>> that audio output is now available (or unavailable) - usually as a
>> result of a monitor being plugged in (or unplugged).
>>
>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>> ---
>>   include/drm/i915_component.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>> index c9a8b64..ab5bde37 100644
>> --- a/include/drm/i915_component.h
>> +++ b/include/drm/i915_component.h
>> @@ -34,6 +34,18 @@ 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 *audio_ptr;
>> +		/**
>> +		 * Call from i915 driver, notifying the HDA driver that
>> +		 * pin sense and/or ELD information has changed.
>> +		 * @audio_ptr:		HDA driver object
>> +		 * @port:		Which port has changed (PORTA / PORTB / PORTC etc)
>> +		 * @port_mst_index:	Index within that port, for DisplayPort multistreaming
>> +		 */
>> +		void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);
>> +	} *audio_ops;
>
> This won't work as proper kerneldoc, but you get away with it since it's
> not pulled into the drm.tmpl. See my comments for the new set_audio_rate
> callback.

Sorry, my google failed me, so I can't find your comments for the 
set_audio_rate callback.

Apart from the kerneldoc issue, are you okay with acking the patch, at 
least the first two i915 ones, and agree with Takashi which tree this 
should go through?

-- 
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: [alsa-devel] [PATCH 1/4] drm/i915: Add audio pin sense / ELD callback

[Jani Nikula]

On Wed, 26 Aug 2015, David Henningsson <david.henningsson@canonical.com> wrote:
> On 2015-08-26 10:33, Daniel Vetter wrote:
>> On Wed, Aug 19, 2015 at 10:48:55AM +0200, David Henningsson wrote:
>>> This callback will be called by the i915 driver to notify the hda
>>> driver that its HDMI information needs to be refreshed, i e,
>>> that audio output is now available (or unavailable) - usually as a
>>> result of a monitor being plugged in (or unplugged).
>>>
>>> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
>>> ---
>>>   include/drm/i915_component.h | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
>>> index c9a8b64..ab5bde37 100644
>>> --- a/include/drm/i915_component.h
>>> +++ b/include/drm/i915_component.h
>>> @@ -34,6 +34,18 @@ 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 *audio_ptr;
>>> +		/**
>>> +		 * Call from i915 driver, notifying the HDA driver that
>>> +		 * pin sense and/or ELD information has changed.
>>> +		 * @audio_ptr:		HDA driver object
>>> +		 * @port:		Which port has changed (PORTA / PORTB / PORTC etc)
>>> +		 * @port_mst_index:	Index within that port, for DisplayPort multistreaming
>>> +		 */
>>> +		void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);
>>> +	} *audio_ops;
>>
>> This won't work as proper kerneldoc, but you get away with it since it's
>> not pulled into the drm.tmpl. See my comments for the new set_audio_rate
>> callback.
>
> Sorry, my google failed me, so I can't find your comments for the 
> set_audio_rate callback.

It's on the related thread [1], specifically the subthread starting at
[2]. I guess there's no direct overlap between the two series, but it
would be helpful if you can look at each other's work so there's no
surprises.

BR,
Jani.


[1] http://mid.gmane.org/1439880714-40931-1-git-send-email-libin.yang@intel.com
[2] http://mid.gmane.org/20150826081735.GZ20434@phenom.ffwll.local

>
> Apart from the kerneldoc issue, are you okay with acking the patch, at 
> least the first two i915 ones, and agree with Takashi which tree this 
> should go through?
>
> -- 
> David Henningsson, Canonical Ltd.
> https://launchpad.net/~diwic

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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 pin sense / ELD callback

[Jani Nikula]

On Wed, 19 Aug 2015, David Henningsson <david.henningsson@canonical.com> wrote:
> This callback will be called by the i915 driver to notify the hda
> driver that its HDMI information needs to be refreshed, i e,
> that audio output is now available (or unavailable) - usually as a
> result of a monitor being plugged in (or unplugged).
>
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  include/drm/i915_component.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index c9a8b64..ab5bde37 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -34,6 +34,18 @@ 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 *audio_ptr;
> +		/**
> +		 * Call from i915 driver, notifying the HDA driver that
> +		 * pin sense and/or ELD information has changed.
> +		 * @audio_ptr:		HDA driver object
> +		 * @port:		Which port has changed (PORTA / PORTB / PORTC etc)
> +		 * @port_mst_index:	Index within that port, for DisplayPort multistreaming
> +		 */
> +		void (*pin_eld_notify)(void *audio_ptr, int port, int port_mst_index);

In the HDMI sample rate notification series [1] I explicitly asked Libin
to drop a similar port_mst_index parameter since we don't use it yet,
and we are not sure what to stick there yet. Indeed not having it now
enforces us to change the API and check for each call site when enabling
MST audio.

Other than that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



[1] http://mid.gmane.org/1439880714-40931-1-git-send-email-libin.yang@intel.com

> +	} *audio_ops;
>  };
>  
>  #endif /* _I915_COMPONENT_H_ */
> -- 
> 1.9.1
>

-- 
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 pin/ELD notify function

[David Henningsson]

When the audio codec is enabled or disabled, 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 fd1de45..1fc327d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1809,6 +1809,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..969835d 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->pin_eld_notify)
+		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->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->pin_eld_notify)
+		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->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.9.1

_______________________________________________
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 pin/ELD notify function

[Jani Nikula]

On Wed, 19 Aug 2015, David Henningsson <david.henningsson@canonical.com> wrote:
> When the audio codec is enabled or disabled, 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 fd1de45..1fc327d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1809,6 +1809,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..969835d 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->pin_eld_notify)
> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->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->pin_eld_notify)
> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);

AFAICT this has a race with i915_audio_component_unbind, i.e. modeset
while hda is being unloaded might crash.

What do others think, is drm_modeset_lock_all in combonent bind/unbind
overkill?

With that resolved one way or another (I'm not dismissing "we don't need
to care") this is

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>  }
>  
>  /**
> @@ -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.9.1
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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 pin/ELD notify function

[David Henningsson]

On 2015-08-28 15:07, Jani Nikula wrote:
> On Wed, 19 Aug 2015, David Henningsson <david.henningsson@canonical.com> wrote:
>> When the audio codec is enabled or disabled, 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 fd1de45..1fc327d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1809,6 +1809,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..969835d 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->pin_eld_notify)
>> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->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->pin_eld_notify)
>> +		acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr, (int) port, 0);
>
> AFAICT this has a race with i915_audio_component_unbind, i.e. modeset
> while hda is being unloaded might crash.
>
> What do others think, is drm_modeset_lock_all in combonent bind/unbind
> overkill?

Thanks for the review(s). I'm happy to add a 
drm_modeset_lock_all/drm_modeset_unlock_all - I'm not that familiar with 
the graphics side, really, so, is there another option...?

>
> With that resolved one way or another (I'm not dismissing "we don't need
> to care") this is
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
>>   }
>>
>>   /**
>> @@ -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.9.1
>>
>

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

[PATCH 3/4] ALSA: hda - allow codecs to access the i915 pin/ELD callback

[David Henningsson]

This lets the interested codec be notified when an i915 pin/ELD
event happens.

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 include/sound/hda_i915.h |  7 +++++++
 sound/hda/hdac_i915.c    | 10 ++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/sound/hda_i915.h b/include/sound/hda_i915.h
index adb5ba5..a0a1d67 100644
--- a/include/sound/hda_i915.h
+++ b/include/sound/hda_i915.h
@@ -4,12 +4,15 @@
 #ifndef __SOUND_HDA_I915_H
 #define __SOUND_HDA_I915_H
 
+#include <drm/i915_component.h>
+
 #ifdef CONFIG_SND_HDA_I915
 int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable);
 int snd_hdac_display_power(struct hdac_bus *bus, bool enable);
 int snd_hdac_get_display_clk(struct hdac_bus *bus);
 int snd_hdac_i915_init(struct hdac_bus *bus);
 int snd_hdac_i915_exit(struct hdac_bus *bus);
+int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *);
 #else
 static int snd_hdac_set_codec_wakeup(struct hdac_bus *bus, bool enable)
 {
@@ -31,6 +34,10 @@ static inline int snd_hdac_i915_exit(struct hdac_bus *bus)
 {
 	return 0;
 }
+static inline int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __SOUND_HDA_I915_H */
diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 5676b84..55c3df4 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -134,6 +134,16 @@ static int hdac_component_master_match(struct device *dev, void *data)
 	return !strcmp(dev->driver->name, "i915");
 }
 
+int snd_hdac_i915_register_notifier(const struct i915_audio_component_audio_ops *aops)
+{
+	if (WARN_ON(!hdac_acomp))
+		return -ENODEV;
+
+	hdac_acomp->audio_ops = aops;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_i915_register_notifier);
+
 int snd_hdac_i915_init(struct hdac_bus *bus)
 {
 	struct component_match *match = NULL;
-- 
1.9.1

_______________________________________________
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 pin/ELD 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 | 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 a97db5f..932292c 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,8 @@
 #include <sound/jack.h>
 #include <sound/asoundef.h>
 #include <sound/tlv.h>
+#include <sound/hdaudio.h>
+#include <sound/hda_i915.h>
 #include "hda_codec.h"
 #include "hda_local.h"
 #include "hda_jack.h"
@@ -144,6 +146,9 @@ struct hdmi_spec {
 	 */
 	struct hda_multi_out multiout;
 	struct hda_pcm_stream pcm_playback;
+
+	/* i915/powerwell (Haswell+/Valleyview+) specific */
+	struct i915_audio_component_audio_ops i915_audio_ops;
 };
 
 
@@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
 	struct hdmi_spec *spec = codec->spec;
 	int pin_idx;
 
+	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
+		snd_hdac_i915_register_notifier(NULL);
+
 	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
 		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
 
@@ -2316,6 +2324,14 @@ 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_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
+{
+	struct hda_codec *codec = audio_ptr;
+	int pin_nid = port + 0x04;
+
+	check_presence_and_report(codec, pin_nid);
+}
+
 static int patch_generic_hdmi(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
@@ -2342,8 +2358,12 @@ 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;
+		spec->i915_audio_ops.audio_ptr = codec;
+		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
+		snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
+	}
 
 	if (hdmi_parse_codec(codec) < 0) {
 		codec->spec = NULL;
-- 
1.9.1

_______________________________________________
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 pin/ELD notify events

[Takashi Iwai]

On Wed, 19 Aug 2015 10:48:58 +0200,
David Henningsson wrote:
> 
> 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>

This addition looks fine, but then we'll get double notification for
the normal hotplug/unplug, one via component ops and another via unsol
event?


thanks,

Takashi

> ---
>  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 a97db5f..932292c 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -37,6 +37,8 @@
>  #include <sound/jack.h>
>  #include <sound/asoundef.h>
>  #include <sound/tlv.h>
> +#include <sound/hdaudio.h>
> +#include <sound/hda_i915.h>
>  #include "hda_codec.h"
>  #include "hda_local.h"
>  #include "hda_jack.h"
> @@ -144,6 +146,9 @@ struct hdmi_spec {
>  	 */
>  	struct hda_multi_out multiout;
>  	struct hda_pcm_stream pcm_playback;
> +
> +	/* i915/powerwell (Haswell+/Valleyview+) specific */
> +	struct i915_audio_component_audio_ops i915_audio_ops;
>  };
>  
>  
> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
>  	struct hdmi_spec *spec = codec->spec;
>  	int pin_idx;
>  
> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> +		snd_hdac_i915_register_notifier(NULL);
> +
>  	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>  		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>  
> @@ -2316,6 +2324,14 @@ 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_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
> +{
> +	struct hda_codec *codec = audio_ptr;
> +	int pin_nid = port + 0x04;
> +
> +	check_presence_and_report(codec, pin_nid);
> +}
> +
>  static int patch_generic_hdmi(struct hda_codec *codec)
>  {
>  	struct hdmi_spec *spec;
> @@ -2342,8 +2358,12 @@ 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;
> +		spec->i915_audio_ops.audio_ptr = codec;
> +		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
> +		snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
> +	}
>  
>  	if (hdmi_parse_codec(codec) < 0) {
>  		codec->spec = NULL;
> -- 
> 1.9.1
> 
_______________________________________________
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 pin/ELD notify events

[David Henningsson]

On 2015-08-20 11:28, Takashi Iwai wrote:
> On Wed, 19 Aug 2015 10:48:58 +0200,
> David Henningsson wrote:
>>
>> 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>
>
> This addition looks fine, but then we'll get double notification for
> the normal hotplug/unplug, one via component ops and another via unsol
> event?

Right, in case the unsol event actually works...

I would argue that the normal case would be that the controller and 
codec is in D3 which means that the unsol event never gets through - due 
to hw limitations - which is what triggered this patch set in the first 
place.

But yes, in some case we might get double notification, but this should 
not cause any trouble in practice. The unsol event could be turned off, 
but would it be okay to save that for a later patch set (so I don't miss 
the upcoming merge window)?

>
>
> thanks,
>
> Takashi
>
>> ---
>>   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 a97db5f..932292c 100644
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -37,6 +37,8 @@
>>   #include <sound/jack.h>
>>   #include <sound/asoundef.h>
>>   #include <sound/tlv.h>
>> +#include <sound/hdaudio.h>
>> +#include <sound/hda_i915.h>
>>   #include "hda_codec.h"
>>   #include "hda_local.h"
>>   #include "hda_jack.h"
>> @@ -144,6 +146,9 @@ struct hdmi_spec {
>>   	 */
>>   	struct hda_multi_out multiout;
>>   	struct hda_pcm_stream pcm_playback;
>> +
>> +	/* i915/powerwell (Haswell+/Valleyview+) specific */
>> +	struct i915_audio_component_audio_ops i915_audio_ops;
>>   };
>>
>>
>> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
>>   	struct hdmi_spec *spec = codec->spec;
>>   	int pin_idx;
>>
>> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
>> +		snd_hdac_i915_register_notifier(NULL);
>> +
>>   	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>>   		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>>
>> @@ -2316,6 +2324,14 @@ 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_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
>> +{
>> +	struct hda_codec *codec = audio_ptr;
>> +	int pin_nid = port + 0x04;
>> +
>> +	check_presence_and_report(codec, pin_nid);
>> +}
>> +
>>   static int patch_generic_hdmi(struct hda_codec *codec)
>>   {
>>   	struct hdmi_spec *spec;
>> @@ -2342,8 +2358,12 @@ 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;
>> +		spec->i915_audio_ops.audio_ptr = codec;
>> +		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
>> +		snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
>> +	}
>>
>>   	if (hdmi_parse_codec(codec) < 0) {
>>   		codec->spec = NULL;
>> --
>> 1.9.1
>>
>

-- 
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 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

[Takashi Iwai]

On Thu, 20 Aug 2015 11:41:42 +0200,
David Henningsson wrote:
> 
> 
> 
> On 2015-08-20 11:28, Takashi Iwai wrote:
> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > David Henningsson wrote:
> >>
> >> 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>
> >
> > This addition looks fine, but then we'll get double notification for
> > the normal hotplug/unplug, one via component ops and another via unsol
> > event?
> 
> Right, in case the unsol event actually works...
> 
> I would argue that the normal case would be that the controller and 
> codec is in D3 which means that the unsol event never gets through - due 
> to hw limitations - which is what triggered this patch set in the first 
> place.
> 
> But yes, in some case we might get double notification, but this should 
> not cause any trouble in practice. The unsol event could be turned off, 
> but would it be okay to save that for a later patch set (so I don't miss 
> the upcoming merge window)?

In that case, it should be mentioned in the changelog at least.

This series came a bit too late for the merge window, so I'm not sure
whether this can get in.  I personally find it OK, so take my ack for
ALSA parts (patch 3/4), but the rest need review and ack from i915
guys.  And we don't know who to merge this, if any.  The changes are
almost even to i915 and hda.  I don't mind either way, via drm or
sound tree.

In anyway,
   Reviewed-by: Takashi Iwai <tiwai@suse.de>


thanks,

Takashi

> >
> >
> > thanks,
> >
> > Takashi
> >
> >> ---
> >>   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 a97db5f..932292c 100644
> >> --- a/sound/pci/hda/patch_hdmi.c
> >> +++ b/sound/pci/hda/patch_hdmi.c
> >> @@ -37,6 +37,8 @@
> >>   #include <sound/jack.h>
> >>   #include <sound/asoundef.h>
> >>   #include <sound/tlv.h>
> >> +#include <sound/hdaudio.h>
> >> +#include <sound/hda_i915.h>
> >>   #include "hda_codec.h"
> >>   #include "hda_local.h"
> >>   #include "hda_jack.h"
> >> @@ -144,6 +146,9 @@ struct hdmi_spec {
> >>   	 */
> >>   	struct hda_multi_out multiout;
> >>   	struct hda_pcm_stream pcm_playback;
> >> +
> >> +	/* i915/powerwell (Haswell+/Valleyview+) specific */
> >> +	struct i915_audio_component_audio_ops i915_audio_ops;
> >>   };
> >>
> >>
> >> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
> >>   	struct hdmi_spec *spec = codec->spec;
> >>   	int pin_idx;
> >>
> >> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
> >> +		snd_hdac_i915_register_notifier(NULL);
> >> +
> >>   	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
> >>   		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> >>
> >> @@ -2316,6 +2324,14 @@ 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_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
> >> +{
> >> +	struct hda_codec *codec = audio_ptr;
> >> +	int pin_nid = port + 0x04;
> >> +
> >> +	check_presence_and_report(codec, pin_nid);
> >> +}
> >> +
> >>   static int patch_generic_hdmi(struct hda_codec *codec)
> >>   {
> >>   	struct hdmi_spec *spec;
> >> @@ -2342,8 +2358,12 @@ 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;
> >> +		spec->i915_audio_ops.audio_ptr = codec;
> >> +		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
> >> +		snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
> >> +	}
> >>
> >>   	if (hdmi_parse_codec(codec) < 0) {
> >>   		codec->spec = NULL;
> >> --
> >> 1.9.1
> >>
> >
> 
> -- 
> 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 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

[Jani Nikula]

On Thu, 20 Aug 2015, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 20 Aug 2015 11:41:42 +0200,
> David Henningsson wrote:
>> 
>> 
>> 
>> On 2015-08-20 11:28, Takashi Iwai wrote:
>> > On Wed, 19 Aug 2015 10:48:58 +0200,
>> > David Henningsson wrote:
>> >>
>> >> 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>
>> >
>> > This addition looks fine, but then we'll get double notification for
>> > the normal hotplug/unplug, one via component ops and another via unsol
>> > event?
>> 
>> Right, in case the unsol event actually works...
>> 
>> I would argue that the normal case would be that the controller and 
>> codec is in D3 which means that the unsol event never gets through - due 
>> to hw limitations - which is what triggered this patch set in the first 
>> place.
>> 
>> But yes, in some case we might get double notification, but this should 
>> not cause any trouble in practice. The unsol event could be turned off, 
>> but would it be okay to save that for a later patch set (so I don't miss 
>> the upcoming merge window)?
>
> In that case, it should be mentioned in the changelog at least.
>
> This series came a bit too late for the merge window, so I'm not sure
> whether this can get in.  I personally find it OK, so take my ack for
> ALSA parts (patch 3/4), but the rest need review and ack from i915
> guys.  And we don't know who to merge this, if any.  The changes are
> almost even to i915 and hda.  I don't mind either way, via drm or
> sound tree.

Personally I'm fine with this still going for v4.3 since to me it looks
like any regressions this might cause will be on your side. *grin*.

BR,
Jani.


>
> In anyway,
>    Reviewed-by: Takashi Iwai <tiwai@suse.de>
>
>
> thanks,
>
> Takashi
>
>> >
>> >
>> > thanks,
>> >
>> > Takashi
>> >
>> >> ---
>> >>   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 a97db5f..932292c 100644
>> >> --- a/sound/pci/hda/patch_hdmi.c
>> >> +++ b/sound/pci/hda/patch_hdmi.c
>> >> @@ -37,6 +37,8 @@
>> >>   #include <sound/jack.h>
>> >>   #include <sound/asoundef.h>
>> >>   #include <sound/tlv.h>
>> >> +#include <sound/hdaudio.h>
>> >> +#include <sound/hda_i915.h>
>> >>   #include "hda_codec.h"
>> >>   #include "hda_local.h"
>> >>   #include "hda_jack.h"
>> >> @@ -144,6 +146,9 @@ struct hdmi_spec {
>> >>   	 */
>> >>   	struct hda_multi_out multiout;
>> >>   	struct hda_pcm_stream pcm_playback;
>> >> +
>> >> +	/* i915/powerwell (Haswell+/Valleyview+) specific */
>> >> +	struct i915_audio_component_audio_ops i915_audio_ops;
>> >>   };
>> >>
>> >>
>> >> @@ -2191,6 +2196,9 @@ static void generic_hdmi_free(struct hda_codec *codec)
>> >>   	struct hdmi_spec *spec = codec->spec;
>> >>   	int pin_idx;
>> >>
>> >> +	if (is_haswell_plus(codec) || is_valleyview_plus(codec))
>> >> +		snd_hdac_i915_register_notifier(NULL);
>> >> +
>> >>   	for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) {
>> >>   		struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>> >>
>> >> @@ -2316,6 +2324,14 @@ 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_pin_eld_notify(void *audio_ptr, int port, int port_mst_index)
>> >> +{
>> >> +	struct hda_codec *codec = audio_ptr;
>> >> +	int pin_nid = port + 0x04;
>> >> +
>> >> +	check_presence_and_report(codec, pin_nid);
>> >> +}
>> >> +
>> >>   static int patch_generic_hdmi(struct hda_codec *codec)
>> >>   {
>> >>   	struct hdmi_spec *spec;
>> >> @@ -2342,8 +2358,12 @@ 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;
>> >> +		spec->i915_audio_ops.audio_ptr = codec;
>> >> +		spec->i915_audio_ops.pin_eld_notify = intel_pin_eld_notify;
>> >> +		snd_hdac_i915_register_notifier(&spec->i915_audio_ops);
>> >> +	}
>> >>
>> >>   	if (hdmi_parse_codec(codec) < 0) {
>> >>   		codec->spec = NULL;
>> >> --
>> >> 1.9.1
>> >>
>> >
>> 
>> -- 
>> David Henningsson, Canonical Ltd.
>> https://launchpad.net/~diwic
>> 

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
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 pin/ELD notify events

[Daniel Vetter]

On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> On Thu, 20 Aug 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > On Thu, 20 Aug 2015 11:41:42 +0200,
> > David Henningsson wrote:
> >> 
> >> 
> >> 
> >> On 2015-08-20 11:28, Takashi Iwai wrote:
> >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> >> > David Henningsson wrote:
> >> >>
> >> >> 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>
> >> >
> >> > This addition looks fine, but then we'll get double notification for
> >> > the normal hotplug/unplug, one via component ops and another via unsol
> >> > event?
> >> 
> >> Right, in case the unsol event actually works...
> >> 
> >> I would argue that the normal case would be that the controller and 
> >> codec is in D3 which means that the unsol event never gets through - due 
> >> to hw limitations - which is what triggered this patch set in the first 
> >> place.
> >> 
> >> But yes, in some case we might get double notification, but this should 
> >> not cause any trouble in practice. The unsol event could be turned off, 
> >> but would it be okay to save that for a later patch set (so I don't miss 
> >> the upcoming merge window)?
> >
> > In that case, it should be mentioned in the changelog at least.
> >
> > This series came a bit too late for the merge window, so I'm not sure
> > whether this can get in.  I personally find it OK, so take my ack for
> > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > guys.  And we don't know who to merge this, if any.  The changes are
> > almost even to i915 and hda.  I don't mind either way, via drm or
> > sound tree.
> 
> Personally I'm fine with this still going for v4.3 since to me it looks
> like any regressions this might cause will be on your side. *grin*.

The only bit I want is some decent kerneldoc for the ad-hoc growing
i915/hda-intel interfaces. But that can be done in follow-up patches and
needs to be coordinated with the audio rate handling series anyway. So ack
from my side for all getting merged through alsa trees too.
-Daniel
-- 
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 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

[Takashi Iwai]

On Wed, 02 Sep 2015 10:00:44 +0200,
Daniel Vetter wrote:
> 
> On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > On Thu, 20 Aug 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > David Henningsson wrote:
> > >> 
> > >> 
> > >> 
> > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > >> > David Henningsson wrote:
> > >> >>
> > >> >> 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>
> > >> >
> > >> > This addition looks fine, but then we'll get double notification for
> > >> > the normal hotplug/unplug, one via component ops and another via unsol
> > >> > event?
> > >> 
> > >> Right, in case the unsol event actually works...
> > >> 
> > >> I would argue that the normal case would be that the controller and 
> > >> codec is in D3 which means that the unsol event never gets through - due 
> > >> to hw limitations - which is what triggered this patch set in the first 
> > >> place.
> > >> 
> > >> But yes, in some case we might get double notification, but this should 
> > >> not cause any trouble in practice. The unsol event could be turned off, 
> > >> but would it be okay to save that for a later patch set (so I don't miss 
> > >> the upcoming merge window)?
> > >
> > > In that case, it should be mentioned in the changelog at least.
> > >
> > > This series came a bit too late for the merge window, so I'm not sure
> > > whether this can get in.  I personally find it OK, so take my ack for
> > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > guys.  And we don't know who to merge this, if any.  The changes are
> > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > sound tree.
> > 
> > Personally I'm fine with this still going for v4.3 since to me it looks
> > like any regressions this might cause will be on your side. *grin*.
> 
> The only bit I want is some decent kerneldoc for the ad-hoc growing
> i915/hda-intel interfaces. But that can be done in follow-up patches and
> needs to be coordinated with the audio rate handling series anyway. So ack
> from my side for all getting merged through alsa trees too.

Are you going to merge Libin's patchset?  If yes, it's better to align
both patchsets through a single tree.  It'll make merges easier, as
both touch the similar place.


thanks,

Takashi
_______________________________________________
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 pin/ELD notify events

[Daniel Vetter]

On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
> On Wed, 02 Sep 2015 10:00:44 +0200,
> Daniel Vetter wrote:
> > 
> > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > > On Thu, 20 Aug 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > > David Henningsson wrote:
> > > >> 
> > > >> 
> > > >> 
> > > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > > >> > David Henningsson wrote:
> > > >> >>
> > > >> >> 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>
> > > >> >
> > > >> > This addition looks fine, but then we'll get double notification for
> > > >> > the normal hotplug/unplug, one via component ops and another via unsol
> > > >> > event?
> > > >> 
> > > >> Right, in case the unsol event actually works...
> > > >> 
> > > >> I would argue that the normal case would be that the controller and 
> > > >> codec is in D3 which means that the unsol event never gets through - due 
> > > >> to hw limitations - which is what triggered this patch set in the first 
> > > >> place.
> > > >> 
> > > >> But yes, in some case we might get double notification, but this should 
> > > >> not cause any trouble in practice. The unsol event could be turned off, 
> > > >> but would it be okay to save that for a later patch set (so I don't miss 
> > > >> the upcoming merge window)?
> > > >
> > > > In that case, it should be mentioned in the changelog at least.
> > > >
> > > > This series came a bit too late for the merge window, so I'm not sure
> > > > whether this can get in.  I personally find it OK, so take my ack for
> > > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > > guys.  And we don't know who to merge this, if any.  The changes are
> > > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > > sound tree.
> > > 
> > > Personally I'm fine with this still going for v4.3 since to me it looks
> > > like any regressions this might cause will be on your side. *grin*.
> > 
> > The only bit I want is some decent kerneldoc for the ad-hoc growing
> > i915/hda-intel interfaces. But that can be done in follow-up patches and
> > needs to be coordinated with the audio rate handling series anyway. So ack
> > from my side for all getting merged through alsa trees too.
> 
> Are you going to merge Libin's patchset?  If yes, it's better to align
> both patchsets through a single tree.  It'll make merges easier, as
> both touch the similar place.

Oh that was an ack for merging it all through your tree. If you punt it to
4.4 I might ask you for a stable tree to pull in in case I get conflicts.
-Daniel
-- 
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 4/4] ALSA: hda - Wake the codec up on pin/ELD notify events

[Takashi Iwai]

On Wed, 02 Sep 2015 10:32:34 +0200,
Daniel Vetter wrote:
> 
> On Wed, Sep 02, 2015 at 10:03:55AM +0200, Takashi Iwai wrote:
> > On Wed, 02 Sep 2015 10:00:44 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Fri, Aug 28, 2015 at 04:10:36PM +0300, Jani Nikula wrote:
> > > > On Thu, 20 Aug 2015, Takashi Iwai <tiwai@suse.de> wrote:
> > > > > On Thu, 20 Aug 2015 11:41:42 +0200,
> > > > > David Henningsson wrote:
> > > > >> 
> > > > >> 
> > > > >> 
> > > > >> On 2015-08-20 11:28, Takashi Iwai wrote:
> > > > >> > On Wed, 19 Aug 2015 10:48:58 +0200,
> > > > >> > David Henningsson wrote:
> > > > >> >>
> > > > >> >> 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>
> > > > >> >
> > > > >> > This addition looks fine, but then we'll get double notification for
> > > > >> > the normal hotplug/unplug, one via component ops and another via unsol
> > > > >> > event?
> > > > >> 
> > > > >> Right, in case the unsol event actually works...
> > > > >> 
> > > > >> I would argue that the normal case would be that the controller and 
> > > > >> codec is in D3 which means that the unsol event never gets through - due 
> > > > >> to hw limitations - which is what triggered this patch set in the first 
> > > > >> place.
> > > > >> 
> > > > >> But yes, in some case we might get double notification, but this should 
> > > > >> not cause any trouble in practice. The unsol event could be turned off, 
> > > > >> but would it be okay to save that for a later patch set (so I don't miss 
> > > > >> the upcoming merge window)?
> > > > >
> > > > > In that case, it should be mentioned in the changelog at least.
> > > > >
> > > > > This series came a bit too late for the merge window, so I'm not sure
> > > > > whether this can get in.  I personally find it OK, so take my ack for
> > > > > ALSA parts (patch 3/4), but the rest need review and ack from i915
> > > > > guys.  And we don't know who to merge this, if any.  The changes are
> > > > > almost even to i915 and hda.  I don't mind either way, via drm or
> > > > > sound tree.
> > > > 
> > > > Personally I'm fine with this still going for v4.3 since to me it looks
> > > > like any regressions this might cause will be on your side. *grin*.
> > > 
> > > The only bit I want is some decent kerneldoc for the ad-hoc growing
> > > i915/hda-intel interfaces. But that can be done in follow-up patches and
> > > needs to be coordinated with the audio rate handling series anyway. So ack
> > > from my side for all getting merged through alsa trees too.
> > 
> > Are you going to merge Libin's patchset?  If yes, it's better to align
> > both patchsets through a single tree.  It'll make merges easier, as
> > both touch the similar place.
> 
> Oh that was an ack for merging it all through your tree. If you punt it to
> 4.4 I might ask you for a stable tree to pull in in case I get conflicts.

OK, then I'll merge David's patchset now for the upcoming pull request
for 4.3-rc1.


thanks,

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