Re: [PATCH v3 3/3] media: i2c: os02g10: implement crop handling with set_selection

[Tarang Raval]

> On 6/12/26 14:41, Tarang Raval wrote:
> > Hi Vladimir,
> >
> > Thank you for the review.
> >
> > Since I authored this patch, I will try to address the comments below.
> >
> >> On 4/24/26 12:25, Elgin Perumbilly wrote:
> >>> From: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> >>>
> >>> Add crop support to os02g10 by implementing .set_selection() and
> >>> storing the crop rectangle in subdev state.
> >>>
> >>> Initialize the default crop to the active area, make set_fmt() use the
> >>> current crop, and update the output format when the crop size changes.
> >>> Also program the sensor window from the active crop/format state instead
> >>> of using the fixed supported_modes entry.
> >>>
> >>> This allows userspace to configure the sensor crop window explicitly.
> >>>
> >>> Signed-off-by: Tarang Raval <tarang.raval@xxxxxxxxxxxxxxxxx>
> >>> Signed-off-by: Elgin Perumbilly <elgin.perumbilly@xxxxxxxxxxxxxxxxx>
> >>> ---
> >>>    drivers/media/i2c/os02g10.c | 166 ++++++++++++++++++++++--------------
> >>>    1 file changed, 103 insertions(+), 63 deletions(-)
> >>>
> >>> diff --git a/drivers/media/i2c/os02g10.c b/drivers/media/i2c/os02g10.c
> >>> index fad2dd0ad7aa..9bf8f5d1caea 100644
> >>> --- a/drivers/media/i2c/os02g10.c
> >>> +++ b/drivers/media/i2c/os02g10.c
> >>> @@ -112,6 +112,11 @@
> >>>    #define OS02G10_ORIENTATION_BAYER_FIX               0x32
> >>>
> >>>    #define OS02G10_LINK_FREQ_720MHZ            (720 * HZ_PER_MHZ)
> >>> +#define OS02G10_WINDOW_WIDTH_MIN             2
> >>> +#define OS02G10_WINDOW_HEIGHT_MIN            2
> >>
> >> Add a blank line before the new group of macro.
> >>
> >>> +#define OS02G10_VBLANK_DEF                   166
> >>
> >> This one is computable, and it can be dropped.
> >
> >
> > Can you explain how this value can be computed ?
>
> It is (supported_modes[0].vts_def - supported_modes[0].height) on
> the base of 2/3 change.

In this patch, the supported_modes structure has been removed, so the value
can no longer be computed in that way.

> >>> +#define OS02G10_VBLANK_MIN                   25
> >>
> >> This macro shall be added to the group of OS02G10_REG_FRAME_LENGTH
> >> register, and it should be included into the previous change.
> >>
> >>> +#define OS02G10_EXPOSURE_DEF                 1100
> >>
> >> This macro shall be added to the group of OS02G10_REG_LONG_EXPOSURE
> >> register, and it should be included into the previous change.
> >
> >
> > I would prefer to introduce these macros here only, as they are related to
> > this patch. There is no use of these macros in the previous patch, so moving
> > them there would not provide any benefit.
>
> Unavoidably there shall be a user of "default exposure" in the change 2/3, and
> therefore the value is present, please reference to supported_modes[0].exp_def.
>
> So, this macro goes to the 2/3 patch.

In patch 2/3, the supported_modes structure is still present, so I don't
think introducing a separate macro there provides much benefit.

Using:

vts_def = height + OS02G10_VBLANK_DEF;
exp_def = OS02G10_VBLANK_DEF;

doesn't look particularly readable to me either. However, I'd be interested
to hear your preference on this.

> >
> > I will group them in the appropriate place within this patch as per your
> > suggestion.
> >
> >
> >>>
> >>>    /* OS02G10 native and active pixel array size */
> >>>    static const struct v4l2_rect os02g10_native_area = {
> >>> @@ -152,15 +157,6 @@ struct os02g10 {
> >>>        struct v4l2_ctrl *hflip;
> >>>    };
> >>>
> >>> -struct os02g10_mode {
> >>> -     u32 width;
> >>> -     u32 height;
> >>> -     u32 vts_def;
> >>> -     u32 exp_def;
> >>> -     u32 x_start;
> >>> -     u32 y_start;
> >>> -};
> >>> -
> >>>    static const struct cci_reg_sequence os02g10_common_regs[] = {
> >>>        { OS02G10_REG_PLL_DIV_CTRL,             0x0a},
> >>>        { OS02G10_REG_PLL_DCTL_BIAS_CTRL,       0x04},
> >
> > ...
> >
> >>>    static const struct v4l2_subdev_video_ops os02g10_video_ops = {
> >>> @@ -645,6 +684,7 @@ static const struct v4l2_subdev_pad_ops os02g10_pad_ops = {
> >>>        .get_fmt = v4l2_subdev_get_fmt,
> >>>        .set_fmt = os02g10_set_pad_format,
> >>>        .get_selection = os02g10_get_selection,
> >>> +     .set_selection = os02g10_set_selection,
> >>>        .enum_frame_size = os02g10_enum_frame_size,
> >>>        .enable_streams = os02g10_enable_streams,
> >>>        .disable_streams = os02g10_disable_streams,
> >>
> >> I understand that this change is written by another person, and likely
> >> it is not squashed with the previous one to preserve authorship, however
> >> it significantly rewrites the change already found in the series.
> >
> > I don't think this patch significantly rewrites the previous changes. Its
> > main purpose is to introduce a crop rectangle and implement set_selection(),
> > allowing userspace to stream arbitrary resolutions within the sensor limits
> > instead of being restricted to 1920x1080.
>
> What are these "sensor limits" number? Are they defined in this 3/3 change?
> For whatever reason I can't find it, but I may be blind.

The sensor limits are defined by os02g10_active_area and enforced in
os02g10_set_selection(). The crop rectangle cannot extend beyond the
active sensor area, and the minimum crop size is also constrained in
the same function.

+       rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 2),
+                            OS02G10_WINDOW_WIDTH_MIN,
+                            os02g10_active_area.width);
+       rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
+                             OS02G10_WINDOW_HEIGHT_MIN,
+                             os02g10_active_area.height);

> >> I don't see information about the maximum supported frame height/width
> >> or default VTS setting etc. anymore, for me it's hard to say, if
> >> this kind of information can be dropped with no consequences in runtime.
> >
> > The maximum supported width and height are still defined and enforced in
> > os02g10_set_selection(). The default VTS handling is implemented in
> > os02g10_set_pad_format(), where VBLANK is adjusted to maintain 30 fps for
> > the selected resolution. So I don't believe any information has been dropped.
> >
> >> Probably this 3/3 change will break a quick inclusion of the sensor
> >> driver, you may consider to exlcude it from the series now, and publish
> >> it afterwards.
> >
> > Regarding whether patch 3/3 should be included in the current series, I am
> > happy to leave that decision to Sakari. If needed, this patch can be merged
> > separately once the new raw sensor model is finalized.
> >
> > This patch was created based on Laurent's suggestion.
> >
> > Link: https://lore.kernel.org/linux-media/20260414084952.217215-1-elgin.perumbilly@xxxxxxxxxxxxxxxxx/T/#t
> >
>
> That's a helpful reference to the previous patch review, thank you.
> Likely I don't have the whole picture of the new advances in linux-media.

No worries, and thank you for taking the time to review the series and
provide feedback.

Best Regards,
Tarang