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

[Jani Nikula <jani.nikula@linux.intel.com>]

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