diff mbox series

[v5,01/10] media: videobuf2: Rework offset 'cookie' encoding pattern

Message ID 20230824092133.39510-2-benjamin.gaignard@collabora.com (mailing list archive)
State Superseded
Headers show
Series Add DELETE_BUF ioctl | expand

Commit Message

Benjamin Gaignard Aug. 24, 2023, 9:21 a.m. UTC
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@collabora.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(-)

Comments

Hans Verkuil Aug. 30, 2023, 12:25 p.m. UTC | #1
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@collabora.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.

> +	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
Benjamin Gaignard Aug. 30, 2023, 12:51 p.m. UTC | #2
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@collabora.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
>
diff mbox series

Patch

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
+
 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.
+	 */
+	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;
+	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;