Re: [PATCH] uvc_video: check for fid change early in decode_start and avoid wrong error counting

[Michael Grzeschik <mgr@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6283 bytes --]

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
>

-- 
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 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]