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

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

Hi Sakari

On Thu, May 14, 2026 at 12:16:51PM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> 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) {
>
> nit: you can invert the condition and save one indentation level
>
>                 if (fd->num_entries <= V4L2_FRAME_DESC_ENTRY_PREALLOC ||
>                     fd->num_entries >= V4L2_FRAME_DESC_ENTRY_MAX)
>                     return -E2BIG;
>
> > +			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;
>
> If we end up calling d->ops->pad->get_frame_desc() don't we overwrite
> the returned 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)
> > +{
>
> As this might be used as a __cleanup() should we check if desc is
> valid ?
>
> > +	if (desc->entry != desc->entry_mem)
> > +		kfree(desc->entry);

Sorry, in my below comment  ....


> > +
> > +	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;
> >  };
> >
> >  /**
> > @@ -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
>
> Is it unconditionally true ? If fd->entry is not initialized aren't we
> deflecting to the pre-allocated entries in call_get_frame_desc() ?
>
> 	if (!fd->entry) {
> 		fd->entry = fd->entry_mem;
> 		fd->len_entries = ARRAY_SIZE(fd->entry_mem);
> 	}
>
> v4l2_subdev_free_frame_desc() shouldn't be called in this case, right ?

... here I missed this, so it's safe to call
v4l2_subdev_free_frame_desc() unconditionally

>
>
> > + * 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
> >
> >