Hi Laurent
On Mon, 15 May 2023 at 16:33, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> >
> > Reviewed-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> >
> > > ---
> > > 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
--
Ricardo Ribalda
