From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Sakari Ailus <sakari.ailus@linux.intel.com>
Subject: Re: [PATCH v2 1/1] media: v4l2-subdev: add subdev-wide state struct
Date: Mon, 7 Jun 2021 09:39:25 +0300 [thread overview]
Message-ID: <65b7ffd6-ff5e-fe3a-d764-093026ad75fe@ideasonboard.com> (raw)
In-Reply-To: <YLpEpMRxZxdmpPPa@pendragon.ideasonboard.com>
On 04/06/2021 18:20, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Thu, May 27, 2021 at 12:42:44PM +0300, 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.
>
> I really like this change, especially for the next steps that it
> enables.
>
>> 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:
>>
>> dirs="drivers/media/i2c drivers/media/platform drivers/media/usb drivers/media/test-drivers/vimc drivers/media/pci drivers/staging/media"
>> for dir in $dirs; do spatch -j8 --dir --include-headers --no-show-diff --in-place --sp-file v4l2-subdev-state.cocci $dir; done
>>
>> Note that Coccinelle chokes on a few drivers (gcc extensions?). With
>> minor changes we can make Coccinelle run fine, and these changes can be
>> reverted after spatch. The diff for these changes is:
>>
>> diff --git a/drivers/media/i2c/s5k5baf.c b/drivers/media/i2c/s5k5baf.c
>> index 6e702b57c37d..5f136831039f 100644
>> --- a/drivers/media/i2c/s5k5baf.c
>> +++ b/drivers/media/i2c/s5k5baf.c
>> @@ -1481,7 +1481,7 @@ static int s5k5baf_set_selection(struct v4l2_subdev *sd,
>> &s5k5baf_cis_rect,
>> v4l2_subdev_get_try_crop(sd, cfg, PAD_CIS),
>> v4l2_subdev_get_try_compose(sd, cfg, PAD_CIS),
>> - v4l2_subdev_get_try_crop(sd, cfg, PAD_OUT)
>> + v4l2_subdev_get_try_crop(sd, cfg, PAD_OUT),
>> };
>> s5k5baf_set_rect_and_adjust(rects, rtype, &sel->r);
>> return 0;
>> diff --git a/drivers/media/platform/s3c-camif/camif-capture.c b/drivers/media/platform/s3c-camif/camif-capture.c
>> index 9ca49af29542..d92a5fb8835f 100644
>> --- a/drivers/media/platform/s3c-camif/camif-capture.c
>> +++ b/drivers/media/platform/s3c-camif/camif-capture.c
>> @@ -1230,7 +1230,7 @@ static int s3c_camif_subdev_get_fmt(struct v4l2_subdev *sd,
>> *mf = camif->mbus_fmt;
>> break;
>>
>> - case CAMIF_SD_PAD_SOURCE_C...CAMIF_SD_PAD_SOURCE_P:
>> + case CAMIF_SD_PAD_SOURCE_C:
>> /* crop rectangle at camera interface input */
>> mf->width = camif->camif_crop.width;
>> mf->height = camif->camif_crop.height;
>> @@ -1332,7 +1332,7 @@ static int s3c_camif_subdev_set_fmt(struct v4l2_subdev *sd,
>> }
>> break;
>>
>> - case CAMIF_SD_PAD_SOURCE_C...CAMIF_SD_PAD_SOURCE_P:
>> + case CAMIF_SD_PAD_SOURCE_C:
>> /* Pixel format can be only changed on the sink pad. */
>> mf->code = camif->mbus_fmt.code;
>> mf->width = crop->width;
>>
>> The semantic patch is:
>>
>> // <smpl>
>>
>> // Change function parameter
>>
>> @@
>> identifier func;
>> identifier cfg;
>> @@
>>
>> func(...,
>> - struct v4l2_subdev_pad_config *cfg
>> + struct v4l2_subdev_state *sd_state
>> , ...)
>> {
>> <...
>> - cfg
>> + sd_state
>> ...>
>> }
>>
>> // Change function declaration parameter
>>
>> @@
>> identifier func;
>> identifier cfg;
>> type T;
>> @@
>> T func(...,
>> - struct v4l2_subdev_pad_config *cfg
>> + struct v4l2_subdev_state *sd_state
>> , ...);
>>
>> // Change function return value
>>
>> @@
>> identifier func;
>> @@
>> - struct v4l2_subdev_pad_config
>> + struct v4l2_subdev_state
>> *func(...)
>> {
>> ...
>> }
>>
>> // Change function declaration return value
>>
>> @@
>> identifier func;
>> @@
>> - struct v4l2_subdev_pad_config
>> + struct v4l2_subdev_state
>> *func(...);
>>
>> // Some drivers pass a local pad_cfg for a single pad to a called function. Wrap it
>> // inside a pad_state.
>>
>> @@
>> identifier func;
>> identifier pad_cfg;
>> @@
>> func(...)
>> {
>> ...
>> struct v4l2_subdev_pad_config pad_cfg;
>> + struct v4l2_subdev_state pad_state = { .pads = &pad_cfg };
>>
>> <+...
>>
>> (
>> v4l2_subdev_call
>> |
>> sensor_call
>> |
>> isi_try_fse
>> |
>> isc_try_fse
>> |
>> saa_call_all
>> )
>> (...,
>> - &pad_cfg
>> + &pad_state
>> ,...)
>>
>> ...+>
>> }
>>
>> // If the function uses fields from pad_config, access via state->pads
>>
>> @@
>> identifier func;
>> identifier state;
>> @@
>> func(...,
>> struct v4l2_subdev_state *state
>> , ...)
>> {
>> <...
>> (
>> - state->try_fmt
>> + state->pads->try_fmt
>> |
>> - state->try_crop
>> + state->pads->try_crop
>> |
>> - state->try_compose
>> + state->pads->try_compose
>> )
>> ...>
>> }
>>
>> // If the function accesses the filehandle, use fh->state instead
>>
>> @@
>> struct v4l2_subdev_fh *fh;
>> @@
>> - fh->pad
>> + fh->state
>>
>> @@
>> struct v4l2_subdev_fh fh;
>> @@
>> - fh.pad
>> + fh.state
>>
>> // Start of vsp1 specific
>>
>> @@
>> @@
>> struct vsp1_entity {
>> ...
>> - struct v4l2_subdev_pad_config *config;
>> + struct v4l2_subdev_state *config;
>> ...
>> };
>>
>> @@
>> symbol entity;
>> @@
>> vsp1_entity_init(...)
>> {
>> ...
>> entity->config =
>> - v4l2_subdev_alloc_pad_config
>> + v4l2_subdev_alloc_state
>> (&entity->subdev);
>> ...
>> }
>>
>> @@
>> symbol entity;
>> @@
>> vsp1_entity_destroy(...)
>> {
>> ...
>> - v4l2_subdev_free_pad_config
>> + v4l2_subdev_free_state
>> (entity->config);
>> ...
>> }
>>
>> @exists@
>> identifier func =~ "(^vsp1.*)|(hsit_set_format)|(sru_enum_frame_size)|(sru_set_format)|(uif_get_selection)|(uif_set_selection)|(uds_enum_frame_size)|(uds_set_format)|(brx_set_format)|(brx_get_selection)|(histo_get_selection)|(histo_set_selection)|(brx_set_selection)";
>> symbol config;
>> @@
>> func(...) {
>> ...
>> - struct v4l2_subdev_pad_config *config;
>> + struct v4l2_subdev_state *config;
>> ...
>> }
>>
>> // End of vsp1 specific
>>
>> // Start of rcar specific
>>
>> @@
>> identifier sd;
>> identifier pad_cfg;
>> @@
>> rvin_try_format(...)
>> {
>> ...
>> - struct v4l2_subdev_pad_config *pad_cfg;
>> + struct v4l2_subdev_state *sd_state;
>> ...
>> - pad_cfg = v4l2_subdev_alloc_pad_config(sd);
>> + sd_state = v4l2_subdev_alloc_state(sd);
>> <...
>> - pad_cfg
>> + sd_state
>> ...>
>> - v4l2_subdev_free_pad_config(pad_cfg);
>> + v4l2_subdev_free_state(sd_state);
>> ...
>> }
>>
>> // End of rcar specific
>>
>> // Start of rockchip specific
>>
>> @@
>> identifier func =~ "(rkisp1_rsz_get_pad_fmt)|(rkisp1_rsz_get_pad_crop)|(rkisp1_rsz_register)";
>> symbol rsz;
>> symbol pad_cfg;
>> @@
>>
>> func(...)
>> {
>> + struct v4l2_subdev_state state = { .pads = rsz->pad_cfg };
>> ...
>> - rsz->pad_cfg
>> + &state
>> ...
>> }
>>
>> @@
>> identifier func =~ "(rkisp1_isp_get_pad_fmt)|(rkisp1_isp_get_pad_crop)";
>> symbol isp;
>> symbol pad_cfg;
>> @@
>>
>> func(...)
>> {
>> + struct v4l2_subdev_state state = { .pads = isp->pad_cfg };
>> ...
>> - isp->pad_cfg
>> + &state
>> ...
>> }
>>
>> @@
>> symbol rkisp1;
>> symbol isp;
>> symbol pad_cfg;
>> @@
>>
>> rkisp1_isp_register(...)
>> {
>> + struct v4l2_subdev_state state = { .pads = rkisp1->isp.pad_cfg };
>> ...
>> - rkisp1->isp.pad_cfg
>> + &state
>> ...
>> }
>>
>> // End of rockchip specific
>>
>> // Start of tegra-video specific
>>
>> @@
>> identifier sd;
>> identifier pad_cfg;
>> @@
>> __tegra_channel_try_format(...)
>> {
>> ...
>> - struct v4l2_subdev_pad_config *pad_cfg;
>> + struct v4l2_subdev_state *sd_state;
>> ...
>> - pad_cfg = v4l2_subdev_alloc_pad_config(sd);
>> + sd_state = v4l2_subdev_alloc_state(sd);
>> <...
>> - pad_cfg
>> + sd_state
>> ...>
>> - v4l2_subdev_free_pad_config(pad_cfg);
>> + v4l2_subdev_free_state(sd_state);
>> ...
>> }
>>
>> @@
>> identifier sd_state;
>> @@
>> __tegra_channel_try_format(...)
>> {
>> ...
>> struct v4l2_subdev_state *sd_state;
>> <...
>> - sd_state->try_crop
>> + sd_state->pads->try_crop
>> ...>
>> }
>>
>> // End of tegra-video specific
>>
>> // </smpl>
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> 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/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 | 141 ++++++++++--------
>> .../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 | 72 +++++----
>> 165 files changed, 2123 insertions(+), 1769 deletions(-)
>
> [snip]
>
> Skipping the drivers, or the mail won't make it to the list (and I'm not
> sure I'd survive the process anyway).
>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 956dafab43d4..bd996dcace3b 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)
>> {
>> - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg)
>> + if (which == V4L2_SUBDEV_FORMAT_TRY && !state)
>> 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,50 @@ 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;
>> -
>> - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg),
>> - GFP_KERNEL | __GFP_ZERO);
>> - if (!cfg)
>> - return NULL;
>> + state = kzalloc(sizeof(*state), GFP_KERNEL);
>> + if (!state) {
>> + ret = -ENOMEM;
>> + goto err;
>
> You can return ERR_PTR(-ENOMEM) directly.
>
>> + }
>>
>> - 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);
>> + 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);
>> + kvfree(state);
>
> state was allocated with kzalloc(), this should be kfree().
>
>> }
>> -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..79f9932d2b00 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config {
>> struct v4l2_rect try_compose;
>> };
>>
>> +/**
>> + * struct v4l2_subdev_state - Used for storing subdev information.
>
> s/information/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.
>
> We should fix this by storing an instance of v4l2_subdev_state in
> v4l2_subdev (on top of this patch).
Probably, although I fear it might open a can of worms. We'll then have
the situation where active formats are stored in v4l2_subdev _and_ in
the driver's own struct, and also I'm pretty sure there are drivers that
do funny stuff in get_fmt etc.
Maybe this could be made explicit and handled with a subdev flag,
indicating that this driver supports the state in v4l2_subdev (and if
not, the state pointer in v4l2_subdev would be null).
It also bring up the question of locking, as currently get_fmt can do
any kind of locking it needs (or anything else it needs to have a valid
fmt), whereas if the state is stored in v4l2_subdev, we must define when
it is ok to change it and when to read it.
>> + */
>> +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;
>
> I'd expect the v4l2_subdev_state structure to remain small, as it will
> mostly contains a few pointers. Should we embed it in v4l2_subdev_fh
> instead of allocating it separately ?
That's what I initially had, but I changed it after someone commented:
> I have a feeling we may be better off allocating states dynamically
> (storing a pointer in v4l2_subdev_fh instead of embedding an
> instance), in which case v4l2_subdev_state_alloc() and
> v4l2_subdev_state_free() would be better
I changed it, as it did make the life-cycle of the state more obvious,
and also because the state grows a bit with the routing and stream patches.
If we have the state in v4l2_subdev, and that one is a pointer (to allow
null), maybe it's easier to keep it as a pointer in the v4l2_subdev_fh too.
I don't have a strong opinion on this, except that I don't want to
change anything in this patch unless really needed, as it's a PITA to
update the semantic patch =).
>> #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 @state array.
>
> @state isn't an 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;
>
> As the goal is to not limit the state to TRY formats but store it in the
> v4l2_subdev structure too, I'd rename try_fmt to format.
> v4l2_subdev_get_try_format() should then become
> v4l2_subdev_state_get_oad_format() or something similar (likely in a
> separate patch).
Yes, there are plenty of opportunities to do more on top of this patch,
especially renames. I did notice a few, but I didn't even try to go
there as the patch is so huge already.
>> }
>>
>> /**
>> @@ -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 @state 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 @state 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,17 @@ 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
>
> Won't kerneldoc complain if not all parameters are documented ?
Yes it does. I just missed it with the huge amount of warnings it spams.
I've addressed the other issues you raised. Thanks for review!
I'll send a v3 when we come to a conclusion on the pointer vs embedding.
Tomi
next prev parent reply other threads:[~2021-06-07 6:39 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-27 9:42 [PATCH v2 0/1] media: v4l2-subdev: add subdev-wide state struct Tomi Valkeinen
[not found] ` <20210527094244.617473-2-tomi.valkeinen@ideasonboard.com>
2021-06-04 15:20 ` [PATCH v2 1/1] " Laurent Pinchart
2021-06-05 22:56 ` Laurent Pinchart
2021-06-06 7:27 ` Tomi Valkeinen
2021-06-07 6:39 ` Tomi Valkeinen [this message]
2021-06-07 7:47 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=65b7ffd6-ff5e-fe3a-d764-093026ad75fe@ideasonboard.com \
--to=tomi.valkeinen@ideasonboard.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).