| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| *** | kbingham[m] has quit IRC (Ping timeout: 255 seconds)
ageis has quit IRC (Ping timeout: 260 seconds) | [05:34] |
| ................................................. (idle for 4h4mn) | ||
| hverkuil | sailus: I have time for your request api question today, just ping me when you are ready. | [09:40] |
| sailus | hverkuil: How about in about two hours, 14:00 Norwegian time? | [09:54] |
| hverkuil | sailus: sounds good | [10:04] |
| ....................... (idle for 1h53mn) | ||
| sailus | Ouch. I just realised I have a meeting now...
hverkuil: Would an hour later work for you? | [11:57] |
| hverkuil | sure | [11:58] |
| ............. (idle for 1h3mn) | ||
| sailus | hverkuil: Hello! | [13:01] |
| hverkuil | sailus: hi! I need to finish something, but just go ahead, I should be done in 1-2 minutes.
done | [13:05] |
| sailus | Good.
Finally. ;-) The first thing is about return codes. EACCESS means that something cannot be done; it's not really about lacking permissions --- which is what EPERM is for. Both seem to be used in the current patchset where EACCES would seem to be the right choice. It'd seem to be the right thing to just replace EPERM with EACCES where EPERM is added, and I don't think there are exceptions. What do you think? | [13:07] |
| hverkuil | let me check where EPERM is still used.
EPERM is only used if the driver doesn't support requests and a request_fd is passed to QBUF/EXT_CTRLS ioctls. I have no problem replacing this with EACCES. | [13:09] |
| sailus | Ack.
It may actually be that some of the EPERMs I've seen have since been replaced by other error codes in later patches. | [13:12] |
| hverkuil | EPERM suggests that by changing some file/device permissions it will work, but that's not the case here. | [13:13] |
| sailus | Indeed. | [13:13] |
| hverkuil | yes, most have been replaced by EACCES. | [13:13] |
| sailus | Good.
The second thing is behaviour of control, or non-volatile controls in particular. | [13:13] |
| hverkuil | Please put this in a reply on the mailinglist, otherwise I'll forget this. | [13:14] |
| sailus | I will.
In general, the approach in the request API has been that the device state is made a part of the request, for the relevant part of the device, whatever that precisely matters in detail level may be still somewhat unclear. Right now for codecs that's still likely not a major concern. What's the behaviour currently for non-volatile controls? As the controls are not copied to the request, I presume that they should not be queriable after completing the request either. As far as the user did not put them to the request --- I mean a case where the user just puts a buffer to a request, and then tries to get a control from a request once the request completes. I don't think there's a real use case for this at the moment, but returning an error here would allow addressing this later on without changing the functionality. | [13:14] |
| hverkuil | When a request is completed the values of *all* controls at that time are copied to the request, i.e. the request contains a snapshot of all control values. I make no difference between volatile or non-volatile controls at the moment.
Note that this works fine for stateless codecs, but more complex drivers will no doubt need to have more control over this. Also, if there are controls with a big payload, then this is inefficient. It can be done smarter (avoid copying large payloads unless they actually changed), but I left that for the future. | [13:18] |
| sailus | Do you see a need to do this for non-volatile controls?
If their values are copied, then those values should rather reflect the latest queued request, not the value at the time of request completion. | [13:20] |
| hverkuil | Why not? Besides, why would controls that return the state be volatile? It is more likely that they will be read-only. They are not really volatile. | [13:22] |
| sailus | Read-only might be a better condition indeed.
The behaviour will be different whether or not a control was associated with a request. It should be the same. In other words, I see this is a bug. s/is/as/ | [13:22] |
| hverkuil | For volatile controls the intention is that the value used at the time that the buffer was captured is copied to the request. So you know what e.g. gain value the hardware used when capturing the frame.
"The behaviour will be different whether or not a control was associated with a request. It should be the same" ???? | [13:24] |
| sailus | Yes.
I perfectly understand the purpose of being able to get back the values of non-volatile controls at the time of request completion. | [13:25] |
| hverkuil | If you query controls from a completed request, then you want the values used when capturing the frame.
Not the current values. | [13:25] |
| sailus | But the non-volatile controls in general should have their values fixed at the time the request is queued, not later.
Especially if setting controls is allowed while there are request queued, the checking of which is rather non-trivial in the general case. | [13:25] |
| hverkuil | Not for read-only values (obviously), but I don't think we can guarantee that the value will never change. Hardware constraints may require changes.
I don't know enough whether or not that can ever happen. | [13:26] |
| sailus | I would say that the software would in general have information to determine this much earlier. | [13:27] |
| hverkuil | 'would in general': exactly :-) Are we 100% certain about this? | [13:28] |
| sailus | For now I'd suggest not copying the values of non-volatile controls to the request at all, unless they were a part of the request.
The API will be more or less broken unless this is fixed. Or the behaviour is changed later on, neither of which I think is a good thing. So if there's no need for this at the moment, I'd just not add the non-volatile controls to the request. | [13:28] |
| hverkuil | Why is it broken? I don't see the problem at all. | [13:29] |
| sailus | The value of the control may be changed between the time of queueing the request and completing it.
That is the problem. It's not supposed to happen. | [13:29] |
| hverkuil | Says who? I don't think we can guarantee that at all. It's a best effort. | [13:31] |
| sailus | Best effort request API?
...what? I think we rather need more than that. | [13:31] |
| hverkuil | If you set control X to value V, then we will do our best to apply that control to the corresponding frame. But we cannot guarantee it. | [13:32] |
| sailus | The API cannot guarantee it perhaps, as it's dependent on the implementation in the driver. | [13:32] |
| hverkuil | and on hardware limitations.
You may not be able to set e.g. a gain value for the specified frame, it might slip to a frame later. It all depends on what the hardware can and cannot do. | [13:33] |
| sailus | But guaranteed implementation in driver must lead to deterministic API behaviour.
At the moment this is not the case. | [13:33] |
| hverkuil | There is no guaranteed implementation. There is a best-effort implementation.
Just as with VIDIOC_S_EXT_CTRLS when setting multiple controls in one go: we try to do this atomically, but it's best effort as well. There is no way you can do it 100% atomically when you have to address multiple i2c devices. | [13:34] |
| sailus | That does not make implementing Android Camera API possible, which has been one of the main drivers for the Request API.
As far as the implementation uses controls, that is. I'm not talking about I²C devices, I'm talking about the API implementation in the control framework. | [13:35] |
| hverkuil | All I am doing is make a snapshot of all controls upon request completion. If you trust the driver/hardware implementation, then you only need to read back the controls with the new status.
I don't see how this can break anything. | [13:38] |
| sailus | This value could have been set directly from the user space and not actually even implemented in the request.
Also it may not have been available at the time the request is validated. | [13:47] |
| hverkuil | So? If you mix setting controls directly and via requests, then you were warned that that is a bad idea unless you know what you are doing. | [13:50] |
| sailus | Or are you saying such controls would need to be explicitly grabbed in order to prevent changing them?
The applications working on a device could be independent and not designed to work together. | [13:50] |
| hverkuil | If you care enough that you want to completely prevent other from modifying controls directly, then use S_PRIORITY. | [13:51] |
| sailus | In most cases they wouldn't need to, though.
The controls are supposed to come with the request, as the rest of the configuration. | [13:51] |
| hverkuil | I think you are looking for problems were there aren't any.
s/were/where/ | [13:51] |
| sailus | Perhaps the applications for codec drivers wouldn't be affected even if the behaviour was changed. | [13:55] |
| hverkuil | let's try again, because I might still not understand what you want. | [13:56] |
| sailus | But it'd mean that there would be subtly different API semantics between drivers, and possibly some amount of driver code due to this. | [13:56] |
| hverkuil | I have a request with controls A, B and C. I queue it, and once it is completed I can read back the contents of the request. In the current implementation all controls (except write-only controls of course) can be read back with the value used when the frame was captured. | [13:58] |
| sailus | What happens if an application sets a control at any random time between the execution of these three requests --- that may well have been queued by a different application? | [13:59] |
| hverkuil | You want that only controls containing the new (?) state can be read back? So the values of volatile controls and of read-only controls set by the driver? | [13:59] |
| sailus | Just non-volatile controls in this case. | [14:00] |
| hverkuil | only one application can queue requests. | [14:00] |
| sailus | Uh, why?
I think that'd be new... V4L2 may have limitations but I don't think the request API should. | [14:00] |
| hverkuil | phone | [14:01] |
| sailus | That's still a different matter. | [14:03] |
| hverkuil | back | [14:06] |
| sailus | If an application queues requests with controls, another application can poke the same controls and the application gets the "poked" values back when the request completes. | [14:07] |
| hverkuil | It's vb2 that sets the owner of a queue based on the first application that requests buffers. You don't want different applications queuing buffers, that's a very bad idea. So this will also force that only a single application can queue requests.
It's not a media limitation as such. | [14:09] |
| sailus | Good. | [14:09] |
| hverkuil | but since all requests also contain at least one buffer, it is an automatic consequence of the v4l2 limitation. | [14:10] |
| sailus | It might be good to change that one day. I presume it's not trivial though. | [14:11] |
| hverkuil | It's asking for problems for no good reason. | [14:12] |
| sailus | There are drivers that have file handle specific buffer queues.
The reasons why that was done was a bit different though; there was no CREATE_BUFS at the time. | [14:12] |
| hverkuil | nothing to do with CREATE_BUFS. it's used by v4l2-mem2mem so multiple applications can access a codec, but each fh has it's own set of controls and is independent (at least from the PoV of userspace) from others. | [14:14] |
| sailus | Yes, it does.
The omap3isp at least did use to have this as well. The reason was that you couldn't allocate buffers of different sizes for different purposes. Now you can. So I'm thinking about a use case where multiple applications could work on the same media device, albeit still *possibly* on distinct pipelines. Everything should work through requests; allowing setting controls or anything else behind requests could only cause trouble. Setting controls without going through requests would be disallowed, at least as long as there would be requests queued. I remember the preference was to allow setting controls without using requests even if requests _are_ queued. | [14:14] |
| hverkuil | Yes, I agree. If you set controls directly you are on your own. But that's clearly stated in the spec, and it is useful for debugging and some corner cases, and the general consensus by mchehab, tfiga and me is to allow this. | [14:19] |
| sailus | This way it would be the driver implementation that determines what kind of use cases are supported, wouldn't it? | [14:19] |
| hverkuil | A driver can block the directly setting of controls if it wants to, yes.
It's probably possible to add a flag or something in the control framework to let that handle it. The idea is that EBUSY is returned in that case, which is perfectly legal and does not require any changes in the spec. | [14:20] |
| sailus | The control framework could likely only support on/off, i.e. either require control setting access through requests independently of the streaming state.
I'm fine with that approach. I wonder if someone would still complain it'll break the API... | [14:22] |
| hverkuil | It can keep track of whether controls are set via a request or not, so I can do something along the lines of what I did for vb2. But it's something that I only do when we need it. | [14:24] |
| sailus | Right, makes sense. | [14:24] |
| hverkuil | drivers can always return EBUSY if a control is for some reason not settable. | [14:24] |
| sailus | Right. The control framework likely needs some changes to support this in either case, but that's fine to do later. | [14:26] |
| hverkuil | userspace should always check error codes. Especially uvc devices often fail with EIO when setting controls :-( | [14:26] |
| sailus | There will be other work to be done there, too. | [14:26] |
| hverkuil | So the only thing you want me to do is to change EPERM to EACCES? | [14:26] |
| sailus | Not quite.
Then about getting control values from a completed request. The request state is checked, and completed and idle seem to be accepted. | [14:26] |
| hverkuil | No: I made a follow-up patch so you can only get controls when completed.
getting controls when idle is now blocked. | [14:27] |
| sailus | Ack, good.
Still, nothing prevents changing the request state between obtaining the reference to the request and obtaining the control value. | [14:27] |
| hverkuil | This is something we might relax in the future, but the general consensus was to block this for now. | [14:28] |
| sailus | I wonder if something bad can or cannot happen as a result.
It might be the user space's own problem if it's getting a control value and re-initialising the request at the same ime. | [14:28] |
| hverkuil | When a request is marked completed it cannot be reinited/freed until the last reference it gone. So there is no race condition. | [14:29] |
| sailus | What exactly prevents this?
AFAIK there's only a check for IDLE or COMPLETED state. | [14:30] |
| hverkuil | Only for COMPLETED. Please check my "[PATCH 0/5] Post-v18: Request API updates" series. | [14:31] |
| sailus | I think we're talking about a different check.
I meant the check in media_request_ioctl_reinit. The request objects still remain intact whenever there are references though, but the request itself will be cleaned of its objects. Unless something else that I missed got changed, that is. | [14:32] |
| hverkuil | Ah. S_EXT_CTRLS uses media_request_lock_for_update() for this, but g_ext_ctrls does not have an equivalent.
Hmm. I need to think about this. | [14:36] |
| sailus | I wonder if we need another locking primitive for this going forward. | [14:38] |
| hverkuil | I may have to do the same thing for g_ext_ctrls, although UPDATING should perhaps be renamed to ACCESSING or something like that.
That would make sense. Good point, I hadn't seen that corner case. | [14:39] |
| sailus | Yes, a new state is needed.
I can write a patch for this, too, if you like, but I wouldn't be able to test it. | [14:45] |
| hverkuil | I'm already working on this. | [14:47] |
| sailus | Ok. :-) | [14:47] |
| hverkuil | Just rename STATE_UPDATING to STATE_ACCESSING, and it all makes sense.
I can then use it in g_ext_ctrls as well. | [14:47] |
| sailus | Not quite.
Otherwise it would allow, unintentionally, attaching new objects to the completed request. It'll need to be a new state. For you'll also need to return it to completed state afterwards, not idle. The same updating_count field could be used but it should be renamed. | [14:48] |
| hverkuil | darn, you're right. I need to do this in the morning, when my brain is working on all cylinders :-) | [14:50] |
| sailus | Ack.
Those were my concerns. I'll check the patches once you send them. I'd like to ack the rest at once when I've seen them all. | [14:52] |
| ........... (idle for 50mn) | ||
| wzyy22222 | ff | [15:43] |
| ....... (idle for 31mn) | ||
| mjourdan | hverkuil: Hi! I fixed all the v4l2-compliance issues in my driver, except one:
info: test buftype Video Capture Multiplanar fail: v4l2-test-buffers.cpp(497): q.create_bufs(node, 1) test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL I use the v4l2_m2m_ioctl_* helpers for all 3 functions. I thought it was because I only allow one fd to call queue_setup (other fds will get -EBUSY), but removing the lock still fails the test. Any ideas ? Ah, nevermind, running with -T shows "VIDIOC_CREATE_BUFS returned -1 (Cannot allocate memory)" | [16:14] |
| ..................... (idle for 1h40mn) | ||
| angelo_ts | hi all, brief question
after a "v4l2-ctl -d /dev/video4 --set-fmt-video=..." some hw registers are set in my csi driver then i capture with "v4l2-ctl -d /dev/video4 --stream-mmap=3 ..." and i see those registers set by "v4l2-ctl -d /dev/video4 --set-fmt-video" have lost their previous value, as resetted is this a wrong behavior from the v4l2 point of virew ? i was expecting the values to be maintained | [17:55] |
| hverkuil | angelo_ts: they should be maintained, yes.
Unless it is a memory-to-memory device (codec). but this is a regular capture device, so it should keep the state. | [18:05] |
| ............................... (idle for 2h30mn) | ||
| angelo_ts | hverkuil, thanks
i tested now exec 3</dev/video4 before applying those commands, same result so it should be some error in the imx drivers, that are out-fo-tree still | [20:37] |
| ................ (idle for 1h15mn) | ||
| ok the issue is that, even if i open with exec 3</dev/video4, at each next v4l2-ctl invocation open and release file ops are fully executed
i changed them using v4l2_fh_is_singular_file | [21:53] | |
| .................. (idle for 1h29mn) | ||
| *** | r0kc4t has quit IRC (Ping timeout: 268 seconds) | [23:23] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |