| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | gnurou has quit IRC (Remote host closed the connection) | [04:00] |
| ........................................................................... (idle for 6h12mn) | ||
| sailus | hverkuil: Heippa! | [10:12] |
| ........ (idle for 38mn) | ||
| hverkuil | sailus: Sorry, missed this. I have a meeting in 10 minutes :-) | [10:50] |
| sailus | Never-ending meetings...
And the next one will be in one hour and 10 minutes? Perhaps we could start at least. On the variable size IOCTL argument structs. What happens if the user space argument struct size is larger than what the kernel supports? That means that the application is at least compiled with a newer set of kernel headers than what the kernel supports. Right now we do return an error back to the user. This may be something that happens rarely, but it's still worth considering. The earlier behaviour --- with reserved fields --- was that we ignored the values in the reserved fields, and perhaps thought it was a badly written application because it didn't zero those fields. What do you think? | [10:51] |
| ........... (idle for 50mn) | ||
| hverkuil | sailus: There are two options: accept the ioctl and ignore (and zero upon return) the extra data, or return -ENOTTY.
I'm leaning towards the latter since that clearly tells userspace that the new functionality that it is trying to use is not available. We would need to have a macro that apps can use to tell whether the new ioctl is present or not based on a version number (as returned by DEVICE_INFO or QUERYCAP). | [11:43] |
| .... (idle for 17mn) | ||
| sailus | hverkuil: How about just using -ENOTTY? Or another error code, such as -E2BIG or -ERANGE?
These are new IOCTLs so we can pick. | [12:02] |
| hverkuil | -ENOTTY should be used anyway if the 'extended' ioctl is not recognized, but it's painful for applications to first try one version, then another, etc. until it finally finds a working ioctl. Easier if it can check up front what is available.
In V4L2 we can get away without doing this since there are so few ioctls that have a newer version (although that number is increasing) | [12:05] |
| .................... (idle for 1h37mn) | ||
| *** | syoung has quit IRC (Quit: leaving) | [13:43] |
| ....... (idle for 33mn) | ||
| sailus | hverkuil: Do you imagine applications actually trying multiple variants, vs. just the one they've been compiled against?
I don't think that's a common use case. | [14:16] |
| hverkuil | Depends on the applications and ioctl. It certainly happens with e.g. VIDIOC_S_CTRL vs VIDIOC_S_EXT_CTRLS or CROP vs SELECTION.
Certainly applications that have to be generic will do this (qv4l2, v4l2-ctl all do) | [14:17] |
| sailus | That's a quite different situation, isn't it? | [14:21] |
| hverkuil | From what? | [14:21] |
| sailus | These are different IOCTLs rather than different versions of the same.
You'd only have a problem if you're running an application compiled against a newer set of headers than the kernel it's running on. The uAPI headers tend to lag a bit behind the kernel and that's been the situation for a long time. The kernel would always support the latest defined IOCTL, after all. For controls vs. extended controls, some drivers didn't support extended controls for a long time. I think that's probably been fixed though. | [14:21] |
| hverkuil | No, there are still drivers like that. Old ones. | [14:24] |
| sailus | We won't have any "older drivers" that would implement an old version of the IOCTL because the implementation is in the MC framework.
Well, if you think we'd need way for the user space to figure out the latest supported IOCTL size, I guess it could be implemented later on if there's a need to. | [14:25] |
| hverkuil | Huh? If you extend the size of the argument of an ioctl, then that IS a new ioctl. You have added some sort of new functionality that it didn't have before. It's the same when you take a reserved field and turn it into something new (e.g. request_fd). | [14:29] |
| sailus | Indeed. | [14:29] |
| hverkuil | For applications it is difficult to detect whether the current kernel supports that new functionality. | [14:29] |
| sailus | I fully agree. :-)
Ah, well, that one I'd like to hear from application developers. :-) Considering that one option is we'd return an error if we don't know about something an application might. | [14:29] |
| hverkuil | I never liked the way it works today in applications, it is sometimes hard to figure it out (I wrote v4l-helpers.h to help with that).
It is easier for applications (I think) if they can use a macro with the version number as argument to see what is supported. No need to 'test call' an ioctl, as is required today. | [14:30] |
| sailus | Where would that macro be defined?
In the header? | [14:34] |
| hverkuil | The alternative (accepting the ioctl and zeroing the additional fields provided by userspace) makes this cumbersome and also requires that if a field is reset to 0 on return then that indicates that the 'feature' is not available.
In the header, I had that in the media.h patches I posted a few weeks back. https://patchwork.linuxtv.org/patch/48715/ | [14:34] |
| sailus | I thought we were discussing the case where an application is compiled against a newer header than the definitiong the kernel had. | [14:35] |
| hverkuil | Yes, we are. | [14:35] |
| sailus | Uh... thanks for reminding me about the set.
What do you benefit from using the definitions on a header if the header doesn't match the kernel? | [14:36] |
| hverkuil | You do this in the application:
ioctl(media_fd, MEDIA_IOC_DEVICE_INFO, &info); if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) { // I can use pad.index! } info.media_version has the version number of the API (actually the current kernel version, except when you use media_build) The macro has the version when this feature was added. | [14:37] |
| sailus | Would it be possible to define the macro in a way that it would not be specific to a particular IOCTL and a particular field?
Otherwise we'll have lots of hand-written macros to write and maintain... I think it should be entirely doable. | [14:41] |
| hverkuil | Suggestions are welcome. I don't off-hand see how that can be done.
If a completely new feature was added, then you can have a single macro for that, of course. | [14:42] |
| mchehab: tfiga: sailus: gnurou: paulk-gagarine: posted v13 of the request API. Docs should be (mostly?) complete, dropped RFC tag. Please read the cover letter for the TODOs before reviewing.
Only two left, the second of those is something that can be done later if I don't get around to it. | [14:55] | |
| mchehab | ok | [14:55] |
| sailus | I've barely reviewed the first couple of patches.
I'd suppose they should be largely fine now though. hverkuil: Thanks for working on this. | [14:56] |
| hverkuil | I wanted to get this out of the door today since I probably won't have much time to work on this next week.
My target is to get this in for 4.19. | [14:57] |
| sailus | That's be cool. | [14:59] |
| hverkuil | paulk-gagarine: how will that fit with the timeline for the cedrus driver? It would be good to get that in at the same time. | [14:59] |
| mripard_ | hverkuil: I guess the cedrus driver (at least for mpeg2) is in pretty good shape now (at least feature-wise), so I'd say the main blocking factor would be the request API | [15:01] |
| hverkuil | OK, that's good. So then there is 'real hardware' as well that uses the Request API, which is something mchehab likes to see :-) | [15:02] |
| paulk-gagarine | hverkuil, yes, looks like that can fit well within our timeline :) | [15:03] |
| mchehab | yep | [15:03] |
| hverkuil | paulk-gagarine: is the v2 cedrus patch series still useful to review? Or are you planning a v3 soon? I don't think I've reviewed it yet. | [15:06] |
| paulk-gagarine | hverkuil, I haven't touched it a lot since, but mripard_ has so he probably knows best :) | [15:07] |
| mripard_ | we can do a v3 based on the v13 tomorrow if you'd prefer
on the v4l side it shouldn't have changed that much, I mostly changed the internals | [15:09] |
| hverkuil | OK, that would be nice. I'll try to review it sometime next week. | [15:11] |
| .... (idle for 15mn) | ||
| sailus | mripard_: Could you address Hans's comments on the CSI-2 RX driver? | [15:26] |
| .................. (idle for 1h25mn) | ||
| *** | pfalleno1 has quit IRC (Excess Flood)
neg has quit IRC (Ping timeout: 260 seconds) | [16:51] |
| ezequielg | question: how do I need to set the type field for a buffer when I call VIDIOC_DQBUF? | [17:06] |
| mripard_ | sailus: yeah, I will do it tomorrow | [17:06] |
| ezequielg | seems just a sanity check. | [17:07] |
| .... (idle for 18mn) | ||
| he, small erratum up there. The question was "WHY do I need to set the 'memory' field for a buffer when I call VIDIOC_DQBUF?" | [17:25] | |
| ...... (idle for 26mn) | ||
| *** | al has quit IRC (Quit: al) | [17:51] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |