Re: [PATCH 14/21] media: atomisp: Refactor atomisp_try_fmt() / atomisp_set_fmt()

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	 Andy Shevchenko <andy@kernel.org>, Kate Hsuan <hpa@redhat.com>,
	Tsuchiya Yuto <kitakar@gmail.com>,
	 Yury Luneff <yury.lunev@gmail.com>,
	Nable <nable.maininbox@googlemail.com>,
	 andrey.i.trufanov@gmail.com,
	Fabio Aiuto <fabioaiuto83@gmail.com>,
	 linux-media@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH 14/21] media: atomisp: Refactor atomisp_try_fmt() / atomisp_set_fmt()
Date: Tue, 30 May 2023 00:05:41 +0300	[thread overview]
Message-ID: <CAHp75VeCfTjw_Lu5SUx6u-O7QtFe2QXvCy2wZLSajJi6WDFiwQ@mail.gmail.com> (raw)
In-Reply-To: <20230529103741.11904-15-hdegoede@redhat.com>

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> There are a number of bugs in atomisp_try_fmt_cap() and atomisp_set_fmt():
>
> 1. atomisp_try_fmt_cap() uses atomisp_adjust_fmt() which adds the sensor
>    padding to the width passed to atomisp_adjust_fmt() to calculate
>    bytesperline. This is buggy for 2 reasons:
>
>    a) The width passed to atomisp_adjust_fmt() already contains
>       the sensor padding.
>
>    b) The fmt returned by atomisp_try_fmt_cap() is the fmt outputted by
>       the ISP and the sensor padding applies to the input side of the ISP
>       not the output side. The output side of the ISP has its own padding /
>       pitch requirements which have nothing to do with the sensor.
>
>    Both these issues are fixed in this refactor by switching to
>    ia_css_frame_pad_width() to calculate the padding.
>
> 2. atomisp_set_fmt() takes the passed in bytesperline value without
>    doing any validation on it and then passes this unchecked value to
>    the configure_output() callback.
>
>    If bytesperline converted to pixels is > 1920 ia_css_binary_find()
>    will fail to find a valid binary for the preview pipeline triggering
>    a dump_stack_lvl() call inside ia_css_binary_find() and causing
>    atomisp_set_fmt() to fail.
>
>    This is fixed by making atomisp_set_fmt() call atomisp_try_fmt()
>    first which we override the userspace specified bytesperline with
>    the correct value.
>
> Besides this bug there is also a bunch of weirdness and a lot of
> duplication in the code:
>
> 1. atomisp_try_fmt_cap() adds the sensor padding itself but then
>    it gets substracted again in atomisp_adjust_fmt() not doing
>    the addition + substraction in the same place makes the code hard
>    to follow (weirdness).
>
> 2. atomisp_set_fmt() starts with basically an atomisp_try_fmt() call,
>    except that the only atomisp_try_fmt() caller: atomisp_try_fmt_cap()
>    adds the sensor padding itself rather then letting atomisp_try_fmt()
>    do this (duplication).
>
> 3. Both atomisp_try_fmt_cap() and atomisp_set_fmt() contain code to
>    lookup the bridge-format matching the requested pixelformat and
>    both will fallback to YUV420 if this is not set (duplication).
>
> 4. Both atomisp_try_fmt_cap() and atomisp_set_fmt() contain code to
>    fill in the passed in v4l2_pix_format struct (duplication).
>
> Cleanup all of this (and fix the bugs mentioned above) by:
>
> 1. Adding a new atomisp_fill_pix_format() helper which properly uses
>    ia_css_frame_pad_width() to calculate bytesperline.
>
> 2. Move all sensor padding handling to atomisp_try_fmt() and
>    make atomisp_try_fmt() fill the passed in v4l2_pix_format struct.
>
> 3. This reduces atomisp_try_fmt_cap() to just a small wrapper around
>    atomisp_try_fmt().
>
> 4. Replace the DIY try_fmt code at the beginning of atomisp_set_fmt()
>    with atomisp_try_fmt(), this will also override/fix the bytersperline
>    passed by userspace.
>
> 5. Replace the DIY v4l2_pix_format fillint at the end of atomisp_set_fmt()

filling ?

>    with atomisp_fill_pix_format().

...

> +static void atomisp_fill_pix_format(struct v4l2_pix_format *f,
> +                                   u32 width, u32 height,
> +                                   const struct atomisp_format_bridge *br_fmt)
>  {

  size_t bytes; // unsigned int / u32 whatever name

> +       f->width = width;
> +       f->height = height;
> +       f->pixelformat = br_fmt->pixelformat;
> +
> +       /* Adding padding to width for bytesperline calculation */
> +       width = ia_css_frame_pad_width(width, br_fmt->sh_fmt);

  bytes = DIV_ROUND_UP(br_fmt->depth * width, BITS_PER_BYTE);

> +       if (br_fmt->planar) {
> +               f->bytesperline = width;
> +               f->sizeimage = PAGE_ALIGN(height * DIV_ROUND_UP(br_fmt->depth * width, 8));

  ... = PAGE_ALIGN(height * bytes);

> +       } else {
> +               f->bytesperline = DIV_ROUND_UP(br_fmt->depth * width, 8);

  ... bytesperline = bytes;

> +               f->sizeimage = PAGE_ALIGN(height * f->bytesperline);
> +       }

...

> +       /*
> +        * FIXME: do we need to setup this differently, depending on the

to set this up

> +        * sensor or the pipeline?
> +        */

-- 
With Best Regards,
Andy Shevchenko