| Message ID | 20210609100228.278798-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: v4l2-subdev: add subdev-wide state struct | expand |
Hi Tomi, A few small comments: On 09/06/2021 12:02, Tomi Valkeinen wrote: > We have 'struct v4l2_subdev_pad_config' which contains configuration for > a single pad used for the TRY functionality, and an array of those > structs is passed to various v4l2_subdev_pad_ops. > > I was working on subdev internal routing between pads, and realized that > there's no way to add TRY functionality for routes, which is not pad > specific configuration. Adding a separate struct for try-route config > wouldn't work either, as e.g. set-fmt needs to know the try-route > configuration to propagate the settings. > > This patch adds a new struct, 'struct v4l2_subdev_state' (which at the > moment only contains the v4l2_subdev_pad_config array) and the new > struct is used in most of the places where v4l2_subdev_pad_config was > used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config > are changed to instead take v4l2_subdev_state. > > The changes to drivers/media/v4l2-core/v4l2-subdev.c and > include/media/v4l2-subdev.h were written by hand, and all the driver > changes were done with the semantic patch below. The spatch needs to be > applied to a select list of directories. I used the following shell > commands to apply the spatch: <snip> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 956dafab43d4..dacae53b68d5 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -26,19 +26,21 @@ > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > { > - if (sd->entity.num_pads) { > - fh->pad = v4l2_subdev_alloc_pad_config(sd); > - if (fh->pad == NULL) > - return -ENOMEM; > - } > + struct v4l2_subdev_state *state; > + > + state = v4l2_subdev_alloc_state(sd); > + if (IS_ERR(state)) > + return PTR_ERR(state); > + > + fh->state = state; > > return 0; > } > > static void subdev_fh_free(struct v4l2_subdev_fh *fh) > { > - v4l2_subdev_free_pad_config(fh->pad); > - fh->pad = NULL; > + v4l2_subdev_free_state(fh->state); > + fh->state = NULL; > } > > static int subdev_open(struct file *file) > @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) > return 0; > } > > -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) > +static int check_cfg(u32 which, struct v4l2_subdev_state *state) This should be renamed to check_state or check_state_pads (see next comment). > { > - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) > + if (which == V4L2_SUBDEV_FORMAT_TRY && !state) Should this also check for !state->pads? At least in the current code it really checks for the pad configuration, so I think it should. If so, then this function should probably be renamed to check_state_pads. > return -EINVAL; > > return 0; > } > > static inline int check_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > if (!format) > return -EINVAL; > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > - check_cfg(format->which, cfg); > + check_cfg(format->which, state); > } > > static int call_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > - return check_format(sd, cfg, format) ? : > - sd->ops->pad->get_fmt(sd, cfg, format); > + return check_format(sd, state, format) ? : > + sd->ops->pad->get_fmt(sd, state, format); > } > > static int call_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format) > { > - return check_format(sd, cfg, format) ? : > - sd->ops->pad->set_fmt(sd, cfg, format); > + return check_format(sd, state, format) ? : > + sd->ops->pad->set_fmt(sd, state, format); > } > > static int call_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_mbus_code_enum *code) > { > if (!code) > return -EINVAL; > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > - check_cfg(code->which, cfg) ? : > - sd->ops->pad->enum_mbus_code(sd, cfg, code); > + check_cfg(code->which, state) ? : > + sd->ops->pad->enum_mbus_code(sd, state, code); > } > > static int call_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_frame_size_enum *fse) > { > if (!fse) > return -EINVAL; > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > - check_cfg(fse->which, cfg) ? : > - sd->ops->pad->enum_frame_size(sd, cfg, fse); > + check_cfg(fse->which, state) ? : > + sd->ops->pad->enum_frame_size(sd, state, fse); > } > > static inline int check_frame_interval(struct v4l2_subdev *sd, > @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd, > } > > static int call_enum_frame_interval(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_frame_interval_enum *fie) > { > if (!fie) > return -EINVAL; > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > - check_cfg(fie->which, cfg) ? : > - sd->ops->pad->enum_frame_interval(sd, cfg, fie); > + check_cfg(fie->which, state) ? : > + sd->ops->pad->enum_frame_interval(sd, state, fie); > } > > static inline int check_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel) > { > if (!sel) > return -EINVAL; > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > - check_cfg(sel->which, cfg); > + check_cfg(sel->which, state); > } > > static int call_get_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel) > { > - return check_selection(sd, cfg, sel) ? : > - sd->ops->pad->get_selection(sd, cfg, sel); > + return check_selection(sd, state, sel) ? : > + sd->ops->pad->get_selection(sd, state, sel); > } > > static int call_set_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel) > { > - return check_selection(sd, cfg, sel) ? : > - sd->ops->pad->set_selection(sd, cfg, sel); > + return check_selection(sd, state, sel) ? : > + sd->ops->pad->set_selection(sd, state, sel); > } > > static inline int check_edid(struct v4l2_subdev *sd, > @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(format->reserved, 0, sizeof(format->reserved)); > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format); > + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format); > } > > case VIDIOC_SUBDEV_S_FMT: { > @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(format->reserved, 0, sizeof(format->reserved)); > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format); > + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format); > } > > case VIDIOC_SUBDEV_G_CROP: { > @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > sel.target = V4L2_SEL_TGT_CROP; > > rval = v4l2_subdev_call( > - sd, pad, get_selection, subdev_fh->pad, &sel); > + sd, pad, get_selection, subdev_fh->state, &sel); > > crop->rect = sel.r; > > @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > sel.r = crop->rect; > > rval = v4l2_subdev_call( > - sd, pad, set_selection, subdev_fh->pad, &sel); > + sd, pad, set_selection, subdev_fh->state, &sel); > > crop->rect = sel.r; > > @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_mbus_code_enum *code = arg; > > memset(code->reserved, 0, sizeof(code->reserved)); > - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad, > + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state, > code); > } > > @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_frame_size_enum *fse = arg; > > memset(fse->reserved, 0, sizeof(fse->reserved)); > - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad, > + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state, > fse); > } > > @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > struct v4l2_subdev_frame_interval_enum *fie = arg; > > memset(fie->reserved, 0, sizeof(fie->reserved)); > - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad, > + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state, > fie); > } > > @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(sel->reserved, 0, sizeof(sel->reserved)); > return v4l2_subdev_call( > - sd, pad, get_selection, subdev_fh->pad, sel); > + sd, pad, get_selection, subdev_fh->state, sel); > } > > case VIDIOC_SUBDEV_S_SELECTION: { > @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > memset(sel->reserved, 0, sizeof(sel->reserved)); > return v4l2_subdev_call( > - sd, pad, set_selection, subdev_fh->pad, sel); > + sd, pad, set_selection, subdev_fh->state, sel); > } > > case VIDIOC_G_EDID: { > @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link) > } > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); > > -struct v4l2_subdev_pad_config * > -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd) > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > { > - struct v4l2_subdev_pad_config *cfg; > + struct v4l2_subdev_state *state; > int ret; > > - if (!sd->entity.num_pads) > - return NULL; > + state = kzalloc(sizeof(*state), GFP_KERNEL); > + if (!state) > + return ERR_PTR(-ENOMEM); > > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), > - GFP_KERNEL | __GFP_ZERO); > - if (!cfg) > - return NULL; > - > - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); > - if (ret < 0 && ret != -ENOIOCTLCMD) { > - kvfree(cfg); > - return NULL; > + if (sd->entity.num_pads) { > + state->pads = kvmalloc_array(sd->entity.num_pads, > + sizeof(*state->pads), > + GFP_KERNEL | __GFP_ZERO); > + if (!state->pads) { > + ret = -ENOMEM; > + goto err; > + } > } > > - return cfg; > + ret = v4l2_subdev_call(sd, pad, init_cfg, state); The init_cfg callback should also be renamed to init_state, but this can be done in a second patch. > + if (ret < 0 && ret != -ENOIOCTLCMD) > + goto err; > + > + return state; > + > +err: > + if (state && state->pads) > + kvfree(state->pads); > + > + kfree(state); > + > + return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); > > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state) > { > - kvfree(cfg); > + kvfree(state->pads); I'd change this to: if (state) kvfree(state->pads); That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust. > + kfree(state); > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) <snip> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index d0e9a5bdb08b..89115ba4c0f2 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config { I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology. This too can be done in a separate patch. I like the 'state' term much better than 'config'. Regards, Hans > struct v4l2_rect try_compose; > }; > > +/** > + * struct v4l2_subdev_state - Used for storing subdev state information. > + * > + * @pads: &struct v4l2_subdev_pad_config array > + * > + * This structure only needs to be passed to the pad op if the 'which' field > + * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For > + * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL. > + */ > +struct v4l2_subdev_state { > + struct v4l2_subdev_pad_config *pads; > +}; > + > /** > * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations > * > @@ -687,27 +700,27 @@ struct v4l2_subdev_pad_config { > */ > struct v4l2_subdev_pad_ops { > int (*init_cfg)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg); > + struct v4l2_subdev_state *state); > int (*enum_mbus_code)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_mbus_code_enum *code); > int (*enum_frame_size)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_frame_size_enum *fse); > int (*enum_frame_interval)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_frame_interval_enum *fie); > int (*get_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format); > int (*set_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_format *format); > int (*get_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel); > int (*set_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel); > int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid); > int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid); > @@ -918,14 +931,14 @@ struct v4l2_subdev { > * struct v4l2_subdev_fh - Used for storing subdev information per file handle > * > * @vfh: pointer to &struct v4l2_fh > - * @pad: pointer to &struct v4l2_subdev_pad_config > + * @state: pointer to &struct v4l2_subdev_state > * @owner: module pointer to the owner of this file handle > */ > struct v4l2_subdev_fh { > struct v4l2_fh vfh; > struct module *owner; > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > - struct v4l2_subdev_pad_config *pad; > + struct v4l2_subdev_state *state; > #endif > }; > > @@ -945,17 +958,17 @@ struct v4l2_subdev_fh { > * &struct v4l2_subdev_pad_config->try_fmt > * > * @sd: pointer to &struct v4l2_subdev > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > - * @pad: index of the pad in the @cfg array. > + * @state: pointer to &struct v4l2_subdev_state > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array > */ > static inline struct v4l2_mbus_framefmt * > v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_fmt; > + return &state->pads[pad].try_fmt; > } > > /** > @@ -963,17 +976,17 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > * &struct v4l2_subdev_pad_config->try_crop > * > * @sd: pointer to &struct v4l2_subdev > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > - * @pad: index of the pad in the @cfg array. > + * @state: pointer to &struct v4l2_subdev_state. > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array. > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_crop; > + return &state->pads[pad].try_crop; > } > > /** > @@ -981,17 +994,17 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > * &struct v4l2_subdev_pad_config->try_compose > * > * @sd: pointer to &struct v4l2_subdev > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > - * @pad: index of the pad in the @cfg array. > + * @state: pointer to &struct v4l2_subdev_state. > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array. > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_state *state, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_compose; > + return &state->pads[pad].try_compose; > } > > #endif > @@ -1093,20 +1106,21 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd, > int v4l2_subdev_link_validate(struct media_link *link); > > /** > - * v4l2_subdev_alloc_pad_config - Allocates memory for pad config > + * v4l2_subdev_alloc_state - allocate v4l2_subdev_state > * > - * @sd: pointer to struct v4l2_subdev > + * @sd: pointer to &struct v4l2_subdev for which the state is being allocated. > + * > + * Must call v4l2_subdev_free_state() when state is no longer needed. > */ > -struct > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd); > > /** > - * v4l2_subdev_free_pad_config - Frees memory allocated by > - * v4l2_subdev_alloc_pad_config(). > + * v4l2_subdev_free_state - free a v4l2_subdev_state > * > - * @cfg: pointer to &struct v4l2_subdev_pad_config > + * @state: v4l2_subdev_state to be freed. > */ > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** >
On 10/06/2021 11:00, Hans Verkuil wrote: > Hi Tomi, > > A few small comments: > > On 09/06/2021 12:02, Tomi Valkeinen wrote: >> We have 'struct v4l2_subdev_pad_config' which contains configuration for >> a single pad used for the TRY functionality, and an array of those >> structs is passed to various v4l2_subdev_pad_ops. >> >> I was working on subdev internal routing between pads, and realized that >> there's no way to add TRY functionality for routes, which is not pad >> specific configuration. Adding a separate struct for try-route config >> wouldn't work either, as e.g. set-fmt needs to know the try-route >> configuration to propagate the settings. >> >> This patch adds a new struct, 'struct v4l2_subdev_state' (which at the >> moment only contains the v4l2_subdev_pad_config array) and the new >> struct is used in most of the places where v4l2_subdev_pad_config was >> used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config >> are changed to instead take v4l2_subdev_state. >> >> The changes to drivers/media/v4l2-core/v4l2-subdev.c and >> include/media/v4l2-subdev.h were written by hand, and all the driver >> changes were done with the semantic patch below. The spatch needs to be >> applied to a select list of directories. I used the following shell >> commands to apply the spatch: > > <snip> > >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 956dafab43d4..dacae53b68d5 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -26,19 +26,21 @@ >> #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) >> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) >> { >> - if (sd->entity.num_pads) { >> - fh->pad = v4l2_subdev_alloc_pad_config(sd); >> - if (fh->pad == NULL) >> - return -ENOMEM; >> - } >> + struct v4l2_subdev_state *state; >> + >> + state = v4l2_subdev_alloc_state(sd); >> + if (IS_ERR(state)) >> + return PTR_ERR(state); >> + >> + fh->state = state; >> >> return 0; >> } >> >> static void subdev_fh_free(struct v4l2_subdev_fh *fh) >> { >> - v4l2_subdev_free_pad_config(fh->pad); >> - fh->pad = NULL; >> + v4l2_subdev_free_state(fh->state); >> + fh->state = NULL; >> } >> >> static int subdev_open(struct file *file) >> @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) >> return 0; >> } >> >> -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) >> +static int check_cfg(u32 which, struct v4l2_subdev_state *state) > > This should be renamed to check_state or check_state_pads (see next comment). Ok. >> { >> - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) >> + if (which == V4L2_SUBDEV_FORMAT_TRY && !state) > > Should this also check for !state->pads? At least in the current code > it really checks for the pad configuration, so I think it should. > > If so, then this function should probably be renamed to check_state_pads. Hmm, yes, I think you're right. We can have a case where we have state, but state->pads is NULL. >> return -EINVAL; >> >> return 0; >> } >> >> static inline int check_format(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> { >> if (!format) >> return -EINVAL; >> >> return check_which(format->which) ? : check_pad(sd, format->pad) ? : >> - check_cfg(format->which, cfg); >> + check_cfg(format->which, state); >> } >> >> static int call_get_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> { >> - return check_format(sd, cfg, format) ? : >> - sd->ops->pad->get_fmt(sd, cfg, format); >> + return check_format(sd, state, format) ? : >> + sd->ops->pad->get_fmt(sd, state, format); >> } >> >> static int call_set_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_format *format) >> { >> - return check_format(sd, cfg, format) ? : >> - sd->ops->pad->set_fmt(sd, cfg, format); >> + return check_format(sd, state, format) ? : >> + sd->ops->pad->set_fmt(sd, state, format); >> } >> >> static int call_enum_mbus_code(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_mbus_code_enum *code) >> { >> if (!code) >> return -EINVAL; >> >> return check_which(code->which) ? : check_pad(sd, code->pad) ? : >> - check_cfg(code->which, cfg) ? : >> - sd->ops->pad->enum_mbus_code(sd, cfg, code); >> + check_cfg(code->which, state) ? : >> + sd->ops->pad->enum_mbus_code(sd, state, code); >> } >> >> static int call_enum_frame_size(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> if (!fse) >> return -EINVAL; >> >> return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : >> - check_cfg(fse->which, cfg) ? : >> - sd->ops->pad->enum_frame_size(sd, cfg, fse); >> + check_cfg(fse->which, state) ? : >> + sd->ops->pad->enum_frame_size(sd, state, fse); >> } >> >> static inline int check_frame_interval(struct v4l2_subdev *sd, >> @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd, >> } >> >> static int call_enum_frame_interval(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_frame_interval_enum *fie) >> { >> if (!fie) >> return -EINVAL; >> >> return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : >> - check_cfg(fie->which, cfg) ? : >> - sd->ops->pad->enum_frame_interval(sd, cfg, fie); >> + check_cfg(fie->which, state) ? : >> + sd->ops->pad->enum_frame_interval(sd, state, fie); >> } >> >> static inline int check_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_selection *sel) >> { >> if (!sel) >> return -EINVAL; >> >> return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : >> - check_cfg(sel->which, cfg); >> + check_cfg(sel->which, state); >> } >> >> static int call_get_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_selection *sel) >> { >> - return check_selection(sd, cfg, sel) ? : >> - sd->ops->pad->get_selection(sd, cfg, sel); >> + return check_selection(sd, state, sel) ? : >> + sd->ops->pad->get_selection(sd, state, sel); >> } >> >> static int call_set_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_state *state, >> struct v4l2_subdev_selection *sel) >> { >> - return check_selection(sd, cfg, sel) ? : >> - sd->ops->pad->set_selection(sd, cfg, sel); >> + return check_selection(sd, state, sel) ? : >> + sd->ops->pad->set_selection(sd, state, sel); >> } >> >> static inline int check_edid(struct v4l2_subdev *sd, >> @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> >> memset(format->reserved, 0, sizeof(format->reserved)); >> memset(format->format.reserved, 0, sizeof(format->format.reserved)); >> - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format); >> + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format); >> } >> >> case VIDIOC_SUBDEV_S_FMT: { >> @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> >> memset(format->reserved, 0, sizeof(format->reserved)); >> memset(format->format.reserved, 0, sizeof(format->format.reserved)); >> - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format); >> + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format); >> } >> >> case VIDIOC_SUBDEV_G_CROP: { >> @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> sel.target = V4L2_SEL_TGT_CROP; >> >> rval = v4l2_subdev_call( >> - sd, pad, get_selection, subdev_fh->pad, &sel); >> + sd, pad, get_selection, subdev_fh->state, &sel); >> >> crop->rect = sel.r; >> >> @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> sel.r = crop->rect; >> >> rval = v4l2_subdev_call( >> - sd, pad, set_selection, subdev_fh->pad, &sel); >> + sd, pad, set_selection, subdev_fh->state, &sel); >> >> crop->rect = sel.r; >> >> @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> struct v4l2_subdev_mbus_code_enum *code = arg; >> >> memset(code->reserved, 0, sizeof(code->reserved)); >> - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad, >> + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state, >> code); >> } >> >> @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> struct v4l2_subdev_frame_size_enum *fse = arg; >> >> memset(fse->reserved, 0, sizeof(fse->reserved)); >> - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad, >> + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state, >> fse); >> } >> >> @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> struct v4l2_subdev_frame_interval_enum *fie = arg; >> >> memset(fie->reserved, 0, sizeof(fie->reserved)); >> - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad, >> + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state, >> fie); >> } >> >> @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> >> memset(sel->reserved, 0, sizeof(sel->reserved)); >> return v4l2_subdev_call( >> - sd, pad, get_selection, subdev_fh->pad, sel); >> + sd, pad, get_selection, subdev_fh->state, sel); >> } >> >> case VIDIOC_SUBDEV_S_SELECTION: { >> @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> >> memset(sel->reserved, 0, sizeof(sel->reserved)); >> return v4l2_subdev_call( >> - sd, pad, set_selection, subdev_fh->pad, sel); >> + sd, pad, set_selection, subdev_fh->state, sel); >> } >> >> case VIDIOC_G_EDID: { >> @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link) >> } >> EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); >> >> -struct v4l2_subdev_pad_config * >> -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd) >> +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd) >> { >> - struct v4l2_subdev_pad_config *cfg; >> + struct v4l2_subdev_state *state; >> int ret; >> >> - if (!sd->entity.num_pads) >> - return NULL; >> + state = kzalloc(sizeof(*state), GFP_KERNEL); >> + if (!state) >> + return ERR_PTR(-ENOMEM); >> >> - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), >> - GFP_KERNEL | __GFP_ZERO); >> - if (!cfg) >> - return NULL; >> - >> - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); >> - if (ret < 0 && ret != -ENOIOCTLCMD) { >> - kvfree(cfg); >> - return NULL; >> + if (sd->entity.num_pads) { >> + state->pads = kvmalloc_array(sd->entity.num_pads, >> + sizeof(*state->pads), >> + GFP_KERNEL | __GFP_ZERO); >> + if (!state->pads) { >> + ret = -ENOMEM; >> + goto err; >> + } >> } >> >> - return cfg; >> + ret = v4l2_subdev_call(sd, pad, init_cfg, state); > > The init_cfg callback should also be renamed to init_state, but this can > be done in a second patch. Yes, there are more renames that can be done, but I didn't want to explode the patch to even bigger size =). >> + if (ret < 0 && ret != -ENOIOCTLCMD) >> + goto err; >> + >> + return state; >> + >> +err: >> + if (state && state->pads) >> + kvfree(state->pads); >> + >> + kfree(state); >> + >> + return ERR_PTR(ret); >> } >> -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); >> +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); >> >> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) >> +void v4l2_subdev_free_state(struct v4l2_subdev_state *state) >> { >> - kvfree(cfg); >> + kvfree(state->pads); > > I'd change this to: > > if (state) > kvfree(state->pads); > > That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust. Yes, good point. >> + kfree(state); >> } >> -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); >> +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); >> + >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> >> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > > <snip> > >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index d0e9a5bdb08b..89115ba4c0f2 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config { > > I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state > or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology. > > This too can be done in a separate patch. > > I like the 'state' term much better than 'config'. Yes, I agree here too, and was in my list of "later". Thanks! Tomi
Hi all, On 09/06/2021 13:02, Tomi Valkeinen wrote: > Hi, > > v3 of the patch. I rebased on top of latest linux-media, and applied the > semantic patch. I've addressed Laurent's comment (fix kfree, kernel doc > fixes, return cleanup). > > v2 can be found from: > > https://lore.kernel.org/linux-media/20210527094244.617473-1-tomi.valkeinen@ideasonboard.com/ > I posted a few fixes to this as a continuation to the series (patches 2/5 - 5/5) in an ad-hoc manner, to help reviewing and avoid sending the half-a-megabyte patch. All those can be squashed to the original patch, or I can send a proper v4 after getting acks for the changes. Tomi
On 10/06/2021 11:51, Tomi Valkeinen wrote: > Hi all, > > On 09/06/2021 13:02, Tomi Valkeinen wrote: >> Hi, >> >> v3 of the patch. I rebased on top of latest linux-media, and applied the >> semantic patch. I've addressed Laurent's comment (fix kfree, kernel doc >> fixes, return cleanup). >> >> v2 can be found from: >> >> https://lore.kernel.org/linux-media/20210527094244.617473-1-tomi.valkeinen@ideasonboard.com/ >> > > I posted a few fixes to this as a continuation to the series (patches > 2/5 - 5/5) in an ad-hoc manner, to help reviewing and avoid sending the > half-a-megabyte patch. > > All those can be squashed to the original patch, or I can send a proper > v4 after getting acks for the changes. I can squash it to a single patch before making a PR. I'm happy with this series (1-5). I'd like an Acked-by from Laurent and/or Sakari as well, though. Regards, Hans > > Tomi >
Hello, On Thu, Jun 10, 2021 at 10:00:06AM +0200, Hans Verkuil wrote: > On 09/06/2021 12:02, Tomi Valkeinen wrote: > > We have 'struct v4l2_subdev_pad_config' which contains configuration for > > a single pad used for the TRY functionality, and an array of those > > structs is passed to various v4l2_subdev_pad_ops. > > > > I was working on subdev internal routing between pads, and realized that > > there's no way to add TRY functionality for routes, which is not pad > > specific configuration. Adding a separate struct for try-route config > > wouldn't work either, as e.g. set-fmt needs to know the try-route > > configuration to propagate the settings. > > > > This patch adds a new struct, 'struct v4l2_subdev_state' (which at the > > moment only contains the v4l2_subdev_pad_config array) and the new > > struct is used in most of the places where v4l2_subdev_pad_config was > > used. All v4l2_subdev_pad_ops functions taking v4l2_subdev_pad_config > > are changed to instead take v4l2_subdev_state. > > > > The changes to drivers/media/v4l2-core/v4l2-subdev.c and > > include/media/v4l2-subdev.h were written by hand, and all the driver > > changes were done with the semantic patch below. The spatch needs to be > > applied to a select list of directories. I used the following shell > > commands to apply the spatch: > > <snip> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 956dafab43d4..dacae53b68d5 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -26,19 +26,21 @@ > > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd) > > { > > - if (sd->entity.num_pads) { > > - fh->pad = v4l2_subdev_alloc_pad_config(sd); > > - if (fh->pad == NULL) > > - return -ENOMEM; > > - } > > + struct v4l2_subdev_state *state; > > + > > + state = v4l2_subdev_alloc_state(sd); > > + if (IS_ERR(state)) > > + return PTR_ERR(state); > > + > > + fh->state = state; > > > > return 0; > > } > > > > static void subdev_fh_free(struct v4l2_subdev_fh *fh) > > { > > - v4l2_subdev_free_pad_config(fh->pad); > > - fh->pad = NULL; > > + v4l2_subdev_free_state(fh->state); > > + fh->state = NULL; > > } > > > > static int subdev_open(struct file *file) > > @@ -146,63 +148,63 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) > > return 0; > > } > > > > -static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) > > +static int check_cfg(u32 which, struct v4l2_subdev_state *state) > > This should be renamed to check_state or check_state_pads (see next comment). > > > { > > - if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) > > + if (which == V4L2_SUBDEV_FORMAT_TRY && !state) > > Should this also check for !state->pads? At least in the current code > it really checks for the pad configuration, so I think it should. > > If so, then this function should probably be renamed to check_state_pads. > > > return -EINVAL; > > > > return 0; > > } > > > > static inline int check_format(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format) > > { > > if (!format) > > return -EINVAL; > > > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > > - check_cfg(format->which, cfg); > > + check_cfg(format->which, state); > > } > > > > static int call_get_fmt(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format) > > { > > - return check_format(sd, cfg, format) ? : > > - sd->ops->pad->get_fmt(sd, cfg, format); > > + return check_format(sd, state, format) ? : > > + sd->ops->pad->get_fmt(sd, state, format); > > } > > > > static int call_set_fmt(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format) > > { > > - return check_format(sd, cfg, format) ? : > > - sd->ops->pad->set_fmt(sd, cfg, format); > > + return check_format(sd, state, format) ? : > > + sd->ops->pad->set_fmt(sd, state, format); > > } > > > > static int call_enum_mbus_code(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_mbus_code_enum *code) > > { > > if (!code) > > return -EINVAL; > > > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > > - check_cfg(code->which, cfg) ? : > > - sd->ops->pad->enum_mbus_code(sd, cfg, code); > > + check_cfg(code->which, state) ? : > > + sd->ops->pad->enum_mbus_code(sd, state, code); > > } > > > > static int call_enum_frame_size(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_frame_size_enum *fse) > > { > > if (!fse) > > return -EINVAL; > > > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > > - check_cfg(fse->which, cfg) ? : > > - sd->ops->pad->enum_frame_size(sd, cfg, fse); > > + check_cfg(fse->which, state) ? : > > + sd->ops->pad->enum_frame_size(sd, state, fse); > > } > > > > static inline int check_frame_interval(struct v4l2_subdev *sd, > > @@ -229,42 +231,42 @@ static int call_s_frame_interval(struct v4l2_subdev *sd, > > } > > > > static int call_enum_frame_interval(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_frame_interval_enum *fie) > > { > > if (!fie) > > return -EINVAL; > > > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > > - check_cfg(fie->which, cfg) ? : > > - sd->ops->pad->enum_frame_interval(sd, cfg, fie); > > + check_cfg(fie->which, state) ? : > > + sd->ops->pad->enum_frame_interval(sd, state, fie); > > } > > > > static inline int check_selection(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel) > > { > > if (!sel) > > return -EINVAL; > > > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > > - check_cfg(sel->which, cfg); > > + check_cfg(sel->which, state); > > } > > > > static int call_get_selection(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel) > > { > > - return check_selection(sd, cfg, sel) ? : > > - sd->ops->pad->get_selection(sd, cfg, sel); > > + return check_selection(sd, state, sel) ? : > > + sd->ops->pad->get_selection(sd, state, sel); > > } > > > > static int call_set_selection(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel) > > { > > - return check_selection(sd, cfg, sel) ? : > > - sd->ops->pad->set_selection(sd, cfg, sel); > > + return check_selection(sd, state, sel) ? : > > + sd->ops->pad->set_selection(sd, state, sel); > > } > > > > static inline int check_edid(struct v4l2_subdev *sd, > > @@ -506,7 +508,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > memset(format->reserved, 0, sizeof(format->reserved)); > > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > > - return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format); > > + return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->state, format); > > } > > > > case VIDIOC_SUBDEV_S_FMT: { > > @@ -517,7 +519,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > memset(format->reserved, 0, sizeof(format->reserved)); > > memset(format->format.reserved, 0, sizeof(format->format.reserved)); > > - return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format); > > + return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->state, format); > > } > > > > case VIDIOC_SUBDEV_G_CROP: { > > @@ -531,7 +533,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > sel.target = V4L2_SEL_TGT_CROP; > > > > rval = v4l2_subdev_call( > > - sd, pad, get_selection, subdev_fh->pad, &sel); > > + sd, pad, get_selection, subdev_fh->state, &sel); > > > > crop->rect = sel.r; > > > > @@ -553,7 +555,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > sel.r = crop->rect; > > > > rval = v4l2_subdev_call( > > - sd, pad, set_selection, subdev_fh->pad, &sel); > > + sd, pad, set_selection, subdev_fh->state, &sel); > > > > crop->rect = sel.r; > > > > @@ -564,7 +566,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > struct v4l2_subdev_mbus_code_enum *code = arg; > > > > memset(code->reserved, 0, sizeof(code->reserved)); > > - return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad, > > + return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->state, > > code); > > } > > > > @@ -572,7 +574,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > struct v4l2_subdev_frame_size_enum *fse = arg; > > > > memset(fse->reserved, 0, sizeof(fse->reserved)); > > - return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad, > > + return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->state, > > fse); > > } > > > > @@ -597,7 +599,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > struct v4l2_subdev_frame_interval_enum *fie = arg; > > > > memset(fie->reserved, 0, sizeof(fie->reserved)); > > - return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad, > > + return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->state, > > fie); > > } > > > > @@ -606,7 +608,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > memset(sel->reserved, 0, sizeof(sel->reserved)); > > return v4l2_subdev_call( > > - sd, pad, get_selection, subdev_fh->pad, sel); > > + sd, pad, get_selection, subdev_fh->state, sel); > > } > > > > case VIDIOC_SUBDEV_S_SELECTION: { > > @@ -617,7 +619,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > > > > memset(sel->reserved, 0, sizeof(sel->reserved)); > > return v4l2_subdev_call( > > - sd, pad, set_selection, subdev_fh->pad, sel); > > + sd, pad, set_selection, subdev_fh->state, sel); > > } > > > > case VIDIOC_G_EDID: { > > @@ -892,35 +894,48 @@ int v4l2_subdev_link_validate(struct media_link *link) > > } > > EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate); > > > > -struct v4l2_subdev_pad_config * > > -v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd) > > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd) > > { > > - struct v4l2_subdev_pad_config *cfg; > > + struct v4l2_subdev_state *state; > > int ret; > > > > - if (!sd->entity.num_pads) > > - return NULL; > > + state = kzalloc(sizeof(*state), GFP_KERNEL); > > + if (!state) > > + return ERR_PTR(-ENOMEM); > > > > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), > > - GFP_KERNEL | __GFP_ZERO); > > - if (!cfg) > > - return NULL; > > - > > - ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); > > - if (ret < 0 && ret != -ENOIOCTLCMD) { > > - kvfree(cfg); > > - return NULL; > > + if (sd->entity.num_pads) { > > + state->pads = kvmalloc_array(sd->entity.num_pads, > > + sizeof(*state->pads), > > + GFP_KERNEL | __GFP_ZERO); > > + if (!state->pads) { > > + ret = -ENOMEM; > > + goto err; > > + } > > } > > > > - return cfg; > > + ret = v4l2_subdev_call(sd, pad, init_cfg, state); > > The init_cfg callback should also be renamed to init_state, but this can > be done in a second patch. > > > + if (ret < 0 && ret != -ENOIOCTLCMD) > > + goto err; > > + > > + return state; > > + > > +err: > > + if (state && state->pads) > > + kvfree(state->pads); > > + > > + kfree(state); > > + > > + return ERR_PTR(ret); > > } > > -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); > > +EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); > > > > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) > > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state) > > { > > - kvfree(cfg); > > + kvfree(state->pads); > > I'd change this to: > > if (state) > kvfree(state->pads); > > That way you can call v4l2_subdev_free_state() with a NULL pointer. It's more robust. I'd do if (!state) return; at the top in that case, as I expect the state will be extended over time, with more things to free. This can be done now or later, doesn't matter much to me. > > + kfree(state); > > } > > -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); > > +EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); > > + > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > > <snip> > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index d0e9a5bdb08b..89115ba4c0f2 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -623,6 +623,19 @@ struct v4l2_subdev_pad_config { > > I wonder if v4l2_subdev_pad_config shouldn't be renamed to v4l2_subdev_pad_state > or would v4l2_subdev_state_pad. It's more consistent with the 'state' terminology. > > This too can be done in a separate patch. > > I like the 'state' term much better than 'config'. > > > struct v4l2_rect try_compose; > > }; > > > > +/** > > + * struct v4l2_subdev_state - Used for storing subdev state information. > > + * > > + * @pads: &struct v4l2_subdev_pad_config array > > + * > > + * This structure only needs to be passed to the pad op if the 'which' field > > + * of the main argument is set to %V4L2_SUBDEV_FORMAT_TRY. For > > + * %V4L2_SUBDEV_FORMAT_ACTIVE it is safe to pass %NULL. > > + */ > > +struct v4l2_subdev_state { > > + struct v4l2_subdev_pad_config *pads; > > +}; > > + > > /** > > * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations > > * > > @@ -687,27 +700,27 @@ struct v4l2_subdev_pad_config { > > */ > > struct v4l2_subdev_pad_ops { > > int (*init_cfg)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg); > > + struct v4l2_subdev_state *state); > > int (*enum_mbus_code)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_mbus_code_enum *code); > > int (*enum_frame_size)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_frame_size_enum *fse); > > int (*enum_frame_interval)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_frame_interval_enum *fie); > > int (*get_fmt)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format); > > int (*set_fmt)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_format *format); > > int (*get_selection)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel); > > int (*set_selection)(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel); > > int (*get_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid); > > int (*set_edid)(struct v4l2_subdev *sd, struct v4l2_edid *edid); > > @@ -918,14 +931,14 @@ struct v4l2_subdev { > > * struct v4l2_subdev_fh - Used for storing subdev information per file handle > > * > > * @vfh: pointer to &struct v4l2_fh > > - * @pad: pointer to &struct v4l2_subdev_pad_config > > + * @state: pointer to &struct v4l2_subdev_state > > * @owner: module pointer to the owner of this file handle > > */ > > struct v4l2_subdev_fh { > > struct v4l2_fh vfh; > > struct module *owner; > > #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API) > > - struct v4l2_subdev_pad_config *pad; > > + struct v4l2_subdev_state *state; > > #endif > > }; > > > > @@ -945,17 +958,17 @@ struct v4l2_subdev_fh { > > * &struct v4l2_subdev_pad_config->try_fmt > > * > > * @sd: pointer to &struct v4l2_subdev > > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > > - * @pad: index of the pad in the @cfg array. > > + * @state: pointer to &struct v4l2_subdev_state > > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array > > */ > > static inline struct v4l2_mbus_framefmt * > > v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > unsigned int pad) > > { > > if (WARN_ON(pad >= sd->entity.num_pads)) > > pad = 0; > > - return &cfg[pad].try_fmt; > > + return &state->pads[pad].try_fmt; > > } > > > > /** > > @@ -963,17 +976,17 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > > * &struct v4l2_subdev_pad_config->try_crop > > * > > * @sd: pointer to &struct v4l2_subdev > > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > > - * @pad: index of the pad in the @cfg array. > > + * @state: pointer to &struct v4l2_subdev_state. > > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array. > > */ > > static inline struct v4l2_rect * > > v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > unsigned int pad) > > { > > if (WARN_ON(pad >= sd->entity.num_pads)) > > pad = 0; > > - return &cfg[pad].try_crop; > > + return &state->pads[pad].try_crop; > > } > > > > /** > > @@ -981,17 +994,17 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > > * &struct v4l2_subdev_pad_config->try_compose > > * > > * @sd: pointer to &struct v4l2_subdev > > - * @cfg: pointer to &struct v4l2_subdev_pad_config array. > > - * @pad: index of the pad in the @cfg array. > > + * @state: pointer to &struct v4l2_subdev_state. > > + * @pad: index of the pad in the &struct v4l2_subdev_state->pads array. > > */ > > static inline struct v4l2_rect * > > v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, > > - struct v4l2_subdev_pad_config *cfg, > > + struct v4l2_subdev_state *state, > > unsigned int pad) > > { > > if (WARN_ON(pad >= sd->entity.num_pads)) > > pad = 0; > > - return &cfg[pad].try_compose; > > + return &state->pads[pad].try_compose; > > } > > > > #endif > > @@ -1093,20 +1106,21 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd, > > int v4l2_subdev_link_validate(struct media_link *link); > > > > /** > > - * v4l2_subdev_alloc_pad_config - Allocates memory for pad config > > + * v4l2_subdev_alloc_state - allocate v4l2_subdev_state > > * > > - * @sd: pointer to struct v4l2_subdev > > + * @sd: pointer to &struct v4l2_subdev for which the state is being allocated. > > + * > > + * Must call v4l2_subdev_free_state() when state is no longer needed. > > */ > > -struct > > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); > > +struct v4l2_subdev_state *v4l2_subdev_alloc_state(struct v4l2_subdev *sd); > > > > /** > > - * v4l2_subdev_free_pad_config - Frees memory allocated by > > - * v4l2_subdev_alloc_pad_config(). > > + * v4l2_subdev_free_state - free a v4l2_subdev_state > > * > > - * @cfg: pointer to &struct v4l2_subdev_pad_config > > + * @state: v4l2_subdev_state to be freed. > > */ > > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); > > +void v4l2_subdev_free_state(struct v4l2_subdev_state *state); > > + > > #endif /* CONFIG_MEDIA_CONTROLLER */ > > > > /** -- Regards, : Laurent Pinchart
Hi Tomi, Thank you for the patch. On Thu, Jun 10, 2021 at 12:49:00PM +0300, Tomi Valkeinen wrote: > Fix a few indentation warnings from checkpatch. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/platform/omap3isp/ispccp2.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c > index 366da6fb8b4f..acb58b6ddba1 100644 > --- a/drivers/media/platform/omap3isp/ispccp2.c > +++ b/drivers/media/platform/omap3isp/ispccp2.c > @@ -619,8 +619,8 @@ static const unsigned int ccp2_fmts[] = { > */ > static struct v4l2_mbus_framefmt * > __ccp2_get_format(struct isp_ccp2_device *ccp2, > - struct v4l2_subdev_state *sd_state, > - unsigned int pad, enum v4l2_subdev_format_whence which) > + struct v4l2_subdev_state *sd_state, > + unsigned int pad, enum v4l2_subdev_format_whence which) > { > if (which == V4L2_SUBDEV_FORMAT_TRY) > return v4l2_subdev_get_try_format(&ccp2->subdev, sd_state, > @@ -708,7 +708,7 @@ static int ccp2_enum_mbus_code(struct v4l2_subdev *sd, > return -EINVAL; > > format = __ccp2_get_format(ccp2, sd_state, CCP2_PAD_SINK, > - code->which); > + code->which); > code->code = format->code; > } > > @@ -753,8 +753,8 @@ static int ccp2_enum_frame_size(struct v4l2_subdev *sd, > * return -EINVAL or zero on success > */ > static int ccp2_get_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *fmt) > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *fmt) > { > struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd); > struct v4l2_mbus_framefmt *format; > @@ -775,8 +775,8 @@ static int ccp2_get_format(struct v4l2_subdev *sd, > * returns zero > */ > static int ccp2_set_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *fmt) > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *fmt) > { > struct isp_ccp2_device *ccp2 = v4l2_get_subdevdata(sd); > struct v4l2_mbus_framefmt *format; -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Thu, Jun 10, 2021 at 12:49:01PM +0300, Tomi Valkeinen wrote: > "media: v4l2-subdev: add subdev-wide state struct" introduced some > kernel doc errors due to cfg -> sd_state rename. Fix these. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/i2c/imx274.c | 6 +++--- > drivers/media/i2c/imx334.c | 10 +++++----- > drivers/media/i2c/mt9m032.c | 4 ++-- > drivers/media/i2c/tvp514x.c | 6 +++--- > drivers/media/platform/vsp1/vsp1_entity.c | 6 +++--- > drivers/media/platform/xilinx/xilinx-csi2rxss.c | 6 +++--- > drivers/media/platform/xilinx/xilinx-vip.c | 4 ++-- > 7 files changed, 21 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c > index e0c413cc94d7..0dce92872176 100644 > --- a/drivers/media/i2c/imx274.c > +++ b/drivers/media/i2c/imx274.c > @@ -996,7 +996,7 @@ static int imx274_binning_goodness(struct stimx274 *imx274, > * Must be called with imx274->lock locked. > * > * @imx274: The device object > - * @cfg: The pad config we are editing for TRY requests > + * @sd_state: The subdev state we are editing for TRY requests > * @which: V4L2_SUBDEV_FORMAT_ACTIVE or V4L2_SUBDEV_FORMAT_TRY from the caller > * @width: Input-output parameter: set to the desired width before > * the call, contains the chosen value after returning successfully > @@ -1061,7 +1061,7 @@ static int __imx274_change_compose(struct stimx274 *imx274, > /** > * imx274_get_fmt - Get the pad format > * @sd: Pointer to V4L2 Sub device structure > - * @cfg: Pointer to sub device pad information structure > + * @sd_state: Pointer to sub device state structure > * @fmt: Pointer to pad level media bus format > * > * This function is used to get the pad format information. > @@ -1083,7 +1083,7 @@ static int imx274_get_fmt(struct v4l2_subdev *sd, > /** > * imx274_set_fmt - This is used to set the pad format > * @sd: Pointer to V4L2 Sub device structure > - * @cfg: Pointer to sub device pad information structure > + * @sd_state: Pointer to sub device state information structure > * @format: Pointer to pad level media bus format > * > * This function is used to set the pad format. > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c > index 751ce1f8f817..062125501788 100644 > --- a/drivers/media/i2c/imx334.c > +++ b/drivers/media/i2c/imx334.c > @@ -497,7 +497,7 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = { > /** > * imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes > * @sd: pointer to imx334 V4L2 sub-device structure > - * @cfg: V4L2 sub-device pad configuration > + * @sd_state: V4L2 sub-device state > * @code: V4L2 sub-device code enumeration need to be filled > * > * Return: 0 if successful, error code otherwise. > @@ -517,7 +517,7 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd, > /** > * imx334_enum_frame_size() - Enumerate V4L2 sub-device frame sizes > * @sd: pointer to imx334 V4L2 sub-device structure > - * @cfg: V4L2 sub-device pad configuration > + * @sd_state: V4L2 sub-device state > * @fsize: V4L2 sub-device size enumeration need to be filled > * > * Return: 0 if successful, error code otherwise. > @@ -564,7 +564,7 @@ static void imx334_fill_pad_format(struct imx334 *imx334, > /** > * imx334_get_pad_format() - Get subdevice pad format > * @sd: pointer to imx334 V4L2 sub-device structure > - * @cfg: V4L2 sub-device pad configuration > + * @sd_state: V4L2 sub-device state > * @fmt: V4L2 sub-device format need to be set > * > * Return: 0 if successful, error code otherwise. > @@ -594,7 +594,7 @@ static int imx334_get_pad_format(struct v4l2_subdev *sd, > /** > * imx334_set_pad_format() - Set subdevice pad format > * @sd: pointer to imx334 V4L2 sub-device structure > - * @cfg: V4L2 sub-device pad configuration > + * @sd_state: V4L2 sub-device state > * @fmt: V4L2 sub-device format need to be set > * > * Return: 0 if successful, error code otherwise. > @@ -631,7 +631,7 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd, > /** > * imx334_init_pad_cfg() - Initialize sub-device pad configuration > * @sd: pointer to imx334 V4L2 sub-device structure > - * @cfg: V4L2 sub-device pad configuration > + * @sd_state: V4L2 sub-device state > * > * Return: 0 if successful, error code otherwise. > */ > diff --git a/drivers/media/i2c/mt9m032.c b/drivers/media/i2c/mt9m032.c > index 8a0741058c98..ba0c0ea91c95 100644 > --- a/drivers/media/i2c/mt9m032.c > +++ b/drivers/media/i2c/mt9m032.c > @@ -332,7 +332,7 @@ static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev, > /** > * __mt9m032_get_pad_crop() - get crop rect > * @sensor: pointer to the sensor struct > - * @cfg: v4l2_subdev_pad_config for getting the try crop rect from > + * @sd_state: v4l2_subdev_state for getting the try crop rect from > * @which: select try or active crop rect > * > * Returns a pointer the current active or fh relative try crop rect > @@ -355,7 +355,7 @@ __mt9m032_get_pad_crop(struct mt9m032 *sensor, > /** > * __mt9m032_get_pad_format() - get format > * @sensor: pointer to the sensor struct > - * @cfg: v4l2_subdev_pad_config for getting the try format from > + * @sd_state: v4l2_subdev_state for getting the try format from > * @which: select try or active format > * > * Returns a pointer the current active or fh relative try format > diff --git a/drivers/media/i2c/tvp514x.c b/drivers/media/i2c/tvp514x.c > index 5f159588aaeb..cee60f945036 100644 > --- a/drivers/media/i2c/tvp514x.c > +++ b/drivers/media/i2c/tvp514x.c > @@ -853,7 +853,7 @@ static const struct v4l2_ctrl_ops tvp514x_ctrl_ops = { > /** > * tvp514x_enum_mbus_code() - V4L2 decoder interface handler for enum_mbus_code > * @sd: pointer to standard V4L2 sub-device structure > - * @cfg: pad configuration > + * @sd_state: subdev state > * @code: pointer to v4l2_subdev_mbus_code_enum structure > * > * Enumertaes mbus codes supported > @@ -880,7 +880,7 @@ static int tvp514x_enum_mbus_code(struct v4l2_subdev *sd, > /** > * tvp514x_get_pad_format() - V4L2 decoder interface handler for get pad format > * @sd: pointer to standard V4L2 sub-device structure > - * @cfg: pad configuration > + * @sd_state: subdev state > * @format: pointer to v4l2_subdev_format structure > * > * Retrieves pad format which is active or tried based on requirement > @@ -912,7 +912,7 @@ static int tvp514x_get_pad_format(struct v4l2_subdev *sd, > /** > * tvp514x_set_pad_format() - V4L2 decoder interface handler for set pad format > * @sd: pointer to standard V4L2 sub-device structure > - * @cfg: pad configuration > + * @sd_state: subdev state > * @fmt: pointer to v4l2_subdev_format structure > * > * Set pad format for the output pad > diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c > index 7d92262f3378..6ef874d29eb7 100644 > --- a/drivers/media/platform/vsp1/vsp1_entity.c > +++ b/drivers/media/platform/vsp1/vsp1_entity.c > @@ -103,7 +103,7 @@ void vsp1_entity_configure_partition(struct vsp1_entity *entity, > /** > * vsp1_entity_get_pad_config - Get the pad configuration for an entity > * @entity: the entity > - * @cfg: the TRY pad configuration > + * @sd_state: the TRY pad configuration Should this read "the TRY state" ? > * @which: configuration selector (ACTIVE or TRY) > * > * When called with which set to V4L2_SUBDEV_FORMAT_ACTIVE the caller must hold > @@ -131,7 +131,7 @@ vsp1_entity_get_pad_config(struct vsp1_entity *entity, > /** > * vsp1_entity_get_pad_format - Get a pad format from storage for an entity > * @entity: the entity > - * @cfg: the configuration storage > + * @sd_state: the configuration storage And here, "the state storage". Same below. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * @pad: the pad number > * > * Return the format stored in the given configuration for an entity's pad. The > @@ -148,7 +148,7 @@ vsp1_entity_get_pad_format(struct vsp1_entity *entity, > /** > * vsp1_entity_get_pad_selection - Get a pad selection from storage for entity > * @entity: the entity > - * @cfg: the configuration storage > + * @sd_state: the configuration storage > * @pad: the pad number > * @target: the selection target > * > diff --git a/drivers/media/platform/xilinx/xilinx-csi2rxss.c b/drivers/media/platform/xilinx/xilinx-csi2rxss.c > index 2773145a2226..b1baf9d7b6ec 100644 > --- a/drivers/media/platform/xilinx/xilinx-csi2rxss.c > +++ b/drivers/media/platform/xilinx/xilinx-csi2rxss.c > @@ -698,7 +698,7 @@ __xcsi2rxss_get_pad_format(struct xcsi2rxss_state *xcsi2rxss, > /** > * xcsi2rxss_init_cfg - Initialise the pad format config to default > * @sd: Pointer to V4L2 Sub device structure > - * @cfg: Pointer to sub device pad information structure > + * @sd_state: Pointer to sub device state structure > * > * This function is used to initialize the pad format with the default > * values. > @@ -725,7 +725,7 @@ static int xcsi2rxss_init_cfg(struct v4l2_subdev *sd, > /** > * xcsi2rxss_get_format - Get the pad format > * @sd: Pointer to V4L2 Sub device structure > - * @cfg: Pointer to sub device pad information structure > + * @sd_state: Pointer to sub device state structure > * @fmt: Pointer to pad level media bus format > * > * This function is used to get the pad format information. > @@ -750,7 +750,7 @@ static int xcsi2rxss_get_format(struct v4l2_subdev *sd, > /** > * xcsi2rxss_set_format - This is used to set the pad format > * @sd: Pointer to V4L2 Sub device structure > - * @cfg: Pointer to sub device pad information structure > + * @sd_state: Pointer to sub device state structure > * @fmt: Pointer to pad level media bus format > * > * This function is used to set the pad format. Since the pad format is fixed > diff --git a/drivers/media/platform/xilinx/xilinx-vip.c b/drivers/media/platform/xilinx/xilinx-vip.c > index b989fee8351d..07d16a007d5f 100644 > --- a/drivers/media/platform/xilinx/xilinx-vip.c > +++ b/drivers/media/platform/xilinx/xilinx-vip.c > @@ -234,7 +234,7 @@ EXPORT_SYMBOL_GPL(xvip_cleanup_resources); > /** > * xvip_enum_mbus_code - Enumerate the media format code > * @subdev: V4L2 subdevice > - * @cfg: V4L2 subdev pad configuration > + * @sd_state: V4L2 subdev state > * @code: returning media bus code > * > * Enumerate the media bus code of the subdevice. Return the corresponding > @@ -271,7 +271,7 @@ EXPORT_SYMBOL_GPL(xvip_enum_mbus_code); > /** > * xvip_enum_frame_size - Enumerate the media bus frame size > * @subdev: V4L2 subdevice > - * @cfg: V4L2 subdev pad configuration > + * @sd_state: V4L2 subdev state > * @fse: returning media bus frame size > * > * This function is a drop-in implementation of the subdev enum_frame_size pad -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Thu, Jun 10, 2021 at 12:49:02PM +0300, Tomi Valkeinen wrote: > Make v4l2_subdev_free_state() accept a NULL state. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index dacae53b68d5..25c80d6de23b 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -931,7 +931,8 @@ EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_state); > > void v4l2_subdev_free_state(struct v4l2_subdev_state *state) > { > - kvfree(state->pads); > + if (state) > + kvfree(state->pads); > kfree(state); I'd write if (!state) return; as I expect we'll have more code in the future. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > EXPORT_SYMBOL_GPL(v4l2_subdev_free_state); -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. On Thu, Jun 10, 2021 at 12:49:03PM +0300, Tomi Valkeinen wrote: > Rename check_cfg() to check_state_pads() as it is now checking the pads > of the state. > > Also fix the check so that it verifies that both state and state->pads > exist. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 25c80d6de23b..daf7ffdd8882 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -148,9 +148,9 @@ static inline int check_pad(struct v4l2_subdev *sd, u32 pad) > return 0; > } > > -static int check_cfg(u32 which, struct v4l2_subdev_state *state) > +static int check_state_pads(u32 which, struct v4l2_subdev_state *state) > { > - if (which == V4L2_SUBDEV_FORMAT_TRY && !state) > + if (which == V4L2_SUBDEV_FORMAT_TRY && (!state || !state->pads)) > return -EINVAL; > > return 0; > @@ -164,7 +164,7 @@ static inline int check_format(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(format->which) ? : check_pad(sd, format->pad) ? : > - check_cfg(format->which, state); > + check_state_pads(format->which, state); > } > > static int call_get_fmt(struct v4l2_subdev *sd, > @@ -191,7 +191,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(code->which) ? : check_pad(sd, code->pad) ? : > - check_cfg(code->which, state) ? : > + check_state_pads(code->which, state) ? : > sd->ops->pad->enum_mbus_code(sd, state, code); > } > > @@ -203,7 +203,7 @@ static int call_enum_frame_size(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(fse->which) ? : check_pad(sd, fse->pad) ? : > - check_cfg(fse->which, state) ? : > + check_state_pads(fse->which, state) ? : > sd->ops->pad->enum_frame_size(sd, state, fse); > } > > @@ -238,7 +238,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(fie->which) ? : check_pad(sd, fie->pad) ? : > - check_cfg(fie->which, state) ? : > + check_state_pads(fie->which, state) ? : > sd->ops->pad->enum_frame_interval(sd, state, fie); > } > > @@ -250,7 +250,7 @@ static inline int check_selection(struct v4l2_subdev *sd, > return -EINVAL; > > return check_which(sel->which) ? : check_pad(sd, sel->pad) ? : > - check_cfg(sel->which, state); > + check_state_pads(sel->which, state); > } > > static int call_get_selection(struct v4l2_subdev *sd, -- Regards, Laurent Pinchart