Re: Adding a control for Sensor Orientation

[Hans de Goede <hdegoede@redhat.com>]

Mauro Carvalho Chehab wrote:
> On Mon, 16 Feb 2009 10:44:03 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> I've discussed this with Laurent Pinchart (and other webcam driver authors) and 
>> the conclusion was that having a table of USB-ID's + DMI strings in the driver, 
>> and design an API to tell userspace to sensor is upside down and have code for 
>> all this both in the driver and in userspace makes no sense. Esp since such a 
>> table will probably be more easy to update in userspace too. So the conclusion 
>> was to just put the entire table of cams with known upside down mounted sensors 
>> in userspace. This is currently in libv4l and making many philips webcam users 
>> happy (philips has a tendency to mount the sensor upside down).
> 
> Are you saying that you have a table at libv4l for what cameras have sensors
> flipped? 

Yes.

> This is really ugly and proofs that the api is broken. No userspace
> application or library should need to do any special hack based on usb id,
> driver name or querycap names.

Well libv4l is already pretty full of cam specific knowledge in the form of 
decompression algorithm's etc.

Quirk tables like this are best kept in userspace, esp. when userspace is the 
only consumer of the information, why store information in the kernel if the 
kernel never uses it at all? Take a look at HAL quirks for suspend resume, 
wireless on/off buttons, etc. for example.

> In the case of flipping, kernel should provide this info for userspace, at
> least for the cameras it knows it is flipped (based on USB ID or any other
> method). In the case of DMI, it seems ok to let userspace to use the kernel DMI
> support to read this info and detect if the sensor were mounted flipped on a
> notebook, but for those cams where such info is known based on USB ID, we need
> to have an interface to read this information. I can see some ways for doing it:
> 
> 1) via VIDIOC_QUERYCAP capabilities flag;
> 2) via VIDIOC_*CNTL read-only interfaces;
> 3) another ioctl for querying the webcam capabilities;
> 4) some info via sysfs interface;
> 
> IMO, the easier and more adequate way for this case is creating an enumbered
> control. Something like:
> 
> #define V4L2_CID_MOUNTED_ANGLE    (V4L2_CID_CAMERA_CLASS_BASE+17)
> 
> enum v4l2_mounted_angle {
> 	V4L2_CID_MOUNTED_ANGLE_0_DEGREES = 0,
> 	V4L2_CID_MOUNTED_ANGLE_90_DEGREES = 1,
> 	V4L2_CID_MOUNTED_ANGLE_180_DEGREES = 2,
> 	V4L2_CID_MOUNTED_ANGLE_270_DEGREES = 3,
> 	V4L2_CID_MOUNTED_ANGLE_VIA_DMI = 4,
> };
> 

Here you are making things nice and inconsistent, so the information is in the 
kernel, except where it is not (the DMI case). If we move this in to the 
kernel, we should move it *completely* in to the kernel.

I've discussed this with Laurent Pinchart, and it really makes the most sense 
to do this in userspace.

Userspace approach:
1 table is in userspace, libv4l reads it directly, done.

Kernelspace approach:
1 add a (smaller) table to *each* driver (which the driver has 0 use for)
2 add code to *each* driver to export this info
3 add code to libv4l to read this

You've just created a kernel round trip for no good reason at all, and added a 
significant amount of code to the kernel, which can live in userspace just as 
well. The userspace approach is the KISS way. Also it is far easier for people 
to upgrade libv4l, then it is to upgrade a kernel. Given that this table will 
most likely change regulary the ease of updating is another argument for doing 
this in userspace.

Also can we please STOP with coming up of new and novel ways of abusing the 
control API, the control API's purpose is for userspace to control v4l device 
settings. It is way overkill for things like communicating a few simple flags 
to userspace (and is a pain to use for things like that both on the kernel and 
the userspace side).

Regards,

Hans