[PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Shaik Ameer Basha]

Make gsc-m2m propagate the timestamp field from source to destination
buffers

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);
+
+	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;
-- 
1.7.0.4

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Sylwester Nawrocki]

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

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Sakari Ailus]

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?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

RE: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Kamil Debski]

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,
-- 
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

['Sakari Ailus']

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? :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Sylwester Nawrocki]

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

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Sakari Ailus]

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")?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH] [media] exynos-gsc: propagate timestamps from src to dst buffers

[Laurent Pinchart]

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")?

-- 
Regards,

Laurent Pinchart