Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Mon, 6 Nov 2023 12:42:45 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Ricardo Ribalda <ribalda@chromium.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        Esker Wong <esker@chromium.org>
Subject: Re: [PATCH] media: uvcvideo: Implement V4L2_EVENT_FRAME_SYNC
Message-ID: <20231106104245.GB19272@pendragon.ideasonboard.com>
References: <20231020-uvc-event-v1-1-3baa0e9f6952@chromium.org>
 <20231106103925.GA19272@pendragon.ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20231106103925.GA19272@pendragon.ideasonboard.com>
Precedence: bulk

On Mon, Nov 06, 2023 at 12:39:26PM +0200, Laurent Pinchart wrote:
> On Fri, Oct 20, 2023 at 06:41:45AM +0000, Ricardo Ribalda wrote:
> > Add support for the frame_sync event, so user-space can become aware
> > earlier of new frames.
> > 
> > Suggested-by: Esker Wong <esker@chromium.org>
> > Tested-by: Esker Wong <esker@chromium.org>
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> > We have measured a latency of around 30msecs between frame sync
> > and dqbuf.
> 
> Not surprising, especially for large resolutions. I'm curious though,
> what do you use this event for ?
> 
> > ---
> >  drivers/media/usb/uvc/uvc_v4l2.c  | 2 ++
> >  drivers/media/usb/uvc/uvc_video.c | 8 +++++++-
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> > index f4988f03640a..9f3fb5fd2375 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -1352,6 +1352,8 @@ static int uvc_ioctl_subscribe_event(struct v4l2_fh *fh,
> >  	switch (sub->type) {
> >  	case V4L2_EVENT_CTRL:
> >  		return v4l2_event_subscribe(fh, sub, 0, &uvc_ctrl_sub_ev_ops);
> > +	case V4L2_EVENT_FRAME_SYNC:
> > +		return v4l2_event_subscribe(fh, sub, 0, NULL);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 28dde08ec6c5..1d4b4807b005 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1073,9 +1073,15 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> >  	 * that discontinuous sequence numbers always indicate lost frames.
> >  	 */
> >  	if (stream->last_fid != fid) {
> > -		stream->sequence++;
> > +		struct v4l2_event event = {
> > +			.type = V4L2_EVENT_FRAME_SYNC,
> > +			.u.frame_sync.frame_sequence =  ++stream->sequence,
> 
> Extra space before ++.
> 
> It's easy to miss the ++ there when reading the code, would the
> following be more readable ?
> 
> 		struct v4l2_event event = {
> 			.type = V4L2_EVENT_FRAME_SYNC,
> 		};
> 
> 		stream->sequence++;
> 		if (stream->sequence)
> 			uvc_video_stats_update(stream);
> 
> 		.u.frame_sync.frame_sequence = stream->sequence;

Obviously this should read

 		event.u.frame_sync.frame_sequence = stream->sequence;

> 		v4l2_event_queue(&stream->vdev, &event);
> 
> > +		};
> > +
> >  		if (stream->sequence)
> >  			uvc_video_stats_update(stream);
> > +
> > +		v4l2_event_queue(&stream->vdev, &event);
> >  	}
> >  
> >  	uvc_video_clock_decode(stream, buf, data, len);
> > 
> > ---
> > base-commit: ce55c22ec8b223a90ff3e084d842f73cfba35588
> > change-id: 20231020-uvc-event-d3d1bbbdcb2f

-- 
Regards,

Laurent Pinchart