[PATCH 0/2] media: Make v4l2_subdev_stream_config private

[PATCH 0/2] media: Make v4l2_subdev_stream_config private

[Laurent Pinchart]

Hello,

This small patch series attempts to avoid usage of the
v4l2_subdev_stream_config structure in drivers. The rationale is that
the structure was meant to be an implementation detail. Recent
discussions about how we store stream config led to considering a need
to rework the internals, and usage of the structure in drivers makes
this more difficult.

Patch 1/2 drops usage of the structure in the ds90ub913 driver, and
patch 2/2 then makes the structure private to v4l2-subdev.c.

Ideally we should also make v4l2_subdev_pad_config private, but it has a
few more users:

drivers/media/pci/saa7134/saa7134-empress.c
drivers/media/platform/atmel/atmel-isi.c
drivers/media/platform/intel/pxa_camera.c
drivers/media/platform/marvell/mcam-core.c
drivers/media/platform/renesas/renesas-ceu.c
drivers/media/platform/via/via-camera.c
drivers/staging/media/deprecated/atmel/atmel-isc-base.c

All those drivers are video-centric drivers that use
v4l2_subdev_pad_config to implement TRY_FMT. I think we could use
v4l2_subdev_call_state_try() in those drivers, like done in

commit f076057f0107c3ef890bfad8d6658387504e7f11
Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date:   Fri Jul 1 14:15:59 2022 +0100

    media: stm32: dcmi: Fix subdev op call with uninitialized state

I however lack the ability to test that beside compile testing.

Laurent Pinchart (2):
  media: i2c: ds90ub913: Stop accessing streams configs directly
  media: v4l2-subdev: Make struct v4l2_subdev_stream_config private

 drivers/media/i2c/ds90ub913.c         | 17 +++++++++--------
 drivers/media/v4l2-core/v4l2-subdev.c | 24 ++++++++++++++++++++++++
 include/media/v4l2-subdev.h           | 25 +------------------------
 3 files changed, 34 insertions(+), 32 deletions(-)


base-commit: c0b1da281d84d33281fc49289f0c7f8aada450ff
prerequisite-patch-id: e09080849e2b6fabfc70eb12b5c99c42111d3823
-- 
Regards,

Laurent Pinchart

[PATCH 1/2] media: i2c: ds90ub913: Stop accessing streams configs directly

[Laurent Pinchart]

The v4l2_subdev_stream_config structure will be made private. Stop
accessing it directly, iterate over routes instead to initialize
formats.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/ds90ub913.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ds90ub913.c b/drivers/media/i2c/ds90ub913.c
index c34ffeae5f50..8c94e91ab1cc 100644
--- a/drivers/media/i2c/ds90ub913.c
+++ b/drivers/media/i2c/ds90ub913.c
@@ -333,8 +333,7 @@ static int _ub913_set_routing(struct v4l2_subdev *sd,
 		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
 		.xfer_func = V4L2_XFER_FUNC_SRGB,
 	};
-	struct v4l2_subdev_stream_configs *stream_configs;
-	unsigned int i;
+	struct v4l2_subdev_route *route;
 	int ret;
 
 	/*
@@ -354,13 +353,15 @@ static int _ub913_set_routing(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	stream_configs = &state->stream_configs;
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_framefmt *fmt;
 
-	for (i = 0; i < stream_configs->num_configs; i++) {
-		if (stream_configs->configs[i].pad == UB913_PAD_SINK)
-			stream_configs->configs[i].fmt = in_format;
-		else
-			stream_configs->configs[i].fmt = out_format;
+		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
+						   route->sink_stream);
+		*fmt = in_format;
+		fmt = v4l2_subdev_state_get_format(state, route->source_pad,
+						   route->source_stream);
+		*fmt = out_format;
 	}
 
 	return 0;
-- 
Regards,

Laurent Pinchart

[PATCH 2/2] media: v4l2-subdev: Make struct v4l2_subdev_stream_config private

[Laurent Pinchart]

The v4l2_subdev_stream_config structure holds configuration data for a
stream. It was meant to be used internally only, but already found its
way into the ds90ub913 driver. Now that the driver has been fixed, make
the structure private to v4l2-subdev.c to avoid using it by accident.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 24 ++++++++++++++++++++++++
 include/media/v4l2-subdev.h           | 25 +------------------------
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index a3074f469b15..bc813f25c0d2 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -26,6 +26,30 @@
 #include <media/v4l2-fh.h>
 #include <media/v4l2-ioctl.h>
 
+/**
+ * struct v4l2_subdev_stream_config - Used for storing stream configuration.
+ *
+ * @pad: pad number
+ * @stream: stream number
+ * @enabled: has the stream been enabled with v4l2_subdev_enable_streams()
+ * @fmt: &struct v4l2_mbus_framefmt
+ * @crop: &struct v4l2_rect to be used for crop
+ * @compose: &struct v4l2_rect to be used for compose
+ * @interval: frame interval
+ *
+ * This structure stores configuration for a stream.
+ */
+struct v4l2_subdev_stream_config {
+	u32 pad;
+	u32 stream;
+	bool enabled;
+
+	struct v4l2_mbus_framefmt fmt;
+	struct v4l2_rect crop;
+	struct v4l2_rect compose;
+	struct v4l2_fract interval;
+};
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 /*
  * The Streams API is an experimental feature. To use the Streams API, set
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 57f2bcb4eb16..b867d3a96a51 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -36,6 +36,7 @@ struct v4l2_event_subscription;
 struct v4l2_fh;
 struct v4l2_subdev;
 struct v4l2_subdev_fh;
+struct v4l2_subdev_stream_config;
 struct tuner_setup;
 struct v4l2_mbus_frame_desc;
 struct led_classdev;
@@ -686,30 +687,6 @@ struct v4l2_subdev_pad_config {
 	struct v4l2_fract interval;
 };
 
-/**
- * struct v4l2_subdev_stream_config - Used for storing stream configuration.
- *
- * @pad: pad number
- * @stream: stream number
- * @enabled: has the stream been enabled with v4l2_subdev_enable_streams()
- * @fmt: &struct v4l2_mbus_framefmt
- * @crop: &struct v4l2_rect to be used for crop
- * @compose: &struct v4l2_rect to be used for compose
- * @interval: frame interval
- *
- * This structure stores configuration for a stream.
- */
-struct v4l2_subdev_stream_config {
-	u32 pad;
-	u32 stream;
-	bool enabled;
-
-	struct v4l2_mbus_framefmt fmt;
-	struct v4l2_rect crop;
-	struct v4l2_rect compose;
-	struct v4l2_fract interval;
-};
-
 /**
  * struct v4l2_subdev_stream_configs - A collection of stream configs.
  *
-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/2] media: Make v4l2_subdev_stream_config private

[Jacopo Mondi]

Hi Laurent

On Mon, Jun 30, 2025 at 03:46:00AM +0300, Laurent Pinchart wrote:
> Hello,
>
> This small patch series attempts to avoid usage of the
> v4l2_subdev_stream_config structure in drivers. The rationale is that
> the structure was meant to be an implementation detail. Recent
> discussions about how we store stream config led to considering a need
> to rework the internals, and usage of the structure in drivers makes
> this more difficult.
>
> Patch 1/2 drops usage of the structure in the ds90ub913 driver, and
> patch 2/2 then makes the structure private to v4l2-subdev.c.
>
> Ideally we should also make v4l2_subdev_pad_config private, but it has a
> few more users:
>
> drivers/media/pci/saa7134/saa7134-empress.c
> drivers/media/platform/atmel/atmel-isi.c
> drivers/media/platform/intel/pxa_camera.c
> drivers/media/platform/marvell/mcam-core.c
> drivers/media/platform/renesas/renesas-ceu.c
> drivers/media/platform/via/via-camera.c
> drivers/staging/media/deprecated/atmel/atmel-isc-base.c
>
> All those drivers are video-centric drivers that use
> v4l2_subdev_pad_config to implement TRY_FMT. I think we could use
> v4l2_subdev_call_state_try() in those drivers, like done in
>
> commit f076057f0107c3ef890bfad8d6658387504e7f11
> Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Date:   Fri Jul 1 14:15:59 2022 +0100
>
>     media: stm32: dcmi: Fix subdev op call with uninitialized state
>
> I however lack the ability to test that beside compile testing.
>
> Laurent Pinchart (2):
>   media: i2c: ds90ub913: Stop accessing streams configs directly
>   media: v4l2-subdev: Make struct v4l2_subdev_stream_config private
>

Sorry, the series went ignored...

For both patches
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>  drivers/media/i2c/ds90ub913.c         | 17 +++++++++--------
>  drivers/media/v4l2-core/v4l2-subdev.c | 24 ++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           | 25 +------------------------
>  3 files changed, 34 insertions(+), 32 deletions(-)
>
>
> base-commit: c0b1da281d84d33281fc49289f0c7f8aada450ff
> prerequisite-patch-id: e09080849e2b6fabfc70eb12b5c99c42111d3823
> --
> Regards,
>
> Laurent Pinchart
>
>

Re: [PATCH 0/2] media: Make v4l2_subdev_stream_config private

[Laurent Pinchart]

On Mon, Jul 28, 2025 at 04:04:10PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 30, 2025 at 03:46:00AM +0300, Laurent Pinchart wrote:
> > Hello,
> >
> > This small patch series attempts to avoid usage of the
> > v4l2_subdev_stream_config structure in drivers. The rationale is that
> > the structure was meant to be an implementation detail. Recent
> > discussions about how we store stream config led to considering a need
> > to rework the internals, and usage of the structure in drivers makes
> > this more difficult.
> >
> > Patch 1/2 drops usage of the structure in the ds90ub913 driver, and
> > patch 2/2 then makes the structure private to v4l2-subdev.c.
> >
> > Ideally we should also make v4l2_subdev_pad_config private, but it has a
> > few more users:
> >
> > drivers/media/pci/saa7134/saa7134-empress.c
> > drivers/media/platform/atmel/atmel-isi.c
> > drivers/media/platform/intel/pxa_camera.c
> > drivers/media/platform/marvell/mcam-core.c
> > drivers/media/platform/renesas/renesas-ceu.c
> > drivers/media/platform/via/via-camera.c
> > drivers/staging/media/deprecated/atmel/atmel-isc-base.c
> >
> > All those drivers are video-centric drivers that use
> > v4l2_subdev_pad_config to implement TRY_FMT. I think we could use
> > v4l2_subdev_call_state_try() in those drivers, like done in
> >
> > commit f076057f0107c3ef890bfad8d6658387504e7f11
> > Author: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > Date:   Fri Jul 1 14:15:59 2022 +0100
> >
> >     media: stm32: dcmi: Fix subdev op call with uninitialized state
> >
> > I however lack the ability to test that beside compile testing.
> >
> > Laurent Pinchart (2):
> >   media: i2c: ds90ub913: Stop accessing streams configs directly
> >   media: v4l2-subdev: Make struct v4l2_subdev_stream_config private
> >
> 
> Sorry, the series went ignored...

And of course in the meantime another user appeared in
drivers/staging/media/ipu7/ipu7-isys-video.c *sigh*

I'll see if I can send a v2 to address this.

> For both patches
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>   j
> 
> >  drivers/media/i2c/ds90ub913.c         | 17 +++++++++--------
> >  drivers/media/v4l2-core/v4l2-subdev.c | 24 ++++++++++++++++++++++++
> >  include/media/v4l2-subdev.h           | 25 +------------------------
> >  3 files changed, 34 insertions(+), 32 deletions(-)
> >
> >
> > base-commit: c0b1da281d84d33281fc49289f0c7f8aada450ff
> > prerequisite-patch-id: e09080849e2b6fabfc70eb12b5c99c42111d3823

-- 
Regards,

Laurent Pinchart