| Message ID | 1352270424-14683-1-git-send-email-shaik.ameer@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shaik, On 11/07/2012 07:40 AM, Shaik Ameer Basha wrote: > Make gsc-m2m propagate the timestamp field from source to destination > buffers We probably need some means for letting know the mem-to-mem drivers and applications whether timestamps are copied from OUTPUT to CAPTURE or not. Timestamps at only OUTPUT interface are normally used to control buffer processing time [1]. "struct timeval timestamp For input streams this is the system time (as returned by the gettimeofday() function) when the first data byte was captured. For output streams the data will not be displayed before this time, secondary to the nominal frame rate determined by the current video standard in enqueued order. Applications can for example zero this field to display frames as soon as possible. The driver stores the time at which the first data byte was actually sent out in the timestamp field. This permits applications to monitor the drift between the video and system clock." In some use cases it might be useful to know exact frame processing time, where driver would be filling OUTPUT and CAPTURE value with exact monotonic clock values corresponding to a frame processing start and end time. [1] http://linuxtv.org/downloads/v4l-dvb-apis/buffer.html#v4l2-buffer For the time being I'm OK with this patch, just one comment below... > Signed-off-by: John Sheu<sheu@google.com> > Signed-off-by: Shaik Ameer Basha<shaik.ameer@samsung.com> > --- > drivers/media/platform/exynos-gsc/gsc-m2m.c | 19 ++++++++++++------- > 1 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c > index 047f0f0..1139276 100644 > --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c > +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c > @@ -99,22 +99,27 @@ static void gsc_m2m_job_abort(void *priv) > gsc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR); > } > > -static int gsc_fill_addr(struct gsc_ctx *ctx) > +static int gsc_get_bufs(struct gsc_ctx *ctx) > { > struct gsc_frame *s_frame, *d_frame; > - struct vb2_buffer *vb = NULL; > + struct vb2_buffer *src_vb, *dst_vb; > int ret; > > s_frame =&ctx->s_frame; > d_frame =&ctx->d_frame; > > - vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx); > - ret = gsc_prepare_addr(ctx, vb, s_frame,&s_frame->addr); > + src_vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx); > + ret = gsc_prepare_addr(ctx, src_vb, s_frame,&s_frame->addr); Might be better to just return any error code if (ret) return ret; > + dst_vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx); > + ret |= gsc_prepare_addr(ctx, dst_vb, d_frame,&d_frame->addr); ...rather than obfuscating the return value here. > if (ret) > return ret; > > - vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx); > - return gsc_prepare_addr(ctx, vb, d_frame,&d_frame->addr); > + memcpy(&dst_vb->v4l2_buf.timestamp,&src_vb->v4l2_buf.timestamp, > + sizeof(dst_vb->v4l2_buf.timestamp)); Is there any advantage of memcpy over simple assignment dst_vb->v4l2_buf.timestamp = src_vb->v4l2_buf.timestamp; ? > + return 0; > } > > static void gsc_m2m_device_run(void *priv) > @@ -148,7 +153,7 @@ static void gsc_m2m_device_run(void *priv) > goto put_device; > } > > - ret = gsc_fill_addr(ctx); > + ret = gsc_get_bufs(ctx); > if (ret) { > pr_err("Wrong address"); > goto put_device; -- Thanks, Sylwester -- 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
Hi Sylwester and Shaik, On Mon, Nov 19, 2012 at 11:06:34PM +0100, Sylwester Nawrocki wrote: > On 11/07/2012 07:40 AM, Shaik Ameer Basha wrote: > >Make gsc-m2m propagate the timestamp field from source to destination > >buffers > > We probably need some means for letting know the mem-to-mem drivers and > applications whether timestamps are copied from OUTPUT to CAPTURE or not. > Timestamps at only OUTPUT interface are normally used to control buffer > processing time [1]. > > > "struct timeval timestamp > > For input streams this is the system time (as returned by the > gettimeofday() > function) when the first data byte was captured. For output streams Thanks for notifying me; this is going to be dependent on the timestamp type. Also most drivers use the time the buffer is finished rather than when the "first data byte was captured", but that's separate I think. > the data > will not be displayed before this time, secondary to the nominal frame rate > determined by the current video standard in enqueued order. > Applications can > for example zero this field to display frames as soon as possible. > The driver > stores the time at which the first data byte was actually sent out in the > timestamp field. This permits applications to monitor the drift between the > video and system clock." > > In some use cases it might be useful to know exact frame processing time, > where driver would be filling OUTPUT and CAPTURE value with exact monotonic > clock values corresponding to a frame processing start and end time. Shouldn't this always be done in memory-to-memory processing? I could imagine only performance measurements can benefit from other kind of timestamps. We could use different timestamp type to tell the timestamp source isn't any system clock but an input buffer. What do you think?
Hi Sakari, > From: Sakari Ailus [mailto:sakari.ailus@iki.fi] > Sent: Wednesday, November 21, 2012 8:40 PM > > Hi Sylwester and Shaik, > > On Mon, Nov 19, 2012 at 11:06:34PM +0100, Sylwester Nawrocki wrote: > > On 11/07/2012 07:40 AM, Shaik Ameer Basha wrote: > > >Make gsc-m2m propagate the timestamp field from source to > destination > > >buffers > > > > We probably need some means for letting know the mem-to-mem drivers > > and applications whether timestamps are copied from OUTPUT to CAPTURE > or not. > > Timestamps at only OUTPUT interface are normally used to control > > buffer processing time [1]. > > > > > > "struct timeval timestamp > > > > For input streams this is the system time (as returned by the > > gettimeofday() > > function) when the first data byte was captured. For output streams > > Thanks for notifying me; this is going to be dependent on the timestamp > type. > > Also most drivers use the time the buffer is finished rather than when > the "first data byte was captured", but that's separate I think. > > > the data > > will not be displayed before this time, secondary to the nominal > frame > > rate determined by the current video standard in enqueued order. > > Applications can > > for example zero this field to display frames as soon as possible. > > The driver > > stores the time at which the first data byte was actually sent out in > > the timestamp field. This permits applications to monitor the drift > > between the video and system clock." > > > > In some use cases it might be useful to know exact frame processing > > time, where driver would be filling OUTPUT and CAPTURE value with > > exact monotonic clock values corresponding to a frame processing > start and end time. > > Shouldn't this always be done in memory-to-memory processing? I could > imagine only performance measurements can benefit from other kind of > timestamps. > > We could use different timestamp type to tell the timestamp source > isn't any system clock but an input buffer. I hope that by input buffer you mean the OUTPUT buffer. So the timestamp is copied from the OUTPUT buffer to the corresponding CAPTURE buffer. > > What do you think? Definite yes, if my assumption above is true. I did reply to your RFC suggesting to include this, but got no reply whatsoever. Maybe it got lost somewhere. Best wishes,
Hi Kamil, On Thu, Nov 22, 2012 at 10:32:09AM +0100, Kamil Debski wrote: > Hi Sakari, > > > From: Sakari Ailus [mailto:sakari.ailus@iki.fi] > > Sent: Wednesday, November 21, 2012 8:40 PM > > > > Hi Sylwester and Shaik, > > > > On Mon, Nov 19, 2012 at 11:06:34PM +0100, Sylwester Nawrocki wrote: > > > On 11/07/2012 07:40 AM, Shaik Ameer Basha wrote: > > > >Make gsc-m2m propagate the timestamp field from source to > > destination > > > >buffers > > > > > > We probably need some means for letting know the mem-to-mem drivers > > > and applications whether timestamps are copied from OUTPUT to CAPTURE > > or not. > > > Timestamps at only OUTPUT interface are normally used to control > > > buffer processing time [1]. > > > > > > > > > "struct timeval timestamp > > > > > > For input streams this is the system time (as returned by the > > > gettimeofday() > > > function) when the first data byte was captured. For output streams > > > > Thanks for notifying me; this is going to be dependent on the timestamp > > type. > > > > Also most drivers use the time the buffer is finished rather than when > > the "first data byte was captured", but that's separate I think. > > > > > the data > > > will not be displayed before this time, secondary to the nominal > > frame > > > rate determined by the current video standard in enqueued order. > > > Applications can > > > for example zero this field to display frames as soon as possible. > > > The driver > > > stores the time at which the first data byte was actually sent out in > > > the timestamp field. This permits applications to monitor the drift > > > between the video and system clock." > > > > > > In some use cases it might be useful to know exact frame processing > > > time, where driver would be filling OUTPUT and CAPTURE value with > > > exact monotonic clock values corresponding to a frame processing > > start and end time. > > > > Shouldn't this always be done in memory-to-memory processing? I could > > imagine only performance measurements can benefit from other kind of > > timestamps. > > > > We could use different timestamp type to tell the timestamp source > > isn't any system clock but an input buffer. > > I hope that by input buffer you mean the OUTPUT buffer. > So the timestamp is copied from the OUTPUT buffer to the corresponding > CAPTURE buffer. Correct. Input for the device. I think the OMAP 3 ISP also uses these names; should it, that's another question. OUTPUT and CAPTURE are good. > > > > What do you think? > > Definite yes, if my assumption above is true. I did reply to your RFC > suggesting to include this, but got no reply whatsoever. Maybe it got > lost somewhere. Could be. Anyway, I think we can then say that we agree. :-) Who will write the patch? :-)
Hi Sakari, On 11/21/2012 08:39 PM, Sakari Ailus wrote: > Hi Sylwester and Shaik, > > On Mon, Nov 19, 2012 at 11:06:34PM +0100, Sylwester Nawrocki wrote: >> On 11/07/2012 07:40 AM, Shaik Ameer Basha wrote: >>> Make gsc-m2m propagate the timestamp field from source to destination >>> buffers >> >> We probably need some means for letting know the mem-to-mem drivers and >> applications whether timestamps are copied from OUTPUT to CAPTURE or not. >> Timestamps at only OUTPUT interface are normally used to control buffer >> processing time [1]. >> >> >> "struct timeval timestamp >> >> For input streams this is the system time (as returned by the >> gettimeofday() >> function) when the first data byte was captured. For output streams > > Thanks for notifying me; this is going to be dependent on the timestamp > type. > > Also most drivers use the time the buffer is finished rather than when the > "first data byte was captured", but that's separate I think. Yes, that's an another ambiguity that might need to be resolved. >> the data >> will not be displayed before this time, secondary to the nominal frame rate >> determined by the current video standard in enqueued order. >> Applications can >> for example zero this field to display frames as soon as possible. >> The driver >> stores the time at which the first data byte was actually sent out in the >> timestamp field. This permits applications to monitor the drift between the >> video and system clock." >> >> In some use cases it might be useful to know exact frame processing time, >> where driver would be filling OUTPUT and CAPTURE value with exact monotonic >> clock values corresponding to a frame processing start and end time. > > Shouldn't this always be done in memory-to-memory processing? I could > imagine only performance measurements can benefit from other kind of > timestamps. > > We could use different timestamp type to tell the timestamp source isn't any > system clock but an input buffer. > > What do you think? Yes, it makes sense to me to report with the buffer flag that the source of timestamp is just an OUTPUT buffer. At least this would solve the reporting part of the issue. Oh wait, could applications tell by setting buffer flag what timestamping behaviour they expect from a driver ? I can't see an important use of timestamping m2m buffers at device drivers. Performance measurement can probably be done in user space with sufficient accuracy as well. However, it wouldn't be difficult for drivers to implement multiple time stamping techniques, e.g. OUTPUT -> CAPTURE timestamp copying or getting timestamps from monotonic clock at frame processing beginning and end for OUTPUT and CAPTURE respectively. I believe the buffer flags might be a good solution. -- Regards, Sylwester -- 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
Hi Sylwester, On Thu, Nov 22, 2012 at 10:44:22PM +0100, Sylwester Nawrocki wrote: > >>the data > >>will not be displayed before this time, secondary to the nominal frame rate > >>determined by the current video standard in enqueued order. > >>Applications can > >>for example zero this field to display frames as soon as possible. > >>The driver > >>stores the time at which the first data byte was actually sent out in the > >>timestamp field. This permits applications to monitor the drift between the > >>video and system clock." > >> > >>In some use cases it might be useful to know exact frame processing time, > >>where driver would be filling OUTPUT and CAPTURE value with exact monotonic > >>clock values corresponding to a frame processing start and end time. > > > >Shouldn't this always be done in memory-to-memory processing? I could > >imagine only performance measurements can benefit from other kind of > >timestamps. > > > >We could use different timestamp type to tell the timestamp source isn't any > >system clock but an input buffer. > > > >What do you think? > > Yes, it makes sense to me to report with the buffer flag that the source of > timestamp is just an OUTPUT buffer. At least this would solve the reporting > part of the issue. Oh wait, could applications tell by setting buffer flag > what timestamping behaviour they expect from a driver ? I'd prefer not to. Timestamps not involving use of video nodes or buffers would have no way to choose this. Timestamps only make sense if they're all the same kind of, so you can cmopare them, with possibly some exceptions this could be one of. In memory-to-memory processing we could possibly also force such timestamps, but that'd require making vb2 timestamp source-aware. I certainly have nothing against that: it's been already planned. Handling queryctrl requirest that, and I prefer to avoid involving drivers in it. (Cc Laurent.) > I can't see an important use of timestamping m2m buffers at device drivers. How not important is it then? > Performance measurement can probably be done in user space with sufficient > accuracy as well. However, it wouldn't be difficult for drivers to I agree. > implement > multiple time stamping techniques, e.g. OUTPUT -> CAPTURE timestamp copying > or getting timestamps from monotonic clock at frame processing beginning and > end for OUTPUT and CAPTURE respectively. > > I believe the buffer flags might be a good solution. Have you looked at the monotonic timestamp patches ("[PATCH 0/4] Monotonic timestamps")?
Hi Sakari, On Sunday 25 November 2012 14:09:50 Sakari Ailus wrote: > On Thu, Nov 22, 2012 at 10:44:22PM +0100, Sylwester Nawrocki wrote: > >>> the data will not be displayed before this time, secondary to the > >>> nominal frame rate determined by the current video standard in enqueued > >>> order. Applications can for example zero this field to display frames as > >>> soon as possible. The driver stores the time at which the first data > >>> byte was actually sent out in the timestamp field. This permits > >>> applications to monitor the drift between the video and system clock." > >>> > >>> In some use cases it might be useful to know exact frame processing > >>> time, where driver would be filling OUTPUT and CAPTURE value with exact > >>> monotonic clock values corresponding to a frame processing start and end > >>> time. > >> > >> Shouldn't this always be done in memory-to-memory processing? I could > >> imagine only performance measurements can benefit from other kind of > >> timestamps. > >> > >> We could use different timestamp type to tell the timestamp source isn't > >> any system clock but an input buffer. > >> > >> What do you think? > > > > Yes, it makes sense to me to report with the buffer flag that the source > > of timestamp is just an OUTPUT buffer. At least this would solve the > > reporting part of the issue. Oh wait, could applications tell by setting > > buffer flag what timestamping behaviour they expect from a driver ? > > I'd prefer not to. Timestamps not involving use of video nodes or buffers > would have no way to choose this. Timestamps only make sense if they're all > the same kind of, so you can cmopare them, with possibly some exceptions > this could be one of. I agree, I'd rather select the timestamp type without involving the buffer. If we used buffer flags to select the timestamp type we would essentially delay timestamp type selection until QBUF time, which could be too late for some devices. > In memory-to-memory processing we could possibly also force such timestamps, > but that'd require making vb2 timestamp source-aware. I certainly have > nothing against that: it's been already planned. > > Handling queryctrl requirest that, and I prefer to avoid involving drivers > in it. > > (Cc Laurent.) > > > I can't see an important use of timestamping m2m buffers at device > > drivers. > > How not important is it then? > > > Performance measurement can probably be done in user space with sufficient > > accuracy as well. However, it wouldn't be difficult for drivers to > > I agree. > > > implement multiple time stamping techniques, e.g. OUTPUT -> CAPTURE > > timestamp copying or getting timestamps from monotonic clock at frame > > processing beginning and end for OUTPUT and CAPTURE respectively. > > > > I believe the buffer flags might be a good solution. > > Have you looked at the monotonic timestamp patches ("[PATCH 0/4] Monotonic > timestamps")?
diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c index 047f0f0..1139276 100644 --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c @@ -99,22 +99,27 @@ static void gsc_m2m_job_abort(void *priv) gsc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR); } -static int gsc_fill_addr(struct gsc_ctx *ctx) +static int gsc_get_bufs(struct gsc_ctx *ctx) { struct gsc_frame *s_frame, *d_frame; - struct vb2_buffer *vb = NULL; + struct vb2_buffer *src_vb, *dst_vb; int ret; s_frame = &ctx->s_frame; d_frame = &ctx->d_frame; - vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx); - ret = gsc_prepare_addr(ctx, vb, s_frame, &s_frame->addr); + src_vb = v4l2_m2m_next_src_buf(ctx->m2m_ctx); + ret = gsc_prepare_addr(ctx, src_vb, s_frame, &s_frame->addr); + + dst_vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx); + ret |= gsc_prepare_addr(ctx, dst_vb, d_frame, &d_frame->addr); if (ret) return ret; - vb = v4l2_m2m_next_dst_buf(ctx->m2m_ctx); - return gsc_prepare_addr(ctx, vb, d_frame, &d_frame->addr); + memcpy(&dst_vb->v4l2_buf.timestamp, &src_vb->v4l2_buf.timestamp, + sizeof(dst_vb->v4l2_buf.timestamp)); + + return 0; } static void gsc_m2m_device_run(void *priv) @@ -148,7 +153,7 @@ static void gsc_m2m_device_run(void *priv) goto put_device; } - ret = gsc_fill_addr(ctx); + ret = gsc_get_bufs(ctx); if (ret) { pr_err("Wrong address"); goto put_device;