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

[Jai Luthra]

Quoting Jai Luthra (2026-05-14 10:42:11)
> Hi Dave
> 
> Thank you for the review.
> 
> Quoting Dave Stevenson (2026-05-13 23:58:32)
> > Hi Jai
> > 
> > I think Soho Enterprises sent me an IMX678 module a while back, so
> > I'll try and track it down and test the driver out.
> > 
> > A couple of comments based on a quick read though.
> > 
> > On Wed, 13 May 2026 at 16:42, Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx> wrote:
> > >
> > > 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 resolution through S_FMT ioctl
> > > - Freely configurable crop through S_SELECTION ioctl
> > > - 2x2 binning configurable via S_FMT/S_SELECTION APIs
> > > - 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>

[snip]

> > > +
> > > +static u64 imx678_output_pixel_rate(struct imx678 *imx678)
> > > +{
> > > +       const u32 lane_count = imx678->lane_count;
> > > +       const u64 link_freq = link_freqs[imx678->link_freq_idx];
> > > +       const u8 bpp = 12;
> > > +       u64 numerator = link_freq * 2 * lane_count;
> > > +
> > > +       do_div(numerator, bpp);
> > 
> > I don't believe the pixel rate changes with link frequency or bit
> > depth, and plausibly you're ending up with the same number every time.
> > 
> > Reading the Software Reference Manual (rev 7.0), Section 4 "Operating Mode".
> > 2 lanes at 1440Mbit/s/lane 10bpp 25fps readout requires 1H period
> > (HMAX) of 1320, and 1V period (VMAX) of 2250.
> > 2 lanes at 1782Mbit/s/lane 12bpp 25fps readout requires 1H period
> > (HMAX) of 1320, and 1V period (VMAX) of 2250.
> > So there is no change in HMAX or VMAX when changing bit depth, so
> > pixel rate must be the same.
> > You can get away with a lower link frequency as there is less data to send.
> > 
> > 4 lanes at 720Mbit/s/lane 10bpp 25fps readout requires 1H period
> > (HMAX) of 1320, and 1V period (VMAX) of 2250.
> > So again no change with number of lanes and link frequency.
> > 
> > 
> > Potentially coincidence, but if you play with the numbers from the
> > 12bit readout mode:
> > 2 lanes at 1782Mbit/s/lane 12bpp 25fps readout requires 1H period
> > (HMAX) of 1320, and 1V period (VMAX) of 2250.
> > 
> > 1782Mbits/lane * 2 lanes / 12bpp / 25fps = 11,880,000Pixel_clocks/frame.
> > 11880000 / 2250 (VMAX) = 5280, which so happens to be exactly 1320 (HMAX) * 4
> 
> Good find! You're right the FPS is only tied to 1H/1V period in that table,
> so pixel rate should be independent of lane/linkrate/bpp.
> 
> I'll update this in v2, and cleaner blanking calculation will anyway be
> useful to support higher FPS (lower hblank) when doing cropping. It would
> be ideal if I manage to figure out the minimum HMAX programmatically for
> each link rate/crop so the table can be dropped.
> 
> > 
> > If you define the pixel rate as 297MPix/s (1782*2/12), treat the
> > HBLANK and VBLANK controls in the normal manner, but write HMAX as
> > (width + hblank / 4), I believe you'll find all the numbers fall out
> > to perfectly match the datasheet.
> > 

The trouble with 4x was that it resulted in negative blankings for higher
link frequency 4-lane modes.

The Operation Mode section also runs at 60FPS with 4 lanes of 1728Mbps,
where (1728M * 4 / 12 / 60)/2250 = 4400 which matches 550 (HMAX) * 8.

So I've ended up with 8x ratio, i.e. pixel rate of 594MPix/s. I'll post v2
with that.

> > This is the approach I found worked for IMX415 (needed /12) and IMX662
> > (needed /3).
> > 
> > > +
> > > +       return numerator;
> > > +}
> > > +

[snip]

Thanks,
    Jai