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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 27/11/2023 12:13, 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.
> 
> The series is based on Sakari's latest master branch ([1]).
> 
> Given the large number of drivers that this series touches, I would like
> to get it merged in v6.8 without too much delay to avoid rebasing.
> 
> [1] https://git.linuxtv.org/sailus/media_tree.git/log/
> 
> 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

Wouldn't it be possible to first add the get/set_frame_interval() op
to v4l2-subdev.h (so keep the old one), then add the which field,
and only after that convert the subdev drivers.

At the end there is a final patch removing the old ops.

Main reason is that the core changes are easier to review, and it is
easier to deal with cases where a subdev patch no longer applies, you
can merge the remainder and fix that subdev in a follow-up patch.

Only when all subdevs are converted is the final patch applied.

I might well have missed something that prevents doing this, but if
possible I think this would be a better approach.

Regards,

	Hans

> 
>  .../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                   | 160 ++++++++++--------
>  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         | 128 ++++++++++----
>  .../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                   |  65 +++++--
>  include/uapi/linux/v4l2-subdev.h              |  13 +-
>  44 files changed, 706 insertions(+), 413 deletions(-)
> 
> 
> base-commit: 543efaddeac0c7769c39d7aaa886e8b001acac76





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux