| Message ID | 20230506071427.28108-6-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: uvcvideo: Misc cleanups | expand |
Hi Laurent On Sat, 6 May 2023 at 09:14, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The intervals pointer is incremented for each interval when parsing the > format descriptor. This doesn't cause any issue as such, but gets in the > way of constifying some pointers. Modify the parsing code to index the > intervals pointer as an array and increment it in one go at end of > parsing. > > Careful readers will notice that the maxIntervalIndex variable is set to > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is > functionally equivalent, as n is equal to 3 in that case. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_driver.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 446bd8ff128c..11e4fa007f3f 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev, > */ > while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && > buffer[2] == ftype) { > + unsigned int maxIntervalIndex; > + > frame = &format->frames[format->nframes]; > if (ftype != UVC_VS_FRAME_FRAME_BASED) > n = buflen > 25 ? buffer[25] : 0; > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev, > > for (i = 0; i < n; ++i) { > interval = get_unaligned_le32(&buffer[26+4*i]); > - *(*intervals)++ = interval ? interval : 1; > + (*intervals)[i] = interval ? interval : 1; > } > > /* > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev, > * frame->wWidth * frame->wHeight / 8; > > /* Clamp the default frame interval to the boundaries. */ > - n -= frame->bFrameIntervalType ? 1 : 2; > + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; Maybe it is worth mentioning that bFrameIntervalType == 0 is a continuous interval. idex 0 is min and 1 is max. > frame->dwDefaultFrameInterval = > clamp(frame->dwDefaultFrameInterval, > frame->dwFrameInterval[0], > - frame->dwFrameInterval[n]); > + frame->dwFrameInterval[maxIntervalIndex]); > > /* > * Some devices report frame intervals that are not functional. > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev, > (100000000 / frame->dwDefaultFrameInterval) % 10); > > format->nframes++; > + *intervals += n; > + > buflen -= buffer[0]; > buffer += buffer[0]; > } > -- > Regards, > > Laurent Pinchart >
Hi Ricardo, On Mon, May 15, 2023 at 03:22:13PM +0200, Ricardo Ribalda wrote: > On Sat, 6 May 2023 at 09:14, Laurent Pinchart wrote: > > > > The intervals pointer is incremented for each interval when parsing the > > format descriptor. This doesn't cause any issue as such, but gets in the > > way of constifying some pointers. Modify the parsing code to index the > > intervals pointer as an array and increment it in one go at end of > > parsing. > > > > Careful readers will notice that the maxIntervalIndex variable is set to > > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is > > functionally equivalent, as n is equal to 3 in that case. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > drivers/media/usb/uvc/uvc_driver.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > index 446bd8ff128c..11e4fa007f3f 100644 > > --- a/drivers/media/usb/uvc/uvc_driver.c > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev, > > */ > > while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && > > buffer[2] == ftype) { > > + unsigned int maxIntervalIndex; > > + > > frame = &format->frames[format->nframes]; > > if (ftype != UVC_VS_FRAME_FRAME_BASED) > > n = buflen > 25 ? buffer[25] : 0; > > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev, > > > > for (i = 0; i < n; ++i) { > > interval = get_unaligned_le32(&buffer[26+4*i]); > > - *(*intervals)++ = interval ? interval : 1; > > + (*intervals)[i] = interval ? interval : 1; > > } > > > > /* > > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev, > > * frame->wWidth * frame->wHeight / 8; > > > > /* Clamp the default frame interval to the boundaries. */ > > - n -= frame->bFrameIntervalType ? 1 : 2; > > + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; > > Maybe it is worth mentioning that bFrameIntervalType == 0 is a > continuous interval. idex 0 is min and 1 is max. I'll update the comment to /* * Clamp the default frame interval to the boundaries. A zero * bFrameIntervalType value indicates a continuous frame * interval range, with dwFrameInterval[0] storing the minimum * value and dwFrameInterval[1] storing the maximum value. */ > > frame->dwDefaultFrameInterval = > > clamp(frame->dwDefaultFrameInterval, > > frame->dwFrameInterval[0], > > - frame->dwFrameInterval[n]); > > + frame->dwFrameInterval[maxIntervalIndex]); > > > > /* > > * Some devices report frame intervals that are not functional. > > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev, > > (100000000 / frame->dwDefaultFrameInterval) % 10); > > > > format->nframes++; > > + *intervals += n; > > + > > buflen -= buffer[0]; > > buffer += buffer[0]; > > }
Hi Laurent On Mon, 15 May 2023 at 16:33, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Mon, May 15, 2023 at 03:22:13PM +0200, Ricardo Ribalda wrote: > > On Sat, 6 May 2023 at 09:14, Laurent Pinchart wrote: > > > > > > The intervals pointer is incremented for each interval when parsing the > > > format descriptor. This doesn't cause any issue as such, but gets in the > > > way of constifying some pointers. Modify the parsing code to index the > > > intervals pointer as an array and increment it in one go at end of > > > parsing. > > > > > > Careful readers will notice that the maxIntervalIndex variable is set to > > > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is > > > functionally equivalent, as n is equal to 3 in that case. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > --- > > > drivers/media/usb/uvc/uvc_driver.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > > > index 446bd8ff128c..11e4fa007f3f 100644 > > > --- a/drivers/media/usb/uvc/uvc_driver.c > > > +++ b/drivers/media/usb/uvc/uvc_driver.c > > > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev, > > > */ > > > while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && > > > buffer[2] == ftype) { > > > + unsigned int maxIntervalIndex; > > > + > > > frame = &format->frames[format->nframes]; > > > if (ftype != UVC_VS_FRAME_FRAME_BASED) > > > n = buflen > 25 ? buffer[25] : 0; > > > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev, > > > > > > for (i = 0; i < n; ++i) { > > > interval = get_unaligned_le32(&buffer[26+4*i]); > > > - *(*intervals)++ = interval ? interval : 1; > > > + (*intervals)[i] = interval ? interval : 1; > > > } > > > > > > /* > > > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev, > > > * frame->wWidth * frame->wHeight / 8; > > > > > > /* Clamp the default frame interval to the boundaries. */ > > > - n -= frame->bFrameIntervalType ? 1 : 2; > > > + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; > > > > Maybe it is worth mentioning that bFrameIntervalType == 0 is a > > continuous interval. idex 0 is min and 1 is max. > > I'll update the comment to > > /* > * Clamp the default frame interval to the boundaries. A zero > * bFrameIntervalType value indicates a continuous frame > * interval range, with dwFrameInterval[0] storing the minimum > * value and dwFrameInterval[1] storing the maximum value. > */ > Thanks! It looks much better :) > > > frame->dwDefaultFrameInterval = > > > clamp(frame->dwDefaultFrameInterval, > > > frame->dwFrameInterval[0], > > > - frame->dwFrameInterval[n]); > > > + frame->dwFrameInterval[maxIntervalIndex]); > > > > > > /* > > > * Some devices report frame intervals that are not functional. > > > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev, > > > (100000000 / frame->dwDefaultFrameInterval) % 10); > > > > > > format->nframes++; > > > + *intervals += n; > > > + > > > buflen -= buffer[0]; > > > buffer += buffer[0]; > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 446bd8ff128c..11e4fa007f3f 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev, */ while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE && buffer[2] == ftype) { + unsigned int maxIntervalIndex; + frame = &format->frames[format->nframes]; if (ftype != UVC_VS_FRAME_FRAME_BASED) n = buflen > 25 ? buffer[25] : 0; @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev, for (i = 0; i < n; ++i) { interval = get_unaligned_le32(&buffer[26+4*i]); - *(*intervals)++ = interval ? interval : 1; + (*intervals)[i] = interval ? interval : 1; } /* @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev, * frame->wWidth * frame->wHeight / 8; /* Clamp the default frame interval to the boundaries. */ - n -= frame->bFrameIntervalType ? 1 : 2; + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1; frame->dwDefaultFrameInterval = clamp(frame->dwDefaultFrameInterval, frame->dwFrameInterval[0], - frame->dwFrameInterval[n]); + frame->dwFrameInterval[maxIntervalIndex]); /* * Some devices report frame intervals that are not functional. @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev, (100000000 / frame->dwDefaultFrameInterval) % 10); format->nframes++; + *intervals += n; + buflen -= buffer[0]; buffer += buffer[0]; }
The intervals pointer is incremented for each interval when parsing the format descriptor. This doesn't cause any issue as such, but gets in the way of constifying some pointers. Modify the parsing code to index the intervals pointer as an array and increment it in one go at end of parsing. Careful readers will notice that the maxIntervalIndex variable is set to 1 instead of n - 2 when bFrameIntervalType has a zero value. This is functionally equivalent, as n is equal to 3 in that case. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/usb/uvc/uvc_driver.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)