[PATCH] v4l2: v4l2_camera_proxy: Fix VIDIOC_[GS]_PARM support

[Kieran Bingham]

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 ?

> 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 ?

> +       arg->parm.capture.timeperframe = v4l2TimePerFrame_;

What does this return /before/ the camera is configured? Does it work ?


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
>