Message ID | 20210409133659.389544-1-tomi.valkeinen@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series | RFC: media: v4l2-subdev: add subdev-wide config struct | expand |
Hi Tomi, Thank you for the patch. Hans, Sakari, could you share your opinion on this ? I see lots of potential in this proposal (both for bikeshedding, and for actual value). We need an ack on the concept, and likely naming scheme too, before developing a final patch, as it's lots of churn in lots of drivers. On Fri, Apr 09, 2021 at 04:36:59PM +0300, 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_config' (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_config. > > Two drivers are changed to work with the above changes (drivers for HW > which I have) as an example. > > I worked on a semantic patch (included below, my first spatch...) to do > this change to all drivers, but hit lots of problems with non-trivial > uses of v4l2_subdev_pad_config. > > As it looks like substantial amount of manual work is needed, I'm > posting this RFC to get an ack on the changes before continuing that > work. > > @ v4l2_subdev_pad_ops @ > identifier pad_ops; > identifier func; > @@ > > ( > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_mbus_code = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_frame_size = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_frame_interval = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .get_fmt = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .set_fmt = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .get_selection = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .set_selection = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .init_cfg = func, > ..., > }; > ) > > @@ > identifier v4l2_subdev_pad_ops.func; > identifier sd; > identifier cfg; > @@ > > func(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > ... > ) > { > ... > } > > @@ > identifier v4l2_subdev_pad_ops.func; > identifier sd; > identifier cfg; > @@ > > func(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg > + struct v4l2_subdev_config *cfg > ) > { > ... > } > > @@ > struct v4l2_subdev_fh *fh; > @@ > - fh->pad > + &fh->cfg > > @@ > identifier func; > identifier cfg; > @@ > > func(..., > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > ...) > { > ... > } > > @@ > struct v4l2_subdev_config *cfg; > @@ > { > <... > ( > - cfg->try_fmt > + cfg->pad_configs->try_fmt > | > - cfg->try_crop > + cfg->pad_configs->try_crop > | > - cfg->try_compose > + cfg->pad_configs->try_compose > ) > ...> > } > > @@ > identifier pad_cfg; > @@ > { > ... > struct v4l2_subdev_pad_config pad_cfg; > + struct v4l2_subdev_config cfg = { .pad_configs = &pad_cfg }; > <... > - &pad_cfg > + &cfg > ...> > } > This doesn't look too bad (although the last change seems a bit weird), but I suppose it's not the whole story, as you've mentioned above. > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/i2c/ov5640.c | 10 +-- > drivers/media/platform/ti-vpe/cal-camerarx.c | 12 +-- > drivers/media/v4l2-core/v4l2-subdev.c | 85 ++++++++++---------- > include/media/v4l2-subdev.h | 49 +++++------ > 4 files changed, 78 insertions(+), 78 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 14f3afa7721a..7c76c0754426 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -2227,7 +2227,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > } > > static int ov5640_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2285,7 +2285,7 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, > } > > static int ov5640_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2818,7 +2818,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > } > > static int ov5640_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > if (fse->pad != 0) > @@ -2838,7 +2838,7 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd, > > static int ov5640_enum_frame_interval( > struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2924,7 +2924,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > } > > static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > if (code->pad != 0) > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index cbe6114908de..c708c61f2188 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -586,7 +586,7 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd) > > static struct v4l2_mbus_framefmt * > cal_camerarx_get_pad_format(struct cal_camerarx *phy, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad, u32 which) > { > switch (which) { > @@ -611,7 +611,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable) > } > > static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -639,7 +639,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -692,7 +692,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -742,7 +742,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg) > + struct v4l2_subdev_config *cfg) > { > struct v4l2_subdev_format format = { > .which = cfg ? V4L2_SUBDEV_FORMAT_TRY > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 956dafab43d4..248fa4ee9356 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -26,19 +26,18 @@ > #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; > - } > + int ret; > + > + ret = v4l2_subdev_init_config(sd, &fh->cfg); > + if (ret) > + return ret; > > return 0; > } > > static void subdev_fh_free(struct v4l2_subdev_fh *fh) > { > - v4l2_subdev_free_pad_config(fh->pad); > - fh->pad = NULL; > + v4l2_subdev_uninit_config(&fh->cfg); > } > > static int subdev_open(struct file *file) > @@ -146,7 +145,7 @@ 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_config *cfg) > { > if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) > return -EINVAL; > @@ -155,7 +154,7 @@ static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) > } > > static inline int check_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > if (!format) > @@ -166,7 +165,7 @@ static inline int check_format(struct v4l2_subdev *sd, > } > > static int call_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > return check_format(sd, cfg, format) ? : > @@ -174,7 +173,7 @@ static int call_get_fmt(struct v4l2_subdev *sd, > } > > static int call_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > return check_format(sd, cfg, format) ? : > @@ -182,7 +181,7 @@ static int call_set_fmt(struct v4l2_subdev *sd, > } > > static int call_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > if (!code) > @@ -194,7 +193,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > } > > static int call_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > if (!fse) > @@ -229,7 +228,7 @@ 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_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie) > { > if (!fie) > @@ -241,7 +240,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > } > > static inline int check_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > if (!sel) > @@ -252,7 +251,7 @@ static inline int check_selection(struct v4l2_subdev *sd, > } > > static int call_get_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > return check_selection(sd, cfg, sel) ? : > @@ -260,7 +259,7 @@ static int call_get_selection(struct v4l2_subdev *sd, > } > > static int call_set_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > return check_selection(sd, cfg, sel) ? : > @@ -506,7 +505,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->cfg, format); > } > > case VIDIOC_SUBDEV_S_FMT: { > @@ -517,7 +516,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->cfg, format); > } > > case VIDIOC_SUBDEV_G_CROP: { > @@ -531,7 +530,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->cfg, &sel); > > crop->rect = sel.r; > > @@ -553,7 +552,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->cfg, &sel); > > crop->rect = sel.r; > > @@ -564,7 +563,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->cfg, > code); > } > > @@ -572,7 +571,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->cfg, > fse); > } > > @@ -597,7 +596,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->cfg, > fie); > } > > @@ -606,7 +605,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->cfg, sel); > } > > case VIDIOC_SUBDEV_S_SELECTION: { > @@ -617,7 +616,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->cfg, sel); > } > > case VIDIOC_G_EDID: { So far, it's a pretty simple change :-) I'd actually take it one step further (on top of course), and store an instance of v4l2_subdev_config in v4l2_subdev for the active configuration. This can massively simplify subdev drivers. > @@ -892,35 +891,35 @@ 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) > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg) Line wrap (and below too). > { > - struct v4l2_subdev_pad_config *cfg; > int ret; > > - if (!sd->entity.num_pads) > - return NULL; > - > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), > - GFP_KERNEL | __GFP_ZERO); > - if (!cfg) > - return NULL; > + if (sd->entity.num_pads) { > + cfg->pad_configs = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg->pad_configs), > + GFP_KERNEL | __GFP_ZERO); > + if (!cfg->pad_configs) > + return -ENOMEM; > + } else { > + cfg->pad_configs = NULL; > + } How about memsetting the whole structure to 0 first ? > > ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); > if (ret < 0 && ret != -ENOIOCTLCMD) { > - kvfree(cfg); > - return NULL; > + kvfree(cfg->pad_configs); > + return ret; > } > > - return cfg; > + return 0; > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_init_config); > > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) > +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg) > { > - kvfree(cfg); > + kvfree(cfg->pad_configs); > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_uninit_config); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index d0e9a5bdb08b..1873e527987a 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -623,6 +623,10 @@ struct v4l2_subdev_pad_config { > struct v4l2_rect try_compose; > }; > > +struct v4l2_subdev_config { > + struct v4l2_subdev_pad_config *pad_configs; s/pad_configs/pads/ ? I'm tempted to also store the number of pads here. It would be redundant as the value is available from the v4l2_subdev structure, but it would allow operating on v4l2_subdev_config without a subdev pointer. Or, possibly better, we could store a pointer to the subdev. I'm also tempted to name the structure v4l2_subdev_state, as that's what it stores. I may be biased by DRM/KMS, but I don't see a downside in using identical names for identical concepts. > +}; > + > /** > * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations > * > @@ -687,27 +691,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_config *cfg); > int (*enum_mbus_code)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code); > int (*enum_frame_size)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse); > int (*enum_frame_interval)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie); > int (*get_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format); > int (*set_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format); > int (*get_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel); > int (*set_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > 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); > @@ -925,7 +929,7 @@ 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_config cfg; > #endif > }; > > @@ -950,12 +954,12 @@ struct v4l2_subdev_fh { > */ > static inline struct v4l2_mbus_framefmt * > v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_fmt; > + return &cfg->pad_configs[pad].try_fmt; > } > > /** > @@ -968,12 +972,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_crop; > + return &cfg->pad_configs[pad].try_crop; > } > > /** > @@ -986,12 +990,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_compose; > + return &cfg->pad_configs[pad].try_compose; > } > > #endif > @@ -1093,20 +1097,17 @@ 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_init_config - initialize v4l2_subdev_config > * > - * @sd: pointer to struct v4l2_subdev > + * Must call v4l2_subdev_uninit_config() when config is no longer needed. > */ > -struct > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg); > > /** > - * v4l2_subdev_free_pad_config - Frees memory allocated by > - * v4l2_subdev_alloc_pad_config(). > - * > - * @cfg: pointer to &struct v4l2_subdev_pad_config > + * v4l2_subdev_uninit_config - uninitialize v4l2_subdev_config > */ > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); > +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg); Finally, still on the naming topic, if you agree with v4l2_subdev_state, I'd name the function v4l2_subdev_state_init() and v4l2_subdev_state_cleanup(). I have a feeling we may be better off allocating states dynamically (storing a pointer in v4l2_subdev_fh instead of embedding an instance), in which case v4l2_subdev_state_alloc() and v4l2_subdev_state_free() would be better, but that's just a feeling at this point. > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** -- Regards, Laurent Pinchart
On 18/04/2021 18:07, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > Hans, Sakari, could you share your opinion on this ? I see lots of > potential in this proposal (both for bikeshedding, and for actual > value). We need an ack on the concept, and likely naming scheme too, > before developing a final patch, as it's lots of churn in lots of > drivers. > > On Fri, Apr 09, 2021 at 04:36:59PM +0300, 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_config' (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_config. >> >> Two drivers are changed to work with the above changes (drivers for HW >> which I have) as an example. >> >> I worked on a semantic patch (included below, my first spatch...) to do >> this change to all drivers, but hit lots of problems with non-trivial >> uses of v4l2_subdev_pad_config. >> >> As it looks like substantial amount of manual work is needed, I'm >> posting this RFC to get an ack on the changes before continuing that >> work. >> >> @ v4l2_subdev_pad_ops @ >> identifier pad_ops; >> identifier func; >> @@ >> >> ( >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .enum_mbus_code = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .enum_frame_size = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .enum_frame_interval = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .get_fmt = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .set_fmt = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .get_selection = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .set_selection = func, >> ..., >> }; >> | >> static const struct v4l2_subdev_pad_ops pad_ops = { >> ..., >> .init_cfg = func, >> ..., >> }; >> ) >> >> @@ >> identifier v4l2_subdev_pad_ops.func; >> identifier sd; >> identifier cfg; >> @@ >> >> func(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> ... >> ) >> { >> ... >> } >> >> @@ >> identifier v4l2_subdev_pad_ops.func; >> identifier sd; >> identifier cfg; >> @@ >> >> func(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg >> + struct v4l2_subdev_config *cfg >> ) >> { >> ... >> } >> >> @@ >> struct v4l2_subdev_fh *fh; >> @@ >> - fh->pad >> + &fh->cfg >> >> @@ >> identifier func; >> identifier cfg; >> @@ >> >> func(..., >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> ...) >> { >> ... >> } >> >> @@ >> struct v4l2_subdev_config *cfg; >> @@ >> { >> <... >> ( >> - cfg->try_fmt >> + cfg->pad_configs->try_fmt >> | >> - cfg->try_crop >> + cfg->pad_configs->try_crop >> | >> - cfg->try_compose >> + cfg->pad_configs->try_compose >> ) >> ...> >> } >> >> @@ >> identifier pad_cfg; >> @@ >> { >> ... >> struct v4l2_subdev_pad_config pad_cfg; >> + struct v4l2_subdev_config cfg = { .pad_configs = &pad_cfg }; >> <... >> - &pad_cfg >> + &cfg >> ...> >> } >> > > This doesn't look too bad (although the last change seems a bit weird), > but I suppose it's not the whole story, as you've mentioned above. > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/i2c/ov5640.c | 10 +-- >> drivers/media/platform/ti-vpe/cal-camerarx.c | 12 +-- >> drivers/media/v4l2-core/v4l2-subdev.c | 85 ++++++++++---------- >> include/media/v4l2-subdev.h | 49 +++++------ >> 4 files changed, 78 insertions(+), 78 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 14f3afa7721a..7c76c0754426 100644 >> --- a/drivers/media/i2c/ov5640.c >> +++ b/drivers/media/i2c/ov5640.c >> @@ -2227,7 +2227,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor, >> } >> >> static int ov5640_get_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> struct ov5640_dev *sensor = to_ov5640_dev(sd); >> @@ -2285,7 +2285,7 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, >> } >> >> static int ov5640_set_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> struct ov5640_dev *sensor = to_ov5640_dev(sd); >> @@ -2818,7 +2818,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) >> } >> >> static int ov5640_enum_frame_size(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> if (fse->pad != 0) >> @@ -2838,7 +2838,7 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd, >> >> static int ov5640_enum_frame_interval( >> struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_interval_enum *fie) >> { >> struct ov5640_dev *sensor = to_ov5640_dev(sd); >> @@ -2924,7 +2924,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, >> } >> >> static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_mbus_code_enum *code) >> { >> if (code->pad != 0) >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >> index cbe6114908de..c708c61f2188 100644 >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >> @@ -586,7 +586,7 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd) >> >> static struct v4l2_mbus_framefmt * >> cal_camerarx_get_pad_format(struct cal_camerarx *phy, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> unsigned int pad, u32 which) >> { >> switch (which) { >> @@ -611,7 +611,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable) >> } >> >> static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_mbus_code_enum *code) >> { >> struct cal_camerarx *phy = to_cal_camerarx(sd); >> @@ -639,7 +639,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, >> } >> >> static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> struct cal_camerarx *phy = to_cal_camerarx(sd); >> @@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, >> } >> >> static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> struct cal_camerarx *phy = to_cal_camerarx(sd); >> @@ -692,7 +692,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, >> } >> >> static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> struct cal_camerarx *phy = to_cal_camerarx(sd); >> @@ -742,7 +742,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, >> } >> >> static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg) >> + struct v4l2_subdev_config *cfg) >> { >> struct v4l2_subdev_format format = { >> .which = cfg ? V4L2_SUBDEV_FORMAT_TRY >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 956dafab43d4..248fa4ee9356 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -26,19 +26,18 @@ >> #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; >> - } >> + int ret; >> + >> + ret = v4l2_subdev_init_config(sd, &fh->cfg); >> + if (ret) >> + return ret; >> >> return 0; >> } >> >> static void subdev_fh_free(struct v4l2_subdev_fh *fh) >> { >> - v4l2_subdev_free_pad_config(fh->pad); >> - fh->pad = NULL; >> + v4l2_subdev_uninit_config(&fh->cfg); >> } >> >> static int subdev_open(struct file *file) >> @@ -146,7 +145,7 @@ 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_config *cfg) >> { >> if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) >> return -EINVAL; >> @@ -155,7 +154,7 @@ static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) >> } >> >> static inline int check_format(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> if (!format) >> @@ -166,7 +165,7 @@ static inline int check_format(struct v4l2_subdev *sd, >> } >> >> static int call_get_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> return check_format(sd, cfg, format) ? : >> @@ -174,7 +173,7 @@ static int call_get_fmt(struct v4l2_subdev *sd, >> } >> >> static int call_set_fmt(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format) >> { >> return check_format(sd, cfg, format) ? : >> @@ -182,7 +181,7 @@ static int call_set_fmt(struct v4l2_subdev *sd, >> } >> >> static int call_enum_mbus_code(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_mbus_code_enum *code) >> { >> if (!code) >> @@ -194,7 +193,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, >> } >> >> static int call_enum_frame_size(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_size_enum *fse) >> { >> if (!fse) >> @@ -229,7 +228,7 @@ 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_config *cfg, >> struct v4l2_subdev_frame_interval_enum *fie) >> { >> if (!fie) >> @@ -241,7 +240,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, >> } >> >> static inline int check_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_selection *sel) >> { >> if (!sel) >> @@ -252,7 +251,7 @@ static inline int check_selection(struct v4l2_subdev *sd, >> } >> >> static int call_get_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_selection *sel) >> { >> return check_selection(sd, cfg, sel) ? : >> @@ -260,7 +259,7 @@ static int call_get_selection(struct v4l2_subdev *sd, >> } >> >> static int call_set_selection(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_selection *sel) >> { >> return check_selection(sd, cfg, sel) ? : >> @@ -506,7 +505,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->cfg, format); >> } >> >> case VIDIOC_SUBDEV_S_FMT: { >> @@ -517,7 +516,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->cfg, format); >> } >> >> case VIDIOC_SUBDEV_G_CROP: { >> @@ -531,7 +530,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->cfg, &sel); >> >> crop->rect = sel.r; >> >> @@ -553,7 +552,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->cfg, &sel); >> >> crop->rect = sel.r; >> >> @@ -564,7 +563,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->cfg, >> code); >> } >> >> @@ -572,7 +571,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->cfg, >> fse); >> } >> >> @@ -597,7 +596,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->cfg, >> fie); >> } >> >> @@ -606,7 +605,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->cfg, sel); >> } >> >> case VIDIOC_SUBDEV_S_SELECTION: { >> @@ -617,7 +616,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->cfg, sel); >> } >> >> case VIDIOC_G_EDID: { > > So far, it's a pretty simple change :-) > > I'd actually take it one step further (on top of course), and store an > instance of v4l2_subdev_config in v4l2_subdev for the active > configuration. This can massively simplify subdev drivers. Hmm, struct v4l2_subdev_pad_config talks about try_fmt/crop/compose, that would be weird for the active configuration. I guess you would have to experiment a bit with this. > >> @@ -892,35 +891,35 @@ 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) >> +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg) > > Line wrap (and below too). > >> { >> - struct v4l2_subdev_pad_config *cfg; >> int ret; >> >> - if (!sd->entity.num_pads) >> - return NULL; >> - >> - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), >> - GFP_KERNEL | __GFP_ZERO); >> - if (!cfg) >> - return NULL; >> + if (sd->entity.num_pads) { >> + cfg->pad_configs = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg->pad_configs), >> + GFP_KERNEL | __GFP_ZERO); >> + if (!cfg->pad_configs) >> + return -ENOMEM; >> + } else { >> + cfg->pad_configs = NULL; >> + } > > How about memsetting the whole structure to 0 first ? > >> >> ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); >> if (ret < 0 && ret != -ENOIOCTLCMD) { >> - kvfree(cfg); >> - return NULL; >> + kvfree(cfg->pad_configs); >> + return ret; >> } >> >> - return cfg; >> + return 0; >> } >> -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); >> +EXPORT_SYMBOL_GPL(v4l2_subdev_init_config); >> >> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) >> +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg) >> { >> - kvfree(cfg); >> + kvfree(cfg->pad_configs); >> } >> -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); >> +EXPORT_SYMBOL_GPL(v4l2_subdev_uninit_config); >> + >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> >> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h >> index d0e9a5bdb08b..1873e527987a 100644 >> --- a/include/media/v4l2-subdev.h >> +++ b/include/media/v4l2-subdev.h >> @@ -623,6 +623,10 @@ struct v4l2_subdev_pad_config { >> struct v4l2_rect try_compose; >> }; >> >> +struct v4l2_subdev_config { >> + struct v4l2_subdev_pad_config *pad_configs; > > s/pad_configs/pads/ ? I agree. > > I'm tempted to also store the number of pads here. It would be redundant > as the value is available from the v4l2_subdev structure, but it would > allow operating on v4l2_subdev_config without a subdev pointer. Or, > possibly better, we could store a pointer to the subdev. > > I'm also tempted to name the structure v4l2_subdev_state, as that's what > it stores. I may be biased by DRM/KMS, but I don't see a downside in > using identical names for identical concepts. I like v4l2_subdev_state better than v4l2_subdev_config, so I would be OK with such a change. > >> +}; >> + >> /** >> * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations >> * >> @@ -687,27 +691,27 @@ struct v4l2_subdev_pad_config { >> */ >> struct v4l2_subdev_pad_ops { >> int (*init_cfg)(struct v4l2_subdev *sd, If we do change the name, then this probably should be renamed to init_state as well. >> - struct v4l2_subdev_pad_config *cfg); >> + struct v4l2_subdev_config *cfg); >> int (*enum_mbus_code)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_mbus_code_enum *code); >> int (*enum_frame_size)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_size_enum *fse); >> int (*enum_frame_interval)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_frame_interval_enum *fie); >> int (*get_fmt)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format); >> int (*set_fmt)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_format *format); >> int (*get_selection)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> struct v4l2_subdev_selection *sel); >> int (*set_selection)(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> 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); >> @@ -925,7 +929,7 @@ 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_config cfg; And of course cfg becomes 'state' too. >> #endif >> }; >> >> @@ -950,12 +954,12 @@ struct v4l2_subdev_fh { >> */ >> static inline struct v4l2_mbus_framefmt * >> v4l2_subdev_get_try_format(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> unsigned int pad) >> { >> if (WARN_ON(pad >= sd->entity.num_pads)) >> pad = 0; >> - return &cfg[pad].try_fmt; >> + return &cfg->pad_configs[pad].try_fmt; >> } >> >> /** >> @@ -968,12 +972,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, >> */ >> static inline struct v4l2_rect * >> v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> unsigned int pad) >> { >> if (WARN_ON(pad >= sd->entity.num_pads)) >> pad = 0; >> - return &cfg[pad].try_crop; >> + return &cfg->pad_configs[pad].try_crop; >> } >> >> /** >> @@ -986,12 +990,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, >> */ >> static inline struct v4l2_rect * >> v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, >> - struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_config *cfg, >> unsigned int pad) >> { >> if (WARN_ON(pad >= sd->entity.num_pads)) >> pad = 0; >> - return &cfg[pad].try_compose; >> + return &cfg->pad_configs[pad].try_compose; >> } >> >> #endif >> @@ -1093,20 +1097,17 @@ 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_init_config - initialize v4l2_subdev_config >> * >> - * @sd: pointer to struct v4l2_subdev >> + * Must call v4l2_subdev_uninit_config() when config is no longer needed. >> */ >> -struct >> -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); >> +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg); >> >> /** >> - * v4l2_subdev_free_pad_config - Frees memory allocated by >> - * v4l2_subdev_alloc_pad_config(). >> - * >> - * @cfg: pointer to &struct v4l2_subdev_pad_config >> + * v4l2_subdev_uninit_config - uninitialize v4l2_subdev_config >> */ >> -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); >> +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg); > > Finally, still on the naming topic, if you agree with v4l2_subdev_state, > I'd name the function v4l2_subdev_state_init() and > v4l2_subdev_state_cleanup(). I have a feeling we may be better off > allocating states dynamically (storing a pointer in v4l2_subdev_fh > instead of embedding an instance), in which case > v4l2_subdev_state_alloc() and v4l2_subdev_state_free() would be better, > but that's just a feeling at this point. > >> + >> #endif /* CONFIG_MEDIA_CONTROLLER */ >> >> /** > Regards, Hans
Moi, Thanks for the patch. On Fri, Apr 09, 2021 at 04:36:59PM +0300, 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_config' (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_config. > > Two drivers are changed to work with the above changes (drivers for HW > which I have) as an example. > > I worked on a semantic patch (included below, my first spatch...) to do > this change to all drivers, but hit lots of problems with non-trivial > uses of v4l2_subdev_pad_config. > > As it looks like substantial amount of manual work is needed, I'm > posting this RFC to get an ack on the changes before continuing that > work. Previously the try configuration has been just about pads, and routing isn't specific to them now. I think this change makes sense. Please see my comments below. > > @ v4l2_subdev_pad_ops @ > identifier pad_ops; > identifier func; > @@ > > ( > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_mbus_code = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_frame_size = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .enum_frame_interval = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .get_fmt = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .set_fmt = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .get_selection = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .set_selection = func, > ..., > }; > | > static const struct v4l2_subdev_pad_ops pad_ops = { > ..., > .init_cfg = func, > ..., > }; > ) > > @@ > identifier v4l2_subdev_pad_ops.func; > identifier sd; > identifier cfg; > @@ > > func(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > ... > ) > { > ... > } > > @@ > identifier v4l2_subdev_pad_ops.func; > identifier sd; > identifier cfg; > @@ > > func(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg > + struct v4l2_subdev_config *cfg > ) > { > ... > } > > @@ > struct v4l2_subdev_fh *fh; > @@ > - fh->pad > + &fh->cfg > > @@ > identifier func; > identifier cfg; > @@ > > func(..., > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > ...) > { > ... > } > > @@ > struct v4l2_subdev_config *cfg; > @@ > { > <... > ( > - cfg->try_fmt > + cfg->pad_configs->try_fmt > | > - cfg->try_crop > + cfg->pad_configs->try_crop > | > - cfg->try_compose > + cfg->pad_configs->try_compose > ) > ...> > } > > @@ > identifier pad_cfg; > @@ > { > ... > struct v4l2_subdev_pad_config pad_cfg; > + struct v4l2_subdev_config cfg = { .pad_configs = &pad_cfg }; > <... > - &pad_cfg > + &cfg > ...> > } > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/i2c/ov5640.c | 10 +-- > drivers/media/platform/ti-vpe/cal-camerarx.c | 12 +-- > drivers/media/v4l2-core/v4l2-subdev.c | 85 ++++++++++---------- > include/media/v4l2-subdev.h | 49 +++++------ > 4 files changed, 78 insertions(+), 78 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 14f3afa7721a..7c76c0754426 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -2227,7 +2227,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor, > } > > static int ov5640_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2285,7 +2285,7 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, > } > > static int ov5640_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2818,7 +2818,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) > } > > static int ov5640_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > if (fse->pad != 0) > @@ -2838,7 +2838,7 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd, > > static int ov5640_enum_frame_interval( > struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie) > { > struct ov5640_dev *sensor = to_ov5640_dev(sd); > @@ -2924,7 +2924,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, > } > > static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > if (code->pad != 0) > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index cbe6114908de..c708c61f2188 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -586,7 +586,7 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd) > > static struct v4l2_mbus_framefmt * > cal_camerarx_get_pad_format(struct cal_camerarx *phy, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad, u32 which) > { > switch (which) { > @@ -611,7 +611,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable) > } > > static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -639,7 +639,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -692,7 +692,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > struct cal_camerarx *phy = to_cal_camerarx(sd); > @@ -742,7 +742,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, > } > > static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg) > + struct v4l2_subdev_config *cfg) > { > struct v4l2_subdev_format format = { > .which = cfg ? V4L2_SUBDEV_FORMAT_TRY > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 956dafab43d4..248fa4ee9356 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -26,19 +26,18 @@ > #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; > - } > + int ret; > + > + ret = v4l2_subdev_init_config(sd, &fh->cfg); > + if (ret) > + return ret; You can do: return v4l2_subdev_init_config(sd, &fh->cfg); > > return 0; > } > > static void subdev_fh_free(struct v4l2_subdev_fh *fh) > { > - v4l2_subdev_free_pad_config(fh->pad); > - fh->pad = NULL; > + v4l2_subdev_uninit_config(&fh->cfg); > } > > static int subdev_open(struct file *file) > @@ -146,7 +145,7 @@ 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_config *cfg) > { > if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) > return -EINVAL; > @@ -155,7 +154,7 @@ static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) > } > > static inline int check_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > if (!format) > @@ -166,7 +165,7 @@ static inline int check_format(struct v4l2_subdev *sd, > } > > static int call_get_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > return check_format(sd, cfg, format) ? : > @@ -174,7 +173,7 @@ static int call_get_fmt(struct v4l2_subdev *sd, > } > > static int call_set_fmt(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format) > { > return check_format(sd, cfg, format) ? : > @@ -182,7 +181,7 @@ static int call_set_fmt(struct v4l2_subdev *sd, > } > > static int call_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code) > { > if (!code) > @@ -194,7 +193,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, > } > > static int call_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse) > { > if (!fse) > @@ -229,7 +228,7 @@ 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_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie) > { > if (!fie) > @@ -241,7 +240,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, > } > > static inline int check_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > if (!sel) > @@ -252,7 +251,7 @@ static inline int check_selection(struct v4l2_subdev *sd, > } > > static int call_get_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > return check_selection(sd, cfg, sel) ? : > @@ -260,7 +259,7 @@ static int call_get_selection(struct v4l2_subdev *sd, > } > > static int call_set_selection(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel) > { > return check_selection(sd, cfg, sel) ? : > @@ -506,7 +505,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->cfg, format); > } > > case VIDIOC_SUBDEV_S_FMT: { > @@ -517,7 +516,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->cfg, format); > } > > case VIDIOC_SUBDEV_G_CROP: { > @@ -531,7 +530,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->cfg, &sel); > > crop->rect = sel.r; > > @@ -553,7 +552,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->cfg, &sel); > > crop->rect = sel.r; > > @@ -564,7 +563,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->cfg, > code); > } > > @@ -572,7 +571,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->cfg, > fse); > } > > @@ -597,7 +596,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->cfg, > fie); > } > > @@ -606,7 +605,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->cfg, sel); > } > > case VIDIOC_SUBDEV_S_SELECTION: { > @@ -617,7 +616,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->cfg, sel); > } > > case VIDIOC_G_EDID: { > @@ -892,35 +891,35 @@ 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) > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg) > { > - struct v4l2_subdev_pad_config *cfg; > int ret; > > - if (!sd->entity.num_pads) > - return NULL; > - > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), > - GFP_KERNEL | __GFP_ZERO); > - if (!cfg) > - return NULL; > + if (sd->entity.num_pads) { > + cfg->pad_configs = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg->pad_configs), > + GFP_KERNEL | __GFP_ZERO); > + if (!cfg->pad_configs) > + return -ENOMEM; > + } else { > + cfg->pad_configs = NULL; > + } > > ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); > if (ret < 0 && ret != -ENOIOCTLCMD) { > - kvfree(cfg); > - return NULL; > + kvfree(cfg->pad_configs); You can call v4l2_subdev_uninit_config() here... > + return ret; > } > > - return cfg; > + return 0; > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_init_config); > > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) > +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg) > { > - kvfree(cfg); > + kvfree(cfg->pad_configs); Please set cfg->pad_configs to NULL here. > } > -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); > +EXPORT_SYMBOL_GPL(v4l2_subdev_uninit_config); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index d0e9a5bdb08b..1873e527987a 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -623,6 +623,10 @@ struct v4l2_subdev_pad_config { > struct v4l2_rect try_compose; > }; > > +struct v4l2_subdev_config { > + struct v4l2_subdev_pad_config *pad_configs; Could this be called just "pad"? > +}; > + > /** > * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations > * > @@ -687,27 +691,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_config *cfg); > int (*enum_mbus_code)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_mbus_code_enum *code); > int (*enum_frame_size)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_size_enum *fse); > int (*enum_frame_interval)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_frame_interval_enum *fie); > int (*get_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format); > int (*set_fmt)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_format *format); > int (*get_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > struct v4l2_subdev_selection *sel); > int (*set_selection)(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > 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); > @@ -925,7 +929,7 @@ 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_config cfg; > #endif > }; > > @@ -950,12 +954,12 @@ struct v4l2_subdev_fh { > */ > static inline struct v4l2_mbus_framefmt * > v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_fmt; > + return &cfg->pad_configs[pad].try_fmt; > } > > /** > @@ -968,12 +972,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_crop; > + return &cfg->pad_configs[pad].try_crop; > } > > /** > @@ -986,12 +990,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, > */ > static inline struct v4l2_rect * > v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, > - struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_config *cfg, > unsigned int pad) > { > if (WARN_ON(pad >= sd->entity.num_pads)) > pad = 0; > - return &cfg[pad].try_compose; > + return &cfg->pad_configs[pad].try_compose; > } > > #endif > @@ -1093,20 +1097,17 @@ 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_init_config - initialize v4l2_subdev_config > * > - * @sd: pointer to struct v4l2_subdev > + * Must call v4l2_subdev_uninit_config() when config is no longer needed. > */ > -struct > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg); > > /** > - * v4l2_subdev_free_pad_config - Frees memory allocated by > - * v4l2_subdev_alloc_pad_config(). > - * > - * @cfg: pointer to &struct v4l2_subdev_pad_config > + * v4l2_subdev_uninit_config - uninitialize v4l2_subdev_config > */ > -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); > +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg); > + > #endif /* CONFIG_MEDIA_CONTROLLER */ > > /** -- Kind regards, Sakari Ailus
A few more more mundane comments... On Tue, Apr 20, 2021 at 06:35:10PM +0300, Sakari Ailus wrote: ... > > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg) > > { > > - struct v4l2_subdev_pad_config *cfg; > > int ret; > > > > - if (!sd->entity.num_pads) > > - return NULL; > > - > > - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), > > - GFP_KERNEL | __GFP_ZERO); > > - if (!cfg) > > - return NULL; > > + if (sd->entity.num_pads) { > > + cfg->pad_configs = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg->pad_configs), > > + GFP_KERNEL | __GFP_ZERO); Please also pay attention to line length and aligning arguments. ... > > @@ -1093,20 +1097,17 @@ 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_init_config - initialize v4l2_subdev_config > > * > > - * @sd: pointer to struct v4l2_subdev > > + * Must call v4l2_subdev_uninit_config() when config is no longer needed. > > */ > > -struct > > -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); > > +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg); This should be wrapped, too... there are some others as well in this patch I think. -- Sakari Ailus
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 14f3afa7721a..7c76c0754426 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2227,7 +2227,7 @@ static int ov5640_try_frame_interval(struct ov5640_dev *sensor, } static int ov5640_get_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { struct ov5640_dev *sensor = to_ov5640_dev(sd); @@ -2285,7 +2285,7 @@ static int ov5640_try_fmt_internal(struct v4l2_subdev *sd, } static int ov5640_set_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { struct ov5640_dev *sensor = to_ov5640_dev(sd); @@ -2818,7 +2818,7 @@ static int ov5640_init_controls(struct ov5640_dev *sensor) } static int ov5640_enum_frame_size(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_size_enum *fse) { if (fse->pad != 0) @@ -2838,7 +2838,7 @@ static int ov5640_enum_frame_size(struct v4l2_subdev *sd, static int ov5640_enum_frame_interval( struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_interval_enum *fie) { struct ov5640_dev *sensor = to_ov5640_dev(sd); @@ -2924,7 +2924,7 @@ static int ov5640_s_frame_interval(struct v4l2_subdev *sd, } static int ov5640_enum_mbus_code(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { if (code->pad != 0) diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c index cbe6114908de..c708c61f2188 100644 --- a/drivers/media/platform/ti-vpe/cal-camerarx.c +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c @@ -586,7 +586,7 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd) static struct v4l2_mbus_framefmt * cal_camerarx_get_pad_format(struct cal_camerarx *phy, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, unsigned int pad, u32 which) { switch (which) { @@ -611,7 +611,7 @@ static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable) } static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { struct cal_camerarx *phy = to_cal_camerarx(sd); @@ -639,7 +639,7 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd, } static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_size_enum *fse) { struct cal_camerarx *phy = to_cal_camerarx(sd); @@ -679,7 +679,7 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd, } static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { struct cal_camerarx *phy = to_cal_camerarx(sd); @@ -692,7 +692,7 @@ static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd, } static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { struct cal_camerarx *phy = to_cal_camerarx(sd); @@ -742,7 +742,7 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd, } static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg) + struct v4l2_subdev_config *cfg) { struct v4l2_subdev_format format = { .which = cfg ? V4L2_SUBDEV_FORMAT_TRY diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 956dafab43d4..248fa4ee9356 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -26,19 +26,18 @@ #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; - } + int ret; + + ret = v4l2_subdev_init_config(sd, &fh->cfg); + if (ret) + return ret; return 0; } static void subdev_fh_free(struct v4l2_subdev_fh *fh) { - v4l2_subdev_free_pad_config(fh->pad); - fh->pad = NULL; + v4l2_subdev_uninit_config(&fh->cfg); } static int subdev_open(struct file *file) @@ -146,7 +145,7 @@ 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_config *cfg) { if (which == V4L2_SUBDEV_FORMAT_TRY && !cfg) return -EINVAL; @@ -155,7 +154,7 @@ static int check_cfg(u32 which, struct v4l2_subdev_pad_config *cfg) } static inline int check_format(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { if (!format) @@ -166,7 +165,7 @@ static inline int check_format(struct v4l2_subdev *sd, } static int call_get_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { return check_format(sd, cfg, format) ? : @@ -174,7 +173,7 @@ static int call_get_fmt(struct v4l2_subdev *sd, } static int call_set_fmt(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format) { return check_format(sd, cfg, format) ? : @@ -182,7 +181,7 @@ static int call_set_fmt(struct v4l2_subdev *sd, } static int call_enum_mbus_code(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_mbus_code_enum *code) { if (!code) @@ -194,7 +193,7 @@ static int call_enum_mbus_code(struct v4l2_subdev *sd, } static int call_enum_frame_size(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_size_enum *fse) { if (!fse) @@ -229,7 +228,7 @@ 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_config *cfg, struct v4l2_subdev_frame_interval_enum *fie) { if (!fie) @@ -241,7 +240,7 @@ static int call_enum_frame_interval(struct v4l2_subdev *sd, } static inline int check_selection(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_selection *sel) { if (!sel) @@ -252,7 +251,7 @@ static inline int check_selection(struct v4l2_subdev *sd, } static int call_get_selection(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_selection *sel) { return check_selection(sd, cfg, sel) ? : @@ -260,7 +259,7 @@ static int call_get_selection(struct v4l2_subdev *sd, } static int call_set_selection(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_selection *sel) { return check_selection(sd, cfg, sel) ? : @@ -506,7 +505,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->cfg, format); } case VIDIOC_SUBDEV_S_FMT: { @@ -517,7 +516,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->cfg, format); } case VIDIOC_SUBDEV_G_CROP: { @@ -531,7 +530,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->cfg, &sel); crop->rect = sel.r; @@ -553,7 +552,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->cfg, &sel); crop->rect = sel.r; @@ -564,7 +563,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->cfg, code); } @@ -572,7 +571,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->cfg, fse); } @@ -597,7 +596,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->cfg, fie); } @@ -606,7 +605,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->cfg, sel); } case VIDIOC_SUBDEV_S_SELECTION: { @@ -617,7 +616,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->cfg, sel); } case VIDIOC_G_EDID: { @@ -892,35 +891,35 @@ 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) +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg) { - struct v4l2_subdev_pad_config *cfg; int ret; - if (!sd->entity.num_pads) - return NULL; - - cfg = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg), - GFP_KERNEL | __GFP_ZERO); - if (!cfg) - return NULL; + if (sd->entity.num_pads) { + cfg->pad_configs = kvmalloc_array(sd->entity.num_pads, sizeof(*cfg->pad_configs), + GFP_KERNEL | __GFP_ZERO); + if (!cfg->pad_configs) + return -ENOMEM; + } else { + cfg->pad_configs = NULL; + } ret = v4l2_subdev_call(sd, pad, init_cfg, cfg); if (ret < 0 && ret != -ENOIOCTLCMD) { - kvfree(cfg); - return NULL; + kvfree(cfg->pad_configs); + return ret; } - return cfg; + return 0; } -EXPORT_SYMBOL_GPL(v4l2_subdev_alloc_pad_config); +EXPORT_SYMBOL_GPL(v4l2_subdev_init_config); -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg) +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg) { - kvfree(cfg); + kvfree(cfg->pad_configs); } -EXPORT_SYMBOL_GPL(v4l2_subdev_free_pad_config); +EXPORT_SYMBOL_GPL(v4l2_subdev_uninit_config); + #endif /* CONFIG_MEDIA_CONTROLLER */ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops) diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index d0e9a5bdb08b..1873e527987a 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -623,6 +623,10 @@ struct v4l2_subdev_pad_config { struct v4l2_rect try_compose; }; +struct v4l2_subdev_config { + struct v4l2_subdev_pad_config *pad_configs; +}; + /** * struct v4l2_subdev_pad_ops - v4l2-subdev pad level operations * @@ -687,27 +691,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_config *cfg); int (*enum_mbus_code)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_mbus_code_enum *code); int (*enum_frame_size)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_size_enum *fse); int (*enum_frame_interval)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_frame_interval_enum *fie); int (*get_fmt)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format); int (*set_fmt)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_format *format); int (*get_selection)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, struct v4l2_subdev_selection *sel); int (*set_selection)(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, 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); @@ -925,7 +929,7 @@ 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_config cfg; #endif }; @@ -950,12 +954,12 @@ struct v4l2_subdev_fh { */ static inline struct v4l2_mbus_framefmt * v4l2_subdev_get_try_format(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, unsigned int pad) { if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; - return &cfg[pad].try_fmt; + return &cfg->pad_configs[pad].try_fmt; } /** @@ -968,12 +972,12 @@ v4l2_subdev_get_try_format(struct v4l2_subdev *sd, */ static inline struct v4l2_rect * v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, unsigned int pad) { if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; - return &cfg[pad].try_crop; + return &cfg->pad_configs[pad].try_crop; } /** @@ -986,12 +990,12 @@ v4l2_subdev_get_try_crop(struct v4l2_subdev *sd, */ static inline struct v4l2_rect * v4l2_subdev_get_try_compose(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, unsigned int pad) { if (WARN_ON(pad >= sd->entity.num_pads)) pad = 0; - return &cfg[pad].try_compose; + return &cfg->pad_configs[pad].try_compose; } #endif @@ -1093,20 +1097,17 @@ 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_init_config - initialize v4l2_subdev_config * - * @sd: pointer to struct v4l2_subdev + * Must call v4l2_subdev_uninit_config() when config is no longer needed. */ -struct -v4l2_subdev_pad_config *v4l2_subdev_alloc_pad_config(struct v4l2_subdev *sd); +int v4l2_subdev_init_config(struct v4l2_subdev *sd, struct v4l2_subdev_config *cfg); /** - * v4l2_subdev_free_pad_config - Frees memory allocated by - * v4l2_subdev_alloc_pad_config(). - * - * @cfg: pointer to &struct v4l2_subdev_pad_config + * v4l2_subdev_uninit_config - uninitialize v4l2_subdev_config */ -void v4l2_subdev_free_pad_config(struct v4l2_subdev_pad_config *cfg); +void v4l2_subdev_uninit_config(struct v4l2_subdev_config *cfg); + #endif /* CONFIG_MEDIA_CONTROLLER */ /**
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_config' (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_config. Two drivers are changed to work with the above changes (drivers for HW which I have) as an example. I worked on a semantic patch (included below, my first spatch...) to do this change to all drivers, but hit lots of problems with non-trivial uses of v4l2_subdev_pad_config. As it looks like substantial amount of manual work is needed, I'm posting this RFC to get an ack on the changes before continuing that work. @ v4l2_subdev_pad_ops @ identifier pad_ops; identifier func; @@ ( static const struct v4l2_subdev_pad_ops pad_ops = { ..., .enum_mbus_code = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .enum_frame_size = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .enum_frame_interval = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .get_fmt = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .set_fmt = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .get_selection = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .set_selection = func, ..., }; | static const struct v4l2_subdev_pad_ops pad_ops = { ..., .init_cfg = func, ..., }; ) @@ identifier v4l2_subdev_pad_ops.func; identifier sd; identifier cfg; @@ func(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, ... ) { ... } @@ identifier v4l2_subdev_pad_ops.func; identifier sd; identifier cfg; @@ func(struct v4l2_subdev *sd, - struct v4l2_subdev_pad_config *cfg + struct v4l2_subdev_config *cfg ) { ... } @@ struct v4l2_subdev_fh *fh; @@ - fh->pad + &fh->cfg @@ identifier func; identifier cfg; @@ func(..., - struct v4l2_subdev_pad_config *cfg, + struct v4l2_subdev_config *cfg, ...) { ... } @@ struct v4l2_subdev_config *cfg; @@ { <... ( - cfg->try_fmt + cfg->pad_configs->try_fmt | - cfg->try_crop + cfg->pad_configs->try_crop | - cfg->try_compose + cfg->pad_configs->try_compose ) ...> } @@ identifier pad_cfg; @@ { ... struct v4l2_subdev_pad_config pad_cfg; + struct v4l2_subdev_config cfg = { .pad_configs = &pad_cfg }; <... - &pad_cfg + &cfg ...> } Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/i2c/ov5640.c | 10 +-- drivers/media/platform/ti-vpe/cal-camerarx.c | 12 +-- drivers/media/v4l2-core/v4l2-subdev.c | 85 ++++++++++---------- include/media/v4l2-subdev.h | 49 +++++------ 4 files changed, 78 insertions(+), 78 deletions(-)