[alsa-devel] New snd-hda warning spew

[Ville Syrjälä]

On Fri, Mar 18, 2016 at 08:18:10PM +0100, Takashi Iwai wrote:
> On Fri, 18 Mar 2016 20:10:58 +0100,
> Ville Syrjälä wrote:
> > 
> > On Fri, Mar 18, 2016 at 07:51:43PM +0100, Takashi Iwai wrote:
> > > On Fri, 18 Mar 2016 18:49:19 +0100,
> > > Ville Syrjälä wrote:
> > > > 
> > > > On Fri, Mar 18, 2016 at 03:22:15PM +0100, Takashi Iwai wrote:
> > > > > On Fri, 18 Mar 2016 14:54:59 +0100,
> > > > > Ville Syrjälä wrote:
> > > > > > 
> > > > > > On Wed, Mar 16, 2016 at 04:04:20PM +0200, Ville Syrjälä wrote:
> > > > > > > On Tue, Mar 15, 2016 at 06:22:56PM +0100, Takashi Iwai wrote:
> > > > > > > > On Tue, 15 Mar 2016 17:02:07 +0100,
> > > > > > > > Ville Syrjälä wrote:
> > > > > > > > > 
> > > > > > > > > We have a few new WARN spews from snd-hda causing some grief in i915 CI.
> > > > > > > > > 
> > > > > > > > > This one happens on ILK and BYT. Looks like it happens 100% of the time on driver load:
> > > > > > > > > [   18.809850] ------------[ cut here ]------------
> > > > > > > > > [   18.809866] WARNING: CPU: 0 PID: 39 at sound/hda/hdac_i915.c:129 pin2port+0x25/0x30 [snd_hda_core]()
> > > > > > > > 
> > > > > > > > This is bad.  Basically we had a naive assumption of the fixed mapping
> > > > > > > > between the port number and the HD-audio widget, but it doesn't apply
> > > > > > > > properly to pre-HSW models.
> > > > > > > > 
> > > > > > > > The patch attached below disables the audio binding for pre-HSW
> > > > > > > > models.  I'm going to queue to for-linus branch.
> > > > > > > 
> > > > > > > That seems to eliminate the warn on my ILK.
> > > > > > 
> > > > > > Apparently it was less effective on BYT. We still get this:
> > > > > 
> > > > > Ouch, I forgot that Baytrail had already i915 component binding in the
> > > > > controller side.  I assumed too naively that all old models have no
> > > > > binding.
> > > > > 
> > > > > Below is the additional fix patch.
> > > > 
> > > > Still getting blasted at least via snd_hdac_sync_audio_rate()
> > > 
> > > That code path is a slightly different.  This is a kind of false
> > > positive, but now the function checks the validity of the passed
> > > argument more strictly, and starts grumbling.
> > > 
> > > The fix is attached below.
> > > 
> > > 
> > > Takashi
> > > 
> > > -- 8< --
> > > From: Takashi Iwai <tiwai at suse.de>
> > > Subject: [PATCH] ALSA: hda - Fix spurious kernel WARNING on Baytrail HDMI
> > > 
> > > snd_hdac_sync_audio_rate() call is mandatory only for HSW and later
> > > models, but we call the function unconditionally blindly assuming that
> > > the function doesn't do anything harmful.  But since recently, the
> > > function checks the validity of the passed pin NID, and eventually
> > > spews the warning if an unexpected pin is passed.  This is seen on old
> > > chips like Baytrail.
> > > 
> > > The fix is to limit the call of this function again only for the chips
> > > with the proper binding.  This can be identified by the same flag as
> > > the eld notifier.
> > > 
> > > Reported-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > Cc: <stable at vger.kernel.org> # v4.5
> > > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > > ---
> > >  sound/pci/hda/patch_hdmi.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > > index e7d9453ecd10..56d3575ee6cc 100644
> > > --- a/sound/pci/hda/patch_hdmi.c
> > > +++ b/sound/pci/hda/patch_hdmi.c
> > > @@ -1741,7 +1741,8 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> > >  
> > >  	/* Call sync_audio_rate to set the N/CTS/M manually if necessary */
> > >  	/* Todo: add DP1.2 MST audio support later */
> > > -	snd_hdac_sync_audio_rate(&codec->bus->core, pin_nid, runtime->rate);
> > > +	if (codec_has_acomp(codec))
> > > +		snd_hdac_sync_audio_rate(&codec->bus->core, pin_nid, runtime->rate);
> > 
> > Yeah seems like it should do it.
> > 
> > Though I'm not entirely enthusiastic about maintaining two code paths
> > for audio stuff. So longer term I'd love to be able rip out the
> > hardware based ELD passing from i915 and just use the audio component
> > even for the oldest platforms.
> 
> Yes, that was my "grand plan", but it failed miserably because of the
> uncertain mapping between the digital port and audio widget node.
> Once when this mapping is clarified, we can reenable the direct
> notification.  I hope it can be done for 4.7.

Cool. I'll keep my fingers crossed :)

-- 
Ville Syrjälä
Intel OTC