[libcamera-devel] [RFC PATCH 3/6] libcamera: v4l2_subdevice: Add methods to get/set frame interval

[Laurent Pinchart]

Hi Marian,

On Wed, Mar 17, 2021 at 10:55:25AM +0100, Marian Cichy wrote:
> On 3/16/21 8:28 PM, Laurent Pinchart wrote:
> > On Tue, Mar 16, 2021 at 04:52:08PM +0100, Marian Cichy wrote:
> >> Add API-methods to get and set the frame interval on V4L2-subdevices.
> >>
> >> Signed-off-by: Marian Cichy <m.cichy at pengutronix.de>
> >> ---
> >>   include/libcamera/internal/v4l2_subdevice.h |  4 ++
> >>   src/libcamera/v4l2_subdevice.cpp            | 43 +++++++++++++++++++++
> >>   2 files changed, 47 insertions(+)
> >>
> >> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> >> index d2b9ca55..310746d9 100644
> >> --- a/include/libcamera/internal/v4l2_subdevice.h
> >> +++ b/include/libcamera/internal/v4l2_subdevice.h
> >> @@ -12,6 +12,7 @@
> >>   #include <vector>
> >>   
> >>   #include <libcamera/class.h>
> >> +#include <libcamera/fraction.h>
> >>   #include <libcamera/geometry.h>
> >>   
> >>   #include "libcamera/internal/formats.h"
> >> @@ -60,6 +61,9 @@ public:
> >>   	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >>   		      Whence whence = ActiveFormat);
> >>   
> >> +	int getFrameInterval(unsigned int pad, Fraction *frameInterval);
> >> +	int setFrameInterval(unsigned int pad, Fraction *frameInterval);
> >> +
> >>   	static std::unique_ptr<V4L2Subdevice>
> >>   	fromEntityName(const MediaDevice *media, const std::string &entity);
> >>   
> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> >> index 721ff5a9..eac2d7d3 100644
> >> --- a/src/libcamera/v4l2_subdevice.cpp
> >> +++ b/src/libcamera/v4l2_subdevice.cpp
> >> @@ -17,6 +17,7 @@
> >>   #include <linux/media-bus-format.h>
> >>   #include <linux/v4l2-subdev.h>
> >>   
> >> +#include <libcamera/fraction.h>
> >>   #include <libcamera/geometry.h>
> >>   
> >>   #include "libcamera/internal/log.h"
> >> @@ -440,6 +441,48 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> >>   	return 0;
> >>   }
> >>   
> >> +int V4L2Subdevice::getFrameInterval(unsigned int pad, Fraction *frameInterval)
> >> +{
> >> +	struct v4l2_subdev_frame_interval subdevFrameInterval = {};
> >> +	subdevFrameInterval.pad = pad;
> >> +
> >> +	int ret = ioctl(VIDIOC_SUBDEV_G_FRAME_INTERVAL, &subdevFrameInterval);
> >
> > Sensor drivers are supposed to control the frame rate through horizontal
> > and vertical blanking. Only in very specific cases do we want to use
> > these ioctls, when the sensor exposes a frame interval at the register
> > level (this will only be applicable to sensors containing an ISP). What
> > sensor are you using ?
> 
> I am using the ar0237 sensor, it's driver is not upstream yet. We 
> implement hblank/vblank only as read-only controls for now.

Looks like there's some work to do on the sensor side then ;-)

> However, using the i.MX-6Q, it is also possible to configure 
> frame-dropping on the CSI-subdev. Afaik, this can only be configured 
> with the frame_interval ioctl since the frame interval has to be set on 
> source and sink pad.

Do you have a use case for this, compared to configuring the sensor with
a longer vertical blanking ?

> This is also why we propagate the frame interval from the sensor through 
> the media pipeline. If for some reason, the CSI is configured to drop 
> frames, we query the frame interval at the sensor, then try to set it on 
> all following subdevices to effectively deactivate frame dropping.
> 
> Of course, it is also thinkable that we want to configure the CSI to 
> drop frames, for example to manage bandwidth.
> 
> >> +	if (ret) {
> >> +		LOG(V4L2, Debug)
> >> +			<< "Unable to get frame interval on pad " << pad
> >> +			<< ": " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	frameInterval->numerator = subdevFrameInterval.interval.numerator;
> >> +	frameInterval->denominator = subdevFrameInterval.interval.denominator;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int V4L2Subdevice::setFrameInterval(unsigned int pad, Fraction *frameInterval)
> >> +{
> >> +	struct v4l2_subdev_frame_interval subdevFrameInterval = {};
> >> +	struct v4l2_fract fract = { frameInterval->numerator,
> >> +				    frameInterval->denominator };
> >> +
> >> +	subdevFrameInterval.pad = pad;
> >> +	subdevFrameInterval.interval = fract;
> >> +
> >> +	int ret = ioctl(VIDIOC_SUBDEV_S_FRAME_INTERVAL, &subdevFrameInterval);
> >> +	if (ret) {
> >> +		LOG(V4L2, Debug)
> >> +			<< "Unable to set frame interval on pad " << pad
> >> +			<< ": " << strerror(-ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	frameInterval->numerator = subdevFrameInterval.interval.numerator;
> >> +	frameInterval->denominator = subdevFrameInterval.interval.denominator;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>   /**
> >>    * \brief Create a new video subdevice instance from \a entity in media device
> >>    * \a media

-- 
Regards,

Laurent Pinchart