From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [RFC PATCH] ALSA: hda - Implement a poll loop for jacks as a module parameter Date: Wed, 10 Oct 2012 16:07:49 +0200 Message-ID: References: <1349788784-11700-1-git-send-email-david.henningsson@canonical.com> <50743477.9050401@canonical.com> <50757C73.3050602@canonical.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id A38602616C1 for ; Wed, 10 Oct 2012 16:07:49 +0200 (CEST) In-Reply-To: <50757C73.3050602@canonical.com> 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: David Henningsson Cc: alsa-devel@alsa-project.org List-Id: alsa-devel@alsa-project.org At Wed, 10 Oct 2012 15:47:31 +0200, David Henningsson wrote: > > On 10/09/2012 04:57 PM, Takashi Iwai wrote: > > At Tue, 09 Oct 2012 16:28:07 +0200, > > David Henningsson wrote: > >> > >> On 10/09/2012 03:32 PM, Takashi Iwai wrote: > >>> At Tue, 9 Oct 2012 15:19:44 +0200, > >>> David Henningsson wrote: > >>>> > >>>> Now that we have a generic unsol mechanism, we can implement a generic > >>>> poll loop, which can be used for debugging, or if a codec's unsol > >>>> mechanism is broken. > >> > >> Oh, and I was also considering not turning on unsol at all (in > >> snd_hda_jack_detect_enable) when this is enabled. Then it would help > >> against "unstable" pins which trigger repeatedly on/off even though > >> nothing is happening physically. > > > > In that case, it's fine. But it's no broken codec's unsol mechanism. > > It's a broken jack detection in hardware, i.e. it's no fault of > > codec. > > Ok, will send an additional patch for that once this one is applied. > > >>>> Signed-off-by: David Henningsson > >>> > >>> The patch almost looks good, but I postpone as a 3.8 thing. > >> > >> Ok. I never understood the release timings, but I figured anything > >> before rc1 would be okay to merge as a feature... > > > > No, basically when the merge window opens, all new features should be > > frozen. I took a few your patches yesterday just because I've been > > traveling in the last weeks, and the generic unsol event code itself > > is mostly nothing but a code shuffling from here to there. > > > >> (And I couldn't start > >> a week earlier; because I didn't know if you would accept the generic > >> unsol event.) > >> That said, it's not very important to me personally which kernel this > >> goes into. > > > > OK. > > > >>>> I'm also considering activating it by default if we go into single_cmd mode > >>>> due to codec not responding. What do you think? > >>> > >>> Well, I don't buy it. Falling back to single_cmd doesn't mean that we > >>> are saving the world perfectly.I recently even think it might be > >>> even better not to do that, otherwise people won't notice the > >>> breakage. > >> > >> This is almost philosophical, but if a bug occurs and no user notices, > >> is it really a bug? :-) > > > > Yes, it performs worse secretly. The fallback to single_cmd is really > > a last resort and it's no peaceful place to live at all. > > If that is your real thinking, maybe we should make the warning message > reflect that: > > snd_printk(KERN_ERR "hda_intel: azx_get_response timeout, " > "switching to single_cmd mode! This performs worse " > "secretly, is not saving the world perfectly, and " > "it's no peaceful place to live at all!\n"); > Yes, it's a plan (but in a different text form). > >> Or, to make it a bit more pragmatic, what other things are broken with > >> single_cmd? Could you give an example where this change would be harmful? > > > > The single cmd mode itself was introduced as a sort of rescue command > > without CORB/RIRB. We shouldn't use it in normal situations. It's > > simply no considered use case. > > > > With a lack of unsol event, you can't handle the volume knob, or any > > GPIO events, too. > > Those two are quite uncommon, and can be polled too if necessary. I wonder from where your love to polling comes... > >>>> + codec->jackpoll_interval = > >>>> + msecs_to_jiffies(jackpoll_ms[chip->dev_index]); > >>> > >>> Better to add a sanity check of the interval value. > >> > >> Ok. Attaching modified patch. > > > > Well, do you want to allow 1ms polling? > > I can't see why not, if people want to burn CPU cycles, why not let > them. You seem to trust users too much :) > However, the real limit should probably be one jiffy, so attaching > modified patch. > If you think there should be another lower limit, feel free to adjust > the patch before applying. It's no big deal. Well, do you really think 1 jiffies is the _sane_ lower limit for this polling behavior? (And did you imagine what would happen if doing it on a non-preemptive kernel?) Hypothetically we may set any value. But whether it really helps in practice, one needs to think twice. thanks, Takashi