On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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
