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
