| From: D. Hugh Redelmeier <[EMAIL PROTECTED]>
| So: I suspect that a number of the "then" variables should be declared
| "unsigned long", not "u32" and not "u64". Besides being correct, this
| is also portable. Since I've only read the diff, I cannot be sure
OK. This is now more than theoretically interesting to me. I've just
tried to install this software and I think I'm hitting the original
problem. So I'm reading the code for ivtv-0.2.0-rc3d as well as the
diff.
All of these declarations of "then" should be of type unsigned long.
The patch changes them to u64. So every place that the patch has
"u64" should instead say "unsigned long". Simple to fix!
I have not looked if there are other variables that the patch missed.
The declaration of "then" in driver/ivtv-kthreads.c:376 initializes it
to 0. That initialization is redundant since the first thing done to
"then" is reassign it with the value of jiffies. The initialization
to 0 should be removed since
(1) it is redundant
(2) it is confusing, and
(3) 0 is a meaningless value for this variable.
Several format strings know to format "jiffies" and "then" using %ul.
Funny that the code didn't declare "then" the corresponding way
(unsigned long). The patch, as modified above, fixes this.
The declaration of "then" in driver/ivtv-osd.c:949 and its assignment
in line 966 should be deleted since the variable is never otherwise
used.
[NOTE: the rest is extremely nit-picky. Except for one BUG, marked
as such. Skip unless you are someone who might accept my patch.]
The use of "then" in driver/streams.c lines 1082 through 1095 is
rather confusing. In the rest of the driver, "then" is a sample from
"jiffies". In this range, it is a number of jiffies between a sample
and now. It is actually just a cheap reuse of a variable for Common
Subexpression Elimination, and not even well done at that.
BUG: the type of the format for printing "then" (%d) is wrong.
The proper (i.e. maximal and correct) common subexpression is:
(int) (1000/HZ)*(jiffies - then)
I don't actually know why int is used. unsigned would be slightly
better. Perhaps unsigned long would be better still (if delays might
be longer than 24 days). If int were being used to save CPU time, it
is funny that the subtraction and the multiplication are being done in
unsigned long and only the result is being narrowed to int.
The division by 1000 is problematic if HZ is 1024: the duration will
always be reported as 0. Smaller inaccuracies come in whenever HZ is
not a proper divisor of 1000. The most accurate formula would be:
(int) (1000 * (jiffies - then) / HZ)
Unfortunately this would cost a runtime division where the original
formulation does the division at compile time (assuming HZ is a
#define).
Here's a really complicated way of avoiding the runtime division if it
would produce an answer withing 90% of the correct answer. I don't
think that the complexity is worth it.
#if defined(HZ)
# if 1000 / HZ * HZ > 900
(int) (1000/HZ)*(jiffies - then)
# else
(int) (1000 * (jiffies - then) / HZ)
# endif
#else
(int) (1000 * (jiffies - then) / HZ)
#endif
As a compromize, I suggest using a rounded multiplicative constant:
(int) ((1000 + HZ/2) / HZ) * (jiffies - then)
This should work well enough
The preceding assignment to "then" is outside the enclosing "if" but
it could be inside. Moving it would make the driver marginally
clearer.
Since explaining this is awkward, I'll just include a TOTALLY UNTESTED
diff at the end.
Hugh Redelmeier
[EMAIL PROTECTED] voice: +1 416 482-8253
===================================================================
RCS file: RCS/ivtv-streams.c,v
retrieving revision 1.1
diff -u -r1.1 ivtv-streams.c
--- ivtv-streams.c 2005/01/23 22:31:34 1.1
+++ ivtv-streams.c 2005/01/23 23:22:12
@@ -1065,9 +1065,11 @@
up(&st->mlock);
if (!test_bit(IVTV_F_S_NO_DMA, &st->s_flags)) {
- then = jiffies;
if ((type == IVTV_ENC_STREAM_TYPE_MPG) && itv->end_gop == 1) {
/* only run these if we're shutting down the last cap
*/
+ unsigned long duration;
+
+ then = jiffies;
add_wait_queue(&itv->cap_w, &wait);
set_current_state(TASK_INTERRUPTIBLE);
@@ -1079,20 +1081,28 @@
{
schedule_timeout(HZ/100);
}
- then = jiffies - then;
+
+ /* To convert jiffies to ms, we must multiply by 1000
+ * and divide by HZ. To avoid runtime division, we
+ * convert this to multiplication by 1000/HZ.
+ * Since integer division truncates, we get the best
+ * accuracy if we do a rounding calculation of the
constant.
+ * Think of the case where HZ is 1024.
+ */
+ duration = ((1000 + HZ/2) / HZ) * (jiffies - then);
if (!test_bit(IVTV_F_I_EOS, &itv->i_flags)) {
IVTV_DEBUG(IVTV_DEBUG_ERR,
"ENC: EOS interrupt not "
"received! stopping anyway.\n");
IVTV_DEBUG(IVTV_DEBUG_ERR,
- "ENC: waited %d ms.\n",
- (1000/HZ)*then);
+ "ENC: waited %lu ms.\n",
+ duration);
} else {
IVTV_DEBUG(IVTV_DEBUG_ERR,
- "ENC: EOS took %d "
+ "ENC: EOS took %lu "
"ms to occur.\n",
- (1000/HZ)*then);
+ duration);
}
set_current_state(TASK_RUNNING);
remove_wait_queue(&itv->cap_w, &wait);
================ end ================
-------------------------------------------------------
This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting
Tool for open source databases. Create drag-&-drop reports. Save time
by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc.
Download a FREE copy at http://www.intelliview.com/go/osdn_nl
_______________________________________________
ivtv-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ivtv-devel
