[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <643a7811-60ff-6ac9-be3d-edcd9a0b95cc@collabora.com>
Date: Wed, 30 Aug 2023 14:51:30 +0200
From: Benjamin Gaignard <benjamin.gaignard@...labora.com>
To: Hans Verkuil <hverkuil-cisco@...all.nl>, mchehab@...nel.org,
tfiga@...omium.org, m.szyprowski@...sung.com, ming.qian@....com,
ezequiel@...guardiasur.com.ar, p.zabel@...gutronix.de,
gregkh@...uxfoundation.org, nicolas.dufresne@...labora.com
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
linux-rockchip@...ts.infradead.org, linux-staging@...ts.linux.dev,
kernel@...labora.com
Subject: Re: [PATCH v5 01/10] media: videobuf2: Rework offset 'cookie'
encoding pattern
Le 30/08/2023 à 14:25, Hans Verkuil a écrit :
> 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@...labora.com>
>> ---
>> 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.
I will follow your advice and fix that in next version.
I will until you have finish your review in this series before send it.
Regards,
Benjamin
>
>> + 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
>
Powered by blists - more mailing lists