Fedora kernel list - Mar 2008 - [New Driver]: usbvideo2 webcam core + pac207 driver using it.

<Sorry, this time with attachments>

Hi All,

As explained in my introduction mail I''ve been working on a standalone
v4l2
driver for pac207 based usb webcams. I''ve attached the hopefully pretty
clean
result to this mail.

This is the promised split version of the pac207 driver I''ve been
working on, I
would like to ask everyone to take a good look at this, as I plan to base a
number of other (gspca derived) v4l2 drivers on this same core.

I''m currently posting these as .c files for easy reading and
compilation /
testing, but I still hope to get a lot of feedback / a thorough review, esp of
the core <-> pac207 split version as I hope to submit that as a patch for
mainline inclusion soon.

Thanks & Regards,

Hans

On Thu, 3 Apr 2008 14:27:28 -0700, Brandon Philips <brandon@ifup.org>
wrote:
> > struct pac207_decompress_table_t {
> > 	u8 is_abs;
> > 	u8 len;
> > 	s8 val;
> > };
> 
> Why add the _t?
What Brandon is trying to say here is "add _t to typedefs, don''t
add _t to structs". In kernel we use explicit structs.
> > #define CLIP(color) (unsigned
char)(((color)>0xFF)?0xff:(((color)<0)?0:(color)))
> 
> Add a comment about what this is doing?  Could you just do it as a
> static function instead?
The macro itself is too trivial to be commented, IMHO, but I have
to ask just what it is doing there. It is only applied to
precomputed values from pac207_decompress_table, as far as I see.
So, they cannot be out of range. Or can they?

-- Pete

On Fri, 28 Mar 2008 22:53:39 +0100, Hans de Goede <j.w.r.degoede@hhs.nl>
wrote:
> 	u8 users;
Make it int, too easy to overflow.
> static u32
> usbvideo2_request_buffers(struct usbvideo2_device* cam, u32 count)
> {
> 		if ((buff = vmalloc_32_user(cam->nbuffers *
> 						PAGE_ALIGN(cam->imagesize))))
I think that placing buffers into the lower 4GB is a senseless
limitation to build into a library. Although today the only system
which can run DMA between memory above 4GB and USB is Opteron,
with XHCI it''s going to change.
> 					spin_unlock(&cam->queue_lock);
> 					wake_up_interruptible(&cam->wait_frame);
Why only interruptible, and why outside of the lock? Bizarre.
> 	urb->dev = cam->usbdev;
> 	ret = usb_submit_urb(urb, GFP_ATOMIC);
If you don''t plan on using this code on kernel 2.4, don''t
restore
urb->dev. This was unnecessary for years. Just set it where you
fill the URB.
> 			urb->transfer_buffer = usb_buffer_alloc(udev,
> 							 (psz + 1) *
> 							 USBVIDEO2_ISO_PACKETS,
> 							 GFP_KERNEL,
> 							 &urb->transfer_dma);
Why are you using usb_buffer_alloc here? Why not use kmalloc?

Secondly, let''s suppose you''re allocating for 512-byte packets
(actually 513 for some reason). You''re above 8KB by a few bytes,
thus making this an order 2 allocation. I am quite certain this
is going to fail on loaded systems.

If you ever run across a device with a bigger maximum packet size
(e.g. a WUSB webcam) this is just going to crash and burn.

So, why do you need to stuff 16 ISO blocks into one URB? Hardware
limitation?

Any why is this affinity to device''s declared packet size? The HC
transparently merges transfers for you. So, the only thing you
need to think about is if your buffers are an integral number
of packets.

What is this +1 business, anyway?
> 	usbvideo2_release_buffers(cam);
> 	if (b->count)
> 		b->count = usbvideo2_request_buffers(cam, b->count);
OK, I''m going to skip this. V4L people know that area.

Same for mmap. I only made sure you aren''t trying to run DMA
directly off a vmalloc area.

The API between usbvideo2 and camera modules seems ok, nothing
to comment. It''ll become clearer if it was done right when you
add more camera modules.

-- Pete

Pete Zaitcev wrote:
>>> #define CLIP(color) (unsigned
char)(((color)>0xFF)?0xff:(((color)<0)?0:(color)))
>> Add a comment about what this is doing?  Could you just do it as a
>> static function instead?
> 
> The macro itself is too trivial to be commented, IMHO, but I have
> to ask just what it is doing there. It is only applied to
> precomputed values from pac207_decompress_table, as far as I see.
> So, they cannot be out of range. Or can they?
> 
Its being applied to the addition of a value read from the sensor and a 
precomputed value from the pac207_decompress_table, and the total of these can 
be out of range.

Regards,

Hans

Pete Zaitcev wrote:
> On Fri, 28 Mar 2008 22:53:39 +0100, Hans de Goede
<j.w.r.degoede@hhs.nl> wrote:
> 
>> 	u8 users;
> 
> Make it int, too easy to overflow.
> 
The current code doesn''t allow simultanious opens, so this can become
only 0 or 1 .
>> static u32
>> usbvideo2_request_buffers(struct usbvideo2_device* cam, u32 count)
>> {
>> 		if ((buff = vmalloc_32_user(cam->nbuffers *
>> 						PAGE_ALIGN(cam->imagesize))))
> 
> I think that placing buffers into the lower 4GB is a senseless
> limitation to build into a library. Although today the only system
> which can run DMA between memory above 4GB and USB is Opteron,
> with XHCI it''s going to change.
Actually this isn''t used for dma at all, so the _32_ can indeed go
away.
> 
>> 					spin_unlock(&cam->queue_lock);
>> 					wake_up_interruptible(&cam->wait_frame);
> 
> Why only interruptible
Because there are only interruptable waits done on it and My Linux Device 
Drivers 3th edition (Corbet et All) says to use _interruptible when there are 
only interruptible waiters. Also I took most of this from exisiting v4l2 usb 
drivers which do it the same.
> , and why outside of the lock? Bizarre.
Why would this need to be done inside the lock? The wait_frame queue has its 
own internal locking, no need to keep the queue lock longer then necessary esp 
as its a spinlock.

>> 	urb->dev = cam->usbdev;
>> 	ret = usb_submit_urb(urb, GFP_ATOMIC);
> 
> If you don''t plan on using this code on kernel 2.4, don''t
restore
> urb->dev. This was unnecessary for years. Just set it where you
> fill the URB.
> 
Ok.

>> 			urb->transfer_buffer = usb_buffer_alloc(udev,
>> 							 (psz + 1) *
>> 							 USBVIDEO2_ISO_PACKETS,
>> 							 GFP_KERNEL,
>> 							 &urb->transfer_dma);
> 
> Why are you using usb_buffer_alloc here? Why not use kmalloc?
> 
I took this from gspca, and I believe this is necessary to get a buffer will at 
all the proper attributes for being able todo a dma transfer to it.
> Secondly, let''s suppose you''re allocating for 512-byte
packets
> (actually 513 for some reason). You''re above 8KB by a few bytes,
> thus making this an order 2 allocation. I am quite certain this
> is going to fail on loaded systems.
> 
Good point!
> If you ever run across a device with a bigger maximum packet size
> (e.g. a WUSB webcam) this is just going to crash and burn.
> 
> So, why do you need to stuff 16 ISO blocks into one URB? Hardware
> limitation?
> 
No special reason I copied this from gspca, I guess I can just make it 
PAGE_SIZE / (packet_size + 1)
> Any why is this affinity to device''s declared packet size? The HC
> transparently merges transfers for you. So, the only thing you
> need to think about is if your buffers are an integral number
> of packets.
> 
Some webcams are so nice as to only put a sof (start of frame) marker/header at 
the start of a packet, so if the packets match actually device packets, then 
one only needs to check the first few bytes for the magic sof marker, instead 
of having to search through the entire packet (as one must do with other less 
nice webcams).
> What is this +1 business, anyway?
> 
The pac207 (and sn109c bayer compression code uses variable length bit 
patterns, the decoder always reads 8 bits and the looks this up in a table 
which tells it amongst other things how much bits where actually used. So if 
the last pixel in a row is coded in say 2 bits, and it happens to also be at 
the end of a packet 6 additional bits will get read (and ignored).

So the buffer gets allocated 1 byte too large to allow safe reading of these 6 
bits.

Regards,

Hans

On Sat, 05 Apr 2008 09:52:33 +0200, Hans de Goede <j.w.r.degoede@hhs.nl>
wrote:
> The current code doesn''t allow simultanious opens, so this can
become only 0 or 1 .
I see...
> > I think that placing buffers into the lower 4GB is a senseless
> > limitation to build into a library. Although today the only system
> > which can run DMA between memory above 4GB and USB is Opteron,
> > with XHCI it''s going to change.
> 
> Actually this isn''t used for dma at all, so the _32_ can indeed go
away.
I figured it out but I thought you wanted to do it eventually.
> > Why are you using usb_buffer_alloc here? Why not use kmalloc?
> I took this from gspca, and I believe this is necessary to get a buffer
will at
> all the proper attributes for being able todo a dma transfer to it.
No, kmalloc is good enough on all architectures. The only time
to use usb_buffer_alloc is when you have lots of small buffers,
or attempt to do some kind of zero copy transfer. It saves you
the MMU setup costs in that case. The USB stack hackers are
trying to de-emphasize its use because it conflicts with usbmon
(also it eats MMU resources, but this is not a big deal unless
you''re on SPARC).
> > What is this +1 business, anyway?
> 
> The pac207 (and sn109c bayer compression code uses variable length bit 
> patterns, the decoder always reads 8 bits and the looks this up in a table 
> which tells it amongst other things how much bits where actually used. So
if
> the last pixel in a row is coded in say 2 bits, and it happens to also be
at
> the end of a packet 6 additional bits will get read (and ignored).
> 
> So the buffer gets allocated 1 byte too large to allow safe reading of
these 6
> bits.
If I understand you correctly, you don''t need to allocate
(psz+1) * NUM_ISO  then. You could allocate psz*NUM_ISO + 1. So,
I thought it was necessary to force an empty DATA IN token due to
a device quirk... Some people said on linux-usb list that asking
for anything other than an integral number of packets did not
work with their ISO device, which is exact opposite of what
you''re doing. But if your webcam works, then by all means
continue. I''m just wondering if you''re going to step on it
when
you add a next webcam into usbvideo2 framework.

-- Pete