Hi Michael So if my understanding is correct, what you want to achieve is to avoid double stats_decode when the function returns -EAGAIN. Wouldn't it be simpler to simply move uvc_video_clock_decode() and uvc_video_stats_decode() before stream->last_fid = fid; just at the end of the function? Or am I missing something? Besides being a small and documented function, uvc_video_decode_start() is difficult to follow :), So I might be saying something stupid Regards On Sat, 24 Feb 2024 at 23:52, Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> wrote: > > When the uvc request will get parsed by uvc_video_decode_start it will > leave the function with -EAGAIN to be restarted on the next frame. While > the first wrong parse the statistics will already be updated with > uvc_video_stats_decode. > > One value e.g. is the error_count, which therefor will be incremented > twice in case the fid has changed on the way. This patch fixes the > unnecessary extra parsing by returning early from the function when the > fid has changed. > > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > --- > Changes in v2: > - Moved the EAGAIN bailout after the sequence handling as mentioned by Ricardo Ribalda > - Link to v1: https://lore.kernel.org/r/20240221-uvc-host-video-decode-start-v1-1-228995925c70@xxxxxxxxxxxxxx > --- > drivers/media/usb/uvc/uvc_video.c | 64 +++++++++++++++++++-------------------- > 1 file changed, 32 insertions(+), 32 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c > index 7cbf4692bd875..af368c45c4297 100644 > --- a/drivers/media/usb/uvc/uvc_video.c > +++ b/drivers/media/usb/uvc/uvc_video.c > @@ -1068,6 +1068,15 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > > fid = data[1] & UVC_STREAM_FID; > > + /* > + * Store the payload FID bit and return immediately when the buffer is > + * NULL. > + */ > + if (buf == NULL) { > + stream->last_fid = fid; > + return -ENODATA; > + } > + > /* > * Increase the sequence number regardless of any buffer states, so > * that discontinuous sequence numbers always indicate lost frames. > @@ -1076,20 +1085,34 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > stream->sequence++; > if (stream->sequence) > uvc_video_stats_update(stream); > + > + /* > + * Mark the buffer as done if we're at the beginning of a new frame. > + * End of frame detection is better implemented by checking the EOF > + * bit (FID bit toggling is delayed by one frame compared to the EOF > + * bit), but some devices don't set the bit at end of frame (and the > + * last payload can be lost anyway). We thus must check if the FID has > + * been toggled. > + * > + * stream->last_fid is initialized to -1, so the first isochronous > + * frame will never trigger an end of frame detection. > + * > + * Empty buffers (bytesused == 0) don't trigger end of frame detection > + * as it doesn't make sense to return an empty buffer. This also > + * avoids detecting end of frame conditions at FID toggling if the > + * previous payload had the EOF bit set. > + */ > + if (buf->bytesused) { > + uvc_dbg(stream->dev, FRAME, > + "Frame complete (FID bit toggled)\n"); > + buf->state = UVC_BUF_STATE_READY; > + return -EAGAIN; > + } > } > > uvc_video_clock_decode(stream, buf, data, len); > uvc_video_stats_decode(stream, data, len); > > - /* > - * Store the payload FID bit and return immediately when the buffer is > - * NULL. > - */ > - if (buf == NULL) { > - stream->last_fid = fid; > - return -ENODATA; > - } > - > /* Mark the buffer as bad if the error bit is set. */ > if (data[1] & UVC_STREAM_ERR) { > uvc_dbg(stream->dev, FRAME, > @@ -1124,29 +1147,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, > buf->state = UVC_BUF_STATE_ACTIVE; > } > > - /* > - * Mark the buffer as done if we're at the beginning of a new frame. > - * End of frame detection is better implemented by checking the EOF > - * bit (FID bit toggling is delayed by one frame compared to the EOF > - * bit), but some devices don't set the bit at end of frame (and the > - * last payload can be lost anyway). We thus must check if the FID has > - * been toggled. > - * > - * stream->last_fid is initialized to -1, so the first isochronous > - * frame will never trigger an end of frame detection. > - * > - * Empty buffers (bytesused == 0) don't trigger end of frame detection > - * as it doesn't make sense to return an empty buffer. This also > - * avoids detecting end of frame conditions at FID toggling if the > - * previous payload had the EOF bit set. > - */ > - if (fid != stream->last_fid && buf->bytesused != 0) { > - uvc_dbg(stream->dev, FRAME, > - "Frame complete (FID bit toggled)\n"); > - buf->state = UVC_BUF_STATE_READY; > - return -EAGAIN; > - } > - > stream->last_fid = fid; > > return data[0]; > > --- > base-commit: e89fbb5bc21a10a0de2bb878d4df09f538dc523b > change-id: 20240221-uvc-host-video-decode-start-af53df5924cd > > Best regards, > -- > Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx> > > -- Ricardo Ribalda
