[02:24] <gnurou> pinchartl: hverkuil: so right now we are implementing request API support in Chromium. We have a bit of a chicken-and-egg problem to get a kernel driver that also supports it, but are making progress towards that
[02:25] <gnurou> my testing was essentially limited to my own kernel implementation of the request API if you remember. I have done a bit of testing with the new one and vivid, but nothing too extensive.
[02:25] <gnurou> however since the user-facing API has seen little change I expect little trouble there
[02:26] <gnurou> re: the possibility of the API evolving, it seems to me that for stateless codecs we have little risk for that
[02:27] <gnurou> and for other use-cases, we can always extend/tune the use of the API. The documentation will also be separated anyway, we have "request API for stateless codecs" and will have "request API for camera pipeline", so this should not be too confusing if it happens
[02:29] <gnurou> but being conservative until we have a couple working drivers doesn't hurt, so I'm not against making the request API depend on STAGING, if that means we can make incompatible changes to the userspace interface
[04:59] <pinchartl> gnurou: I'd like to avoid having two different request APIs. how the API is exercised for different classes of devices can of course differ, but if we end up having to implement different ioctls it will be painful
[04:59] <pinchartl> which driver are you targetting now, if that's not confidential ?
[05:45] <gnurou> yeah, I am not suggesting different request API - just that behavior may show some subtle differences in practice
[05:45] <gnurou> I'm not sure whether I can reveal the driver in question at the moment
[06:04] <pinchartl> gnurou: no worries. we'll all watch the git logs :-)
[07:15] <tfiga> gnurou: pinchartl: I think we can reveal, but the driver code is in a very early state so not very useful for the discussion here :)
[07:16] <tfiga> Basically a new firmware/hardware variant of mtk-vcodec would have stateless decode
[07:59] <hverkuil> Question for encoder HW experts: when encoding a frame that is not a multiple of the macroblock size extra padding is typically required by the HW.
[07:59] <pinchartl> hverkuil: is that a question ? :-)
[08:00] <hverkuil> But does the HW encoder actually encode the data in the padding, or is the hardware aware of this and duplicates the last line/column or use some fixed color value for the encoding of macroblocks at the edges?
[08:01] <hverkuil> tfiga: ndufresne: mripard: ^^^^
[08:04] <pinchartl> now that's a question
[08:06] <hverkuil> pinchartl: you're welcome!
[08:22] *** prabhakarlad has left 
[08:30] *** prabhakarlad has left 
[10:21] <hverkuil> paulk-leonov: ping
[10:22] <paulk-leonov> hverkuil, pong
[10:22] <hverkuil> I've just posted v4 for the tag patch series.
[10:22] <hverkuil> My plan is to make a pull request for that next week.
[10:22] <paulk-leonov> hverkuil, sounds good to me!
[10:23] <hverkuil> The thing that worries me a bit is the last patch for the cedrus driver.
[10:23] <hverkuil> (Oops, I just realized I forgot to update the control documentation! So there will be an additional patch)
[10:24] <hverkuil> There were several other things you wanted to change there if I remember correctly.
[10:24] <hverkuil> Do you think you have time for that?
[10:25] <hverkuil> I'd rather prefer getting everything stable for 4.21 for those controls instead of having to change it again in 4.2..
[10:25] <hverkuil> 4.22
[10:28] <paulk-leonov> yes I understand that it would be best to have everything stabilized for 4.21
[10:28] <paulk-leonov> but it will be hard to fit such a big change in the time we have
[10:29] <paulk-leonov> what I had in mind was changing some fields to flags but most importantly, moving to per-slice controls
[10:34] <hverkuil> paulk-leonov: posted follow-up doc fix. Please review!
[10:35] <hverkuil> I'm a bit unhappy with having an unstable API for this driver in include/uapi.
[10:36] <hverkuil> I wonder if it isn't better to move the definitions of these controls to staging.
[10:37] <paulk-leonov> hverkuil, I would be happy with that too, yes
[10:39] <paulk-leonov> hverkuil, so I'm told that I can get more time available to rework the mpeg-2 controls if the h.264 and h.265 controls also make it to 4.21
[10:41] <paulk-leonov> but I really like the idea of moving the definitions to staging until we have feedback and a second driver using them
[10:45] <syoung> I've started looking at smatch errors and I'm having troubling understanding them, if anyone can help that would be appreciated :)
[10:45] <syoung> drivers/media/rc/lirc_dev.c:250 ir_lirc_transmit_ir() error: double lock 'mutex:&dev->lock'
[10:45] <syoung> drivers/media/rc/lirc_dev.c:398 ir_lirc_ioctl() error: double lock 'mutex:&dev->lock'
[10:45] <syoung> drivers/media/rc/lirc_dev.c:660 ir_lirc_read_mode2() error: double lock 'mutex:&rcdev->lock'
[10:45] <syoung> drivers/media/rc/lirc_dev.c:699 ir_lirc_read_scancode() error: double lock 'mutex:&rcdev->lock'
[10:46] <hverkuil> I don't think H264/5 will make 4.21.  It looks like 4.20 will close fairly soon and with Christmas coming up it is unlikely to get in.
[10:46] <syoung> so I guess this means that mutex_lock() might be called by a thread which already holds the lock. Is that true?
[10:46] <paulk-leonov> hverkuil, alright, noted
[10:46] <hverkuil> I'm looking at creating a media/drv-intf/cedrus.h header.
[10:46] <paulk-leonov> hverkuil, thanks
[10:47] <paulk-leonov> I'll do my best to rework MPEG-2 (on my spare time, I guess)
[10:49] <sailus> mchehab: A quick question: do you have some idea when you'd handle the last pull requests for 4.21?
[10:52] <mchehab> sailus: I handled today all the pending ones
[10:52] <mchehab> handling right now individual patches
[10:54] <sailus> I'd have more sensor driver improvements.
[10:54] <hverkuil> mchehab: question: the two new controls that the cedrus driver added (V4L2_CID_MPEG_VIDEO_MPEG2_SLICE_PARAMS and V4L2_CID_MPEG_VIDEO_MPEG2_QUANTIZATION) will have to change some more.
[10:55] <hverkuil> I really don't like it that in 4.20 these are part of the public API. I want to make a patch for 4.20 to stop exposing those controls in the uAPI.
[10:56] <hverkuil> Two options: move it all to a include/media/drv-intf/cedrus.h header, or add #ifndef __KERNEL__ around the bits in the public headers relating to those cedrus controls.
[10:56] <sailus> mchehab: Sent.
[10:56] <sailus> And... there's the ImgU driver as well.
[10:56] <hverkuil> Either works, the second option will have the least impact.
[10:56] <sailus> I'll be back after a moment...
[10:56] <mchehab> sailus: send what you have... I'll try to handle
[10:57] <mchehab> please notice that I'm travelling on Thr/Fri
[10:57] <mchehab> won't be able to apply patches while there
[10:57] <sailus> Ack.
[10:57] <mchehab> so, I guess today is the last day
[10:57] <sailus> Hmm.
[10:57] <mchehab> for our merge window
[10:57] <sailus> Not even a staging driver? :-)
[10:57] <sailus> Well, I guess we could see how things stand on Monday. That driver isn't quiet yet ready today anyway.
[10:58] <sailus> The timing for 4.21 seems ambitious. There could be changes there, too.
[10:58] <mchehab> yes
[10:58] <mchehab> hverkuil: yeah, let's fix it
[10:59] <mchehab> and make a Kconfig option to disable request API as a hole
[10:59] <mchehab> (if we didn't add yet... don't remember)
[10:59] <sailus> mchehab: I'll send a patch for that in... an hour or so.
[10:59] <mchehab> ok
[10:59] <hverkuil> mchehab: which approach do you prefer regarding the cedrus controls? ifndef __KERNEL__ or move it all to a cedrus header?
[10:59] <mchehab> making a core feature depending on staging is a crap idea, btw
[10:59] <sailus> I'll see if it could fit reasonably under the staging menu.
[11:00] <mchehab> I'm pretty sure Greg will complain
[11:00] <mchehab> hverkuil: move to cedrus header
[11:00] <hverkuil> ok
[11:00] <mchehab> seems cleaner
[11:01] <mchehab> sailus: no, please don't mess with core stuff inside staging
[11:01] <sailus> mchehab: Under the media then.
[11:01] <mchehab> something like we did for media controller DVB (and other similar cases, like CEC) seems ok
[11:01] <mchehab> say that it is (EXPERIMENTAL)
[11:02] <mchehab> and add warnings at the Kconfig saying that the api may change
[11:02] <mchehab> to be frank, while it is just CromeOS and cedrus driver using it, I don't see much issues
[11:03] <mchehab> but better to be safer adding the needed warnings
[11:06] <hverkuil> mchehab: should these MPEG2 controls also be removed from the documentation?
[11:07] <hverkuil> Or just add a note that they are experimental and will change?
[11:07] <hverkuil> I have no preference one way or another (well, keeping them is less work :-) )
[11:08] <mchehab> hverkuil: good question...
[11:08] <mchehab> I guess that, if you don't remove, you'll start getting warnings
[11:08] <mchehab> I would prefer to keep them with a notice
[11:09] <mchehab> if it won't cause warnings
[11:09] <hverkuil> OK. Let me see what I can do.
[11:09] <hverkuil> lunch first, though :-)
[11:09] <mchehab> :-)
[11:10] <mchehab> it is 9am here... I still have the full day to handle those stuff
[11:10] <mchehab> (If not sidetracked from some travel-related issue)
[11:27] <mchehab> sailus: I'll handle your PR in a few
[11:28] <mchehab> hopefully, it will mean less patches for me to read twice
[11:28] <mchehab> there are lots of patches that were already applied but weren't marked as such at patchwork
[11:46] <sailus> mchehab: I'll check whether there are some left not marked as accepted there.
[11:47] <sailus> Thanks for the info.
[11:47] <mchehab> thanks
[11:47] <mchehab> btw, I delegated some stuff to you
[11:48] <mchehab> (like the ipu3 patchset and other sensor-related stuff)
[11:52] <hverkuil> pushed a v4.21 PR: just one patch, cleaning up highly confusing variable names in vicodec. Will help a lot with the vicodec work that Dafna is doing.
[11:53] <mchehab> is anyone reviewing this series?
[11:53] <mchehab> usb: pwc: Introduce TRACE_EVENTs for pwc_isoc_handler
[11:54] <mchehab> there are already 6 versions on it... only Steven Rostedt commented
[11:54] <mchehab> hverkuil: I guess it makes sense to delegate to you
[11:56] <hverkuil> It's part of the 'media: usb: pwc: Don't use coherent DMA buffers for ISO transfer' series, and I thought pinchartl was handling that. Esp. since I have little to no knowledge about ISO transfers.
[11:56] <hverkuil> pinchartl has definitely been involved in that series.
[11:56] <pinchartl> hverkuil: haven't I acked it ?
[11:56] <pinchartl> or at least said I was fine with it ?
[11:57] <mchehab> pinchartl: I'm not seeing any replies from you on this specific tracing series
[11:58] <mchehab> at least on patch 1/2
[11:58] <mchehab> you did comment patch 2/2
[12:01] <pinchartl> I have no comment on patch 1/2, I have little experience with implementing trace events
[12:10] <hverkuil> paulk-leonov: please review the "[PATCH for v4.20 0/2] cedrus: move MPEG controls out of the uAPI" series asap. If you are OK with it, then I'll make a pull request for it.
[12:14] <hverkuil> I think this is a good idea regardless, as it will make it easier to proceed with H264/5 using the same method.
[12:24] <sailus> mchehab: Sent.
[12:24] <sailus> hverkuil, pinchartl: ^
[13:02] <hverkuil> mchehab: posted 4.20 PR.
[13:05] <hverkuil> paulk-leonov: mripard: looking at the cedrus H.264 code and it is really very clean. I'm impressed.
[13:05] <hverkuil> It's the uAPI that will take the most time to sort out.
[13:07] <hverkuil> I think the same approach should be used as with the mpeg2 controls: make a h264-ctrls.h header in include/media for the new controls.
[13:08] <hverkuil> That makes it much easier to merge the code while the uAPI is not stable yet.
[13:09] <paulk-leonov> hverkuil, I'll definitely do that for the version of h265 support yes
[13:32] <ndufresne> hverkuil, yes, it will encode the macroblock padding
[13:33] <ndufresne> the higher quality encoder will duplicate the last line/column to avoid leaking green into the visible data
[13:33] <hverkuil> but for el cheapo encoders userspace has to do that (or at least fill with some solid color), right?
[13:34] <ndufresne> we could, but we don't, in general they don't leak on odd resolutions because they don't have any fancy filters that would cause that
[15:03] <hverkuil> ezequielg: reviewed v11 of the jpeg driver. If you are really quick and post a v12 in the next hour, then I'll make a pull request in time for 4.21. Otherwise it'll get in early for 4.22.
[15:09] <ezequielg> hverkuil: i'll do it
[15:11] <hverkuil> ezequielg: great. I'll go home and when I've arrived I expect a shiny new v12 :-)  I'd likely accepted v11 if it wasn't for the broken TIMECODE flag check.
[16:11] <ezequielg> hverkuil: v12 sent
[16:11] <ezequielg> i'm working on the video decoding, so we'll hopefully see more changes soon to this driver :-)
[16:20] <sailus> mchehab: Patchwork states for the pulled patches fixed.
[16:20] <sailus> I guess what was left there was my changes to make a patch compile.
[16:20] <mchehab> sailus: thanks!
[16:20] <sailus> There are so many patches to review still. :'-I
[16:20] <mchehab> yeah, my script update automatically when the md5sum of the diff is identical
[16:21] <mchehab> any change (even on context lines) will make it skip
[16:22] <sailus> Ack.
[16:30] <hverkuil> ezequielg: posted pull request
[16:35] <ezequielg> hverkuil: thanks!
[16:44] *** benjiG has left 
[16:50] <sailus> mchehab: Do you have an opinion on the Request API Kconfig patch?
[16:50] <sailus> There are a few random driver fixes around, I could create a new pull request if you'd have time to pick it still.
[16:51] <sailus> The others are not urgent IMO.
[16:51] <sailus> hverkuil acked the patch.
[17:16] <mchehab> hmm... wouldn't it generate warnings?
[17:16] <mchehab> I don't like this hunk:
[17:16] <mchehab> +#ifndef CONFIG_MEDIA_CONTROLLER_REQUEST_API
[17:16] <mchehab>  	if (!mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue)
[17:16] <mchehab> +#endif
[17:16] <sailus> That there would be dead code?
[17:16] <sailus> Or what?
[17:17] <mchehab> maybe, maybe not, but I guess GCC will warn
[17:17] <sailus> I didn't see any here.
[17:17] <mchehab> aldo, it is very ugly do to:
[17:17] <sailus> But I didn't use W=1.
[17:17] <mchehab> +#ifndef CONFIG_MEDIA_CONTROLLER_REQUEST_API
[17:17] <mchehab>  	if (!mdev->ops || !mdev->ops->req_validate || !mdev->ops->req_queue)
[17:17] <mchehab> +#endif
[17:17] <mchehab>  		return -ENOTTY;
[17:17] <sailus> Perhaps. The alternative might not be more readable though.
[17:17] <mchehab> I would prefer to put the entire code under the #ifdef
[17:18] <mchehab> and an #else
[17:18] <sailus> That's fine.
[17:18] <sailus> I can send v2.
[17:18] <mchehab> with just return -ENOTTY
[17:18] <mchehab> OK, thanks
[17:20] <sailus> Actually I think there was a bug there. An extra "n".
[17:20] <sailus> I'll fix that at the same time. :-)
[17:22] <mchehab> ok
[17:23] <mchehab> please test, with W=1 with both it enabled and disabled
[17:24] <sailus> mchehab: I've resent the patch.
[17:24] <sailus> Testing with W=1 is not feasible for every patch.
[17:24] <sailus> It'll take hours as everything is recompiled.
[17:24] <mchehab> huh?
[17:24] <mchehab> touch <file>
[17:24] <mchehab> make M=1
[17:24] <sailus> Plus the possible warnings will drown in the sea of other warnings.
[17:25] <mchehab> ops W=1
[17:25] <mchehab> i actually do make M=drivers/media W=1
[17:25] <mchehab> there are no other W=1 warnings on media
[17:25] <mchehab> there aren't any other W=1 warnings on media
[17:26] <sailus> I can do that but it takes longer than I have time today.
[17:26] <sailus> (Another sign we'd need automated testing.)
[17:26] <sailus> Which makes having that all the more important.
[17:27] <mchehab> adding W=1 doesn't make it recompile anything
[17:27] <mchehab> it will just add new compilation flags to the new stuff to be built
[17:27] <sailus> Hmm. Perhaps something is different in your environment.
[17:28] <sailus> I'm compiling for armel btw.
[17:28] <mchehab> if it is rebuilding everything, you probably have a serious stuff somewhere
[17:28] <mchehab> I usually build for x86, but it shouldn't actually matter
[17:29] <mchehab> W=1 just changes the gcc command line. nothing else
[17:29] <mchehab> it won't change the dependency chain
[17:29] <mchehab> it shouldn't be forcing rebuilds
[17:30] <mchehab> maybe your arm building script is doing a make clean
[17:32] <sailus> M=drivers/media helps a lot.
[17:32] <sailus> But still, everything under drivers/media is still compiled.
[17:33] <sailus> I don't have a build script.
[17:33] <sailus> And make clean would actually remove the objects independently of W=1.
[17:33] <mchehab> there's no object dependency of W=1
[17:34] <mchehab> W=1 doesn't generate any dependency, as it does exactly the same build
[17:34] <mchehab> the only thing it does is that it adds some -Wwarning lines
[17:34] <mchehab> to gcc call
[17:36] <sailus> I'm just telling what is the effect, I don't know what happens there.
[17:37] <sailus> Anyway, I need to leave the office.
[17:37] <sailus> Btw. tomorrow is a public holiday here, I probably won't be in the weekly meeting either.
[17:38] <mchehab> ok
[17:38] <mchehab> btw, just canceled my trip
[17:38] <sailus> mchehab: Have a nice evening!
[17:38] <sailus> Ack.
[17:38] <mchehab> I should be able to be at the meeting
[18:13] <mchehab> hverkuil: applied, thanks!
[18:13] <mchehab> also applied sailus patch with the new Kconfig option for request api
[18:26] <mchehab> I suspect that we'll have problems with the 4.20 fixes branch
[18:27] <mchehab> due to that renaming patch
[18:28] <mchehab> hmm... no it is only at the 4.21 branch
[18:28] <mchehab> b9bbbbfef991 media: vicodec: Change variable names
[18:29] <mchehab> I had to solve a trivial conflict there between 4.20 and 4.21
[18:29] <mchehab> -                       p = memchr(p, magic[ctx->comp_magic_cnt], sz);
[18:29] <mchehab> +                       p = memchr(p, magic[ctx->comp_magic_cnt],
[18:29] <mchehab>  -                                 p_out + sz - p);
[18:29] <mchehab> ++                                 p_src + sz - p);
[20:13] <ezequielg> mchehab: i'm gonna run some tests on your master now. thanks for the pick!
[20:14] <ezequielg> mchehab: i think those alignment changes look worse, but i'm too happy to complain :-)
[20:15] <ezequielg> hm, there is something kind of wrong. i think others have already complained about this...
[20:15] <ezequielg> from time to time, a rebase causes my build to seemingly rebuild everything.
[20:15] <ezequielg> but i don't think i have that many changes in the rebase.
[20:37] <ezequielg> hverkuil: there's a patch for v4l2-compliance, "v4l2-compliance: Remove spurious error messages", not sure you've seen it.
[21:27] <hverkuil> ezequielg: I've seen that patch. It's on my todo list.
[23:31] <mchehab> ezequielg: there is a patch there that touched on one .h file that it is included on several places
[23:31] <mchehab> that's likely what caused the mass rebuild
[23:34] <mchehab> for my eyes, the alignment makes a lot easier for my brain to quickly parse a line
[23:34] <mchehab> when it is not aligned, I need to stop and look back
[23:35] <mchehab> also, we opted to not be 100% 80-cols compliant
[23:35] <mchehab> so, among all checkpatch warnings, the one I care less is 80 cols
[23:36] <mchehab> as there are several good reasons why we should violate it
[23:36] <pinchartl> when did we decide to relax the 80 columns limit rule ?
[23:37] <mchehab> pinchartl: just see the submitted patches and you'll see
[23:37] <mchehab> the obvious ones are long strings
[23:38] <pinchartl> I know that not all patches comply, my question was whether we at some point made a collegial decision to relax that rule
[23:38] <mchehab> we have also several violations at v4l2 core, like comments:
[23:38] <mchehab> 	t->tv_freq = 400 * 16; /* Sets freq to VHF High - needed for some PLL's to properly start */
[23:39] <mchehab> and even uglier stuff like this:
[23:39] <mchehab> 	case V4L2_CID_WHITE_BALANCE_TEMPERATURE: return "White Balance Temperature";
[23:39] <mchehab> 	case V4L2_CID_SHARPNESS:		return "Sharpness";
[23:39] <mchehab> 	case V4L2_CID_BACKLIGHT_COMPENSATION:	return "Backlight Compensation";
[23:39] <mchehab> 	case V4L2_CID_CHROMA_AGC:		return "Chroma AGC";
[23:40] <mchehab> at v4l2 control framework
[23:40] <pinchartl> that still wasn't the question :-)
[23:43] <mchehab> the thing is: if we're keep continuing submitting and applying patches (specially at the core) violating it, it doesn't make sense to not relax
[23:43] <mchehab> also, there was several upstream discussions about that at lkml
[23:43] <pinchartl> I personally believe that exceptions to the rule are fine in specific cases
[23:43] <mchehab> the majority opted to let some lines violate 80 cols
[23:43] <pinchartl> long strings being the most obvious example
[23:43] <mchehab> like on long strings
[23:44] <mchehab> that doesn't mean we should not look on it
[23:44] <mchehab> just that it has less precedence with regards to other coding style issues
[23:44] <pinchartl> so we should document the allowed exceptions if we want to make that clear. otherwise saying that we opted to not be 100% 80-cols compliant is a bit misleading
[23:44] <pinchartl> a good candidate for the subsystem profile possible ?
[23:44] <pinchartl> s/possible/possibly/
[23:45] <sailus> mchehab: Done compiling with W=1, no added warnings...
[23:45] <mchehab> sailus: thanks for testing
[23:45] <mchehab> sailus: btw, I suggest you to always use W=!
[23:45] <mchehab> W=1
[23:45] <mchehab> that would avoid rebuilds
[23:46] <mchehab> btw, I tested here... indeed with arm, changing W= made it rebuild stuff
[23:46] <mchehab> (tested actually with arm64)
[23:47] <mchehab> btw, I actually noticed a few new warnings....
[23:47] <pinchartl> mchehab: is W=1 compatible with -Werror, or would it break most builds ?
[23:47] <mchehab> one is in a driver that is doing:
[23:47] <mchehab> struct foo = { 0 };
[23:48] <mchehab> pinchartl: it will likely break
[23:48] <pinchartl> :-(
[23:48] <mchehab> initializing a struct with 0 like the above is wrong
[23:48] <mchehab> it actually initializes the first element with 0
[23:48] <pinchartl> not a very good option for me then, as I run builds with -Werror to make sure I don't introduce warnings
[23:48] <mchehab> on that particular warning, the first element was a pointer
[23:49] <sailus> OTOH distcc building is currently broken on x86_64...
[23:49] <sailus> Unless it got fixed, that is.
[23:49] <mchehab> so, it complains that a pointer is being initialized with 0 instead of NULL
[23:49] <mchehab> pinchartl: well, you could do W=1 -Werror M=drivers/media
[23:50] <mchehab> our policy has been for a while to have zero W=1 warnings
[23:52] <mchehab> I'll double check if the recent changes didn't introduce any warning and fix it
[23:55] <pinchartl> that's a good idea
[23:55] <mchehab> just a few added at the rockchip vpu new driver
[23:55] <mchehab> trivial to fix
[23:56] <mchehab> just sent a patch