Re: [PATCH v3 03/29] media: v4l2-subdev: Prepare for changes in getting frame descriptors

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

Hej Linus,

On Mon, Aug 31, 2026 at 02:43:09PM +0200, Linus Walleij wrote:
> Hi Sakari,
> 
> thanks for your patch!
> 
> On Mon, Aug 24, 2026 at 2:14 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> 
> > Introduce v4l2_subdev_alloc_frame_desc()
> 
> The patch doesn't introduce this _alloc_ thing? Rather _get_ and _free_.

Right, this got renamed on the way; I'll fix it for v4.

> 
> > and v4l2_subdev_free_frame_desc()
> > to both facilitate implementing drivers that need frame descriptors as
> > well as prepare for having a larger number of frame descriptors.
> > If the remote sub-device does not support frame descriptors,
> > v4l2_subdev_get_frame_desc() creates one (with a single entry)
> > opportunistically, thus avoiding the need to add frame descriptor support
> > to sensor drivers the device for which only generates a single stream, or
> > managing the situation on the caller side.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> (...)
> 
> > +struct v4l2_mbus_frame_desc *
> > +v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +                          enum v4l2_mbus_frame_desc_type type)
> > +{
> > +       int ret;
> > +
> > +       if (type != V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL &&
> > +           type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > +               return ERR_PTR(-EINVAL);
> (...)
> > +
> > +       struct v4l2_mbus_frame_desc *desc = kzalloc_obj(*desc, GFP_KERNEL);
> > +       if (!desc)
> > +               return ERR_PTR(-ENOMEM);
> 
> I'm not used to this style of C where you declare variables in the
> middle of the code, but I suppose if this is common in the media
> framework I just have to get used to it.

It is the recommended way to declare objects that are to be released using
__free(), but I don't see an issue in doing that elsewhere either.

> 
> > +       struct v4l2_subdev_format subdev_fmt = {
> > +               .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > +               .pad = pad,
> > +       };
> 
> Ugh even inline struct variables.
> 
> > +       struct v4l2_subdev_state *state =
> > +               v4l2_subdev_lock_and_get_active_state(sd);
> 
> This can return NULL according to the definition:

Right. Actually this isn't even needed, the subdev call wrappers handle
this already. I'll drop it.

> 
> /**
>  * v4l2_subdev_lock_and_get_active_state() - Locks and returns the active subdev
>  *                                           state for the subdevice
>  * @sd: The subdevice
>  *
>  * Returns the locked active state for the subdevice, or NULL if the subdev
>  * does not support active state.
>  *
>  * The state must be unlocked with v4l2_subdev_unlock_state() after use.
>  */
> static inline struct v4l2_subdev_state *
> v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
> {
>         if (sd->active_state)
>                 v4l2_subdev_lock_state(sd->active_state);
>         return sd->active_state;
> }
> 
> > +       ret = v4l2_subdev_call(sd, pad, get_fmt, state, &subdev_fmt);
> > +       v4l2_subdev_unlock_state(state);
> 
> ... and this call isn't gonna tolerate state being NULL because
> it will directly dereference state:
> 
> static inline void v4l2_subdev_unlock_state(struct v4l2_subdev_state *state)
> {
>         mutex_unlock(state->lock);
> }
> 
> Just some
> 
> if (!state)
>     goto err_free;
> 
> ?
> 
> Maybe you can just use v4l2_subdev_call() or v4l2_subdev_call_state_active()
> because they seem to be dealing with NULL already?

Yes.

-- 
Med trevliga hälsningar,

Sakari Ailus