diff mbox

[3/3,media] uvcvideo: skip non-extension unit controls on Oculus Rift Sensors

Message ID 20170714201424.23592-3-philipp.zabel@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel July 14, 2017, 8:14 p.m. UTC
The Oculus Rift Sensors (DK2 and CV1) allow to configure their sensor chips
directly via I2C commands using extension unit controls. The processing and
camera unit controls do not function at all.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Laurent Pinchart July 15, 2017, 9:54 a.m. UTC | #1
Hi Philipp,

Thank you for the patch.

On Friday 14 Jul 2017 22:14:24 Philipp Zabel wrote:
> The Oculus Rift Sensors (DK2 and CV1) allow to configure their sensor chips
> directly via I2C commands using extension unit controls. The processing and
> camera unit controls do not function at all.

Do the processing and camera units they report controls that don't work when 
exercised ? Who in a sane state of mind could have designed such a terrible 
product ?

If I understand you correctly, this device requires userspace code that knows 
how to program the sensor (and possibly other chips). If that's the case, is 
there an open-source implementation of that code publicly available ?

> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index 86cb16a2e7f4..573e1f8735bf 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -2165,6 +2165,10 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  {
>  	struct uvc_entity *entity;
>  	unsigned int i;
> +	const struct usb_device_id xu_only[] = {
> +		{ USB_DEVICE(0x2833, 0x0201) },
> +		{ USB_DEVICE(0x2833, 0x0211) },
> +	};
> 
>  	/* Walk the entities list and instantiate controls */
>  	list_for_each_entry(entity, &dev->entities, list) {
> @@ -2172,6 +2176,16 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  		unsigned int bControlSize = 0, ncontrols;
>  		__u8 *bmControls = NULL;
> 
> +		/* Oculus Sensors only handle extension unit controls */
> +		if (UVC_ENTITY_TYPE(entity) != UVC_VC_EXTENSION_UNIT) {
> +			for (i = 0; i < ARRAY_SIZE(xu_only); i++) {
> +				if (usb_match_one_id(dev->intf, &xu_only[i]))
> +					break;
> +			}
> +			if (i != ARRAY_SIZE(xu_only))
> +				continue;
> +		}
> +
>  		if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
>  			bmControls = entity->extension.bmControls;
>  			bControlSize = entity->extension.bControlSize;
Philipp Zabel July 15, 2017, 1:13 p.m. UTC | #2
Am Samstag, den 15.07.2017, 12:54 +0300 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.
> 
> On Friday 14 Jul 2017 22:14:24 Philipp Zabel wrote:
> > The Oculus Rift Sensors (DK2 and CV1) allow to configure their sensor chips
> > directly via I2C commands using extension unit controls. The processing and
> > camera unit controls do not function at all.
> 
> Do the processing and camera units they report controls that don't work when 
> exercised ? Who in a sane state of mind could have designed such a terrible 
> product ?

Yes. Without this patch I get a bunch of controls that have no effect
whatsoever.

> If I understand you correctly, this device requires userspace code that knows 
> how to program the sensor (and possibly other chips). If that's the case, is 
> there an open-source implementation of that code publicly available ?

Well, it's all still a bit in the experimentation phase. We have an
implementation to set up the DK2 camera for synchronised exposure
triggered by the Rift DK2 HMD and to read the calibration data from
flash, here:

https://github.com/pH5/ouvrt/blob/master/src/esp570.c
https://github.com/pH5/ouvrt/blob/master/src/mt9v034.c

And an even more rough version to set up the CV1 camera for
synchronised exposure triggered by the Rift CV1 HMD here:

https://github.com/OpenHMD/OpenHMD-RiftPlayground/blob/master/src/main.c

The latter is using libusb, as it needs the variable length SPI data
control.

Do you think adding a pseudo i2c driver for the eSP570/eSP770u webcam
controllers and then exposing the sensor chips as V4L2 subdevices could
be a good idea? We already have a sensor driver for the MT9V034 in the
DK2 USB camera.

regards
Philipp
Laurent Pinchart July 17, 2017, 2:25 a.m. UTC | #3
Hi Philipp,

On Saturday 15 Jul 2017 15:13:45 Philipp Zabel wrote:
> Am Samstag, den 15.07.2017, 12:54 +0300 schrieb Laurent Pinchart:
> > On Friday 14 Jul 2017 22:14:24 Philipp Zabel wrote:
> >> The Oculus Rift Sensors (DK2 and CV1) allow to configure their sensor
> >> chips directly via I2C commands using extension unit controls. The
> >> processing and camera unit controls do not function at all.
> > 
> > Do the processing and camera units they report controls that don't work
> > when  exercised ? Who in a sane state of mind could have designed such a
> > terrible product ?
> 
> Yes. Without this patch I get a bunch of controls that have no effect
> whatsoever.
> 
> > If I understand you correctly, this device requires userspace code that
> > knows  how to program the sensor (and possibly other chips). If that's
> > the case, is there an open-source implementation of that code publicly
> > available ?
>
> Well, it's all still a bit in the experimentation phase. We have an
> implementation to set up the DK2 camera for synchronised exposure
> triggered by the Rift DK2 HMD and to read the calibration data from
> flash, here:
> 
> https://github.com/pH5/ouvrt/blob/master/src/esp570.c
> https://github.com/pH5/ouvrt/blob/master/src/mt9v034.c
> 
> And an even more rough version to set up the CV1 camera for
> synchronised exposure triggered by the Rift CV1 HMD here:
> 
> https://github.com/OpenHMD/OpenHMD-RiftPlayground/blob/master/src/main.c
> 
> The latter is using libusb, as it needs the variable length SPI data
> control.
> 
> Do you think adding a pseudo i2c driver for the eSP570/eSP770u webcam
> controllers and then exposing the sensor chips as V4L2 subdevices could
> be a good idea? We already have a sensor driver for the MT9V034 in the
> DK2 USB camera.

Yes, I think a device-specific driver would make sense, especially if we can 
implement support for the sensor as a standalone V4L2 subdev driver. The 
device only fakes UVC compatibility :-(
Philipp Zabel July 24, 2017, 5:52 a.m. UTC | #4
Hi Laurent,

Am Montag, den 17.07.2017, 05:25 +0300 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> On Saturday 15 Jul 2017 15:13:45 Philipp Zabel wrote:
> > Am Samstag, den 15.07.2017, 12:54 +0300 schrieb Laurent Pinchart:
> > > On Friday 14 Jul 2017 22:14:24 Philipp Zabel wrote:
> > > > The Oculus Rift Sensors (DK2 and CV1) allow to configure their
> > > > sensor
> > > > chips directly via I2C commands using extension unit controls.
> > > > The
> > > > processing and camera unit controls do not function at all.
> > > 
> > > Do the processing and camera units they report controls that
> > > don't work
> > > when  exercised ? Who in a sane state of mind could have designed
> > > such a
> > > terrible product ?
> > 
> > Yes. Without this patch I get a bunch of controls that have no
> > effect
> > whatsoever.
> > 
> > > If I understand you correctly, this device requires userspace
> > > code that
> > > knows  how to program the sensor (and possibly other chips). If
> > > that's
> > > the case, is there an open-source implementation of that code
> > > publicly
> > > available ?
> > 
> > Well, it's all still a bit in the experimentation phase. We have an
> > implementation to set up the DK2 camera for synchronised exposure
> > triggered by the Rift DK2 HMD and to read the calibration data from
> > flash, here:
> > 
> > https://github.com/pH5/ouvrt/blob/master/src/esp570.c
> > https://github.com/pH5/ouvrt/blob/master/src/mt9v034.c
> > 
> > And an even more rough version to set up the CV1 camera for
> > synchronised exposure triggered by the Rift CV1 HMD here:
> > 
> > https://github.com/OpenHMD/OpenHMD-RiftPlayground/blob/master/src/m
> > ain.c
> > 
> > The latter is using libusb, as it needs the variable length SPI
> > data
> > control.
> > 
> > Do you think adding a pseudo i2c driver for the eSP570/eSP770u
> > webcam
> > controllers and then exposing the sensor chips as V4L2 subdevices
> > could
> > be a good idea? We already have a sensor driver for the MT9V034 in
> > the
> > DK2 USB camera.
> 
> Yes, I think a device-specific driver would make sense, especially if
> we can 
> implement support for the sensor as a standalone V4L2 subdev driver.
> The 
> device only fakes UVC compatibility :-(

When you say standalone driver, do you mean I can reuse uvcvideo
device/stream/chain handling, and just replace the control handling?

I'll try this, but it isn't a straightforward as I initially thought.
For example, the mt9v032 subdev driver expects to have control over
power during probe and s_power. But in this case power is controlled by
UVC streaming. I'd either have to modify the subdev driver to support a
passive mode or fake the chip id register reads in the i2c adapter
driver to make mt9v032 probe at all.

Do you have any further comments on the first two patches?

regards
Philipp
Laurent Pinchart July 24, 2017, 11:10 p.m. UTC | #5
Hi Philipp,

On Monday 24 Jul 2017 07:52:22 Philipp Zabel wrote:
> Am Montag, den 17.07.2017, 05:25 +0300 schrieb Laurent Pinchart:
> > On Saturday 15 Jul 2017 15:13:45 Philipp Zabel wrote:
> >> Am Samstag, den 15.07.2017, 12:54 +0300 schrieb Laurent Pinchart:
> >>> On Friday 14 Jul 2017 22:14:24 Philipp Zabel wrote:
> >>>> The Oculus Rift Sensors (DK2 and CV1) allow to configure their
> >>>> sensor chips directly via I2C commands using extension unit
> >>>> controls. The processing and camera unit controls do not function at
> >>>> all.
> >>> 
> >>> Do the processing and camera units they report controls that
> >>> don't work when  exercised ? Who in a sane state of mind could have
> >>> designed such a terrible product ?
> >> 
> >> Yes. Without this patch I get a bunch of controls that have no
> >> effect whatsoever.
> >> 
> >>> If I understand you correctly, this device requires userspace
> >>> code that knows  how to program the sensor (and possibly other chips).
> >>> If that's the case, is there an open-source implementation of that
> >>> code publicly available ?
> >> 
> >> Well, it's all still a bit in the experimentation phase. We have an
> >> implementation to set up the DK2 camera for synchronised exposure
> >> triggered by the Rift DK2 HMD and to read the calibration data from
> >> flash, here:
> >> 
> >> https://github.com/pH5/ouvrt/blob/master/src/esp570.c
> >> https://github.com/pH5/ouvrt/blob/master/src/mt9v034.c
> >> 
> >> And an even more rough version to set up the CV1 camera for
> >> synchronised exposure triggered by the Rift CV1 HMD here:
> >> 
> >> https://github.com/OpenHMD/OpenHMD-RiftPlayground/blob/master/src/m
> >> ain.c
> >> 
> >> The latter is using libusb, as it needs the variable length SPI
> >> data control.
> >> 
> >> Do you think adding a pseudo i2c driver for the eSP570/eSP770u
> >> webcam controllers and then exposing the sensor chips as V4L2 subdevices
> >> could be a good idea? We already have a sensor driver for the MT9V034 in
> >> the DK2 USB camera.
> > 
> > Yes, I think a device-specific driver would make sense, especially if
> > we can implement support for the sensor as a standalone V4L2 subdev
> > driver. The device only fakes UVC compatibility :-(
> 
> When you say standalone driver, do you mean I can reuse uvcvideo
> device/stream/chain handling, and just replace the control handling?

No, I mean a completely separate driver. Given that the driver will be used 
for a single device, you can hardcode lots of assumptions and don't have to 
parse UVC descriptors.

> I'll try this, but it isn't a straightforward as I initially thought.
> For example, the mt9v032 subdev driver expects to have control over
> power during probe and s_power. But in this case power is controlled by
> UVC streaming.

How does that work with the device ? If the sensor is powered off until you 
start video streaming, I assume it won't reply to I2C commands. Do you need to 
configure it after stream start ?

> I'd either have to modify the subdev driver to support a passive mode or
> fake the chip id register reads in the i2c adapter driver to make mt9v032
> probe at all.
> 
> Do you have any further comments on the first two patches?

Just that those patches are not needed if we implement a driver specific to 
the Oculus Rift :-)
Philipp Zabel July 25, 2017, 5:51 a.m. UTC | #6
Hi Laurent,

Am Dienstag, den 25.07.2017, 02:10 +0300 schrieb Laurent Pinchart:
> > > Yes, I think a device-specific driver would make sense, especially if
> > > we can implement support for the sensor as a standalone V4L2 subdev
> > > driver. The device only fakes UVC compatibility :-(
> > 
> > When you say standalone driver, do you mean I can reuse uvcvideo
> > device/stream/chain handling, and just replace the control handling?
> 
> No, I mean a completely separate driver. Given that the driver will be used 
> for a single device, you can hardcode lots of assumptions and don't have to 
> parse UVC descriptors.

I see, I was hoping I wouldn't have to (re)write all the video transfer
and timing parts myself.

> > I'll try this, but it isn't a straightforward as I initially thought.
> > For example, the mt9v032 subdev driver expects to have control over
> > power during probe and s_power. But in this case power is controlled by
> > UVC streaming.
> 
> How does that work with the device ? If the sensor is powered off until you 
> start video streaming, I assume it won't reply to I2C commands. Do you need to 
> configure it after stream start ?

Yes. The webcam controllers replay the stored initialization sequences
to the sensors on startup, like any other usb cameras, and start
streaming images. That is why I added them to uvcvideo in the first
place.

After the stream has started, I'd like to change the controls from the
defaults (enable AEC/AGC or increase gain for normal camera use, or
reduce gain and exposure time and enable trigger mode for Rift
tracking). Unfortunatley those can only be changed via I2C.

> > I'd either have to modify the subdev driver to support a passive mode or
> > fake the chip id register reads in the i2c adapter driver to make mt9v032
> > probe at all.
> > 
> > Do you have any further comments on the first two patches?
> 
> Just that those patches are not needed if we implement a driver specific to 
> the Oculus Rift :-)

Ok. I'll give that a shot then.

regards
Philipp
diff mbox

Patch

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 86cb16a2e7f4..573e1f8735bf 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -2165,6 +2165,10 @@  int uvc_ctrl_init_device(struct uvc_device *dev)
 {
 	struct uvc_entity *entity;
 	unsigned int i;
+	const struct usb_device_id xu_only[] = {
+		{ USB_DEVICE(0x2833, 0x0201) },
+		{ USB_DEVICE(0x2833, 0x0211) },
+	};
 
 	/* Walk the entities list and instantiate controls */
 	list_for_each_entry(entity, &dev->entities, list) {
@@ -2172,6 +2176,16 @@  int uvc_ctrl_init_device(struct uvc_device *dev)
 		unsigned int bControlSize = 0, ncontrols;
 		__u8 *bmControls = NULL;
 
+		/* Oculus Sensors only handle extension unit controls */
+		if (UVC_ENTITY_TYPE(entity) != UVC_VC_EXTENSION_UNIT) {
+			for (i = 0; i < ARRAY_SIZE(xu_only); i++) {
+				if (usb_match_one_id(dev->intf, &xu_only[i]))
+					break;
+			}
+			if (i != ARRAY_SIZE(xu_only))
+				continue;
+		}
+
 		if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT) {
 			bmControls = entity->extension.bmControls;
 			bControlSize = entity->extension.bControlSize;