| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | ChanServ sets mode: +v mchehab` | [05:48] |
| ................................................ (idle for 3h57mn) | ||
| mchehab | it seems that this time kbuild bot didn't make his work right... there was a warning in the middle of the series about a new unused var that was solved by a latter patch...
plus the error above sailus: ^ also, it introduced several new checkpatch warnings | [09:45] |
| sailus | mchehab: So what do you propose?
I can fix those and resend if you like. I reviewed your patches, too. | [09:46] |
| mchehab | no need to fix/resend
but next time, please test patch per patch instead of relying on kbuild bot | [09:47] |
| sailus | Do you happen to know e.g. scripts that could be used to facilitate that?
kbuild bot generally does a good job. I have to say that the kbuild bot generally can find a lot of problems with the patches I can not, for the reason that it compiles for multiple configurations and on multiple architectures. That's why I've given up on doing that work myself. I checked the kbuild bot message; usually it compiles the patches one by one but this time it compiled the head only --- and all was well. I think I'll ask Fengguang why could that be. | [09:48] |
| pinchartl | sailus: it starts by the head only, and then later compiles the patches individually
as for a tool to help, there's git rebase -x make <base commit ID> replace make with ./make.sh in which you pass a -Werror extra argument to the kernel build and git rebase will then stop on all warnings | [10:03] |
| sailus | mchehab: Speaking of renaming, I'd prefer v4.21, so there's more time to think about the API in general. There's more work there anyway.
pinchartl: make.sh must be your own script. | [10:10] |
| mchehab | sailus: if are there more stuff for 4.21, let's postpone the renames
git rebase -x make_script seems a good option... | [10:13] |
| sailus | mchehab: I'm not fully certain it'll be in v4.21, but there are unaddressed matters there I'd like to address.
Yeah... I'll need to check that. | [10:14] |
| mchehab | here, I just put everyhing on a quilt list and apply patch per patch
so I don't need to use git rebase I always keep a separate terminal building new stuff my build script relies on: inotifywait -e close_write -qq -r .git/objects in order to identify when I add a new patch I also double check if it was really a new patch with: PATCH=`git log HEAD^1..HEAD --pretty=oneline|cut -d' ' -f 1` TAG=`echo $PATCH|cut -d' ' -f 1` if [ "$TAG" != "$OLDTAG" ]; then echo "===> Recompiling due to: $(git show -s --pretty='%h (\"%s\")' --date=short --no-patch $PATCH)" sleep 2 make.sh --check (all of these inside an infinite while loop | [10:14] |
| ....... (idle for 34mn) | ||
| sailus: fwnode patches pushed | [10:51] | |
| sailus | mchehab: Obrigado! | [10:51] |
| mchehab | anytime | [10:51] |
| sailus | ...and my inbox is exploding. :-o | [10:51] |
| mchehab | :-D | [10:52] |
| all pull requests handled | [10:57] | |
| hverkuil | mchehab: including the one I posted this morning? | [10:57] |
| mchehab | no, the ones that were already there :-)
didn't update my local cache will handle it right now | [10:57] |
| hverkuil | There definitely will be another pull request for CEC bug fixes. But that's all fixes, not new features. | [10:59] |
| mchehab | mkrufky: did you submit your pull request? | [11:00] |
| mkrufky | i did not. i can have it to you tomorrow early .. is it ok? | [11:00] |
| mchehab | I probably won't be merging stuff at weekends
I may handle it on Monday, but if you could send it earlier, that would be better | [11:01] |
| mkrufky | I just want to be sure its in time for the window
I will TRY to send tonight | [11:01] |
| mchehab | ok | [11:01] |
| mkrufky | i think it wont happen
but i will aim anyway for tonight better to aim for earlier so ......... i will do my best if its not there tonight, it will be tomorrow AM | [11:01] |
| mchehab | please be sure that the patches you'll be adding to your PR will be delegated to you at patchwork... | [11:02] |
| mkrufky | ah yes. i must get better at the process
will do :-) | [11:02] |
| mchehab | as I intend to check again for pending patches today
getting the ones that people forgot to pick :-) | [11:02] |
| mkrufky | of course | [11:03] |
| mchehab | hverkuil: what's the difference between V4L2_PIX_FMT_JPEG and V4L2_PIX_FMT_JPEG_RAW?
IMO, "media: Add JPEG_RAW format" doesn't make it clear | [11:04] |
| hverkuil | JPEG has headers (quant matrices, etc.), JPEG_RAW is just the image, no headers. Userspace will have to insert those. | [11:06] |
| mchehab | Did Ezequiel submit the userspace patch for libv4l?
there are a number of camera drivers that add the headers internally | [11:09] |
| hverkuil | No, and that's why it is in staging. It's in the TODO.
See also the discussion I had with him on this topic. https://linuxtv.org/irc/irclogger_log/v4l?date=2018-10-04,Thu | [11:10] |
| mchehab | the problem I'm seeing is that the patch with adds support for JPEG_RAW is not touching -staging code
it is touching V4L2 core once we add something there and userspace starts using it, we can't remove it anymore | [11:11] |
| hverkuil | But only a staging driver uses it. I don't see how else you could do this. Frankly, we do the same for the Request API. | [11:12] |
| sailus | mchehab: I'll post a few more today; fixes and new drivers. | [11:14] |
| mchehab | sailus: ok
hverkuil: from what I'm seeing from your discussions with ezequiel: "I don't have the JPEG_RAW decoder done, yet, unfortunately. " I wouldn't rush merging it for this merge window as it is a driver that nobody can use (as there's no decoder for it yet) | [11:14] |
| hverkuil | it's why it is in staging. | [11:15] |
| mchehab | s/decoder/userspace/ | [11:15] |
| hverkuil | the whole idea of staging is that you can put drivers there that are not yet ready for mainline. | [11:16] |
| mchehab | yes, but in this case, it is adding a badly described new format at the V4L2 core
the V4L2 core patch doesn't say when someone should use V4L2_PIX_FMT_JPEG or V4L2_PIX_FMT_JPEG_RAW and, if some new driver decides to use V4L2_PIX_FMT_JPEG_RAW, there's no userspace for that | [11:17] |
| hverkuil | So, again, how do you want to solve that?
One option might be to let the driver abuse JPEG for the time being, thus avoiding this discussion. | [11:18] |
| mchehab | IMO, the best option is to delay this pull request | [11:19] |
| hverkuil | Or you add #ifdef __KERNEL__ around the videodev2.h definition. | [11:19] |
| mchehab | waiting for userspace code
the problem I see is that someone could, for example, submit a patch for a driver like: drivers/media/usb/gspca/conex.c with internally receives "raw jpeg" and produces the jpeg header dynamically in order for it to also accept the new format | [11:19] |
| hverkuil | I think this is a nonsense argument. It would make it impossible to ever add staging drivers that use a new pixel format.
That's insane. | [11:21] |
| mchehab | this is *not* a new pixel format
it is an existing one we do have JPEG | [11:21] |
| hverkuil | It is. You can't feed it to a JPEG decoder since it is missing essential tables.
(aka headers) | [11:21] |
| mchehab | see the conex.c code:
/* create the JPEG header */ jpeg_define(sd->jpeg_hdr, gspca_dev->pixfmt.height, gspca_dev->pixfmt.width, 0x22); /* JPEG 411 */ jpeg_set_qual(sd->jpeg_hdr, QUALITY); we have a few jpeg drivers that do the same they all receive headerless data from the camera and they all add JPEG headers internally why this new driver can't do the same? and, if there is a good reason for that - we should clearly document it and have an userspace code that will work also with the existing jpeg drivers | [11:22] |
| hverkuil | https://www.mail-archive.com/linux-media@vger.kernel.org/msg135404.html
Anyway, let's postpone this to 4.21. No need to hurry. | [11:24] |
| mchehab | frankly, I don's ess why we should hush to merge it at the end of a merge window
s/ess/see/ | [11:24] |
| hverkuil | I thought this would be fine as a staging driver (we've had awful staging drivers in the past).
Anyway, I'm too busy debugging CEC issues :-) I'm OK with postponing and Ezequiel can discuss this further with you. | [11:25] |
| mchehab | ok, let's postpone this one
we can later discuss with ezequiel my main concern here is that it changes the V4L2 core, affecting a previously agreed position of using only JPEG with headers even if we end by dropping the driver from staging, we'll have the V4L2 core new definition so, I'd like more discussions on it before merging I'll reply to that Ezequiel's email for us to have a discussion at the ML | [11:26] |
| hverkuil | Actually, _JPEG and _MJPEG are used randomly by drivers. I posted an RFC on this a few days ago. That needs to be cleaned up as well. | [11:29] |
| ......... (idle for 42mn) | ||
| mchehab | answered to both threads
thanks for pointing me that RFC | [12:11] |
| .......... (idle for 47mn) | ||
| pinchartl | mchehab: does git.linuxtv.org support multiple committers for a single git tree ? | [12:58] |
| mchehab | yes. we do that for v4l-utils
why? | [12:58] |
| pinchartl | just to know if that would be an option. we're exploring co-maintainer options for various projects, including the Renesas drivers
so how could I give commit access to a git tree I own to someone ? | [13:01] |
| mchehab | everyone that will access it will need to have an account there | [13:02] |
| pinchartl | that sounds quite logical :-) | [13:03] |
| mchehab | so, they need to send me an email with the ssh public key they'll be using
I'll then need to manually setup the server (it is not too trivial - requires adding a new group and changing repository permissions) | [13:03] |
| pinchartl | there's no way to do that through ssh-menu or a similar interface ? :-(
damn | [13:04] |
| mchehab | no | [13:04] |
| pinchartl | that seriously limits the practical usefulness of the feature :-S
but ok, good to know | [13:04] |
| mchehab | no, it doesn't
once it is done, adding/removing new users is trivial but it has do be done per tree | [13:05] |
| pinchartl | do you think adding/removing could be added to ssh-menu, once the initial setup for a repository is done ? | [13:05] |
| mchehab | it *could*
but I don't like very much the idea of letting ssh-menu to create new accounts at the server hmm.. no, it won't work ssh-menu doesn't run as root in order to create a new account, one has to be root at the server | [13:06] |
| pinchartl | no, not creating new account, but adding/removing existing accounts to an existing tree which has already been setup to support that | [13:09] |
| mchehab | it won't work either
this is groupadd | [13:09] |
| pinchartl | ouch | [13:10] |
| mchehab | in order to touch /etc/group, root permission is also required
the way git works is by using standard Linux permissions | [13:10] |
| pinchartl | if it's handled at the OS level then indeed there are not many options
(and running ssh-menu with setuid is likely a very bad idea) | [13:10] |
| mchehab | yep | [13:10] |
| pinchartl | thanks for the information | [13:12] |
| mchehab | if the list of co-maintainers don't change too much, it is doable
we have a script at the server that automates most of the user creation process but people will still need to send me e-mails with their ssh public keys and I'll need to log at the server and run such script manually (I guess that, right now, the script doesn't run a groupadd for a random group, but it would be trivial to add support for it) please let me know if you decide doing it at linuxtv.org | [13:13] |
| pinchartl | I will, thanks | [13:24] |
| ........ (idle for 37mn) | ||
| mchehab | sailus: please check the patches with checkpatch --strict
I'm finding a lot of warnings at the imx319 driver CHECK: Alignment should match open parenthesis #2477: FILE: drivers/media/i2c/imx319.c:2383: + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", + &cfg->ext_clk); | [14:01] |
| sailus | Oh dear. | [14:02] |
| mchehab | irc is not good handling white spaces | [14:02] |
| sailus | Apologies for that. | [14:02] |
| mchehab | it sounds that those are off by just one space | [14:03] |
| hverkuil | mchehab: posted final pull request for 4.20 with cec fixes. | [14:03] |
| mchehab | ok, thanks
sailus: np.. would you mind fixing it? Otherwise, I can do it checkpatch.pl --fix-inplace -p --strict should do the work | [14:03] |
| sailus | If you're at it, feel free to do it.
But I can resend. | [14:04] |
| mchehab | (but it has to be verified) | [14:04] |
| sailus | I have the patches right here... | [14:04] |
| mchehab | I can do it | [14:04] |
| sailus | mchehab: Ack, thanks! | [14:04] |
| mchehab | if you prefer
it is probably not worth waiting for your review on such trivial patch... I'll post it anyway after finishing reviewing the patch itself (9 checks) yeah, they're off alignment by just one space perhaps some side effect of a cut-and-paste :-p | [14:04] |
| sailus | I suppose the author might be using an editor that doesn't do the job, so it's a manual process. | [14:10] |
| mchehab | maybe
anyway, fixup sent same issue at the next driver CHECK: Alignment should match open parenthesis #1235: FILE: drivers/media/i2c/imx355.c:1139: +static int imx355_write_regs(struct imx355 *imx355, + const struct imx355_reg *regs, u32 len) also, off by 1 space | [14:13] |
| fixup for the second driver sent
please let me know if I should wait or not for your ack/review on those | [14:18] | |
| .... (idle for 15mn) | ||
| sailus: ^
handled both your new pull requests... waiting for your answer before pushing | [14:34] | |
| sailus | Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
(That applies only to the two imx sensor driver patches. :)) | [14:38] |
| ...... (idle for 25mn) | ||
| mchehab | ok! | [15:03] |
| ................................................................................ (idle for 6h36mn) | ||
| mkrufky | hello
i have to double check but, it LOOKS like Mauro has already picked up the urgent patches I had queued Still gonna triple check | [21:39] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |