[PATCH 00/21] media: atomisp: Use selection API info to determine sensor padding

[PATCH 00/21] media: atomisp: Use selection API info to determine sensor padding

[Hans de Goede]

Hi All,

Here is my next round of atomisp work.

The atomisp wants some extra padding for processing in the data it receives
from the sensor. E.g. For 1600x1200 it wants to receive 1616x1216 from
the sensor. Currently the private sensor driver copies it uses give it
e.g. 1616x1216 and the ISP2 code then substracts 16 before reporting
the resolution to userspace.

This patch series adds support for the v4l2 selections API and specifically
the crop target so that atomisp can request the extra padding from standard
v4l2 sensor drivers. This is implemented / tested with the atomisp_ov2680
driver.

Besides that there is the usual cleanups / prep work.

With the padding solved, the last bit of private atomisp sensor API is
gone now. So we can start working on getting rid of its private sensor
driver copies.

As mentioned in the updated TODO file the next step is to port
various improvements from the atomisp_ov2680 private sensor driver
to the generic ov2680 sensor driver (such as the selections support)
and then switch to the generic ov2680 sensor driver.

Regards,

Hans


Hans de Goede (21):
  media: atomisp: Update TODO
  media: atomisp: ov2680: s/ov2680_device/ov2680_dev/
  media: atomisp: ov2680: s/input_lock/lock/
  media: atomisp: ov2680: Add missing ov2680_calc_mode() call to probe()
  media: atomisp: ov2680: Add init_cfg pad-op
  media: atomisp: ov2680: Implement selection support
  media: atomisp: Remove a bunch of sensor related custom IOCTLs
  media: atomisp: Remove redundant atomisp_subdev_set_selection() calls
    from atomisp_set_fmt()
  media: atomisp: Simplify atomisp_subdev_set_selection() calls in
    atomisp_set_fmt()
  media: atomisp: Add target validation to
    atomisp_subdev_set_selection()
  media: atomisp: Remove bogus fh use from atomisp_set_fmt*()
  media: atomisp: Add input helper variable for
    isp->asd->inputs[asd->input_curr]
  media: atomisp: Add ia_css_frame_pad_width() helper function
  media: atomisp: Refactor atomisp_try_fmt() / atomisp_set_fmt()
  media: atomisp: Add support for sensors which implement selection API
    / cropping
  media: atomisp: Pass MEDIA_BUS_FMT_* code when calling enum_frame_size
    pad-op
  media: atomisp: Make atomisp_init_sensor() check if the sensor
    supports binning
  media: atomisp: Use selection API info to determine sensor padding
  media: atomisp: Set crop before setting fmt
  media: atomisp: Add enum_framesizes function for sensors with
    selection / crop support
  media: atomisp: csi2-bridge: Set PMC clk-rate for sensors to 19.2 MHz

 drivers/staging/media/atomisp/TODO            | 233 ++-----
 .../media/atomisp/i2c/atomisp-ov2680.c        | 204 +++++-
 drivers/staging/media/atomisp/i2c/ov2680.h    |  20 +-
 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c |  40 --
 .../media/atomisp/include/linux/atomisp.h     | 120 ----
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 612 ++++++------------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |  14 +-
 .../atomisp/pci/atomisp_compat_ioctl32.h      |  55 --
 .../media/atomisp/pci/atomisp_csi2_bridge.c   |  68 +-
 .../media/atomisp/pci/atomisp_internal.h      |   9 +
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 174 +++--
 .../media/atomisp/pci/atomisp_subdev.c        |  18 +-
 .../media/atomisp/pci/atomisp_subdev.h        |   3 +
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  85 +++
 .../runtime/frame/interface/ia_css_frame.h    |   2 +
 .../atomisp/pci/runtime/frame/src/frame.c     |  44 +-
 16 files changed, 685 insertions(+), 1016 deletions(-)

-- 
2.40.1

[PATCH 01/21] media: atomisp: Update TODO

[Hans de Goede]

A lot of work has been done on the atomisp driver lately.

Rewrite the TODO file to drop all the already fixed items:

* Moved to videobuf2 + fixed mmap support
* Whole bunch of v4l2 API fixes making more apps work
* v4l2-async sensor probing support
* pm-runtime support (for some sensor drivers at least)
* buffer MM code was cleaned up / replaced when moving the videobuf2

And add a new TODO list (retaining some of the old items) split
into items which absolutely must be fixed before the driver can
be moved out of staging:

1. Conflicting hw-ids with regular sensor drivers
2. Private userspace API stuff

As well as a list of items which also definitely needs to be fixed
but which could also be fixed after moving the driver out of staging.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/TODO | 236 +++++++----------------------
 1 file changed, 51 insertions(+), 185 deletions(-)

diff --git a/drivers/staging/media/atomisp/TODO b/drivers/staging/media/atomisp/TODO
index 43b842043f29..5e7bb6eb351a 100644
--- a/drivers/staging/media/atomisp/TODO
+++ b/drivers/staging/media/atomisp/TODO
@@ -1,213 +1,79 @@
-For both Cherrytrail (CHT) and Baytrail (BHT) the driver
-requires the "candrpv_0415_20150521_0458" firmware version.
-It should be noticed that the firmware file is different,
-depending on the ISP model, so they're stored with different
-names:
+Required firmware
+=================
 
-- for BHT: /lib/firmware/shisp_2400b0_v21.bin
+The atomisp driver requires the following firmware:
 
-  Warning: The driver was not tested yet for BHT.
+- for BYT: /lib/firmware/shisp_2400b0_v21.bin
+
+  With a version of "irci_stable_candrpv_0415_20150423_1753" to check
+  the version run: "strings shisp_2400b0_v21.bin | head -n1" .
+
+  The shisp_2400b0_v21.bin file with this version can be found on
+  the Android factory images of various X86 Android tablets such as
+  e.g. the Chuwi Hi8 Pro.
 
 - for CHT: /lib/firmware/shisp_2401a0_v21.bin
 
+  With a version of "irci_stable_candrpv_0415_20150521_0458"
+
+  This can be found here:
   https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin
 
-NOTE:
-=====
-
-This driver currently doesn't work with most V4L2 applications,
-as there are still some issues with regards to implementing
-certain APIs at the standard way.
-
-Also, currently only USERPTR streaming mode is working.
-
-In order to test, it is needed to know what's the sensor's
-resolution. This can be checked with:
-
-$ v4l2-ctl --get-fmt-video
-  Format Video Capture:
-	Width/Height      : 1600/1200
-	...
-
-It is known to work with:
-
-- v4l2grab at contrib/test directory at https://git.linuxtv.org/v4l-utils.git/
-
-  The resolution should not be bigger than the max resolution
-  supported by the sensor, or it will fail. So, if the sensor
-  reports:
-
-  The driver can be tested with:
-
-  v4l2grab -f YUYV -x 1600 -y 1200 -d /dev/video2 -u
-
-- NVT at https://github.com/intel/nvt
-
-  $ ./v4l2n -o testimage_@.raw \
-		 --device /dev/video2 \
-		 --input 0 \
-		 --exposure=30000,30000,30000,30000 \
-		 --parm type=1,capturemode=CI_MODE_PREVIEW \
-		 --fmt type=1,width=1600,height=1200,pixelformat=YUYV \
-		 --reqbufs count=2,memory=USERPTR \
-		 --parameters=wb_config.r=32768,wb_config.gr=21043,wb_config.gb=21043,wb_config.b=30863 \
-		 --capture=20
-
-  As the output is in raw format, images need to be converted with:
-
-  $ for i in $(seq 0 19); do
-	name="testimage_$(printf "%03i" $i)"
-	./raw2pnm -x$WIDTH -y$HEIGHT -f$FORMAT $name.raw $name.pnm
-	rm $name.raw
-    done
 
 TODO
 ====
 
-1.  Fix support for MMAP streaming mode. This is required for most
-    V4L2 applications;
+1. items which MUST be fixed before the driver can be moved out of staging:
 
-2.  Implement and/or fix V4L2 ioctls in order to allow a normal app to
-    use it;
+* The atomisp ov2680 and ov5693 sensor drivers bind to the same hw-ids as
+  the standard ov2680 and ov5693 drivers under drivers/media/i2c, which
+  conflicts. Drop the atomisp private ov2680 and ov5693 drivers:
+  * Make atomisp code use v4l2 selections to deal with the extra padding
+    it wants to receive from sensors instead of relying on the ov2680 code
+    sending e.g. 1616x1216 for a 1600x1200 mode
+  * Port various ov2680 improvements from atomisp_ov2680.c to regular ov2680.c
+    and switch to regular ov2680 driver
+  * Make atomisp work with the regular ov5693 driver and drop atomisp_ov5693
 
-3.  Ensure that the driver will pass v4l2-compliance tests;
+* Fix atomisp causing the whole machine to hang in its probe() error-exit
+  path taken in the firmware missing case
 
-4.  Get manufacturer's authorization to redistribute the binaries for
-    the firmware files;
+* Remove/disable private ioctls
 
-5.  remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
-    register address differences between ISP2400 and ISP2401;
+* Remove/disable custom v4l2-ctrls
 
-6.  Cleanup the driver code, removing the abstraction layers inside it;
+* Remove custom sysfs files created by atomisp_drvfs.c
 
-7.  The atomisp doesn't rely at the usual i2c stuff to discover the
-    sensors. Instead, it calls a function from atomisp_gmin_platform.c.
-    There are some hacks added there for it to wait for sensors to be
-    probed (with a timeout of 2 seconds or so). This should be converted
-    to the usual way, using V4L2 async subdev framework to wait for
-    cameras to be probed;
+* Remove abuse of priv field in various v4l2 userspace API structs
 
-8.  Switch to standard V4L2 sub-device API for sensor and lens. In
-    particular, the user space API needs to support V4L2 controls as
-    defined in the V4L2 spec and references to atomisp must be removed from
-    these drivers.
+* Without a 3A library the capture behaviour is not very good. To take a good
+  picture, the exposure/gain needs to be tuned using v4l2-ctl on the sensor
+  subdev. To fix this support for the atomisp needs to be added to libcamera.
 
-9.  Use LED flash API for flash LED drivers such as LM3554 (which already
-    has a LED class driver).
-
-10. Migrate the sensor drivers out of staging or re-using existing
-    drivers;
-
-11. Switch the driver to use pm_runtime stuff. Right now, it probes the
-    existing PMIC code and sensors call it directly.
-
-12. There's a problem on sensor drivers: when trying to set a video
-    format, the atomisp main driver calls the sensor drivers with the
-    sensor turned off. This causes them to fail.
-
-    This was fixed at atomisp-ov2880, which has a hack inside it
-    to turn it on when VIDIOC_S_FMT is called, but this has to be
-    cheked on other drivers as well.
-
-    The right fix seems to power on the sensor when a video device is
-    opened (or at the first VIDIOC_ ioctl - except for VIDIOC_QUERYCAP),
-    powering it down at close() syscall.
-
-    Such kind of control would need to be done inside the atomisp driver,
-    not at the sensors code.
-
-13. There are several issues related to memory management, that can
-    cause crashes and/or memory leaks. The atomisp splits the memory
-    management on three separate regions:
-
-	- dynamic pool;
-	- reserved pool;
-	- generic pool
-
-    The code implementing it is at:
-
-	drivers/staging/media/atomisp/pci/hmm/
-
-    It also has a separate code for managing DMA buffers at:
-
-	drivers/staging/media/atomisp/pci/mmu/
-
-    The code there is really dirty, ugly and probably wrong. I fixed
-    one bug there already, but the best would be to just trash it and use
-    something else. Maybe the code from the newer intel driver could
-    serve as a model:
-
-	drivers/staging/media/ipu3/ipu3-mmu.c
-
-    But converting it to use something like that is painful and may
-    cause some breakages.
-
-14. The file structure needs to get tidied up to resemble a normal Linux
-    driver.
-
-15. Lots of the midlayer glue. Unused code and abstraction needs removing.
-
-16. The AtomISP driver includes some special IOCTLS (ATOMISP_IOC_XXXX_XXXX)
-    and controls that require some cleanup. Some of those code may have
-    been removed during the cleanups. They could be needed in order to
-    properly support 3A algorithms.
-
-    Such IOCTL interface needs more documentation. The better would
-    be to use something close to the interface used by the IPU3 IMGU driver.
-
-17. The ISP code has some dependencies of the exact FW version.
-    The version defined in pci/sh_css_firmware.c:
-
-    BYT (isp2400): "irci_stable_candrpv_0415_20150521_0458"
-
-    CHT (isp2401): "irci_ecr - master_20150911_0724"
-
-    Those versions don't seem to be available anymore. On the tests we've
-    done so far, this version also seems to work for CHT:
-
-		"irci_stable_candrpv_0415_20150521_0458"
-
-    Which can be obtainable from Yocto Atom ISP respository.
-
-    but this was not thoroughly tested.
-
-    At some point we may need to round up a few driver versions and see if
-    there are any specific things that can be done to fold in support for
-    multiple firmware versions.
+  This MUST be done before moving the driver out of staging so that we can
+  still make changes to e.g. the mediactl topology if necessary for
+  libcamera integration. Since this would be a userspace API break this
+  means that at least proof-of-concept libcamera integration needs to be
+  ready before moving the driver out of staging.
 
 
-18. Switch from videobuf1 to videobuf2. Videobuf1 is being removed!
+2. items which SHOULD also be fixed eventually:
 
-19. Correct Coding Style. Please refrain sending coding style patches
-    for this driver until the other work is done, as there will be a lot
-    of code churn until this driver becomes functional again.
+* Remove VIDEO_ATOMISP_ISP2401, making the driver to auto-detect the
+  register address differences between ISP2400 and ISP2401
 
-20. Remove the logic which sets up pipelines inside it, moving it to
-    libcamera and implement MC support.
+* The driver is intended to drive the PCI exposed versions of the device.
+  It will not detect those devices enumerated via ACPI as a field of the
+  i915 GPU driver (only a problem on BYT).
 
+  There are some patches adding i915 GPU support floating at the Yocto's
+  Aero repository (so far, untested upstream).
 
-Limitations
-===========
+* Ensure that the driver will pass v4l2-compliance tests
 
-1. To test the patches, you also need the ISP firmware
+* Fix not all v4l2 apps working, e.g. cheese does not work
 
-   for BYT: /lib/firmware/shisp_2400b0_v21.bin
-   for CHT: /lib/firmware/shisp_2401a0_v21.bin
+* Get manufacturer's authorization to redistribute the binaries for
+  the firmware files
 
-   The firmware files will usually be found in /etc/firmware on an Android
-   device but can also be extracted from the upgrade kit if you've managed
-   to lose them somehow.
-
-2. Without a 3A library the capture behaviour is not very good. To take a good
-   picture, you need tune ISP parameters by IOCTL functions or use a 3A library
-   such as libxcam.
-
-3. The driver is intended to drive the PCI exposed versions of the device.
-   It will not detect those devices enumerated via ACPI as a field of the
-   i915 GPU driver.
-
-   There are some patches adding i915 GPU support floating at the Yocto's
-   Aero repository (so far, untested upstream).
-
-4. The driver supports only v2 of the IPU/Camera. It will not work with the
-   versions of the hardware in other SoCs.
+* The atomisp code still has a lot of cruft which needs cleaning up
-- 
2.40.1

Re: [PATCH 01/21] media: atomisp: Update TODO

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> A lot of work has been done on the atomisp driver lately.
>
> Rewrite the TODO file to drop all the already fixed items:
>
> * Moved to videobuf2 + fixed mmap support
> * Whole bunch of v4l2 API fixes making more apps work
> * v4l2-async sensor probing support
> * pm-runtime support (for some sensor drivers at least)
> * buffer MM code was cleaned up / replaced when moving the videobuf2
>
> And add a new TODO list (retaining some of the old items) split
> into items which absolutely must be fixed before the driver can
> be moved out of staging:
>
> 1. Conflicting hw-ids with regular sensor drivers
> 2. Private userspace API stuff
>
> As well as a list of items which also definitely needs to be fixed
> but which could also be fixed after moving the driver out of staging.

...

> +Required firmware
> +=================
>
> +The atomisp driver requires the following firmware:
>
> +- for BYT: /lib/firmware/shisp_2400b0_v21.bin
> +
> +  With a version of "irci_stable_candrpv_0415_20150423_1753" to check
> +  the version run: "strings shisp_2400b0_v21.bin | head -n1" .
> +
> +  The shisp_2400b0_v21.bin file with this version can be found on
> +  the Android factory images of various X86 Android tablets such as
> +  e.g. the Chuwi Hi8 Pro.
>
>  - for CHT: /lib/firmware/shisp_2401a0_v21.bin
>
> +  With a version of "irci_stable_candrpv_0415_20150521_0458"
> +
> +  This can be found here:
>    https://github.com/intel-aero/meta-intel-aero-base/blob/master/recipes-kernel/linux/linux-yocto/shisp_2401a0_v21.bin

Can you add md5/SHA-1/SHA-256 sum(s) for the firmwares?

...

> +1. items which MUST be fixed before the driver can be moved out of staging:

Items

...

> +* Remove/disable private ioctls

IOCTLs

...

> +* Without a 3A library the capture behaviour is not very good. To take a good
> +  picture, the exposure/gain needs to be tuned using v4l2-ctl on the sensor
> +  subdev. To fix this support for the atomisp needs to be added to libcamera.

this, support

...

> +  This MUST be done before moving the driver out of staging so that we can
> +  still make changes to e.g. the mediactl topology if necessary for
> +  libcamera integration. Since this would be a userspace API break this

break, this

> +  means that at least proof-of-concept libcamera integration needs to be
> +  ready before moving the driver out of staging.

...

> +2. items which SHOULD also be fixed eventually:

Items

-- 
With Best Regards,
Andy Shevchenko

[PATCH 02/21] media: atomisp: ov2680: s/ov2680_device/ov2680_dev/

[Hans de Goede]

s/ov2680_device/ov2680_dev/ ov2680_dev is used by the generic
drivers/media/i2c/ov2680.c driver. Bring the atomisp ov2680 code
inline to make it easier to port changes between the two,
with the end goal of getting rid of the atomisp specific version.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2680.c        | 38 +++++++++----------
 drivers/staging/media/atomisp/i2c/ov2680.h    |  8 ++--
 2 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index b35ddf611e2b..cd6557c9a4c9 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -45,7 +45,7 @@ static int ov2680_write_reg_array(struct i2c_client *client,
 	return 0;
 }
 
-static void ov2680_set_bayer_order(struct ov2680_device *sensor, struct v4l2_mbus_framefmt *fmt)
+static void ov2680_set_bayer_order(struct ov2680_dev *sensor, struct v4l2_mbus_framefmt *fmt)
 {
 	static const int ov2680_hv_flip_bayer_order[] = {
 		MEDIA_BUS_FMT_SBGGR10_1X10,
@@ -64,7 +64,7 @@ static void ov2680_set_bayer_order(struct ov2680_device *sensor, struct v4l2_mbu
 	fmt->code = ov2680_hv_flip_bayer_order[hv_flip];
 }
 
-static int ov2680_set_vflip(struct ov2680_device *sensor, s32 val)
+static int ov2680_set_vflip(struct ov2680_dev *sensor, s32 val)
 {
 	int ret;
 
@@ -79,7 +79,7 @@ static int ov2680_set_vflip(struct ov2680_device *sensor, s32 val)
 	return 0;
 }
 
-static int ov2680_set_hflip(struct ov2680_device *sensor, s32 val)
+static int ov2680_set_hflip(struct ov2680_dev *sensor, s32 val)
 {
 	int ret;
 
@@ -94,17 +94,17 @@ static int ov2680_set_hflip(struct ov2680_device *sensor, s32 val)
 	return 0;
 }
 
-static int ov2680_exposure_set(struct ov2680_device *sensor, u32 exp)
+static int ov2680_exposure_set(struct ov2680_dev *sensor, u32 exp)
 {
 	return ov_write_reg24(sensor->client, OV2680_REG_EXPOSURE_PK_HIGH, exp << 4);
 }
 
-static int ov2680_gain_set(struct ov2680_device *sensor, u32 gain)
+static int ov2680_gain_set(struct ov2680_dev *sensor, u32 gain)
 {
 	return ov_write_reg16(sensor->client, OV2680_REG_GAIN_PK, gain);
 }
 
-static int ov2680_test_pattern_set(struct ov2680_device *sensor, int value)
+static int ov2680_test_pattern_set(struct ov2680_dev *sensor, int value)
 {
 	int ret;
 
@@ -125,7 +125,7 @@ static int ov2680_test_pattern_set(struct ov2680_device *sensor, int value)
 static int ov2680_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 	int ret;
 
 	/* Only apply changes to the controls if the device is powered up */
@@ -174,7 +174,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
 }
 
 static struct v4l2_mbus_framefmt *
-__ov2680_get_pad_format(struct ov2680_device *sensor,
+__ov2680_get_pad_format(struct ov2680_dev *sensor,
 			struct v4l2_subdev_state *state,
 			unsigned int pad, enum v4l2_subdev_format_whence which)
 {
@@ -184,7 +184,7 @@ __ov2680_get_pad_format(struct ov2680_device *sensor,
 	return &sensor->mode.fmt;
 }
 
-static void ov2680_fill_format(struct ov2680_device *sensor,
+static void ov2680_fill_format(struct ov2680_dev *sensor,
 			       struct v4l2_mbus_framefmt *fmt,
 			       unsigned int width, unsigned int height)
 {
@@ -195,7 +195,7 @@ static void ov2680_fill_format(struct ov2680_device *sensor,
 	ov2680_set_bayer_order(sensor, fmt);
 }
 
-static void ov2680_calc_mode(struct ov2680_device *sensor, int width, int height)
+static void ov2680_calc_mode(struct ov2680_dev *sensor, int width, int height)
 {
 	int orig_width = width;
 	int orig_height = height;
@@ -221,7 +221,7 @@ static void ov2680_calc_mode(struct ov2680_device *sensor, int width, int height
 	sensor->mode.vts = OV2680_LINES_PER_FRAME;
 }
 
-static int ov2680_set_mode(struct ov2680_device *sensor)
+static int ov2680_set_mode(struct ov2680_dev *sensor)
 {
 	struct i2c_client *client = sensor->client;
 	u8 pll_div, unknown, inc, fmt1, fmt2;
@@ -322,7 +322,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 	struct v4l2_mbus_framefmt *fmt;
 	unsigned int width, height;
 
@@ -347,7 +347,7 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 	struct v4l2_mbus_framefmt *fmt;
 
 	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad, format->which);
@@ -390,7 +390,7 @@ static int ov2680_detect(struct i2c_client *client)
 
 static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 {
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
@@ -548,7 +548,7 @@ static const struct v4l2_subdev_ops ov2680_ops = {
 	.sensor = &ov2680_sensor_ops,
 };
 
-static int ov2680_init_controls(struct ov2680_device *sensor)
+static int ov2680_init_controls(struct ov2680_dev *sensor)
 {
 	static const char * const test_pattern_menu[] = {
 		"Disabled",
@@ -590,7 +590,7 @@ static int ov2680_init_controls(struct ov2680_device *sensor)
 static void ov2680_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 
 	dev_dbg(&client->dev, "ov2680_remove...\n");
 
@@ -605,7 +605,7 @@ static void ov2680_remove(struct i2c_client *client)
 static int ov2680_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
-	struct ov2680_device *sensor;
+	struct ov2680_dev *sensor;
 	int ret;
 
 	sensor = devm_kzalloc(dev, sizeof(*sensor), GFP_KERNEL);
@@ -673,7 +673,7 @@ static int ov2680_probe(struct i2c_client *client)
 static int ov2680_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 
 	gpiod_set_value_cansleep(sensor->powerdown, 1);
 	return 0;
@@ -682,7 +682,7 @@ static int ov2680_suspend(struct device *dev)
 static int ov2680_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct ov2680_device *sensor = to_ov2680_sensor(sd);
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 
 	/* according to DS, at least 5ms is needed after DOVDD (enabled by ACPI) */
 	usleep_range(5000, 6000);
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index a3eeb0c2de5c..077ca6ee08b6 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -106,7 +106,7 @@
 /*
  * ov2680 device structure.
  */
-struct ov2680_device {
+struct ov2680_dev {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct mutex input_lock;
@@ -151,12 +151,12 @@ struct ov2680_reg {
 	u32 val;	/* @set value for read/mod/write, @mask */
 };
 
-#define to_ov2680_sensor(x) container_of(x, struct ov2680_device, sd)
+#define to_ov2680_sensor(x) container_of(x, struct ov2680_dev, sd)
 
 static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 {
-	struct ov2680_device *sensor =
-		container_of(ctrl->handler, struct ov2680_device, ctrls.handler);
+	struct ov2680_dev *sensor =
+		container_of(ctrl->handler, struct ov2680_dev, ctrls.handler);
 
 	return &sensor->sd;
 }
-- 
2.40.1

[PATCH 03/21] media: atomisp: ov2680: s/input_lock/lock/

[Hans de Goede]

s/input_lock/lock/ lock is used by the generic drivers/media/i2c/ov2680.c
driver. Bring the atomisp ov2680 code inline to make it easier to port
changes between the two, with the end goal of getting rid of
the atomisp specific version.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/i2c/atomisp-ov2680.c   | 16 ++++++++--------
 drivers/staging/media/atomisp/i2c/ov2680.h       |  3 ++-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index cd6557c9a4c9..8bcfa5ae2ea0 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -337,9 +337,9 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY)
 		return 0;
 
-	mutex_lock(&sensor->input_lock);
+	mutex_lock(&sensor->lock);
 	ov2680_calc_mode(sensor, fmt->width, fmt->height);
-	mutex_unlock(&sensor->input_lock);
+	mutex_unlock(&sensor->lock);
 	return 0;
 }
 
@@ -394,7 +394,7 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
-	mutex_lock(&sensor->input_lock);
+	mutex_lock(&sensor->lock);
 
 	if (sensor->is_streaming == enable) {
 		dev_warn(&client->dev, "stream already %s\n", enable ? "started" : "stopped");
@@ -427,14 +427,14 @@ static int ov2680_s_stream(struct v4l2_subdev *sd, int enable)
 	v4l2_ctrl_activate(sensor->ctrls.vflip, !enable);
 	v4l2_ctrl_activate(sensor->ctrls.hflip, !enable);
 
-	mutex_unlock(&sensor->input_lock);
+	mutex_unlock(&sensor->lock);
 	return 0;
 
 error_power_down:
 	pm_runtime_put(sensor->sd.dev);
 	sensor->is_streaming = false;
 error_unlock:
-	mutex_unlock(&sensor->input_lock);
+	mutex_unlock(&sensor->lock);
 	return ret;
 }
 
@@ -564,7 +564,7 @@ static int ov2680_init_controls(struct ov2680_dev *sensor)
 
 	v4l2_ctrl_handler_init(hdl, 4);
 
-	hdl->lock = &sensor->input_lock;
+	hdl->lock = &sensor->lock;
 
 	ctrls->hflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HFLIP, 0, 1, 1, 0);
 	ctrls->vflip = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VFLIP, 0, 1, 1, 0);
@@ -597,7 +597,7 @@ static void ov2680_remove(struct i2c_client *client)
 	v4l2_async_unregister_subdev(&sensor->sd);
 	media_entity_cleanup(&sensor->sd.entity);
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
-	mutex_destroy(&sensor->input_lock);
+	mutex_destroy(&sensor->lock);
 	fwnode_handle_put(sensor->ep_fwnode);
 	pm_runtime_disable(&client->dev);
 }
@@ -612,7 +612,7 @@ static int ov2680_probe(struct i2c_client *client)
 	if (!sensor)
 		return -ENOMEM;
 
-	mutex_init(&sensor->input_lock);
+	mutex_init(&sensor->lock);
 
 	sensor->client = client;
 	v4l2_i2c_subdev_init(&sensor->sd, client, &ov2680_ops);
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 077ca6ee08b6..4bcdd9fd0ce7 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -109,7 +109,8 @@
 struct ov2680_dev {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
-	struct mutex input_lock;
+	/* Protect against concurrent changes to controls */
+	struct mutex lock;
 	struct i2c_client *client;
 	struct gpio_desc *powerdown;
 	struct fwnode_handle *ep_fwnode;
-- 
2.40.1

[PATCH 04/21] media: atomisp: ov2680: Add missing ov2680_calc_mode() call to probe()

[Hans de Goede]

Call ov2680_calc_mode() from probe() instead of relying on userspace
to make at least one s_fmt call to fill the mode parameters.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-ov2680.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 8bcfa5ae2ea0..6cbc470bce91 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -195,8 +195,10 @@ static void ov2680_fill_format(struct ov2680_dev *sensor,
 	ov2680_set_bayer_order(sensor, fmt);
 }
 
-static void ov2680_calc_mode(struct ov2680_dev *sensor, int width, int height)
+static void ov2680_calc_mode(struct ov2680_dev *sensor)
 {
+	int width = sensor->mode.fmt.width;
+	int height = sensor->mode.fmt.height;
 	int orig_width = width;
 	int orig_height = height;
 
@@ -338,7 +340,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		return 0;
 
 	mutex_lock(&sensor->lock);
-	ov2680_calc_mode(sensor, fmt->width, fmt->height);
+	ov2680_calc_mode(sensor);
 	mutex_unlock(&sensor->lock);
 	return 0;
 }
@@ -660,6 +662,7 @@ static int ov2680_probe(struct i2c_client *client)
 	}
 
 	ov2680_fill_format(sensor, &sensor->mode.fmt, OV2680_NATIVE_WIDTH, OV2680_NATIVE_HEIGHT);
+	ov2680_calc_mode(sensor);
 
 	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
 	if (ret) {
-- 
2.40.1

[PATCH 05/21] media: atomisp: ov2680: Add init_cfg pad-op

[Hans de Goede]

Having an init_cfg to initialize the passed in subdev-state is
important to make which == V4L2_SUBDEV_FORMAT_TRY ops work when
userspace is talking to a /dev/v4l2-subdev# node.

Copy the ov2680_init_cfg() from the standard drivers/media/i2c/ov2680.c
driver.

This is esp. relevant once support for cropping is added where
the v4l2_subdev_state.pads[pad].try_crop rectangle needs to be set
correctly for set_fmt which == V4L2_SUBDEV_FORMAT_TRY calls to work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/i2c/atomisp-ov2680.c   | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 6cbc470bce91..17fb773540e5 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -357,6 +357,21 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov2680_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *sd_state)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
+		: V4L2_SUBDEV_FORMAT_ACTIVE,
+		.format = {
+			.width = 800,
+			.height = 600,
+		}
+	};
+
+	return ov2680_set_fmt(sd, sd_state, &fmt);
+}
+
 static int ov2680_detect(struct i2c_client *client)
 {
 	struct i2c_adapter *adapter = client->adapter;
@@ -537,6 +552,7 @@ static const struct v4l2_subdev_sensor_ops ov2680_sensor_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
+	.init_cfg = ov2680_init_cfg,
 	.enum_mbus_code = ov2680_enum_mbus_code,
 	.enum_frame_size = ov2680_enum_frame_size,
 	.enum_frame_interval = ov2680_enum_frame_interval,
-- 
2.40.1

Re: [PATCH 05/21] media: atomisp: ov2680: Add init_cfg pad-op

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Having an init_cfg to initialize the passed in subdev-state is
> important to make which == V4L2_SUBDEV_FORMAT_TRY ops work when
> userspace is talking to a /dev/v4l2-subdev# node.
>
> Copy the ov2680_init_cfg() from the standard drivers/media/i2c/ov2680.c
> driver.
>
> This is esp. relevant once support for cropping is added where
> the v4l2_subdev_state.pads[pad].try_crop rectangle needs to be set
> correctly for set_fmt which == V4L2_SUBDEV_FORMAT_TRY calls to work.

...

> +static int ov2680_init_cfg(struct v4l2_subdev *sd,
> +                          struct v4l2_subdev_state *sd_state)
> +{
> +       struct v4l2_subdev_format fmt = {
> +               .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> +               : V4L2_SUBDEV_FORMAT_ACTIVE,
> +               .format = {
> +                       .width = 800,
> +                       .height = 600,

> +               }

I would keep a trailing comma here.

> +       };
> +
> +       return ov2680_set_fmt(sd, sd_state, &fmt);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 05/21] media: atomisp: ov2680: Add init_cfg pad-op

[Andy Shevchenko]

On Mon, May 29, 2023 at 9:13 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> > +static int ov2680_init_cfg(struct v4l2_subdev *sd,
> > +                          struct v4l2_subdev_state *sd_state)
> > +{
> > +       struct v4l2_subdev_format fmt = {
> > +               .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
> > +               : V4L2_SUBDEV_FORMAT_ACTIVE,
> > +               .format = {
> > +                       .width = 800,
> > +                       .height = 600,
>
> > +               }
>
> I would keep a trailing comma here.
>
> > +       };
> > +
> > +       return ov2680_set_fmt(sd, sd_state, &fmt);
> > +}

This is not addressed in your branch.


--
With Best Regards,
Andy Shevchenko

Re: [PATCH 05/21] media: atomisp: ov2680: Add init_cfg pad-op

[Hans de Goede]

Hi,

On 5/30/23 13:51, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 9:13 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>> +static int ov2680_init_cfg(struct v4l2_subdev *sd,
>>> +                          struct v4l2_subdev_state *sd_state)
>>> +{
>>> +       struct v4l2_subdev_format fmt = {
>>> +               .which = sd_state ? V4L2_SUBDEV_FORMAT_TRY
>>> +               : V4L2_SUBDEV_FORMAT_ACTIVE,
>>> +               .format = {
>>> +                       .width = 800,
>>> +                       .height = 600,
>>
>>> +               }
>>
>> I would keep a trailing comma here.
>>
>>> +       };
>>> +
>>> +       return ov2680_set_fmt(sd, sd_state, &fmt);
>>> +}
> 
> This is not addressed in your branch.

Oops, sorry I've fixed this in my branch now and I've also changed
6/21 to make __ov2680_get_pad_crop() look like this:

static struct v4l2_rect *
__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
                      unsigned int pad, enum v4l2_subdev_format_whence which)
{
        if (which == V4L2_SUBDEV_FORMAT_TRY)
                return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);

        return &sensor->mode.crop;
}


Regards,

Hans

[PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Hans de Goede]

Implement selection support. Modelled after ov5693 selection support,
but allow setting sizes smaller then crop-size through set_fmt since
that was already allowed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-ov2680.c        | 131 ++++++++++++++++--
 drivers/staging/media/atomisp/i2c/ov2680.h    |   9 ++
 2 files changed, 132 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 17fb773540e5..7fc0ccc66ab9 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -30,6 +30,13 @@
 
 #include "ov2680.h"
 
+static const struct v4l2_rect ov2680_default_crop = {
+	.left = OV2680_ACTIVE_START_LEFT,
+	.top = OV2680_ACTIVE_START_TOP,
+	.width = OV2680_ACTIVE_WIDTH,
+	.height = OV2680_ACTIVE_HEIGHT,
+};
+
 static int ov2680_write_reg_array(struct i2c_client *client,
 				  const struct ov2680_reg *reglist)
 {
@@ -174,8 +181,7 @@ static int ov2680_init_registers(struct v4l2_subdev *sd)
 }
 
 static struct v4l2_mbus_framefmt *
-__ov2680_get_pad_format(struct ov2680_dev *sensor,
-			struct v4l2_subdev_state *state,
+__ov2680_get_pad_format(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
 			unsigned int pad, enum v4l2_subdev_format_whence which)
 {
 	if (which == V4L2_SUBDEV_FORMAT_TRY)
@@ -184,6 +190,20 @@ __ov2680_get_pad_format(struct ov2680_dev *sensor,
 	return &sensor->mode.fmt;
 }
 
+static struct v4l2_rect *
+__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &sensor->mode.crop;
+	}
+
+	return NULL;
+}
+
 static void ov2680_fill_format(struct ov2680_dev *sensor,
 			       struct v4l2_mbus_framefmt *fmt,
 			       unsigned int width, unsigned int height)
@@ -202,8 +222,8 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 	int orig_width = width;
 	int orig_height = height;
 
-	if (width  <= (OV2680_NATIVE_WIDTH / 2) &&
-	    height <= (OV2680_NATIVE_HEIGHT / 2)) {
+	if (width  <= (sensor->mode.crop.width / 2) &&
+	    height <= (sensor->mode.crop.height / 2)) {
 		sensor->mode.binning = true;
 		width *= 2;
 		height *= 2;
@@ -211,8 +231,10 @@ static void ov2680_calc_mode(struct ov2680_dev *sensor)
 		sensor->mode.binning = false;
 	}
 
-	sensor->mode.h_start = ((OV2680_NATIVE_WIDTH - width) / 2) & ~1;
-	sensor->mode.v_start = ((OV2680_NATIVE_HEIGHT - height) / 2) & ~1;
+	sensor->mode.h_start =
+		(sensor->mode.crop.left + (sensor->mode.crop.width - width) / 2) & ~1;
+	sensor->mode.v_start =
+		(sensor->mode.crop.top + (sensor->mode.crop.height - height) / 2) & ~1;
 	sensor->mode.h_end = min(sensor->mode.h_start + width + OV2680_END_MARGIN - 1,
 				 OV2680_NATIVE_WIDTH - 1);
 	sensor->mode.v_end = min(sensor->mode.v_start + height + OV2680_END_MARGIN - 1,
@@ -326,10 +348,16 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 {
 	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
 	struct v4l2_mbus_framefmt *fmt;
+	const struct v4l2_rect *crop;
 	unsigned int width, height;
 
-	width = min_t(unsigned int, ALIGN(format->format.width, 2), OV2680_NATIVE_WIDTH);
-	height = min_t(unsigned int, ALIGN(format->format.height, 2), OV2680_NATIVE_HEIGHT);
+	crop = __ov2680_get_pad_crop(sensor, sd_state, format->pad, format->which);
+
+	/* Limit set_fmt max size to crop width / height */
+	width = clamp_t(unsigned int, ALIGN(format->format.width, 2),
+			OV2680_MIN_CROP_WIDTH, crop->width);
+	height = clamp_t(unsigned int, ALIGN(format->format.height, 2),
+			 OV2680_MIN_CROP_HEIGHT, crop->height);
 
 	fmt = __ov2680_get_pad_format(sensor, sd_state, format->pad, format->which);
 	ov2680_fill_format(sensor, fmt, width, height);
@@ -357,6 +385,88 @@ static int ov2680_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov2680_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
+
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+		mutex_lock(&sensor->lock);
+		sel->r = *__ov2680_get_pad_crop(sensor, state, sel->pad, sel->which);
+		mutex_unlock(&sensor->lock);
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV2680_NATIVE_WIDTH;
+		sel->r.height = OV2680_NATIVE_HEIGHT;
+		break;
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = OV2680_ACTIVE_START_TOP;
+		sel->r.left = OV2680_ACTIVE_START_LEFT;
+		sel->r.width = OV2680_ACTIVE_WIDTH;
+		sel->r.height = OV2680_ACTIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ov2680_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	struct ov2680_dev *sensor = to_ov2680_sensor(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *__crop;
+	struct v4l2_rect rect;
+
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	/*
+	 * Clamp the boundaries of the crop rectangle to the size of the sensor
+	 * pixel array. Align to multiples of 2 to ensure Bayer pattern isn't
+	 * disrupted.
+	 */
+	rect.left = clamp(ALIGN(sel->r.left, 2), OV2680_NATIVE_START_LEFT,
+			  OV2680_NATIVE_WIDTH);
+	rect.top = clamp(ALIGN(sel->r.top, 2), OV2680_NATIVE_START_TOP,
+			 OV2680_NATIVE_HEIGHT);
+	rect.width = clamp_t(unsigned int, ALIGN(sel->r.width, 2),
+			     OV2680_MIN_CROP_WIDTH, OV2680_NATIVE_WIDTH);
+	rect.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
+			      OV2680_MIN_CROP_HEIGHT, OV2680_NATIVE_HEIGHT);
+
+	/* Make sure the crop rectangle isn't outside the bounds of the array */
+	rect.width = min_t(unsigned int, rect.width,
+			   OV2680_NATIVE_WIDTH - rect.left);
+	rect.height = min_t(unsigned int, rect.height,
+			    OV2680_NATIVE_HEIGHT - rect.top);
+
+	__crop = __ov2680_get_pad_crop(sensor, state, sel->pad, sel->which);
+
+	if (rect.width != __crop->width || rect.height != __crop->height) {
+		/*
+		 * Reset the output image size if the crop rectangle size has
+		 * been modified.
+		 */
+		format = __ov2680_get_pad_format(sensor, state, sel->pad, sel->which);
+		format->width = rect.width;
+		format->height = rect.height;
+	}
+
+	*__crop = rect;
+	sel->r = rect;
+
+	return 0;
+}
+
 static int ov2680_init_cfg(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state)
 {
@@ -369,6 +479,8 @@ static int ov2680_init_cfg(struct v4l2_subdev *sd,
 		}
 	};
 
+	sd_state->pads[0].try_crop = ov2680_default_crop;
+
 	return ov2680_set_fmt(sd, sd_state, &fmt);
 }
 
@@ -558,6 +670,8 @@ static const struct v4l2_subdev_pad_ops ov2680_pad_ops = {
 	.enum_frame_interval = ov2680_enum_frame_interval,
 	.get_fmt = ov2680_get_fmt,
 	.set_fmt = ov2680_set_fmt,
+	.get_selection = ov2680_get_selection,
+	.set_selection = ov2680_set_selection,
 };
 
 static const struct v4l2_subdev_ops ov2680_ops = {
@@ -677,6 +791,7 @@ static int ov2680_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	sensor->mode.crop = ov2680_default_crop;
 	ov2680_fill_format(sensor, &sensor->mode.fmt, OV2680_NATIVE_WIDTH, OV2680_NATIVE_HEIGHT);
 	ov2680_calc_mode(sensor);
 
diff --git a/drivers/staging/media/atomisp/i2c/ov2680.h b/drivers/staging/media/atomisp/i2c/ov2680.h
index 4bcdd9fd0ce7..fd9c7485f8c1 100644
--- a/drivers/staging/media/atomisp/i2c/ov2680.h
+++ b/drivers/staging/media/atomisp/i2c/ov2680.h
@@ -32,6 +32,14 @@
 
 #define OV2680_NATIVE_WIDTH			1616
 #define OV2680_NATIVE_HEIGHT			1216
+#define OV2680_NATIVE_START_LEFT		0
+#define OV2680_NATIVE_START_TOP			0
+#define OV2680_ACTIVE_WIDTH			1600
+#define OV2680_ACTIVE_HEIGHT			1200
+#define OV2680_ACTIVE_START_LEFT		8
+#define OV2680_ACTIVE_START_TOP			8
+#define OV2680_MIN_CROP_WIDTH			2
+#define OV2680_MIN_CROP_HEIGHT			2
 
 /* 1704 * 1294 * 30fps = 66MHz pixel clock */
 #define OV2680_PIXELS_PER_LINE			1704
@@ -117,6 +125,7 @@ struct ov2680_dev {
 	bool is_streaming;
 
 	struct ov2680_mode {
+		struct v4l2_rect crop;
 		struct v4l2_mbus_framefmt fmt;
 		bool binning;
 		u16 h_start;
-- 
2.40.1

Re: [PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Implement selection support. Modelled after ov5693 selection support,
> but allow setting sizes smaller then crop-size through set_fmt since

than

> that was already allowed.

...

> +static struct v4l2_rect *
> +__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
> +                     unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +       switch (which) {
> +       case V4L2_SUBDEV_FORMAT_TRY:
> +               return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
> +       case V4L2_SUBDEV_FORMAT_ACTIVE:
> +               return &sensor->mode.crop;
> +       }
> +
> +       return NULL;

I would move this to default: case.

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Hans de Goede]

Hi,

On 5/29/23 22:31, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Implement selection support. Modelled after ov5693 selection support,
>> but allow setting sizes smaller then crop-size through set_fmt since
> 
> than
> 
>> that was already allowed.
> 
> ...
> 
>> +static struct v4l2_rect *
>> +__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
>> +                     unsigned int pad, enum v4l2_subdev_format_whence which)
>> +{
>> +       switch (which) {
>> +       case V4L2_SUBDEV_FORMAT_TRY:
>> +               return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
>> +       case V4L2_SUBDEV_FORMAT_ACTIVE:
>> +               return &sensor->mode.crop;
>> +       }
>> +
>> +       return NULL;
> 
> I would move this to default: case.

That may cause the reader of the code to think that there are other cases,
which there are not. All possible values of enum v4l2_subdev_format_whence
are already handled, otherwise the compiler would also complain.

The "return NULL" is there to shut up other compiler warnings.

I'll add a /* never reached */ to it to make this clear.

Regards,

Hans

Re: [PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Andy Shevchenko]

On Tue, May 30, 2023 at 1:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/29/23 22:31, Andy Shevchenko wrote:
> > On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +       switch (which) {
> >> +       case V4L2_SUBDEV_FORMAT_TRY:
> >> +               return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
> >> +       case V4L2_SUBDEV_FORMAT_ACTIVE:
> >> +               return &sensor->mode.crop;
> >> +       }
> >> +
> >> +       return NULL;
> >
> > I would move this to default: case.
>
> That may cause the reader of the code to think that there are other cases,
> which there are not. All possible values of enum v4l2_subdev_format_whence
> are already handled, otherwise the compiler would also complain.

Why do we care about that?
What is the common practice in the v4l2 subsystem?

> The "return NULL" is there to shut up other compiler warnings.

Can you elaborate (I mean if the default will be present)?

> I'll add a /* never reached */ to it to make this clear.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 06/21] media: atomisp: ov2680: Implement selection support

[Hans de Goede]

Hi,

On 5/30/23 13:28, Andy Shevchenko wrote:
> On Tue, May 30, 2023 at 1:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 5/29/23 22:31, Andy Shevchenko wrote:
>>> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> ...
> 
>>>> +       switch (which) {
>>>> +       case V4L2_SUBDEV_FORMAT_TRY:
>>>> +               return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);
>>>> +       case V4L2_SUBDEV_FORMAT_ACTIVE:
>>>> +               return &sensor->mode.crop;
>>>> +       }
>>>> +
>>>> +       return NULL;
>>>
>>> I would move this to default: case.
>>
>> That may cause the reader of the code to think that there are other cases,
>> which there are not. All possible values of enum v4l2_subdev_format_whence
>> are already handled, otherwise the compiler would also complain.
> 
> Why do we care about that?
> What is the common practice in the v4l2 subsystem?
>> The "return NULL" is there to shut up other compiler warnings.
> 
> Can you elaborate (I mean if the default will be present)?

As a human when I'm reading code if there is a default:
present in a switch-case then I expect that to be something which
may actually happen (which is not the case here).

This is important here because if the default can happen then
the function can return NULL and then callers need to check for
NULL (which they currently do not do).

Looking at other v4l2 drivers I think it would be best to
rewrite the function as:

static struct v4l2_rect *
__ov2680_get_pad_crop(struct ov2680_dev *sensor, struct v4l2_subdev_state *state,
                      unsigned int pad, enum v4l2_subdev_format_whence which)
{
        if (which == V4L2_SUBDEV_FORMAT_TRY)
                return v4l2_subdev_get_try_crop(&sensor->sd, state, pad);

        return &sensor->mode.crop;
}

Regards,

Hans

[PATCH 07/21] media: atomisp: Remove a bunch of sensor related custom IOCTLs

[Hans de Goede]

Remove a bunch of sensor related custom IOCTLs because:

1. They are custom IOCTLs and all custom IOCTLs should be removed
2. Userspace should directly talk to the sensor v4l2-subdev, rather
   then relying on ioctl-s on the output /dev/video# node to pass
   through ioctl-s to the senor
3. Some of these rely on the atomisp specific camera_mipi_info struct
   which is going away as we are switching to using standard v4l2
   sensor drivers
4. In the case of ATOMISP_IOC_S_EXPOSURE_WINDOW this was using the
   v4l2-subdev set_selection API in an undocumented atomisp custom way

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/ov5693/atomisp-ov5693.c |  40 ---
 .../media/atomisp/include/linux/atomisp.h     | 120 ---------
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 240 ------------------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |  10 -
 .../atomisp/pci/atomisp_compat_ioctl32.h      |  55 ----
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  33 ---
 6 files changed, 498 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
index c94fe8e861a5..460a4e34c55b 100644
--- a/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
+++ b/drivers/staging/media/atomisp/i2c/ov5693/atomisp-ov5693.c
@@ -726,51 +726,11 @@ static void *ov5693_otp_read(struct v4l2_subdev *sd)
 	return buf;
 }
 
-static int ov5693_g_priv_int_data(struct v4l2_subdev *sd,
-				  struct v4l2_private_int_data *priv)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct ov5693_device *dev = to_ov5693_sensor(sd);
-	u8 __user *to = priv->data;
-	u32 read_size = priv->size;
-	int ret;
-
-	/* No need to copy data if size is 0 */
-	if (!read_size)
-		goto out;
-
-	if (IS_ERR(dev->otp_data)) {
-		dev_err(&client->dev, "OTP data not available");
-		return PTR_ERR(dev->otp_data);
-	}
-
-	/* Correct read_size value only if bigger than maximum */
-	if (read_size > OV5693_OTP_DATA_SIZE)
-		read_size = OV5693_OTP_DATA_SIZE;
-
-	ret = copy_to_user(to, dev->otp_data, read_size);
-	if (ret) {
-		dev_err(&client->dev, "%s: failed to copy OTP data to user\n",
-			__func__);
-		return -EFAULT;
-	}
-
-	pr_debug("%s read_size:%d\n", __func__, read_size);
-
-out:
-	/* Return correct size */
-	priv->size = dev->otp_size;
-
-	return 0;
-}
-
 static long ov5693_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void *arg)
 {
 	switch (cmd) {
 	case ATOMISP_IOC_S_EXPOSURE:
 		return ov5693_s_exposure(sd, arg);
-	case ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA:
-		return ov5693_g_priv_int_data(sd, arg);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
index bada4c9911fd..14b1757e6674 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
@@ -149,12 +149,6 @@ enum atomisp_calibration_type {
 	calibration_type3
 };
 
-struct atomisp_calibration_group {
-	unsigned int size;
-	unsigned int type;
-	unsigned short *calb_grp_values;
-};
-
 struct atomisp_gc_config {
 	__u16 gain_k1;
 	__u16 gain_k2;
@@ -265,26 +259,6 @@ enum atomisp_metadata_type {
 	ATOMISP_METADATA_TYPE_NUM,
 };
 
-struct atomisp_metadata_with_type {
-	/* to specify which type of metadata to get */
-	enum atomisp_metadata_type type;
-	void __user *data;
-	u32 width;
-	u32 height;
-	u32 stride; /* in bytes */
-	u32 exp_id; /* exposure ID */
-	u32 *effective_width; /* mipi packets valid data size */
-};
-
-struct atomisp_metadata {
-	void __user *data;
-	u32 width;
-	u32 height;
-	u32 stride; /* in bytes */
-	u32 exp_id; /* exposure ID */
-	u32 *effective_width; /* mipi packets valid data size */
-};
-
 struct atomisp_ext_isp_ctrl {
 	u32 id;
 	u32 data;
@@ -298,14 +272,6 @@ struct atomisp_3a_statistics {
 	u32 isp_config_id; /* isp config ID */
 };
 
-struct atomisp_ae_window {
-	int x_left;
-	int x_right;
-	int y_top;
-	int y_bottom;
-	int weight;
-};
-
 /* White Balance (Gain Adjust) */
 struct atomisp_wb_config {
 	unsigned int integer_bits;
@@ -754,53 +720,6 @@ struct atomisp_s_runmode {
 	__u32 mode;
 };
 
-struct atomisp_update_exposure {
-	unsigned int gain;
-	unsigned int digi_gain;
-	unsigned int update_gain;
-	unsigned int update_digi_gain;
-};
-
-/*
- * V4L2 private internal data interface.
- * -----------------------------------------------------------------------------
- * struct v4l2_private_int_data - request private data stored in video device
- * internal memory.
- * @size: sanity check to ensure userspace's buffer fits whole private data.
- *	  If not, kernel will make partial copy (or nothing if @size == 0).
- *	  @size is always corrected for the minimum necessary if IOCTL returns
- *	  no error.
- * @data: pointer to userspace buffer.
- */
-struct v4l2_private_int_data {
-	__u32 size;
-	void __user *data;
-	__u32 reserved[2];
-};
-
-enum atomisp_sensor_ae_bracketing_mode {
-	SENSOR_AE_BRACKETING_MODE_OFF = 0,
-	SENSOR_AE_BRACKETING_MODE_SINGLE, /* back to SW standby after bracketing */
-	SENSOR_AE_BRACKETING_MODE_SINGLE_TO_STREAMING, /* back to normal streaming after bracketing */
-	SENSOR_AE_BRACKETING_MODE_LOOP, /* continue AE bracketing in loop mode */
-};
-
-struct atomisp_sensor_ae_bracketing_info {
-	unsigned int modes; /* bit mask to indicate supported modes  */
-	unsigned int lut_depth;
-};
-
-struct atomisp_sensor_ae_bracketing_lut_entry {
-	__u16 coarse_integration_time;
-	__u16 analog_gain;
-	__u16 digital_gain;
-};
-
-struct atomisp_sensor_ae_bracketing_lut {
-	struct atomisp_sensor_ae_bracketing_lut_entry *lut;
-	unsigned int lut_size;
-};
-
 /*Private IOCTLs for ISP */
 #define ATOMISP_IOC_G_XNR \
 	_IOR('v', BASE_VIDIOC_PRIVATE + 0, int)
@@ -905,20 +824,12 @@ struct atomisp_sensor_ae_bracketing_lut {
 #define ATOMISP_IOC_S_EXPOSURE \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 21, struct atomisp_exposure)
 
-/* sensor calibration registers group */
-#define ATOMISP_IOC_G_SENSOR_CALIBRATION_GROUP \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 22, struct atomisp_calibration_group)
-
 /* white balance Correction */
 #define ATOMISP_IOC_G_3A_CONFIG \
 	_IOR('v', BASE_VIDIOC_PRIVATE + 23, struct atomisp_3a_config)
 #define ATOMISP_IOC_S_3A_CONFIG \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 23, struct atomisp_3a_config)
 
-/* sensor OTP memory read */
-#define ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 26, struct v4l2_private_int_data)
-
 /* LCS (shading) table write */
 #define ATOMISP_IOC_S_ISP_SHD_TAB \
 	_IOWR('v', BASE_VIDIOC_PRIVATE + 27, struct atomisp_shading_table)
@@ -930,19 +841,9 @@ struct atomisp_sensor_ae_bracketing_lut {
 #define ATOMISP_IOC_S_ISP_GAMMA_CORRECTION \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 28, struct atomisp_gc_config)
 
-/* motor internal memory read */
-#define ATOMISP_IOC_G_MOTOR_PRIV_INT_DATA \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 29, struct v4l2_private_int_data)
-
 #define ATOMISP_IOC_S_PARAMETERS \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 32, struct atomisp_parameters)
 
-#define ATOMISP_IOC_G_METADATA \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 34, struct atomisp_metadata)
-
-#define ATOMISP_IOC_G_METADATA_BY_TYPE \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 34, struct atomisp_metadata_with_type)
-
 #define ATOMISP_IOC_EXT_ISP_CTRL \
 	_IOWR('v', BASE_VIDIOC_PRIVATE + 35, struct atomisp_ext_isp_ctrl)
 
@@ -961,27 +862,9 @@ struct atomisp_sensor_ae_bracketing_lut {
 #define ATOMISP_IOC_S_FORMATS_CONFIG \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 39, struct atomisp_formats_config)
 
-#define ATOMISP_IOC_S_EXPOSURE_WINDOW \
-	_IOW('v', BASE_VIDIOC_PRIVATE + 40, struct atomisp_ae_window)
-
 #define ATOMISP_IOC_INJECT_A_FAKE_EVENT \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 42, int)
 
-#define ATOMISP_IOC_G_SENSOR_AE_BRACKETING_INFO \
-	_IOR('v', BASE_VIDIOC_PRIVATE + 43, struct atomisp_sensor_ae_bracketing_info)
-
-#define ATOMISP_IOC_S_SENSOR_AE_BRACKETING_MODE \
-	_IOW('v', BASE_VIDIOC_PRIVATE + 43, unsigned int)
-
-#define ATOMISP_IOC_G_SENSOR_AE_BRACKETING_MODE \
-	_IOR('v', BASE_VIDIOC_PRIVATE + 43, unsigned int)
-
-#define ATOMISP_IOC_S_SENSOR_AE_BRACKETING_LUT \
-	_IOW('v', BASE_VIDIOC_PRIVATE + 43, struct atomisp_sensor_ae_bracketing_lut)
-
-#define ATOMISP_IOC_G_INVALID_FRAME_NUM \
-	_IOR('v', BASE_VIDIOC_PRIVATE + 44, unsigned int)
-
 #define ATOMISP_IOC_S_ARRAY_RESOLUTION \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 45, struct atomisp_resolution)
 
@@ -995,9 +878,6 @@ struct atomisp_sensor_ae_bracketing_lut {
 #define ATOMISP_IOC_S_SENSOR_RUNMODE \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 48, struct atomisp_s_runmode)
 
-#define ATOMISP_IOC_G_UPDATE_EXPOSURE \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 49, struct atomisp_update_exposure)
-
 /*
  * Reserved ioctls. We have customer implementing it internally.
  * We can't use both numbers to not cause ABI conflict.
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 5b244d173b9a..748cb78d1ee8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1851,161 +1851,6 @@ int atomisp_3a_stat(struct atomisp_sub_device *asd, int flag,
 	return 0;
 }
 
-int atomisp_get_metadata(struct atomisp_sub_device *asd, int flag,
-			 struct atomisp_metadata *md)
-{
-	struct atomisp_device *isp = asd->isp;
-	struct ia_css_stream_info *stream_info;
-	struct camera_mipi_info *mipi_info;
-	struct atomisp_metadata_buf *md_buf;
-	enum atomisp_metadata_type md_type = ATOMISP_MAIN_METADATA;
-	int ret, i;
-
-	if (flag != 0)
-		return -EINVAL;
-
-	stream_info = &asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].
-		      stream_info;
-
-	/* We always return the resolution and stride even if there is
-	 * no valid metadata. This allows the caller to get the information
-	 * needed to allocate user-space buffers. */
-	md->width  = stream_info->metadata_info.resolution.width;
-	md->height = stream_info->metadata_info.resolution.height;
-	md->stride = stream_info->metadata_info.stride;
-
-	/* sanity check to avoid writing into unallocated memory.
-	 * This does not return an error because it is a valid way
-	 * for applications to detect that metadata is not enabled. */
-	if (md->width == 0 || md->height == 0 || !md->data)
-		return 0;
-
-	/* This is done in the atomisp_buf_done() */
-	if (list_empty(&asd->metadata_ready[md_type])) {
-		dev_warn(isp->dev, "Metadata queue is empty now!\n");
-		return -EAGAIN;
-	}
-
-	mipi_info = atomisp_to_sensor_mipi_info(
-			isp->inputs[asd->input_curr].camera);
-	if (!mipi_info)
-		return -EINVAL;
-
-	if (mipi_info->metadata_effective_width) {
-		for (i = 0; i < md->height; i++)
-			md->effective_width[i] =
-			    mipi_info->metadata_effective_width[i];
-	}
-
-	md_buf = list_entry(asd->metadata_ready[md_type].next,
-			    struct atomisp_metadata_buf, list);
-	md->exp_id = md_buf->metadata->exp_id;
-	if (md_buf->md_vptr) {
-		ret = copy_to_user(md->data,
-				   md_buf->md_vptr,
-				   stream_info->metadata_info.size);
-	} else {
-		hmm_load(md_buf->metadata->address,
-			 asd->params.metadata_user[md_type],
-			 stream_info->metadata_info.size);
-
-		ret = copy_to_user(md->data,
-				   asd->params.metadata_user[md_type],
-				   stream_info->metadata_info.size);
-	}
-	if (ret) {
-		dev_err(isp->dev, "copy to user failed: copied %d bytes\n",
-			ret);
-		return -EFAULT;
-	}
-
-	list_del_init(&md_buf->list);
-	list_add_tail(&md_buf->list, &asd->metadata[md_type]);
-
-	dev_dbg(isp->dev, "%s: HAL de-queued metadata type %d with exp_id %d\n",
-		__func__, md_type, md->exp_id);
-	return 0;
-}
-
-int atomisp_get_metadata_by_type(struct atomisp_sub_device *asd, int flag,
-				 struct atomisp_metadata_with_type *md)
-{
-	struct atomisp_device *isp = asd->isp;
-	struct ia_css_stream_info *stream_info;
-	struct camera_mipi_info *mipi_info;
-	struct atomisp_metadata_buf *md_buf;
-	enum atomisp_metadata_type md_type;
-	int ret, i;
-
-	if (flag != 0)
-		return -EINVAL;
-
-	stream_info = &asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].
-		      stream_info;
-
-	/* We always return the resolution and stride even if there is
-	 * no valid metadata. This allows the caller to get the information
-	 * needed to allocate user-space buffers. */
-	md->width  = stream_info->metadata_info.resolution.width;
-	md->height = stream_info->metadata_info.resolution.height;
-	md->stride = stream_info->metadata_info.stride;
-
-	/* sanity check to avoid writing into unallocated memory.
-	 * This does not return an error because it is a valid way
-	 * for applications to detect that metadata is not enabled. */
-	if (md->width == 0 || md->height == 0 || !md->data)
-		return 0;
-
-	md_type = md->type;
-	if (md_type < 0 || md_type >= ATOMISP_METADATA_TYPE_NUM)
-		return -EINVAL;
-
-	/* This is done in the atomisp_buf_done() */
-	if (list_empty(&asd->metadata_ready[md_type])) {
-		dev_warn(isp->dev, "Metadata queue is empty now!\n");
-		return -EAGAIN;
-	}
-
-	mipi_info = atomisp_to_sensor_mipi_info(
-			isp->inputs[asd->input_curr].camera);
-	if (!mipi_info)
-		return -EINVAL;
-
-	if (mipi_info->metadata_effective_width) {
-		for (i = 0; i < md->height; i++)
-			md->effective_width[i] =
-			    mipi_info->metadata_effective_width[i];
-	}
-
-	md_buf = list_entry(asd->metadata_ready[md_type].next,
-			    struct atomisp_metadata_buf, list);
-	md->exp_id = md_buf->metadata->exp_id;
-	if (md_buf->md_vptr) {
-		ret = copy_to_user(md->data,
-				   md_buf->md_vptr,
-				   stream_info->metadata_info.size);
-	} else {
-		hmm_load(md_buf->metadata->address,
-			 asd->params.metadata_user[md_type],
-			 stream_info->metadata_info.size);
-
-		ret = copy_to_user(md->data,
-				   asd->params.metadata_user[md_type],
-				   stream_info->metadata_info.size);
-	}
-	if (ret) {
-		dev_err(isp->dev, "copy to user failed: copied %d bytes\n",
-			ret);
-		return -EFAULT;
-	} else {
-		list_del_init(&md_buf->list);
-		list_add_tail(&md_buf->list, &asd->metadata[md_type]);
-	}
-	dev_dbg(isp->dev, "%s: HAL de-queued metadata type %d with exp_id %d\n",
-		__func__, md_type, md->exp_id);
-	return 0;
-}
-
 /*
  * Function to calculate real zoom region for every pipe
  */
@@ -4665,30 +4510,6 @@ int atomisp_set_shading_table(struct atomisp_sub_device *asd,
 	return ret;
 }
 
-/*
- * set auto exposure metering window to camera sensor
- */
-int atomisp_s_ae_window(struct atomisp_sub_device *asd,
-			struct atomisp_ae_window *arg)
-{
-	struct atomisp_device *isp = asd->isp;
-	/* Coverity CID 298071 - initialzize struct */
-	struct v4l2_subdev_selection sel = { 0 };
-
-	sel.r.left = arg->x_left;
-	sel.r.top = arg->y_top;
-	sel.r.width = arg->x_right - arg->x_left + 1;
-	sel.r.height = arg->y_bottom - arg->y_top + 1;
-
-	if (v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-			     pad, set_selection, NULL, &sel)) {
-		dev_err(isp->dev, "failed to call sensor set_selection.\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
 {
 	struct atomisp_device *isp = asd->isp;
@@ -4868,64 +4689,3 @@ int atomisp_inject_a_fake_event(struct atomisp_sub_device *asd, int *event)
 
 	return 0;
 }
-
-static int atomisp_get_pipe_id(struct atomisp_video_pipe *pipe)
-{
-	struct atomisp_sub_device *asd = pipe->asd;
-
-	if (!asd) {
-		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, pipe->vdev.name);
-		return -EINVAL;
-	}
-
-	if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
-		return IA_CSS_PIPE_ID_VIDEO;
-	} else if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
-		return IA_CSS_PIPE_ID_CAPTURE;
-	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
-		return IA_CSS_PIPE_ID_VIDEO;
-	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
-		return IA_CSS_PIPE_ID_PREVIEW;
-	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_STILL_CAPTURE) {
-		if (asd->copy_mode)
-			return IA_CSS_PIPE_ID_COPY;
-		else
-			return IA_CSS_PIPE_ID_CAPTURE;
-	}
-
-	/* fail through */
-	dev_warn(asd->isp->dev, "%s failed to find proper pipe\n",
-		 __func__);
-	return IA_CSS_PIPE_ID_CAPTURE;
-}
-
-int atomisp_get_invalid_frame_num(struct video_device *vdev,
-				  int *invalid_frame_num)
-{
-	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
-	struct atomisp_sub_device *asd = pipe->asd;
-	enum ia_css_pipe_id pipe_id;
-	struct ia_css_pipe_info p_info;
-	int ret;
-
-	pipe_id = atomisp_get_pipe_id(pipe);
-	if (!asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].pipes[pipe_id]) {
-		dev_warn(asd->isp->dev,
-			 "%s pipe %d has not been created yet, do SET_FMT first!\n",
-			 __func__, pipe_id);
-		return -EINVAL;
-	}
-
-	ret = ia_css_pipe_get_info(
-		  asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL]
-		  .pipes[pipe_id], &p_info);
-	if (!ret) {
-		*invalid_frame_num = p_info.num_invalid_frames;
-		return 0;
-	} else {
-		dev_warn(asd->isp->dev, "%s get pipe infor failed %d\n",
-			 __func__, ret);
-		return -EINVAL;
-	}
-}
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index 5270c370e463..ff2178c96fb8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -159,13 +159,6 @@ int atomisp_set_dis_vector(struct atomisp_sub_device *asd,
 int atomisp_3a_stat(struct atomisp_sub_device *asd, int flag,
 		    struct atomisp_3a_statistics *config);
 
-/* Function to get metadata from isp */
-int atomisp_get_metadata(struct atomisp_sub_device *asd, int flag,
-			 struct atomisp_metadata *config);
-
-int atomisp_get_metadata_by_type(struct atomisp_sub_device *asd, int flag,
-				 struct atomisp_metadata_with_type *config);
-
 int atomisp_set_parameters(struct video_device *vdev,
 			   struct atomisp_parameters *arg);
 
@@ -267,9 +260,6 @@ int atomisp_set_shading_table(struct atomisp_sub_device *asd,
 
 void atomisp_free_internal_buffers(struct atomisp_sub_device *asd);
 
-int atomisp_s_ae_window(struct atomisp_sub_device *asd,
-			struct atomisp_ae_window *arg);
-
 int  atomisp_flash_enable(struct atomisp_sub_device *asd,
 			  int num_frames);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.h b/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.h
index 33821b51d90e..762520ed87a5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_ioctl32.h
@@ -67,26 +67,6 @@ struct atomisp_3a_statistics32 {
 	u32 isp_config_id;
 };
 
-struct atomisp_metadata_with_type32 {
-	/* to specify which type of metadata to get */
-	enum atomisp_metadata_type type;
-	compat_uptr_t data;
-	u32 width;
-	u32 height;
-	u32 stride; /* in bytes */
-	u32 exp_id; /* exposure ID */
-	compat_uptr_t effective_width;
-};
-
-struct atomisp_metadata32 {
-	compat_uptr_t data;
-	u32 width;
-	u32 height;
-	u32 stride;
-	u32 exp_id;
-	compat_uptr_t effective_width;
-};
-
 struct atomisp_morph_table32 {
 	unsigned int enabled;
 	unsigned int height;
@@ -134,18 +114,6 @@ struct atomisp_overlay32 {
 	unsigned int overlay_start_y;
 };
 
-struct atomisp_calibration_group32 {
-	unsigned int size;
-	unsigned int type;
-	compat_uptr_t calb_grp_values;
-};
-
-struct v4l2_private_int_data32 {
-	__u32 size;
-	compat_uptr_t data;
-	__u32 reserved[2];
-};
-
 struct atomisp_shading_table32 {
 	__u32 enable;
 	__u32 sensor_width;
@@ -249,11 +217,6 @@ struct atomisp_dvs_6axis_config32 {
 	compat_uptr_t ycoords_uv;
 };
 
-struct atomisp_sensor_ae_bracketing_lut32 {
-	compat_uptr_t lut;
-	unsigned int lut_size;
-};
-
 #define ATOMISP_IOC_G_HISTOGRAM32 \
 	_IOWR('v', BASE_VIDIOC_PRIVATE + 3, struct atomisp_histogram32)
 #define ATOMISP_IOC_S_HISTOGRAM32 \
@@ -283,28 +246,10 @@ struct atomisp_sensor_ae_bracketing_lut32 {
 #define ATOMISP_IOC_S_ISP_OVERLAY32 \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 18, struct atomisp_overlay32)
 
-#define ATOMISP_IOC_G_SENSOR_CALIBRATION_GROUP32 \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 22, struct atomisp_calibration_group32)
-
-#define ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA32 \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 26, struct v4l2_private_int_data32)
-
 #define ATOMISP_IOC_S_ISP_SHD_TAB32 \
 	_IOWR('v', BASE_VIDIOC_PRIVATE + 27, struct atomisp_shading_table32)
 
-#define ATOMISP_IOC_G_MOTOR_PRIV_INT_DATA32 \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 29, struct v4l2_private_int_data32)
-
 #define ATOMISP_IOC_S_PARAMETERS32 \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 32, struct atomisp_parameters32)
 
-#define ATOMISP_IOC_G_METADATA32 \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 34, struct atomisp_metadata32)
-
-#define ATOMISP_IOC_G_METADATA_BY_TYPE32 \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 34, struct atomisp_metadata_with_type32)
-
-#define ATOMISP_IOC_S_SENSOR_AE_BRACKETING_LUT32 \
-	_IOW('v', BASE_VIDIOC_PRIVATE + 43, struct atomisp_sensor_ae_bracketing_lut32)
-
 #endif /* __ATOMISP_COMPAT_IOCTL32_H__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 2cde1af77a2d..6a86dc7a933f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1921,31 +1921,10 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		err = atomisp_fixed_pattern_table(asd, arg);
 		break;
 
-	case ATOMISP_IOC_G_MOTOR_PRIV_INT_DATA:
-		if (motor)
-			err = v4l2_subdev_call(motor, core, ioctl, cmd, arg);
-		else
-			err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-					       core, ioctl, cmd, arg);
-		break;
-
 	case ATOMISP_IOC_S_EXPOSURE:
-	case ATOMISP_IOC_G_SENSOR_CALIBRATION_GROUP:
-	case ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA:
-	case ATOMISP_IOC_G_SENSOR_AE_BRACKETING_INFO:
-	case ATOMISP_IOC_S_SENSOR_AE_BRACKETING_MODE:
-	case ATOMISP_IOC_G_SENSOR_AE_BRACKETING_MODE:
-	case ATOMISP_IOC_S_SENSOR_AE_BRACKETING_LUT:
 		err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
 				       core, ioctl, cmd, arg);
 		break;
-	case ATOMISP_IOC_G_UPDATE_EXPOSURE:
-		if (IS_ISP2401)
-			err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-					       core, ioctl, cmd, arg);
-		else
-			err = -EINVAL;
-		break;
 
 	case ATOMISP_IOC_S_ISP_SHD_TAB:
 		err = atomisp_set_shading_table(asd, arg);
@@ -1963,12 +1942,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		err = atomisp_set_parameters(vdev, arg);
 		break;
 
-	case ATOMISP_IOC_G_METADATA:
-		err = atomisp_get_metadata(asd, 0, arg);
-		break;
-	case ATOMISP_IOC_G_METADATA_BY_TYPE:
-		err = atomisp_get_metadata_by_type(asd, 0, arg);
-		break;
 	case ATOMISP_IOC_EXT_ISP_CTRL:
 		err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
 				       core, ioctl, cmd, arg);
@@ -1989,15 +1962,9 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 	case ATOMISP_IOC_S_FORMATS_CONFIG:
 		err = atomisp_formats(asd, 1, arg);
 		break;
-	case ATOMISP_IOC_S_EXPOSURE_WINDOW:
-		err = atomisp_s_ae_window(asd, arg);
-		break;
 	case ATOMISP_IOC_INJECT_A_FAKE_EVENT:
 		err = atomisp_inject_a_fake_event(asd, arg);
 		break;
-	case ATOMISP_IOC_G_INVALID_FRAME_NUM:
-		err = atomisp_get_invalid_frame_num(vdev, arg);
-		break;
 	case ATOMISP_IOC_S_ARRAY_RESOLUTION:
 		err = atomisp_set_array_res(asd, arg);
 		break;
-- 
2.40.1

[PATCH 08/21] media: atomisp: Remove redundant atomisp_subdev_set_selection() calls from atomisp_set_fmt()

[Hans de Goede]

atomisp_subdev_set_selection(sink-pad, V4L2_SEL_TGT_CROP, rect)
ignores the passed in rect, using the width and height from the last
atomisp_subdev_set_ffmt(ATOMISP_SUBDEV_PAD_SINK, ffmt) call instead.

The atomisp_subdev_set_ffmt() call done by atomisp_set_fmt_to_snr()
already propagates the sink ffmt changes to V4L2_SEL_TGT_CROP
(this is what allows atomisp_set_fmt() to get the isp_sink_crop in
the first place).

Remove the redundant atomisp_subdev_set_selection(sink-pad, ...)
calls.

Note the removed aspect ratio correction in the last else block is
is already done by atomisp_subdev_set_selection() itself when
setting V4L2_SEL_TGT_COMPOSE on the source-pad.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 33 -------------------
 1 file changed, 33 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 748cb78d1ee8..36618d2eba72 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -4327,12 +4327,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		isp_sink_crop.width = f->fmt.pix.width;
 		isp_sink_crop.height = f->fmt.pix.height;
 
-		atomisp_subdev_set_selection(&asd->subdev, fh.state,
-					     V4L2_SUBDEV_FORMAT_ACTIVE,
-					     ATOMISP_SUBDEV_PAD_SINK,
-					     V4L2_SEL_TGT_CROP,
-					     V4L2_SEL_FLAG_KEEP_CONFIG,
-					     &isp_sink_crop);
 		atomisp_subdev_set_selection(&asd->subdev, fh.state,
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE, V4L2_SEL_TGT_COMPOSE,
@@ -4358,38 +4352,11 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 					     V4L2_SEL_TGT_COMPOSE, 0,
 					     &main_compose);
 	} else {
-		struct v4l2_rect sink_crop = {0};
 		struct v4l2_rect main_compose = {0};
 
 		main_compose.width = f->fmt.pix.width;
 		main_compose.height = f->fmt.pix.height;
 
-		/* WORKAROUND: this override is universally enabled in
-		 * GMIN to work around a CTS failures (GMINL-539)
-		 * which appears to be related by a hardware
-		 * performance limitation.  It's unclear why this
-		 * particular code triggers the issue. */
-		if (isp_sink_crop.width * main_compose.height >
-		    isp_sink_crop.height * main_compose.width) {
-			sink_crop.height = isp_sink_crop.height;
-			sink_crop.width =
-				DIV_NEAREST_STEP(sink_crop.height * f->fmt.pix.width,
-						 f->fmt.pix.height,
-						 ATOM_ISP_STEP_WIDTH);
-		} else {
-			sink_crop.width = isp_sink_crop.width;
-			sink_crop.height =
-				DIV_NEAREST_STEP(sink_crop.width * f->fmt.pix.height,
-						 f->fmt.pix.width,
-						 ATOM_ISP_STEP_HEIGHT);
-		}
-		atomisp_subdev_set_selection(&asd->subdev, fh.state,
-					     V4L2_SUBDEV_FORMAT_ACTIVE,
-					     ATOMISP_SUBDEV_PAD_SINK,
-					     V4L2_SEL_TGT_CROP,
-					     V4L2_SEL_FLAG_KEEP_CONFIG,
-					     &sink_crop);
-
 		atomisp_subdev_set_selection(&asd->subdev, fh.state,
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE,
-- 
2.40.1

[PATCH 09/21] media: atomisp: Simplify atomisp_subdev_set_selection() calls in atomisp_set_fmt()

[Hans de Goede]

With the atomisp_subdev_set_selection(sink-pad, V4L2_SEL_TGT_CROP, rect)
calls dropped. The first and last compount code blocks of the 3 code blocks
in the if (...) {} else if (...) {} else {} code setting the source-pad
V4L2_SEL_TGT_COMPOSE selection are the same.

The both set V4L2_SEL_TGT_COMPOSE to a rectangle with the same dimensions
as f->fmt.pix.height.

Remove the else {} block at the end, drop the second if and prepend
the first if condition with "!second-if-condition ||" to remove
the code duplication.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 36618d2eba72..2a1cb3049019 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -4319,7 +4319,8 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 
 	/* Try to enable YUV downscaling if ISP input is 10 % (either
 	 * width or height) bigger than the desired result. */
-	if (isp_sink_crop.width * 9 / 10 < f->fmt.pix.width ||
+	if (!IS_MOFD ||
+	    isp_sink_crop.width * 9 / 10 < f->fmt.pix.width ||
 	    isp_sink_crop.height * 9 / 10 < f->fmt.pix.height ||
 	    (atomisp_subdev_format_conversion(asd) &&
 	     (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO ||
@@ -4331,7 +4332,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE, V4L2_SEL_TGT_COMPOSE,
 					     0, &isp_sink_crop);
-	} else if (IS_MOFD) {
+	} else {
 		struct v4l2_rect main_compose = {0};
 
 		main_compose.width = isp_sink_crop.width;
@@ -4346,17 +4347,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 					 f->fmt.pix.height);
 		}
 
-		atomisp_subdev_set_selection(&asd->subdev, fh.state,
-					     V4L2_SUBDEV_FORMAT_ACTIVE,
-					     ATOMISP_SUBDEV_PAD_SOURCE,
-					     V4L2_SEL_TGT_COMPOSE, 0,
-					     &main_compose);
-	} else {
-		struct v4l2_rect main_compose = {0};
-
-		main_compose.width = f->fmt.pix.width;
-		main_compose.height = f->fmt.pix.height;
-
 		atomisp_subdev_set_selection(&asd->subdev, fh.state,
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE,
-- 
2.40.1

[PATCH 10/21] media: atomisp: Add target validation to atomisp_subdev_set_selection()

[Hans de Goede]

As the 2 comments in the function already say both the sink and the source
pads only support setting the selection for 1 target:

		/* Only crop target supported on sink pad. */
		/* Only compose target is supported on source pads. */

Validate that the passed in target actually matches these expectations.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_subdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 7985a0319a39..04e257ede7d4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -360,6 +360,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
 
+	if ((pad == ATOMISP_SUBDEV_PAD_SINK && target != V4L2_SEL_TGT_CROP) ||
+	    (pad == ATOMISP_SUBDEV_PAD_SOURCE && target != V4L2_SEL_TGT_COMPOSE))
+		return -EINVAL;
+
 	isp_get_fmt_rect(sd, sd_state, which, ffmt, crop, comp);
 
 	dev_dbg(isp->dev,
-- 
2.40.1

[PATCH 11/21] media: atomisp: Remove bogus fh use from atomisp_set_fmt*()

[Hans de Goede]

atomisp_set_fmt*() use a local v4l2_subdev_fh declared on the stack,
specifically they use fh.state which is never initialized so when
passing fh.state to atomisp_subdev_set_ffmt() / to
atomisp_subdev_set_selection() these functions are passing random
stack contents as a pointer.

The reason this works is because when the which parameter is
V4L2_SUBDEV_FORMAT_ACTIVE the passed in state is not used.

Remove the bogus fh usage and just pass NULL as state.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 2a1cb3049019..87184ddf94c5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3908,7 +3908,6 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	const struct atomisp_format_bridge *format;
 	struct v4l2_rect *isp_sink_crop;
 	enum ia_css_pipe_id pipe_id;
-	struct v4l2_subdev_fh fh;
 	int (*configure_output)(struct atomisp_sub_device *asd,
 				unsigned int width, unsigned int height,
 				unsigned int min_width,
@@ -3929,8 +3928,6 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 		return -EINVAL;
 	}
 
-	v4l2_fh_init(&fh.vfh, vdev);
-
 	isp_sink_crop = atomisp_subdev_get_rect(
 			    &asd->subdev, NULL, V4L2_SUBDEV_FORMAT_ACTIVE,
 			    ATOMISP_SUBDEV_PAD_SINK, V4L2_SEL_TGT_CROP);
@@ -4138,7 +4135,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	struct atomisp_device *isp;
 	struct atomisp_input_stream_info *stream_info =
 	    (struct atomisp_input_stream_info *)ffmt->reserved;
-	struct v4l2_subdev_fh fh;
 	int ret;
 
 	if (!asd) {
@@ -4149,8 +4145,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 
 	isp = asd->isp;
 
-	v4l2_fh_init(&fh.vfh, vdev);
-
 	format = atomisp_get_format_bridge(f->pixelformat);
 	if (!format)
 		return -EINVAL;
@@ -4210,7 +4204,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 		asd->params.video_dis_en = false;
 	}
 
-	atomisp_subdev_set_ffmt(&asd->subdev, fh.state,
+	atomisp_subdev_set_ffmt(&asd->subdev, NULL,
 				V4L2_SUBDEV_FORMAT_ACTIVE,
 				ATOMISP_SUBDEV_PAD_SINK, ffmt);
 
@@ -4232,7 +4226,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 	struct v4l2_rect isp_sink_crop;
-	struct v4l2_subdev_fh fh;
 	int ret;
 
 	ret = atomisp_pipe_check(pipe, true);
@@ -4243,8 +4236,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		"setting resolution %ux%u bytesperline %u\n",
 		f->fmt.pix.width, f->fmt.pix.height, f->fmt.pix.bytesperline);
 
-	v4l2_fh_init(&fh.vfh, vdev);
-
 	format_bridge = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
 	if (!format_bridge)
 		return -EINVAL;
@@ -4288,7 +4279,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 				    snr_format_bridge->mbus_code;
 
 	isp_source_fmt.code = format_bridge->mbus_code;
-	atomisp_subdev_set_ffmt(&asd->subdev, fh.state,
+	atomisp_subdev_set_ffmt(&asd->subdev, NULL,
 				V4L2_SUBDEV_FORMAT_ACTIVE,
 				ATOMISP_SUBDEV_PAD_SOURCE, &isp_source_fmt);
 
@@ -4328,7 +4319,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		isp_sink_crop.width = f->fmt.pix.width;
 		isp_sink_crop.height = f->fmt.pix.height;
 
-		atomisp_subdev_set_selection(&asd->subdev, fh.state,
+		atomisp_subdev_set_selection(&asd->subdev, NULL,
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE, V4L2_SEL_TGT_COMPOSE,
 					     0, &isp_sink_crop);
@@ -4347,7 +4338,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 					 f->fmt.pix.height);
 		}
 
-		atomisp_subdev_set_selection(&asd->subdev, fh.state,
+		atomisp_subdev_set_selection(&asd->subdev, NULL,
 					     V4L2_SUBDEV_FORMAT_ACTIVE,
 					     ATOMISP_SUBDEV_PAD_SOURCE,
 					     V4L2_SEL_TGT_COMPOSE, 0,
-- 
2.40.1

[PATCH 12/21] media: atomisp: Add input helper variable for isp->asd->inputs[asd->input_curr]

[Hans de Goede]

Passing 'isp->asd->inputs[asd->input_curr].foo' as argument to
various function calls is rather long.

Add a local input helper variable for this, so that the function
calls will fit on one line.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 33 +++++++++----------
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  4 +--
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 87184ddf94c5..243a789fef6a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3662,6 +3662,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 {
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
 		.pads = &pad_cfg,
@@ -3678,7 +3679,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 		return -EINVAL;
 	}
 
-	if (!isp->inputs[asd->input_curr].camera)
+	if (!input->camera)
 		return -EINVAL;
 
 	fmt = atomisp_get_format_bridge(f->pixelformat);
@@ -3700,8 +3701,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 	dev_dbg(isp->dev, "try_mbus_fmt: asking for %ux%u\n",
 		format.format.width, format.format.height);
 
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-			       pad, set_fmt, &pad_state, &format);
+	ret = v4l2_subdev_call(input->camera, pad, set_fmt, &pad_state, &format);
 	if (ret)
 		return ret;
 
@@ -3761,6 +3761,7 @@ static inline int atomisp_set_sensor_mipi_to_isp(
 {
 	struct v4l2_control ctrl;
 	struct atomisp_device *isp = asd->isp;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_in_fmt_conv *fc;
 	int mipi_freq = 0;
 	unsigned int input_format, bayer_order;
@@ -3768,8 +3769,7 @@ static inline int atomisp_set_sensor_mipi_to_isp(
 	u32 mipi_port, metadata_width = 0, metadata_height = 0;
 
 	ctrl.id = V4L2_CID_LINK_FREQ;
-	if (v4l2_g_ctrl
-	    (isp->inputs[asd->input_curr].camera->ctrl_handler, &ctrl) == 0)
+	if (v4l2_g_ctrl(input->camera->ctrl_handler, &ctrl) == 0)
 		mipi_freq = ctrl.value;
 
 	if (asd->stream_env[stream_id].isys_configs == 1) {
@@ -3827,7 +3827,7 @@ static inline int atomisp_set_sensor_mipi_to_isp(
 		return -EINVAL;
 
 	input_format = fc->atomisp_in_fmt;
-	mipi_port = atomisp_port_to_mipi_port(isp, isp->inputs[asd->input_curr].port);
+	mipi_port = atomisp_port_to_mipi_port(isp, input->port);
 	atomisp_css_input_configure_port(asd, mipi_port,
 					 isp->sensor_lanes[mipi_port],
 					 0xffff4, mipi_freq,
@@ -3905,6 +3905,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	struct camera_mipi_info *mipi_info;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_format_bridge *format;
 	struct v4l2_rect *isp_sink_crop;
 	enum ia_css_pipe_id pipe_id;
@@ -3936,9 +3937,8 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	if (!format)
 		return -EINVAL;
 
-	if (isp->inputs[asd->input_curr].type != TEST_PATTERN) {
-		mipi_info = atomisp_to_sensor_mipi_info(
-				isp->inputs[asd->input_curr].camera);
+	if (input->type != TEST_PATTERN) {
+		mipi_info = atomisp_to_sensor_mipi_info(input->camera);
 
 		if (atomisp_set_sensor_mipi_to_isp(asd, ATOMISP_INPUT_STREAM_GENERAL,
 						   mipi_info))
@@ -4122,6 +4122,8 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 {
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
+	struct atomisp_device *isp = asd->isp;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_format_bridge *format;
 	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
@@ -4132,7 +4134,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	};
 	struct v4l2_mbus_framefmt *ffmt = &vformat.format;
 	struct v4l2_mbus_framefmt *req_ffmt;
-	struct atomisp_device *isp;
 	struct atomisp_input_stream_info *stream_info =
 	    (struct atomisp_input_stream_info *)ffmt->reserved;
 	int ret;
@@ -4143,8 +4144,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 		return -EINVAL;
 	}
 
-	isp = asd->isp;
-
 	format = atomisp_get_format_bridge(f->pixelformat);
 	if (!format)
 		return -EINVAL;
@@ -4164,8 +4163,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	/* Disable dvs if resolution can't be supported by sensor */
 	if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
 		vformat.which = V4L2_SUBDEV_FORMAT_TRY;
-		ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-				       pad, set_fmt, &pad_state, &vformat);
+		ret = v4l2_subdev_call(input->camera, pad, set_fmt, &pad_state, &vformat);
 		if (ret)
 			return ret;
 
@@ -4183,8 +4181,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 		}
 	}
 	vformat.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera, pad,
-			       set_fmt, NULL, &vformat);
+	ret = v4l2_subdev_call(input->camera, pad, set_fmt, NULL, &vformat);
 	if (ret)
 		return ret;
 
@@ -4216,6 +4213,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_format_bridge *format_bridge;
 	const struct atomisp_format_bridge *snr_format_bridge;
 	struct ia_css_frame_info output_info;
@@ -4259,8 +4257,7 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	vformat.format.height += padding_h;
 	vformat.format.width += padding_w;
 
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera, pad,
-			       set_fmt, NULL, &vformat);
+	ret = v4l2_subdev_call(input->camera, pad, set_fmt, NULL, &vformat);
 	if (ret)
 		return ret;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 6a86dc7a933f..fc3f9883914e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -703,14 +703,14 @@ static int atomisp_enum_framesizes(struct file *file, void *priv,
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	struct v4l2_subdev_frame_size_enum fse = {
 		.index = fsize->index,
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 	int ret;
 
-	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-			       pad, enum_frame_size, NULL, &fse);
+	ret = v4l2_subdev_call(input->camera, pad, enum_frame_size, NULL, &fse);
 	if (ret)
 		return ret;
 
-- 
2.40.1

[PATCH 13/21] media: atomisp: Add ia_css_frame_pad_width() helper function

[Hans de Goede]

Factor the code to go from width to a properly aligned pitch out of
ia_css_frame_info_set_width().

This is a preparation patch to fix try_fmt() calls returning a bogus
bytesperline value.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../runtime/frame/interface/ia_css_frame.h    |  2 +
 .../atomisp/pci/runtime/frame/src/frame.c     | 44 +++++++++++--------
 2 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/frame/interface/ia_css_frame.h b/drivers/staging/media/atomisp/pci/runtime/frame/interface/ia_css_frame.h
index 700070c58eda..90c17884968b 100644
--- a/drivers/staging/media/atomisp/pci/runtime/frame/interface/ia_css_frame.h
+++ b/drivers/staging/media/atomisp/pci/runtime/frame/interface/ia_css_frame.h
@@ -138,4 +138,6 @@ bool ia_css_frame_is_same_type(
 int ia_css_dma_configure_from_info(struct dma_port_config *config,
 				   const struct ia_css_frame_info *info);
 
+unsigned int ia_css_frame_pad_width(unsigned int width, enum ia_css_frame_format format);
+
 #endif /* __IA_CSS_FRAME_H__ */
diff --git a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
index 1e374ae848e3..1ccf13c01476 100644
--- a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
+++ b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
@@ -269,6 +269,29 @@ int ia_css_frame_init_planes(struct ia_css_frame *frame)
 	return 0;
 }
 
+unsigned int ia_css_frame_pad_width(unsigned int width, enum ia_css_frame_format format)
+{
+	/*
+	 * frames with a U and V plane of 8 bits per pixel need to have
+	 * all planes aligned, this means double the alignment for the
+	 * Y plane if the horizontal decimation is 2.
+	 */
+	if (format == IA_CSS_FRAME_FORMAT_YUV420 ||
+	    format == IA_CSS_FRAME_FORMAT_YV12 ||
+	    format == IA_CSS_FRAME_FORMAT_NV12 ||
+	    format == IA_CSS_FRAME_FORMAT_NV21 ||
+	    format == IA_CSS_FRAME_FORMAT_BINARY_8 ||
+	    format == IA_CSS_FRAME_FORMAT_YUV_LINE)
+		return CEIL_MUL(width, 2 * HIVE_ISP_DDR_WORD_BYTES);
+	else if (format == IA_CSS_FRAME_FORMAT_NV12_TILEY)
+		return CEIL_MUL(width, NV12_TILEY_TILE_WIDTH);
+	else if (format == IA_CSS_FRAME_FORMAT_RAW ||
+		 format == IA_CSS_FRAME_FORMAT_RAW_PACKED)
+		return CEIL_MUL(width, 2 * ISP_VEC_NELEMS);
+	else
+		return CEIL_MUL(width, HIVE_ISP_DDR_WORD_BYTES);
+}
+
 void ia_css_frame_info_set_width(struct ia_css_frame_info *info,
 				 unsigned int width,
 				 unsigned int min_padded_width)
@@ -285,25 +308,8 @@ void ia_css_frame_info_set_width(struct ia_css_frame_info *info,
 	align = max(min_padded_width, width);
 
 	info->res.width = width;
-	/* frames with a U and V plane of 8 bits per pixel need to have
-	   all planes aligned, this means double the alignment for the
-	   Y plane if the horizontal decimation is 2. */
-	if (info->format == IA_CSS_FRAME_FORMAT_YUV420 ||
-	    info->format == IA_CSS_FRAME_FORMAT_YV12 ||
-	    info->format == IA_CSS_FRAME_FORMAT_NV12 ||
-	    info->format == IA_CSS_FRAME_FORMAT_NV21 ||
-	    info->format == IA_CSS_FRAME_FORMAT_BINARY_8 ||
-	    info->format == IA_CSS_FRAME_FORMAT_YUV_LINE)
-		info->padded_width =
-		    CEIL_MUL(align, 2 * HIVE_ISP_DDR_WORD_BYTES);
-	else if (info->format == IA_CSS_FRAME_FORMAT_NV12_TILEY)
-		info->padded_width = CEIL_MUL(align, NV12_TILEY_TILE_WIDTH);
-	else if (info->format == IA_CSS_FRAME_FORMAT_RAW ||
-		 info->format == IA_CSS_FRAME_FORMAT_RAW_PACKED)
-		info->padded_width = CEIL_MUL(align, 2 * ISP_VEC_NELEMS);
-	else {
-		info->padded_width = CEIL_MUL(align, HIVE_ISP_DDR_WORD_BYTES);
-	}
+	info->padded_width = ia_css_frame_pad_width(align, info->format);
+
 	IA_CSS_LEAVE_PRIVATE("");
 }
 
-- 
2.40.1

Re: [PATCH 13/21] media: atomisp: Add ia_css_frame_pad_width() helper function

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Factor the code to go from width to a properly aligned pitch out of
> ia_css_frame_info_set_width().
>
> This is a preparation patch to fix try_fmt() calls returning a bogus
> bytesperline value.

...

> +       /*
> +        * frames with a U and V plane of 8 bits per pixel need to have

Frames

> +        * all planes aligned, this means double the alignment for the
> +        * Y plane if the horizontal decimation is 2.
> +        */
> +       if (format == IA_CSS_FRAME_FORMAT_YUV420 ||
> +           format == IA_CSS_FRAME_FORMAT_YV12 ||
> +           format == IA_CSS_FRAME_FORMAT_NV12 ||
> +           format == IA_CSS_FRAME_FORMAT_NV21 ||
> +           format == IA_CSS_FRAME_FORMAT_BINARY_8 ||
> +           format == IA_CSS_FRAME_FORMAT_YUV_LINE)
> +               return CEIL_MUL(width, 2 * HIVE_ISP_DDR_WORD_BYTES);

> +       else if (format == IA_CSS_FRAME_FORMAT_NV12_TILEY)
> +               return CEIL_MUL(width, NV12_TILEY_TILE_WIDTH);
> +       else if (format == IA_CSS_FRAME_FORMAT_RAW ||
> +                format == IA_CSS_FRAME_FORMAT_RAW_PACKED)
> +               return CEIL_MUL(width, 2 * ISP_VEC_NELEMS);
> +       else

All 'else':s can be dropped.

> +               return CEIL_MUL(width, HIVE_ISP_DDR_WORD_BYTES);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/21] media: atomisp: Add ia_css_frame_pad_width() helper function

[Hans de Goede]

Hi,

On 5/29/23 22:57, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Factor the code to go from width to a properly aligned pitch out of
>> ia_css_frame_info_set_width().
>>
>> This is a preparation patch to fix try_fmt() calls returning a bogus
>> bytesperline value.
> 
> ...
> 
>> +       /*
>> +        * frames with a U and V plane of 8 bits per pixel need to have
> 
> Frames
> 
>> +        * all planes aligned, this means double the alignment for the
>> +        * Y plane if the horizontal decimation is 2.
>> +        */
>> +       if (format == IA_CSS_FRAME_FORMAT_YUV420 ||
>> +           format == IA_CSS_FRAME_FORMAT_YV12 ||
>> +           format == IA_CSS_FRAME_FORMAT_NV12 ||
>> +           format == IA_CSS_FRAME_FORMAT_NV21 ||
>> +           format == IA_CSS_FRAME_FORMAT_BINARY_8 ||
>> +           format == IA_CSS_FRAME_FORMAT_YUV_LINE)
>> +               return CEIL_MUL(width, 2 * HIVE_ISP_DDR_WORD_BYTES);
> 
>> +       else if (format == IA_CSS_FRAME_FORMAT_NV12_TILEY)
>> +               return CEIL_MUL(width, NV12_TILEY_TILE_WIDTH);
>> +       else if (format == IA_CSS_FRAME_FORMAT_RAW ||
>> +                format == IA_CSS_FRAME_FORMAT_RAW_PACKED)
>> +               return CEIL_MUL(width, 2 * ISP_VEC_NELEMS);
>> +       else
> 
> All 'else':s can be dropped.

Actually the whole function is really just a single switch-case,
so I've switched to that since that is even better IMHO.

Regards,

Hans



> 
>> +               return CEIL_MUL(width, HIVE_ISP_DDR_WORD_BYTES);
>> +}
> 

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

[Hans de Goede]

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()
   with atomisp_fill_pix_format().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 168 ++++++++----------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |   4 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  67 +------
 3 files changed, 76 insertions(+), 163 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 243a789fef6a..9531baa26fe0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3657,11 +3657,44 @@ static void __atomisp_init_stream_info(u16 stream_index,
 	}
 }
 
-/* This function looks up the closest available resolution. */
-int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
+static void atomisp_fill_pix_format(struct v4l2_pix_format *f,
+				    u32 width, u32 height,
+				    const struct atomisp_format_bridge *br_fmt)
 {
-	struct atomisp_device *isp = video_get_drvdata(vdev);
-	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
+	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);
+
+	if (br_fmt->planar) {
+		f->bytesperline = width;
+		f->sizeimage = PAGE_ALIGN(height * DIV_ROUND_UP(br_fmt->depth * width, 8));
+	} else {
+		f->bytesperline = DIV_ROUND_UP(br_fmt->depth * width, 8);
+		f->sizeimage = PAGE_ALIGN(height * f->bytesperline);
+	}
+
+	if (f->field == V4L2_FIELD_ANY)
+		f->field = V4L2_FIELD_NONE;
+
+	/*
+	 * FIXME: do we need to setup this differently, depending on the
+	 * sensor or the pipeline?
+	 */
+	f->colorspace = V4L2_COLORSPACE_REC709;
+	f->ycbcr_enc = V4L2_YCBCR_ENC_709;
+	f->xfer_func = V4L2_XFER_FUNC_709;
+}
+
+/* This function looks up the closest available resolution. */
+int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
+		    const struct atomisp_format_bridge **fmt_ret,
+		    const struct atomisp_format_bridge **snr_fmt_ret)
+{
+	const struct atomisp_format_bridge *fmt, *snr_fmt;
+	struct atomisp_sub_device *asd = &isp->asd;
 	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
@@ -3670,33 +3703,29 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
-	const struct atomisp_format_bridge *fmt;
 	int ret;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (!input->camera)
 		return -EINVAL;
 
 	fmt = atomisp_get_format_bridge(f->pixelformat);
-	if (!fmt) {
-		dev_err(isp->dev, "unsupported pixelformat!\n");
-		fmt = atomisp_output_fmts;
+	/* Currently, raw formats are broken!!! */
+	if (!fmt || fmt->sh_fmt == IA_CSS_FRAME_FORMAT_RAW) {
+		f->pixelformat = V4L2_PIX_FMT_YUV420;
+
+		fmt = atomisp_get_format_bridge(f->pixelformat);
+		if (!fmt)
+			return -EINVAL;
 	}
 
-	if (f->width <= 0 || f->height <= 0)
-		return -EINVAL;
-
-	format.format.code = fmt->mbus_code;
-	format.format.width = f->width;
-	format.format.height = f->height;
-
-	__atomisp_init_stream_info(ATOMISP_INPUT_STREAM_GENERAL,
-				   (struct atomisp_input_stream_info *)format.format.reserved);
+	/*
+	 * atomisp_set_fmt() will set the sensor resolution to the requested
+	 * resolution + padding. Add padding here and remove it again after
+	 * the set_fmt call, like atomisp_set_fmt_to_snr() does.
+	 */
+	v4l2_fill_mbus_format(&format.format, f, fmt->mbus_code);
+	format.format.width += pad_w;
+	format.format.height += pad_h;
 
 	dev_dbg(isp->dev, "try_mbus_fmt: asking for %ux%u\n",
 		format.format.width, format.format.height);
@@ -3708,16 +3737,15 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 	dev_dbg(isp->dev, "try_mbus_fmt: got %ux%u\n",
 		format.format.width, format.format.height);
 
-	fmt = atomisp_get_format_bridge_from_mbus(format.format.code);
-	if (!fmt) {
+	snr_fmt = atomisp_get_format_bridge_from_mbus(format.format.code);
+	if (!snr_fmt) {
 		dev_err(isp->dev, "unknown sensor format 0x%8.8x\n",
 			format.format.code);
 		return -EINVAL;
 	}
 
-	f->pixelformat = fmt->pixelformat;
-	f->width = format.format.width;
-	f->height = format.format.height;
+	f->width = format.format.width - pad_w;
+	f->height = format.format.height - pad_h;
 
 	/*
 	 * If the format is jpeg or custom RAW, then the width and height will
@@ -3727,7 +3755,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 	 */
 	if (f->pixelformat == V4L2_PIX_FMT_JPEG ||
 	    f->pixelformat == V4L2_PIX_FMT_CUSTOM_M10MO_RAW)
-		return 0;
+		goto out_fill_pix_format;
 
 	/* app vs isp */
 	f->width = rounddown(clamp_t(u32, f->width, ATOM_ISP_MIN_WIDTH,
@@ -3735,6 +3763,15 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f)
 	f->height = rounddown(clamp_t(u32, f->height, ATOM_ISP_MIN_HEIGHT,
 				      ATOM_ISP_MAX_HEIGHT), ATOM_ISP_STEP_HEIGHT);
 
+out_fill_pix_format:
+	atomisp_fill_pix_format(f, f->width, f->height, fmt);
+
+	if (fmt_ret)
+		*fmt_ret = fmt;
+
+	if (snr_fmt_ret)
+		*snr_fmt_ret = snr_fmt;
+
 	return 0;
 }
 
@@ -3900,7 +3937,7 @@ static int css_input_resolution_changed(struct atomisp_sub_device *asd,
 
 static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 				  struct ia_css_frame_info *output_info,
-				  struct v4l2_pix_format *pix)
+				  const struct v4l2_pix_format *pix)
 {
 	struct camera_mipi_info *mipi_info;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
@@ -4213,16 +4250,12 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
-	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_format_bridge *format_bridge;
 	const struct atomisp_format_bridge *snr_format_bridge;
 	struct ia_css_frame_info output_info;
 	unsigned int dvs_env_w = 0, dvs_env_h = 0;
 	unsigned int padding_w = pad_w, padding_h = pad_h;
 	struct v4l2_mbus_framefmt isp_source_fmt = {0};
-	struct v4l2_subdev_format vformat = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
 	struct v4l2_rect isp_sink_crop;
 	int ret;
 
@@ -4234,41 +4267,13 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		"setting resolution %ux%u bytesperline %u\n",
 		f->fmt.pix.width, f->fmt.pix.height, f->fmt.pix.bytesperline);
 
-	format_bridge = atomisp_get_format_bridge(f->fmt.pix.pixelformat);
-	if (!format_bridge)
-		return -EINVAL;
-
-	/* Currently, raw formats are broken!!! */
-
-	if (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;
-	}
-	pipe->sh_fmt = format_bridge->sh_fmt;
-	pipe->pix.pixelformat = f->fmt.pix.pixelformat;
-
 	/* Ensure that the resolution is equal or below the maximum supported */
-
-	vformat.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-	v4l2_fill_mbus_format(&vformat.format, &f->fmt.pix, format_bridge->mbus_code);
-	vformat.format.height += padding_h;
-	vformat.format.width += padding_w;
-
-	ret = v4l2_subdev_call(input->camera, pad, set_fmt, NULL, &vformat);
+	ret = atomisp_try_fmt(isp, &f->fmt.pix, &format_bridge, &snr_format_bridge);
 	if (ret)
 		return ret;
 
-	f->fmt.pix.width = vformat.format.width - padding_w;
-	f->fmt.pix.height = vformat.format.height - padding_h;
-
-	snr_format_bridge = atomisp_get_format_bridge_from_mbus(vformat.format.code);
-	if (!snr_format_bridge) {
-		dev_warn(isp->dev, "Can't find bridge format\n");
-		return -EINVAL;
-	}
+	pipe->sh_fmt = format_bridge->sh_fmt;
+	pipe->pix.pixelformat = format_bridge->pixelformat;
 
 	atomisp_subdev_get_ffmt(&asd->subdev, NULL,
 				V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -4348,42 +4353,11 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 		return -EINVAL;
 	}
 
-	pipe->pix.width = f->fmt.pix.width;
-	pipe->pix.height = f->fmt.pix.height;
-	pipe->pix.pixelformat = f->fmt.pix.pixelformat;
-	/*
-	 * FIXME: do we need to setup this differently, depending on the
-	 * sensor or the pipeline?
-	 */
-	pipe->pix.colorspace = V4L2_COLORSPACE_REC709;
-	pipe->pix.ycbcr_enc = V4L2_YCBCR_ENC_709;
-	pipe->pix.xfer_func = V4L2_XFER_FUNC_709;
-
-	if (format_bridge->planar) {
-		pipe->pix.bytesperline = output_info.padded_width;
-		pipe->pix.sizeimage = PAGE_ALIGN(f->fmt.pix.height *
-						 DIV_ROUND_UP(format_bridge->depth *
-							 output_info.padded_width, 8));
-	} else {
-		pipe->pix.bytesperline =
-		    DIV_ROUND_UP(format_bridge->depth *
-				 output_info.padded_width, 8);
-		pipe->pix.sizeimage =
-		    PAGE_ALIGN(f->fmt.pix.height * pipe->pix.bytesperline);
-	}
-	dev_dbg(isp->dev, "%s: image size: %d, %d bytes per line\n",
-		__func__, pipe->pix.sizeimage, pipe->pix.bytesperline);
-
-	if (f->fmt.pix.field == V4L2_FIELD_ANY)
-		f->fmt.pix.field = V4L2_FIELD_NONE;
-	pipe->pix.field = f->fmt.pix.field;
+	atomisp_fill_pix_format(&pipe->pix, f->fmt.pix.width, f->fmt.pix.height, format_bridge);
 
 	f->fmt.pix = pipe->pix;
 	f->fmt.pix.priv = PAGE_ALIGN(pipe->pix.width *
 				     pipe->pix.height * 2);
-	/* Report the needed sizes */
-	f->fmt.pix.sizeimage = pipe->pix.sizeimage;
-	f->fmt.pix.bytesperline = pipe->pix.bytesperline;
 
 	dev_dbg(isp->dev, "%s: %dx%d, image size: %d, %d bytes per line\n",
 		__func__,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index ff2178c96fb8..c9a587b34e70 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -251,7 +251,9 @@ int atomisp_compare_grid(struct atomisp_sub_device *asd,
 			 struct atomisp_grid_info *atomgrid);
 
 /* This function looks up the closest available resolution. */
-int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f);
+int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
+		    const struct atomisp_format_bridge **fmt_ret,
+		    const struct atomisp_format_bridge **snr_fmt_ret);
 
 int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index fc3f9883914e..8ba1d11ae907 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;
+	struct atomisp_device *isp = video_get_drvdata(vdev);
 
-	/*
-	 * 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;
-
-	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,
-- 
2.40.1

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

[Andy Shevchenko]

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

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

[Andy Shevchenko]

On Tue, May 30, 2023 at 12:05 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:

A couple of additional comments (based on the branch contents).

...

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

than

> >    do this (duplication).

...

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

It's actually NIH BITS_TO_BYTES() from bitops.h.




--
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 5/30/23 13:58, Andy Shevchenko wrote:
> On Tue, May 30, 2023 at 12:05 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> A couple of additional comments (based on the branch contents).
> 
> ...
> 
>>> 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()
> 
> than
> 
>>>    do this (duplication).
> 
> ...
> 
>>   bytes = DIV_ROUND_UP(br_fmt->depth * width, BITS_PER_BYTE);
> 
> It's actually NIH BITS_TO_BYTES() from bitops.h.

Thank you, both are fixed in my media-atomisp branch now.

Regards,

Hans

[PATCH 15/21] media: atomisp: Add support for sensors which implement selection API / cropping

[Hans de Goede]

Sensor drivers which implement set_selection V4L2_SEL_TGT_CROP expect
v4l2_subdev_state.pads[pad].try_crop to have valid contents when calling
set_fmt with which == V4L2_SUBDEV_FORMAT_TRY since the crop-rectangle
may influence the available image size.

Just passing an uninitalized struct v4l2_subdev_pad_config from
the stack to set_fmt with which == V4L2_SUBDEV_FORMAT_TRY will result
in wrong results with such drivers.

Store a per sensor v4l2_subdev_pad_config and add a new
atomisp_init_sensor_crop() function to initialize this before
registering /dev/* nodes with userspace.

Sensor drivers which implement the selection API will allow
the atomisp to properly deal with the extra padding the ISP wants
on a per sensor basis instead of hardcoding this.
atomisp_init_sensor_crop() stores the native and active rects
of the sensor in preparation for using these for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   |  6 +-
 .../media/atomisp/pci/atomisp_internal.h      |  7 +++
 .../staging/media/atomisp/pci/atomisp_v4l2.c  | 55 +++++++++++++++++++
 3 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 9531baa26fe0..8c48d566131f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3696,9 +3696,8 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 	const struct atomisp_format_bridge *fmt, *snr_fmt;
 	struct atomisp_sub_device *asd = &isp->asd;
 	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
-	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg,
+		.pads = &input->pad_cfg,
 	};
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
@@ -4162,9 +4161,8 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	struct atomisp_device *isp = asd->isp;
 	struct atomisp_input_subdev *input = &isp->inputs[asd->input_curr];
 	const struct atomisp_format_bridge *format;
-	struct v4l2_subdev_pad_config pad_cfg;
 	struct v4l2_subdev_state pad_state = {
-		.pads = &pad_cfg,
+		.pads = &input->pad_cfg,
 	};
 	struct v4l2_subdev_format vformat = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index e59c0f1e7f53..14d21c6e227d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -125,7 +125,14 @@
 struct atomisp_input_subdev {
 	unsigned int type;
 	enum atomisp_camera_port port;
+	bool crop_support;
 	struct v4l2_subdev *camera;
+	/* Sensor rects for sensors which support crop */
+	struct v4l2_rect native_rect;
+	struct v4l2_rect active_rect;
+	/* Sensor pad_cfg for which == V4L2_SUBDEV_FORMAT_TRY calls */
+	struct v4l2_subdev_pad_config pad_cfg;
+
 	struct v4l2_subdev *motor;
 
 	/*
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index a2ce9bbf10df..506f04ca92b1 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -931,6 +931,59 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 	return ret;
 }
 
+static void atomisp_init_sensor(struct atomisp_input_subdev *input)
+{
+	struct v4l2_subdev_state sd_state = {
+		.pads = &input->pad_cfg,
+	};
+	struct v4l2_subdev_selection sel = { };
+	int err;
+
+	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sel.target = V4L2_SEL_TGT_NATIVE_SIZE;
+	err = v4l2_subdev_call(input->camera, pad, get_selection, NULL, &sel);
+	if (err)
+		return;
+
+	input->native_rect = sel.r;
+
+	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sel.target = V4L2_SEL_TGT_CROP_DEFAULT;
+	err = v4l2_subdev_call(input->camera, pad, get_selection, NULL, &sel);
+	if (err)
+		return;
+
+	input->active_rect = sel.r;
+
+	/*
+	 * The ISP also wants the non-active pixels at the border of the sensor
+	 * for padding, set the crop rect to cover the entire sensor instead
+	 * of only the default active area.
+	 *
+	 * Do this for both try and active formats since the try_crop rect in
+	 * pad_cfg may influence (clamp) future try_fmt calls with which == try.
+	 */
+	sel.which = V4L2_SUBDEV_FORMAT_TRY;
+	sel.target = V4L2_SEL_TGT_CROP;
+	sel.r = input->native_rect;
+	err = v4l2_subdev_call(input->camera, pad, set_selection, &sd_state, &sel);
+	if (err)
+		return;
+
+	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	sel.target = V4L2_SEL_TGT_CROP;
+	sel.r = input->native_rect;
+	err = v4l2_subdev_call(input->camera, pad, set_selection, NULL, &sel);
+	if (err)
+		return;
+
+	dev_info(input->camera->dev, "Supports crop native %dx%d active %dx%d\n",
+		 input->native_rect.width, input->native_rect.height,
+		 input->active_rect.width, input->active_rect.height);
+
+	input->crop_support = true;
+}
+
 int atomisp_register_device_nodes(struct atomisp_device *isp)
 {
 	struct atomisp_input_subdev *input;
@@ -952,6 +1005,8 @@ int atomisp_register_device_nodes(struct atomisp_device *isp)
 		input->port = i;
 		input->camera = isp->sensor_subdevs[i];
 
+		atomisp_init_sensor(input);
+
 		/*
 		 * HACK: Currently VCM belongs to primary sensor only, but correct
 		 * approach must be to acquire from platform code which sensor
-- 
2.40.1

[PATCH 16/21] media: atomisp: Pass MEDIA_BUS_FMT_* code when calling enum_frame_size pad-op

[Hans de Goede]

A sensor driver's enum_frame_size pad-op may return -EINVAL when
v4l2_subdev_frame_size_enum.code is not set to a supported
MEDIA_BUS_FMT_* code.

Make atomisp_init_sensor() get the sensor's MEDIA_BUS_FMT_* code and
pass this when calling the enum_frame_size pad-op.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_internal.h | 1 +
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c    | 1 +
 drivers/staging/media/atomisp/pci/atomisp_v4l2.c     | 6 ++++++
 3 files changed, 8 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index 14d21c6e227d..9fded17a2c71 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -125,6 +125,7 @@
 struct atomisp_input_subdev {
 	unsigned int type;
 	enum atomisp_camera_port port;
+	u32 code; /* MEDIA_BUS_FMT_* */
 	bool crop_support;
 	struct v4l2_subdev *camera;
 	/* Sensor rects for sensors which support crop */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 8ba1d11ae907..980465fd5a83 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -707,6 +707,7 @@ static int atomisp_enum_framesizes(struct file *file, void *priv,
 	struct v4l2_subdev_frame_size_enum fse = {
 		.index = fsize->index,
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+		.code = input->code,
 	};
 	int ret;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 506f04ca92b1..3a2e15605919 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -933,12 +933,18 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 
 static void atomisp_init_sensor(struct atomisp_input_subdev *input)
 {
+	struct v4l2_subdev_mbus_code_enum mbus_code_enum = { };
 	struct v4l2_subdev_state sd_state = {
 		.pads = &input->pad_cfg,
 	};
 	struct v4l2_subdev_selection sel = { };
 	int err;
 
+	mbus_code_enum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+	err = v4l2_subdev_call(input->camera, pad, enum_mbus_code, NULL, &mbus_code_enum);
+	if (!err)
+		input->code = mbus_code_enum.code;
+
 	sel.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	sel.target = V4L2_SEL_TGT_NATIVE_SIZE;
 	err = v4l2_subdev_call(input->camera, pad, get_selection, NULL, &sel);
-- 
2.40.1

[PATCH 17/21] media: atomisp: Make atomisp_init_sensor() check if the sensor supports binning

[Hans de Goede]

Make atomisp_init_sensor() check if the sensor supports binning.

This is a preparation patch for using the selection / crop support
to determine the padding values to use with a specific sensor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_internal.h      |  1 +
 .../staging/media/atomisp/pci/atomisp_v4l2.c  | 30 +++++++++++++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index 9fded17a2c71..f7b4bee9574b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -126,6 +126,7 @@ struct atomisp_input_subdev {
 	unsigned int type;
 	enum atomisp_camera_port port;
 	u32 code; /* MEDIA_BUS_FMT_* */
+	bool binning_support;
 	bool crop_support;
 	struct v4l2_subdev *camera;
 	/* Sensor rects for sensors which support crop */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 3a2e15605919..c43b916a006e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -934,11 +934,12 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 static void atomisp_init_sensor(struct atomisp_input_subdev *input)
 {
 	struct v4l2_subdev_mbus_code_enum mbus_code_enum = { };
+	struct v4l2_subdev_frame_size_enum fse = { };
 	struct v4l2_subdev_state sd_state = {
 		.pads = &input->pad_cfg,
 	};
 	struct v4l2_subdev_selection sel = { };
-	int err;
+	int i, err;
 
 	mbus_code_enum.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	err = v4l2_subdev_call(input->camera, pad, enum_mbus_code, NULL, &mbus_code_enum);
@@ -961,6 +962,28 @@ static void atomisp_init_sensor(struct atomisp_input_subdev *input)
 
 	input->active_rect = sel.r;
 
+	/*
+	 * Check for a framesize with half active_rect width and height,
+	 * if found assume the sensor supports binning.
+	 * Do this before changing the crop-rect since that may influence
+	 * enum_frame_size results.
+	 */
+	for (i = 0; ; i++) {
+		fse.index = i;
+		fse.code = input->code;
+		fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
+
+		err = v4l2_subdev_call(input->camera, pad, enum_frame_size, NULL, &fse);
+		if (err)
+			break;
+
+		if (fse.min_width <= (input->active_rect.width / 2) &&
+		    fse.min_height <= (input->active_rect.height / 2)) {
+			input->binning_support = true;
+			break;
+		}
+	}
+
 	/*
 	 * The ISP also wants the non-active pixels at the border of the sensor
 	 * for padding, set the crop rect to cover the entire sensor instead
@@ -983,9 +1006,10 @@ static void atomisp_init_sensor(struct atomisp_input_subdev *input)
 	if (err)
 		return;
 
-	dev_info(input->camera->dev, "Supports crop native %dx%d active %dx%d\n",
+	dev_info(input->camera->dev, "Supports crop native %dx%d active %dx%d binning %d\n",
 		 input->native_rect.width, input->native_rect.height,
-		 input->active_rect.width, input->active_rect.height);
+		 input->active_rect.width, input->active_rect.height,
+		 input->binning_support);
 
 	input->crop_support = true;
 }
-- 
2.40.1

[PATCH 18/21] media: atomisp: Use selection API info to determine sensor padding

[Hans de Goede]

Using the selection / crop info to determine the padding values
to use with a specific resolution on specific sensor.

This allows e.g. automatically halving the padding when using
the max binned resolution and also ensures the right amount
of padding is used on models with 2 sensors with different
padding requirements.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 57 ++++++++++++++-----
 .../media/atomisp/pci/atomisp_subdev.c        | 14 +----
 .../media/atomisp/pci/atomisp_subdev.h        |  3 +
 3 files changed, 48 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 8c48d566131f..ce16850170c4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3688,6 +3688,33 @@ static void atomisp_fill_pix_format(struct v4l2_pix_format *f,
 	f->xfer_func = V4L2_XFER_FUNC_709;
 }
 
+/* Get sensor padding values for the non padded width x height resolution */
+static void atomisp_get_padding(struct atomisp_device *isp,
+				u32 width, u32 height,
+				u32 *padding_w, u32 *padding_h)
+{
+	struct atomisp_input_subdev *input = &isp->inputs[isp->asd.input_curr];
+	struct v4l2_rect native_rect = input->native_rect;
+
+	if (!input->crop_support) {
+		*padding_w = pad_w;
+		*padding_h = pad_h;
+		return;
+	}
+
+	width = min(width, input->active_rect.width);
+	height = min(height, input->active_rect.height);
+
+	if (input->binning_support && width <= (input->active_rect.width / 2) &&
+				      height <= (input->active_rect.height / 2)) {
+		native_rect.width /= 2;
+		native_rect.height /= 2;
+	}
+
+	*padding_w = min_t(u32, (native_rect.width - width) & ~1, pad_w);
+	*padding_h = min_t(u32, (native_rect.height - height) & ~1, pad_h);
+}
+
 /* This function looks up the closest available resolution. */
 int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 		    const struct atomisp_format_bridge **fmt_ret,
@@ -3702,6 +3729,7 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 	struct v4l2_subdev_format format = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 	};
+	u32 padding_w, padding_h;
 	int ret;
 
 	if (!input->camera)
@@ -3722,9 +3750,10 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 	 * resolution + padding. Add padding here and remove it again after
 	 * the set_fmt call, like atomisp_set_fmt_to_snr() does.
 	 */
+	atomisp_get_padding(isp, f->width, f->height, &padding_w, &padding_h);
 	v4l2_fill_mbus_format(&format.format, f, fmt->mbus_code);
-	format.format.width += pad_w;
-	format.format.height += pad_h;
+	format.format.width += padding_w;
+	format.format.height += padding_h;
 
 	dev_dbg(isp->dev, "try_mbus_fmt: asking for %ux%u\n",
 		format.format.width, format.format.height);
@@ -3743,8 +3772,8 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 		return -EINVAL;
 	}
 
-	f->width = format.format.width - pad_w;
-	f->height = format.format.height - pad_h;
+	f->width = format.format.width - padding_w;
+	f->height = format.format.height - padding_h;
 
 	/*
 	 * If the format is jpeg or custom RAW, then the width and height will
@@ -4153,7 +4182,6 @@ static void atomisp_check_copy_mode(struct atomisp_sub_device *asd,
 }
 
 static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_pix_format *f,
-				  unsigned int padding_w, unsigned int padding_h,
 				  unsigned int dvs_env_w, unsigned int dvs_env_h)
 {
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
@@ -4184,11 +4212,11 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 		return -EINVAL;
 
 	v4l2_fill_mbus_format(ffmt, f, format->mbus_code);
-	ffmt->height += padding_h + dvs_env_h;
-	ffmt->width += padding_w + dvs_env_w;
+	ffmt->height += asd->sink_pad_padding_h + dvs_env_h;
+	ffmt->width += asd->sink_pad_padding_w + dvs_env_w;
 
 	dev_dbg(isp->dev, "s_mbus_fmt: ask %ux%u (padding %ux%u, dvs %ux%u)\n",
-		ffmt->width, ffmt->height, padding_w, padding_h,
+		ffmt->width, ffmt->height, asd->sink_pad_padding_w, asd->sink_pad_padding_h,
 		dvs_env_w, dvs_env_h);
 
 	__atomisp_init_stream_info(ATOMISP_INPUT_STREAM_GENERAL, stream_info);
@@ -4252,7 +4280,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	const struct atomisp_format_bridge *snr_format_bridge;
 	struct ia_css_frame_info output_info;
 	unsigned int dvs_env_w = 0, dvs_env_h = 0;
-	unsigned int padding_w = pad_w, padding_h = pad_h;
 	struct v4l2_mbus_framefmt isp_source_fmt = {0};
 	struct v4l2_rect isp_sink_crop;
 	int ret;
@@ -4283,16 +4310,18 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 				V4L2_SUBDEV_FORMAT_ACTIVE,
 				ATOMISP_SUBDEV_PAD_SOURCE, &isp_source_fmt);
 
-	if (!atomisp_subdev_format_conversion(asd)) {
-		padding_w = 0;
-		padding_h = 0;
+	if (atomisp_subdev_format_conversion(asd)) {
+		atomisp_get_padding(isp, f->fmt.pix.width, f->fmt.pix.height,
+				    &asd->sink_pad_padding_w, &asd->sink_pad_padding_h);
+	} else {
+		asd->sink_pad_padding_w = 0;
+		asd->sink_pad_padding_h = 0;
 	}
 
 	atomisp_get_dis_envelop(asd, f->fmt.pix.width, f->fmt.pix.height,
 				&dvs_env_w, &dvs_env_h);
 
-	ret = atomisp_set_fmt_to_snr(vdev, &f->fmt.pix,
-				     padding_w, padding_h, dvs_env_w, dvs_env_h);
+	ret = atomisp_set_fmt_to_snr(vdev, &f->fmt.pix, dvs_env_w, dvs_env_h);
 	if (ret) {
 		dev_warn(isp->dev,
 			 "Set format to sensor failed with %d\n", ret);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 04e257ede7d4..45073e401bac 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -357,8 +357,6 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *ffmt[ATOMISP_SUBDEV_PADS_NUM];
 	struct v4l2_rect *crop[ATOMISP_SUBDEV_PADS_NUM],
 		       *comp[ATOMISP_SUBDEV_PADS_NUM];
-	unsigned int padding_w = pad_w;
-	unsigned int padding_h = pad_h;
 
 	if ((pad == ATOMISP_SUBDEV_PAD_SINK && target != V4L2_SEL_TGT_CROP) ||
 	    (pad == ATOMISP_SUBDEV_PAD_SOURCE && target != V4L2_SEL_TGT_COMPOSE))
@@ -384,18 +382,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		crop[pad]->width = ffmt[pad]->width;
 		crop[pad]->height = ffmt[pad]->height;
 
-		/* Workaround for BYT 1080p perfectshot since the maxinum resolution of
-		 * front camera ov2722 is 1932x1092 and cannot use pad_w > 12*/
-		if (!strncmp(isp->inputs[isp_sd->input_curr].camera->name,
-			     "ov2722", 6) && crop[pad]->height == 1092) {
-			padding_w = 12;
-			padding_h = 12;
-		}
-
 		if (atomisp_subdev_format_conversion(isp_sd)
 		    && crop[pad]->width && crop[pad]->height) {
-			crop[pad]->width -= padding_w;
-			crop[pad]->height -= padding_h;
+			crop[pad]->width -= isp_sd->sink_pad_padding_w;
+			crop[pad]->height -= isp_sd->sink_pad_padding_h;
 		}
 
 		if (isp_sd->params.video_dis_en &&
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index c9f6561dbcb6..9a04511b9efd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -239,6 +239,9 @@ struct atomisp_sub_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[ATOMISP_SUBDEV_PADS_NUM];
 	struct atomisp_pad_format fmt[ATOMISP_SUBDEV_PADS_NUM];
+	/* Padding for currently set sink-pad fmt */
+	u32 sink_pad_padding_w;
+	u32 sink_pad_padding_h;
 
 	unsigned int output;
 	struct atomisp_video_pipe video_out;
-- 
2.40.1

[PATCH 19/21] media: atomisp: Set crop before setting fmt

[Hans de Goede]

Some drivers which implement selections/crop only allow setting the format
with and height to either the crop rectangle width and height or to half
the crop rectangle width and height (binning). An example of such
a driver is the standard v4l2 ov5693 driver.

First set the crop rectangle to match the requested format
when trying or setting the sensor format, to match these drivers
expectations.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index ce16850170c4..9dfc4bda24f5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3715,6 +3715,47 @@ static void atomisp_get_padding(struct atomisp_device *isp,
 	*padding_h = min_t(u32, (native_rect.height - height) & ~1, pad_h);
 }
 
+static int atomisp_set_crop(struct atomisp_device *isp,
+			    const struct v4l2_mbus_framefmt *format,
+			    int which)
+{
+	struct atomisp_input_subdev *input = &isp->inputs[isp->asd.input_curr];
+	struct v4l2_subdev_state pad_state = {
+		.pads = &input->pad_cfg,
+	};
+	struct v4l2_subdev_selection sel = {
+		.which = which,
+		.target = V4L2_SEL_TGT_CROP,
+		.r.width = format->width,
+		.r.height = format->height,
+	};
+	int ret;
+
+	if (!input->crop_support)
+		return 0;
+
+	/* Cropping is done before binning, when binning double the crop rect */
+	if (input->binning_support && sel.r.width <= (input->active_rect.width / 2) &&
+				      sel.r.height <= (input->active_rect.height / 2)) {
+		sel.r.width *= 2;
+		sel.r.height *= 2;
+	}
+
+	/* Clamp to avoid top/left calculations overflowing */
+	sel.r.width = min(sel.r.width, input->native_rect.width);
+	sel.r.height = min(sel.r.height, input->native_rect.height);
+
+	sel.r.left = ((input->native_rect.width - sel.r.width) / 2) & ~1;
+	sel.r.top = ((input->native_rect.height - sel.r.height) / 2) & ~1;
+
+	ret = v4l2_subdev_call(input->camera, pad, set_selection, &pad_state, &sel);
+	if (ret)
+		dev_err(isp->dev, "Error setting crop to %ux%u @%ux%u: %d\n",
+			sel.r.width, sel.r.height, sel.r.left, sel.r.top, ret);
+
+	return ret;
+}
+
 /* This function looks up the closest available resolution. */
 int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 		    const struct atomisp_format_bridge **fmt_ret,
@@ -3758,6 +3799,10 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 	dev_dbg(isp->dev, "try_mbus_fmt: asking for %ux%u\n",
 		format.format.width, format.format.height);
 
+	ret = atomisp_set_crop(isp, &format.format, V4L2_SUBDEV_FORMAT_TRY);
+	if (ret)
+		return ret;
+
 	ret = v4l2_subdev_call(input->camera, pad, set_fmt, &pad_state, &format);
 	if (ret)
 		return ret;
@@ -4225,6 +4270,10 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 
 	/* Disable dvs if resolution can't be supported by sensor */
 	if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
+		ret = atomisp_set_crop(isp, &vformat.format, V4L2_SUBDEV_FORMAT_TRY);
+		if (ret)
+			return ret;
+
 		vformat.which = V4L2_SUBDEV_FORMAT_TRY;
 		ret = v4l2_subdev_call(input->camera, pad, set_fmt, &pad_state, &vformat);
 		if (ret)
@@ -4243,6 +4292,11 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 			asd->params.video_dis_en = false;
 		}
 	}
+
+	ret = atomisp_set_crop(isp, &vformat.format, V4L2_SUBDEV_FORMAT_ACTIVE);
+	if (ret)
+		return ret;
+
 	vformat.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	ret = v4l2_subdev_call(input->camera, pad, set_fmt, NULL, &vformat);
 	if (ret)
-- 
2.40.1

[PATCH 20/21] media: atomisp: Add enum_framesizes function for sensors with selection / crop support

[Hans de Goede]

Some sensor drivers with crop support (e.g. the ov5693 driver) only
return the current crop rectangle + 1/2 (binning) of the current crop
rectangle when calling their enum_frame_sizes op.

This causes 2 issues:
1. Atomisp sets to the crop area to include the padding, where as
   the enum_framesizes ioctl should return values without padding.

2. With cropping a lot more standard resolutions are possible then
   just these 2 and many apps limit the list given to the end user
   to the list returned by enum_framesizes.

Add an alternative enum_framesizes function for sensors which support
cropping to fix both issues.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/TODO            |  3 -
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 69 +++++++++++++++++++
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/TODO b/drivers/staging/media/atomisp/TODO
index 5e7bb6eb351a..13b4a9015a7b 100644
--- a/drivers/staging/media/atomisp/TODO
+++ b/drivers/staging/media/atomisp/TODO
@@ -28,9 +28,6 @@ TODO
 * The atomisp ov2680 and ov5693 sensor drivers bind to the same hw-ids as
   the standard ov2680 and ov5693 drivers under drivers/media/i2c, which
   conflicts. Drop the atomisp private ov2680 and ov5693 drivers:
-  * Make atomisp code use v4l2 selections to deal with the extra padding
-    it wants to receive from sensors instead of relying on the ov2680 code
-    sending e.g. 1616x1216 for a 1600x1200 mode
   * Port various ov2680 improvements from atomisp_ov2680.c to regular ov2680.c
     and switch to regular ov2680 driver
   * Make atomisp work with the regular ov5693 driver and drop atomisp_ov5693
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 980465fd5a83..196ef250aedd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -697,6 +697,72 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	return 0;
 }
 
+/*
+ * With crop any framesize <= sensor-size can be made, give
+ * userspace a list of sizes to choice from.
+ */
+static int atomisp_enum_framesizes_crop_inner(struct atomisp_device *isp,
+					      struct v4l2_frmsizeenum *fsize,
+					      struct v4l2_rect *active,
+					      int *valid_sizes)
+{
+	static const struct v4l2_frmsize_discrete frame_sizes[] = {
+		{ 1600, 1200 },
+		{ 1600, 1080 },
+		{ 1600,  900 },
+		{ 1440, 1080 },
+		{ 1280,  960 },
+		{ 1280,  720 },
+		{  800,  600 },
+		{  640,  480 },
+	};
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(frame_sizes); i++) {
+		if (frame_sizes[i].width > active->width ||
+		    frame_sizes[i].height > active->height)
+			continue;
+
+		/*
+		 * Skip sizes where width and height are less then 2/3th of the
+		 * sensor size to avoid sizes with a too small field of view.
+		 */
+		if (frame_sizes[i].width < (active->width * 2 / 3) &&
+		    frame_sizes[i].height < (active->height * 2 / 3))
+			continue;
+
+		if (*valid_sizes == fsize->index) {
+			fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
+			fsize->discrete = frame_sizes[i];
+			return 0;
+		}
+
+		(*valid_sizes)++;
+	}
+
+	return -EINVAL;
+}
+
+static int atomisp_enum_framesizes_crop(struct atomisp_device *isp,
+					struct v4l2_frmsizeenum *fsize)
+{
+	struct atomisp_input_subdev *input = &isp->inputs[isp->asd.input_curr];
+	struct v4l2_rect active = input->active_rect;
+	int ret, valid_sizes = 0;
+
+	ret = atomisp_enum_framesizes_crop_inner(isp, fsize, &active, &valid_sizes);
+	if (ret == 0)
+		return 0;
+
+	if (!input->binning_support)
+		return -EINVAL;
+
+	active.width /= 2;
+	active.height /= 2;
+
+	return atomisp_enum_framesizes_crop_inner(isp, fsize, &active, &valid_sizes);
+}
+
 static int atomisp_enum_framesizes(struct file *file, void *priv,
 				   struct v4l2_frmsizeenum *fsize)
 {
@@ -711,6 +777,9 @@ static int atomisp_enum_framesizes(struct file *file, void *priv,
 	};
 	int ret;
 
+	if (input->crop_support)
+		return atomisp_enum_framesizes_crop(isp, fsize);
+
 	ret = v4l2_subdev_call(input->camera, pad, enum_frame_size, NULL, &fse);
 	if (ret)
 		return ret;
-- 
2.40.1

[PATCH 21/21] media: atomisp: csi2-bridge: Set PMC clk-rate for sensors to 19.2 MHz

[Hans de Goede]

The ACPI code takes care of enabling/disabling the PMC clk(s) for
the sensors as necessary based on the runtime-pm state of the sensor.

But the GMIN code this replaces also set the clk-rate of the PMC clk
to 19.2 MHz. At least on BYT devices the PMC clks may come up running
at 25 MHz instead of the expected 19.2 MHz.

Ensure the sensor clk also runs at the expected 19.2 MHz for sensors
using v4l2-async probing by explicitly setting it to 19.2 MHz when
enumerating sensors in atomisp_csi2_bridge.c.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 68 ++++++++++++++++---
 1 file changed, 60 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index b55a7ff9844e..28d8779bbbc4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/property.h>
@@ -38,6 +39,8 @@
 		.properties = _PROPS,		\
 	})
 
+#define PMC_CLK_RATE_19_2MHZ			19200000
+
 /*
  * 79234640-9e10-4fea-a5c1-b5aa8b19756f
  * This _DSM GUID returns information about the GPIO lines mapped to a sensor.
@@ -250,24 +253,61 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
 	}
 
 	ACPI_FREE(buffer.pointer);
+
+	if (ret < 0)
+		acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n");
+
 	return ret;
 }
 
-static int atomisp_csi2_get_port(struct acpi_device *adev)
+static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num)
 {
-	int clock_num, port;
+	struct clk *clk;
+	char name[14];
+	int ret;
+
+	if (clock_num < 0)
+		return 0;
+
+	snprintf(name, sizeof(name), "pmc_plt_clk_%d", clock_num);
+
+	clk = clk_get(NULL, name);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret);
+		return ret;
+	}
 
 	/*
-	 * Get PMC-clock number from ACPI _PR0 method and compare this to
-	 * the CsiPort 1 PMC-clock used in the CHT/BYT reference designs.
+	 * The firmware might enable the clock at boot, to change
+	 * the rate we must ensure the clock is disabled.
+	 */
+	ret = clk_prepare_enable(clk);
+	if (!ret)
+		clk_disable_unprepare(clk);
+	if (!ret)
+		ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ);
+	if (ret)
+		acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret);
+
+	clk_put(clk);
+	return ret;
+}
+
+static int atomisp_csi2_get_port(struct acpi_device *adev, int clock_num)
+{
+	int port;
+
+	/*
+	 * Compare clock-number to the PMC-clock used for CsiPort 1
+	 * in the CHT/BYT reference designs.
 	 */
-	clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
 	if (IS_ISP2401)
 		port = clock_num == 4 ? 1 : 0;
 	else
 		port = clock_num == 0 ? 1 : 0;
 
-	/* Intel DSM or DMI quirk overrides PR0 derived default */
+	/* Intel DSM or DMI quirk overrides _PR0 CLK derived default */
 	return gmin_cfg_get_int(adev, "CsiPort", port);
 }
 
@@ -551,7 +591,7 @@ static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *
 	struct fwnode_handle *fwnode, *primary;
 	struct atomisp_csi2_sensor *sensor;
 	struct acpi_device *adev;
-	int ret;
+	int ret, clock_num;
 
 	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
 		if (!adev->status.enabled)
@@ -565,7 +605,19 @@ static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *
 
 		sensor = &bridge->sensors[bridge->n_sensors];
 
-		sensor->port = atomisp_csi2_get_port(adev);
+		/*
+		 * ACPI takes care of turning the PMC clock on and off, but on BYT
+		 * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
+		 * Get the PMC-clock number from ACPI _PR0 method and set it to 19.2 MHz.
+		 * The PMC-clock number is also used to determine the default CSI port.
+		 */
+		clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
+
+		ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
+		if (ret)
+			goto err_put_adev;
+
+		sensor->port = atomisp_csi2_get_port(adev, clock_num);
 		if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
 			acpi_handle_err(adev->handle, "Invalid port: %d\n", sensor->port);
 			ret = -EINVAL;
-- 
2.40.1

Re: [PATCH 21/21] media: atomisp: csi2-bridge: Set PMC clk-rate for sensors to 19.2 MHz

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The ACPI code takes care of enabling/disabling the PMC clk(s) for
> the sensors as necessary based on the runtime-pm state of the sensor.
>
> But the GMIN code this replaces also set the clk-rate of the PMC clk
> to 19.2 MHz. At least on BYT devices the PMC clks may come up running
> at 25 MHz instead of the expected 19.2 MHz.
>
> Ensure the sensor clk also runs at the expected 19.2 MHz for sensors
> using v4l2-async probing by explicitly setting it to 19.2 MHz when
> enumerating sensors in atomisp_csi2_bridge.c.

...

> +       ret = clk_prepare_enable(clk);
> +       if (!ret)
> +               clk_disable_unprepare(clk);

I'm wondering if _enable / _disable required.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 21/21] media: atomisp: csi2-bridge: Set PMC clk-rate for sensors to 19.2 MHz

[Hans de Goede]

Hi,

On 5/29/23 23:48, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 1:39 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The ACPI code takes care of enabling/disabling the PMC clk(s) for
>> the sensors as necessary based on the runtime-pm state of the sensor.
>>
>> But the GMIN code this replaces also set the clk-rate of the PMC clk
>> to 19.2 MHz. At least on BYT devices the PMC clks may come up running
>> at 25 MHz instead of the expected 19.2 MHz.
>>
>> Ensure the sensor clk also runs at the expected 19.2 MHz for sensors
>> using v4l2-async probing by explicitly setting it to 19.2 MHz when
>> enumerating sensors in atomisp_csi2_bridge.c.
> 
> ...
> 
>> +       ret = clk_prepare_enable(clk);
>> +       if (!ret)
>> +               clk_disable_unprepare(clk);
> 
> I'm wondering if _enable / _disable required.


As the comment says the BIOS may have the clock enabled
at boot, the hw won't allow changing the rate while
the clk is enabled and the clk-framework won't
allow calling clk_disable_unprepare(clk) without
first calling clk_prepare_enable().

All the sound/soc/intel/boards/*.c files which are
used on BYT / CHT do the same thing before setting
the codec clk speed.

Regards,

Hans

Re: [PATCH 21/21] media: atomisp: csi2-bridge: Set PMC clk-rate for sensors to 19.2 MHz

[Andy Shevchenko]

On Tue, May 30, 2023 at 1:28 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/29/23 23:48, Andy Shevchenko wrote:
> > On Mon, May 29, 2023 at 1:39 PM Hans de Goede <hdegoede@redhat.com> wrote:

...

> >> +       ret = clk_prepare_enable(clk);
> >> +       if (!ret)
> >> +               clk_disable_unprepare(clk);
> >
> > I'm wondering if _enable / _disable required.
>
> As the comment says the BIOS may have the clock enabled
> at boot, the hw won't allow changing the rate while
> the clk is enabled and the clk-framework won't
> allow calling clk_disable_unprepare(clk) without
> first calling clk_prepare_enable().
>
> All the sound/soc/intel/boards/*.c files which are
> used on BYT / CHT do the same thing before setting
> the codec clk speed.

Interesting... It might be that x86 (under drivers/clk/x86 or
somewhere there) can gain a common helper to do this trick, so we
won't repeat this over and over again.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/21] media: atomisp: Use selection API info to determine sensor padding

[Andy Shevchenko]

On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is my next round of atomisp work.
>
> The atomisp wants some extra padding for processing in the data it receives
> from the sensor. E.g. For 1600x1200 it wants to receive 1616x1216 from
> the sensor. Currently the private sensor driver copies it uses give it
> e.g. 1616x1216 and the ISP2 code then substracts 16 before reporting
> the resolution to userspace.
>
> This patch series adds support for the v4l2 selections API and specifically
> the crop target so that atomisp can request the extra padding from standard
> v4l2 sensor drivers. This is implemented / tested with the atomisp_ov2680
> driver.
>
> Besides that there is the usual cleanups / prep work.
>
> With the padding solved, the last bit of private atomisp sensor API is
> gone now. So we can start working on getting rid of its private sensor
> driver copies.
>
> As mentioned in the updated TODO file the next step is to port
> various improvements from the atomisp_ov2680 private sensor driver
> to the generic ov2680 sensor driver (such as the selections support)
> and then switch to the generic ov2680 sensor driver.

For non-commented
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

If others will be addressed in the suggested way, feel free to add the
tag as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/21] media: atomisp: Use selection API info to determine sensor padding

[Hans de Goede]

Hi Andy,

On 5/29/23 23:59, Andy Shevchenko wrote:
> On Mon, May 29, 2023 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is my next round of atomisp work.
>>
>> The atomisp wants some extra padding for processing in the data it receives
>> from the sensor. E.g. For 1600x1200 it wants to receive 1616x1216 from
>> the sensor. Currently the private sensor driver copies it uses give it
>> e.g. 1616x1216 and the ISP2 code then substracts 16 before reporting
>> the resolution to userspace.
>>
>> This patch series adds support for the v4l2 selections API and specifically
>> the crop target so that atomisp can request the extra padding from standard
>> v4l2 sensor drivers. This is implemented / tested with the atomisp_ov2680
>> driver.
>>
>> Besides that there is the usual cleanups / prep work.
>>
>> With the padding solved, the last bit of private atomisp sensor API is
>> gone now. So we can start working on getting rid of its private sensor
>> driver copies.
>>
>> As mentioned in the updated TODO file the next step is to port
>> various improvements from the atomisp_ov2680 private sensor driver
>> to the generic ov2680 sensor driver (such as the selections support)
>> and then switch to the generic ov2680 sensor driver.
> 
> For non-commented
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> If others will be addressed in the suggested way, feel free to add the
> tag as well.

Thank you for all the reviews. I've pushed this to my media-atomisp
branch now with all remarks addressed:

https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

Regards,

Hans