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

Hi Trent,

Sorry for the late answer, I have been busy with higher priority stuff
lately.

On Sat, 26 May 2007 07:17:44 -0700 (PDT), Trent Piepho wrote:
> On Sat, 26 May 2007, Jean Delvare wrote:
> > On Thu, 24 May 2007 13:10:11 -0700 (PDT), Trent Piepho wrote:
> > > 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.

Easiest, most efficient and partly incorrect IMHO. My opinion is that
one shouldn't make a module parameter writable through sysfs without
also making sure that the code will behave properly when values are
actually written to the sysfs files. But you seem to have a different
opinion on this matter, and given that I am neither the zr36067 driver
maintainer not a v4l subsystem maintainer, my opinion probably doesn't
matter that much.

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

Sorry but I have to insist here, because you still seem to miss the key
problem I have been pointing out.

Here we have a variable which can have 3 different states A, B and C. I
_do_ agree that, if the variable is originally in state A, and the user
changes it to state B, and for a short period of time, the driver still
considers it is in state A, this is a only a minor race condition,
which may not be worth addressing.

But the code after your patch does something different. It is such
that, when the variable is in state A, and the user changes it to state
B, the driver might, for a short period of time, do something which
should only occur when the variable is in state C. This is a major race
condition. This is a _bug_, nothing less. And given how easy it is to
change the code to fall under the "minor race condition" case above, I
believe this should be done.

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

Interesting. I'll follow up in a separate thread.

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

Ah, "tvtime --inputwidth=136"? It seems 112 is the max for me due to
SECAM having more lines than NTSC. Thanks for the tip.

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