* [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct
@ 2021-06-09 10:02 Tomi Valkeinen
[not found] ` <20210609100228.278798-2-tomi.valkeinen@ideasonboard.com>
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-09 10:02 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: Tomi Valkeinen
Hi,
v3 of the patch. I rebased on top of latest linux-media, and applied the
semantic patch. I've addressed Laurent's comment (fix kfree, kernel doc
fixes, return cleanup).
v2 can be found from:
https://lore.kernel.org/linux-media/20210527094244.617473-1-tomi.valkeinen@ideasonboard.com/
Tomi
Tomi Valkeinen (1):
media: v4l2-subdev: add subdev-wide state struct
drivers/media/i2c/adv7170.c | 6 +-
drivers/media/i2c/adv7175.c | 6 +-
drivers/media/i2c/adv7180.c | 18 +--
drivers/media/i2c/adv7183.c | 8 +-
drivers/media/i2c/adv748x/adv748x-afe.c | 13 +-
drivers/media/i2c/adv748x/adv748x-csi2.c | 14 +-
drivers/media/i2c/adv748x/adv748x-hdmi.c | 13 +-
drivers/media/i2c/adv7511-v4l2.c | 10 +-
drivers/media/i2c/adv7604.c | 12 +-
drivers/media/i2c/adv7842.c | 12 +-
drivers/media/i2c/ak881x.c | 6 +-
drivers/media/i2c/ccs/ccs-core.c | 84 ++++++-----
drivers/media/i2c/cx25840/cx25840-core.c | 2 +-
drivers/media/i2c/et8ek8/et8ek8_driver.c | 23 +--
drivers/media/i2c/hi556.c | 15 +-
drivers/media/i2c/imx208.c | 19 +--
drivers/media/i2c/imx214.c | 37 ++---
drivers/media/i2c/imx219.c | 30 ++--
drivers/media/i2c/imx258.c | 19 +--
drivers/media/i2c/imx274.c | 32 ++--
drivers/media/i2c/imx290.c | 20 +--
drivers/media/i2c/imx319.c | 18 +--
drivers/media/i2c/imx334.c | 18 +--
drivers/media/i2c/imx355.c | 18 +--
drivers/media/i2c/m5mols/m5mols_core.c | 21 ++-
drivers/media/i2c/max9286.c | 17 ++-
drivers/media/i2c/ml86v7667.c | 4 +-
drivers/media/i2c/mt9m001.c | 18 +--
drivers/media/i2c/mt9m032.c | 34 +++--
drivers/media/i2c/mt9m111.c | 18 +--
drivers/media/i2c/mt9p031.c | 45 +++---
drivers/media/i2c/mt9t001.c | 44 +++---
drivers/media/i2c/mt9t112.c | 14 +-
drivers/media/i2c/mt9v011.c | 6 +-
drivers/media/i2c/mt9v032.c | 44 +++---
drivers/media/i2c/mt9v111.c | 23 +--
drivers/media/i2c/noon010pc30.c | 19 ++-
drivers/media/i2c/ov02a10.c | 17 ++-
drivers/media/i2c/ov13858.c | 18 +--
drivers/media/i2c/ov2640.c | 16 +-
drivers/media/i2c/ov2659.c | 14 +-
drivers/media/i2c/ov2680.c | 23 +--
drivers/media/i2c/ov2685.c | 10 +-
drivers/media/i2c/ov2740.c | 15 +-
drivers/media/i2c/ov5640.c | 14 +-
drivers/media/i2c/ov5645.c | 38 ++---
drivers/media/i2c/ov5647.c | 26 ++--
drivers/media/i2c/ov5648.c | 14 +-
drivers/media/i2c/ov5670.c | 19 +--
drivers/media/i2c/ov5675.c | 15 +-
drivers/media/i2c/ov5695.c | 15 +-
drivers/media/i2c/ov6650.c | 28 ++--
drivers/media/i2c/ov7251.c | 39 ++---
drivers/media/i2c/ov7670.c | 17 ++-
drivers/media/i2c/ov772x.c | 12 +-
drivers/media/i2c/ov7740.c | 17 ++-
drivers/media/i2c/ov8856.c | 15 +-
drivers/media/i2c/ov8865.c | 14 +-
drivers/media/i2c/ov9640.c | 8 +-
drivers/media/i2c/ov9650.c | 17 ++-
drivers/media/i2c/ov9734.c | 15 +-
drivers/media/i2c/rdacm20.c | 4 +-
drivers/media/i2c/rdacm21.c | 4 +-
drivers/media/i2c/rj54n1cb0c.c | 12 +-
drivers/media/i2c/s5c73m3/s5c73m3-core.c | 55 +++----
drivers/media/i2c/s5k4ecgx.c | 22 +--
drivers/media/i2c/s5k5baf.c | 49 +++---
drivers/media/i2c/s5k6a3.c | 19 ++-
drivers/media/i2c/s5k6aa.c | 39 ++---
drivers/media/i2c/saa6752hs.c | 6 +-
drivers/media/i2c/saa7115.c | 2 +-
drivers/media/i2c/saa717x.c | 2 +-
drivers/media/i2c/sr030pc30.c | 8 +-
drivers/media/i2c/st-mipid02.c | 21 +--
drivers/media/i2c/tc358743.c | 8 +-
drivers/media/i2c/tda1997x.c | 14 +-
drivers/media/i2c/tvp514x.c | 6 +-
drivers/media/i2c/tvp5150.c | 20 +--
drivers/media/i2c/tvp7002.c | 11 +-
drivers/media/i2c/tw9910.c | 10 +-
drivers/media/i2c/vs6624.c | 8 +-
drivers/media/pci/cx18/cx18-av-core.c | 2 +-
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 17 ++-
drivers/media/pci/saa7134/saa7134-empress.c | 5 +-
drivers/media/platform/atmel/atmel-isc-base.c | 19 ++-
drivers/media/platform/atmel/atmel-isi.c | 19 ++-
drivers/media/platform/cadence/cdns-csi2tx.c | 14 +-
.../media/platform/exynos4-is/fimc-capture.c | 22 +--
drivers/media/platform/exynos4-is/fimc-isp.c | 37 +++--
drivers/media/platform/exynos4-is/fimc-lite.c | 39 ++---
drivers/media/platform/exynos4-is/mipi-csis.c | 17 ++-
.../media/platform/marvell-ccic/mcam-core.c | 5 +-
drivers/media/platform/omap3isp/ispccdc.c | 85 ++++++-----
drivers/media/platform/omap3isp/ispccp2.c | 41 +++---
drivers/media/platform/omap3isp/ispcsi2.c | 41 +++---
drivers/media/platform/omap3isp/isppreview.c | 69 +++++----
drivers/media/platform/omap3isp/ispresizer.c | 70 +++++----
drivers/media/platform/pxa_camera.c | 5 +-
.../media/platform/qcom/camss/camss-csid.c | 35 ++---
.../media/platform/qcom/camss/camss-csiphy.c | 40 ++---
.../media/platform/qcom/camss/camss-ispif.c | 36 ++---
drivers/media/platform/qcom/camss/camss-vfe.c | 84 +++++------
drivers/media/platform/rcar-vin/rcar-csi2.c | 8 +-
drivers/media/platform/rcar-vin/rcar-v4l2.c | 10 +-
drivers/media/platform/renesas-ceu.c | 7 +-
.../platform/rockchip/rkisp1/rkisp1-isp.c | 112 ++++++++------
.../platform/rockchip/rkisp1/rkisp1-resizer.c | 95 +++++++-----
.../media/platform/s3c-camif/camif-capture.c | 18 +--
drivers/media/platform/stm32/stm32-dcmi.c | 14 +-
.../platform/sunxi/sun4i-csi/sun4i_v4l2.c | 16 +-
drivers/media/platform/ti-vpe/cal-camerarx.c | 35 +++--
drivers/media/platform/via-camera.c | 5 +-
drivers/media/platform/video-mux.c | 22 +--
drivers/media/platform/vsp1/vsp1_brx.c | 34 +++--
drivers/media/platform/vsp1/vsp1_clu.c | 13 +-
drivers/media/platform/vsp1/vsp1_entity.c | 53 +++----
drivers/media/platform/vsp1/vsp1_entity.h | 20 +--
drivers/media/platform/vsp1/vsp1_histo.c | 51 ++++---
drivers/media/platform/vsp1/vsp1_hsit.c | 14 +-
drivers/media/platform/vsp1/vsp1_lif.c | 13 +-
drivers/media/platform/vsp1/vsp1_lut.c | 13 +-
drivers/media/platform/vsp1/vsp1_rwpf.c | 32 ++--
drivers/media/platform/vsp1/vsp1_rwpf.h | 2 +-
drivers/media/platform/vsp1/vsp1_sru.c | 22 +--
drivers/media/platform/vsp1/vsp1_uds.c | 22 +--
drivers/media/platform/vsp1/vsp1_uif.c | 27 ++--
.../media/platform/xilinx/xilinx-csi2rxss.c | 20 +--
drivers/media/platform/xilinx/xilinx-tpg.c | 25 ++--
drivers/media/platform/xilinx/xilinx-vip.c | 8 +-
drivers/media/platform/xilinx/xilinx-vip.h | 4 +-
.../media/test-drivers/vimc/vimc-debayer.c | 20 +--
drivers/media/test-drivers/vimc/vimc-scaler.c | 36 ++---
drivers/media/test-drivers/vimc/vimc-sensor.c | 16 +-
drivers/media/usb/go7007/s2250-board.c | 2 +-
drivers/media/v4l2-core/v4l2-subdev.c | 139 ++++++++++--------
.../media/atomisp/i2c/atomisp-gc0310.c | 10 +-
.../media/atomisp/i2c/atomisp-gc2235.c | 10 +-
.../media/atomisp/i2c/atomisp-mt9m114.c | 12 +-
.../media/atomisp/i2c/atomisp-ov2680.c | 10 +-
.../media/atomisp/i2c/atomisp-ov2722.c | 10 +-
.../media/atomisp/i2c/ov5693/atomisp-ov5693.c | 10 +-
.../staging/media/atomisp/pci/atomisp_cmd.c | 33 +++--
.../staging/media/atomisp/pci/atomisp_csi2.c | 28 ++--
.../staging/media/atomisp/pci/atomisp_csi2.h | 2 +-
.../staging/media/atomisp/pci/atomisp_file.c | 14 +-
.../staging/media/atomisp/pci/atomisp_fops.c | 6 +-
.../media/atomisp/pci/atomisp_subdev.c | 64 ++++----
.../media/atomisp/pci/atomisp_subdev.h | 9 +-
.../staging/media/atomisp/pci/atomisp_tpg.c | 12 +-
drivers/staging/media/imx/imx-ic-prp.c | 19 +--
drivers/staging/media/imx/imx-ic-prpencvf.c | 31 ++--
drivers/staging/media/imx/imx-media-csi.c | 82 ++++++-----
drivers/staging/media/imx/imx-media-utils.c | 4 +-
drivers/staging/media/imx/imx-media-vdic.c | 24 +--
drivers/staging/media/imx/imx-media.h | 2 +-
drivers/staging/media/imx/imx6-mipi-csi2.c | 12 +-
drivers/staging/media/imx/imx7-media-csi.c | 33 +++--
drivers/staging/media/imx/imx7-mipi-csis.c | 34 +++--
drivers/staging/media/ipu3/ipu3-v4l2.c | 26 ++--
drivers/staging/media/omap4iss/iss_csi2.c | 37 ++---
drivers/staging/media/omap4iss/iss_ipipe.c | 37 ++---
drivers/staging/media/omap4iss/iss_ipipeif.c | 47 +++---
drivers/staging/media/omap4iss/iss_resizer.c | 39 ++---
drivers/staging/media/tegra-video/csi.c | 10 +-
drivers/staging/media/tegra-video/vi.c | 24 +--
include/media/v4l2-subdev.h | 74 ++++++----
166 files changed, 2134 insertions(+), 1777 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] media: v4l2-subdev: add subdev-wide state struct
[not found] ` <20210609100228.278798-2-tomi.valkeinen@ideasonboard.com>
@ 2021-06-10 8:00 ` Hans Verkuil
2021-06-10 8:24 ` Tomi Valkeinen
2021-06-10 11:11 ` Laurent Pinchart
0 siblings, 2 replies; 14+ messages in thread
From: Hans Verkuil @ 2021-06-10 8:00 UTC (permalink / raw)
To: Tomi Valkeinen, linux-media, Mauro Carvalho Chehab,
Laurent Pinchart, Sakari Ailus
Hi Tomi,
A few small comments:
On 09/06/2021 12:02, Tomi Valkeinen wrote:
> We have 'struct v4l2_subdev_pad_config' which contains configuration for
> a single pad used for the TRY functionality, and an array of those
> structs is passed to various v4l2_subdev_pad_ops.
>
> I was working on subdev internal routing between pads, and realized that
> there's no way to add TRY functionality for routes, which is not pad
> specific configuration. Adding a separate struct for try-route config
> wouldn't work either, as e.g. set-fmt needs to know the try-route
> configuration to propagate the settings.
>
> This patch adds a new struct, 'struct v4l2_subdev_state' (which at the
> moment only contains the v4l2_subdev_pad_config array) and the new
> struct is used in most of the places where v4l2_subdev_pad_config was
> used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config
> are changed to instead take v4l2_subdev_state.
>
> The changes to drivers/media/v4l2-core/v4l2-subdev.c and
> include/media/v4l2-subdev.h were written by hand, and all the driver
> changes were done with the semantic patch below. The spatch needs to be
> applied to a select list of directories. I used the following shell
> commands to apply the spatch:
<snip>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 956dafab43d4..dacae53b68d5 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -26,19 +26,21 @@
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> {
> - if (sd->entity.num_pads) {
> - fh->pad = v4l2_subdev_alloc_pad_config(sd);
> - if (fh->pad == NULL)
> - return -ENOMEM;
> - }
> + struct v4l2_subdev_state *state;
> +
> + state = v4l2_subdev_alloc_state(sd);
> + if (IS_ERR(state))
> + return PTR_ERR(state);
> +
> + fh->state = state;
>
> return 0;
> }
>
> static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> {
> - v4l2_subdev_free_pad_config(fh->pad);
> - fh->pad = NULL;
> + v4l2_subdev_free_state(fh->state);
> + fh->state = NULL;
> }
>
> static int subdev_open(struct file *file)
> @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
> return 0;
> }
>
> -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg)
> +static int check_cfg(u32 which, struct v4l2_subdev_state *state)
This should be renamed to check_state or check_state_pads (see next comment).
> {
> - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
> + if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
Should this also check for !state->pads? At least in the current code
it really checks for the pad configuration, so I think it should.
If so, then this function should probably be renamed to check_state_pads.
> return -EINVAL;
>
> return 0;
> }
>
> static inline int check_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format)
> {
> if (!format)
> return -EINVAL;
>
> return check_which(format->which) ? : check_pad(sd, format->pad) ? :
> - check_cfg(format->which, cfg);
> + check_cfg(format->which, state);
> }
>
> static int call_get_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format)
> {
> - return check_format(sd, cfg, format) ? :
> - sd->ops->pad->get_fmt(sd, cfg, format);
> + return check_format(sd, state, format) ? :
> + sd->ops->pad->get_fmt(sd, state, format);
> }
>
> static int call_set_fmt(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format)
> {
> - return check_format(sd, cfg, format) ? :
> - sd->ops->pad->set_fmt(sd, cfg, format);
> + return check_format(sd, state, format) ? :
> + sd->ops->pad->set_fmt(sd, state, format);
> }
>
> static int call_enum_mbus_code(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> if (!code)
> return -EINVAL;
>
> return check_which(code->which) ? : check_pad(sd, code->pad) ? :
> - check_cfg(code->which, cfg) ? :
> - sd->ops->pad->enum_mbus_code(sd, cfg, code);
> + check_cfg(code->which, state) ? :
> + sd->ops->pad->enum_mbus_code(sd, state, code);
> }
>
> static int call_enum_frame_size(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> if (!fse)
> return -EINVAL;
>
> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
> - check_cfg(fse->which, cfg) ? :
> - sd->ops->pad->enum_frame_size(sd, cfg, fse);
> + check_cfg(fse->which, state) ? :
> + sd->ops->pad->enum_frame_size(sd, state, fse);
> }
>
> static inline int check_frame_interval(struct v4l2_subdev *sd,
> @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd,
> }
>
> static int call_enum_frame_interval(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_frame_interval_enum *fie)
> {
> if (!fie)
> return -EINVAL;
>
> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
> - check_cfg(fie->which, cfg) ? :
> - sd->ops->pad->enum_frame_interval(sd, cfg, fie);
> + check_cfg(fie->which, state) ? :
> + sd->ops->pad->enum_frame_interval(sd, state, fie);
> }
>
> static inline int check_selection(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel)
> {
> if (!sel)
> return -EINVAL;
>
> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
> - check_cfg(sel->which, cfg);
> + check_cfg(sel->which, state);
> }
>
> static int call_get_selection(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel)
> {
> - return check_selection(sd, cfg, sel) ? :
> - sd->ops->pad->get_selection(sd, cfg, sel);
> + return check_selection(sd, state, sel) ? :
> + sd->ops->pad->get_selection(sd, state, sel);
> }
>
> static int call_set_selection(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel)
> {
> - return check_selection(sd, cfg, sel) ? :
> - sd->ops->pad->set_selection(sd, cfg, sel);
> + return check_selection(sd, state, sel) ? :
> + sd->ops->pad->set_selection(sd, state, sel);
> }
>
> static inline int check_edid(struct v4l2_subdev *sd,
> @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
> + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format);
> }
>
> case VIDIOC_SUBDEV_S_FMT: {
> @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> memset(format->reserved, 0, sizeof(format->reserved));
> memset(format->format.reserved, 0, sizeof(format->format.reserved));
> - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format);
> }
>
> case VIDIOC_SUBDEV_G_CROP: {
> @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> sel.target = V4L2_SEL_TGT_CROP;
>
> rval = v4l2_subdev_call(
> - sd, pad, get_selection, subdev_fh->pad, &sel);
> + sd, pad, get_selection, subdev_fh->state, &sel);
>
> crop->rect = sel.r;
>
> @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> sel.r = crop->rect;
>
> rval = v4l2_subdev_call(
> - sd, pad, set_selection, subdev_fh->pad, &sel);
> + sd, pad, set_selection, subdev_fh->state, &sel);
>
> crop->rect = sel.r;
>
> @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct v4l2_subdev_mbus_code_enum *code = arg;
>
> memset(code->reserved, 0, sizeof(code->reserved));
> - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
> + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state,
> code);
> }
>
> @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct v4l2_subdev_frame_size_enum *fse = arg;
>
> memset(fse->reserved, 0, sizeof(fse->reserved));
> - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
> + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state,
> fse);
> }
>
> @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct v4l2_subdev_frame_interval_enum *fie = arg;
>
> memset(fie->reserved, 0, sizeof(fie->reserved));
> - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
> + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state,
> fie);
> }
>
> @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> - sd, pad, get_selection, subdev_fh->pad, sel);
> + sd, pad, get_selection, subdev_fh->state, sel);
> }
>
> case VIDIOC_SUBDEV_S_SELECTION: {
> @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> memset(sel->reserved, 0, sizeof(sel->reserved));
> return v4l2_subdev_call(
> - sd, pad, set_selection, subdev_fh->pad, sel);
> + sd, pad, set_selection, subdev_fh->state, sel);
> }
>
> case VIDIOC_G_EDID: {
> @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link)
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>
> -struct v4l2_subdev_pad_config *
> -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd)
> +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
> {
> - struct v4l2_subdev_pad_config *cfg;
> + struct v4l2_subdev_state *state;
> int ret;
>
> - if (!sd->entity.num_pads)
> - return NULL;
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return ERR_PTR(-ENOMEM);
>
> - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg),
> - GFP_KERNEL | __GFP_ZERO);
> - if (!cfg)
> - return NULL;
> -
> - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg);
> - if (ret < 0 && ret != -ENOIOCTLCMD) {
> - kvfree(cfg);
> - return NULL;
> + if (sd->entity.num_pads) {
> + state->pads = kvmalloc_array(sd->entity.num_pads,
> + sizeof(*state->pads),
> + GFP_KERNEL | __GFP_ZERO);
> + if (!state->pads) {
> + ret = -ENOMEM;
> + goto err;
> + }
> }
>
> - return cfg;
> + ret = v4l2_subdev_call(sd, pad, init_cfg, state);
The init_cfg callback should also be renamed to init_state, but this can
be done in a second patch.
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto err;
> +
> + return state;
> +
> +err:
> + if (state && state->pads)
> + kvfree(state->pads);
> +
> + kfree(state);
> +
> + return ERR_PTR(ret);
> }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config);
> +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
>
> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg)
> +void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
> {
> - kvfree(cfg);
> + kvfree(state->pads);
I'd change this to:
if (state)
kvfree(state->pads);
That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust.
> + kfree(state);
> }
> -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config);
> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
<snip>
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d0e9a5bdb08b..89115ba4c0f2 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config {
I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state
or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology.
This too can be done in a separate patch.
I like the 'state' term much better than 'config'.
Regards,
Hans
> struct v4l2_rect try_compose;
> };
>
> +/**
> + * struct v4l2_subdev_state - Used for storing subdev state information.
> + *
> + * @pads: &struct v4l2_subdev_pad_config array
> + *
> + * This structure only needs to be passed to the pad op if the 'which' field
> + * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For
> + * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
> + */
> +struct v4l2_subdev_state {
> + struct v4l2_subdev_pad_config *pads;
> +};
> +
> /**
> * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations
> *
> @@ -687,27 +700,27 @@ struct v4l2_subdev_pad_config {
> */
> struct v4l2_subdev_pad_ops {
> int (*init_cfg)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg);
> + struct v4l2_subdev_state *state);
> int (*enum_mbus_code)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_mbus_code_enum *code);
> int (*enum_frame_size)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_frame_size_enum *fse);
> int (*enum_frame_interval)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_frame_interval_enum *fie);
> int (*get_fmt)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format);
> int (*set_fmt)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_format *format);
> int (*get_selection)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel);
> int (*set_selection)(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel);
> int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
> int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
> @@ -918,14 +931,14 @@ struct v4l2_subdev {
> * struct v4l2_subdev_fh - Used for storing subdev information per file handle
> *
> * @vfh: pointer to &struct v4l2_fh
> - * @pad: pointer to &struct v4l2_subdev_pad_config
> + * @state: pointer to &struct v4l2_subdev_state
> * @owner: module pointer to the owner of this file handle
> */
> struct v4l2_subdev_fh {
> struct v4l2_fh vfh;
> struct module *owner;
> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> - struct v4l2_subdev_pad_config *pad;
> + struct v4l2_subdev_state *state;
> #endif
> };
>
> @@ -945,17 +958,17 @@ struct v4l2_subdev_fh {
> * &struct v4l2_subdev_pad_config->try_fmt
> *
> * @sd: pointer to &struct v4l2_subdev
> - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> - * @pad: index of the pad in the @cfg array.
> + * @state: pointer to &struct v4l2_subdev_state
> + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array
> */
> static inline struct v4l2_mbus_framefmt *
> v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> unsigned int pad)
> {
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> - return &cfg[pad].try_fmt;
> + return &state->pads[pad].try_fmt;
> }
>
> /**
> @@ -963,17 +976,17 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> * &struct v4l2_subdev_pad_config->try_crop
> *
> * @sd: pointer to &struct v4l2_subdev
> - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> - * @pad: index of the pad in the @cfg array.
> + * @state: pointer to &struct v4l2_subdev_state.
> + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array.
> */
> static inline struct v4l2_rect *
> v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> unsigned int pad)
> {
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> - return &cfg[pad].try_crop;
> + return &state->pads[pad].try_crop;
> }
>
> /**
> @@ -981,17 +994,17 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> * &struct v4l2_subdev_pad_config->try_compose
> *
> * @sd: pointer to &struct v4l2_subdev
> - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> - * @pad: index of the pad in the @cfg array.
> + * @state: pointer to &struct v4l2_subdev_state.
> + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array.
> */
> static inline struct v4l2_rect *
> v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> - struct v4l2_subdev_pad_config *cfg,
> + struct v4l2_subdev_state *state,
> unsigned int pad)
> {
> if (WARN_ON(pad >= sd->entity.num_pads))
> pad = 0;
> - return &cfg[pad].try_compose;
> + return &state->pads[pad].try_compose;
> }
>
> #endif
> @@ -1093,20 +1106,21 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> int v4l2_subdev_link_validate(struct media_link *link);
>
> /**
> - * v4l2_subdev_alloc_pad_config - Allocates memory for pad config
> + * v4l2_subdev_alloc_state - allocate v4l2_subdev_state
> *
> - * @sd: pointer to struct v4l2_subdev
> + * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
> + *
> + * Must call v4l2_subdev_free_state() when state is no longer needed.
> */
> -struct
> -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd);
> +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd);
>
> /**
> - * v4l2_subdev_free_pad_config - Frees memory allocated by
> - * v4l2_subdev_alloc_pad_config().
> + * v4l2_subdev_free_state - free a v4l2_subdev_state
> *
> - * @cfg: pointer to &struct v4l2_subdev_pad_config
> + * @state: v4l2_subdev_state to be freed.
> */
> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg);
> +void v4l2_subdev_free_state(struct v4l2_subdev_state *state);
> +
> #endif /* CONFIG_MEDIA_CONTROLLER */
>
> /**
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] media: v4l2-subdev: add subdev-wide state struct
2021-06-10 8:00 ` [PATCH v3 1/1] " Hans Verkuil
@ 2021-06-10 8:24 ` Tomi Valkeinen
2021-06-10 11:11 ` Laurent Pinchart
1 sibling, 0 replies; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 8:24 UTC (permalink / raw)
To: Hans Verkuil, linux-media, Mauro Carvalho Chehab,
Laurent Pinchart, Sakari Ailus
On 10/06/2021 11:00, Hans Verkuil wrote:
> Hi Tomi,
>
> A few small comments:
>
> On 09/06/2021 12:02, Tomi Valkeinen wrote:
>> We have 'struct v4l2_subdev_pad_config' which contains configuration for
>> a single pad used for the TRY functionality, and an array of those
>> structs is passed to various v4l2_subdev_pad_ops.
>>
>> I was working on subdev internal routing between pads, and realized that
>> there's no way to add TRY functionality for routes, which is not pad
>> specific configuration. Adding a separate struct for try-route config
>> wouldn't work either, as e.g. set-fmt needs to know the try-route
>> configuration to propagate the settings.
>>
>> This patch adds a new struct, 'struct v4l2_subdev_state' (which at the
>> moment only contains the v4l2_subdev_pad_config array) and the new
>> struct is used in most of the places where v4l2_subdev_pad_config was
>> used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config
>> are changed to instead take v4l2_subdev_state.
>>
>> The changes to drivers/media/v4l2-core/v4l2-subdev.c and
>> include/media/v4l2-subdev.h were written by hand, and all the driver
>> changes were done with the semantic patch below. The spatch needs to be
>> applied to a select list of directories. I used the following shell
>> commands to apply the spatch:
>
> <snip>
>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 956dafab43d4..dacae53b68d5 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -26,19 +26,21 @@
>> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
>> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
>> {
>> - if (sd->entity.num_pads) {
>> - fh->pad = v4l2_subdev_alloc_pad_config(sd);
>> - if (fh->pad == NULL)
>> - return -ENOMEM;
>> - }
>> + struct v4l2_subdev_state *state;
>> +
>> + state = v4l2_subdev_alloc_state(sd);
>> + if (IS_ERR(state))
>> + return PTR_ERR(state);
>> +
>> + fh->state = state;
>>
>> return 0;
>> }
>>
>> static void subdev_fh_free(struct v4l2_subdev_fh *fh)
>> {
>> - v4l2_subdev_free_pad_config(fh->pad);
>> - fh->pad = NULL;
>> + v4l2_subdev_free_state(fh->state);
>> + fh->state = NULL;
>> }
>>
>> static int subdev_open(struct file *file)
>> @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
>> return 0;
>> }
>>
>> -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg)
>> +static int check_cfg(u32 which, struct v4l2_subdev_state *state)
>
> This should be renamed to check_state or check_state_pads (see next comment).
Ok.
>> {
>> - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
>> + if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
>
> Should this also check for !state->pads? At least in the current code
> it really checks for the pad configuration, so I think it should.
>
> If so, then this function should probably be renamed to check_state_pads.
Hmm, yes, I think you're right. We can have a case where we have state,
but state->pads is NULL.
>> return -EINVAL;
>>
>> return 0;
>> }
>>
>> static inline int check_format(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_format *format)
>> {
>> if (!format)
>> return -EINVAL;
>>
>> return check_which(format->which) ? : check_pad(sd, format->pad) ? :
>> - check_cfg(format->which, cfg);
>> + check_cfg(format->which, state);
>> }
>>
>> static int call_get_fmt(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_format *format)
>> {
>> - return check_format(sd, cfg, format) ? :
>> - sd->ops->pad->get_fmt(sd, cfg, format);
>> + return check_format(sd, state, format) ? :
>> + sd->ops->pad->get_fmt(sd, state, format);
>> }
>>
>> static int call_set_fmt(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_format *format)
>> {
>> - return check_format(sd, cfg, format) ? :
>> - sd->ops->pad->set_fmt(sd, cfg, format);
>> + return check_format(sd, state, format) ? :
>> + sd->ops->pad->set_fmt(sd, state, format);
>> }
>>
>> static int call_enum_mbus_code(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_mbus_code_enum *code)
>> {
>> if (!code)
>> return -EINVAL;
>>
>> return check_which(code->which) ? : check_pad(sd, code->pad) ? :
>> - check_cfg(code->which, cfg) ? :
>> - sd->ops->pad->enum_mbus_code(sd, cfg, code);
>> + check_cfg(code->which, state) ? :
>> + sd->ops->pad->enum_mbus_code(sd, state, code);
>> }
>>
>> static int call_enum_frame_size(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_frame_size_enum *fse)
>> {
>> if (!fse)
>> return -EINVAL;
>>
>> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
>> - check_cfg(fse->which, cfg) ? :
>> - sd->ops->pad->enum_frame_size(sd, cfg, fse);
>> + check_cfg(fse->which, state) ? :
>> + sd->ops->pad->enum_frame_size(sd, state, fse);
>> }
>>
>> static inline int check_frame_interval(struct v4l2_subdev *sd,
>> @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd,
>> }
>>
>> static int call_enum_frame_interval(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_frame_interval_enum *fie)
>> {
>> if (!fie)
>> return -EINVAL;
>>
>> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
>> - check_cfg(fie->which, cfg) ? :
>> - sd->ops->pad->enum_frame_interval(sd, cfg, fie);
>> + check_cfg(fie->which, state) ? :
>> + sd->ops->pad->enum_frame_interval(sd, state, fie);
>> }
>>
>> static inline int check_selection(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_selection *sel)
>> {
>> if (!sel)
>> return -EINVAL;
>>
>> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
>> - check_cfg(sel->which, cfg);
>> + check_cfg(sel->which, state);
>> }
>>
>> static int call_get_selection(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_selection *sel)
>> {
>> - return check_selection(sd, cfg, sel) ? :
>> - sd->ops->pad->get_selection(sd, cfg, sel);
>> + return check_selection(sd, state, sel) ? :
>> + sd->ops->pad->get_selection(sd, state, sel);
>> }
>>
>> static int call_set_selection(struct v4l2_subdev *sd,
>> - struct v4l2_subdev_pad_config *cfg,
>> + struct v4l2_subdev_state *state,
>> struct v4l2_subdev_selection *sel)
>> {
>> - return check_selection(sd, cfg, sel) ? :
>> - sd->ops->pad->set_selection(sd, cfg, sel);
>> + return check_selection(sd, state, sel) ? :
>> + sd->ops->pad->set_selection(sd, state, sel);
>> }
>>
>> static inline int check_edid(struct v4l2_subdev *sd,
>> @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>
>> memset(format->reserved, 0, sizeof(format->reserved));
>> memset(format->format.reserved, 0, sizeof(format->format.reserved));
>> - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
>> + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format);
>> }
>>
>> case VIDIOC_SUBDEV_S_FMT: {
>> @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>
>> memset(format->reserved, 0, sizeof(format->reserved));
>> memset(format->format.reserved, 0, sizeof(format->format.reserved));
>> - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
>> + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format);
>> }
>>
>> case VIDIOC_SUBDEV_G_CROP: {
>> @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> sel.target = V4L2_SEL_TGT_CROP;
>>
>> rval = v4l2_subdev_call(
>> - sd, pad, get_selection, subdev_fh->pad, &sel);
>> + sd, pad, get_selection, subdev_fh->state, &sel);
>>
>> crop->rect = sel.r;
>>
>> @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> sel.r = crop->rect;
>>
>> rval = v4l2_subdev_call(
>> - sd, pad, set_selection, subdev_fh->pad, &sel);
>> + sd, pad, set_selection, subdev_fh->state, &sel);
>>
>> crop->rect = sel.r;
>>
>> @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> struct v4l2_subdev_mbus_code_enum *code = arg;
>>
>> memset(code->reserved, 0, sizeof(code->reserved));
>> - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
>> + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state,
>> code);
>> }
>>
>> @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> struct v4l2_subdev_frame_size_enum *fse = arg;
>>
>> memset(fse->reserved, 0, sizeof(fse->reserved));
>> - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
>> + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state,
>> fse);
>> }
>>
>> @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>> struct v4l2_subdev_frame_interval_enum *fie = arg;
>>
>> memset(fie->reserved, 0, sizeof(fie->reserved));
>> - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
>> + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state,
>> fie);
>> }
>>
>> @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>
>> memset(sel->reserved, 0, sizeof(sel->reserved));
>> return v4l2_subdev_call(
>> - sd, pad, get_selection, subdev_fh->pad, sel);
>> + sd, pad, get_selection, subdev_fh->state, sel);
>> }
>>
>> case VIDIOC_SUBDEV_S_SELECTION: {
>> @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>
>> memset(sel->reserved, 0, sizeof(sel->reserved));
>> return v4l2_subdev_call(
>> - sd, pad, set_selection, subdev_fh->pad, sel);
>> + sd, pad, set_selection, subdev_fh->state, sel);
>> }
>>
>> case VIDIOC_G_EDID: {
>> @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link)
>> }
>> EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
>>
>> -struct v4l2_subdev_pad_config *
>> -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd)
>> +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
>> {
>> - struct v4l2_subdev_pad_config *cfg;
>> + struct v4l2_subdev_state *state;
>> int ret;
>>
>> - if (!sd->entity.num_pads)
>> - return NULL;
>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return ERR_PTR(-ENOMEM);
>>
>> - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg),
>> - GFP_KERNEL | __GFP_ZERO);
>> - if (!cfg)
>> - return NULL;
>> -
>> - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg);
>> - if (ret < 0 && ret != -ENOIOCTLCMD) {
>> - kvfree(cfg);
>> - return NULL;
>> + if (sd->entity.num_pads) {
>> + state->pads = kvmalloc_array(sd->entity.num_pads,
>> + sizeof(*state->pads),
>> + GFP_KERNEL | __GFP_ZERO);
>> + if (!state->pads) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> }
>>
>> - return cfg;
>> + ret = v4l2_subdev_call(sd, pad, init_cfg, state);
>
> The init_cfg callback should also be renamed to init_state, but this can
> be done in a second patch.
Yes, there are more renames that can be done, but I didn't want to
explode the patch to even bigger size =).
>> + if (ret < 0 && ret != -ENOIOCTLCMD)
>> + goto err;
>> +
>> + return state;
>> +
>> +err:
>> + if (state && state->pads)
>> + kvfree(state->pads);
>> +
>> + kfree(state);
>> +
>> + return ERR_PTR(ret);
>> }
>> -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config);
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
>>
>> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg)
>> +void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
>> {
>> - kvfree(cfg);
>> + kvfree(state->pads);
>
> I'd change this to:
>
> if (state)
> kvfree(state->pads);
>
> That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust.
Yes, good point.
>> + kfree(state);
>> }
>> -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config);
>> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
>> +
>> #endif /* CONFIG_MEDIA_CONTROLLER */
>>
>> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>
> <snip>
>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index d0e9a5bdb08b..89115ba4c0f2 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config {
>
> I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state
> or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology.
>
> This too can be done in a separate patch.
>
> I like the 'state' term much better than 'config'.
Yes, I agree here too, and was in my list of "later".
Thanks!
Tomi
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3 2/5] media: omap3isp: fix indentation
2021-06-09 10:02 [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
[not found] ` <20210609100228.278798-2-tomi.valkeinen@ideasonboard.com>
@ 2021-06-10 9:49 ` Tomi Valkeinen
2021-06-10 9:49 ` [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter Tomi Valkeinen
` (3 more replies)
2021-06-10 9:51 ` [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
2 siblings, 4 replies; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 9:49 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: Tomi Valkeinen
Fix a few indentation warnings from checkpatch.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/platform/omap3isp/ispccp2.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 366da6fb8b4f..acb58b6ddba1 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -619,8 +619,8 @@ static const unsigned int ccp2_fmts[] = {
*/
static struct v4l2_mbus_framefmt *
__ccp2_get_format(struct isp_ccp2_device *ccp2,
- struct v4l2_subdev_state *sd_state,
- unsigned int pad, enum v4l2_subdev_format_whence which)
+ struct v4l2_subdev_state *sd_state,
+ unsigned int pad, enum v4l2_subdev_format_whence which)
{
if (which == V4L2_SUBDEV_FORMAT_TRY)
return v4l2_subdev_get_try_format(&ccp2->subdev, sd_state,
@@ -708,7 +708,7 @@ static int ccp2_enum_mbus_code(struct v4l2_subdev *sd,
return -EINVAL;
format = __ccp2_get_format(ccp2, sd_state, CCP2_PAD_SINK,
- code->which);
+ code->which);
code->code = format->code;
}
@@ -753,8 +753,8 @@ static int ccp2_enum_frame_size(struct v4l2_subdev *sd,
* return -EINVAL or zero on success
*/
static int ccp2_get_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *fmt)
{
struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd);
struct v4l2_mbus_framefmt *format;
@@ -775,8 +775,8 @@ static int ccp2_get_format(struct v4l2_subdev *sd,
* returns zero
*/
static int ccp2_set_format(struct v4l2_subdev *sd,
- struct v4l2_subdev_state *sd_state,
- struct v4l2_subdev_format *fmt)
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_format *fmt)
{
struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd);
struct v4l2_mbus_framefmt *format;
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
@ 2021-06-10 9:49 ` Tomi Valkeinen
2021-06-10 11:25 ` Laurent Pinchart
2021-06-10 9:49 ` [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state Tomi Valkeinen
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 9:49 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: Tomi Valkeinen
"media: v4l2-subdev: add subdev-wide state struct" introduced some
kernel doc errors due to cfg -> sd_state rename. Fix these.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/i2c/imx274.c | 6 +++---
drivers/media/i2c/imx334.c | 10 +++++-----
drivers/media/i2c/mt9m032.c | 4 ++--
drivers/media/i2c/tvp514x.c | 6 +++---
drivers/media/platform/vsp1/vsp1_entity.c | 6 +++---
drivers/media/platform/xilinx/xilinx-csi2rxss.c | 6 +++---
drivers/media/platform/xilinx/xilinx-vip.c | 4 ++--
7 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index e0c413cc94d7..0dce92872176 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -996,7 +996,7 @@ static int imx274_binning_goodness(struct stimx274 *imx274,
* Must be called with imx274->lock locked.
*
* @imx274: The device object
- * @cfg: The pad config we are editing for TRY requests
+ * @sd_state: The subdev state we are editing for TRY requests
* @which: V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller
* @width: Input-output parameter: set to the desired width before
* the call, contains the chosen value after returning successfully
@@ -1061,7 +1061,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
/**
* imx274_get_fmt - Get the pad format
* @sd: Pointer to V4L2 Sub device structure
- * @cfg: Pointer to sub device pad information structure
+ * @sd_state: Pointer to sub device state structure
* @fmt: Pointer to pad level media bus format
*
* This function is used to get the pad format information.
@@ -1083,7 +1083,7 @@ static int imx274_get_fmt(struct v4l2_subdev *sd,
/**
* imx274_set_fmt - This is used to set the pad format
* @sd: Pointer to V4L2 Sub device structure
- * @cfg: Pointer to sub device pad information structure
+ * @sd_state: Pointer to sub device state information structure
* @format: Pointer to pad level media bus format
*
* This function is used to set the pad format.
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 751ce1f8f817..062125501788 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -497,7 +497,7 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
/**
* imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
* @sd: pointer to imx334 V4L2 sub-device structure
- * @cfg: V4L2 sub-device pad configuration
+ * @sd_state: V4L2 sub-device state
* @code: V4L2 sub-device code enumeration need to be filled
*
* Return: 0 if successful, error code otherwise.
@@ -517,7 +517,7 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
/**
* imx334_enum_frame_size() - Enumerate V4L2 sub-device frame sizes
* @sd: pointer to imx334 V4L2 sub-device structure
- * @cfg: V4L2 sub-device pad configuration
+ * @sd_state: V4L2 sub-device state
* @fsize: V4L2 sub-device size enumeration need to be filled
*
* Return: 0 if successful, error code otherwise.
@@ -564,7 +564,7 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
/**
* imx334_get_pad_format() - Get subdevice pad format
* @sd: pointer to imx334 V4L2 sub-device structure
- * @cfg: V4L2 sub-device pad configuration
+ * @sd_state: V4L2 sub-device state
* @fmt: V4L2 sub-device format need to be set
*
* Return: 0 if successful, error code otherwise.
@@ -594,7 +594,7 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
/**
* imx334_set_pad_format() - Set subdevice pad format
* @sd: pointer to imx334 V4L2 sub-device structure
- * @cfg: V4L2 sub-device pad configuration
+ * @sd_state: V4L2 sub-device state
* @fmt: V4L2 sub-device format need to be set
*
* Return: 0 if successful, error code otherwise.
@@ -631,7 +631,7 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
/**
* imx334_init_pad_cfg() - Initialize sub-device pad configuration
* @sd: pointer to imx334 V4L2 sub-device structure
- * @cfg: V4L2 sub-device pad configuration
+ * @sd_state: V4L2 sub-device state
*
* Return: 0 if successful, error code otherwise.
*/
diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
index 8a0741058c98..ba0c0ea91c95 100644
--- a/drivers/media/i2c/mt9m032.c
+++ b/drivers/media/i2c/mt9m032.c
@@ -332,7 +332,7 @@ static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
/**
* __mt9m032_get_pad_crop() - get crop rect
* @sensor: pointer to the sensor struct
- * @cfg: v4l2_subdev_pad_config for getting the try crop rect from
+ * @sd_state: v4l2_subdev_state for getting the try crop rect from
* @which: select try or active crop rect
*
* Returns a pointer the current active or fh relative try crop rect
@@ -355,7 +355,7 @@ __mt9m032_get_pad_crop(struct mt9m032 *sensor,
/**
* __mt9m032_get_pad_format() - get format
* @sensor: pointer to the sensor struct
- * @cfg: v4l2_subdev_pad_config for getting the try format from
+ * @sd_state: v4l2_subdev_state for getting the try format from
* @which: select try or active format
*
* Returns a pointer the current active or fh relative try format
diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
index 5f159588aaeb..cee60f945036 100644
--- a/drivers/media/i2c/tvp514x.c
+++ b/drivers/media/i2c/tvp514x.c
@@ -853,7 +853,7 @@ static const struct v4l2_ctrl_ops tvp514x_ctrl_ops = {
/**
* tvp514x_enum_mbus_code() - V4L2 decoder interface handler for enum_mbus_code
* @sd: pointer to standard V4L2 sub-device structure
- * @cfg: pad configuration
+ * @sd_state: subdev state
* @code: pointer to v4l2_subdev_mbus_code_enum structure
*
* Enumertaes mbus codes supported
@@ -880,7 +880,7 @@ static int tvp514x_enum_mbus_code(struct v4l2_subdev *sd,
/**
* tvp514x_get_pad_format() - V4L2 decoder interface handler for get pad format
* @sd: pointer to standard V4L2 sub-device structure
- * @cfg: pad configuration
+ * @sd_state: subdev state
* @format: pointer to v4l2_subdev_format structure
*
* Retrieves pad format which is active or tried based on requirement
@@ -912,7 +912,7 @@ static int tvp514x_get_pad_format(struct v4l2_subdev *sd,
/**
* tvp514x_set_pad_format() - V4L2 decoder interface handler for set pad format
* @sd: pointer to standard V4L2 sub-device structure
- * @cfg: pad configuration
+ * @sd_state: subdev state
* @fmt: pointer to v4l2_subdev_format structure
*
* Set pad format for the output pad
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index 7d92262f3378..6ef874d29eb7 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -103,7 +103,7 @@ void vsp1_entity_configure_partition(struct vsp1_entity *entity,
/**
* vsp1_entity_get_pad_config - Get the pad configuration for an entity
* @entity: the entity
- * @cfg: the TRY pad configuration
+ * @sd_state: the TRY pad configuration
* @which: configuration selector (ACTIVE or TRY)
*
* When called with which set to V4L2_SUBDEV_FORMAT_ACTIVE the caller must hold
@@ -131,7 +131,7 @@ vsp1_entity_get_pad_config(struct vsp1_entity *entity,
/**
* vsp1_entity_get_pad_format - Get a pad format from storage for an entity
* @entity: the entity
- * @cfg: the configuration storage
+ * @sd_state: the configuration storage
* @pad: the pad number
*
* Return the format stored in the given configuration for an entity's pad. The
@@ -148,7 +148,7 @@ vsp1_entity_get_pad_format(struct vsp1_entity *entity,
/**
* vsp1_entity_get_pad_selection - Get a pad selection from storage for entity
* @entity: the entity
- * @cfg: the configuration storage
+ * @sd_state: the configuration storage
* @pad: the pad number
* @target: the selection target
*
diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
index 2773145a2226..b1baf9d7b6ec 100644
--- a/drivers/media/platform/xilinx/xilinx-csi2rxss.c
+++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
@@ -698,7 +698,7 @@ __xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
/**
* xcsi2rxss_init_cfg - Initialise the pad format config to default
* @sd: Pointer to V4L2 Sub device structure
- * @cfg: Pointer to sub device pad information structure
+ * @sd_state: Pointer to sub device state structure
*
* This function is used to initialize the pad format with the default
* values.
@@ -725,7 +725,7 @@ static int xcsi2rxss_init_cfg(struct v4l2_subdev *sd,
/**
* xcsi2rxss_get_format - Get the pad format
* @sd: Pointer to V4L2 Sub device structure
- * @cfg: Pointer to sub device pad information structure
+ * @sd_state: Pointer to sub device state structure
* @fmt: Pointer to pad level media bus format
*
* This function is used to get the pad format information.
@@ -750,7 +750,7 @@ static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
/**
* xcsi2rxss_set_format - This is used to set the pad format
* @sd: Pointer to V4L2 Sub device structure
- * @cfg: Pointer to sub device pad information structure
+ * @sd_state: Pointer to sub device state structure
* @fmt: Pointer to pad level media bus format
*
* This function is used to set the pad format. Since the pad format is fixed
diff --git a/drivers/media/platform/xilinx/xilinx-vip.c b/drivers/media/platform/xilinx/xilinx-vip.c
index b989fee8351d..07d16a007d5f 100644
--- a/drivers/media/platform/xilinx/xilinx-vip.c
+++ b/drivers/media/platform/xilinx/xilinx-vip.c
@@ -234,7 +234,7 @@ EXPORT_SYMBOL_GPL(xvip_cleanup_resources);
/**
* xvip_enum_mbus_code - Enumerate the media format code
* @subdev: V4L2 subdevice
- * @cfg: V4L2 subdev pad configuration
+ * @sd_state: V4L2 subdev state
* @code: returning media bus code
*
* Enumerate the media bus code of the subdevice. Return the corresponding
@@ -271,7 +271,7 @@ EXPORT_SYMBOL_GPL(xvip_enum_mbus_code);
/**
* xvip_enum_frame_size - Enumerate the media bus frame size
* @subdev: V4L2 subdevice
- * @cfg: V4L2 subdev pad configuration
+ * @sd_state: V4L2 subdev state
* @fse: returning media bus frame size
*
* This function is a drop-in implementation of the subdev enum_frame_size pad
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
2021-06-10 9:49 ` [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter Tomi Valkeinen
@ 2021-06-10 9:49 ` Tomi Valkeinen
2021-06-10 11:26 ` Laurent Pinchart
2021-06-10 9:49 ` [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg() Tomi Valkeinen
2021-06-10 11:23 ` [PATCH v3 2/5] media: omap3isp: fix indentation Laurent Pinchart
3 siblings, 1 reply; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 9:49 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: Tomi Valkeinen
Make v4l2_subdev_free_state() accept a NULL state.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index dacae53b68d5..25c80d6de23b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -931,7 +931,8 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
{
- kvfree(state->pads);
+ if (state)
+ kvfree(state->pads);
kfree(state);
}
EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg()
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
2021-06-10 9:49 ` [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter Tomi Valkeinen
2021-06-10 9:49 ` [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state Tomi Valkeinen
@ 2021-06-10 9:49 ` Tomi Valkeinen
2021-06-10 11:26 ` Laurent Pinchart
2021-06-10 11:23 ` [PATCH v3 2/5] media: omap3isp: fix indentation Laurent Pinchart
3 siblings, 1 reply; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 9:49 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Cc: Tomi Valkeinen
Rename check_cfg() to check_state_pads() as it is now checking the pads
of the state.
Also fix the check so that it verifies that both state and state->pads
exist.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 25c80d6de23b..daf7ffdd8882 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -148,9 +148,9 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
return 0;
}
-static int check_cfg(u32 which, struct v4l2_subdev_state *state)
+static int check_state_pads(u32 which, struct v4l2_subdev_state *state)
{
- if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
+ if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads))
return -EINVAL;
return 0;
@@ -164,7 +164,7 @@ static inline int check_format(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(format->which) ? : check_pad(sd, format->pad) ? :
- check_cfg(format->which, state);
+ check_state_pads(format->which, state);
}
static int call_get_fmt(struct v4l2_subdev *sd,
@@ -191,7 +191,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(code->which) ? : check_pad(sd, code->pad) ? :
- check_cfg(code->which, state) ? :
+ check_state_pads(code->which, state) ? :
sd->ops->pad->enum_mbus_code(sd, state, code);
}
@@ -203,7 +203,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
- check_cfg(fse->which, state) ? :
+ check_state_pads(fse->which, state) ? :
sd->ops->pad->enum_frame_size(sd, state, fse);
}
@@ -238,7 +238,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
- check_cfg(fie->which, state) ? :
+ check_state_pads(fie->which, state) ? :
sd->ops->pad->enum_frame_interval(sd, state, fie);
}
@@ -250,7 +250,7 @@ static inline int check_selection(struct v4l2_subdev *sd,
return -EINVAL;
return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
- check_cfg(sel->which, state);
+ check_state_pads(sel->which, state);
}
static int call_get_selection(struct v4l2_subdev *sd,
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct
2021-06-09 10:02 [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
[not found] ` <20210609100228.278798-2-tomi.valkeinen@ideasonboard.com>
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
@ 2021-06-10 9:51 ` Tomi Valkeinen
2021-06-10 9:54 ` Hans Verkuil
2 siblings, 1 reply; 14+ messages in thread
From: Tomi Valkeinen @ 2021-06-10 9:51 UTC (permalink / raw)
To: linux-media, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus
Hi all,
On 09/06/2021 13:02, Tomi Valkeinen wrote:
> Hi,
>
> v3 of the patch. I rebased on top of latest linux-media, and applied the
> semantic patch. I've addressed Laurent's comment (fix kfree, kernel doc
> fixes, return cleanup).
>
> v2 can be found from:
>
> https://lore.kernel.org/linux-media/20210527094244.617473-1-tomi.valkeinen@ideasonboard.com/
>
I posted a few fixes to this as a continuation to the series (patches
2/5 - 5/5) in an ad-hoc manner, to help reviewing and avoid sending the
half-a-megabyte patch.
All those can be squashed to the original patch, or I can send a proper
v4 after getting acks for the changes.
Tomi
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct
2021-06-10 9:51 ` [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
@ 2021-06-10 9:54 ` Hans Verkuil
0 siblings, 0 replies; 14+ messages in thread
From: Hans Verkuil @ 2021-06-10 9:54 UTC (permalink / raw)
To: Tomi Valkeinen, linux-media, Mauro Carvalho Chehab,
Laurent Pinchart, Sakari Ailus
On 10/06/2021 11:51, Tomi Valkeinen wrote:
> Hi all,
>
> On 09/06/2021 13:02, Tomi Valkeinen wrote:
>> Hi,
>>
>> v3 of the patch. I rebased on top of latest linux-media, and applied the
>> semantic patch. I've addressed Laurent's comment (fix kfree, kernel doc
>> fixes, return cleanup).
>>
>> v2 can be found from:
>>
>> https://lore.kernel.org/linux-media/20210527094244.617473-1-tomi.valkeinen@ideasonboard.com/
>>
>
> I posted a few fixes to this as a continuation to the series (patches
> 2/5 - 5/5) in an ad-hoc manner, to help reviewing and avoid sending the
> half-a-megabyte patch.
>
> All those can be squashed to the original patch, or I can send a proper
> v4 after getting acks for the changes.
I can squash it to a single patch before making a PR.
I'm happy with this series (1-5). I'd like an Acked-by from Laurent and/or Sakari
as well, though.
Regards,
Hans
>
> Tomi
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 1/1] media: v4l2-subdev: add subdev-wide state struct
2021-06-10 8:00 ` [PATCH v3 1/1] " Hans Verkuil
2021-06-10 8:24 ` Tomi Valkeinen
@ 2021-06-10 11:11 ` Laurent Pinchart
1 sibling, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2021-06-10 11:11 UTC (permalink / raw)
To: Hans Verkuil
Cc: Tomi Valkeinen, linux-media, Mauro Carvalho Chehab, Sakari Ailus
Hello,
On Thu, Jun 10, 2021 at 10:00:06AM +0200, Hans Verkuil wrote:
> On 09/06/2021 12:02, Tomi Valkeinen wrote:
> > We have 'struct v4l2_subdev_pad_config' which contains configuration for
> > a single pad used for the TRY functionality, and an array of those
> > structs is passed to various v4l2_subdev_pad_ops.
> >
> > I was working on subdev internal routing between pads, and realized that
> > there's no way to add TRY functionality for routes, which is not pad
> > specific configuration. Adding a separate struct for try-route config
> > wouldn't work either, as e.g. set-fmt needs to know the try-route
> > configuration to propagate the settings.
> >
> > This patch adds a new struct, 'struct v4l2_subdev_state' (which at the
> > moment only contains the v4l2_subdev_pad_config array) and the new
> > struct is used in most of the places where v4l2_subdev_pad_config was
> > used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config
> > are changed to instead take v4l2_subdev_state.
> >
> > The changes to drivers/media/v4l2-core/v4l2-subdev.c and
> > include/media/v4l2-subdev.h were written by hand, and all the driver
> > changes were done with the semantic patch below. The spatch needs to be
> > applied to a select list of directories. I used the following shell
> > commands to apply the spatch:
>
> <snip>
>
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 956dafab43d4..dacae53b68d5 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -26,19 +26,21 @@
> > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> > {
> > - if (sd->entity.num_pads) {
> > - fh->pad = v4l2_subdev_alloc_pad_config(sd);
> > - if (fh->pad == NULL)
> > - return -ENOMEM;
> > - }
> > + struct v4l2_subdev_state *state;
> > +
> > + state = v4l2_subdev_alloc_state(sd);
> > + if (IS_ERR(state))
> > + return PTR_ERR(state);
> > +
> > + fh->state = state;
> >
> > return 0;
> > }
> >
> > static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> > {
> > - v4l2_subdev_free_pad_config(fh->pad);
> > - fh->pad = NULL;
> > + v4l2_subdev_free_state(fh->state);
> > + fh->state = NULL;
> > }
> >
> > static int subdev_open(struct file *file)
> > @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
> > return 0;
> > }
> >
> > -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg)
> > +static int check_cfg(u32 which, struct v4l2_subdev_state *state)
>
> This should be renamed to check_state or check_state_pads (see next comment).
>
> > {
> > - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
> > + if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
>
> Should this also check for !state->pads? At least in the current code
> it really checks for the pad configuration, so I think it should.
>
> If so, then this function should probably be renamed to check_state_pads.
>
> > return -EINVAL;
> >
> > return 0;
> > }
> >
> > static inline int check_format(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_format *format)
> > {
> > if (!format)
> > return -EINVAL;
> >
> > return check_which(format->which) ? : check_pad(sd, format->pad) ? :
> > - check_cfg(format->which, cfg);
> > + check_cfg(format->which, state);
> > }
> >
> > static int call_get_fmt(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_format *format)
> > {
> > - return check_format(sd, cfg, format) ? :
> > - sd->ops->pad->get_fmt(sd, cfg, format);
> > + return check_format(sd, state, format) ? :
> > + sd->ops->pad->get_fmt(sd, state, format);
> > }
> >
> > static int call_set_fmt(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_format *format)
> > {
> > - return check_format(sd, cfg, format) ? :
> > - sd->ops->pad->set_fmt(sd, cfg, format);
> > + return check_format(sd, state, format) ? :
> > + sd->ops->pad->set_fmt(sd, state, format);
> > }
> >
> > static int call_enum_mbus_code(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_mbus_code_enum *code)
> > {
> > if (!code)
> > return -EINVAL;
> >
> > return check_which(code->which) ? : check_pad(sd, code->pad) ? :
> > - check_cfg(code->which, cfg) ? :
> > - sd->ops->pad->enum_mbus_code(sd, cfg, code);
> > + check_cfg(code->which, state) ? :
> > + sd->ops->pad->enum_mbus_code(sd, state, code);
> > }
> >
> > static int call_enum_frame_size(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_frame_size_enum *fse)
> > {
> > if (!fse)
> > return -EINVAL;
> >
> > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
> > - check_cfg(fse->which, cfg) ? :
> > - sd->ops->pad->enum_frame_size(sd, cfg, fse);
> > + check_cfg(fse->which, state) ? :
> > + sd->ops->pad->enum_frame_size(sd, state, fse);
> > }
> >
> > static inline int check_frame_interval(struct v4l2_subdev *sd,
> > @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd,
> > }
> >
> > static int call_enum_frame_interval(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_frame_interval_enum *fie)
> > {
> > if (!fie)
> > return -EINVAL;
> >
> > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
> > - check_cfg(fie->which, cfg) ? :
> > - sd->ops->pad->enum_frame_interval(sd, cfg, fie);
> > + check_cfg(fie->which, state) ? :
> > + sd->ops->pad->enum_frame_interval(sd, state, fie);
> > }
> >
> > static inline int check_selection(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_selection *sel)
> > {
> > if (!sel)
> > return -EINVAL;
> >
> > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
> > - check_cfg(sel->which, cfg);
> > + check_cfg(sel->which, state);
> > }
> >
> > static int call_get_selection(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_selection *sel)
> > {
> > - return check_selection(sd, cfg, sel) ? :
> > - sd->ops->pad->get_selection(sd, cfg, sel);
> > + return check_selection(sd, state, sel) ? :
> > + sd->ops->pad->get_selection(sd, state, sel);
> > }
> >
> > static int call_set_selection(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_selection *sel)
> > {
> > - return check_selection(sd, cfg, sel) ? :
> > - sd->ops->pad->set_selection(sd, cfg, sel);
> > + return check_selection(sd, state, sel) ? :
> > + sd->ops->pad->set_selection(sd, state, sel);
> > }
> >
> > static inline int check_edid(struct v4l2_subdev *sd,
> > @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > memset(format->reserved, 0, sizeof(format->reserved));
> > memset(format->format.reserved, 0, sizeof(format->format.reserved));
> > - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
> > + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format);
> > }
> >
> > case VIDIOC_SUBDEV_S_FMT: {
> > @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > memset(format->reserved, 0, sizeof(format->reserved));
> > memset(format->format.reserved, 0, sizeof(format->format.reserved));
> > - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> > + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format);
> > }
> >
> > case VIDIOC_SUBDEV_G_CROP: {
> > @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > sel.target = V4L2_SEL_TGT_CROP;
> >
> > rval = v4l2_subdev_call(
> > - sd, pad, get_selection, subdev_fh->pad, &sel);
> > + sd, pad, get_selection, subdev_fh->state, &sel);
> >
> > crop->rect = sel.r;
> >
> > @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > sel.r = crop->rect;
> >
> > rval = v4l2_subdev_call(
> > - sd, pad, set_selection, subdev_fh->pad, &sel);
> > + sd, pad, set_selection, subdev_fh->state, &sel);
> >
> > crop->rect = sel.r;
> >
> > @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > struct v4l2_subdev_mbus_code_enum *code = arg;
> >
> > memset(code->reserved, 0, sizeof(code->reserved));
> > - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
> > + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state,
> > code);
> > }
> >
> > @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > struct v4l2_subdev_frame_size_enum *fse = arg;
> >
> > memset(fse->reserved, 0, sizeof(fse->reserved));
> > - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
> > + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state,
> > fse);
> > }
> >
> > @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > struct v4l2_subdev_frame_interval_enum *fie = arg;
> >
> > memset(fie->reserved, 0, sizeof(fie->reserved));
> > - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
> > + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state,
> > fie);
> > }
> >
> > @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > memset(sel->reserved, 0, sizeof(sel->reserved));
> > return v4l2_subdev_call(
> > - sd, pad, get_selection, subdev_fh->pad, sel);
> > + sd, pad, get_selection, subdev_fh->state, sel);
> > }
> >
> > case VIDIOC_SUBDEV_S_SELECTION: {
> > @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > memset(sel->reserved, 0, sizeof(sel->reserved));
> > return v4l2_subdev_call(
> > - sd, pad, set_selection, subdev_fh->pad, sel);
> > + sd, pad, set_selection, subdev_fh->state, sel);
> > }
> >
> > case VIDIOC_G_EDID: {
> > @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link)
> > }
> > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
> >
> > -struct v4l2_subdev_pad_config *
> > -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd)
> > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd)
> > {
> > - struct v4l2_subdev_pad_config *cfg;
> > + struct v4l2_subdev_state *state;
> > int ret;
> >
> > - if (!sd->entity.num_pads)
> > - return NULL;
> > + state = kzalloc(sizeof(*state), GFP_KERNEL);
> > + if (!state)
> > + return ERR_PTR(-ENOMEM);
> >
> > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg),
> > - GFP_KERNEL | __GFP_ZERO);
> > - if (!cfg)
> > - return NULL;
> > -
> > - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg);
> > - if (ret < 0 && ret != -ENOIOCTLCMD) {
> > - kvfree(cfg);
> > - return NULL;
> > + if (sd->entity.num_pads) {
> > + state->pads = kvmalloc_array(sd->entity.num_pads,
> > + sizeof(*state->pads),
> > + GFP_KERNEL | __GFP_ZERO);
> > + if (!state->pads) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > }
> >
> > - return cfg;
> > + ret = v4l2_subdev_call(sd, pad, init_cfg, state);
>
> The init_cfg callback should also be renamed to init_state, but this can
> be done in a second patch.
>
> > + if (ret < 0 && ret != -ENOIOCTLCMD)
> > + goto err;
> > +
> > + return state;
> > +
> > +err:
> > + if (state && state->pads)
> > + kvfree(state->pads);
> > +
> > + kfree(state);
> > +
> > + return ERR_PTR(ret);
> > }
> > -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config);
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
> >
> > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg)
> > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
> > {
> > - kvfree(cfg);
> > + kvfree(state->pads);
>
> I'd change this to:
>
> if (state)
> kvfree(state->pads);
>
> That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust.
I'd do
if (!state)
return;
at the top in that case, as I expect the state will be extended over
time, with more things to free. This can be done now or later, doesn't
matter much to me.
> > + kfree(state);
> > }
> > -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config);
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
> > +
> > #endif /* CONFIG_MEDIA_CONTROLLER */
> >
> > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>
> <snip>
>
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index d0e9a5bdb08b..89115ba4c0f2 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config {
>
> I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state
> or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology.
>
> This too can be done in a separate patch.
>
> I like the 'state' term much better than 'config'.
>
> > struct v4l2_rect try_compose;
> > };
> >
> > +/**
> > + * struct v4l2_subdev_state - Used for storing subdev state information.
> > + *
> > + * @pads: &struct v4l2_subdev_pad_config array
> > + *
> > + * This structure only needs to be passed to the pad op if the 'which' field
> > + * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For
> > + * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL.
> > + */
> > +struct v4l2_subdev_state {
> > + struct v4l2_subdev_pad_config *pads;
> > +};
> > +
> > /**
> > * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations
> > *
> > @@ -687,27 +700,27 @@ struct v4l2_subdev_pad_config {
> > */
> > struct v4l2_subdev_pad_ops {
> > int (*init_cfg)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg);
> > + struct v4l2_subdev_state *state);
> > int (*enum_mbus_code)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_mbus_code_enum *code);
> > int (*enum_frame_size)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_frame_size_enum *fse);
> > int (*enum_frame_interval)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_frame_interval_enum *fie);
> > int (*get_fmt)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_format *format);
> > int (*set_fmt)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_format *format);
> > int (*get_selection)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_selection *sel);
> > int (*set_selection)(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > struct v4l2_subdev_selection *sel);
> > int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
> > int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid);
> > @@ -918,14 +931,14 @@ struct v4l2_subdev {
> > * struct v4l2_subdev_fh - Used for storing subdev information per file handle
> > *
> > * @vfh: pointer to &struct v4l2_fh
> > - * @pad: pointer to &struct v4l2_subdev_pad_config
> > + * @state: pointer to &struct v4l2_subdev_state
> > * @owner: module pointer to the owner of this file handle
> > */
> > struct v4l2_subdev_fh {
> > struct v4l2_fh vfh;
> > struct module *owner;
> > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > - struct v4l2_subdev_pad_config *pad;
> > + struct v4l2_subdev_state *state;
> > #endif
> > };
> >
> > @@ -945,17 +958,17 @@ struct v4l2_subdev_fh {
> > * &struct v4l2_subdev_pad_config->try_fmt
> > *
> > * @sd: pointer to &struct v4l2_subdev
> > - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> > - * @pad: index of the pad in the @cfg array.
> > + * @state: pointer to &struct v4l2_subdev_state
> > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array
> > */
> > static inline struct v4l2_mbus_framefmt *
> > v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > unsigned int pad)
> > {
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > - return &cfg[pad].try_fmt;
> > + return &state->pads[pad].try_fmt;
> > }
> >
> > /**
> > @@ -963,17 +976,17 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd,
> > * &struct v4l2_subdev_pad_config->try_crop
> > *
> > * @sd: pointer to &struct v4l2_subdev
> > - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> > - * @pad: index of the pad in the @cfg array.
> > + * @state: pointer to &struct v4l2_subdev_state.
> > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array.
> > */
> > static inline struct v4l2_rect *
> > v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > unsigned int pad)
> > {
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > - return &cfg[pad].try_crop;
> > + return &state->pads[pad].try_crop;
> > }
> >
> > /**
> > @@ -981,17 +994,17 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd,
> > * &struct v4l2_subdev_pad_config->try_compose
> > *
> > * @sd: pointer to &struct v4l2_subdev
> > - * @cfg: pointer to &struct v4l2_subdev_pad_config array.
> > - * @pad: index of the pad in the @cfg array.
> > + * @state: pointer to &struct v4l2_subdev_state.
> > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array.
> > */
> > static inline struct v4l2_rect *
> > v4l2_subdev_get_try_compose(struct v4l2_subdev *sd,
> > - struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_state *state,
> > unsigned int pad)
> > {
> > if (WARN_ON(pad >= sd->entity.num_pads))
> > pad = 0;
> > - return &cfg[pad].try_compose;
> > + return &state->pads[pad].try_compose;
> > }
> >
> > #endif
> > @@ -1093,20 +1106,21 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> > int v4l2_subdev_link_validate(struct media_link *link);
> >
> > /**
> > - * v4l2_subdev_alloc_pad_config - Allocates memory for pad config
> > + * v4l2_subdev_alloc_state - allocate v4l2_subdev_state
> > *
> > - * @sd: pointer to struct v4l2_subdev
> > + * @sd: pointer to &struct v4l2_subdev for which the state is being allocated.
> > + *
> > + * Must call v4l2_subdev_free_state() when state is no longer needed.
> > */
> > -struct
> > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd);
> > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd);
> >
> > /**
> > - * v4l2_subdev_free_pad_config - Frees memory allocated by
> > - * v4l2_subdev_alloc_pad_config().
> > + * v4l2_subdev_free_state - free a v4l2_subdev_state
> > *
> > - * @cfg: pointer to &struct v4l2_subdev_pad_config
> > + * @state: v4l2_subdev_state to be freed.
> > */
> > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg);
> > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state);
> > +
> > #endif /* CONFIG_MEDIA_CONTROLLER */
> >
> > /**
--
Regards,
:
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 2/5] media: omap3isp: fix indentation
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
` (2 preceding siblings ...)
2021-06-10 9:49 ` [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg() Tomi Valkeinen
@ 2021-06-10 11:23 ` Laurent Pinchart
3 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2021-06-10 11:23 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus
Hi Tomi,
Thank you for the patch.
On Thu, Jun 10, 2021 at 12:49:00PM +0300, Tomi Valkeinen wrote:
> Fix a few indentation warnings from checkpatch.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/omap3isp/ispccp2.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
> index 366da6fb8b4f..acb58b6ddba1 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -619,8 +619,8 @@ static const unsigned int ccp2_fmts[] = {
> */
> static struct v4l2_mbus_framefmt *
> __ccp2_get_format(struct isp_ccp2_device *ccp2,
> - struct v4l2_subdev_state *sd_state,
> - unsigned int pad, enum v4l2_subdev_format_whence which)
> + struct v4l2_subdev_state *sd_state,
> + unsigned int pad, enum v4l2_subdev_format_whence which)
> {
> if (which == V4L2_SUBDEV_FORMAT_TRY)
> return v4l2_subdev_get_try_format(&ccp2->subdev, sd_state,
> @@ -708,7 +708,7 @@ static int ccp2_enum_mbus_code(struct v4l2_subdev *sd,
> return -EINVAL;
>
> format = __ccp2_get_format(ccp2, sd_state, CCP2_PAD_SINK,
> - code->which);
> + code->which);
> code->code = format->code;
> }
>
> @@ -753,8 +753,8 @@ static int ccp2_enum_frame_size(struct v4l2_subdev *sd,
> * return -EINVAL or zero on success
> */
> static int ccp2_get_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> {
> struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *format;
> @@ -775,8 +775,8 @@ static int ccp2_get_format(struct v4l2_subdev *sd,
> * returns zero
> */
> static int ccp2_set_format(struct v4l2_subdev *sd,
> - struct v4l2_subdev_state *sd_state,
> - struct v4l2_subdev_format *fmt)
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_format *fmt)
> {
> struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd);
> struct v4l2_mbus_framefmt *format;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter
2021-06-10 9:49 ` [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter Tomi Valkeinen
@ 2021-06-10 11:25 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2021-06-10 11:25 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus
Hi Tomi,
Thank you for the patch.
On Thu, Jun 10, 2021 at 12:49:01PM +0300, Tomi Valkeinen wrote:
> "media: v4l2-subdev: add subdev-wide state struct" introduced some
> kernel doc errors due to cfg -> sd_state rename. Fix these.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/i2c/imx274.c | 6 +++---
> drivers/media/i2c/imx334.c | 10 +++++-----
> drivers/media/i2c/mt9m032.c | 4 ++--
> drivers/media/i2c/tvp514x.c | 6 +++---
> drivers/media/platform/vsp1/vsp1_entity.c | 6 +++---
> drivers/media/platform/xilinx/xilinx-csi2rxss.c | 6 +++---
> drivers/media/platform/xilinx/xilinx-vip.c | 4 ++--
> 7 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index e0c413cc94d7..0dce92872176 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -996,7 +996,7 @@ static int imx274_binning_goodness(struct stimx274 *imx274,
> * Must be called with imx274->lock locked.
> *
> * @imx274: The device object
> - * @cfg: The pad config we are editing for TRY requests
> + * @sd_state: The subdev state we are editing for TRY requests
> * @which: V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller
> * @width: Input-output parameter: set to the desired width before
> * the call, contains the chosen value after returning successfully
> @@ -1061,7 +1061,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
> /**
> * imx274_get_fmt - Get the pad format
> * @sd: Pointer to V4L2 Sub device structure
> - * @cfg: Pointer to sub device pad information structure
> + * @sd_state: Pointer to sub device state structure
> * @fmt: Pointer to pad level media bus format
> *
> * This function is used to get the pad format information.
> @@ -1083,7 +1083,7 @@ static int imx274_get_fmt(struct v4l2_subdev *sd,
> /**
> * imx274_set_fmt - This is used to set the pad format
> * @sd: Pointer to V4L2 Sub device structure
> - * @cfg: Pointer to sub device pad information structure
> + * @sd_state: Pointer to sub device state information structure
> * @format: Pointer to pad level media bus format
> *
> * This function is used to set the pad format.
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 751ce1f8f817..062125501788 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -497,7 +497,7 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
> /**
> * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
> * @sd: pointer to imx334 V4L2 sub-device structure
> - * @cfg: V4L2 sub-device pad configuration
> + * @sd_state: V4L2 sub-device state
> * @code: V4L2 sub-device code enumeration need to be filled
> *
> * Return: 0 if successful, error code otherwise.
> @@ -517,7 +517,7 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
> /**
> * imx334_enum_frame_size() - Enumerate V4L2 sub-device frame sizes
> * @sd: pointer to imx334 V4L2 sub-device structure
> - * @cfg: V4L2 sub-device pad configuration
> + * @sd_state: V4L2 sub-device state
> * @fsize: V4L2 sub-device size enumeration need to be filled
> *
> * Return: 0 if successful, error code otherwise.
> @@ -564,7 +564,7 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
> /**
> * imx334_get_pad_format() - Get subdevice pad format
> * @sd: pointer to imx334 V4L2 sub-device structure
> - * @cfg: V4L2 sub-device pad configuration
> + * @sd_state: V4L2 sub-device state
> * @fmt: V4L2 sub-device format need to be set
> *
> * Return: 0 if successful, error code otherwise.
> @@ -594,7 +594,7 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd,
> /**
> * imx334_set_pad_format() - Set subdevice pad format
> * @sd: pointer to imx334 V4L2 sub-device structure
> - * @cfg: V4L2 sub-device pad configuration
> + * @sd_state: V4L2 sub-device state
> * @fmt: V4L2 sub-device format need to be set
> *
> * Return: 0 if successful, error code otherwise.
> @@ -631,7 +631,7 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
> /**
> * imx334_init_pad_cfg() - Initialize sub-device pad configuration
> * @sd: pointer to imx334 V4L2 sub-device structure
> - * @cfg: V4L2 sub-device pad configuration
> + * @sd_state: V4L2 sub-device state
> *
> * Return: 0 if successful, error code otherwise.
> */
> diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c
> index 8a0741058c98..ba0c0ea91c95 100644
> --- a/drivers/media/i2c/mt9m032.c
> +++ b/drivers/media/i2c/mt9m032.c
> @@ -332,7 +332,7 @@ static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
> /**
> * __mt9m032_get_pad_crop() - get crop rect
> * @sensor: pointer to the sensor struct
> - * @cfg: v4l2_subdev_pad_config for getting the try crop rect from
> + * @sd_state: v4l2_subdev_state for getting the try crop rect from
> * @which: select try or active crop rect
> *
> * Returns a pointer the current active or fh relative try crop rect
> @@ -355,7 +355,7 @@ __mt9m032_get_pad_crop(struct mt9m032 *sensor,
> /**
> * __mt9m032_get_pad_format() - get format
> * @sensor: pointer to the sensor struct
> - * @cfg: v4l2_subdev_pad_config for getting the try format from
> + * @sd_state: v4l2_subdev_state for getting the try format from
> * @which: select try or active format
> *
> * Returns a pointer the current active or fh relative try format
> diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c
> index 5f159588aaeb..cee60f945036 100644
> --- a/drivers/media/i2c/tvp514x.c
> +++ b/drivers/media/i2c/tvp514x.c
> @@ -853,7 +853,7 @@ static const struct v4l2_ctrl_ops tvp514x_ctrl_ops = {
> /**
> * tvp514x_enum_mbus_code() - V4L2 decoder interface handler for enum_mbus_code
> * @sd: pointer to standard V4L2 sub-device structure
> - * @cfg: pad configuration
> + * @sd_state: subdev state
> * @code: pointer to v4l2_subdev_mbus_code_enum structure
> *
> * Enumertaes mbus codes supported
> @@ -880,7 +880,7 @@ static int tvp514x_enum_mbus_code(struct v4l2_subdev *sd,
> /**
> * tvp514x_get_pad_format() - V4L2 decoder interface handler for get pad format
> * @sd: pointer to standard V4L2 sub-device structure
> - * @cfg: pad configuration
> + * @sd_state: subdev state
> * @format: pointer to v4l2_subdev_format structure
> *
> * Retrieves pad format which is active or tried based on requirement
> @@ -912,7 +912,7 @@ static int tvp514x_get_pad_format(struct v4l2_subdev *sd,
> /**
> * tvp514x_set_pad_format() - V4L2 decoder interface handler for set pad format
> * @sd: pointer to standard V4L2 sub-device structure
> - * @cfg: pad configuration
> + * @sd_state: subdev state
> * @fmt: pointer to v4l2_subdev_format structure
> *
> * Set pad format for the output pad
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
> index 7d92262f3378..6ef874d29eb7 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -103,7 +103,7 @@ void vsp1_entity_configure_partition(struct vsp1_entity *entity,
> /**
> * vsp1_entity_get_pad_config - Get the pad configuration for an entity
> * @entity: the entity
> - * @cfg: the TRY pad configuration
> + * @sd_state: the TRY pad configuration
Should this read "the TRY state" ?
> * @which: configuration selector (ACTIVE or TRY)
> *
> * When called with which set to V4L2_SUBDEV_FORMAT_ACTIVE the caller must hold
> @@ -131,7 +131,7 @@ vsp1_entity_get_pad_config(struct vsp1_entity *entity,
> /**
> * vsp1_entity_get_pad_format - Get a pad format from storage for an entity
> * @entity: the entity
> - * @cfg: the configuration storage
> + * @sd_state: the configuration storage
And here, "the state storage". Same below.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> * @pad: the pad number
> *
> * Return the format stored in the given configuration for an entity's pad. The
> @@ -148,7 +148,7 @@ vsp1_entity_get_pad_format(struct vsp1_entity *entity,
> /**
> * vsp1_entity_get_pad_selection - Get a pad selection from storage for entity
> * @entity: the entity
> - * @cfg: the configuration storage
> + * @sd_state: the configuration storage
> * @pad: the pad number
> * @target: the selection target
> *
> diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> index 2773145a2226..b1baf9d7b6ec 100644
> --- a/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c
> @@ -698,7 +698,7 @@ __xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss,
> /**
> * xcsi2rxss_init_cfg - Initialise the pad format config to default
> * @sd: Pointer to V4L2 Sub device structure
> - * @cfg: Pointer to sub device pad information structure
> + * @sd_state: Pointer to sub device state structure
> *
> * This function is used to initialize the pad format with the default
> * values.
> @@ -725,7 +725,7 @@ static int xcsi2rxss_init_cfg(struct v4l2_subdev *sd,
> /**
> * xcsi2rxss_get_format - Get the pad format
> * @sd: Pointer to V4L2 Sub device structure
> - * @cfg: Pointer to sub device pad information structure
> + * @sd_state: Pointer to sub device state structure
> * @fmt: Pointer to pad level media bus format
> *
> * This function is used to get the pad format information.
> @@ -750,7 +750,7 @@ static int xcsi2rxss_get_format(struct v4l2_subdev *sd,
> /**
> * xcsi2rxss_set_format - This is used to set the pad format
> * @sd: Pointer to V4L2 Sub device structure
> - * @cfg: Pointer to sub device pad information structure
> + * @sd_state: Pointer to sub device state structure
> * @fmt: Pointer to pad level media bus format
> *
> * This function is used to set the pad format. Since the pad format is fixed
> diff --git a/drivers/media/platform/xilinx/xilinx-vip.c b/drivers/media/platform/xilinx/xilinx-vip.c
> index b989fee8351d..07d16a007d5f 100644
> --- a/drivers/media/platform/xilinx/xilinx-vip.c
> +++ b/drivers/media/platform/xilinx/xilinx-vip.c
> @@ -234,7 +234,7 @@ EXPORT_SYMBOL_GPL(xvip_cleanup_resources);
> /**
> * xvip_enum_mbus_code - Enumerate the media format code
> * @subdev: V4L2 subdevice
> - * @cfg: V4L2 subdev pad configuration
> + * @sd_state: V4L2 subdev state
> * @code: returning media bus code
> *
> * Enumerate the media bus code of the subdevice. Return the corresponding
> @@ -271,7 +271,7 @@ EXPORT_SYMBOL_GPL(xvip_enum_mbus_code);
> /**
> * xvip_enum_frame_size - Enumerate the media bus frame size
> * @subdev: V4L2 subdevice
> - * @cfg: V4L2 subdev pad configuration
> + * @sd_state: V4L2 subdev state
> * @fse: returning media bus frame size
> *
> * This function is a drop-in implementation of the subdev enum_frame_size pad
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state
2021-06-10 9:49 ` [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state Tomi Valkeinen
@ 2021-06-10 11:26 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2021-06-10 11:26 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus
Hi Tomi,
Thank you for the patch.
On Thu, Jun 10, 2021 at 12:49:02PM +0300, Tomi Valkeinen wrote:
> Make v4l2_subdev_free_state() accept a NULL state.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index dacae53b68d5..25c80d6de23b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -931,7 +931,8 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state);
>
> void v4l2_subdev_free_state(struct v4l2_subdev_state *state)
> {
> - kvfree(state->pads);
> + if (state)
> + kvfree(state->pads);
> kfree(state);
I'd write
if (!state)
return;
as I expect we'll have more code in the future.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> }
> EXPORT_SYMBOL_GPL(v4l2_subdev_free_state);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg()
2021-06-10 9:49 ` [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg() Tomi Valkeinen
@ 2021-06-10 11:26 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2021-06-10 11:26 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-media, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus
Hi Tomi,
Thank you for the patch.
On Thu, Jun 10, 2021 at 12:49:03PM +0300, Tomi Valkeinen wrote:
> Rename check_cfg() to check_state_pads() as it is now checking the pads
> of the state.
>
> Also fix the check so that it verifies that both state and state->pads
> exist.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 25c80d6de23b..daf7ffdd8882 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -148,9 +148,9 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad)
> return 0;
> }
>
> -static int check_cfg(u32 which, struct v4l2_subdev_state *state)
> +static int check_state_pads(u32 which, struct v4l2_subdev_state *state)
> {
> - if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
> + if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads))
> return -EINVAL;
>
> return 0;
> @@ -164,7 +164,7 @@ static inline int check_format(struct v4l2_subdev *sd,
> return -EINVAL;
>
> return check_which(format->which) ? : check_pad(sd, format->pad) ? :
> - check_cfg(format->which, state);
> + check_state_pads(format->which, state);
> }
>
> static int call_get_fmt(struct v4l2_subdev *sd,
> @@ -191,7 +191,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd,
> return -EINVAL;
>
> return check_which(code->which) ? : check_pad(sd, code->pad) ? :
> - check_cfg(code->which, state) ? :
> + check_state_pads(code->which, state) ? :
> sd->ops->pad->enum_mbus_code(sd, state, code);
> }
>
> @@ -203,7 +203,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd,
> return -EINVAL;
>
> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? :
> - check_cfg(fse->which, state) ? :
> + check_state_pads(fse->which, state) ? :
> sd->ops->pad->enum_frame_size(sd, state, fse);
> }
>
> @@ -238,7 +238,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd,
> return -EINVAL;
>
> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? :
> - check_cfg(fie->which, state) ? :
> + check_state_pads(fie->which, state) ? :
> sd->ops->pad->enum_frame_interval(sd, state, fie);
> }
>
> @@ -250,7 +250,7 @@ static inline int check_selection(struct v4l2_subdev *sd,
> return -EINVAL;
>
> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? :
> - check_cfg(sel->which, state);
> + check_state_pads(sel->which, state);
> }
>
> static int call_get_selection(struct v4l2_subdev *sd,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-10 11:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 10:02 [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
[not found] ` <20210609100228.278798-2-tomi.valkeinen@ideasonboard.com>
2021-06-10 8:00 ` [PATCH v3 1/1] " Hans Verkuil
2021-06-10 8:24 ` Tomi Valkeinen
2021-06-10 11:11 ` Laurent Pinchart
2021-06-10 9:49 ` [PATCH v3 2/5] media: omap3isp: fix indentation Tomi Valkeinen
2021-06-10 9:49 ` [PATCH v3 3/5] media: fix kernel doc errors for sd_state parameter Tomi Valkeinen
2021-06-10 11:25 ` Laurent Pinchart
2021-06-10 9:49 ` [PATCH v3 4/5] media: v4l2-subdev: v4l2_subdev_free_state() to accept a NULL state Tomi Valkeinen
2021-06-10 11:26 ` Laurent Pinchart
2021-06-10 9:49 ` [PATCH v3 5/5] media: v4l2-subdev: fix and rename check_cfg() Tomi Valkeinen
2021-06-10 11:26 ` Laurent Pinchart
2021-06-10 11:23 ` [PATCH v3 2/5] media: omap3isp: fix indentation Laurent Pinchart
2021-06-10 9:51 ` [PATCH v3 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
2021-06-10 9:54 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.