[PATCH 0/3] media: subdev: v4l2_subdev_get_frame_desc_passthrough improvements

[PATCH 0/3] media: subdev: v4l2_subdev_get_frame_desc_passthrough improvements

[Tomi Valkeinen]

Improve v4l2_subdev_get_frame_desc_passthrough() kernel doc, minor
cleanups, and a "locked" version.

The last patch can be left for later, if there's anything controversial
there, as my need for it (UB953 TPG) depends on internal pads which are
not in upstream.

Note: This is based on Sakari's 'for-7.1-1.1-signed' pull request.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
Tomi Valkeinen (3):
      media: subdev: Improve v4l2_subdev_get_frame_desc_passthrough() kdoc
      media: subdev: Minor v4l2_subdev_get_frame_desc_passthrough() cleanups
      media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

 drivers/media/v4l2-core/v4l2-subdev.c | 56 +++++++++++++++++++----------------
 include/media/v4l2-subdev.h           | 50 ++++++++++++++++++++++++++-----
 2 files changed, 72 insertions(+), 34 deletions(-)
---
base-commit: 621887bc7b9a6c37a5ed3b1f24dd446e3034253e
change-id: 20260312-frame-desc-passthrough-impro-6a9ccb32d9a5

Best regards,
-- 
Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

[PATCH 1/3] media: subdev: Improve v4l2_subdev_get_frame_desc_passthrough() kdoc

[Tomi Valkeinen]

Improve the v4l2_subdev_get_frame_desc_passthrough() kernel doc:

- Fix 'v4l2_get_frame_desc' operation to 'get_frame_desc' operation
- Rewrite the body text to be more understandable and specific, and
  specifically mention the frame desc type handling.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 include/media/v4l2-subdev.h | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index e754ed3421c5..23c03ba7f84c 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1724,19 +1724,25 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
 
 /**
  * v4l2_subdev_get_frame_desc_passthrough() - Helper to implement the subdev
- *	v4l2_get_frame_desc operation in simple passthrough cases
+ *	get_frame_desc operation in simple passthrough cases
  * @sd: The subdevice
  * @pad: The source pad index
  * @fd: The mbus frame desc
  *
- * Subdevice drivers that only pass through the streams can use this helper
- * to implement the &v4l2_subdev_pad_ops.v4l2_get_frame_desc operation.
+ * This helper implements get_frame_desc operation for subdevices that pass
+ * streams through without modification. It can be assigned directly as the
+ * .get_frame_desc callback in &v4l2_subdev_pad_ops.
  *
- * The helper will call get_frame_desc on the subdevice's sources, create a new
- * frame desc which contains only the streams on the given source pad. The data
- * for each frame desc entry is copied directly from the data provided from the
- * calls to the subdevice's sources, with the exception of the 'stream' field
- * which is set according to the subdevice's routing table.
+ * The helper iterates over the subdevice's sink pads, calls get_frame_desc on
+ * the remote subdevice connected to each sink pad, and collects the frame desc
+ * entries for streams that are routed to the given source pad according to the
+ * subdevice's routing table. Each entry is copied as-is from the upstream
+ * source, with the exception of the 'stream' field which is remapped to the
+ * source stream ID from the routing table.
+ *
+ * The frame desc type is taken from the first upstream source. If multiple
+ * sink pads are involved and the upstream sources report different frame desc
+ * types, -EPIPE is returned.
  *
  * Return: 0 on success, or a negative error code otherwise.
  */

-- 
2.43.0

[PATCH 2/3] media: subdev: Minor v4l2_subdev_get_frame_desc_passthrough() cleanups

[Tomi Valkeinen]

Minor code cleanups, no functional change.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 9efd14d4026f..2757378c628a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2549,14 +2549,13 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 					   unsigned int pad,
 					   struct v4l2_mbus_frame_desc *fd)
 {
-	const struct media_pad *pads = sd->entity.pads;
 	struct media_pad *local_sink_pad;
 	struct v4l2_subdev_route *route;
 	struct v4l2_subdev_state *state;
 	struct device *dev = sd->dev;
 	int ret = 0;
 
-	if (WARN_ON(!(pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
+	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
 		return -EINVAL;
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
@@ -2577,7 +2576,6 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 			struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
 			struct media_pad *remote_source_pad;
 			struct v4l2_subdev *remote_sd;
-			unsigned int i;
 
 			if (route->source_pad != pad ||
 			    route->sink_pad != local_sink_pad->index)
@@ -2622,7 +2620,7 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 				}
 			}
 
-			for (i = 0; i < source_fd.num_entries; i++) {
+			for (unsigned int i = 0; i < source_fd.num_entries; i++) {
 				if (source_fd.entry[i].stream == route->sink_stream) {
 					source_entry = &source_fd.entry[i];
 					break;
@@ -2630,7 +2628,7 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 			}
 
 			if (!source_entry) {
-				dev_dbg(sd->dev,
+				dev_dbg(dev,
 					"Failed to find stream %u from source frame desc\n",
 					route->sink_stream);
 				ret = -EPIPE;
@@ -2638,7 +2636,7 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 			}
 
 			if (fd->num_entries >= V4L2_FRAME_DESC_ENTRY_MAX) {
-				dev_dbg(sd->dev, "Frame desc entry limit reached\n");
+				dev_dbg(dev, "Frame desc entry limit reached\n");
 				ret = -ENOSPC;
 				goto out_unlock;
 			}

-- 
2.43.0

[PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Tomi Valkeinen]

The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
directly as an implementation for .get_frame_desc subdev op. However, in
some cases the drivers may want to add some customizations, while the
bulk of the work is still identical to what
v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
makes this impossible to do properly.

Split v4l2_subdev_get_frame_desc_passthrough() into two functions:

v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
subdev state as a parameter, instead of locking and getting the active
state internally. Other than that, it does the same as
v4l2_subdev_get_frame_desc_passthrough() used to do.

v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
and calls v4l2_subdev_get_frame_desc_passthrough_locked().

In other words, v4l2_subdev_get_frame_desc_passthrough() works as
before, but drivers can now alternatively add custom .get_frame_desc
code and call v4l2_subdev_get_frame_desc_passthrough().

An example use case is with DS90UB953 serializer: in normal use the
serializer passes through everything, but when test-pattern-generator
(TPG) is used, an internal TPG source is used. After this commit, the
UB953 get_frame_desc() can lock the state, look at the routing table to
see if we're in normal or TPG mode, then either call
v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
construct a TPG frame desc if in TPG mode.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
 include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
 2 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 2757378c628a..8b1a7f00c86b 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
 }
 EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
 
-int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
-					   unsigned int pad,
-					   struct v4l2_mbus_frame_desc *fd)
+int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
+						  struct v4l2_subdev_state *state,
+						  unsigned int pad,
+						  struct v4l2_mbus_frame_desc *fd)
 {
 	struct media_pad *local_sink_pad;
 	struct v4l2_subdev_route *route;
-	struct v4l2_subdev_state *state;
 	struct device *dev = sd->dev;
 	int ret = 0;
 
 	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
 		return -EINVAL;
 
-	state = v4l2_subdev_lock_and_get_active_state(sd);
-
 	/* Iterate over sink pads */
 	media_entity_for_each_pad(&sd->entity, local_sink_pad) {
 		struct v4l2_mbus_frame_desc source_fd;
@@ -2586,15 +2584,12 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 				if (!remote_source_pad) {
 					dev_dbg(dev, "Failed to find remote pad for sink pad %u\n",
 						local_sink_pad->index);
-					ret = -EINVAL;
-					goto out_unlock;
+					return -EINVAL;
 				}
 
 				remote_sd = media_entity_to_v4l2_subdev(remote_source_pad->entity);
-				if (!remote_sd) {
-					ret = -EINVAL;
-					goto out_unlock;
-				}
+				if (!remote_sd)
+					return -EINVAL;
 
 				ret = v4l2_subdev_call(remote_sd, pad,
 						       get_frame_desc,
@@ -2604,7 +2599,7 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 					dev_err(dev,
 						"Failed to get frame desc from remote subdev %s\n",
 						remote_sd->name);
-					goto out_unlock;
+					return ret;
 				}
 
 				have_source_fd = true;
@@ -2615,8 +2610,7 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 					dev_err(dev,
 						"Frame desc type mismatch: %u != %u\n",
 						fd->type, source_fd.type);
-					ret = -EPIPE;
-					goto out_unlock;
+					return -EPIPE;
 				}
 			}
 
@@ -2631,14 +2625,12 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 				dev_dbg(dev,
 					"Failed to find stream %u from source frame desc\n",
 					route->sink_stream);
-				ret = -EPIPE;
-				goto out_unlock;
+				return -EPIPE;
 			}
 
 			if (fd->num_entries >= V4L2_FRAME_DESC_ENTRY_MAX) {
 				dev_dbg(dev, "Frame desc entry limit reached\n");
-				ret = -ENOSPC;
-				goto out_unlock;
+				return -ENOSPC;
 			}
 
 			fd->entry[fd->num_entries] = *source_entry;
@@ -2649,7 +2641,21 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
 		}
 	}
 
-out_unlock:
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc_passthrough_locked);
+
+int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
+					   unsigned int pad,
+					   struct v4l2_mbus_frame_desc *fd)
+{
+	struct v4l2_subdev_state *state;
+	int ret;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	ret = v4l2_subdev_get_frame_desc_passthrough_locked(sd, state, pad, fd);
+
 	v4l2_subdev_unlock_state(state);
 
 	return ret;
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 23c03ba7f84c..6eafbef246e6 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -1723,15 +1723,15 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
 
 /**
- * v4l2_subdev_get_frame_desc_passthrough() - Helper to implement the subdev
- *	get_frame_desc operation in simple passthrough cases
+ * v4l2_subdev_get_frame_desc_passthrough_locked() - Helper to implement the
+ *	subdev get_frame_desc operation in simple passthrough cases
  * @sd: The subdevice
+ * @state: The locked subdevice active state
  * @pad: The source pad index
  * @fd: The mbus frame desc
  *
- * This helper implements get_frame_desc operation for subdevices that pass
- * streams through without modification. It can be assigned directly as the
- * .get_frame_desc callback in &v4l2_subdev_pad_ops.
+ * This helper implements the get_frame_desc operation for subdevices that pass
+ * streams through without modification.
  *
  * The helper iterates over the subdevice's sink pads, calls get_frame_desc on
  * the remote subdevice connected to each sink pad, and collects the frame desc
@@ -1744,6 +1744,34 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
  * sink pads are involved and the upstream sources report different frame desc
  * types, -EPIPE is returned.
  *
+ * The caller must hold the subdevice's active state lock. This variant is
+ * intended for drivers that need to perform additional work around the
+ * passthrough frame descriptor collection. Drivers that do not need any
+ * customization should use v4l2_subdev_get_frame_desc_passthrough() instead.
+ *
+ * Return: 0 on success, or a negative error code otherwise.
+ */
+int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
+						  struct v4l2_subdev_state *state,
+						  unsigned int pad,
+						  struct v4l2_mbus_frame_desc *fd);
+
+/**
+ * v4l2_subdev_get_frame_desc_passthrough() - Helper to implement the subdev
+ *	get_frame_desc operation in simple passthrough cases
+ * @sd: The subdevice
+ * @pad: The source pad index
+ * @fd: The mbus frame desc
+ *
+ * This function locks the subdevice's active state, calls
+ * v4l2_subdev_get_frame_desc_passthrough_locked(), and unlocks the state.
+ *
+ * This function can be assigned directly as the .get_frame_desc callback in
+ * &v4l2_subdev_pad_ops for subdevices that pass streams through without
+ * modification. Drivers that need to perform additional work should use
+ * v4l2_subdev_get_frame_desc_passthrough_locked() in their custom
+ * .get_frame_desc implementation instead.
+ *
  * Return: 0 on success, or a negative error code otherwise.
  */
 int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,

-- 
2.43.0

Re: [PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Sakari Ailus]

Moi,

On Thu, Mar 12, 2026 at 02:15:30PM +0200, Tomi Valkeinen wrote:
> The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
> directly as an implementation for .get_frame_desc subdev op. However, in
> some cases the drivers may want to add some customizations, while the
> bulk of the work is still identical to what
> v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
> makes this impossible to do properly.
> 
> Split v4l2_subdev_get_frame_desc_passthrough() into two functions:
> 
> v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
> subdev state as a parameter, instead of locking and getting the active
> state internally. Other than that, it does the same as
> v4l2_subdev_get_frame_desc_passthrough() used to do.
> 
> v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
> and calls v4l2_subdev_get_frame_desc_passthrough_locked().
> 
> In other words, v4l2_subdev_get_frame_desc_passthrough() works as
> before, but drivers can now alternatively add custom .get_frame_desc
> code and call v4l2_subdev_get_frame_desc_passthrough().
> 
> An example use case is with DS90UB953 serializer: in normal use the
> serializer passes through everything, but when test-pattern-generator
> (TPG) is used, an internal TPG source is used. After this commit, the
> UB953 get_frame_desc() can lock the state, look at the routing table to
> see if we're in normal or TPG mode, then either call
> v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
> construct a TPG frame desc if in TPG mode.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
>  2 files changed, 59 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 2757378c628a..8b1a7f00c86b 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
>  
> -int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
> -					   unsigned int pad,
> -					   struct v4l2_mbus_frame_desc *fd)
> +int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
> +						  struct v4l2_subdev_state *state,
> +						  unsigned int pad,
> +						  struct v4l2_mbus_frame_desc *fd)
>  {
>  	struct media_pad *local_sink_pad;
>  	struct v4l2_subdev_route *route;
> -	struct v4l2_subdev_state *state;
>  	struct device *dev = sd->dev;
>  	int ret = 0;
>  
>  	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
>  		return -EINVAL;
>  
> -	state = v4l2_subdev_lock_and_get_active_state(sd);

This variant appears to be unlocked rather than locked.

Could you instead use two underscores as a prefix to the same name? That's
an established practice.

-- 
Terveisin,

Sakari Ailus

Re: [PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Tomi Valkeinen]

Hi,

On 17/03/2026 10:02, Sakari Ailus wrote:
> Moi,
> 
> On Thu, Mar 12, 2026 at 02:15:30PM +0200, Tomi Valkeinen wrote:
>> The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
>> directly as an implementation for .get_frame_desc subdev op. However, in
>> some cases the drivers may want to add some customizations, while the
>> bulk of the work is still identical to what
>> v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
>> makes this impossible to do properly.
>>
>> Split v4l2_subdev_get_frame_desc_passthrough() into two functions:
>>
>> v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
>> subdev state as a parameter, instead of locking and getting the active
>> state internally. Other than that, it does the same as
>> v4l2_subdev_get_frame_desc_passthrough() used to do.
>>
>> v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
>> and calls v4l2_subdev_get_frame_desc_passthrough_locked().
>>
>> In other words, v4l2_subdev_get_frame_desc_passthrough() works as
>> before, but drivers can now alternatively add custom .get_frame_desc
>> code and call v4l2_subdev_get_frame_desc_passthrough().
>>
>> An example use case is with DS90UB953 serializer: in normal use the
>> serializer passes through everything, but when test-pattern-generator
>> (TPG) is used, an internal TPG source is used. After this commit, the
>> UB953 get_frame_desc() can lock the state, look at the routing table to
>> see if we're in normal or TPG mode, then either call
>> v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
>> construct a TPG frame desc if in TPG mode.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
>>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
>>  2 files changed, 59 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 2757378c628a..8b1a7f00c86b 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>>  }
>>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
>>  
>> -int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>> -					   unsigned int pad,
>> -					   struct v4l2_mbus_frame_desc *fd)
>> +int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
>> +						  struct v4l2_subdev_state *state,
>> +						  unsigned int pad,
>> +						  struct v4l2_mbus_frame_desc *fd)
>>  {
>>  	struct media_pad *local_sink_pad;
>>  	struct v4l2_subdev_route *route;
>> -	struct v4l2_subdev_state *state;
>>  	struct device *dev = sd->dev;
>>  	int ret = 0;
>>  
>>  	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
>>  		return -EINVAL;
>>  
>> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> 
> This variant appears to be unlocked rather than locked.

The variant expects locked parameters, thus "locked". This is widely
used at least on DRM drivers.

> Could you instead use two underscores as a prefix to the same name? That's
> an established practice.
I can do that, although I don't personally like it. Double underscore
hints at an internal function, something that shouldn't be called
normally, whereas this is not internal or anything to avoid.

 Tomi

Re: [PATCH 3/3] media: subdev: Split v4l2_subdev_get_frame_desc_passthrough() into locked and unlocked

[Sakari Ailus]

Moi,

On Tue, Mar 17, 2026 at 10:37:12AM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 17/03/2026 10:02, Sakari Ailus wrote:
> > Moi,
> > 
> > On Thu, Mar 12, 2026 at 02:15:30PM +0200, Tomi Valkeinen wrote:
> >> The recently added v4l2_subdev_get_frame_desc_passthrough() can be used
> >> directly as an implementation for .get_frame_desc subdev op. However, in
> >> some cases the drivers may want to add some customizations, while the
> >> bulk of the work is still identical to what
> >> v4l2_subdev_get_frame_desc_passthrough() does. Current locking scheme
> >> makes this impossible to do properly.
> >>
> >> Split v4l2_subdev_get_frame_desc_passthrough() into two functions:
> >>
> >> v4l2_subdev_get_frame_desc_passthrough_locked(), which takes a locked
> >> subdev state as a parameter, instead of locking and getting the active
> >> state internally. Other than that, it does the same as
> >> v4l2_subdev_get_frame_desc_passthrough() used to do.
> >>
> >> v4l2_subdev_get_frame_desc_passthrough(), which locks the active state
> >> and calls v4l2_subdev_get_frame_desc_passthrough_locked().
> >>
> >> In other words, v4l2_subdev_get_frame_desc_passthrough() works as
> >> before, but drivers can now alternatively add custom .get_frame_desc
> >> code and call v4l2_subdev_get_frame_desc_passthrough().
> >>
> >> An example use case is with DS90UB953 serializer: in normal use the
> >> serializer passes through everything, but when test-pattern-generator
> >> (TPG) is used, an internal TPG source is used. After this commit, the
> >> UB953 get_frame_desc() can lock the state, look at the routing table to
> >> see if we're in normal or TPG mode, then either call
> >> v4l2_subdev_get_frame_desc_passthrough_locked() if in normal mode, or
> >> construct a TPG frame desc if in TPG mode.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-subdev.c | 46 ++++++++++++++++++++---------------
> >>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++++----
> >>  2 files changed, 59 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >> index 2757378c628a..8b1a7f00c86b 100644
> >> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >> @@ -2545,21 +2545,19 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> >>  }
> >>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
> >>  
> >> -int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
> >> -					   unsigned int pad,
> >> -					   struct v4l2_mbus_frame_desc *fd)
> >> +int v4l2_subdev_get_frame_desc_passthrough_locked(struct v4l2_subdev *sd,
> >> +						  struct v4l2_subdev_state *state,
> >> +						  unsigned int pad,
> >> +						  struct v4l2_mbus_frame_desc *fd)
> >>  {
> >>  	struct media_pad *local_sink_pad;
> >>  	struct v4l2_subdev_route *route;
> >> -	struct v4l2_subdev_state *state;
> >>  	struct device *dev = sd->dev;
> >>  	int ret = 0;
> >>  
> >>  	if (WARN_ON(!(sd->entity.pads[pad].flags & MEDIA_PAD_FL_SOURCE)))
> >>  		return -EINVAL;
> >>  
> >> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> > 
> > This variant appears to be unlocked rather than locked.
> 
> The variant expects locked parameters, thus "locked". This is widely

From the name I'd expect otherwise. But...

> used at least on DRM drivers.

it maybe used in DRM for that but this isn't DRM. :-)

> 
> > Could you instead use two underscores as a prefix to the same name? That's
> > an established practice.
> I can do that, although I don't personally like it. Double underscore
> hints at an internal function, something that shouldn't be called
> normally, whereas this is not internal or anything to avoid.

Or know what you're doing. You could add a lockdep annotation while at it.

-- 
Sakari Ailus