| 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: <176346530691.268162.12757814341347187181@freya>
Date: Tue, 18 Nov 2025 16:58:26 +0530
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Jacopo Mondi <jacopo.mondi@...asonboard.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, Dave Stevenson <dave.stevenson@...pberrypi.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,
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.
>
> 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).
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.
> > +};
> > +
> > 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