[PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[Hans de Goede]

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The acpi_evaluate_dsm_typed() provides a way to check the type of the
object evaluated by _DSM call. Use it instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Link: https://lore.kernel.org/r/20220730155905.90091-1-andriy.shevchenko@linux.intel.com
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 133d7d56fff2..648469de2cdc 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1250,16 +1250,14 @@ static int gmin_get_config_dsm_var(struct device *dev,
 	if (!strcmp(var, "CamClk"))
 		return -EINVAL;
 
-	obj = acpi_evaluate_dsm(handle, &atomisp_dsm_guid, 0, 0, NULL);
+	/* Return on unexpected object type */
+	obj = acpi_evaluate_dsm_typed(handle, &atomisp_dsm_guid, 0, 0, NULL,
+				      ACPI_TYPE_PACKAGE);
 	if (!obj) {
 		dev_info_once(dev, "Didn't find ACPI _DSM table.\n");
 		return -EINVAL;
 	}
 
-	/* Return on unexpected object type */
-	if (obj->type != ACPI_TYPE_PACKAGE)
-		return -EINVAL;
-
 #if 0 /* Just for debugging purposes */
 	for (i = 0; i < obj->package.count; i++) {
 		union acpi_object *cur = &obj->package.elements[i];
-- 
2.37.2

[PATCH 02/13] media: atomisp-ov2680: Fix ov2680_set_fmt()

[Hans de Goede]

On sets actually store the set (closest) format inside ov2680_device.dev,
so that it also properly gets returned by get_fmt.

This fixes the following problem:

1. App does an VIDIOC_SET_FMT 640x480, calling ov2680_set_fmt()
2. Internal buffers (atomisp_create_pipes_stream()) get allocated
   at 640x480 size by atomisp_set_fmt()
3. ov2680_get_fmt() gets called later on and returns 1600x1200
   since ov2680_device.dev was not updated. So things get configured
   to stream at 1600x1200, but the internal buffers created during
   atomisp_create_pipes_stream() do not get updated in size
4. streaming starts, internal buffers overflow and the entire
   machine freezes eventually due to memory being corrupted

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 4ba99c660681..ab52e35266bb 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -894,11 +894,7 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	if (v_flag)
 		ov2680_v_flip(sd, v_flag);
 
-	/*
-	 * ret = startup(sd);
-	 * if (ret)
-	 * dev_err(&client->dev, "ov2680 startup err\n");
-	 */
+	dev->res = res;
 err:
 	mutex_unlock(&dev->input_lock);
 	return ret;
-- 
2.37.2

[PATCH 03/13] media: atomisp-ov2680: Don't take the input_lock for try_fmt calls.

[Hans de Goede]

On ov2680_set_fmt() calls with format->which == V4L2_SUBDEV_FORMAT_TRY,
ov2680_set_fmt() does not talk to the sensor, so there is no need to
lock the dev->input_lock mutex in this case.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index ab52e35266bb..9ac469878eea 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -841,8 +841,6 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	if (!ov2680_info)
 		return -EINVAL;
 
-	mutex_lock(&dev->input_lock);
-
 	res = v4l2_find_nearest_size(ov2680_res_preview,
 				     ARRAY_SIZE(ov2680_res_preview), width,
 				     height, fmt->width, fmt->height);
@@ -855,13 +853,14 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	fmt->code = MEDIA_BUS_FMT_SBGGR10_1X10;
 	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
 		sd_state->pads->try_fmt = *fmt;
-		mutex_unlock(&dev->input_lock);
 		return 0;
 	}
 
 	dev_dbg(&client->dev, "%s: %dx%d\n",
 		__func__, fmt->width, fmt->height);
 
+	mutex_lock(&dev->input_lock);
+
 	/* s_power has not been called yet for std v4l2 clients (camorama) */
 	power_up(sd);
 	ret = ov2680_write_reg_array(client, dev->res->regs);
-- 
2.37.2

[PATCH 04/13] media: atomisp-ov2680: Improve ov2680_set_fmt() error handling

[Hans de Goede]

Exit with an error on any i2c-write errors, rather then only
exiting with an error when ov2680_get_intg_factor() fails.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
index 9ac469878eea..5ba4c52a06a2 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-ov2680.c
@@ -864,9 +864,11 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 	/* s_power has not been called yet for std v4l2 clients (camorama) */
 	power_up(sd);
 	ret = ov2680_write_reg_array(client, dev->res->regs);
-	if (ret)
+	if (ret) {
 		dev_err(&client->dev,
 			"ov2680 write resolution register err: %d\n", ret);
+		goto err;
+	}
 
 	vts = dev->res->lines_per_frame;
 
@@ -875,8 +877,10 @@ static int ov2680_set_fmt(struct v4l2_subdev *sd,
 		vts = dev->exposure + OV2680_INTEGRATION_TIME_MARGIN;
 
 	ret = ov2680_write_reg(client, 2, OV2680_TIMING_VTS_H, vts);
-	if (ret)
+	if (ret) {
 		dev_err(&client->dev, "ov2680 write vts err: %d\n", ret);
+		goto err;
+	}
 
 	ret = ov2680_get_intg_factor(client, ov2680_info, res);
 	if (ret) {
-- 
2.37.2

[PATCH 05/13] media: atomisp-notes: Add info about sensors v4l2_get_subdev_hostdata() use

[Hans de Goede]

Add info about sensors v4l2_get_subdev_hostdata() use, to notes.txt.

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

diff --git a/drivers/staging/media/atomisp/notes.txt b/drivers/staging/media/atomisp/notes.txt
index d128b792e05f..d3cf6ed547ae 100644
--- a/drivers/staging/media/atomisp/notes.txt
+++ b/drivers/staging/media/atomisp/notes.txt
@@ -28,3 +28,22 @@ Since getting a picture requires multiple processing steps,
 this means that unlike in fixed pipelines the soft pipelines
 on the ISP can do multiple processing steps in a single pipeline
 element (in a single binary).
+
+###
+
+The sensor drivers use of v4l2_get_subdev_hostdata(), which returns
+a camera_mipi_info struct. This struct is allocated/managed by
+the core atomisp code. The most important parts of the struct
+are filled by the atomisp core itself, like e.g. the port number.
+
+The sensor drivers on a set_fmt call do fill in camera_mipi_info.data
+which is a atomisp_sensor_mode_data struct. This gets filled from
+a function called <sensor_name>_get_intg_factor(). This struct is not
+used by the atomisp code at all. It is returned to userspace by
+a ATOMISP_IOC_G_SENSOR_MODE_DATA and the Android userspace does use this.
+
+Other members of camera_mipi_info which are set by some drivers are:
+-metadata_width, metadata_height, metadata_effective_width, set by
+ the ov5693 driver (and used by the atomisp core)
+-raw_bayer_order, adjusted by the ov2680 driver when flipping since
+ flipping can change the bayer order
-- 
2.37.2

[PATCH 06/13] media: atomisp: Fix VIDIOC_TRY_FMT

[Hans de Goede]

atomisp_try_fmt() calls the sensor's try_fmt handler but it does
not copy the result back to the passed in v4l2_pix_format under
some circumstances.

Potentially returning an unsupported resolution to userspace,
which VIDIOC_TRY_FMT is not supposed to do.

atomisp_set_fmt() also uses atomisp_try_fmt() and relies
on this wrong behavior. The VIDIOC_TRY_FMT call passes NULL for
the res_overflow argument where as the atomisp_set_fmt() call
passes non NULL.

Use the res_overflow argument to differentiate between the 2 callers
and always propagate the sensors result in the VIDIOC_TRY_FMT case.

This fixes the resolution list in camorama showing resolutions like e.g.
1584x1184 instead of 1600x1200.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index c932f340068f..db6465756e49 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -4886,8 +4886,8 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 		return 0;
 	}
 
-	if (snr_mbus_fmt->width < f->width
-	    && snr_mbus_fmt->height < f->height) {
+	if (!res_overflow || (snr_mbus_fmt->width < f->width &&
+			      snr_mbus_fmt->height < f->height)) {
 		f->width = snr_mbus_fmt->width;
 		f->height = snr_mbus_fmt->height;
 		/* Set the flag when resolution requested is
-- 
2.37.2

[PATCH 07/13] media: atomisp: Make atomisp_try_fmt_cap() take padding into account

[Hans de Goede]

atomisp_try_fmt() gives results with padding included. So when userspace
asks for e.g. 1600x1200 then we should pass 1616x1216 to atomisp_try_fmt()
this will then get adjusted back to 1600x1200 before returning it to
userspace by the atomisp_adjust_fmt() call at the end of atomisp_try_fmt().

This fixes the resolution list in camorama showing resolutions like e.g.
1584x1184 instead of 1600x1200.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 459645c2e2a7..7ecee39ef5a4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -960,6 +960,13 @@ static int atomisp_try_fmt_cap(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret;
 
+	/*
+	 * atomisp_try_fmt() gived results with padding included, note
+	 * (this gets removed again by the atomisp_adjust_fmt() call below.
+	 */
+	f->fmt.pix.width += pad_w;
+	f->fmt.pix.height += pad_h;
+
 	rt_mutex_lock(&isp->mutex);
 	ret = atomisp_try_fmt(vdev, &f->fmt.pix, NULL);
 	rt_mutex_unlock(&isp->mutex);
-- 
2.37.2

[PATCH 08/13] media: atomisp: hmm_bo: Simplify alloc_private_pages()

[Hans de Goede]

Since lack_mem starts initialized to true, alloc_private_pages() will
always set order to HMM_MIN_ORDER aka 0 / will always alloc 1 page at
a time.

So all the magic to decrease order if allocs fail is not necessary
and can be removed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/include/hmm/hmm_bo.h        |  3 -
 .../staging/media/atomisp/pci/hmm/hmm_bo.c    | 83 +++----------------
 2 files changed, 10 insertions(+), 76 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
index 385e22fc4a46..901dc37c80bc 100644
--- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
+++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
@@ -65,9 +65,6 @@
 #define	check_bo_null_return_void(bo)	\
 	check_null_return_void(bo, "NULL hmm buffer object.\n")
 
-#define	HMM_MAX_ORDER		3
-#define	HMM_MIN_ORDER		0
-
 #define	ISP_VM_START	0x0
 #define	ISP_VM_SIZE	(0x7FFFFFFF)	/* 2G address space */
 #define	ISP_PTR_NULL	NULL
diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index f50494123f03..275314241263 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -44,16 +44,6 @@
 #include "hmm/hmm_common.h"
 #include "hmm/hmm_bo.h"
 
-static unsigned int order_to_nr(unsigned int order)
-{
-	return 1U << order;
-}
-
-static unsigned int nr_to_order_bottom(unsigned int nr)
-{
-	return fls(nr) - 1;
-}
-
 static int __bo_init(struct hmm_bo_device *bdev, struct hmm_buffer_object *bo,
 		     unsigned int pgnr)
 {
@@ -653,13 +643,10 @@ static void free_private_bo_pages(struct hmm_buffer_object *bo,
 static int alloc_private_pages(struct hmm_buffer_object *bo)
 {
 	int ret;
-	unsigned int pgnr, order, blk_pgnr, alloc_pgnr;
+	unsigned int pgnr, blk_pgnr, alloc_pgnr;
 	struct page *pages;
 	gfp_t gfp = GFP_NOWAIT | __GFP_NOWARN; /* REVISIT: need __GFP_FS too? */
 	int i, j;
-	int failure_number = 0;
-	bool reduce_order = false;
-	bool lack_mem = true;
 
 	pgnr = bo->pgnr;
 
@@ -667,58 +654,17 @@ static int alloc_private_pages(struct hmm_buffer_object *bo)
 	alloc_pgnr = 0;
 
 	while (pgnr) {
-		order = nr_to_order_bottom(pgnr);
-		/*
-		 * if be short of memory, we will set order to 0
-		 * everytime.
-		 */
-		if (lack_mem)
-			order = HMM_MIN_ORDER;
-		else if (order > HMM_MAX_ORDER)
-			order = HMM_MAX_ORDER;
-retry:
-		/*
-		 * When order > HMM_MIN_ORDER, for performance reasons we don't
-		 * want alloc_pages() to sleep. In case it fails and fallbacks
-		 * to HMM_MIN_ORDER or in case the requested order is originally
-		 * the minimum value, we can allow alloc_pages() to sleep for
-		 * robustness purpose.
-		 *
-		 * REVISIT: why __GFP_FS is necessary?
-		 */
-		if (order == HMM_MIN_ORDER) {
-			gfp &= ~GFP_NOWAIT;
-			gfp |= __GFP_RECLAIM | __GFP_FS;
-		}
+		gfp &= ~GFP_NOWAIT;
+		gfp |= __GFP_RECLAIM | __GFP_FS;
 
-		pages = alloc_pages(gfp, order);
+		pages = alloc_pages(gfp, 0); // alloc 1 page
 		if (unlikely(!pages)) {
-			/*
-			 * in low memory case, if allocation page fails,
-			 * we turn to try if order=0 allocation could
-			 * succeed. if order=0 fails too, that means there is
-			 * no memory left.
-			 */
-			if (order == HMM_MIN_ORDER) {
-				dev_err(atomisp_dev,
-					"%s: cannot allocate pages\n",
-					__func__);
-				goto cleanup;
-			}
-			order = HMM_MIN_ORDER;
-			failure_number++;
-			reduce_order = true;
-			/*
-			 * if fail two times continuously, we think be short
-			 * of memory now.
-			 */
-			if (failure_number == 2) {
-				lack_mem = true;
-				failure_number = 0;
-			}
-			goto retry;
+			dev_err(atomisp_dev,
+				"%s: cannot allocate pages\n",
+				__func__);
+			goto cleanup;
 		} else {
-			blk_pgnr = order_to_nr(order);
+			blk_pgnr = 1;
 
 			/*
 			 * set memory to uncacheable -- UC_MINUS
@@ -728,7 +674,7 @@ static int alloc_private_pages(struct hmm_buffer_object *bo)
 				dev_err(atomisp_dev,
 					"set page uncacheablefailed.\n");
 
-				__free_pages(pages, order);
+				__free_pages(pages, 0);
 
 				goto cleanup;
 			}
@@ -738,15 +684,6 @@ static int alloc_private_pages(struct hmm_buffer_object *bo)
 			}
 
 			pgnr -= blk_pgnr;
-
-			/*
-			 * if order is not reduced this time, clear
-			 * failure_number.
-			 */
-			if (reduce_order)
-				reduce_order = false;
-			else
-				failure_number = 0;
 		}
 	}
 
-- 
2.37.2

[PATCH 09/13] media: atomisp: hmm_bo: Further simplify alloc_private_pages()

[Hans de Goede]

Further simplify alloc_private_pages().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/hmm/hmm_bo.c    | 29 ++++---------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index 275314241263..bb52171a9d87 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -642,21 +642,11 @@ static void free_private_bo_pages(struct hmm_buffer_object *bo,
 /*Allocate pages which will be used only by ISP*/
 static int alloc_private_pages(struct hmm_buffer_object *bo)
 {
-	int ret;
-	unsigned int pgnr, blk_pgnr, alloc_pgnr;
+	const gfp_t gfp = __GFP_NOWARN | __GFP_RECLAIM | __GFP_FS;
 	struct page *pages;
-	gfp_t gfp = GFP_NOWAIT | __GFP_NOWARN; /* REVISIT: need __GFP_FS too? */
-	int i, j;
-
-	pgnr = bo->pgnr;
-
-	i = 0;
-	alloc_pgnr = 0;
-
-	while (pgnr) {
-		gfp &= ~GFP_NOWAIT;
-		gfp |= __GFP_RECLAIM | __GFP_FS;
+	int i, ret;
 
+	for (i = 0; i < bo->pgnr; i++) {
 		pages = alloc_pages(gfp, 0); // alloc 1 page
 		if (unlikely(!pages)) {
 			dev_err(atomisp_dev,
@@ -664,12 +654,10 @@ static int alloc_private_pages(struct hmm_buffer_object *bo)
 				__func__);
 			goto cleanup;
 		} else {
-			blk_pgnr = 1;
-
 			/*
 			 * set memory to uncacheable -- UC_MINUS
 			 */
-			ret = set_pages_uc(pages, blk_pgnr);
+			ret = set_pages_uc(pages, 1);
 			if (ret) {
 				dev_err(atomisp_dev,
 					"set page uncacheablefailed.\n");
@@ -679,18 +667,13 @@ static int alloc_private_pages(struct hmm_buffer_object *bo)
 				goto cleanup;
 			}
 
-			for (j = 0; j < blk_pgnr; j++, i++) {
-				bo->pages[i] = pages + j;
-			}
-
-			pgnr -= blk_pgnr;
+			bo->pages[i] = pages;
 		}
 	}
 
 	return 0;
 cleanup:
-	alloc_pgnr = i;
-	free_private_bo_pages(bo, alloc_pgnr);
+	free_private_bo_pages(bo, i);
 	return -ENOMEM;
 }
 
-- 
2.37.2

[PATCH 10/13] media: atomisp: hmm_bo: Rewrite alloc_private_pages() using pages_array helper funcs

[Hans de Goede]

Rewrite alloc_private_pages() using pages_array helper funcs.

Note alloc_pages_bulk_array() skips non NULL pages, so switch
the allocating of the pages pointer array to kcalloc to ensure
the pages are initially all set to NULL.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/hmm/hmm_bo.c    | 48 ++++++++-----------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index bb52171a9d87..40b1137dcc31 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -615,6 +615,14 @@ struct hmm_buffer_object *hmm_bo_device_search_vmap_start(
 	return bo;
 }
 
+static void free_pages_bulk_array(unsigned long nr_pages, struct page **page_array)
+{
+	unsigned long i;
+
+	for (i = 0; i < nr_pages; i++)
+		__free_pages(page_array[i], 0);
+}
+
 static void free_private_bo_pages(struct hmm_buffer_object *bo,
 				  int free_pgnr)
 {
@@ -643,38 +651,22 @@ static void free_private_bo_pages(struct hmm_buffer_object *bo,
 static int alloc_private_pages(struct hmm_buffer_object *bo)
 {
 	const gfp_t gfp = __GFP_NOWARN | __GFP_RECLAIM | __GFP_FS;
-	struct page *pages;
-	int i, ret;
-
-	for (i = 0; i < bo->pgnr; i++) {
-		pages = alloc_pages(gfp, 0); // alloc 1 page
-		if (unlikely(!pages)) {
-			dev_err(atomisp_dev,
-				"%s: cannot allocate pages\n",
-				__func__);
-			goto cleanup;
-		} else {
-			/*
-			 * set memory to uncacheable -- UC_MINUS
-			 */
-			ret = set_pages_uc(pages, 1);
-			if (ret) {
-				dev_err(atomisp_dev,
-					"set page uncacheablefailed.\n");
-
-				__free_pages(pages, 0);
+	int ret;
 
-				goto cleanup;
-			}
+	ret = alloc_pages_bulk_array(gfp, bo->pgnr, bo->pages);
+	if (ret != bo->pgnr) {
+		free_pages_bulk_array(ret, bo->pages);
+		return -ENOMEM;
+	}
 
-			bo->pages[i] = pages;
-		}
+	ret = set_pages_array_uc(bo->pages, bo->pgnr);
+	if (ret) {
+		dev_err(atomisp_dev, "set pages uncacheable failed.\n");
+		free_pages_bulk_array(bo->pgnr, bo->pages);
+		return ret;
 	}
 
 	return 0;
-cleanup:
-	free_private_bo_pages(bo, i);
-	return -ENOMEM;
 }
 
 static void free_user_pages(struct hmm_buffer_object *bo,
@@ -774,7 +766,7 @@ int hmm_bo_alloc_pages(struct hmm_buffer_object *bo,
 	mutex_lock(&bo->mutex);
 	check_bo_status_no_goto(bo, HMM_BO_PAGE_ALLOCED, status_err);
 
-	bo->pages = kmalloc_array(bo->pgnr, sizeof(struct page *), GFP_KERNEL);
+	bo->pages = kcalloc(bo->pgnr, sizeof(struct page *), GFP_KERNEL);
 	if (unlikely(!bo->pages)) {
 		ret = -ENOMEM;
 		goto alloc_err;
-- 
2.37.2

[PATCH 11/13] media: atomisp: hmm_bo: Rewrite free_private_pages() using pages_array helper funcs

[Hans de Goede]

Rewrite free_private_pages() using pages_array helper funcs.

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

diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index 40b1137dcc31..d7f42a4ce40a 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -623,28 +623,10 @@ static void free_pages_bulk_array(unsigned long nr_pages, struct page **page_arr
 		__free_pages(page_array[i], 0);
 }
 
-static void free_private_bo_pages(struct hmm_buffer_object *bo,
-				  int free_pgnr)
+static void free_private_bo_pages(struct hmm_buffer_object *bo)
 {
-	int i, ret;
-
-	for (i = 0; i < free_pgnr; i++) {
-		ret = set_pages_wb(bo->pages[i], 1);
-		if (ret)
-			dev_err(atomisp_dev,
-				"set page to WB err ...ret = %d\n",
-				ret);
-		/*
-		W/A: set_pages_wb seldom return value = -EFAULT
-		indicate that address of page is not in valid
-		range(0xffff880000000000~0xffffc7ffffffffff)
-		then, _free_pages would panic; Do not know why page
-		address be valid,it maybe memory corruption by lowmemory
-		*/
-		if (!ret) {
-			__free_pages(bo->pages[i], 0);
-		}
-	}
+	set_pages_array_wb(bo->pages, bo->pgnr);
+	free_pages_bulk_array(bo->pgnr, bo->pages);
 }
 
 /*Allocate pages which will be used only by ISP*/
@@ -822,7 +804,7 @@ void hmm_bo_free_pages(struct hmm_buffer_object *bo)
 	bo->status &= (~HMM_BO_PAGE_ALLOCED);
 
 	if (bo->type == HMM_BO_PRIVATE)
-		free_private_bo_pages(bo, bo->pgnr);
+		free_private_bo_pages(bo);
 	else if (bo->type == HMM_BO_USER)
 		free_user_pages(bo, bo->pgnr);
 	else
-- 
2.37.2

[PATCH 12/13] media: atomisp: hmm_bo: Drop PFN code path from alloc_user_pages()

[Hans de Goede]

alloc_user_pages() is only ever called on qbuf for USERPTR buffers which
always hits the get_user_pages_fast() path, so the pin_user_pages() path
can be removed.

Getting the vma then also is no longer necessary since that is only
done to determine which path to use.

And this also removes the only users of the mem_type struct hmm_bo member,
so remove that as well.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/include/hmm/hmm_bo.h        |  3 --
 .../staging/media/atomisp/pci/hmm/hmm_bo.c    | 48 ++++---------------
 2 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
index 901dc37c80bc..c5cbae1d9cf9 100644
--- a/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
+++ b/drivers/staging/media/atomisp/include/hmm/hmm_bo.h
@@ -86,8 +86,6 @@ enum hmm_bo_type {
 #define	HMM_BO_VMAPED		0x10
 #define	HMM_BO_VMAPED_CACHED	0x20
 #define	HMM_BO_ACTIVE		0x1000
-#define	HMM_BO_MEM_TYPE_USER     0x1
-#define	HMM_BO_MEM_TYPE_PFN      0x2
 
 struct hmm_bo_device {
 	struct isp_mmu		mmu;
@@ -123,7 +121,6 @@ struct hmm_buffer_object {
 	enum hmm_bo_type	type;
 	int		mmap_count;
 	int		status;
-	int		mem_type;
 	void		*vmap_addr; /* kernel virtual address by vmap */
 
 	struct rb_node	node;
diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
index d7f42a4ce40a..c1d5490742be 100644
--- a/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
+++ b/drivers/staging/media/atomisp/pci/hmm/hmm_bo.c
@@ -656,12 +656,8 @@ static void free_user_pages(struct hmm_buffer_object *bo,
 {
 	int i;
 
-	if (bo->mem_type == HMM_BO_MEM_TYPE_PFN) {
-		unpin_user_pages(bo->pages, page_nr);
-	} else {
-		for (i = 0; i < page_nr; i++)
-			put_page(bo->pages[i]);
-	}
+	for (i = 0; i < page_nr; i++)
+		put_page(bo->pages[i]);
 }
 
 /*
@@ -671,43 +667,17 @@ static int alloc_user_pages(struct hmm_buffer_object *bo,
 			    const void __user *userptr)
 {
 	int page_nr;
-	struct vm_area_struct *vma;
-
-	mutex_unlock(&bo->mutex);
-	mmap_read_lock(current->mm);
-	vma = find_vma(current->mm, (unsigned long)userptr);
-	mmap_read_unlock(current->mm);
-	if (!vma) {
-		dev_err(atomisp_dev, "find_vma failed\n");
-		mutex_lock(&bo->mutex);
-		return -EFAULT;
-	}
-	mutex_lock(&bo->mutex);
-	/*
-	 * Handle frame buffer allocated in other kerenl space driver
-	 * and map to user space
-	 */
 
 	userptr = untagged_addr(userptr);
 
-	if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
-		page_nr = pin_user_pages((unsigned long)userptr, bo->pgnr,
-					 FOLL_LONGTERM | FOLL_WRITE,
-					 bo->pages, NULL);
-		bo->mem_type = HMM_BO_MEM_TYPE_PFN;
-	} else {
-		/*Handle frame buffer allocated in user space*/
-		mutex_unlock(&bo->mutex);
-		page_nr = get_user_pages_fast((unsigned long)userptr,
-					      (int)(bo->pgnr), 1, bo->pages);
-		mutex_lock(&bo->mutex);
-		bo->mem_type = HMM_BO_MEM_TYPE_USER;
-	}
+	/*Handle frame buffer allocated in user space*/
+	mutex_unlock(&bo->mutex);
+	page_nr = get_user_pages_fast((unsigned long)userptr,
+				      (int)(bo->pgnr), 1, bo->pages);
+	mutex_lock(&bo->mutex);
 
-	dev_dbg(atomisp_dev, "%s: %d %s pages were allocated as 0x%08x\n",
-		__func__,
-		bo->pgnr,
-		bo->mem_type == HMM_BO_MEM_TYPE_USER ? "user" : "pfn", page_nr);
+	dev_dbg(atomisp_dev, "%s: %d user pages were allocated as 0x%08x\n",
+		__func__, bo->pgnr, page_nr);
 
 	/* can be written by caller, not forced */
 	if (page_nr != bo->pgnr) {
-- 
2.37.2

[PATCH 13/13] media: atomisp: Ensure that USERPTR pointers are page aligned

[Hans de Goede]

The atomisp code needs USERPTR pointers to be page aligned,
otherwise bad things (scribbling over other parts of the
process' RAM) happen.

Add a check to ensure this and exit VIDIOC_QBUF calls with
unaligned pointers with -EINVAL.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 7ecee39ef5a4..c8c6f9f8f0b8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1345,6 +1345,12 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	 * address and reprograme out page table properly
 	 */
 	if (buf->memory == V4L2_MEMORY_USERPTR) {
+		if (buf->m.userptr & ~PAGE_MASK) {
+			dev_err(isp->dev, "Error userptr is not page aligned.\n");
+			ret = -EINVAL;
+			goto error;
+		}
+
 		vb = pipe->capq.bufs[buf->index];
 		vm_mem = vb->priv;
 		if (!vm_mem) {
-- 
2.37.2

Re: [PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> The acpi_evaluate_dsm_typed() provides a way to check the type of the
> object evaluated by _DSM call. Use it instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Link: https://lore.kernel.org/r/20220730155905.90091-1-andriy.shevchenko@linux.intel.com
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Thanks!
I believe the v2 of this patch (when it was sent standalone) has been
Acked by Sakari. But I might be wrong.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[Hans de Goede]

Hi,

On 8/22/22 00:31, Andy Shevchenko wrote:
> On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> The acpi_evaluate_dsm_typed() provides a way to check the type of the
>> object evaluated by _DSM call. Use it instead of open coded variant.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Link: https://lore.kernel.org/r/20220730155905.90091-1-andriy.shevchenko@linux.intel.com
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks!
> I believe the v2 of this patch (when it was sent standalone) has been
> Acked by Sakari. But I might be wrong.

You are right:
https://lore.kernel.org/linux-media/Yud2hzq3JQBzf+oK@paasikivi.fi.intel.com/

So let me add that here, then patchwork should pick it up:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Regards,

Hans

Re: [PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 11:02 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8/22/22 00:31, Andy Shevchenko wrote:

...

> So let me add that here, then patchwork should pick it up:
>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

You may also add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
to all patches, but the 1st (it has already SoB) and commented ones,
which I'm going to do soon.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/13] media: atomisp_gmin_platform: Switch to use acpi_evaluate_dsm_typed()

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 3:57 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 22, 2022 at 11:02 AM Hans de Goede <hdegoede@redhat.com> wrote:

...

> You may also add
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> to all patches, but the 1st (it has already SoB)

> and commented ones,
> which I'm going to do soon.

(should be read as)
... and non-commented, that become visible after I comment on a
couple, which I'm going to do soon.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 12/13] media: atomisp: hmm_bo: Drop PFN code path from alloc_user_pages()

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> alloc_user_pages() is only ever called on qbuf for USERPTR buffers which
> always hits the get_user_pages_fast() path, so the pin_user_pages() path
> can be removed.
>
> Getting the vma then also is no longer necessary since that is only
> done to determine which path to use.
>
> And this also removes the only users of the mem_type struct hmm_bo member,
> so remove that as well.

...

> +       /*Handle frame buffer allocated in user space*/

Spaces?

> +       mutex_unlock(&bo->mutex);

> +       page_nr = get_user_pages_fast((unsigned long)userptr,
> +                                     (int)(bo->pgnr), 1, bo->pages);

No need for parentheses in the first argument.

> +       mutex_lock(&bo->mutex);

> +       dev_dbg(atomisp_dev, "%s: %d user pages were allocated as 0x%08x\n",
> +               __func__, bo->pgnr, page_nr);

Since you touch this you may remove __func__, which can be enabled via
dynamic debug. OTOH, it might be better to go and drop __func__
everywhere in the driver in the debug messages.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 13/13] media: atomisp: Ensure that USERPTR pointers are page aligned

[Andy Shevchenko]

On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The atomisp code needs USERPTR pointers to be page aligned,
> otherwise bad things (scribbling over other parts of the
> process' RAM) happen.
>
> Add a check to ensure this and exit VIDIOC_QBUF calls with
> unaligned pointers with -EINVAL.

...

>         if (buf->memory == V4L2_MEMORY_USERPTR) {
> +               if (buf->m.userptr & ~PAGE_MASK) {

offset_in_page() ?

Further we may utilize helpers from pfn.h in the driver.

> +                       dev_err(isp->dev, "Error userptr is not page aligned.\n");
> +                       ret = -EINVAL;
> +                       goto error;
> +               }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 12/13] media: atomisp: hmm_bo: Drop PFN code path from alloc_user_pages()

[Hans de Goede]

Hi,

On 8/22/22 15:02, Andy Shevchenko wrote:
> On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> alloc_user_pages() is only ever called on qbuf for USERPTR buffers which
>> always hits the get_user_pages_fast() path, so the pin_user_pages() path
>> can be removed.
>>
>> Getting the vma then also is no longer necessary since that is only
>> done to determine which path to use.
>>
>> And this also removes the only users of the mem_type struct hmm_bo member,
>> so remove that as well.
> 
> ...
> 
>> +       /*Handle frame buffer allocated in user space*/
> 
> Spaces?

I just changed the indentation on this, but I might just as well
add the spaces while at it, will do so for v2.

> 
>> +       mutex_unlock(&bo->mutex);
> 
>> +       page_nr = get_user_pages_fast((unsigned long)userptr,
>> +                                     (int)(bo->pgnr), 1, bo->pages);
> 
> No need for parentheses in the first argument.

Ack, will fix for v2.

> 
>> +       mutex_lock(&bo->mutex);
> 
>> +       dev_dbg(atomisp_dev, "%s: %d user pages were allocated as 0x%08x\n",
>> +               __func__, bo->pgnr, page_nr);
> 
> Since you touch this you may remove __func__, which can be enabled via
> dynamic debug. OTOH, it might be better to go and drop __func__
> everywhere in the driver in the debug messages.

Or even better, I'll just drop this useless debug statement entirely for v2.

Regards,

Hans

Re: [PATCH 13/13] media: atomisp: Ensure that USERPTR pointers are page aligned

[Hans de Goede]

Hi,

On 8/22/22 15:03, Andy Shevchenko wrote:
> On Mon, Aug 22, 2022 at 12:50 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The atomisp code needs USERPTR pointers to be page aligned,
>> otherwise bad things (scribbling over other parts of the
>> process' RAM) happen.
>>
>> Add a check to ensure this and exit VIDIOC_QBUF calls with
>> unaligned pointers with -EINVAL.
> 
> ...
> 
>>         if (buf->memory == V4L2_MEMORY_USERPTR) {
>> +               if (buf->m.userptr & ~PAGE_MASK) {
> 
> offset_in_page() ?

Ack I've switched to offset_in_page() for v2.

Regards,

Hans


> 
> Further we may utilize helpers from pfn.h in the driver.
> 
>> +                       dev_err(isp->dev, "Error userptr is not page aligned.\n");
>> +                       ret = -EINVAL;
>> +                       goto error;
>> +               }
>