| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | ChanServ sets mode: +v mchehab | [02:25] |
| ChanServ sets mode: +v mchehab` | [02:39] | |
| ................................................................................................ (idle for 7h58mn) | ||
| ChanServ sets mode: +v mchehab | [10:37] | |
| ................. (idle for 1h22mn) | ||
| mchehab | hi all | [11:59] |
| sailus | EHLO | [12:00] |
| hverkuil | hi!
let me get a coffee, and then I'm ready. | [12:03] |
| mchehab | Our pull request was already merged:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=71f3a82fab1b631ae9cb1feb677f498d4ca5007d | [12:06] |
| hverkuil | I noticed.
Regarding the request API: I propose that v18 is used for a topic branch, and I'll make patches on top of that for doc/code fixes. | [12:08] |
| mchehab | works for me | [12:10] |
| hverkuil | I'm getting really tired of having to deal with this large patch series.
I'll have at least one patch fixing/clarifying a number of doc issues Laurent found. | [12:10] |
| sailus | hverkuil: I'll reply to the RFC tomorrow. | [12:11] |
| hverkuil | sailus: OK.
sailus: didn't you want to discuss something about control handling and request API with me? | [12:11] |
| sailus | Yes, it was about controls.
It seem to be discussed in your RFC as well. My concern is basically that while this might work for some devices, it's not something applications in general can rely on. It is rather timing-sensitive as well, and inherently risky. | [12:16] |
| hverkuil | using G_EXT_CTRLS with a request_fd? | [12:17] |
| sailus | S_EXT_CTRLS
I mean S_EXT_CTRLS without a request fd combined with requests queued at the same time. | [12:17] |
| hverkuil | You mean setting controls via a request and direct? | [12:17] |
| sailus | It's good that the spec now says that it may lead to undefined behaviour. | [12:18] |
| hverkuil | Ah. Yes, it is timing sensitive, but it is also something you only do when debugging or experimenting, and I want to keep the functionality. | [12:18] |
| sailus | It might be good to add that not all devices could support this even if they do support controls as such.
hverkuil: Why not to put the control in the request in that case? | [12:19] |
| hverkuil | I experimented trying to detect when it is used in a request and block the direct path in that case but it is 1) not all that easy to do, 2) hard to explain in the documentation, and 3) I think the caution should be sufficient. | [12:20] |
| sailus | Let's suppose you have a rotation control, for example.
Create a few requests, then add buffers to those requests and queue the requests. And then change the rotation control. And finally start streaming. What will happen? | [12:22] |
| hverkuil | Bad example: you typically can't set the rotation control directly while streaming. | [12:23] |
| sailus | But streaming isn't started while the rotation control is changed.
So changing that control is still entirely possible. | [12:24] |
| hverkuil | Sorry, while the vb2 queue is busy. Which it is when you queue the request.
Besides, the rotation control is really poorly defined in any case :-( | [12:24] |
| sailus | Ok, if drivers check that, then it should be fine.
I thought they checked whether the device was streaming. | [12:25] |
| pinchartl | hverkuil: I'm fine with adding patches on top to ease review. we can also do a final rebase to squash fixes where applicable before merging upstream to keep the history clean | [12:26] |
| hverkuil | Basically, my position is that mixing setting controls directly and via a request should just be possible, unless the driver decides not to for some good reason. It can always return EBUSY in that case. | [12:26] |
| sailus | There are still caveats: the driver that controls the DMA (and the video buffer queue) may well be different from the one that implements the rotation control.
So it wouldn't know there are buffers queued. | [12:26] |
| hverkuil | Actually, it does know this. It is certainly not possible to mix queueing buffers directly and via a request, and this is stored in vb2_queue. | [12:27] |
| mchehab | the point is: there is a .. caution warning that people shouldn't mix
there's nothing that prevent people of doing stupid things... like not listening to a caution warning | [12:27] |
| sailus | hverkuil: How does the driver that implements the rotation control know about the state of the video buffer queue, if it has no clue about the queue? | [12:28] |
| mchehab | if people don't listen and implement apps violating the API, bad things will happen
and people shouldn't complain later, because it was *their decision* to do it wrong | [12:29] |
| hverkuil | sailus: you probably need to store this information in the media_device as well. We have all the information, and we can expose this to drivers.
Note: the request API as it is now is probably 80% or so of what you need to complex camera drivers. It does need more work to fully support those drivers as well. | [12:30] |
| sailus | hverkuil: How would it find the video buffer queue (or the video node)? The pipeline isn't started at that point yet, and it may well be that it is not even configured yet.
The current APIs make this possible still. Perhaps this case is a bit far-fetched, but I think it demonstrates the problems of what kind of trouble you get into if you allow mixing different ways of doing things (stream-centric vs. request-centric). | [12:32] |
| mchehab | sailus: sorry, but I think either you or me are missing the poind
s/poind/point/ mchehab needs coffee | [12:35] |
| hverkuil | The moment you allocate a request you know this in the media device. Since all drivers that are part of the topology have access to the media_device they can just ask if requests are in use. And if so, disallow direct setting of selected controls. | [12:36] |
| pinchartl | sailus: I agree with you that allowing mixing request/non-request operations is very tricky and leads to lots of corner cases
one way to solve the problem would be to create a request internally to implement the non-request API so that drivers would only need to deal with requests, and the behaviour would be better defined | [12:37] |
| sailus | pinchartl: Not everything will be able to make use of requests in practice, such as sensors over the I²C bus. Or quite a bit of new stuff in the kernel is needed, and the feasibility of the approach is a big question mark to me at least. | [12:39] |
| mchehab | there are controls with, IMHO, it doesn't require using a request... you're talking about cameras... so, controls like bright, contrast, etc could easily keep setting those things outside a request | [12:39] |
| sailus | One way to proceed would be to keep this in mind, and disable controls that could create truble with requests, whenever there are requests queued. | [12:39] |
| hverkuil | Sorry, I think you are making it way too difficult. I think it is very rare that you run into problems doing this. And in that case you can protect against it in the driver. Let's keep this in the driver, not in the framework. | [12:39] |
| sailus | I mean, disallow using them without requests.
hverkuil: What do you think of that? | [12:40] |
| pinchartl | hverkuil: I believe problems will be more common than you think. we'll need to experiment, and going through staging for a few releases is certainly a good thing for that reason
but it also means that we need a real userspace before we destage the API | [12:40] |
| hverkuil | I wouldn't disable it, I would prefer to return EBUSY in that case. We might need a new flag. | [12:41] |
| pinchartl | by real userspace, I mean real codec support, and real camera support
not test applications | [12:41] |
| mchehab | pinchartl: I agree | [12:41] |
| hverkuil | Note that all this is something we can deal with when we start to use this for complex camera drivers. None of this is relevant to codecs.
pinchartl: the cedrus driver will be merged together with this API. It will initially be in staging, in case we missed something. | [12:41] |
| mchehab | keeping drivers that use request on staging for a couple of kernel versions could be a good strategy | [12:42] |
| hverkuil | mchehab: that was my plan. I would prefer to keep it in staging for 2-3 cycles and I would also like to have at least one encoder supported. | [12:43] |
| sailus | hverkuil: How about labelling the API experimental? | [12:43] |
| hverkuil | (cedrus is decoding only at the moment, although they are looking already into the encoder) | [12:43] |
| mchehab | in special, I don't like the idea of this causing even more fragmentation with that notion of mandatory request controls | [12:43] |
| hverkuil | sailus: the only driver using it are virtual drivers (vim2m, vivid) and a staging driver. I don't think we need to mark it experimental. | [12:44] |
| sailus | Ack. | [12:44] |
| mchehab | I'd like to see patches for the new camera library that would support mandatory requests control merged before moving it out of staging | [12:44] |
| hverkuil | I don't think you'll have mandatory controls for cameras. It's very specific to codecs.
stateless codecs, that is. It's really the whole point of this exercise. Queuing a buffer without a state is meaningless for stateless codecs. | [12:45] |
| mchehab | even so, such library (we're calling "camera" - IMO a bad name) should be able to keep V4L2 apps running no matter what mess we're doing internally at the Kernel level to break userspace from work
forcing userspace to always use request is an API breakage | [12:46] |
| hverkuil | But stateless codecs can *only* work with the request API. | [12:47] |
| mchehab | so, the library should be able to handle it | [12:47] |
| pinchartl | mchehab: not for new drivers. a new driver can force usage of requests, it breaks no API | [12:47] |
| mchehab | in a transparent way
pinchartl: that was the same argument that was used when MC got merged still giving us headaches | [12:47] |
| pinchartl | and the point is still valid | [12:48] |
| hverkuil | There are two difference use-cases: stateless codecs, which require the Request API, and others which do not (although in that case the functionality will likey be limited) | [12:48] |
| pinchartl | libcamera will provide backward-compatibility with plain V4L2
requests won't be visible to V4L2 applications, even if they're mandatory at the driver level | [12:48] |
| mchehab | the library should handle requests internally
if needed | [12:49] |
| pinchartl | yes, that's what it will do
so it's fine if ISP drivers make requests mandatory | [12:49] |
| mchehab | yes - *after* we have such code merged | [12:50] |
| pinchartl | yes, for new ISP drivers | [12:50] |
| mchehab | while not, we should keep everything requiring mandatory requests in staging | [12:50] |
| sailus | mchehab: Can you use the ISP drivers from a regular V4L2 application now without requests? | [12:50] |
| pinchartl | I'm ok with that | [12:51] |
| mchehab | sailus: at the time MC got merged, we didn't have staging | [12:51] |
| sailus | I don't think there's any real difference there: regular applications do not control the ISPs today. | [12:51] |
| mchehab | if we had, I would merge it there | [12:51] |
| sailus | mchehab: Drivers using MC or MC itself? | [12:52] |
| mchehab | MC was not ready to be merged... the only reason I took was to avoid losing all the work that was done when Nokia started firing Linux devs | [12:52] |
| sailus | I thought it was ready to be merged. :-) | [12:53] |
| mchehab | if you remember, one of the things I required on that time was the libv4l support to transparently handle cameras | [12:53] |
| sailus | That was intended, but there are several reasons why that did not materialise in the end.
Both technical and non-technical. But none I see were related to MC. | [12:53] |
| mchehab | If it wasn't the Nokia decision, MC would be waiting until such library would materialize | [12:54] |
| sailus | Anyway, that's history now. | [12:54] |
| mchehab | I won't repeat the same mistake again | [12:54] |
| pinchartl | mchehab: staging appeared in v2.6.28, MC in v2.6.39 | [12:55] |
| mchehab | crap, I should have merged it into staging then
staging were too new on that time | [12:55] |
| sailus | X-) | [12:56] |
| mchehab | the point is: mandatory requests is something that breaks again userspace, as no existing userspace works with requests
(either generic or not) | [12:57] |
| sailus | mchehab: Would you consider that for stateless codec drivers as well? | [12:58] |
| hverkuil | So, to conclude: this will remain in staging until: 1) at least 2-3 kernel cycles have passed, 2) there is a stateless encoder driver as well, and 3) there is proper userspace support for this that can deal with stateless codecs. | [12:58] |
| mchehab | (1) and (3) seems enough for me | [12:58] |
| hverkuil | I want (2) as well. It is unlikely to be a problem, but I prefer to be certain. | [12:59] |
| mchehab | OK | [12:59] |
| sailus | Fair enough. | [12:59] |
| pinchartl | mchehab: I fundamentally disagree, mandatory requests don't break userspace for new drivers as there is nothing to break there. it may raise the bar to making supporting userspace a reality, but it's *not* a breakage
there is no regression | [12:59] |
| hverkuil | I agree with pinchartl, I don't think this is a regression. Applications will look at the pixelformat list, and skip those pixelformats they do not support. | [13:00] |
| sailus | Wouldn't that be a case for any new API, or any new functionality in existing API? | [13:01] |
| hverkuil | Since stateless codecs will use a new pixelformat, applications will just say that they can't support that codec. | [13:01] |
| sailus | We essentially have the latter here. | [13:01] |
| pinchartl | I'm certainly fine keeping drivers in staging until we have proper userspace support (as I think that's key to testing the API), but let's not incorrectly call this a regression or a breakage | [13:01] |
| hverkuil | Just like any other new pixelformat, it isn't supported unless applications implement it. | [13:02] |
| mchehab | hverkuil: when we added libv4l, one of the mandatory requirements for merging a driver (non-MC ones) were that, if the driver has some weird pixelformat, libv4l should be patched to convert, in order to keep all userspace apps working, no matter what camera the user has | [13:04] |
| hverkuil | So I agree with pinchartl that this is not breakage or a regression, but also that we want a proper library that can handle these formats before moving it out of staging.
mchehab: sorry, that's never been a requirement. | [13:04] |
| mchehab | the same should apply here: once we have the camera library, new drivers with only weird pixelformats should require the userspace counterpart to decode it
hverkuil: it is. I enforced it when new gspca sub-drivers got added - and so hansdegoede the same rule also applied to a few non-gspca camera drivers | [13:05] |
| hverkuil | There are a lot of new pixelformats added since then that are not supported by libv4l. | [13:06] |
| mchehab | yes, but almost all went via a mc-centric driver | [13:07] |
| hverkuil | BTW, I am fairly certain that codecs won't be part of libcamera, it will likely be a separate library. Codecs are quite different in how they behave. | [13:07] |
| mchehab | the point is: a driver should have at least *one* format that would be supported by all apps | [13:07] |
| pinchartl | hverkuil: agreed, codecs won't be handled by libcamera
mchehab: I don't agree. an H.264 encoder driver will only expose H.264 as a capture format, which won't be supported by camera-oriented applications for instance we have different classes of devices, we can't enforce that they all work with all applications | [13:08] |
| mchehab | we can't keep adding devices that will only work with vendors-only apps | [13:11] |
| hverkuil | Anyway, we all agree that it stays in staging untl there is some userspace library that can handle stateless codecs. It's needed regardless since you need to parse the bitstream and that should be maintained in a central place. | [13:11] |
| pinchartl | hverkuil: is anyone planning to work on such a library ? | [13:11] |
| hverkuil | I know Paul/Maxime and possibly Nicolas have been looking at that. | [13:12] |
| pinchartl | ok | [13:13] |
| hverkuil | One thing at a time :-) | [13:13] |
| pinchartl | sure, but as we're making it a requirement, it's important to plan it :-) | [13:16] |
| mchehab | anything else for today's meeting? | [13:17] |
| hverkuil | Not from me. | [13:18] |
| sailus | mchehab: Anything new on the media summit plans? | [13:19] |
| mchehab | it was confirmed to happen at ELC-EU | [13:20] |
| sailus | How about the dates? | [13:21] |
| hverkuil | mchehab: good point, have to sent out an announcement yet? Probably needs to go to media, dri-devel and alsa mailinglists. | [13:21] |
| mchehab | > > Thursday, October 25
I'll send anyway, it is now at ELC-E register site, since Mon | [13:21] |
| sailus | Excellent!
Nothing planned for Friday? | [13:21] |
| mchehab | room can have up to 65 people, on round tables | [13:22] |
| sailus | As I'll need to leave on Friday already. | [13:22] |
| mchehab | Cisco will sponsor coffee breaks up to 35 people
so, I asked LF to warn me if more than 35 people would register for it no plans for Friday | [13:22] |
| sailus | Ack.
Obrigado! | [13:23] |
| .... (idle for 16mn) | ||
| mchehab | announce for the summit sent
feel free to forward to other ML if you think it would be worth | [13:39] |
| hverkuil | Thank you! | [13:40] |
| mchehab | ah, and don't forget to submit your proposals for themes to be discussed
from my side, I think it would be worth to have a discussion about complex cameras - assuming that some progress would happen with that matter | [13:40] |
| hverkuil | I agree, if nothing else we can discuss what is missing in the Request API for such devices and what needs to be done to add support for that. | [13:45] |
| ................... (idle for 1h31mn) | ||
| *** | hverkuil_ has left | [15:16] |
| ChanServ sets mode: +v hverkuil | [15:22] | |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |