Re: [PATCHv17 19/34] vb2: drop VB2_BUF_STATE_PREPARED, use bool prepared/synced instead
Em Tue, 14 Aug 2018 10:58:59 +0200
Hans Verkuil escreveu:
> On 13/08/18 13:30, Mauro Carvalho Chehab wrote:
> > Em Sat, 4 Aug 2018 14:45:11 +0200
> > Hans Verkuil escreveu:
> >
> >> From: Hans Verkuil
> >>
> >> The PREPARED state becomes a problem with the request API: a buffer
> >> could be PREPARED but dequeued, or PREPARED and in state IN_REQUEST.
> >>
> >> PREPARED is really not a state as such, but more a property of the
> >> buffer. So make new 'prepared' and 'synced' bools instead to remember
> >> whether the buffer is prepared and/or synced or not.
> >>
> >> V4L2_BUF_FLAG_PREPARED is only set if the buffer is both synced and
> >> prepared and in the DEQUEUED state.
> >>
> >> Signed-off-by: Hans Verkuil
> >> ---
> >> .../media/common/videobuf2/videobuf2-core.c | 38 +--
> >> .../media/common/videobuf2/videobuf2-v4l2.c | 16 +---
> >> include/media/videobuf2-core.h| 6 ++-
> >> 3 files changed, 40 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> >> b/drivers/media/common/videobuf2/videobuf2-core.c
> >> index 7401a17c80ca..eead693ba619 100644
> >> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> >> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> >> @@ -682,7 +682,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
> >> vb2_memory memory,
> >>}
> >>
> >>/*
> >> - * Call queue_cancel to clean up any buffers in the PREPARED or
> >> + * Call queue_cancel to clean up any buffers in the
> >> * QUEUED state which is possible if buffers were prepared or
> >> * queued without ever calling STREAMON.
> >> */
> >> @@ -921,6 +921,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> >> vb2_buffer_state state)
> >>/* sync buffers */
> >>for (plane = 0; plane < vb->num_planes; ++plane)
> >>call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> >> + vb->synced = false;
> >
> > Shouldn't be prepared cleaned as well on reqbufs?
>
> reqbufs calls queue_cancel which will in turn call the buf_finish op and sets
> prepared to false.
Ok. Not sure if I like to have it implicit somewhere.
>
> >
> >>}
> >>
> >>spin_lock_irqsave(>done_lock, flags);
> >> @@ -1239,6 +1240,7 @@ static void __enqueue_in_driver(struct vb2_buffer
> >> *vb)
> >> static int __buf_prepare(struct vb2_buffer *vb)
> >> {
> >>struct vb2_queue *q = vb->vb2_queue;
> >> + enum vb2_buffer_state orig_state = vb->state;
> >>unsigned int plane;
> >>int ret;
> >>
> >> @@ -1247,6 +1249,10 @@ static int __buf_prepare(struct vb2_buffer *vb)
> >>return -EIO;
> >>}
> >>
> >> + if (vb->prepared)
> >> + return 0;
> >> + WARN_ON(vb->synced);
> >> +
> >>vb->state = VB2_BUF_STATE_PREPARING;
> >>
> >>switch (q->memory) {
> >
> >> @@ -1262,11 +1268,12 @@ static int __buf_prepare(struct vb2_buffer *vb)
> >>default:
> >>WARN(1, "Invalid queue type\n");
> >>ret = -EINVAL;
> >> + break;
> >>}
> >
> > Hmm... is this hunk a bug fix? if so, please split into a separate patch
> > and add a c/c stable.
>
> No, just a clean up. I hate switch cases that do not end with a 'break'.
> I can leave it our if you prefer.
Ah, this is at the default clause. I don't personally add a break
after default: when it is the last clause. No need for that, and
very unlikely that it would ever cause problems.
Also, as now gcc complains about missing breaks, at the very unlikely
case where someone would add something after the default: clause,
gcc would warn.
So, for my taste, this is overkill.
>
> >
> >>
> >>if (ret) {
> >>dprintk(1, "buffer preparation failed: %d\n", ret);
> >> - vb->state = VB2_BUF_STATE_DEQUEUED;
> >> + vb->state = orig_state;
> >>return ret;
> >>}
> >>
> >> @@ -1274,7 +1281,9 @@ static int __buf_prepare(struct vb2_buffer *vb)
> >>for (plane = 0; plane < vb->num_planes; ++plane)
> >>call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
> >>
> >> - vb->state = VB2_BUF_STATE_PREPARED;
> >> + vb->synced = true;
> >> + vb->prepared = true;
> >> + vb->state = orig_state;
> >>
> >>return 0;
> >> }
> >> @@ -1290,6 +1299,10 @@ int vb2_core_prepare_buf(struct vb2_queue *q,
> >> unsigned int index, void *pb)
> >>vb->state);
> >>return -EINVAL;
> >>}
> >> + if (vb->prepared) {
> >> + dprintk(1, "buffer already prepared\n");
> >> + return -EINVAL;
> >> + }
> >>
> >>ret = __buf_prepare(vb);
> >>if (ret)
> >> @@ -1381,11 +1394,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned
> >> int index, void *pb)
> >>
> >>switch (vb->state) {
> >>case VB2_BUF_STATE_DEQUEUED:
> >> - ret = __buf_prepare(vb);
> >> - if (ret)
> >> -
Re: [PATCHv17 19/34] vb2: drop VB2_BUF_STATE_PREPARED, use bool prepared/synced instead
On 13/08/18 13:30, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 14:45:11 +0200
> Hans Verkuil escreveu:
>
>> From: Hans Verkuil
>>
>> The PREPARED state becomes a problem with the request API: a buffer
>> could be PREPARED but dequeued, or PREPARED and in state IN_REQUEST.
>>
>> PREPARED is really not a state as such, but more a property of the
>> buffer. So make new 'prepared' and 'synced' bools instead to remember
>> whether the buffer is prepared and/or synced or not.
>>
>> V4L2_BUF_FLAG_PREPARED is only set if the buffer is both synced and
>> prepared and in the DEQUEUED state.
>>
>> Signed-off-by: Hans Verkuil
>> ---
>> .../media/common/videobuf2/videobuf2-core.c | 38 +--
>> .../media/common/videobuf2/videobuf2-v4l2.c | 16 +---
>> include/media/videobuf2-core.h| 6 ++-
>> 3 files changed, 40 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
>> b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 7401a17c80ca..eead693ba619 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -682,7 +682,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum
>> vb2_memory memory,
>> }
>>
>> /*
>> - * Call queue_cancel to clean up any buffers in the PREPARED or
>> + * Call queue_cancel to clean up any buffers in the
>> * QUEUED state which is possible if buffers were prepared or
>> * queued without ever calling STREAMON.
>> */
>> @@ -921,6 +921,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
>> vb2_buffer_state state)
>> /* sync buffers */
>> for (plane = 0; plane < vb->num_planes; ++plane)
>> call_void_memop(vb, finish, vb->planes[plane].mem_priv);
>> +vb->synced = false;
>
> Shouldn't be prepared cleaned as well on reqbufs?
reqbufs calls queue_cancel which will in turn call the buf_finish op and sets
prepared to false.
>
>> }
>>
>> spin_lock_irqsave(>done_lock, flags);
>> @@ -1239,6 +1240,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>> static int __buf_prepare(struct vb2_buffer *vb)
>> {
>> struct vb2_queue *q = vb->vb2_queue;
>> +enum vb2_buffer_state orig_state = vb->state;
>> unsigned int plane;
>> int ret;
>>
>> @@ -1247,6 +1249,10 @@ static int __buf_prepare(struct vb2_buffer *vb)
>> return -EIO;
>> }
>>
>> +if (vb->prepared)
>> +return 0;
>> +WARN_ON(vb->synced);
>> +
>> vb->state = VB2_BUF_STATE_PREPARING;
>>
>> switch (q->memory) {
>
>> @@ -1262,11 +1268,12 @@ static int __buf_prepare(struct vb2_buffer *vb)
>> default:
>> WARN(1, "Invalid queue type\n");
>> ret = -EINVAL;
>> +break;
>> }
>
> Hmm... is this hunk a bug fix? if so, please split into a separate patch
> and add a c/c stable.
No, just a clean up. I hate switch cases that do not end with a 'break'.
I can leave it our if you prefer.
>
>>
>> if (ret) {
>> dprintk(1, "buffer preparation failed: %d\n", ret);
>> -vb->state = VB2_BUF_STATE_DEQUEUED;
>> +vb->state = orig_state;
>> return ret;
>> }
>>
>> @@ -1274,7 +1281,9 @@ static int __buf_prepare(struct vb2_buffer *vb)
>> for (plane = 0; plane < vb->num_planes; ++plane)
>> call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>>
>> -vb->state = VB2_BUF_STATE_PREPARED;
>> +vb->synced = true;
>> +vb->prepared = true;
>> +vb->state = orig_state;
>>
>> return 0;
>> }
>> @@ -1290,6 +1299,10 @@ int vb2_core_prepare_buf(struct vb2_queue *q,
>> unsigned int index, void *pb)
>> vb->state);
>> return -EINVAL;
>> }
>> +if (vb->prepared) {
>> +dprintk(1, "buffer already prepared\n");
>> +return -EINVAL;
>> +}
>>
>> ret = __buf_prepare(vb);
>> if (ret)
>> @@ -1381,11 +1394,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
>> index, void *pb)
>>
>> switch (vb->state) {
>> case VB2_BUF_STATE_DEQUEUED:
>> -ret = __buf_prepare(vb);
>> -if (ret)
>> -return ret;
>> -break;
>> -case VB2_BUF_STATE_PREPARED:
>> +if (!vb->prepared) {
>> +ret = __buf_prepare(vb);
>> +if (ret)
>> +return ret;
>> +}
>> break;
>> case VB2_BUF_STATE_PREPARING:
>> dprintk(1, "buffer still being prepared\n");
>> @@ -1611,6 +1624,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int
>> *pindex, void *pb,
>> }
>>
>> call_void_vb_qop(vb, buf_finish, vb);
>> +vb->prepared = false;
>>
>> if (pindex)
>> *pindex = vb->index;
>> @@
Re: [PATCHv17 19/34] vb2: drop VB2_BUF_STATE_PREPARED, use bool prepared/synced instead
Em Sat, 4 Aug 2018 14:45:11 +0200
Hans Verkuil escreveu:
> From: Hans Verkuil
>
> The PREPARED state becomes a problem with the request API: a buffer
> could be PREPARED but dequeued, or PREPARED and in state IN_REQUEST.
>
> PREPARED is really not a state as such, but more a property of the
> buffer. So make new 'prepared' and 'synced' bools instead to remember
> whether the buffer is prepared and/or synced or not.
>
> V4L2_BUF_FLAG_PREPARED is only set if the buffer is both synced and
> prepared and in the DEQUEUED state.
>
> Signed-off-by: Hans Verkuil
> ---
> .../media/common/videobuf2/videobuf2-core.c | 38 +--
> .../media/common/videobuf2/videobuf2-v4l2.c | 16 +---
> include/media/videobuf2-core.h| 6 ++-
> 3 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
> b/drivers/media/common/videobuf2/videobuf2-core.c
> index 7401a17c80ca..eead693ba619 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -682,7 +682,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
> memory,
> }
>
> /*
> - * Call queue_cancel to clean up any buffers in the PREPARED or
> + * Call queue_cancel to clean up any buffers in the
>* QUEUED state which is possible if buffers were prepared or
>* queued without ever calling STREAMON.
>*/
> @@ -921,6 +921,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
> vb2_buffer_state state)
> /* sync buffers */
> for (plane = 0; plane < vb->num_planes; ++plane)
> call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> + vb->synced = false;
Shouldn't be prepared cleaned as well on reqbufs?
> }
>
> spin_lock_irqsave(>done_lock, flags);
> @@ -1239,6 +1240,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> static int __buf_prepare(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> + enum vb2_buffer_state orig_state = vb->state;
> unsigned int plane;
> int ret;
>
> @@ -1247,6 +1249,10 @@ static int __buf_prepare(struct vb2_buffer *vb)
> return -EIO;
> }
>
> + if (vb->prepared)
> + return 0;
> + WARN_ON(vb->synced);
> +
> vb->state = VB2_BUF_STATE_PREPARING;
>
> switch (q->memory) {
> @@ -1262,11 +1268,12 @@ static int __buf_prepare(struct vb2_buffer *vb)
> default:
> WARN(1, "Invalid queue type\n");
> ret = -EINVAL;
> + break;
> }
Hmm... is this hunk a bug fix? if so, please split into a separate patch
and add a c/c stable.
>
> if (ret) {
> dprintk(1, "buffer preparation failed: %d\n", ret);
> - vb->state = VB2_BUF_STATE_DEQUEUED;
> + vb->state = orig_state;
> return ret;
> }
>
> @@ -1274,7 +1281,9 @@ static int __buf_prepare(struct vb2_buffer *vb)
> for (plane = 0; plane < vb->num_planes; ++plane)
> call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>
> - vb->state = VB2_BUF_STATE_PREPARED;
> + vb->synced = true;
> + vb->prepared = true;
> + vb->state = orig_state;
>
> return 0;
> }
> @@ -1290,6 +1299,10 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned
> int index, void *pb)
> vb->state);
> return -EINVAL;
> }
> + if (vb->prepared) {
> + dprintk(1, "buffer already prepared\n");
> + return -EINVAL;
> + }
>
> ret = __buf_prepare(vb);
> if (ret)
> @@ -1381,11 +1394,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
> index, void *pb)
>
> switch (vb->state) {
> case VB2_BUF_STATE_DEQUEUED:
> - ret = __buf_prepare(vb);
> - if (ret)
> - return ret;
> - break;
> - case VB2_BUF_STATE_PREPARED:
> + if (!vb->prepared) {
> + ret = __buf_prepare(vb);
> + if (ret)
> + return ret;
> + }
> break;
> case VB2_BUF_STATE_PREPARING:
> dprintk(1, "buffer still being prepared\n");
> @@ -1611,6 +1624,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int
> *pindex, void *pb,
> }
>
> call_void_vb_qop(vb, buf_finish, vb);
> + vb->prepared = false;
>
> if (pindex)
> *pindex = vb->index;
> @@ -1699,18 +1713,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> for (i = 0; i < q->num_buffers; ++i) {
> struct vb2_buffer *vb = q->bufs[i];
>
> - if (vb->state == VB2_BUF_STATE_PREPARED ||
> - vb->state == VB2_BUF_STATE_QUEUED) {
> + if (vb->synced) {
>
[PATCHv17 19/34] vb2: drop VB2_BUF_STATE_PREPARED, use bool prepared/synced instead
From: Hans Verkuil
The PREPARED state becomes a problem with the request API: a buffer
could be PREPARED but dequeued, or PREPARED and in state IN_REQUEST.
PREPARED is really not a state as such, but more a property of the
buffer. So make new 'prepared' and 'synced' bools instead to remember
whether the buffer is prepared and/or synced or not.
V4L2_BUF_FLAG_PREPARED is only set if the buffer is both synced and
prepared and in the DEQUEUED state.
Signed-off-by: Hans Verkuil
---
.../media/common/videobuf2/videobuf2-core.c | 38 +--
.../media/common/videobuf2/videobuf2-v4l2.c | 16 +---
include/media/videobuf2-core.h| 6 ++-
3 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c
b/drivers/media/common/videobuf2/videobuf2-core.c
index 7401a17c80ca..eead693ba619 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -682,7 +682,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory
memory,
}
/*
-* Call queue_cancel to clean up any buffers in the PREPARED or
+* Call queue_cancel to clean up any buffers in the
* QUEUED state which is possible if buffers were prepared or
* queued without ever calling STREAMON.
*/
@@ -921,6 +921,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum
vb2_buffer_state state)
/* sync buffers */
for (plane = 0; plane < vb->num_planes; ++plane)
call_void_memop(vb, finish, vb->planes[plane].mem_priv);
+ vb->synced = false;
}
spin_lock_irqsave(>done_lock, flags);
@@ -1239,6 +1240,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
static int __buf_prepare(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
+ enum vb2_buffer_state orig_state = vb->state;
unsigned int plane;
int ret;
@@ -1247,6 +1249,10 @@ static int __buf_prepare(struct vb2_buffer *vb)
return -EIO;
}
+ if (vb->prepared)
+ return 0;
+ WARN_ON(vb->synced);
+
vb->state = VB2_BUF_STATE_PREPARING;
switch (q->memory) {
@@ -1262,11 +1268,12 @@ static int __buf_prepare(struct vb2_buffer *vb)
default:
WARN(1, "Invalid queue type\n");
ret = -EINVAL;
+ break;
}
if (ret) {
dprintk(1, "buffer preparation failed: %d\n", ret);
- vb->state = VB2_BUF_STATE_DEQUEUED;
+ vb->state = orig_state;
return ret;
}
@@ -1274,7 +1281,9 @@ static int __buf_prepare(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane)
call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
- vb->state = VB2_BUF_STATE_PREPARED;
+ vb->synced = true;
+ vb->prepared = true;
+ vb->state = orig_state;
return 0;
}
@@ -1290,6 +1299,10 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned
int index, void *pb)
vb->state);
return -EINVAL;
}
+ if (vb->prepared) {
+ dprintk(1, "buffer already prepared\n");
+ return -EINVAL;
+ }
ret = __buf_prepare(vb);
if (ret)
@@ -1381,11 +1394,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int
index, void *pb)
switch (vb->state) {
case VB2_BUF_STATE_DEQUEUED:
- ret = __buf_prepare(vb);
- if (ret)
- return ret;
- break;
- case VB2_BUF_STATE_PREPARED:
+ if (!vb->prepared) {
+ ret = __buf_prepare(vb);
+ if (ret)
+ return ret;
+ }
break;
case VB2_BUF_STATE_PREPARING:
dprintk(1, "buffer still being prepared\n");
@@ -1611,6 +1624,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int
*pindex, void *pb,
}
call_void_vb_qop(vb, buf_finish, vb);
+ vb->prepared = false;
if (pindex)
*pindex = vb->index;
@@ -1699,18 +1713,18 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
for (i = 0; i < q->num_buffers; ++i) {
struct vb2_buffer *vb = q->bufs[i];
- if (vb->state == VB2_BUF_STATE_PREPARED ||
- vb->state == VB2_BUF_STATE_QUEUED) {
+ if (vb->synced) {
unsigned int plane;
for (plane = 0; plane < vb->num_planes; ++plane)
call_void_memop(vb, finish,
vb->planes[plane].mem_priv);
+ vb->synced = false;
}
-
