v4l2: v4l2_camera_proxy: Fix VIDIOC_[GS]_PARM support
diff mbox series

Message ID 20241104153825.21053-1-laurent.pinchart@ideasonboard.com
State Accepted
Commit 77438775b39b04748284ce86fe64e913d711482b
Headers show
Series
  • v4l2: v4l2_camera_proxy: Fix VIDIOC_[GS]_PARM support
Related show

Commit Message

Laurent Pinchart Nov. 4, 2024, 3:38 p.m. UTC 
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.

Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)


base-commit: 58598f4dde9a3d4236ed52881da9b155443cbc50

Comments

Kieran Bingham Nov. 4, 2024, 4:16 p.m. UTC | #1 
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@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@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
>
Laurent Pinchart Nov. 4, 2024, 10:10 p.m. UTC | #2 
Hi Kieran,

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.

> > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

> Otherwise seems like a good fix.
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@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
Laurent Pinchart Nov. 4, 2024, 10:54 p.m. UTC | #3 
On Tue, Nov 05, 2024 at 12:10:35AM +0200, Laurent Pinchart wrote:
> Hi Kieran,
> 
> 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.
> 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 the open() function, not the constructor, sorry. The conclusion
still holds.

> > Otherwise seems like a good fix.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@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
Kieran Bingham Nov. 5, 2024, 9:10 a.m. UTC | #4 
Quoting Laurent Pinchart (2024-11-04 22:10:35)
> Hi Kieran,
> 
> 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?

> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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...

> 
> > Otherwise seems like a good fix.
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@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
Laurent Pinchart Nov. 5, 2024, 9:23 a.m. UTC | #5 
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")

> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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.

> > > Otherwise seems like a good fix.
> > > 
> > > 
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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
Kieran Bingham Nov. 5, 2024, 10:05 a.m. UTC | #6 
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@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@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
Paul Elder Nov. 5, 2024, 11:44 a.m. UTC | #7 
On Mon, Nov 04, 2024 at 05:38:25PM +0200, Laurent Pinchart wrote:
> 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.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good to me.

Reviewed-by: Paul Elder <paul.elder@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;
> +	arg->parm.capture.timeperframe = v4l2TimePerFrame_;
> +
> +	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
>

Patch
diff mbox series 

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;
+	arg->parm.capture.timeperframe = v4l2TimePerFrame_;
+
+	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_;