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: <CAPY8ntCj=u4ZQJwjhvZF30x08Cf0h7R5yQTim7QCKd8bi_M08w@mail.gmail.com>
Date: Mon, 2 Sep 2024 19:00:12 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: bbara93@...il.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 Benjamin

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

  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
>
>