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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Tue, Mar 17, 2026 at 02:09:42PM +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(), 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().
> 
> 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().

This looks fine (and quite standard) to me.

> 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() 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 | 48 ++++++++++++++++++++---------------
>  include/media/v4l2-subdev.h           | 38 +++++++++++++++++++++++----
>  2 files changed, 61 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 2757378c628a..b73ccd1c83d2 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2545,21 +2545,21 @@ 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(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;
>  
> +	lockdep_assert_held(state->lock);
> +
>  	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 +2586,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 +2601,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 +2612,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 +2627,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 +2643,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);
> +
> +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(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..d256b7ec8f84 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 - 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

Nitpicking, the insertion of "the" belongs to a previous patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> + * 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(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(), 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() 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,
> 

-- 
Regards,

Laurent Pinchart