[07:09] <hverkuil> kbingham: there is no shortcut to reset controls, but it shouldn't be too difficult to add to v4l2-ctl. Patches welcome!
[09:25] <sailus> hverkuil: Huomenta!
[09:27] <kbingham> hverkuil, thanks. I've already got a patch for yavta, will look at a similar option for v4l2-ctl. It was one of those "surely I can't be the first to need this?" moments... But I guess no one does any testing :)
[09:44] <hverkuil> sailus: Good morning!
[09:45] <hverkuil> kbingham: it shouldn't be difficult to add a --reset-ctrls option.
[09:45] <hverkuil> All the information is there already.
[09:47] <kbingham> hverkuil, Yup.
[09:48] <kbingham> hverkuil, Ok - so to save bikeshedding time on a patch, for yavta I had written "--reset-controls" ... would you prefer "--reset-ctrls" ?
[09:48] <hverkuil> yes, to be consistent with the other v4l2-ctl control options which all use 'ctrl'.
[09:49] <kbingham> hverkuil, Ok - I'll use that in yavta as well then. Then they can be consistent. Thanks.
[09:49] <hverkuil> kbingham: it should be consistent with each application.
[09:50] <kbingham> hverkuil, Hrm ... ah yes - of course - and yavta already has "--get-control"
[09:51] <kbingham> So it's "--reset-controls" for yavta ... and "--reset-ctrls" for v4l2-ctl ... shame they're not the same but it's fine :)
[10:01] <sailus> hverkuil: On requests and preparing buffers.
[10:01] <sailus> Does that make sense? :-)
[10:02] <sailus> In other words, is there use for preparing a buffer if it's part of a request?
[10:02] <hverkuil> You'd prepare it before you add it to a request.
[10:03] <hverkuil> If you add a unprepared buffer to a request then it should be 'prepared' either when you add it to the request or when the request is queued (I forgot the sequence, have to look it up).
[10:04] <hverkuil> I think it should be prepared when you add it to a request, but I am not sure if that actually happens.
[10:05] <sailus> When you queue buffers, the device can use them right away --- if there is no request.
[10:06] <sailus> But if there is a request, the buffer is just sitting around idle, until the request is queued.
[10:06] <pinchartl> sailus: I was going to explain the same :-)
[10:06] <pinchartl> I don't see a use case for VIDIOC_PREPARE_BUF with requests
[10:07] <sailus> Supporting preparing buffers with requests complicates already complex error handling, but I don't see a use case for doing that.
[10:07] <hverkuil> All PREPARE_BUF does is lock the buffer into memory. It really is completely independent of requests.
[10:08] <sailus> hverkuil: There's cache flushing and a few other things.
[10:08] <sailus> But what preparing a buffer is without requests, queueing it is effectively the same with requests.
[10:09] <sailus> I need to go but I'll be back later on. An hour and a half maybe.
[10:09] <hverkuil> queuing the buffer to a request, or queuing a request containing a buffer to the driver?
[10:09] <hverkuil> sorry, didn't understand what you meant.
[10:09] <pinchartl> sailus: if it simplifies the implementation, I think we shouldn't support VIDIOC_PREPARE_BUF with requests
[10:10] <hverkuil> It would actually make the implementation harder. Adding exceptions always makes life harder.
[10:12] <pinchartl> hverkuil: I'll judge that based on the code :-)
[10:22] <hverkuil> double-checked the code: you cannot prepare a buffer once it has been queued to a request.
[10:23] <hverkuil> You can queue a prepared buffer to a request. The only difference is that the buffer is already pinned in memory.
[10:25] <hverkuil> If you queue a buffer that is not prepared to a request (i.e. you didn't call PREPARE_BUF first), then the buffer is prepared during request validation.
[10:29] <hverkuil> The purpose of PREPARE_BUF is the same: you can use it to prepare a large memory buffer beforehand, and only queue it when you need it knowing that all the cache management is already done. Whether you are using requests or not does not matter.
[12:11] <sailus> hverkuil: What will happen if a request is queued with a buffer that is prepared but not queued?
[12:12] <sailus> That will need to be handled if we do allow preparing buffers for requests.
[12:12] <hverkuil> it just skips the prepare step since that's already done. Everything else stays the same.
[12:12] <sailus> I think it's an error.
[12:12] <hverkuil> It's identical to the non-request case: if you queue a prepared buffer it just skips the prepare step. Otherwise it prepares it as well.
[12:13] <hverkuil> I don't see the problem.
[12:13] <sailus> That's not the point.
[12:13] <sailus> What happens currently if you queue a request with a buffer that is prepared but not queued?
[12:13] <sailus> Obviously the buffer is not prepared since it is already prepared, but then what?
[12:14] <hverkuil> The non-prepare case is:
[12:14] <hverkuil> QBUF
[12:14] <hverkuil> sorry, let me restart
[12:15] <hverkuil> MEDIA_IOC_REQUEST_ALLOC to get a request
[12:15] <hverkuil> QBUF(request_fd)
[12:15] <hverkuil> MEDIA_REQUEST_IOC_QUEUE to queue this request.
[12:16] <hverkuil> At MEDIA_REQUEST_IOC_QUEUE time the buffer(s) queued to the request will be prepared.
[12:16] <hverkuil> Unless the buffer was already prepared before QBUF(request_fd) was called, in that case that step is skipped.
[12:17] <hverkuil> You can't call PREPARE_BUF for a buffer that is already queued (to the driver or to a request, that doesn't matter).
[12:24] <zid`> Well, I got access to the machine with the dongle again, my code seems to work and grabs a video frame, nice
[12:32] <sailus> hverkuil: What will happen if you replace QBUF above with PREPARE_BUF?
[12:58] <hverkuil> sailus: PREPARE_BUF(request_fd) results in -EINVAL. Not allowed to do that.
[12:59] <hverkuil> It makes no sense.
[13:14] <sailus> How about the patch "[PATCH for v4.20 2/5] vb2: skip request checks for VIDIOC_PREPARE_BUF" then?
[13:17] <hverkuil> vb2_prepare_buf() checks if V4L2_BUF_FLAG_REQUEST_FD was set and returns -EINVAL in that case.
[13:18] <hverkuil> The bug addressed in the patch comes from the check afterwards:
[13:18] <hverkuil> if an earlier QBUF(request_fd) was called, then the queue is marked as 'using requests' and a QBUF that's not for a request should be rejected.
[13:19] <hverkuil> That worked fine.
[13:19] <hverkuil> Unfortunately, PREPARE_BUF without a request was tested the same way, but that's wrong. You can always call PREPARE_BUF() regardless of whether the queue is in 'request' mode or not.
[13:21] <sailus> Well, fair enough.
[13:22] <hverkuil> Basically the 'if (q->uses_requests)' check is wrong for PREPARE_BUF. It's for QBUF only. Just a logic error that v4l2-compliance found.
[13:22] <sailus> I don't see need for the PREPARE_BUF but if there's no way to associate the buffer with a request then I guess there's no harm either.
[13:24] <hverkuil> Why wouldn't prepare_buf be useful? It was created to be able to prepare large buffers before they were needed (for example for the buffer needed to take a snapshot with a camera). This to avoid having to spend time invalidating the cache.
[13:24] <hverkuil> Isn't the same use-case just as valid for requests?
[13:25] <sailus> You could put the buffer into a request as well in that case.
[13:26] <hverkuil> No, the buffer isn't prepared until the request is queued to the driver.
[13:26] <hverkuil> Just queuing the buffer to a request doesn't prepare it.
[13:26] <sailus> Ok.
[13:26] <sailus> Which tree the patches are intended for?
[13:27] <hverkuil> 4.20, but I could live with 4.21 given that the request API and the cedrus driver are still in staging.
[13:27] <sailus> Ah, yes.
[13:28] <hverkuil> Although for proper vivid behavior I would prefer 4.20.
[13:29] <sailus> The media request pointer is also stored in the media request objects.
[13:29] <sailus> Instead of adding a field, could you use that instead in the 3rd patch?
[13:29] <hverkuil> sailus: no, it is cleared when the object is unbound. Which is correct since the object is removed from the request at that time.
[13:30] <hverkuil> (I thought I could do the same thing originally)
[13:32] <hverkuil> In fact, when in dqbuf the buffer object has already disappeared.
[13:36] <hverkuil> Related note: I'm working on improving the compliance tests for this even further, and creating scripts to run the tests for all virtual drivers in various combinations. Once that's working correctly I would like to do some refactoring for vb2 since it's getting hard to keep track of everything.
[13:36] <hverkuil> There is definitely room for improvement.
[13:37] <sailus> What kind of problems does calling kfree() in an interrupt cause?
[13:38] <sailus> I don't think it's a great idea to try freeing memory in an interrupt though.
[13:40] <hverkuil> It's not the kfree() that's the problem, it's a mutex_lock when a spinlock is held. The mutex comes from v4l2_ctrl_handler_free() which is called because the whole request is freed.
[13:41] <sailus> Uh, right.
[13:41] <hverkuil> (I think it's fine to free memory in atomic context, not 100% certain though)
[13:43] <sailus> AFAIU yes, but it adds to the latency.
[13:43] <sailus> It's complicated.
[13:44] <hverkuil> kfree is definitely not something that should happen in vb2_buffer_done.
[13:44] <sailus> I wonder if there could be other places where a request might be unbound, put and subsequently released in an interrupt context.
[13:45] <hverkuil> Not in anything we have now, but it is something to remember for the future.
[13:47] <sailus> An alternative could be to add a work queue to release the requests.
[13:47] <sailus> It wouldn't complicate the request handling further, but it'd be a bit heavy-weight solution IMO.
[13:48] <hverkuil> I think that would be a viable plan B :-)
[13:49] <hverkuil> I considered a workqueue as well, but I think the current solution is a bit cleaner. Certainly less of an impact if this is to be merged for 4.20.
[13:49] <hverkuil> But if we run into this again in the future, then a workqueue might be a better option.
[13:52] <hverkuil> BTW, I also need to take another look at https://patchwork.linuxtv.org/patch/51391/ This shows in-use requests and request-objects, which is really something you want v4l2-compliance to be able to read out to check for memory leaks.
[13:53] <sailus> On the 4th patch --- I thought returning the buffer back to the user was intentional.
[13:54] <hverkuil> No, the buffer is not returned to the user. It's returned to vb2.
[13:54] <sailus> Yes, that's what obviously happens with the 4th patch. :-)
[13:54] <hverkuil> Basically the state after a failed STREAMON should be the same after as before.
[13:55] <sailus> Streamon is troublesome with requests.
[13:55] <hverkuil> It's rather cool that v4l2-compliance can now use vivid's error injection mechanism to test these corner cases.
[13:56] <sailus> This might be something we'll need to revisit.
[13:56] <hverkuil> troublesome for complex camera pipelines?
[13:56] <sailus> In general.
[13:57] <hverkuil> why?
[13:57] <sailus> Error handling in general is pretty complex, as well as pipeline management.
[13:57] <sailus> If there are requests queued with buffers to different video nodes, is there another way to reclaim the buffers than to start closing file handles?
[13:58] <hverkuil> REQBUFS(0) will always reclaim buffers.
[13:59] <hverkuil> STREAMOFF will also complete any pending requests.
[13:59] <hverkuil> Note: how this will interact with subdevs is a whole different matter.
[13:59] <sailus> I wonder how does that work now if there are multiple buffer queues involved.
[13:59] <sailus> I'm not saying it's broken though.
[14:00] <hverkuil> The request *should* complete when all devices are either REQBUFS(0), close()d. or STREAMOFF-ed.
[14:00] <sailus> Yes, indeed.
[14:01] <sailus> Btw. on locking --- why does the v4l2_ctrl_handler_free() acquire the mutex?
[14:01] <sailus> At that point, the handler may no longer be accessible anyway.
[14:05] <hverkuil> The lock might not be strictly needed there, but I would have to do a full analysis of it.
[14:07] <hverkuil> It was designed so you could actually at runtime free the handler and add new controls to it (it is actually a bit of a misnomer, it frees the controls, not the handler itself.)
[14:35] <sailus> hverkuil: Thanks for working on the request API!
[14:36] <hverkuil> np
[14:36] <sailus> It's still apparently in need of some extra care.
[14:36] <sailus> Leaving now, back later today, or tomorrow...
[14:36] <sailus> hverkuil: Have a nice evening!
[14:36] <hverkuil> you too
[15:58] <ezequielg> hverkuil: can you comment a bit more about timecode and V4L2_BUF_FLAG_TIMECODE for memory-to-memory devices?
[15:59] <ezequielg> v4l2-compliance apparently checks the timecode is copied from output to capture queues
[16:21] <hverkuil> ezequielg: m2m devices should just copy timecode data, just like timestamp data. I.e., just preserve what userspace gives you.
[16:22] <hverkuil> it's a no-brainer once this patch is merged: https://patchwork.linuxtv.org/patch/52961/
[16:23] <hverkuil> mchehab: FYI: I plan to post a pull request with several fixes for 4.20 tomorrow.
[16:43] <pinchartl> mchehab: same
[16:43] <pinchartl> or rather a single fix
[16:43] <pinchartl> or rather today :-)
[16:55] <ezequielg> hverkuil: oh, nice helper!
[19:02] <mchehab> hverkuil, pinchartl: ok, I'll try to handle them tomorrow
[21:13] *** jernej_ has left 
[21:16] *** jernej_ has left