* [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state
@ 2019-01-23 8:30 Hans Verkuil
2019-01-23 9:48 ` Alexandre Courbot
2019-01-24 5:49 ` Tomasz Figa
0 siblings, 2 replies; 5+ messages in thread
From: Hans Verkuil @ 2019-01-23 8:30 UTC (permalink / raw)
To: Linux Media Mailing List, Paul Kocialkowski, Alexandre Courbot,
Tomasz Figa
There really is no reason why vb2_find_timestamp can't just find
buffers in any state. Drop that part of the test.
This also means that vb->timestamp should only be set to 0 when a
capture buffer is queued AND when the driver doesn't copy timestamps.
This change allows for more efficient pipelining (i.e. you can use
a buffer for a reference frame even when it is queued).
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 75ea90e795d8..2a093bff0bf5 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
unsigned int plane;
- if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
+ if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
vb->timestamp = 0;
for (plane = 0; plane < vb->num_planes; ++plane) {
@@ -594,14 +594,9 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
{
unsigned int i;
- for (i = start_idx; i < q->num_buffers; i++) {
- struct vb2_buffer *vb = q->bufs[i];
-
- if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
- vb->state == VB2_BUF_STATE_DONE) &&
- vb->timestamp == timestamp)
+ for (i = start_idx; i < q->num_buffers; i++)
+ if (q->bufs[i]->timestamp == timestamp)
return i;
- }
return -1;
}
EXPORT_SYMBOL_GPL(vb2_find_timestamp);
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index a9961bc776dc..8a10889dc2fd 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -59,8 +59,7 @@ struct vb2_v4l2_buffer {
* vb2_find_timestamp() - Find buffer with given timestamp in the queue
*
* @q: pointer to &struct vb2_queue with videobuf2 queue.
- * @timestamp: the timestamp to find. Only buffers in state DEQUEUED or DONE
- * are considered.
+ * @timestamp: the timestamp to find.
* @start_idx: the start index (usually 0) in the buffer array to start
* searching from. Note that there may be multiple buffers
* with the same timestamp value, so you can restart the search
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state
2019-01-23 8:30 [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state Hans Verkuil
@ 2019-01-23 9:48 ` Alexandre Courbot
2019-01-23 9:57 ` Hans Verkuil
2019-01-24 5:49 ` Tomasz Figa
1 sibling, 1 reply; 5+ messages in thread
From: Alexandre Courbot @ 2019-01-23 9:48 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa
On Wed, Jan 23, 2019 at 5:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> There really is no reason why vb2_find_timestamp can't just find
> buffers in any state. Drop that part of the test.
>
> This also means that vb->timestamp should only be set to 0 when a
> capture buffer is queued AND when the driver doesn't copy timestamps.
>
> This change allows for more efficient pipelining (i.e. you can use
> a buffer for a reference frame even when it is queued).
So I suppose the means the stateless codec API needs to be updated to
reflect this? I cannot find any case where that would be a problem,
but just out of curiosity, what triggered this change?
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 75ea90e795d8..2a093bff0bf5 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> unsigned int plane;
>
> - if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
> + if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
> vb->timestamp = 0;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> @@ -594,14 +594,9 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> {
> unsigned int i;
>
> - for (i = start_idx; i < q->num_buffers; i++) {
> - struct vb2_buffer *vb = q->bufs[i];
> -
> - if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
> - vb->state == VB2_BUF_STATE_DONE) &&
> - vb->timestamp == timestamp)
> + for (i = start_idx; i < q->num_buffers; i++)
> + if (q->bufs[i]->timestamp == timestamp)
> return i;
> - }
> return -1;
> }
> EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index a9961bc776dc..8a10889dc2fd 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -59,8 +59,7 @@ struct vb2_v4l2_buffer {
> * vb2_find_timestamp() - Find buffer with given timestamp in the queue
> *
> * @q: pointer to &struct vb2_queue with videobuf2 queue.
> - * @timestamp: the timestamp to find. Only buffers in state DEQUEUED or DONE
> - * are considered.
> + * @timestamp: the timestamp to find.
> * @start_idx: the start index (usually 0) in the buffer array to start
> * searching from. Note that there may be multiple buffers
> * with the same timestamp value, so you can restart the search
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state
2019-01-23 9:48 ` Alexandre Courbot
@ 2019-01-23 9:57 ` Hans Verkuil
0 siblings, 0 replies; 5+ messages in thread
From: Hans Verkuil @ 2019-01-23 9:57 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Linux Media Mailing List, Paul Kocialkowski, Tomasz Figa
On 01/23/19 10:48, Alexandre Courbot wrote:
> On Wed, Jan 23, 2019 at 5:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> There really is no reason why vb2_find_timestamp can't just find
>> buffers in any state. Drop that part of the test.
>>
>> This also means that vb->timestamp should only be set to 0 when a
>> capture buffer is queued AND when the driver doesn't copy timestamps.
>>
>> This change allows for more efficient pipelining (i.e. you can use
>> a buffer for a reference frame even when it is queued).
>
> So I suppose the means the stateless codec API needs to be updated to
> reflect this? I cannot find any case where that would be a problem,
> but just out of curiosity, what triggered this change?
Two reasons, really: one was the discussion about decoding an interlaced
stream where the second field had to refer to the buffer for the first field,
requiring special code in the cedrus driver. With this change you no longer
need to do anything special.
The second was a discussion where it was pointed out that in the current
situation you cannot queue a capture buffer containing a reference frame
that is referred to by a queued, but not yet processed, output buffer.
This means you need to allocate more buffers than is strictly necessary.
The main limitation here was that the timestamp of a capture buffer was
set to 0, so you couldn't find these buffers anymore.
It's easy enough to fix in vb2 and I see no downsides to this change.
Regards,
Hans
>
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 75ea90e795d8..2a093bff0bf5 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> unsigned int plane;
>>
>> - if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
>> + if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
>> vb->timestamp = 0;
>>
>> for (plane = 0; plane < vb->num_planes; ++plane) {
>> @@ -594,14 +594,9 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> {
>> unsigned int i;
>>
>> - for (i = start_idx; i < q->num_buffers; i++) {
>> - struct vb2_buffer *vb = q->bufs[i];
>> -
>> - if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
>> - vb->state == VB2_BUF_STATE_DONE) &&
>> - vb->timestamp == timestamp)
>> + for (i = start_idx; i < q->num_buffers; i++)
>> + if (q->bufs[i]->timestamp == timestamp)
>> return i;
>> - }
>> return -1;
>> }
>> EXPORT_SYMBOL_GPL(vb2_find_timestamp);
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index a9961bc776dc..8a10889dc2fd 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -59,8 +59,7 @@ struct vb2_v4l2_buffer {
>> * vb2_find_timestamp() - Find buffer with given timestamp in the queue
>> *
>> * @q: pointer to &struct vb2_queue with videobuf2 queue.
>> - * @timestamp: the timestamp to find. Only buffers in state DEQUEUED or DONE
>> - * are considered.
>> + * @timestamp: the timestamp to find.
>> * @start_idx: the start index (usually 0) in the buffer array to start
>> * searching from. Note that there may be multiple buffers
>> * with the same timestamp value, so you can restart the search
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state
2019-01-23 8:30 [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state Hans Verkuil
2019-01-23 9:48 ` Alexandre Courbot
@ 2019-01-24 5:49 ` Tomasz Figa
2019-01-24 7:35 ` Hans Verkuil
1 sibling, 1 reply; 5+ messages in thread
From: Tomasz Figa @ 2019-01-24 5:49 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Paul Kocialkowski, Alexandre Courbot
Hi Hans,
On Wed, Jan 23, 2019 at 5:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> There really is no reason why vb2_find_timestamp can't just find
> buffers in any state. Drop that part of the test.
>
> This also means that vb->timestamp should only be set to 0 when a
> capture buffer is queued AND when the driver doesn't copy timestamps.
>
> This change allows for more efficient pipelining (i.e. you can use
> a buffer for a reference frame even when it is queued).
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 75ea90e795d8..2a093bff0bf5 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> unsigned int plane;
>
> - if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
> + if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
Is the change fully as expected?
Current behavior:
COPY !COPY
CAP 0 0
OUT keep 0
New behavior:
COPY !COPY
CAP keep 0
OUT keep keep
Don't we still want to zero OUT if !COPY? I suppose that would make
the condition as simple as if (!vb->vb2_queue->copy_timestamp).
Best regards,
Tomasz
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state
2019-01-24 5:49 ` Tomasz Figa
@ 2019-01-24 7:35 ` Hans Verkuil
0 siblings, 0 replies; 5+ messages in thread
From: Hans Verkuil @ 2019-01-24 7:35 UTC (permalink / raw)
To: Tomasz Figa
Cc: Linux Media Mailing List, Paul Kocialkowski, Alexandre Courbot
On 1/24/19 6:49 AM, Tomasz Figa wrote:
> Hi Hans,
>
> On Wed, Jan 23, 2019 at 5:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> There really is no reason why vb2_find_timestamp can't just find
>> buffers in any state. Drop that part of the test.
>>
>> This also means that vb->timestamp should only be set to 0 when a
>> capture buffer is queued AND when the driver doesn't copy timestamps.
>>
>> This change allows for more efficient pipelining (i.e. you can use
>> a buffer for a reference frame even when it is queued).
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 75ea90e795d8..2a093bff0bf5 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -567,7 +567,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> unsigned int plane;
>>
>> - if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
>> + if (!vb->vb2_queue->is_output && !vb->vb2_queue->copy_timestamp)
>
> Is the change fully as expected?
>
> Current behavior:
>
> COPY !COPY
> CAP 0 0
> OUT keep 0
>
> New behavior:
>
> COPY !COPY
> CAP keep 0
> OUT keep keep
>
> Don't we still want to zero OUT if !COPY? I suppose that would make
> the condition as simple as if (!vb->vb2_queue->copy_timestamp).
Ouch. Yes, you are completely correct. I'll make a new patch for this.
Regards,
Hans
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-24 7:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-23 8:30 [PATCH] vb2: vb2_find_timestamp: drop restriction on buffer state Hans Verkuil
2019-01-23 9:48 ` Alexandre Courbot
2019-01-23 9:57 ` Hans Verkuil
2019-01-24 5:49 ` Tomasz Figa
2019-01-24 7:35 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).