[PATCH] v4l2: v4l2_camera_proxy: Fix VIDIOC_[GS]_PARM support
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 5 11:05:02 CET 2024
Quoting Laurent Pinchart (2024-11-05 09:23:36)
> On Tue, Nov 05, 2024 at 09:10:01AM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2024-11-04 22:10:35)
> > > On Mon, Nov 04, 2024 at 04:16:20PM +0000, Kieran Bingham wrote:
> > > > Quoting Laurent Pinchart (2024-11-04 15:38:25)
> > > > > v4l2-compliance reports an error due to VIDIOC_S_PARM being supported
> > > > > without VIDIOC_G_PARM. Fix it by implementing VIDIOC_G_PARM. To satisfy
> > > > > all compliance tests, VIDIOC_S_PARM also needs to be updated to properly
> > > > > zero reserved fields.
> > > >
> > > > Fixes tag ?
> > >
> > > It has never worked. I can point to the commit that introduced the
> > > compat layer.
> >
> > But the VIDIOC_S_PARM was added?
>
> I'll use
>
> Fixes: 5456e02d3f5b ("v4l2: Support setting frame rate in the V4L2 Adaptation layer")
Ack, that's what I was expecting..
>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > > ---
> > > > > src/v4l2/v4l2_camera.h | 1 +
> > > > > src/v4l2/v4l2_camera_proxy.cpp | 63 ++++++++++++++++++++++++++++++++--
> > > > > src/v4l2/v4l2_camera_proxy.h | 2 ++
> > > > > 3 files changed, 64 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > > > index e54371fb4e00..9bd161b909de 100644
> > > > > --- a/src/v4l2/v4l2_camera.h
> > > > > +++ b/src/v4l2/v4l2_camera.h
> > > > > @@ -52,6 +52,7 @@ public:
> > > > > libcamera::StreamConfiguration *streamConfigOut);
> > > > >
> > > > > libcamera::ControlList &controls() { return controls_; }
> > > > > + const libcamera::ControlInfoMap &controlInfo() { return camera_->controls(); }
> > > > >
> > > > > int allocBuffers(unsigned int count);
> > > > > void freeBuffers();
> > > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > > > index 5ac8df4cffef..559ffc6170b1 100644
> > > > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > > @@ -195,6 +195,29 @@ void V4L2CameraProxy::setFmtFromConfig(const StreamConfiguration &streamConfig)
> > > > > v4l2PixFormat_.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> > > > >
> > > > > sizeimage_ = streamConfig.frameSize;
> > > > > +
> > > > > + const ControlInfoMap &controls = vcam_->controlInfo();
> > > > > + const auto &it = controls.find(&controls::FrameDurationLimits);
> > > > > +
> > > > > + if (it != controls.end()) {
> > > > > + const int64_t duration = it->second.def().get<int64_t>();
> > > > > +
> > > > > + v4l2TimePerFrame_.numerator = duration;
> > > > > + v4l2TimePerFrame_.denominator = 1000000;
> > > > > + } else {
> > > > > + /*
> > > > > + * Default to 30fps if the camera doesn't expose the
> > > > > + * FrameDurationLimits control.
> > > > > + *
> > > > > + * \todo Remove this once all pipeline handlers implement the
> > > > > + * control
> > > > > + */
> > > > > + LOG(V4L2Compat, Warning)
> > > > > + << "Camera does not support FrameDurationLimits";
> > > > > +
> > > > > + v4l2TimePerFrame_.numerator = 333333;
> > > > > + v4l2TimePerFrame_.denominator = 1000000;
> > > > > + }
> > > > > }
> > > > >
> > > > > void V4L2CameraProxy::querycap(std::shared_ptr<Camera> camera)
> > > > > @@ -758,6 +781,22 @@ int V4L2CameraProxy::vidioc_streamoff(V4L2CameraFile *file, int *arg)
> > > > > return ret;
> > > > > }
> > > > >
> > > > > +int V4L2CameraProxy::vidioc_g_parm(V4L2CameraFile *file, struct v4l2_streamparm *arg)
> > > > > +{
> > > > > + LOG(V4L2Compat, Debug)
> > > > > + << "[" << file->description() << "] " << __func__ << "()";
> > > > > +
> > > > > + if (!validateBufferType(arg->type))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + memset(&arg->parm, 0, sizeof(arg->parm));
> > > > > +
> > > > > + arg->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > >
> > > > I guess this wasn't being handled at all before ?
> > >
> > > G_PARM wasn't handled at all. Or is your comment about something else ?
> > >
> > > > > + arg->parm.capture.timeperframe = v4l2TimePerFrame_;
> > > >
> > > > What does this return /before/ the camera is configured? Does it work ?
> > >
> > > setFmtFromConfig() is called from the class constructor, so
> > > v4l2TimePerFrame_ is always initialized.
> >
> > I meant that V4L2_CAP_TIMEPERFRAME wasn't used during
> > V4L2CameraProxy::vidioc_s_parm before this patch...
>
> I right. Yes, this patch also modified vidioc_s_parm() below to handle
> V4L2_CAP_TIMEPERFRAME.
>
> I thought about addressing that in a separate patch, but (if I recall
> correctly) it then causes another compliance issue if g_parm isn't set.
> It was not immediately possible to test that change independently, so I
> ended up bundling everything in a single patch.
Together is fine with me.
The Fixes tag will mean it will appear in the Fixes section of the next
libcamera release notes. That's the only reason I care for it ;-)
--
Kieran
>
> > > > Otherwise seems like a good fix.
> > > >
> > > >
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > >
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > int V4L2CameraProxy::vidioc_s_parm(V4L2CameraFile *file, struct v4l2_streamparm *arg)
> > > > > {
> > > > > LOG(V4L2Compat, Debug)
> > > > > @@ -766,9 +805,25 @@ int V4L2CameraProxy::vidioc_s_parm(V4L2CameraFile *file, struct v4l2_streamparm
> > > > > if (!validateBufferType(arg->type))
> > > > > return -EINVAL;
> > > > >
> > > > > - struct v4l2_fract *timeperframe = &arg->parm.capture.timeperframe;
> > > > > - utils::Duration frameDuration = 1.0s * timeperframe->numerator / timeperframe->denominator;
> > > > > + /*
> > > > > + * Store the frame duration if it is valid, otherwise keep the current
> > > > > + * value.
> > > > > + *
> > > > > + * \todo The provided value should be adjusted based on the camera
> > > > > + * capabilities.
> > > > > + */
> > > > > + if (arg->parm.capture.timeperframe.numerator &&
> > > > > + arg->parm.capture.timeperframe.denominator)
> > > > > + v4l2TimePerFrame_ = arg->parm.capture.timeperframe;
> > > > >
> > > > > + memset(&arg->parm, 0, sizeof(arg->parm));
> > > > > +
> > > > > + arg->parm.capture.capability = V4L2_CAP_TIMEPERFRAME;
> > > > > + arg->parm.capture.timeperframe = v4l2TimePerFrame_;
> > > > > +
> > > > > + /* Apply the frame duration. */
> > > > > + utils::Duration frameDuration = 1.0s * v4l2TimePerFrame_.numerator
> > > > > + / v4l2TimePerFrame_.denominator;
> > > > > int64_t uDuration = frameDuration.get<std::micro>();
> > > > > vcam_->controls().set(controls::FrameDurationLimits, { uDuration, uDuration });
> > > > >
> > > > > @@ -795,6 +850,7 @@ const std::set<unsigned long> V4L2CameraProxy::supportedIoctls_ = {
> > > > > VIDIOC_EXPBUF,
> > > > > VIDIOC_STREAMON,
> > > > > VIDIOC_STREAMOFF,
> > > > > + VIDIOC_G_PARM,
> > > > > VIDIOC_S_PARM,
> > > > > };
> > > > >
> > > > > @@ -883,6 +939,9 @@ int V4L2CameraProxy::ioctl(V4L2CameraFile *file, unsigned long longRequest, void
> > > > > case VIDIOC_STREAMOFF:
> > > > > ret = vidioc_streamoff(file, static_cast<int *>(arg));
> > > > > break;
> > > > > + case VIDIOC_G_PARM:
> > > > > + ret = vidioc_g_parm(file, static_cast<struct v4l2_streamparm *>(arg));
> > > > > + break;
> > > > > case VIDIOC_S_PARM:
> > > > > ret = vidioc_s_parm(file, static_cast<struct v4l2_streamparm *>(arg));
> > > > > break;
> > > > > diff --git a/src/v4l2/v4l2_camera_proxy.h b/src/v4l2/v4l2_camera_proxy.h
> > > > > index c957db5349cc..5aa352c36f6a 100644
> > > > > --- a/src/v4l2/v4l2_camera_proxy.h
> > > > > +++ b/src/v4l2/v4l2_camera_proxy.h
> > > > > @@ -67,6 +67,7 @@ private:
> > > > > int vidioc_expbuf(V4L2CameraFile *file, struct v4l2_exportbuffer *arg);
> > > > > int vidioc_streamon(V4L2CameraFile *file, int *arg);
> > > > > int vidioc_streamoff(V4L2CameraFile *file, int *arg);
> > > > > + int vidioc_g_parm(V4L2CameraFile *file, struct v4l2_streamparm *arg);
> > > > > int vidioc_s_parm(V4L2CameraFile *file, struct v4l2_streamparm *arg);
> > > > >
> > > > > bool hasOwnership(V4L2CameraFile *file);
> > > > > @@ -85,6 +86,7 @@ private:
> > > > >
> > > > > struct v4l2_capability capabilities_;
> > > > > struct v4l2_pix_format v4l2PixFormat_;
> > > > > + struct v4l2_fract v4l2TimePerFrame_;
> > > > >
> > > > > std::vector<struct v4l2_buffer> buffers_;
> > > > > std::map<void *, unsigned int> mmaps_;
> > > > >
> > > > > base-commit: 58598f4dde9a3d4236ed52881da9b155443cbc50
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list