| Message ID | 20240910170610.226189-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | media: ov5645: Add support for streams | expand |
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:00PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > The OV5645 sensor exposes controls, so the V4L2_SUBDEV_FL_HAS_EVENTS flag > should be set and implement subscribe_event and unsubscribe_event hooks. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/i2c/ov5645.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 019979f553b1..6eedd0310b02 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -29,6 +29,7 @@ > #include <linux/slab.h> > #include <linux/types.h> > #include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > @@ -1042,7 +1043,13 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > .get_selection = ov5645_get_selection, > }; > > +static const struct v4l2_subdev_core_ops ov5645_core_ops = { > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > +}; > + > static const struct v4l2_subdev_ops ov5645_subdev_ops = { > + .core = &ov5645_core_ops, > .video = &ov5645_video_ops, > .pad = &ov5645_subdev_pad_ops, > }; > @@ -1178,7 +1185,7 @@ static int ov5645_probe(struct i2c_client *client) > > v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops); > ov5645->sd.internal_ops = &ov5645_internal_ops; > - ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > ov5645->pad.flags = MEDIA_PAD_FL_SOURCE; > ov5645->sd.dev = &client->dev; > ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > -- > 2.34.1 >
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:02PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > To simplify the probe error path, enable runtime PM after the > v4l2_async_register_subdev() call. > > This change ensures that runtime PM is only enabled once the subdevice > registration is successful, avoiding unnecessary cleanup in the error > path. The subdev could start being used as soon as it gets registered, so runtime PM initialization should happen before v4l2_async_register_subdev(). > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index ab3a419df2df..78b86438c798 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1239,18 +1239,17 @@ static int ov5645_probe(struct i2c_client *client) > goto power_down; > } > > - pm_runtime_set_active(dev); > - pm_runtime_get_noresume(dev); > - pm_runtime_enable(dev); > - > ov5645_init_state(&ov5645->sd, NULL); > > ret = v4l2_async_register_subdev(&ov5645->sd); > if (ret < 0) { > dev_err(dev, "could not register v4l2 device\n"); > - goto err_pm_runtime; > + goto power_down; > } > > + pm_runtime_set_active(dev); > + pm_runtime_get_noresume(dev); > + pm_runtime_enable(dev); > pm_runtime_set_autosuspend_delay(dev, 1000); > pm_runtime_use_autosuspend(dev); > pm_runtime_mark_last_busy(dev); > @@ -1258,9 +1257,6 @@ static int ov5645_probe(struct i2c_client *client) > > return 0; > > -err_pm_runtime: > - pm_runtime_disable(dev); > - pm_runtime_put_noidle(dev); > power_down: > ov5645_set_power_off(dev); > free_entity:
On Tue, Sep 10, 2024 at 06:06:04PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Make use v4l2_async_register_subdev_sensor() helper to register > the subdev. The commit message should explain why. > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/i2c/ov5645.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 9e6ff1f1b9ac..45687d004004 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1223,7 +1223,7 @@ static int ov5645_probe(struct i2c_client *client) > > ov5645_init_state(&ov5645->sd, NULL); > > - ret = v4l2_async_register_subdev(&ov5645->sd); > + ret = v4l2_async_register_subdev_sensor(&ov5645->sd); > if (ret < 0) { > dev_err_probe(dev, ret, "could not register v4l2 device\n"); > goto power_down;
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:06PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Port the ov5645 sensor driver to use the subdev active state. > > Move all the format configuration to the subdevice state and simplify > the format handling, locking and initialization. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------ > 1 file changed, 39 insertions(+), 70 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 25c60afcc0ec..9497ec737cb7 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -89,7 +89,6 @@ struct ov5645 { > struct v4l2_subdev sd; > struct media_pad pad; > struct v4l2_fwnode_endpoint ep; > - struct v4l2_mbus_framefmt fmt; > struct v4l2_rect crop; > struct clk *xclk; > > @@ -850,49 +849,6 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev, > return 0; > } > > -static struct v4l2_mbus_framefmt * > -__ov5645_get_pad_format(struct ov5645 *ov5645, > - struct v4l2_subdev_state *sd_state, > - unsigned int pad, > - enum v4l2_subdev_format_whence which) > -{ > - switch (which) { > - case V4L2_SUBDEV_FORMAT_TRY: > - return v4l2_subdev_state_get_format(sd_state, pad); > - case V4L2_SUBDEV_FORMAT_ACTIVE: > - return &ov5645->fmt; > - default: > - return NULL; > - } > -} > - > -static int ov5645_get_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *format) > -{ > - struct ov5645 *ov5645 = to_ov5645(sd); > - > - format->format = *__ov5645_get_pad_format(ov5645, sd_state, > - format->pad, > - format->which); > - return 0; > -} > - > -static struct v4l2_rect * > -__ov5645_get_pad_crop(struct ov5645 *ov5645, > - struct v4l2_subdev_state *sd_state, > - unsigned int pad, enum v4l2_subdev_format_whence which) > -{ > - switch (which) { > - case V4L2_SUBDEV_FORMAT_TRY: > - return v4l2_subdev_state_get_crop(sd_state, pad); > - case V4L2_SUBDEV_FORMAT_ACTIVE: > - return &ov5645->crop; > - default: > - return NULL; > - } > -} > - > static int ov5645_set_format(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_format *format) > @@ -903,33 +859,30 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > const struct ov5645_mode_info *new_mode; > int ret; > > - __crop = __ov5645_get_pad_crop(ov5645, sd_state, format->pad, > - format->which); > - > + __crop = v4l2_subdev_state_get_crop(sd_state, 0); > new_mode = v4l2_find_nearest_size(ov5645_mode_info_data, > - ARRAY_SIZE(ov5645_mode_info_data), > - width, height, > - format->format.width, format->format.height); > + ARRAY_SIZE(ov5645_mode_info_data), > + width, height, format->format.width, > + format->format.height); > > __crop->width = new_mode->width; > __crop->height = new_mode->height; > > if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > - ret = v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock, > - new_mode->pixel_clock); > + ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock, > + new_mode->pixel_clock); > if (ret < 0) > return ret; > > - ret = v4l2_ctrl_s_ctrl(ov5645->link_freq, > - new_mode->link_freq); > + ret = __v4l2_ctrl_s_ctrl(ov5645->link_freq, > + new_mode->link_freq); > if (ret < 0) > return ret; > > ov5645->current_mode = new_mode; > } > > - __format = __ov5645_get_pad_format(ov5645, sd_state, format->pad, > - format->which); > + __format = v4l2_subdev_state_get_format(sd_state, 0); > __format->width = __crop->width; > __format->height = __crop->height; > __format->code = MEDIA_BUS_FMT_UYVY8_1X16; > @@ -944,11 +897,15 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > static int ov5645_init_state(struct v4l2_subdev *subdev, > struct v4l2_subdev_state *sd_state) > { > - struct v4l2_subdev_format fmt = { 0 }; > - > - fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE; > - fmt.format.width = 1920; > - fmt.format.height = 1080; > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_TRY, > + .pad = 0, > + .format = { > + .code = MEDIA_BUS_FMT_UYVY8_1X16, > + .width = ov5645_mode_info_data[1].width, > + .height = ov5645_mode_info_data[1].height, > + }, > + }; > > ov5645_set_format(subdev, sd_state, &fmt); > > @@ -959,25 +916,27 @@ static int ov5645_get_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_selection *sel) > { > - struct ov5645 *ov5645 = to_ov5645(sd); > - > if (sel->target != V4L2_SEL_TGT_CROP) > return -EINVAL; > > - sel->r = *__ov5645_get_pad_crop(ov5645, sd_state, sel->pad, > - sel->which); > + sel->r = *v4l2_subdev_state_get_crop(sd_state, 0); > return 0; > } > > static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > { > struct ov5645 *ov5645 = to_ov5645(subdev); > + struct v4l2_subdev_state *state; > int ret; > > + state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd); > + > if (enable) { > ret = pm_runtime_resume_and_get(ov5645->dev); > - if (ret < 0) > + if (ret < 0) { > + v4l2_subdev_unlock_state(state); > return ret; > + } > > ret = ov5645_set_register_array(ov5645, > ov5645->current_mode->data, > @@ -988,7 +947,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > ov5645->current_mode->height); > goto err_rpm_put; > } > - ret = v4l2_ctrl_handler_setup(&ov5645->ctrls); > + ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls); > if (ret < 0) { > dev_err(ov5645->dev, "could not sync v4l2 controls\n"); > goto err_rpm_put; > @@ -1013,6 +972,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > goto stream_off_rpm_put; > } > > + v4l2_subdev_unlock_state(state); > return 0; > > err_rpm_put: > @@ -1022,6 +982,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > stream_off_rpm_put: > pm_runtime_mark_last_busy(ov5645->dev); > pm_runtime_put_autosuspend(ov5645->dev); > + v4l2_subdev_unlock_state(state); > return ret; > } > > @@ -1032,7 +993,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = { > static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > .enum_mbus_code = ov5645_enum_mbus_code, > .enum_frame_size = ov5645_enum_frame_size, > - .get_fmt = ov5645_get_format, > + .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = ov5645_set_format, > .get_selection = ov5645_get_selection, > }; > @@ -1213,12 +1174,17 @@ static int ov5645_probe(struct i2c_client *client) > goto power_down; > } > > - ov5645_init_state(&ov5645->sd, NULL); > + ov5645->sd.state_lock = ov5645->ctrls.lock; > + ret = v4l2_subdev_init_finalize(&ov5645->sd); > + if (ret < 0) { > + dev_err_probe(dev, ret, "subdev init error\n"); > + goto power_down; > + } > > ret = v4l2_async_register_subdev_sensor(&ov5645->sd); > if (ret < 0) { > dev_err_probe(dev, ret, "could not register v4l2 device\n"); > - goto power_down; > + goto error_subdev_cleanup; > } > > pm_runtime_set_active(dev); > @@ -1231,6 +1197,8 @@ static int ov5645_probe(struct i2c_client *client) > > return 0; > > +error_subdev_cleanup: > + v4l2_subdev_cleanup(&ov5645->sd); > power_down: > ov5645_set_power_off(dev); > free_entity: > @@ -1247,6 +1215,7 @@ static void ov5645_remove(struct i2c_client *client) > struct ov5645 *ov5645 = to_ov5645(sd); > > v4l2_async_unregister_subdev(&ov5645->sd); > + v4l2_subdev_cleanup(sd); > media_entity_cleanup(&ov5645->sd.entity); > v4l2_ctrl_handler_free(&ov5645->ctrls); > pm_runtime_disable(ov5645->dev);
Hi Laurent, Thank you for the review. On Tue, Sep 24, 2024 at 11:38 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Sep 10, 2024 at 06:06:02PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > To simplify the probe error path, enable runtime PM after the > > v4l2_async_register_subdev() call. > > > > This change ensures that runtime PM is only enabled once the subdevice > > registration is successful, avoiding unnecessary cleanup in the error > > path. > > The subdev could start being used as soon as it gets registered, so > runtime PM initialization should happen before > v4l2_async_register_subdev(). > Agreed, I will drop this patch. Cheers, Prabhakar
Hi Laurent, Thank you for the review. On Tue, Sep 24, 2024 at 11:45 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Sep 10, 2024 at 06:06:04PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Make use v4l2_async_register_subdev_sensor() helper to register > > the subdev. > > The commit message should explain why. > Sure I'll update the commit message. Cheers, Prabhakar
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Use the newly added internal pad API to expose the internal > configuration of the sensor to userspace in a standard manner. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++---------- > 1 file changed, 79 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index dc93514608ee..255c6395a268 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -60,6 +60,10 @@ > #define OV5645_SDE_SAT_U 0x5583 > #define OV5645_SDE_SAT_V 0x5584 > > +#define OV5645_NATIVE_FORMAT MEDIA_BUS_FMT_SBGGR8_1X8 > +#define OV5645_NATIVE_WIDTH 2592 > +#define OV5645_NATIVE_HEIGHT 1944 > + > /* regulator supplies */ > static const char * const ov5645_supply_name[] = { > "vdddo", /* Digital I/O (1.8V) supply */ > @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = { > > #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name) > > +enum ov5645_pad_ids { > + OV5645_PAD_SOURCE, > + OV5645_PAD_IMAGE, > + OV5645_NUM_PADS, > +}; > + > struct reg_value { > u16 reg; > u8 val; > @@ -87,7 +97,7 @@ struct ov5645 { > struct i2c_client *i2c_client; > struct device *dev; > struct v4l2_subdev sd; > - struct media_pad pad; > + struct media_pad pads[OV5645_NUM_PADS]; > struct v4l2_fwnode_endpoint ep; > struct v4l2_rect crop; > struct clk *xclk; > @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > if (code->index > 0) > return -EINVAL; > > - code->code = MEDIA_BUS_FMT_UYVY8_1X16; > + if (code->pad == OV5645_PAD_IMAGE) > + code->code = OV5645_NATIVE_FORMAT; > + else > + code->code = MEDIA_BUS_FMT_UYVY8_1X16; > > return 0; > } > @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_frame_size_enum *fse) > { > - if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16) > - return -EINVAL; > - > - if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > - return -EINVAL; > - > - fse->min_width = ov5645_mode_info_data[fse->index].width; > - fse->max_width = ov5645_mode_info_data[fse->index].width; > - fse->min_height = ov5645_mode_info_data[fse->index].height; > - fse->max_height = ov5645_mode_info_data[fse->index].height; > + if (fse->pad == OV5645_PAD_IMAGE) { > + if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0) > + return -EINVAL; > + > + fse->min_width = OV5645_NATIVE_WIDTH; > + fse->max_width = OV5645_NATIVE_WIDTH; > + fse->min_height = OV5645_NATIVE_HEIGHT; > + fse->max_height = OV5645_NATIVE_HEIGHT; > + } else { > + if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 || This will be interesting. With internal pads we will introduce the concept of a "subdev model", which will formally document how the V4L2 subdev configuration items (formats, selection rectangles, ...) maps to hardware features. Sakari is working on the definition of a subdev model for raw sensors, that should catter for the needs of raw sensors without a bayer scaler (the vast majority of them). To use internal pads with a non-raw sensor, we'll have to define a model. It may be more challenging/complicated to do so, as the YUV sensor features are less standardized, but it will be an interesting development. > + fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > + return -EINVAL; > + > + fse->min_width = ov5645_mode_info_data[fse->index].width; > + fse->max_width = ov5645_mode_info_data[fse->index].width; > + fse->min_height = ov5645_mode_info_data[fse->index].height; > + fse->max_height = ov5645_mode_info_data[fse->index].height; > + } > > return 0; > } > @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > { > struct ov5645 *ov5645 = to_ov5645(sd); > struct v4l2_mbus_framefmt *__format; > + struct v4l2_rect *compose; > struct v4l2_rect *__crop; While at it, could you rename __crop to crop ? > const struct ov5645_mode_info *new_mode; > int ret; > > - __crop = v4l2_subdev_state_get_crop(sd_state, 0); > + if (format->pad != OV5645_PAD_SOURCE) > + return v4l2_subdev_get_fmt(sd, sd_state, format); > + > new_mode = v4l2_find_nearest_size(ov5645_mode_info_data, > ARRAY_SIZE(ov5645_mode_info_data), > width, height, format->format.width, > format->format.height); > - > - __crop->width = new_mode->width; > - __crop->height = new_mode->height; > + format->format.code = MEDIA_BUS_FMT_UYVY8_1X16; > + format->format.width = new_mode->width; > + format->format.height = new_mode->height; > + format->format.field = V4L2_FIELD_NONE; > + format->format.colorspace = V4L2_COLORSPACE_SRGB; > + format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > + format->format.quantization = V4L2_QUANTIZATION_DEFAULT; > + format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT; Drivers are not supposed to return DEFAULT values, you should pick appropriate values. > + > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE); Maybe __format could also become fmt. > + *__format = format->format; > + __format->code = OV5645_NATIVE_FORMAT; > + __format->width = OV5645_NATIVE_WIDTH; > + __format->height = OV5645_NATIVE_HEIGHT; > + > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE); > + __crop->width = format->format.width; > + __crop->height = format->format.height; > + > + /* > + * The compose rectangle models binning, its size is the sensor output > + * size. > + */ > + compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE); > + compose->left = 0; > + compose->top = 0; > + compose->width = format->format.width; > + compose->height = format->format.height; > + > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE); > + __crop->left = 0; > + __crop->top = 0; > + __crop->width = format->format.width; > + __crop->height = format->format.height; > + > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE); > + *__format = format->format; > > if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock, > @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > ov5645->current_mode = new_mode; > } > > - __format = v4l2_subdev_state_get_format(sd_state, 0); > - __format->width = __crop->width; > - __format->height = __crop->height; > - __format->code = MEDIA_BUS_FMT_UYVY8_1X16; > - __format->field = V4L2_FIELD_NONE; > - __format->colorspace = V4L2_COLORSPACE_SRGB; > - > - format->format = *__format; > > return 0; > } > @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev, > { > struct v4l2_subdev_format fmt = { > .which = V4L2_SUBDEV_FORMAT_TRY, > - .pad = 0, > + .pad = OV5645_PAD_SOURCE, > .format = { > .code = MEDIA_BUS_FMT_UYVY8_1X16, > .width = ov5645_mode_info_data[1].width, > @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd, > if (sel->target != V4L2_SEL_TGT_CROP) > return -EINVAL; > > - sel->r = *v4l2_subdev_state_get_crop(sd_state, 0); > + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad); Now there's a compose rectangle, you should support getting it. > return 0; > } > > @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client) > v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops); > ov5645->sd.internal_ops = &ov5645_internal_ops; > ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > - ov5645->pad.flags = MEDIA_PAD_FL_SOURCE; > + ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > + ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL; > ov5645->sd.dev = dev; > ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > - ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); > + ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads); Line wrap. > if (ret < 0) { > dev_err_probe(dev, ret, "could not register media entity\n"); > goto free_ctrl;
Hi Prabhakar, Thank you for the patch. On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Implement the .get_frame_desc() subdev operation to report information > about streams to the connected CSI-2 receiver. This is required to let > the CSI-2 receiver driver know about virtual channels and data types for > each stream. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 7f1133292ffc..c24eb6e7a7b5 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -28,6 +28,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/types.h> > +#include <media/mipi-csi2.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-event.h> > #include <media/v4l2-fwnode.h> > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > .s_ctrl = ov5645_s_ctrl, > }; > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + const struct v4l2_mbus_framefmt *fmt; > + struct v4l2_subdev_state *state; > + > + if (pad != OV5645_PAD_SOURCE) > + return -EINVAL; > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0); > + v4l2_subdev_unlock_state(state); Once you unlock the state fmt could change, so you should instead do state = v4l2_subdev_lock_and_get_active_state(sd); code = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0)->code; v4l2_subdev_unlock_state(state); Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > + fd->num_entries = 1; > + > + memset(fd->entry, 0, sizeof(fd->entry)); > + > + fd->entry[0].pixelcode = fmt->code; > + fd->entry[0].stream = 0; > + fd->entry[0].bus.csi2.vc = 0; > + fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B; > + > + return 0; > +} > + > static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = { > }; > > static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > + .get_frame_desc = ov5645_get_frame_desc, > .enum_mbus_code = ov5645_enum_mbus_code, > .enum_frame_size = ov5645_enum_frame_size, > .get_fmt = v4l2_subdev_get_fmt,
Hi Laurent, Thank you for the review. On Wed, Sep 25, 2024 at 5:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Implement the .get_frame_desc() subdev operation to report information > > about streams to the connected CSI-2 receiver. This is required to let > > the CSI-2 receiver driver know about virtual channels and data types for > > each stream. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index 7f1133292ffc..c24eb6e7a7b5 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -28,6 +28,7 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > +#include <media/mipi-csi2.h> > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-event.h> > > #include <media/v4l2-fwnode.h> > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > > .s_ctrl = ov5645_s_ctrl, > > }; > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + const struct v4l2_mbus_framefmt *fmt; > > + struct v4l2_subdev_state *state; > > + > > + if (pad != OV5645_PAD_SOURCE) > > + return -EINVAL; > > + > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0); > > + v4l2_subdev_unlock_state(state); > > Once you unlock the state fmt could change, so you should instead do > > state = v4l2_subdev_lock_and_get_active_state(sd); > code = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0)->code; > v4l2_subdev_unlock_state(state); > Ok, I will update the above. Cheers, Prabhakar > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > + > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > + fd->num_entries = 1; > > + > > + memset(fd->entry, 0, sizeof(fd->entry)); > > + > > + fd->entry[0].pixelcode = fmt->code; > > + fd->entry[0].stream = 0; > > + fd->entry[0].bus.csi2.vc = 0; > > + fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B; > > + > > + return 0; > > +} > > + > > static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_mbus_code_enum *code) > > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = { > > }; > > > > static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > > + .get_frame_desc = ov5645_get_frame_desc, > > .enum_mbus_code = ov5645_enum_mbus_code, > > .enum_frame_size = ov5645_enum_frame_size, > > .get_fmt = v4l2_subdev_get_fmt, > > -- > Regards, > > Laurent Pinchart
Hi Prabhakar, Thanks for the set. It looks largely very nice to me, after addressing Laurent's comments. A few comments here and possibly on other patches... On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Implement the .get_frame_desc() subdev operation to report information > about streams to the connected CSI-2 receiver. This is required to let > the CSI-2 receiver driver know about virtual channels and data types for > each stream. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index 7f1133292ffc..c24eb6e7a7b5 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -28,6 +28,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/slab.h> > #include <linux/types.h> > +#include <media/mipi-csi2.h> > #include <media/v4l2-ctrls.h> > #include <media/v4l2-event.h> > #include <media/v4l2-fwnode.h> > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > .s_ctrl = ov5645_s_ctrl, > }; > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > + struct v4l2_mbus_frame_desc *fd) > +{ > + const struct v4l2_mbus_framefmt *fmt; > + struct v4l2_subdev_state *state; > + > + if (pad != OV5645_PAD_SOURCE) > + return -EINVAL; As you have a single source pad, and pretty much all sensor drivers will, I think it'd be nice to add a check for this (that it's not an internal pad) to the caller side in v4l2-subdev.c. And of course drop this one. > + > + state = v4l2_subdev_lock_and_get_active_state(sd); > + fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0); > + v4l2_subdev_unlock_state(state); > + > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > + fd->num_entries = 1; > + > + memset(fd->entry, 0, sizeof(fd->entry)); > + > + fd->entry[0].pixelcode = fmt->code; > + fd->entry[0].stream = 0; > + fd->entry[0].bus.csi2.vc = 0; > + fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B; > + > + return 0; > +} > + > static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = { > }; > > static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > + .get_frame_desc = ov5645_get_frame_desc, > .enum_mbus_code = ov5645_enum_mbus_code, > .enum_frame_size = ov5645_enum_frame_size, > .get_fmt = v4l2_subdev_get_fmt, >
Hi Laurent, Prabhakar, On Wed, Sep 25, 2024 at 07:21:53PM +0300, Laurent Pinchart wrote: > Hi Prabhakar, > > Thank you for the patch. > > On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Use the newly added internal pad API to expose the internal > > configuration of the sensor to userspace in a standard manner. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++---------- > > 1 file changed, 79 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index dc93514608ee..255c6395a268 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -60,6 +60,10 @@ > > #define OV5645_SDE_SAT_U 0x5583 > > #define OV5645_SDE_SAT_V 0x5584 > > > > +#define OV5645_NATIVE_FORMAT MEDIA_BUS_FMT_SBGGR8_1X8 > > +#define OV5645_NATIVE_WIDTH 2592 > > +#define OV5645_NATIVE_HEIGHT 1944 > > + > > /* regulator supplies */ > > static const char * const ov5645_supply_name[] = { > > "vdddo", /* Digital I/O (1.8V) supply */ > > @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = { > > > > #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name) > > > > +enum ov5645_pad_ids { > > + OV5645_PAD_SOURCE, > > + OV5645_PAD_IMAGE, > > + OV5645_NUM_PADS, > > +}; > > + > > struct reg_value { > > u16 reg; > > u8 val; > > @@ -87,7 +97,7 @@ struct ov5645 { > > struct i2c_client *i2c_client; > > struct device *dev; > > struct v4l2_subdev sd; > > - struct media_pad pad; > > + struct media_pad pads[OV5645_NUM_PADS]; > > struct v4l2_fwnode_endpoint ep; > > struct v4l2_rect crop; > > struct clk *xclk; > > @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > > if (code->index > 0) > > return -EINVAL; > > > > - code->code = MEDIA_BUS_FMT_UYVY8_1X16; > > + if (code->pad == OV5645_PAD_IMAGE) > > + code->code = OV5645_NATIVE_FORMAT; > > + else > > + code->code = MEDIA_BUS_FMT_UYVY8_1X16; > > > > return 0; > > } > > @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_frame_size_enum *fse) > > { > > - if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16) > > - return -EINVAL; > > - > > - if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > > - return -EINVAL; > > - > > - fse->min_width = ov5645_mode_info_data[fse->index].width; > > - fse->max_width = ov5645_mode_info_data[fse->index].width; > > - fse->min_height = ov5645_mode_info_data[fse->index].height; > > - fse->max_height = ov5645_mode_info_data[fse->index].height; > > + if (fse->pad == OV5645_PAD_IMAGE) { > > + if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0) > > + return -EINVAL; > > + > > + fse->min_width = OV5645_NATIVE_WIDTH; > > + fse->max_width = OV5645_NATIVE_WIDTH; > > + fse->min_height = OV5645_NATIVE_HEIGHT; > > + fse->max_height = OV5645_NATIVE_HEIGHT; > > + } else { > > + if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 || > > This will be interesting. With internal pads we will introduce the > concept of a "subdev model", which will formally document how the V4L2 > subdev configuration items (formats, selection rectangles, ...) maps to > hardware features. Sakari is working on the definition of a subdev model > for raw sensors, that should catter for the needs of raw sensors without > a bayer scaler (the vast majority of them). To use internal pads with a > non-raw sensor, we'll have to define a model. It may be more > challenging/complicated to do so, as the YUV sensor features are less > standardized, but it will be an interesting development. I think I'll make the sub-device model a bitmask, to allow implementing more than one at the same time. I'll try to remember to cc you to the patchset when I'll send it, likely next week. > > > + fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > > + return -EINVAL; > > + > > + fse->min_width = ov5645_mode_info_data[fse->index].width; > > + fse->max_width = ov5645_mode_info_data[fse->index].width; > > + fse->min_height = ov5645_mode_info_data[fse->index].height; > > + fse->max_height = ov5645_mode_info_data[fse->index].height; > > + } > > > > return 0; > > } > > @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > > { > > struct ov5645 *ov5645 = to_ov5645(sd); > > struct v4l2_mbus_framefmt *__format; > > + struct v4l2_rect *compose; > > struct v4l2_rect *__crop; > > While at it, could you rename __crop to crop ? > > > const struct ov5645_mode_info *new_mode; > > int ret; > > > > - __crop = v4l2_subdev_state_get_crop(sd_state, 0); > > + if (format->pad != OV5645_PAD_SOURCE) > > + return v4l2_subdev_get_fmt(sd, sd_state, format); > > + > > new_mode = v4l2_find_nearest_size(ov5645_mode_info_data, > > ARRAY_SIZE(ov5645_mode_info_data), > > width, height, format->format.width, > > format->format.height); > > - > > - __crop->width = new_mode->width; > > - __crop->height = new_mode->height; > > + format->format.code = MEDIA_BUS_FMT_UYVY8_1X16; > > + format->format.width = new_mode->width; > > + format->format.height = new_mode->height; > > + format->format.field = V4L2_FIELD_NONE; > > + format->format.colorspace = V4L2_COLORSPACE_SRGB; > > + format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > + format->format.quantization = V4L2_QUANTIZATION_DEFAULT; > > + format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > Drivers are not supposed to return DEFAULT values, you should pick > appropriate values. > > > + > > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE); > > Maybe __format could also become fmt. > > > + *__format = format->format; > > + __format->code = OV5645_NATIVE_FORMAT; > > + __format->width = OV5645_NATIVE_WIDTH; > > + __format->height = OV5645_NATIVE_HEIGHT; > > + > > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE); > > + __crop->width = format->format.width; > > + __crop->height = format->format.height; > > + > > + /* > > + * The compose rectangle models binning, its size is the sensor output > > + * size. > > + */ > > + compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE); > > + compose->left = 0; > > + compose->top = 0; > > + compose->width = format->format.width; > > + compose->height = format->format.height; > > + > > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE); > > + __crop->left = 0; > > + __crop->top = 0; > > + __crop->width = format->format.width; > > + __crop->height = format->format.height; > > + > > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE); > > + *__format = format->format; > > > > if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock, > > @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > > ov5645->current_mode = new_mode; > > } > > > > - __format = v4l2_subdev_state_get_format(sd_state, 0); > > - __format->width = __crop->width; > > - __format->height = __crop->height; > > - __format->code = MEDIA_BUS_FMT_UYVY8_1X16; > > - __format->field = V4L2_FIELD_NONE; > > - __format->colorspace = V4L2_COLORSPACE_SRGB; > > - > > - format->format = *__format; > > > > return 0; > > } > > @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev, > > { > > struct v4l2_subdev_format fmt = { > > .which = V4L2_SUBDEV_FORMAT_TRY, > > - .pad = 0, > > + .pad = OV5645_PAD_SOURCE, > > .format = { > > .code = MEDIA_BUS_FMT_UYVY8_1X16, > > .width = ov5645_mode_info_data[1].width, > > @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd, > > if (sel->target != V4L2_SEL_TGT_CROP) > > return -EINVAL; > > > > - sel->r = *v4l2_subdev_state_get_crop(sd_state, 0); > > + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad); > > Now there's a compose rectangle, you should support getting it. > > > return 0; > > } > > > > @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client) > > v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops); > > ov5645->sd.internal_ops = &ov5645_internal_ops; > > ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > - ov5645->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > > + ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL; > > ov5645->sd.dev = dev; > > ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > > > - ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); > > + ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads); > > Line wrap. It's good to run: scripts/checkpatch.pl --strict --max-line-length=80 on the patches. > > > if (ret < 0) { > > dev_err_probe(dev, ret, "could not register media entity\n"); > > goto free_ctrl; >
On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote: > Hi Prabhakar, > > Thanks for the set. It looks largely very nice to me, after addressing > Laurent's comments. A few comments here and possibly on other patches... > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Implement the .get_frame_desc() subdev operation to report information > > about streams to the connected CSI-2 receiver. This is required to let > > the CSI-2 receiver driver know about virtual channels and data types for > > each stream. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index 7f1133292ffc..c24eb6e7a7b5 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -28,6 +28,7 @@ > > #include <linux/regulator/consumer.h> > > #include <linux/slab.h> > > #include <linux/types.h> > > +#include <media/mipi-csi2.h> > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-event.h> > > #include <media/v4l2-fwnode.h> > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > > .s_ctrl = ov5645_s_ctrl, > > }; > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + const struct v4l2_mbus_framefmt *fmt; > > + struct v4l2_subdev_state *state; > > + > > + if (pad != OV5645_PAD_SOURCE) > > + return -EINVAL; > > As you have a single source pad, and pretty much all sensor drivers will, I > think it'd be nice to add a check for this (that it's not an internal pad) > to the caller side in v4l2-subdev.c. And of course drop this one. What check would you add, just verifying that the pad is a source pad ? > > + > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0); > > + v4l2_subdev_unlock_state(state); > > + > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > + fd->num_entries = 1; > > + > > + memset(fd->entry, 0, sizeof(fd->entry)); > > + > > + fd->entry[0].pixelcode = fmt->code; > > + fd->entry[0].stream = 0; > > + fd->entry[0].bus.csi2.vc = 0; > > + fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B; > > + > > + return 0; > > +} > > + > > static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *sd_state, > > struct v4l2_subdev_mbus_code_enum *code) > > @@ -1062,6 +1089,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = { > > }; > > > > static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = { > > + .get_frame_desc = ov5645_get_frame_desc, > > .enum_mbus_code = ov5645_enum_mbus_code, > > .enum_frame_size = ov5645_enum_frame_size, > > .get_fmt = v4l2_subdev_get_fmt, > >
On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote: > On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote: > > Hi Prabhakar, > > > > Thanks for the set. It looks largely very nice to me, after addressing > > Laurent's comments. A few comments here and possibly on other patches... > > > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Implement the .get_frame_desc() subdev operation to report information > > > about streams to the connected CSI-2 receiver. This is required to let > > > the CSI-2 receiver driver know about virtual channels and data types for > > > each stream. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index 7f1133292ffc..c24eb6e7a7b5 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -28,6 +28,7 @@ > > > #include <linux/regulator/consumer.h> > > > #include <linux/slab.h> > > > #include <linux/types.h> > > > +#include <media/mipi-csi2.h> > > > #include <media/v4l2-ctrls.h> > > > #include <media/v4l2-event.h> > > > #include <media/v4l2-fwnode.h> > > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > > > .s_ctrl = ov5645_s_ctrl, > > > }; > > > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > + struct v4l2_mbus_frame_desc *fd) > > > +{ > > > + const struct v4l2_mbus_framefmt *fmt; > > > + struct v4l2_subdev_state *state; > > > + > > > + if (pad != OV5645_PAD_SOURCE) > > > + return -EINVAL; > > > > As you have a single source pad, and pretty much all sensor drivers will, I > > think it'd be nice to add a check for this (that it's not an internal pad) > > to the caller side in v4l2-subdev.c. And of course drop this one. > > What check would you add, just verifying that the pad is a source pad ? I think you could add that, too, besides the absence of the internal flag.
Hi Sakari and Laurent, On Thu, Sep 26, 2024 at 7:57 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote: > > On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote: > > > Hi Prabhakar, > > > > > > Thanks for the set. It looks largely very nice to me, after addressing > > > Laurent's comments. A few comments here and possibly on other patches... > > > > > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Implement the .get_frame_desc() subdev operation to report information > > > > about streams to the connected CSI-2 receiver. This is required to let > > > > the CSI-2 receiver driver know about virtual channels and data types for > > > > each stream. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > --- > > > > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > > > > 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > > index 7f1133292ffc..c24eb6e7a7b5 100644 > > > > --- a/drivers/media/i2c/ov5645.c > > > > +++ b/drivers/media/i2c/ov5645.c > > > > @@ -28,6 +28,7 @@ > > > > #include <linux/regulator/consumer.h> > > > > #include <linux/slab.h> > > > > #include <linux/types.h> > > > > +#include <media/mipi-csi2.h> > > > > #include <media/v4l2-ctrls.h> > > > > #include <media/v4l2-event.h> > > > > #include <media/v4l2-fwnode.h> > > > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > > > > .s_ctrl = ov5645_s_ctrl, > > > > }; > > > > > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > > + struct v4l2_mbus_frame_desc *fd) > > > > +{ > > > > + const struct v4l2_mbus_framefmt *fmt; > > > > + struct v4l2_subdev_state *state; > > > > + > > > > + if (pad != OV5645_PAD_SOURCE) > > > > + return -EINVAL; > > > > > > As you have a single source pad, and pretty much all sensor drivers will, I > > > think it'd be nice to add a check for this (that it's not an internal pad) > > > to the caller side in v4l2-subdev.c. And of course drop this one. > > > > What check would you add, just verifying that the pad is a source pad ? > > I think you could add that, too, besides the absence of the internal flag. > Checking only for the source flag should suffice, as the MEDIA_PAD_FL_INTERNAL flag cannot be set for a source pad because media_entity_pads_init() enforces this restriction. Do you agree? Cheers, Prabhakar
Hi Sakari, On Thu, Sep 26, 2024 at 4:49 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Laurent, Prabhakar, > > On Wed, Sep 25, 2024 at 07:21:53PM +0300, Laurent Pinchart wrote: > > Hi Prabhakar, > > > > Thank you for the patch. > > > > On Tue, Sep 10, 2024 at 06:06:08PM +0100, Prabhakar wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Use the newly added internal pad API to expose the internal > > > configuration of the sensor to userspace in a standard manner. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > drivers/media/i2c/ov5645.c | 107 +++++++++++++++++++++++++++---------- > > > 1 file changed, 79 insertions(+), 28 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index dc93514608ee..255c6395a268 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -60,6 +60,10 @@ > > > #define OV5645_SDE_SAT_U 0x5583 > > > #define OV5645_SDE_SAT_V 0x5584 > > > > > > +#define OV5645_NATIVE_FORMAT MEDIA_BUS_FMT_SBGGR8_1X8 > > > +#define OV5645_NATIVE_WIDTH 2592 > > > +#define OV5645_NATIVE_HEIGHT 1944 > > > + > > > /* regulator supplies */ > > > static const char * const ov5645_supply_name[] = { > > > "vdddo", /* Digital I/O (1.8V) supply */ > > > @@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = { > > > > > > #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name) > > > > > > +enum ov5645_pad_ids { > > > + OV5645_PAD_SOURCE, > > > + OV5645_PAD_IMAGE, > > > + OV5645_NUM_PADS, > > > +}; > > > + > > > struct reg_value { > > > u16 reg; > > > u8 val; > > > @@ -87,7 +97,7 @@ struct ov5645 { > > > struct i2c_client *i2c_client; > > > struct device *dev; > > > struct v4l2_subdev sd; > > > - struct media_pad pad; > > > + struct media_pad pads[OV5645_NUM_PADS]; > > > struct v4l2_fwnode_endpoint ep; > > > struct v4l2_rect crop; > > > struct clk *xclk; > > > @@ -826,7 +836,10 @@ static int ov5645_enum_mbus_code(struct v4l2_subdev *sd, > > > if (code->index > 0) > > > return -EINVAL; > > > > > > - code->code = MEDIA_BUS_FMT_UYVY8_1X16; > > > + if (code->pad == OV5645_PAD_IMAGE) > > > + code->code = OV5645_NATIVE_FORMAT; > > > + else > > > + code->code = MEDIA_BUS_FMT_UYVY8_1X16; > > > > > > return 0; > > > } > > > @@ -835,16 +848,24 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev, > > > struct v4l2_subdev_state *sd_state, > > > struct v4l2_subdev_frame_size_enum *fse) > > > { > > > - if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16) > > > - return -EINVAL; > > > - > > > - if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > > > - return -EINVAL; > > > - > > > - fse->min_width = ov5645_mode_info_data[fse->index].width; > > > - fse->max_width = ov5645_mode_info_data[fse->index].width; > > > - fse->min_height = ov5645_mode_info_data[fse->index].height; > > > - fse->max_height = ov5645_mode_info_data[fse->index].height; > > > + if (fse->pad == OV5645_PAD_IMAGE) { > > > + if (fse->code != OV5645_NATIVE_FORMAT || fse->index > 0) > > > + return -EINVAL; > > > + > > > + fse->min_width = OV5645_NATIVE_WIDTH; > > > + fse->max_width = OV5645_NATIVE_WIDTH; > > > + fse->min_height = OV5645_NATIVE_HEIGHT; > > > + fse->max_height = OV5645_NATIVE_HEIGHT; > > > + } else { > > > + if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16 || > > > > This will be interesting. With internal pads we will introduce the > > concept of a "subdev model", which will formally document how the V4L2 > > subdev configuration items (formats, selection rectangles, ...) maps to > > hardware features. Sakari is working on the definition of a subdev model > > for raw sensors, that should catter for the needs of raw sensors without > > a bayer scaler (the vast majority of them). To use internal pads with a > > non-raw sensor, we'll have to define a model. It may be more > > challenging/complicated to do so, as the YUV sensor features are less > > standardized, but it will be an interesting development. > > I think I'll make the sub-device model a bitmask, to allow implementing > more than one at the same time. > > I'll try to remember to cc you to the patchset when I'll send it, likely > next week. > Great, I'll withhold sending a v3. > > > > > + fse->index >= ARRAY_SIZE(ov5645_mode_info_data)) > > > + return -EINVAL; > > > + > > > + fse->min_width = ov5645_mode_info_data[fse->index].width; > > > + fse->max_width = ov5645_mode_info_data[fse->index].width; > > > + fse->min_height = ov5645_mode_info_data[fse->index].height; > > > + fse->max_height = ov5645_mode_info_data[fse->index].height; > > > + } > > > > > > return 0; > > > } > > > @@ -855,18 +876,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > > > { > > > struct ov5645 *ov5645 = to_ov5645(sd); > > > struct v4l2_mbus_framefmt *__format; > > > + struct v4l2_rect *compose; > > > struct v4l2_rect *__crop; > > > > While at it, could you rename __crop to crop ? > > > > > const struct ov5645_mode_info *new_mode; > > > int ret; > > > > > > - __crop = v4l2_subdev_state_get_crop(sd_state, 0); > > > + if (format->pad != OV5645_PAD_SOURCE) > > > + return v4l2_subdev_get_fmt(sd, sd_state, format); > > > + > > > new_mode = v4l2_find_nearest_size(ov5645_mode_info_data, > > > ARRAY_SIZE(ov5645_mode_info_data), > > > width, height, format->format.width, > > > format->format.height); > > > - > > > - __crop->width = new_mode->width; > > > - __crop->height = new_mode->height; > > > + format->format.code = MEDIA_BUS_FMT_UYVY8_1X16; > > > + format->format.width = new_mode->width; > > > + format->format.height = new_mode->height; > > > + format->format.field = V4L2_FIELD_NONE; > > > + format->format.colorspace = V4L2_COLORSPACE_SRGB; > > > + format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT; > > > + format->format.quantization = V4L2_QUANTIZATION_DEFAULT; > > > + format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT; > > > > Drivers are not supposed to return DEFAULT values, you should pick > > appropriate values. > > > > > + > > > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE); > > > > Maybe __format could also become fmt. > > > > > + *__format = format->format; > > > + __format->code = OV5645_NATIVE_FORMAT; > > > + __format->width = OV5645_NATIVE_WIDTH; > > > + __format->height = OV5645_NATIVE_HEIGHT; > > > + > > > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE); > > > + __crop->width = format->format.width; > > > + __crop->height = format->format.height; > > > + > > > + /* > > > + * The compose rectangle models binning, its size is the sensor output > > > + * size. > > > + */ > > > + compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE); > > > + compose->left = 0; > > > + compose->top = 0; > > > + compose->width = format->format.width; > > > + compose->height = format->format.height; > > > + > > > + __crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE); > > > + __crop->left = 0; > > > + __crop->top = 0; > > > + __crop->width = format->format.width; > > > + __crop->height = format->format.height; > > > + > > > + __format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE); > > > + *__format = format->format; > > > > > > if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > > ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock, > > > @@ -882,14 +940,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd, > > > ov5645->current_mode = new_mode; > > > } > > > > > > - __format = v4l2_subdev_state_get_format(sd_state, 0); > > > - __format->width = __crop->width; > > > - __format->height = __crop->height; > > > - __format->code = MEDIA_BUS_FMT_UYVY8_1X16; > > > - __format->field = V4L2_FIELD_NONE; > > > - __format->colorspace = V4L2_COLORSPACE_SRGB; > > > - > > > - format->format = *__format; > > > > > > return 0; > > > } > > > @@ -899,7 +949,7 @@ static int ov5645_init_state(struct v4l2_subdev *subdev, > > > { > > > struct v4l2_subdev_format fmt = { > > > .which = V4L2_SUBDEV_FORMAT_TRY, > > > - .pad = 0, > > > + .pad = OV5645_PAD_SOURCE, > > > .format = { > > > .code = MEDIA_BUS_FMT_UYVY8_1X16, > > > .width = ov5645_mode_info_data[1].width, > > > @@ -919,7 +969,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd, > > > if (sel->target != V4L2_SEL_TGT_CROP) > > > return -EINVAL; > > > > > > - sel->r = *v4l2_subdev_state_get_crop(sd_state, 0); > > > + sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad); > > > > Now there's a compose rectangle, you should support getting it. > > > > > return 0; > > > } > > > > > > @@ -1123,11 +1173,12 @@ static int ov5645_probe(struct i2c_client *client) > > > v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops); > > > ov5645->sd.internal_ops = &ov5645_internal_ops; > > > ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS; > > > - ov5645->pad.flags = MEDIA_PAD_FL_SOURCE; > > > + ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > > > + ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL; > > > ov5645->sd.dev = dev; > > > ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > > > > > - ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad); > > > + ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads); > > > > Line wrap. > > It's good to run: > > scripts/checkpatch.pl --strict --max-line-length=80 > > on the patches. > Thanks for the pointer. Cheers, Prabhakar
Hi Prabhakar, On Fri, Sep 27, 2024 at 04:31:31PM +0100, Lad, Prabhakar wrote: > Hi Sakari and Laurent, > > On Thu, Sep 26, 2024 at 7:57 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > On Thu, Sep 26, 2024 at 08:48:19PM +0300, Laurent Pinchart wrote: > > > On Thu, Sep 26, 2024 at 03:45:26PM +0000, Sakari Ailus wrote: > > > > Hi Prabhakar, > > > > > > > > Thanks for the set. It looks largely very nice to me, after addressing > > > > Laurent's comments. A few comments here and possibly on other patches... > > > > > > > > On Tue, Sep 10, 2024 at 06:06:10PM +0100, Prabhakar wrote: > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > Implement the .get_frame_desc() subdev operation to report information > > > > > about streams to the connected CSI-2 receiver. This is required to let > > > > > the CSI-2 receiver driver know about virtual channels and data types for > > > > > each stream. > > > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > --- > > > > > drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++ > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > > > index 7f1133292ffc..c24eb6e7a7b5 100644 > > > > > --- a/drivers/media/i2c/ov5645.c > > > > > +++ b/drivers/media/i2c/ov5645.c > > > > > @@ -28,6 +28,7 @@ > > > > > #include <linux/regulator/consumer.h> > > > > > #include <linux/slab.h> > > > > > #include <linux/types.h> > > > > > +#include <media/mipi-csi2.h> > > > > > #include <media/v4l2-ctrls.h> > > > > > #include <media/v4l2-event.h> > > > > > #include <media/v4l2-fwnode.h> > > > > > @@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = { > > > > > .s_ctrl = ov5645_s_ctrl, > > > > > }; > > > > > > > > > > +static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > > > > + struct v4l2_mbus_frame_desc *fd) > > > > > +{ > > > > > + const struct v4l2_mbus_framefmt *fmt; > > > > > + struct v4l2_subdev_state *state; > > > > > + > > > > > + if (pad != OV5645_PAD_SOURCE) > > > > > + return -EINVAL; > > > > > > > > As you have a single source pad, and pretty much all sensor drivers will, I > > > > think it'd be nice to add a check for this (that it's not an internal pad) > > > > to the caller side in v4l2-subdev.c. And of course drop this one. > > > > > > What check would you add, just verifying that the pad is a source pad ? > > > > I think you could add that, too, besides the absence of the internal flag. > > > Checking only for the source flag should suffice, as the > MEDIA_PAD_FL_INTERNAL flag cannot be set for a source pad because > media_entity_pads_init() enforces this restriction. > > Do you agree? Works for me.
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Hi All, This patch series aims to add the below features, - Support subdev active state - Support for streams - Support for virtual channel - Code cleanup Note, these patches are dependent on below: 1] https://patchwork.kernel.org/project/linux-media/patch/20240416193319.778192-27-sakari.ailus@linux.intel.com/ 2] https://patchwork.kernel.org/project/linux-media/patch/20240416193319.778192-26-sakari.ailus@linux.intel.com/ RFC->v2 - Dropped setting of VC using routes - Defaulted the native format to MEDIA_BUS_FMT_SBGGR8_1X8 - Fixed ov5645_enum_frame_size and ov5645_enum_mbus_code for internal image pad RFC patch, Link: https://lore.kernel.org/all/20240904210719.52466-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Test logs: ==================================== root@smarc-rzg2l:~# media-ctl -p ..... - entity 4: ov5645 0-003c (2 pads, 1 link, 1 route) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev1 routes: 1/0 -> 0/0 [ACTIVE] pad0: SOURCE [stream:0 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb crop:(0,0)/1920x1080] -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE] pad1: SINK,0x8 [stream:0 fmt:SBGGR8_1X8/2592x1944 field:none colorspace:srgb crop:(0,0)/1920x1080] ..... root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-mbus-codes pad=0 ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0,stream=0) 0x200f: MEDIA_BUS_FMT_UYVY8_1X16 root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-mbus-codes pad=1 ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=1,stream=0) 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8 root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-framesizes pad=1,code=0x3001 ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=1,stream=0) Size Range: 2592x1944 - 2592x1944 root@smarc-rzg2l:~# v4l2-ctl --device /dev/v4l-subdev1 --list-subdev-framesizes pad=0,code=0x200f ioctl: VIDIOC_SUBDEV_ENUM_FRAME_SIZE (pad=0,stream=0) Size Range: 1280x960 - 1280x960 Size Range: 1920x1080 - 1920x1080 Size Range: 2592x1944 - 2592x1944 root@smarc-rzg2l:~# v4l2-compliance log: ------------------- root@smarc-rzg2l:~# v4l2-compliance -u /dev/v4l-subdev1 v4l2-compliance 1.28.1-5233, 64 bits, 64-bit time_t v4l2-compliance SHA: fc15e229d9d3 2024-07-23 19:22:15 Compliance test for device /dev/v[ 6347.789338] ov5645 0-003c: ================= START STATUS ================= 4l-subdev1: Driver In[ 6347.798197] ov5645 0-003c: ================== END STATUS ================== fo: Driver version : 6.11.0 Capabilities : 0x00000002 Streams Support Client Capabilities: 0x0000000000000003 streams interval-uses-which Required ioctls: test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev1 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device routing ioctls: test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 12 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test CREATE_BUFS maximum buffers: OK test VIDIOC_REMOVE_BUFS: OK test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for device /dev/v4l-subdev1: 47, Succeeded: 47, Failed: 0, Warnings: 0 Lad Prabhakar (11): media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks media: i2c: ov5645: Use local `dev` pointer for subdev device assignment media: i2c: ov5645: Enable runtime PM after v4l2_async_register_subdev() media: i2c: ov5645: Use dev_err_probe instead of dev_err media: i2c: ov5645: Use v4l2_async_register_subdev_sensor() media: i2c: ov5645: Drop `power_lock` mutex media: i2c: ov5645: Use subdev active state media: i2c: ov5645: Switch to {enable,disable}_streams media: i2c: ov5645: Add internal image sink pad media: i2c: ov5645: Report internal routes to userspace media: i2c: ov5645: Report streams using frame descriptors drivers/media/i2c/ov5645.c | 433 ++++++++++++++++++++----------------- 1 file changed, 240 insertions(+), 193 deletions(-)