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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAJpcXm6KiSgzxJ+AuC2gatSDmYZJhoEuKyF1QGCficJbYDuDmg@mail.gmail.com>
Date: Mon, 2 Sep 2024 23:17:27 +0200
From: Benjamin Bara <bbara93@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.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 2/7] media: i2c: imx290: Define absolute control ranges

Hi Laurent!

On Mon, 2 Sept 2024 at 22:06, Laurent Pinchart
<laurent.pinchart@...asonboard.com> wrote:
> On Mon, Sep 02, 2024 at 09:43:36PM +0200, Benjamin Bara wrote:
> > On Mon, 2 Sept 2024 at 20:00, Dave Stevenson
> > <dave.stevenson@...pberrypi.com> wrote:
> > > On Mon, 2 Sept 2024 at 16:58, <bbara93@...il.com> wrote:
> > > >
> > > > From: Benjamin Bara <benjamin.bara@...data.com>
> > > >
> > > > For now, the driver activates the first mode (1080p) as current active
> > > > mode in probe(). This e.g. means that one cannot set VBLANK below 45
> > > > (vmax_min - height), although theoretically the minimum is 30 (720p
> > > > mode). Define the absolute possible/supported ranges to have them
> > > > available later.
> > >
> > > Currently the driver will set the ranges for VBLANK and HBLANK
> > > whenever the mode changes.
> > >
> > > How is it helpful to fake these numbers? Seeing as they aren't
> > > reflecting anything useful, they may as well all be 0.
> > >
> > > > Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>
> > > > ---
> > > > Changes since v2:
> > > > - new
> > > > ---
> > > >  drivers/media/i2c/imx290.c | 36 ++++++++++++++++++++++++++++++++----
> > > >  1 file changed, 32 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > > > index 1c97f9650eb4..466492bab600 100644
> > > > --- a/drivers/media/i2c/imx290.c
> > > > +++ b/drivers/media/i2c/imx290.c
> > > > @@ -499,6 +499,10 @@ static const struct imx290_clk_cfg imx290_720p_clock_config[] = {
> > > >  };
> > > >
> > > >  /* Mode configs */
> > > > +#define WIDTH_720P     1280
> > > > +#define HEIGHT_720P    720
>
> That's pure obfuscation :-) Just use the values directly.
>
> > > > +#define MINIMUM_WIDTH  WIDTH_720P
> > > > +#define MINIMUM_HEIGHT HEIGHT_720P
>
> Driver-specific macros should have a driver-specific prefix.
>
> > > >  static const struct imx290_mode imx290_modes_2lanes[] = {
> > > >         {
> > > >                 .width = 1920,
> > > > @@ -512,8 +516,8 @@ static const struct imx290_mode imx290_modes_2lanes[] = {
> > > >                 .clk_cfg = imx290_1080p_clock_config,
> > > >         },
> > > >         {
> > > > -               .width = 1280,
> > > > -               .height = 720,
> > > > +               .width = WIDTH_720P,
> > > > +               .height = HEIGHT_720P,
> > > >                 .hmax_min = 3300,
> > > >                 .vmax_min = 750,
> > > >                 .link_freq_index = FREQ_INDEX_720P,
> > > > @@ -537,8 +541,8 @@ static const struct imx290_mode imx290_modes_4lanes[] = {
> > > >                 .clk_cfg = imx290_1080p_clock_config,
> > > >         },
> > > >         {
> > > > -               .width = 1280,
> > > > -               .height = 720,
> > > > +               .width = WIDTH_720P,
> > > > +               .height = HEIGHT_720P,
> > > >                 .hmax_min = 3300,
> > > >                 .vmax_min = 750,
> > > >                 .link_freq_index = FREQ_INDEX_720P,
> > > > @@ -846,6 +850,30 @@ static const char * const imx290_test_pattern_menu[] = {
> > > >         "000/555h Toggle Pattern",
> > > >  };
> > > >
> > > > +/* absolute supported control ranges */
> > > > +#define HBLANK_MAX     (IMX290_HMAX_MAX - MINIMUM_WIDTH)
> > > > +#define VBLANK_MAX     (IMX290_VMAX_MAX - MINIMUM_HEIGHT)
>
> Here too.

Thanks for the feedback but I will drop this patch. As we need to decide
on a link frequency, we need to decide on one of the two "all pixel"
modes the imx290 supports and therefore already know the mode-specific,
tight ranges starting from probe time.

Kind regards
Benjamin

> > > > +static unsigned int imx290_get_blank_min(const struct imx290 *imx290, bool v)
> > > > +{
> > >
> > > This function is never used in this patch. I'm surprised the compiler
> > > doesn't throw an error on a static function not being used.
> > > You first use it in patch 4 "Introduce initial "off" mode & link freq"
> > >
> > > > +       const struct imx290_mode *modes = imx290_modes_ptr(imx290);
> > > > +       unsigned int min = UINT_MAX;
> > > > +       int i;
> > > > +
> > > > +       for (i = 0; i < imx290_modes_num(imx290); i++) {
> > > > +               unsigned int tmp;
> > > > +
> > > > +               if (v)
> > > > +                       tmp = modes[i].hmax_min - modes[i].width;
> > >
> > > if (v)
> > >    return h
> > >
> > > With the complete series my sensor comes up with controls defined as
> > > vertical_blanking 0x009e0901 (int)    : min=280 max=261423 step=1
> > > default=280 value=280
> > > horizontal_blanking 0x009e0902 (int)    : min=30 max=64255 step=1
> > > default=30 value=30
> > >
> > > Set the mode to 1080p and I get
> > > vertical_blanking 0x009e0901 (int)    : min=45 max=261063 step=1
> > > default=45 value=1169
> > > horizontal_blanking 0x009e0902 (int)    : min=280 max=63615 step=1
> > > default=280 value=280
> >
> > The idea here is to have VBLANK=30 available in the initial "after
> > probe" state of the sensor. VBLANK=30 is a valid value for 720p mode,
> > but it cannot be set after probe, because the driver (not the user)
> > decided that 1080 mode is active. The idea is to relax the ranges while
> > the mode is not set. Once the mode is known, the values are tightened
> > to the real mode-dependent values.
> >
> > Kind regards
> > Benjamin
> >
> > >   Dave
> > >
> > > > +               else
> > > > +                       tmp = modes[i].vmax_min - modes[i].height;
> > > > +
> > > > +               if (tmp < min)
> > > > +                       min = tmp;
> > > > +       }
> > > > +
> > > > +       return min;
> > > > +}
> > > > +
> > > >  static void imx290_ctrl_update(struct imx290 *imx290,
> > > >                                const struct imx290_mode *mode)
> > > >  {
>
> --
> Regards,
>
> Laurent Pinchart

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ