Re: [RFC PATCH] ALSA: hda - Implement a poll loop for jacks as a module parameter

[Takashi Iwai <tiwai@suse.de>]

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 <david.henningsson@canonical.com>
> >>>
> >>> 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