| Message ID | 1347659868-17398-1-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks, Laurent! On Fri, Sep 14, 2012 at 11:57:48PM +0200, Laurent Pinchart wrote: > V4L2 buffers use the monotonic clock, while statistics buffers use wall > time. This makes it difficult to correlate video frames and statistics. > > Switch statistics buffers to the monotonic clock to fix this, and > replace struct timeval with struct timespec. > > Reported-by: Antoine Reversat <a.reversat@gmail.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/omap3isp/ispstat.c | 2 +- > drivers/media/platform/omap3isp/ispstat.h | 2 +- > include/linux/omap3isp.h | 7 ++++++- > 3 files changed, 8 insertions(+), 3 deletions(-) Acked-by: Sakari Ailus <sakari.ailu@iki.fi>
Em Fri, 14 Sep 2012 23:57:48 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > V4L2 buffers use the monotonic clock, while statistics buffers use wall > time. This makes it difficult to correlate video frames and statistics. > > Switch statistics buffers to the monotonic clock to fix this, and > replace struct timeval with struct timespec. > > Reported-by: Antoine Reversat <a.reversat@gmail.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/omap3isp/ispstat.c | 2 +- > drivers/media/platform/omap3isp/ispstat.h | 2 +- > include/linux/omap3isp.h | 7 ++++++- > 3 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > index b8640be..bb21c4e 100644 > --- a/drivers/media/platform/omap3isp/ispstat.c > +++ b/drivers/media/platform/omap3isp/ispstat.c > @@ -256,7 +256,7 @@ static int isp_stat_buf_queue(struct ispstat *stat) > if (!stat->active_buf) > return STAT_NO_BUF; > > - do_gettimeofday(&stat->active_buf->ts); > + ktime_get_ts(&stat->active_buf->ts); > > stat->active_buf->buf_size = stat->buf_size; > if (isp_stat_buf_check_magic(stat, stat->active_buf)) { > diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h > index 9b7c865..8221d0c 100644 > --- a/drivers/media/platform/omap3isp/ispstat.h > +++ b/drivers/media/platform/omap3isp/ispstat.h > @@ -50,7 +50,7 @@ struct ispstat_buffer { > struct iovm_struct *iovm; > void *virt_addr; > dma_addr_t dma_addr; > - struct timeval ts; > + struct timespec ts; > u32 buf_size; > u32 frame_number; > u16 config_counter; > diff --git a/include/linux/omap3isp.h b/include/linux/omap3isp.h > index c090cf9..263a0c0 100644 > --- a/include/linux/omap3isp.h > +++ b/include/linux/omap3isp.h > @@ -27,6 +27,11 @@ > #ifndef OMAP3_ISP_USER_H > #define OMAP3_ISP_USER_H > > +#ifdef __KERNEL__ > +#include <linux/time.h> /* need struct timespec */ > +#else > +#include <sys/time.h> > +#endif > #include <linux/types.h> > #include <linux/videodev2.h> > > @@ -164,7 +169,7 @@ struct omap3isp_h3a_aewb_config { > * @config_counter: Number of the configuration associated with the data. > */ > struct omap3isp_stat_data { > - struct timeval ts; > + struct timespec ts; NACK. That breaks userspace API, as this structure is part of an ioctl. It is too late to touch here. Please keep timeval. It is ok to fill it with a mononotic time, but replacing it is an API breakage. > void __user *buf; > __u32 buf_size; > __u16 frame_number;
Hi Mauro, Mauro Carvalho Chehab wrote: > Em Fri, 14 Sep 2012 23:57:48 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > >> V4L2 buffers use the monotonic clock, while statistics buffers use wall >> time. This makes it difficult to correlate video frames and statistics. >> >> Switch statistics buffers to the monotonic clock to fix this, and >> replace struct timeval with struct timespec. >> >> Reported-by: Antoine Reversat <a.reversat@gmail.com> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> --- >> drivers/media/platform/omap3isp/ispstat.c | 2 +- >> drivers/media/platform/omap3isp/ispstat.h | 2 +- >> include/linux/omap3isp.h | 7 ++++++- >> 3 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c >> index b8640be..bb21c4e 100644 >> --- a/drivers/media/platform/omap3isp/ispstat.c >> +++ b/drivers/media/platform/omap3isp/ispstat.c >> @@ -256,7 +256,7 @@ static int isp_stat_buf_queue(struct ispstat *stat) >> if (!stat->active_buf) >> return STAT_NO_BUF; >> >> - do_gettimeofday(&stat->active_buf->ts); >> + ktime_get_ts(&stat->active_buf->ts); >> >> stat->active_buf->buf_size = stat->buf_size; >> if (isp_stat_buf_check_magic(stat, stat->active_buf)) { >> diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h >> index 9b7c865..8221d0c 100644 >> --- a/drivers/media/platform/omap3isp/ispstat.h >> +++ b/drivers/media/platform/omap3isp/ispstat.h >> @@ -50,7 +50,7 @@ struct ispstat_buffer { >> struct iovm_struct *iovm; >> void *virt_addr; >> dma_addr_t dma_addr; >> - struct timeval ts; >> + struct timespec ts; >> u32 buf_size; >> u32 frame_number; >> u16 config_counter; >> diff --git a/include/linux/omap3isp.h b/include/linux/omap3isp.h >> index c090cf9..263a0c0 100644 >> --- a/include/linux/omap3isp.h >> +++ b/include/linux/omap3isp.h >> @@ -27,6 +27,11 @@ >> #ifndef OMAP3_ISP_USER_H >> #define OMAP3_ISP_USER_H >> >> +#ifdef __KERNEL__ >> +#include <linux/time.h> /* need struct timespec */ >> +#else >> +#include <sys/time.h> >> +#endif >> #include <linux/types.h> >> #include <linux/videodev2.h> >> >> @@ -164,7 +169,7 @@ struct omap3isp_h3a_aewb_config { >> * @config_counter: Number of the configuration associated with the data. >> */ >> struct omap3isp_stat_data { >> - struct timeval ts; >> + struct timespec ts; > > NACK. That breaks userspace API, as this structure is part of an ioctl. > > It is too late to touch here. Please keep timeval. It is ok to fill it with > a mononotic time, but replacing it is an API breakage. I beg to present a differing opinion. The timestamp that has been taken from a realtime clock has NOT been useful to begin with in this context: the OMAP3ISP driver has used monotonic time on video buffers since the very beginning of its existence in mainline kernel. As no-one has complained about this --- except Antoine very recently --- I'm pretty certain we wouldn't be breaking any application by changing this. The statistics timestamp is only useful when it's comparable to other timestamps (from video buffers and events), which this patch achieves. Kind regards,
Hi Mauro, On Thursday 27 September 2012 22:49:34 Sakari Ailus wrote: > Mauro Carvalho Chehab wrote: > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > >> V4L2 buffers use the monotonic clock, while statistics buffers use wall > >> time. This makes it difficult to correlate video frames and statistics. > >> > >> Switch statistics buffers to the monotonic clock to fix this, and > >> replace struct timeval with struct timespec. > >> > >> Reported-by: Antoine Reversat <a.reversat@gmail.com> > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- > >> > >> drivers/media/platform/omap3isp/ispstat.c | 2 +- > >> drivers/media/platform/omap3isp/ispstat.h | 2 +- > >> include/linux/omap3isp.h | 7 ++++++- > >> 3 files changed, 8 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/media/platform/omap3isp/ispstat.c > >> b/drivers/media/platform/omap3isp/ispstat.c index b8640be..bb21c4e > >> 100644 > >> --- a/drivers/media/platform/omap3isp/ispstat.c > >> +++ b/drivers/media/platform/omap3isp/ispstat.c > >> @@ -256,7 +256,7 @@ static int isp_stat_buf_queue(struct ispstat *stat) > >> > >> if (!stat->active_buf) > >> > >> return STAT_NO_BUF; > >> > >> - do_gettimeofday(&stat->active_buf->ts); > >> + ktime_get_ts(&stat->active_buf->ts); > >> > >> stat->active_buf->buf_size = stat->buf_size; > >> if (isp_stat_buf_check_magic(stat, stat->active_buf)) { > >> > >> diff --git a/drivers/media/platform/omap3isp/ispstat.h > >> b/drivers/media/platform/omap3isp/ispstat.h index 9b7c865..8221d0c > >> 100644 > >> --- a/drivers/media/platform/omap3isp/ispstat.h > >> +++ b/drivers/media/platform/omap3isp/ispstat.h > >> @@ -50,7 +50,7 @@ struct ispstat_buffer { > >> > >> struct iovm_struct *iovm; > >> void *virt_addr; > >> dma_addr_t dma_addr; > >> > >> - struct timeval ts; > >> + struct timespec ts; > >> > >> u32 buf_size; > >> u32 frame_number; > >> u16 config_counter; > >> > >> diff --git a/include/linux/omap3isp.h b/include/linux/omap3isp.h > >> index c090cf9..263a0c0 100644 > >> --- a/include/linux/omap3isp.h > >> +++ b/include/linux/omap3isp.h > >> @@ -27,6 +27,11 @@ > >> > >> #ifndef OMAP3_ISP_USER_H > >> #define OMAP3_ISP_USER_H > >> > >> +#ifdef __KERNEL__ > >> +#include <linux/time.h> /* need struct timespec */ > >> +#else > >> +#include <sys/time.h> > >> +#endif > >> > >> #include <linux/types.h> > >> #include <linux/videodev2.h> > >> > >> @@ -164,7 +169,7 @@ struct omap3isp_h3a_aewb_config { > >> > >> * @config_counter: Number of the configuration associated with the > >> data. > >> */ > >> > >> struct omap3isp_stat_data { > >> > >> - struct timeval ts; > >> + struct timespec ts; > > > > NACK. That breaks userspace API, as this structure is part of an ioctl. > > > > It is too late to touch here. Please keep timeval. It is ok to fill it > > with a mononotic time, but replacing it is an API breakage. > > I beg to present a differing opinion. > > The timestamp that has been taken from a realtime clock has NOT been > useful to begin with in this context: the OMAP3ISP driver has used > monotonic time on video buffers since the very beginning of its > existence in mainline kernel. As no-one has complained about this --- > except Antoine very recently --- I'm pretty certain we wouldn't be > breaking any application by changing this. The statistics timestamp is > only useful when it's comparable to other timestamps (from video buffers > and events), which this patch achieves. I second this opinion. We're not chaging the size of the omap3isp_stat_data structure here, and I'm very confident that there's currently no user of the ts field.
Em 27-09-2012 16:49, Sakari Ailus escreveu: > Hi Mauro, > > Mauro Carvalho Chehab wrote: >> Em Fri, 14 Sep 2012 23:57:48 +0200 >> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: ... >>> struct omap3isp_stat_data { >>> - struct timeval ts; >>> + struct timespec ts; >> >> NACK. That breaks userspace API, as this structure is part of an ioctl. >> >> It is too late to touch here. Please keep timeval. It is ok to fill it with >> a mononotic time, but replacing it is an API breakage. > > I beg to present a differing opinion. > > The timestamp that has been taken from a realtime clock has NOT been useful to begin with in this context: the OMAP3ISP driver has used monotonic time on video buffers since the very beginning of its existence in mainline kernel. As no-one has complained about this --- except Antoine very recently --- I'm pretty certain we wouldn't be breaking any application by changing this. The statistics timestamp is only useful when it's comparable to other timestamps (from video buffers and events), which this patch achieves. Technically, the only gain here would be to improve the precision from microsseconds to nanosseconds. Breaking the API due to that could only be justified if you have really fast sensors that would be able to take more than one picture at the 1 microssecond period of time. Regards, Mauro -- 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
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c index b8640be..bb21c4e 100644 --- a/drivers/media/platform/omap3isp/ispstat.c +++ b/drivers/media/platform/omap3isp/ispstat.c @@ -256,7 +256,7 @@ static int isp_stat_buf_queue(struct ispstat *stat) if (!stat->active_buf) return STAT_NO_BUF; - do_gettimeofday(&stat->active_buf->ts); + ktime_get_ts(&stat->active_buf->ts); stat->active_buf->buf_size = stat->buf_size; if (isp_stat_buf_check_magic(stat, stat->active_buf)) { diff --git a/drivers/media/platform/omap3isp/ispstat.h b/drivers/media/platform/omap3isp/ispstat.h index 9b7c865..8221d0c 100644 --- a/drivers/media/platform/omap3isp/ispstat.h +++ b/drivers/media/platform/omap3isp/ispstat.h @@ -50,7 +50,7 @@ struct ispstat_buffer { struct iovm_struct *iovm; void *virt_addr; dma_addr_t dma_addr; - struct timeval ts; + struct timespec ts; u32 buf_size; u32 frame_number; u16 config_counter; diff --git a/include/linux/omap3isp.h b/include/linux/omap3isp.h index c090cf9..263a0c0 100644 --- a/include/linux/omap3isp.h +++ b/include/linux/omap3isp.h @@ -27,6 +27,11 @@ #ifndef OMAP3_ISP_USER_H #define OMAP3_ISP_USER_H +#ifdef __KERNEL__ +#include <linux/time.h> /* need struct timespec */ +#else +#include <sys/time.h> +#endif #include <linux/types.h> #include <linux/videodev2.h> @@ -164,7 +169,7 @@ struct omap3isp_h3a_aewb_config { * @config_counter: Number of the configuration associated with the data. */ struct omap3isp_stat_data { - struct timeval ts; + struct timespec ts; void __user *buf; __u32 buf_size; __u16 frame_number;
V4L2 buffers use the monotonic clock, while statistics buffers use wall time. This makes it difficult to correlate video frames and statistics. Switch statistics buffers to the monotonic clock to fix this, and replace struct timeval with struct timespec. Reported-by: Antoine Reversat <a.reversat@gmail.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/platform/omap3isp/ispstat.c | 2 +- drivers/media/platform/omap3isp/ispstat.h | 2 +- include/linux/omap3isp.h | 7 ++++++- 3 files changed, 8 insertions(+), 3 deletions(-)