| Message ID | 20231013104424.404768-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Unify sub-device state access functions | expand |
Hi Sakari, Thank you for the patch. On Fri, Oct 13, 2023 at 01:44:22PM +0300, Sakari Ailus wrote: > Now that v4l2_subdev_get_format() always returns format, don't call > alternative v4l2_subdev_get_pad_format() anymore. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Assuming we go forward for 2/6, this patch looks fine. Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index a522cd8096cf..153e9b1958d6 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1580,14 +1580,7 @@ int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state, > { > struct v4l2_mbus_framefmt *fmt; > > - if (sd->flags & V4L2_SUBDEV_FL_STREAMS) > - fmt = v4l2_subdev_get_format(state, format->pad, > - format->stream); > - else if (format->pad < sd->entity.num_pads && format->stream == 0) > - fmt = v4l2_subdev_get_pad_format(sd, state, format->pad); > - else > - fmt = NULL; > - > + fmt = v4l2_subdev_get_format(state, format->pad, format->stream); > if (!fmt) > return -EINVAL; >
Hi Sakari, Thank you for the patch. On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > There are two sets of functions that return information from sub-device > state, one for stream-unaware users and another for stream-aware users. > Add support for stream-aware functions to return format, crop and compose > information from pad-based array that are functionally equivalent to the > old, stream-unaware ones. > > Also check state is non-NULL, in order to guard against old drivers > potentially calling this with NULL state for active formats or selection > rectangles. I'm not too keen on this I'm afraid :-( I think it gets confusing for drivers that are not stream-aware to have to call a function that takes a stream number. I don't see a problem with keeping two different sets of functions, one for stream-aware drivers, and one for other drivers. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 52a8043ab556..7d0ce8c8aab4 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1684,6 +1684,19 @@ v4l2_subdev_state_get_stream_format(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_fmt; > + } > + > lockdep_assert_held(state->lock); > > stream_configs = &state->stream_configs; > @@ -1705,6 +1718,19 @@ v4l2_subdev_state_get_stream_crop(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_crop; > + } > + > lockdep_assert_held(state->lock); > > stream_configs = &state->stream_configs; > @@ -1726,6 +1752,19 @@ v4l2_subdev_state_get_stream_compose(struct v4l2_subdev_state *state, > struct v4l2_subdev_stream_configs *stream_configs; > unsigned int i; > > + if (WARN_ON(!state)) > + return NULL; > + > + if (state->pads) { > + if (stream) > + return NULL; > + > + if (WARN_ON(pad >= state->num_pads)) > + pad = 0; > + > + return &state->pads[pad].try_compose; > + } > + > lockdep_assert_held(state->lock); > > stream_configs = &state->stream_configs;
On Fri, Oct 13, 2023 at 11:06:08AM +0000, Sakari Ailus wrote: > On Fri, Oct 13, 2023 at 01:57:49PM +0300, Laurent Pinchart wrote: > > On Fri, Oct 13, 2023 at 01:44:19PM +0300, Sakari Ailus wrote: > > > Store the number of pads in the sub-device state. This will be needed to > > > validate pad when retrieving information for non-stream-aware users. > > > > I'd rather store a pointer to the subdev. You can get the number of pads > > from there. > > The value is initialised after the array is allocated so this won't change. > > I don't have a strong opinion either way. It's still more efficient to > store just the value. Slightly so, but I don't think it will matter in practice. I believe we'll have more needs to access the subdev from the state in the future, which is why I'd rather store the pointer already.
Hi Sakari, On Fri, Oct 13, 2023 at 11:13:00AM +0000, Sakari Ailus wrote: > On Fri, Oct 13, 2023 at 02:07:41PM +0300, Laurent Pinchart wrote: > > On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > > > There are two sets of functions that return information from sub-device > > > state, one for stream-unaware users and another for stream-aware users. > > > Add support for stream-aware functions to return format, crop and compose > > > information from pad-based array that are functionally equivalent to the > > > old, stream-unaware ones. > > > > > > Also check state is non-NULL, in order to guard against old drivers > > > potentially calling this with NULL state for active formats or selection > > > rectangles. > > > > I'm not too keen on this I'm afraid :-( I think it gets confusing for > > drivers that are not stream-aware to have to call a function that takes > > a stream number. I don't see a problem with keeping two different sets > > of functions, one for stream-aware drivers, and one for other drivers. > > This becomes a nuisance in drivers such as CCS that work with sub-devices > some of which have streams and others which don't. I don't see why we > should have two sets of functions to access the same information, even > though it's stored differently. > > I can add a wrapper using C11 _Generic to make the stream number go away. That could possibly be interesting.
On Mon, Oct 16, 2023 at 11:24:52AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Fri, Oct 13, 2023 at 11:13:00AM +0000, Sakari Ailus wrote: > > On Fri, Oct 13, 2023 at 02:07:41PM +0300, Laurent Pinchart wrote: > > > On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > > > > There are two sets of functions that return information from sub-device > > > > state, one for stream-unaware users and another for stream-aware users. > > > > Add support for stream-aware functions to return format, crop and compose > > > > information from pad-based array that are functionally equivalent to the > > > > old, stream-unaware ones. > > > > > > > > Also check state is non-NULL, in order to guard against old drivers > > > > potentially calling this with NULL state for active formats or selection > > > > rectangles. > > > > > > I'm not too keen on this I'm afraid :-( I think it gets confusing for > > > drivers that are not stream-aware to have to call a function that takes > > > a stream number. I don't see a problem with keeping two different sets > > > of functions, one for stream-aware drivers, and one for other drivers. > > > > This becomes a nuisance in drivers such as CCS that work with sub-devices > > some of which have streams and others which don't. I don't see why we > > should have two sets of functions to access the same information, even > > though it's stored differently. > > > > I can add a wrapper using C11 _Generic to make the stream number go away. > > That could possibly be interesting. I'll add this for v2.
On Mon, Oct 16, 2023 at 08:59:15AM +0000, Sakari Ailus wrote: > On Mon, Oct 16, 2023 at 11:24:52AM +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Fri, Oct 13, 2023 at 11:13:00AM +0000, Sakari Ailus wrote: > > > On Fri, Oct 13, 2023 at 02:07:41PM +0300, Laurent Pinchart wrote: > > > > On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > > > > > There are two sets of functions that return information from sub-device > > > > > state, one for stream-unaware users and another for stream-aware users. > > > > > Add support for stream-aware functions to return format, crop and compose > > > > > information from pad-based array that are functionally equivalent to the > > > > > old, stream-unaware ones. > > > > > > > > > > Also check state is non-NULL, in order to guard against old drivers > > > > > potentially calling this with NULL state for active formats or selection > > > > > rectangles. > > > > > > > > I'm not too keen on this I'm afraid :-( I think it gets confusing for > > > > drivers that are not stream-aware to have to call a function that takes > > > > a stream number. I don't see a problem with keeping two different sets > > > > of functions, one for stream-aware drivers, and one for other drivers. > > > > > > This becomes a nuisance in drivers such as CCS that work with sub-devices > > > some of which have streams and others which don't. I don't see why we > > > should have two sets of functions to access the same information, even > > > though it's stored differently. > > > > > > I can add a wrapper using C11 _Generic to make the stream number go away. > > > > That could possibly be interesting. > > I'll add this for v2. C11 _Generic() isn't up to the task as it only deals with argument types. On the other hand, variadic arguments supports this. It'll look like: #define v4l2_subdev_get_crop(state, pad, ...) \ __v4l2_subdev_get_crop_ ## __VA_OPT__(stream) \ (state, pad __VA_OPT__(,) __VA_ARGS__) #define __v4l2_subdev_get_crop_(state, pad) \ __v4l2_subdev_get_crop(state, pad, 0) #define __v4l2_subdev_get_crop_stream(state, pad, stream) \ __v4l2_subdev_get_crop(state, pad, stream) struct v4l2_rect * __v4l2_subdev_get_crop(struct v4l2_subdev_state *state, unsigned int pad, u32 stream); Which I'd say is tolerable, given the result is a single function to access format, crop and compose information in the sub-device state. Any thoughts on this?
On Mon, Oct 16, 2023 at 10:21:45AM +0000, Sakari Ailus wrote: > On Mon, Oct 16, 2023 at 08:59:15AM +0000, Sakari Ailus wrote: > > On Mon, Oct 16, 2023 at 11:24:52AM +0300, Laurent Pinchart wrote: > > > On Fri, Oct 13, 2023 at 11:13:00AM +0000, Sakari Ailus wrote: > > > > On Fri, Oct 13, 2023 at 02:07:41PM +0300, Laurent Pinchart wrote: > > > > > On Fri, Oct 13, 2023 at 01:44:20PM +0300, Sakari Ailus wrote: > > > > > > There are two sets of functions that return information from sub-device > > > > > > state, one for stream-unaware users and another for stream-aware users. > > > > > > Add support for stream-aware functions to return format, crop and compose > > > > > > information from pad-based array that are functionally equivalent to the > > > > > > old, stream-unaware ones. > > > > > > > > > > > > Also check state is non-NULL, in order to guard against old drivers > > > > > > potentially calling this with NULL state for active formats or selection > > > > > > rectangles. > > > > > > > > > > I'm not too keen on this I'm afraid :-( I think it gets confusing for > > > > > drivers that are not stream-aware to have to call a function that takes > > > > > a stream number. I don't see a problem with keeping two different sets > > > > > of functions, one for stream-aware drivers, and one for other drivers. > > > > > > > > This becomes a nuisance in drivers such as CCS that work with sub-devices > > > > some of which have streams and others which don't. I don't see why we > > > > should have two sets of functions to access the same information, even > > > > though it's stored differently. > > > > > > > > I can add a wrapper using C11 _Generic to make the stream number go away. > > > > > > That could possibly be interesting. > > > > I'll add this for v2. > > C11 _Generic() isn't up to the task as it only deals with argument types. On > the other hand, variadic arguments supports this. It'll look like: > > #define v4l2_subdev_get_crop(state, pad, ...) \ > __v4l2_subdev_get_crop_ ## __VA_OPT__(stream) \ > (state, pad __VA_OPT__(,) __VA_ARGS__) > #define __v4l2_subdev_get_crop_(state, pad) \ > __v4l2_subdev_get_crop(state, pad, 0) > #define __v4l2_subdev_get_crop_stream(state, pad, stream) \ > __v4l2_subdev_get_crop(state, pad, stream) > struct v4l2_rect * > __v4l2_subdev_get_crop(struct v4l2_subdev_state *state, unsigned int pad, > u32 stream); > > Which I'd say is tolerable, given the result is a single function to access > format, crop and compose information in the sub-device state. > > Any thoughts on this? With s/v4l2_subdev_get_crop/v4l2_subdev_state_get_crop/ I'm OK with this.
Hi folks, This set unifies historical and stream-aware sub-device state access functions into one set of functions called v4l2_subdev_get_{format,crop,compose}. No functional change is intended. This happened after Tomi suggested adding a num_pads field to struct v4l2_subdev_state. I'd like to merge these somewhat soon, assuming there's a rough consensus on them, as this is a fairly hot set, touching 136 files. Sakari Ailus (6): media: v4l: subdev: Store the number of pads in sub-device state media: v4l: subdev: Also return pads array information on stream functions media: v4l: subdev: Rename sub-device state information access functions media: v4l: subdev: v4l2_subdev_get_format always returns format now media: v4l: subdev: Switch to stream-aware state functions media: v4l: subdev: Remove stream-unaware sub-device state access drivers/media/i2c/adv7180.c | 4 +- drivers/media/i2c/adv748x/adv748x-afe.c | 6 +- drivers/media/i2c/adv748x/adv748x-csi2.c | 2 +- drivers/media/i2c/adv748x/adv748x-hdmi.c | 6 +- drivers/media/i2c/adv7511-v4l2.c | 4 +- drivers/media/i2c/adv7604.c | 4 +- drivers/media/i2c/adv7842.c | 4 +- drivers/media/i2c/ar0521.c | 5 +- drivers/media/i2c/ccs/ccs-core.c | 21 ++-- drivers/media/i2c/et8ek8/et8ek8_driver.c | 3 +- drivers/media/i2c/hi556.c | 12 +- drivers/media/i2c/hi846.c | 11 +- drivers/media/i2c/hi847.c | 8 +- drivers/media/i2c/imx208.c | 8 +- drivers/media/i2c/imx214.c | 4 +- drivers/media/i2c/imx219.c | 12 +- drivers/media/i2c/imx258.c | 8 +- drivers/media/i2c/imx290.c | 8 +- drivers/media/i2c/imx296.c | 18 +-- drivers/media/i2c/imx319.c | 7 +- drivers/media/i2c/imx334.c | 4 +- drivers/media/i2c/imx335.c | 4 +- drivers/media/i2c/imx355.c | 7 +- drivers/media/i2c/imx412.c | 4 +- drivers/media/i2c/imx415.c | 6 +- drivers/media/i2c/isl7998x.c | 6 +- drivers/media/i2c/max9286.c | 4 +- drivers/media/i2c/mt9m001.c | 4 +- drivers/media/i2c/mt9m111.c | 4 +- drivers/media/i2c/mt9m114.c | 58 +++++----- drivers/media/i2c/mt9p031.c | 6 +- drivers/media/i2c/mt9v032.c | 10 +- drivers/media/i2c/mt9v111.c | 2 +- drivers/media/i2c/og01a1b.c | 9 +- drivers/media/i2c/ov01a10.c | 2 +- drivers/media/i2c/ov02a10.c | 5 +- drivers/media/i2c/ov08d10.c | 8 +- drivers/media/i2c/ov08x40.c | 7 +- drivers/media/i2c/ov13858.c | 10 +- drivers/media/i2c/ov13b10.c | 10 +- drivers/media/i2c/ov2640.c | 4 +- drivers/media/i2c/ov2659.c | 6 +- drivers/media/i2c/ov2680.c | 6 +- drivers/media/i2c/ov2685.c | 4 +- drivers/media/i2c/ov2740.c | 4 +- drivers/media/i2c/ov4689.c | 2 +- drivers/media/i2c/ov5640.c | 9 +- drivers/media/i2c/ov5645.c | 4 +- drivers/media/i2c/ov5647.c | 12 +- drivers/media/i2c/ov5648.c | 6 +- drivers/media/i2c/ov5670.c | 12 +- drivers/media/i2c/ov5675.c | 8 +- drivers/media/i2c/ov5693.c | 4 +- drivers/media/i2c/ov5695.c | 7 +- drivers/media/i2c/ov7251.c | 4 +- drivers/media/i2c/ov7670.c | 7 +- drivers/media/i2c/ov7740.c | 7 +- drivers/media/i2c/ov8856.c | 8 +- drivers/media/i2c/ov8858.c | 6 +- drivers/media/i2c/ov8865.c | 8 +- drivers/media/i2c/ov9282.c | 6 +- drivers/media/i2c/ov9650.c | 7 +- drivers/media/i2c/ov9734.c | 8 +- drivers/media/i2c/s5c73m3/s5c73m3-core.c | 35 +++--- drivers/media/i2c/s5k5baf.c | 30 +++-- drivers/media/i2c/s5k6a3.c | 8 +- drivers/media/i2c/st-mipid02.c | 11 +- drivers/media/i2c/st-vgxy61.c | 5 +- drivers/media/i2c/tc358746.c | 12 +- drivers/media/i2c/tda1997x.c | 6 +- drivers/media/i2c/tvp5150.c | 2 +- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +- drivers/media/pci/intel/ivsc/mei_csi.c | 4 +- drivers/media/platform/cadence/cdns-csi2rx.c | 4 +- drivers/media/platform/cadence/cdns-csi2tx.c | 3 +- .../platform/microchip/microchip-csi2dc.c | 15 ++- .../platform/microchip/microchip-isc-scaler.c | 17 +-- drivers/media/platform/nxp/imx-mipi-csis.c | 10 +- drivers/media/platform/nxp/imx7-media-csi.c | 15 ++- .../platform/nxp/imx8-isi/imx8-isi-pipe.c | 18 +-- .../platform/nxp/imx8-isi/imx8-isi-video.c | 2 +- drivers/media/platform/nxp/imx8mq-mipi-csi2.c | 12 +- .../media/platform/qcom/camss/camss-csid.c | 3 +- .../media/platform/qcom/camss/camss-csiphy.c | 3 +- .../media/platform/qcom/camss/camss-ispif.c | 3 +- drivers/media/platform/qcom/camss/camss-vfe.c | 9 +- drivers/media/platform/renesas/rcar-isp.c | 4 +- .../platform/renesas/rcar-vin/rcar-csi2.c | 4 +- .../platform/renesas/rzg2l-cru/rzg2l-csi2.c | 6 +- .../platform/renesas/rzg2l-cru/rzg2l-ip.c | 6 +- .../media/platform/renesas/vsp1/vsp1_brx.c | 2 +- .../media/platform/renesas/vsp1/vsp1_entity.c | 8 +- .../media/platform/renesas/vsp1/vsp1_rwpf.c | 3 +- .../platform/rockchip/rkisp1/rkisp1-csi.c | 16 ++- .../platform/rockchip/rkisp1/rkisp1-isp.c | 103 ++++++++--------- .../platform/rockchip/rkisp1/rkisp1-resizer.c | 53 ++++----- .../samsung/exynos4-is/fimc-capture.c | 12 +- .../platform/samsung/exynos4-is/fimc-isp.c | 23 ++-- .../platform/samsung/exynos4-is/fimc-lite.c | 17 ++- .../platform/samsung/exynos4-is/mipi-csis.c | 3 +- .../samsung/s3c-camif/camif-capture.c | 8 +- .../platform/sunxi/sun4i-csi/sun4i_v4l2.c | 8 +- .../sunxi/sun6i-csi/sun6i_csi_bridge.c | 7 +- .../sunxi/sun6i-mipi-csi2/sun6i_mipi_csi2.c | 7 +- .../sun8i_a83t_mipi_csi2.c | 7 +- drivers/media/platform/ti/cal/cal-camerarx.c | 13 +-- drivers/media/platform/ti/cal/cal-video.c | 2 +- drivers/media/platform/ti/omap3isp/ispccdc.c | 6 +- drivers/media/platform/ti/omap3isp/ispccp2.c | 3 +- drivers/media/platform/ti/omap3isp/ispcsi2.c | 3 +- .../media/platform/ti/omap3isp/isppreview.c | 6 +- .../media/platform/ti/omap3isp/ispresizer.c | 5 +- drivers/media/platform/video-mux.c | 18 +-- .../media/platform/xilinx/xilinx-csi2rxss.c | 5 +- drivers/media/platform/xilinx/xilinx-tpg.c | 9 +- drivers/media/platform/xilinx/xilinx-vip.c | 4 +- .../media/test-drivers/vimc/vimc-debayer.c | 10 +- drivers/media/test-drivers/vimc/vimc-scaler.c | 9 +- drivers/media/test-drivers/vimc/vimc-sensor.c | 6 +- drivers/media/v4l2-core/v4l2-subdev.c | 73 ++++++++---- .../media/atomisp/i2c/atomisp-gc0310.c | 2 +- .../staging/media/atomisp/pci/atomisp_csi2.c | 3 +- .../media/atomisp/pci/atomisp_subdev.c | 6 +- drivers/staging/media/imx/imx-ic-prp.c | 2 +- drivers/staging/media/imx/imx-ic-prpencvf.c | 2 +- drivers/staging/media/imx/imx-media-csi.c | 8 +- drivers/staging/media/imx/imx-media-utils.c | 2 +- drivers/staging/media/imx/imx-media-vdic.c | 2 +- drivers/staging/media/imx/imx6-mipi-csi2.c | 2 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 14 +-- drivers/staging/media/omap4iss/iss_csi2.c | 3 +- drivers/staging/media/omap4iss/iss_ipipe.c | 3 +- drivers/staging/media/omap4iss/iss_ipipeif.c | 3 +- drivers/staging/media/omap4iss/iss_resizer.c | 3 +- .../media/sunxi/sun6i-isp/sun6i_isp_proc.c | 7 +- include/media/v4l2-subdev.h | 107 +++--------------- 136 files changed, 597 insertions(+), 751 deletions(-)