Hi Benoît,
Thank you for the patch.
On Thu, Nov 07, 2024 at 02:22:03PM +0000, Benoit Sevens wrote:
> The ftype value does not change in the while loop so we can check it
> before entering the while loop. Refactoring the frame parsing code into
> a dedicated uvc_parse_frame function makes this more readable.
>
> Signed-off-by: Benoit Sevens <bsevens@xxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_driver.c | 228 ++++++++++++++++-------------
> 1 file changed, 123 insertions(+), 105 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index 13db0026dc1a..99f811ace5d6 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -220,6 +220,117 @@ static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
> * Descriptors parsing
> */
>
> +static int uvc_parse_frame(struct uvc_device *dev, struct uvc_streaming *streaming,
> + struct uvc_format *format, struct uvc_frame *frame, u32 **intervals,
> + u8 ftype, int width_multiplier, const unsigned char *buffer, int buflen)
> +{
> + struct usb_interface *intf = streaming->intf;
> + struct usb_host_interface *alts = intf->cur_altsetting;
The intf variable is only used here, so you can write
struct usb_host_interface *alts = streaming->intf->cur_altsetting;
> + unsigned int i, n;
> + unsigned int interval;
> + unsigned int maxIntervalIndex;
We usually sort variables in (more or less) decreasing line length order
if nothing makes that inpractical.
> +
> + if (ftype != UVC_VS_FRAME_FRAME_BASED)
> + n = buflen > 25 ? buffer[25] : 0;
> + else
> + n = buflen > 21 ? buffer[21] : 0;
> +
> + n = n ? n : 3;
> +
> + if (buflen < 26 + 4*n) {
> + uvc_dbg(dev, DESCR,
> + "device %d videostreaming interface %d FRAME error\n",
> + dev->udev->devnum,
> + alts->desc.bInterfaceNumber);
We can now reflow the code as the indentation level has decreased, for instance
dev->udev->devnum, alts->desc.bInterfaceNumber);
> + return -EINVAL;
> + }
> +
> + frame->bFrameIndex = buffer[3];
> + frame->bmCapabilities = buffer[4];
> + frame->wWidth = get_unaligned_le16(&buffer[5])
> + * width_multiplier;
> + frame->wHeight = get_unaligned_le16(&buffer[7]);
> + frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> + frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> + if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> + frame->dwMaxVideoFrameBufferSize =
> + get_unaligned_le32(&buffer[17]);
> + frame->dwDefaultFrameInterval =
> + get_unaligned_le32(&buffer[21]);
> + frame->bFrameIntervalType = buffer[25];
> + } else {
> + frame->dwMaxVideoFrameBufferSize = 0;
> + frame->dwDefaultFrameInterval =
> + get_unaligned_le32(&buffer[17]);
> + frame->bFrameIntervalType = buffer[21];
> + }
> +
> + /*
> + * Copy the frame intervals.
> + *
> + * Some bogus devices report dwMinFrameInterval equal to
> + * dwMaxFrameInterval and have dwFrameIntervalStep set to
> + * zero. Setting all null intervals to 1 fixes the problem and
> + * some other divisions by zero that could happen.
> + */
> + frame->dwFrameInterval = *intervals;
> +
> + for (i = 0; i < n; ++i) {
> + interval = get_unaligned_le32(&buffer[26+4*i]);
> + (*intervals)[i] = interval ? interval : 1;
> + }
> +
> + /*
> + * Apply more fixes, quirks and workarounds to handle incorrect
> + * or broken descriptors.
> + */
> +
> + /*
> + * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> + * completely. Observed behaviours range from setting the
> + * value to 1.1x the actual frame size to hardwiring the
> + * 16 low bits to 0. This results in a higher than necessary
> + * memory usage as well as a wrong image size information. For
> + * uncompressed formats this can be fixed by computing the
> + * value from the frame size.
> + */
> + if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> + frame->dwMaxVideoFrameBufferSize = format->bpp
> + * frame->wWidth * frame->wHeight / 8;
> +
> + /*
> + * 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.
> + */
> + maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> + frame->dwDefaultFrameInterval =
> + clamp(frame->dwDefaultFrameInterval,
> + frame->dwFrameInterval[0],
> + frame->dwFrameInterval[maxIntervalIndex]);
> +
> + /*
> + * Some devices report frame intervals that are not functional.
> + * If the corresponding quirk is set, restrict operation to the
> + * first interval only.
> + */
> + if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> + frame->bFrameIntervalType = 1;
> + (*intervals)[0] = frame->dwDefaultFrameInterval;
> + }
> +
> + uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> + frame->wWidth, frame->wHeight,
> + 10000000 / frame->dwDefaultFrameInterval,
> + (100000000 / frame->dwDefaultFrameInterval) % 10);
> +
> + *intervals += n;
> +
> + return buffer[0];
> +}
> +
> +
> static int uvc_parse_format(struct uvc_device *dev,
> struct uvc_streaming *streaming, struct uvc_format *format,
> struct uvc_frame *frames, u32 **intervals, const unsigned char *buffer,
> @@ -231,9 +342,9 @@ static int uvc_parse_format(struct uvc_device *dev,
While at it, we can also merge the intf and alts variables here too.
With this addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
I can make all those small changes locally when applying the patch,
unless you prefer submitting a new version yourself.
> struct uvc_frame *frame;
> const unsigned char *start = buffer;
> unsigned int width_multiplier = 1;
> - unsigned int interval;
> unsigned int i, n;
> u8 ftype;
> + int ret;
>
> format->type = buffer[2];
> format->index = buffer[3];
> @@ -371,111 +482,18 @@ static int uvc_parse_format(struct uvc_device *dev,
> * Parse the frame descriptors. Only uncompressed, MJPEG and frame
> * based formats have frame descriptors.
> */
> - while (ftype && buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> - buffer[2] == ftype) {
> - unsigned int maxIntervalIndex;
> -
> - frame = &frames[format->nframes];
> - if (ftype != UVC_VS_FRAME_FRAME_BASED)
> - n = buflen > 25 ? buffer[25] : 0;
> - else
> - n = buflen > 21 ? buffer[21] : 0;
> -
> - n = n ? n : 3;
> -
> - if (buflen < 26 + 4*n) {
> - uvc_dbg(dev, DESCR,
> - "device %d videostreaming interface %d FRAME error\n",
> - dev->udev->devnum,
> - alts->desc.bInterfaceNumber);
> - return -EINVAL;
> - }
> -
> - frame->bFrameIndex = buffer[3];
> - frame->bmCapabilities = buffer[4];
> - frame->wWidth = get_unaligned_le16(&buffer[5])
> - * width_multiplier;
> - frame->wHeight = get_unaligned_le16(&buffer[7]);
> - frame->dwMinBitRate = get_unaligned_le32(&buffer[9]);
> - frame->dwMaxBitRate = get_unaligned_le32(&buffer[13]);
> - if (ftype != UVC_VS_FRAME_FRAME_BASED) {
> - frame->dwMaxVideoFrameBufferSize =
> - get_unaligned_le32(&buffer[17]);
> - frame->dwDefaultFrameInterval =
> - get_unaligned_le32(&buffer[21]);
> - frame->bFrameIntervalType = buffer[25];
> - } else {
> - frame->dwMaxVideoFrameBufferSize = 0;
> - frame->dwDefaultFrameInterval =
> - get_unaligned_le32(&buffer[17]);
> - frame->bFrameIntervalType = buffer[21];
> - }
> -
> - /*
> - * Copy the frame intervals.
> - *
> - * Some bogus devices report dwMinFrameInterval equal to
> - * dwMaxFrameInterval and have dwFrameIntervalStep set to
> - * zero. Setting all null intervals to 1 fixes the problem and
> - * some other divisions by zero that could happen.
> - */
> - frame->dwFrameInterval = *intervals;
> -
> - for (i = 0; i < n; ++i) {
> - interval = get_unaligned_le32(&buffer[26+4*i]);
> - (*intervals)[i] = interval ? interval : 1;
> - }
> -
> - /*
> - * Apply more fixes, quirks and workarounds to handle incorrect
> - * or broken descriptors.
> - */
> -
> - /*
> - * Several UVC chipsets screw up dwMaxVideoFrameBufferSize
> - * completely. Observed behaviours range from setting the
> - * value to 1.1x the actual frame size to hardwiring the
> - * 16 low bits to 0. This results in a higher than necessary
> - * memory usage as well as a wrong image size information. For
> - * uncompressed formats this can be fixed by computing the
> - * value from the frame size.
> - */
> - if (!(format->flags & UVC_FMT_FLAG_COMPRESSED))
> - frame->dwMaxVideoFrameBufferSize = format->bpp
> - * frame->wWidth * frame->wHeight / 8;
> -
> - /*
> - * 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.
> - */
> - maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> - frame->dwDefaultFrameInterval =
> - clamp(frame->dwDefaultFrameInterval,
> - frame->dwFrameInterval[0],
> - frame->dwFrameInterval[maxIntervalIndex]);
> -
> - /*
> - * Some devices report frame intervals that are not functional.
> - * If the corresponding quirk is set, restrict operation to the
> - * first interval only.
> - */
> - if (dev->quirks & UVC_QUIRK_RESTRICT_FRAME_RATE) {
> - frame->bFrameIntervalType = 1;
> - (*intervals)[0] = frame->dwDefaultFrameInterval;
> + if (ftype) {
> + while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> + buffer[2] == ftype) {
> + frame = &frames[format->nframes];
> + ret = uvc_parse_frame(dev, streaming, format, frame, intervals, ftype,
> + width_multiplier, buffer, buflen);
> + if (ret < 0)
> + return ret;
> + format->nframes++;
> + buflen -= ret;
> + buffer += ret;
> }
> -
> - uvc_dbg(dev, DESCR, "- %ux%u (%u.%u fps)\n",
> - frame->wWidth, frame->wHeight,
> - 10000000 / frame->dwDefaultFrameInterval,
> - (100000000 / frame->dwDefaultFrameInterval) % 10);
> -
> - format->nframes++;
> - *intervals += n;
> -
> - buflen -= buffer[0];
> - buffer += buffer[0];
> }
>
> if (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
--
Regards,
Laurent Pinchart
