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