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

[Takashi Iwai]

At Thu, 24 Oct 2013 17:20:09 +0300,
Anssi Hannula wrote:
> 
> 24.10.2013 14:01, Takashi Iwai kirjoitti:
> > At Tue, 15 Oct 2013 19:22:00 +0300,
> > Anssi Hannula wrote:
> >>
> >> 15.10.2013 18:32, Takashi Iwai kirjoitti:
> >>> At Mon, 07 Oct 2013 15:09:15 +0200,
> >>> Takashi Iwai wrote:
> >>>>
> >>>> 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.
> >>>
> >>> [dropped stable from Cc]
> >>>
> >>> Anssi, is the lock fix/cleanup patch ready?
> >>>
> >>> I already have patche to extend mutex lock over other places and
> >>> move it into per_pin, but if you have already similar patches, I'll
> >>> take yours, hopefully together with other fixes.
> >>
> >> Nope, haven't had the time to work on it yet (my excuse is that I was at
> >> XBMC DevCon last weekend :) ).
> >>
> >> So feel free to apply yours and I'll work on top of that (at some point
> >> later this week I guess).
> > 
> > Anssi, do you have the merge-ready version of AMD HDMI fix patches?
> > Since 3.13 merge window will be opened soon, I'd like to merge such
> > big (and good) things now if possible.
> 
> Well, I have finished the hdmi_ops conversion, but the updated patchset
> has not yet been tested on ATI/AMD. But I guess I could post the updated
> patchset anyway (today), and fix any possible remaining issues afterwards...

Yeah, I know that the stuff already worked, so it'd be easy to fix
even if something went wrong by rebasing.


Takashi