Hi Ricardo On Fri, Feb 23, 2024 at 03:09:39PM +0100, Ricardo Ribalda wrote: >In your code when is uvc_video_stats_update() called or >stream->sequence incremented in normal use case? > >I might be interpreting it wrong, but it seems like if buf->bytesused >is !=0 that code is never called. Doh, Seems I missed that. I will move the condition behind the sequence handling code. Thanks for pointing this out. Michael >On Wed, 21 Feb 2024 at 23:53, Michael Grzeschik > 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 >> --- >> 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..fce5349b5f9fa 100644 >> --- a/drivers/media/usb/uvc/uvc_video.c >> +++ b/drivers/media/usb/uvc/uvc_video.c >> @@ -1068,11 +1068,43 @@ 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. >> */ >> if (stream->last_fid != fid) { >> + /* >> + * 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; >> + } >> + >> stream->sequence++; >> if (stream->sequence) >> uvc_video_stats_update(stream); >> @@ -1081,15 +1113,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream, >> 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: 3bf0514dc6f36f81ee11b1becd977cb87b4c90c6 >> change-id: 20240221-uvc-host-video-decode-start-af53df5924cd >> >> Best regards, >> -- >> Michael Grzeschik >> >> > > >-- >Ricardo Ribalda > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |