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.0Moi,
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
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
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
© 2016 - 2026 Red Hat, Inc.