[PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

From: Jasmin Jessich <jasmin@anw.at>

Commit 828ee8c71950 ("media: uvcvideo: Use ktime_t for timestamps")
changed to use ktime_t for timestamps. Older Kernels use a struct for
ktime_t, which requires the conversion function ktime_to_ns to be used on
some places. With this patch it will compile now also for older Kernel
versions.

Signed-off-by: Jasmin Jessich <jasmin@anw.at>
---
 drivers/media/usb/uvc/uvc_video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 5441553..1670aeb 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1009,7 +1009,7 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
 
 		buf->buf.field = V4L2_FIELD_NONE;
 		buf->buf.sequence = stream->sequence;
-		buf->buf.vb2_buf.timestamp = uvc_video_get_time();
+		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
 
 		/* TODO: Handle PTS and SCR. */
 		buf->state = UVC_BUF_STATE_ACTIVE;
@@ -1191,7 +1191,8 @@ static void uvc_video_decode_meta(struct uvc_streaming *stream,
 
 	uvc_trace(UVC_TRACE_FRAME,
 		  "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame SOF %u\n",
-		  __func__, time, meta->sof, meta->length, meta->flags,
+		  __func__, ktime_to_ns(time), meta->sof, meta->length,
+		  meta->flags,
 		  has_pts ? *(u32 *)meta->buf : 0,
 		  has_scr ? *(u32 *)scr : 0,
 		  has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
-- 
2.7.4

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Hi!

I tested this patch (compile only) on Kernel 4.4.
For 3.13 there is something else not working, so I (or Hans) needs to have a
look on that.

BR,
   Jasmin

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Arnd Bergmann]

On Sun, Jan 14, 2018 at 11:21 AM, Jasmin J. <jasmin@anw.at> wrote:
> From: Jasmin Jessich <jasmin@anw.at>
>
> Commit 828ee8c71950 ("media: uvcvideo: Use ktime_t for timestamps")
> changed to use ktime_t for timestamps. Older Kernels use a struct for
> ktime_t, which requires the conversion function ktime_to_ns to be used on
> some places. With this patch it will compile now also for older Kernel
> versions.
>
> Signed-off-by: Jasmin Jessich <jasmin@anw.at>

Looks good to me,

Acked-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Ping!
It is required to compile for Kernels older 4.10.

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Hello Mauro!

> Ping!
> It is required to compile for Kernels older 4.10.

It would be nice to get this merged, so that we can see if older Kernels
will compile again.

BR,
   Jasmin

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Laurent Pinchart]

Hi Jasmin,

Thank you for the patch.

On Sunday, 14 January 2018 12:21:43 EET Jasmin J. wrote:
> From: Jasmin Jessich <jasmin@anw.at>
> 
> Commit 828ee8c71950 ("media: uvcvideo: Use ktime_t for timestamps")
> changed to use ktime_t for timestamps. Older Kernels use a struct for
> ktime_t, which requires the conversion function ktime_to_ns to be used on
> some places. With this patch it will compile now also for older Kernel
> versions.
> 
> Signed-off-by: Jasmin Jessich <jasmin@anw.at>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and taken into my tree for v4.17.

> ---
>  drivers/media/usb/uvc/uvc_video.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 5441553..1670aeb 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1009,7 +1009,7 @@ static int uvc_video_decode_start(struct uvc_streaming
> *stream,
> 
>  		buf->buf.field = V4L2_FIELD_NONE;
>  		buf->buf.sequence = stream->sequence;
> -		buf->buf.vb2_buf.timestamp = uvc_video_get_time();
> +		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
> 
>  		/* TODO: Handle PTS and SCR. */
>  		buf->state = UVC_BUF_STATE_ACTIVE;
> @@ -1191,7 +1191,8 @@ static void uvc_video_decode_meta(struct uvc_streaming
> *stream,
> 
>  	uvc_trace(UVC_TRACE_FRAME,
>  		  "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame
> SOF %u\n", -		  __func__, time, meta->sof, meta->length, meta->flags,
> +		  __func__, ktime_to_ns(time), meta->sof, meta->length,
> +		  meta->flags,
>  		  has_pts ? *(u32 *)meta->buf : 0,
>  		  has_scr ? *(u32 *)scr : 0,
>  		  has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);


-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Hi Laurent!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
THX!
Don't forget the "Acked-by: Arnd Bergmann <arnd@arndb.de>" (see Patchwork:
https://patchwork.linuxtv.org/patch/46464 ).

> and taken into my tree for v4.17.
When will this merged to the media-tree trunk?
In another month or earlier?

This issue was overlooked when merging the change from Arnd in the first place.
This broke the Kernel build for older Kernels more than two months ago! I fixed
that in my holidays expecting this gets merged soon and now the build is still
broken because of this problem.

In the past Mauro merged those simple fixes soon and now it seems nobody
cares about building for older Kernels (it's broken for more than two months
now!). I mostly try to fix such issues in a short time frame (even on
vacation), but then it gets lost ... . Sorry, but this is frustrating!

We don't talk about a nice to have fix but a essential fix to get the media
build system working again. Such patches need to get merged as early as
possible in my opinion, especially when someone else sent already an "Acked-by"
(THX to Arnd).

I could have made this as a patch in the Build system also, but this would be
the wrong place, but then Hans would have merged it already and I could look
into the other build problems.

BR,
   Jasmin

*************************************************************************

On 02/02/2018 12:32 PM, Laurent Pinchart wrote:
> Hi Jasmin,
> 
> Thank you for the patch.
> 
> On Sunday, 14 January 2018 12:21:43 EET Jasmin J. wrote:
>> From: Jasmin Jessich <jasmin@anw.at>
>>
>> Commit 828ee8c71950 ("media: uvcvideo: Use ktime_t for timestamps")
>> changed to use ktime_t for timestamps. Older Kernels use a struct for
>> ktime_t, which requires the conversion function ktime_to_ns to be used on
>> some places. With this patch it will compile now also for older Kernel
>> versions.
>>
>> Signed-off-by: Jasmin Jessich <jasmin@anw.at>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and taken into my tree for v4.17.
> 
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 5441553..1670aeb 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1009,7 +1009,7 @@ static int uvc_video_decode_start(struct uvc_streaming
>> *stream,
>>
>>  		buf->buf.field = V4L2_FIELD_NONE;
>>  		buf->buf.sequence = stream->sequence;
>> -		buf->buf.vb2_buf.timestamp = uvc_video_get_time();
>> +		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
>>
>>  		/* TODO: Handle PTS and SCR. */
>>  		buf->state = UVC_BUF_STATE_ACTIVE;
>> @@ -1191,7 +1191,8 @@ static void uvc_video_decode_meta(struct uvc_streaming
>> *stream,
>>
>>  	uvc_trace(UVC_TRACE_FRAME,
>>  		  "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame
>> SOF %u\n", -		  __func__, time, meta->sof, meta->length, meta->flags,
>> +		  __func__, ktime_to_ns(time), meta->sof, meta->length,
>> +		  meta->flags,
>>  		  has_pts ? *(u32 *)meta->buf : 0,
>>  		  has_scr ? *(u32 *)scr : 0,
>>  		  has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
> 
> 

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Hans Verkuil]

Hi Jasmin,

On 02/04/2018 11:37 AM, Jasmin J. wrote:
> Hi Laurent!
> 
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> THX!
> Don't forget the "Acked-by: Arnd Bergmann <arnd@arndb.de>" (see Patchwork:
> https://patchwork.linuxtv.org/patch/46464 ).
> 
>> and taken into my tree for v4.17.
> When will this merged to the media-tree trunk?

We're waiting for the end of the 4.16 merge window. So until it closes no
4.17 pull requests will be merged.

> In another month or earlier?
> 
> This issue was overlooked when merging the change from Arnd in the first place.
> This broke the Kernel build for older Kernels more than two months ago! I fixed
> that in my holidays expecting this gets merged soon and now the build is still
> broken because of this problem.
> 
> In the past Mauro merged those simple fixes soon and now it seems nobody
> cares about building for older Kernels (it's broken for more than two months
> now!). I mostly try to fix such issues in a short time frame (even on
> vacation), but then it gets lost ... . Sorry, but this is frustrating!

Both Mauro (a USB regression) and myself (a nasty v4l2-compat-ioctl32.c bug fix)
have been very busy lately leaving us little time for other things.

For me that meant that I simply did (and still don't) have time to look at
fixing the media_build.

The media_build is also broken worse than usual AFAIK, so it isn't a quick fix
I can do in between other jobs.

> 
> We don't talk about a nice to have fix but a essential fix to get the media
> build system working again. Such patches need to get merged as early as
> possible in my opinion, especially when someone else sent already an "Acked-by"
> (THX to Arnd).
> 
> I could have made this as a patch in the Build system also, but this would be
> the wrong place, but then Hans would have merged it already and I could look
> into the other build problems.

I'm OK to take a patch and then revert it later when the real fix has been merged.

BTW, if you are interested then I would be more than happy to hand over media_build
maintenance to you. I can't give it the attention it deserves, I am well aware of
that. And I don't see that changing in the foreseeable future.

Regards,

	Hans

> 
> BR,
>    Jasmin
> 
> *************************************************************************
> 
> On 02/02/2018 12:32 PM, Laurent Pinchart wrote:
>> Hi Jasmin,
>>
>> Thank you for the patch.
>>
>> On Sunday, 14 January 2018 12:21:43 EET Jasmin J. wrote:
>>> From: Jasmin Jessich <jasmin@anw.at>
>>>
>>> Commit 828ee8c71950 ("media: uvcvideo: Use ktime_t for timestamps")
>>> changed to use ktime_t for timestamps. Older Kernels use a struct for
>>> ktime_t, which requires the conversion function ktime_to_ns to be used on
>>> some places. With this patch it will compile now also for older Kernel
>>> versions.
>>>
>>> Signed-off-by: Jasmin Jessich <jasmin@anw.at>
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> and taken into my tree for v4.17.
>>
>>> ---
>>>  drivers/media/usb/uvc/uvc_video.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>>> b/drivers/media/usb/uvc/uvc_video.c index 5441553..1670aeb 100644
>>> --- a/drivers/media/usb/uvc/uvc_video.c
>>> +++ b/drivers/media/usb/uvc/uvc_video.c
>>> @@ -1009,7 +1009,7 @@ static int uvc_video_decode_start(struct uvc_streaming
>>> *stream,
>>>
>>>  		buf->buf.field = V4L2_FIELD_NONE;
>>>  		buf->buf.sequence = stream->sequence;
>>> -		buf->buf.vb2_buf.timestamp = uvc_video_get_time();
>>> +		buf->buf.vb2_buf.timestamp = ktime_to_ns(uvc_video_get_time());
>>>
>>>  		/* TODO: Handle PTS and SCR. */
>>>  		buf->state = UVC_BUF_STATE_ACTIVE;
>>> @@ -1191,7 +1191,8 @@ static void uvc_video_decode_meta(struct uvc_streaming
>>> *stream,
>>>
>>>  	uvc_trace(UVC_TRACE_FRAME,
>>>  		  "%s(): t-sys %lluns, SOF %u, len %u, flags 0x%x, PTS %u, STC %u frame
>>> SOF %u\n", -		  __func__, time, meta->sof, meta->length, meta->flags,
>>> +		  __func__, ktime_to_ns(time), meta->sof, meta->length,
>>> +		  meta->flags,
>>>  		  has_pts ? *(u32 *)meta->buf : 0,
>>>  		  has_scr ? *(u32 *)scr : 0,
>>>  		  has_scr ? *(u32 *)(scr + 4) & 0x7ff : 0);
>>
>>

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Hi!

> We're waiting for the end of the 4.16 merge window.
Ahh, this is the reason for the delay.

> For me that meant that I simply did (and still don't) have time to look at
> fixing the media_build.
No problem, I do it when I have time ;)

> The media_build is also broken worse than usual AFAIK, so it isn't a quick fix
> I can do in between other jobs.
I will look into that, when I fixed the current problem in uvcvideo
temporarily.

> I'm OK to take a patch and then revert it later when the real fix has been merged.
So ... just sent a patch for media_build.

> BTW, if you are interested then I would be more than happy to hand over media_build
> maintenance to you.
I am not sure if I know enough to completely do this. But I can be a substitute
and continue what I did the last month. This means I would prefer that you keep
the head of media_build and I do the work.
But I would need write access to the media_build GIT tree. Please contact me
off list for details.

BR,
   Jasmin

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Laurent Pinchart]

Hi Jasmin,

On Sunday, 4 February 2018 12:37:08 EET Jasmin J. wrote:
> Hi Laurent!
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> THX!
> Don't forget the "Acked-by: Arnd Bergmann <arnd@arndb.de>" (see Patchwork:
> https://patchwork.linuxtv.org/patch/46464 ).

Sure.

> > and taken into my tree for v4.17.
> 
> When will this merged to the media-tree trunk?
> In another month or earlier?

As Hans explained, the v4.16 merge window is open. It should close in a week, 
and I'll then send a pull request to Mauro.

> This issue was overlooked when merging the change from Arnd in the first
> place. This broke the Kernel build for older Kernels more than two months
> ago! I fixed that in my holidays expecting this gets merged soon and now
> the build is still broken because of this problem.

I had missed this patch as I wasn't CC'ed, until you pinged me directly. 
Please try to CC me when submitting uvcvideo patches in the future, otherwise 
there's a high chance I won't see them.

> In the past Mauro merged those simple fixes soon and now it seems nobody
> cares about building for older Kernels (it's broken for more than two months
> now!). I mostly try to fix such issues in a short time frame (even on
> vacation), but then it gets lost ... . Sorry, but this is frustrating!
> 
> We don't talk about a nice to have fix but a essential fix to get the media
> build system working again. Such patches need to get merged as early as
> possible in my opinion, especially when someone else sent already an
> "Acked-by" (THX to Arnd).
> 
> I could have made this as a patch in the Build system also, but this would
> be the wrong place, but then Hans would have merged it already and I could
> look into the other build problems.

Strictly speaking, building the media subsystem on older kernels is a job of 
the media build system. In general I would thus ask for the fix to be merged 
in media-build.git. In this specific case, as the mainline code uses both u64 
and ktime_t types, I'm fine with merging your patch to use explicit conversion 
functions in mainline even if the two types are now equivalent. However, as 
this doesn't fix a bug in the mainline kernel, I don't think this patch is a 
candidate for stable releases, and should thus get merged in v4.17. It can 
also be included in media-build.git in order to build kernels that currently 
fail.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Fixed ktime_t to ns conversion

[Jasmin J.]

Hi!

> Strictly speaking, building the media subsystem on older kernels is a job of 
> the media build system.
Yes I agree.

> In general I would thus ask for the fix to be merged in media-build.git.
Which I do mostly, but in this case it is a coding error in mainline. 

> In this specific case, as the mainline code uses both u64 and ktime_t types,
> I'm fine with merging your patch to use explicit conversion functions in
> mainline even if the two types are now equivalent.
We had this already with other drivers and changes for ktime_t. In fact you
should always use the macro accessors. We don't know what will be changed in
the future with ktime_t, so using the macros -- on all places -- is save for
the future also. And using the macro accessors is automatically backward
compatible also.

> However, as this doesn't fix a bug in the mainline kernel, I don't think this
> patch is a candidate for stable releases, and should thus get merged in v4.17.
I am fine with this.

> It can also be included in media-build.git in order to build kernels that
> currently fail.
I just sent a temporary fix for media-build.
media-build works always with the latest media.git. So once it is merged, the
temporary fix can be removed again.

BR,
   Jasmin