| Message ID | 1515925303-5160-1-git-send-email-jasmin@anw.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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>
Ping! It is required to compile for Kernels older 4.10.
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
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);
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); > >
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); >> >>
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.
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
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
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);