Re: [GIT PATCH FOR 2.6.40] uvcvideo patches

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 20-05-2011 16:47, Laurent Pinchart escreveu:
> Hi Mauro,
> 
> On Friday 20 May 2011 21:16:49 Mauro Carvalho Chehab wrote:
>> Em 20-05-2011 12:49, Laurent Pinchart escreveu:
>>> On Friday 20 May 2011 17:32:45 Mauro Carvalho Chehab wrote:
>>>> Em 15-05-2011 04:48, Laurent Pinchart escreveu:
>>>>> Hi Mauro,
>>>>>
>>>>> The following changes since commit
>>>
>>> f9b51477fe540fb4c65a05027fdd6f2ecce4db3b:
>>>>>   [media] DVB: return meaningful error codes in dvb_frontend
>>>>>   (2011-05-09 05:47:20 +0200)
>>>>>
>>>>> are available in the git repository at:
>>>>>   git://linuxtv.org/pinchartl/uvcvideo.git uvcvideo-next
>>>>>
>>>>> They replace the git pull request I've sent on Thursday with the same
>>>>> subject.
>>>>>
>>>>> Bob Liu (2):
>>>>>       Revert "V4L/DVB: v4l2-dev: remove get_unmapped_area"
>>>>>       uvcvideo: Add support for NOMMU arch
>>>>
>>>> IMO, such fixes should happen inside the arch bits, and not on each
>>>> driver. If this fix is needed for uvc video, the same fix should
>>>> probably needed to all other USB drivers, in order to work on NOMMU
>>>> arch.
>>>>
>>>> For now, I'm accepting this as a workaround, but please work on a
>>>> generic solution for it.
>>>
>>> A fix at the arch/ level isn't possible, as drivers need to implement the
>>> get_unmapped_area file operation in order to support NOMMU architectures.
>>> The proper fix is of course to port uvcvideo to videobuf2, and implement
>>> support for NOMMU in videobuf2. Modifications to individual drivers will
>>> still be needed to fill the get_unmapped_area operation pointer with a
>>> videobuf2 handler though.
>>
>> This doesn't sound nice, as most people test their drivers only on an
>> specific architecture. If the driver can work on more then one
>> architecture (e. g. if it is not part of the IP block of some SoC chip,
>> but, instead, uses some generic bus like USB or PCI), the driver itself
>> shouldn't contain any arch-specific bits. IMO, the proper fix should be
>> either at the DMA stuff or somewhere inside the bus driver implementation.
> 
> It might not sound nice, but that's how NOMMU works. It needs 
> get_unmapped_area. If you can get rid of that requirement I'll be happy to 
> remove NOMMU-specific support from the driver :-)

As I said, the patches were added, but we need to work on a better solution
than polluting every driver with

#if CONFIG_NOMMU

just because arm arch is crappy.

> 
>>>>> Hans de Goede (2):
>>>>>       v4l: Add M420 format definition
>>>>>       uvcvideo: Add M420 format support
>>>>
>>>> OK.
>>>>
>>>>> Laurent Pinchart (4):
>>>>>       v4l: Release module if subdev registration fails
>>>>>       uvcvideo: Register a v4l2_device
>>>>>       uvcvideo: Register subdevices for each entity
>>>>>       uvcvideo: Connect video devices to media entities
>>>>
>>>> We've discussed already about using the media controller for uvcvideo,
>>>> but I can't remember anymore what where your aguments in favor of
>>>> merging it (and, even if I've remembered it right now, the #irc channel
>>>> log is not the proper way to document the rationale to apply a patch).
>>>>
>>>> The thing is: it is clear that SoC embedded devices need the media
>>>> controller, as they have IP blocks that do weird things, and userspace
>>>> may need to access those, as it is not possible to control such IP
>>>> blocks using the V4L2 API.
>>>>
>>>> However, I have serious concerns about media_controller API usage on
>>>> generic drivers, as it is required that all drivers should be fully
>>>> configurable via V4L2 API alone, otherwise we'll have regressions, as no
>>>> generic applications use the media_controller.
>>>>
>>>> In other words, if you have enough arguments about why we should add
>>>> media_controller support at the uvcvideo, please clearly provide them at
>>>> the patch descriptions, as this is not obvious. It would equally
>>>> important do document, at the uvcvideo doc, what kind of information is
>>>> provided via the media_controller and why an userspace application
>>>> should care to use it.
>>>
>>> First of all, the uvcvideo driver doesn't require application to use the
>>> media controller API to operate. All configuration is handled through a
>>> V4L2 video device node, and these patches do not modify that. No change
>>> is required to applications to use the uvcvideo driver.
>>>
>>> There's however a reason why I want to add support for the media
>>> controller API to the uvcvideo driver (I wouldn't have submitted the
>>> patches otherwise
>>>
>>> :-)). UVC-compliant devices are modeled as a connected graph of entities
>>>
>>> (called terminals and units in the UVC world). The device topology can be
>>> arbitrarily complex (or simple, which is fortunately often the case) and
>>> is exported by the device to the host through USB descriptors. The
>>> uvcvideo driver parses the descriptor, creates an internal
>>> representation of the device internal topology, and maps V4L2 operations
>>> to the various entities that the device contains.
>>> The UVC specification standardizes a couple of entities (camera terminal,
>>> processing unit, ...) and allows device vendors to create vendor-specific
>>> entities called extension units (XUs in short). Those XUs are usually
>>> used to expose controls that are not standardized by UVC to the host.
>>> These controls can be anything from an activity LED to a firmware update
>>> system. The uvcvideo tries to map those XU controls to V4L2 controls
>>> when it makes sense (and when the controls are documented by the device
>>> manufacturer, which is unfortunately often not the case). If an XU
>>> control can't be mapped to a V4L2 control, it can be accessed through
>>> uvcvideo-specific (documented) ioctls.
>>>
>>> In order to access those XU controls, device-specific applications (such
>>> as a firmware update application for instance) need to know what XUs are
>>> present in the device and possibly how they are connected. That
>>> information can't be exported through V4L2. That's why I'm adding media
>>> controller support to the uvcvideo driver.
>>
>> By allowing access to those undocumented XU controls an Evil
>> Manufacturer(tm) could try to sell its own proprietary software that will
>> work on Linux where all other software will deadly fail or will produce
>> very bad results. We've seen that history before.

As you're putting some names, let me be clearer. In the past we had bad stuff
like this one:
	http://kerneltrap.org/node/3729

that ended by the need of somebody to rewrite the entire pwc driver, because
the only way to get a decent image using a Philips camera, with the 
"open source" driver were to run a closed-source binary.

undocumented controls that can do everything, as you said, can create a
similar situation to what we had in the past.

> We have several alternatives. One of them, that is being shipped in some 
> systems, is a uvcvideo driver patched by the Evil Manufacturer(tm), 
> incompatible with the mainline version. Another one is a closed-source 
> userspace driver based on libusb shipped by the Evil Manufacturer(tm). Yet 
> another one is webcams that work on Windows only. Which one do you prefer ?

I prefer to ask the vendor about the XU controls that he needs and add a proper
interface for them.

>> That's why I'm concerned a lot about exposing such internal raw interfaces
>> to userspace.
>>
>> It should be noticed that such XU-specific Linux applications will depend
>> if the vendor is working on Linux or if somebody else did some reverse
>> engineering and discovered (for example) how to upgrade a firmware for a
>> certain camera.
> 
> The only Evil Manufacturer(tm) I know of that really uses XUs is Logitech. 
> They've been quite supportive so far, and have documented at least part of 
> their XU controls.

If they're quite supportive, they are not an Evil Manufacturer. We can ask them
to document the XU controls they need and add a proper documented support for
them.

>> In the first case, we should simply ask the vendor to document that XU
>> control and export it as something else.

s/something else/other controls/

(that line got mangled when I've removed another phase from the sentence while
editing it)

> 
> Why "something else" ? The XU interface has been designed by the USB-IF to be 
> a kernelspace to userspace API. That's how Microsoft, and I think Apple, 
> implemented it (it might not be a reference though).
> 
>> In the latter case, the developer that did the reverse engineering can just
>> send us a patch adding the new V4L control/firmware upgrade logic/whatever
>> to us.
> 
> UVC is a class specification. I don't want to cripple the driver with tons of 
> device-specific code. We already have Apple iSight- and Logitech-specific code 
> and way too many quirks for my taste.

Unfortunately, by being a generic driver for an USB class, and with vendors not
quite following the specs, there's no way to avoid having device-specific stuff
there. Other similar drivers like snd-usb-audio and sound hda driver has lots
of quirks. In particular, the hda driver contains more lines to the patch-*.c
drivers (with the device-specific stuff) than the driver core:

$ wc -l sound/pci/hda/*.c
    267 sound/pci/hda/hda_beep.c
   5072 sound/pci/hda/hda_codec.c
    637 sound/pci/hda/hda_eld.c
   1084 sound/pci/hda/hda_generic.c
    818 sound/pci/hda/hda_hwdep.c
   2854 sound/pci/hda/hda_intel.c
    727 sound/pci/hda/hda_proc.c
   4988 sound/pci/hda/patch_analog.c
    572 sound/pci/hda/patch_ca0110.c
   1314 sound/pci/hda/patch_cirrus.c
    776 sound/pci/hda/patch_cmedia.c
   3905 sound/pci/hda/patch_conexant.c
   1749 sound/pci/hda/patch_hdmi.c
  20167 sound/pci/hda/patch_realtek.c
    335 sound/pci/hda/patch_si3054.c
   6434 sound/pci/hda/patch_sigmatel.c
   6107 sound/pci/hda/patch_via.c
  57806 total

Yeah, device-specific stuff sucks, but sometimes there's no way to properly
support a device.

>> So, I'm yet not convinced ;) In fact, I think we should just deprecate the
>> XU private ioctls.
> 
> http://www.quickcamteam.net/uvc-h264/USB_Video_Payload_H.264_0.87.pdf
> 
> That's a brain-dead proposal for a new H.264 payload format pushed by Logitech 
> and Microsoft. The document is a bit outdated, but the final version will 
> likely be close. It requires direct XU access from applications. I don't like 
> it either, and the alternative will be to not support H.264 UVC cameras at all 
> (something I might consider, by blacklisting the product completely). Are you 
> ready to refuse supporting large classes of USB hardware ?

What's the difference between:
	1) exposing XU access to userspace and having no applications using it;
	2) just blacklisting them.

The end result is the same.

The V4L2 API is capable of handling H.264 payload format.

So, between the two above alternatives, I would choose 3: 
	- to handle such XU access internally at the driver.

Mauro.