Received-SPF: pass (google.com: domain of linux-kernel+bounces-87269-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240221-uvc-host-video-decode-start-v2-1-88c6e17e487a@pengutronix.de>
In-Reply-To: <20240221-uvc-host-video-decode-start-v2-1-88c6e17e487a@pengutronix.de>
From: Ricardo Ribalda <ribalda@chromium.org>
Date: Thu, 29 Feb 2024 19:20:53 +0100
Message-ID: <CANiDSCtCse74oK_nCcJXRRQ__RnAAfYEFzfftty58stsFVKoYg@mail.gmail.com>
Subject: Re: [PATCH v2] uvc_video: check for fid change early in decode_start
 and avoid wrong error counting
To: Michael Grzeschik <m.grzeschik@pengutronix.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, 
	Mauro Carvalho Chehab <mchehab@kernel.org>, linux-media@vger.kernel.org, 
	linux-kernel@vger.kernel.org, kernel@pengutronix.de
Content-Type: text/plain; charset="UTF-8"

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@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>
> ---
> 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 <m.grzeschik@pengutronix.de>
>
>


-- 
Ricardo Ribalda