From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anssi Hannula Subject: Re: [PATCH] ALSA: hda - hdmi: Fix channel map switch not taking effect Date: Tue, 15 Oct 2013 19:22:00 +0300 Message-ID: <525D6BA8.3000804@iki.fi> References: <1380737612-16512-1-git-send-email-anssi.hannula@iki.fi> <73e325dd7c696e2d16946890a66dc838@mail.onse.fi> <884f0056bbbc27a2f35c581de11a07d9@mail.onse.fi> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from tulikuusama.dnainternet.net (tulikuusama.dnainternet.net [83.102.40.132]) by alsa0.perex.cz (Postfix) with ESMTP id 1F7282610C5 for ; Tue, 15 Oct 2013 18:22:04 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Iwai Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org 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 >>>>> Cc: >>>>> --- >>>>> >>>>> 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