[alsa-devel] [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect

[Takashi Iwai]

At Mon, 07 Oct 2013 15:45:09 +0300,
Anssi Hannula wrote:
> 
> Takashi Iwai kirjoitti 2013-10-07 13:43:
> > At Mon, 07 Oct 2013 13:31:17 +0300,
> > Anssi Hannula wrote:
> > 
> > Takashi Iwai kirjoitti 2013-10-07 10:48:
> > > At Wed,  2 Oct 2013 21:13:32 +0300,
> > > Anssi Hannula wrote:
> > >
> > > Currently hdmi_setup_audio_infoframe() reprograms the HDA channel
> > > mapping only when the infoframe is not up-to-date or the non-PCM flag
> > > has changed.
> > >
> > > However, when just the channel map has been changed, the infoframe may
> > > still be up-to-date and non-PCM flag may not have changed, so the new
> > > channel map is not actually programmed into the HDA codec.
> > >
> > > Notably, this failing case is also always triggered when the device is
> > > already in a prepared state and a new channel map is configured while
> > > changing only the channel positions (for example, plain
> > > "speaker-test -c2 -m FR,FL").
> > >
> > > Fix that by keeping track of the last programmed channel map and making
> > > hdmi_setup_audio_infoframe() always update the hardware mapping if the
> > > channel map has changed.
> > >
> > > Hmm, maybe it's easier (and safer) to remove the check and always call
> > > hdmi_setup_channel_mapping()?  Of course, the drawback is the
> > > unnecessary verbs, but it's not too much.
> > 
> > Easier, yes. I don't really have that good of an idea how big of a 
> > delay
> > do 8 write verbs cause, so I'll rely on your judgment :)
> > 
> > I guess I could replace the writes with read-and-write-if-differs 
> > cycles
> > when I add the set_slot ops for ATI/AMD, since it seems to be used for
> > other similar cases (like channel count)?
> > 
> > Well, this would result in more cost, as read requires a sync.
> 
> Hmm, so why does e.g. converter channel count set-up use that, then?

It can be optimized as well :)

> > If any, we can use the cached write/update.  But this assumes that the
> > hardware never clears the value by some operation internally.
> 
> It should not, but better be safe than sorry here (at least for stable) 
> :)
> 
> > > Signed-off-by: Anssi Hannula <anssi.hannula at iki.fi>
> > > Cc: <stable at vger.kernel.org>
> > > ---
> > >
> > > BTW, is there some mechanism preventing hdmi_chmap_ctl_put() to be
> > > called
> > > at the same time with generic_hdmi_playback_pcm_prepare() that I didn't
> > > see with a quick look?
> > > Both of them call hdmi_setup_audio_infoframe() which is not
> > > thread-safe.
> > >
> > > Yeah, we need to introduce a mutex there.
> > 
> > I guess hdmi_present_sense() is the same, as it can also call
> > hdmi_setup_audio_infoframe()?
> > 
> > Hm, looking at the code, we have already pin_eld->lock in
> > hdmi_present_sense().  This can be used in other places, too.
> 
> Well, hdmi_setup_audio_infoframe() is non-threadsafe for other reasons 
> than
> eld as well (e.g. write verbs to hw), though, so it might be a bit 
> confusing
> as-is. Better move/rename the lock if we end up using it? Or not?

Yeah, it'd make sense.

> > (And pin_eld->monitor_present change at the beginning of
> > hdmi_present_sense() should be protected in the mutex, too...)
> > 
> > BTW, shouldn't hdmi_present_sense() call hdmi_setup_audio_infoframe()
> > always
> > when eld_changed is true? hdmi_setup_audio_infoframe() does not do its
> > stuff if !monitor_present, so if one starts playback before plugging 
> > in,
> > the channel mappings etc will never be set even when plug-in happens
> > (from what I can see, that is -  did not test yet).
> > 
> > Well, there is such a call but currently specific to Haswell because
> > it hasn't been tested for others.  Maybe we can simply get rid of
> > is_haswell() check there.
> 
> OK, will test later and provide a patch if it works as expected.

Thanks.


Takashi


> 
> > 
> > 
> > 
> > 
> > > thanks,
> > >
> > > Takashi
> > >
> > > sound/pci/hda/patch_hdmi.c | 14 +++++++++++---
> > > 1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index 7ea0245..09f38a1 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -74,6 +74,8 @@ struct hdmi_spec_per_pin {
> > > 	bool non_pcm;
> > > 	bool chmap_set;		/* channel-map override by ALSA API? */
> > > 	unsigned char chmap[8]; /* ALSA API channel-map */
> > > +	bool last_chmap_set;	/* channel-map override programmed? */
> > > +	unsigned char last_chmap[8]; /* last programmed channel-map */
> > > 	char pcm_name[8];	/* filled in build_pcm callbacks */
> > > };
> > >
> > > @@ -955,15 +957,21 @@ static void hdmi_setup_audio_infoframe(struct
> > > hda_codec *codec,
> > > 					    ai.bytes, sizeof(ai));
> > > 		hdmi_start_infoframe_trans(codec, pin_nid);
> > > 	} else {
> > > -		/* For non-pcm audio switch, setup new channel mapping
> > > -		 * accordingly */
> > > -		if (per_pin->non_pcm != non_pcm)
> > > +		/* For non-pcm audio switch or channel-map switch,
> > > +		 * setup new channel mapping accordingly */
> > > +		if (per_pin->non_pcm != non_pcm
> > > +		    || per_pin->chmap_set != per_pin->last_chmap_set
> > > +		    || (per_pin->chmap_set && memcmp(per_pin->chmap,
> > > per_pin->last_chmap,
> > > +						     ARRAY_SIZE(per_pin->chmap)) != 0))
> > > 			hdmi_setup_channel_mapping(codec, pin_nid, non_pcm, ca,
> > > 						   channels, per_pin->chmap,
> > > 						   per_pin->chmap_set);
> > > 	}
> > >
> > > 	per_pin->non_pcm = non_pcm;
> > > +	per_pin->last_chmap_set = per_pin->chmap_set;
> > > +	if (per_pin->chmap_set)
> > > +		memcpy(per_pin->last_chmap, per_pin->chmap,
> > > ARRAY_SIZE(per_pin->chmap));
> > > }
> > >
> > >
> > > --
> > > 1.8.1.5
> > >
> > 
>