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

Hi Trent,

On Thu, 24 May 2007 13:10:11 -0700 (PDT), Trent Piepho wrote:
> On Thu, 24 May 2007, Jean Delvare wrote:
> > >  /*
> > >     Number and size of grab buffers for Video 4 Linux
> > > @@ -130,21 +132,21 @@ MODULE_PARM_DESC(video_nr, "video device
> > >
> > >  int v4l_nbufs = 2;
> > >  int v4l_bufsize = 128;		/* Everybody should be able to work with this setting */
> > > -module_param(v4l_nbufs, int, 0);
> > > +module_param(v4l_nbufs, int, 0644);
> > >  MODULE_PARM_DESC(v4l_nbufs, "Maximum number of V4L buffers to use");
> >
> > This one I don't think is safe in its current form. First reason,
> > v4l_nbufs is used in VIDIOC_REQBUFS to control user input. If its value
> > has changed since the device was opened, I expect problems. To be on
> 
> Why?  The buffers aren't allocated until the call to REQBUFS.
> 
> > the safe side, I believe that you need to change the code in
> > VIDIOC_REQBUFS to use fh->v4l_buffers.num_buffers instead of v4l_nbufs.
> 
> 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.

> > 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.

> > Wouldn't it make more sense to print a warning in the logs and default
> > to 0 in this case? Also, default_input is an int so checking for
> > negative values would be welcome too.
> 
> Maybe it would be better to just change the parameter to unsigned.  I could
> add a warning, but I don't want to bloat the driver with a lot of warnings no
> one is every going to care about.

As you wish.

> > 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.

> 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.

> 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?

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

> Changing inputs in tvtime still doesn't work.  I think it's a driver bug
> from stopping and re-starting capture.

Yes I noticed this problem too. I would be happy to test patches if you
manage to fix it.

Thanks,
-- 
Jean Delvare

-------------------------------------------------------------------------
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/