Re: [linux-uvc-devel] [PATCH 0/2] uvcvideo: Support Logitech RGB Bayer formats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Oleksij,

(CC'ing linux-media)

On Wednesday 02 January 2013 11:50:51 Oleksij Rempel wrote:
> Am 02.01.2013 10:03, schrieb Peter Ross:
> > On Tue, Jan 01, 2013 at 05:16:44PM +0100, Oleksij Rempel wrote:
> >> Hi Peter,
> >> 
> >> thank you for your work, but most of it belongs to uvcdynctrl.
> >> You will need to add it here:
> >> /usr/share/uvcdynctrl/data/046d/logitech.xml
> > 
> > Using uvcdynctrl is problematic for two reasons.
> > 
> > 1. Setting the Logitech 'disable video processing' and 'raw data bpp'
> >    controls independently from uvcvideo causes the driver to *always* drop
> >    frames.
> >
> >    uvc_video_decode_isoc() performs a sanity check on the size of incoming
> >    uncompressed frames. It expects to receive frame sizes reported by the
> >    8-bit yuyv 4:2:2 format description. The check fails when video
> >    processing is disabled, because the 8-/10-bit RGB Bayer frame sizes are
> >    always smaller.
> >
> > 2. Userspace applications need to distinguish the yuyv 4:2:2 format from
> >    the the additional RGB Bayer formats.
> >     
> >    Currently, when video processing is disabled, user applications expect
> >    to receive yuyv 4:2:2 pixels, because thats what it and uvcvideo has
> >    agreed to (mediate by V4L2). Instead the application receives RGB Bayer
> >    pixels and attempts to process them as yuyv resulting in visual
> >    garbage.

One could argue that an application that explicitly disables processing would 
be able to handle that. However, I agree that point 1 is problematic, and 
point 2 is definitely not clean.

> > Some of this has been discussed previously, see
> > <http://article.gmane.org/gmane.linux.drivers.uvc.devel/3061/>.
> > 
> > The patch tries to solve both of these problems: 1) making uvcvideo aware
> > of the Logitech controls and recalculating the expected frame sizes, and
> > 2) presenting the RGB Bayer formats through the V4L2 interface, so user
> > application can request them.
> 
> If, the problem seems to be bigger.
> We have:
> 1. Formats provided by usb descriptor and controlled over uvc protocol.
> 2. Formats provided by usb descriptor and controlled over XU

Do we really have devices that expose formats through the standard UVC 
descriptors but require XU access to select them ?

> 3. Formats controlled over XU and provided by documentation other kind
> of knowledge.
> 
> Last case in not properly handled by uvcvideo.ko, but IMHO it is not good
> way to have all possible XU in driver.
> Both problems you described should be fixed, but not in this way. If it
> is possible - uvcdynctl should provide/update format descriptor over
> uvc_xu_control_query interface or some other way.
> 
> Beside, i was working on kernel space plugin system for uvcvideo driver,
> which was probably not the best idea. How about to do some part of it in
> userspace? For example, uvcdynctl can provide bandwidth information too.
> This way we can fix many buggy cams without needing to permanently
> update kernel driver.
> Laurent?

I'm really undecided here.

On one hand I agree with you, from a theoretical point of view the driver 
should not know about all possible XUs. This just can't scale.

On the other hand, it's pretty clear that we don't need to scale. We only have 
a single public dynamic controls XML file, and looking at the descriptors of 
most devices it's pretty clear that they reuse the same XUs, as they are based 
on only a dozen or so different chips.

The dynamic controls API brings additional complexity to the driver, and I 
think that it was a good design decision. However, in some cases, the 
implications of changing an XU control value go well beyond what is usually 
expected of a control. I'm thus tempted to allow handling of XU controls in 
the kernel when there's a good reason to do so. Of course I want the code to 
be clean, without lots of hooks all over the place that would make the driver 
sources impossible to read and understand :-)

Comments and thoughts will be appreciated.

-- 
Regards,

Laurent Pinchart