| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| mchehab | hi all | [11:55] |
| pinchartl | hi Mauro | [11:55] |
| mchehab | sailus: with regards to the last PR you sent, Jenkins reported:
Summary: got 17/43 patches with issues, being 0 at build time, plus one error when buinding PDF document (didn't check the actual contents) could you please double-check the warnings if they're false positives or not? | [11:57] |
| hverkuil | hi all | [11:58] |
| sailus | Heippa! | [12:00] |
| syoung | hi all | [12:00] |
| mchehab | Re: [GIT PULL v2 for 5.12] More V4L2 patches (#71294) | [12:00] |
| hverkuil | mchehab: is tomorrow the last day for 5.12 PRs? | [12:01] |
| sailus | mchehab: Most of these warnings are from lines ending with opening parenthesis.
The function names are long so the arguments don't really fit on the same line so well inside a loop. | [12:02] |
| mchehab | any reason why they end with opening parenthesis? | [12:02] |
| sailus | I guess you could do something about this but I doubt it would improve readability of the code, rather the opposite. | [12:02] |
| mchehab | btw, this seems more serious:
Macro argument '__type' may be better as '(__type)' to avoid precedence issues I found very annoying when I review something with an open parenthesis at the end it takes me more time to review when that happens ;-) | [12:02] |
| sailus | I don't mind that personally.
Would you instead prefer too long lines, or split the function into two? It could also mean both. | [12:04] |
| mchehab | yeah, longer lines are better | [12:05] |
| sailus | Over 80?
That is against the coding style. | [12:05] |
| mchehab | the current limit is 100 | [12:05] |
| sailus | It's 80, that hasn't changed.
I really hate long lines in code I review. | [12:05] |
| mchehab | checkpatch starts warning after 100 | [12:05] |
| sailus | It's only checkpatch.pl ratning limit that has changed. | [12:05] |
| mchehab | nowadays | [12:05] |
| sailus | But checkpatch.pl is not coding style.
The warning on the macro argument is from a type the result is cast to. | [12:06] |
| mchehab | see, Kernel good practices are to use 80 columns as a limit.... but exceptions are allowed | [12:07] |
| sailus | Putting parentheses there doesn't really make sense.
Yes, that's true. But here I don't see a need for an exception. | [12:07] |
| mchehab | sailus: as I said, I didn't check the patches... just did a very quickly view at the e-mail from Jenkins
if putting parenthesis doesn't make sense, that's OK | [12:07] |
| sailus | Ack.
I guess these matters are debatable, and there isn't a single ground rule that would always yield most readable code. | [12:08] |
| mchehab | the high number of patches with issues on this series got my attention enough to ask you to double check :-) | [12:09] |
| sailus | Sounds like a reasonable thing to ask about, admittedly.
I do run checkpatch.pl on patches I put in a pull request nowadays. | [12:10] |
| hverkuil | I looked at the code and it is in a gray area. I'd probably have kept the first arg after the (, but it does become a rather long line. | [12:10] |
| mchehab | lines ending with open parenthesis usually means that the arguments on the next lines won't be aligned with "(",
which makes harder to review the code, as one needs to stop on each of those and look more carefully | [12:10] |
| sailus | mchehab: Yeah, usually the following lines are aligned one tab stop right of the first line of the statement. | [12:11] |
| mchehab | I mean, it is easiser to review:
void func(foo, bar) than | [12:11] |
| sailus | And that helps quite a bit if the function you're calling has a long name. | [12:11] |
| mchehab | void func (
foo,bar) | [12:11] |
| sailus | The function names in the V4L2 async framework are long and descriptive. If we made them shorter, they'd be less descriptive.
There are helper functions for specific cases so drivers can avoid implementing things when there's a common pattern. Such as v4l2_async_notifier_add_fwnode_remote_subdev. It gets hard to keep arguments under 80 column even if the function is not called inside a loop. | [12:12] |
| mchehab | ok. that's a good argument to have such lines > 80 columns | [12:14] |
| sailus | My own preference has been to allow arguments starting on the next line in such cases.
Well, I don't disagree, but I would wrap after the opening parenthesis. | [12:14] |
| mchehab | the problem is that the arguments won't be aligned with the parenthesis. That's really the issue behind this checkpatch warning | [12:15] |
| sailus | And it wasn't my patch btw. | [12:15] |
| mchehab | I won't be rejecting this PR due to that, but, specially at the core, we should be more careful about coding styles, due to 2 reasons:
- it makes the core easier to be understood; - people tend to mimic the core behavior on drivers. If the core is messy, drivers may become messier | [12:16] |
| sailus | I fully agree.
The same applies to 80 column limit. :-) | [12:17] |
| pinchartl | (we should start using C++, we would have shorter function names) | [12:17] |
| sailus | Recently I've seen a lot of patches that use longer lines than 80 for no apparent reason, too. | [12:17] |
| pinchartl | for the 80 columns limit, I tend to sill abide by it, and sometimes use longer lines when it actually increases readability
s/sill/still/ | [12:17] |
| sailus | pinchartl: I agree.
E.g. many tables fall into that category. | [12:18] |
| mchehab | c++ won't solve... they're usually bigger, as they have the class prepended :-) | [12:18] |
| sailus | I mean, arrays. | [12:18] |
| pinchartl | I wrap comment blocks at 80 columns for instances, but write code with longer lines when splitting for just a few characters would be weird | [12:18] |
| syoung | please no C++ | [12:18] |
| sailus | :-D
I don't think we could decide to use C++ for the Linux kernel here in any case. Hopefully that comforts you. :-) | [12:18] |
| pinchartl | mchehab: it's "v4l2_async_notifier_add_fwnode_subdev(notifier, fwnode, sizeof(...))" vs. "notifier->add_fwnode_subdev(fwnode, sizeof(...))". the latter is definitely shorter :-)
but that's not a serious suggestion of course | [12:19] |
| hverkuil | mchehab: is tomorrow the last day for 5.12 PRs? | [12:19] |
| mchehab | pinchartl: a C++ function would likely have all class inherited, like v4l2::async::notifier::fwnode::add_subdev() :-)
hverkuil: my plan is to handle the last PRs tomorrow or during the weekend | [12:21] |
| sailus | mchehab: All patches I had are on that big pull request. If I get more, I could still send a small pull request tomorrow. | [12:22] |
| hverkuil | mchehab: OK, I'll see if I can put out another fixes PR this afternoon or tomorrow morning. | [12:22] |
| mchehab | so, yeah, if you send a PR tomorrow, I may still be able to handle it - but that will depend on what time I'll start handling them ;-) | [12:22] |
| hverkuil | I would appreciate it if all of you can check this patch series (not for 5.12): https://patchwork.linuxtv.org/project/linux-media/cover/20210202144926.620104-1-hverkuil-cisco@xs4all.nl/ | [12:23] |
| mchehab | It is more likely that I'll handle those only during the weekend - although I would prefer to handle them during the working hours | [12:23] |
| hverkuil | I should be able to make a PR this afternoon.
I don't think there are many 'fixes' patches pending. | [12:24] |
| sailus | mchehab: Sure. I don't expect anything that couldn't wait for 5.13. | [12:26] |
| mchehab | hverkuil: if I understood the idea behind the patch is that, from userspace, /sys/class/video4linux/v4l-subdev3/media_dev will show the major:minor, right? | [12:27] |
| hverkuil | yes | [12:27] |
| mchehab | wouldn't be better, instead, to provide some information for udev to create a soft link to /dev/media0?
udev already creates something: $ ls /dev/v4l/by-path/ pci-0000:00:14.0-usb-0:10.3.4:1.0-video-index0 pci-0000:00:14.0-usb-0:10.3.4:1.0-video-index1 (right now, in an ugly way, as right now it uses a small V4L application that runs VIDOC_QUERYCAP | [12:29] |
| hverkuil | Well, udev uses the index attribute to do this. So once a media_dev attribute is created, then it can make a media softlink as well based on that. But you need to provide that attribute first. | [12:33] |
| mchehab | the normal udev way would be to read /sys/class/*/uevent
hverkuil: yeah, the attribute should be passed to userspace somewhere | [12:33] |
| hverkuil | When a V4L2/DVB device node is created, then udev gets a uevent, it can read the attribute and make a softlink. Of course, that would require work in the udev userspace application.
But it seems overkill to me, only a few specialized applications would want to use this. | [12:34] |
| mchehab | hverkuil: I guess patch 1 may break apps
initializing media controller before registering the nodes can case race conditions at least for media API v1 as it doesn't support hot-pluging devices at the media bus | [12:36] |
| hverkuil | vim2m? Why? The media_device struct is initialized, but it is not yet registered. | [12:38] |
| mchehab | if a program uses something like inotify() to detect when a new media device is created, it can use that information to parse the media devices and then try to access each one | [12:39] |
| hverkuil | Since video_register_device can add entities, links, etc. you really need a properly initialized (empty) media_device. | [12:39] |
| pinchartl | hverkuil: I'll review your latest series, and reply to your reply in the previous version | [12:39] |
| mchehab | but if those weren't created yet, you'll have a race condition | [12:39] |
| hverkuil | No, the media device is created in media_device_register() at the end of the probe().
media_device_init just initializes the media_device struct. | [12:40] |
| mchehab | ah, OK
you're splitting the MC code on two parts... the first one initializing the structs, and the last one actually registering that has a race condition too... udev will call the V4L helper app before the media controller is created so, it can't create any link to the media controller because /dev/media? won't exist | [12:40] |
| hverkuil | Correct. That's why creating a link is a bad idea and not necessary anyway :-)
There is a short window where media_dev will return nothing. (i.e. "") | [12:42] |
| mchehab | IMO, the right way to fix this is to patch /usr/lib/udev/v4l_id
making it to parse the media controller when it is created and letting it create the links for the V4L2 subdevs | [12:45] |
| hverkuil | Hmm.
Interesting suggestion. Let me take a look at v4l_id. | [12:46] |
| mchehab | ok | [12:47] |
| hverkuil | Urg, v4l_id is *very* out of date. | [12:48] |
| mchehab | btw, that's the one responsible to create /dev/v4l/* stuff
I guess it was written once by udev maintainer and nobody ever touched it afterwards | [12:48] |
| hverkuil | OK, I'll look some more into this.
Since DVB also supports the media controller, this needs to do the same thing. Perhaps an mc_id app is needed for that. | [12:50] |
| mchehab | $ /usr/lib/udev/v4l_id /dev/video1
ID_V4L_VERSION=2 ID_V4L_PRODUCT=USB Video: USB Video ID_V4L_CAPABILITIES=: funny... it doesn't detect it as metadata :-) as you said... too old anything else for today? | [12:51] |
| hverkuil | not from me | [12:56] |
| ............. (idle for 1h1mn) | ||
| *** | ChanServ sets mode: +v mchehab | [13:57] |
| .............. (idle for 1h6mn) | ||
| hverkuil | pinchartl: can I pick up this uvc patch for a 'fixes' PR? https://patchwork.linuxtv.org/project/linux-media/patch/4ecb8867-6678-aa1a-3d86-65f815d34f5b@xs4all.nl/ | [15:03] |
| ..... (idle for 24mn) | ||
| pinchartl | hverkuil: please do. I've just reviewed the patch | [15:27] |
| hverkuil | thanks | [15:28] |
| ......... (idle for 41mn) | ||
| mchehab: posted one more fixes PR.
There might be another PR tomorrow for a pwc patch: this commit 69c9e825e812 ("media: pwc: Use correct device for DMA") and this patch https://patchwork.linuxtv.org/project/linux-media/patch/20210121202855.17400-1-tiwai@suse.de/ conflict and it looks like they attempt to solve the same problem. I think the patch from Takashi should probably replace the existing patch, but I'll have to test that tomorrow. | [16:09] | |
| .......................... (idle for 2h9mn) | ||
| mchehab | ok | [18:21] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |