<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
***: myself has quit IRC (Ping timeout: 246 seconds)
<br> mchehab has quit IRC (Ping timeout: 264 seconds)
<br> tizbac has quit IRC (Ping timeout: 246 seconds)
<br> smartin_ has quit IRC (Ping timeout: 250 seconds)
<br> Nikhil_D has quit IRC (Remote host closed the connection)
javier__: <u>sailus</u>: sorry for misunderstanding you when posting the patches. If you agree to remove the check in media_device_init(), I can re-spin a v2 probably later today
<br> <u>sailus</u>: btw, I audited all the current callers of media_device_register() and indeed all drivers are filling mdev->{dev,model} so the function is just being paranoid
hverkuil: <u>pinchartl</u>: sailus: mchehab: note that I have a meeting in one hour that will likely last 30-60 minutes.
mchehab: let's start the meeting then? pinchartl, sailus: are you ok with that?
<br> to start now, I mean
pinchartl: I'm ready
<br> and it's good to have a time limit :-)
mchehab: yep
pinchartl: I've pinged Sakari
sailus: Hello!
mchehab: just to remember, last meeting we came to one solution that would allow keeping a single range: dynamically generate the v1 entity_id when entities are enumerated inside media-device.c
hverkuil: <u>mchehab</u>: Junghak's vb2 patch series looks much, much better now.
sailus: I was just about to send RFC patches. :-)
mchehab: the remaining issue is with regards to split the type from the object ID or not at the userspace API
<br> comments?
pinchartl: that summarizes my recollection of the discussion, yes
sailus: In principle I'm ok with both options, with the conditions that 1) it has to be easy for the user to distinguish the ID from the type and 2) the user must not be encouraged to print the combined type/ID as a single number.
mchehab: <u>hverkuil</u>: good! I may not have time to review his vb2 series next week
sailus: But I'm leaning towards keeping it separate from the ID.
hverkuil: And I very much like to keep the type as part of the ID.
mchehab: me too
sailus: There will be other data related to the graph objects as well.
pinchartl: I'd prefer having them separate
sailus: Type is just one of them.
pinchartl: do you prefer combining them for kernelspace, userspace or both ?
sailus: Assuming it's part of the ID in G_TOPOLOGY, would it be part of the ID in the property API as well?
<br> I think that in the property API they definitely should be separate.
pinchartl: or, to put it differently, where do you see an advantage in combining them, in kernelspace, userspace or both ?
mchehab: keeping them together reduces the number of pages that will be passed via G_TOPOLOGY at DVB side (and IIO) where thousands of entities exist
sailus: If there's any real benefit in keeping them together in the kernel, I think that's what we should do -- in the kernel.
<br> The mchehab: How much?
<br> But a couple of pages, while the total number is... in what kind of range?
pinchartl: if memory consumption is a real concern, then it's not just about the type and ID, everything should be optimized in that case
sailus: There will have to be reserved fields, unless, we start changing the size of the argument struct when adding fields.
mchehab: it is not just memory consumption... it affects cache misses
pinchartl: we're talking about saving 4 bytes per object for the ioctl call when there are hundreds of bytes of reserved fields
mchehab: and sailus was concerned about that on the graph traversal algo
<br> if we have 1,000 entities, that would mean adding one 4K-page
<br> and for no good reason
hverkuil: Frankly, I don't think any of this (memory and performance) will be relevant since I very, very much doubt it will make any noticible difference.
sailus: <u>mchehab</u>: Now you start worrying about cache misses? :-)
mchehab: you're the one that rised that ;)
pinchartl: enumeration will typically be performed once by most applications, and possibly once per graph dynamic update. I don't think performances are the culprit there
mchehab: that would also require adding the type at both ends of the links
<br> making mroe complex to handle links than it should
pinchartl: if you're worried about cache we should profile access to the structure and reorder the fields accordingly
sailus: The pages are consecutive in memory. I'm worrying about hopping wildly around the entire system memory when just going through objects of a certain type.
pinchartl: why would it requite adding the type at both ends of the links ? IDs will be unique
sailus: And the argument structs will end up to cache anyway. They're copied between the kernel and the user.
pinchartl: s/requite/require/
mchehab: because both Kernelspace and userspace needs to identify what elements are on both ends of the links
<br> right now, there are only two options, but we may need to add more in the future
<br> IIO doesn't require PAD (from what I understand)...
sailus: mchehab, hverkuil: Supposing the type is a part of the ID in G_TOPOLOGY, what do you think of the property API?
mchehab: so, for IIO, we may need to add entity-entity links without pads
sailus: I'd definitely keep the two separate on that side.
pinchartl: you can easily do that by looking up the object based on its ID and checking its type
mchehab: <u>pinchartl</u>: that would require to do an ID lookup for every object
<br> that will badly affect performance
pinchartl: <u>mchehab</u>: which you will have to do anyway
mchehab: no
pinchartl: in kernelspace you will already have it, as the in-kernel link structures store pointers to objects, not IDs
mchehab: I don't need to store all object IDs into some database and seek for its type
sailus: <u>mchehab</u>: We have to have fast object lookup in any case.
pinchartl: in userspace the media controller library will also translate from IDs to pointers
<br> IDs for links should only be used for the ioctls
sailus: Whether the object type is known or not should make only a negligible difference.
hverkuil: (in a call)
mchehab: <u>sailus</u>: first of all, I don't see why putting it together or not would possibly affect the property API
<br> second, why do you both want to sacrifice performance for no real gain?
sailus: <u>mchehab</u>: No, it shouldn't. That's my point. :-)
pinchartl: if you look at the existing media controller library you'll see that the API for userspace uses pointers to entities (which will be replaced by pointers to graph objects) and not IDs
sailus: In the property API the two should be separate.
<br> <u>mchehab</u>: Could you elaborate your performance-related concern?
mchehab: <u>pinchartl</u>: you're thinking on your particular design to that. Did you see my code for mc_nextgen_test?
<br> there's no need to do such conversion
pinchartl: <u>mchehab</u>: then you should add it
mchehab: why? there's no need
pinchartl: userspace applications should use pointers to objects to refer to objects
<br> not IDs
mchehab: why?
<br> to loose performances?
pinchartl: and then finding the type of an object (and of course its ID) will be really simple
<br> no, to improve performances
mchehab: with dynamic graph changes, performande is an impact
pinchartl: applications will often need to find information about the graph objects
sailus: The current user space API requires looking up both of the entities, and then pads. When the graph objects have an ID, the user can tell to enable a link by the graph object ID. There's only need for a single lookup in the latter case.
pinchartl: if they have to lookup the object based on its ID every time it will be less efficient
mchehab: what I did was to create an ancillary struct to do object lookups
<br> but this is used _only_ when needed
pinchartl: if the lookup is done once only during enumeration, and then pointers used instead, applications will be simpler to code
mchehab: there are very few places where this is used, actually
pinchartl: this is the same reason why we use pointers internally in the kernel, not IDs
<br> our APIs pass struct v4l2_ctrl or struct vb2_buffer around for instance, not just the control ID or the buffer ID
mchehab: <u>pinchartl</u>: in order to use pointers, userspace would need to allocate their own media objects...
<br> duplicating the allocation
<br> of the data
<br> one for the G_TOPOLOGY, and another one for their own representation
pinchartl: yes, and the first one will be freed immediately after calling G_TOPOLOGY
hverkuil: Let's split it up. If Mauro discovers while doing that that some of the code (either kernel and/or userspace) becomes overly complicated compared to keeping type + ID in the same unsigned, then we can discuss it again.
mchehab: if the links between objects are on a separate struct, there's no need to do that
<br> check the code I wrote
pinchartl: even if we passed the type and ID in a single field I think we should convert to pointers in userspace anyway
<br> and if we do the conversion, I'm wondering what the advantage of combining type and ID would be
mchehab: <u>pinchartl</u>: try to measure the performance of media_ctl and my tool if you think you're gaining performance
<br> by doing hundreds of memory allocation instead of just one
pinchartl: it's not about media-ctl, it's about real applications
mchehab: the problem is the same
pinchartl: and also the ease of use of the MC library for them
mchehab: you're against some code that you didn't even looked into it
pinchartl: I'm not against a specific piece of code, I'm against a design principle :-)
mchehab: <u>pinchartl</u>: you cannot dictate how people will code it
<br> on userspace
hverkuil: <u>pinchartl</u>: what's wrong with combining type + ID in one unsigned? Why would that hurt? How is that a wrong design?
mchehab: or assume that everyone would agree to your way of coding... if people don't like it, they'll just do their own implementation
pinchartl: I can't prevent people from rolling up their own code that will do whatever they want, but I can try to come up with a good design for the userspace library API that will make application developers likely to use it
hverkuil: You might not code it like that, but neither is it wrong or bad, just a different approach.
mchehab: yep. that's my point
<br> back to hans question: (07:09:28) hverkuil: pinchartl: what's wrong with combining type + ID in one unsigned? Why would that hurt? How is that a wrong design?
pinchartl: and using pointers in userspace is a better design, making the library easier to use for applications
mchehab: I don't agree
<br> but we're not discussing the library
hverkuil: I'll be honest, I prefer the way it is done today (8 bits type, 24 bits for the number, four counters (one for each type) so the 24 bits are used efficiently.
mchehab: me too
hverkuil: Sure, in userspace it will be translated to a graph with pointers etc., but that is separate from how you construct the ID field.
pinchartl: not only are types and IDs two different and separate pieces of information, combining them will result in unpractical ID values when processed by humans during development and debugging
<br> (and we spend lots of time doing development and especially debugging)
mchehab: absolutely not true
sailus: mchehab, hverkuil: On Wednesday we agree to proceed with a single counter. Let's not go to that anymore.
mchehab: I find a way easyier to read entity#1
<br> than 1
sailus: I just sent the set to support the single range in graph walk and drivers btw.
pinchartl: <u>hverkuil</u>: my point is that, if it's translated to a graph with pointers, I see no compeling reason to combine the two
mchehab: entity#1 tells me what I need to do...
<br> "1" would require me to do a lookup to check if "1" is an entity, an interface or a link
hverkuil: You want to show the object type anyway when debugging. You should never print just the ID. With a lot of objects an ID of 1254 is just as hard to read as 1234345345.
sailus: <u>mchehab</u>: If you're going to do something about the entity (or whichver object it is), you'll still need to look it up based on the ID.
mchehab: <u>pinchartl</u>: what would be the compelling reason to split it?
<br> <u>sailus</u>: if I'm looking to a debug output, no, I don't...
<br> basically, if I'm looking to anything printed (either on Kernelspace or userspace), I do want to know the type together with the ID
sailus: <u>mchehab</u>: We shouldn't design this for efficient debug output only. :-)
pinchartl: <u>mchehab</u>: it combines unrelated information in a single field. why don't we put everything in a bitmap then ? :-)
mchehab: <u>sailus</u>: neither you nor pinchartl answered: what's the compelling reason to split?
sailus: We should have a function that just obtains a string-based type for a graph object.
<br> That's all that's required.
<br> <u>mchehab</u>: As said, I'm ok with either option, with the said conditions.
pinchartl: <u>mchehab</u>: but you haven't given any compeling reason to combine them
mchehab: <u>pinchartl</u>: it was already coded like that, due to the reviews we had at v2 or v3
<br> and it worked like a charm
hverkuil: Personally I like it that when describing a link you only need to give two 32 bit numbers and you know the type immediately without having to look it up. If there was a change that 8 bits for the type or 24 bits for the ID would be too small, then it would need to be split up, but that's not the case here.
mchehab: don't requiring hunderds of lookups
pinchartl: I've never said it can't work :-)
<br> just that it's a bad design in my opinion
<br> <u>hverkuil</u>: but there will be a lookup anyway in userspace when creating the pointer-based graph
hverkuil: Both approaches will obviously work, but a good reason is needed for Mauro to rework the patches.
pinchartl: so I don't really see where getting the type without a lookup would be useful
hverkuil: The problem is that what is bad design for one person is good design for another.
sailus: <u>hverkuil</u>: I think that's only true to a certain degree.
hverkuil: I think it will be useful. I certainly don't think it would hurt to have type+ID in one field.
pinchartl: just out of curiosity, how do you see it being potentially helpful ?
<br> I might be missing valid use cases
sailus: The size of struct media_v2_entity is 96 bytes at the moment.
<br> Including the name which is 64 bytes.
hverkuil: I am also not convinced that all applications will use a media library to transform the G_TOPOLOGY data. I think that there will be applications that just walk over the entities/links without converting it to tree data structures, especially if they are only interested in specific entities/links.
<br> Being able to quickly filter on the type without doing a lookup is very helpful for those apps.
sailus: If we're concerned about the space, should we first debate whether the name field should be there?
pinchartl: haven't we agreed that the only officially supported API for applications will be the MC library API ?
sailus: (Well, there's a comment suggesting moving it to properties. But it's still there.)
mchehab: I agree with hans: that will be the case for example for the libv4l, qv4l2, libdvbv5, etc...
hverkuil: We have agreed that we'll make such a library. Whether companies will use it is something you do not control.
mchehab: that will only be interested on interfaces and their links
pinchartl: <u>mchehab</u>: those should use the MC library. I'd argue that especially those should, if we don't use it in our own projects, we can't ask other developers to use it
<br> we need to show the way
mchehab: such apps want to know what alsa, video, vbi and dvb devices are bound together
<br> I don't think they'll need to do lookups
hverkuil: (have to go in 3 minutes)
pinchartl: those apps will need to lookup anyway, as the interface type won't be part of the object type
<br> you can't know from type + ID whether you're dealing with a V4L node or a DVB node
mchehab: no need to lookup
pinchartl: you'll need to lookup the media interface for that
mchehab: <u>pinchartl</u>: intf has a type
<br> it is just intf->type
pinchartl: that's a different thing
mchehab: G_TOPOLOGY will put the interface on a separate memory
pinchartl: the type you want to combine with the ID is the media object type
<br> not the interface type
mchehab: all apps need to do is to look the interfaces it needs there
pinchartl: applications that only need to enumerate interfaces won't even need the media object ID
<br> they will loop over the media interfaces array, and that's it
hverkuil: meeting starts, I'll lurk here :-)
mchehab: so, no need to use a library that would force to do hundreds/thousands of memory alloc/free
<br> sorry, but I failed to understand any compelling reason to redesign everything
<br> it is there, it works, it is a good design IMHO
<br> and it was result of a review feedback
pinchartl: the fact that you have rushed to publish patches don't make them automatically right, I'm sorry. time spent on an implementation has never been a valid argument for which version will end up in mainline
mchehab: at early stages of this series
pinchartl: I've always mentioned that I don't like combining the type and ID, it's nothing new
<br> now, if we have good reasons to combine them, that's a different thing
<br> but I don't see the use cases now
hverkuil: <u>pinchartl</u>: I *did* review it carefully, and I *am* happy with the design.
mchehab: I do: it makes things simpler
<br> no need to do lookups to get the type
pinchartl: I still don't see the real life cases where you would need the type without having to do a lookup anyway
mchehab: check the code
pinchartl: (and we need to design lookups to be fast)
mchehab: all prints need to do it
pinchartl: can't you explain those cases ? you wrote the code, I assume you know it :-)
mchehab: as, with thousands of objects, humans won't be able to tell if object 1234
<br> is entity, link, interface, ...
<br> and any system is expected to be operated by humans
pinchartl: a human brain can remember 4-digit numbers. 9-digit numbers are more difficult
<br> I'm not saying we shouldn't print the type of course
mchehab: nobody is arguing in favor of using 9-digits
javier__: maybe it will help to enumerate the pros and cons of each approach?
pinchartl: if you combine the type in the MSBs you'll end up with 8-9 digits
mchehab: no. we'll end up with type+ID
<br> entity#1234
sailus: <u>mchehab</u>: how do you print that?
<br> Supposing the type wasn't a part of the ID, a function would simply look it up based on an ID.
<br> And it'd be printed.
<br> I think such function should be used even if the type *was* part of the ID.
mchehab: sailus:
pinchartl: <u>sailus</u>: Mauro's point is that we should avoid lookups just to print the type
sailus: Ah.
pinchartl: and my point is that we'll do lookups in userspace anyway for other reasons
sailus: I think it'd be nice to have strings in the debug output.
mchehab: printk("object %s#%d", id>>24, id&&0xffffff)
pinchartl: so there won't be a cost associated with printing the type
sailus: Vb2 does that quite extensively, for instance, and I like it.
pinchartl: <u>mchehab</u>: that would be %u#%u
sailus: <u>mchehab</u>: That costs a crash.
pinchartl: unless you want to put the type as a string in the ID :-D
mchehab: printk("object %s#%d", name[id>>24], id&&0xffffff)
sailus: <u>mchehab</u>: Yes.
mchehab: that's what it is coded there at both userspace and Kernelspace
<br> no lookups
<br> just one memory read to get both type/ID
sailus: I don't see really much difference between that and a lookup function returning the name.
mchehab: 32 bits, so atomic
sailus: Besides, that way you don't have to validate the type before printing it.
mchehab: a lookup function is O(log2)
<br> the above is O(1)
sailus: Are we really debating about the efficiency of debug prints? :-)
<br> I think we must be doing really well if this is the most pressing problem. :-)
mchehab: this may not be debug
sailus: I'd think most of the users of such functions will always be debug prints.
<br> There could be some use elsewhere, but debug tends to dominate.
mchehab: as the entities, links, etc can be used by the humans to change the pilelines
<br> pipelines
<br> humans need to know
<br> back to the point:
sailus: That's certainly not performance critical then, I suppose.
mchehab: what's the advantage of going from O(1) to O(log2) for types?
sailus: <u>mchehab</u>: There is no advantage in doing so performance-wise.
mchehab: huh??????????????????
pinchartl: <u>mchehab</u>: my point is that we don't go from O(1) to O(log(n)) if we consider that userspace will anyway look up objects to create pointer-based structures
<br> if there's a lookup *anyway*, there's no additional cost in using the looked up object to find the type
sailus: I think it's a cleaner design to keep the type and the ID separate.
mchehab: we don't write Kernel APIs considering that userspace will do the implementation using algorithm A instead of B
<br> this is a really really bad way to design things
pinchartl: of course we do. APIs are designed with the user in mind
<br> we've actually failed to do so in several occasions
<br> resulting in new ioctls being added later
<br> to overcome limitations
mchehab: no, we never assume that userspace would be forced to convert to a very specific format if performance is needed
pinchartl: kernelspace to userspace APIs need to take userspace into account
mchehab: but that's exactly what you're wanting to do here
<br> what's wrong if userspace won't convert object IDs into pointers?
<br> nothing
<br> we should not try to enforce design decisions on userspace
pinchartl: it makes their life much more difficult. if they want to shoot themselves in the foot, sure, but they can't complain :-)
mchehab: whatever works best for the specific userspace case
<br> I'm running out of time
pinchartl: apart from debug messages, what are the use cases you can see in userspace for finding out the type of an object without looking it up ?
<br> I can be convinced by real use cases, but I haven't seen one yet
mchehab: to use only the links it wants, discarding the rest
<br> why to even care about the links that the app will never use?
pinchartl: can you give me an example ?
mchehab: you should remind that, with dynamic changes, there will be an endless loop reading entities
pinchartl: one enumeration per change, yes
mchehab: userspace won't want to recreate everything every time
<br> it would want to just update the relevant changes on the object types he's interested on
<br> discarding the rest
pinchartl: I envision that userspace will discard objects with the same ID
mchehab: with some type. yes
pinchartl: still, especially for dynamic updates, usage of the library will be quite crucial
<br> it will be way too easy to get it wrong in userspace
mchehab: <u>pinchartl</u>: the library is a separate discussion
pinchartl: so is dynamic updates, yes. I think it will be a long discussion, but it's separate
<br> (fortunately)
mchehab: I really think that the library should do the minimal amount of alloc/free if we want it to be used on dynamic apps
pinchartl: although we'll want to make sure that the MC API can support dynamic updates
<br> I'd like to avoid a v3 when we'll add dynamic updates supports
mchehab: yes, me too
<br> that's why I think that we should take the O(1) design
<br> it won't hurt if userspace would do the way you're thinking...
<br> but it may help on the dynamic update case
pinchartl: I think we need something better than maybe's. we need to design the dynamic updates case and validate it as much as possible
mchehab: <u>pinchartl</u>: we need to do that in parts...
pinchartl: but back to the question, apart from debug messages, what are the use cases you can see in userspace for finding out the type of an object without looking it up ? can you give me an example ?
mchehab: it is already a chaos to maintain 70 patches out-of-the-tree
<br> <u>yes</u>: userspace may be interested only in interfaces->entities links
pinchartl: what's a typical userspace use case for that ?
mchehab: and interfaces and referenced links
pinchartl: all I'm hearing is "maybe userspace would benefit from having the type and ID combined", but without any concrete example
mchehab: a replacement for libmedia_dev
<br> v4l-utils/utils/libmedia_dev/get_media_devices.c
pinchartl: http://git.ideasonboard.org/media-enum.git
<br> it already exists :-)
hverkuil: back
pinchartl: needs to be ported to MC v2 of course
mchehab: <u>pinchartl</u>: it has to be rewritten ;)
pinchartl: no issue with that
<br> and it's based on the MC library by the way
mchehab: G_TOPOLOGY offers different ways to do it
<br> making the current MC library supporting G_TOPOLOGY is actually a big nightmare
hverkuil: my question is: how would userspace benefit from splitting off the type. I don't see anyway it would benefit from that. It may not matter at all to userspace in the end, but neither does it hurt.
mchehab: it is easier, IMHO, to write a library taking G_TOPOLOGY into account and then add the backward bits
<br> than doing the reverse
<br> believe-me, I tried
pinchartl: we've never committed to the MC library API, so it can be changed, that's not much of an issue. separate discussion though
mchehab: <u>hverkuil</u>: agreed. actually, userspace could hurt with the O(log(n)) design, with is a good reason to use the O(1) approach
<br> as javier__ mentioned: enumerate pros/cons...
<br> I can't see any cons on using type+ID
<br> but I do see gains on doing that, because type+ID is O(1)
pinchartl: <u>hverkuil</u>: one thing that comes to mind is possible abuse of the links API
<br> but I don't really follow the "let's do it because there's no drawback" argument
javier__: <u>pinchartl</u>: I really think is a matter of personal preference. mchehab, hverkuil and myself like the type + ID in a single field
mchehab: <u>pinchartl</u>: what abuse?
pinchartl: otherwise we could go for the extreme solution of combining everything in a bitfield and be done with it
javier__: <u>pinchartl</u>: sailus and you don't
pinchartl: and nobody would think it's a good idea (hopefully at least :-))
hverkuil: Mauro did a lot of work. I am perfectly happy with it as well. So for Mauro to change it you need a good argument. And I don't see one, other than a personal design preference.
mchehab: <u>pinchartl</u>: let's not invent weird solutions to negate that... this is not a logic argument ;)
pinchartl: you want to combine two separate pieces of information in a single u32, and I still haven't been given any use case for that
hverkuil: you want to split two separate pieces of information into two u32's, and I still haven't been given any use case for that
<br> :-)
<br> Both approaches will work. No doubt about that.
pinchartl: I want to store two separate pieces of information in two different fields, where do you see anything weird about that ? :-)
<br> why don't you make the link sink and source fields u64 and store (type << 32) | id ? :-)
mchehab: why are you proposing that?
javier__: <u>pinchartl</u>: I believe that's the thing. I don't believe those 2 types of information are that separate
pinchartl: to show that it doesn't make more sense
javier__: as each type will have their own number space
mchehab: that shows nothing
hverkuil: Nothing weird about that, but in this case I think there is a real benefit of having an O(1) lookup of the type.
pinchartl: <u>javier__</u>: no, we've decided last time that there will be a single number space
javier__: <u>pinchartl</u>: is like with IP addresses, a single u32 encode both the network and host
hverkuil: So I see no need for a rewrite.
pinchartl: if there were separate number spaces then we'd need to carry the type too, of course
mchehab: we might spend hours, days, weeks, months, years enumerating lots of cases... just time being throwing away...
<br> are there any CLEAR disadvantage of joining type+ID?
pinchartl: I'm not asking for years or use cases, I'd like *one*
mchehab: we provided
<br> printk/printf
<br> we provided a second one
pinchartl: one beside debug messages, as stated above :-)
mchehab: filtering out data based on the type
pinchartl: but what's the use case of that ?
<br> as far as I can see it could be used to filter links only
<br> what would be the use case of that ?
mchehab: all is told above
<br> you doesn't seem to be listening
pinchartl: I could say exactly the same
<br> and it's extremely demotivating
mchehab: (08:00:45) mchehab: v4l-utils/utils/libmedia_dev/get_media_devices.c
<br> it is demotivating when all it seems you want to do is to prevent any changes at the code
pinchartl: my goal is to create a good API
mchehab: as you've been reluctant even to add printks at the code
pinchartl: my only motivation is technical
mchehab: sorry, but it doesn't look so
<br> why adding a printk would affect the API
<br> ?
pinchartl: what are you talking about ?
mchehab: your comment to patch 7/8 for example
<br> do you have any hidden agenda?
pinchartl: API and implementation, of course
<br> I'll have to use and maintain the code
<br> I'd like that to be as painless as possible
mchehab: <u>pinchartl</u>: it is beeing really painful
<br> as, for every single design decision you want to see the hole code, including userspace done
pinchartl: how can one possibly design an API without seeing the big picture ?
mchehab: things will only move forward if we do it step by step
pinchartl: there's nothing wrong with a step by step implementation, the only problem is that all the steps must be done before merging the API to mainline
mchehab: in this specific case: let's not add an O(log(n)) logic where an O(1) would work, and it is already implemented
<br> did we merge a single bit of MCv2 at mainline? No, we didn't.
pinchartl: have I said we have ? :-)
mchehab: we need to take one step-by-step decision here:
***: tfiga has quit IRC (Ping timeout: 244 seconds)
mchehab: use the O(log(n)) design for no good reason or use the O(1) design, with is already done
<br> why doing the O(log(n)) design?
<br> if there's no good reason, let's do the O(1).
<br> the question is as simple as that
pinchartl: regarding the step by step approach
hverkuil: Sorry Laurent, I honestly agree with Mauro here.
pinchartl: what's the latitude we have to come back to previous steps and perform extensive modifications ?
<br> I don't want to *commit* to a particular step without the full picture being in place, but I can say that a particular step looks good enough for now and that we can move to the next question, as long as we can go back later and change things if needed
<br> (and the amount of work that has been performed is not an argument in either direction in that case)
hverkuil: if there are clear technical reasons why something was a bad idea, then we should go back and change it before it is merged.
<br> Regardless of the amount of work.
pinchartl: <u>hverkuil</u>: that's how I see it too
hverkuil: At the moment, I don't see a reason to redesign it.
<br> That might change, but I don't expect it to. But I have been wrong before.
pinchartl: I still don't like combining type + id, but I'm fine moving forward with the rest without waiting on a agreement on that point
<br> however, regarding the userspace library, we'll need a pointer-based API
<br> that's doesn't call for separate fields for type and ID
<br> but we'll need it anyway, so that part will need to be rewritten
hverkuil: Agreed on that. That's always been clear.
pinchartl: or possibly implemented separately, and then media-ctl (or rather its new version) being implemented on top of that
mchehab: <u>pinchartl</u>: ok, then let's move forward, at Kernel side with type+id
pinchartl: why on the kernel side too ?
<br> isn't it enough to combine them both in the ioctls ?
<br> the kernel uses pointers anyway
mchehab: I mean: kABI/kAPI
pinchartl: right
<br> so in the media object structure in the kernel we'll still have two separate fields, right ?
mchehab: what I actually meant to say is that userspace will be discussed/designed in the future
<br> <u>pinchartl</u>: it was designed with a single field
<br> at the Kernel side
hverkuil: !@#$@#%@#: neighbor starts drilling :-(
pinchartl: is there a reason to combine them together inside the kernel ?
mchehab: there's not a good reason to change it, regardless what we do at the kAPI
pinchartl: I mean in struct media_gobj
mchehab: yet, as you pointed, this could be changed latter anytime
<br> on a separate patchset if this would ever be a problem
pinchartl: it's very easy to change it now, inside the kernel I'd be even more strongly against combining them
<br> *if* we combine them, I believe it should be in the uAPI only
mchehab: i'm not sure if it is that easy to change them at the core
<br> need to verify
pinchartl: sure
mchehab: we're using macros to get the type, so in thesis, not much has to be changed
pinchartl: I don't think it would be difficult
mchehab: but I guess there are some optimizations that rely on that at some graph traversal routines
pinchartl: but if it ends up being difficult then it means the kernel framework design somehow hardcodes the assumption that type and ids must be combined, which would seem to me like an indication that something is wrong
mchehab: no, is not a matter of being difficult
<br> it is more a matter of checking the impact of such change
<br> and doing the real work
<br> in any case, this is something that the effort of doing now or doing latter is the same
<br> and there's no benefit on doing it
pinchartl: there's a benefit, it's much cleaner
<br> it doesn't have to be done just today
mchehab: no, it isn't. the cases where the type is needed as type are handled with
<br> media_type()
pinchartl: if you feel like I push back on every change that you want to make, I feel you want to ignore every suggestion I make. I'm not sure how we got there, but it's a pretty bad situation
mchehab: and where we need the local ID, media_local_id()
<br> so, it is a matter of looking on those places
<br> media_local_id() could be replaced by a direct access to the object ID, but I would keep the macro even with the change (perhaps renaming it)...
pinchartl: I'm fine with keeping the macro
<br> I would call it media_gobj_id() though
mchehab: so, I guess the change would actually be just at the struct
pinchartl: and media_gobj_type()
<br> as they both act on a struct media_gobj instance
mchehab: well, increasing the macro name would require more work...
hverkuil: <u>pinchartl</u>: it's not ignoring (we've been discussing this for ages, so you're certainly not ignored), it's an honest disagreement on this specific bit of the design.
mchehab: as it will cause 80-cols warnings to be solved to make checkpatch.ph happy
pinchartl: right
<br> maybe media_id() then
<br> it's a bit short though
mchehab: that works
pinchartl: note that media_gobj_id() is shorter than media_local_id() ;-)
mchehab: anyway, let's postpone this cleanup thing to the end
pinchartl: <u>hverkuil</u>: ignoring was the wrong word. I meant considering they are all wrong
mchehab: as there will be almost zero impact
pinchartl: when do you plan to rebase by the way ? at the end ?
mchehab: and there are still other important things to be done
<br> <u>pinchartl</u>: I don't plan to rebase anymore
<br> rebasing a /55 patch series cause serious regression risks
pinchartl: I'm concerned about that, bisection will break
<br> I think at least one final rebase is needed
sailus: <u>mchehab</u>: Consider doing that using Coccinelle, it automatically splits the lines.
mchehab: I will actually insert some Javier patches in the middle
<br> <u>sailus</u>: coccinelle removes comments
pinchartl: and I believe we're at a stage where, if we rebase the patches now, an initial part of the series could be merged
mchehab: <u>pinchartl</u>: I'm OK with that
<br> a final rebase before merging is indeed needed
<br> I just don't want to do intermediate rebases anymore
pinchartl: it's not specific to MC development, rebasing is part of the traditional kernel development workflow (for best or worse)
<br> right
<br> I don't think we need frequent rebases
mchehab: well, he had 9 rebases already... (actually, internally I did a lot more)
pinchartl: but if it can allow merging an initial part of the series, I think an intermediate rebase might be helpful to decrease the size of the patch set
mchehab: <u>pinchartl</u>: right now, there were very few patches fixing stuff outside the /55
<br> maybe 3 or 4 patches only
<br> (the ones addressing Sakari's comments)
pinchartl: in which branch is the latest patch series ?
<br> I'm looking at http://git.linuxtv.org/cgit.cgi/mchehab/experimental.git/
<br> there are quite a few v8.3 branches
mchehab: yes...
<br> let me explain what's there... just a sec...
<br> v8.3+fixes have the /55 plus the /18 patch series
<br> v8.3 is just the /55
pinchartl: there are several versions of v8 patches that got posted to the mailing list and I'm not always sure to know which one is the latest one
mchehab: the v8.3+fixes+javier has a merge from javier__'s tree
<br> the patches there will need to be inserted in the middle
<br> javier__ actually wrote a few patches after my merge...
<br> so I'll actually pull back from his tree on the final rebase and insert his patches before the one that convert links from array to a list
<br> this will ensure no bisect breakages
<br> the branches there marked with WIP are incomplete work
pinchartl: so which branch should I review ? :-)
mchehab: mc_next_gen.v8.3+test
<br> and the patches that javier__ submitted to the ML
<br> <u>javier__</u>: what series should pinchartl review?
<br> as part of the MC nextgen rework, I mean
sailus: <u>javier__</u>: Are your omap3isp patches before the media_device_init() / media_device_register() split still needed btw.?
mchehab: <u>sailus</u>: yes, those are needed
sailus: <u>mchehab</u>: What do they achieve?
mchehab: all graph objects need to be added at the media_device linked lits
<br> s/lits/lists/
<br> as those linked lists are the ones used by G_TOPOLOGY
sailus: <u>mchehab</u>: Please see: [PATCH 0/2] [media] Fix race between graph enumeration and entities registration
<br> The linked list is now initialised quite early.
pinchartl: <u>mchehab</u>: would it be a lot of work to put all patches in one branch ?
<br> that would help for review
mchehab: you mean my patches + javier__'s one?
sailus: Or, well, actually should be.
<br> But it isn't.
pinchartl: yes. all the patches that are needed
mchehab: ok, I can do that
pinchartl: it's a bit painful without a rebase as I'll find things to comment on that are fixed by later patches
mchehab: I just don't want to send a /80 patch series to the main ML
pinchartl: I can probably live with that though
<br> I'll start by reviewing the topmost patches and move down
mchehab: I can probably send to your emails or the the workshop ML, if you prefer having it via e-mail
<br> yes, going from top to the bottom may make more sense for you
pinchartl: a branch will be enough
mchehab: OK, I'll do that
pinchartl: I'll then reply to the patches from the ML
<br> I just want to make sure I don't forget some of them
mchehab: I'll call it mc_next_gen_v8.4
<br> ah, javier__ is out for lunch
<br> I'll get the right patches with him after his return
<br> and put everything there
sailus: I think media_device should be initialised early so there's no need to delay working with it.
mchehab: <u>sailus</u>: yes, media_device should be initialized early
<br> as soon it gets initialized, the better
sailus: I'll still discuss that with Javier.
mchehab: and registering it late only after having the entire graph there is the best thing to do, IMHO
sailus: I thought his patches would have done it, but apparently not.
<br> <u>mchehab</u>: Agreed.
mchehab: I'll live the actual approach for you both to decide
sailus: The patches now move it to the V4L2 async complete callback on DT based drivers.
mchehab: seems OK to me
<br> <u>pinchartl</u>: my plan, after getting the remaining acks from you/sakari is to merge them at the media-controller topic branch...
<br> that should help people to test..
<br> merging back at upstream only after we all agree that what's there is ok for upstream merge
<br> I'm actually meaning:
<br> http://git.linuxtv.org/cgit.cgi/media_tree.git/log/?h=media-controller
<br> once having the acks
<br> and http://git.linuxtv.org/cgit.cgi/media_tree.git/log/ after we're OK with G_TOPOLOGY
<br> that should be enough for the first Kernel version with the MC nextgen rework
<br> (hopefully, 4.4)
sailus: <u>mchehab</u>: Sounds good.
mchehab: leaving SETUP_LINKS_V2 to the -next kernel version
pinchartl: <u>mchehab</u>: ok
mchehab: (probably together with dynamic support - yet to be discussed)
<br> ok, it sounds like a plan!
<br> thanks everyone!
sailus: <u>mchehab</u>: Thanks!
pinchartl: <u>mchehab</u>: you're welcome
<br> <u>mchehab</u>: we should try to find a way to defuse all this, I want to go back to a more serene situation
mchehab: <u>pinchartl</u>: that's all I want!
pinchartl: so do I
<br> it's not personal as far as I'm concerned
mchehab: neither it is personal from my side
<br> I guess we're just too passional when trying to write the best code
pinchartl: there have been communication problems too
mchehab: I propose to solve all of this on a Korean barbecue ;)
pinchartl: :-)
mchehab: that will be on me :)
javier__: sorry I was out for lunch and doing some errands
-: javier__ reads the backlog
javier__: <u>mchebab</u>: the patches that pinchartl should revied is the "[PATCH 0/5] [media] Create pads links after entities registration" series
<br> *review
sailus: <u>javier__</u>: Do we need these patches, assuming media_device_init() will be moved earlier in probe()?
<br> That's what I missed to comment in your later series.
javier__: <u>sailus</u>: I still going through the backlog so sorry if that was discussed already
<br> let me finish then before doing any other comment
<br> <u>sailus</u>: yes those patches are still needed
<br> because there are 2 issues
<br> when the media device is registered (so the lists are initialized) and when the entities are registered (so the mdev is avaialble to get the obj IDs)
<br> <u>sailus</u>: my patches deal with the later since pads where created before the entities were registered with the media device so a link ID couldn't be created witouth an mdev
<br> <u>sailus</u>: while the patch that splits media_device_register() deals with the former
<br> <u>sailus</u>: which is not really related to mc next gen since is a current issue
<br> <u>sailus</u>: but the fact that entities have to be registered with the media device after an entity has been registered with a media device is a new requirement now that links are list and get an ID from the mdev embedded obj
<br> mchehab, pinchartl ^
mchehab: <u>javier__</u>: OK, so I'll add your 5 patches before the patch that changes links from arrays to lists...
javier__: <u>mchehab</u>: exactly
<br> my second set IMHO should not block mc next gen and can be picked later as a followup
mchehab: and I'll let you/sailus finish the discussions with the 2-patch series and add them at the end
<br> OK
<br> I'll handle those two as a followup after merging the initial series at the topic branch
javier__: <u>mchehab</u>: agreed
sailus: <u>javier__</u>: The lists are now initialised in media_device_init(), not in media_device_register().
javier__: <u>sailus</u>: yes but that is a separate issue...
***: koike_ is now known as koike
javier__: <u>sailus</u>: look at media_device_register_entity() with mc next gen
<br> entity->graph_obj.mdev = mdev
sailus: Your patch moves media_device_register() to the very last operation, so the link creation cannot depend on it, can it?
javier__: <u>sailus</u>: it does not depend on media_device_register() but on media_device_register_entity()
<br> since media_create_pad_link() needs an entity to have an mdev set
<br> for media_gobj_init(source->graph_obj.mdev, MEDIA_GRAPH_LINK,...);
<br> <u>sailus</u>: that's the issue that my split pad link creation does
<br> s/does/fixes
<br> <u>sailus</u>: of course the media device has to be registered before the entities but that was always the case
<br> <u>sailus</u>: what my second series do is just to delay the media device node creation so user space is not able to call ioctl syscalls before the driver finished setting the graph
sailus: <u>javier__</u>: Ok, got it.
javier__: <u>sailus</u>: great!
koike: mchehab, Hi! I would like to ask you about the virtual driver (vimc), as it seems the MC API will have a lot of changes, ideally should the vimc driver be merged before of after these changes?
mchehab: <u>koike</u>: better to do it after it
koike: I am asking this to see if I wait for the MC API changes to keep going with the vimc development or not
<br> ok
mchehab: the internal changes are already done
<br> I guess the perfect moment to merge is after merging it to the media-controller topic
koike: newby question (I still need to keep up with all the changes being made), what do you mean by media-controller topic? Its the set of changes planned for the media-controller?
mchehab: media_tree branch "media-controller"
<br> I'll be merging soon the patches from my experimental tree there
<br> "soon" means after getting pinchartl and sailus acks
<br> <u>pinchartl</u>: all patches are at devel/mc_next_gen.v8.4 branch
<br> except for the 2 patches that sailus and javier__ are still discussing and are not related to the MC nextgen
<br> (that delay media devnode creation to happen after updating everything)
javier__: <u>mchehab</u>: which are not really a dependency for your set since fixes a current inssue in mainline
<br> I just based on top to avoid conflicts
mchehab: I made some effort to reorder them to avoid runtime bisectability breakages
sailus: <u>mchehab</u>: Could you send v9 to the list when you have it, please?
mchehab: I'll check it again before merging at the topic branch
<br> <u>sailus</u>: 82 patches
<br> too much for just resending
<br> if you want, I may send it just for you or to the MC ML
<br> but I don't want to send an 82 patches to the main ML specially provided that they're untouched
sailus: <u>mchehab</u>: Wow.
<br> <u>mchehab</u>: Are they?
pinchartl: <u>mchehab</u>: thank you
sailus: I thought I had comments on them.
<br> Or did you intend to keep the changes on top of the v8?
<br> I mean, changes done based on the review.
mchehab: <u>sailus</u>: on the top of v8
<br> at least during the review time
<br> I'll very likely merge them on the final series, when applying them
sailus: I'd prefer merging them before the review.
<br> If there are issues, they are spotted there much easier.
mchehab: if they won't cause the need of retest
sailus: That's why we review. :-)
<br> Merging should not be done *after* the review.
mchehab: <u>sailus</u>: resending 82 patches due to a few changes is not practical
sailus: Well, you're the maintainer but this is my opinion. :-)
mchehab: just 2 patches with changes from your review
<br> <u>sailus</u>: if one of you would sending me 82 patches for me to re-review, I would not like it ;)
-: mchehab with maintainer's hat
mchehab: specially due to 2 changes
<br> actually, several times I asked people to do like that...
<br> add the patches with the reviewed changes at the end
<br> to save me time to re-read and re-review everything
<br> anyway, I need to go
<br> I'm travelling in a bit
shuah: safe travels
sailus: <u>mchehab</u>: In that case, you shouldn't merge them at all.
<br> If it doesn't break compilation or have other serious matters, I guess it's ok.
<br> <u>mchehab</u>: Have you looked at the media entity ID range patches btw.?
<br> I need to leave now.
<br> Have a nice week-end!
mchehab: <u>sailus</u>: thank!
<br> thanks
<br> did just a quick look on your series. I intend to do a deeper look hopefully duirng the weekend
<br> but I can't promise, as I'll be in a short trip
<br> and then going to China on Monday
<br> afaik no patch breaks compilation
<br> even before Javier's patches
<br> anyway, I'll do a final check before merging at the topic branch
<br> and check for other trivial issues, like checkpatch, W=1 warnings, etc
<br> as I usually do when anyone else sends me a patchset
<br> have a nice weekend!
-: mchehab is leaving now
***: _daniel_ has left
<br> capOM has quit IRC (Quit: Konversation terminated!)
larsc: there is a proposal to export error information from the kernel to userspace in json, maybe we should use that for exporting the mctl graph as well ;)
***: Muzer has quit IRC (Ping timeout: 264 seconds)
hverkuil: <u>sailus</u>: I've reviewed your vb2 patch series. As you can see I have a bunch of concerns.
<br> I think you cannot do this reliably until we are much more explicit in our drivers when they need cpu access. You have a much better foundation to add this sync support once that's done. IMHO.
<br> The good news: I have code. The bad news: not all drivers are converted to use begin/end_cpu_access.
javier__: <u>sailus</u>: sorry it's not clear to me if you agreed at the end with removing the WARN_ON() in media_device_init()
<br> I know you suggested to change to BUG_ON() but I explained why I disagree about that
<br> already changed the function to void so the options for me are to either keep the WARN_ON() but no return an error code or just remove it
<br> <u>sailus</u>: I'll wait for your answer before posting v2 to be sure that I didn't misunderstand you again :)
***: javier__ has quit IRC (Ping timeout: 260 seconds)
<br> pocek has quit IRC (Ping timeout: 256 seconds)
<br> _abbenormal has quit IRC (Quit: Yup Im Leaving)
<br> awalls has left
<br> pinchartl has quit IRC (Ping timeout: 252 seconds)