media: atomisp: Refactor atomisp_try_fmt() / atomisp_set_fmt()

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 than 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 filling at the end of atomisp_set_fmt()
   with atomisp_fill_pix_format().

Link: https://lore.kernel.org/r/20230529103741.11904-15-hdegoede@redhat.com

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>

1 files changed, 2 insertions, 65 deletions

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 384d3bf2b61e..446297fef861 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -806,77 +806,14 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 	return -EINVAL;
 }
 
-static int atomisp_adjust_fmt(struct v4l2_format *f)
-{
-	const struct atomisp_format_bridge *format_bridge;
-	u32 padded_width;
-
-	format_bridge = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
-	/* Currently, raw formats are broken!!! */
-	if (!format_bridge || format_bridge->sh_fmt == IA_CSS_FRAME_FORMAT_RAW) {
-		f->fmt.pix.pixelformat = V4L2_PIX_FMT_YUV420;
-
-		format_bridge = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
-		if (!format_bridge)
-			return -EINVAL;
-	}
-
-	padded_width = f->fmt.pix.width + pad_w;
-
-	if (format_bridge->planar) {
-		f->fmt.pix.bytesperline = padded_width;
-		f->fmt.pix.sizeimage = PAGE_ALIGN(f->fmt.pix.height *
-						  DIV_ROUND_UP(format_bridge->depth *
-						  padded_width, 8));
-	} else {
-		f->fmt.pix.bytesperline = DIV_ROUND_UP(format_bridge->depth *
-						      padded_width, 8);
-		f->fmt.pix.sizeimage = PAGE_ALIGN(f->fmt.pix.height * f->fmt.pix.bytesperline);
-	}
-
-	if (f->fmt.pix.field == V4L2_FIELD_ANY)
-		f->fmt.pix.field = V4L2_FIELD_NONE;
-
-	/*
-	 * FIXME: do we need to setup this differently, depending on the
-	 * sensor or the pipeline?
-	 */
-	f->fmt.pix.colorspace = V4L2_COLORSPACE_REC709;
-	f->fmt.pix.ycbcr_enc = V4L2_YCBCR_ENC_709;
-	f->fmt.pix.xfer_func = V4L2_XFER_FUNC_709;
-
-	f->fmt.pix.width -= pad_w;
-	f->fmt.pix.height -= pad_h;
-
-	return 0;
-}
-
 /* This function looks up the closest available resolution. */
 static int atomisp_try_fmt_cap(struct file *file, void *fh,
 			       struct v4l2_format *f)
 {
 	struct video_device *vdev = video_devdata(file);
-	u32 pixfmt = f->fmt.pix.pixelformat;
-	int ret;
-
-	/*
-	 * atomisp_try_fmt() gived results with padding included, note
-	 * (this gets removed again by the atomisp_adjust_fmt() call below.
-	 */
-	f->fmt.pix.width += pad_w;
-	f->fmt.pix.height += pad_h;
-
-	ret = atomisp_try_fmt(vdev, &f->fmt.pix);
-	if (ret)
-		return ret;
-
-	/*
-	 * atomisp_try_fmt() replaces pixelformat with the sensors native
-	 * format, restore the actual format requested by userspace.
-	 */
-	f->fmt.pix.pixelformat = pixfmt;
+	struct atomisp_device *isp = video_get_drvdata(vdev);
 
-	return atomisp_adjust_fmt(f);
+	return atomisp_try_fmt(isp, &f->fmt.pix, NULL, NULL);
 }
 
 static int atomisp_g_fmt_cap(struct file *file, void *fh,