[PATCH] Show processing coefficients in codec proc file

[PATCH] Show processing coefficients in codec proc file

[David Henningsson]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

This patch is helpful for tracking down bugs without having all
information about the chip.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

[-- Attachment #2: 0001-Show-processing-coefficients-in-codec-proc-file.patch --]
[-- Type: text/x-patch, Size: 1591 bytes --]

>From 568a3c0e45396cf6cfd8e8dac13f76f756c7e3f3 Mon Sep 17 00:00:00 2001
From: David Henningsson <david.henningsson@canonical.com>
Date: Thu, 19 Aug 2010 18:40:44 +0200
Subject: [PATCH 1/2] Show processing coefficients in codec proc file

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---
 sound/pci/hda/hda_proc.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
index f025200..88e40f0 100644
--- a/sound/pci/hda/hda_proc.c
+++ b/sound/pci/hda/hda_proc.c
@@ -467,11 +467,23 @@ static void print_unsol_cap(struct snd_info_buffer *buffer,
 static void print_proc_caps(struct snd_info_buffer *buffer,
 			    struct hda_codec *codec, hda_nid_t nid)
 {
+        int i, val;
 	unsigned int proc_caps = snd_hda_param_read(codec, nid,
 						    AC_PAR_PROC_CAP);
+	unsigned int ncoeff = (proc_caps & AC_PCAP_NUM_COEF) >> 
+                              AC_PCAP_NUM_COEF_SHIFT;
+
 	snd_iprintf(buffer, "  Processing caps: benign=%d, ncoeff=%d\n",
-		    proc_caps & AC_PCAP_BENIGN,
-		    (proc_caps & AC_PCAP_NUM_COEF) >> AC_PCAP_NUM_COEF_SHIFT);
+		    proc_caps & AC_PCAP_BENIGN, ncoeff);
+	for (i = 0; i < ncoeff; i++) {
+		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_COEF_INDEX,
+				    i);
+		val = snd_hda_codec_read(codec, nid, 0,
+					 AC_VERB_GET_PROC_COEF, 0);
+                snd_iprintf(buffer, "  Processing coef %d: 0x%x\n", i, 
+                            val);
+        }
+        
 }
 
 static void print_conn_list(struct snd_info_buffer *buffer,
-- 
1.7.0.4


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] Show processing coefficients in codec proc file

[Takashi Iwai]

At Thu, 19 Aug 2010 20:17:42 +0200,
David Henningsson wrote:
> 
> This patch is helpful for tracking down bugs without having all
> information about the chip.

I don't want to change coef index in reading a proc file in general.
If any, you should implement a codec-specific hook.


thanks,

Takashi


> 
> -- 
> David Henningsson, Canonical Ltd.
> http://launchpad.net/~diwic
> [2 0001-Show-processing-coefficients-in-codec-proc-file.patch <text/x-patch (7bit)>]
> >From 568a3c0e45396cf6cfd8e8dac13f76f756c7e3f3 Mon Sep 17 00:00:00 2001
> From: David Henningsson <david.henningsson@canonical.com>
> Date: Thu, 19 Aug 2010 18:40:44 +0200
> Subject: [PATCH 1/2] Show processing coefficients in codec proc file
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
>  sound/pci/hda/hda_proc.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/hda_proc.c b/sound/pci/hda/hda_proc.c
> index f025200..88e40f0 100644
> --- a/sound/pci/hda/hda_proc.c
> +++ b/sound/pci/hda/hda_proc.c
> @@ -467,11 +467,23 @@ static void print_unsol_cap(struct snd_info_buffer *buffer,
>  static void print_proc_caps(struct snd_info_buffer *buffer,
>  			    struct hda_codec *codec, hda_nid_t nid)
>  {
> +        int i, val;
>  	unsigned int proc_caps = snd_hda_param_read(codec, nid,
>  						    AC_PAR_PROC_CAP);
> +	unsigned int ncoeff = (proc_caps & AC_PCAP_NUM_COEF) >> 
> +                              AC_PCAP_NUM_COEF_SHIFT;
> +
>  	snd_iprintf(buffer, "  Processing caps: benign=%d, ncoeff=%d\n",
> -		    proc_caps & AC_PCAP_BENIGN,
> -		    (proc_caps & AC_PCAP_NUM_COEF) >> AC_PCAP_NUM_COEF_SHIFT);
> +		    proc_caps & AC_PCAP_BENIGN, ncoeff);
> +	for (i = 0; i < ncoeff; i++) {
> +		snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_COEF_INDEX,
> +				    i);
> +		val = snd_hda_codec_read(codec, nid, 0,
> +					 AC_VERB_GET_PROC_COEF, 0);
> +                snd_iprintf(buffer, "  Processing coef %d: 0x%x\n", i, 
> +                            val);
> +        }
> +        
>  }
>  
>  static void print_conn_list(struct snd_info_buffer *buffer,
> -- 
> 1.7.0.4
> 

Re: [PATCH] Show processing coefficients in codec proc file

[David Henningsson]

2010-08-19 20:28, Takashi Iwai skrev:
> At Thu, 19 Aug 2010 20:17:42 +0200,
> David Henningsson wrote:
>>
>> This patch is helpful for tracking down bugs without having all
>> information about the chip.
> 
> I don't want to change coef index in reading a proc file in general.

Good point. But I think the solution to that problem would be to
- read current index
- do the loop
- restore current index

...and perhaps protect that with an appropriate mutex (which one)?

> If any, you should implement a codec-specific hook.

I'm sorry, but I don't really see how that would help...?

Another option would be to have a generic snd_hda_codec_write_coef(nid,
index, value) and snd_hda_codec_read_coef(nid, index) in hda_codec.c
(and perhaps protect it with a spin lock?), then remove all other code
that directly sets the coefficient index.

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [PATCH] Show processing coefficients in codec proc file

[Takashi Iwai]

At Thu, 19 Aug 2010 20:48:56 +0200,
David Henningsson wrote:
> 
> 2010-08-19 20:28, Takashi Iwai skrev:
> > At Thu, 19 Aug 2010 20:17:42 +0200,
> > David Henningsson wrote:
> >>
> >> This patch is helpful for tracking down bugs without having all
> >> information about the chip.
> > 
> > I don't want to change coef index in reading a proc file in general.
> 
> Good point. But I think the solution to that problem would be to
> - read current index
> - do the loop
> - restore current index
> 
> ...and perhaps protect that with an appropriate mutex (which one)?
> 
> > If any, you should implement a codec-specific hook.
> 
> I'm sorry, but I don't really see how that would help...?

If it's specific to a codec, then you know whether reading the coef
in that way can be harmful or not.  It's not only about the race
via proc file but also the influence of coef on the actual codec
behavior.


Takashi

Re: [PATCH] Show processing coefficients in codec proc file

[David Henningsson]

2010-08-19 22:24, Takashi Iwai skrev:
> At Thu, 19 Aug 2010 20:48:56 +0200,
> David Henningsson wrote:
>>
>> 2010-08-19 20:28, Takashi Iwai skrev:
>>> At Thu, 19 Aug 2010 20:17:42 +0200,
>>> David Henningsson wrote:
>>>>
>>>> This patch is helpful for tracking down bugs without having all
>>>> information about the chip.
>>>
>>> I don't want to change coef index in reading a proc file in general.
>>
>> Good point. But I think the solution to that problem would be to
>> - read current index
>> - do the loop
>> - restore current index
>>
>> ...and perhaps protect that with an appropriate mutex (which one)?
>>
>>> If any, you should implement a codec-specific hook.
>>
>> I'm sorry, but I don't really see how that would help...?
> 
> If it's specific to a codec, then you know whether reading the coef
> in that way can be harmful or not.  It's not only about the race
> via proc file but also the influence of coef on the actual codec
> behavior.

Oh, so there are chips that are *that* broken...

Yet it could be a lifesaver so I wouldn't want to give up on the idea
just yet...

Do you know off the top of your head approximately what codec chips this
is about, where reading a coef (or setting its index) can cause unwanted
side-effects?


-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic

Re: [PATCH] Show processing coefficients in codec proc file

[Takashi Iwai]

At Fri, 20 Aug 2010 16:46:25 +0200,
David Henningsson wrote:
> 
> 2010-08-19 22:24, Takashi Iwai skrev:
> > At Thu, 19 Aug 2010 20:48:56 +0200,
> > David Henningsson wrote:
> >>
> >> 2010-08-19 20:28, Takashi Iwai skrev:
> >>> At Thu, 19 Aug 2010 20:17:42 +0200,
> >>> David Henningsson wrote:
> >>>>
> >>>> This patch is helpful for tracking down bugs without having all
> >>>> information about the chip.
> >>>
> >>> I don't want to change coef index in reading a proc file in general.
> >>
> >> Good point. But I think the solution to that problem would be to
> >> - read current index
> >> - do the loop
> >> - restore current index
> >>
> >> ...and perhaps protect that with an appropriate mutex (which one)?
> >>
> >>> If any, you should implement a codec-specific hook.
> >>
> >> I'm sorry, but I don't really see how that would help...?
> > 
> > If it's specific to a codec, then you know whether reading the coef
> > in that way can be harmful or not.  It's not only about the race
> > via proc file but also the influence of coef on the actual codec
> > behavior.
> 
> Oh, so there are chips that are *that* broken...
> 
> Yet it could be a lifesaver so I wouldn't want to give up on the idea
> just yet...

As mentioned, I don't mind to add it but as a beginning, let's add it
conditionally for known-working codecs.  Eventually we can put it to
the generic part, with a blacklisting if needed.

> Do you know off the top of your head approximately what codec chips this
> is about, where reading a coef (or setting its index) can cause unwanted
> side-effects?

I don't remember exactly which one.  I thought either an old Realtek
or AD codec.


Takashi