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
