Re: [PATCH] zr36067: Debugging cleanups (updated again)

On Sat, 26 May 2007, Jean Delvare wrote:
> On Thu, 24 May 2007 13:10:11 -0700 (PDT), Trent Piepho wrote:
> > That's wrong, fh->v4l_buffers.num_buffers is the number of buffers actually
> > allocated, not the limit.  One could call REQBUFS to get 2 buffers, then
> > unmap them and call REQBUFS with a different amount.
>
> You're correct. What confused me is that fh->v4l_buffers.num_buffers is
> initialized to v4l_nbufs in zoran_open_init_session(). Now this looks
> wrong to me as there are no buffers allocated at this point. What do
> you think? Same for fh->jpg_buffers.num_buffers.

There is fh->v4l_buffers.allocated to indicate if the buffers have actually
been allocated or not.  I think the initialization in
zoran_open_init_session() isn't actually needed.  I think every code path
that leads to the buffers getting allocated will also set the number of
buffers to allocate in num_buffers first, but I'm not sure about that.  I've
mostly looked at the v4l2 code and not the older interface.

> > > Second reason, there are limit checks done at init time on the value of
> > > v4l_nbufs (in init_dc10_cards). If the user can change the values
> > > through sysfs, these checks are bypassed. Thus I would suggest that you
> > > move the limit checks to zoran_open_init_session, where the value of
> > > v4l_nbufs is copied to the device-specific structure.
> >
> > They're also changed from kB to bytes.  I figure someone changing the
> > values at runtime should know what they're doing.
>
> Good point, I had missed that detail. This is indeed confusing. But
> this is easy to fix, all you have to do is move the multiplication,
> together with the checks, from init_dc10_cards() to
> zoran_open_init_session(). Same for jpg_bufsize.

That doesn't work, jpg_bufsize is used at other times too, such as in a call
to REQBUFS.  I think it's a general assumption that people setting parameters
via sysfs should know what they are doing.  There must be thousands of module
parameter one can set to values that aren't correct via sysfs.  It's easiest
and most efficient to check a module's parameters in its init function.

> > > Imagine that lock_norm is originally set to 2, and the user switches it
> > > to 0 between the 1st and 2nd line. The test "lock_norm > 1"  will fail,
> > > and the "else" statement will be executed, while it corresponds to
> > > lock_norm = 1 which was never set.
> > >
> > > I agree it's not very important in practice, but OTOH this can be easily
> > > fixed that way:
> >
> > That won't fix any race conditions.  You have no spinlock or memory barrier
> > or even made lock_norm volatile.  The compiler can and will re-order the
> > instructions in a way that might re-introduce a race condition.
>
> No. There's a fundamental difference between the original code and
> mine. The original code might cause something to happen which doesn't
> correspond to either the old value or the new value of lock_norm. This
> cannot possibly happen with my code.

Sure it can.  You must understand, lacking a memory barrier the C compiler
can and will reorder your code.  If reading or writing an integer variable
was atomic, and if comparisons were done in the order written, and if each
access to a variable caused it to be read from memory, then your code would
fix a race condition.  But none of those things are true, and so your code
doesn't actually fix a race condition.

> > After my patch to fix poll, tvtime will work.
>
> Yes, I confirm it works (somewhat) now. I have jitter with most (all?)
> deinterlacing modes, though. So I have to set tvtime in "half framerate"
> mode.

I get that too.  The driver probably isn't returning the correct field
order.  I think zoran hardware has a hard time with this.  For bt848 based
hardware, the command one gives to the chip is something like "write the
next even field here".  You know the field captured is the even field.  For
the zoran chip, the command is "write the next field here".  You don't know
if you got the even or odd field.  You can try to keep track which field it
_should_ be by counting the number captured, but that can easily get out of
sync with the input signal.

> > Note that tvtime only supports a vertical resolution of 480 for NTSC.  You
> > need to tell tvtime to use a horizontal resolution that fits in the v4l
> > buffer, 136 is the max for 128k buffers.
>
> I'm using SECAM not NTSC, but I guess the problem is the same. I am
> using the bigphysarea patch so that I can have larger buffers. I guess
> there is no other way to have a full-size output?

I was able to change the driver's kmalloc() limit from 128k to 1024k, and it
appeared to be able to allocate the larger memory region fine.  This is an
old driver, and the rules wrt large kmalloc()s may have changed since the
128k limit was put in.

> How do you tell tvtime to use a horizontal resolution of 136? The
> minimum I can select is 360.

command line option.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/