Re: [RFC PATCH 1/3] media: v4l2-subdev.h: Add min and max enum

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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 <daniel@qtec.com>
> >>> ---
> >>>  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