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

[Marian Cichy]

On 3/17/21 11:58 AM, Laurent Pinchart wrote:
> 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 ?

Not yet, but is it always possible to make my vblank arbitrarily long? 
For example I have a 60 fps camera but I want to reduce it down to 10 
fps. Maybe some devices won't allow big enough vblank values.

Another possibility could be that you don't have a sensor but an HDMI 
input as your source, where you can't control the frame rate (but maybe 
this isn't even in the scope of libcamera).

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