This is my second attempt to convert subsystem-wide code in v4l for y2038 changes, removing uses of time_t in common files and adding support for user space that defines time_t as 64 bit.
Based on the initial feedback from Hans Verkuil, I've changed the ioctl handling to remain 100% compatible with existing headers, which also makes it more likely that existing source code can compile without changes.
This comes at a noticeable expense of adding complexity to the v4l2-ioctl.c file, as we now have to handle two versions of each ioctl command that passes a time_t in its arguments.
I have not added support for the new binary layout of v4l2_timeval to v4l2-compat-ioctl32 yet, that is something I can do when the basic approach has been agreed on.
Arnd
Arnd Bergmann (9): [media] dvb: use ktime_t for internal timeout [media] dvb: remove unused systime() function [media] dvb: don't use 'time_t' in event ioctl [media] exynos4-is: use monotonic timestamps as advertized [media] make VIDIOC_DQEVENT work with 64-bit time_t [media] use v4l2_get_timestamp where possible [media] v4l2: introduce v4l2_timeval [media] handle 64-bit time_t in v4l2_buffer [media] omap3isp: support 64-bit version of omap3isp_stat_data
drivers/media/dvb-core/demux.h | 2 +- drivers/media/dvb-core/dmxdev.c | 2 +- drivers/media/dvb-core/dvb_demux.c | 17 +- drivers/media/dvb-core/dvb_demux.h | 4 +- drivers/media/dvb-core/dvb_net.c | 2 +- drivers/media/dvb-frontends/dibx000_common.c | 10 - drivers/media/dvb-frontends/dibx000_common.h | 2 - drivers/media/pci/bt8xx/bttv-driver.c | 7 +- drivers/media/pci/cx18/cx18-mailbox.c | 2 +- drivers/media/pci/meye/meye.h | 2 +- drivers/media/pci/zoran/zoran.h | 2 +- drivers/media/platform/coda/coda.h | 2 +- drivers/media/platform/exynos4-is/fimc-capture.c | 8 +- drivers/media/platform/exynos4-is/fimc-lite.c | 7 +- drivers/media/platform/omap/omap_vout.c | 4 +- drivers/media/platform/omap3isp/isph3a_aewb.c | 2 + drivers/media/platform/omap3isp/isph3a_af.c | 2 + drivers/media/platform/omap3isp/isphist.c | 2 + drivers/media/platform/omap3isp/ispstat.c | 20 +- drivers/media/platform/omap3isp/ispstat.h | 4 +- drivers/media/platform/s3c-camif/camif-capture.c | 8 +- drivers/media/platform/vim2m.c | 2 +- drivers/media/platform/vivid/vivid-ctrls.c | 2 +- drivers/media/usb/cpia2/cpia2.h | 2 +- drivers/media/usb/cpia2/cpia2_v4l.c | 2 +- drivers/media/usb/gspca/gspca.c | 6 +- drivers/media/usb/usbvision/usbvision.h | 2 +- drivers/media/v4l2-core/v4l2-common.c | 6 +- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 35 ---- drivers/media/v4l2-core/v4l2-dev.c | 1 + drivers/media/v4l2-core/v4l2-event.c | 35 +++- drivers/media/v4l2-core/v4l2-ioctl.c | 227 ++++++++++++++++++++--- drivers/media/v4l2-core/v4l2-subdev.c | 6 + drivers/staging/media/omap4iss/iss_video.c | 5 +- include/media/v4l2-common.h | 2 +- include/media/v4l2-event.h | 2 + include/media/videobuf-core.h | 2 +- include/trace/events/v4l2.h | 12 +- include/uapi/linux/dvb/video.h | 3 +- include/uapi/linux/omap3isp.h | 19 ++ include/uapi/linux/videodev2.h | 78 ++++++++ 41 files changed, 415 insertions(+), 145 deletions(-)
The dvb demuxer code uses a 'struct timespec' to pass a timeout as absolute time. This will cause problems on 32-bit architectures in 2038 when time_t overflows, and it is racy with a concurrent settimeofday() call.
This patch changes the code to use ktime_get() instead, using the monotonic time base to avoid both the race and the overflow.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/dvb-core/demux.h | 2 +- drivers/media/dvb-core/dmxdev.c | 2 +- drivers/media/dvb-core/dvb_demux.c | 17 ++++++----------- drivers/media/dvb-core/dvb_demux.h | 4 ++-- drivers/media/dvb-core/dvb_net.c | 2 +- 5 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/media/dvb-core/demux.h b/drivers/media/dvb-core/demux.h index 833191bcd810..d8e2b1213bef 100644 --- a/drivers/media/dvb-core/demux.h +++ b/drivers/media/dvb-core/demux.h @@ -92,7 +92,7 @@ struct dmx_ts_feed { int type, enum dmx_ts_pes pes_type, size_t circular_buffer_size, - struct timespec timeout); + ktime_t timeout); int (*start_filtering) (struct dmx_ts_feed* feed); int (*stop_filtering) (struct dmx_ts_feed* feed); }; diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c index d0e3f9d85f34..0d20b379eeec 100644 --- a/drivers/media/dvb-core/dmxdev.c +++ b/drivers/media/dvb-core/dmxdev.c @@ -558,7 +558,7 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev, struct dmxdev_filter *filter, struct dmxdev_feed *feed) { - struct timespec timeout = { 0 }; + ktime_t timeout = ktime_set(0, 0); struct dmx_pes_filter_params *para = &filter->params.pes; dmx_output_t otype; int ret; diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c index 6c7ff0cdcd32..f9f676112d30 100644 --- a/drivers/media/dvb-core/dvb_demux.c +++ b/drivers/media/dvb-core/dvb_demux.c @@ -399,28 +399,23 @@ static void dvb_dmx_swfilter_packet(struct dvb_demux *demux, const u8 *buf) int dvr_done = 0;
if (dvb_demux_speedcheck) { - struct timespec cur_time, delta_time; + ktime_t cur_time; u64 speed_bytes, speed_timedelta;
demux->speed_pkts_cnt++;
/* show speed every SPEED_PKTS_INTERVAL packets */ if (!(demux->speed_pkts_cnt % SPEED_PKTS_INTERVAL)) { - cur_time = current_kernel_time(); + cur_time = ktime_get();
- if (demux->speed_last_time.tv_sec != 0 && - demux->speed_last_time.tv_nsec != 0) { - delta_time = timespec_sub(cur_time, - demux->speed_last_time); + if (ktime_to_ns(demux->speed_last_time) != 0) { speed_bytes = (u64)demux->speed_pkts_cnt * 188 * 8; /* convert to 1024 basis */ speed_bytes = 1000 * div64_u64(speed_bytes, 1024); - speed_timedelta = - (u64)timespec_to_ns(&delta_time); - speed_timedelta = div64_u64(speed_timedelta, - 1000000); /* nsec -> usec */ + speed_timedelta = ktime_ms_delta(cur_time, + demux->speed_last_time); printk(KERN_INFO "TS speed %llu Kbits/sec \n", div64_u64(speed_bytes, speed_timedelta)); @@ -667,7 +662,7 @@ out:
static int dmx_ts_feed_set(struct dmx_ts_feed *ts_feed, u16 pid, int ts_type, enum dmx_ts_pes pes_type, - size_t circular_buffer_size, struct timespec timeout) + size_t circular_buffer_size, ktime_t timeout) { struct dvb_demux_feed *feed = (struct dvb_demux_feed *)ts_feed; struct dvb_demux *demux = feed->demux; diff --git a/drivers/media/dvb-core/dvb_demux.h b/drivers/media/dvb-core/dvb_demux.h index ae7fc33c3231..5ed3cab4ad28 100644 --- a/drivers/media/dvb-core/dvb_demux.h +++ b/drivers/media/dvb-core/dvb_demux.h @@ -83,7 +83,7 @@ struct dvb_demux_feed { u8 *buffer; int buffer_size;
- struct timespec timeout; + ktime_t timeout; struct dvb_demux_filter *filter;
int ts_type; @@ -134,7 +134,7 @@ struct dvb_demux {
uint8_t *cnt_storage; /* for TS continuity check */
- struct timespec speed_last_time; /* for TS speed check */ + ktime_t speed_last_time; /* for TS speed check */ uint32_t speed_pkts_cnt; /* for TS speed check */ };
diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c index b81e026edab3..df3ba15c007e 100644 --- a/drivers/media/dvb-core/dvb_net.c +++ b/drivers/media/dvb-core/dvb_net.c @@ -998,7 +998,7 @@ static int dvb_net_feed_start(struct net_device *dev) netdev_dbg(dev, "start filtering\n"); priv->secfeed->start_filtering(priv->secfeed); } else if (priv->feedtype == DVB_NET_FEEDTYPE_ULE) { - struct timespec timeout = { 0, 10000000 }; // 10 msec + ktime_t timeout = ns_to_ktime(10 * NSEC_PER_MSEC);
/* we have payloads encapsulated in TS */ netdev_dbg(dev, "alloc tsfeed\n");
The systime function uses struct timespec, which we want to stop using in the kernel because it overflows in 2038. Fortunately, this use in dibx000_common is in a function that is never called, so we can just remove it.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/dvb-frontends/dibx000_common.c | 10 ---------- drivers/media/dvb-frontends/dibx000_common.h | 2 -- 2 files changed, 12 deletions(-)
diff --git a/drivers/media/dvb-frontends/dibx000_common.c b/drivers/media/dvb-frontends/dibx000_common.c index 43be7238311e..79cb007b401f 100644 --- a/drivers/media/dvb-frontends/dibx000_common.c +++ b/drivers/media/dvb-frontends/dibx000_common.c @@ -500,16 +500,6 @@ void dibx000_exit_i2c_master(struct dibx000_i2c_master *mst) } EXPORT_SYMBOL(dibx000_exit_i2c_master);
- -u32 systime(void) -{ - struct timespec t; - - t = current_kernel_time(); - return (t.tv_sec * 10000) + (t.tv_nsec / 100000); -} -EXPORT_SYMBOL(systime); - MODULE_AUTHOR("Patrick Boettcher pboettcher@dibcom.fr"); MODULE_DESCRIPTION("Common function the DiBcom demodulator family"); MODULE_LICENSE("GPL"); diff --git a/drivers/media/dvb-frontends/dibx000_common.h b/drivers/media/dvb-frontends/dibx000_common.h index b538e0555c95..61f4152f24ee 100644 --- a/drivers/media/dvb-frontends/dibx000_common.h +++ b/drivers/media/dvb-frontends/dibx000_common.h @@ -47,8 +47,6 @@ extern void dibx000_exit_i2c_master(struct dibx000_i2c_master *mst); extern void dibx000_reset_i2c_master(struct dibx000_i2c_master *mst); extern int dibx000_i2c_set_speed(struct i2c_adapter *i2c_adap, u16 speed);
-extern u32 systime(void); - #define BAND_LBAND 0x01 #define BAND_UHF 0x02 #define BAND_VHF 0x04
'struct video_event' is used for the VIDEO_GET_EVENT ioctl, implemented by drivers/media/pci/ivtv/ivtv-ioctl.c and drivers/media/pci/ttpci/av7110_av.c. The structure contains a 'time_t', which will be redefined in the future to be 64-bit wide, causing an incompatible ABI change for this ioctl.
As it turns out, neither of the drivers currently sets the timestamp field, and it is presumably useless anyway because of the limited resolutions (no sub-second times). This means we can simply change the structure definition to use a 'long' instead of 'time_t' and remain compatible with all existing user space binaries when time_t gets changed.
If anybody ever starts using this field, they have to make sure not to use 1970 based seconds in there, as those overflow in 2038.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/uapi/linux/dvb/video.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/dvb/video.h b/include/uapi/linux/dvb/video.h index d3d14a59d2d5..6c7f9298d7c2 100644 --- a/include/uapi/linux/dvb/video.h +++ b/include/uapi/linux/dvb/video.h @@ -135,7 +135,8 @@ struct video_event { #define VIDEO_EVENT_FRAME_RATE_CHANGED 2 #define VIDEO_EVENT_DECODER_STOPPED 3 #define VIDEO_EVENT_VSYNC 4 - __kernel_time_t timestamp; + /* unused, make sure to use atomic time for y2038 if it ever gets used */ + long timestamp; union { video_size_t size; unsigned int frame_rate; /* in frames per 1000sec */
The exynos4 fimc capture driver claims to use monotonic timestamps but calls ktime_get_real_ts(). This is both an incorrect API use, and a bad idea because of the y2038 problem and the fact that the wall clock time is not reliable for timestamps across suspend or settimeofday().
This changes the driver to use the normal v4l2_get_timestamp() function like all other drivers.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/platform/exynos4-is/fimc-capture.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c index cfebf292e15a..776ea6d78d03 100644 --- a/drivers/media/platform/exynos4-is/fimc-capture.c +++ b/drivers/media/platform/exynos4-is/fimc-capture.c @@ -183,8 +183,6 @@ void fimc_capture_irq_handler(struct fimc_dev *fimc, int deq_buf) struct v4l2_subdev *csis = p->subdevs[IDX_CSIS]; struct fimc_frame *f = &cap->ctx->d_frame; struct fimc_vid_buffer *v_buf; - struct timeval *tv; - struct timespec ts;
if (test_and_clear_bit(ST_CAPT_SHUT, &fimc->state)) { wake_up(&fimc->irq_queue); @@ -193,13 +191,9 @@ void fimc_capture_irq_handler(struct fimc_dev *fimc, int deq_buf)
if (!list_empty(&cap->active_buf_q) && test_bit(ST_CAPT_RUN, &fimc->state) && deq_buf) { - ktime_get_real_ts(&ts); - v_buf = fimc_active_queue_pop(cap);
- tv = &v_buf->vb.v4l2_buf.timestamp; - tv->tv_sec = ts.tv_sec; - tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC; + v4l2_get_timestamp(&v_buf->vb.v4l2_buf.timestamp); v_buf->vb.v4l2_buf.sequence = cap->frame_count++;
vb2_buffer_done(&v_buf->vb, VB2_BUF_STATE_DONE);
The v4l2 event queue uses a 'struct timespec' to pass monotonic timestamps. This is not a problem by itself, but it breaks when user space redefines timespec to use 'long long' on 32-bit systems.
This is the second approach on fixing the problem, by changing the kernel to internally use 64-bit members for the timestamps in struct v4l2_event, and letting user space use either version.
Unfortunately, we cannot use timespec64 here, because that does not have a well-defined ABI yet, and differs from the 64-bit version of 'timespec'. Using a pair of __s64 members makes sure the structure is well-defined and contains no padding that might leak kernel stack data.
We now need the handling for VIDIOC_DQEVENT32 on both 32-bit and 64-bit architectures, so the handling for that is moved from v4l2-compat-ioctl32.c to v4l2-ioctl.c and v4l2-subdev.c.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 35 ------------------ drivers/media/v4l2-core/v4l2-event.c | 35 +++++++++++++++--- drivers/media/v4l2-core/v4l2-ioctl.c | 53 ++++++++++++++++++++------- drivers/media/v4l2-core/v4l2-subdev.c | 6 +++ include/media/v4l2-event.h | 2 + include/uapi/linux/videodev2.h | 31 ++++++++++++++++ 6 files changed, 107 insertions(+), 55 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index af635430524e..9ffe7302206e 100644 --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c @@ -735,32 +735,6 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls *kp, struct v4l2_ext return 0; }
-struct v4l2_event32 { - __u32 type; - union { - __u8 data[64]; - } u; - __u32 pending; - __u32 sequence; - struct compat_timespec timestamp; - __u32 id; - __u32 reserved[8]; -}; - -static int put_v4l2_event32(struct v4l2_event *kp, struct v4l2_event32 __user *up) -{ - if (!access_ok(VERIFY_WRITE, up, sizeof(struct v4l2_event32)) || - put_user(kp->type, &up->type) || - copy_to_user(&up->u, &kp->u, sizeof(kp->u)) || - put_user(kp->pending, &up->pending) || - put_user(kp->sequence, &up->sequence) || - compat_put_timespec(&kp->timestamp, &up->timestamp) || - put_user(kp->id, &up->id) || - copy_to_user(up->reserved, kp->reserved, 8 * sizeof(__u32))) - return -EFAULT; - return 0; -} - struct v4l2_edid32 { __u32 pad; __u32 start_block; @@ -814,7 +788,6 @@ static int put_v4l2_edid32(struct v4l2_edid *kp, struct v4l2_edid32 __user *up) #define VIDIOC_G_EXT_CTRLS32 _IOWR('V', 71, struct v4l2_ext_controls32) #define VIDIOC_S_EXT_CTRLS32 _IOWR('V', 72, struct v4l2_ext_controls32) #define VIDIOC_TRY_EXT_CTRLS32 _IOWR('V', 73, struct v4l2_ext_controls32) -#define VIDIOC_DQEVENT32 _IOR ('V', 89, struct v4l2_event32) #define VIDIOC_CREATE_BUFS32 _IOWR('V', 92, struct v4l2_create_buffers32) #define VIDIOC_PREPARE_BUF32 _IOWR('V', 93, struct v4l2_buffer32)
@@ -860,7 +833,6 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar case VIDIOC_G_EXT_CTRLS32: cmd = VIDIOC_G_EXT_CTRLS; break; case VIDIOC_S_EXT_CTRLS32: cmd = VIDIOC_S_EXT_CTRLS; break; case VIDIOC_TRY_EXT_CTRLS32: cmd = VIDIOC_TRY_EXT_CTRLS; break; - case VIDIOC_DQEVENT32: cmd = VIDIOC_DQEVENT; break; case VIDIOC_OVERLAY32: cmd = VIDIOC_OVERLAY; break; case VIDIOC_STREAMON32: cmd = VIDIOC_STREAMON; break; case VIDIOC_STREAMOFF32: cmd = VIDIOC_STREAMOFF; break; @@ -940,9 +912,6 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar err = get_v4l2_ext_controls32(&karg.v2ecs, up); compatible_arg = 0; break; - case VIDIOC_DQEVENT: - compatible_arg = 0; - break; } if (err) return err; @@ -983,10 +952,6 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar err = put_v4l2_framebuffer32(&karg.v2fb, up); break;
- case VIDIOC_DQEVENT: - err = put_v4l2_event32(&karg.v2ev, up); - break; - case VIDIOC_G_EDID: case VIDIOC_S_EDID: err = put_v4l2_edid32(&karg.v2edid, up); diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c index 8d3171c6bee8..149342490e91 100644 --- a/drivers/media/v4l2-core/v4l2-event.c +++ b/drivers/media/v4l2-core/v4l2-event.c @@ -92,6 +92,28 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, } EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
+int v4l2_event_dequeue32(struct v4l2_fh *fh, struct v4l2_event32 *event32, + int nonblocking) +{ + struct v4l2_event event64; + int ret; + + ret = v4l2_event_dequeue(fh, &event64, nonblocking); + if (ret) + return ret; + + /* only the timestamp differs, so use memcpy to copy everything else */ + memcpy(event32, &event64, offsetof(struct v4l2_event32, timestamp)); + event32->timestamp.tv_sec = (long)event64.timestamp.tv_sec; + event32->timestamp.tv_nsec = (long)event64.timestamp.tv_nsec; + memcpy(&event32->id, &event64.id, + sizeof(event32->id) + sizeof(event32->reserved)); + + return ret; +} +EXPORT_SYMBOL_GPL(v4l2_event_dequeue32); + + /* Caller must hold fh->vdev->fh_lock! */ static struct v4l2_subscribed_event *v4l2_event_subscribed( struct v4l2_fh *fh, u32 type, u32 id) @@ -108,7 +130,7 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed( }
static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev, - const struct timespec *ts) + const struct timespec64 *ts) { struct v4l2_subscribed_event *sev; struct v4l2_kevent *kev; @@ -156,7 +178,8 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e if (copy_payload) kev->event.u = ev->u; kev->event.id = ev->id; - kev->event.timestamp = *ts; + kev->event.timestamp.tv_sec = ts->tv_sec; + kev->event.timestamp.tv_nsec = ts->tv_nsec; kev->event.sequence = fh->sequence; sev->in_use++; list_add_tail(&kev->list, &fh->available); @@ -170,12 +193,12 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev) { struct v4l2_fh *fh; unsigned long flags; - struct timespec timestamp; + struct timespec64 timestamp;
if (vdev == NULL) return;
- ktime_get_ts(×tamp); + ktime_get_ts64(×tamp);
spin_lock_irqsave(&vdev->fh_lock, flags);
@@ -189,9 +212,9 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue); void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev) { unsigned long flags; - struct timespec timestamp; + struct timespec64 timestamp;
- ktime_get_ts(×tamp); + ktime_get_ts64(×tamp);
spin_lock_irqsave(&fh->vdev->fh_lock, flags); __v4l2_event_queue_fh(fh, ev, ×tamp); diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 4a384fc765b8..7aab18dd2ca5 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -797,22 +797,18 @@ static void v4l_print_frmivalenum(const void *arg, bool write_only) } }
-static void v4l_print_event(const void *arg, bool write_only) +static void v4l_print_event_data(const void *arg, bool write_only, u32 type) { - const struct v4l2_event *p = arg; - const struct v4l2_event_ctrl *c; + const struct v4l2_event_vsync *vsync = arg; + const struct v4l2_event_ctrl *c = arg; + const struct v4l2_event_frame_sync *frame_sync = arg;
- pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, " - "timestamp=%lu.%9.9lu\n", - p->type, p->pending, p->sequence, p->id, - p->timestamp.tv_sec, p->timestamp.tv_nsec); - switch (p->type) { + switch (type) { case V4L2_EVENT_VSYNC: printk(KERN_DEBUG "field=%s\n", - prt_names(p->u.vsync.field, v4l2_field_names)); + prt_names(vsync->field, v4l2_field_names)); break; case V4L2_EVENT_CTRL: - c = &p->u.ctrl; printk(KERN_DEBUG "changes=0x%x, type=%u, ", c->changes, c->type); if (c->type == V4L2_CTRL_TYPE_INTEGER64) @@ -825,12 +821,34 @@ static void v4l_print_event(const void *arg, bool write_only) c->step, c->default_value); break; case V4L2_EVENT_FRAME_SYNC: - pr_cont("frame_sequence=%u\n", - p->u.frame_sync.frame_sequence); + pr_cont("frame_sequence=%u\n", frame_sync->frame_sequence); break; } }
+static void v4l_print_event32(const void *arg, bool write_only) +{ + const struct v4l2_event32 *p = arg; + pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, " + "timestamp=%d.%9.9u\n", + p->type, p->pending, p->sequence, p->id, + p->timestamp.tv_sec, p->timestamp.tv_nsec); + + return v4l_print_event_data(&p->u, write_only, p->type); +} + +static void v4l_print_event64(const void *arg, bool write_only) +{ + const struct v4l2_event *p = arg; + + pr_cont("type=0x%x, pending=%u, sequence=%u, id=%u, " + "timestamp=%lld.%9.9llu\n", + p->type, p->pending, p->sequence, p->id, + p->timestamp.tv_sec, p->timestamp.tv_nsec); + + return v4l_print_event_data(&p->u, write_only, p->type); +} + static void v4l_print_event_subscription(const void *arg, bool write_only) { const struct v4l2_event_subscription *p = arg; @@ -2235,12 +2253,18 @@ static int v4l_dbg_g_chip_info(const struct v4l2_ioctl_ops *ops, #endif }
-static int v4l_dqevent(const struct v4l2_ioctl_ops *ops, +static int v4l_dqevent64(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { return v4l2_event_dequeue(fh, arg, file->f_flags & O_NONBLOCK); }
+static int v4l_dqevent32(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *arg) +{ + return v4l2_event_dequeue32(fh, arg, file->f_flags & O_NONBLOCK); +} + static int v4l_subscribe_event(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { @@ -2449,7 +2473,8 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_S_HW_FREQ_SEEK, v4l_s_hw_freq_seek, v4l_print_hw_freq_seek, INFO_FL_PRIO), IOCTL_INFO_STD(VIDIOC_S_DV_TIMINGS, vidioc_s_dv_timings, v4l_print_dv_timings, INFO_FL_PRIO), IOCTL_INFO_STD(VIDIOC_G_DV_TIMINGS, vidioc_g_dv_timings, v4l_print_dv_timings, 0), - IOCTL_INFO_FNC(VIDIOC_DQEVENT, v4l_dqevent, v4l_print_event, 0), + IOCTL_INFO_FNC(VIDIOC_DQEVENT, v4l_dqevent64, v4l_print_event64, 0), + IOCTL_INFO_FNC(VIDIOC_DQEVENT32, v4l_dqevent32, v4l_print_event32, 0), IOCTL_INFO_FNC(VIDIOC_SUBSCRIBE_EVENT, v4l_subscribe_event, v4l_print_event_subscription, 0), IOCTL_INFO_FNC(VIDIOC_UNSUBSCRIBE_EVENT, v4l_unsubscribe_event, v4l_print_event_subscription, 0), IOCTL_INFO_FNC(VIDIOC_CREATE_BUFS, v4l_create_bufs, v4l_print_create_buffers, INFO_FL_PRIO | INFO_FL_QUEUE), diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 83615b8fb46a..9b0cde143c6c 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -211,6 +211,12 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) case VIDIOC_TRY_EXT_CTRLS: return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
+ case VIDIOC_DQEVENT32: + if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) + return -ENOIOCTLCMD; + + return v4l2_event_dequeue32(vfh, arg, file->f_flags & O_NONBLOCK); + case VIDIOC_DQEVENT: if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)) return -ENOIOCTLCMD; diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h index 9792f906423b..5e8ce27a0671 100644 --- a/include/media/v4l2-event.h +++ b/include/media/v4l2-event.h @@ -124,6 +124,8 @@ struct v4l2_subscribed_event {
int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event, int nonblocking); +int v4l2_event_dequeue32(struct v4l2_fh *fh, struct v4l2_event32 *event, + int nonblocking); void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev); void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev); int v4l2_event_pending(struct v4l2_fh *fh); diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 3228fbebcd63..3e2c497c31fd 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -2082,10 +2082,40 @@ struct v4l2_event { } u; __u32 pending; __u32 sequence; +#ifdef __KERNEL__ + struct { + __s64 tv_sec; + __s64 tv_nsec; + } timestamp; +#else struct timespec timestamp; +#endif + __u32 id; + __u32 reserved[8]; +}; + +#ifdef __KERNEL__ +/* + * User space will see either 64-bit or 32-bit time_t, which changes + * the v4l2_event layout. Both are y2038 safe because the timestamps + * are in monotonic time, but the kernel has to handle both cases. + */ +struct v4l2_event32 { + __u32 type; + union { + __u8 data[64]; + } u; + __u32 pending; + __u32 sequence; + struct { + __s32 tv_sec; + __s32 tv_nsec; + } timestamp; + __u32 id; __u32 reserved[8]; }; +#endif
#define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0) #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK (1 << 1) @@ -2237,6 +2267,7 @@ struct v4l2_create_buffers { #define VIDIOC_S_DV_TIMINGS _IOWR('V', 87, struct v4l2_dv_timings) #define VIDIOC_G_DV_TIMINGS _IOWR('V', 88, struct v4l2_dv_timings) #define VIDIOC_DQEVENT _IOR('V', 89, struct v4l2_event) +#define VIDIOC_DQEVENT32 _IOR('V', 89, struct v4l2_event32) #define VIDIOC_SUBSCRIBE_EVENT _IOW('V', 90, struct v4l2_event_subscription) #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 91, struct v4l2_event_subscription)
This is a preparation for a change to the type of v4l2 timestamps. v4l2_get_timestamp() is a helper function that reads the monotonic time and stores it into a 'struct timeval'. Multiple drivers implement the same thing themselves for historic reasons.
Changing them all to use v4l2_get_timestamp() is more consistent and reduces the amount of code duplication, and most importantly simplifies the following changes.
If desired, this patch can easily be split up into one patch per driver.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/pci/bt8xx/bttv-driver.c | 5 +---- drivers/media/pci/cx18/cx18-mailbox.c | 2 +- drivers/media/platform/exynos4-is/fimc-lite.c | 7 +------ drivers/media/platform/omap3isp/ispstat.c | 5 ++--- drivers/media/platform/omap3isp/ispstat.h | 2 +- drivers/media/platform/s3c-camif/camif-capture.c | 8 +------- drivers/media/usb/gspca/gspca.c | 4 ++-- drivers/media/v4l2-core/v4l2-dev.c | 1 + drivers/staging/media/omap4iss/iss_video.c | 5 +---- 9 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c index 3632958f2158..15a4ebc2844d 100644 --- a/drivers/media/pci/bt8xx/bttv-driver.c +++ b/drivers/media/pci/bt8xx/bttv-driver.c @@ -3625,13 +3625,10 @@ static void bttv_irq_wakeup_vbi(struct bttv *btv, struct bttv_buffer *wakeup, unsigned int state) { - struct timeval ts; - if (NULL == wakeup) return;
- v4l2_get_timestamp(&ts); - wakeup->vb.ts = ts; + v4l2_get_timestamp(&wakeup->vb.ts); wakeup->vb.field_count = btv->field_count; wakeup->vb.state = state; wake_up(&wakeup->vb.done); diff --git a/drivers/media/pci/cx18/cx18-mailbox.c b/drivers/media/pci/cx18/cx18-mailbox.c index eabf00c6351b..1f8aa9a749a1 100644 --- a/drivers/media/pci/cx18/cx18-mailbox.c +++ b/drivers/media/pci/cx18/cx18-mailbox.c @@ -202,7 +202,7 @@ static void cx18_mdl_send_to_videobuf(struct cx18_stream *s, }
if (dispatch) { - vb_buf->vb.ts = ktime_to_timeval(ktime_get()); + v4l2_get_timestamp(&vb_buf->vb.ts); list_del(&vb_buf->vb.queue); vb_buf->vb.state = VIDEOBUF_DONE; wake_up(&vb_buf->vb.done); diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c index ca6261a86a5f..459bc65b545d 100644 --- a/drivers/media/platform/exynos4-is/fimc-lite.c +++ b/drivers/media/platform/exynos4-is/fimc-lite.c @@ -254,8 +254,6 @@ static irqreturn_t flite_irq_handler(int irq, void *priv) struct fimc_lite *fimc = priv; struct flite_buffer *vbuf; unsigned long flags; - struct timeval *tv; - struct timespec ts; u32 intsrc;
spin_lock_irqsave(&fimc->slock, flags); @@ -294,10 +292,7 @@ static irqreturn_t flite_irq_handler(int irq, void *priv) test_bit(ST_FLITE_RUN, &fimc->state) && !list_empty(&fimc->active_buf_q)) { vbuf = fimc_lite_active_queue_pop(fimc); - ktime_get_ts(&ts); - tv = &vbuf->vb.v4l2_buf.timestamp; - tv->tv_sec = ts.tv_sec; - tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC; + v4l2_get_timestamp(&vbuf->vb.v4l2_buf.timestamp); vbuf->vb.v4l2_buf.sequence = fimc->frame_count++; flite_hw_mask_dma_buffer(fimc, vbuf->index); vb2_buffer_done(&vbuf->vb, VB2_BUF_STATE_DONE); diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index 20434e83e801..94d4c295d3d0 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -235,7 +235,7 @@ static int isp_stat_buf_queue(struct ispstat *stat) if (!stat->active_buf) return STAT_NO_BUF;
- ktime_get_ts(&stat->active_buf->ts); + v4l2_get_timestamp(&stat->active_buf->ts);
stat->active_buf->buf_size = stat->buf_size; if (isp_stat_buf_check_magic(stat, stat->active_buf)) { @@ -496,8 +496,7 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return PTR_ERR(buf); }
- data->ts.tv_sec = buf->ts.tv_sec; - data->ts.tv_usec = buf->ts.tv_nsec / NSEC_PER_USEC; + data->ts = buf->ts; data->config_counter = buf->config_counter; data->frame_number = buf->frame_number; data->buf_size = buf->buf_size; diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index b79380d83fcf..6d9b0244f320 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -39,7 +39,7 @@ struct ispstat_buffer { struct sg_table sgt; void *virt_addr; dma_addr_t dma_addr; - struct timespec ts; + struct timeval ts; u32 buf_size; u32 frame_number; u16 config_counter; diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c index 76e6289a5612..edf70725ecf3 100644 --- a/drivers/media/platform/s3c-camif/camif-capture.c +++ b/drivers/media/platform/s3c-camif/camif-capture.c @@ -328,23 +328,17 @@ irqreturn_t s3c_camif_irq_handler(int irq, void *priv) !list_empty(&vp->active_buf_q)) { unsigned int index; struct camif_buffer *vbuf; - struct timeval *tv; - struct timespec ts; /* * Get previous DMA write buffer index: * 0 => DMA buffer 0, 2; * 1 => DMA buffer 1, 3. */ index = (CISTATUS_FRAMECNT(status) + 2) & 1; - - ktime_get_ts(&ts); vbuf = camif_active_queue_peek(vp, index);
if (!WARN_ON(vbuf == NULL)) { /* Dequeue a filled buffer */ - tv = &vbuf->vb.v4l2_buf.timestamp; - tv->tv_sec = ts.tv_sec; - tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC; + v4l2_get_timestamp(&vbuf->vb.v4l2_buf.timestamp); vbuf->vb.v4l2_buf.sequence = vp->frame_sequence++; vb2_buffer_done(&vbuf->vb, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index e54cee856a80..af5cd8213e8b 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -436,7 +436,7 @@ void gspca_frame_add(struct gspca_dev *gspca_dev, } j = gspca_dev->fr_queue[i]; frame = &gspca_dev->frame[j]; - frame->v4l2_buf.timestamp = ktime_to_timeval(ktime_get()); + v4l2_get_timestamp(&frame->v4l2_buf.timestamp); frame->v4l2_buf.sequence = gspca_dev->sequence++; gspca_dev->image = frame->data; gspca_dev->image_len = 0; @@ -1909,7 +1909,7 @@ static ssize_t dev_read(struct file *file, char __user *data, }
/* get a frame */ - timestamp = ktime_to_timeval(ktime_get()); + v4l2_get_timestamp(×tamp); timestamp.tv_sec--; n = 2; for (;;) { diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index 71a1b93b0790..8faa50dc5b19 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -565,6 +565,7 @@ static void determine_valid_ioctls(struct video_device *vdev) #endif /* yes, really vidioc_subscribe_event */ SET_VALID_IOCTL(ops, VIDIOC_DQEVENT, vidioc_subscribe_event); + SET_VALID_IOCTL(ops, VIDIOC_DQEVENT32, vidioc_subscribe_event); SET_VALID_IOCTL(ops, VIDIOC_SUBSCRIBE_EVENT, vidioc_subscribe_event); SET_VALID_IOCTL(ops, VIDIOC_UNSUBSCRIBE_EVENT, vidioc_unsubscribe_event); if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator) diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c index 40405d8710a6..485a90ce12df 100644 --- a/drivers/staging/media/omap4iss/iss_video.c +++ b/drivers/staging/media/omap4iss/iss_video.c @@ -420,7 +420,6 @@ struct iss_buffer *omap4iss_video_buffer_next(struct iss_video *video) enum iss_pipeline_state state; struct iss_buffer *buf; unsigned long flags; - struct timespec ts;
spin_lock_irqsave(&video->qlock, flags); if (WARN_ON(list_empty(&video->dmaqueue))) { @@ -433,9 +432,7 @@ struct iss_buffer *omap4iss_video_buffer_next(struct iss_video *video) list_del(&buf->list); spin_unlock_irqrestore(&video->qlock, flags);
- ktime_get_ts(&ts); - buf->vb.v4l2_buf.timestamp.tv_sec = ts.tv_sec; - buf->vb.v4l2_buf.timestamp.tv_usec = ts.tv_nsec / NSEC_PER_USEC; + v4l2_get_timestamp(&buf->vb.v4l2_buf.timestamp);
/* Do frame number propagation only if this is the output video node. * Frame number either comes from the CSI receivers or it gets
The v4l2 API uses a 'struct timeval' to communicate time stamps to user space. This is broken on 32-bit architectures as soon as we have a C library that defines time_t as 64 bit, which then changes the structure layout of struct v4l2_buffer.
Since existing user space source code relies on the type to be 'struct timeva' and we want to preserve compile-time compatibility when moving to a new libc, we cannot make user-visible changes to the header file.
In this patch, we change the type of the timestamp to 'struct v4l2_timeval' as a preparation for a follow-up that adds API compatiblity for both 32-bit and 64-bit time_t. This patch should have no impact on generated code in either user space or kernel.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/pci/bt8xx/bttv-driver.c | 2 +- drivers/media/pci/meye/meye.h | 2 +- drivers/media/pci/zoran/zoran.h | 2 +- drivers/media/platform/coda/coda.h | 2 +- drivers/media/platform/omap/omap_vout.c | 4 ++-- drivers/media/platform/omap3isp/ispstat.c | 3 ++- drivers/media/platform/omap3isp/ispstat.h | 2 +- drivers/media/platform/vim2m.c | 2 +- drivers/media/platform/vivid/vivid-ctrls.c | 2 +- drivers/media/usb/cpia2/cpia2.h | 2 +- drivers/media/usb/cpia2/cpia2_v4l.c | 2 +- drivers/media/usb/gspca/gspca.c | 2 +- drivers/media/usb/usbvision/usbvision.h | 2 +- drivers/media/v4l2-core/v4l2-common.c | 6 +++--- include/media/v4l2-common.h | 2 +- include/media/videobuf-core.h | 2 +- include/trace/events/v4l2.h | 12 ++++++++++-- include/uapi/linux/videodev2.h | 17 +++++++++++++++++ 18 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c index 15a4ebc2844d..b0ed8d799c14 100644 --- a/drivers/media/pci/bt8xx/bttv-driver.c +++ b/drivers/media/pci/bt8xx/bttv-driver.c @@ -3585,7 +3585,7 @@ static void bttv_irq_wakeup_video(struct bttv *btv, struct bttv_buffer_set *wakeup, struct bttv_buffer_set *curr, unsigned int state) { - struct timeval ts; + struct v4l2_timeval ts;
v4l2_get_timestamp(&ts);
diff --git a/drivers/media/pci/meye/meye.h b/drivers/media/pci/meye/meye.h index 751be5e533c7..a06aa5ba9abc 100644 --- a/drivers/media/pci/meye/meye.h +++ b/drivers/media/pci/meye/meye.h @@ -281,7 +281,7 @@ struct meye_grab_buffer { int state; /* state of buffer */ unsigned long size; /* size of jpg frame */ - struct timeval timestamp; /* timestamp */ + struct v4l2_timeval timestamp; /* timestamp */ unsigned long sequence; /* sequence number */ };
diff --git a/drivers/media/pci/zoran/zoran.h b/drivers/media/pci/zoran/zoran.h index 4e7db8939c2b..9a9f782cede9 100644 --- a/drivers/media/pci/zoran/zoran.h +++ b/drivers/media/pci/zoran/zoran.h @@ -39,7 +39,7 @@ struct zoran_sync { unsigned long frame; /* number of buffer that has been free'd */ unsigned long length; /* number of code bytes in buffer (capture only) */ unsigned long seq; /* frame sequence number */ - struct timeval timestamp; /* timestamp */ + struct v4l2_timeval timestamp; /* timestamp */ };
diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h index 59b2af9c7749..fa1e15a757cd 100644 --- a/drivers/media/platform/coda/coda.h +++ b/drivers/media/platform/coda/coda.h @@ -138,7 +138,7 @@ struct coda_buffer_meta { struct list_head list; u32 sequence; struct v4l2_timecode timecode; - struct timeval timestamp; + struct v4l2_timeval timestamp; u32 start; u32 end; }; diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c index 70c28d19ea04..974a238a86fe 100644 --- a/drivers/media/platform/omap/omap_vout.c +++ b/drivers/media/platform/omap/omap_vout.c @@ -513,7 +513,7 @@ static int omapvid_apply_changes(struct omap_vout_device *vout) }
static int omapvid_handle_interlace_display(struct omap_vout_device *vout, - unsigned int irqstatus, struct timeval timevalue) + unsigned int irqstatus, struct v4l2_timeval timevalue) { u32 fid;
@@ -557,7 +557,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus) int ret, fid, mgr_id; u32 addr, irq; struct omap_overlay *ovl; - struct timeval timevalue; + struct v4l2_timeval timevalue; struct omapvideo_info *ovid; struct omap_dss_device *cur_display; struct omap_vout_device *vout = (struct omap_vout_device *)arg; diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index 94d4c295d3d0..fa96e330c563 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -496,7 +496,8 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return PTR_ERR(buf); }
- data->ts = buf->ts; + data->ts.tv_sec = buf->ts.tv_sec; + data->ts.tv_usec = buf->ts.tv_usec; data->config_counter = buf->config_counter; data->frame_number = buf->frame_number; data->buf_size = buf->buf_size; diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index 6d9b0244f320..7b4f136567a3 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -39,7 +39,7 @@ struct ispstat_buffer { struct sg_table sgt; void *virt_addr; dma_addr_t dma_addr; - struct timeval ts; + struct v4l2_timeval ts; u32 buf_size; u32 frame_number; u16 config_counter; diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 295fde5fdb75..df5daac6d099 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -235,7 +235,7 @@ static int device_process(struct vim2m_ctx *ctx, in_vb->v4l2_buf.sequence = q_data->sequence++; memcpy(&out_vb->v4l2_buf.timestamp, &in_vb->v4l2_buf.timestamp, - sizeof(struct timeval)); + sizeof(struct v4l2_timeval)); if (in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TIMECODE) memcpy(&out_vb->v4l2_buf.timecode, &in_vb->v4l2_buf.timecode, sizeof(struct v4l2_timecode)); diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c index 339c8b7e53c8..065f3d6c2409 100644 --- a/drivers/media/platform/vivid/vivid-ctrls.c +++ b/drivers/media/platform/vivid/vivid-ctrls.c @@ -935,7 +935,7 @@ static const struct v4l2_ctrl_config vivid_ctrl_has_scaler_out = { static int vivid_streaming_s_ctrl(struct v4l2_ctrl *ctrl) { struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, ctrl_hdl_streaming); - struct timeval tv; + struct v4l2_timeval tv;
switch (ctrl->id) { case VIVID_CID_DQBUF_ERROR: diff --git a/drivers/media/usb/cpia2/cpia2.h b/drivers/media/usb/cpia2/cpia2.h index cdef677d57ec..3e7c523784e7 100644 --- a/drivers/media/usb/cpia2/cpia2.h +++ b/drivers/media/usb/cpia2/cpia2.h @@ -354,7 +354,7 @@ struct cpia2_sbuf { };
struct framebuf { - struct timeval timestamp; + struct v4l2_timeval timestamp; unsigned long seq; int num; int length; diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c index 9caea8344547..ce50d7ef4da7 100644 --- a/drivers/media/usb/cpia2/cpia2_v4l.c +++ b/drivers/media/usb/cpia2/cpia2_v4l.c @@ -892,7 +892,7 @@ static int find_earliest_filled_buffer(struct camera_data *cam) found = i; } else { /* find which buffer is earlier */ - struct timeval *tv1, *tv2; + struct v4l2_timeval *tv1, *tv2; tv1 = &cam->buffers[i].timestamp; tv2 = &cam->buffers[found].timestamp; if(tv1->tv_sec < tv2->tv_sec || diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index af5cd8213e8b..c977937da9c4 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -1898,7 +1898,7 @@ static ssize_t dev_read(struct file *file, char __user *data, struct gspca_dev *gspca_dev = video_drvdata(file); struct gspca_frame *frame; struct v4l2_buffer v4l2_buf; - struct timeval timestamp; + struct v4l2_timeval timestamp; int n, ret, ret2;
PDEBUG(D_FRAM, "read (%zd)", count); diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h index 4f2e4fde38f2..512de31613df 100644 --- a/drivers/media/usb/usbvision/usbvision.h +++ b/drivers/media/usb/usbvision/usbvision.h @@ -320,7 +320,7 @@ struct usbvision_frame { long bytes_read; /* amount of scanlength that has been read from data */ struct usbvision_v4l2_format_st v4l2_format; /* format the user needs*/ int v4l2_linesize; /* bytes for one videoline*/ - struct timeval timestamp; + struct v4l2_timeval timestamp; int sequence; /* How many video frames we send to user */ };
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 5b808500e7e7..589fab615f21 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -396,11 +396,11 @@ const struct v4l2_frmsize_discrete *v4l2_find_nearest_format( } EXPORT_SYMBOL_GPL(v4l2_find_nearest_format);
-void v4l2_get_timestamp(struct timeval *tv) +void v4l2_get_timestamp(struct v4l2_timeval *tv) { - struct timespec ts; + struct timespec64 ts;
- ktime_get_ts(&ts); + ktime_get_ts64(&ts); tv->tv_sec = ts.tv_sec; tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC; } diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 1cc0c5ba16b3..6e77d889c3f8 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -187,6 +187,6 @@ const struct v4l2_frmsize_discrete *v4l2_find_nearest_format( const struct v4l2_discrete_probe *probe, s32 width, s32 height);
-void v4l2_get_timestamp(struct timeval *tv); +void v4l2_get_timestamp(struct v4l2_timeval *tv);
#endif /* V4L2_COMMON_H_ */ diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h index d760aa73ebbb..6381338a9588 100644 --- a/include/media/videobuf-core.h +++ b/include/media/videobuf-core.h @@ -80,7 +80,7 @@ struct videobuf_buffer { struct list_head queue; wait_queue_head_t done; unsigned int field_count; - struct timeval ts; + struct v4l2_timeval ts;
/* Memory type */ enum v4l2_memory memory; diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h index dbf017bfddd9..a88be48dc473 100644 --- a/include/trace/events/v4l2.h +++ b/include/trace/events/v4l2.h @@ -6,6 +6,14 @@
#include <linux/tracepoint.h>
+#ifndef v4l2_timeval_to_ns +#define v4l2_timeval_to_ns v4l2_timeval_to_ns +static inline u64 v4l2_timeval_to_ns(struct v4l2_timeval *tv) +{ + return (u64)tv->tv_sec * NSEC_PER_SEC + tv->tv_usec * NSEC_PER_USEC; +} +#endif + /* Enums require being exported to userspace, for user tool parsing */ #undef EM #undef EMe @@ -126,7 +134,7 @@ DECLARE_EVENT_CLASS(v4l2_event_class, __entry->bytesused = buf->bytesused; __entry->flags = buf->flags; __entry->field = buf->field; - __entry->timestamp = timeval_to_ns(&buf->timestamp); + __entry->timestamp = v4l2_timeval_to_ns(&buf->timestamp); __entry->timecode_type = buf->timecode.type; __entry->timecode_flags = buf->timecode.flags; __entry->timecode_frames = buf->timecode.frames; @@ -211,7 +219,7 @@ DECLARE_EVENT_CLASS(vb2_event_class, __entry->bytesused = vb->v4l2_planes[0].bytesused; __entry->flags = vb->v4l2_buf.flags; __entry->field = vb->v4l2_buf.field; - __entry->timestamp = timeval_to_ns(&vb->v4l2_buf.timestamp); + __entry->timestamp = v4l2_timeval_to_ns(&vb->v4l2_buf.timestamp); __entry->timecode_type = vb->v4l2_buf.timecode.type; __entry->timecode_flags = vb->v4l2_buf.timecode.flags; __entry->timecode_frames = vb->v4l2_buf.timecode.frames; diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 3e2c497c31fd..450b3249ba30 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -803,6 +803,19 @@ struct v4l2_plane { __u32 reserved[11]; };
+#ifdef __KERNEL__ +/* + * This is used for the in-kernel version of v4l2_buffer, as we are + * migrating away from using time_t based structures in the kernel. + * User space might see this defined either using 32-bit or 64-bit + * time_t, so we have to convert it when accessing user data. + */ +struct v4l2_timeval { + long tv_sec; + long tv_usec; +}; +#endif + /** * struct v4l2_buffer - video buffer info * @index: id number of the buffer @@ -839,7 +852,11 @@ struct v4l2_buffer { __u32 bytesused; __u32 flags; __u32 field; +#ifdef __KERNEL__ + struct v4l2_timeval timestamp; +#else struct timeval timestamp; +#endif struct v4l2_timecode timecode; __u32 sequence;
On 09/17/15 23:19, Arnd Bergmann wrote:
The v4l2 API uses a 'struct timeval' to communicate time stamps to user space. This is broken on 32-bit architectures as soon as we have a C library that defines time_t as 64 bit, which then changes the structure layout of struct v4l2_buffer.
Since existing user space source code relies on the type to be 'struct timeva' and we want to preserve compile-time compatibility when moving
s/timeva/timeval/
to a new libc, we cannot make user-visible changes to the header file.
In this patch, we change the type of the timestamp to 'struct v4l2_timeval'
Don't we need a kernel-wide timeval64? Rather than adding a v4l2-specific struct?
as a preparation for a follow-up that adds API compatiblity for both 32-bit and 64-bit time_t. This patch should have no impact on generated code in either user space or kernel.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/media/pci/bt8xx/bttv-driver.c | 2 +- drivers/media/pci/meye/meye.h | 2 +- drivers/media/pci/zoran/zoran.h | 2 +- drivers/media/platform/coda/coda.h | 2 +- drivers/media/platform/omap/omap_vout.c | 4 ++-- drivers/media/platform/omap3isp/ispstat.c | 3 ++- drivers/media/platform/omap3isp/ispstat.h | 2 +- drivers/media/platform/vim2m.c | 2 +- drivers/media/platform/vivid/vivid-ctrls.c | 2 +- drivers/media/usb/cpia2/cpia2.h | 2 +- drivers/media/usb/cpia2/cpia2_v4l.c | 2 +- drivers/media/usb/gspca/gspca.c | 2 +- drivers/media/usb/usbvision/usbvision.h | 2 +- drivers/media/v4l2-core/v4l2-common.c | 6 +++--- include/media/v4l2-common.h | 2 +- include/media/videobuf-core.h | 2 +- include/trace/events/v4l2.h | 12 ++++++++++-- include/uapi/linux/videodev2.h | 17 +++++++++++++++++ 18 files changed, 47 insertions(+), 21 deletions(-)
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c index 15a4ebc2844d..b0ed8d799c14 100644 --- a/drivers/media/pci/bt8xx/bttv-driver.c +++ b/drivers/media/pci/bt8xx/bttv-driver.c @@ -3585,7 +3585,7 @@ static void bttv_irq_wakeup_video(struct bttv *btv, struct bttv_buffer_set *wakeup, struct bttv_buffer_set *curr, unsigned int state) {
- struct timeval ts;
- struct v4l2_timeval ts;
v4l2_get_timestamp(&ts); diff --git a/drivers/media/pci/meye/meye.h b/drivers/media/pci/meye/meye.h index 751be5e533c7..a06aa5ba9abc 100644 --- a/drivers/media/pci/meye/meye.h +++ b/drivers/media/pci/meye/meye.h @@ -281,7 +281,7 @@ struct meye_grab_buffer { int state; /* state of buffer */ unsigned long size; /* size of jpg frame */
- struct timeval timestamp; /* timestamp */
- struct v4l2_timeval timestamp; /* timestamp */ unsigned long sequence; /* sequence number */
}; diff --git a/drivers/media/pci/zoran/zoran.h b/drivers/media/pci/zoran/zoran.h index 4e7db8939c2b..9a9f782cede9 100644 --- a/drivers/media/pci/zoran/zoran.h +++ b/drivers/media/pci/zoran/zoran.h @@ -39,7 +39,7 @@ struct zoran_sync { unsigned long frame; /* number of buffer that has been free'd */ unsigned long length; /* number of code bytes in buffer (capture only) */ unsigned long seq; /* frame sequence number */
- struct timeval timestamp; /* timestamp */
- struct v4l2_timeval timestamp; /* timestamp */
}; diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h index 59b2af9c7749..fa1e15a757cd 100644 --- a/drivers/media/platform/coda/coda.h +++ b/drivers/media/platform/coda/coda.h @@ -138,7 +138,7 @@ struct coda_buffer_meta { struct list_head list; u32 sequence; struct v4l2_timecode timecode;
- struct timeval timestamp;
- struct v4l2_timeval timestamp; u32 start; u32 end;
}; diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c index 70c28d19ea04..974a238a86fe 100644 --- a/drivers/media/platform/omap/omap_vout.c +++ b/drivers/media/platform/omap/omap_vout.c @@ -513,7 +513,7 @@ static int omapvid_apply_changes(struct omap_vout_device *vout) } static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
unsigned int irqstatus, struct timeval timevalue)
unsigned int irqstatus, struct v4l2_timeval timevalue)
{ u32 fid; @@ -557,7 +557,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus) int ret, fid, mgr_id; u32 addr, irq; struct omap_overlay *ovl;
- struct timeval timevalue;
- struct v4l2_timeval timevalue; struct omapvideo_info *ovid; struct omap_dss_device *cur_display; struct omap_vout_device *vout = (struct omap_vout_device *)arg;
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index 94d4c295d3d0..fa96e330c563 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -496,7 +496,8 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return PTR_ERR(buf); }
- data->ts = buf->ts;
- data->ts.tv_sec = buf->ts.tv_sec;
- data->ts.tv_usec = buf->ts.tv_usec; data->config_counter = buf->config_counter; data->frame_number = buf->frame_number; data->buf_size = buf->buf_size;
diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index 6d9b0244f320..7b4f136567a3 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -39,7 +39,7 @@ struct ispstat_buffer { struct sg_table sgt; void *virt_addr; dma_addr_t dma_addr;
- struct timeval ts;
- struct v4l2_timeval ts; u32 buf_size; u32 frame_number; u16 config_counter;
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 295fde5fdb75..df5daac6d099 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -235,7 +235,7 @@ static int device_process(struct vim2m_ctx *ctx, in_vb->v4l2_buf.sequence = q_data->sequence++; memcpy(&out_vb->v4l2_buf.timestamp, &in_vb->v4l2_buf.timestamp,
sizeof(struct timeval));
if (in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TIMECODE) memcpy(&out_vb->v4l2_buf.timecode, &in_vb->v4l2_buf.timecode, sizeof(struct v4l2_timecode));sizeof(struct v4l2_timeval));
See https://patchwork.linuxtv.org/patch/31405/
I'll merge that one for 4.4 very soon.
diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c index 339c8b7e53c8..065f3d6c2409 100644 --- a/drivers/media/platform/vivid/vivid-ctrls.c +++ b/drivers/media/platform/vivid/vivid-ctrls.c @@ -935,7 +935,7 @@ static const struct v4l2_ctrl_config vivid_ctrl_has_scaler_out = { static int vivid_streaming_s_ctrl(struct v4l2_ctrl *ctrl) { struct vivid_dev *dev = container_of(ctrl->handler, struct vivid_dev, ctrl_hdl_streaming);
- struct timeval tv;
- struct v4l2_timeval tv;
switch (ctrl->id) { case VIVID_CID_DQBUF_ERROR: diff --git a/drivers/media/usb/cpia2/cpia2.h b/drivers/media/usb/cpia2/cpia2.h index cdef677d57ec..3e7c523784e7 100644 --- a/drivers/media/usb/cpia2/cpia2.h +++ b/drivers/media/usb/cpia2/cpia2.h @@ -354,7 +354,7 @@ struct cpia2_sbuf { }; struct framebuf {
- struct timeval timestamp;
- struct v4l2_timeval timestamp; unsigned long seq; int num; int length;
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c index 9caea8344547..ce50d7ef4da7 100644 --- a/drivers/media/usb/cpia2/cpia2_v4l.c +++ b/drivers/media/usb/cpia2/cpia2_v4l.c @@ -892,7 +892,7 @@ static int find_earliest_filled_buffer(struct camera_data *cam) found = i; } else { /* find which buffer is earlier */
struct timeval *tv1, *tv2;
struct v4l2_timeval *tv1, *tv2; tv1 = &cam->buffers[i].timestamp; tv2 = &cam->buffers[found].timestamp; if(tv1->tv_sec < tv2->tv_sec ||
diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c index af5cd8213e8b..c977937da9c4 100644 --- a/drivers/media/usb/gspca/gspca.c +++ b/drivers/media/usb/gspca/gspca.c @@ -1898,7 +1898,7 @@ static ssize_t dev_read(struct file *file, char __user *data, struct gspca_dev *gspca_dev = video_drvdata(file); struct gspca_frame *frame; struct v4l2_buffer v4l2_buf;
- struct timeval timestamp;
- struct v4l2_timeval timestamp; int n, ret, ret2;
PDEBUG(D_FRAM, "read (%zd)", count); diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h index 4f2e4fde38f2..512de31613df 100644 --- a/drivers/media/usb/usbvision/usbvision.h +++ b/drivers/media/usb/usbvision/usbvision.h @@ -320,7 +320,7 @@ struct usbvision_frame { long bytes_read; /* amount of scanlength that has been read from data */ struct usbvision_v4l2_format_st v4l2_format; /* format the user needs*/ int v4l2_linesize; /* bytes for one videoline*/
- struct timeval timestamp;
- struct v4l2_timeval timestamp; int sequence; /* How many video frames we send to user */
}; diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index 5b808500e7e7..589fab615f21 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -396,11 +396,11 @@ const struct v4l2_frmsize_discrete *v4l2_find_nearest_format( } EXPORT_SYMBOL_GPL(v4l2_find_nearest_format); -void v4l2_get_timestamp(struct timeval *tv) +void v4l2_get_timestamp(struct v4l2_timeval *tv) {
- struct timespec ts;
- struct timespec64 ts;
- ktime_get_ts(&ts);
- ktime_get_ts64(&ts); tv->tv_sec = ts.tv_sec; tv->tv_usec = ts.tv_nsec / NSEC_PER_USEC;
} diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index 1cc0c5ba16b3..6e77d889c3f8 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -187,6 +187,6 @@ const struct v4l2_frmsize_discrete *v4l2_find_nearest_format( const struct v4l2_discrete_probe *probe, s32 width, s32 height); -void v4l2_get_timestamp(struct timeval *tv); +void v4l2_get_timestamp(struct v4l2_timeval *tv); #endif /* V4L2_COMMON_H_ */ diff --git a/include/media/videobuf-core.h b/include/media/videobuf-core.h index d760aa73ebbb..6381338a9588 100644 --- a/include/media/videobuf-core.h +++ b/include/media/videobuf-core.h @@ -80,7 +80,7 @@ struct videobuf_buffer { struct list_head queue; wait_queue_head_t done; unsigned int field_count;
- struct timeval ts;
- struct v4l2_timeval ts;
/* Memory type */ enum v4l2_memory memory; diff --git a/include/trace/events/v4l2.h b/include/trace/events/v4l2.h index dbf017bfddd9..a88be48dc473 100644 --- a/include/trace/events/v4l2.h +++ b/include/trace/events/v4l2.h @@ -6,6 +6,14 @@ #include <linux/tracepoint.h> +#ifndef v4l2_timeval_to_ns +#define v4l2_timeval_to_ns v4l2_timeval_to_ns +static inline u64 v4l2_timeval_to_ns(struct v4l2_timeval *tv) +{
- return (u64)tv->tv_sec * NSEC_PER_SEC + tv->tv_usec * NSEC_PER_USEC;
+} +#endif
/* Enums require being exported to userspace, for user tool parsing */ #undef EM #undef EMe @@ -126,7 +134,7 @@ DECLARE_EVENT_CLASS(v4l2_event_class, __entry->bytesused = buf->bytesused; __entry->flags = buf->flags; __entry->field = buf->field;
__entry->timestamp = timeval_to_ns(&buf->timestamp);
__entry->timecode_type = buf->timecode.type; __entry->timecode_flags = buf->timecode.flags; __entry->timecode_frames = buf->timecode.frames;__entry->timestamp = v4l2_timeval_to_ns(&buf->timestamp);
@@ -211,7 +219,7 @@ DECLARE_EVENT_CLASS(vb2_event_class, __entry->bytesused = vb->v4l2_planes[0].bytesused; __entry->flags = vb->v4l2_buf.flags; __entry->field = vb->v4l2_buf.field;
__entry->timestamp = timeval_to_ns(&vb->v4l2_buf.timestamp);
__entry->timecode_type = vb->v4l2_buf.timecode.type; __entry->timecode_flags = vb->v4l2_buf.timecode.flags; __entry->timecode_frames = vb->v4l2_buf.timecode.frames;__entry->timestamp = v4l2_timeval_to_ns(&vb->v4l2_buf.timestamp);
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 3e2c497c31fd..450b3249ba30 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -803,6 +803,19 @@ struct v4l2_plane { __u32 reserved[11]; }; +#ifdef __KERNEL__ +/*
- This is used for the in-kernel version of v4l2_buffer, as we are
- migrating away from using time_t based structures in the kernel.
- User space might see this defined either using 32-bit or 64-bit
- time_t, so we have to convert it when accessing user data.
- */
+struct v4l2_timeval {
- long tv_sec;
- long tv_usec;
+}; +#endif
/**
- struct v4l2_buffer - video buffer info
- @index: id number of the buffer
@@ -839,7 +852,11 @@ struct v4l2_buffer { __u32 bytesused; __u32 flags; __u32 field; +#ifdef __KERNEL__
- struct v4l2_timeval timestamp;
+#else struct timeval timestamp; +#endif struct v4l2_timecode timecode; __u32 sequence;
Regards,
Hans
On Friday 18 September 2015 10:05:06 Hans Verkuil wrote:
On 09/17/15 23:19, Arnd Bergmann wrote:
The v4l2 API uses a 'struct timeval' to communicate time stamps to user space. This is broken on 32-bit architectures as soon as we have a C library that defines time_t as 64 bit, which then changes the structure layout of struct v4l2_buffer.
Since existing user space source code relies on the type to be 'struct timeva' and we want to preserve compile-time compatibility when moving
s/timeva/timeval/
Fixed
to a new libc, we cannot make user-visible changes to the header file.
In this patch, we change the type of the timestamp to 'struct v4l2_timeval'
Don't we need a kernel-wide timeval64? Rather than adding a v4l2-specific struct?
I still hope to avoid doing that. All in-kernel users should be changed to use timespec64 or ktime_t, which are always more efficient and accurate.
For the system call interface, all timeval APIs are deprecated and have replacements using timespec64 (e.g. clock_gettime() replaces gettimeofday).
Only a handful of ioctls pass timeval, and so far my impression is that we are better off handling each one separately. The total amount of code we need to add this way should be less than if we have to duplicate all common code functions that today operate on timeval and can eventually get removed.
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 295fde5fdb75..df5daac6d099 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -235,7 +235,7 @@ static int device_process(struct vim2m_ctx *ctx, in_vb->v4l2_buf.sequence = q_data->sequence++; memcpy(&out_vb->v4l2_buf.timestamp, &in_vb->v4l2_buf.timestamp,
sizeof(struct timeval));
if (in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TIMECODE) memcpy(&out_vb->v4l2_buf.timecode, &in_vb->v4l2_buf.timecode, sizeof(struct v4l2_timecode));sizeof(struct v4l2_timeval));
See https://patchwork.linuxtv.org/patch/31405/
I'll merge that one for 4.4 very soon.
Ok.
Arnd
On 09/18/15 11:09, Arnd Bergmann wrote:
On Friday 18 September 2015 10:05:06 Hans Verkuil wrote:
On 09/17/15 23:19, Arnd Bergmann wrote:
The v4l2 API uses a 'struct timeval' to communicate time stamps to user space. This is broken on 32-bit architectures as soon as we have a C library that defines time_t as 64 bit, which then changes the structure layout of struct v4l2_buffer.
Since existing user space source code relies on the type to be 'struct timeva' and we want to preserve compile-time compatibility when moving
s/timeva/timeval/
Fixed
to a new libc, we cannot make user-visible changes to the header file.
In this patch, we change the type of the timestamp to 'struct v4l2_timeval'
Don't we need a kernel-wide timeval64? Rather than adding a v4l2-specific struct?
I still hope to avoid doing that. All in-kernel users should be changed to use timespec64 or ktime_t, which are always more efficient and accurate.
For the system call interface, all timeval APIs are deprecated and have replacements using timespec64 (e.g. clock_gettime() replaces gettimeofday).
Only a handful of ioctls pass timeval, and so far my impression is that we are better off handling each one separately. The total amount of code we need to add this way should be less than if we have to duplicate all common code functions that today operate on timeval and can eventually get removed.
Ah, OK. Got it.
I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c).
I am not really keen on having v4l2_timeval in all these drivers. I would have to check them anyway since I suspect that in several drivers the local timeval variable can be avoided by rewriting that part of the driver.
Personally I am in favor of a redesigned v4l2_buffer: it's awkward to use with multiplanar formats, there is cruft in there that can be removed (timecode), and there is little space for additions (HW-specific timecodes, more buffer meta data, etc).
We'll see.
Regards,
Hans
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c index 295fde5fdb75..df5daac6d099 100644 --- a/drivers/media/platform/vim2m.c +++ b/drivers/media/platform/vim2m.c @@ -235,7 +235,7 @@ static int device_process(struct vim2m_ctx *ctx, in_vb->v4l2_buf.sequence = q_data->sequence++; memcpy(&out_vb->v4l2_buf.timestamp, &in_vb->v4l2_buf.timestamp,
sizeof(struct timeval));
if (in_vb->v4l2_buf.flags & V4L2_BUF_FLAG_TIMECODE) memcpy(&out_vb->v4l2_buf.timecode, &in_vb->v4l2_buf.timecode, sizeof(struct v4l2_timecode));sizeof(struct v4l2_timeval));
See https://patchwork.linuxtv.org/patch/31405/
I'll merge that one for 4.4 very soon.
Ok.
Arnd
On Friday 18 September 2015 11:27:40 Hans Verkuil wrote:
Ah, OK. Got it.
I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c).
Ah, I think I understood the idea now, I missed that earlier when you mention the idea.
So what you are saying here is that you could come up with a new unambiguous (using only __u32 and __u64 types and no pointers) format that gets exposed to a new set of ioctls, and then change the handling of the existing three formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) so they get converted into the new format by the common ioctl handling code?
I am not really keen on having v4l2_timeval in all these drivers. I would have to check them anyway since I suspect that in several drivers the local timeval variable can be avoided by rewriting that part of the driver.
I've tried to do that for all the drivers where I could find an easy solution in patch 6/9, but I'm sure you can do it for a couple more.
We could also do a lightweight redesign and use 'timespec64' internally in all the drivers and then convert that to 'timeval' or the 64-bit format of that in the ioctl handler. This is also something I tried at some point but then found it a bit more intuitive to leave the normal ioctl path alone and have an explicit type.
Personally I am in favor of a redesigned v4l2_buffer: it's awkward to use with multiplanar formats, there is cruft in there that can be removed (timecode), and there is little space for additions (HW-specific timecodes, more buffer meta data, etc).
We'll see.
Ok.
Arnd
On 09/18/15 11:43, Arnd Bergmann wrote:
On Friday 18 September 2015 11:27:40 Hans Verkuil wrote:
Ah, OK. Got it.
I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c).
Ah, I think I understood the idea now, I missed that earlier when you mention the idea.
So what you are saying here is that you could come up with a new unambiguous (using only __u32 and __u64 types and no pointers) format that gets exposed to a new set of ioctls, and then change the handling of the existing three formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) so they get converted into the new format by the common ioctl handling code?
Right. Drivers only see the new struct, and only v4l2-ioctl.c (and possible v4l2-compat-ioctl32.c) see the old ones.
BTW, I will probably pick up patches 4 and 6 for 4.4. That should help a bit.
Regards,
Hans
I am not really keen on having v4l2_timeval in all these drivers. I would have to check them anyway since I suspect that in several drivers the local timeval variable can be avoided by rewriting that part of the driver.
I've tried to do that for all the drivers where I could find an easy solution in patch 6/9, but I'm sure you can do it for a couple more.
We could also do a lightweight redesign and use 'timespec64' internally in all the drivers and then convert that to 'timeval' or the 64-bit format of that in the ioctl handler. This is also something I tried at some point but then found it a bit more intuitive to leave the normal ioctl path alone and have an explicit type.
Personally I am in favor of a redesigned v4l2_buffer: it's awkward to use with multiplanar formats, there is cruft in there that can be removed (timecode), and there is little space for additions (HW-specific timecodes, more buffer meta data, etc).
We'll see.
Ok.
Arnd
On Friday 18 September 2015 11:52:28 Hans Verkuil wrote:
On 09/18/15 11:43, Arnd Bergmann wrote:
On Friday 18 September 2015 11:27:40 Hans Verkuil wrote:
Ah, OK. Got it.
I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c).
Ah, I think I understood the idea now, I missed that earlier when you mention the idea.
So what you are saying here is that you could come up with a new unambiguous (using only __u32 and __u64 types and no pointers) format that gets exposed to a new set of ioctls, and then change the handling of the existing three formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) so they get converted into the new format by the common ioctl handling code?
Right. Drivers only see the new struct, and only v4l2-ioctl.c (and possible v4l2-compat-ioctl32.c) see the old ones.
BTW, I will probably pick up patches 4 and 6 for 4.4. That should help a bit.
Ok, thanks!
I guess it's up to Mauro to pick up the first three patches?
As I don't see anything more to do for me here until you've had the discussion about the new format, I'll move on to another subsystem now. I have around 70 patches waiting to be submitted, plus the system call series.
Arnd
On 09/18/15 12:08, Arnd Bergmann wrote:
On Friday 18 September 2015 11:52:28 Hans Verkuil wrote:
On 09/18/15 11:43, Arnd Bergmann wrote:
On Friday 18 September 2015 11:27:40 Hans Verkuil wrote:
Ah, OK. Got it.
I think this is dependent on the upcoming media workshop next month. If we decide to redesign v4l2_buffer anyway, then we can avoid timeval completely. And the only place where we would need to convert it in the compat code hidden in the v4l2 core (likely v4l2-ioctl.c).
Ah, I think I understood the idea now, I missed that earlier when you mention the idea.
So what you are saying here is that you could come up with a new unambiguous (using only __u32 and __u64 types and no pointers) format that gets exposed to a new set of ioctls, and then change the handling of the existing three formats (native 64-bit, traditional 32-bit, and 32-bit with 64-bit time_t) so they get converted into the new format by the common ioctl handling code?
Right. Drivers only see the new struct, and only v4l2-ioctl.c (and possible v4l2-compat-ioctl32.c) see the old ones.
BTW, I will probably pick up patches 4 and 6 for 4.4. That should help a bit.
Ok, thanks!
I guess it's up to Mauro to pick up the first three patches?
Yes.
As I don't see anything more to do for me here until you've had the discussion about the new format, I'll move on to another subsystem now. I have around 70 patches waiting to be submitted, plus the system call series.
Agreed.
Thanks again for working on this!
Regards,
Hans
This is the final change to enable user space with 64-bit time_t in the core v4l2 code.
Four ioctls are affected here: VIDIOC_QUERYBUF, VIDIOC_QBUF, VIDIOC_DQBUF, and VIDIOC_PREPARE_BUF. All of them pass a struct v4l2_buffer, which can either contain a 32-bit time_t or a 64-bit time on 32-bit architectures.
In this patch, we redefine the in-kernel version of this structure to use the 64-bit __s64 type through the use of v4l2_timeval. This means the normal ioctl handling now assumes 64-bit time_t and so we have to add support for 32-bit time_t in new handlers.
This is somewhat similar to the 32-bit compat handling on 64-bit architectures, but those also have to modify other fields of the structure. For now, I leave that compat code alone, as it handles the normal case (32-bit compat_time_t) correctly, this has to be done as a follow-up.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/v4l2-core/v4l2-ioctl.c | 174 +++++++++++++++++++++++++++++++++-- include/uapi/linux/videodev2.h | 34 ++++++- 2 files changed, 199 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 7aab18dd2ca5..137475c28e8f 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -438,15 +438,14 @@ static void v4l_print_buffer(const void *arg, bool write_only) const struct v4l2_timecode *tc = &p->timecode; const struct v4l2_plane *plane; int i; + /* y2038 safe because of monotonic time */ + unsigned int seconds = p->timestamp.tv_sec;
- pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, " + pr_cont("%02d:%02d:%02d.%08ld index=%d, type=%s, " "flags=0x%08x, field=%s, sequence=%d, memory=%s", - p->timestamp.tv_sec / 3600, - (int)(p->timestamp.tv_sec / 60) % 60, - (int)(p->timestamp.tv_sec % 60), - (long)p->timestamp.tv_usec, - p->index, - prt_names(p->type, v4l2_type_names), + seconds / 3600, (seconds / 60) % 60, + seconds % 60, (long)p->timestamp.tv_usec, + p->index, prt_names(p->type, v4l2_type_names), p->flags, prt_names(p->field, v4l2_field_names), p->sequence, prt_names(p->memory, v4l2_memory_names));
@@ -1846,6 +1845,123 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops, return ret ? ret : ops->vidioc_prepare_buf(file, fh, b); }
+#ifndef CONFIG_64BIT +static void v4l_get_buffer_time32(struct v4l2_buffer *to, + const struct v4l2_buffer_time32 *from) +{ + to->index = from->index; + to->type = from->type; + to->bytesused = from->bytesused; + to->flags = from->flags; + to->field = from->field; + to->timestamp.tv_sec = from->timestamp.tv_sec; + to->timestamp.tv_usec = from->timestamp.tv_usec; + to->timecode = from->timecode; + to->sequence = from->sequence; + to->memory = from->memory; + to->m.offset = from->m.offset; + to->length = from->length; + to->reserved2 = from->reserved2; + to->reserved = from->reserved; +} + +static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to, + const struct v4l2_buffer *from) +{ + to->index = from->index; + to->type = from->type; + to->bytesused = from->bytesused; + to->flags = from->flags; + to->field = from->field; + to->timestamp.tv_sec = from->timestamp.tv_sec; + to->timestamp.tv_usec = from->timestamp.tv_usec; + to->timecode = from->timecode; + to->sequence = from->sequence; + to->memory = from->memory; + to->m.offset = from->m.offset; + to->length = from->length; + to->reserved2 = from->reserved2; + to->reserved = from->reserved; +} + +static int v4l_querybuf_time32(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *arg) +{ + struct v4l2_buffer buffer; + int ret; + + v4l_get_buffer_time32(&buffer, arg); + ret = v4l_querybuf(ops, file, fh, &buffer); + v4l_put_buffer_time32(arg, &buffer); + + return ret; +} + +static int v4l_qbuf_time32(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *arg) +{ + struct v4l2_buffer buffer; + int ret; + + v4l_get_buffer_time32(&buffer, arg); + ret = v4l_qbuf(ops, file, fh, &buffer); + v4l_put_buffer_time32(arg, &buffer); + + return ret; +} + +static int v4l_dqbuf_time32(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *arg) +{ + struct v4l2_buffer buffer; + int ret; + + v4l_get_buffer_time32(&buffer, arg); + ret = v4l_dqbuf(ops, file, fh, &buffer); + v4l_put_buffer_time32(arg, &buffer); + + return ret; +} + +static int v4l_prepare_buf_time32(const struct v4l2_ioctl_ops *ops, + struct file *file, void *fh, void *arg) +{ + struct v4l2_buffer buffer; + int ret; + + v4l_get_buffer_time32(&buffer, arg); + ret = v4l_prepare_buf(ops, file, fh, &buffer); + + return ret; +} + +static void v4l_print_buffer_time32(const void *arg, bool write_only) +{ + struct v4l2_buffer buffer; + + v4l_get_buffer_time32(&buffer, arg); + v4l_print_buffer(&buffer, write_only); +} + +#ifdef CONFIG_TRACEPOINTS +static void trace_v4l2_dqbuf_time32(int minor, const struct v4l2_buffer_time32 *arg) +{ + struct v4l2_buffer buffer; + + v4l_get_buffer_time32(&buffer, arg); + trace_v4l2_dqbuf(minor, &buffer); +} + +static void trace_v4l2_qbuf_time32(int minor, const struct v4l2_buffer_time32 *arg) +{ + struct v4l2_buffer buffer; + + v4l_get_buffer_time32(&buffer, arg); + trace_v4l2_qbuf(minor, &buffer); +} +#endif +#endif + static int v4l_g_parm(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)), +#ifndef CONFIG_64BIT + IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)), +#endif IOCTL_INFO_STD(VIDIOC_G_FBUF, vidioc_g_fbuf, v4l_print_framebuffer, 0), IOCTL_INFO_STD(VIDIOC_S_FBUF, vidioc_s_fbuf, v4l_print_framebuffer, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_OVERLAY, v4l_overlay, v4l_print_u32, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_QBUF, v4l_qbuf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT + IOCTL_INFO_FNC(VIDIOC_QBUF_TIME32, v4l_qbuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE), +#endif IOCTL_INFO_STD(VIDIOC_EXPBUF, vidioc_expbuf, v4l_print_exportbuffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_exportbuffer, flags)), IOCTL_INFO_FNC(VIDIOC_DQBUF, v4l_dqbuf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT + IOCTL_INFO_FNC(VIDIOC_DQBUF_TIME32, v4l_dqbuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE), +#endif IOCTL_INFO_FNC(VIDIOC_STREAMON, v4l_streamon, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_STREAMOFF, v4l_streamoff, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_G_PARM, v4l_g_parm, v4l_print_streamparm, INFO_FL_CLEAR(v4l2_streamparm, type)), @@ -2479,6 +2604,9 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_UNSUBSCRIBE_EVENT, v4l_unsubscribe_event, v4l_print_event_subscription, 0), IOCTL_INFO_FNC(VIDIOC_CREATE_BUFS, v4l_create_bufs, v4l_print_create_buffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF, v4l_prepare_buf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT + IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF_TIME32, v4l_prepare_buf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE), +#endif IOCTL_INFO_STD(VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings, v4l_print_enum_dv_timings, 0), IOCTL_INFO_STD(VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings, v4l_print_dv_timings, 0), IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)), @@ -2608,6 +2736,12 @@ done: (cmd == VIDIOC_QBUF || cmd == VIDIOC_DQBUF)) return ret;
+#ifndef CONFIG_64BIT + if (!(dev_debug & V4L2_DEV_DEBUG_STREAMING) && + (cmd == VIDIOC_QBUF_TIME32 || cmd == VIDIOC_DQBUF_TIME32)) + return ret; +#endif + v4l_printk_ioctl(video_device_node_name(vfd), cmd); if (ret < 0) pr_cont(": error %ld", ret); @@ -2630,6 +2764,26 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, int ret = 0;
switch (cmd) { +#ifndef CONFIG_64BIT + case VIDIOC_PREPARE_BUF_TIME32: + case VIDIOC_QUERYBUF_TIME32: + case VIDIOC_QBUF_TIME32: + case VIDIOC_DQBUF_TIME32: { + struct v4l2_buffer_time32 *buf = parg; + + if (V4L2_TYPE_IS_MULTIPLANAR(buf->type) && buf->length > 0) { + if (buf->length > VIDEO_MAX_PLANES) { + ret = -EINVAL; + break; + } + *user_ptr = (void __user *)buf->m.planes; + *kernel_ptr = (void **)&buf->m.planes; + *array_size = sizeof(struct v4l2_plane) * buf->length; + ret = 1; + } + break; + } +#endif case VIDIOC_PREPARE_BUF: case VIDIOC_QUERYBUF: case VIDIOC_QBUF: @@ -2774,6 +2928,12 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, trace_v4l2_dqbuf(video_devdata(file)->minor, parg); else if (cmd == VIDIOC_QBUF) trace_v4l2_qbuf(video_devdata(file)->minor, parg); +#ifndef CONFIG_64BIT + else if (cmd == VIDIOC_DQBUF_TIME32) + trace_v4l2_dqbuf_time32(video_devdata(file)->minor, parg); + else if (cmd == VIDIOC_QBUF_TIME32) + trace_v4l2_qbuf_time32(video_devdata(file)->minor, parg); +#endif }
if (has_array_args) { diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 450b3249ba30..0d3688a97ab8 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -811,8 +811,8 @@ struct v4l2_plane { * time_t, so we have to convert it when accessing user data. */ struct v4l2_timeval { - long tv_sec; - long tv_usec; + __s64 tv_sec; + __s64 tv_usec; }; #endif
@@ -873,6 +873,32 @@ struct v4l2_buffer { __u32 reserved; };
+struct v4l2_buffer_time32 { + __u32 index; + __u32 type; + __u32 bytesused; + __u32 flags; + __u32 field; + struct { + __s32 tv_sec; + __s32 tv_usec; + } timestamp; + struct v4l2_timecode timecode; + __u32 sequence; + + /* memory location */ + __u32 memory; + union { + __u32 offset; + unsigned long userptr; + struct v4l2_plane *planes; + __s32 fd; + } m; + __u32 length; + __u32 reserved2; + __u32 reserved; +}; + /* Flags for 'flags' field */ /* Buffer is mapped (flag) */ #define V4L2_BUF_FLAG_MAPPED 0x00000001 @@ -2216,12 +2242,15 @@ struct v4l2_create_buffers { #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format) #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers) #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer) +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32) #define VIDIOC_G_FBUF _IOR('V', 10, struct v4l2_framebuffer) #define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer) #define VIDIOC_OVERLAY _IOW('V', 14, int) #define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer) +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32) #define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer) #define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer) +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32) #define VIDIOC_STREAMON _IOW('V', 18, int) #define VIDIOC_STREAMOFF _IOW('V', 19, int) #define VIDIOC_G_PARM _IOWR('V', 21, struct v4l2_streamparm) @@ -2292,6 +2321,7 @@ struct v4l2_create_buffers { versions */ #define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers) #define VIDIOC_PREPARE_BUF _IOWR('V', 93, struct v4l2_buffer) +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32)
/* Experimental selection API */ #define VIDIOC_G_SELECTION _IOWR('V', 94, struct v4l2_selection)
Hi Arnd,
Thanks once again for working on this! Unfortunately, this approach won't work, see my comments below.
BTW, I would expect to see compile errors when compiling for 32 bit. Did you try that?
On 09/17/2015 11:19 PM, Arnd Bergmann wrote:
This is the final change to enable user space with 64-bit time_t in the core v4l2 code.
Four ioctls are affected here: VIDIOC_QUERYBUF, VIDIOC_QBUF, VIDIOC_DQBUF, and VIDIOC_PREPARE_BUF. All of them pass a struct v4l2_buffer, which can either contain a 32-bit time_t or a 64-bit time on 32-bit architectures.
In this patch, we redefine the in-kernel version of this structure to use the 64-bit __s64 type through the use of v4l2_timeval. This means the normal ioctl handling now assumes 64-bit time_t and so we have to add support for 32-bit time_t in new handlers.
This is somewhat similar to the 32-bit compat handling on 64-bit architectures, but those also have to modify other fields of the structure. For now, I leave that compat code alone, as it handles the normal case (32-bit compat_time_t) correctly, this has to be done as a follow-up.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/media/v4l2-core/v4l2-ioctl.c | 174 +++++++++++++++++++++++++++++++++-- include/uapi/linux/videodev2.h | 34 ++++++- 2 files changed, 199 insertions(+), 9 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c index 7aab18dd2ca5..137475c28e8f 100644 --- a/drivers/media/v4l2-core/v4l2-ioctl.c +++ b/drivers/media/v4l2-core/v4l2-ioctl.c @@ -438,15 +438,14 @@ static void v4l_print_buffer(const void *arg, bool write_only) const struct v4l2_timecode *tc = &p->timecode; const struct v4l2_plane *plane; int i;
- /* y2038 safe because of monotonic time */
- unsigned int seconds = p->timestamp.tv_sec;
- pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, "
- pr_cont("%02d:%02d:%02d.%08ld index=%d, type=%s, " "flags=0x%08x, field=%s, sequence=%d, memory=%s",
p->timestamp.tv_sec / 3600,
(int)(p->timestamp.tv_sec / 60) % 60,
(int)(p->timestamp.tv_sec % 60),
(long)p->timestamp.tv_usec,
p->index,
prt_names(p->type, v4l2_type_names),
seconds / 3600, (seconds / 60) % 60,
seconds % 60, (long)p->timestamp.tv_usec,
p->index, prt_names(p->type, v4l2_type_names), p->flags, prt_names(p->field, v4l2_field_names), p->sequence, prt_names(p->memory, v4l2_memory_names));
@@ -1846,6 +1845,123 @@ static int v4l_prepare_buf(const struct v4l2_ioctl_ops *ops, return ret ? ret : ops->vidioc_prepare_buf(file, fh, b); } +#ifndef CONFIG_64BIT +static void v4l_get_buffer_time32(struct v4l2_buffer *to,
const struct v4l2_buffer_time32 *from)
+{
- to->index = from->index;
- to->type = from->type;
- to->bytesused = from->bytesused;
- to->flags = from->flags;
- to->field = from->field;
- to->timestamp.tv_sec = from->timestamp.tv_sec;
- to->timestamp.tv_usec = from->timestamp.tv_usec;
- to->timecode = from->timecode;
- to->sequence = from->sequence;
- to->memory = from->memory;
- to->m.offset = from->m.offset;
- to->length = from->length;
- to->reserved2 = from->reserved2;
- to->reserved = from->reserved;
+}
+static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
const struct v4l2_buffer *from)
+{
- to->index = from->index;
- to->type = from->type;
- to->bytesused = from->bytesused;
- to->flags = from->flags;
- to->field = from->field;
- to->timestamp.tv_sec = from->timestamp.tv_sec;
- to->timestamp.tv_usec = from->timestamp.tv_usec;
- to->timecode = from->timecode;
- to->sequence = from->sequence;
- to->memory = from->memory;
- to->m.offset = from->m.offset;
- to->length = from->length;
- to->reserved2 = from->reserved2;
- to->reserved = from->reserved;
+}
Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT (path 5/9)? I would prefer that over these explicit assignments.
+static int v4l_querybuf_time32(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
+{
- struct v4l2_buffer buffer;
- int ret;
- v4l_get_buffer_time32(&buffer, arg);
- ret = v4l_querybuf(ops, file, fh, &buffer);
- v4l_put_buffer_time32(arg, &buffer);
- return ret;
+}
+static int v4l_qbuf_time32(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
+{
- struct v4l2_buffer buffer;
- int ret;
- v4l_get_buffer_time32(&buffer, arg);
- ret = v4l_qbuf(ops, file, fh, &buffer);
- v4l_put_buffer_time32(arg, &buffer);
- return ret;
+}
+static int v4l_dqbuf_time32(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
+{
- struct v4l2_buffer buffer;
- int ret;
- v4l_get_buffer_time32(&buffer, arg);
- ret = v4l_dqbuf(ops, file, fh, &buffer);
- v4l_put_buffer_time32(arg, &buffer);
- return ret;
+}
+static int v4l_prepare_buf_time32(const struct v4l2_ioctl_ops *ops,
struct file *file, void *fh, void *arg)
+{
- struct v4l2_buffer buffer;
- int ret;
- v4l_get_buffer_time32(&buffer, arg);
- ret = v4l_prepare_buf(ops, file, fh, &buffer);
- return ret;
+}
+static void v4l_print_buffer_time32(const void *arg, bool write_only) +{
- struct v4l2_buffer buffer;
- v4l_get_buffer_time32(&buffer, arg);
- v4l_print_buffer(&buffer, write_only);
+}
+#ifdef CONFIG_TRACEPOINTS +static void trace_v4l2_dqbuf_time32(int minor, const struct v4l2_buffer_time32 *arg) +{
- struct v4l2_buffer buffer;
- v4l_get_buffer_time32(&buffer, arg);
- trace_v4l2_dqbuf(minor, &buffer);
+}
+static void trace_v4l2_qbuf_time32(int minor, const struct v4l2_buffer_time32 *arg) +{
- struct v4l2_buffer buffer;
- v4l_get_buffer_time32(&buffer, arg);
- trace_v4l2_qbuf(minor, &buffer);
+} +#endif +#endif
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
+#endif
This doesn't work. These IOCTL macros use the ioctl nr as the index in the array. Since VIDIOC_QUERYBUF and VIDIOC_QUERYBUF_TIME32 have the same index, this will fail.
I think (not 100% certain, there may be better suggestions) that the conversion is best done in video_usercopy(): just before func() is called have a switch for the TIME32 variants, convert to the regular variant, call func() and convert back.
My only concern here is that an additional v4l2_buffer struct (68 bytes) is needed on the stack. I don't see any way around that, though.
IOCTL_INFO_STD(VIDIOC_G_FBUF, vidioc_g_fbuf, v4l_print_framebuffer, 0), IOCTL_INFO_STD(VIDIOC_S_FBUF, vidioc_s_fbuf, v4l_print_framebuffer, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_OVERLAY, v4l_overlay, v4l_print_u32, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_QBUF, v4l_qbuf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_QBUF_TIME32, v4l_qbuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE),
+#endif IOCTL_INFO_STD(VIDIOC_EXPBUF, vidioc_expbuf, v4l_print_exportbuffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_exportbuffer, flags)), IOCTL_INFO_FNC(VIDIOC_DQBUF, v4l_dqbuf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_DQBUF_TIME32, v4l_dqbuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE),
+#endif IOCTL_INFO_FNC(VIDIOC_STREAMON, v4l_streamon, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_STREAMOFF, v4l_streamoff, v4l_print_buftype, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_G_PARM, v4l_g_parm, v4l_print_streamparm, INFO_FL_CLEAR(v4l2_streamparm, type)), @@ -2479,6 +2604,9 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_UNSUBSCRIBE_EVENT, v4l_unsubscribe_event, v4l_print_event_subscription, 0), IOCTL_INFO_FNC(VIDIOC_CREATE_BUFS, v4l_create_bufs, v4l_print_create_buffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF, v4l_prepare_buf, v4l_print_buffer, INFO_FL_QUEUE), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_PREPARE_BUF_TIME32, v4l_prepare_buf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE),
+#endif IOCTL_INFO_STD(VIDIOC_ENUM_DV_TIMINGS, vidioc_enum_dv_timings, v4l_print_enum_dv_timings, 0), IOCTL_INFO_STD(VIDIOC_QUERY_DV_TIMINGS, vidioc_query_dv_timings, v4l_print_dv_timings, 0), IOCTL_INFO_STD(VIDIOC_DV_TIMINGS_CAP, vidioc_dv_timings_cap, v4l_print_dv_timings_cap, INFO_FL_CLEAR(v4l2_dv_timings_cap, type)), @@ -2608,6 +2736,12 @@ done: (cmd == VIDIOC_QBUF || cmd == VIDIOC_DQBUF)) return ret; +#ifndef CONFIG_64BIT
if (!(dev_debug & V4L2_DEV_DEBUG_STREAMING) &&
(cmd == VIDIOC_QBUF_TIME32 || cmd == VIDIOC_DQBUF_TIME32))
return ret;
+#endif
- v4l_printk_ioctl(video_device_node_name(vfd), cmd); if (ret < 0) pr_cont(": error %ld", ret);
@@ -2630,6 +2764,26 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, int ret = 0; switch (cmd) { +#ifndef CONFIG_64BIT
- case VIDIOC_PREPARE_BUF_TIME32:
- case VIDIOC_QUERYBUF_TIME32:
- case VIDIOC_QBUF_TIME32:
- case VIDIOC_DQBUF_TIME32: {
struct v4l2_buffer_time32 *buf = parg;
if (V4L2_TYPE_IS_MULTIPLANAR(buf->type) && buf->length > 0) {
if (buf->length > VIDEO_MAX_PLANES) {
ret = -EINVAL;
break;
}
*user_ptr = (void __user *)buf->m.planes;
*kernel_ptr = (void **)&buf->m.planes;
*array_size = sizeof(struct v4l2_plane) * buf->length;
ret = 1;
}
break;
- }
+#endif case VIDIOC_PREPARE_BUF: case VIDIOC_QUERYBUF: case VIDIOC_QBUF: @@ -2774,6 +2928,12 @@ video_usercopy(struct file *file, unsigned int cmd, unsigned long arg, trace_v4l2_dqbuf(video_devdata(file)->minor, parg); else if (cmd == VIDIOC_QBUF) trace_v4l2_qbuf(video_devdata(file)->minor, parg); +#ifndef CONFIG_64BIT
else if (cmd == VIDIOC_DQBUF_TIME32)
trace_v4l2_dqbuf_time32(video_devdata(file)->minor, parg);
else if (cmd == VIDIOC_QBUF_TIME32)
trace_v4l2_qbuf_time32(video_devdata(file)->minor, parg);
+#endif } if (has_array_args) { diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 450b3249ba30..0d3688a97ab8 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -811,8 +811,8 @@ struct v4l2_plane {
- time_t, so we have to convert it when accessing user data.
*/ struct v4l2_timeval {
- long tv_sec;
- long tv_usec;
- __s64 tv_sec;
- __s64 tv_usec;
}; #endif @@ -873,6 +873,32 @@ struct v4l2_buffer { __u32 reserved; }; +struct v4l2_buffer_time32 {
- __u32 index;
- __u32 type;
- __u32 bytesused;
- __u32 flags;
- __u32 field;
- struct {
__s32 tv_sec;
__s32 tv_usec;
- } timestamp;
- struct v4l2_timecode timecode;
- __u32 sequence;
- /* memory location */
- __u32 memory;
- union {
__u32 offset;
unsigned long userptr;
struct v4l2_plane *planes;
__s32 fd;
- } m;
- __u32 length;
- __u32 reserved2;
- __u32 reserved;
+};
Should this be part of a public API at all? Userspace will never use this struct or the TIME32 ioctls in the source code, right? This would be more appropriate for v4l2-ioctl.h.
/* Flags for 'flags' field */ /* Buffer is mapped (flag) */ #define V4L2_BUF_FLAG_MAPPED 0x00000001 @@ -2216,12 +2242,15 @@ struct v4l2_create_buffers { #define VIDIOC_S_FMT _IOWR('V', 5, struct v4l2_format) #define VIDIOC_REQBUFS _IOWR('V', 8, struct v4l2_requestbuffers) #define VIDIOC_QUERYBUF _IOWR('V', 9, struct v4l2_buffer) +#define VIDIOC_QUERYBUF_TIME32 _IOWR('V', 9, struct v4l2_buffer_time32) #define VIDIOC_G_FBUF _IOR('V', 10, struct v4l2_framebuffer) #define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer) #define VIDIOC_OVERLAY _IOW('V', 14, int) #define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer) +#define VIDIOC_QBUF_TIME32 _IOWR('V', 15, struct v4l2_buffer_time32) #define VIDIOC_EXPBUF _IOWR('V', 16, struct v4l2_exportbuffer) #define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer) +#define VIDIOC_DQBUF_TIME32 _IOWR('V', 17, struct v4l2_buffer_time32) #define VIDIOC_STREAMON _IOW('V', 18, int) #define VIDIOC_STREAMOFF _IOW('V', 19, int) #define VIDIOC_G_PARM _IOWR('V', 21, struct v4l2_streamparm) @@ -2292,6 +2321,7 @@ struct v4l2_create_buffers { versions */ #define VIDIOC_CREATE_BUFS _IOWR('V', 92, struct v4l2_create_buffers) #define VIDIOC_PREPARE_BUF _IOWR('V', 93, struct v4l2_buffer) +#define VIDIOC_PREPARE_BUF_TIME32 _IOWR('V', 93, struct v4l2_buffer_time32) /* Experimental selection API */ #define VIDIOC_G_SELECTION _IOWR('V', 94, struct v4l2_selection)
Regards,
Hans
On Friday 18 September 2015 09:18:45 Hans Verkuil wrote:
Hi Arnd,
Thanks once again for working on this! Unfortunately, this approach won't work, see my comments below.
BTW, I would expect to see compile errors when compiling for 32 bit. Did you try that?
I only tested on 32-bit, both ARM and x86, but had a longer set of patches applied below.
+static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
const struct v4l2_buffer *from)
+{
- to->index = from->index;
- to->type = from->type;
- to->bytesused = from->bytesused;
- to->flags = from->flags;
- to->field = from->field;
- to->timestamp.tv_sec = from->timestamp.tv_sec;
- to->timestamp.tv_usec = from->timestamp.tv_usec;
- to->timecode = from->timecode;
- to->sequence = from->sequence;
- to->memory = from->memory;
- to->m.offset = from->m.offset;
- to->length = from->length;
- to->reserved2 = from->reserved2;
- to->reserved = from->reserved;
+}
Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT (path 5/9)? I would prefer that over these explicit assignments.
No strong reason. I went back and forth a bit on the m.offset field that is part of a union: In a previous version, I planned to move all the compat handling here, and doing the conversion one field at a time would make it easier to share the code between 32-bit and 64-bit kernels handling old 32-bit user space. This version doesn't do that, so I can use the memcpy approach instead.
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
+#endif
This doesn't work. These IOCTL macros use the ioctl nr as the index in the array. Since VIDIOC_QUERYBUF and VIDIOC_QUERYBUF_TIME32 have the same index, this will fail.
Ah, I see. No idea why that did not cause a compile-time error. I got some errors for duplicate 'case' values when the structures are the same size (that's why we need the '#ifdef CONFIG_64BIT' in some places) but not here.
I think (not 100% certain, there may be better suggestions) that the conversion is best done in video_usercopy(): just before func() is called have a switch for the TIME32 variants, convert to the regular variant, call func() and convert back.
Does the handler have access to the _IOC_SIZE() value that was passed? If it does, we could add a conditional inside of v4l_querybuf(). I did not see an easy way to do that though.
My only concern here is that an additional v4l2_buffer struct (68 bytes) is needed on the stack. I don't see any way around that, though.
Agreed.
+struct v4l2_buffer_time32 {
- __u32 index;
- __u32 type;
- __u32 bytesused;
- __u32 flags;
- __u32 field;
- struct {
__s32 tv_sec;
__s32 tv_usec;
- } timestamp;
- struct v4l2_timecode timecode;
- __u32 sequence;
- /* memory location */
- __u32 memory;
- union {
__u32 offset;
unsigned long userptr;
struct v4l2_plane *planes;
__s32 fd;
- } m;
- __u32 length;
- __u32 reserved2;
- __u32 reserved;
+};
Should this be part of a public API at all? Userspace will never use this struct or the TIME32 ioctls in the source code, right? This would be more appropriate for v4l2-ioctl.h.
Yes, makes sense. I think for the other structures I just enclosed them inside #ifdef __KERNEL__ so they get stripped at 'make headers_install' time, but I forgot to do that here.
My intention was to keep the structure close to the other one, so any changes to them would be more likely to make it into both versions.
Let me know if you prefer to have an #ifdef added here, or if I should move all three structures to v4l2-ioctl.h.
Thanks a lot for the review!
Arnd
On 09/18/15 11:26, Arnd Bergmann wrote:
On Friday 18 September 2015 09:18:45 Hans Verkuil wrote:
Hi Arnd,
Thanks once again for working on this! Unfortunately, this approach won't work, see my comments below.
BTW, I would expect to see compile errors when compiling for 32 bit. Did you try that?
I only tested on 32-bit, both ARM and x86, but had a longer set of patches applied below.
+static void v4l_put_buffer_time32(struct v4l2_buffer_time32 *to,
const struct v4l2_buffer *from)
+{
- to->index = from->index;
- to->type = from->type;
- to->bytesused = from->bytesused;
- to->flags = from->flags;
- to->field = from->field;
- to->timestamp.tv_sec = from->timestamp.tv_sec;
- to->timestamp.tv_usec = from->timestamp.tv_usec;
- to->timecode = from->timecode;
- to->sequence = from->sequence;
- to->memory = from->memory;
- to->m.offset = from->m.offset;
- to->length = from->length;
- to->reserved2 = from->reserved2;
- to->reserved = from->reserved;
+}
Is there a reason why you didn't use memcpy like you did for VIDIOC_DQEVENT (path 5/9)? I would prefer that over these explicit assignments.
No strong reason. I went back and forth a bit on the m.offset field that is part of a union: In a previous version, I planned to move all the compat handling here, and doing the conversion one field at a time would make it easier to share the code between 32-bit and 64-bit kernels handling old 32-bit user space. This version doesn't do that, so I can use the memcpy approach instead.
static int v4l_g_parm(const struct v4l2_ioctl_ops *ops, struct file *file, void *fh, void *arg) { @@ -2408,12 +2524,21 @@ static struct v4l2_ioctl_info v4l2_ioctls[] = { IOCTL_INFO_FNC(VIDIOC_S_FMT, v4l_s_fmt, v4l_print_format, INFO_FL_PRIO), IOCTL_INFO_FNC(VIDIOC_REQBUFS, v4l_reqbufs, v4l_print_requestbuffers, INFO_FL_PRIO | INFO_FL_QUEUE), IOCTL_INFO_FNC(VIDIOC_QUERYBUF, v4l_querybuf, v4l_print_buffer, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)), +#ifndef CONFIG_64BIT
- IOCTL_INFO_FNC(VIDIOC_QUERYBUF_TIME32, v4l_querybuf_time32, v4l_print_buffer_time32, INFO_FL_QUEUE | INFO_FL_CLEAR(v4l2_buffer, length)),
+#endif
This doesn't work. These IOCTL macros use the ioctl nr as the index in the array. Since VIDIOC_QUERYBUF and VIDIOC_QUERYBUF_TIME32 have the same index, this will fail.
Ah, I see. No idea why that did not cause a compile-time error. I got some errors for duplicate 'case' values when the structures are the same size (that's why we need the '#ifdef CONFIG_64BIT' in some places) but not here.
I think (not 100% certain, there may be better suggestions) that the conversion is best done in video_usercopy(): just before func() is called have a switch for the TIME32 variants, convert to the regular variant, call func() and convert back.
Does the handler have access to the _IOC_SIZE() value that was passed? If it does, we could add a conditional inside of v4l_querybuf(). I did not see an easy way to do that though.
No, the v4l_ handlers don't have access to that. But I think it is much easier to do the conversion at the first opportunity (video_usercopy), so no other code needs to be modified. Easier to review and maintain that way.
My only concern here is that an additional v4l2_buffer struct (68 bytes) is needed on the stack. I don't see any way around that, though.
Agreed.
+struct v4l2_buffer_time32 {
- __u32 index;
- __u32 type;
- __u32 bytesused;
- __u32 flags;
- __u32 field;
- struct {
__s32 tv_sec;
__s32 tv_usec;
- } timestamp;
- struct v4l2_timecode timecode;
- __u32 sequence;
- /* memory location */
- __u32 memory;
- union {
__u32 offset;
unsigned long userptr;
struct v4l2_plane *planes;
__s32 fd;
- } m;
- __u32 length;
- __u32 reserved2;
- __u32 reserved;
+};
Should this be part of a public API at all? Userspace will never use this struct or the TIME32 ioctls in the source code, right? This would be more appropriate for v4l2-ioctl.h.
Yes, makes sense. I think for the other structures I just enclosed them inside #ifdef __KERNEL__ so they get stripped at 'make headers_install' time, but I forgot to do that here.
My intention was to keep the structure close to the other one, so any changes to them would be more likely to make it into both versions.
Let me know if you prefer to have an #ifdef added here, or if I should move all three structures to v4l2-ioctl.h.
*If* the conversion takes place only in v4l2-ioctl.c, then it makes sense have these structs + ioctls moved to v4l2-ioctl.h.
I noticed that v4l2-compat-ioctl32.c wasn't changed. Is that right? I have unfortunately no time to double-check that, but it surprised me.
Regards,
Hans
Thanks a lot for the review!
Arnd
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 18 September 2015 11:49:50 Hans Verkuil wrote:
*If* the conversion takes place only in v4l2-ioctl.c, then it makes sense have these structs + ioctls moved to v4l2-ioctl.h.
Ok.
I noticed that v4l2-compat-ioctl32.c wasn't changed. Is that right? I have unfortunately no time to double-check that, but it surprised me.
Yes, I pointed that out in the cover letter as something that is still left to be done. If we end up taking the current approach (plus the change to the location of the conversion), the compat code would basically be duplicated.
Alternatively, the video_usercopy() function could just be changed to handle both 32-bit formats directly (including conversion of the multiplane array) and then we can remove that compat handler for all eight ioctls.
Arnd
C libraries with 64-bit time_t use an incompatible format for struct omap3isp_stat_data. This changes the kernel code to support either version, by moving over the normal handling to the 64-bit variant, and adding compatiblity code to handle the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure, so the difference gets handled automatically.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++ drivers/media/platform/omap3isp/isph3a_af.c | 2 ++ drivers/media/platform/omap3isp/isphist.c | 2 ++ drivers/media/platform/omap3isp/ispstat.c | 18 ++++++++++++++++-- drivers/media/platform/omap3isp/ispstat.h | 2 ++ include/uapi/linux/omap3isp.h | 19 +++++++++++++++++++ 6 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c index ccaf92f39236..2d567725608f 100644 --- a/drivers/media/platform/omap3isp/isph3a_aewb.c +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg); + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32: + return omap3isp_stat_request_statistics_time32(stat, arg); case VIDIOC_OMAP3ISP_STAT_EN: { unsigned long *en = arg; return omap3isp_stat_enable(stat, !!*en); diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c index 92937f7eecef..2ac02371318b 100644 --- a/drivers/media/platform/omap3isp/isph3a_af.c +++ b/drivers/media/platform/omap3isp/isph3a_af.c @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg); + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32: + return omap3isp_stat_request_statistics_time32(stat, arg); case VIDIOC_OMAP3ISP_STAT_EN: { int *en = arg; return omap3isp_stat_enable(stat, !!*en); diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c index 7138b043a4aa..669b97b079ee 100644 --- a/drivers/media/platform/omap3isp/isphist.c +++ b/drivers/media/platform/omap3isp/isphist.c @@ -436,6 +436,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg); + case VIDIOC_OMAP3ISP_STAT_REQ_TIME32: + return omap3isp_stat_request_statistics_time32(stat, arg); case VIDIOC_OMAP3ISP_STAT_EN: { int *en = arg; return omap3isp_stat_enable(stat, !!*en); diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index fa96e330c563..3d70332422b5 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -496,8 +496,7 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return PTR_ERR(buf); }
- data->ts.tv_sec = buf->ts.tv_sec; - data->ts.tv_usec = buf->ts.tv_usec; + data->ts = buf->ts; data->config_counter = buf->config_counter; data->frame_number = buf->frame_number; data->buf_size = buf->buf_size; @@ -509,6 +508,21 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return 0; }
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat, + struct omap3isp_stat_data_time32 *data) +{ + struct omap3isp_stat_data data64; + int ret; + + ret = omap3isp_stat_request_statistics(stat, &data64); + + data->ts.tv_sec = data64.ts.tv_sec; + data->ts.tv_usec = data64.ts.tv_usec; + memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts)); + + return ret; +} + /* * omap3isp_stat_config - Receives new statistic engine configuration. * @new_conf: Pointer to config structure. diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index 7b4f136567a3..b19ea6c8f733 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -130,6 +130,8 @@ struct ispstat_generic_config { int omap3isp_stat_config(struct ispstat *stat, void *new_conf); int omap3isp_stat_request_statistics(struct ispstat *stat, struct omap3isp_stat_data *data); +int omap3isp_stat_request_statistics_time32(struct ispstat *stat, + struct omap3isp_stat_data_time32 *data); int omap3isp_stat_init(struct ispstat *stat, const char *name, const struct v4l2_subdev_ops *sd_ops); void omap3isp_stat_cleanup(struct ispstat *stat); diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h index c090cf9249bb..4bff66aefca5 100644 --- a/include/uapi/linux/omap3isp.h +++ b/include/uapi/linux/omap3isp.h @@ -54,6 +54,8 @@ _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config) #define VIDIOC_OMAP3ISP_STAT_REQ \ _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data) +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \ + _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32) #define VIDIOC_OMAP3ISP_STAT_EN \ _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
@@ -164,7 +166,11 @@ struct omap3isp_h3a_aewb_config { * @config_counter: Number of the configuration associated with the data. */ struct omap3isp_stat_data { +#ifdef __KERNEL__ + struct v4l2_timeval ts; +#else struct timeval ts; +#endif void __user *buf; __u32 buf_size; __u16 frame_number; @@ -172,6 +178,19 @@ struct omap3isp_stat_data { __u16 config_counter; };
+#ifdef __KERNEL__ +struct omap3isp_stat_data_time32 { + struct { + __s32 tv_sec; + __s32 tv_usec; + } ts; + __u32 buf; + __u32 buf_size; + __u16 frame_number; + __u16 cur_frame; + __u16 config_counter; +}; +#endif
/* Histogram related structs */
Hi Arnd,
Thank you for the patch.
On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
C libraries with 64-bit time_t use an incompatible format for struct omap3isp_stat_data. This changes the kernel code to support either version, by moving over the normal handling to the 64-bit variant, and adding compatiblity code to handle the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure, so the difference gets handled automatically.
We plan to design a new interface to handle statistics in V4L2. That API should support proper 64-bit timestamps out of the box, and will be implemented by the OMAP3 ISP driver. Userspace should then move to it. I wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl given that I expect it to have a handful of users at most.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/media/platform/omap3isp/isph3a_aewb.c | 2 ++ drivers/media/platform/omap3isp/isph3a_af.c | 2 ++ drivers/media/platform/omap3isp/isphist.c | 2 ++ drivers/media/platform/omap3isp/ispstat.c | 18 ++++++++++++++++-- drivers/media/platform/omap3isp/ispstat.h | 2 ++ include/uapi/linux/omap3isp.h | 19 +++++++++++++++++++ 6 files changed, 43 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isph3a_aewb.c b/drivers/media/platform/omap3isp/isph3a_aewb.c index ccaf92f39236..2d567725608f 100644 --- a/drivers/media/platform/omap3isp/isph3a_aewb.c +++ b/drivers/media/platform/omap3isp/isph3a_aewb.c @@ -250,6 +250,8 @@ static long h3a_aewb_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg);
- case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
case VIDIOC_OMAP3ISP_STAT_EN: { unsigned long *en = arg; return omap3isp_stat_enable(stat, !!*en);return omap3isp_stat_request_statistics_time32(stat, arg);
diff --git a/drivers/media/platform/omap3isp/isph3a_af.c b/drivers/media/platform/omap3isp/isph3a_af.c index 92937f7eecef..2ac02371318b 100644 --- a/drivers/media/platform/omap3isp/isph3a_af.c +++ b/drivers/media/platform/omap3isp/isph3a_af.c @@ -314,6 +314,8 @@ static long h3a_af_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg);
- case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
case VIDIOC_OMAP3ISP_STAT_EN: { int *en = arg; return omap3isp_stat_enable(stat, !!*en);return omap3isp_stat_request_statistics_time32(stat, arg);
diff --git a/drivers/media/platform/omap3isp/isphist.c b/drivers/media/platform/omap3isp/isphist.c index 7138b043a4aa..669b97b079ee 100644 --- a/drivers/media/platform/omap3isp/isphist.c +++ b/drivers/media/platform/omap3isp/isphist.c @@ -436,6 +436,8 @@ static long hist_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg) return omap3isp_stat_config(stat, arg); case VIDIOC_OMAP3ISP_STAT_REQ: return omap3isp_stat_request_statistics(stat, arg);
- case VIDIOC_OMAP3ISP_STAT_REQ_TIME32:
case VIDIOC_OMAP3ISP_STAT_EN: { int *en = arg; return omap3isp_stat_enable(stat, !!*en);return omap3isp_stat_request_statistics_time32(stat, arg);
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index fa96e330c563..3d70332422b5 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -496,8 +496,7 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return PTR_ERR(buf); }
- data->ts.tv_sec = buf->ts.tv_sec;
- data->ts.tv_usec = buf->ts.tv_usec;
- data->ts = buf->ts; data->config_counter = buf->config_counter; data->frame_number = buf->frame_number; data->buf_size = buf->buf_size;
@@ -509,6 +508,21 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, return 0; }
+int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
struct omap3isp_stat_data_time32 *data)
+{
- struct omap3isp_stat_data data64;
- int ret;
- ret = omap3isp_stat_request_statistics(stat, &data64);
- data->ts.tv_sec = data64.ts.tv_sec;
- data->ts.tv_usec = data64.ts.tv_usec;
- memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts));
- return ret;
+}
/*
- omap3isp_stat_config - Receives new statistic engine configuration.
- @new_conf: Pointer to config structure.
diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index 7b4f136567a3..b19ea6c8f733 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -130,6 +130,8 @@ struct ispstat_generic_config { int omap3isp_stat_config(struct ispstat *stat, void *new_conf); int omap3isp_stat_request_statistics(struct ispstat *stat, struct omap3isp_stat_data *data); +int omap3isp_stat_request_statistics_time32(struct ispstat *stat,
struct omap3isp_stat_data_time32 *data);
int omap3isp_stat_init(struct ispstat *stat, const char *name, const struct v4l2_subdev_ops *sd_ops); void omap3isp_stat_cleanup(struct ispstat *stat); diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h index c090cf9249bb..4bff66aefca5 100644 --- a/include/uapi/linux/omap3isp.h +++ b/include/uapi/linux/omap3isp.h @@ -54,6 +54,8 @@ _IOWR('V', BASE_VIDIOC_PRIVATE + 5, struct omap3isp_h3a_af_config) #define VIDIOC_OMAP3ISP_STAT_REQ \ _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data) +#define VIDIOC_OMAP3ISP_STAT_REQ_TIME32 \
- _IOWR('V', BASE_VIDIOC_PRIVATE + 6, struct omap3isp_stat_data_time32)
#define VIDIOC_OMAP3ISP_STAT_EN \ _IOWR('V', BASE_VIDIOC_PRIVATE + 7, unsigned long)
@@ -164,7 +166,11 @@ struct omap3isp_h3a_aewb_config {
- @config_counter: Number of the configuration associated with the data.
*/ struct omap3isp_stat_data { +#ifdef __KERNEL__
- struct v4l2_timeval ts;
+#else struct timeval ts; +#endif void __user *buf; __u32 buf_size; __u16 frame_number; @@ -172,6 +178,19 @@ struct omap3isp_stat_data { __u16 config_counter; };
+#ifdef __KERNEL__ +struct omap3isp_stat_data_time32 {
- struct {
__s32 tv_sec;
__s32 tv_usec;
- } ts;
- __u32 buf;
- __u32 buf_size;
- __u16 frame_number;
- __u16 cur_frame;
- __u16 config_counter;
+}; +#endif
/* Histogram related structs */
On Monday 09 November 2015 22:09:26 Laurent Pinchart wrote:
Hi Arnd,
Thank you for the patch.
On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
C libraries with 64-bit time_t use an incompatible format for struct omap3isp_stat_data. This changes the kernel code to support either version, by moving over the normal handling to the 64-bit variant, and adding compatiblity code to handle the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure, so the difference gets handled automatically.
We plan to design a new interface to handle statistics in V4L2. That API should support proper 64-bit timestamps out of the box, and will be implemented by the OMAP3 ISP driver. Userspace should then move to it. I wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl given that I expect it to have a handful of users at most.
We still need to do something to the driver. The alternative would be to make the existing ioctl command optional at kernel compile-time so we can still build the driver once we remove the 'struct timeval' definition. That patch would add slightly less complexity here but also lose functionality.
As my patch here depends on the struct v4l2_timeval I introduced in an earlier patch of the series, we will have to change it anyways, but I'd prefer to keep the basic idea. Let's get back to this one after the v4l_buffer replacement work is done.
Arnd
Hi Arnd,
On Monday 09 November 2015 21:30:41 Arnd Bergmann wrote:
On Monday 09 November 2015 22:09:26 Laurent Pinchart wrote:
On Thursday 17 September 2015 23:19:40 Arnd Bergmann wrote:
C libraries with 64-bit time_t use an incompatible format for struct omap3isp_stat_data. This changes the kernel code to support either version, by moving over the normal handling to the 64-bit variant, and adding compatiblity code to handle the old binary format with the existing ioctl command code.
Fortunately, the command code includes the size of the structure, so the difference gets handled automatically.
We plan to design a new interface to handle statistics in V4L2. That API should support proper 64-bit timestamps out of the box, and will be implemented by the OMAP3 ISP driver. Userspace should then move to it. I wonder if it's worth it to fix the existing VIDIOC_OMAP3ISP_STAT_REQ ioctl given that I expect it to have a handful of users at most.
We still need to do something to the driver. The alternative would be to make the existing ioctl command optional at kernel compile-time so we can still build the driver once we remove the 'struct timeval' definition. That patch would add slightly less complexity here but also lose functionality.
As my patch here depends on the struct v4l2_timeval I introduced in an earlier patch of the series, we will have to change it anyways, but I'd prefer to keep the basic idea. Let's get back to this one after the v4l_buffer replacement work is done.
Fine with me. I'll mark the patch as "Obsoleted" in patchwork in the meantime.