Re: [RFC PATCH v1 0/4] media: v4l2-subdev: Improve frame interval handling

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

On Tue, Oct 24, 2023 at 10:31:57AM +0300, Tomi Valkeinen wrote:
> On 24/10/2023 03:51, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series improves frame interval handling in the V4L2 subdev
> > in-kernel and userspace APIs.
> > 
> > Frame interval are exposed to userspace on pads and streams, but the
> > frame interval handling is currently implemented through a v4l2_subdev
> > video operation, without involving the subdev state. This makes frame
> > intervals a second class citizen compared to formats and selection
> > rectangles.
> > 
> > Patch 1/4 starts by addressing the first issue, namely the frame
> > interval operations being video ops. This requires touching all the
> > drivers using frame intervals.
> > 
> > Patch 2/4 then adds a 'which' field to the subdev frame interval
> > userspace API, allowing frame intervals to be tried the same way formats
> > and selection rectangles can. Again, the same drivers need to be touched
> > to preserve their current behaviour.
> > 
> > Patch 3/4 adds support for storing the frame interval in the subdev
> > state, alongside the formats and selection rectangles, with similar
> > accessors and helper functions.
> > 
> > Finally, patch 4/4 demonstrates how this is used in drivers, with the
> > thp7312 driver serving as an example. This driver isn't upstream yet,
> > the latest version can be found on the linux-media mailing list ([1]).
> > 
> > Sakari, these patches conflict with your "[PATCH v3 0/8] Unify
> > sub-device state access functions" series. I plan to rebase on top once
> > it reaches the media tree. This shouldn't prevent reviewing this RFC in
> > the meantime :-)
> 
> I think these look fine, but I'll do an official review when the non-RFC 
> is posted.
> 
> Should v4l2-compliance be updated?

Probably, although it's going to be difficult to test this feature,
especially given that none of the virtual drivers implement it.

> > [1] https://lore.kernel.org/linux-media/20231017132103.9914-1-laurent.pinchart@ideasonboard.com/
> > 
> > Laurent Pinchart (4):
> >    media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations
> >    media: v4l2-subdev: Add which field to struct
> >      v4l2_subdev_frame_interval
> >    media: v4l2-subdev: Store frame interval in subdev state
> >    media: i2c: thp7312: Store frame interval in subdev state
> > 
> >   .../media/v4l/vidioc-subdev-g-client-cap.rst  |   5 +
> >   .../v4l/vidioc-subdev-g-frame-interval.rst    |  17 +-
> >   drivers/media/i2c/adv7180.c                   |  10 +-
> >   drivers/media/i2c/et8ek8/et8ek8_driver.c      |  12 +-
> >   drivers/media/i2c/imx214.c                    |  12 +-
> >   drivers/media/i2c/imx274.c                    |  54 +++----
> >   drivers/media/i2c/max9286.c                   |  20 ++-
> >   drivers/media/i2c/mt9m111.c                   |  20 ++-
> >   drivers/media/i2c/mt9m114.c                   |  20 ++-
> >   drivers/media/i2c/mt9v011.c                   |  24 +--
> >   drivers/media/i2c/mt9v111.c                   |  22 ++-
> >   drivers/media/i2c/ov2680.c                    |  10 +-
> >   drivers/media/i2c/ov5640.c                    |  22 ++-
> >   drivers/media/i2c/ov5648.c                    |  62 ++++----
> >   drivers/media/i2c/ov5693.c                    |  10 +-
> >   drivers/media/i2c/ov6650.c                    |  22 ++-
> >   drivers/media/i2c/ov7251.c                    |  12 +-
> >   drivers/media/i2c/ov7670.c                    |  22 +--
> >   drivers/media/i2c/ov772x.c                    |  20 ++-
> >   drivers/media/i2c/ov7740.c                    |  40 ++---
> >   drivers/media/i2c/ov8865.c                    |  54 ++++---
> >   drivers/media/i2c/ov9650.c                    |  20 ++-
> >   drivers/media/i2c/s5c73m3/s5c73m3-core.c      |  20 ++-
> >   drivers/media/i2c/s5k5baf.c                   |  26 ++--
> >   drivers/media/i2c/thp7312.c                   | 145 ++++++++++--------
> >   drivers/media/i2c/tvp514x.c                   |  33 ++--
> >   drivers/media/usb/em28xx/em28xx-video.c       |   6 +-
> >   drivers/media/v4l2-core/v4l2-common.c         |   8 +-
> >   drivers/media/v4l2-core/v4l2-subdev.c         | 120 +++++++++++----
> >   .../media/atomisp/i2c/atomisp-gc0310.c        |  10 +-
> >   .../media/atomisp/i2c/atomisp-gc2235.c        |  10 +-
> >   .../media/atomisp/i2c/atomisp-mt9m114.c       |  10 +-
> >   .../media/atomisp/i2c/atomisp-ov2722.c        |  10 +-
> >   .../staging/media/atomisp/pci/atomisp_cmd.c   |   4 +-
> >   .../staging/media/atomisp/pci/atomisp_ioctl.c |   4 +-
> >   drivers/staging/media/imx/imx-ic-prp.c        |  20 ++-
> >   drivers/staging/media/imx/imx-ic-prpencvf.c   |  20 ++-
> >   drivers/staging/media/imx/imx-media-capture.c |   6 +-
> >   drivers/staging/media/imx/imx-media-csi.c     |  20 ++-
> >   drivers/staging/media/imx/imx-media-vdic.c    |  20 ++-
> >   drivers/staging/media/tegra-video/csi.c       |  12 +-
> >   include/media/v4l2-common.h                   |   4 +-
> >   include/media/v4l2-subdev.h                   |  79 ++++++++--
> >   include/uapi/linux/v4l2-subdev.h              |  13 +-
> >   44 files changed, 706 insertions(+), 404 deletions(-)

-- 
Regards,

Laurent Pinchart