Re: [PATCH 04/17] media: v4l2-subdev: Allocate frame descriptors based on the need

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari,

   one more comment

On Wed, May 13, 2026 at 01:43:45PM +0300, Sakari Ailus wrote:
> Frame descriptors entries require a small amount of memory per entry (20
> bytes), but if the number of entries in a frame descriptor is large, an
> unreasonably large amount of memory would need to be allocated in the
> stack. Therefore the number of entries has been limited to 8.
>
> Support larger frame descriptors by allocating as much memory as required.
> The get_frame_desc() op can now set the num_entries to a number larger
> than V4L2_FRAME_BUS_ENTRY_MAX and return -ENOSPC. The caller,
> v4l2_subdev_get_frame_desc(), will then allocate memory for that amount of
> memory and call the get_frame_desc() op again.
>
> The caller is also responsible for releasing the allocated memory by
> calling v4l2_subdev_free_frame_desc().
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 49 +++++++++++++++++++++------
>  include/media/v4l2-subdev.h           | 44 +++++++++++++++++++-----
>  2 files changed, 74 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index b8acce8f9c33..012ff6fb2988 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -63,10 +63,6 @@ static bool v4l2_subdev_enable_streams_api;
>  /*
>   * Maximum stream ID is 63 for now, as we use u64 bitmask to represent a set
>   * of streams.
> - *
> - * Note that V4L2_FRAME_DESC_ENTRY_MAX is related: V4L2_FRAME_DESC_ENTRY_MAX
> - * restricts the total number of streams in a pad, although the stream ID is
> - * not restricted.
>   */
>  #define V4L2_SUBDEV_MAX_STREAM_ID 63
>
> @@ -354,6 +350,7 @@ static int call_set_frame_interval(struct v4l2_subdev *sd,
>  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  			       struct v4l2_mbus_frame_desc *fd)
>  {
> +	unsigned int type;
>  	unsigned int i;
>  	int ret;
>
> @@ -362,16 +359,38 @@ static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  		return -EOPNOTSUPP;
>  #endif
>
> +	type = fd->type;
>  	memset(fd, 0, sizeof(*fd));
> +	if (!fd->entry) {
> +		fd->entry = fd->entry_mem;
> +		fd->len_entries = ARRAY_SIZE(fd->entry_mem);
> +	}
>
>  	ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> +	if (ret == -ENOSPC) {
> +		if (fd->num_entries > V4L2_FRAME_DESC_ENTRY_PREALLOC &&
> +		    fd->num_entries < V4L2_FRAME_DESC_ENTRY_MAX) {
> +			fd->entry = kzalloc_objs(*fd->entry, fd->num_entries,
> +						 GFP_KERNEL);
> +			if (!fd->entry)
> +				return -ENOMEM;
> +
> +			fd->len_entries = fd->num_entries;
> +			fd->num_entries = 0;
> +
> +			ret = sd->ops->pad->get_frame_desc(sd, pad, fd);
> +		} else {
> +			return -E2BIG;
> +		}
> +	}
>  	if (ret)
>  		return ret;
>
>  	dev_dbg(sd->dev, "Frame descriptor on pad %u, type %s\n", pad,
> -		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> -		fd->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" :
> -		"unknown");
> +		type == V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL ? "parallel" :
> +		type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2 ? "CSI-2" : "unknown");
> +
> +	fd->type = type;
>
>  	for (i = 0; i < fd->num_entries; i++) {
>  		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[i];
> @@ -1086,9 +1105,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		 * descriptor accordingly, with up to one entry per route. Until
>  		 * the frame descriptors entries get allocated dynamically,
>  		 * limit the number of active routes to
> -		 * V4L2_FRAME_DESC_ENTRY_MAX.
> +		 * V4L2_FRAME_DESC_ENTRY_PREALLOC.
>  		 */
> -		if (num_active_routes > V4L2_FRAME_DESC_ENTRY_MAX)
> +		if (num_active_routes > V4L2_FRAME_DESC_ENTRY_PREALLOC)
>  			return -E2BIG;
>
>  		/*
> @@ -2638,7 +2657,7 @@ int __v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>  				return -EPIPE;
>  			}
>
> -			if (fd->num_entries >= V4L2_FRAME_DESC_ENTRY_MAX) {
> +			if (fd->num_entries >= V4L2_FRAME_DESC_ENTRY_PREALLOC) {
>  				dev_dbg(dev, "Frame desc entry limit reached\n");
>  				return -E2BIG;
>  			}
> @@ -2730,6 +2749,16 @@ int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc);
>
> +void v4l2_subdev_free_frame_desc(struct v4l2_mbus_frame_desc *desc)
> +{
> +	if (desc->entry != desc->entry_mem)
> +		kfree(desc->entry);
> +
> +	desc->entry = NULL;
> +	desc->len_entries = desc->num_entries = 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_frame_desc);
> +
>  #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 c9e74566c85a..bd97510c7024 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -365,11 +365,13 @@ struct v4l2_mbus_frame_desc_entry {
>  	} bus;
>  };
>
> - /*
> -  * If this number is too small, it should be dropped altogether and the
> -  * API switched to a dynamic number of frame descriptor entries.
> -  */
> -#define V4L2_FRAME_DESC_ENTRY_MAX	8
> +/* Size of the statically allocated frame descriptor array. */
> +#define V4L2_FRAME_DESC_ENTRY_PREALLOC	8
> +/*
> + * Maximum number of dynamically allocated frame descriptors. Note that
> + * V4L2_SUBDEV_MAX_STREAM_ID is related to this limit as well.
> + */
> +#define V4L2_FRAME_DESC_ENTRY_MAX	64
>
>  /**
>   * enum v4l2_mbus_frame_desc_type - media bus frame description type
> @@ -392,13 +394,17 @@ enum v4l2_mbus_frame_desc_type {
>  /**
>   * struct v4l2_mbus_frame_desc - media bus data frame description
>   * @type: type of the bus (enum v4l2_mbus_frame_desc_type)
> - * @entry: frame descriptors array
> - * @num_entries: number of entries in @entry array
> + * @entry_mem: memory for the frame descriptors (@entry)
> + * @entry: pointer to the frame descriptors
> + * @num_entries: number of entries in @entry
> + * @len_entries: number of entries allocated for @entry
>   */
>  struct v4l2_mbus_frame_desc {
>  	enum v4l2_mbus_frame_desc_type type;
> -	struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX];
> +	struct v4l2_mbus_frame_desc_entry entry_mem[V4L2_FRAME_DESC_ENTRY_PREALLOC];
> +	struct v4l2_mbus_frame_desc_entry *entry;
>  	unsigned short num_entries;
> +	unsigned short len_entries;

What are the implications of this change on this part of
v4l2_subdev_get_frame_desc() ?

	struct v4l2_subdev_state *state =
		v4l2_subdev_lock_and_get_active_state(sd);
	ret = v4l2_subdev_call(sd, pad, get_fmt, state, &subdev_fmt);
	v4l2_subdev_unlock_state(state);
	if (ret)
		return ret;

	struct v4l2_mbus_frame_desc_entry entry = {
		.pixelcode = subdev_fmt.format.code,
	};

	if (desc->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
		int dt;

		dt = mipi_csi2_dt_for_mbus(subdev_fmt.format.code);
		if (dt < 0)
			return dt;

		entry.bus.csi2.dt = dt;
	}

	desc->entry[0] = entry;

        ^
        |---- here ?

Is there any risk this now access invalid memory ?
Should we do the same thing that you're doing call_get_frame_desc() ?

	fd->entry = fd->entry_mem;

before assigning its first member ?

>  };
>
>  /**
> @@ -780,7 +786,13 @@ struct v4l2_subdev_state {
>   * @link_validate: used by the media controller code to check if the links
>   *		   that belongs to a pipeline can be used for stream.
>   *
> - * @get_frame_desc: get the current low level media bus frame parameters.
> + * @get_frame_desc: get the current low level media bus frame parameters. The
> + *		    callback is required to update the num_entries field to the
> + *		    total number of entries in the frame descriptor. The
> + *		    callback shall fill the first entries array up to
> + *		    len_entries, which signifies the number of entries
> + *		    allocated. If num_entries exceeds len_entries, the callback
> + *		    shall return -ENOSPC.
>   *
>   * @set_frame_desc: set the low level media bus frame parameters, @fd array
>   *                  may be adjusted by the subdev driver to device capabilities.
> @@ -1793,11 +1805,25 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>   *
>   * The caller is required to set @desc->type to the expected bus type.
>   *
> + * The entries in the frame descriptor are allocated based on the need. The
> + * caller is required to release the memory of the frame descriptor entries for
> + * each frame descriptor obtained by calling this function using
> + * v4l2_subdev_free_frame_desc().
> + *
>   * Return: %0 on success or negative error code on failure.
>   */
>  int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>  			       struct v4l2_mbus_frame_desc *desc);
>
> +/**
> + * v4l2_subdev_free_frame_desc() - Release the memory of a frame descriptor
> + * @desc: A pointer to a frame descriptor
> + *
> + * Release the frame descriptor entries in a frame descriptor. The number of
> + * entries in the descriptor are set to 0 again.
> + */
> +void v4l2_subdev_free_frame_desc(struct v4l2_mbus_frame_desc *desc);
> +
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> --
> 2.47.3
>