Re: [PATCH 1/3] media: subdev: Add v4l2_get_frame_desc_passthrough helper

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Moi,

Thanks for the set.

On Tue, Feb 18, 2025 at 06:15:43PM +0200, Tomi Valkeinen wrote:
> Add a helper for v4l2_subdev_pad_ops.v4l2_get_frame_desc operation. The
> helper can be used when the subdevice directly passes through the
> streams.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           | 21 ++++++++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index cde1774c9098..f04de851d777 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2500,6 +2500,102 @@ int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_s_stream_helper);
>  
> +int v4l2_get_frame_desc_passthrough(struct v4l2_subdev *sd, unsigned int pad,

Shouldn't this be called v4l2_subdev_get_frame_desc_passthrough? It's long
but that's unlikely to be a problem still.

> +				    struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct v4l2_subdev_route *route;
> +	struct v4l2_subdev_state *state;
> +	int ret;
> +	const struct media_pad *pads = sd->entity.pads;
> +	struct media_pad *local_sink_pad;
> +
> +	if (WARN_ON(!(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;
> +		bool have_source_fd = false;
> +
> +		if (!(local_sink_pad->flags & MEDIA_PAD_FL_SINK))
> +			continue;
> +
> +		/*
> +		 * Copy frame desc entries for the streams going from the sink
> +		 * pad to the requested pad
> +		 */
> +		for_each_active_route(&state->routing, route) {
> +			struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> +			unsigned int i;
> +			struct media_pad *remote_source_pad;
> +			struct v4l2_subdev *remote_sd;
> +
> +			if (route->source_pad != pad ||
> +			    route->sink_pad != local_sink_pad->index)
> +				continue;
> +
> +			if (!have_source_fd) {
> +				remote_source_pad = media_pad_remote_pad_unique(local_sink_pad);
> +				if (!remote_source_pad) {
> +					ret = -EINVAL;
> +					goto out_unlock;
> +				}
> +
> +				remote_sd = media_entity_to_v4l2_subdev(remote_source_pad->entity);
> +				if (!remote_sd) {
> +					ret = -EINVAL;

Having dev_dbg()'s for all these cases would be nice. Otherwise it'll be
hard to figure out what went wrong.

> +					goto out_unlock;
> +				}
> +
> +				ret = v4l2_subdev_call(remote_sd, pad,
> +						       get_frame_desc,
> +						       remote_source_pad->index,
> +						       &source_fd);
> +				if (ret)
> +					goto out_unlock;
> +
> +				have_source_fd = true;
> +
> +				if (fd->num_entries == 0) {
> +					fd->type = source_fd.type;
> +				} else if (fd->type != source_fd.type) {
> +					ret = -EPIPE;
> +					goto out_unlock;
> +				}
> +			}
> +
> +			for (i = 0; i < source_fd.num_entries; i++) {
> +				if (source_fd.entry[i].stream == route->sink_stream) {
> +					source_entry = &source_fd.entry[i];
> +					break;
> +				}
> +			}
> +
> +			if (!source_entry) {
> +				dev_err(sd->dev,
> +					"Failed to find stream %u from source frame desc\n",
> +					route->sink_stream);
> +				ret = -EPIPE;
> +				goto out_unlock;
> +			}
> +
> +			fd->entry[fd->num_entries] = *source_entry;
> +
> +			fd->entry[fd->num_entries].stream = route->source_stream;
> +
> +			fd->num_entries++;

Could you check you're not overflowing what's allocated?

> +		}
> +	}
> +
> +out_unlock:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_get_frame_desc_passthrough);
> +
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>  
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 2f2200875b03..d42fb3f55515 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1746,6 +1746,27 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>   */
>  int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable);
>  
> +/**
> + * v4l2_get_frame_desc_passthrough() - Helper to implement the subdev
> + *	v4l2_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.
> + *
> + * 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.
> + *
> + * Return: 0 on success, or a negative error code otherwise.
> + */
> +int v4l2_get_frame_desc_passthrough(struct v4l2_subdev *sd, unsigned int pad,
> +				    struct v4l2_mbus_frame_desc *fd);
> +
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>  
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> 

-- 
Kind regards,

Sakari Ailus