| 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: <CAPY8ntD7M5k+v-oPOMhyJV=_OcHwVjq_32m18_MrHO4zsYQOzg@mail.gmail.com>
Date: Tue, 3 Sep 2024 15:13:20 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Benjamin Bara <bbara93@...il.com>, Mauro Carvalho Chehab <mchehab@...nel.org>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans de Goede <hdegoede@...hat.com>,
Alexander Stein <alexander.stein@...tq-group.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH v3 4/7] media: i2c: imx290: Introduce initial "off" mode &
link freq
On Tue, 3 Sept 2024 at 14:01, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
>
> On Mon, Sep 02, 2024 at 10:55:04PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 21:58, Laurent Pinchart wrote:
> > > On Mon, Sep 02, 2024 at 05:57:29PM +0200, bbara93@...il.com wrote:
> > > > From: Benjamin Bara <benjamin.bara@...data.com>
> > > >
> > > > To be compliant to the V4L2 API, the driver currently "randomly" decides
> > > > on one of the two supported modes which also implies a link frequency.
> > > >
> > > > Add a new mode and frequency which symbolize that the sensor is not in
> > > > use. This can be used as a default value during probe() and enables us
> > > > to avoid communication with the sensor.
> > >
> > > I really doin't like this change. I would like to instead move away from
> > > modes and make the driver freely configurable.
> >
> > Which controls do you want to have freely configurable? At least on the
> > imx290 the exposure limits depend on the blanking, and the blanking
> > limits depend on the format. As the format is defined by the mode on
> > imx290, I think this will be quite hard with the current controls.
>
> I want to make the format freely configurable.
Isn't this partly limited by the discussion led by Jacopo in Dublin
back in 2022 [1]?
How do we shift from the current list of 2 modes, to a single defined
mode that the selection API can then crop?
Changing the list of enumerated modes is likely to break existing
users of this driver, and AIUI it isn't currently permitted for
selecting a new enumerated mode to update the selection rectangles.
It also needs someone to sit down and fully digest the window cropping
mode section of the datasheet, and hope that the sensor actually
behaves as documented. I know I don't have the time to do that at
present.
[1] https://www.spinics.net/lists/linux-media/msg218231.html
> > > Furthermore, the concept of an initial unconfigured state isn't valid
> > > in V4L2. The driver must fully initialize the whole device state at
> > > probe time.
> >
> > I understand that and it makes sense to me. But given the dependencies
> > from above and the fact that the format is currently part of this
> > "state", it makes the "freely configurable" intention even harder :-(
>
> Why can't we simply initialize the controls with limits that correspond
> to the default format ? I don't understand what issue this is trying to
> solve.
If I'm following the full discussion correctly now, it's trying to
avoid those couple of register writes during probe due to updating
VBLANK and HBLANK control ranges. Adding all this "dummy mode" code
seems overkill to achieve that.
I see 3 simpler approaches:
- Move the pm_runtime calls so that the range updates are when the
sensor is powered down. Ack on that needing very careful handling of
what is initialised when.
- Move the lines
state = v4l2_subdev_lock_and_get_active_state(&imx290->sd);
imx290_ctrl_update(imx290, imx290->current_mode);
v4l2_subdev_unlock_state(state);
out of imx290_subdev_init to the end of imx290_probe, after the pm_runtime_put
- Add a flag to the state (in addition to the pm_runtime handling) to
denote that we're between stream_on and stream_off, and therefore
imx290_set_ctrl should update registers.
A quick test of the 2nd option (with patches 1, 3, and 6) appears to
achieve that aim - I only see the one read from patch 6.
IMHO trying to address the case where runtime PM is disabled is fairly
redundant, but others may disagree. Option 3 would achieve that with
minimal extra overhead though.
Dave
> > > > Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> > > > ---
> > > > Changes since v2:
> > > > - new
> > > > ---
> > > > drivers/media/i2c/imx290.c | 29 +++++++++++++++++++++++------
> > > > 1 file changed, 23 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 6812e7cb9e23..ece4d66001f5 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -425,14 +425,17 @@ static const struct imx290_csi_cfg imx290_csi_297mhz = {
> > > > /* supported link frequencies */
> > > > #define FREQ_INDEX_1080P 0
> > > > #define FREQ_INDEX_720P 1
> > > > +#define FREQ_INDEX_OFF 2
> > > > static const s64 imx290_link_freq_2lanes[] = {
> > > > [FREQ_INDEX_1080P] = 445500000,
> > > > [FREQ_INDEX_720P] = 297000000,
> > > > + [FREQ_INDEX_OFF] = 0,
> > > > };
> > > >
> > > > static const s64 imx290_link_freq_4lanes[] = {
> > > > [FREQ_INDEX_1080P] = 222750000,
> > > > [FREQ_INDEX_720P] = 148500000,
> > > > + [FREQ_INDEX_OFF] = 0,
> > > > };
> > > >
> > > > /*
> > > > @@ -552,6 +555,10 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > > > },
> > > > };
> > > >
> > > > +static const struct imx290_mode imx290_mode_off = {
> > > > + .link_freq_index = FREQ_INDEX_OFF,
> > > > +};
> > > > +
> > > > static inline const struct imx290_mode *imx290_modes_ptr(const struct imx290 *imx290)
> > > > {
> > > > if (imx290->nlanes == 2)
> > > > @@ -876,10 +883,19 @@ static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > > static void imx290_ctrl_update(struct imx290 *imx290,
> > > > const struct imx290_mode *mode)
> > > > {
> > > > - unsigned int hblank_min = mode->hmax_min - mode->width;
> > > > - unsigned int hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > - unsigned int vblank_min = mode->vmax_min - mode->height;
> > > > - unsigned int vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > + unsigned int hblank_min, hblank_max, vblank_min, vblank_max;
> > > > +
> > > > + if (mode == &imx290_mode_off) {
> > > > + hblank_min = imx290_get_blank_min(imx290, false);
> > > > + hblank_max = HBLANK_MAX;
> > > > + vblank_min = imx290_get_blank_min(imx290, true);
> > > > + vblank_max = VBLANK_MAX;
> > > > + } else {
> > > > + hblank_min = mode->hmax_min - mode->width;
> > > > + hblank_max = IMX290_HMAX_MAX - mode->width;
> > > > + vblank_min = mode->vmax_min - mode->height;
> > > > + vblank_max = IMX290_VMAX_MAX - mode->height;
> > > > + }
> > > >
> > > > __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
> > > >
> > > > @@ -932,7 +948,8 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > > > imx290->link_freq =
> > > > v4l2_ctrl_new_int_menu(&imx290->ctrls, &imx290_ctrl_ops,
> > > > V4L2_CID_LINK_FREQ,
> > > > - imx290_link_freqs_num(imx290) - 1, 0,
> > > > + imx290_link_freqs_num(imx290) - 1,
> > > > + FREQ_INDEX_OFF,
> > > > imx290_link_freqs_ptr(imx290));
> > > > if (imx290->link_freq)
> > > > imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > > @@ -1278,7 +1295,7 @@ static int imx290_subdev_init(struct imx290 *imx290)
> > > > struct v4l2_subdev_state *state;
> > > > int ret;
> > > >
> > > > - imx290->current_mode = &imx290_modes_ptr(imx290)[0];
> > > > + imx290->current_mode = &imx290_mode_off;
> > > >
> > > > v4l2_i2c_subdev_init(&imx290->sd, client, &imx290_subdev_ops);
> > > > imx290->sd.internal_ops = &imx290_internal_ops;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
>
Powered by blists - more mailing lists