diff mbox

[RFC,4/4] v4l: Tell user space we're using monotonic timestamps

Message ID 1351102583-682-4-git-send-email-sakari.ailus@iki.fi (mailing list archive)
State New, archived
Headers show

Commit Message

Sakari Ailus Oct. 24, 2012, 6:16 p.m. UTC
Set buffer timestamp flags for videobuf, videobuf2 and drivers that use
neither.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/pci/meye/meye.c                 |    4 ++--
 drivers/media/pci/zoran/zoran_driver.c        |    2 +-
 drivers/media/platform/omap3isp/ispqueue.c    |    1 +
 drivers/media/platform/vino.c                 |    3 +++
 drivers/media/usb/cpia2/cpia2_v4l.c           |    5 ++++-
 drivers/media/usb/sn9c102/sn9c102_core.c      |    2 +-
 drivers/media/usb/stkwebcam/stk-webcam.c      |    1 +
 drivers/media/usb/usbvision/usbvision-video.c |    5 +++--
 drivers/media/v4l2-core/videobuf-core.c       |    2 +-
 drivers/media/v4l2-core/videobuf2-core.c      |   10 ++++++----
 10 files changed, 23 insertions(+), 12 deletions(-)

Comments

Laurent Pinchart Nov. 4, 2012, 12:07 p.m. UTC | #1
Hi Sakari,

Thanks for the patch.

On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
> Set buffer timestamp flags for videobuf, videobuf2 and drivers that use
> neither.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/media/pci/meye/meye.c                 |    4 ++--
>  drivers/media/pci/zoran/zoran_driver.c        |    2 +-
>  drivers/media/platform/omap3isp/ispqueue.c    |    1 +
>  drivers/media/platform/vino.c                 |    3 +++
>  drivers/media/usb/cpia2/cpia2_v4l.c           |    5 ++++-
>  drivers/media/usb/sn9c102/sn9c102_core.c      |    2 +-
>  drivers/media/usb/stkwebcam/stk-webcam.c      |    1 +
>  drivers/media/usb/usbvision/usbvision-video.c |    5 +++--
>  drivers/media/v4l2-core/videobuf-core.c       |    2 +-
>  drivers/media/v4l2-core/videobuf2-core.c      |   10 ++++++----
>  10 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
> index 86713e0..d3e5ca0 100644
> --- a/drivers/media/pci/meye/meye.c
> +++ b/drivers/media/pci/meye/meye.c
> @@ -1426,7 +1426,7 @@ static int vidioc_querybuf(struct file *file, void
> *fh, struct v4l2_buffer *buf) return -EINVAL;
> 
>  	buf->bytesused = meye.grab_buffer[index].size;
> -	buf->flags = V4L2_BUF_FLAG_MAPPED;
> +	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> 
>  	if (meye.grab_buffer[index].state == MEYE_BUF_USING)
>  		buf->flags |= V4L2_BUF_FLAG_QUEUED;
> @@ -1499,7 +1499,7 @@ static int vidioc_dqbuf(struct file *file, void *fh,
> struct v4l2_buffer *buf)
> 
>  	buf->index = reqnr;
>  	buf->bytesused = meye.grab_buffer[reqnr].size;
> -	buf->flags = V4L2_BUF_FLAG_MAPPED;
> +	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	buf->field = V4L2_FIELD_NONE;
>  	buf->timestamp = meye.grab_buffer[reqnr].timestamp;
>  	buf->sequence = meye.grab_buffer[reqnr].sequence;
> diff --git a/drivers/media/pci/zoran/zoran_driver.c
> b/drivers/media/pci/zoran/zoran_driver.c index 53f12c7..33521a4 100644
> --- a/drivers/media/pci/zoran/zoran_driver.c
> +++ b/drivers/media/pci/zoran/zoran_driver.c
> @@ -1334,7 +1334,7 @@ static int zoran_v4l2_buffer_status(struct zoran_fh
> *fh, struct zoran *zr = fh->zr;
>  	unsigned long flags;
> 
> -	buf->flags = V4L2_BUF_FLAG_MAPPED;
> +	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> 
>  	switch (fh->map_mode) {
>  	case ZORAN_MAP_MODE_RAW:
> diff --git a/drivers/media/platform/omap3isp/ispqueue.c
> b/drivers/media/platform/omap3isp/ispqueue.c index 15bf3ea..6599963 100644
> --- a/drivers/media/platform/omap3isp/ispqueue.c
> +++ b/drivers/media/platform/omap3isp/ispqueue.c
> @@ -674,6 +674,7 @@ static int isp_video_queue_alloc(struct isp_video_queue
> *queue, buf->vbuf.index = i;
>  		buf->vbuf.length = size;
>  		buf->vbuf.type = queue->type;
> +		buf->vbuf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  		buf->vbuf.field = V4L2_FIELD_NONE;
>  		buf->vbuf.memory = memory;
> 
> diff --git a/drivers/media/platform/vino.c b/drivers/media/platform/vino.c
> index 28350e7..eb5d6f9 100644
> --- a/drivers/media/platform/vino.c
> +++ b/drivers/media/platform/vino.c
> @@ -3410,6 +3410,9 @@ static void vino_v4l2_get_buffer_status(struct
> vino_channel_settings *vcs, if (fb->map_count > 0)
>  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> 
> +	b->flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK;
> +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +
>  	b->index = fb->id;
>  	b->memory = (vcs->fb_queue.type == VINO_MEMORY_MMAP) ?
>  		V4L2_MEMORY_MMAP : V4L2_MEMORY_USERPTR;
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c
> b/drivers/media/usb/cpia2/cpia2_v4l.c index aeb9d22..d5d42b6 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -825,6 +825,8 @@ static int cpia2_querybuf(struct file *file, void *fh,
> struct v4l2_buffer *buf) else
>  		buf->flags = 0;
> 
> +	buf->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> +
>  	switch (cam->buffers[buf->index].status) {
>  	case FRAME_EMPTY:
>  	case FRAME_ERROR:
> @@ -943,7 +945,8 @@ static int cpia2_dqbuf(struct file *file, void *fh,
> struct v4l2_buffer *buf)
> 
>  	buf->index = frame;
>  	buf->bytesused = cam->buffers[buf->index].length;
> -	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE;
> +	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE
> +		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	buf->field = V4L2_FIELD_NONE;
>  	buf->timestamp = cam->buffers[buf->index].timestamp;
>  	buf->sequence = cam->buffers[buf->index].seq;
> diff --git a/drivers/media/usb/sn9c102/sn9c102_core.c
> b/drivers/media/usb/sn9c102/sn9c102_core.c index 843fadc..2e0e2ff 100644
> --- a/drivers/media/usb/sn9c102/sn9c102_core.c
> +++ b/drivers/media/usb/sn9c102/sn9c102_core.c
> @@ -173,7 +173,7 @@ sn9c102_request_buffers(struct sn9c102_device* cam, u32
> count, cam->frame[i].buf.sequence = 0;
>  		cam->frame[i].buf.field = V4L2_FIELD_NONE;
>  		cam->frame[i].buf.memory = V4L2_MEMORY_MMAP;
> -		cam->frame[i].buf.flags = 0;
> +		cam->frame[i].buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	}
> 
>  	return cam->nbuffers;
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c
> b/drivers/media/usb/stkwebcam/stk-webcam.c index c22a4d0..459ebc6 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -470,6 +470,7 @@ static int stk_setup_siobuf(struct stk_camera *dev, int
> index) buf->dev = dev;
>  	buf->v4lbuf.index = index;
>  	buf->v4lbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	buf->v4lbuf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	buf->v4lbuf.field = V4L2_FIELD_NONE;
>  	buf->v4lbuf.memory = V4L2_MEMORY_MMAP;
>  	buf->v4lbuf.m.offset = 2*index*buf->v4lbuf.length;
> diff --git a/drivers/media/usb/usbvision/usbvision-video.c
> b/drivers/media/usb/usbvision/usbvision-video.c index 5c36a57..c6bc8ce
> 100644
> --- a/drivers/media/usb/usbvision/usbvision-video.c
> +++ b/drivers/media/usb/usbvision/usbvision-video.c
> @@ -761,7 +761,7 @@ static int vidioc_querybuf(struct file *file,
>  	if (vb->index >= usbvision->num_frames)
>  		return -EINVAL;
>  	/* Updating the corresponding frame state */
> -	vb->flags = 0;
> +	vb->flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	frame = &usbvision->frame[vb->index];
>  	if (frame->grabstate >= frame_state_ready)
>  		vb->flags |= V4L2_BUF_FLAG_QUEUED;
> @@ -843,7 +843,8 @@ static int vidioc_dqbuf(struct file *file, void *priv,
> struct v4l2_buffer *vb) vb->memory = V4L2_MEMORY_MMAP;
>  	vb->flags = V4L2_BUF_FLAG_MAPPED |
>  		V4L2_BUF_FLAG_QUEUED |
> -		V4L2_BUF_FLAG_DONE;
> +		V4L2_BUF_FLAG_DONE |
> +		V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	vb->index = f->index;
>  	vb->sequence = f->sequence;
>  	vb->timestamp = f->timestamp;
> diff --git a/drivers/media/v4l2-core/videobuf-core.c
> b/drivers/media/v4l2-core/videobuf-core.c index bf7a326..e98db7e 100644
> --- a/drivers/media/v4l2-core/videobuf-core.c
> +++ b/drivers/media/v4l2-core/videobuf-core.c
> @@ -337,7 +337,7 @@ static void videobuf_status(struct videobuf_queue *q,
> struct v4l2_buffer *b, break;
>  	}
> 
> -	b->flags    = 0;
> +	b->flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
>  	if (vb->map)
>  		b->flags |= V4L2_BUF_FLAG_MAPPED;
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..19a5866 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -40,9 +40,10 @@ module_param(debug, int, 0644);
>  #define call_qop(q, op, args...)					\
>  	(((q)->ops->op) ? ((q)->ops->op(args)) : 0)
> 
> -#define V4L2_BUFFER_STATE_FLAGS	(V4L2_BUF_FLAG_MAPPED |
> V4L2_BUF_FLAG_QUEUED | \ +#define
> V4L2_BUFFER_MASK_FLAGS	(V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
> V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
> -				 V4L2_BUF_FLAG_PREPARED)
> +				 V4L2_BUF_FLAG_PREPARED | \
> +				 V4L2_BUF_FLAG_TIMESTAMP_MASK)
> 
>  /**
>   * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
> @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
> struct v4l2_buffer *b) /*
>  	 * Clear any buffer state related flags.
>  	 */
> -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;

That's an issue. Drivers that use videobuf2 would always be restricted to 
monotonic timestamps in the future, even if they provide support for a device-
specific clock.

Would it instead make sense to pass a v4l2_buffer pointer to 
v4l2_get_timestamp() and set the monotonic flag there ? Not all callers of 
v4l2_get_timestamp() might have a v4l2_buffer pointer though.

>  	switch (vb->state) {
>  	case VB2_BUF_STATE_QUEUED:
> @@ -863,7 +865,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb,
> const struct v4l2_buffer *b
> 
>  	vb->v4l2_buf.field = b->field;
>  	vb->v4l2_buf.timestamp = b->timestamp;
> -	vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_STATE_FLAGS;
> +	vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS;
>  }
> 
>  /**
Sakari Ailus Nov. 5, 2012, 2:04 p.m. UTC | #2
Hi Laurent,

On Sun, Nov 04, 2012 at 01:07:25PM +0100, Laurent Pinchart wrote:
> On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
...
> > @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb,
> > struct v4l2_buffer *b) /*
> >  	 * Clear any buffer state related flags.
> >  	 */
> > -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> > +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> > +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> 
> That's an issue. Drivers that use videobuf2 would always be restricted to 
> monotonic timestamps in the future, even if they provide support for a device-
> specific clock.
> 
> Would it instead make sense to pass a v4l2_buffer pointer to 
> v4l2_get_timestamp() and set the monotonic flag there ? Not all callers of 
> v4l2_get_timestamp() might have a v4l2_buffer pointer though.

For now, this patch assumes that all the VB2 users will use monotonic
timestamps only. Once we have a good use case for different kind of
timestamps and have agreed how to implement them, I was thinking of adding a
similar function to v4l2_get_timestamp() but which would be VB2-aware, with
one argument being the timestamp type. That function could then get the
timestamp and apply the relevant flags.

Do you think it'd be enough to support changeable timestamp type for drivers
using VB2 only?

Alternatively we could make v4l2_get_timestamp() v4l2_buffer-aware, and for
drivers that can't provide the buffer pointer we could just set the pointer
to NULL, and v4l2_get_timestamp() could ignore it. The driver would then be
responsible for setting the right flags to the buffer on its own. As far as
I understand, this is essentially what you proposed.

Kind regards,
Laurent Pinchart Nov. 8, 2012, 12:18 p.m. UTC | #3
Hi Sakari,

On Monday 05 November 2012 16:04:32 Sakari Ailus wrote:
> On Sun, Nov 04, 2012 at 01:07:25PM +0100, Laurent Pinchart wrote:
> > On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
> ...
> 
> > > @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer
> > > *vb,
> > > struct v4l2_buffer *b) /*
> > > 
> > >  	 * Clear any buffer state related flags.
> > >  	 */
> > > 
> > > -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> > > +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> > > +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > 
> > That's an issue. Drivers that use videobuf2 would always be restricted to
> > monotonic timestamps in the future, even if they provide support for a
> > device- specific clock.
> > 
> > Would it instead make sense to pass a v4l2_buffer pointer to
> > v4l2_get_timestamp() and set the monotonic flag there ? Not all callers of
> > v4l2_get_timestamp() might have a v4l2_buffer pointer though.
> 
> For now, this patch assumes that all the VB2 users will use monotonic
> timestamps only. Once we have a good use case for different kind of
> timestamps and have agreed how to implement them, I was thinking of adding a
> similar function to v4l2_get_timestamp() but which would be VB2-aware, with
> one argument being the timestamp type. That function could then get the
> timestamp and apply the relevant flags.
> 
> Do you think it'd be enough to support changeable timestamp type for drivers
> using VB2 only?

Given that there's no reason to use anything else than VB2 in V4L2 drivers, I 
don't see any problem there.

How would that work in practice ? You won't be able to override the timestamp 
type flag unconditionally in __fill_v4l2_buffer() anymore.

> Alternatively we could make v4l2_get_timestamp() v4l2_buffer-aware, and for
> drivers that can't provide the buffer pointer we could just set the pointer
> to NULL, and v4l2_get_timestamp() could ignore it. The driver would then be
> responsible for setting the right flags to the buffer on its own. As far as
> I understand, this is essentially what you proposed.
Sakari Ailus Nov. 8, 2012, 10:33 p.m. UTC | #4
Hi Laurent,

On Thu, Nov 08, 2012 at 01:18:15PM +0100, Laurent Pinchart wrote:
> On Monday 05 November 2012 16:04:32 Sakari Ailus wrote:
> > On Sun, Nov 04, 2012 at 01:07:25PM +0100, Laurent Pinchart wrote:
> > > On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
> > ...
> > 
> > > > @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer
> > > > *vb,
> > > > struct v4l2_buffer *b) /*
> > > > 
> > > >  	 * Clear any buffer state related flags.
> > > >  	 */
> > > > 
> > > > -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> > > > +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> > > > +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > 
> > > That's an issue. Drivers that use videobuf2 would always be restricted to
> > > monotonic timestamps in the future, even if they provide support for a
> > > device- specific clock.
> > > 
> > > Would it instead make sense to pass a v4l2_buffer pointer to
> > > v4l2_get_timestamp() and set the monotonic flag there ? Not all callers of
> > > v4l2_get_timestamp() might have a v4l2_buffer pointer though.
> > 
> > For now, this patch assumes that all the VB2 users will use monotonic
> > timestamps only. Once we have a good use case for different kind of
> > timestamps and have agreed how to implement them, I was thinking of adding a
> > similar function to v4l2_get_timestamp() but which would be VB2-aware, with
> > one argument being the timestamp type. That function could then get the
> > timestamp and apply the relevant flags.
> > 
> > Do you think it'd be enough to support changeable timestamp type for drivers
> > using VB2 only?
> 
> Given that there's no reason to use anything else than VB2 in V4L2 drivers, I 
> don't see any problem there.
> 
> How would that work in practice ? You won't be able to override the timestamp 
> type flag unconditionally in __fill_v4l2_buffer() anymore.

The vb2 already stores struct v4l2_buffer, but unfortunately driver's
queue_setup() is called before alloctaing buffer objects, or after if less
buffers can be allocated that way.

The information could be stored in the buffer queue itself. That'd likely
make it the easies for the drivers: otherwise drivers need to be involed
e.g. in querybuf.

What do you think?

Kind regards,
Laurent Pinchart Nov. 12, 2012, 12:17 p.m. UTC | #5
Hi Sakari,

On Friday 09 November 2012 00:33:40 Sakari Ailus wrote:
> On Thu, Nov 08, 2012 at 01:18:15PM +0100, Laurent Pinchart wrote:
> > On Monday 05 November 2012 16:04:32 Sakari Ailus wrote:
> > > On Sun, Nov 04, 2012 at 01:07:25PM +0100, Laurent Pinchart wrote:
> > > > On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
> > > ...
> > > 
> > > > > @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer
> > > > > *vb,
> > > > > struct v4l2_buffer *b) /*
> > > > > 
> > > > >  	 * Clear any buffer state related flags.
> > > > >  	 */
> > > > > 
> > > > > -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> > > > > +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> > > > > +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > 
> > > > That's an issue. Drivers that use videobuf2 would always be restricted
> > > > to monotonic timestamps in the future, even if they provide support
> > > > for a device- specific clock.
> > > > 
> > > > Would it instead make sense to pass a v4l2_buffer pointer to
> > > > v4l2_get_timestamp() and set the monotonic flag there ? Not all
> > > > callers of v4l2_get_timestamp() might have a v4l2_buffer pointer
> > > > though.
> > > 
> > > For now, this patch assumes that all the VB2 users will use monotonic
> > > timestamps only. Once we have a good use case for different kind of
> > > timestamps and have agreed how to implement them, I was thinking of
> > > adding a similar function to v4l2_get_timestamp() but which would be
> > > VB2-aware, with one argument being the timestamp type. That function
> > > could then get the timestamp and apply the relevant flags.
> > > 
> > > Do you think it'd be enough to support changeable timestamp type for
> > > drivers using VB2 only?
> > 
> > Given that there's no reason to use anything else than VB2 in V4L2
> > drivers, I don't see any problem there.
> > 
> > How would that work in practice ? You won't be able to override the
> > timestamp type flag unconditionally in __fill_v4l2_buffer() anymore.
> 
> The vb2 already stores struct v4l2_buffer, but unfortunately driver's
> queue_setup() is called before alloctaing buffer objects, or after if less
> buffers can be allocated that way.
> 
> The information could be stored in the buffer queue itself. That'd likely
> make it the easies for the drivers: otherwise drivers need to be involed
> e.g. in querybuf.
> 
> What do you think?

I don't foresee any need for per-buffer timestamps for now, so I would be fine 
with a per-queue flag.
Sakari Ailus Nov. 12, 2012, 3:20 p.m. UTC | #6
On Mon, Nov 12, 2012 at 01:17:31PM +0100, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Friday 09 November 2012 00:33:40 Sakari Ailus wrote:
> > On Thu, Nov 08, 2012 at 01:18:15PM +0100, Laurent Pinchart wrote:
> > > On Monday 05 November 2012 16:04:32 Sakari Ailus wrote:
> > > > On Sun, Nov 04, 2012 at 01:07:25PM +0100, Laurent Pinchart wrote:
> > > > > On Wednesday 24 October 2012 21:16:23 Sakari Ailus wrote:
> > > > ...
> > > > 
> > > > > > @@ -367,7 +368,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer
> > > > > > *vb,
> > > > > > struct v4l2_buffer *b) /*
> > > > > > 
> > > > > >  	 * Clear any buffer state related flags.
> > > > > >  	 */
> > > > > > 
> > > > > > -	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
> > > > > > +	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
> > > > > > +	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> > > > > 
> > > > > That's an issue. Drivers that use videobuf2 would always be restricted
> > > > > to monotonic timestamps in the future, even if they provide support
> > > > > for a device- specific clock.
> > > > > 
> > > > > Would it instead make sense to pass a v4l2_buffer pointer to
> > > > > v4l2_get_timestamp() and set the monotonic flag there ? Not all
> > > > > callers of v4l2_get_timestamp() might have a v4l2_buffer pointer
> > > > > though.
> > > > 
> > > > For now, this patch assumes that all the VB2 users will use monotonic
> > > > timestamps only. Once we have a good use case for different kind of
> > > > timestamps and have agreed how to implement them, I was thinking of
> > > > adding a similar function to v4l2_get_timestamp() but which would be
> > > > VB2-aware, with one argument being the timestamp type. That function
> > > > could then get the timestamp and apply the relevant flags.
> > > > 
> > > > Do you think it'd be enough to support changeable timestamp type for
> > > > drivers using VB2 only?
> > > 
> > > Given that there's no reason to use anything else than VB2 in V4L2
> > > drivers, I don't see any problem there.
> > > 
> > > How would that work in practice ? You won't be able to override the
> > > timestamp type flag unconditionally in __fill_v4l2_buffer() anymore.
> > 
> > The vb2 already stores struct v4l2_buffer, but unfortunately driver's
> > queue_setup() is called before alloctaing buffer objects, or after if less
> > buffers can be allocated that way.
> > 
> > The information could be stored in the buffer queue itself. That'd likely
> > make it the easies for the drivers: otherwise drivers need to be involed
> > e.g. in querybuf.
> > 
> > What do you think?
> 
> I don't foresee any need for per-buffer timestamps for now, so I would be fine 
> with a per-queue flag.

Should I take that as Acked-by to this patch? :-)
diff mbox

Patch

diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
index 86713e0..d3e5ca0 100644
--- a/drivers/media/pci/meye/meye.c
+++ b/drivers/media/pci/meye/meye.c
@@ -1426,7 +1426,7 @@  static int vidioc_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		return -EINVAL;
 
 	buf->bytesused = meye.grab_buffer[index].size;
-	buf->flags = V4L2_BUF_FLAG_MAPPED;
+	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	if (meye.grab_buffer[index].state == MEYE_BUF_USING)
 		buf->flags |= V4L2_BUF_FLAG_QUEUED;
@@ -1499,7 +1499,7 @@  static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 
 	buf->index = reqnr;
 	buf->bytesused = meye.grab_buffer[reqnr].size;
-	buf->flags = V4L2_BUF_FLAG_MAPPED;
+	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
 	buf->timestamp = meye.grab_buffer[reqnr].timestamp;
 	buf->sequence = meye.grab_buffer[reqnr].sequence;
diff --git a/drivers/media/pci/zoran/zoran_driver.c b/drivers/media/pci/zoran/zoran_driver.c
index 53f12c7..33521a4 100644
--- a/drivers/media/pci/zoran/zoran_driver.c
+++ b/drivers/media/pci/zoran/zoran_driver.c
@@ -1334,7 +1334,7 @@  static int zoran_v4l2_buffer_status(struct zoran_fh *fh,
 	struct zoran *zr = fh->zr;
 	unsigned long flags;
 
-	buf->flags = V4L2_BUF_FLAG_MAPPED;
+	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	switch (fh->map_mode) {
 	case ZORAN_MAP_MODE_RAW:
diff --git a/drivers/media/platform/omap3isp/ispqueue.c b/drivers/media/platform/omap3isp/ispqueue.c
index 15bf3ea..6599963 100644
--- a/drivers/media/platform/omap3isp/ispqueue.c
+++ b/drivers/media/platform/omap3isp/ispqueue.c
@@ -674,6 +674,7 @@  static int isp_video_queue_alloc(struct isp_video_queue *queue,
 		buf->vbuf.index = i;
 		buf->vbuf.length = size;
 		buf->vbuf.type = queue->type;
+		buf->vbuf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		buf->vbuf.field = V4L2_FIELD_NONE;
 		buf->vbuf.memory = memory;
 
diff --git a/drivers/media/platform/vino.c b/drivers/media/platform/vino.c
index 28350e7..eb5d6f9 100644
--- a/drivers/media/platform/vino.c
+++ b/drivers/media/platform/vino.c
@@ -3410,6 +3410,9 @@  static void vino_v4l2_get_buffer_status(struct vino_channel_settings *vcs,
 	if (fb->map_count > 0)
 		b->flags |= V4L2_BUF_FLAG_MAPPED;
 
+	b->flags &= ~V4L2_BUF_FLAG_TIMESTAMP_MASK;
+	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+
 	b->index = fb->id;
 	b->memory = (vcs->fb_queue.type == VINO_MEMORY_MMAP) ?
 		V4L2_MEMORY_MMAP : V4L2_MEMORY_USERPTR;
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index aeb9d22..d5d42b6 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -825,6 +825,8 @@  static int cpia2_querybuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	else
 		buf->flags = 0;
 
+	buf->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+
 	switch (cam->buffers[buf->index].status) {
 	case FRAME_EMPTY:
 	case FRAME_ERROR:
@@ -943,7 +945,8 @@  static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 
 	buf->index = frame;
 	buf->bytesused = cam->buffers[buf->index].length;
-	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE;
+	buf->flags = V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_DONE
+		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->field = V4L2_FIELD_NONE;
 	buf->timestamp = cam->buffers[buf->index].timestamp;
 	buf->sequence = cam->buffers[buf->index].seq;
diff --git a/drivers/media/usb/sn9c102/sn9c102_core.c b/drivers/media/usb/sn9c102/sn9c102_core.c
index 843fadc..2e0e2ff 100644
--- a/drivers/media/usb/sn9c102/sn9c102_core.c
+++ b/drivers/media/usb/sn9c102/sn9c102_core.c
@@ -173,7 +173,7 @@  sn9c102_request_buffers(struct sn9c102_device* cam, u32 count,
 		cam->frame[i].buf.sequence = 0;
 		cam->frame[i].buf.field = V4L2_FIELD_NONE;
 		cam->frame[i].buf.memory = V4L2_MEMORY_MMAP;
-		cam->frame[i].buf.flags = 0;
+		cam->frame[i].buf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	}
 
 	return cam->nbuffers;
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index c22a4d0..459ebc6 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -470,6 +470,7 @@  static int stk_setup_siobuf(struct stk_camera *dev, int index)
 	buf->dev = dev;
 	buf->v4lbuf.index = index;
 	buf->v4lbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	buf->v4lbuf.flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	buf->v4lbuf.field = V4L2_FIELD_NONE;
 	buf->v4lbuf.memory = V4L2_MEMORY_MMAP;
 	buf->v4lbuf.m.offset = 2*index*buf->v4lbuf.length;
diff --git a/drivers/media/usb/usbvision/usbvision-video.c b/drivers/media/usb/usbvision/usbvision-video.c
index 5c36a57..c6bc8ce 100644
--- a/drivers/media/usb/usbvision/usbvision-video.c
+++ b/drivers/media/usb/usbvision/usbvision-video.c
@@ -761,7 +761,7 @@  static int vidioc_querybuf(struct file *file,
 	if (vb->index >= usbvision->num_frames)
 		return -EINVAL;
 	/* Updating the corresponding frame state */
-	vb->flags = 0;
+	vb->flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	frame = &usbvision->frame[vb->index];
 	if (frame->grabstate >= frame_state_ready)
 		vb->flags |= V4L2_BUF_FLAG_QUEUED;
@@ -843,7 +843,8 @@  static int vidioc_dqbuf(struct file *file, void *priv, struct v4l2_buffer *vb)
 	vb->memory = V4L2_MEMORY_MMAP;
 	vb->flags = V4L2_BUF_FLAG_MAPPED |
 		V4L2_BUF_FLAG_QUEUED |
-		V4L2_BUF_FLAG_DONE;
+		V4L2_BUF_FLAG_DONE |
+		V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	vb->index = f->index;
 	vb->sequence = f->sequence;
 	vb->timestamp = f->timestamp;
diff --git a/drivers/media/v4l2-core/videobuf-core.c b/drivers/media/v4l2-core/videobuf-core.c
index bf7a326..e98db7e 100644
--- a/drivers/media/v4l2-core/videobuf-core.c
+++ b/drivers/media/v4l2-core/videobuf-core.c
@@ -337,7 +337,7 @@  static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 		break;
 	}
 
-	b->flags    = 0;
+	b->flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 	if (vb->map)
 		b->flags |= V4L2_BUF_FLAG_MAPPED;
 
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 432df11..19a5866 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -40,9 +40,10 @@  module_param(debug, int, 0644);
 #define call_qop(q, op, args...)					\
 	(((q)->ops->op) ? ((q)->ops->op(args)) : 0)
 
-#define V4L2_BUFFER_STATE_FLAGS	(V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
+#define V4L2_BUFFER_MASK_FLAGS	(V4L2_BUF_FLAG_MAPPED | V4L2_BUF_FLAG_QUEUED | \
 				 V4L2_BUF_FLAG_DONE | V4L2_BUF_FLAG_ERROR | \
-				 V4L2_BUF_FLAG_PREPARED)
+				 V4L2_BUF_FLAG_PREPARED | \
+				 V4L2_BUF_FLAG_TIMESTAMP_MASK)
 
 /**
  * __vb2_buf_mem_alloc() - allocate video memory for the given buffer
@@ -367,7 +368,8 @@  static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
 	/*
 	 * Clear any buffer state related flags.
 	 */
-	b->flags &= ~V4L2_BUFFER_STATE_FLAGS;
+	b->flags &= ~V4L2_BUFFER_MASK_FLAGS;
+	b->flags |= V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 
 	switch (vb->state) {
 	case VB2_BUF_STATE_QUEUED:
@@ -863,7 +865,7 @@  static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b
 
 	vb->v4l2_buf.field = b->field;
 	vb->v4l2_buf.timestamp = b->timestamp;
-	vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_STATE_FLAGS;
+	vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS;
 }
 
 /**