linux-kernel - Re: [PATCH v3 2/7] media: i2c: imx290: Define absolute control ranges

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: <CAJpcXm5cLjvqkuCB25strgYaUo4p058yLAXg8+LZ_7T12+3-ug@mail.gmail.com>
Date: Mon, 2 Sep 2024 21:43:36 +0200
From: Benjamin Bara <bbara93@...il.com>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
Cc: 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>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.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 Dave!

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
> > +#define MINIMUM_WIDTH  WIDTH_720P
> > +#define MINIMUM_HEIGHT HEIGHT_720P
> >  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)
> > +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)
> >  {
> >
> > --
> > 2.46.0
> >
> >