| Message ID | 20200619054123.19052-18-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Fri, Jun 19, 2020 at 02:41:23PM +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 Why ? V4L2_BUF_FLAG_DONE is never set anymore, that doesn't seem right. > - set the mapped flag on enqueueing Why ? Isn't it valid to queued an unmapped buffer ? > - set the flags in V4L2CameraProxy's internal buffers, and not just in > the buffers returned from qbuf It's really tempting to bundle multiple changes in the last patch, right ? :-) It would be nice to split this, especially given that for the second and third fixes a rationale should be given in the commit message (assuming the fixes are correct). > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > New in v2 > - split from "Fix v4l2-compliance streaming tests" > --- > src/v4l2/v4l2_camera_proxy.cpp | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > index 2eb2fcc..3e5fb7c 100644 > --- a/src/v4l2/v4l2_camera_proxy.cpp > +++ b/src/v4l2/v4l2_camera_proxy.cpp > @@ -221,7 +221,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; > @@ -569,6 +568,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) > if (arg->index >= bufferCount_) > return -EINVAL; > > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED) > + return -EINVAL; > + > int ret = lock(cf); > if (ret < 0) > return ret; > @@ -582,9 +584,12 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) > if (ret < 0) > return ret; > > - arg->flags |= V4L2_BUF_FLAG_QUEUED; > + arg->flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; > arg->flags &= ~V4L2_BUF_FLAG_DONE; > > + buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; > + buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE; Maybe buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE; arg->flags = buffers_[arg->index].flags; to avoid duplicating the logic ? > + > return ret; > } >
On Sat, Jun 20, 2020 at 05:33:14AM +0300, Laurent Pinchart wrote: > Hi Paul, > > Thank you for the patch. > > On Fri, Jun 19, 2020 at 02:41:23PM +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 > > Why ? V4L2_BUF_FLAG_DONE is never set anymore, that doesn't seem right. It is. It's set in V4L2CameraProxy::updateBuffers(), in the first patch that added the compatibility layer. > > - set the mapped flag on enqueueing > > Why ? Isn't it valid to queued an unmapped buffer ? That's what I thought too. Anyway looks like it complies fine without it so I'll get rid of it. > > - set the flags in V4L2CameraProxy's internal buffers, and not just in > > the buffers returned from qbuf > > It's really tempting to bundle multiple changes in the last patch, right Well they're related and small soo... :p > ? :-) It would be nice to split this, especially given that for the > second and third fixes a rationale should be given in the commit message > (assuming the fixes are correct). I'll expand the rationale then. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > > --- > > New in v2 > > - split from "Fix v4l2-compliance streaming tests" > > --- > > src/v4l2/v4l2_camera_proxy.cpp | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp > > index 2eb2fcc..3e5fb7c 100644 > > --- a/src/v4l2/v4l2_camera_proxy.cpp > > +++ b/src/v4l2/v4l2_camera_proxy.cpp > > @@ -221,7 +221,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; > > @@ -569,6 +568,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) > > if (arg->index >= bufferCount_) > > return -EINVAL; > > > > + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED) > > + return -EINVAL; > > + > > int ret = lock(cf); > > if (ret < 0) > > return ret; > > @@ -582,9 +584,12 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) > > if (ret < 0) > > return ret; > > > > - arg->flags |= V4L2_BUF_FLAG_QUEUED; > > + arg->flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; > > arg->flags &= ~V4L2_BUF_FLAG_DONE; > > > > + buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; > > + buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE; > > Maybe > > buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; > buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE; > arg->flags = buffers_[arg->index].flags; > > to avoid duplicating the logic ? > > > + > > return ret; > > } > > Thanks, Paul
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp index 2eb2fcc..3e5fb7c 100644 --- a/src/v4l2/v4l2_camera_proxy.cpp +++ b/src/v4l2/v4l2_camera_proxy.cpp @@ -221,7 +221,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; @@ -569,6 +568,9 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) if (arg->index >= bufferCount_) return -EINVAL; + if (buffers_[arg->index].flags & V4L2_BUF_FLAG_QUEUED) + return -EINVAL; + int ret = lock(cf); if (ret < 0) return ret; @@ -582,9 +584,12 @@ int V4L2CameraProxy::vidioc_qbuf(V4L2CameraFile *cf, struct v4l2_buffer *arg) if (ret < 0) return ret; - arg->flags |= V4L2_BUF_FLAG_QUEUED; + arg->flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; arg->flags &= ~V4L2_BUF_FLAG_DONE; + buffers_[arg->index].flags |= V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_MAPPED; + buffers_[arg->index].flags &= ~V4L2_BUF_FLAG_DONE; + return ret; }
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 - set the mapped flag on enqueueing - 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> --- New in v2 - split from "Fix v4l2-compliance streaming tests" --- src/v4l2/v4l2_camera_proxy.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)