On 24/08/2023 11:21, Benjamin Gaignard wrote:
> Change how offset 'cookie' field value is computed to make possible
> to use more buffers (up to 0xffff).
> With this encoding pattern we know the maximum number that a queue
> could store so we can check ing at queue init time.
> It also make easier and faster to find buffer and plane from using
> the offset field.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxxxxx>
> ---
> v5:
> - I haven't change DST_QUEUE_OFF_BASE definition because it used in
> v4l2-mem2mem and s5p_mfc driver with a shift.
>
> .../media/common/videobuf2/videobuf2-core.c | 48 +++++++++----------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cf6727d9c81f..e06905533ef4 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -31,6 +31,10 @@
>
> #include <trace/events/vb2.h>
>
> +#define PLANE_INDEX_SHIFT 3
> +#define PLANE_INDEX_MASK 0x7
> +#define MAX_BUFFERS 0xffff
Very poor name, see below.
> +
> static int debug;
> module_param(debug, int, 0644);
>
> @@ -358,21 +362,23 @@ static void __setup_offsets(struct vb2_buffer *vb)
> unsigned int plane;
> unsigned long off = 0;
>
> - if (vb->index) {
> - struct vb2_buffer *prev = q->bufs[vb->index - 1];
> - struct vb2_plane *p = &prev->planes[prev->num_planes - 1];
> -
> - off = PAGE_ALIGN(p->m.offset + p->length);
> - }
> + /*
> + * Offsets cookies value have the following constraints:
> + * - a buffer could have up to 8 planes.
> + * - v4l2 mem2mem use bit 30 to distinguish between source and destination buffers.
> + * - must be page aligned
> + * That led to this bit mapping:
> + * |30 |29 15|14 12|11 0|
> + * |DST_QUEUE_OFF_BASE|buffer index|plane index| 0 |
> + * where there is 16 bits to store buffer index.
16 -> 15: there are 15 (not 16!) bits available for buffer indices. So the maximum
number of buffers is 32768, given that the indices start at 0.
> + */
> + off = vb->index << (PLANE_INDEX_SHIFT + PAGE_SHIFT);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - vb->planes[plane].m.offset = off;
> + vb->planes[plane].m.offset = off + (plane << PAGE_SHIFT);
>
> dprintk(q, 3, "buffer %d, plane %d offset 0x%08lx\n",
> vb->index, plane, off);
> -
> - off += vb->planes[plane].length;
> - off = PAGE_ALIGN(off);
> }
> }
>
> @@ -2209,21 +2215,15 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off,
> return -EBUSY;
> }
>
> - /*
> - * Go over all buffers and their planes, comparing the given offset
> - * with an offset assigned to each plane. If a match is found,
> - * return its buffer and plane numbers.
> - */
> - for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - vb = q->bufs[buffer];
> + /* Get buffer and plane from the offset */
> + buffer = (off >> (PLANE_INDEX_SHIFT + PAGE_SHIFT)) & MAX_BUFFERS;
Hmm, you use it as a mask. The name MAX_BUFFERS is really confusing.
How about BUFFER_INDEX_MASK? That is consistent with PLANE_INDEX_MASK.
> + plane = (off >> PAGE_SHIFT) & PLANE_INDEX_MASK;
>
> - for (plane = 0; plane < vb->num_planes; ++plane) {
> - if (vb->planes[plane].m.offset == off) {
> - *_buffer = buffer;
> - *_plane = plane;
> - return 0;
> - }
> - }
> + vb = q->bufs[buffer];
> + if (vb->planes[plane].m.offset == off) {
> + *_buffer = buffer;
> + *_plane = plane;
> + return 0;
> }
>
> return -EINVAL;
Regards,
Hans
