On 13/08/15(Thu) 19:41, ludovic coues wrote:
> 2015-08-13 15:11 GMT+02:00 Martin Pieuchot <m...@openbsd.org>:
> >
> > With this diff I get the following error on my x220:
> >
> > uvideo_vs_negotiation: uvideo0: no frame descriptors found!
> >
> > I have this camera:
> >
> > uvideo0 at uhub3 port 6 configuration 1 interface 0 "Chicony Electronics
> > Co., Ltd. Integrated Camera" rev 2.00/8.54 addr 3
> >
> > And the descriptor dump:
> >
>
> I haven't any clue yet on why that happen.
> But that dump show that the diff is wrong. I have assumed bControlSize
> to be always 3 for usb_video_input_terminal_desc and
> usb_video_vc_processing_desc, as it's a constant in the spec 1.5 .
> Obviously, that the value can be less than 3 with device following the
> spec 1.00
>
I found the problem with the patch. Somehow, I forget to update some
section of the code to work with the new structure. More exactly, the
struct usb_video_frame_desc have gained 3 byte but the underlying data
can be 2 byte shorter than structure size if the device have fixed frame
rate. The size of the data was often compared to the size of the
structure. When updating the code, most of the test have been adjusted
by I missed a couple of them. One of these missed test is at line 1076
revision 1.181. As the test returned false for usb video device
returning a discrete range of less than 3 frame rate, the driver didn't
parse frame descriptor in these case.
Following patch should work better.
I made 2 patch this time. First patch if for the merged struct, second
is for usb_video_frame_desc.
Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.181
diff -u -p -r1.181 uvideo.c
--- sys/dev/usb/uvideo.c 9 Jul 2015 14:58:32 -0000 1.181
+++ sys/dev/usb/uvideo.c 23 Jul 2015 14:31:18 -0000
@@ -84,8 +84,8 @@ struct uvideo_softc {
int sc_nframes;
struct usb_video_probe_commit sc_desc_probe;
- struct usb_video_header_desc_all sc_desc_vc_header;
- struct usb_video_input_header_desc_all sc_desc_vs_input_header;
+ struct usb_video_header_desc *sc_desc_vc_header;
+ struct usb_video_input_header_desc *sc_desc_vs_input_header;
#define UVIDEO_MAX_PU 8
int sc_desc_vc_pu_num;
@@ -694,16 +693,15 @@ uvideo_vc_parse_desc_header(struct uvide
{
struct usb_video_header_desc *d;
- d = (struct usb_video_header_desc *)(uint8_t *)desc;
+ d = (struct usb_video_header_desc *)desc;
if (d->bInCollection == 0) {
printf("%s: no VS interface found!\n",
DEVNAME(sc));
return (USBD_INVAL);
}
-
- sc->sc_desc_vc_header.fix = d;
- sc->sc_desc_vc_header.baInterfaceNr = (uByte *)(d + 1);
+
+ sc->sc_desc_vc_header = d;
return (USBD_NORMAL_COMPLETION);
}
@@ -838,7 +836,7 @@ uvideo_vs_parse_desc(struct uvideo_softc
DPRINTF(1, "%s: number of total interfaces=%d\n",
DEVNAME(sc), sc->sc_nifaces);
DPRINTF(1, "%s: number of VS interfaces=%d\n",
- DEVNAME(sc), sc->sc_desc_vc_header.fix->bInCollection);
+ DEVNAME(sc), sc->sc_desc_vc_header->bInCollection);
usbd_desc_iter_init(sc->sc_udev, &iter);
desc = usbd_desc_iter_next(&iter);
@@ -874,8 +872,8 @@ uvideo_vs_parse_desc(struct uvideo_softc
return (error);
/* parse interface collection */
- for (i = 0; i < sc->sc_desc_vc_header.fix->bInCollection; i++) {
- iface = sc->sc_desc_vc_header.baInterfaceNr[i];
+ for (i = 0; i < sc->sc_desc_vc_header->bInCollection; i++) {
+ iface = sc->sc_desc_vc_header->baInterfaceNr[i];
id = usbd_get_interface_descriptor(&sc->sc_udev->ifaces[iface]);
if (id == NULL) {
@@ -916,8 +914,7 @@ uvideo_vs_parse_desc_input_header(struct
return (USBD_INVAL);
}
- sc->sc_desc_vs_input_header.fix = d;
- sc->sc_desc_vs_input_header.bmaControls = (uByte *)(d + 1);
+ sc->sc_desc_vs_input_header = d;
return (USBD_NORMAL_COMPLETION);
}
@@ -1500,12 +1498,12 @@ uvideo_vs_negotiation(struct uvideo_soft
* Some UVC 1.00 devices return dwMaxVideoFrameSize = 0.
* If so, fix it by format/frame descriptors.
*/
- hd = sc->sc_desc_vc_header.fix;
+ hd = sc->sc_desc_vc_header;
if (UGETDW(pc->dwMaxVideoFrameSize) == 0 &&
UGETW(hd->bcdUVC) < 0x0110 ) {
DPRINTF(1, "%s: dwMaxVideoFrameSize == 0, fixed\n",
DEVNAME(sc));
- USETDW(pc->dwMaxVideoFrameSize,
+ USETDW(pc->dwMaxVideoFrameSize,
UGETDW(frame->dwMaxVideoFrameBufferSize));
}
}
Index: sys/dev/usb/uvideo.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.h,v
retrieving revision 1.57
diff -u -p -r1.57 uvideo.h
--- sys/dev/usb/uvideo.h 9 Jul 2015 14:58:32 -0000 1.57
+++ sys/dev/usb/uvideo.h 23 Jul 2015 14:31:18 -0000
@@ -162,13 +162,9 @@ struct usb_video_header_desc {
uWord wTotalLength;
uDWord dwClockFrequency;
uByte bInCollection;
+ uByte baInterfaceNr[1];
} __packed;
-struct usb_video_header_desc_all {
- struct usb_video_header_desc *fix;
- uByte *baInterfaceNr;
-};
-
/* Table 3-4: Input Terminal Descriptor */
struct usb_video_input_terminal_desc {
uByte bLength;
@@ -255,13 +251,9 @@ struct usb_video_input_header_desc {
uByte bTriggerSupport;
uByte bTriggerUsage;
uByte bControlSize;
+ uByte bmaControls[1];
} __packed;
-struct usb_video_input_header_desc_all {
- struct usb_video_input_header_desc *fix;
- uByte *bmaControls;
-};
-
/* Table 3-18: Color Matching Descriptor */
struct usb_video_color_matching_descr {
uByte bLength;
Index: sys/dev/usb/uvideo.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.c,v
retrieving revision 1.181
diff -u -p -r1.181 uvideo.c
--- sys/dev/usb/uvideo.c 9 Jul 2015 14:58:32 -0000 1.181
+++ sys/dev/usb/uvideo.c 15 Aug 2015 09:59:29 -0000
@@ -668,8 +668,7 @@ uvideo_vc_parse_desc(struct uvideo_softc
break;
case UDESCSUB_VC_PROCESSING_UNIT:
/* XXX do correct length calculation */
- if (desc->bLength <
- sizeof(struct usb_video_frame_desc)) {
+ if (desc->bLength < UVIDEO_FRAME_DESC_LENGTH) {
(void)uvideo_vc_parse_desc_pu(sc, desc);
}
break;
@@ -1073,9 +1070,10 @@ uvideo_vs_parse_desc_frame(struct uvideo
desc = usbd_desc_iter_next(&iter);
while (desc) {
if (desc->bDescriptorType == UDESC_CS_INTERFACE &&
- desc->bLength > sizeof(struct usb_video_frame_desc) &&
+ desc->bLength > UVIDEO_FRAME_DESC_LENGTH &&
(desc->bDescriptorSubtype == UDESCSUB_VS_FRAME_MJPEG ||
- desc->bDescriptorSubtype ==
UDESCSUB_VS_FRAME_UNCOMPRESSED)) {
+ desc->bDescriptorSubtype == UDESCSUB_VS_FRAME_UNCOMPRESSED)
+ ) {
error = uvideo_vs_parse_desc_frame_sub(sc, desc);
if (error != USBD_NORMAL_COMPLETION)
return (error);
@@ -1090,8 +1088,8 @@ usbd_status
uvideo_vs_parse_desc_frame_sub(struct uvideo_softc *sc,
const usb_descriptor_t *desc)
{
- struct usb_video_frame_desc *fd =
- (struct usb_video_frame_desc *)(uint8_t *)desc;
+ /* Descriptor is variable sized */
+ struct usb_video_frame_desc *fd = (void *)desc;
int fmtidx, frame_num;
uint32_t fbuf_size;
@@ -1356,11 +1354,10 @@ uvideo_vs_negotiation(struct uvideo_soft
struct uvideo_format_group *fmtgrp;
struct usb_video_header_desc *hd;
struct usb_video_frame_desc *frame;
- uint8_t *p, *cur;
uint8_t probe_data[34];
uint32_t frame_ival, nivals, min, max, step, diff;
usbd_status error;
- int i, ival_bytes, changed = 0;
+ int i, cur, ival_bytes, changed = 0;
pc = (struct usb_video_probe_commit *)probe_data;
@@ -1384,19 +1381,15 @@ uvideo_vs_negotiation(struct uvideo_soft
if (sc->sc_frame_rate != 0) {
frame_ival = 10000000 / sc->sc_frame_rate;
/* find closest matching interval the device supports */
- p = (uint8_t *)fmtgrp->frame_cur;
- p += sizeof(struct usb_video_frame_desc);
+ frame = fmtgrp->frame_cur;
nivals = fmtgrp->frame_cur->bFrameIntervalType;
ival_bytes = fmtgrp->frame_cur->bLength -
- sizeof(struct usb_video_frame_desc);
+ UVIDEO_FRAME_DESC_LENGTH;
if (!nivals && (ival_bytes >= sizeof(uDWord) * 3)) {
/* continuous */
- min = UGETDW(p);
- p += sizeof(uDWord);
- max = UGETDW(p);
- p += sizeof(uDWord);
- step = UGETDW(p);
- p += sizeof(uDWord);
+ min = UGETDW(frame->continuous.dwMinFrameInterval);
+ max = UGETDW(frame->continuous.dwMaxFrameInterval);
+ step = UGETDW(frame->continuous.dwFrameIntervalStep);
if (frame_ival <= min)
frame_ival = min;
else if (frame_ival >= max)
@@ -1408,24 +1401,26 @@ uvideo_vs_negotiation(struct uvideo_soft
}
} else if (nivals > 0 && ival_bytes >= sizeof(uDWord)) {
/* discrete */
- cur = p;
+ cur = 0;
min = UINT_MAX;
for (i = 0; i < nivals; i++) {
if (ival_bytes < sizeof(uDWord)) {
/* short descriptor ? */
break;
}
- diff = abs(UGETDW(p) - frame_ival);
+ diff = abs(
+ UGETDW(frame->discrete.dwFrameInterval[i])
+ - frame_ival);
if (diff < min) {
min = diff;
- cur = p;
+ cur = i;
if (diff == 0)
break;
}
- p += sizeof(uDWord);
ival_bytes -= sizeof(uDWord);
}
- frame_ival = UGETDW(cur);
+ frame_ival =
+ UGETDW(frame->discrete.dwFrameInterval[cur]);
} else {
DPRINTF(1, "%s: %s: bad frame ival descriptor\n",
DEVNAME(sc), __func__);
@@ -2334,8 +2329,7 @@ uvideo_dump_desc_all(struct uvideo_softc
case UDESCSUB_VC_PROCESSING_UNIT:
printf("bDescriptorSubtype=0x%02x",
desc->bDescriptorSubtype);
- if (desc->bLength >
- sizeof(struct usb_video_frame_desc)) {
+ if (desc->bLength > UVIDEO_FRAME_DESC_LENGTH) {
printf(" (UDESCSUB_VS_FRAME_"
"UNCOMPRESSED)\n");
uvideo_dump_desc_frame(sc, desc);
@@ -2364,8 +2358,7 @@ uvideo_dump_desc_all(struct uvideo_softc
printf("bDescriptorSubtype=0x%02x",
desc->bDescriptorSubtype);
printf(" (UDESCSUB_VS_FRAME_MJPEG)\n");
- if (desc->bLength >
- sizeof(struct usb_video_frame_desc)) {
+ if (desc->bLength > UVIDEO_FRAME_DESC_LENGTH) {
printf("|\n");
uvideo_dump_desc_frame(sc, desc);
}
@@ -2629,10 +2622,10 @@ void
uvideo_dump_desc_frame(struct uvideo_softc *sc, const usb_descriptor_t *desc)
{
struct usb_video_frame_desc *d;
- uint8_t *p;
- int length, i;
+ int i;
- d = (struct usb_video_frame_desc *)(uint8_t *)desc;
+ /* Descriptor is variable sized */
+ d = (void*) desc;
printf("bLength=%d\n", d->bLength);
printf("bDescriptorType=0x%02x\n", d->bDescriptorType);
@@ -2649,33 +2642,23 @@ uvideo_dump_desc_frame(struct uvideo_sof
UGETDW(d->dwDefaultFrameInterval));
printf("bFrameIntervalType=0x%02x\n", d->bFrameIntervalType);
- p = (uint8_t *)d;
- p += sizeof(struct usb_video_frame_desc);
-
- if (!d->bFrameIntervalType) {
+ if (d->bFrameIntervalType == 0) {
/* continuous */
- if (d->bLength < (sizeof(struct usb_video_frame_desc) +
- sizeof(uDWord) * 3)) {
- printf("invalid frame descriptor length\n");
- } else {
- printf("dwMinFrameInterval = %d\n", UGETDW(p));
- p += sizeof(uDWord);
- printf("dwMaxFrameInterval = %d\n", UGETDW(p));
- p += sizeof(uDWord);
- printf("dwFrameIntervalStep = %d\n", UGETDW(p));
- p += sizeof(uDWord);
- }
+ printf("dwMinFrameInterval = %d\n",
+ UGETDW(d->continuous.dwMinFrameInterval));
+ printf("dwMaxFrameInterval = %d\n",
+ UGETDW(d->continuous.dwMaxFrameInterval));
+ printf("dwFrameIntervalStep = %d\n",
+ UGETDW(d->continuous.dwFrameIntervalStep));
} else {
/* discrete */
- length = d->bLength - sizeof(struct usb_video_frame_desc);
for (i = 0; i < d->bFrameIntervalType; i++) {
- if (length <= 0) {
+ if (d->bLength < UVIDEO_FRAME_DESC_LENGTH + i) {
printf("frame descriptor ended early\n");
break;
}
- printf("dwFrameInterval = %d\n", UGETDW(p));
- p += sizeof(uDWord);
- length -= sizeof(uDWord);
+ printf("dwFrameInterval = %d\n",
+ UGETDW(d->discrete.dwFrameInterval[i]));
}
}
}
@@ -2924,7 +2906,6 @@ uvideo_enum_fivals(void *v, struct v4l2_
int idx;
struct uvideo_format_group *fmtgrp = NULL;
struct usb_video_frame_desc *frame = NULL;
- uint8_t *p;
for (idx = 0; idx < sc->sc_fmtgrp_num; idx++) {
if (sc->sc_fmtgrp[idx].pixelformat == fivals->pixel_format) {
@@ -2945,33 +2926,29 @@ uvideo_enum_fivals(void *v, struct v4l2_
if (frame == NULL)
return (EINVAL);
- /* byte-wise pointer to start of frame intervals */
- p = (uint8_t *)frame;
- p += sizeof(struct usb_video_frame_desc);
-
if (frame->bFrameIntervalType == 0) {
if (fivals->index != 0)
return (EINVAL);
fivals->type = V4L2_FRMIVAL_TYPE_STEPWISE;
- fivals->stepwise.min.numerator = UGETDW(p);
+ fivals->stepwise.min.numerator =
+ UGETDW(frame->continuous.dwMinFrameInterval);
fivals->stepwise.min.denominator = 10000000;
- p += sizeof(uDWord);
- fivals->stepwise.max.numerator = UGETDW(p);
+ fivals->stepwise.max.numerator =
+ UGETDW(frame->continuous.dwMaxFrameInterval);
fivals->stepwise.max.denominator = 10000000;
- p += sizeof(uDWord);
- fivals->stepwise.step.numerator = UGETDW(p);
+ fivals->stepwise.step.numerator =
+ UGETDW(frame->continuous.dwFrameIntervalStep);
fivals->stepwise.step.denominator = 10000000;
- p += sizeof(uDWord);
} else {
if (fivals->index >= frame->bFrameIntervalType)
return (EINVAL);
- p += sizeof(uDWord) * fivals->index;
- if (p > frame->bLength + (uint8_t *)frame) {
+ if (fivals->index + UVIDEO_FRAME_DESC_LENGTH > frame->bLength){
printf("%s: frame desc too short?\n", __func__);
return (EINVAL);
}
fivals->type = V4L2_FRMIVAL_TYPE_DISCRETE;
- fivals->discrete.numerator = UGETDW(p);
+ fivals->discrete.numerator =
+ UGETDW(frame->discrete.dwFrameInterval[fivals->index]);
fivals->discrete.denominator = 10000000;
}
Index: sys/dev/usb/uvideo.h
===================================================================
RCS file: /cvs/src/sys/dev/usb/uvideo.h,v
retrieving revision 1.57
diff -u -p -r1.57 uvideo.h
--- sys/dev/usb/uvideo.h 9 Jul 2015 14:58:32 -0000 1.57
+++ sys/dev/usb/uvideo.h 15 Aug 2015 09:59:29 -0000
@@ -357,7 +349,17 @@ struct usb_video_frame_desc {
uDWord dwMaxVideoFrameBufferSize;
uDWord dwDefaultFrameInterval;
uByte bFrameIntervalType;
- /* uDWord ivals[]; frame intervals, length varies */
+#define UVIDEO_FRAME_DESC_LENGTH 26 /* That's the size up to this point */
+ union {
+ struct {
+ uDWord dwMinFrameInterval;
+ uDWord dwMaxFrameInterval;
+ uDWord dwFrameIntervalStep;
+ } continuous;
+ struct {
+ uDWord dwFrameInterval[1];
+ } discrete;
+ };
} __packed;
/*
