patch for hammerfall driver

patch for hammerfall driver

[Paul Davis]

i don't know how we ended up with such incorrect values for the
period/buffer size limits, but this corrects them. this previously
prevented applications from requesting a period size of 8192
frames. an identical patch is needed for the hdsp driver. i'll post
one tomorrow if jaroslav or takashi don't want to just hand-apply this
one. let me know.

--p


Index: rme9652.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/rme9652/rme9652.c,v
retrieving revision 1.22
diff -u -u -r1.22 rme9652.c
--- rme9652.c   14 Jul 2002 21:01:18 -0000      1.22
+++ rme9652.c   2 Feb 2003 21:31:49 -0000
@@ -2279,9 +2279,9 @@
        rate_max:               96000,
        channels_min:           10,
        channels_max:           26,
-       buffer_bytes_max:       1024*1024,
-       period_bytes_min:       1,
-       period_bytes_max:       1024*1024,
+       buffer_bytes_max:       RME9652_CHANNEL_BUFFER_BYTES * 26,
+       period_bytes_min:       (64 * sizeof(int)) * 10,
+       period_bytes_max:       (8192 * sizeof(int)) * 26,
        periods_min:            2,
        periods_max:            2,
        fifo_size:              0,
@@ -2302,9 +2302,9 @@
        rate_max:               96000,
        channels_min:           10,
        channels_max:           26,
-       buffer_bytes_max:       1024*1024,
-       period_bytes_min:       1,
-       period_bytes_max:       1024*1024,
+       buffer_bytes_max:       RME9652_CHANNEL_BUFFER_BYTES *26,
+       period_bytes_min:       (64 * sizeof(int)) * 10,
+       period_bytes_max:       (8192 * sizeof(int)) * 26,
        periods_min:            2,
        periods_max:            2,
        fifo_size:              0,



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: patch for hammerfall driver

[Takashi Iwai]

At Sun, 2 Feb 2003 16:37:46 -0500,
Paul Davis wrote:
> 
> i don't know how we ended up with such incorrect values for the
> period/buffer size limits, but this corrects them. this previously
> prevented applications from requesting a period size of 8192
> frames. an identical patch is needed for the hdsp driver. i'll post
> one tomorrow if jaroslav or takashi don't want to just hand-apply this
> one. let me know.

no problem, fixed on cvs for both rme9652.c and hdsp.c now.
(btw, i used 4 instead of sizeof(int) because the latter might not be
 always equal with 32bit (in future) :)


thanks,

Takashi


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: patch for hammerfall driver

[Takashi Iwai]

At Mon, 03 Feb 2003 07:28:41 -0500,
John S. Denker <jsd@monmouth.com> wrote:
> 
> Takashi Iwai wrote:
> 
> > (btw, i used 4 instead of sizeof(int) because the latter might not be
> >  always equal with 32bit (in future) :)
> 
> Certainly assuming sizeof(int)==4 is unwise.
> But using "4" isn't the optimal solution, either.
> 
> It is not a good programming practice to throw
> around magic numbers like "4" without documenting
> where they came from.  In this case it would be
> better to use
>    snd_pcm_format_size(format, nsamples)

well, unfortunately, it's not a constant, so we cannot use it as the
initializer.

i believe, in this case, giving a comment like /* 8192 frames */ would
suffice, because it's obvious that the hardware supports only 32bit
samples.  surely it was my fault, only putting a magic number there.

btw, sizeof() is not a general solution for this, too, because the
buffer size is not always aligned to any values of architecture
(e.g. 24bit/3bytes format).


> At Sun, 2 Feb 2003 16:37:46 -0500,
> Paul Davis wrote:
> 
>  >i don't know how we ended up with such incorrect values for the
>  >period/buffer size limits,
> 
> That is a very important question!
> It is by asking such questions that one grows
> as a programmer.
> 
> To be explicit:  whenever you see a bug, don't just
> ask how to fix this instance of the bug;  ask what
> it would take to make sure no bug of this ilk ever
> occurs again.
> 
> In this case:
>   a) It would help to have some comments in the code
> saying where the constants are coming from, so that
> misconceptions can be more easily spotted.
>   b) It would help to use correctly computed constants
> such as
>    snd_pcm_format_size(format, nsamples)
> rather than magic numbers such as "4".  There are
> far too many magic numbers in the ALSA code at
> the moment.
>   c) When applying the sizeof() operator, don't
> apply it to general-purpose types such as int, but
> rather apply it to an object that is provably
> of the type you care about, in this case something
> like sizeof(s32_le_t).  Now there isn't AFAIK any
> typedef for s32_le_t, but you could define one
> (carefully!).  If you don't have such a typedef,
> don't assume sizeof(int) is synonymous with the
> sizeof(the_thing_you_care_about).

for kernel codes, you can use sizeof(s32).


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: patch for hammerfall driver

[Paul Davis]

>> To be explicit:  whenever you see a bug, don't just
>> ask how to fix this instance of the bug;  ask what
>> it would take to make sure no bug of this ilk ever
>> occurs again.
>> 
>> In this case:
>>   a) It would help to have some comments in the code
>> saying where the constants are coming from, so that
>> misconceptions can be more easily spotted.
>>   b) It would help to use correctly computed constants
>> such as

actually, neither would have really helped very much here. i don't
know who set the values to those that were here before. the values
were clearly wrong on any reading of the code, and i've thought about
them on previous readings. the values that were there before were not
"buggy" - they were completely wrong.

the problem here was social rather than technical: i never had a
reason or the time to do anything to answer my questions about the
values that were there.

the last few days have seen me trying to debug some rather deep
problems with a specific hdsp configuration, and i wanted to run JACK
with 8192 frame periods. i couldn't do it, and ended up having to go
back and deal with those numbers so that i could. 

>>   c) When applying the sizeof() operator, don't
>> apply it to general-purpose types such as int, but
>> rather apply it to an object that is provably
>> of the type you care about, in this case something

agreed. i had meant to remove the sizeof() from the patch before i
posted it. 

--p


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: patch for hammerfall driver

[Takashi Iwai]

At Mon, 03 Feb 2003 09:49:59 -0500,
Paul Davis wrote:
> 
> >> To be explicit:  whenever you see a bug, don't just
> >> ask how to fix this instance of the bug;  ask what
> >> it would take to make sure no bug of this ilk ever
> >> occurs again.
> >> 
> >> In this case:
> >>   a) It would help to have some comments in the code
> >> saying where the constants are coming from, so that
> >> misconceptions can be more easily spotted.
> >>   b) It would help to use correctly computed constants
> >> such as
> 
> actually, neither would have really helped very much here. i don't
> know who set the values to those that were here before. the values
> were clearly wrong on any reading of the code, and i've thought about
> them on previous readings. the values that were there before were not
> "buggy" - they were completely wrong.
 
i also don't know how they got in, but at least the values were
already there more than one year ago...


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com