On Thu, Dec 14, 2023 at 10:59:09AM +0100, Tommaso Merciai wrote: > On Thu, Dec 14, 2023 at 11:17:10AM +0200, Laurent Pinchart wrote: > > On Thu, Dec 14, 2023 at 09:23:07AM +0100, Tommaso Merciai wrote: > > > On Wed, Dec 13, 2023 at 05:00:04PM +0200, Laurent Pinchart wrote: > > > > The subdev .[gs]_frame_interval are video operations, but they operate > > > > on pads (and even on streams). Not only is this confusing, it causes > > > > practical issues for drivers as the operations don't receive a subdev > > > > state pointer, requiring manual state handling. > > > > > > > > To improve the situation, turn the operations into pad operations, and > > > > extend them to receive a state pointer like other pad operations. > > > > > > > > While at it, rename the operations to .[gs]et_frame_interval at the same > > > > time to match the naming scheme of other pad operations. This isn't > > > > strictly necessary, but given that all drivers using those operations > > > > need to be modified, handling the rename separately would generate more > > > > churn for very little gain (if at all). > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx> > > > > Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> # for imx-media > > > > Reviewed-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> # for tegra-video > > > > --- > > > > Changes since v5: > > > > > > > > - Address the alvium-csi2 driver > > > > > > > > Changes since v3: > > > > > > > > - Address the thp7312 driver > > > > --- > > > > drivers/media/i2c/adv7180.c | 7 ++- > > > > drivers/media/i2c/alvium-csi2.c | 11 ++-- > > > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 6 +- > > > > drivers/media/i2c/imx214.c | 9 +-- > > > > drivers/media/i2c/imx274.c | 48 +++++--------- > > > > drivers/media/i2c/max9286.c | 14 +++-- > > > > drivers/media/i2c/mt9m111.c | 14 +++-- > > > > drivers/media/i2c/mt9m114.c | 14 +++-- > > > > drivers/media/i2c/mt9v011.c | 18 +++--- > > > > drivers/media/i2c/mt9v111.c | 16 ++--- > > > > drivers/media/i2c/ov2680.c | 7 ++- > > > > drivers/media/i2c/ov5640.c | 16 ++--- > > > > drivers/media/i2c/ov5648.c | 59 ++++++++--------- > > > > drivers/media/i2c/ov5693.c | 7 ++- > > > > drivers/media/i2c/ov6650.c | 16 ++--- > > > > drivers/media/i2c/ov7251.c | 6 +- > > > > drivers/media/i2c/ov7670.c | 18 +++--- > > > > drivers/media/i2c/ov772x.c | 14 +++-- > > > > drivers/media/i2c/ov7740.c | 40 +++++------- > > > > drivers/media/i2c/ov8865.c | 51 +++++++-------- > > > > drivers/media/i2c/ov9650.c | 14 +++-- > > > > drivers/media/i2c/s5c73m3/s5c73m3-core.c | 14 +++-- > > > > drivers/media/i2c/s5k5baf.c | 20 +++--- > > > > drivers/media/i2c/thp7312.c | 22 +++---- > > > > drivers/media/i2c/tvp514x.c | 29 +++------ > > > > drivers/media/usb/em28xx/em28xx-video.c | 6 +- > > > > drivers/media/v4l2-core/v4l2-common.c | 8 +-- > > > > drivers/media/v4l2-core/v4l2-subdev.c | 63 +++++++++++-------- > > > > .../media/atomisp/i2c/atomisp-gc0310.c | 7 ++- > > > > .../media/atomisp/i2c/atomisp-gc2235.c | 7 ++- > > > > .../media/atomisp/i2c/atomisp-mt9m114.c | 7 ++- > > > > .../media/atomisp/i2c/atomisp-ov2722.c | 7 ++- > > > > .../staging/media/atomisp/pci/atomisp_cmd.c | 4 +- > > > > .../staging/media/atomisp/pci/atomisp_ioctl.c | 4 +- > > > > drivers/staging/media/imx/imx-ic-prp.c | 14 +++-- > > > > drivers/staging/media/imx/imx-ic-prpencvf.c | 14 +++-- > > > > drivers/staging/media/imx/imx-media-capture.c | 6 +- > > > > drivers/staging/media/imx/imx-media-csi.c | 14 +++-- > > > > drivers/staging/media/imx/imx-media-vdic.c | 14 +++-- > > > > drivers/staging/media/tegra-video/csi.c | 9 +-- > > > > include/media/v4l2-common.h | 4 +- > > > > include/media/v4l2-subdev.h | 22 ++++--- > > > > 42 files changed, 352 insertions(+), 348 deletions(-) > > > > [snip] > > > > > > diff --git a/drivers/media/i2c/alvium-csi2.c b/drivers/media/i2c/alvium-csi2.c > > > > index c8827603eb23..a173abb0509f 100644 > > > > --- a/drivers/media/i2c/alvium-csi2.c > > > > +++ b/drivers/media/i2c/alvium-csi2.c > > > > @@ -1649,6 +1649,7 @@ static int alvium_hw_init(struct alvium_dev *alvium) > > > > /* --------------- Subdev Operations --------------- */ > > > > > > > > static int alvium_g_frame_interval(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *sd_state, > > > > struct v4l2_subdev_frame_interval *fi) > > > > { > > > > struct alvium_dev *alvium = sd_to_alvium(sd); > > > > @@ -1696,23 +1697,19 @@ static int alvium_set_frame_interval(struct alvium_dev *alvium, > > > > } > > > > > > > > static int alvium_s_frame_interval(struct v4l2_subdev *sd, > > > > + struct v4l2_subdev_state *sd_state, > > > > struct v4l2_subdev_frame_interval *fi) > > > > { > > > > struct alvium_dev *alvium = sd_to_alvium(sd); > > > > - struct v4l2_subdev_state *state; > > > > int ret; > > > > > > > > if (alvium->streaming) > > > > return -EBUSY; > > > > > > > > - state = v4l2_subdev_lock_and_get_active_state(sd); > > > > - > > > > ret = alvium_set_frame_interval(alvium, fi); > > > > if (!ret) > > > > ret = alvium_set_frame_rate(alvium); > > > > > > > > - v4l2_subdev_unlock_state(state); > > > > - > > > > return ret; > > > > } > > > > > > > > @@ -2238,8 +2235,6 @@ static const struct v4l2_subdev_core_ops alvium_core_ops = { > > > > }; > > > > > > > > static const struct v4l2_subdev_video_ops alvium_video_ops = { > > > > - .g_frame_interval = alvium_g_frame_interval, > > > > - .s_frame_interval = alvium_s_frame_interval, > > > > .s_stream = alvium_s_stream, > > > > }; > > > > > > > > @@ -2249,6 +2244,8 @@ static const struct v4l2_subdev_pad_ops alvium_pad_ops = { > > > > .set_fmt = alvium_set_fmt, > > > > .get_selection = alvium_get_selection, > > > > .set_selection = alvium_set_selection, > > > > + .get_frame_interval = alvium_g_frame_interval, > > > > + .set_frame_interval = alvium_s_frame_interval, > > > > }; > > > > > > > > static const struct v4l2_subdev_internal_ops alvium_internal_ops = { > > > > > > Applied and tested on my side. From functional perspective > > > all is working properly on my hw. Hope this help. > > > > > > Tested-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx> > > > > Would you be able to send a patch on top of the media_stage master > > branch to support the TRY frame intervals in the alvium driver ? > > Can I use ov5640_try_frame_interval as reference? No, that driver hasn't been converted. You can look at patch 8/8 in this series instead. The goal is to drop at least the following fields from the alvium_dev structure: struct v4l2_fract frame_interval; u64 dft_fr; u64 min_fr; u64 max_fr; u64 fr; Some of them are used only to pass data from alvium_get_frame_interval() to its callers, so you can turn that into function parameters instead. fr and frame_interval should be replaced by usage of the subdev state. Looking some more at the driver, you should also drop the mode field. > > [snip] -- Regards, Laurent Pinchart