[4/4] ALSA: hda - Wake the codec up on pin/ELD notify events
diff mbox

Message ID 1439974138-3296-5-git-send-email-david.henningsson@canonical.com
State New
Headers show

Commit Message

David Henningsson Aug. 19, 2015, 8:48 a.m. UTC
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(-)

Comments

Takashi Iwai Aug. 20, 2015, 9:28 a.m. UTC | #1
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
>
David Henningsson Aug. 20, 2015, 9:41 a.m. UTC | #2
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
>>
>
Takashi Iwai Aug. 20, 2015, 9:46 a.m. UTC | #3
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
>
Jani Nikula Aug. 28, 2015, 1:10 p.m. UTC | #4
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
>>
Daniel Vetter Sept. 2, 2015, 8 a.m. UTC | #5
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
Takashi Iwai Sept. 2, 2015, 8:03 a.m. UTC | #6
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
Daniel Vetter Sept. 2, 2015, 8:32 a.m. UTC | #7
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
Takashi Iwai Sept. 2, 2015, 9:39 a.m. UTC | #8
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

Patch
diff mbox

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;