<!-- 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>
***: Voce changes his alias to mchehab
tasslehoff: When I launch gstreamer with v4l2src, and width=800, height=600, soc_camera just starts trying all sorts of odd formats (like soc_camera_try_fmt: YUYV, 1x1). It never tries the format I want, though :)
javier__: <u>sailus</u>: hi, is this what you had in mind to avoid the early links enumeration? http://hastebin.com/qofedupise.coffee
pinchartl: <u>javier__</u>: haven't checked the details, but that looks like it. maybe v4l2_device_register_subdev_nodes() could then be included in media_device_register()
javier__: <u>pinchartl</u>: great, I'll do that then and also change the other media platform drivers and post as a proper patch later today
<br> <u>pinchartl</u>: thanks for the feedback
pinchartl: you're welcome
<br> but please ask Sakari for feedback too
javier__: <u>pinchartl</u>: yes, that's why I don't plan to post it until later today in case he has some comments
pinchartl: I've pinged him about that
javier__: <u>pinchartl</u>: thanks a lot
sailus: <u>javier__</u>: Yes, it was.
<br> I'd make it return void, though, as the sole source of error would have to be a driver bug.
javier__: <u>sailus</u>: you mean for media_device_init() right? I in fact thought about it but decided to keep the check to don't make it less safe
sailus: :-)
<br> Correct.
<br> You can add my ack, but please use my @linux.intel.com e-mail address.
javier__: <u>sailus</u>: will do, thanks a lot for your feedback
sailus: Thanks!
hverkuil: <u>pinchartl</u>: can you review this patch? https://patchwork.linuxtv.org/patch/30979/
<br> That would unblock this patch series, allowing me to get it into 4.4.
<br> this week or next week would be fine.
pinchartl: <u>hverkuil</u>: I'll try to do that on Friday
javier__: <u>pinchartl</u>: I'm not sure about moving v4l2_device_register_subdev_nodes() into media_device_register() since not all drivers that register a media device do that
pinchartl: and it would be a layering violation anyway
<br> please scratch that proposal
javier__: <u>pinchartl</u>: OK
pinchartl: I would have nacked it if someone had proposed it ;-)
javier__: :)
sailus: <u>javier__</u>: Btw. we should also have media_device_destroy() --- the mutex should be destroyed.
<br> It was not done previously at all.
<br> Now that you're at it, I thought to mention this. :-)
<br> Or, perhaps, media_device_cleanup().
javier__: <u>sailus</u>: I'll prefer to do it at media_device_unregister() instead of having a _cleanup() function
sailus: It'd be one option.
javier__: I don't think a use case is for a driver to init a media dev, register, unregister and register it again
ribasushi: <u>pinchartl</u>: hi!
sailus: media_device_unregister() should cope with getting called without media_device_register() before that.
javier__: In fact I would like to get rid of media_entity_cleanup() as well :)
ribasushi: <u>pinchartl</u>: it seems that https://patchwork.linuxtv.org/patch/30695/ never entered into any of the maint trees
<br> <u>pinchartl</u>: is there a rough ETA on this / something holding it up / did it just get forgotten
javier__: <u>sailus</u>: hmm, but what would be the point to unregister something that was not registered
<br> <u>sailus</u>: but I'll look on that
pinchartl: <u>ribasushi</u>: I've just sent a pull request. thank you for the reminder
ribasushi: <u>pinchartl</u>: can you link me to the request, so I can monitor it without bothering you?
pinchartl: "[GIT PULL FOR v4.4] uvcvideo fixes" sent to linux-media
<br> there will be no e-mail reply though
ribasushi: <u>pinchartl</u>: will it appear here eventually? http://git.linuxtv.org/cgit.cgi/media_tree.git/log/
<br> or I am looking at the wrong tree?
pinchartl: yes it will
ribasushi: pinchartl++ # thank you much sir
pinchartl: you're welcome
<br> sorry for the delay
ribasushi: it is what it is, especially given I wasn't able to do it myself
javier__: <u>sailus</u>: I see what you meant, the problem with destroying the mutex in media_device_unregister() is that it will never be destroyed if media_device_register() fails
<br> in any case, this is a separate issue so I'll think about it after finishing the init and registration patch
tasslehoff: hm. seems gstreamer tries all sort of weird formats because enum_framsizes is not implemented for my camera.
<br> but, none of the other soc_camera drivers have implemented it either
sailus: <u>javier__</u>: Fine with me.
<br> I might still rename media_device_unregister() as media_device_unregister_cleanup(). It's perhaps not a pretty name, but suggests what it does, since the symmetry is missing with the addition of media_device_init().
javier__: <u>sailus</u>: in fact, I'll add a media_device_cleanup() at the end since I noticed that also a v4l2_device_unregister_subdev_nodes() does not exist
<br> and sicne I've to audit all the error code paths for the drivers using media_device_register(), I prefer to fix all issues for once
sailus: Ok. :-)
<br> Even better that way.
<br> Thanks!
javier__: <u>sailus</u>: thanks to you for the feedback
mchehab: <u>sailus</u>: just send two patches addressing the issues you pointed and that I've agreed ;)
<br> there are two things left, that I'm waiting either for your ack or for your reply:
<br> 1) not adding pad_index at this point, postponing its addition to when we implement SETUP_LINK_V2
sailus: <u>mchehab</u>: Could you merge the changes to the original patches?
mchehab: I'm not really against a pad_index. I just think that it is better to have the visibility of the properties API before implementing it
sailus: I've just received the first, waiting to see the second.
mchehab: <u>sailus</u>: I really prefer not...
sailus: Any particular reason why not?
mchehab: there are too many patches already pending merge
<br> rebasing the tree with a huge pile of patches is very painful
sailus: Yeah, there are many, but if you add two, there are two more. :-)
mchehab: and may introduce bugs
<br> my main concern is with bugs introdution due to rebase
<br> that happens a lot when rebasing big patch series
sailus: Noo... why would you introduce bugs in a rebase? :-)
mchehab: because....
<br> I'm just a human :-D
sailus: X-)
mchehab: I spent hours fixing bugs introduced on the last rebase done due to the hverkuil's request
<br> introduced by a silly mistake
sailus: Seriously, I've yet to read some of your replies.
<br> I'll try to do that later today.
mchehab: thanks for that
hverkuil: I've reserved the whole Friday for doing my MC reviews :-)
mchehab: 2) (on the open issues) the object ID range. I think it looks better with one range per object type, but we need a minimum of two ranges, otherwise we'll break both kernelspace and userspace
sailus: <u>mchehab</u>: Why one range per object type?
mchehab: see my comments on the e-mail
sailus: That's one of the few remaining things I dislike in the latest set.
<br> I'll read that right now.
pinchartl: <u>mchehab</u>: why do we need a minimum of two ranges ?
mchehab: <u>pinchartl</u>: both Kernelspace and userspace relies on having entities < 32 or <64 (depending on the driver)
sailus: <u>mchehab</u>: The user space shouldn't depend on such things. It's never been a part of the API (as such).
pinchartl: in kernelspace we'll need to fix it anyway. we could have a temporary range internal to the kernel until drivers get fixed though
<br> what userspace relies on that ?
sailus: Some drivers would need fixing, and the graph traversal algorithm..
pinchartl: yes, kernel code will need to be fixed
<br> I'm fine with a temporary hack inside the kernel to buy us a bit of time to fix these problems
mchehab: <u>pinchartl</u>: any app that may be using the same graph algorithm that it is used at Kernelspace
<br> I bet some apps would just copy the algorithm there on userspace
<br> so, it is too risky of changing it
<br> we'll need to fix Kernelspace anyway, but this is a separate independent issue
pinchartl: the API has never given that guarantee to applications
<br> and it even explicitly mentions that code must not rely on IDs being contiguous
mchehab: well try using entity = 0
<br> you'll see that media-ctl will break :-p
<br> unfortunately, userspace do rely at the numberspace
pinchartl: have you seen userspace applications relying on the id being < 32 ?
mchehab: didn't actually seek for it
<br> what I did notice was some weird behaviors at media-ctl depending on how the range was changed
<br> entity ID = 0 was one of the bad things I noticed there
pinchartl: we could expose different IDs in v1 and v2 of the API, but that's messy
mchehab: it considers the links invalid if entity_ID is zero
<br> yes
<br> that's why I think using a separate range for entity is the best approach
pinchartl: that's even messier :-)
sailus: <u>mchehab</u>: I just checked with our user space developers.
<br> Guess what.
mchehab: and the only approach that it is garanteed to not have userspace breakages
pinchartl: entity ID 0 isn't valid according to the current API anyway
sailus: They use entity IDs only to perform the enumeration.
<br> I wouldn't be surprised if others did the same, especially when it comes to hardware specific applications.
<br> And we have practically no generic applications, at least not ones I'd be aware of.
<br> Future applications do not count.
<br> They will use new interfaces anyway.
mchehab: <u>sailus</u>: the problem is that, as there are no generic apps/no generic library that everybody would be using, it is really unpredictable
<br> about what people are doing
<br> we use the lowest range right now at graph traversal routines to detect loops
sailus: <u>mchehab</u>: The algorithm in the kernel is under GPL.
pinchartl: everything is unpredictable. buggy applications can break when we make perfectly valid changes to kernel code
mchehab: if one would need to do the same thing on userspace, they'll very likely doing something similar to the Kernel algorithm
sailus: If people would use it outside the kernel, it should be probably easy to find it by googling a bit.
<br> Or, well, the implementation of the algorithm is.
mchehab: <u>sailus</u>: the implementation is, but using the bits on an u32/u64 integer to detect loops is the obvious and fastest solution
<br> there's nothing bad on doing that, specially if you know, in advance, that there will be no more than 32 (or 64) entities
hverkuil: I'd keep it simple: 0 is reserved (as it is today), 1-63 are used for the first 63 entities, anything else uses IDs 64 and up. So you need two counters: one for entities, one for everything else.
sailus: MEDIA_ENTITY_ENUM_MAX_ID is also an internal definition. It'd be very bad design to import that to the user space.
pinchartl: <u>hverkuil</u>: we need to plan for more than 63 entities. would you give them higher IDs ?
hverkuil: Ideally we could use just one counter, but I agree with mchehab that this might break apps.
sailus: MEDIA_ENTITY_ENUM_MAX_ID has been 64 only since there never has been a need to have more entities.
<br> It'd change the day someone would need more than that.
hverkuil: <u>pinchartl</u>: yes.
pinchartl: if we go for two counters, wouldn't it make more sense to keep the current counter just for entities, and add a new counter for graph objects ? entities would have two different IDs, one for v1 of the API, and one for v2
mchehab: <u>sailus</u>: MEDIA_ENTITY_ENUM_MAX_ID needs to be increased or either removed
sailus: Be it a board that has a slightly larger number of sensors, lens or flash devices. Not necessarily even a new driver would be needed.
hverkuil: this is purely to keep compat with existing drivers.
mchehab: but some drivers don't use MEDIA_ENTITY_ENUM_MAX_ID. Instead, they use "32"
<br> hardcoded there
sailus: <u>mchehab</u>: Ouch.
mchehab: IMHO, there's no need to touch on those drivers, as they only have <32 entities
pinchartl: drivers can be fixed, that's not an issue
sailus: <u>mchehab</u>: Can you name one for an example?
mchehab: need to re-check
<br> I got this when doing some remanufacturing thing
hverkuil: (BTW, I thought the ID included the type of the object as well? Or has this been split into separate fields now?)
sailus: <u>hverkuil</u>: Yes, that's a part of the current patchset.
<br> And a reason why this discussion started. :-)
mchehab: <u>sailus</u>: if I not mistaken, I found that pattern on two drivers that call media_entity_graph_walk_next() directly
<br> so, either exynos4-is, omap3isp, vsp1, xilinx, davinci and/or omap4iss
<br> <u>sailus</u>: splitting the type from the ID or not is, IMHO, a completely separate discussion
<br> even if we would use just one or two ranges, I would still embeed the ID with the object
<br> s/object/object type/
sailus: Certainly not an entirely separate discussion.
mchehab: as usually we need both ID and type at the same time, an that means a single memory read operation
<br> it also saves memory, with is always a good thing
sailus: But the proposal reserved 8 highest bits for the type to allow keeping entity IDs small. I think it's good for backwards compatibility, but bad for the future.
<br> Saves memory in the IOCTL argument struct?
mchehab: why is it bad for the future?
<br> saves memory on both ioctl and Kernelspace
sailus: I don't like having arbitrarily large object id numbers for all other objects except entities.
<br> People will read these numbers and write them as well.
mchehab: and accessing an u32 is faster than accessing two u32 integers. Also, u32 read is atomic
<br> on most archs
<br> <u>sailus</u>: you're the one proposing large object IDs ;)
sailus: Come on, you don't really benefit from that, do you?
mchehab: right now, objects are called as "entity#1" "intf_devnode#1"
sailus: I'm proposing keeping all object IDs reasonably small in the vast majority of cases. :-)
mchehab: no, you're proposing to have object IDs large, by using one single range
hverkuil: <u>sailus</u>: I don't think you will typically just use the ID as is. Applications should print them as "type:id" where type is a string based on the 'type' bits of the ID and 'id' is the value of the remaining bits.
sailus: The IDs will only grow if graph objects are dynamically registered.
pinchartl: <u>hverkuil</u>: I'm pretty sure that, at least during development and debugging, IDs will be printed 'raw'. it would be easier to keep them small
mchehab: no, even with statically register, they'll still be large, as we need to reserve the first range for entities
<br> and a DVB device has typically thousands of entities
hverkuil: There really isn't much of a difference between 532 and 237193475482 (or whatever :-) ), both are equally uninformative. But entity:70 and pad:123 is much more understandable.
mchehab: so, the next available number would start at 10000
<br> maybe
hverkuil: It just takes a bit more care in media-ctl and other apps that print IDs.
sailus: <u>hverkuil</u>: As we're designing a new API, I'd add a new field for the type then. The existing range was only used by entities anyway.
mchehab: with your proposal, users would need to type big numbers
sailus: (As an alternative to a common range.)
mchehab: with mine, they'll always use <type>:<low number>
hverkuil: <u>sailus</u>: the problem is that the ID is no longer unique. You need to read two fields, which I think makes the code more complex.
sailus: I'm ok with type:id, but then the type field will always be required.
<br> Instead of just a single numeric ID.
mchehab: yes
<br> well, you could use a single numeric ID if you want:
sailus: How about making the type a separate field for the user space, and keeping it together with the object ID in the kernel?
mchehab: 0x010000010
<br> not human readable
<br> what would be the advantage of that?
<br> my feeling is that keeping type+ID packed together would also be better for userspace
<br> (and my own experience when writing the mc_nextgen_test utility)
sailus: <u>mchehab</u>: It'd be up to the appliation to display it properly.
<br> We should provide convenience functions to access it.
mchehab: see my code
sailus: MEDIA_BITS_PER_TYPE etc. are kernel-only definitions in the current patchset, aren't they?
mchehab: ah, yes, we'll need to put this at the interface
hverkuil: <u>mchehab</u>: I'm confused. You posted replies to a subset of patches 14-55 on September 6th. Are those the v9 versions of those patches?
mchehab: <u>hverkuil</u>: yes
hverkuil: OK, good to know.
mchehab: I wrote a script that only re-sends the patches that got the diff part of the patch changed
<br> to avoid re-sending 25 patches that were otherwise untouched
pinchartl: <u>mchehab</u>: please bump the version number next time, it gets confusing otherwise
mchehab: Ok, will do
sailus: <u>mchehab</u>: Could you move the definitions to the uapi headers, with convenience functions (or macros) to access number and type separately?
mchehab: yes
<br> the idea is to do that...
pinchartl: userspace will use the MC library
mchehab: if we agree with packing them
sailus: ... and how to call the number part of the ID, if the entire field is called ID?
pinchartl: where we'll have structures for graph objects
<br> the structures will contain the ID and type
sailus: type:number?
pinchartl: why would it be simpler for applications if they're stored in a single field ?
hverkuil: <u>sailus</u>: fine by me.
sailus: Or should the field be called type_id, or something else?
<br> Just thinking aloud. :-)
mchehab: <u>pinchartl</u>: it is easier for links representation
pinchartl: the link structures in the kernel and in the library should have pointers to the graph objects on both ends of the link anyway, it won't make a difference
hverkuil: also, IDs are unique. Since all objects are derived from the graph_obj struct it makes sense that the ID is unique for all graph_objs.
pinchartl: that's why I believe a single counter is better, yes
<br> a graph object ID counter
mchehab: we're going in circles...
pinchartl: possibly with a second counter for the legacy API
mchehab: a single ID would break entities
<br> having a separate number for legacy API will be really messy
<br> and would require extra bytes for all entities just due to backward compatibility
<br> right now, a link can be either between PAD->PAD or Entity->interface...
pinchartl: having separate counters is messier
mchehab: but we may need in the future other types of links...
<br> if I remember correctly, larsc mentioned that IIO don't have or need the concept of PADs
<br> so, for IIO, maybe we'll need to come up with entity->entiy link
<br> by using an u32 object_ID that has the type embedded on it, the link will use just the object_ID
<br> so, 2 u32 bit integers are enough to describe the link endpoints
pinchartl: the link can just use the object ID even if the type isn't embedded as long as the IDs are unique
sailus: <u>pinchartl</u>: Indeed.
mchehab: yes, but then we'll need to add something to represent the source/sink types
<br> making the code more complex
hverkuil: yes, but you don't have a clean type lookup anymore. You need to keep track of that in a separate data struct.
mchehab: for no good reason
pinchartl: <u>hverkuil</u>: and I think that's the way it should be
hverkuil: I have absolutely no problem with a few extra counters (one for each graph_obj type).
mchehab: yes, the lookup tables will be more complex
pinchartl: I do
mchehab: why?
pinchartl: concessions will need to be made on all sides otherwise we'll never sort this out
<br> I might be able to accept usage of media_link to describe entity to interface associations, that's already a big concession on my side
hverkuil: An alternative can be to use a single counter that you use for the lowest 24 bits and put the type in the top 8 bits. Basically a bitfield with 8 bits for the type and 24 for the ID.
<br> That way you can do with 2 counters (one global, one for the first 63 entities for backwards compat)
pinchartl: we can't use bitfields for userspace API, can we ? :-(
mchehab: <u>pinchartl</u>: I don't have any big use against using two counters for object ID instead of four, although I prefer using 4
<br> let's not go back to a single counter...
<br> it will break things
<br> we can either use 2 or 4 counters
pinchartl: what will it break ? userspace applications ?
mchehab: not again... we're running in circles
<br> the graph traversal algorithms that assume entities < 32 (or < 64) at core...
<br> and that someone might have used a similar approach at userspace
pinchartl: all kernel code can be fixed
<br> and will need to
mchehab: but not the userspace
<br> no, they wont need... just the core graph traversal needs to be fixed
<br> not the graph traversal done inside the drivers
pinchartl: graph traversal done inside drivers can be fixed too
<br> it's open-source code
sailus: The same code should be used to do the same thing in the framework and the drivers.
mchehab: yes, then can be changed.... but why to spend time there?
sailus: When the framework implementation is fixed, it should also be used by drivers.
mchehab: s/then/they/
<br> <u>sailus</u>: I actually think that we should force all drivers to use the graph traversal inside the core
sailus: It's just been that for keeping track which entities were already handled and which not, a u32 or u64 was enough.
<br> No common implementation was needed.
mchehab: but that's a separate matter
pinchartl: because we're trying to design a proper framework, and adding hacks to avoid changes to drivers isn't how it's done
mchehab: touching on that code would require someone with the hardware to test
sailus: I can do that for omap3.
<br> The current patchset should be also tested on that btw.
mchehab: drivers/media/platform/omap3isp/ispvideo.c: pipe->entities |= 1 << media_entity_id(entity);
<br> drivers/media/platform/vsp1/vsp1_video.c: entities |= 1 << media_entity_id(&entity->subdev.entity);
sailus: <u>mchehab</u>: I told you.
mchehab: drivers/media/platform/omap3isp/ispvideo.c: pipe->entities |= 1 << media_entity_id(entity);
<br> drivers/staging/media/omap4iss/iss.c: iss->crashed |= 1U << media_entity_id(&subdev->entity);
sailus: That's just for keeping track which entities are a part of the pipeline.
hverkuil: <u>pinchartl</u>: keeping the IDs of the first 63 entities in the range 1-63 is purely for keeping *userspace* backwards compatibility. We simply don't know how apps use it since these apps will typically be closed source.
sailus: A different data structure could be used.
hverkuil: Kernel space we can (and should eventually) fix not to rely on this.
mchehab: <u>sailus</u>: those 3 drivers assume a max of 32 entities
<br> omap3, omap4 and vsp1
sailus: <u>mchehab</u>: Yes. But it'd take at least a new board to get over that limit.
pinchartl: <u>hverkuil</u>: how can we know that apps don't use a 32-bit counter ? or, for that matter, a 8-bit counter ?
sailus: But now they need to be fixed for another reason.
pinchartl: or worse, hardcode IDs, even if we've told them not to ?
mchehab: <u>sailus</u>: if we keep the entity range as a separate counter, there's no need to touch on those 3 drivers
sailus: I don't see why we should so much avoid touching them. :-)
mchehab: and we need to do that anyway, as we don't really have any garantee that changing the entity range won't break existing closed source userspace
sailus: It would be a small change to them anyway.
hverkuil: We don't. But when we go to v2 of this API the entity IDs should remain the same.
mchehab: I agree with hverkuil: the entity ID should be the same for v1 and v2
hverkuil: I.e. if the device had 4 entities with IDs 1-4, then in v2 the same should happen.
pinchartl: I disagree
<br> if applications hardcode IDs, too bad for them
<br> keeping the ID lower than 64 or 32 is a separate issue
hverkuil: It's not about hardcoding them, it's about using a bitmask to store IDs in (1 << entity.id).
mchehab: <u>pinchartl</u>: I agree with "if applications hardcode IDs, too bad for them"
<br> yes, the big issue here is with the bitmask
<br> it is just too easy to do it, that even Kernel developers are doing that ;)
sailus: <u>mchehab</u>: I haven't seen a hardware specific user space application doing that.
mchehab: <u>sailus</u>: that doesn't mean they don't
sailus: Well, if you know the hardware, you tend to just look at the entity names.
<br> Generic applications probably do need something like that.
mchehab: still, you could use a bitmask to see if something is deadly wrong
<br> omap4 driver seems to do that:
<br> drivers/staging/media/omap4iss/iss.c: iss->crashed |= 1U << media_entity_id(&subdev->entity);
<br> note the name: "crashed"
sailus: Anyway, I've already told what I think and why.
<br> <u>mchehab</u>: Same thing. Keeping track of which entities are affected, whether it was for start stream error handling, or which ones aren't doing well.
mchehab: I won't doubt that an omap4 userspace code, likely written by the same author, would be doing the same
sailus: A generic, better solution is needed for this.
<br> Luckily it's relatively easy to spot such use in drivers.
hverkuil: I don't get this discussion. This is a tiny part of the API, just there to ensure that we don't break existing apps. It's perhaps not ideal, but that's life. There is no performance impact for all practical purposes.
mchehab: I'm not so sure. Whatever other solution you do, it will be slower
<br> bitmask is simply very fast
-: mchehab is commenting on sailus reply
hverkuil: Whether 2 or 4 counters are used is mostly a preference (one reason I prefer 4 is that with 2 it looks more like a hack, which it is of course)
mchehab: I agree with hverkuil
<br> I'm ok with either 2 or 4
<br> but just one would risk breaking userspace for no good reason
hverkuil: furthermore, the way we count things can be changed later.
mchehab: yes, we can start with 2 and change to 4 or vice versa...
pinchartl: following the same argument of not breaking userspace, we can't
mchehab: but a single counter would save just 4 bytes at the risk of breaking userspace
sailus: But no matter what we do, we have to document that the entire ID space is in use.
mchehab: sure
<br> <u>pinchartl</u>: yes, you're right. changing this latter could be a problem
ankk: hi
<br> what is the best video converter?
mchehab: so, let's try to get a consensus here...
<br> who prefers 2 counters? who prefers 4 counters? any other alternative?
pinchartl: I'd prefer one counter. 2 counter is something I can discuss. I don't want 4 counters
hverkuil: 4, but ok with 2.
<br> OK, 2 it is.
mchehab: 2 is OK for me, although I would prefer 4.
<br> <u>sailus</u>: is that ok for you?
sailus: I prefer one; whether it's two or four are about the same IMO.
mchehab: 2 seems to be the only solution that satisfies we all
sailus: With possibly calling the ID a "graph object ID" and detaching it from the media entity ID in v1 API.
<br> That should resolve user space backwards compatibility worries, too.
pinchartl: I said I can discuss it :-) it also depends on whether we embed the type
sailus: Just because entities were the only thing in v1 API having a media device wide ID space does *not* mean they have to be the same as graph object IDs.
mchehab: <u>sailus</u>: there's just one u32 bits per counter at a system, if we add the conter at media_device
hverkuil: <u>sailus</u>: I thought about having different IDs for the v1 API, but I think that is just really messy. Not worth the effort.
mchehab: but storing a separate ID for v1 and v2 would mean two counters at media_device (one for v1 and one for v2)...
<br> plus one extra u32 per _each_ entity
<br> that's is a way worse and messier
pinchartl: <u>hverkuil</u>: how can we evaluate the risk of using a single counter ?
sailus: v1 entity ID firts in u8. :-)
mchehab: there is a way of doing that...
<br> don't use any counter at all for v1...
sailus: Later on, we can also deprecate v1 API and add a new Kconfig option for including it.
mchehab: just add some logic at media_device that would count the entities
<br> however, that would mean that v1 won't have stable entity ID for dynamic entity changes
pinchartl: and speaking of v1 and v2, I wonder whether we couldn't encourage applications to implement v2 by offering v1 for existing drivers only
<br> v1 is broken for dynamic updates anyway
<br> applications that need dynamic updates must use v2
mchehab: or when the number of entities is bigger than 256
sailus: v1 cannot support dynamic (un)registration if you assume no entity ID re-use and that entity IDs would stay under, say, 64.
<br> <u>pinchartl</u>: I'm in favour of that.
<br> New drivers should use v2 only.
hverkuil: <u>pinchartl</u>: that was my plan as well (keep v1 only for existing drivers)
mchehab: ok, so the plan would be:
<br> 1) use a single range for object_id
<br> 2) v1 would use a counter at the enum_entities function that would count from 1 to n, where n < 256
pinchartl: <u>mchehab</u>: 2) is an interesting idea actually
<br> very interesting
<br> I like it, but let me make sure it won't cause any issue
mchehab: 3) enum links would likely be slower, I guess, as it needs the entity ID, right
<br> so, a separate routine would count it and return the counter
<br> 4) we'll have a flag at media_device struct to tell if the driver supports v1. By default, this flag will be false, and we'll only enable on legacy drivers
pinchartl: that sounds quite good to me
mchehab: hmm... find_entity() is already a loop... it would solve (3)
<br> hverkuil, sailus: would that work for you?
sailus: <u>mchehab</u>: Sounds good to me.
hverkuil: note that setup_link also need to use the 'old-style' entity IDs.
mchehab: yes, but it also calls find_entity
sailus: <u>hverkuil</u>: We need a new IOCTL for that.
mchehab: with will count the entities and produce the number internally
hverkuil: I would perhaps also add a kernel config option to enable v1 support (by default on for now)
sailus: It could simply work on a single graph object ID (link).
<br> <u>hverkuil</u>: I agree.
pinchartl: <u>hverkuil</u>: yes, v1 should be marked as deprecated but enabled by default
mchehab: <u>sailus</u>: if I understood well, you'll be doing the patches that will fix the drivers, right?
hverkuil: With a kernel option you can also choose to have a old_entity_id field as part of the entity struct that is used if the v1 config is set.
pinchartl: <u>sailus</u>: while we're at it, for v2 I'd make it a list of links. SETUP_LINKS instead of SETUP_LINK
sailus: <u>pinchartl</u>: Sounds good to me.
<br> <u>mchehab</u>: We need an efficient way to keep track of entities for various purposes in the framework.
<br> Would you implement that, or me?
mchehab: <u>sailus</u>: not sure what you're meaning?
sailus: <u>mchehab</u>: What did you actually meant btw.?
mchehab: what I meant to say is that the omap3, omap4 and vsp1 drivers need to be fixed
sailus: Entity ID ranges, or API changes in general?
mchehab: plus the core
<br> graph traversal
sailus: Fine for me.
mchehab: in order to work with big entity numbers
sailus: That includes efficient tracking of entities with higher ID numbers.
mchehab: yes
sailus: That's needed anyway for dynamic entity (un)registration, as the IDs can grow quickly over 63.
<br> I'm fine with doing that.
mchehab: well, this is needed only a the core (as omap4, omap3 and vsp1 will never have more than 32 entities as they don't use dynamic entity creation)...
sailus: I'll try to have at least preliminary patches for that early next week.
mchehab: but if we decide by using a single range ID, they'll break
<br> ok, in this case, I'm OK with using a single range ID
<br> anyone against?
<br> no answer. I'm assuming everybody is OK then ;)
<br> good. the next thing is with regards to put both ID and type at the same u32 integer
<br> pinchartl, sailus: any comments, or that would work for you both?
pinchartl: <u>mchehab</u>: I have to go for half an hour I'm afraid
sailus: <u>mchehab</u>: As there's a common ID space, the type isn't needed anymore to make the graph object ID unique.
mchehab: ok. can we continue this discussion after pinchartl returns?
sailus: I'd use a separate field for the type.
<br> I think I'll need to leave in around half an hour I'm afraid. :-P
<br> How about tomorrow?
mchehab: ok, let's finish it tomorrow then
sailus: What time?
mchehab: I usually start work 6AM GMT-3
<br> anytime after that is OK for me
sailus: That's noon Finnish time.
<br> That'd be fine.
mchehab: <u>sailus</u>: please base your patches on my experimental tree: ssh://linuxtv.org/git/mchehab/experimental.git mc_next_gen.v8.3+fixes+javier
<br> the branch "mc_next_gen.v8.3+fixes+javier" is where I'm consolidating all patches
<br> while I don't get all the acks to put at the media-controller topic branch
***: headless_ changes his alias to headless
jmleo_: Suppose I have two modules, each one is a video device, and I want one video device to open and use the other one from kernel space and not user space. How can I do that ? I would answer v4l2_subdev_call() calls but maybe is there a better way ?
pinchartl: video device as in video node ?
<br> struct video_device ?
jmleo_: <u>pinchartl</u>: yes
hverkuil: <u>mchehab</u>: FYI: the irc logger seems to have died a few days ago.
jmleo_: I probably won't need to do such a thing, but was wondering if there is a way
***: jmleo_ has quit IRC (Quit: jmleo_)
<br> Muzer_ changes his alias to Muzer