From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tommaso Merciai <tomm.merciai@gmail.com>
Cc: linux-media@vger.kernel.org,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Subject: Re: [PATCH v6 2/8] media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations
Date: Thu, 14 Dec 2023 13:12:52 +0200 [thread overview]
Message-ID: <20231214111252.GA21146@pendragon.ideasonboard.com> (raw)
In-Reply-To: <ZXrR7W4BlHCJWNX9@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
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@ideasonboard.com>
> > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> # for imx-media
> > > > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com> # 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@gmail.com>
> >
> > 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
next prev parent reply other threads:[~2023-12-14 11:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-13 15:00 [PATCH v6 0/8] media: v4l2-subdev: Improve frame interval handling Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 1/8] media: v4l: subdev: Move out subdev state lock macros outside CONFIG_MEDIA_CONTROLLER Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 2/8] media: v4l2-subdev: Turn .[gs]_frame_interval into pad operations Laurent Pinchart
2023-12-14 8:23 ` Tommaso Merciai
2023-12-14 9:17 ` Laurent Pinchart
2023-12-14 9:59 ` Tommaso Merciai
2023-12-14 11:12 ` Laurent Pinchart [this message]
2023-12-13 15:00 ` [PATCH v6 3/8] media: v4l2-subdev: Add which field to struct v4l2_subdev_frame_interval Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 4/8] media: v4l2-subdev: Store frame interval in subdev state Laurent Pinchart
2023-12-14 8:26 ` Tommaso Merciai
2023-12-14 8:36 ` Sakari Ailus
2023-12-14 8:44 ` Tommaso Merciai
2023-12-13 15:00 ` [PATCH v6 5/8] media: docs: uAPI: Clarify error documentation for invalid 'which' value Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 6/8] media: docs: uAPI: Expand " Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 7/8] media: docs: uAPI: Fix documentation of 'which' field for routing ioctls Laurent Pinchart
2023-12-13 15:00 ` [PATCH v6 8/8] media: i2c: thp7312: Store frame interval in subdev state Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231214111252.GA21146@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=tomm.merciai@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox