<!-- Some styling for better description lists --><style type='text/css'>dt { font-weight: bold;float: left;display:inline;margin-right: 1em} dd { display:block; margin-left: 2em}</style>
pinchartl: <u>hverkuil</u>: ping
hverkuil: <u>pinchartl</u>: pong
pinchartl: <u>hverkuil</u>: epaul1 had a few questions regarding the V4L2 API
<br> and v4l2-compliance
<br> where the two didn't seem to agree
<br> I've been wondering why we allow VIDIOC_S_FMT from multiple fds
<br> instead of considering VIDIOC_S_FMT to "lock" the device, the same way VIDIOC_REQBUFS does
<br> (that's what uvcvideo implements btw)
<br> <u>epaul1</u>: ^^
epaul1: hi
<br> my question was about VIDIOC_G/S_PRIORITY, but yeah i was wondering that about VIDIOC_S_FMT as well
hverkuil: <u>pinchartl</u>: that's just how v4l2 was designed. As long as no buffers are allocated, anyone can change the format (unless the priority ioctls are used). A format is no different from e.g. a control. Only when you allocate buffers do you fix the format and only the fd that allocated the buffer can change it (if allowed by the driver).
pinchartl: <u>hverkuil</u>: so an application would need to call VIDIOC_G_FMT after VIDIOC_REQBUFS to ensure the format hasn't been changed ?
hverkuil: Without using the prio system, yes.
pinchartl: not very nice :-(
<br> <u>epaul1</u>: could you describe how you think the spec and v4l2-compliance disagree regarding VIDIOC_G/S_PRIORITY ?
hverkuil: <u>epaul1</u>: I'm looking into the prio code, there is a mismatch with the docs and I probably have to go back to really old kernels to compare what happened there. v4l2-compliance is in line with what the kernel does, so at this moment it is the documentation for G_PRIO that seems to be wrong.
epaul1: <u>hverkuil</u>: ah, okay
hverkuil: G_PRIO returns the highest prio for any open fd of the device, and not the prio of the current fd.
epaul1: oh :o
<br> i didn't know it was the higest of any fd
hverkuil: I'm fairly certain it's always been like that, and so the doc has always been wrong, but I'll double check.
pinchartl: <u>hverkuil</u>: thanks for checking
hverkuil: <u>epaul1</u>: yes, it's always been like this. And the documentation needs to be updated. So G_PRIO returns the priority of the video device, and not that of the fd. I guess the idea is that an application can call G_PRIO, sees that it is RECORD and just returns with an error telling the user that the device is in use.
epaul1: i see, thank you
pinchartl: <u>epaul1</u>: do you have all the information you need, will it be possible to implement that semantics ?
epaul1: i implemented those semantics already while waiting :)
pinchartl: great :-)
hverkuil: Unfortunately, the prio system was never implemented for v4l-subdevX devices, nor for media devices. And it is painful to use when there are many devices open by an application.
<br> Patches are welcome, I suspect that this is an area where libcamera would need some better support.
***: unidan has quit IRC (Quit: WeeChat 2.4)
epaul1: <u>hverkuil</u>: v4l2-compliance requires that a child process can do VIDIOC_STREAMOFF
<br> without blocking
<br> libcamera doesn't support multiple processes accessing a camera simultaneously
<br> i also don't really think it's a good idea that a different process can do streamoff
<br> can we just say that it's not supported?
pinchartl: calling VIDIOC_STREAMOFF from a different thread is a useful use case, but from a different process, I think it's quite a stretch
epaul1: also since the child process that blocks on streamoff isn't killed after v4l2-compliance exits, there's a process that holds on to the v4l2 node indefinitely
<br> i'll send a patch for that
pinchartl: btw, should we set CLOEXEC on the eventfd ?
<br> and maybe interfect fork to clean up the state in the child ?
epaul1: oh in case someone forks anyway
<br> tbh i don't think it's a good idea, since CLOEXEC is a flag that the application would feed to open, so it'll be confused if it gets CLOEXECed when it didn't set it
pinchartl: oh and it's not about fork
<br> it's about exec anyway
epaul1: intersept fork, maybe; just shut down libcamera in the child?
<br> oo
pinchartl: we have to make sure that the child will not crash if it tries to access the camera
<br> it will not work (everything should return an error)
epaul1: yes
pinchartl: but it should not crash, especially on close()
epaul1: yeah
pinchartl: <u>hverkuil</u>: why does v4l2-compliance try to streamoff from a child process ?
hverkuil: <u>epaul1</u>: can you point to the specific test for that in the v4l2-compliance source code?
<br> it doesn't ring a bell for me.
<br> testBlockingDQBuf()?
epaul1: <u>hverkuil</u>: yes
<br> fail_on_test(pid != pid_streamoff);
<br> line 2285
<br> in the block before that it forks and the child tries to streamoff
<br> and expects to succeed without blocking
hverkuil: When you fork() the child process inherits all fds, i.e. they point to the exact same struct file as those of the parent. So from the PoV of V4L2 this is OK. That said, that's not what this test is about, it is about testing if a blocking wait doesn't block other ioctls, and that can also be tested with threads instead of a fork(). Can libcamera handle multiple threads?
epaul1: i see
<br> yes, it works with multiple threads
hverkuil: If you convert that code to use pthread, then I'm happy to merge that. I just never realised when I used fork() that that it would cause problems with libcamera.
pinchartl: <u>hverkuil</u>: thanks for the feedback
epaul1: i see, alright
<br> thank you
hverkuil: No problem, it's a good point. Using threads in that test is a more realistic scenario anyway.
ezequielg: <u>hverkuil</u>: hey there -- I'm seeing this new controls being proposed, but I was wondering, shouldn't we require drivers for them?
<br> I wonder if we are not bloating the controls.
<br> <u>svarbanov</u>: wrt reqbuf=0, maybe we need to start exploring the RELEASE_BUF op (to balance the CREATE_BUF one) ?
svarbanov: ezequielg, yep we can start, but reqbuf(count=0) is on top for me:)
<br> ezequielg, do we have an rfc for release_buf
ezequielg: i am unware of what issues you are dealing wrt reqbuf(0)
<br> can you enlighten me?
svarbanov: ezequielg, I have some driver internal resources (or reinit list_head) which I have to release on reqbuf(0)
<br> ezequielg, also I release dpb buffers for example
ezequielg: so?
svarbanov: I need to know when that reqbuf(0) is issued from client
ezequielg: you can currently know that, afaik.
<br> ah ah. no.
<br> <u>svarbanov</u>: you cannot do that, i think.
<br> you should not handle resources in queue_setup.
<br> is there any reason why you can't just use start and stop streaming?
<br> i am yet skeptic that you need any core changes to do what you describe.
<br> <u>svarbanov</u>: are you talking about the vdec_queue_setup? i.e. the call to vdec_session_init ?
hverkuil: Re RELEASE_BUF: see this old RFC from me, for whatever it is worth: stable@vger.kernel.org
<br> Oops, I meant: https://patchwork.linuxtv.org/patch/60210/
<br> <u>svarbanov</u>: this old patch might also be of interest for you: https://patchwork.linuxtv.org/patch/44424/
<br> <u>ezequielg</u>: yes, new controls require a driver as well. You're referring to the "Add LTR controls" patch, I guess? But it's OK to discuss it in RFC patches first.
ezequielg: <u>hverkuil</u>: +1 i will reply to the patches then.
svarbanov: ezequielg, see decoder state diagram and what is the purpose of reqbuf(0)
<br> https://linuxtv.org/downloads/v4l-dvb-apis-new/userspace-api/v4l/dev-decoder.html?highlight=stateful%20decoder
<br> basically streamoff / streamon sequence is used for example for seek, I don't want to release session to the firmware in that case, hence I did not release some driver internal resources
ezequielg: ok, let me think... as you probably know, i have more experience with stateless than with stateful.
<br> would the following work? 1) user issues reqbuf(0) 2) driver caches this internally, in queue_setup, but doesn't actually release anything. 3) on stop_streaming, if reqbuf(0) was called, the resources are released.
***: benjiG has left
svarbanov: ezequielg, that is the point, the drivers don't know that reqbuf(0) is called from the client. Only v4l2 core knows that
ezequielg: ok, thinking some more.
<br> how about testing num_buffers > 0 ? vb2_is_busy checks that.
<br> so it would be, if stop_streaming is called and vb2_is_busy is false, you know you can release your resources.
svarbanov: ezequielg, no, I this will not fly, __vb2_queue_cancel() is called before __vb2_queue_free() where the q->num_buffers will eventually (eventually because we have CREATE_BUF) become zero
<br> hverkuil, do you remember the problem with decoders and reqbuf(0)?
<br> ezequielg, but again, we have two streamoff, I have to release on the last. This way is again implicit (I already have workaround in the the driver)
ezequielg: ok, then seems you have a case.
<br> you would have to be careful, because AFAIK, you are not supposed to allocate in queue_setup. there is no API guarantee that queue_setup won't be called multiple times.
<br> I would defer allocation as much as possible, but I see your point with regard to seeking.
svarbanov: ezequielg, I'm allocating in streamon
ezequielg: right, so you'd be allocating on streamon, but not releasing on streamoff right?
svarbanov: and I don't want to release in streamoff but on reqbuf(0)
<br> because streamoff has other meanings in codec spec
ezequielg: perhaps a non-invasive RFC, proposing a solution based on queue_setup could do.
<br> and tbh, a new queue_release hook might be a good idea as well. as often happens, it might be more clear which one is best only after seeing them in action.