From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp-vbr12.xs4all.nl ([194.109.24.32]:1839 "EHLO smtp-vbr12.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752967Ab3KKMQZ (ORCPT ); Mon, 11 Nov 2013 07:16:25 -0500 Message-ID: <5280CA88.8030207@xs4all.nl> Date: Mon, 11 Nov 2013 13:16:08 +0100 From: Hans Verkuil MIME-Version: 1.0 To: Sakari Ailus CC: linux-media@vger.kernel.org, vinod.govindapillai@intel.com Subject: Re: [PATCH 1/1] v4l: Add frame end event References: <1383311443-7863-1-git-send-email-sakari.ailus@linux.intel.com> In-Reply-To: <1383311443-7863-1-git-send-email-sakari.ailus@linux.intel.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Hi Sakari, On 11/01/2013 02:10 PM, Sakari Ailus wrote: > Add an event to signal frame end. This is not necessarily the same timestamp > as the video buffer done timestamp, and can be also subscribed by other > processes than the one controlling streaming and buffer (de)queueing. > > Also make all event type constants appear as constants in documentation and > move frame sync event struct documentation after all control event > documentation. > > Signed-off-by: Sakari Ailus > --- > Hi folks, > > As we have a frame sync event that's being used to tell about the frame > start, I thought of having a frame end event as well. This isn't exactly the > same as a buffer ready event which could take place already earlier for > instance if cropping is being done. > > I propose to use the id field for the purpose (V4L2_EVENT_FRAME_SYNC_START / > V4L2_EVENT_FRAME_SYNC_END). The frame start event will retain its old id > zero. > > Originally I think we thought of using the id field for the line, but > now I think it's worth adding a distinct event for that purpose: line based > events are typically triggered from other sources than line based events. > Frame sync, in my opinion, matches better with frame start and frame end > than to anything related to lines. > > So should line based events be supported, they should have their own event > type and use the id field as the line number. I have no objections to this patch. You do need to adapt drivers/media/platform/omap3isp/ispccdc.c a bit since it is using the FRAME_SYNC event and so it should check the id field. But will you also be upstreaming a driver that uses the SYNC_END? I don't really want to merge this if nobody is using it. Regards, Hans > > Kind regards, > Sakari > > Documentation/DocBook/media/v4l/vidioc-dqevent.xml | 58 +++++++++++++++------- > .../DocBook/media/v4l/vidioc-subscribe-event.xml | 11 +++- > include/uapi/linux/videodev2.h | 3 ++ > 3 files changed, 51 insertions(+), 21 deletions(-) > > diff --git a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml > index 89891ad..e258c6e 100644 > --- a/Documentation/DocBook/media/v4l/vidioc-dqevent.xml > +++ b/Documentation/DocBook/media/v4l/vidioc-dqevent.xml > @@ -76,21 +76,22 @@ > > &v4l2-event-vsync; > vsync > - Event data for event V4L2_EVENT_VSYNC. > + Event data for event V4L2_EVENT_VSYNC. > > > > > &v4l2-event-ctrl; > ctrl > - Event data for event V4L2_EVENT_CTRL. > + Event data for event V4L2_EVENT_CTRL. > > > > > &v4l2-event-frame-sync; > frame_sync > - Event data for event V4L2_EVENT_FRAME_SYNC. > + Event data for event V4L2_EVENT_FRAME_SYNC > + . > > > > @@ -226,22 +227,6 @@ > > > > - > - struct <structname>v4l2_event_frame_sync</structname> > - > - &cs-str; > - > - > - __u32 > - frame_sequence > - > - The sequence number of the frame being received. > - > - > - > - > -
> - > > Changes > > @@ -270,6 +255,41 @@ > > >
> + > + > + struct <structname>v4l2_event_frame_sync</structname> > + > + &cs-str; > + > + > + __u32 > + frame_sequence > + > + The sequence number of the frame being received. > + > + > + > + > +
> + > + > + Frame sync event IDs > + > + &cs-def; > + > + > + V4L2_EVENT_FRAME_SYNC_START > + 0x0000 > + Frame sync event delivered at frame start. > + > + > + V4L2_EVENT_FRAME_SYNC_END > + 0x0001 > + Frame sync event delivered at frame end. > + > + > + > +
> > > &return-value; > diff --git a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml > index 5c70b61..aaa6fe1 100644 > --- a/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml > +++ b/Documentation/DocBook/media/v4l/vidioc-subscribe-event.xml > @@ -143,15 +143,22 @@ > 4 > > Triggered immediately when the reception of a > - frame has begun. This event has a > + frame has begun or ended. This event has a > &v4l2-event-frame-sync; associated with it. > > If the hardware needs to be stopped in the case of a > buffer underrun it might not be able to generate this event. > In such cases the frame_sequence > field in &v4l2-event-frame-sync; will not be incremented. This > - causes two consecutive frame sequence numbers to have n times > + causes two consecutive frame sync events to have n times > frame interval in between them. > + > + The id field must be set to > + either V4L2_EVENT_FRAME_SYNC_START or > + V4L2_EVENT_FRAME_SYNC_END which signify > + the start of frame and end of frame, respectively. See > + for more > + information. > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 437f1b0..133957a 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1760,6 +1760,9 @@ struct v4l2_event_ctrl { > __s32 default_value; > }; > > +#define V4L2_EVENT_FRAME_SYNC_START 0 > +#define V4L2_EVENT_FRAME_SYNC_END 1 > + > struct v4l2_event_frame_sync { > __u32 frame_sequence; > }; >