cx88-dsp.c: missing __divdi3 on 32bit kernel

DISCUSSION Miroslav Šustek 2 participants 4 messages Apr 6, 2009 – Apr 8, 2009 8 messages not in the archive Media (V4L/DVB)

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Miroslav Šustek

author

2009-04-06 20:40 UTC #8 lore raw

Hello,
Commit 02fde69f31dc (and 7152a23142bc) on http://linuxtv.org/hg/v4l-dvb/
adds new file cx88-dsp.c which uses 64bit divisions, but these are somehow not supported on 32bit kernels.

message during compile:
WARNING: "__divdi3" [/root/v4l-dvb/v4l/cx88xx.ko] undefined

Maybe we can use only s32, but I don't know if it's precise enough for that magic math. Or use some ugly hacks to do 64bit division with 32bit variables.

What hardware do I need to test the cx88-dsp code?

Any suggestions?

- Miroslav

Miroslav Šustek

author

2009-04-06 23:10 UTC #9 lore raw

Well this patch should solve it.

I don't know how many samples are processed so:
First patch is for situation when N*N fits in s32.
Second one uses two divisions, but doesn't have any abnormal restrictions for N.

Personally I think that two divisions won't hurt. :)

----- FILE: cx88-dsp_64bit_math1.patch -----

cx88-dsp: fixing 64bit math on 32bit kernels

Note the limitation of N.
Personally I know nothing about possible size of samples array.

From: Miroslav Sustek <[email protected]>

Signed-off-by Miroslav Sustek <[email protected]>

linux/drivers/media/video/cx88/cx88-dsp.c +11 -2

diff -r 8aa1d865373c linux/drivers/media/video/cx88/cx88-dsp.c
@@ -100,13 +100,22 @@
 	s32 s_prev2 = 0;
 	s32 coeff = 2*int_cos(freq);
 	u32 i;
+
+	s64 tmp;
+	u32 remainder;
+
 	for (i = 0; i < N; i++) {
 		s32 s = x[i] + ((s64)coeff*s_prev/32768) - s_prev2;
 		s_prev2 = s_prev;
 		s_prev = s;
 	}
-	return (u32)(((s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
-		      (s64)coeff*s_prev2*s_prev/32768)/N/N);
+
+	tmp = (s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
+		      (s64)coeff*s_prev2*s_prev/32768;
+
+	/* XXX: N must be low enough so that N*N fits in s32.
+	 * Else we need two divisions. */
+	return (u32) div_s64_rem(tmp, N*N, &remainder);
 }
 
 static u32 freq_magnitude(s16 x[], u32 N, u32 freq)

----- FILE: cx88-dsp_64bit_math2.patch -----

cx88-dsp: fixing 64bit math on 32bit kernels

From: Miroslav Sustek <[email protected]>

Signed-off-by Miroslav Sustek <[email protected]>

linux/drivers/media/video/cx88/cx88-dsp.c +11 -2

diff -r 8aa1d865373c linux/drivers/media/video/cx88/cx88-dsp.c
@@ -100,13 +100,22 @@
 	s32 s_prev2 = 0;
 	s32 coeff = 2*int_cos(freq);
 	u32 i;
+
+	s64 tmp;
+	u32 remainder;
+
 	for (i = 0; i < N; i++) {
 		s32 s = x[i] + ((s64)coeff*s_prev/32768) - s_prev2;
 		s_prev2 = s_prev;
 		s_prev = s;
 	}
-	return (u32)(((s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
-		      (s64)coeff*s_prev2*s_prev/32768)/N/N);
+
+	tmp = (s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
+		      (s64)coeff*s_prev2*s_prev/32768;
+
+	tmp = div_s64_rem(tmp, N, &remainder);
+
+	return (u32)div_s64_rem(tmp, N, &remainder);
 }
 
 static u32 freq_magnitude(s16 x[], u32 N, u32 freq)

Marton Balint

2009-04-07 00:27 UTC #10 lore raw

Quoted 5 lines · Miroslav Šustek

Well this patch should solve it.
I don't know how many samples are processed so:
First patch is for situation when N*N fits in s32.
Second one uses two divisions, but doesn't have any abnormal restrictions for N.

Both patches are fine, beacuse in the current implementation N is not bigger than 576. Thanks for fixing this problem.

Regards,
Marton

Quoted 87 lines

Personally I think that two divisions won't hurt. :)
----- FILE: cx88-dsp_64bit_math1.patch -----
cx88-dsp: fixing 64bit math on 32bit kernels
Note the limitation of N.
Personally I know nothing about possible size of samples array.
From: Miroslav Sustek <[email protected]>
Signed-off-by Miroslav Sustek <[email protected]>
diff -r 8aa1d865373c linux/drivers/media/video/cx88/cx88-dsp.c
linux/drivers/media/video/cx88/cx88-dsp.c +11 -3
@@ -100,13 +100,22 @@
 	s32 s_prev2 = 0;
 	s32 coeff = 2*int_cos(freq);
 	u32 i;
+
+	s64 tmp;
+	u32 remainder;
+
 	for (i = 0; i < N; i++) {
 		s32 s = x[i] + ((s64)coeff*s_prev/32768) - s_prev2;
 		s_prev2 = s_prev;
 		s_prev = s;
 	}
-	return (u32)(((s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
-		      (s64)coeff*s_prev2*s_prev/32768)/N/N);
+
+	tmp = (s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
+		      (s64)coeff*s_prev2*s_prev/32768;
+
+	/* XXX: N must be low enough so that N*N fits in s32.
+	 * Else we need two divisions. */
+	return (u32) div_s64_rem(tmp, N*N, &remainder);
 }
 
 static u32 freq_magnitude(s16 x[], u32 N, u32 freq)
 
 
 
----- FILE: cx88-dsp_64bit_math2.patch -----
cx88-dsp: fixing 64bit math on 32bit kernels
From: Miroslav Sustek <[email protected]>
Signed-off-by Miroslav Sustek <[email protected]>
diff -r 8aa1d865373c linux/drivers/media/video/cx88/cx88-dsp.c
linux/drivers/media/video/cx88/cx88-dsp.c +11 -3
@@ -100,13 +100,22 @@
 	s32 s_prev2 = 0;
 	s32 coeff = 2*int_cos(freq);
 	u32 i;
+
+	s64 tmp;
+	u32 remainder;
+
 	for (i = 0; i < N; i++) {
 		s32 s = x[i] + ((s64)coeff*s_prev/32768) - s_prev2;
 		s_prev2 = s_prev;
 		s_prev = s;
 	}
-	return (u32)(((s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
-		      (s64)coeff*s_prev2*s_prev/32768)/N/N);
+
+	tmp = (s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
+		      (s64)coeff*s_prev2*s_prev/32768;
+
+	tmp = div_s64_rem(tmp, N, &remainder);
+
+	return (u32)div_s64_rem(tmp, N, &remainder);
 }
 
 static u32 freq_magnitude(s16 x[], u32 N, u32 freq)
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marton Balint

2009-04-08 20:37 UTC #11 lore raw

Quoted 10 lines · Marton Balint

Quoted 5 lines · Miroslav Šustek
Well this patch should solve it.
I don't know how many samples are processed so:
First patch is for situation when N*N fits in s32.
Second one uses two divisions, but doesn't have any abnormal restrictions for N.
Both patches are fine, beacuse in the current implementation N is not bigger than 576. Thanks for fixing this problem.

It seems that an #include is missing to math64.h. Below is a patch that adds the missing include.

Regards,
Marton

# HG changeset patch
# User Marton Balint <[email protected]>
# Date 1239222228 -7200
# Node ID c07293f1b5e44a8614f8f84c6b4fc586a02e69eb # Parent 202a1c7ec37968f35678980b5f3d9812e5961ef0
cx88: dsp: add missing include to math64.h

From: Marton Balint <[email protected]>

This patch adds a missing include to math64.h and replaces div_s64_rem with div_s64 since the remainder is not used anyway.

Priority: normal

Signed-off-by Marton Balint <[email protected]>

linux/drivers/media/video/cx88/cx88-dsp.c +3 -5

diff -r 202a1c7ec379 -r c07293f1b5e4 linux/drivers/media/video/cx88/cx88-dsp.c
@@ -22,6 +22,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/jiffies.h>
+#include <linux/math64.h>
 
 #include "cx88.h"
 #include "cx88-reg.h"

@@ -100,9 +101,7 @@
 	s32 s_prev2 = 0;
 	s32 coeff = 2*int_cos(freq);
 	u32 i;
-
 	s64 tmp;
-	u32 remainder;
 
 	for (i = 0; i < N; i++) {
 		s32 s = x[i] + ((s64)coeff*s_prev/32768) - s_prev2;

@@ -113,9 +112,8 @@
 	tmp = (s64)s_prev2*s_prev2 + (s64)s_prev*s_prev -
 		      (s64)coeff*s_prev2*s_prev/32768;
 
-	/* XXX: N must be low enough so that N*N fits in s32.
-	 * Else we need two divisions. */
-	return (u32) div_s64_rem(tmp, N*N, &remainder);
+	/* N is low enough so N*N fits in s32. */
+	return (u32) div_s64(tmp, N*N);
 }
 
 static u32 freq_magnitude(s16 x[], u32 N, u32 freq)