Hi Tommaso,
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 ?
[snip]
--
Regards,
Laurent Pinchart
