ndufresne: tfiga: bbrezillon: I think we need a DEL_BUFFER ioctl for stateful encoders as well. When we discussed what to do with a stateful encoder if the capture buffer was too small we decided on a new buffer flag (TOO_SMALL) and: "When userspace sees this it should stop streaming on the capture side, reallocate and requeue capture buffers, and restart streaming."
But the upcoming encoder spec says: "If the client stops the CAPTURE queue during the encode process and then restarts it again, the encoder will begin generating a stream independent from the stream generated before the stop."
That's not what you want. It is much cleaner to allow userspace to create new , larger, buffers and delete the too-small buffers as they are dequeued.
This should also make it possible for encoders to keep the state and resume encoding once the larger buffers are queued.
hverkuil: I propose calling it DESTROY_BUF --- as a complement for CREATE_BUFS.
(Or DESTROY_BUFS.)
fine by me.
DESTROY_BUFS is good. It just needs a start buffer index and a count to destroy buffers idx to idx + count - 1.
wasn't part of the discussion to call it RELEASE_BUF or something to imply the refcounted nature?
It depends on the memory model. I have no strong preference myself. DEL_BUFS, RELEASE_BUFS or DESTROY_BUFS are all fine as far as I am concerned. A slight preference for DESTROY since it is a nice counterpart to CREATE.
hverkuil, I miss that bit of the spec, but implementation so far says that only doing streamoff on both queues will actual reset anything, specially for encoder, saying that capture queue streamoff resets is a bug, since the reference frames are kept in the OUTPUT queue
but I guess there is cases where forcing a dq of the encoder buffers will cause some buffers to be corrupted / incomplete
sailus, to be destroy/delete are both confusing, since all you do if it's a dmabuf is to unref the dmabuf, you don't actually delete/destroy
hverkuil, also, not sure it really depends on the memory model, the orphaning applies to mmap buffers too iirc, or maybe I miss-read that bit ?
I also need to check, but I think it's not sufficient to destroy buf, unless you explicitly says that S_FMT is allowed while streaming
(not sure yet on that last one)
but basically, you said that CREATE_BUFS only cares about the resulting size, you still need to S_FMT before queueing
and to S_FMT, you need to STREAMOFF
maybe that only applies to raw data ?
Re S_FMT I do need to make a modification to one line in the S_FMT description: "When I/O is already in progress or the resource is not available for other reasons drivers return the EBUSY error code."
Should probably become "When I/O is already in progress" should probably be dropped or modified.
Sorry, garbled line.
"When I/O is already in progress" should probably be dropped or modified. (that's what I wanted to say)
hverkuil, right, I was worried there was larger consequences to that, drivers need to properly maintain the up-coming and the current format I guess
hverkuil: i understand your thoughts about create/destroy, but otoh create/release or alloc/release is a very common pattern in the kernel.
iow, it's a well understood allocation model.
ndufresne: The term refers to V4L2 buffer objects, not the DMA bufs..
It may well be an MMAP buffer, or a USERPTR (hopefully not) buffer.
ok them, fair argument
hverkuil: i will post a v12 of the rkisp1 driver, clarifying jacob's authorship.
yet another libv4l2 issue found, if you do TRY_FMT on an emulated format, it will reset the colorspace to 0 (default), which is invalid return value, and will not even try to ask the HW