[PATCH 1/3] [media] uvcvideo: variable size controls

[PATCH 1/3] [media] uvcvideo: variable size controls

[Philipp Zabel]

Some USB webcam controllers have extension unit controls that report
different lengths via GET_LEN, depending on internal state. Add a flag
to mark these controls as variable length and issue GET_LEN before
GET/SET_CUR transfers to verify the current length.

Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++-
 include/uapi/linux/uvcvideo.h    |  2 ++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index c2ee6e39fd0c..ce69e2c6937d 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 		struct usb_device_id id;
 		u8 entity;
 		u8 selector;
-		u8 flags;
+		u16 flags;
 	};
 
 	static const struct uvc_ctrl_fixup fixups[] = {
@@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 		goto done;
 	}
 
+	if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) {
+		data = kmalloc(2, GFP_KERNEL);
+		/* Check if the control length has changed */
+		ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit,
+				     chain->dev->intfnum, xqry->selector, data,
+				     2);
+		size = le16_to_cpup((__le16 *)data);
+		kfree(data);
+		if (ret < 0) {
+			uvc_trace(UVC_TRACE_CONTROL,
+				  "GET_LEN failed on control %pUl/%u (%d).\n",
+				  entity->extension.guidExtensionCode,
+				  xqry->selector, ret);
+			goto done;
+		}
+		if (ctrl->info.size != size) {
+			uvc_trace(UVC_TRACE_CONTROL,
+				  "XU control %pUl/%u queried: len %u -> %u\n",
+				  entity->extension.guidExtensionCode,
+				  xqry->selector, ctrl->info.size, size);
+			ctrl->info.size = size;
+		}
+	}
+
 	if (size != xqry->size) {
 		ret = -ENOBUFS;
 		goto done;
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 3b081862b9e8..0f0d63e79045 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -27,6 +27,8 @@
 #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
 /* Control can be updated by the camera. */
 #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
+/* Control can change LEN */
+#define UVC_CTRL_FLAG_VARIABLE_LEN	(1 << 8)
 
 #define UVC_CTRL_FLAG_GET_RANGE \
 	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
-- 
2.13.2

[PATCH 2/3] [media] uvcvideo: flag variable length control on Oculus Rift CV1 Sensor

[Philipp Zabel]

The extension unit controls with selectors 11 and 12 are used to make the
eSP770u webcam controller issue SPI transfers to configure the nRF51288
radio or to read the flash storage. Depending on internal state controlled
by selector 11, selector 12 reports different lengths.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index ce69e2c6937d..86cb16a2e7f4 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1613,6 +1613,9 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device *dev,
 			UVC_CTRL_FLAG_GET_MIN | UVC_CTRL_FLAG_GET_MAX |
 			UVC_CTRL_FLAG_GET_DEF | UVC_CTRL_FLAG_SET_CUR |
 			UVC_CTRL_FLAG_AUTO_UPDATE },
+		{ { USB_DEVICE(0x2833, 0x0211) }, 4, 12,
+			UVC_CTRL_FLAG_GET_RANGE | UVC_CTRL_FLAG_SET_CUR |
+			UVC_CTRL_FLAG_VARIABLE_LEN },
 	};
 
 	unsigned int i;
-- 
2.13.2

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

[Philipp Zabel]

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(+)

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;
-- 
2.13.2

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

[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 ?

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;

-- 
Regards,

Laurent Pinchart

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

[Philipp Zabel]

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

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

[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/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 :-(

-- 
Regards,

Laurent Pinchart

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

[Philipp Zabel]

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

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

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart

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

[Philipp Zabel]

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

Re: [PATCH 1/3] [media] uvcvideo: variable size controls

[Laurent Pinchart]

Hi Philipp,

Thank you for the patch.

On Friday 14 Jul 2017 22:14:22 Philipp Zabel wrote:
> Some USB webcam controllers have extension unit controls that report
> different lengths via GET_LEN, depending on internal state.

If I ever need to hire a hardware designer, I'll make sure to reject any 
candidate who thinks that creativity is an asset :-(

If the size changes, could the flags change as well ? Should you issue a 
GET_INFO too ?

> Add a flag to mark these controls as variable length and issue GET_LEN
> before GET/SET_CUR transfers to verify the current length.

What happens if the internal state changes between the GET_LEN and the 
GET/SET_CUR ?

> Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++-
>  include/uapi/linux/uvcvideo.h    |  2 ++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e39fd0c..ce69e2c6937d 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device
> *dev, struct usb_device_id id;
>  		u8 entity;
>  		u8 selector;
> -		u8 flags;
> +		u16 flags;
>  	};
> 
>  	static const struct uvc_ctrl_fixup fixups[] = {
> @@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  		goto done;
>  	}
> 
> +	if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) {
> +		data = kmalloc(2, GFP_KERNEL);
> +		/* Check if the control length has changed */
> +		ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit,
> +				     chain->dev->intfnum, xqry->selector, 
data,
> +				     2);
> +		size = le16_to_cpup((__le16 *)data);
> +		kfree(data);

Now data is not NULL.

> +		if (ret < 0) {
> +			uvc_trace(UVC_TRACE_CONTROL,
> +				  "GET_LEN failed on control %pUl/%u (%d).\n",
> +				  entity->extension.guidExtensionCode,
> +				  xqry->selector, ret);
> +			goto done;

And the kfree(data) at the done label will cause a double free.

> +		}
> +		if (ctrl->info.size != size) {
> +			uvc_trace(UVC_TRACE_CONTROL,
> +				  "XU control %pUl/%u queried: len %u -> 
%u\n",
> +				  entity->extension.guidExtensionCode,
> +				  xqry->selector, ctrl->info.size, size);
> +			ctrl->info.size = size;
> +		}
> +	}

How about moving this code (or part of it at least) to a function that could 
be shared with uvc_ctrl_fill_xu_info() ?

>  	if (size != xqry->size) {
>  		ret = -ENOBUFS;
>  		goto done;
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 3b081862b9e8..0f0d63e79045 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -27,6 +27,8 @@
>  #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
>  /* Control can be updated by the camera. */
>  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
> +/* Control can change LEN */
> +#define UVC_CTRL_FLAG_VARIABLE_LEN	(1 << 8)
> 
>  #define UVC_CTRL_FLAG_GET_RANGE \
>  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] [media] uvcvideo: variable size controls

[Philipp Zabel]

Hi Laurent,

Am Samstag, den 15.07.2017, 12:49 +0300 schrieb Laurent Pinchart:
> Hi Philipp,
> 
> Thank you for the patch.

Thank you for the review.

> On Friday 14 Jul 2017 22:14:22 Philipp Zabel wrote:
> > Some USB webcam controllers have extension unit controls that report
> > different lengths via GET_LEN, depending on internal state.
> 
> If I ever need to hire a hardware designer, I'll make sure to reject any 
> candidate who thinks that creativity is an asset :-(

:)

> If the size changes, could the flags change as well ? Should you issue a 
> GET_INFO too ?

The size-changing eSP770U control always reports the same INFO.
Could this be added as a separate flag in the future if needed, when
the next webcam controller designer feels creative?

> > Add a flag to mark these controls as variable length and issue GET_LEN
> > before GET/SET_CUR transfers to verify the current length.
> 
> What happens if the internal state changes between the GET_LEN and the 
> GET/SET_CUR ?

At least for the Oculus Sensor, as far as I can tell, the length only
changes in reaction to a SET_CUR on another control:
Control 11 is written to set up an SPI transfer, apparently. That 16-
byte control contains a length field in byte 9. The length written to
that field becomes the LEN of control 12, which can then be used to
read or write data.

> > Signed-off-by: Philipp Zabel <philipp.zabel@gmail.com>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 26 +++++++++++++++++++++++++-
> >  include/uapi/linux/uvcvideo.h    |  2 ++
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c index c2ee6e39fd0c..ce69e2c6937d 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -1597,7 +1597,7 @@ static void uvc_ctrl_fixup_xu_info(struct uvc_device
> > *dev, struct usb_device_id id;
> > > >  		u8 entity;
> > > >  		u8 selector;
> > > > -		u8 flags;
> > > > +		u16 flags;
> > > >  	};
> > 
> > > >  	static const struct uvc_ctrl_fixup fixups[] = {
> > @@ -1799,6 +1799,30 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> > > >  		goto done;
> > > >  	}
> > 
> > > > +	if ((ctrl->info.flags & UVC_CTRL_FLAG_VARIABLE_LEN) && reqflags) {
> > > > +		data = kmalloc(2, GFP_KERNEL);
> > > > +		/* Check if the control length has changed */
> > > > +		ret = uvc_query_ctrl(chain->dev, UVC_GET_LEN, xqry->unit,
> > +				     chain->dev->intfnum, xqry->selector, 
> 
> data,
> > > > +				     2);
> > > > +		size = le16_to_cpup((__le16 *)data);
> > +		kfree(data);
> 
> Now data is not NULL.
> 
> > > > +		if (ret < 0) {
> > > > +			uvc_trace(UVC_TRACE_CONTROL,
> > > > +				  "GET_LEN failed on control %pUl/%u (%d).\n",
> > > > +				  entity->extension.guidExtensionCode,
> > > > +				  xqry->selector, ret);
> > +			goto done;
> 
> And the kfree(data) at the done label will cause a double free.

Thanks, will fix that.

> > +		}
> > > > +		if (ctrl->info.size != size) {
> > > > +			uvc_trace(UVC_TRACE_CONTROL,
> > +				  "XU control %pUl/%u queried: len %u -> 
> 
> %u\n",
> > > > +				  entity->extension.guidExtensionCode,
> > > > +				  xqry->selector, ctrl->info.size, size);
> > > > +			ctrl->info.size = size;
> > > > +		}
> > +	}
> 
> How about moving this code (or part of it at least) to a function that could 
> be shared with uvc_ctrl_fill_xu_info() ?

I'll try. This will come at the cost of a bit more boilerplate.

regards
Philipp