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 <[email protected]>
---
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@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..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 <[email protected]>
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
<[email protected]> 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 <[email protected]>
> ---
> 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@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..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 <[email protected]>
>
>
--
Ricardo Ribalda
On Thu, Feb 29, 2024 at 07:20:53PM +0100, Ricardo Ribalda wrote:
>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
No, there is nothing stupid. I tested the changed patch. And it works as
expected. So thanks for the review and the extra mile. I will send v3 now.
Regards,
Michael
>On Sat, 24 Feb 2024 at 23:52, Michael Grzeschik
><[email protected]> 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 <[email protected]>
>> ---
>> 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@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..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 <[email protected]>
>>
>>
>
>
>--
>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 |