Re: [PATCH v3 2/2] media: i2c: imx678: Add driver for Sony IMX678

[Tarang Raval]

Hi Jai,

> Quoting Tarang Raval (2026-05-22 19:01:01)
> > Hi Jai,
> >
> > I noticed a few issues and also have one question. Could you please help me
> > understand that part?
> >
> > Please check the comments below.
> >
> > > Add a V4L2 subdev driver for the Sony IMX678 image sensor.
> > >
> > > IMX678 is a diagonal 8.86 mm (Type 1/1.8) CMOS active pixel type
> > > solid-state image sensor with a square pixel array and 8.40 M effective
> > > pixels.
> > >
> > > The following features are supported by the driver:
> > > - Monochrome and Color (Bayer filter) variants
> > > - Multiple input clock frequencies supported
> > > - Multiple link frequencies supported
> > > - VBLANK and HBLANK control for variable framerate
> > > - Freely configurable crop rectangle through S_SELECTION ioctl
> > > - Configurable resolution with 2x2 binning (for the current crop)
> > >   through S_FMT ioctl
> > > - VFLIP and HFLIP control for flipping readout
> > > - Test pattern control support
> > > - Exposure and gain control
> > > - MIPI RAW12 output
> > >
> > > Following features are not currently supported but may be added later:
> > > - Pixel-perfect crop reporting, account for the shift-by-1 when flipping
> > >   using HFLIP/VFLIP, which maintains the bayer readout order
> > > - Increased framerate (lower HMAX/VMAX) when cropping
> > > - MIPI RAW10 output mode
> > > - Embedded data stream
> > >
> > > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > > ---

...

> > > +       }
> > > +
> > > +       /*
> > > +        * Applying V4L2 control value only happens
> > > +        * when power is up for streaming
> > > +        */
> > > +       if (pm_runtime_get_if_in_use(&client->dev) == 0)
> >
> > Use pm_runtime_get_if_active.
> >
>
> We anyway write all control values everytime in enable_streams(), so I
> think it's okay to keep this check a bit strict and skip the writes if the
> sensor is RPM_ACTIVE with 0 users. (i.e. streaming has stopped, but device
> not suspended yet, which is unlikely given we don't have an autosuspend
> timer but suspend immediately here)
>
> Unless of course I misunderstood why you're suggesting it?

Yes, replaying controls in enable_streams makes the next stream start correct.

While the device is still RPM_ACTIVE, the hardware is still accessible, so
skipping the write only because the runtime PM usage count is 0 creates a
temporary mismatch between the cached V4L2 control state and the actual
hardware state.

The immediate-suspend path makes this window small, but the window still exists
between pm_runtime_put and runtime suspend completing. Using pm_runtime_get_if_active
avoids relying on that timing.

So I still prefer to use pm_runtime_get_if_active().

...

> > > +static int imx678_set_selection(struct v4l2_subdev *sd,
> > > +                               struct v4l2_subdev_state *sd_state,
> > > +                               struct v4l2_subdev_selection *sel)
> > > +{
> > > +       struct imx678 *imx678 = to_imx678(sd);
> > > +       struct v4l2_rect *crop;
> > > +       struct v4l2_rect rect;
> > > +
> > > +       if (sel->target != V4L2_SEL_TGT_CROP || sel->pad != 0)
> > > +               return -EINVAL;
> > > +
> > > +       if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE &&
> > > +           v4l2_subdev_is_streaming(sd))
> > > +               return -EBUSY;
> > > +
> > > +       /* Align left, top to 4 */
> > > +       rect.left = clamp_t(s32, ALIGN(sel->r.left, IMX678_CROP_HST_ALIGN),
> > > +                           imx678_active_area.left,
> > > +                           imx678_active_area.width - IMX678_PIXEL_ARRAY_MIN_WIDTH);
> >
> > You are ignoring the active_area offset here; please correct it.
> >
> > In imx296, crop bounds start at (0, 0), so no offset handling is needed there.
> >
> > You can refer to my patch:
> > https://lore.kernel.org/linux-media/20260424092554.26130-4-elgin.perumbilly@xxxxxxxxxxxxxxxxx/#t
> >
>
> Ah good catch, will fix.
>
> > > +       rect.top = clamp_t(s32, ALIGN(sel->r.top, IMX678_CROP_VST_ALIGN),
> > > +                          imx678_active_area.top,
> > > +                          imx678_active_area.height - IMX678_PIXEL_ARRAY_MIN_HEIGHT);
> > > +       /* Align width to 16 and height to 4 */
> > > +       rect.width = clamp_t(u32, ALIGN(sel->r.width, IMX678_CROP_HWIDTH_ALIGN),
> > > +                            IMX678_PIXEL_ARRAY_MIN_WIDTH, imx678_active_area.width);
> > > +       rect.height = clamp_t(u32, ALIGN(sel->r.height, IMX678_CROP_VWIDTH_ALIGN),
> > > +                             IMX678_PIXEL_ARRAY_MIN_HEIGHT, imx678_active_area.height);
> > > +
> > > +       rect.width = min_t(u32, rect.width, imx678_native_area.width - rect.left);
> > > +       rect.height = min_t(u32, rect.height, imx678_native_area.height - rect.top);
> > > +
> > > +       crop = v4l2_subdev_state_get_crop(sd_state, sel->pad);
> > > +
> > > +       if (rect.width != crop->width || rect.height != crop->height) {
> > > +               struct v4l2_mbus_framefmt *format =
> > > +                       v4l2_subdev_state_get_format(sd_state, sel->pad);
> > > +               format->width = rect.width;
> > > +               format->height = rect.height;
> >
> > Why are we not checking here whether binning mode is currently enabled?
> >
> > Suppose binning mode is enabled, and then userspace changes the crop.
> >
> > With the below lines:
> >
> > format->width = rect.width;
> > format->height = rect.height;
> >
> > the format size becomes equal to the crop size, which silently disables binning.
>
> This was intentional.
>
> Let's say I'm streaming in 640x480 binned mode (1280x960 crop) and want to
> switch to 3200x1600, I prefer if a single S_SELECTION call with 3200x1600
> crop size to do that directly.
>
> On the other hand, if I modify just the (top, left) of my crop without
> changing its size (1280x960), I want it to stay in the binned mode and not
> snap back the active format to full size.
>
> This is a matter of opinion though, which I hope would become irrelevant
> once we move to the new raw sensor model with explicit controls for
> binning. I plan to look into it before I post a v4, as Sakari suggested in
> his review.

I see, that makes sense.

I was mainly concerned about the implicit mode switch when changing the crop
size, but your behaviour seems reasonable as well.

Will revisit this once explicit binning controls are available in the new
raw sensor model.

Best Regards,
Tarang