[libcamera-devel,v3,21/22] v4l2: v4l2_camera_proxy: Fix buffer flags related to queueing

Message ID 20200623190836.53446-22-paul.elder@ideasonboard.com
State Superseded
Headers show
Series
  • Support v4l2-compliance
Related show

Commit Message

Paul Elder June 23, 2020, 7:08 p.m. UTC
Fix buffer flags related to queueing and dequeueing:
- don't allow a buffer with the same index that is already in the queue
  to be enqueued again
- don't set the done flag on dequeueing in
  V4L2CameraProxy::updateBuffers()
- set the flags in V4L2CameraProxy's internal buffers, and not just in
  the buffers returned from qbuf

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v3:
- remove "set the mapped flag on enqueueing"
- clean up some logic
- expand commit message very slightly

New in v2
- split from "Fix v4l2-compliance streaming tests"
---
 src/v4l2/v4l2_camera_proxy.cpp | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart June 23, 2020, 11:14 p.m. UTC | #1
Hi Paul,

Thank you for the patch.

On Wed, Jun 24, 2020 at 04:08:35AM +0900, Paul Elder wrote:
> Fix buffer flags related to queueing and dequeueing:
> - don't allow a buffer with the same index that is already in the queue
>   to be enqueued again
> - don't set the done flag on dequeueing in
>   V4L2CameraProxy::updateBuffers()
> - set the flags in V4L2CameraProxy's internal buffers, and not just in
>   the buffers returned from qbuf
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v3:
> - remove "set the mapped flag on enqueueing"
> - clean up some logic
> - expand commit message very slightly
> 
> New in v2
> - split from "Fix v4l2-compliance streaming tests"
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index ce0d755..ed3bcbc 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -219,7 +219,6 @@ void V4L2CameraProxy::updateBuffers()
>  			buf.timestamp.tv_usec = fmd.timestamp % 1000000;
>  			buf.sequence = fmd.sequence;
>  
> -			buf.flags |= V4L2_BUF_FLAG_DONE;

I think this hunk is incorrect, V4L2_BUF_FLAG_DONE isn't set anywhere
anymore. The rest looks good, with this line restored,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			break;
>  		case FrameMetadata::FrameError:
>  			buf.flags |= V4L2_BUF_FLAG_ERROR;
> @@ -570,6 +569,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  	if (arg->index >= bufferCount_)
>  		return -EINVAL;
>  
> +	if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED)
> +		return -EINVAL;
> +
>  	if (!hasOwnership(file))
>  		return -EBUSY;
>  
> @@ -582,8 +584,10 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
>  	if (ret < 0)
>  		return ret;
>  
> -	arg->flags |= V4L2_BUF_FLAG_QUEUED;
> -	arg->flags &= ~V4L2_BUF_FLAG_DONE;
> +	buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED;
> +	buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE;
> +
> +	arg->flags = buffers_[arg->index].flags;
>  
>  	return ret;
>  }

Patch

diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index ce0d755..ed3bcbc 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -219,7 +219,6 @@  void V4L2CameraProxy::updateBuffers()
 			buf.timestamp.tv_usec = fmd.timestamp % 1000000;
 			buf.sequence = fmd.sequence;
 
-			buf.flags |= V4L2_BUF_FLAG_DONE;
 			break;
 		case FrameMetadata::FrameError:
 			buf.flags |= V4L2_BUF_FLAG_ERROR;
@@ -570,6 +569,9 @@  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
 	if (arg->index >= bufferCount_)
 		return -EINVAL;
 
+	if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED)
+		return -EINVAL;
+
 	if (!hasOwnership(file))
 		return -EBUSY;
 
@@ -582,8 +584,10 @@  int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *file, struct v4l2_buffer *arg)
 	if (ret < 0)
 		return ret;
 
-	arg->flags |= V4L2_BUF_FLAG_QUEUED;
-	arg->flags &= ~V4L2_BUF_FLAG_DONE;
+	buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED;
+	buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE;
+
+	arg->flags = buffers_[arg->index].flags;
 
 	return ret;
 }