diff mbox series

[1/1] media: v4l: subdev: Always compile sub-device state access functions

Message ID 20231110101049.890577-1-sakari.ailus@linux.intel.com
State New
Headers show
Series [1/1] media: v4l: subdev: Always compile sub-device state access functions | expand

Commit Message

Sakari Ailus Nov. 10, 2023, 10:10 a.m. UTC 
Compile sub-device state information access functions
v4l2_subdev_state_get_{format,crop,compose} unconditionally as they are
now also used by plain V4L2 drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 204 +++++++++++++-------------
 include/media/v4l2-subdev.h           | 128 ++++++++--------
 2 files changed, 166 insertions(+), 166 deletions(-)

Comments

Laurent Pinchart Nov. 12, 2023, 1:22 a.m. UTC | #1 
Hi Sakari,

On Fri, Nov 10, 2023 at 09:15:52PM +0000, Sakari Ailus wrote:
> On Fri, Nov 10, 2023 at 05:39:40PM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 10, 2023 at 12:10:49PM +0200, Sakari Ailus wrote:
> > > Compile sub-device state information access functions
> > > v4l2_subdev_state_get_{format,crop,compose} unconditionally as they are
> > > now also used by plain V4L2 drivers.
> > 
> > What do you mean by "plain" V4L2 drivers here ?
> 
> Those that do not need MC: if you now disable MC in kernel configuration,
> these functions won't be available.

This covers subdev drivers (such as sensor drivers) too, not just host
drivers ? If so, shouldn't we also drop the dependency on
VIDEO_V4L2_SUBDEV_API from drivers/media/i2c/Kconfig ?
Sakari Ailus Nov. 12, 2023, 8:32 p.m. UTC | #2 
Hi Laurent,

On Sun, Nov 12, 2023 at 03:22:46AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Fri, Nov 10, 2023 at 09:15:52PM +0000, Sakari Ailus wrote:
> > On Fri, Nov 10, 2023 at 05:39:40PM +0200, Laurent Pinchart wrote:
> > > On Fri, Nov 10, 2023 at 12:10:49PM +0200, Sakari Ailus wrote:
> > > > Compile sub-device state information access functions
> > > > v4l2_subdev_state_get_{format,crop,compose} unconditionally as they are
> > > > now also used by plain V4L2 drivers.
> > > 
> > > What do you mean by "plain" V4L2 drivers here ?
> > 
> > Those that do not need MC: if you now disable MC in kernel configuration,
> > these functions won't be available.
> 
> This covers subdev drivers (such as sensor drivers) too, not just host
> drivers ? If so, shouldn't we also drop the dependency on
> VIDEO_V4L2_SUBDEV_API from drivers/media/i2c/Kconfig ?

Good question.

There are still other functions that a number of drivers depend on which
are only available when either MC or sub-device API are enabled.

Given that almost all receiver drivers today use MC, I don't think there
would be much to be gained from enabling building e.g. sensor drivers
without sub-device UAPI.
Laurent Pinchart Nov. 13, 2023, 1:23 a.m. UTC | #3 
On Sun, Nov 12, 2023 at 08:32:33PM +0000, Sakari Ailus wrote:
> On Sun, Nov 12, 2023 at 03:22:46AM +0200, Laurent Pinchart wrote:
> > On Fri, Nov 10, 2023 at 09:15:52PM +0000, Sakari Ailus wrote:
> > > On Fri, Nov 10, 2023 at 05:39:40PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Nov 10, 2023 at 12:10:49PM +0200, Sakari Ailus wrote:
> > > > > Compile sub-device state information access functions
> > > > > v4l2_subdev_state_get_{format,crop,compose} unconditionally as they are
> > > > > now also used by plain V4L2 drivers.
> > > > 
> > > > What do you mean by "plain" V4L2 drivers here ?
> > > 
> > > Those that do not need MC: if you now disable MC in kernel configuration,
> > > these functions won't be available.
> > 
> > This covers subdev drivers (such as sensor drivers) too, not just host
> > drivers ? If so, shouldn't we also drop the dependency on
> > VIDEO_V4L2_SUBDEV_API from drivers/media/i2c/Kconfig ?
> 
> Good question.
> 
> There are still other functions that a number of drivers depend on which
> are only available when either MC or sub-device API are enabled.
> 
> Given that almost all receiver drivers today use MC, I don't think there
> would be much to be gained from enabling building e.g. sensor drivers
> without sub-device UAPI.

I would like to refocus VIDEO_V4L2_SUBDEV_API to cover the UAPI only.
Trying to be too clever with the Kconfig symbol doesn't help.
Sakari Ailus Nov. 13, 2023, 7:08 a.m. UTC | #4 
On Mon, Nov 13, 2023 at 03:23:15AM +0200, Laurent Pinchart wrote:
> On Sun, Nov 12, 2023 at 08:32:33PM +0000, Sakari Ailus wrote:
> > On Sun, Nov 12, 2023 at 03:22:46AM +0200, Laurent Pinchart wrote:
> > > On Fri, Nov 10, 2023 at 09:15:52PM +0000, Sakari Ailus wrote:
> > > > On Fri, Nov 10, 2023 at 05:39:40PM +0200, Laurent Pinchart wrote:
> > > > > On Fri, Nov 10, 2023 at 12:10:49PM +0200, Sakari Ailus wrote:
> > > > > > Compile sub-device state information access functions
> > > > > > v4l2_subdev_state_get_{format,crop,compose} unconditionally as they are
> > > > > > now also used by plain V4L2 drivers.
> > > > > 
> > > > > What do you mean by "plain" V4L2 drivers here ?
> > > > 
> > > > Those that do not need MC: if you now disable MC in kernel configuration,
> > > > these functions won't be available.
> > > 
> > > This covers subdev drivers (such as sensor drivers) too, not just host
> > > drivers ? If so, shouldn't we also drop the dependency on
> > > VIDEO_V4L2_SUBDEV_API from drivers/media/i2c/Kconfig ?
> > 
> > Good question.
> > 
> > There are still other functions that a number of drivers depend on which
> > are only available when either MC or sub-device API are enabled.
> > 
> > Given that almost all receiver drivers today use MC, I don't think there
> > would be much to be gained from enabling building e.g. sensor drivers
> > without sub-device UAPI.
> 
> I would like to refocus VIDEO_V4L2_SUBDEV_API to cover the UAPI only.
> Trying to be too clever with the Kconfig symbol doesn't help.

I guess this has been the intention, with the inclusion of other functions
and definitions that are only needed for sub-device UAPI. This poses some
issues for drivers that can work in both cases but there aren't many of
them. I'm fine with removing the latter.
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 57ef4fce1186..efb39172b20f 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1524,6 +1524,108 @@  void v4l2_subdev_cleanup(struct v4l2_subdev *sd)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_cleanup);
 
+struct v4l2_mbus_framefmt *
+__v4l2_subdev_state_get_format(struct v4l2_subdev_state *state,
+			       unsigned int pad, u32 stream)
+{
+	struct v4l2_subdev_stream_configs *stream_configs;
+	unsigned int i;
+
+	if (WARN_ON_ONCE(!state))
+		return NULL;
+
+	if (state->pads) {
+		if (stream)
+			return NULL;
+
+		if (pad >= state->sd->entity.num_pads)
+			return NULL;
+
+		return &state->pads[pad].format;
+	}
+
+	lockdep_assert_held(state->lock);
+
+	stream_configs = &state->stream_configs;
+
+	for (i = 0; i < stream_configs->num_configs; ++i) {
+		if (stream_configs->configs[i].pad == pad &&
+		    stream_configs->configs[i].stream == stream)
+			return &stream_configs->configs[i].fmt;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_format);
+
+struct v4l2_rect *
+__v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
+			     u32 stream)
+{
+	struct v4l2_subdev_stream_configs *stream_configs;
+	unsigned int i;
+
+	if (WARN_ON_ONCE(!state))
+		return NULL;
+
+	if (state->pads) {
+		if (stream)
+			return NULL;
+
+		if (pad >= state->sd->entity.num_pads)
+			return NULL;
+
+		return &state->pads[pad].crop;
+	}
+
+	lockdep_assert_held(state->lock);
+
+	stream_configs = &state->stream_configs;
+
+	for (i = 0; i < stream_configs->num_configs; ++i) {
+		if (stream_configs->configs[i].pad == pad &&
+		    stream_configs->configs[i].stream == stream)
+			return &stream_configs->configs[i].crop;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_crop);
+
+struct v4l2_rect *
+__v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state,
+				unsigned int pad, u32 stream)
+{
+	struct v4l2_subdev_stream_configs *stream_configs;
+	unsigned int i;
+
+	if (WARN_ON_ONCE(!state))
+		return NULL;
+
+	if (state->pads) {
+		if (stream)
+			return NULL;
+
+		if (pad >= state->sd->entity.num_pads)
+			return NULL;
+
+		return &state->pads[pad].compose;
+	}
+
+	lockdep_assert_held(state->lock);
+
+	stream_configs = &state->stream_configs;
+
+	for (i = 0; i < stream_configs->num_configs; ++i) {
+		if (stream_configs->configs[i].pad == pad &&
+		    stream_configs->configs[i].stream == stream)
+			return &stream_configs->configs[i].compose;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_compose);
+
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 
 static int
@@ -1670,108 +1772,6 @@  int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_set_routing_with_fmt);
 
-struct v4l2_mbus_framefmt *
-__v4l2_subdev_state_get_format(struct v4l2_subdev_state *state,
-			       unsigned int pad, u32 stream)
-{
-	struct v4l2_subdev_stream_configs *stream_configs;
-	unsigned int i;
-
-	if (WARN_ON_ONCE(!state))
-		return NULL;
-
-	if (state->pads) {
-		if (stream)
-			return NULL;
-
-		if (pad >= state->sd->entity.num_pads)
-			return NULL;
-
-		return &state->pads[pad].format;
-	}
-
-	lockdep_assert_held(state->lock);
-
-	stream_configs = &state->stream_configs;
-
-	for (i = 0; i < stream_configs->num_configs; ++i) {
-		if (stream_configs->configs[i].pad == pad &&
-		    stream_configs->configs[i].stream == stream)
-			return &stream_configs->configs[i].fmt;
-	}
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_format);
-
-struct v4l2_rect *
-__v4l2_subdev_state_get_crop(struct v4l2_subdev_state *state, unsigned int pad,
-			     u32 stream)
-{
-	struct v4l2_subdev_stream_configs *stream_configs;
-	unsigned int i;
-
-	if (WARN_ON_ONCE(!state))
-		return NULL;
-
-	if (state->pads) {
-		if (stream)
-			return NULL;
-
-		if (pad >= state->sd->entity.num_pads)
-			return NULL;
-
-		return &state->pads[pad].crop;
-	}
-
-	lockdep_assert_held(state->lock);
-
-	stream_configs = &state->stream_configs;
-
-	for (i = 0; i < stream_configs->num_configs; ++i) {
-		if (stream_configs->configs[i].pad == pad &&
-		    stream_configs->configs[i].stream == stream)
-			return &stream_configs->configs[i].crop;
-	}
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_crop);
-
-struct v4l2_rect *
-__v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state,
-				unsigned int pad, u32 stream)
-{
-	struct v4l2_subdev_stream_configs *stream_configs;
-	unsigned int i;
-
-	if (WARN_ON_ONCE(!state))
-		return NULL;
-
-	if (state->pads) {
-		if (stream)
-			return NULL;
-
-		if (pad >= state->sd->entity.num_pads)
-			return NULL;
-
-		return &state->pads[pad].compose;
-	}
-
-	lockdep_assert_held(state->lock);
-
-	stream_configs = &state->stream_configs;
-
-	for (i = 0; i < stream_configs->num_configs; ++i) {
-		if (stream_configs->configs[i].pad == pad &&
-		    stream_configs->configs[i].stream == stream)
-			return &stream_configs->configs[i].compose;
-	}
-
-	return NULL;
-}
-EXPORT_SYMBOL_GPL(__v4l2_subdev_state_get_compose);
-
 int v4l2_subdev_routing_find_opposite_end(const struct v4l2_subdev_krouting *routing,
 					  u32 pad, u32 stream, u32 *other_pad,
 					  u32 *other_stream)
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index b1bad946d813..33c8e5c93a3d 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1393,70 +1393,6 @@  v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
 	return sd->active_state;
 }
 
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
-
-/**
- * v4l2_subdev_get_fmt() - Fill format based on state
- * @sd: subdevice
- * @state: subdevice state
- * @format: pointer to &struct v4l2_subdev_format
- *
- * Fill @format->format field based on the information in the @format struct.
- *
- * This function can be used by the subdev drivers which support active state to
- * implement v4l2_subdev_pad_ops.get_fmt if the subdev driver does not need to
- * do anything special in their get_fmt op.
- *
- * Returns 0 on success, error value otherwise.
- */
-int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
-			struct v4l2_subdev_format *format);
-
-/**
- * v4l2_subdev_set_routing() - Set given routing to subdev state
- * @sd: The subdevice
- * @state: The subdevice state
- * @routing: Routing that will be copied to subdev state
- *
- * This will release old routing table (if any) from the state, allocate
- * enough space for the given routing, and copy the routing.
- *
- * This can be used from the subdev driver's set_routing op, after validating
- * the routing.
- */
-int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
-			    struct v4l2_subdev_state *state,
-			    const struct v4l2_subdev_krouting *routing);
-
-struct v4l2_subdev_route *
-__v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing,
-				struct v4l2_subdev_route *route);
-
-/**
- * for_each_active_route - iterate on all active routes of a routing table
- * @routing: The routing table
- * @route: The route iterator
- */
-#define for_each_active_route(routing, route) \
-	for ((route) = NULL;                  \
-	     ((route) = __v4l2_subdev_next_active_route((routing), (route)));)
-
-/**
- * v4l2_subdev_set_routing_with_fmt() - Set given routing and format to subdev
- *					state
- * @sd: The subdevice
- * @state: The subdevice state
- * @routing: Routing that will be copied to subdev state
- * @fmt: Format used to initialize all the streams
- *
- * This is the same as v4l2_subdev_set_routing, but additionally initializes
- * all the streams using the given format.
- */
-int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
-				     struct v4l2_subdev_state *state,
-				     const struct v4l2_subdev_krouting *routing,
-				     const struct v4l2_mbus_framefmt *fmt);
-
 /*
  * A macro to generate the macro or function name for sub-devices state access
  * wrapper macros below.
@@ -1533,6 +1469,70 @@  struct v4l2_rect *
 __v4l2_subdev_state_get_compose(struct v4l2_subdev_state *state,
 				unsigned int pad, u32 stream);
 
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+
+/**
+ * v4l2_subdev_get_fmt() - Fill format based on state
+ * @sd: subdevice
+ * @state: subdevice state
+ * @format: pointer to &struct v4l2_subdev_format
+ *
+ * Fill @format->format field based on the information in the @format struct.
+ *
+ * This function can be used by the subdev drivers which support active state to
+ * implement v4l2_subdev_pad_ops.get_fmt if the subdev driver does not need to
+ * do anything special in their get_fmt op.
+ *
+ * Returns 0 on success, error value otherwise.
+ */
+int v4l2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_state *state,
+			struct v4l2_subdev_format *format);
+
+/**
+ * v4l2_subdev_set_routing() - Set given routing to subdev state
+ * @sd: The subdevice
+ * @state: The subdevice state
+ * @routing: Routing that will be copied to subdev state
+ *
+ * This will release old routing table (if any) from the state, allocate
+ * enough space for the given routing, and copy the routing.
+ *
+ * This can be used from the subdev driver's set_routing op, after validating
+ * the routing.
+ */
+int v4l2_subdev_set_routing(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state,
+			    const struct v4l2_subdev_krouting *routing);
+
+struct v4l2_subdev_route *
+__v4l2_subdev_next_active_route(const struct v4l2_subdev_krouting *routing,
+				struct v4l2_subdev_route *route);
+
+/**
+ * for_each_active_route - iterate on all active routes of a routing table
+ * @routing: The routing table
+ * @route: The route iterator
+ */
+#define for_each_active_route(routing, route) \
+	for ((route) = NULL;                  \
+	     ((route) = __v4l2_subdev_next_active_route((routing), (route)));)
+
+/**
+ * v4l2_subdev_set_routing_with_fmt() - Set given routing and format to subdev
+ *					state
+ * @sd: The subdevice
+ * @state: The subdevice state
+ * @routing: Routing that will be copied to subdev state
+ * @fmt: Format used to initialize all the streams
+ *
+ * This is the same as v4l2_subdev_set_routing, but additionally initializes
+ * all the streams using the given format.
+ */
+int v4l2_subdev_set_routing_with_fmt(struct v4l2_subdev *sd,
+				     struct v4l2_subdev_state *state,
+				     const struct v4l2_subdev_krouting *routing,
+				     const struct v4l2_mbus_framefmt *fmt);
+
 /**
  * v4l2_subdev_routing_find_opposite_end() - Find the opposite stream
  * @routing: routing used to find the opposite side