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

[Anssi Hannula <anssi.hannula@iki.fi>]

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@iki.fi>
>>>>> Cc: <stable@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 Hannula