[PATCH AUTOSEL 6.0 10/34] media: meson: vdec: fix possible refcount leak in vdec_probe()

[PATCH AUTOSEL 6.0 10/34] media: meson: vdec: fix possible refcount leak in vdec_probe()

[Sasha Levin]

From: Hangyu Hua <hbh25y@gmail.com>

[ Upstream commit 7718999356234d9cc6a11b4641bb773928f1390f ]

v4l2_device_unregister need to be called to put the refcount got by
v4l2_device_register when vdec_probe fails or vdec_remove is called.

Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/meson/vdec/vdec.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
index 8549d95be0f2..52f224d8def1 100644
--- a/drivers/staging/media/meson/vdec/vdec.c
+++ b/drivers/staging/media/meson/vdec/vdec.c
@@ -1102,6 +1102,7 @@ static int vdec_probe(struct platform_device *pdev)
 
 err_vdev_release:
 	video_device_release(vdev);
+	v4l2_device_unregister(&core->v4l2_dev);
 	return ret;
 }
 
@@ -1110,6 +1111,7 @@ static int vdec_remove(struct platform_device *pdev)
 	struct amvdec_core *core = platform_get_drvdata(pdev);
 
 	video_unregister_device(core->vdev_dec);
+	v4l2_device_unregister(&core->v4l2_dev);
 
 	return 0;
 }
-- 
2.35.1

[PATCH AUTOSEL 6.0 11/34] media: hantro: Store HEVC bit depth in context

[Sasha Levin]

From: Benjamin Gaignard <benjamin.gaignard@collabora.com>

[ Upstream commit 4bec03301ecd81760c159402467dbb2cfd527684 ]

Store HEVC bit depth in context.
Bit depth is equal to hevc sps bit_depth_luma_minus8 + 8.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 2036f72eeb4a..1dd8312d824c 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -251,6 +251,11 @@ queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
 
 static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
 {
+	struct hantro_ctx *ctx;
+
+	ctx = container_of(ctrl->handler,
+			   struct hantro_ctx, ctrl_handler);
+
 	if (ctrl->id == V4L2_CID_STATELESS_H264_SPS) {
 		const struct v4l2_ctrl_h264_sps *sps = ctrl->p_new.p_h264_sps;
 
@@ -272,6 +277,8 @@ static int hantro_try_ctrl(struct v4l2_ctrl *ctrl)
 		if (sps->bit_depth_luma_minus8 != 0)
 			/* Only 8-bit is supported */
 			return -EINVAL;
+
+		ctx->bit_depth = sps->bit_depth_luma_minus8 + 8;
 	} else if (ctrl->id == V4L2_CID_STATELESS_VP9_FRAME) {
 		const struct v4l2_ctrl_vp9_frame *dec_params = ctrl->p_new.p_vp9_frame;
 
-- 
2.35.1

[PATCH AUTOSEL 6.0 12/34] media: hantro: HEVC: Fix auxilary buffer size calculation

[Sasha Levin]

From: Benjamin Gaignard <benjamin.gaignard@collabora.com>

[ Upstream commit 8a438580a09ecef78cd6c5825d628b4d5ae1c127 ]

SAO and FILTER buffers size depend of the bit depth.
Make sure we have enough space for 10bit bitstreams.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/hantro/hantro_hevc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_hevc.c b/drivers/staging/media/hantro/hantro_hevc.c
index b990bc98164c..9383fb7081f6 100644
--- a/drivers/staging/media/hantro/hantro_hevc.c
+++ b/drivers/staging/media/hantro/hantro_hevc.c
@@ -104,7 +104,7 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
 		hevc_dec->tile_bsd.cpu = NULL;
 	}
 
-	size = VERT_FILTER_RAM_SIZE * height64 * (num_tile_cols - 1);
+	size = (VERT_FILTER_RAM_SIZE * height64 * (num_tile_cols - 1) * ctx->bit_depth) / 8;
 	hevc_dec->tile_filter.cpu = dma_alloc_coherent(vpu->dev, size,
 						       &hevc_dec->tile_filter.dma,
 						       GFP_KERNEL);
@@ -112,7 +112,7 @@ static int tile_buffer_reallocate(struct hantro_ctx *ctx)
 		goto err_free_tile_buffers;
 	hevc_dec->tile_filter.size = size;
 
-	size = VERT_SAO_RAM_SIZE * height64 * (num_tile_cols - 1);
+	size = (VERT_SAO_RAM_SIZE * height64 * (num_tile_cols - 1) * ctx->bit_depth) / 8;
 	hevc_dec->tile_sao.cpu = dma_alloc_coherent(vpu->dev, size,
 						    &hevc_dec->tile_sao.dma,
 						    GFP_KERNEL);
-- 
2.35.1

[PATCH AUTOSEL 6.0 13/34] media: hantro: HEVC: Fix chroma offset computation

[Sasha Levin]

From: Benjamin Gaignard <benjamin.gaignard@collabora.com>

[ Upstream commit f64853ad7f964b3bf7c1d63b27ca7ef972797a1c ]

The chroma offset depends of the bitstream depth.
Make sure that ctx->bit_depth is used to compute it.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/hantro/hantro_g2_hevc_dec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
index 233ecd863d5f..a917079a6ed3 100644
--- a/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
+++ b/drivers/staging/media/hantro/hantro_g2_hevc_dec.c
@@ -12,7 +12,7 @@
 
 static size_t hantro_hevc_chroma_offset(struct hantro_ctx *ctx)
 {
-	return ctx->dst_fmt.width * ctx->dst_fmt.height;
+	return ctx->dst_fmt.width * ctx->dst_fmt.height * ctx->bit_depth / 8;
 }
 
 static size_t hantro_hevc_motion_vectors_offset(struct hantro_ctx *ctx)
-- 
2.35.1

[PATCH AUTOSEL 6.0 15/34] media: atomisp-ov2680: Fix ov2680_set_fmt()

[Sasha Levin]

From: Hans de Goede <hdegoede@redhat.com>

[ Upstream commit adea153b4f6537f367fe77abada263fde8a1f7b6 ]

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

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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.35.1

[PATCH AUTOSEL 6.0 16/34] media: atomisp: Fix VIDIOC_TRY_FMT

[Sasha Levin]

From: Hans de Goede <hdegoede@redhat.com>

[ Upstream commit 4d3aafb9c9bba59c9b6f6df8ea6c89483bfed8d4 ]

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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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.35.1

[PATCH AUTOSEL 6.0 17/34] media: atomisp: Ensure that USERPTR pointers are page aligned

[Sasha Levin]

From: Hans de Goede <hdegoede@redhat.com>

[ Upstream commit 6e6c4ae0f0ba295dbf6cbd48d93bec169d6ce431 ]

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.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 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 459645c2e2a7..4de01aa28fe5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1338,6 +1338,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 (offset_in_page(buf->m.userptr)) {
+			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.35.1

[PATCH AUTOSEL 6.0 18/34] media: atomisp: Fix v4l2_fh resource leak on open errors

[Sasha Levin]

From: Hans de Goede <hdegoede@redhat.com>

[ Upstream commit 5b9853ad1329be49343a608d574eb232ff1273d0 ]

When atomisp_open() fails then it must call v4l2_fh_release() to undo
the results of v4l2_fh_open().

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/atomisp/pci/atomisp_fops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 77150e4ae144..92cbc0e263e8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -903,6 +903,7 @@ static int atomisp_open(struct file *file)
 	pm_runtime_put(vdev->v4l2_dev->dev);
 error:
 	rt_mutex_unlock(&isp->mutex);
+	v4l2_fh_release(file);
 	return ret;
 }
 
-- 
2.35.1

[PATCH AUTOSEL 6.0 19/34] media: atomisp: Fix locking around asd->streaming read/write

[Sasha Levin]

From: Hans de Goede <hdegoede@redhat.com>

[ Upstream commit 2468083f799eb9eef7b03f48ebb9673ad5655f88 ]

For reading / writing the asd->streaming enum the following rules
should be followed:

1. Writers of streaming must hold both isp->mutex and isp->lock.
2. Readers of streaming need to hold only one of the two locks.

Not all writers where properly taking both locks this fixes this.

In the case of the readers, many readers depend on their caller
to hold isp->mutex, add asserts for this

And in the case of atomisp_css_get_dis_stat() it is called with
isp->mutex held, so there is no need to take the spinlock just
for reading the streaming value.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 32 +++++++++++++++++--
 .../media/atomisp/pci/atomisp_compat_css20.c  | 10 +++---
 .../staging/media/atomisp/pci/atomisp_fops.c  |  3 ++
 .../media/atomisp/pci/atomisp_internal.h      |  2 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  4 +++
 .../media/atomisp/pci/atomisp_subdev.c        |  8 ++++-
 .../media/atomisp/pci/atomisp_subdev.h        |  6 +++-
 7 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index db6465756e49..4d5c7328610f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -908,6 +908,8 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 	struct v4l2_control ctrl;
 	bool reset_wdt_timer = false;
 
+	lockdep_assert_held(&isp->mutex);
+
 	if (
 	    buf_type != IA_CSS_BUFFER_TYPE_METADATA &&
 	    buf_type != IA_CSS_BUFFER_TYPE_3A_STATISTICS &&
@@ -1307,6 +1309,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 	bool stream_restart[MAX_STREAM_NUM] = {0};
 	bool depth_mode = false;
 	int i, ret, depth_cnt = 0;
+	unsigned long flags;
+
+	lockdep_assert_held(&isp->mutex);
 
 	if (!isp->sw_contex.file_input)
 		atomisp_css_irq_enable(isp,
@@ -1331,7 +1336,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 
 		stream_restart[asd->index] = true;
 
+		spin_lock_irqsave(&isp->lock, flags);
 		asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
+		spin_unlock_irqrestore(&isp->lock, flags);
 
 		/* stream off sensor */
 		ret = v4l2_subdev_call(
@@ -1346,7 +1353,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 		css_pipe_id = atomisp_get_css_pipe_id(asd);
 		atomisp_css_stop(asd, css_pipe_id, true);
 
+		spin_lock_irqsave(&isp->lock, flags);
 		asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+		spin_unlock_irqrestore(&isp->lock, flags);
 
 		asd->preview_exp_id = 1;
 		asd->postview_exp_id = 1;
@@ -1387,11 +1396,14 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 						   IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
 
 		css_pipe_id = atomisp_get_css_pipe_id(asd);
-		if (atomisp_css_start(asd, css_pipe_id, true))
+		if (atomisp_css_start(asd, css_pipe_id, true)) {
 			dev_warn(isp->dev,
 				 "start SP failed, so do not set streaming to be enable!\n");
-		else
+		} else {
+			spin_lock_irqsave(&isp->lock, flags);
 			asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
+			spin_unlock_irqrestore(&isp->lock, flags);
+		}
 
 		atomisp_csi2_configure(asd);
 	}
@@ -1627,6 +1639,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
 {
 	int i;
 
+	lockdep_assert_held(&isp->mutex);
+
 	if (!atomisp_streaming_count(isp))
 		return;
 
@@ -4077,6 +4091,8 @@ void atomisp_handle_parameter_and_buffer(struct atomisp_video_pipe *pipe)
 	unsigned long irqflags;
 	bool need_to_enqueue_buffer = false;
 
+	lockdep_assert_held(&asd->isp->mutex);
+
 	if (!asd) {
 		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
 			__func__, pipe->vdev.name);
@@ -4170,6 +4186,8 @@ int atomisp_set_parameters(struct video_device *vdev,
 	struct atomisp_css_params *css_param = &asd->params.css_param;
 	int ret;
 
+	lockdep_assert_held(&asd->isp->mutex);
+
 	if (!asd) {
 		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
 			__func__, vdev->name);
@@ -5604,6 +5622,8 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct v4l2_subdev_fh fh;
 	int ret;
 
+	lockdep_assert_held(&isp->mutex);
+
 	if (!asd) {
 		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
 			__func__, vdev->name);
@@ -6275,6 +6295,8 @@ int atomisp_offline_capture_configure(struct atomisp_sub_device *asd,
 {
 	struct v4l2_ctrl *c;
 
+	lockdep_assert_held(&asd->isp->mutex);
+
 	/*
 	* In case of M10MO ZSL capture case, we need to issue a separate
 	* capture request to M10MO which will output captured jpeg image
@@ -6549,6 +6571,8 @@ int atomisp_exp_id_capture(struct atomisp_sub_device *asd, int *exp_id)
 	int value = *exp_id;
 	int ret;
 
+	lockdep_assert_held(&isp->mutex);
+
 	ret = __is_raw_buffer_locked(asd, value);
 	if (ret) {
 		dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
@@ -6570,6 +6594,8 @@ int atomisp_exp_id_unlock(struct atomisp_sub_device *asd, int *exp_id)
 	int value = *exp_id;
 	int ret;
 
+	lockdep_assert_held(&isp->mutex);
+
 	ret = __clear_raw_buffer_bitmap(asd, value);
 	if (ret) {
 		dev_err(isp->dev, "%s exp_id %d invalid %d.\n", __func__, value, ret);
@@ -6605,6 +6631,8 @@ int atomisp_inject_a_fake_event(struct atomisp_sub_device *asd, int *event)
 	if (!event || asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
 		return -EINVAL;
 
+	lockdep_assert_held(&asd->isp->mutex);
+
 	dev_dbg(asd->isp->dev, "%s: trying to inject a fake event 0x%x\n",
 		__func__, *event);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 5aa108a1724c..19ecc751d594 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -3626,6 +3626,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
 	struct atomisp_dis_buf *dis_buf;
 	unsigned long flags;
 
+	lockdep_assert_held(&isp->mutex);
+
 	if (!asd->params.dvs_stat->hor_prod.odd_real ||
 	    !asd->params.dvs_stat->hor_prod.odd_imag ||
 	    !asd->params.dvs_stat->hor_prod.even_real ||
@@ -3637,12 +3639,8 @@ int atomisp_css_get_dis_stat(struct atomisp_sub_device *asd,
 		return -EINVAL;
 
 	/* isp needs to be streaming to get DIS statistics */
-	spin_lock_irqsave(&isp->lock, flags);
-	if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED) {
-		spin_unlock_irqrestore(&isp->lock, flags);
+	if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
 		return -EINVAL;
-	}
-	spin_unlock_irqrestore(&isp->lock, flags);
 
 	if (atomisp_compare_dvs_grid(asd, &stats->dvs2_stat.grid_info) != 0)
 		/* If the grid info in the argument differs from the current
@@ -3827,6 +3825,8 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 	bool reset_wdt_timer[MAX_STREAM_NUM] = {false};
 	int i;
 
+	lockdep_assert_held(&isp->mutex);
+
 	while (!ia_css_dequeue_psys_event(&current_event.event)) {
 		if (current_event.event.type ==
 		    IA_CSS_EVENT_TYPE_FW_ASSERT) {
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 92cbc0e263e8..d24be2341a9b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -918,6 +918,7 @@ static int atomisp_release(struct file *file)
 	struct v4l2_requestbuffers req;
 	struct v4l2_subdev_fh fh;
 	struct v4l2_rect clear_compose = {0};
+	unsigned long flags;
 	int ret = 0;
 
 	v4l2_fh_init(&fh.vfh, vdev);
@@ -1008,7 +1009,9 @@ static int atomisp_release(struct file *file)
 
 	/* clear the asd field to show this camera is not used */
 	isp->inputs[asd->input_curr].asd = NULL;
+	spin_lock_irqsave(&isp->lock, flags);
 	asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+	spin_unlock_irqrestore(&isp->lock, flags);
 
 	if (atomisp_dev_users(isp))
 		goto done;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index f71ab1ee6e19..b86f9bd7574c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -280,7 +280,7 @@ struct atomisp_device {
 
 	atomic_t wdt_work_queued;
 
-	spinlock_t lock; /* Just for streaming below */
+	spinlock_t lock; /* Protects asd[i].streaming */
 
 	bool need_gfx_throttle;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 4de01aa28fe5..44ed8aa51fdc 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1925,7 +1925,9 @@ static int atomisp_streamon(struct file *file, void *fh,
 	if (ret)
 		goto out;
 
+	spin_lock_irqsave(&isp->lock, irqflags);
 	asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
+	spin_unlock_irqrestore(&isp->lock, irqflags);
 	atomic_set(&asd->sof_count, -1);
 	atomic_set(&asd->sequence, -1);
 	atomic_set(&asd->sequence_temp, -1);
@@ -2005,7 +2007,9 @@ static int atomisp_streamon(struct file *file, void *fh,
 	ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
 			       video, s_stream, 1);
 	if (ret) {
+		spin_lock_irqsave(&isp->lock, irqflags);
 		asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+		spin_unlock_irqrestore(&isp->lock, irqflags);
 		ret = -EINVAL;
 		goto out;
 	}
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 394fe6959033..5a3dd476f194 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -874,12 +874,18 @@ static int s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct atomisp_sub_device *asd = container_of(
 					     ctrl->handler, struct atomisp_sub_device, ctrl_handler);
+	unsigned int streaming;
+	unsigned long flags;
 
 	switch (ctrl->id) {
 	case V4L2_CID_RUN_MODE:
 		return __atomisp_update_run_mode(asd);
 	case V4L2_CID_DEPTH_MODE:
-		if (asd->streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
+		/* Use spinlock instead of mutex to avoid possible locking issues */
+		spin_lock_irqsave(&asd->isp->lock, flags);
+		streaming = asd->streaming;
+		spin_unlock_irqrestore(&asd->isp->lock, flags);
+		if (streaming != ATOMISP_DEVICE_STREAMING_DISABLED) {
 			dev_err(asd->isp->dev,
 				"ISP is streaming, it is not supported to change the depth mode\n");
 			return -EINVAL;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index 798a93793a9a..a955a38246cf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -364,7 +364,11 @@ struct atomisp_sub_device {
 	atomic_t sequence;      /* Sequence value that is assigned to buffer. */
 	atomic_t sequence_temp;
 
-	unsigned int streaming; /* Hold both mutex and lock to change this */
+	/*
+	 * Writers of streaming must hold both isp->mutex and isp->lock.
+	 * Readers of streaming need to hold only one of the two locks.
+	 */
+	unsigned int streaming;
 	bool stream_prepared; /* whether css stream is created */
 
 	/* subdev index: will be used to show which subdev is holding the
-- 
2.35.1

Re: [PATCH AUTOSEL 6.0 15/34] media: atomisp-ov2680: Fix ov2680_set_fmt()

[Hans de Goede]

Hi Sasha,

I have no specific objections against the backporting of this
and other atomisp related patches.

But in general the atomisp driver is not yet in a state where
it is ready to be used by normal users. Progress is being made
but atm I don't really expect normal users to have it enabled /
in active use.

As such I'm also not sure if there is much value in backporting
atomisp changes to the stable series.

I don't know if you have a way to opt out certain drivers /
file-paths from stable series backporting, but if you do
you may want to consider opting out everything under:

drivers/staging/media/atomisp/

As said above I don't think doing the backports offers
much (if any) value to end users and I assume it does take
you some time, so opting this path out might be better.

Also given the fragile state of atomisp support atm
it is hard to say for me if partially backporting some of
the changes won't break the driver.

Regards,

Hans




On 11/1/22 12:27, Sasha Levin wrote:
> From: Hans de Goede <hdegoede@redhat.com>
> 
> [ Upstream commit adea153b4f6537f367fe77abada263fde8a1f7b6 ]
> 
> 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
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  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;

Re: [PATCH AUTOSEL 6.0 15/34] media: atomisp-ov2680: Fix ov2680_set_fmt()

[Sasha Levin]

On Tue, Nov 01, 2022 at 02:27:53PM +0100, Hans de Goede wrote:
>Hi Sasha,
>
>I have no specific objections against the backporting of this
>and other atomisp related patches.
>
>But in general the atomisp driver is not yet in a state where
>it is ready to be used by normal users. Progress is being made
>but atm I don't really expect normal users to have it enabled /
>in active use.
>
>As such I'm also not sure if there is much value in backporting
>atomisp changes to the stable series.
>
>I don't know if you have a way to opt out certain drivers /
>file-paths from stable series backporting, but if you do
>you may want to consider opting out everything under:
>
>drivers/staging/media/atomisp/
>
>As said above I don't think doing the backports offers
>much (if any) value to end users and I assume it does take
>you some time, so opting this path out might be better.
>
>Also given the fragile state of atomisp support atm
>it is hard to say for me if partially backporting some of
>the changes won't break the driver.

I'll blacklist drivers/staging/media/atomisp/, thank you!

-- 
Thanks,
Sasha