Re: [PATCHv17 19/34] vb2: drop VB2_BUF_STATE_PREPARED, use bool prepared/synced instead

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Sat,  4 Aug 2018 14:45:11 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> 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 <hans.verkuil@cisco.com>
> ---
>  .../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(&q->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) {
>  			unsigned int plane;
>  
>  			for (plane = 0; plane < vb->num_planes; ++plane)
>  				call_void_memop(vb, finish,
>  						vb->planes[plane].mem_priv);
> +			vb->synced = false;
>  		}
>  
> -		if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> -			vb->state = VB2_BUF_STATE_PREPARED;
> +		if (vb->prepared) {
>  			call_void_vb_qop(vb, buf_finish, vb);
> +			vb->prepared = false;
>  		}
>  		__vb2_dqbuf(vb);
>  	}
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 360dc4e7d413..a677e2c26247 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -352,9 +352,13 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
>  	if (ret)
>  		return ret;
>  
> -	/* Copy relevant information provided by the userspace */
> -	memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
> -	return vb2_fill_vb2_v4l2_buffer(vb, b);
> +	if (!vb->prepared) {
> +		/* Copy relevant information provided by the userspace */
> +		memset(vbuf->planes, 0,
> +		       sizeof(vbuf->planes[0]) * vb->num_planes);
> +		ret = vb2_fill_vb2_v4l2_buffer(vb, b);
> +	}
> +	return ret;
>  }
>  
>  /*
> @@ -443,9 +447,6 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	case VB2_BUF_STATE_DONE:
>  		b->flags |= V4L2_BUF_FLAG_DONE;
>  		break;
> -	case VB2_BUF_STATE_PREPARED:
> -		b->flags |= V4L2_BUF_FLAG_PREPARED;
> -		break;
>  	case VB2_BUF_STATE_PREPARING:
>  	case VB2_BUF_STATE_DEQUEUED:
>  	case VB2_BUF_STATE_REQUEUEING:
> @@ -453,6 +454,9 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  		break;
>  	}
>  
> +	if (vb->state == VB2_BUF_STATE_DEQUEUED && vb->synced && vb->prepared)
> +		b->flags |= V4L2_BUF_FLAG_PREPARED;
> +
>  	if (vb2_buffer_in_use(q, vb))
>  		b->flags |= V4L2_BUF_FLAG_MAPPED;
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 224c4820a044..5e760d5f280a 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -204,7 +204,6 @@ enum vb2_io_modes {
>   * enum vb2_buffer_state - current video buffer state.
>   * @VB2_BUF_STATE_DEQUEUED:	buffer under userspace control.
>   * @VB2_BUF_STATE_PREPARING:	buffer is being prepared in videobuf.
> - * @VB2_BUF_STATE_PREPARED:	buffer prepared in videobuf and by the driver.
>   * @VB2_BUF_STATE_QUEUED:	buffer queued in videobuf, but not in driver.
>   * @VB2_BUF_STATE_REQUEUEING:	re-queue a buffer to the driver.
>   * @VB2_BUF_STATE_ACTIVE:	buffer queued in driver and possibly used
> @@ -218,7 +217,6 @@ enum vb2_io_modes {
>  enum vb2_buffer_state {
>  	VB2_BUF_STATE_DEQUEUED,
>  	VB2_BUF_STATE_PREPARING,
> -	VB2_BUF_STATE_PREPARED,
>  	VB2_BUF_STATE_QUEUED,
>  	VB2_BUF_STATE_REQUEUEING,
>  	VB2_BUF_STATE_ACTIVE,
> @@ -250,6 +248,8 @@ struct vb2_buffer {
>  	/* private: internal use only
>  	 *
>  	 * state:		current buffer state; do not change
> +	 * synced:		this buffer has been synced
> +	 * prepared:		this buffer has been prepared

Please describe better what "prepared" and "synced" means, as
the above doesn't really help.

From what I got:

	- "prepared" means that buf_prepare() was called and
	  vidioc_dqbuf() (nor cancel) was not called;
	- "sync" means that the buf_prepare() was called
	  and the v4l2_buffer_done() (nor cancel)

right?


>  	 * queued_entry:	entry on the queued buffers list, which holds
>  	 *			all buffers queued from userspace
>  	 * done_entry:		entry on the list that stores all buffers ready
> @@ -257,6 +257,8 @@ struct vb2_buffer {
>  	 * vb2_plane:		per-plane information; do not change
>  	 */
>  	enum vb2_buffer_state	state;
> +	bool			synced;
> +	bool			prepared;
>  
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;



Thanks,
Mauro