Hi all,
I would like to discuss with you guys the next two following topics:
* VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL:
I'm working on a driver for the imx378 sensor but the current v4l2-subdev API
(VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL) doesn't allow you to set up a range of frame
intervals. However, this is supported in the v4l2 device API level. My idea is
to follow the same approach as the VIDIOC_SUBDEV_ENUM_FRAME_SIZE by setting a
min and max intervals in the VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL to solve this
missing support.
This is the current output of VIDIOC_ENUM_FRAMEINTERVALS in continous mode:
v4l2-ctl --list-frameintervals width=1920,height=1080,pixelformat=pRAA \
-d /dev/video0
ioctl: VIDIOC_ENUM_FRAMEINTERVALS
Interval: Continuous 0.029s - 0.607s (1.648-34.797 fps)
This is the current output of VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL:
v4l2-ctl --list-subdev-frameintervals code=0x300f,width=1920,height=1080 \
-d /dev/v4l-subdev19
ioctl: VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL (pad=0)
Interval: 0.029s (34.797 fps)
So, the idea would be to return the interval range from the v4l2-subdev level
to the device level. Besides that, it would also be necessary to adapt the
v4l-utils tools (compliance and ctl).
What do you think guys?
Please, follow the RFC patch series to see my suggestion.
* V4L2_CID_TEMPERATURE:
In addition to this, the driver is able to report as a v4l2 control the
temperature of the sensor. Since is quite 'general' control I also included the
v4l2 temperature control as part of the common v4l2 control list.
Would it be also possible?
In the RFC patch series you will find the the modified code for the
VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL and V4L2_CID_TEMPERATURE topics as well as
the imx378 driver using the above.
Daniel Gomez (3):
media: v4l2-subdev.h: Add min and max enum
media: v4l2: Add v4l2 control IDs for temperature
media: imx378: Add imx378 camera sensor driver
drivers/media/i2c/Kconfig | 11 +
drivers/media/i2c/Makefile | 1 +
drivers/media/i2c/imx378.c | 1829 ++++++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +
include/uapi/linux/v4l2-controls.h | 4 +-
include/uapi/linux/v4l2-subdev.h | 6 +-
6 files changed, 1854 insertions(+), 2 deletions(-)
create mode 100644 drivers/media/i2c/imx378.c
--
2.25.1
Add min and max structures to the v4l2-subdev callback in order to allow
the subdev to return a range of valid frame intervals.
This would operate similar to the struct v4l2_subdev_frame_size_enum and
its max and min values for the width and the height. In this case, the
possibility to return a frame interval range is added to the v4l2-subdev level
whenever the v4l2 device operates in step-wise or continuous mode.
Signed-off-by: Daniel Gomez <[email protected]>
---
include/uapi/linux/v4l2-subdev.h | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index 03970ce30741..ee15393c58cd 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
* @code: format code (MEDIA_BUS_FMT_ definitions)
* @width: frame width in pixels
* @height: frame height in pixels
+ * @min_interval: min frame interval in seconds
+ * @max_interval: max frame interval in seconds
* @interval: frame interval in seconds
* @which: format type (from enum v4l2_subdev_format_whence)
*/
@@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
__u32 code;
__u32 width;
__u32 height;
+ struct v4l2_fract min_interval;
+ struct v4l2_fract max_interval;
struct v4l2_fract interval;
__u32 which;
- __u32 reserved[8];
+ __u32 reserved[4];
};
/**
--
2.25.1
Add a v4l2 control ID to handle the temperature.
Signed-off-by: Daniel Gomez <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++
include/uapi/linux/v4l2-controls.h | 4 +++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 93d33d1db4e8..17b93111baa8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers";
case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component";
case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr";
+ case V4L2_CID_TEMPERATURE: return "Temperature";
/* Codec controls */
/* The MPEG controls are applicable to all codec controls
@@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
*type = V4L2_CTRL_TYPE_INTEGER;
*flags |= V4L2_CTRL_FLAG_READ_ONLY;
break;
+ case V4L2_CID_TEMPERATURE:
+ *type = V4L2_CTRL_TYPE_INTEGER;
+ *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
+ break;
case V4L2_CID_MPEG_VIDEO_DEC_PTS:
*type = V4L2_CTRL_TYPE_INTEGER64;
*flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 1a58d7cc4ccc..76ec0a6da8d5 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -143,8 +143,10 @@ enum v4l2_colorfx {
#define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41)
#define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42)
+#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43)
+
/* last CID + 1 */
-#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43)
+#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44)
/* USER-class private control IDs */
--
2.25.1
Hi Daniel,
On Tue, Apr 14, 2020 at 10:01:50PM +0200, Daniel Gomez wrote:
> Add a v4l2 control ID to handle the temperature.
>
> Signed-off-by: Daniel Gomez <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++
> include/uapi/linux/v4l2-controls.h | 4 +++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 93d33d1db4e8..17b93111baa8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -783,6 +783,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT: return "Min Number of Output Buffers";
> case V4L2_CID_ALPHA_COMPONENT: return "Alpha Component";
> case V4L2_CID_COLORFX_CBCR: return "Color Effects, CbCr";
> + case V4L2_CID_TEMPERATURE: return "Temperature";
What's the unit of this control? I think it should have one.
As Hans pointed out, documentation is needed.
>
> /* Codec controls */
> /* The MPEG controls are applicable to all codec controls
> @@ -1344,6 +1345,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> *type = V4L2_CTRL_TYPE_INTEGER;
> *flags |= V4L2_CTRL_FLAG_READ_ONLY;
> break;
> + case V4L2_CID_TEMPERATURE:
> + *type = V4L2_CTRL_TYPE_INTEGER;
> + *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> + break;
> case V4L2_CID_MPEG_VIDEO_DEC_PTS:
> *type = V4L2_CTRL_TYPE_INTEGER64;
> *flags |= V4L2_CTRL_FLAG_VOLATILE | V4L2_CTRL_FLAG_READ_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 1a58d7cc4ccc..76ec0a6da8d5 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -143,8 +143,10 @@ enum v4l2_colorfx {
> #define V4L2_CID_ALPHA_COMPONENT (V4L2_CID_BASE+41)
> #define V4L2_CID_COLORFX_CBCR (V4L2_CID_BASE+42)
>
> +#define V4L2_CID_TEMPERATURE (V4L2_CID_BASE+43)
> +
> /* last CID + 1 */
> -#define V4L2_CID_LASTP1 (V4L2_CID_BASE+43)
> +#define V4L2_CID_LASTP1 (V4L2_CID_BASE+44)
>
> /* USER-class private control IDs */
>
--
Kind regards,
Sakari Ailus
Hi Daniel,
Thanks for the patchset.
I'm also cc'ing Laurent who I think could be interested in this one.
On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> Add min and max structures to the v4l2-subdev callback in order to allow
> the subdev to return a range of valid frame intervals.
>
> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> its max and min values for the width and the height. In this case, the
> possibility to return a frame interval range is added to the v4l2-subdev level
> whenever the v4l2 device operates in step-wise or continuous mode.
The current API only allows providing a list of enumerated values. That is
limiting indeed, especially on register list based sensor drivers where
vertical blanking is configurable.
I guess this could be extended to cover what V4L2, more or less. If we tell
it's a range, is it assumed to be contiguous? We don't have try operation
for the frame interval, but I guess set is good enough. The fraction is
probably best for TV standards but it's not what camera sensors natively
use. (But for a register list based driver, the established practice
remains to use frame interval.)
I'm also wondering the effect on existing user space; if a driver gives a
range, how will the existing programs work with such a driver?
I'd add an anonymous union with the interval field, the other field being
min_interval. Then the current applications would get the minimum interval
and still continue to function. I guess compilers are modern enough these
days we can have an anonymous union in the uAPI?
>
> Signed-off-by: Daniel Gomez <[email protected]>
> ---
> include/uapi/linux/v4l2-subdev.h | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 03970ce30741..ee15393c58cd 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> * @code: format code (MEDIA_BUS_FMT_ definitions)
> * @width: frame width in pixels
> * @height: frame height in pixels
> + * @min_interval: min frame interval in seconds
> + * @max_interval: max frame interval in seconds
> * @interval: frame interval in seconds
> * @which: format type (from enum v4l2_subdev_format_whence)
> */
> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> __u32 code;
> __u32 width;
> __u32 height;
> + struct v4l2_fract min_interval;
> + struct v4l2_fract max_interval;
This changes the memory layout of the struct and breaks the ABI. Any new
fields in the struct may only replace reserved fields while the rest must
stay unchanged.
> struct v4l2_fract interval;
> __u32 which;
> - __u32 reserved[8];
> + __u32 reserved[4];
> };
>
> /**
--
Regards,
Sakari Ailus
Hello,
On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the patchset.
>
> I'm also cc'ing Laurent who I think could be interested in this one.
>
> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > Add min and max structures to the v4l2-subdev callback in order to allow
> > the subdev to return a range of valid frame intervals.
> >
> > This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > its max and min values for the width and the height. In this case, the
> > possibility to return a frame interval range is added to the v4l2-subdev level
> > whenever the v4l2 device operates in step-wise or continuous mode.
>
> The current API only allows providing a list of enumerated values. That is
> limiting indeed, especially on register list based sensor drivers where
> vertical blanking is configurable.
>
> I guess this could be extended to cover what V4L2, more or less. If we tell
> it's a range, is it assumed to be contiguous? We don't have try operation
> for the frame interval, but I guess set is good enough. The fraction is
> probably best for TV standards but it's not what camera sensors natively
> use. (But for a register list based driver, the established practice
> remains to use frame interval.)
>
> I'm also wondering the effect on existing user space; if a driver gives a
> range, how will the existing programs work with such a driver?
>
> I'd add an anonymous union with the interval field, the other field being
> min_interval. Then the current applications would get the minimum interval
> and still continue to function. I guess compilers are modern enough these
> days we can have an anonymous union in the uAPI?
We can discuss all this, but given patch 3/3 in this series, I think
this isn't the right API :-) The sensor driver should not expose the
frame interval enumeration API. It should instead expose control of the
frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
V4L2_CID_VBLANK.
> > Signed-off-by: Daniel Gomez <[email protected]>
> > ---
> > include/uapi/linux/v4l2-subdev.h | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > index 03970ce30741..ee15393c58cd 100644
> > --- a/include/uapi/linux/v4l2-subdev.h
> > +++ b/include/uapi/linux/v4l2-subdev.h
> > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> > * @code: format code (MEDIA_BUS_FMT_ definitions)
> > * @width: frame width in pixels
> > * @height: frame height in pixels
> > + * @min_interval: min frame interval in seconds
> > + * @max_interval: max frame interval in seconds
> > * @interval: frame interval in seconds
> > * @which: format type (from enum v4l2_subdev_format_whence)
> > */
> > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> > __u32 code;
> > __u32 width;
> > __u32 height;
> > + struct v4l2_fract min_interval;
> > + struct v4l2_fract max_interval;
>
> This changes the memory layout of the struct and breaks the ABI. Any new
> fields in the struct may only replace reserved fields while the rest must
> stay unchanged.
>
> > struct v4l2_fract interval;
> > __u32 which;
> > - __u32 reserved[8];
> > + __u32 reserved[4];
> > };
> >
> > /**
--
Regards,
Laurent Pinchart
Hi Laurent,
On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> Hello,
>
> On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > Thanks for the patchset.
> >
> > I'm also cc'ing Laurent who I think could be interested in this one.
> >
> > On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > Add min and max structures to the v4l2-subdev callback in order to allow
> > > the subdev to return a range of valid frame intervals.
> > >
> > > This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > its max and min values for the width and the height. In this case, the
> > > possibility to return a frame interval range is added to the v4l2-subdev level
> > > whenever the v4l2 device operates in step-wise or continuous mode.
> >
> > The current API only allows providing a list of enumerated values. That is
> > limiting indeed, especially on register list based sensor drivers where
> > vertical blanking is configurable.
> >
> > I guess this could be extended to cover what V4L2, more or less. If we tell
> > it's a range, is it assumed to be contiguous? We don't have try operation
> > for the frame interval, but I guess set is good enough. The fraction is
> > probably best for TV standards but it's not what camera sensors natively
> > use. (But for a register list based driver, the established practice
> > remains to use frame interval.)
> >
> > I'm also wondering the effect on existing user space; if a driver gives a
> > range, how will the existing programs work with such a driver?
> >
> > I'd add an anonymous union with the interval field, the other field being
> > min_interval. Then the current applications would get the minimum interval
> > and still continue to function. I guess compilers are modern enough these
> > days we can have an anonymous union in the uAPI?
>
> We can discuss all this, but given patch 3/3 in this series, I think
> this isn't the right API :-) The sensor driver should not expose the
> frame interval enumeration API. It should instead expose control of the
> frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> V4L2_CID_VBLANK.
>
That would require also exposing the size of the pixel array (and the
analogue crop), in order to provide all the necessary information to
calculate the frame rate. No objections there; this is a new driver.
There are however existing drivers that implement s_frame_interval subdev
ioctl; those might benefit from this one. Or would you implement the pixel
rate based control as well, and effectively deprecate the s_frame_interval
on those?
> > > Signed-off-by: Daniel Gomez <[email protected]>
> > > ---
> > > include/uapi/linux/v4l2-subdev.h | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > > index 03970ce30741..ee15393c58cd 100644
> > > --- a/include/uapi/linux/v4l2-subdev.h
> > > +++ b/include/uapi/linux/v4l2-subdev.h
> > > @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> > > * @code: format code (MEDIA_BUS_FMT_ definitions)
> > > * @width: frame width in pixels
> > > * @height: frame height in pixels
> > > + * @min_interval: min frame interval in seconds
> > > + * @max_interval: max frame interval in seconds
> > > * @interval: frame interval in seconds
> > > * @which: format type (from enum v4l2_subdev_format_whence)
> > > */
> > > @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> > > __u32 code;
> > > __u32 width;
> > > __u32 height;
> > > + struct v4l2_fract min_interval;
> > > + struct v4l2_fract max_interval;
> >
> > This changes the memory layout of the struct and breaks the ABI. Any new
> > fields in the struct may only replace reserved fields while the rest must
> > stay unchanged.
> >
> > > struct v4l2_fract interval;
> > > __u32 which;
> > > - __u32 reserved[8];
> > > + __u32 reserved[4];
> > > };
> > >
> > > /**
>
--
Regards,
Sakari Ailus
Hi Sakari,
On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> >>> Add min and max structures to the v4l2-subdev callback in order to allow
> >>> the subdev to return a range of valid frame intervals.
> >>>
> >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> >>> its max and min values for the width and the height. In this case, the
> >>> possibility to return a frame interval range is added to the v4l2-subdev level
> >>> whenever the v4l2 device operates in step-wise or continuous mode.
> >>
> >> The current API only allows providing a list of enumerated values. That is
> >> limiting indeed, especially on register list based sensor drivers where
> >> vertical blanking is configurable.
> >>
> >> I guess this could be extended to cover what V4L2, more or less. If we tell
> >> it's a range, is it assumed to be contiguous? We don't have try operation
> >> for the frame interval, but I guess set is good enough. The fraction is
> >> probably best for TV standards but it's not what camera sensors natively
> >> use. (But for a register list based driver, the established practice
> >> remains to use frame interval.)
> >>
> >> I'm also wondering the effect on existing user space; if a driver gives a
> >> range, how will the existing programs work with such a driver?
> >>
> >> I'd add an anonymous union with the interval field, the other field being
> >> min_interval. Then the current applications would get the minimum interval
> >> and still continue to function. I guess compilers are modern enough these
> >> days we can have an anonymous union in the uAPI?
> >
> > We can discuss all this, but given patch 3/3 in this series, I think
> > this isn't the right API :-) The sensor driver should not expose the
> > frame interval enumeration API. It should instead expose control of the
> > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > V4L2_CID_VBLANK.
> >
>
> That would require also exposing the size of the pixel array (and the
> analogue crop), in order to provide all the necessary information to
> calculate the frame rate. No objections there; this is a new driver.
>
> There are however existing drivers that implement s_frame_interval subdev
> ioctl; those might benefit from this one. Or would you implement the pixel
> rate based control as well, and effectively deprecate the s_frame_interval
> on those?
That's what I would recommend, yes. I would only keep
.s_frame_interval() for sensors that expose that concept at the hardware
level (for instance with an integrated ISP whose firmware exposes a
frame interval or frame rate control).
> >>> Signed-off-by: Daniel Gomez <[email protected]>
> >>> ---
> >>> include/uapi/linux/v4l2-subdev.h | 6 +++++-
> >>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> >>> index 03970ce30741..ee15393c58cd 100644
> >>> --- a/include/uapi/linux/v4l2-subdev.h
> >>> +++ b/include/uapi/linux/v4l2-subdev.h
> >>> @@ -117,6 +117,8 @@ struct v4l2_subdev_frame_interval {
> >>> * @code: format code (MEDIA_BUS_FMT_ definitions)
> >>> * @width: frame width in pixels
> >>> * @height: frame height in pixels
> >>> + * @min_interval: min frame interval in seconds
> >>> + * @max_interval: max frame interval in seconds
> >>> * @interval: frame interval in seconds
> >>> * @which: format type (from enum v4l2_subdev_format_whence)
> >>> */
> >>> @@ -126,9 +128,11 @@ struct v4l2_subdev_frame_interval_enum {
> >>> __u32 code;
> >>> __u32 width;
> >>> __u32 height;
> >>> + struct v4l2_fract min_interval;
> >>> + struct v4l2_fract max_interval;
> >>
> >> This changes the memory layout of the struct and breaks the ABI. Any new
> >> fields in the struct may only replace reserved fields while the rest must
> >> stay unchanged.
> >>
> >>> struct v4l2_fract interval;
> >>> __u32 which;
> >>> - __u32 reserved[8];
> >>> + __u32 reserved[4];
> >>> };
> >>>
> >>> /**
--
Regards,
Laurent Pinchart
Hi Laurent,
On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > >>> the subdev to return a range of valid frame intervals.
> > >>>
> > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > >>> its max and min values for the width and the height. In this case, the
> > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > >>
> > >> The current API only allows providing a list of enumerated values. That is
> > >> limiting indeed, especially on register list based sensor drivers where
> > >> vertical blanking is configurable.
> > >>
> > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > >> for the frame interval, but I guess set is good enough. The fraction is
> > >> probably best for TV standards but it's not what camera sensors natively
> > >> use. (But for a register list based driver, the established practice
> > >> remains to use frame interval.)
> > >>
> > >> I'm also wondering the effect on existing user space; if a driver gives a
> > >> range, how will the existing programs work with such a driver?
> > >>
> > >> I'd add an anonymous union with the interval field, the other field being
> > >> min_interval. Then the current applications would get the minimum interval
> > >> and still continue to function. I guess compilers are modern enough these
> > >> days we can have an anonymous union in the uAPI?
> > >
> > > We can discuss all this, but given patch 3/3 in this series, I think
> > > this isn't the right API :-) The sensor driver should not expose the
> > > frame interval enumeration API. It should instead expose control of the
> > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > V4L2_CID_VBLANK.
> > >
> >
> > That would require also exposing the size of the pixel array (and the
> > analogue crop), in order to provide all the necessary information to
> > calculate the frame rate. No objections there; this is a new driver.
> >
> > There are however existing drivers that implement s_frame_interval subdev
> > ioctl; those might benefit from this one. Or would you implement the pixel
> > rate based control as well, and effectively deprecate the s_frame_interval
> > on those?
>
> That's what I would recommend, yes. I would only keep
> .s_frame_interval() for sensors that expose that concept at the hardware
> level (for instance with an integrated ISP whose firmware exposes a
> frame interval or frame rate control).
Sounds good to me.
Jacopo's set exposing read-only subdevs completes the puzzle so the user
space should have all it needs, right?
--
Regards,
Sakari Ailus
On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> > On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> > > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > > > >>> the subdev to return a range of valid frame intervals.
> > > > >>>
> > > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > > >>> its max and min values for the width and the height. In this case, the
> > > > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > > > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > > > >>
> > > > >> The current API only allows providing a list of enumerated values. That is
> > > > >> limiting indeed, especially on register list based sensor drivers where
> > > > >> vertical blanking is configurable.
> > > > >>
> > > > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > > > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > > > >> for the frame interval, but I guess set is good enough. The fraction is
> > > > >> probably best for TV standards but it's not what camera sensors natively
> > > > >> use. (But for a register list based driver, the established practice
> > > > >> remains to use frame interval.)
> > > > >>
> > > > >> I'm also wondering the effect on existing user space; if a driver gives a
> > > > >> range, how will the existing programs work with such a driver?
> > > > >>
> > > > >> I'd add an anonymous union with the interval field, the other field being
> > > > >> min_interval. Then the current applications would get the minimum interval
> > > > >> and still continue to function. I guess compilers are modern enough these
> > > > >> days we can have an anonymous union in the uAPI?
> > > > >
> > > > > We can discuss all this, but given patch 3/3 in this series, I think
> > > > > this isn't the right API :-) The sensor driver should not expose the
> > > > > frame interval enumeration API. It should instead expose control of the
> > > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > > > V4L2_CID_VBLANK.
> > > > >
> > > >
> > > > That would require also exposing the size of the pixel array (and the
> > > > analogue crop), in order to provide all the necessary information to
> > > > calculate the frame rate. No objections there; this is a new driver.
> > > >
> > > > There are however existing drivers that implement s_frame_interval subdev
> > > > ioctl; those might benefit from this one. Or would you implement the pixel
> > > > rate based control as well, and effectively deprecate the s_frame_interval
> > > > on those?
> > >
> > > That's what I would recommend, yes. I would only keep
> > > .s_frame_interval() for sensors that expose that concept at the hardware
> > > level (for instance with an integrated ISP whose firmware exposes a
> > > frame interval or frame rate control).
> >
> > Sounds good to me.
> >
> > Jacopo's set exposing read-only subdevs completes the puzzle so the user
> > space should have all it needs, right?
>
> Until we run into the next missing piece :-)
I was thinking of the frame rate configuration. Can you confirm that?
--
Sakari Ailus
Hi Sakari,
On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> > > >> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> > > >>> Add min and max structures to the v4l2-subdev callback in order to allow
> > > >>> the subdev to return a range of valid frame intervals.
> > > >>>
> > > >>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> > > >>> its max and min values for the width and the height. In this case, the
> > > >>> possibility to return a frame interval range is added to the v4l2-subdev level
> > > >>> whenever the v4l2 device operates in step-wise or continuous mode.
> > > >>
> > > >> The current API only allows providing a list of enumerated values. That is
> > > >> limiting indeed, especially on register list based sensor drivers where
> > > >> vertical blanking is configurable.
> > > >>
> > > >> I guess this could be extended to cover what V4L2, more or less. If we tell
> > > >> it's a range, is it assumed to be contiguous? We don't have try operation
> > > >> for the frame interval, but I guess set is good enough. The fraction is
> > > >> probably best for TV standards but it's not what camera sensors natively
> > > >> use. (But for a register list based driver, the established practice
> > > >> remains to use frame interval.)
> > > >>
> > > >> I'm also wondering the effect on existing user space; if a driver gives a
> > > >> range, how will the existing programs work with such a driver?
> > > >>
> > > >> I'd add an anonymous union with the interval field, the other field being
> > > >> min_interval. Then the current applications would get the minimum interval
> > > >> and still continue to function. I guess compilers are modern enough these
> > > >> days we can have an anonymous union in the uAPI?
> > > >
> > > > We can discuss all this, but given patch 3/3 in this series, I think
> > > > this isn't the right API :-) The sensor driver should not expose the
> > > > frame interval enumeration API. It should instead expose control of the
> > > > frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > > V4L2_CID_VBLANK.
> > > >
> > >
> > > That would require also exposing the size of the pixel array (and the
> > > analogue crop), in order to provide all the necessary information to
> > > calculate the frame rate. No objections there; this is a new driver.
> > >
> > > There are however existing drivers that implement s_frame_interval subdev
> > > ioctl; those might benefit from this one. Or would you implement the pixel
> > > rate based control as well, and effectively deprecate the s_frame_interval
> > > on those?
> >
> > That's what I would recommend, yes. I would only keep
> > .s_frame_interval() for sensors that expose that concept at the hardware
> > level (for instance with an integrated ISP whose firmware exposes a
> > frame interval or frame rate control).
>
> Sounds good to me.
>
> Jacopo's set exposing read-only subdevs completes the puzzle so the user
> space should have all it needs, right?
Until we run into the next missing piece :-)
--
Regards,
Laurent Pinchart
Hi Sakari,
On Thu, Apr 30, 2020 at 05:18:49PM +0300, Sakari Ailus wrote:
> On Thu, Apr 30, 2020 at 05:17:53PM +0300, Laurent Pinchart wrote:
> > On Thu, Apr 30, 2020 at 05:15:52PM +0300, Sakari Ailus wrote:
> >> On Thu, Apr 30, 2020 at 04:59:04PM +0300, Laurent Pinchart wrote:
> >>> On Thu, Apr 30, 2020 at 04:31:25PM +0300, Sakari Ailus wrote:
> >>>> On Thu, Apr 30, 2020 at 02:10:14PM +0300, Laurent Pinchart wrote:
> >>>>> On Thu, Apr 30, 2020 at 12:42:33PM +0300, Sakari Ailus wrote:
> >>>>>> On Tue, Apr 14, 2020 at 10:01:49PM +0200, Daniel Gomez wrote:
> >>>>>>> Add min and max structures to the v4l2-subdev callback in order to allow
> >>>>>>> the subdev to return a range of valid frame intervals.
> >>>>>>>
> >>>>>>> This would operate similar to the struct v4l2_subdev_frame_size_enum and
> >>>>>>> its max and min values for the width and the height. In this case, the
> >>>>>>> possibility to return a frame interval range is added to the v4l2-subdev level
> >>>>>>> whenever the v4l2 device operates in step-wise or continuous mode.
> >>>>>>
> >>>>>> The current API only allows providing a list of enumerated values. That is
> >>>>>> limiting indeed, especially on register list based sensor drivers where
> >>>>>> vertical blanking is configurable.
> >>>>>>
> >>>>>> I guess this could be extended to cover what V4L2, more or less. If we tell
> >>>>>> it's a range, is it assumed to be contiguous? We don't have try operation
> >>>>>> for the frame interval, but I guess set is good enough. The fraction is
> >>>>>> probably best for TV standards but it's not what camera sensors natively
> >>>>>> use. (But for a register list based driver, the established practice
> >>>>>> remains to use frame interval.)
> >>>>>>
> >>>>>> I'm also wondering the effect on existing user space; if a driver gives a
> >>>>>> range, how will the existing programs work with such a driver?
> >>>>>>
> >>>>>> I'd add an anonymous union with the interval field, the other field being
> >>>>>> min_interval. Then the current applications would get the minimum interval
> >>>>>> and still continue to function. I guess compilers are modern enough these
> >>>>>> days we can have an anonymous union in the uAPI?
> >>>>>
> >>>>> We can discuss all this, but given patch 3/3 in this series, I think
> >>>>> this isn't the right API :-) The sensor driver should not expose the
> >>>>> frame interval enumeration API. It should instead expose control of the
> >>>>> frame rate through V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> >>>>> V4L2_CID_VBLANK.
> >>>>>
> >>>>
> >>>> That would require also exposing the size of the pixel array (and the
> >>>> analogue crop), in order to provide all the necessary information to
> >>>> calculate the frame rate. No objections there; this is a new driver.
> >>>>
> >>>> There are however existing drivers that implement s_frame_interval subdev
> >>>> ioctl; those might benefit from this one. Or would you implement the pixel
> >>>> rate based control as well, and effectively deprecate the s_frame_interval
> >>>> on those?
> >>>
> >>> That's what I would recommend, yes. I would only keep
> >>> .s_frame_interval() for sensors that expose that concept at the hardware
> >>> level (for instance with an integrated ISP whose firmware exposes a
> >>> frame interval or frame rate control).
> >>
> >> Sounds good to me.
> >>
> >> Jacopo's set exposing read-only subdevs completes the puzzle so the user
> >> space should have all it needs, right?
> >
> > Until we run into the next missing piece :-)
>
> I was thinking of the frame rate configuration. Can you confirm that?
I believe so, yes.
--
Regards,
Laurent Pinchart