| Message ID | 20240221-uvc-host-video-decode-start-v1-1-228995925c70@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uvc_video: check for fid change early in decode_start and avoid wrong error counting | expand |
Hi Michael 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. Regards! On Wed, 21 Feb 2024 at 23:53, Michael Grzeschik <m.grzeschik@pengutronix.de> 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@pengutronix.de> > --- > 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 <m.grzeschik@pengutronix.de> > >
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 ><m.grzeschik@pengutronix.de> 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@pengutronix.de> >> --- >> 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 <m.grzeschik@pengutronix.de> >> >> > > >-- >Ricardo Ribalda >
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];
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@pengutronix.de> --- drivers/media/usb/uvc/uvc_video.c | 64 +++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 32 deletions(-) --- base-commit: 3bf0514dc6f36f81ee11b1becd977cb87b4c90c6 change-id: 20240221-uvc-host-video-decode-start-af53df5924cd Best regards,