Skip to content

drivers: video: allow allocation with a header preceding the buffer #79168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented Sep 29, 2024

Some protocols expect a header followed the buffer payload. Inserting a header at allocation time permits to have a single buffer including both the header and the payload in a continuous memory region as expected by some drivers, without requiring to memcpy() the header and payload every time.

Before:

vbuf
[                     ]
^.buffer              ^.buffer + .size

After:

vbuf
[          |                     ]
^.header   ^.buffer              ^.buffer + .size

@josuah josuah marked this pull request as draft September 29, 2024 13:22
@josuah josuah added the DNM This PR should not be merged (Do Not Merge) label Sep 29, 2024
@josuah
Copy link
Collaborator Author

josuah commented Sep 29, 2024

Some protocols expect a header followed the buffer payload. Inserting
a header at allocation time permits to have a single buffer including
both the header and the payload in a continuous memory region as
expected by some drivers, without requiring to memcpy the header and
payload every time.

Convert all video buffer allocation functions to use a common internal
function with default parameters provided.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah force-pushed the pr-video-buffer-header branch from d092cdf to b616821 Compare September 29, 2024 15:57
@loicpoulain
Copy link
Collaborator

How are we supposed to know header size when doing allocation? Wouldn't make it sense to allocate the necessary size from the capture driver itself. We could have an internal head pointer and adjust both head and buffer (data/payload) pointer when via some sort of vbuf_reserve function in the driver? (similarly to Linux skb: https://docs.kernel.org/networking/skbuff.html).

@josuah
Copy link
Collaborator Author

josuah commented Nov 3, 2024

Thank you for this feedback!

Looking at how the SKB API is used, the headroom is taken into account during the allocation (RSTN_ALIGN):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/renesas/rtsn.c#n363

		skb = netdev_alloc_skb(ndev, PKT_BUF_SZ + RTSN_ALIGN - 1);
		if (!skb)
			goto error;
		skb_reserve(skb, NET_IP_ALIGN);

If it is a global headroom_reserve API, this wastes a bit of memory in every buffer that did not use any headroom.
If it is a per-device headroom_reserve API, this mean the allocation becomes more complicated as each device must check the headroom required by the sink.

In https://github.com/zephyrproject-rtos/zephyr/pull/76798/files I will memcpy() the first USB packet (64 ~ 512 bytes max per frame) and migrate to a better API once introduced.

@josuah
Copy link
Collaborator Author

josuah commented Nov 29, 2024

The workaround above works: the first packet is memcpy()-ed with the header. It does not look like this memcpy() is impactful, althrough this was not benchmarked. This could be come relevant again if wanting to pass large amount of metadata before a frame. Then the suggestions will be useful!

@josuah josuah closed this Nov 29, 2024
@josuah josuah deleted the pr-video-buffer-header branch November 29, 2024 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Drivers area: Video Video subsystem DNM This PR should not be merged (Do Not Merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants