| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| hverkuil | mchehab: irclogger seems dead again. | [12:03] |
| ..... (idle for 20mn) | ||
| mchehab | weird, the script seems to be running there
http://linuxtv.org/irc/irclogger_log/v4l?date=2015-12-11,Fri seems to be working perhaps some transitory disconnect | [12:23] |
| hverkuil | Ah, I see what causes the confusion: as long as there are no 'real' messages, the logger will not create a new entry for the day. So as soon as I asked the question, the page for Friday was added :-) | [12:28] |
| mchehab | yes | [12:38] |
| javier__ | hverkuil: https://en.wikipedia.org/wiki/Observer_effect_(physics) :) | [12:48] |
| mchehab | :-D
it seems that writing a message on IRC changes the quantum state of LinuxTV.org :-D | [12:49] |
| javier__ | mchehab: haha | [12:50] |
| .... (idle for 19mn) | ||
| MJRider | Nelis: mchehab quantum bugs are the best ;-) | [13:09] |
| mchehab | oh, yes! | [13:09] |
| awalls1 | They are neither a bug nor correct code until you observe them. | [13:12] |
| javier__ | awalls1: and when you observe it, it's always a bug :P | [13:14] |
| mchehab | javier__: actually, if you don't observe the IRC channel, the chances are 50%
you only have 100% if you observe both linuxtv.org and IRC ;) | [13:24] |
| javier__ | mchehab: haha | [13:28] |
| mchehab | in other words, reading one, changes the quantum state of the other ;) | [13:29] |
| javier__: FYI, I'm adding the new MC patches on my experimental tree at media-controller-rc3 | [13:42] | |
| javier__ | mchehab: Ok, I'll rebase on top of that before posting my fixes/cleanups
mchehab: but is unlikely that our patches are going to conflict since mine are mostly on platform drivers | [13:42] |
| mchehab | javier__: yes, true | [13:47] |
| ............. (idle for 1h1mn) | ||
| pinchartl, hverkuil, shuah, javier__: there's one point from the patch review that may require a quick discussion
from my TODO list: Discuss the MEDIA_IOC_G_TOPOLOGY ioctl: use u64 instead of pointers? Change the reserved fields? this action item was generated per pinchartl's review of this patch: uapi/media.h: Add MEDIA_IOC_G_TOPOLOGY ioctl | [14:48] | |
| shuah | ok with quick discussion | [14:51] |
| mchehab | https://www.mail-archive.com/linux-media@vger.kernel.org/msg94057.html | [14:51] |
| pinchartl | regarding __u64 vs void * I don't have a strong opinion, but it seems that the rest of the kernel does | [14:52] |
| javier__ | pinchartl: yes, it seems that __u64 is the current trend | [14:53] |
| pinchartl | it helps with compat32 handling
and if we switch to __u64 then we can use __u32 for reserved fields at the end of the structure, it won't make a difference | [14:53] |
| julius | hi | [14:54] |
| pinchartl | also, one thing I was wondering, is whether we could do without the reserved fields completely
I got the idea while working on DRM code | [14:55] |
| javier__ | pinchartl: agreed, since pointers are self-aligned to different byte boundaries for 32 and 64 bit | [14:55] |
| pinchartl | they match the ioctl based on the ioctl number only, not the structure size
and when they need to add fields that just extends the structure and increases its size, so kernel code can cope | [14:55] |
| mchehab | pinchartl: yes, we don't actually need reserved fields at all | [14:56] |
| julius | inserting my camera shows me this dmesg output: http://pastebin.ca/3281981 the camera works, but even with auto exposure off set to manual i only get around 9fps. does the dmesg output show that it is not supported and i just dont see it? | [14:56] |
| mchehab | I think I pointed that approach during the MC workshop
the input/evdev does that too they use the size increase to detect new versions | [14:56] |
| pinchartl | it's a departure from the ioctl model used by v4l though | [14:57] |
| mchehab | yes | [14:57] |
| pinchartl | no strong opinion either there
but if we want to do that for MC we should do it sooner than later before we get too many ioctls | [14:57] |
| mchehab | from my side, let's do it
so, it seems that both you and I agree with: 1) keep using the v2 names, removing the FIXME comment; 2) use u64 for the pointers; 3) don't use reserved fields right? | [14:58] |
| pinchartl | for the _v2 names let me sleep over it. for the rest, I agree
(and speaking of sleeping over it I'll spend half a day in a plane on Sunday, with time to review the remaining patches) | [14:59] |
| shuah | as for me sounds good - going with u64 is a good thing | [15:00] |
| pinchartl | I'd like hverkuil's opinion on __u64 too | [15:00] |
| mchehab | didn't you finish? I assumed that you finished since you promised doing that before this monday...
the patches got already applied at the media-controller branch, as agreed | [15:00] |
| pinchartl | I've seen your e-mail about that
I've sent you a message, not sure if it was in an e-mail or in here, telling you that I had ~1/3 of the patches remaining | [15:01] |
| mchehab | of course if you find issues, they could be fixed later | [15:01] |
| pinchartl | (it was 5am and I had a flight the next day^W^Wsame morning) | [15:01] |
| mchehab | weird, I didn't receive any such message from you | [15:02] |
| pinchartl | it was on the 6th of December if I recall properly | [15:02] |
| mchehab | last message I got from you was this one:
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: Mauro Carvalho Chehab <mchehab@osg.samsung.com> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>, Mauro Carvalho Chehab <mchehab@infradead.org>, linux-sh@vger.kernel.org Subject: Re: [PATCH v8 01/55] [media] media: create a macro to get entity ID Date: Sun, 06 Dec 2015 05:20:20 +0200 | [15:03] |
| pinchartl | it could have been on IRC | [15:03] |
| hverkuil | Reading up on the discussion... | [15:04] |
| mchehab | I had a bug on my IRC daemon on that weekend
and the IRC logger was not working after the machine migration | [15:04] |
| hverkuil | Regarding 1: I don't like the v2 name, but I don't have a better idea... | [15:04] |
| mchehab | (the current logs were rebuilt from what I had on my own logs) | [15:04] |
| pinchartl | server upgrades always hurt :-/ | [15:06] |
| mchehab | hverkuil: we ended by doing something like adding a _v2_ when we added IR support at the input/evdev
hverkuil: and with regards to (2) and (3)? | [15:06] |
| hverkuil | Regarding 2: how much does going to u64 actually help? You still need compat code due to alignment differences. This would be the only ioctl in the media API that uses u64 for a pointer. I really don't think u64 will help you much, if anything, and it would be completely out of line with the existing API. I vote against. | [15:08] |
| pinchartl | hverkuil: if we switch to __u64 we should use it for all future ioctls | [15:10] |
| mchehab | yes | [15:10] |
| javier__ | hverkuil: the alignment differences will depend on how you organize the files in the struct | [15:10] |
| hverkuil | Regarding 3: I have no strong opinion on this. If this can be done cleanly in the core code, then I'm OK with that. | [15:10] |
| javier__ | hverkuil: with you u64 you can avoid a compat ioctl if things are well organized
s/files/fields | [15:11] |
| mchehab | we can avoid align issues if we use _u64 instead of _u32 at media_v2_topology | [15:11] |
| hverkuil | javier__: you still need compat code to convert the pointers. | [15:11] |
| pinchartl | the whole point it to avoid compat code completely | [15:12] |
| shuah | hverkuil: is it the concern that having one ioctl using __u64 is very diferent from all others - leading to confusion | [15:12] |
| mchehab | I've no idea how compat would work for u32 pointers on an u64 struct.... | [15:14] |
| hverkuil | How can you avoid compat code? As I understand it you always need to convert the pointers. If that's not the case, then using u64 might be a good idea. | [15:14] |
| mchehab | well, let's ask | [15:14] |
| hverkuil | Basically going for u64 instead of void * only makes sense if you can avoid the compat code altogether. | [15:15] |
| mchehab | maybe Arnd knows the answer, as he suggested the usage of:
static inline void __user *get_upointer(u64 arg) { return (void __user *)(uintptr_t)arg; } to get it | [15:15] |
| pinchartl | that's the idea | [15:15] |
| hverkuil | If you have to write compat code anyway, then I see no point in doing this due to the inconsistent API that you introduce. | [15:15] |
| mchehab | so, if all agree, for (2), we'll double check if no compatible code is needed
if this is the case, we'll go for it | [15:16] |
| pinchartl | that could would be the same for 32-bit and 64-bit | [15:16] |
| hverkuil | (and the num_foo fields should be u64 as well in that case to avoid holes in the struct) | [15:16] |
| mchehab | pinchartl: yes, I guess that, if userspace is 32 bits, that code should work fine, even if the Kernel is 64 bits
yes, at the struct, we should only use u64 ints this way, it should likely be compat32 safe | [15:16] |
| hverkuil | Hmm, the contents of the pointers should also all be compat32 safe, but I think that's the case. | [15:17] |
| pinchartl | if we can't avoid compat32 code I agree that using __u64 is pointless | [15:18] |
| arnd | there is one small issue to watch out for: if you have a struct that contains at least one __u64, you need to ensure that the size is a multiple of 64 bit, otherwise x86 will still need a compat handling | [15:18] |
| shuah | Yes that is my opinion as well | [15:18] |
| arnd | "struct foo { __u64 a ; __u32 b; };" is 12 bytes on x86-32, but 16 bytes on all other architectures, 32 and 64 bit | [15:19] |
| pinchartl | arnd: and we need to ensure that the u64 field is 64-bit aligned I suppose | [15:19] |
| mchehab | arnd: if we warrant that everything is u64 there, and we use the above macro, we won't need any compat32 code for this? | [15:19] |
| arnd | correct | [15:19] |
| mchehab | OK, good! | [15:19] |
| hverkuil | OK, then I agree. | [15:19] |
| pinchartl | "struct foo { __u32 a; __u64 b; __u32 c; }" wouldn't work, but "struct foo { __u64 b; __u32 a; __u32 c; }" would | [15:19] |
| mchehab | so, for (2), we'll then go to u64, as everybody agreed | [15:19] |
| shuah | ok that sounds good | [15:19] |
| hverkuil | I do think in that case it would be useful to add static inlines that convert the u64 to the appropriate pointer. | [15:20] |
| javier__ | pinchartl: yes, that's what I meant by organizing the fields in the struct
pinchartl: having the fields sizes in decreasing order | [15:20] |
| mchehab | pinchartl: true, but our struct has 5 u32 and 4 pointers
better to change everything to u64 | [15:20] |
| hverkuil | I agree. | [15:21] |
| arnd | it's not a problem to have __u32 members in the struct really, as long as they come in pairs | [15:21] |
| hverkuil | arnd: they don't in this case :-( | [15:21] |
| mchehab | yes, but we actually have one u32 one pointer per pair | [15:21] |
| shuah | so you might need reserved fields to round them off in somce cases | [15:22] |
| mchehab | struct media_v2_topology {
__u32 topology_version; __u32 num_entities; struct media_v2_entity *entities; __u32 num_interfaces; struct media_v2_interface *interfaces; __u32 num_pads; struct media_v2_pad *pads; __u32 num_links; struct media_v2_link *links; } | [15:22] |
| hverkuil | Oh well, as least we can cope with 2^64 entities. Nobody can accuse us of not being future-proof in this case! | [15:22] |
| mchehab | :) | [15:22] |
| arnd | Using __u8 is not enough, right? | [15:23] |
| mchehab | no
u16 might work | [15:23] |
| arnd | you still need padding with __u16 unfortunately | [15:24] |
| mchehab | but won't solve, so lets move to u64 and be it | [15:24] |
| hverkuil | It's no big deal to use u64 for everything here. | [15:24] |
| mchehab | no, it isn't
ok, with regards to (3), let's remove the reserved stuff? | [15:24] |
| javier__ | so it should be something like this: | [15:25] |
| arnd | agreed. It only matters if the struct becomes too large for the kernel stack, the time to copy it matters for a fast path | [15:25] |
| javier__ | struct media_v2_topology { __u64 entities; __u64 interfaces; __u64 pads; __u64 links; __u32 topology_version; __u32 num_entities; __u32 num_interfaces; __u32 num_pads; __u32 num_links; __u32 padding;
sorry for that... I meant: http://hastebin.com/xuhivafaze.avrasm | [15:25] |
| hverkuil | javier__: you can't easily add new num_foo & u64 foo fields later.
in that proposed structure | [15:26] |
| shuah | right with the last pad fields | [15:26] |
| hverkuil | mchehab: regarding 3: it depends on the impact on the core code. If it can be added cleanly, then I have no objection. | [15:27] |
| pinchartl | I have to run I'm afraid, I'm already late | [15:27] |
| shuah | any changes to the structure will have ensure alignment | [15:27] |
| mchehab | http://pastie.org/10625511this sounds better, IMHO
http://pastie.org/10625511 hverkuil: not much to be changed at the core code | [15:27] |
| shuah | That looks good to me | [15:28] |
| mchehab | if the size of the struct increases | [15:28] |
| javier__ | mchehab: but then you will need a compat code since the __u64 pointer will have different sizes in 32 and 64 bit
IOW, is the same to have __u64 * than void * or am I missing something? | [15:29] |
| mchehab | ops... I mean __u64 entities and so on | [15:29] |
| hverkuil | mchehab: you mean just u64 instead of u64 * :-) | [15:29] |
| mchehab | yes | [15:29] |
| javier__ | mchehab: oh, sorry | [15:29] |
| mchehab | cut-and-paste mistake | [15:30] |
| javier__ | mchehab: yes, your proposal looks better to me as well | [15:30] |
| mchehab | hverkuil: to add a new field, there are actually two ways: | [15:30] |
| hverkuil | although I would suggest naming it ptr_entities instead of entities. Without the '*' you wouldn't know it is a pointer unless the name says so. | [15:30] |
| mchehab | 1) add a new ioctl macro for the version with the bigger size;
2) change the ioctl code to strip away the size and then use a macro that will get the size of the struct that userspace sent what about: entities_p, interfaces_p, pads_p, links_p | [15:30] |
| hverkuil | fine too, but I like the alternating num_foo; ptr_foo; It looks nice :-) | [15:32] |
| mchehab | works for me | [15:32] |
| hverkuil | num_foo; foo_p; looks weird
foo_num; foo_ptr; would also be OK. | [15:32] |
| mchehab | ok...
I'll be preparing the patches for it... for now, I'll keep the _v2_ at the names, removing the FIXME and change the struct to use u64 and removing the reserved fields we can change the _v2_ names later if someone comes with some better idea (but before 4.5 final, of course ;) ) | [15:33] |
| shuah | mchehab: maybe you can do a pastebin for the final to get a quick ok, so patch review goes quicker | [15:35] |
| mchehab | http://pastie.org/10625539 | [15:38] |
| hverkuil | ack | [15:39] |
| mchehab | ideally, the macros won't be returning void pointers, but struct media_v2_foo * ones | [15:39] |
| javier__ | cool | [15:39] |
| shuah | ack | [15:40] |
| mchehab | good. I'll work on the patch and post at the ML once it is done
thank you all! | [15:41] |
| ............ (idle for 56mn) | ||
| *** | benjiG has left | [16:37] |
| .......................... (idle for 2h8mn) | ||
| awalls1 has left | [18:45] | |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |