| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPY8ntA2TCf9FuB6Nk+On+y6N_PMuYPAOAr3Yx8YESwe4skWvw@mail.gmail.com>
Date: Tue, 18 Nov 2025 12:47:25 +0000
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>, Jacopo Mondi <jacopo@...ndi.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Kieran Bingham <kieran.bingham@...asonboard.com>,
David Plowman <david.plowman@...pberrypi.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>, Peter Robinson <pbrobinson@...il.com>,
Stefan Wahren <wahrenst@....net>, "Ivan T. Ivanov" <iivanov@...e.de>
Subject: Re: [PATCH 13/13] media: i2c: ov5647: Add V4L2_CID_LINK_FREQUENCY control
Hi Jacopo & Jai
On Tue, 18 Nov 2025 at 11:28, Jai Luthra <jai.luthra@...asonboard.com> wrote:
>
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-11-02 16:59:02)
> > Hi Jai
> >
> > On Tue, Oct 28, 2025 at 12:57:24PM +0530, Jai Luthra wrote:
> > > From: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > >
> > > The link frequency can vary between modes, so add it as a
> > > control.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > > Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> > > ---
> > > drivers/media/i2c/ov5647.c | 25 ++++++++++++++++++++++++-
> > > 1 file changed, 24 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> > > index be0b96c4372ae0c6d8fc57280b195d6069dd7019..dea978305c3c868819780f7f631b225f4c1e7756 100644
> > > --- a/drivers/media/i2c/ov5647.c
> > > +++ b/drivers/media/i2c/ov5647.c
> > > @@ -97,6 +97,13 @@ static const char * const ov5647_supply_names[] = {
> > >
> > > #define OV5647_NUM_SUPPLIES ARRAY_SIZE(ov5647_supply_names)
> > >
> > > +#define FREQ_INDEX_FULL 0
> > > +#define FREQ_INDEX_VGA 1
> > > +static const s64 ov5647_link_freqs[] = {
> > > + [FREQ_INDEX_FULL] = 218500000,
> >
> > The full mode pixel rate is set to 87500000, which considering CSI-2
> > DDR mode and the 2 lanes in use give me a link freq of 21875000.
>
> Indeed, I get the same value, will update.
Agreed. I obviously lost a digit.
> >
> > Do you know where 218500000 comes from ? (it might be perfectly legit,
> > I'm not questioning that).
> >
>
> > > + [FREQ_INDEX_VGA] = 208333000,
>
> This value should be 137500000 if we do the same calculation using the
> pixel rate for the VGA mode. But for the VGA mode, the sensor does 2x2
> binning + 2x2 subsampling, which is quite a bit different than other modes.
>
> https://docs.kernel.org/driver-api/media/tx-rx.html#pixel-rate mentions
> that the pixel rate value calculated from the bus link frequency does not
> necessarily have to match the PIXEL_RATE control value (which is for the
> sensor's internal readout of pixels including blanking).
Indeed you should never assume that pixel rate and link frequency are
directly linked. So many sensors have separate PLLs for the pixel
array vs the MIPI block.
Having said that, OV5647 appears to use the same PLL for pixel clock
and MIPI, although it does have a separate PLLADCLK which is
presumably for the ADC.
> Ultimately, these values are coming from the BSP where the CFE driver is
> using the link frequency control to configure the DPHY-RX rate, so I think
> it would be wiser to not reduce the VGA link frequency value, which may
> cause issues with DPHY-RX latching. We can always fix it later if needed.
It's been a long time since I looked at these settings, but I do have
a spreadsheet from Omnivision that gives clock frequencies based on
register values.
In my experience the link frequency isn't critical to be exactly right
as it typically only sets up timeout ranges in the PHY. Even if the
value is significantly out it will generally work just fine.
VGA and full modes differ in register 0x3036 (SC_CMMN_PLL_MULTIPLIER)
which alters all the timings.
Running the numbers again, I get the VGA link frequency to be
145.8333MHz, but also the pixel rate to be 58.333MPix/s vs 55 in the
driver. I don't recall the VGA mode being 6% out on frame rate and
exposure setup, so I can't quite square that with reality. I'll try to
find 10 minutes to confirm, unless either of you happen to have one
set up and could validate the frame times.
Dave
> > > +};
> > > +
> > > struct regval_list {
> > > u16 addr;
> > > u8 data;
> > > @@ -106,6 +113,7 @@ struct ov5647_mode {
> > > struct v4l2_mbus_framefmt format;
> > > struct v4l2_rect crop;
> > > u64 pixel_rate;
> > > + unsigned int link_freq_index;
> > > int hts;
> > > int vts;
> > > const struct regval_list *reg_list;
> > > @@ -128,6 +136,7 @@ struct ov5647 {
> > > struct v4l2_ctrl *exposure;
> > > struct v4l2_ctrl *hflip;
> > > struct v4l2_ctrl *vflip;
> > > + struct v4l2_ctrl *link_freq;
> > > };
> > >
> > > static inline struct ov5647 *to_sensor(struct v4l2_subdev *sd)
> > > @@ -376,6 +385,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1944
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 2844,
> > > .vts = 0x7b0,
> > > .reg_list = ov5647_2592x1944_10bpp,
> > > @@ -397,6 +407,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1080,
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 2416,
> > > .vts = 0x450,
> > > .reg_list = ov5647_1080p30_10bpp,
> > > @@ -418,6 +429,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1944,
> > > },
> > > .pixel_rate = 87500000,
> > > + .link_freq_index = FREQ_INDEX_FULL,
> > > .hts = 1896,
> > > .vts = 0x59b,
> > > .reg_list = ov5647_2x2binned_10bpp,
> > > @@ -439,6 +451,7 @@ static const struct ov5647_mode ov5647_modes[] = {
> > > .height = 1920,
> > > },
> > > .pixel_rate = 55000000,
> > > + .link_freq_index = FREQ_INDEX_VGA,
> > > .hts = 1852,
> > > .vts = 0x1f8,
> > > .reg_list = ov5647_640x480_10bpp,
> > > @@ -925,6 +938,8 @@ static int ov5647_set_pad_fmt(struct v4l2_subdev *sd,
> > > sensor->exposure->minimum,
> > > exposure_max, sensor->exposure->step,
> > > exposure_def);
> > > +
> > > + __v4l2_ctrl_s_ctrl(sensor->link_freq, mode->link_freq_index);
> >
> > Doesn't this cause an error in s_ctrl where the control is not handled
> > ?
>
> The framework returns -EACCESS for read-only controls in validate_ctrls()
>
> >
> > > }
> > > *fmt = mode->format;
> > > /* The code we pass back must reflect the current h/vflips. */
> > > @@ -1230,7 +1245,7 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > int hblank, exposure_max, exposure_def;
> > > struct v4l2_fwnode_device_properties props;
> > >
> > > - v4l2_ctrl_handler_init(&sensor->ctrls, 9);
> > > + v4l2_ctrl_handler_init(&sensor->ctrls, 10);
> > >
> > > v4l2_ctrl_new_std(&sensor->ctrls, &ov5647_ctrl_ops,
> > > V4L2_CID_AUTOGAIN, 0, 1, 1, 0);
> > > @@ -1290,6 +1305,14 @@ static int ov5647_init_controls(struct ov5647 *sensor, struct device *dev)
> > > if (sensor->vflip)
> > > sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> > >
> > > + sensor->link_freq =
> > > + v4l2_ctrl_new_int_menu(&sensor->ctrls, &ov5647_ctrl_ops,
> >
> > As suggested for PIXEL_RATE, if you make the control read-only you
> > should set the control ops to NULL.
>
> Will do in v2.
>
> > > + V4L2_CID_LINK_FREQ,
> > > + ARRAY_SIZE(ov5647_link_freqs) - 1, 0,
> > > + ov5647_link_freqs);
> > > + if (sensor->link_freq)
> > > + sensor->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > You know, I thought link_freq was set as READ_ONLY by the framework,
> > but it's actuallt PIXEL_RATE (you can remove setting the flags
> > in the driver if you send a patch to remove the control ops when
> > registering PIXEL_RATE).
>
> Will do.
>
> >
> > Thanks
> > j
> >
> > > +
> > > v4l2_fwnode_device_parse(dev, &props);
> > >
> > > v4l2_ctrl_new_fwnode_properties(&sensor->ctrls, &ov5647_ctrl_ops,
> > >
> > > --
> > > 2.51.0
> > >
>
> Thanks,
> Jai
Powered by blists - more mailing lists