Video: Rework buffer management

[ngphibang]

This PR reworks the buffer management in the video subsystem step by step to have a common buffer management framework.

It fixes the current issues of video buffer management and may help to facilitate #89858.

Still a work-in-progress. I will try to describe the change in more details and complete it gradually.

TODO:

• Support user-provided buffer (static buffer, specific memory type etc.). Discussion: With this, we can move share_multiheap to this type (?) so that each time a specific type of memory is required, no need to modify the core framework => not a good idea, still keep SMH in the framework.
• Discussion : Support driver's memory or not ? => no usecase envisioned for now
• Support m2m devices
• Decouple CONFIG_VIDEO_BUFFER_POOL_NUM_MAX (max buffers for the whole system including all drivers + user-provided buffers) and driver's min_buffer requirement (e.g. CSI needs at least 2 buffers to work while DCMI, smartdma needs only 1 buffer) and user's desire.

[josuah]

Interesting! Instead of having to carry pointers around, one only says "I need 10 buffers", then to enqueue them, "dev0 uses buffers 0, 1, 2; dev1 uses number 3, 4, 5, 6...". Keeping track of which buffer is used is just an integer uint8_t used_buffer_num; and bootstrapping buffer allocation is a lot easier.

[EDIT: answering my own questions after discussing on #92366]

---

Q1: Is it important that the application knows exactly which buffer index was given to a device? If not, then enqueue could be video_buf_grant(dev, num_of_buffers); and let dev do the allocation as needed.

A1: Not all buffers will have the same size, so cannot all be mixed between devices.

---

Q2: It seems it is not possible anymore to manually craft a video buffer. For instance, if video data arrives from the network and then goes through PXP to adapt it to a display, how to submit the buffer coming directly from the network into a struct video_buffer?

A2: It is possible to add a video_request_buffer_with_data(uint8_t *data, size_t size, enum video_buf_type type) to turn an user-provided pointer into a buffer designated by an index.

---

Q3: Would it make sense to start making use of video_buf_release(vbuf) to automate the recycling of buffers? This can simplify the application that does not have to manually keep track of where to queue a buffer back.
-> video_enqueue() is only to push data to an m2m device or a video sink (the driver relesese the buffer),
-> video_dequeue() is only to acquire new data from an m2m device or video source (the application releases the buffer).

A3: This does not seem related to what this PR does: buffer management and accounting vs queueing them to the devices. Possibly solved by RTIO_SQE_MULTISHOT from #89858

---

Q4: In my impression, an application developer does not want to know how many bytes are allocated to video in total, but instead decide on a resolution and let all allocation be automated. The current approach is to open prj.conf and a calculator. Should we try to address this now?

A4: This can come as an overlay on top of this eventually, these functions allow this to be implemented on top. A different topic to solve in parallel.

[josuah]

There seems to be several topics involved:

T1: Memory management: how much in total, where from, which back-end for it
T2: Buffer management: moving the buffers across the devices, build streams out of buffers by queueing/dequeueing
T3: Queue management: the mechanism used to imlement pipes between the application and the drivers

It seems like this current PR aims to address something living between T1 and T2.

[ngphibang]

@josuah Hope you don't mind. I implemented the needed stuffs to generalize your RTIO driver demo and integtated it in this buffer management rework. Effectively, RTIO serves as the backend that replaces the FIFO queues (and some more) but it does not overlap or conflict with some changes in this PR which mainly reworks the buffer allocation and interfacing with application.

The main idea in the RTIO generalization is
- Application side nearly unchanged, still call enqueue / dequeue() except a call to video_release_buf() after finishing with the full buffer to queue it back to sqe. RTIO is opaque to application.
- Driver just needs to hold an io_q and deal a bit with the buffer from sqe in isr. No need to implement rtio_iodev_api's submit()
- The video subsystem does everything needed for buffer management.
- We need the video_interface struct in #88182

It doesn't work yet :). I have error at stream_start(). I will try to debug later.

[josuah]

Thank you for picking it up so early!
I will try with an i.MXRT1160 I have around.

Essential to get it working is probably just to call __frame_done_cb() whenever a frame is submitted on io_q(), which might require api->iodev_submit...

[josuah]

dev and timeout are unused so can go away.

rtio_cqe_consume_block() acts as a combined k_poll() and video_dequeue().
Maybe we need a pointer to struct video_interface in struct video_buffer to keep track of things.

[ngphibang]

Buffers are kept track of by index and type ?

[josuah]

Right! ->index type is fully user-defined, which allows user to organize a reference table in the application to store extra metadata about buffers.

I think this solves this point, feel free to fold this.

[josuah]

This fragment was there for the driver to validate that the operation requested is supported.

It is the equivalent to if (vbuf->type != VIDEO_BUF_TYPE_INPUT) {.
Should this go into video_mcux_csi.c?

[ngphibang]

My understand is this is to get the buffer that is filled from the sqe, to do some check (buf type check or buf_addr check as in csi driver). The code is common for all driver so it could be refactored as a function ?

[josuah]

This makes sense as a helper agreed!

Though this does not allow the caller to report completion/failure, so maybe only the 1st buffer would work.

So maybe something like this?

struct rtio_iodev_sqe *video_pop_io_q(struct mpsc *io_q)
{
	struct mpsc_node *node;
	struct rtio_iodev_sqe *iodev_sqe;
	struct video_buffer *vbuf;

	node = mpsc_pop(io_q);
	if (node == NULL) {
		return NULL;
	}

	iodev_sqe = CONTAINER_OF(node, struct rtio_iodev_sqe, q);
	vbuf = iodev_sqe->sqe.userdata;

	__ASSERT_NO_MSG(vbuf != NULL);

	if ((vbuf->type == VIDEO_BUF_TYPE_OUTPUT && iodev_sqe->sqe.op == RTIO_OP_RX) ||
	    (vbuf->type == VIDEO_BUF_TYPE_INPUT && iodev_sqe->sqe.op == RTIO_OP_TX)) {
		return iodev_sqe;
	} else {
		LOG_ERR("Unsupported RTIO operation (%d) or video buffer type (%d)",
			iodev_sqe->sqe.op, vbuf->type);
		rtio_iodev_sqe_err(iodev_sqe, -EINVAL);
		return NULL;
	}
}

It is only 1 line to get the vbuf out of it:

iodev_sqe = video_pop_io_q(&data->io_q);
if (iodev_sqe == NULL) {
	return;
}

vbuf = iodev_sqe->sqe.userdata;

/* Submit `vbuf` contoent to the hardware */

rtio_iodev_sqe_ok(iodev_sqe, 0);

[josuah]

Maybe there need to be a video_enqueue_read() and video_enqueue_write() or use switch (vbuf->type){}. Currently it is only doing read requests.

RTIO_SQE_MULTISHOT might not be possible with write requests as the user has to specify the buffer to use every time, or the same buffer would be sent in loop.

[ngphibang]

I think I see the problem. Unlike the SW generator, the HW device like CSI needs to enqueue buffer to the HW, I blindly remove the enqueue() API.

[ngphibang]

Just receive the 1st frame corrupted and then stopped.

[josuah]

remove the enqueue() API.

I will follow-up on this conversation: #92370 (comment)

It looks like we are close!

[ngphibang]

Q1: Is it important that the application knows exactly which buffer index was given to a device? If not, then enqueue could be video_buf_grant(dev, num_of_buffers); and let dev do the allocation as needed.

It already know and can track which buffer is enqueued.

Q2: It seems it is not possible anymore to manually craft a video buffer. For instance, if video data arrives from the network and then goes through PXP to adapt it to a display, how to submit the buffer coming directly from the network into a struct video_buffer?

It was in TODO list and done now by commit drivers: video: Add support for user-provided memory

[josuah]

What about VIDEO_MEMORY_POOL to avoid a repetition?

[ngphibang]

POOL is a set of buffers of same size and attribute, seems not describe well the situation. Are VIDEO_MEMORY_INTERNAL and VIDEO_MEMORY_EXTERNAL better ?

[josuah]

There is video_buffer_release() and video_release_buf(). To avoid confusion, how about video_buffer_done() or other name?

[josuah]

Sorry I ran out of time while trying to load anything on my mimxrt1160_evk, maybe a problem with the chip, or more likely between the chair and the keyboard... I cannot test it for now.

I just noticed you pushed an update of the API as you described, I will propagate that to the other drivers.

[ngphibang]

I removed driver's .enqueue() and replaced it by .submit() implementation. With this and the use of video_pop_io_q() in ISR, CSI and sw_generator is now working.

PS: Driver's implementation of .submit() is not mandatory, e.g. sw_generator does not need to implement it as common work is done in the framework.

[josuah]

CSI and sw_generator is now working.

That is great! This is the only one I cannot test easily...
The implementation shrunk a lot!

[ngphibang]

Sorry I ran out of time while trying to load anything on my mimxrt1160_evk

Sorry for not inform you before working on it to avoid wasting your time, but I 've just work on this about 45 minutes ago ...

I will have to put this on hold for about the next 10 days due to some obligation. Thank you for the work !

[josuah]

Sorry for not inform you before working on it to avoid wasting your time, but I 've just work on this about 45 minutes ago ...

No time was lost as I spent it getting familiar with the i.MXRT ecosystem and tooling host-side mostly.

I will have to put this on hold for about the next 10 days due to some obligation. Thank you for the work !

There is time before the end of feature freeze, and plenty of other drivers to convert.

What is left to do AFAIU:

• Decide the best way to handle memory allocation WRT these changes.
• Convert the drivers to the new API.

Thank you for the work as well !

[josuah]

video_stm32_dcmi.c

tinyvision-ai-inc@83f4e9d

# import config from capture_to_lvgl and then:
west build --board  mini_stm32h743 --shield weact_ov2640_cam_module samples/drivers/video/capture

available from this same branch here, which got rebased on top of the latest commits:
https://github.com/tinyvision-ai-inc/zephyr/pull/new/video_rework_buffer_management

Only DCMIPP and we should be done for the hardware drivers (and then, once it is merged, Renesas CEU from #92146)!

Then for the software part, UVC, the video shell, the other samples can be converted, as well as zephyr,video-emul-rx

I will post further updates.

[ngphibang]

Update:

• Allow multiple buffer requests (new request does not invalid and free buffers of old requests)
• index is read-only and set by the framework when buffer allocated

TODO:

• Make appropriate buffer_requests structure to be able to return the the first buffer index and the number of buffers allocated.

[sonarqubecloud]

Quality Gate passed

Issues
36 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[avolmat-st]

Thanks @ngphibang @josuah for all this work.

I haven't yet look at all implementation details but more on the API for the time being.

Looking at the API, and especially the request / queue mechanism I am wondering how use-case of dealing with several kind of buffers (type / size) are handled. I probably missed something, could you clarify this to me ?
If we consider for example a device such as the DCMIPP which has several output (or even 2 video devices such as DCMI and JPEG encoder), there will be several buffers being allocated, of different size (let say, could be bayer size hence big, then small buffer for AI output and another one for the display). The application can request all those buffers via the request API and those would be allocated and accessible via an index. However how would the application know which index correspond to which request / size ? It seems to me that currently request simply says that request/allocation went well but doesn't give the start index of the requested pool of buffers, is that right ?

Another case that I can think of is when 2 IPs are in a sense pipelines, for example DCMI/DCMIPP + JPEG encoder. The output of the DCMI/DCMIPP (aka output buffer) will be also used as input to the JPEG encoder. That same buffer can be used it is up to the application to dequeue from one side and queue it to the other size. Does the current API allow to do that ? Aka, in a sense use a output type buffer (from DCMI/DCMIPP) into the JPEG input ?

[ngphibang]

Hi @avolmat-st ,

It seems to me that currently request simply says that request/allocation went well but doesn't give the start index of the requested pool of buffers, is that right ?

Yes, the request() should return the first index of the batch as in above comment (TODO) and in the commit message. It's not implemented yet in the current code but will be done, sorry I am tight up at this moment.

Does the current API allow to do that ? Aka, in a sense use a output type buffer (from DCMI/DCMIPP) into the JPEG input ?

To enqueue the "empty" buffer (freshly requested / allocated), we have the buffer index so we can enqueue it, video_enqueue() can be rewritten which takes index and type (input / output)

To enqueue a "full" buffer dequeued from previous device to another device, since after dequeue, we have a full handle of the video_buffer struct so it's not a problem, we can call the same above video_enqueue(index, type).

Another way of designing the video_enqueue() API is video_enqueue(struct video_buffer) as before which is simpler to enqueue a freshly dequeued buffer, but to enqueue a newly requested buffer, we need to create a video_buffer struct and set the index, type.

PS: I forgot that to support user provided buffers, we need to give the buffer address to the framework, so the video_enqueue() need to take the following parameters:

• type, memory, index, address, size

So, finally, it seems video_enqueue(struct device, struct video_buffer) should be opted.

[avolmat-st]

Just wondering, it depends on the application logic, but should we consider after allocation that the buffer state is DONE or should it be a state such as INIT for example ?
DONE would mean that it is filled with valid data.
Here, upon allocation, the buffer do not have valid data in it. bytesused is properly set to 0 so there is no chance the application would read bad value from it, so that's the reason why I do not really have strong arguments for a STATE_INIT.

[avolmat-st]

Shouldn't this state setting be done after a successfull enqueue ? Assuming the enqueue would return an error, this would lead to having a buffer marked as QUEUE while it has failed to be enqueued to the driver, hence I'd expect this mean that it is still not owned by the driver. In such case it would still be in the previous state (most probably DONE)

[josuah]

Good point, maybe this needs to be set only if ret = api->enqueue() has ret == 0.

[avolmat-st]

My understanding was that buffer state was partially set by the driver as well. For example the driver, if it knows that the contents of the buffer is bad (ex: failure to DMA data into it or anything ...) would set the buffer state to STATE_ERROR to let the application know that the content of the buffer is bad and it shouldn't be used.
Here, since the framework is setting the buffer state to DONE upon a successfull dequeue, the driver do not have anyway to indicate such thing since the framework will override the state to done.

[josuah]

Here is the new video_dequeue() proposed:
https://github.com/nxp-upstream/zephyr/blob/bfc86f6ecb8f79238086b969abb9bbb35c149de3/drivers/video/video_buffer.c#L172-L175

Here is how it is used for sensors:

			cqe = rtio_cqe_consume_block(&ctx);
			if (cqe->result != 0) {
				printk("I/O failed %d\n", cqe->result);
				return;
			}

So checking cqe->result == 0 would give the result from the driver I/O operation, rather than just the "buffer pop" operation.

[avolmat-st]

state setting to be done after doing the enqueue since enqueue could fail

[josuah]

rtio_sqe_prep_read() allocates a queue buffer.
rtio_submit() load it into the queue.
This effectively load the buffer in the driver's buffer queue. Is that good enough to say that the buffer id enqueued?

There is no more api->enqueue() as the queue data structure is not managed by the driver itself anymore, but by the RTIO functions acting as a "queue middleman".

[avolmat-st]

Maybe this is fixed in further commits but I am wondering if this check is ok when put in a chained devices pipeline. For example, in my current system I have two devices:

1. DCMIPP capture which output data, so I have output type which will get data from the camera pipeline.
2. JPEG Codec which is a M2M and has one input and one output. The M2M input is actually the same buffer as the DCMIPP output. So this unique buffer is sometimes queued / dequeue from the DMCIPP as output and something from the JPEG codec as input.
   If the type is hardcoded in the video_buf table and the check is done when we enqueue, it seems it won't be possible to use a unique buffer between 2 devices. Did I understand it correctly ?

[avolmat-st]

Oh actually seems I already asked this similar question before and @ngphibang replied me. Maybe I just haven't properly understood the code then.

[ngphibang]

It's similar but not the same question :). The other one is the possibility of enqueuing the freshly dequeued buffer to another device (answer = yes). For this one, you are right, type shouldn't be checked here. Even more, type is not needed when requesting buffer. Initially, I did it by having the idea of supporting driver-provided buffer in mind, so each driver will have 1 or 2 buffer queues and the check is needed (buffer dequeued from one device will change its type when being queued into another device). But now, it seems user-provided and framework provided buffers are sufficient.