[PATCH 00/17] media: atomisp: further cleanups / unwanted code removal

[PATCH 00/17] media: atomisp: further cleanups / unwanted code removal

[Hans de Goede]

Hi All,

Here is another atomisp patch-series with further cleanups / unwanted code
removal. Note this mostly cleaning up things which I hit while I'm still
working towards converting the driver to videobuf2.

The main goal of this series was to make the atomisp locking more like
that of other v4l2 drivers, which is acomplished in the
"media: atomisp: Use video_dev.lock for ioctl locking" patch.

Regards,

Hans


Andy Shevchenko (1):
  media: atomisp_gmin_platform: Unexport and split camera_sensor_csi()

Hans de Goede (16):
  media: atomisp: Use a normal mutex for the main lock
  media: atomisp: Remove unused lock member from struct
    atomisp_sub_device
  media: atomisp: Fix locking around asd->streaming read/write
  media: atomisp: Remove asd == NULL checks from ioctl handling
  media: atomisp: Add atomisp_pipe_check() helper
  media: atomisp: Remove watchdog timer
  media: atomisp: Move atomisp_streaming_count() check into
    __atomisp_css_recover()
  media: atomisp: Rework asd->streaming state update in
    __atomisp_streamoff()
  media: atomisp: Drop streamoff_mutex
  media: atomisp: Use video_dev.lock for ioctl locking
  media: atomisp: Remove a couple of not useful function wrappers
  media: atomisp: Drop unnecessary first_streamoff check
  media: atomisp: Make atomisp_set_raw_buffer_bitmap() static
  media: atomisp: Remove unused atomisp_css_get_dis_statistics()
  media: atomisp: Remove const/fixed camera_caps
  media: atomisp: Remove atomisp_source_pad_to_stream_id()

 .../include/linux/atomisp_gmin_platform.h     |   2 -
 .../atomisp/include/linux/atomisp_platform.h  |  18 -
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 581 ++----------------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |   9 +-
 .../media/atomisp/pci/atomisp_compat.h        |   4 -
 .../media/atomisp/pci/atomisp_compat_css20.c  |  74 +--
 .../staging/media/atomisp/pci/atomisp_fops.c  |  87 +--
 .../media/atomisp/pci/atomisp_gmin_platform.c |  86 ++-
 .../media/atomisp/pci/atomisp_internal.h      |  39 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 567 ++++-------------
 .../staging/media/atomisp/pci/atomisp_ioctl.h |  10 +-
 .../media/atomisp/pci/atomisp_subdev.c        |  34 +-
 .../media/atomisp/pci/atomisp_subdev.h        |  24 +-
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  65 +-
 14 files changed, 267 insertions(+), 1333 deletions(-)

-- 
2.37.3

[PATCH 01/17] media: atomisp: Use a normal mutex for the main lock

[Hans de Goede]

There is no reason for atomisp to use a rt_mutex instead of a normal
mutex, so switch over to a normal mutex.

All the changes in this patch are just s/rt_mutex/mutex/.

This is a preparation patch for switching the ioctl locking over
to using the video_dev.lock member so that the v4l2-core takes
care of the locking.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   |  12 +-
 .../staging/media/atomisp/pci/atomisp_fops.c  |  28 ++---
 .../media/atomisp/pci/atomisp_internal.h      |   2 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 106 +++++++++---------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   2 +-
 5 files changed, 75 insertions(+), 75 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 1c4748b7186e..97ef02e4e7a6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1446,10 +1446,10 @@ void atomisp_wdt_work(struct work_struct *work)
 	unsigned int pipe_wdt_cnt[MAX_STREAM_NUM][4] = { {0} };
 	bool css_recover = true;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (!atomisp_streaming_count(isp)) {
 		atomic_set(&isp->wdt_work_queued, 0);
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return;
 	}
 
@@ -1581,7 +1581,7 @@ void atomisp_wdt_work(struct work_struct *work)
 			isp->isp_fatal_error = true;
 			atomic_set(&isp->wdt_work_queued, 0);
 
-			rt_mutex_unlock(&isp->mutex);
+			mutex_unlock(&isp->mutex);
 			return;
 		}
 	}
@@ -1601,7 +1601,7 @@ void atomisp_wdt_work(struct work_struct *work)
 	dev_err(isp->dev, "timeout recovery handling done\n");
 	atomic_set(&isp->wdt_work_queued, 0);
 
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 }
 
 void atomisp_css_flush(struct atomisp_device *isp)
@@ -1861,7 +1861,7 @@ irqreturn_t atomisp_isr_thread(int irq, void *isp_ptr)
 	 * For CSS2.0: we change the way to not dequeue all the event at one
 	 * time, instead, dequue one and process one, then another
 	 */
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (atomisp_css_isr_thread(isp, frame_done_found, css_pipe_done))
 		goto out;
 
@@ -1872,7 +1872,7 @@ irqreturn_t atomisp_isr_thread(int irq, void *isp_ptr)
 		atomisp_setup_flash(asd);
 	}
 out:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	dev_dbg(isp->dev, "<%s\n", __func__);
 
 	return IRQ_HANDLED;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index fa37defa5ecc..57587d739c4b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -711,7 +711,7 @@ static int atomisp_open(struct file *file)
 	if (ret)
 		return ret;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	asd->subdev.devnode = vdev;
 	/* Deferred firmware loading case. */
@@ -745,7 +745,7 @@ static int atomisp_open(struct file *file)
 	 */
 	if (pipe->users) {
 		dev_dbg(isp->dev, "video node already opened\n");
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return -EBUSY;
 	}
 
@@ -788,7 +788,7 @@ static int atomisp_open(struct file *file)
 
 done:
 	pipe->users++;
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	/* Ensure that a mode is set */
 	v4l2_ctrl_s_ctrl(asd->run_mode, pipe->default_run_mode);
@@ -799,7 +799,7 @@ static int atomisp_open(struct file *file)
 	atomisp_css_uninit(isp);
 	pm_runtime_put(vdev->v4l2_dev->dev);
 error:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	v4l2_fh_release(file);
 	return ret;
 }
@@ -822,7 +822,7 @@ static int atomisp_release(struct file *file)
 		return -EBADF;
 
 	mutex_lock(&isp->streamoff_mutex);
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	dev_dbg(isp->dev, "release device %s\n", vdev->name);
 
@@ -905,7 +905,7 @@ static int atomisp_release(struct file *file)
 				     atomisp_subdev_source_pad(vdev),
 				     V4L2_SEL_TGT_COMPOSE, 0,
 				     &clear_compose);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	mutex_unlock(&isp->streamoff_mutex);
 
 	return v4l2_fh_release(file);
@@ -1063,7 +1063,7 @@ static int atomisp_mmap(struct file *file, struct vm_area_struct *vma)
 	if (!(vma->vm_flags & (VM_WRITE | VM_READ)))
 		return -EACCES;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	if (!(vma->vm_flags & VM_SHARED)) {
 		/* Map private buffer.
@@ -1074,7 +1074,7 @@ static int atomisp_mmap(struct file *file, struct vm_area_struct *vma)
 		 */
 		vma->vm_flags |= VM_SHARED;
 		ret = hmm_mmap(vma, vma->vm_pgoff << PAGE_SHIFT);
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return ret;
 	}
 
@@ -1117,7 +1117,7 @@ static int atomisp_mmap(struct file *file, struct vm_area_struct *vma)
 		}
 		raw_virt_addr->data_bytes = origin_size;
 		vma->vm_flags |= VM_IO | VM_DONTEXPAND | VM_DONTDUMP;
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return 0;
 	}
 
@@ -1129,12 +1129,12 @@ static int atomisp_mmap(struct file *file, struct vm_area_struct *vma)
 		ret = -EINVAL;
 		goto error;
 	}
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return atomisp_videobuf_mmap_mapper(&pipe->capq, vma);
 
 error:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return ret;
 }
@@ -1146,12 +1146,12 @@ static __poll_t atomisp_poll(struct file *file,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (pipe->capq.streaming != 1) {
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return EPOLLERR;
 	}
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return videobuf_poll_stream(file, &pipe->capq, pt);
 }
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index fc7bd877dae8..759575cbd356 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -238,7 +238,7 @@ struct atomisp_device {
 
 	/* Purpose of mutex is to protect and serialize use of isp data
 	 * structures and css API calls. */
-	struct rt_mutex mutex;
+	struct mutex mutex;
 	/*
 	 * Serialise streamoff: mutex is dropped during streamoff to
 	 * cancel the watchdog queue. MUST be acquired BEFORE
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index caeb38eadc48..4016ac4fffe0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -638,9 +638,9 @@ static int atomisp_g_input(struct file *file, void *fh, unsigned int *input)
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	*input = asd->input_curr;
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return 0;
 }
@@ -663,7 +663,7 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
 		ret = -EINVAL;
@@ -743,12 +743,12 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	asd->input_curr = input;
 	/* mark this camera is used by the current stream */
 	isp->inputs[input].asd = asd;
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return 0;
 
 error:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return ret;
 }
@@ -831,7 +831,7 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	rval = v4l2_subdev_call(camera, pad, enum_mbus_code, NULL, &code);
 	if (rval == -ENOIOCTLCMD) {
@@ -839,7 +839,7 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 			 "enum_mbus_code pad op not supported by %s. Please fix your sensor driver!\n",
 			 camera->name);
 	}
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	if (rval)
 		return rval;
@@ -952,9 +952,9 @@ static int atomisp_try_fmt_cap(struct file *file, void *fh,
 	f->fmt.pix.width += pad_w;
 	f->fmt.pix.height += pad_h;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	ret = atomisp_try_fmt(vdev, &f->fmt.pix, NULL);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	if (ret)
 		return ret;
@@ -969,9 +969,9 @@ static int atomisp_g_fmt_cap(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_video_pipe *pipe;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	pipe = atomisp_to_video_pipe(vdev);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	f->fmt.pix = pipe->pix;
 
@@ -993,14 +993,14 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (isp->isp_fatal_error) {
 		ret = -EIO;
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return ret;
 	}
 	ret = atomisp_set_fmt(vdev, f);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -1224,9 +1224,9 @@ int atomisp_reqbufs(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	ret = __atomisp_reqbufs(file, fh, req);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return ret;
 }
@@ -1267,7 +1267,7 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (isp->isp_fatal_error) {
 		ret = -EIO;
 		goto error;
@@ -1366,10 +1366,10 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 
 	pipe->frame_params[buf->index] = NULL;
 
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	ret = videobuf_qbuf(&pipe->capq, buf);
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (ret)
 		goto error;
 
@@ -1409,7 +1409,7 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 			asd->pending_capture_request++;
 			dev_dbg(isp->dev, "Add one pending capture request.\n");
 	}
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	dev_dbg(isp->dev, "qbuf buffer %d (%s) for asd%d\n", buf->index,
 		vdev->name, asd->index);
@@ -1417,7 +1417,7 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	return ret;
 
 error:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -1455,21 +1455,21 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	if (isp->isp_fatal_error) {
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return -EIO;
 	}
 
 	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
 			__func__);
 		return -EIO;
 	}
 
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
 	if (ret) {
@@ -1477,7 +1477,7 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 			dev_dbg(isp->dev, "<%s: %d\n", __func__, ret);
 		return ret;
 	}
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	buf->bytesused = pipe->pix.sizeimage;
 	buf->reserved = asd->frame_status[buf->index];
 
@@ -1491,7 +1491,7 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	if (!(buf->flags & V4L2_BUF_FLAG_ERROR))
 		buf->reserved |= __get_frame_exp_id(pipe, buf) << 16;
 	buf->reserved2 = pipe->frame_config_id[buf->index];
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	dev_dbg(isp->dev,
 		"dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
@@ -1720,7 +1720,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	if (isp->isp_fatal_error) {
 		ret = -EIO;
 		goto out;
@@ -1774,11 +1774,11 @@ static int atomisp_streamon(struct file *file, void *fh,
 
 			if (asd->delayed_init == ATOMISP_DELAYED_INIT_QUEUED) {
 				flush_work(&asd->delayed_init_work);
-				rt_mutex_unlock(&isp->mutex);
+				mutex_unlock(&isp->mutex);
 				if (wait_for_completion_interruptible(
 					&asd->init_done) != 0)
 					return -ERESTARTSYS;
-				rt_mutex_lock(&isp->mutex);
+				mutex_lock(&isp->mutex);
 			}
 
 			/* handle per_frame_setting parameter and buffers */
@@ -1938,7 +1938,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 		asd->delayed_init = ATOMISP_DELAYED_INIT_NOT_QUEUED;
 	}
 out:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -2036,9 +2036,9 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	if (first_streamoff) {
 		/* if other streams are running, should not disable watch dog */
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		atomisp_wdt_stop(asd, true);
-		rt_mutex_lock(&isp->mutex);
+		mutex_lock(&isp->mutex);
 	}
 
 	spin_lock_irqsave(&isp->lock, flags);
@@ -2188,9 +2188,9 @@ static int atomisp_streamoff(struct file *file, void *fh,
 	int rval;
 
 	mutex_lock(&isp->streamoff_mutex);
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	rval = __atomisp_streamoff(file, fh, type);
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	mutex_unlock(&isp->streamoff_mutex);
 
 	return rval;
@@ -2225,7 +2225,7 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	switch (control->id) {
 	case V4L2_CID_IRIS_ABSOLUTE:
@@ -2248,7 +2248,7 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	case V4L2_CID_TEST_PATTERN_COLOR_GR:
 	case V4L2_CID_TEST_PATTERN_COLOR_GB:
 	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
 				   ctrl_handler, control);
 	case V4L2_CID_COLORFX:
@@ -2277,7 +2277,7 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 		break;
 	}
 
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -2310,7 +2310,7 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	switch (control->id) {
 	case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
 	case V4L2_CID_EXPOSURE:
@@ -2331,7 +2331,7 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	case V4L2_CID_TEST_PATTERN_COLOR_GR:
 	case V4L2_CID_TEST_PATTERN_COLOR_GB:
 	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		return v4l2_s_ctrl(NULL,
 				   isp->inputs[asd->input_curr].camera->
 				   ctrl_handler, control);
@@ -2363,7 +2363,7 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 		ret = -EINVAL;
 		break;
 	}
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -2488,9 +2488,9 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 						&ctrl);
 			break;
 		case V4L2_CID_ZOOM_ABSOLUTE:
-			rt_mutex_lock(&isp->mutex);
+			mutex_lock(&isp->mutex);
 			ret = atomisp_digital_zoom(asd, 0, &ctrl.value);
-			rt_mutex_unlock(&isp->mutex);
+			mutex_unlock(&isp->mutex);
 			break;
 		case V4L2_CID_G_SKIP_FRAMES:
 			ret = v4l2_subdev_call(
@@ -2603,7 +2603,7 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 		case V4L2_CID_FLASH_STROBE:
 		case V4L2_CID_FLASH_MODE:
 		case V4L2_CID_FLASH_STATUS_REGISTER:
-			rt_mutex_lock(&isp->mutex);
+			mutex_lock(&isp->mutex);
 			if (isp->flash) {
 				ret =
 				    v4l2_s_ctrl(NULL, isp->flash->ctrl_handler,
@@ -2618,12 +2618,12 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 					asd->params.num_flash_frames = 0;
 				}
 			}
-			rt_mutex_unlock(&isp->mutex);
+			mutex_unlock(&isp->mutex);
 			break;
 		case V4L2_CID_ZOOM_ABSOLUTE:
-			rt_mutex_lock(&isp->mutex);
+			mutex_lock(&isp->mutex);
 			ret = atomisp_digital_zoom(asd, 1, &ctrl.value);
-			rt_mutex_unlock(&isp->mutex);
+			mutex_unlock(&isp->mutex);
 			break;
 		default:
 			ctr = v4l2_ctrl_find(&asd->ctrl_handler, ctrl.id);
@@ -2691,9 +2691,9 @@ static int atomisp_g_parm(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 	parm->parm.capture.capturemode = asd->run_mode->val;
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return 0;
 }
@@ -2719,7 +2719,7 @@ static int atomisp_s_parm(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	rt_mutex_lock(&isp->mutex);
+	mutex_lock(&isp->mutex);
 
 	asd->high_speed_mode = false;
 	switch (parm->parm.capture.capturemode) {
@@ -2761,7 +2761,7 @@ static int atomisp_s_parm(struct file *file, void *fh,
 	rval = v4l2_ctrl_s_ctrl(asd->run_mode, mode);
 
 out:
-	rt_mutex_unlock(&isp->mutex);
+	mutex_unlock(&isp->mutex);
 
 	return rval == -ENOIOCTLCMD ? 0 : rval;
 }
@@ -2795,7 +2795,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		/* we do not need take isp->mutex for these IOCTLs */
 		break;
 	default:
-		rt_mutex_lock(&isp->mutex);
+		mutex_lock(&isp->mutex);
 		break;
 	}
 	switch (cmd) {
@@ -3057,7 +3057,7 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 	case ATOMISP_IOC_G_UPDATE_EXPOSURE:
 		break;
 	default:
-		rt_mutex_unlock(&isp->mutex);
+		mutex_unlock(&isp->mutex);
 		break;
 	}
 	return err;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 4d73bf3d6421..aa38e0d33b5b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1514,7 +1514,7 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
 
 	dev_dbg(&pdev->dev, "atomisp mmio base: %p\n", isp->base);
 
-	rt_mutex_init(&isp->mutex);
+	mutex_init(&isp->mutex);
 	mutex_init(&isp->streamoff_mutex);
 	spin_lock_init(&isp->lock);
 
-- 
2.37.3

Re: [PATCH 01/17] media: atomisp: Use a normal mutex for the main lock

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 07:16:37PM +0200, Hans de Goede wrote:
> There is no reason for atomisp to use a rt_mutex instead of a normal
> mutex, so switch over to a normal mutex.
> 
> All the changes in this patch are just s/rt_mutex/mutex/.
> 
> This is a preparation patch for switching the ioctl locking over
> to using the video_dev.lock member so that the v4l2-core takes
> care of the locking.

So the idea behind rt_mutex here is to inherit the priority on the task.
I'm wondering what could be possible the bottle neck this is trying to
solve. If there is no other V4L2 driver that does the same, any specific
run flow of AtomISP v2 code that may suffer of this?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 01/17] media: atomisp: Use a normal mutex for the main lock

[Hans de Goede]

Hi,

On 9/12/22 13:11, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 07:16:37PM +0200, Hans de Goede wrote:
>> There is no reason for atomisp to use a rt_mutex instead of a normal
>> mutex, so switch over to a normal mutex.
>>
>> All the changes in this patch are just s/rt_mutex/mutex/.
>>
>> This is a preparation patch for switching the ioctl locking over
>> to using the video_dev.lock member so that the v4l2-core takes
>> care of the locking.
> 
> So the idea behind rt_mutex here is to inherit the priority on the task.

Right.

> I'm wondering what could be possible the bottle neck this is trying to
> solve.

I don't think there is any specific reasoning behind the code using
this. The atomisp code is quite questionable in lots of cases and
I have a feeling this was just a case of "oh this sounds like
it is faster, lets use this" .

> If there is no other V4L2 driver that does the same, any specific
> run flow of AtomISP v2 code that may suffer of this?

See above.

Regards,

Hans

Re: [PATCH 01/17] media: atomisp: Use a normal mutex for the main lock

[Andy Shevchenko]

On Wed, Sep 21, 2022 at 11:52 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/12/22 13:11, Andy Shevchenko wrote:
> > On Sun, Sep 11, 2022 at 07:16:37PM +0200, Hans de Goede wrote:
> >> There is no reason for atomisp to use a rt_mutex instead of a normal
> >> mutex, so switch over to a normal mutex.
> >>
> >> All the changes in this patch are just s/rt_mutex/mutex/.
> >>
> >> This is a preparation patch for switching the ioctl locking over
> >> to using the video_dev.lock member so that the v4l2-core takes
> >> care of the locking.
> >
> > So the idea behind rt_mutex here is to inherit the priority on the task.
>
> Right.
>
> > I'm wondering what could be possible the bottle neck this is trying to
> > solve.
>
> I don't think there is any specific reasoning behind the code using
> this. The atomisp code is quite questionable in lots of cases and
> I have a feeling this was just a case of "oh this sounds like
> it is faster, lets use this" .

Honestly, this is exactly what is my attitude towards the AtomISP2
codebase. But I asked to be sure that you probably investigated that
more.

> > If there is no other V4L2 driver that does the same, any specific
> > run flow of AtomISP v2 code that may suffer of this?
>
> See above.


-- 
With Best Regards,
Andy Shevchenko

[PATCH 02/17] media: atomisp: Remove unused lock member from struct atomisp_sub_device

[Hans de Goede]

The spin-lock embedded in struct atomisp_sub_device is not used anywhere,
remove it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_subdev.c | 1 -
 drivers/staging/media/atomisp/pci/atomisp_subdev.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 047e2e9d63d7..4a4367701509 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -1356,7 +1356,6 @@ int atomisp_subdev_init(struct atomisp_device *isp)
 		return -ENOMEM;
 	for (i = 0; i < isp->num_of_streams; i++) {
 		asd = &isp->asd[i];
-		spin_lock_init(&asd->lock);
 		asd->isp = isp;
 		isp_subdev_init_params(asd);
 		asd->index = i;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index d8b2dd00a792..eaf767880407 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -272,7 +272,6 @@ struct atomisp_sub_device {
 	/* video pipe main output */
 	struct atomisp_video_pipe video_out_video_capture;
 	/* struct isp_subdev_params params; */
-	spinlock_t lock;
 	struct atomisp_device *isp;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *fmt_auto;
-- 
2.37.3

[PATCH 03/17] media: atomisp: Fix locking around asd->streaming read/write

[Hans de Goede]

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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../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 97ef02e4e7a6..c7f825e38921 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -899,6 +899,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 &&
@@ -1298,6 +1300,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);
 
 	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
 
@@ -1320,7 +1325,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(
@@ -1335,7 +1342,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;
@@ -1376,11 +1385,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);
 	}
@@ -1608,6 +1620,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
 {
 	int i;
 
+	lockdep_assert_held(&isp->mutex);
+
 	if (!atomisp_streaming_count(isp))
 		return;
 
@@ -4046,6 +4060,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);
@@ -4139,6 +4155,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);
@@ -5537,6 +5555,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);
@@ -6159,6 +6179,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
@@ -6433,6 +6455,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);
@@ -6454,6 +6478,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);
@@ -6489,6 +6515,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 cda0b5eba16d..15ef31b0c601 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
@@ -3801,6 +3799,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 57587d739c4b..e1b213ba4686 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -813,6 +813,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);
@@ -878,7 +879,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 759575cbd356..b2c362ef7199 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -266,7 +266,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 4016ac4fffe0..21af5feca386 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1837,7 +1837,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);
@@ -1910,7 +1912,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 4a4367701509..88bf693f4c50 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 eaf767880407..b44f060b0bb5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -330,7 +330,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.37.3

Re: [PATCH 03/17] media: atomisp: Fix locking around asd->streaming read/write

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 07:16:39PM +0200, Hans de Goede wrote:
> 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>

Although description of the 'streaming' doesn't clarify which one should be
used on which circumstances. I assume it's only for sleeping / non-sleeping
context? If there is an IRQ relation, shouldn't we always use spinlock for
readers?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../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 97ef02e4e7a6..c7f825e38921 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -899,6 +899,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 &&
> @@ -1298,6 +1300,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);
>  
>  	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
>  
> @@ -1320,7 +1325,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(
> @@ -1335,7 +1342,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;
> @@ -1376,11 +1385,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);
>  	}
> @@ -1608,6 +1620,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
>  {
>  	int i;
>  
> +	lockdep_assert_held(&isp->mutex);
> +
>  	if (!atomisp_streaming_count(isp))
>  		return;
>  
> @@ -4046,6 +4060,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);
> @@ -4139,6 +4155,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);
> @@ -5537,6 +5555,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);
> @@ -6159,6 +6179,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
> @@ -6433,6 +6455,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);
> @@ -6454,6 +6478,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);
> @@ -6489,6 +6515,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 cda0b5eba16d..15ef31b0c601 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
> @@ -3801,6 +3799,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 57587d739c4b..e1b213ba4686 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
> @@ -813,6 +813,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);
> @@ -878,7 +879,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 759575cbd356..b2c362ef7199 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> @@ -266,7 +266,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 4016ac4fffe0..21af5feca386 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -1837,7 +1837,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);
> @@ -1910,7 +1912,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 4a4367701509..88bf693f4c50 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 eaf767880407..b44f060b0bb5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
> @@ -330,7 +330,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.37.3
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 03/17] media: atomisp: Fix locking around asd->streaming read/write

[Hans de Goede]

Hi,

On 9/12/22 13:26, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 07:16:39PM +0200, Hans de Goede wrote:
>> 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>

Thanks.

> Although description of the 'streaming' doesn't clarify which one should be
> used on which circumstances. I assume it's only for sleeping / non-sleeping
> context?

Correct.

> If there is an IRQ relation, shouldn't we always use spinlock for
> readers?

As the comment says writers always should take both locks. IOW there
are no writers in non-sleeping contexts.

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../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 97ef02e4e7a6..c7f825e38921 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
>> @@ -899,6 +899,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 &&
>> @@ -1298,6 +1300,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);
>>  
>>  	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
>>  
>> @@ -1320,7 +1325,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(
>> @@ -1335,7 +1342,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;
>> @@ -1376,11 +1385,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);
>>  	}
>> @@ -1608,6 +1620,8 @@ void atomisp_css_flush(struct atomisp_device *isp)
>>  {
>>  	int i;
>>  
>> +	lockdep_assert_held(&isp->mutex);
>> +
>>  	if (!atomisp_streaming_count(isp))
>>  		return;
>>  
>> @@ -4046,6 +4060,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);
>> @@ -4139,6 +4155,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);
>> @@ -5537,6 +5555,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);
>> @@ -6159,6 +6179,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
>> @@ -6433,6 +6455,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);
>> @@ -6454,6 +6478,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);
>> @@ -6489,6 +6515,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 cda0b5eba16d..15ef31b0c601 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
>> @@ -3801,6 +3799,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 57587d739c4b..e1b213ba4686 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
>> @@ -813,6 +813,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);
>> @@ -878,7 +879,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 759575cbd356..b2c362ef7199 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
>> @@ -266,7 +266,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 4016ac4fffe0..21af5feca386 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
>> @@ -1837,7 +1837,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);
>> @@ -1910,7 +1912,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 4a4367701509..88bf693f4c50 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 eaf767880407..b44f060b0bb5 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
>> @@ -330,7 +330,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.37.3
>>
> 

[PATCH 04/17] media: atomisp: Remove asd == NULL checks from ioctl handling

[Hans de Goede]

At probe time isp_subdev_init_entities() sets pipe->asd to a non NULL
value for all four (preview/vf/capture/capture_video) pipes by calling
atomisp_init_subdev_pipe() for all 4 pipes.

So it can never be NULL. Remove the redundant NULL checks.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 18 ----
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 89 -------------------
 2 files changed, 107 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index c7f825e38921..087078900415 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1755,12 +1755,6 @@ void atomisp_wdt_refresh(struct atomisp_sub_device *asd, unsigned int delay)
 /* ISP2401 */
 void atomisp_wdt_stop_pipe(struct atomisp_video_pipe *pipe, bool sync)
 {
-	if (!pipe->asd) {
-		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, pipe->vdev.name);
-		return;
-	}
-
 	if (!atomisp_is_wdt_running(pipe))
 		return;
 
@@ -5557,12 +5551,6 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 
 	lockdep_assert_held(&isp->mutex);
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
 		return -EINVAL;
 
@@ -6587,12 +6575,6 @@ int atomisp_get_invalid_frame_num(struct video_device *vdev,
 	struct ia_css_pipe_info p_info;
 	int ret;
 
-	if (!asd) {
-		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (asd->isp->inputs[asd->input_curr].camera_caps->
 	    sensor[asd->sensor_curr].stream_num > 1) {
 		/* External ISP */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 21af5feca386..9c7022be3a06 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -632,12 +632,6 @@ static int atomisp_g_input(struct file *file, void *fh, unsigned int *input)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	mutex_lock(&isp->mutex);
 	*input = asd->input_curr;
 	mutex_unlock(&isp->mutex);
@@ -657,12 +651,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	struct v4l2_subdev *motor;
 	int ret;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	mutex_lock(&isp->mutex);
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
@@ -818,12 +806,6 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 	unsigned int i, fi = 0;
 	int rval;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	camera = isp->inputs[asd->input_curr].camera;
 	if(!camera) {
 		dev_err(isp->dev, "%s(): camera is NULL, device is %s\n",
@@ -1152,11 +1134,6 @@ int __atomisp_reqbufs(struct file *file, void *fh,
 	u16 stream_id;
 	int ret = 0, i = 0;
 
-	if (!asd) {
-		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
 	stream_id = atomisp_source_pad_to_stream_id(asd, source_pad);
 
 	if (req->count == 0) {
@@ -1261,12 +1238,6 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	u32 pgnr;
 	int ret = 0;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	mutex_lock(&isp->mutex);
 	if (isp->isp_fatal_error) {
 		ret = -EIO;
@@ -1449,12 +1420,6 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret = 0;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	mutex_lock(&isp->mutex);
 
 	if (isp->isp_fatal_error) {
@@ -1706,12 +1671,6 @@ static int atomisp_streamon(struct file *file, void *fh,
 	int ret = 0;
 	unsigned long irqflags;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
 
@@ -1963,12 +1922,6 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	unsigned long flags;
 	bool first_streamoff = false;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	dev_dbg(isp->dev, "Stop stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
 
@@ -2213,12 +2166,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	for (i = 0; i < ctrls_num; i++) {
 		if (ci_v4l2_controls[i].id == control->id) {
 			ret = 0;
@@ -2298,12 +2245,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	for (i = 0; i < ctrls_num; i++) {
 		if (ci_v4l2_controls[i].id == control->id) {
 			ret = 0;
@@ -2385,12 +2326,6 @@ static int atomisp_queryctl(struct file *file, void *fh,
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	switch (qc->id) {
 	case V4L2_CID_FOCUS_ABSOLUTE:
 	case V4L2_CID_FOCUS_RELATIVE:
@@ -2436,12 +2371,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 	int i;
 	int ret = 0;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (!IS_ISP2401)
 		motor = isp->inputs[asd->input_curr].motor;
 	else
@@ -2553,12 +2482,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 	int i;
 	int ret = 0;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (!IS_ISP2401)
 		motor = isp->inputs[asd->input_curr].motor;
 	else
@@ -2684,12 +2607,6 @@ static int atomisp_g_parm(struct file *file, void *fh,
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		dev_err(isp->dev, "unsupported v4l2 buf type\n");
 		return -EINVAL;
@@ -2712,12 +2629,6 @@ static int atomisp_s_parm(struct file *file, void *fh,
 	int rval;
 	int fps;
 
-	if (!asd) {
-		dev_err(isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, vdev->name);
-		return -EINVAL;
-	}
-
 	if (parm->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		dev_err(isp->dev, "unsupported v4l2 buf type\n");
 		return -EINVAL;
-- 
2.37.3

[PATCH 05/17] media: atomisp: Add atomisp_pipe_check() helper

[Hans de Goede]

Several of the ioctl handlers all do the same checks
(isp->fatal_error and asd->streaming errors) add
an atomisp_pipe_check() helper for this.

Note this changes the vidioc_s_fmt_vid_cap and vidioc_s_input handlers
to now reject calls made while asd->streaming==STOPPING. This fixes
a possible race where one thread can make this ioctls while
vidioc_streamoff is running from another thread and it has
temporarily released isp->mutex to kill the watchdog timers / work.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 087078900415..7945852ecd13 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -5549,16 +5549,13 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
 	struct v4l2_subdev_fh fh;
 	int ret;
 
-	lockdep_assert_held(&isp->mutex);
+	ret = atomisp_pipe_check(pipe, true);
+	if (ret)
+		return ret;
 
 	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
 		return -EINVAL;
 
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
-		dev_warn(isp->dev, "ISP does not support set format while at streaming!\n");
-		return -EBUSY;
-	}
-
 	dev_dbg(isp->dev,
 		"setting resolution %ux%u on pad %u for asd%d, bytesperline %u\n",
 		f->fmt.pix.width, f->fmt.pix.height, source_pad,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 9c7022be3a06..9b50f637c46a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -535,6 +535,32 @@ atomisp_get_format_bridge_from_mbus(u32 mbus_code)
 	return NULL;
 }
 
+int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool settings_change)
+{
+	lockdep_assert_held(&pipe->isp->mutex);
+
+	if (pipe->isp->isp_fatal_error)
+		return -EIO;
+
+	switch (pipe->asd->streaming) {
+	case ATOMISP_DEVICE_STREAMING_DISABLED:
+		break;
+	case ATOMISP_DEVICE_STREAMING_ENABLED:
+		if (settings_change) {
+			dev_err(pipe->isp->dev, "Set fmt/input IOCTL while streaming\n");
+			return -EBUSY;
+		}
+		break;
+	case ATOMISP_DEVICE_STREAMING_STOPPING:
+		dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
+		return -EBUSY;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * v4l2 ioctls
  * return ISP capabilities
@@ -646,12 +672,18 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 {
 	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_video_pipe *pipe = atomisp_to_video_pipe(vdev);
+	struct atomisp_sub_device *asd = pipe->asd;
 	struct v4l2_subdev *camera = NULL;
 	struct v4l2_subdev *motor;
 	int ret;
 
 	mutex_lock(&isp->mutex);
+
+	ret = atomisp_pipe_check(pipe, true);
+	if (ret)
+		goto error;
+
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
 		ret = -EINVAL;
@@ -678,13 +710,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 		goto error;
 	}
 
-	if (atomisp_subdev_streaming_count(asd)) {
-		dev_err(isp->dev,
-			"ISP is still streaming, stop first\n");
-		ret = -EINVAL;
-		goto error;
-	}
-
 	/* power off the current owned sensor, as it is not used this time */
 	if (isp->inputs[asd->input_curr].asd == asd &&
 	    asd->input_curr != input) {
@@ -976,11 +1001,6 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
 	int ret;
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		mutex_unlock(&isp->mutex);
-		return ret;
-	}
 	ret = atomisp_set_fmt(vdev, f);
 	mutex_unlock(&isp->mutex);
 	return ret;
@@ -1236,20 +1256,13 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct ia_css_frame *handle = NULL;
 	u32 length;
 	u32 pgnr;
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		goto error;
-	}
 
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
-			__func__);
-		ret = -EIO;
+	ret = atomisp_pipe_check(pipe, false);
+	if (ret)
 		goto error;
-	}
 
 	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
 	    !pipe->capq.bufs[buf->index]) {
@@ -1418,23 +1431,13 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
-	int ret = 0;
+	int ret;
 
 	mutex_lock(&isp->mutex);
-
-	if (isp->isp_fatal_error) {
-		mutex_unlock(&isp->mutex);
-		return -EIO;
-	}
-
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		mutex_unlock(&isp->mutex);
-		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
-			__func__);
-		return -EIO;
-	}
-
+	ret = atomisp_pipe_check(pipe, false);
 	mutex_unlock(&isp->mutex);
+	if (ret)
+		return ret;
 
 	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
 	if (ret) {
@@ -1668,8 +1671,8 @@ static int atomisp_streamon(struct file *file, void *fh,
 	enum ia_css_pipe_id css_pipe_id;
 	unsigned int sensor_start_stream;
 	unsigned int wdt_duration = ATOMISP_ISP_TIMEOUT_DURATION;
-	int ret = 0;
 	unsigned long irqflags;
+	int ret;
 
 	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
 		atomisp_subdev_source_pad(vdev), asd->index);
@@ -1680,15 +1683,9 @@ static int atomisp_streamon(struct file *file, void *fh,
 	}
 
 	mutex_lock(&isp->mutex);
-	if (isp->isp_fatal_error) {
-		ret = -EIO;
-		goto out;
-	}
-
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
-		ret = -EBUSY;
+	ret = atomisp_pipe_check(pipe, false);
+	if (ret)
 		goto out;
-	}
 
 	if (pipe->capq.streaming)
 		goto out;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
index 382b78275240..61a6148a6ad5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
@@ -34,6 +34,8 @@ atomisp_format_bridge *atomisp_get_format_bridge(unsigned int pixelformat);
 const struct
 atomisp_format_bridge *atomisp_get_format_bridge_from_mbus(u32 mbus_code);
 
+int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool streaming_ok);
+
 int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 				uint16_t stream_id);
 
-- 
2.37.3

Re: [PATCH 05/17] media: atomisp: Add atomisp_pipe_check() helper

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 07:16:41PM +0200, Hans de Goede wrote:
> Several of the ioctl handlers all do the same checks
> (isp->fatal_error and asd->streaming errors) add
> an atomisp_pipe_check() helper for this.
> 
> Note this changes the vidioc_s_fmt_vid_cap and vidioc_s_input handlers
> to now reject calls made while asd->streaming==STOPPING. This fixes
> a possible race where one thread can make this ioctls while
> vidioc_streamoff is running from another thread and it has
> temporarily released isp->mutex to kill the watchdog timers / work.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

(One minor question below)

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../staging/media/atomisp/pci/atomisp_cmd.c   |  9 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 89 +++++++++----------
>  .../staging/media/atomisp/pci/atomisp_ioctl.h |  2 +
>  3 files changed, 48 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 087078900415..7945852ecd13 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -5549,16 +5549,13 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
>  	struct v4l2_subdev_fh fh;
>  	int ret;
>  
> -	lockdep_assert_held(&isp->mutex);
> +	ret = atomisp_pipe_check(pipe, true);
> +	if (ret)
> +		return ret;
>  
>  	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
>  		return -EINVAL;
>  
> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
> -		dev_warn(isp->dev, "ISP does not support set format while at streaming!\n");
> -		return -EBUSY;
> -	}
> -
>  	dev_dbg(isp->dev,
>  		"setting resolution %ux%u on pad %u for asd%d, bytesperline %u\n",
>  		f->fmt.pix.width, f->fmt.pix.height, source_pad,
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index 9c7022be3a06..9b50f637c46a 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -535,6 +535,32 @@ atomisp_get_format_bridge_from_mbus(u32 mbus_code)
>  	return NULL;
>  }
>  
> +int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool settings_change)
> +{
> +	lockdep_assert_held(&pipe->isp->mutex);
> +
> +	if (pipe->isp->isp_fatal_error)
> +		return -EIO;
> +
> +	switch (pipe->asd->streaming) {
> +	case ATOMISP_DEVICE_STREAMING_DISABLED:
> +		break;
> +	case ATOMISP_DEVICE_STREAMING_ENABLED:
> +		if (settings_change) {
> +			dev_err(pipe->isp->dev, "Set fmt/input IOCTL while streaming\n");
> +			return -EBUSY;
> +		}
> +		break;

> +	case ATOMISP_DEVICE_STREAMING_STOPPING:
> +		dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
> +		return -EBUSY;

Wouldn't -EAGAIN match better in this case?

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * v4l2 ioctls
>   * return ISP capabilities
> @@ -646,12 +672,18 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>  {
>  	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_video_pipe *pipe = atomisp_to_video_pipe(vdev);
> +	struct atomisp_sub_device *asd = pipe->asd;
>  	struct v4l2_subdev *camera = NULL;
>  	struct v4l2_subdev *motor;
>  	int ret;
>  
>  	mutex_lock(&isp->mutex);
> +
> +	ret = atomisp_pipe_check(pipe, true);
> +	if (ret)
> +		goto error;
> +
>  	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
>  		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
>  		ret = -EINVAL;
> @@ -678,13 +710,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>  		goto error;
>  	}
>  
> -	if (atomisp_subdev_streaming_count(asd)) {
> -		dev_err(isp->dev,
> -			"ISP is still streaming, stop first\n");
> -		ret = -EINVAL;
> -		goto error;
> -	}
> -
>  	/* power off the current owned sensor, as it is not used this time */
>  	if (isp->inputs[asd->input_curr].asd == asd &&
>  	    asd->input_curr != input) {
> @@ -976,11 +1001,6 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
>  	int ret;
>  
>  	mutex_lock(&isp->mutex);
> -	if (isp->isp_fatal_error) {
> -		ret = -EIO;
> -		mutex_unlock(&isp->mutex);
> -		return ret;
> -	}
>  	ret = atomisp_set_fmt(vdev, f);
>  	mutex_unlock(&isp->mutex);
>  	return ret;
> @@ -1236,20 +1256,13 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	struct ia_css_frame *handle = NULL;
>  	u32 length;
>  	u32 pgnr;
> -	int ret = 0;
> +	int ret;
>  
>  	mutex_lock(&isp->mutex);
> -	if (isp->isp_fatal_error) {
> -		ret = -EIO;
> -		goto error;
> -	}
>  
> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
> -		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
> -			__func__);
> -		ret = -EIO;
> +	ret = atomisp_pipe_check(pipe, false);
> +	if (ret)
>  		goto error;
> -	}
>  
>  	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
>  	    !pipe->capq.bufs[buf->index]) {
> @@ -1418,23 +1431,13 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
>  	struct atomisp_sub_device *asd = pipe->asd;
>  	struct atomisp_device *isp = video_get_drvdata(vdev);
> -	int ret = 0;
> +	int ret;
>  
>  	mutex_lock(&isp->mutex);
> -
> -	if (isp->isp_fatal_error) {
> -		mutex_unlock(&isp->mutex);
> -		return -EIO;
> -	}
> -
> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
> -		mutex_unlock(&isp->mutex);
> -		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
> -			__func__);
> -		return -EIO;
> -	}
> -
> +	ret = atomisp_pipe_check(pipe, false);
>  	mutex_unlock(&isp->mutex);
> +	if (ret)
> +		return ret;
>  
>  	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
>  	if (ret) {
> @@ -1668,8 +1671,8 @@ static int atomisp_streamon(struct file *file, void *fh,
>  	enum ia_css_pipe_id css_pipe_id;
>  	unsigned int sensor_start_stream;
>  	unsigned int wdt_duration = ATOMISP_ISP_TIMEOUT_DURATION;
> -	int ret = 0;
>  	unsigned long irqflags;
> +	int ret;
>  
>  	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
>  		atomisp_subdev_source_pad(vdev), asd->index);
> @@ -1680,15 +1683,9 @@ static int atomisp_streamon(struct file *file, void *fh,
>  	}
>  
>  	mutex_lock(&isp->mutex);
> -	if (isp->isp_fatal_error) {
> -		ret = -EIO;
> -		goto out;
> -	}
> -
> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
> -		ret = -EBUSY;
> +	ret = atomisp_pipe_check(pipe, false);
> +	if (ret)
>  		goto out;
> -	}
>  
>  	if (pipe->capq.streaming)
>  		goto out;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
> index 382b78275240..61a6148a6ad5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
> @@ -34,6 +34,8 @@ atomisp_format_bridge *atomisp_get_format_bridge(unsigned int pixelformat);
>  const struct
>  atomisp_format_bridge *atomisp_get_format_bridge_from_mbus(u32 mbus_code);
>  
> +int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool streaming_ok);
> +
>  int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
>  				uint16_t stream_id);
>  
> -- 
> 2.37.3
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 05/17] media: atomisp: Add atomisp_pipe_check() helper

[Hans de Goede]

Hi,

On 9/12/22 13:30, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 07:16:41PM +0200, Hans de Goede wrote:
>> Several of the ioctl handlers all do the same checks
>> (isp->fatal_error and asd->streaming errors) add
>> an atomisp_pipe_check() helper for this.
>>
>> Note this changes the vidioc_s_fmt_vid_cap and vidioc_s_input handlers
>> to now reject calls made while asd->streaming==STOPPING. This fixes
>> a possible race where one thread can make this ioctls while
>> vidioc_streamoff is running from another thread and it has
>> temporarily released isp->mutex to kill the watchdog timers / work.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> (One minor question below)
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../staging/media/atomisp/pci/atomisp_cmd.c   |  9 +-
>>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 89 +++++++++----------
>>  .../staging/media/atomisp/pci/atomisp_ioctl.h |  2 +
>>  3 files changed, 48 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
>> index 087078900415..7945852ecd13 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
>> @@ -5549,16 +5549,13 @@ int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
>>  	struct v4l2_subdev_fh fh;
>>  	int ret;
>>  
>> -	lockdep_assert_held(&isp->mutex);
>> +	ret = atomisp_pipe_check(pipe, true);
>> +	if (ret)
>> +		return ret;
>>  
>>  	if (source_pad >= ATOMISP_SUBDEV_PADS_NUM)
>>  		return -EINVAL;
>>  
>> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
>> -		dev_warn(isp->dev, "ISP does not support set format while at streaming!\n");
>> -		return -EBUSY;
>> -	}
>> -
>>  	dev_dbg(isp->dev,
>>  		"setting resolution %ux%u on pad %u for asd%d, bytesperline %u\n",
>>  		f->fmt.pix.width, f->fmt.pix.height, source_pad,
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
>> index 9c7022be3a06..9b50f637c46a 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
>> @@ -535,6 +535,32 @@ atomisp_get_format_bridge_from_mbus(u32 mbus_code)
>>  	return NULL;
>>  }
>>  
>> +int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool settings_change)
>> +{
>> +	lockdep_assert_held(&pipe->isp->mutex);
>> +
>> +	if (pipe->isp->isp_fatal_error)
>> +		return -EIO;
>> +
>> +	switch (pipe->asd->streaming) {
>> +	case ATOMISP_DEVICE_STREAMING_DISABLED:
>> +		break;
>> +	case ATOMISP_DEVICE_STREAMING_ENABLED:
>> +		if (settings_change) {
>> +			dev_err(pipe->isp->dev, "Set fmt/input IOCTL while streaming\n");
>> +			return -EBUSY;
>> +		}
>> +		break;
> 
>> +	case ATOMISP_DEVICE_STREAMING_STOPPING:
>> +		dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
>> +		return -EBUSY;
> 
> Wouldn't -EAGAIN match better in this case?

The original checks this replaces used -EIO (which seems like a poor
choice) resp. -EBUSY (in the streamon callback) so I decided to
keep the -EBUSY here.

Also AFAIK -EAGAIN will make the C-library retry the syscal itself
in some cases ? (not sure if this applies to ioctls though).

This is not what we want, this scenario can only be hit when an app:
1. Uses both the preview and the actual capture /dev/video# nodes
   at the same time (this is allows)
2. Then stops the stream at 1 of them, this transitions the state
   to STOPPING
3. Then does some ioctl other then streamoff on the other /dev/video#

Basically when using more then 1 /dev/video# node then the app must
stop all of them when stopping things. The driver enforces this
by rejecting all calls other the streamoff until all /dev/video#
node streans are off.

This means that simply trying again will result in the same error,
so -EBUSY seems like the best error for this.

Regards,

Hans

   
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  /*
>>   * v4l2 ioctls
>>   * return ISP capabilities
>> @@ -646,12 +672,18 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>>  {
>>  	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_video_pipe *pipe = atomisp_to_video_pipe(vdev);
>> +	struct atomisp_sub_device *asd = pipe->asd;
>>  	struct v4l2_subdev *camera = NULL;
>>  	struct v4l2_subdev *motor;
>>  	int ret;
>>  
>>  	mutex_lock(&isp->mutex);
>> +
>> +	ret = atomisp_pipe_check(pipe, true);
>> +	if (ret)
>> +		goto error;
>> +
>>  	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
>>  		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
>>  		ret = -EINVAL;
>> @@ -678,13 +710,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>>  		goto error;
>>  	}
>>  
>> -	if (atomisp_subdev_streaming_count(asd)) {
>> -		dev_err(isp->dev,
>> -			"ISP is still streaming, stop first\n");
>> -		ret = -EINVAL;
>> -		goto error;
>> -	}
>> -
>>  	/* power off the current owned sensor, as it is not used this time */
>>  	if (isp->inputs[asd->input_curr].asd == asd &&
>>  	    asd->input_curr != input) {
>> @@ -976,11 +1001,6 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
>>  	int ret;
>>  
>>  	mutex_lock(&isp->mutex);
>> -	if (isp->isp_fatal_error) {
>> -		ret = -EIO;
>> -		mutex_unlock(&isp->mutex);
>> -		return ret;
>> -	}
>>  	ret = atomisp_set_fmt(vdev, f);
>>  	mutex_unlock(&isp->mutex);
>>  	return ret;
>> @@ -1236,20 +1256,13 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>>  	struct ia_css_frame *handle = NULL;
>>  	u32 length;
>>  	u32 pgnr;
>> -	int ret = 0;
>> +	int ret;
>>  
>>  	mutex_lock(&isp->mutex);
>> -	if (isp->isp_fatal_error) {
>> -		ret = -EIO;
>> -		goto error;
>> -	}
>>  
>> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
>> -		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
>> -			__func__);
>> -		ret = -EIO;
>> +	ret = atomisp_pipe_check(pipe, false);
>> +	if (ret)
>>  		goto error;
>> -	}
>>  
>>  	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
>>  	    !pipe->capq.bufs[buf->index]) {
>> @@ -1418,23 +1431,13 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>>  	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
>>  	struct atomisp_sub_device *asd = pipe->asd;
>>  	struct atomisp_device *isp = video_get_drvdata(vdev);
>> -	int ret = 0;
>> +	int ret;
>>  
>>  	mutex_lock(&isp->mutex);
>> -
>> -	if (isp->isp_fatal_error) {
>> -		mutex_unlock(&isp->mutex);
>> -		return -EIO;
>> -	}
>> -
>> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
>> -		mutex_unlock(&isp->mutex);
>> -		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
>> -			__func__);
>> -		return -EIO;
>> -	}
>> -
>> +	ret = atomisp_pipe_check(pipe, false);
>>  	mutex_unlock(&isp->mutex);
>> +	if (ret)
>> +		return ret;
>>  
>>  	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
>>  	if (ret) {
>> @@ -1668,8 +1671,8 @@ static int atomisp_streamon(struct file *file, void *fh,
>>  	enum ia_css_pipe_id css_pipe_id;
>>  	unsigned int sensor_start_stream;
>>  	unsigned int wdt_duration = ATOMISP_ISP_TIMEOUT_DURATION;
>> -	int ret = 0;
>>  	unsigned long irqflags;
>> +	int ret;
>>  
>>  	dev_dbg(isp->dev, "Start stream on pad %d for asd%d\n",
>>  		atomisp_subdev_source_pad(vdev), asd->index);
>> @@ -1680,15 +1683,9 @@ static int atomisp_streamon(struct file *file, void *fh,
>>  	}
>>  
>>  	mutex_lock(&isp->mutex);
>> -	if (isp->isp_fatal_error) {
>> -		ret = -EIO;
>> -		goto out;
>> -	}
>> -
>> -	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
>> -		ret = -EBUSY;
>> +	ret = atomisp_pipe_check(pipe, false);
>> +	if (ret)
>>  		goto out;
>> -	}
>>  
>>  	if (pipe->capq.streaming)
>>  		goto out;
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
>> index 382b78275240..61a6148a6ad5 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
>> @@ -34,6 +34,8 @@ atomisp_format_bridge *atomisp_get_format_bridge(unsigned int pixelformat);
>>  const struct
>>  atomisp_format_bridge *atomisp_get_format_bridge_from_mbus(u32 mbus_code);
>>  
>> +int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool streaming_ok);
>> +
>>  int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
>>  				uint16_t stream_id);
>>  
>> -- 
>> 2.37.3
>>
> 

Re: [PATCH 05/17] media: atomisp: Add atomisp_pipe_check() helper

[Andy Shevchenko]

On Wed, Sep 21, 2022 at 12:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 9/12/22 13:30, Andy Shevchenko wrote:
> > On Sun, Sep 11, 2022 at 07:16:41PM +0200, Hans de Goede wrote:

...

> >> +    case ATOMISP_DEVICE_STREAMING_STOPPING:
> >> +            dev_err(pipe->isp->dev, "IOCTL issued while stopping\n");
> >> +            return -EBUSY;
> >
> > Wouldn't -EAGAIN match better in this case?
>
> The original checks this replaces used -EIO (which seems like a poor
> choice) resp. -EBUSY (in the streamon callback) so I decided to
> keep the -EBUSY here.
>
> Also AFAIK -EAGAIN will make the C-library retry the syscal itself
> in some cases ? (not sure if this applies to ioctls though).
>
> This is not what we want, this scenario can only be hit when an app:
> 1. Uses both the preview and the actual capture /dev/video# nodes
>    at the same time (this is allows)
> 2. Then stops the stream at 1 of them, this transitions the state
>    to STOPPING
> 3. Then does some ioctl other then streamoff on the other /dev/video#
>
> Basically when using more then 1 /dev/video# node then the app must
> stop all of them when stopping things. The driver enforces this
> by rejecting all calls other the streamoff until all /dev/video#
> node streans are off.
>
> This means that simply trying again will result in the same error,
> so -EBUSY seems like the best error for this.

Thanks for the explanation, now it's clear to me.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 06/17] media: atomisp: Remove watchdog timer

[Hans de Goede]

The watchdog timer code to recover from the ISP getting stuck has several
major issues:

1. There is no way to do fault injection and normally the ISP does not
get stuck, so is it is impossible to test it.

2. It in essence just stops all streams, resets the ISP and then brings
everything back up. Userspace can easily do this itself by using a
timeout on dqbuf and then closing (which causes a poweroff) +
re-opening the device. Doing this in userspace (if it ever turns out
to be necessary) greatly simplifies the kernel code and in general
will be a more robust solution.

Even just a quick look at the code finds several more issues:

3. The need to sync-cancel the timers + work on streamoff requires
isp->mutex to be dropped halfway during the ioctl opening all sorts of
races.

4. The atomisp code supports setting up 2 pipelines, streaming from
two sensors at the same time. But there is only a single wdt_work
and stopping one of the 2 streams will cancel the timers + work,
stopping the wdt even though the other stream might still be running.

5. In case atomisp_css_flush() the sync cancel is done while keeping
isp->mutex locked, causing a deadlock when racing with wdt_work which
also takes isp->mutex.

6. Even though the watchdog is purely a software/driver thing which
just checkes that new frames keep coming in, there are 2 completely
different implementations for the ISP2400/ISP2401 which is not
necessary at all.

So all in all I believe that it is better to just remove the current
watchdog implementation. Fixing all the issues with the current
implementation will be so much work, that if it turns out that we do
need something like this then doing a clean re-implementation from
scratch will be better anyways.

wdt_work was also (ab)used to reset the ISP after the firmware signalled
an fw-assert error through the irq, add a new assert_recover_work to
replace this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 361 +-----------------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |   3 +-
 .../media/atomisp/pci/atomisp_compat_css20.c  |  43 +--
 .../media/atomisp/pci/atomisp_internal.h      |  24 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  60 ---
 .../media/atomisp/pci/atomisp_subdev.h        |  14 -
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  43 +--
 7 files changed, 9 insertions(+), 539 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 7945852ecd13..4b459c4c6d76 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -897,7 +897,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 	enum atomisp_metadata_type md_type;
 	struct atomisp_device *isp = asd->isp;
 	struct v4l2_control ctrl;
-	bool reset_wdt_timer = false;
 
 	lockdep_assert_held(&isp->mutex);
 
@@ -1006,9 +1005,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 		break;
 	case IA_CSS_BUFFER_TYPE_VF_OUTPUT_FRAME:
 	case IA_CSS_BUFFER_TYPE_SEC_VF_OUTPUT_FRAME:
-		if (IS_ISP2401)
-			reset_wdt_timer = true;
-
 		pipe->buffers_in_css--;
 		frame = buffer.css_buffer.data.frame;
 		if (!frame) {
@@ -1061,9 +1057,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 		break;
 	case IA_CSS_BUFFER_TYPE_OUTPUT_FRAME:
 	case IA_CSS_BUFFER_TYPE_SEC_OUTPUT_FRAME:
-		if (IS_ISP2401)
-			reset_wdt_timer = true;
-
 		pipe->buffers_in_css--;
 		frame = buffer.css_buffer.data.frame;
 		if (!frame) {
@@ -1231,8 +1224,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 		 */
 		wake_up(&vb->done);
 	}
-	if (IS_ISP2401)
-		atomic_set(&pipe->wdt_count, 0);
 
 	/*
 	 * Requeue should only be done for 3a and dis buffers.
@@ -1249,19 +1240,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 	}
 	if (!error && q_buffers)
 		atomisp_qbuffers_to_css(asd);
-
-	if (IS_ISP2401) {
-		/* If there are no buffers queued then
-		* delete wdt timer. */
-		if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
-			return;
-		if (!atomisp_buffers_queued_pipe(pipe))
-			atomisp_wdt_stop_pipe(pipe, false);
-		else if (reset_wdt_timer)
-			/* SOF irq should not reset wdt timer. */
-			atomisp_wdt_refresh_pipe(pipe,
-						ATOMISP_WDT_KEEP_CURRENT_DELAY);
-	}
 }
 
 void atomisp_delayed_init_work(struct work_struct *work)
@@ -1450,350 +1428,32 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 	}
 }
 
-void atomisp_wdt_work(struct work_struct *work)
+void atomisp_assert_recovery_work(struct work_struct *work)
 {
 	struct atomisp_device *isp = container_of(work, struct atomisp_device,
-				     wdt_work);
-	int i;
-	unsigned int pipe_wdt_cnt[MAX_STREAM_NUM][4] = { {0} };
-	bool css_recover = true;
+						  assert_recovery_work);
 
 	mutex_lock(&isp->mutex);
-	if (!atomisp_streaming_count(isp)) {
-		atomic_set(&isp->wdt_work_queued, 0);
-		mutex_unlock(&isp->mutex);
-		return;
-	}
-
-	if (!IS_ISP2401) {
-		dev_err(isp->dev, "timeout %d of %d\n",
-			atomic_read(&isp->wdt_count) + 1,
-			ATOMISP_ISP_MAX_TIMEOUT_COUNT);
-	} else {
-		for (i = 0; i < isp->num_of_streams; i++) {
-			struct atomisp_sub_device *asd = &isp->asd[i];
-
-			pipe_wdt_cnt[i][0] +=
-			    atomic_read(&asd->video_out_capture.wdt_count);
-			pipe_wdt_cnt[i][1] +=
-			    atomic_read(&asd->video_out_vf.wdt_count);
-			pipe_wdt_cnt[i][2] +=
-			    atomic_read(&asd->video_out_preview.wdt_count);
-			pipe_wdt_cnt[i][3] +=
-			    atomic_read(&asd->video_out_video_capture.wdt_count);
-			css_recover =
-			    (pipe_wdt_cnt[i][0] <= ATOMISP_ISP_MAX_TIMEOUT_COUNT &&
-			    pipe_wdt_cnt[i][1] <= ATOMISP_ISP_MAX_TIMEOUT_COUNT &&
-			    pipe_wdt_cnt[i][2] <= ATOMISP_ISP_MAX_TIMEOUT_COUNT &&
-			    pipe_wdt_cnt[i][3] <= ATOMISP_ISP_MAX_TIMEOUT_COUNT)
-			    ? true : false;
-			dev_err(isp->dev,
-				"pipe on asd%d timeout cnt: (%d, %d, %d, %d) of %d, recover = %d\n",
-				asd->index, pipe_wdt_cnt[i][0], pipe_wdt_cnt[i][1],
-				pipe_wdt_cnt[i][2], pipe_wdt_cnt[i][3],
-				ATOMISP_ISP_MAX_TIMEOUT_COUNT, css_recover);
-		}
-	}
-
-	if (css_recover) {
-		ia_css_debug_dump_sp_sw_debug_info();
-		ia_css_debug_dump_debug_info(__func__);
-		for (i = 0; i < isp->num_of_streams; i++) {
-			struct atomisp_sub_device *asd = &isp->asd[i];
-
-			if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
-				continue;
-			dev_err(isp->dev, "%s, vdev %s buffers in css: %d\n",
-				__func__,
-				asd->video_out_capture.vdev.name,
-				asd->video_out_capture.
-				buffers_in_css);
-			dev_err(isp->dev,
-				"%s, vdev %s buffers in css: %d\n",
-				__func__,
-				asd->video_out_vf.vdev.name,
-				asd->video_out_vf.
-				buffers_in_css);
-			dev_err(isp->dev,
-				"%s, vdev %s buffers in css: %d\n",
-				__func__,
-				asd->video_out_preview.vdev.name,
-				asd->video_out_preview.
-				buffers_in_css);
-			dev_err(isp->dev,
-				"%s, vdev %s buffers in css: %d\n",
-				__func__,
-				asd->video_out_video_capture.vdev.name,
-				asd->video_out_video_capture.
-				buffers_in_css);
-			dev_err(isp->dev,
-				"%s, s3a buffers in css preview pipe:%d\n",
-				__func__,
-				asd->s3a_bufs_in_css[IA_CSS_PIPE_ID_PREVIEW]);
-			dev_err(isp->dev,
-				"%s, s3a buffers in css capture pipe:%d\n",
-				__func__,
-				asd->s3a_bufs_in_css[IA_CSS_PIPE_ID_CAPTURE]);
-			dev_err(isp->dev,
-				"%s, s3a buffers in css video pipe:%d\n",
-				__func__,
-				asd->s3a_bufs_in_css[IA_CSS_PIPE_ID_VIDEO]);
-			dev_err(isp->dev,
-				"%s, dis buffers in css: %d\n",
-				__func__, asd->dis_bufs_in_css);
-			dev_err(isp->dev,
-				"%s, metadata buffers in css preview pipe:%d\n",
-				__func__,
-				asd->metadata_bufs_in_css
-				[ATOMISP_INPUT_STREAM_GENERAL]
-				[IA_CSS_PIPE_ID_PREVIEW]);
-			dev_err(isp->dev,
-				"%s, metadata buffers in css capture pipe:%d\n",
-				__func__,
-				asd->metadata_bufs_in_css
-				[ATOMISP_INPUT_STREAM_GENERAL]
-				[IA_CSS_PIPE_ID_CAPTURE]);
-			dev_err(isp->dev,
-				"%s, metadata buffers in css video pipe:%d\n",
-				__func__,
-				asd->metadata_bufs_in_css
-				[ATOMISP_INPUT_STREAM_GENERAL]
-				[IA_CSS_PIPE_ID_VIDEO]);
-			if (asd->enable_raw_buffer_lock->val) {
-				unsigned int j;
-
-				dev_err(isp->dev, "%s, raw_buffer_locked_count %d\n",
-					__func__, asd->raw_buffer_locked_count);
-				for (j = 0; j <= ATOMISP_MAX_EXP_ID / 32; j++)
-					dev_err(isp->dev, "%s, raw_buffer_bitmap[%d]: 0x%x\n",
-						__func__, j,
-						asd->raw_buffer_bitmap[j]);
-			}
-		}
 
-		/*sh_css_dump_sp_state();*/
-		/*sh_css_dump_isp_state();*/
-	} else {
-		for (i = 0; i < isp->num_of_streams; i++) {
-			struct atomisp_sub_device *asd = &isp->asd[i];
-
-			if (asd->streaming ==
-			    ATOMISP_DEVICE_STREAMING_ENABLED) {
-				atomisp_clear_css_buffer_counters(asd);
-				atomisp_flush_bufs_and_wakeup(asd);
-				complete(&asd->init_done);
-			}
-			if (IS_ISP2401)
-				atomisp_wdt_stop(asd, false);
-		}
-
-		if (!IS_ISP2401) {
-			atomic_set(&isp->wdt_count, 0);
-		} else {
-			isp->isp_fatal_error = true;
-			atomic_set(&isp->wdt_work_queued, 0);
-
-			mutex_unlock(&isp->mutex);
-			return;
-		}
-	}
-
-	__atomisp_css_recover(isp, true);
-	if (IS_ISP2401) {
-		for (i = 0; i < isp->num_of_streams; i++) {
-			struct atomisp_sub_device *asd = &isp->asd[i];
-
-			if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
-				continue;
-
-			atomisp_wdt_refresh(asd, ATOMISP_ISP_TIMEOUT_DURATION);
-		}
-	}
-
-	dev_err(isp->dev, "timeout recovery handling done\n");
-	atomic_set(&isp->wdt_work_queued, 0);
+	if (atomisp_streaming_count(isp))
+		__atomisp_css_recover(isp, true);
 
 	mutex_unlock(&isp->mutex);
 }
 
 void atomisp_css_flush(struct atomisp_device *isp)
 {
-	int i;
-
 	lockdep_assert_held(&isp->mutex);
 
 	if (!atomisp_streaming_count(isp))
 		return;
 
-	/* Disable wdt */
-	for (i = 0; i < isp->num_of_streams; i++) {
-		struct atomisp_sub_device *asd = &isp->asd[i];
-
-		atomisp_wdt_stop(asd, true);
-	}
-
 	/* Start recover */
 	__atomisp_css_recover(isp, false);
-	/* Restore wdt */
-	for (i = 0; i < isp->num_of_streams; i++) {
-		struct atomisp_sub_device *asd = &isp->asd[i];
-
-		if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
-			continue;
 
-		atomisp_wdt_refresh(asd, ATOMISP_ISP_TIMEOUT_DURATION);
-	}
 	dev_dbg(isp->dev, "atomisp css flush done\n");
 }
 
-void atomisp_wdt(struct timer_list *t)
-{
-	struct atomisp_sub_device *asd;
-	struct atomisp_device *isp;
-
-	if (!IS_ISP2401) {
-		asd = from_timer(asd, t, wdt);
-		isp = asd->isp;
-	} else {
-		struct atomisp_video_pipe *pipe = from_timer(pipe, t, wdt);
-
-		asd = pipe->asd;
-		isp = asd->isp;
-
-		atomic_inc(&pipe->wdt_count);
-		dev_warn(isp->dev,
-			"[WARNING]asd %d pipe %s ISP timeout %d!\n",
-			asd->index, pipe->vdev.name,
-			atomic_read(&pipe->wdt_count));
-	}
-
-	if (atomic_read(&isp->wdt_work_queued)) {
-		dev_dbg(isp->dev, "ISP watchdog was put into workqueue\n");
-		return;
-	}
-	atomic_set(&isp->wdt_work_queued, 1);
-	queue_work(isp->wdt_work_queue, &isp->wdt_work);
-}
-
-/* ISP2400 */
-void atomisp_wdt_start(struct atomisp_sub_device *asd)
-{
-	atomisp_wdt_refresh(asd, ATOMISP_ISP_TIMEOUT_DURATION);
-}
-
-/* ISP2401 */
-void atomisp_wdt_refresh_pipe(struct atomisp_video_pipe *pipe,
-			      unsigned int delay)
-{
-	unsigned long next;
-
-	if (!pipe->asd) {
-		dev_err(pipe->isp->dev, "%s(): asd is NULL, device is %s\n",
-			__func__, pipe->vdev.name);
-		return;
-	}
-
-	if (delay != ATOMISP_WDT_KEEP_CURRENT_DELAY)
-		pipe->wdt_duration = delay;
-
-	next = jiffies + pipe->wdt_duration;
-
-	/* Override next if it has been pushed beyon the "next" time */
-	if (atomisp_is_wdt_running(pipe) && time_after(pipe->wdt_expires, next))
-		next = pipe->wdt_expires;
-
-	pipe->wdt_expires = next;
-
-	if (atomisp_is_wdt_running(pipe))
-		dev_dbg(pipe->asd->isp->dev, "WDT will hit after %d ms (%s)\n",
-			((int)(next - jiffies) * 1000 / HZ), pipe->vdev.name);
-	else
-		dev_dbg(pipe->asd->isp->dev, "WDT starts with %d ms period (%s)\n",
-			((int)(next - jiffies) * 1000 / HZ), pipe->vdev.name);
-
-	mod_timer(&pipe->wdt, next);
-}
-
-void atomisp_wdt_refresh(struct atomisp_sub_device *asd, unsigned int delay)
-{
-	if (!IS_ISP2401) {
-		unsigned long next;
-
-		if (delay != ATOMISP_WDT_KEEP_CURRENT_DELAY)
-			asd->wdt_duration = delay;
-
-		next = jiffies + asd->wdt_duration;
-
-		/* Override next if it has been pushed beyon the "next" time */
-		if (atomisp_is_wdt_running(asd) && time_after(asd->wdt_expires, next))
-			next = asd->wdt_expires;
-
-		asd->wdt_expires = next;
-
-		if (atomisp_is_wdt_running(asd))
-			dev_dbg(asd->isp->dev, "WDT will hit after %d ms\n",
-				((int)(next - jiffies) * 1000 / HZ));
-		else
-			dev_dbg(asd->isp->dev, "WDT starts with %d ms period\n",
-				((int)(next - jiffies) * 1000 / HZ));
-
-		mod_timer(&asd->wdt, next);
-		atomic_set(&asd->isp->wdt_count, 0);
-	} else {
-		dev_dbg(asd->isp->dev, "WDT refresh all:\n");
-		if (atomisp_is_wdt_running(&asd->video_out_capture))
-			atomisp_wdt_refresh_pipe(&asd->video_out_capture, delay);
-		if (atomisp_is_wdt_running(&asd->video_out_preview))
-			atomisp_wdt_refresh_pipe(&asd->video_out_preview, delay);
-		if (atomisp_is_wdt_running(&asd->video_out_vf))
-			atomisp_wdt_refresh_pipe(&asd->video_out_vf, delay);
-		if (atomisp_is_wdt_running(&asd->video_out_video_capture))
-			atomisp_wdt_refresh_pipe(&asd->video_out_video_capture, delay);
-	}
-}
-
-/* ISP2401 */
-void atomisp_wdt_stop_pipe(struct atomisp_video_pipe *pipe, bool sync)
-{
-	if (!atomisp_is_wdt_running(pipe))
-		return;
-
-	dev_dbg(pipe->asd->isp->dev,
-		"WDT stop asd %d (%s)\n", pipe->asd->index, pipe->vdev.name);
-
-	if (sync) {
-		del_timer_sync(&pipe->wdt);
-		cancel_work_sync(&pipe->asd->isp->wdt_work);
-	} else {
-		del_timer(&pipe->wdt);
-	}
-}
-
-/* ISP 2401 */
-void atomisp_wdt_start_pipe(struct atomisp_video_pipe *pipe)
-{
-	atomisp_wdt_refresh_pipe(pipe, ATOMISP_ISP_TIMEOUT_DURATION);
-}
-
-void atomisp_wdt_stop(struct atomisp_sub_device *asd, bool sync)
-{
-	dev_dbg(asd->isp->dev, "WDT stop:\n");
-
-	if (!IS_ISP2401) {
-		if (sync) {
-			del_timer_sync(&asd->wdt);
-			cancel_work_sync(&asd->isp->wdt_work);
-		} else {
-			del_timer(&asd->wdt);
-		}
-	} else {
-		atomisp_wdt_stop_pipe(&asd->video_out_capture, sync);
-		atomisp_wdt_stop_pipe(&asd->video_out_preview, sync);
-		atomisp_wdt_stop_pipe(&asd->video_out_vf, sync);
-		atomisp_wdt_stop_pipe(&asd->video_out_video_capture, sync);
-	}
-}
-
 void atomisp_setup_flash(struct atomisp_sub_device *asd)
 {
 	struct atomisp_device *isp = asd->isp;
@@ -4122,19 +3782,6 @@ void atomisp_handle_parameter_and_buffer(struct atomisp_video_pipe *pipe)
 		return;
 
 	atomisp_qbuffers_to_css(asd);
-
-	if (!IS_ISP2401) {
-		if (!atomisp_is_wdt_running(asd) && atomisp_buffers_queued(asd))
-			atomisp_wdt_start(asd);
-	} else {
-		if (atomisp_buffers_queued_pipe(pipe)) {
-			if (!atomisp_is_wdt_running(pipe))
-				atomisp_wdt_start_pipe(pipe);
-			else
-				atomisp_wdt_refresh_pipe(pipe,
-							ATOMISP_WDT_KEEP_CURRENT_DELAY);
-		}
-	}
 }
 
 /*
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index c4472516487b..5ab7d6aca7fa 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -65,8 +65,7 @@ bool atomisp_buffers_queued_pipe(struct atomisp_video_pipe *pipe);
 /* Interrupt functions */
 void atomisp_msi_irq_init(struct atomisp_device *isp);
 void atomisp_msi_irq_uninit(struct atomisp_device *isp);
-void atomisp_wdt_work(struct work_struct *work);
-void atomisp_wdt(struct timer_list *t);
+void atomisp_assert_recovery_work(struct work_struct *work);
 void atomisp_setup_flash(struct atomisp_sub_device *asd);
 irqreturn_t atomisp_isr(int irq, void *dev);
 irqreturn_t atomisp_isr_thread(int irq, void *isp_ptr);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 15ef31b0c601..0154ebf2cba5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -3796,8 +3796,6 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 	enum atomisp_input_stream_id stream_id = 0;
 	struct atomisp_css_event current_event;
 	struct atomisp_sub_device *asd;
-	bool reset_wdt_timer[MAX_STREAM_NUM] = {false};
-	int i;
 
 	lockdep_assert_held(&isp->mutex);
 
@@ -3813,14 +3811,8 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 				__func__,
 				current_event.event.fw_assert_module_id,
 				current_event.event.fw_assert_line_no);
-			for (i = 0; i < isp->num_of_streams; i++)
-				atomisp_wdt_stop(&isp->asd[i], 0);
-
-			if (!IS_ISP2401)
-				atomisp_wdt(&isp->asd[0].wdt);
-			else
-				queue_work(isp->wdt_work_queue, &isp->wdt_work);
 
+			queue_work(system_long_wq, &isp->assert_recovery_work);
 			return -EINVAL;
 		} else if (current_event.event.type == IA_CSS_EVENT_TYPE_FW_WARNING) {
 			dev_warn(isp->dev, "%s: ISP reports warning, code is %d, exp_id %d\n",
@@ -3849,20 +3841,12 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 			frame_done_found[asd->index] = true;
 			atomisp_buf_done(asd, 0, IA_CSS_BUFFER_TYPE_OUTPUT_FRAME,
 					 current_event.pipe, true, stream_id);
-
-			if (!IS_ISP2401)
-				reset_wdt_timer[asd->index] = true; /* ISP running */
-
 			break;
 		case IA_CSS_EVENT_TYPE_SECOND_OUTPUT_FRAME_DONE:
 			dev_dbg(isp->dev, "event: Second output frame done");
 			frame_done_found[asd->index] = true;
 			atomisp_buf_done(asd, 0, IA_CSS_BUFFER_TYPE_SEC_OUTPUT_FRAME,
 					 current_event.pipe, true, stream_id);
-
-			if (!IS_ISP2401)
-				reset_wdt_timer[asd->index] = true; /* ISP running */
-
 			break;
 		case IA_CSS_EVENT_TYPE_3A_STATISTICS_DONE:
 			dev_dbg(isp->dev, "event: 3A stats frame done");
@@ -3883,19 +3867,12 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 			atomisp_buf_done(asd, 0,
 					 IA_CSS_BUFFER_TYPE_VF_OUTPUT_FRAME,
 					 current_event.pipe, true, stream_id);
-
-			if (!IS_ISP2401)
-				reset_wdt_timer[asd->index] = true; /* ISP running */
-
 			break;
 		case IA_CSS_EVENT_TYPE_SECOND_VF_OUTPUT_FRAME_DONE:
 			dev_dbg(isp->dev, "event: second VF output frame done");
 			atomisp_buf_done(asd, 0,
 					 IA_CSS_BUFFER_TYPE_SEC_VF_OUTPUT_FRAME,
 					 current_event.pipe, true, stream_id);
-			if (!IS_ISP2401)
-				reset_wdt_timer[asd->index] = true; /* ISP running */
-
 			break;
 		case IA_CSS_EVENT_TYPE_DIS_STATISTICS_DONE:
 			dev_dbg(isp->dev, "event: dis stats frame done");
@@ -3918,24 +3895,6 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
 		}
 	}
 
-	if (IS_ISP2401)
-		return 0;
-
-	/* ISP2400: If there are no buffers queued then delete wdt timer. */
-	for (i = 0; i < isp->num_of_streams; i++) {
-		asd = &isp->asd[i];
-		if (!asd)
-			continue;
-		if (asd->streaming != ATOMISP_DEVICE_STREAMING_ENABLED)
-			continue;
-		if (!atomisp_buffers_queued(asd))
-			atomisp_wdt_stop(asd, false);
-		else if (reset_wdt_timer[i])
-			/* SOF irq should not reset wdt timer. */
-			atomisp_wdt_refresh(asd,
-					    ATOMISP_WDT_KEEP_CURRENT_DELAY);
-	}
-
 	return 0;
 }
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index b2c362ef7199..2279d45e7d7a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -258,13 +258,7 @@ struct atomisp_device {
 	/* isp timeout status flag */
 	bool isp_timeout;
 	bool isp_fatal_error;
-	struct workqueue_struct *wdt_work_queue;
-	struct work_struct wdt_work;
-
-	/* ISP2400 */
-	atomic_t wdt_count;
-
-	atomic_t wdt_work_queued;
+	struct work_struct assert_recovery_work;
 
 	spinlock_t lock; /* Protects asd[i].streaming */
 
@@ -282,20 +276,4 @@ struct atomisp_device {
 
 extern struct device *atomisp_dev;
 
-#define atomisp_is_wdt_running(a) timer_pending(&(a)->wdt)
-
-/* ISP2401 */
-void atomisp_wdt_refresh_pipe(struct atomisp_video_pipe *pipe,
-			      unsigned int delay);
-void atomisp_wdt_refresh(struct atomisp_sub_device *asd, unsigned int delay);
-
-/* ISP2400 */
-void atomisp_wdt_start(struct atomisp_sub_device *asd);
-
-/* ISP2401 */
-void atomisp_wdt_start_pipe(struct atomisp_video_pipe *pipe);
-void atomisp_wdt_stop_pipe(struct atomisp_video_pipe *pipe, bool sync);
-
-void atomisp_wdt_stop(struct atomisp_sub_device *asd, bool sync);
-
 #endif /* __ATOMISP_INTERNAL_H__ */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 9b50f637c46a..daecdcdeb27c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1363,15 +1363,6 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 			atomisp_handle_parameter_and_buffer(pipe);
 		} else {
 			atomisp_qbuffers_to_css(asd);
-
-			if (!IS_ISP2401) {
-				if (!atomisp_is_wdt_running(asd) && atomisp_buffers_queued(asd))
-					atomisp_wdt_start(asd);
-			} else {
-				if (!atomisp_is_wdt_running(pipe) &&
-				    atomisp_buffers_queued_pipe(pipe))
-					atomisp_wdt_start_pipe(pipe);
-			}
 		}
 	}
 
@@ -1594,33 +1585,6 @@ int atomisp_stream_on_master_slave_sensor(struct atomisp_device *isp,
 	return 0;
 }
 
-/* FIXME! ISP2400 */
-static void __wdt_on_master_slave_sensor(struct atomisp_device *isp,
-					 unsigned int wdt_duration)
-{
-	if (atomisp_buffers_queued(&isp->asd[0]))
-		atomisp_wdt_refresh(&isp->asd[0], wdt_duration);
-	if (atomisp_buffers_queued(&isp->asd[1]))
-		atomisp_wdt_refresh(&isp->asd[1], wdt_duration);
-}
-
-/* FIXME! ISP2401 */
-static void __wdt_on_master_slave_sensor_pipe(struct atomisp_video_pipe *pipe,
-					      unsigned int wdt_duration,
-					      bool enable)
-{
-	static struct atomisp_video_pipe *pipe0;
-
-	if (enable) {
-		if (atomisp_buffers_queued_pipe(pipe0))
-			atomisp_wdt_refresh_pipe(pipe0, wdt_duration);
-		if (atomisp_buffers_queued_pipe(pipe))
-			atomisp_wdt_refresh_pipe(pipe, wdt_duration);
-	} else {
-		pipe0 = pipe;
-	}
-}
-
 static void atomisp_pause_buffer_event(struct atomisp_device *isp)
 {
 	struct v4l2_event event = {0};
@@ -1670,7 +1634,6 @@ static int atomisp_streamon(struct file *file, void *fh,
 	struct pci_dev *pdev = to_pci_dev(isp->dev);
 	enum ia_css_pipe_id css_pipe_id;
 	unsigned int sensor_start_stream;
-	unsigned int wdt_duration = ATOMISP_ISP_TIMEOUT_DURATION;
 	unsigned long irqflags;
 	int ret;
 
@@ -1845,15 +1808,9 @@ static int atomisp_streamon(struct file *file, void *fh,
 			dev_err(isp->dev, "master slave sensor stream on failed!\n");
 			goto out;
 		}
-		if (!IS_ISP2401)
-			__wdt_on_master_slave_sensor(isp, wdt_duration);
-		else
-			__wdt_on_master_slave_sensor_pipe(pipe, wdt_duration, true);
 		goto start_delay_wq;
 	} else if (asd->depth_mode->val && (atomisp_streaming_count(isp) <
 					    ATOMISP_DEPTH_SENSOR_STREAMON_COUNT)) {
-		if (IS_ISP2401)
-			__wdt_on_master_slave_sensor_pipe(pipe, wdt_duration, false);
 		goto start_delay_wq;
 	}
 
@@ -1875,14 +1832,6 @@ static int atomisp_streamon(struct file *file, void *fh,
 		goto out;
 	}
 
-	if (!IS_ISP2401) {
-		if (atomisp_buffers_queued(asd))
-			atomisp_wdt_refresh(asd, wdt_duration);
-	} else {
-		if (atomisp_buffers_queued_pipe(pipe))
-			atomisp_wdt_refresh_pipe(pipe, wdt_duration);
-	}
-
 start_delay_wq:
 	if (asd->continuous_mode->val) {
 		struct v4l2_mbus_framefmt *sink;
@@ -1986,16 +1935,7 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 		asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
 		first_streamoff = true;
 	}
-	spin_unlock_irqrestore(&isp->lock, flags);
-
-	if (first_streamoff) {
-		/* if other streams are running, should not disable watch dog */
-		mutex_unlock(&isp->mutex);
-		atomisp_wdt_stop(asd, true);
-		mutex_lock(&isp->mutex);
-	}
 
-	spin_lock_irqsave(&isp->lock, flags);
 	if (atomisp_subdev_streaming_count(asd) == 1)
 		asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
 	spin_unlock_irqrestore(&isp->lock, flags);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index b44f060b0bb5..43e6a1d1e410 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -108,15 +108,6 @@ struct atomisp_video_pipe {
 	 */
 	unsigned int frame_request_config_id[VIDEO_MAX_FRAME];
 	struct atomisp_css_params_with_list *frame_params[VIDEO_MAX_FRAME];
-
-	/*
-	* move wdt from asd struct to create wdt for each pipe
-	*/
-	/* ISP2401 */
-	struct timer_list wdt;
-	unsigned int wdt_duration;	/* in jiffies */
-	unsigned long wdt_expires;
-	atomic_t wdt_count;
 };
 
 struct atomisp_pad_format {
@@ -360,11 +351,6 @@ struct atomisp_sub_device {
 	int raw_buffer_locked_count;
 	spinlock_t raw_buffer_bitmap_lock;
 
-	/* ISP 2400 */
-	struct timer_list wdt;
-	unsigned int wdt_duration;	/* in jiffies */
-	unsigned long wdt_expires;
-
 	/* ISP2401 */
 	bool re_trigger_capture;
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index aa38e0d33b5b..d55e8d32a286 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1433,39 +1433,6 @@ static bool is_valid_device(struct pci_dev *pdev, const struct pci_device_id *id
 	return true;
 }
 
-static int init_atomisp_wdts(struct atomisp_device *isp)
-{
-	int i, err;
-
-	atomic_set(&isp->wdt_work_queued, 0);
-	isp->wdt_work_queue = alloc_workqueue(isp->v4l2_dev.name, 0, 1);
-	if (!isp->wdt_work_queue) {
-		dev_err(isp->dev, "Failed to initialize wdt work queue\n");
-		err = -ENOMEM;
-		goto alloc_fail;
-	}
-	INIT_WORK(&isp->wdt_work, atomisp_wdt_work);
-
-	for (i = 0; i < isp->num_of_streams; i++) {
-		struct atomisp_sub_device *asd = &isp->asd[i];
-
-		if (!IS_ISP2401) {
-			timer_setup(&asd->wdt, atomisp_wdt, 0);
-		} else {
-			timer_setup(&asd->video_out_capture.wdt,
-				    atomisp_wdt, 0);
-			timer_setup(&asd->video_out_preview.wdt,
-				    atomisp_wdt, 0);
-			timer_setup(&asd->video_out_vf.wdt, atomisp_wdt, 0);
-			timer_setup(&asd->video_out_video_capture.wdt,
-				    atomisp_wdt, 0);
-		}
-	}
-	return 0;
-alloc_fail:
-	return err;
-}
-
 #define ATOM_ISP_PCI_BAR	0
 
 static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
@@ -1698,10 +1665,8 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
 		dev_err(&pdev->dev, "atomisp_register_entities failed (%d)\n", err);
 		goto register_entities_fail;
 	}
-	/* init atomisp wdts */
-	err = init_atomisp_wdts(isp);
-	if (err != 0)
-		goto wdt_work_queue_fail;
+
+	INIT_WORK(&isp->assert_recovery_work, atomisp_assert_recovery_work);
 
 	/* save the iunit context only once after all the values are init'ed. */
 	atomisp_save_iunit_reg(isp);
@@ -1748,8 +1713,6 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
 request_irq_fail:
 	hmm_cleanup();
 	pm_runtime_get_noresume(&pdev->dev);
-	destroy_workqueue(isp->wdt_work_queue);
-wdt_work_queue_fail:
 	atomisp_unregister_entities(isp);
 register_entities_fail:
 	atomisp_uninitialize_modules(isp);
@@ -1809,8 +1772,6 @@ static void atomisp_pci_remove(struct pci_dev *pdev)
 	atomisp_msi_irq_uninit(isp);
 	atomisp_unregister_entities(isp);
 
-	destroy_workqueue(isp->wdt_work_queue);
-
 	release_firmware(isp->firmware);
 }
 
-- 
2.37.3

[PATCH 07/17] media: atomisp: Move atomisp_streaming_count() check into __atomisp_css_recover()

[Hans de Goede]

Both callers of __atomisp_css_recover() check atomisp_streaming_count()
first, move the check into __atomisp_css_recover().

And __atomisp_css_recover() already calls lockdep_assert_held(&isp->mutex),
so drop that from atomisp_css_flush().

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 4b459c4c6d76..a96a4658e113 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1282,6 +1282,9 @@ static void __atomisp_css_recover(struct atomisp_device *isp, bool isp_timeout)
 
 	lockdep_assert_held(&isp->mutex);
 
+	if (!atomisp_streaming_count(isp))
+		return;
+
 	atomisp_css_irq_enable(isp, IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF, false);
 
 	BUG_ON(isp->num_of_streams > MAX_STREAM_NUM);
@@ -1434,20 +1437,12 @@ void atomisp_assert_recovery_work(struct work_struct *work)
 						  assert_recovery_work);
 
 	mutex_lock(&isp->mutex);
-
-	if (atomisp_streaming_count(isp))
-		__atomisp_css_recover(isp, true);
-
+	__atomisp_css_recover(isp, true);
 	mutex_unlock(&isp->mutex);
 }
 
 void atomisp_css_flush(struct atomisp_device *isp)
 {
-	lockdep_assert_held(&isp->mutex);
-
-	if (!atomisp_streaming_count(isp))
-		return;
-
 	/* Start recover */
 	__atomisp_css_recover(isp, false);
 
-- 
2.37.3

[PATCH 08/17] media: atomisp: Rework asd->streaming state update in __atomisp_streamoff()

[Hans de Goede]

During the first __atomisp_streamoff() call on an asd with only one pipe
streaming asd->streaming would get set twice:

asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;

Rework the code a bit so that it gets set to the correct value
right away instead of doing this in 2 steps.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index daecdcdeb27c..899157584951 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1930,14 +1930,14 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (!pipe->capq.streaming)
 		return 0;
 
-	spin_lock_irqsave(&isp->lock, flags);
-	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
-		asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
+	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED)
 		first_streamoff = true;
-	}
 
+	spin_lock_irqsave(&isp->lock, flags);
 	if (atomisp_subdev_streaming_count(asd) == 1)
 		asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
+	else
+		asd->streaming = ATOMISP_DEVICE_STREAMING_STOPPING;
 	spin_unlock_irqrestore(&isp->lock, flags);
 
 	if (!first_streamoff) {
-- 
2.37.3

[PATCH 09/17] media: atomisp: Drop streamoff_mutex

[Hans de Goede]

Now that __atomisp_streamoff() no longer drops isp->mutex to cancel
the watchdog timer, the streamoff_mutex is no longer necessary to
avoid multiple streamoffs racing with each other.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index e1b213ba4686..531bbd6d7ee0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -822,7 +822,6 @@ static int atomisp_release(struct file *file)
 	if (!isp)
 		return -EBADF;
 
-	mutex_lock(&isp->streamoff_mutex);
 	mutex_lock(&isp->mutex);
 
 	dev_dbg(isp->dev, "release device %s\n", vdev->name);
@@ -909,7 +908,6 @@ static int atomisp_release(struct file *file)
 				     V4L2_SEL_TGT_COMPOSE, 0,
 				     &clear_compose);
 	mutex_unlock(&isp->mutex);
-	mutex_unlock(&isp->streamoff_mutex);
 
 	return v4l2_fh_release(file);
 }
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index 2279d45e7d7a..f3ef840c640a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -239,12 +239,6 @@ struct atomisp_device {
 	/* Purpose of mutex is to protect and serialize use of isp data
 	 * structures and css API calls. */
 	struct mutex mutex;
-	/*
-	 * Serialise streamoff: mutex is dropped during streamoff to
-	 * cancel the watchdog queue. MUST be acquired BEFORE
-	 * "mutex".
-	 */
-	struct mutex streamoff_mutex;
 
 	unsigned int input_cnt;
 	struct atomisp_input_subdev inputs[ATOM_ISP_MAX_INPUTS];
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 899157584951..6d84a7e9cb57 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1872,7 +1872,6 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 		atomisp_subdev_source_pad(vdev), asd->index);
 
 	lockdep_assert_held(&isp->mutex);
-	lockdep_assert_held(&isp->streamoff_mutex);
 
 	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		dev_dbg(isp->dev, "unsupported v4l2 buf type\n");
@@ -2081,11 +2080,9 @@ static int atomisp_streamoff(struct file *file, void *fh,
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int rval;
 
-	mutex_lock(&isp->streamoff_mutex);
 	mutex_lock(&isp->mutex);
 	rval = __atomisp_streamoff(file, fh, type);
 	mutex_unlock(&isp->mutex);
-	mutex_unlock(&isp->streamoff_mutex);
 
 	return rval;
 }
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index d55e8d32a286..4ab91858d308 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1482,7 +1482,6 @@ static int atomisp_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
 	dev_dbg(&pdev->dev, "atomisp mmio base: %p\n", isp->base);
 
 	mutex_init(&isp->mutex);
-	mutex_init(&isp->streamoff_mutex);
 	spin_lock_init(&isp->lock);
 
 	/* This is not a true PCI device on SoC, so the delay is not needed. */
-- 
2.37.3

[PATCH 10/17] media: atomisp: Use video_dev.lock for ioctl locking

[Hans de Goede]

Set video_dev.lock to point to isp->mutex so that the core does
the locking surroundig ioctls for us and drop all the now no longer
necessary (and conflicting) locking from the ioctl handling code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 203 ++++--------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 2 files changed, 42 insertions(+), 162 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 6d84a7e9cb57..42d8d1267553 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -655,13 +655,9 @@ unsigned int atomisp_streaming_count(struct atomisp_device *isp)
 static int atomisp_g_input(struct file *file, void *fh, unsigned int *input)
 {
 	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;
 
-	mutex_lock(&isp->mutex);
 	*input = asd->input_curr;
-	mutex_unlock(&isp->mutex);
-
 	return 0;
 }
 
@@ -678,16 +674,13 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	struct v4l2_subdev *motor;
 	int ret;
 
-	mutex_lock(&isp->mutex);
-
 	ret = atomisp_pipe_check(pipe, true);
 	if (ret)
-		goto error;
+		return ret;
 
 	if (input >= ATOM_ISP_MAX_INPUTS || input >= isp->input_cnt) {
 		dev_dbg(isp->dev, "input_cnt: %d\n", isp->input_cnt);
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	/*
@@ -699,15 +692,13 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 		dev_err(isp->dev,
 			"%s, camera is already used by stream: %d\n", __func__,
 			isp->inputs[input].asd->index);
-		ret = -EBUSY;
-		goto error;
+		return -EBUSY;
 	}
 
 	camera = isp->inputs[input].camera;
 	if (!camera) {
 		dev_err(isp->dev, "%s, no camera\n", __func__);
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	/* power off the current owned sensor, as it is not used this time */
@@ -726,7 +717,7 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	ret = v4l2_subdev_call(isp->inputs[input].camera, core, s_power, 1);
 	if (ret) {
 		dev_err(isp->dev, "Failed to power-on sensor\n");
-		goto error;
+		return ret;
 	}
 	/*
 	 * Some sensor driver resets the run mode during power-on, thus force
@@ -739,7 +730,7 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 			       0, isp->inputs[input].sensor_index, 0);
 	if (ret && (ret != -ENOIOCTLCMD)) {
 		dev_err(isp->dev, "Failed to select sensor\n");
-		goto error;
+		return ret;
 	}
 
 	if (!IS_ISP2401) {
@@ -756,14 +747,8 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	asd->input_curr = input;
 	/* mark this camera is used by the current stream */
 	isp->inputs[input].asd = asd;
-	mutex_unlock(&isp->mutex);
 
 	return 0;
-
-error:
-	mutex_unlock(&isp->mutex);
-
-	return ret;
 }
 
 static int atomisp_enum_framesizes(struct file *file, void *priv,
@@ -838,15 +823,12 @@ static int atomisp_enum_fmt_cap(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	mutex_lock(&isp->mutex);
-
 	rval = v4l2_subdev_call(camera, pad, enum_mbus_code, NULL, &code);
 	if (rval == -ENOIOCTLCMD) {
 		dev_warn(isp->dev,
 			 "enum_mbus_code pad op not supported by %s. Please fix your sensor driver!\n",
 			 camera->name);
 	}
-	mutex_unlock(&isp->mutex);
 
 	if (rval)
 		return rval;
@@ -949,7 +931,6 @@ static int atomisp_try_fmt_cap(struct file *file, void *fh,
 			       struct v4l2_format *f)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret;
 
 	/*
@@ -959,10 +940,7 @@ static int atomisp_try_fmt_cap(struct file *file, void *fh,
 	f->fmt.pix.width += pad_w;
 	f->fmt.pix.height += pad_h;
 
-	mutex_lock(&isp->mutex);
 	ret = atomisp_try_fmt(vdev, &f->fmt.pix, NULL);
-	mutex_unlock(&isp->mutex);
-
 	if (ret)
 		return ret;
 
@@ -973,12 +951,9 @@ static int atomisp_g_fmt_cap(struct file *file, void *fh,
 			     struct v4l2_format *f)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_video_pipe *pipe;
 
-	mutex_lock(&isp->mutex);
 	pipe = atomisp_to_video_pipe(vdev);
-	mutex_unlock(&isp->mutex);
 
 	f->fmt.pix = pipe->pix;
 
@@ -997,13 +972,8 @@ static int atomisp_s_fmt_cap(struct file *file, void *fh,
 			     struct v4l2_format *f)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
-	int ret;
 
-	mutex_lock(&isp->mutex);
-	ret = atomisp_set_fmt(vdev, f);
-	mutex_unlock(&isp->mutex);
-	return ret;
+	return atomisp_set_fmt(vdev, f);
 }
 
 /*
@@ -1217,15 +1187,7 @@ int __atomisp_reqbufs(struct file *file, void *fh,
 int atomisp_reqbufs(struct file *file, void *fh,
 		    struct v4l2_requestbuffers *req)
 {
-	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
-	int ret;
-
-	mutex_lock(&isp->mutex);
-	ret = __atomisp_reqbufs(file, fh, req);
-	mutex_unlock(&isp->mutex);
-
-	return ret;
+	return __atomisp_reqbufs(file, fh, req);
 }
 
 /* application query the status of a buffer */
@@ -1258,17 +1220,14 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	u32 pgnr;
 	int ret;
 
-	mutex_lock(&isp->mutex);
-
 	ret = atomisp_pipe_check(pipe, false);
 	if (ret)
-		goto error;
+		return ret;
 
 	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
 	    !pipe->capq.bufs[buf->index]) {
 		dev_err(isp->dev, "Invalid index for qbuf.\n");
-		ret = -EINVAL;
-		goto error;
+		return -EINVAL;
 	}
 
 	/*
@@ -1278,16 +1237,13 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	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;
+			return -EINVAL;
 		}
 
 		vb = pipe->capq.bufs[buf->index];
 		vm_mem = vb->priv;
-		if (!vm_mem) {
-			ret = -EINVAL;
-			goto error;
-		}
+		if (!vm_mem)
+			return -EINVAL;
 
 		length = vb->bsize;
 		pgnr = (length + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
@@ -1296,17 +1252,15 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 			goto done;
 
 		if (atomisp_get_css_frame_info(asd,
-					       atomisp_subdev_source_pad(vdev), &frame_info)) {
-			ret = -EIO;
-			goto error;
-		}
+					       atomisp_subdev_source_pad(vdev), &frame_info))
+			return -EIO;
 
 		ret = ia_css_frame_map(&handle, &frame_info,
 					    (void __user *)buf->m.userptr,
 					    pgnr);
 		if (ret) {
 			dev_err(isp->dev, "Failed to map user buffer\n");
-			goto error;
+			return ret;
 		}
 
 		if (vm_mem->vaddr) {
@@ -1351,11 +1305,10 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	pipe->frame_params[buf->index] = NULL;
 
 	mutex_unlock(&isp->mutex);
-
 	ret = videobuf_qbuf(&pipe->capq, buf);
 	mutex_lock(&isp->mutex);
 	if (ret)
-		goto error;
+		return ret;
 
 	/* TODO: do this better, not best way to queue to css */
 	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
@@ -1384,16 +1337,11 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 			asd->pending_capture_request++;
 			dev_dbg(isp->dev, "Add one pending capture request.\n");
 	}
-	mutex_unlock(&isp->mutex);
 
 	dev_dbg(isp->dev, "qbuf buffer %d (%s) for asd%d\n", buf->index,
 		vdev->name, asd->index);
 
-	return ret;
-
-error:
-	mutex_unlock(&isp->mutex);
-	return ret;
+	return 0;
 }
 
 static int __get_frame_exp_id(struct atomisp_video_pipe *pipe,
@@ -1424,19 +1372,19 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int ret;
 
-	mutex_lock(&isp->mutex);
 	ret = atomisp_pipe_check(pipe, false);
-	mutex_unlock(&isp->mutex);
 	if (ret)
 		return ret;
 
+	mutex_unlock(&isp->mutex);
 	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
+	mutex_lock(&isp->mutex);
 	if (ret) {
 		if (ret != -EAGAIN)
 			dev_dbg(isp->dev, "<%s: %d\n", __func__, ret);
 		return ret;
 	}
-	mutex_lock(&isp->mutex);
+
 	buf->bytesused = pipe->pix.sizeimage;
 	buf->reserved = asd->frame_status[buf->index];
 
@@ -1450,7 +1398,6 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	if (!(buf->flags & V4L2_BUF_FLAG_ERROR))
 		buf->reserved |= __get_frame_exp_id(pipe, buf) << 16;
 	buf->reserved2 = pipe->frame_config_id[buf->index];
-	mutex_unlock(&isp->mutex);
 
 	dev_dbg(isp->dev,
 		"dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
@@ -1645,13 +1592,12 @@ static int atomisp_streamon(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	mutex_lock(&isp->mutex);
 	ret = atomisp_pipe_check(pipe, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	if (pipe->capq.streaming)
-		goto out;
+		return 0;
 
 	/* Input system HW workaround */
 	atomisp_dma_burst_len_cfg(asd);
@@ -1666,14 +1612,13 @@ static int atomisp_streamon(struct file *file, void *fh,
 	if (list_empty(&pipe->capq.stream)) {
 		spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
 		dev_dbg(isp->dev, "no buffer in the queue\n");
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 	spin_unlock_irqrestore(&pipe->irq_lock, irqflags);
 
 	ret = videobuf_streamon(&pipe->capq);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* Reset pending capture request count. */
 	asd->pending_capture_request = 0;
@@ -1694,10 +1639,10 @@ static int atomisp_streamon(struct file *file, void *fh,
 			if (asd->delayed_init == ATOMISP_DELAYED_INIT_QUEUED) {
 				flush_work(&asd->delayed_init_work);
 				mutex_unlock(&isp->mutex);
-				if (wait_for_completion_interruptible(
-					&asd->init_done) != 0)
-					return -ERESTARTSYS;
+				ret = wait_for_completion_interruptible(&asd->init_done);
 				mutex_lock(&isp->mutex);
+				if (ret != 0)
+					return -ERESTARTSYS;
 			}
 
 			/* handle per_frame_setting parameter and buffers */
@@ -1719,16 +1664,15 @@ static int atomisp_streamon(struct file *file, void *fh,
 					asd->params.offline_parm.num_captures,
 					asd->params.offline_parm.skip_frames,
 					asd->params.offline_parm.offset);
-				if (ret) {
-					ret = -EINVAL;
-					goto out;
-				}
+				if (ret)
+					return -EINVAL;
+
 				if (asd->depth_mode->val)
 					atomisp_pause_buffer_event(isp);
 			}
 		}
 		atomisp_qbuffers_to_css(asd);
-		goto out;
+		return 0;
 	}
 
 	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
@@ -1754,7 +1698,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 
 	ret = atomisp_css_start(asd, css_pipe_id, false);
 	if (ret)
-		goto out;
+		return ret;
 
 	spin_lock_irqsave(&isp->lock, irqflags);
 	asd->streaming = ATOMISP_DEVICE_STREAMING_ENABLED;
@@ -1775,7 +1719,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 
 	/* Only start sensor when the last streaming instance started */
 	if (atomisp_subdev_streaming_count(asd) < sensor_start_stream)
-		goto out;
+		return 0;
 
 start_sensor:
 	if (isp->flash) {
@@ -1806,7 +1750,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 		ret = atomisp_stream_on_master_slave_sensor(isp, false);
 		if (ret) {
 			dev_err(isp->dev, "master slave sensor stream on failed!\n");
-			goto out;
+			return ret;
 		}
 		goto start_delay_wq;
 	} else if (asd->depth_mode->val && (atomisp_streaming_count(isp) <
@@ -1828,8 +1772,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 		spin_lock_irqsave(&isp->lock, irqflags);
 		asd->streaming = ATOMISP_DEVICE_STREAMING_DISABLED;
 		spin_unlock_irqrestore(&isp->lock, irqflags);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 start_delay_wq:
@@ -1846,9 +1789,8 @@ static int atomisp_streamon(struct file *file, void *fh,
 	} else {
 		asd->delayed_init = ATOMISP_DELAYED_INIT_NOT_QUEUED;
 	}
-out:
-	mutex_unlock(&isp->mutex);
-	return ret;
+
+	return 0;
 }
 
 int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
@@ -2076,15 +2018,7 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 static int atomisp_streamoff(struct file *file, void *fh,
 			     enum v4l2_buf_type type)
 {
-	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
-	int rval;
-
-	mutex_lock(&isp->mutex);
-	rval = __atomisp_streamoff(file, fh, type);
-	mutex_unlock(&isp->mutex);
-
-	return rval;
+	return __atomisp_streamoff(file, fh, type);
 }
 
 /*
@@ -2110,8 +2044,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	mutex_lock(&isp->mutex);
-
 	switch (control->id) {
 	case V4L2_CID_IRIS_ABSOLUTE:
 	case V4L2_CID_EXPOSURE_ABSOLUTE:
@@ -2133,7 +2065,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 	case V4L2_CID_TEST_PATTERN_COLOR_GR:
 	case V4L2_CID_TEST_PATTERN_COLOR_GB:
 	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		mutex_unlock(&isp->mutex);
 		return v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
 				   ctrl_handler, control);
 	case V4L2_CID_COLORFX:
@@ -2162,7 +2093,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 		break;
 	}
 
-	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -2189,7 +2119,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	if (ret)
 		return ret;
 
-	mutex_lock(&isp->mutex);
 	switch (control->id) {
 	case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
 	case V4L2_CID_EXPOSURE:
@@ -2210,7 +2139,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 	case V4L2_CID_TEST_PATTERN_COLOR_GR:
 	case V4L2_CID_TEST_PATTERN_COLOR_GB:
 	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		mutex_unlock(&isp->mutex);
 		return v4l2_s_ctrl(NULL,
 				   isp->inputs[asd->input_curr].camera->
 				   ctrl_handler, control);
@@ -2242,7 +2170,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 		ret = -EINVAL;
 		break;
 	}
-	mutex_unlock(&isp->mutex);
 	return ret;
 }
 
@@ -2355,9 +2282,7 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 						&ctrl);
 			break;
 		case V4L2_CID_ZOOM_ABSOLUTE:
-			mutex_lock(&isp->mutex);
 			ret = atomisp_digital_zoom(asd, 0, &ctrl.value);
-			mutex_unlock(&isp->mutex);
 			break;
 		case V4L2_CID_G_SKIP_FRAMES:
 			ret = v4l2_subdev_call(
@@ -2464,7 +2389,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 		case V4L2_CID_FLASH_STROBE:
 		case V4L2_CID_FLASH_MODE:
 		case V4L2_CID_FLASH_STATUS_REGISTER:
-			mutex_lock(&isp->mutex);
 			if (isp->flash) {
 				ret =
 				    v4l2_s_ctrl(NULL, isp->flash->ctrl_handler,
@@ -2479,12 +2403,9 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 					asd->params.num_flash_frames = 0;
 				}
 			}
-			mutex_unlock(&isp->mutex);
 			break;
 		case V4L2_CID_ZOOM_ABSOLUTE:
-			mutex_lock(&isp->mutex);
 			ret = atomisp_digital_zoom(asd, 1, &ctrl.value);
-			mutex_unlock(&isp->mutex);
 			break;
 		default:
 			ctr = v4l2_ctrl_find(&asd->ctrl_handler, ctrl.id);
@@ -2546,9 +2467,7 @@ static int atomisp_g_parm(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	mutex_lock(&isp->mutex);
 	parm->parm.capture.capturemode = asd->run_mode->val;
-	mutex_unlock(&isp->mutex);
 
 	return 0;
 }
@@ -2568,8 +2487,6 @@ static int atomisp_s_parm(struct file *file, void *fh,
 		return -EINVAL;
 	}
 
-	mutex_lock(&isp->mutex);
-
 	asd->high_speed_mode = false;
 	switch (parm->parm.capture.capturemode) {
 	case CI_MODE_NONE: {
@@ -2588,7 +2505,7 @@ static int atomisp_s_parm(struct file *file, void *fh,
 				asd->high_speed_mode = true;
 		}
 
-		goto out;
+		return rval == -ENOIOCTLCMD ? 0 : rval;
 	}
 	case CI_MODE_VIDEO:
 		mode = ATOMISP_RUN_MODE_VIDEO;
@@ -2603,15 +2520,11 @@ static int atomisp_s_parm(struct file *file, void *fh,
 		mode = ATOMISP_RUN_MODE_PREVIEW;
 		break;
 	default:
-		rval = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	rval = v4l2_ctrl_s_ctrl(asd->run_mode, mode);
 
-out:
-	mutex_unlock(&isp->mutex);
-
 	return rval == -ENOIOCTLCMD ? 0 : rval;
 }
 
@@ -2629,24 +2542,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 	else
 		motor = isp->motor;
 
-	switch (cmd) {
-	case ATOMISP_IOC_G_MOTOR_PRIV_INT_DATA:
-	case ATOMISP_IOC_S_EXPOSURE:
-	case ATOMISP_IOC_G_SENSOR_CALIBRATION_GROUP:
-	case ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA:
-	case ATOMISP_IOC_EXT_ISP_CTRL:
-	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:
-	case ATOMISP_IOC_S_SENSOR_EE_CONFIG:
-	case ATOMISP_IOC_G_UPDATE_EXPOSURE:
-		/* we do not need take isp->mutex for these IOCTLs */
-		break;
-	default:
-		mutex_lock(&isp->mutex);
-		break;
-	}
 	switch (cmd) {
 	case ATOMISP_IOC_S_SENSOR_RUNMODE:
 		if (IS_ISP2401)
@@ -2893,22 +2788,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		break;
 	}
 
-	switch (cmd) {
-	case ATOMISP_IOC_G_MOTOR_PRIV_INT_DATA:
-	case ATOMISP_IOC_S_EXPOSURE:
-	case ATOMISP_IOC_G_SENSOR_CALIBRATION_GROUP:
-	case ATOMISP_IOC_G_SENSOR_PRIV_INT_DATA:
-	case ATOMISP_IOC_EXT_ISP_CTRL:
-	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:
-	case ATOMISP_IOC_G_UPDATE_EXPOSURE:
-		break;
-	default:
-		mutex_unlock(&isp->mutex);
-		break;
-	}
 	return err;
 }
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 4ab91858d308..026ff3ca5c04 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -441,6 +441,7 @@ int atomisp_video_init(struct atomisp_video_pipe *video, const char *name,
 		video->pad.flags = MEDIA_PAD_FL_SINK;
 		video->vdev.fops = &atomisp_fops;
 		video->vdev.ioctl_ops = &atomisp_ioctl_ops;
+		video->vdev.lock = &video->isp->mutex;
 		break;
 	default:
 		return -EINVAL;
-- 
2.37.3

[PATCH 11/17] media: atomisp: Remove a couple of not useful function wrappers

[Hans de Goede]

The __atomisp_reqbufs(), __atomisp_streamoff() are 1:1 wrappers for
the non __ prefixed functions now, drop these wrappers.

The atomisp_s_fmt_cap() wrapper is almost a 1:1 wrapper for
atomisp_set_fmt() adjust the latter to have the right function
prototype and drop the wrapper.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   |  3 ++-
 .../staging/media/atomisp/pci/atomisp_cmd.h   |  2 +-
 .../staging/media/atomisp/pci/atomisp_fops.c  | 10 +++----
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 27 +++----------------
 .../staging/media/atomisp/pci/atomisp_ioctl.h |  8 ++----
 5 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index a96a4658e113..20962d4c2b76 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -5167,8 +5167,9 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 	return css_input_resolution_changed(asd, ffmt);
 }
 
-int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f)
+int atomisp_set_fmt(struct file *file, void *unused, struct v4l2_format *f)
 {
+	struct video_device *vdev = video_devdata(file);
 	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;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index 5ab7d6aca7fa..cfc970b531f0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -266,7 +266,7 @@ int atomisp_get_sensor_mode_data(struct atomisp_sub_device *asd,
 int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 		    bool *res_overflow);
 
-int atomisp_set_fmt(struct video_device *vdev, struct v4l2_format *f);
+int atomisp_set_fmt(struct file *file, void *fh, struct v4l2_format *f);
 
 int atomisp_set_shading_table(struct atomisp_sub_device *asd,
 			      struct atomisp_shading_table *shading_table);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 531bbd6d7ee0..047e1180e35f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -836,18 +836,16 @@ static int atomisp_release(struct file *file)
 			 __func__);
 
 	if (pipe->capq.streaming &&
-	    __atomisp_streamoff(file, NULL, V4L2_BUF_TYPE_VIDEO_CAPTURE)) {
-		dev_err(isp->dev,
-			"atomisp_streamoff failed on release, driver bug");
+	    atomisp_streamoff(file, NULL, V4L2_BUF_TYPE_VIDEO_CAPTURE)) {
+		dev_err(isp->dev, "atomisp_streamoff failed on release, driver bug");
 		goto done;
 	}
 
 	if (pipe->users)
 		goto done;
 
-	if (__atomisp_reqbufs(file, NULL, &req)) {
-		dev_err(isp->dev,
-			"atomisp_reqbufs failed on release, driver bug");
+	if (atomisp_reqbufs(file, NULL, &req)) {
+		dev_err(isp->dev, "atomisp_reqbufs failed on release, driver bug");
 		goto done;
 	}
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 42d8d1267553..ed3ec603a713 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -968,14 +968,6 @@ static int atomisp_g_fmt_cap(struct file *file, void *fh,
 	return atomisp_try_fmt_cap(file, fh, f);
 }
 
-static int atomisp_s_fmt_cap(struct file *file, void *fh,
-			     struct v4l2_format *f)
-{
-	struct video_device *vdev = video_devdata(file);
-
-	return atomisp_set_fmt(vdev, f);
-}
-
 /*
  * Free videobuffer buffer priv data
  */
@@ -1111,8 +1103,7 @@ int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 /*
  * Initiate Memory Mapping or User Pointer I/O
  */
-int __atomisp_reqbufs(struct file *file, void *fh,
-		      struct v4l2_requestbuffers *req)
+int atomisp_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *req)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
@@ -1184,12 +1175,6 @@ int __atomisp_reqbufs(struct file *file, void *fh,
 	return -ENOMEM;
 }
 
-int atomisp_reqbufs(struct file *file, void *fh,
-		    struct v4l2_requestbuffers *req)
-{
-	return __atomisp_reqbufs(file, fh, req);
-}
-
 /* application query the status of a buffer */
 static int atomisp_querybuf(struct file *file, void *fh,
 			    struct v4l2_buffer *buf)
@@ -1793,7 +1778,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 	return 0;
 }
 
-int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
+int atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 {
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_device *isp = video_get_drvdata(vdev);
@@ -2015,12 +2000,6 @@ int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	return ret;
 }
 
-static int atomisp_streamoff(struct file *file, void *fh,
-			     enum v4l2_buf_type type)
-{
-	return __atomisp_streamoff(file, fh, type);
-}
-
 /*
  * To get the current value of a control.
  * applications initialize the id field of a struct v4l2_control and
@@ -2806,7 +2785,7 @@ const struct v4l2_ioctl_ops atomisp_ioctl_ops = {
 	.vidioc_enum_fmt_vid_cap = atomisp_enum_fmt_cap,
 	.vidioc_try_fmt_vid_cap = atomisp_try_fmt_cap,
 	.vidioc_g_fmt_vid_cap = atomisp_g_fmt_cap,
-	.vidioc_s_fmt_vid_cap = atomisp_s_fmt_cap,
+	.vidioc_s_fmt_vid_cap = atomisp_set_fmt,
 	.vidioc_reqbufs = atomisp_reqbufs,
 	.vidioc_querybuf = atomisp_querybuf,
 	.vidioc_qbuf = atomisp_qbuf,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
index 61a6148a6ad5..c660f631d371 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.h
@@ -39,12 +39,8 @@ int atomisp_pipe_check(struct atomisp_video_pipe *pipe, bool streaming_ok);
 int atomisp_alloc_css_stat_bufs(struct atomisp_sub_device *asd,
 				uint16_t stream_id);
 
-int __atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type);
-int __atomisp_reqbufs(struct file *file, void *fh,
-		      struct v4l2_requestbuffers *req);
-
-int atomisp_reqbufs(struct file *file, void *fh,
-		    struct v4l2_requestbuffers *req);
+int atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type);
+int atomisp_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *req);
 
 enum ia_css_pipe_id atomisp_get_css_pipe_id(struct atomisp_sub_device
 	*asd);
-- 
2.37.3

[PATCH 12/17] media: atomisp: Drop unnecessary first_streamoff check

[Hans de Goede]

Drop an unnecessary first_streamoff check from atomisp_streamoff(),
above the check there is a:

	if (!first_streamoff)
		goto stop_sensor;

Code block which will jump over the code with the test, so the test
is only executed when first_streamoff is true and therefor the test
is not necessary.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index ed3ec603a713..77c0d55ab409 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1880,10 +1880,10 @@ int atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 		cancel_work_sync(&asd->delayed_init_work);
 		asd->delayed_init = ATOMISP_DELAYED_INIT_NOT_QUEUED;
 	}
-	if (first_streamoff) {
-		css_pipe_id = atomisp_get_css_pipe_id(asd);
-		atomisp_css_stop(asd, css_pipe_id, false);
-	}
+
+	css_pipe_id = atomisp_get_css_pipe_id(asd);
+	atomisp_css_stop(asd, css_pipe_id, false);
+
 	/* cancel work queue*/
 	if (asd->video_out_capture.users) {
 		capture_pipe = &asd->video_out_capture;
-- 
2.37.3

[PATCH 13/17] media: atomisp: Make atomisp_set_raw_buffer_bitmap() static

[Hans de Goede]

atomisp_set_raw_buffer_bitmap() is only used in atomisp_cmd.c,
make it static. Unfortunately this still requires a forward
declaration (the function cannot be moved easily).

Still this will at least make it obvious to anyone reading the code
that the function is not used elsewhere.

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

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 20962d4c2b76..8cc8ee64fb44 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -80,6 +80,8 @@ union host {
 	} ptr;
 };
 
+static int atomisp_set_raw_buffer_bitmap(struct atomisp_sub_device *asd, int exp_id);
+
 /*
  * get sensor:dis71430/ov2720 related info from v4l2_subdev->priv data field.
  * subdev->priv is set in mrst.c
@@ -5993,7 +5995,7 @@ void atomisp_init_raw_buffer_bitmap(struct atomisp_sub_device *asd)
 	spin_unlock_irqrestore(&asd->raw_buffer_bitmap_lock, flags);
 }
 
-int atomisp_set_raw_buffer_bitmap(struct atomisp_sub_device *asd, int exp_id)
+static int atomisp_set_raw_buffer_bitmap(struct atomisp_sub_device *asd, int exp_id)
 {
 	int *bitmap, bit;
 	unsigned long flags;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index cfc970b531f0..ba3433a63595 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -321,8 +321,6 @@ void atomisp_flush_params_queue(struct atomisp_video_pipe *asd);
 int atomisp_exp_id_unlock(struct atomisp_sub_device *asd, int *exp_id);
 int atomisp_exp_id_capture(struct atomisp_sub_device *asd, int *exp_id);
 
-/* Function to update Raw Buffer bitmap */
-int atomisp_set_raw_buffer_bitmap(struct atomisp_sub_device *asd, int exp_id);
 void atomisp_init_raw_buffer_bitmap(struct atomisp_sub_device *asd);
 
 /* Function to enable/disable zoom for capture pipe */
-- 
2.37.3

[PATCH 14/17] media: atomisp: Remove unused atomisp_css_get_dis_statistics()

[Hans de Goede]

Remove the unused atomisp_css_get_dis_statistics() function. This seems to
be a leftover variant / older version of atomisp_css_get_dis_stats() which
is actually used.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_compat.h |  4 ----
 .../media/atomisp/pci/atomisp_compat_css20.c       | 14 --------------
 2 files changed, 18 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat.h b/drivers/staging/media/atomisp/pci/atomisp_compat.h
index af6ab8434b5e..a6d85d0f9ae5 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat.h
@@ -129,10 +129,6 @@ int atomisp_alloc_metadata_output_buf(struct atomisp_sub_device *asd);
 
 void atomisp_free_metadata_output_buf(struct atomisp_sub_device *asd);
 
-void atomisp_css_get_dis_statistics(struct atomisp_sub_device *asd,
-				    struct atomisp_css_buffer *isp_css_buffer,
-				    struct ia_css_isp_dvs_statistics_map *dvs_map);
-
 void atomisp_css_temp_pipe_to_pipe_id(struct atomisp_sub_device *asd,
 				      struct atomisp_css_event *current_event);
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 0154ebf2cba5..64dd63ddc29c 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -1574,20 +1574,6 @@ void atomisp_free_metadata_output_buf(struct atomisp_sub_device *asd)
 	}
 }
 
-void atomisp_css_get_dis_statistics(struct atomisp_sub_device *asd,
-				    struct atomisp_css_buffer *isp_css_buffer,
-				    struct ia_css_isp_dvs_statistics_map *dvs_map)
-{
-	if (asd->params.dvs_stat) {
-		if (dvs_map)
-			ia_css_translate_dvs2_statistics(
-			    asd->params.dvs_stat, dvs_map);
-		else
-			ia_css_get_dvs2_statistics(asd->params.dvs_stat,
-						   isp_css_buffer->css_buffer.data.stats_dvs);
-	}
-}
-
 void atomisp_css_temp_pipe_to_pipe_id(struct atomisp_sub_device *asd,
 				      struct atomisp_css_event *current_event)
 {
-- 
2.37.3

[PATCH 15/17] media: atomisp: Remove const/fixed camera_caps

[Hans de Goede]

The code checks a camera_caps struct in various places, but this always
points to the same const camera_caps struct.

Remove the checks, keeping the code paths which would be taken with
the fixed camera caps struct still in place and remove the camera_caps
struct itself.

Note this completely removes atomisp_pause_buffer_event() because that
only ever does something if camera_caps.sensors[0].is_slave is true and
that never is true.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../atomisp/include/linux/atomisp_platform.h  |  18 ---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 111 +-----------------
 .../staging/media/atomisp/pci/atomisp_fops.c  |  44 -------
 .../media/atomisp/pci/atomisp_gmin_platform.c |  18 ---
 .../media/atomisp/pci/atomisp_internal.h      |   5 +-
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  81 ++-----------
 .../media/atomisp/pci/atomisp_subdev.h        |   3 -
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  18 ---
 8 files changed, 17 insertions(+), 281 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
index 8c65733e0255..0253661d4332 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
@@ -141,23 +141,6 @@ struct atomisp_platform_data {
 	struct intel_v4l2_subdev_table *subdevs;
 };
 
-/* Describe the capacities of one single sensor. */
-struct atomisp_sensor_caps {
-	/* The number of streams this sensor can output. */
-	int stream_num;
-	bool is_slave;
-};
-
-/* Describe the capacities of sensors connected to one camera port. */
-struct atomisp_camera_caps {
-	/* The number of sensors connected to this camera port. */
-	int sensor_num;
-	/* The capacities of each sensor. */
-	struct atomisp_sensor_caps sensor[MAX_SENSORS_PER_PORT];
-	/* Define whether stream control is required for multiple streams. */
-	bool multi_stream_ctrl;
-};
-
 /*
  *  Sensor of external ISP can send multiple steams with different mipi data
  * type in the same virtual channel. This information needs to come from the
@@ -235,7 +218,6 @@ struct camera_mipi_info {
 };
 
 const struct atomisp_platform_data *atomisp_get_platform_data(void);
-const struct atomisp_camera_caps *atomisp_get_default_camera_caps(void);
 
 /* API from old platform_camera.h, new CPUID implementation */
 #define __IS_SOC(x) (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL && \
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 8cc8ee64fb44..b01cacb8d2a8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -770,24 +770,6 @@ static struct atomisp_video_pipe *__atomisp_get_pipe(
     enum ia_css_pipe_id css_pipe_id,
     enum ia_css_buffer_type buf_type)
 {
-	struct atomisp_device *isp = asd->isp;
-
-	if (css_pipe_id == IA_CSS_PIPE_ID_COPY &&
-	    isp->inputs[asd->input_curr].camera_caps->
-	    sensor[asd->sensor_curr].stream_num > 1) {
-		switch (stream_id) {
-		case ATOMISP_INPUT_STREAM_PREVIEW:
-			return &asd->video_out_preview;
-		case ATOMISP_INPUT_STREAM_POSTVIEW:
-			return &asd->video_out_vf;
-		case ATOMISP_INPUT_STREAM_VIDEO:
-			return &asd->video_out_video_capture;
-		case ATOMISP_INPUT_STREAM_CAPTURE:
-		default:
-			return &asd->video_out_capture;
-		}
-	}
-
 	/* video is same in online as in continuouscapture mode */
 	if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
 		/*
@@ -5051,12 +5033,7 @@ static void atomisp_check_copy_mode(struct atomisp_sub_device *asd,
 	src = atomisp_subdev_get_ffmt(&asd->subdev, NULL,
 				      V4L2_SUBDEV_FORMAT_ACTIVE, source_pad);
 
-	if ((sink->code == src->code &&
-	     sink->width == f->width &&
-	     sink->height == f->height) ||
-	    ((asd->isp->inputs[asd->input_curr].type == SOC_CAMERA) &&
-	     (asd->isp->inputs[asd->input_curr].camera_caps->
-	      sensor[asd->sensor_curr].stream_num > 1)))
+	if (sink->code == src->code && sink->width == f->width && sink->height == f->height)
 		asd->copy_mode = true;
 	else
 		asd->copy_mode = false;
@@ -5282,58 +5259,7 @@ int atomisp_set_fmt(struct file *file, void *unused, struct v4l2_format *f)
 			f->fmt.pix.height = r.height;
 		}
 
-		if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW &&
-		    (asd->isp->inputs[asd->input_curr].type == SOC_CAMERA) &&
-		    (asd->isp->inputs[asd->input_curr].camera_caps->
-		     sensor[asd->sensor_curr].stream_num > 1)) {
-			/* For M10MO outputing YUV preview images. */
-			u16 video_index =
-			    atomisp_source_pad_to_stream_id(asd,
-							    ATOMISP_SUBDEV_PAD_SOURCE_VIDEO);
-
-			ret = atomisp_css_copy_get_output_frame_info(asd,
-				video_index, &output_info);
-			if (ret) {
-				dev_err(isp->dev,
-					"copy_get_output_frame_info ret %i", ret);
-				return -EINVAL;
-			}
-			if (!asd->yuvpp_mode) {
-				/*
-				 * If viewfinder was configured into copy_mode,
-				 * we switch to using yuvpp pipe instead.
-				 */
-				asd->yuvpp_mode = true;
-				ret = atomisp_css_copy_configure_output(
-					  asd, video_index, 0, 0, 0, 0);
-				if (ret) {
-					dev_err(isp->dev,
-						"failed to disable copy pipe");
-					return -EINVAL;
-				}
-				ret = atomisp_css_yuvpp_configure_output(
-					  asd, video_index,
-					  output_info.res.width,
-					  output_info.res.height,
-					  output_info.padded_width,
-					  output_info.format);
-				if (ret) {
-					dev_err(isp->dev,
-						"failed to set up yuvpp pipe\n");
-					return -EINVAL;
-				}
-				atomisp_css_video_enable_online(asd, false);
-				atomisp_css_preview_enable_online(asd,
-								  ATOMISP_INPUT_STREAM_GENERAL, false);
-			}
-			atomisp_css_yuvpp_configure_viewfinder(asd, video_index,
-							       f->fmt.pix.width, f->fmt.pix.height,
-							       format_bridge->planar ? f->fmt.pix.bytesperline
-							       : f->fmt.pix.bytesperline * 8
-							       / format_bridge->depth, format_bridge->sh_fmt);
-			atomisp_css_yuvpp_get_viewfinder_frame_info(
-			    asd, video_index, &output_info);
-		} else if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW) {
+		if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW) {
 			atomisp_css_video_configure_viewfinder(asd,
 							       f->fmt.pix.width, f->fmt.pix.height,
 							       format_bridge->planar ? f->fmt.pix.bytesperline
@@ -5918,31 +5844,7 @@ int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
 int atomisp_source_pad_to_stream_id(struct atomisp_sub_device *asd,
 				    uint16_t source_pad)
 {
-	int stream_id;
-	struct atomisp_device *isp = asd->isp;
-
-	if (isp->inputs[asd->input_curr].camera_caps->
-	    sensor[asd->sensor_curr].stream_num == 1)
-		return ATOMISP_INPUT_STREAM_GENERAL;
-
-	switch (source_pad) {
-	case ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE:
-		stream_id = ATOMISP_INPUT_STREAM_CAPTURE;
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_VF:
-		stream_id = ATOMISP_INPUT_STREAM_POSTVIEW;
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW:
-		stream_id = ATOMISP_INPUT_STREAM_PREVIEW;
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_VIDEO:
-		stream_id = ATOMISP_INPUT_STREAM_VIDEO;
-		break;
-	default:
-		stream_id = ATOMISP_INPUT_STREAM_GENERAL;
-	}
-
-	return stream_id;
+	return ATOMISP_INPUT_STREAM_GENERAL;
 }
 
 bool atomisp_is_vf_pipe(struct atomisp_video_pipe *pipe)
@@ -6217,13 +6119,6 @@ int atomisp_get_invalid_frame_num(struct video_device *vdev,
 	struct ia_css_pipe_info p_info;
 	int ret;
 
-	if (asd->isp->inputs[asd->input_curr].camera_caps->
-	    sensor[asd->sensor_curr].stream_num > 1) {
-		/* External ISP */
-		*invalid_frame_num = 0;
-		return 0;
-	}
-
 	pipe_id = atomisp_get_pipe_id(pipe);
 	if (!asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].pipes[pipe_id]) {
 		dev_warn(asd->isp->dev,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c
index 047e1180e35f..84a84e0cdeef 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_fops.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c
@@ -369,45 +369,6 @@ static int atomisp_get_css_buf_type(struct atomisp_sub_device *asd,
 		return IA_CSS_BUFFER_TYPE_VF_OUTPUT_FRAME;
 }
 
-static int atomisp_qbuffers_to_css_for_all_pipes(struct atomisp_sub_device *asd)
-{
-	enum ia_css_buffer_type buf_type;
-	enum ia_css_pipe_id css_capture_pipe_id = IA_CSS_PIPE_ID_COPY;
-	enum ia_css_pipe_id css_preview_pipe_id = IA_CSS_PIPE_ID_COPY;
-	enum ia_css_pipe_id css_video_pipe_id = IA_CSS_PIPE_ID_COPY;
-	enum atomisp_input_stream_id input_stream_id;
-	struct atomisp_video_pipe *capture_pipe;
-	struct atomisp_video_pipe *preview_pipe;
-	struct atomisp_video_pipe *video_pipe;
-
-	capture_pipe = &asd->video_out_capture;
-	preview_pipe = &asd->video_out_preview;
-	video_pipe = &asd->video_out_video_capture;
-
-	buf_type = atomisp_get_css_buf_type(
-		       asd, css_preview_pipe_id,
-		       atomisp_subdev_source_pad(&preview_pipe->vdev));
-	input_stream_id = ATOMISP_INPUT_STREAM_PREVIEW;
-	atomisp_q_video_buffers_to_css(asd, preview_pipe,
-				       input_stream_id,
-				       buf_type, css_preview_pipe_id);
-
-	buf_type = atomisp_get_css_buf_type(asd, css_capture_pipe_id,
-					    atomisp_subdev_source_pad(&capture_pipe->vdev));
-	input_stream_id = ATOMISP_INPUT_STREAM_GENERAL;
-	atomisp_q_video_buffers_to_css(asd, capture_pipe,
-				       input_stream_id,
-				       buf_type, css_capture_pipe_id);
-
-	buf_type = atomisp_get_css_buf_type(asd, css_video_pipe_id,
-					    atomisp_subdev_source_pad(&video_pipe->vdev));
-	input_stream_id = ATOMISP_INPUT_STREAM_VIDEO;
-	atomisp_q_video_buffers_to_css(asd, video_pipe,
-				       input_stream_id,
-				       buf_type, css_video_pipe_id);
-	return 0;
-}
-
 /* queue all available buffers to css */
 int atomisp_qbuffers_to_css(struct atomisp_sub_device *asd)
 {
@@ -423,11 +384,6 @@ int atomisp_qbuffers_to_css(struct atomisp_sub_device *asd)
 	bool raw_mode = atomisp_is_mbuscode_raw(
 			    asd->fmt[asd->capture_pad].fmt.code);
 
-	if (asd->isp->inputs[asd->input_curr].camera_caps->
-	    sensor[asd->sensor_curr].stream_num == 2 &&
-	    !asd->yuvpp_mode)
-		return atomisp_qbuffers_to_css_for_all_pipes(asd);
-
 	if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
 		video_pipe = &asd->video_out_video_capture;
 		css_video_pipe_id = IA_CSS_PIPE_ID_VIDEO;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index f7fc5137199c..254e8c97f71f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -134,24 +134,6 @@ static DEFINE_MUTEX(vcm_lock);
 
 static struct gmin_subdev *find_gmin_subdev(struct v4l2_subdev *subdev);
 
-/*
- * Legacy/stub behavior copied from upstream platform_camera.c.  The
- * atomisp driver relies on these values being non-NULL in a few
- * places, even though they are hard-coded in all current
- * implementations.
- */
-const struct atomisp_camera_caps *atomisp_get_default_camera_caps(void)
-{
-	static const struct atomisp_camera_caps caps = {
-		.sensor_num = 1,
-		.sensor = {
-			{ .stream_num = 1, },
-		},
-	};
-	return &caps;
-}
-EXPORT_SYMBOL_GPL(atomisp_get_default_camera_caps);
-
 const struct atomisp_platform_data *atomisp_get_platform_data(void)
 {
 	return &pdata;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index f3ef840c640a..d9d158cdf09e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -127,9 +127,7 @@
  * Moorefield/Baytrail platform.
  */
 #define ATOMISP_SOC_CAMERA(asd)  \
-	(asd->isp->inputs[asd->input_curr].type == SOC_CAMERA \
-	&& asd->isp->inputs[asd->input_curr].camera_caps-> \
-	   sensor[asd->sensor_curr].stream_num == 1)
+	(asd->isp->inputs[asd->input_curr].type == SOC_CAMERA)
 
 #define ATOMISP_USE_YUVPP(asd)  \
 	(ATOMISP_SOC_CAMERA(asd) && ATOMISP_CSS_SUPPORT_YUVPP && \
@@ -162,7 +160,6 @@ struct atomisp_input_subdev {
 	 */
 	struct atomisp_sub_device *asd;
 
-	const struct atomisp_camera_caps *camera_caps;
 	int sensor_index;
 };
 
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 77c0d55ab409..cbbb25d3e5fe 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1431,16 +1431,6 @@ enum ia_css_pipe_id atomisp_get_css_pipe_id(struct atomisp_sub_device *asd)
 
 static unsigned int atomisp_sensor_start_stream(struct atomisp_sub_device *asd)
 {
-	struct atomisp_device *isp = asd->isp;
-
-	if (isp->inputs[asd->input_curr].camera_caps->
-	    sensor[asd->sensor_curr].stream_num > 1) {
-		if (asd->high_speed_mode)
-			return 1;
-		else
-			return 2;
-	}
-
 	if (asd->vfpp->val != ATOMISP_VFPP_ENABLE ||
 	    asd->copy_mode)
 		return 1;
@@ -1459,31 +1449,15 @@ static unsigned int atomisp_sensor_start_stream(struct atomisp_sub_device *asd)
 int atomisp_stream_on_master_slave_sensor(struct atomisp_device *isp,
 	bool isp_timeout)
 {
-	unsigned int master = -1, slave = -1, delay_slave = 0;
-	int i, ret;
-
-	/*
-	 * ISP only support 2 streams now so ignore multiple master/slave
-	 * case to reduce the delay between 2 stream_on calls.
-	 */
-	for (i = 0; i < isp->num_of_streams; i++) {
-		int sensor_index = isp->asd[i].input_curr;
-
-		if (isp->inputs[sensor_index].camera_caps->
-		    sensor[isp->asd[i].sensor_curr].is_slave)
-			slave = sensor_index;
-		else
-			master = sensor_index;
-	}
+	unsigned int master, slave, delay_slave = 0;
+	int ret;
 
-	if (master == -1 || slave == -1) {
-		master = ATOMISP_DEPTH_DEFAULT_MASTER_SENSOR;
-		slave = ATOMISP_DEPTH_DEFAULT_SLAVE_SENSOR;
-		dev_warn(isp->dev,
-			 "depth mode use default master=%s.slave=%s.\n",
-			 isp->inputs[master].camera->name,
-			 isp->inputs[slave].camera->name);
-	}
+	master = ATOMISP_DEPTH_DEFAULT_MASTER_SENSOR;
+	slave = ATOMISP_DEPTH_DEFAULT_SLAVE_SENSOR;
+	dev_warn(isp->dev,
+		 "depth mode use default master=%s.slave=%s.\n",
+		 isp->inputs[master].camera->name,
+		 isp->inputs[slave].camera->name);
 
 	ret = v4l2_subdev_call(isp->inputs[master].camera, core,
 			       ioctl, ATOMISP_IOC_G_DEPTH_SYNC_COMP,
@@ -1517,24 +1491,6 @@ int atomisp_stream_on_master_slave_sensor(struct atomisp_device *isp,
 	return 0;
 }
 
-static void atomisp_pause_buffer_event(struct atomisp_device *isp)
-{
-	struct v4l2_event event = {0};
-	int i;
-
-	event.type = V4L2_EVENT_ATOMISP_PAUSE_BUFFER;
-
-	for (i = 0; i < isp->num_of_streams; i++) {
-		int sensor_index = isp->asd[i].input_curr;
-
-		if (isp->inputs[sensor_index].camera_caps->
-		    sensor[isp->asd[i].sensor_curr].is_slave) {
-			v4l2_event_queue(isp->asd[i].subdev.devnode, &event);
-			break;
-		}
-	}
-}
-
 /* Input system HW workaround */
 /* Input system address translation corrupts burst during */
 /* invalidate. SW workaround for this is to set burst length */
@@ -1608,8 +1564,7 @@ static int atomisp_streamon(struct file *file, void *fh,
 	/* Reset pending capture request count. */
 	asd->pending_capture_request = 0;
 
-	if ((atomisp_subdev_streaming_count(asd) > sensor_start_stream) &&
-	    (!isp->inputs[asd->input_curr].camera_caps->multi_stream_ctrl)) {
+	if (atomisp_subdev_streaming_count(asd) > sensor_start_stream) {
 		/* trigger still capture */
 		if (asd->continuous_mode->val &&
 		    atomisp_subdev_source_pad(vdev)
@@ -1651,9 +1606,6 @@ static int atomisp_streamon(struct file *file, void *fh,
 					asd->params.offline_parm.offset);
 				if (ret)
 					return -EINVAL;
-
-				if (asd->depth_mode->val)
-					atomisp_pause_buffer_event(isp);
 			}
 		}
 		atomisp_qbuffers_to_css(asd);
@@ -1809,17 +1761,10 @@ int atomisp_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	 * do only videobuf_streamoff for capture & vf pipes in
 	 * case of continuous capture
 	 */
-	if ((asd->continuous_mode->val ||
-	     isp->inputs[asd->input_curr].camera_caps->multi_stream_ctrl) &&
-	    atomisp_subdev_source_pad(vdev) !=
-	    ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW &&
-	    atomisp_subdev_source_pad(vdev) !=
-	    ATOMISP_SUBDEV_PAD_SOURCE_VIDEO) {
-		if (isp->inputs[asd->input_curr].camera_caps->multi_stream_ctrl) {
-			v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-					 video, s_stream, 0);
-		} else if (atomisp_subdev_source_pad(vdev)
-			   == ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE) {
+	if (asd->continuous_mode->val &&
+	    atomisp_subdev_source_pad(vdev) != ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW &&
+	    atomisp_subdev_source_pad(vdev) != ATOMISP_SUBDEV_PAD_SOURCE_VIDEO) {
+		if (atomisp_subdev_source_pad(vdev) == ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE) {
 			/* stop continuous still capture if needed */
 			if (asd->params.offline_parm.num_captures == -1)
 				atomisp_css_offline_capture_configure(asd,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.h b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
index 43e6a1d1e410..a1f4da35235d 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.h
@@ -313,9 +313,6 @@ struct atomisp_sub_device {
 
 	/* This field specifies which camera (v4l2 input) is selected. */
 	int input_curr;
-	/* This field specifies which sensor is being selected when there
-	   are multiple sensors connected to the same MIPI port. */
-	int sensor_curr;
 
 	atomic_t sof_count;
 	atomic_t sequence;      /* Sequence value that is assigned to buffer. */
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 026ff3ca5c04..d5bb9906ca6f 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1007,7 +1007,6 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
 			    &subdevs->v4l2_subdev.board_info;
 		struct i2c_adapter *adapter =
 		    i2c_get_adapter(subdevs->v4l2_subdev.i2c_adapter_id);
-		int sensor_num, i;
 
 		dev_info(isp->dev, "Probing Subdev %s\n", board_info->type);
 
@@ -1066,22 +1065,7 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
 			 * pixel_format.
 			 */
 			isp->inputs[isp->input_cnt].frame_size.pixel_format = 0;
-			isp->inputs[isp->input_cnt].camera_caps =
-			    atomisp_get_default_camera_caps();
-			sensor_num = isp->inputs[isp->input_cnt]
-				     .camera_caps->sensor_num;
 			isp->input_cnt++;
-			for (i = 1; i < sensor_num; i++) {
-				if (isp->input_cnt >= ATOM_ISP_MAX_INPUTS) {
-					dev_warn(isp->dev,
-						 "atomisp inputs out of range\n");
-					break;
-				}
-				isp->inputs[isp->input_cnt] =
-				    isp->inputs[isp->input_cnt - 1];
-				isp->inputs[isp->input_cnt].sensor_index = i;
-				isp->input_cnt++;
-			}
 			break;
 		case CAMERA_MOTOR:
 			if (isp->motor) {
@@ -1239,8 +1223,6 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 			"TPG detected, camera_cnt: %d\n", isp->input_cnt);
 		isp->inputs[isp->input_cnt].type = TEST_PATTERN;
 		isp->inputs[isp->input_cnt].port = -1;
-		isp->inputs[isp->input_cnt].camera_caps =
-		    atomisp_get_default_camera_caps();
 		isp->inputs[isp->input_cnt++].camera = &isp->tpg.sd;
 	} else {
 		dev_warn(isp->dev, "too many atomisp inputs, TPG ignored.\n");
-- 
2.37.3

[PATCH 16/17] media: atomisp: Remove atomisp_source_pad_to_stream_id()

[Hans de Goede]

atomisp_source_pad_to_stream_id() returns ATOMISP_INPUT_STREAM_GENERAL
unconditionally now. Drop it and directly use ATOMISP_INPUT_STREAM_GENERAL
in its callers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 38 ++++++-------------
 .../staging/media/atomisp/pci/atomisp_cmd.h   |  2 -
 .../media/atomisp/pci/atomisp_compat_css20.c  |  7 ++--
 .../staging/media/atomisp/pci/atomisp_ioctl.c |  5 +--
 .../media/atomisp/pci/atomisp_subdev.c        | 25 +++++-------
 5 files changed, 25 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index b01cacb8d2a8..431defa30acf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -1938,7 +1938,6 @@ static void atomisp_update_grid_info(struct atomisp_sub_device *asd,
 {
 	struct atomisp_device *isp = asd->isp;
 	int err;
-	u16 stream_id = atomisp_source_pad_to_stream_id(asd, source_pad);
 
 	if (atomisp_css_get_grid_info(asd, pipe_id, source_pad))
 		return;
@@ -1947,7 +1946,7 @@ static void atomisp_update_grid_info(struct atomisp_sub_device *asd,
 	   the grid size. */
 	atomisp_css_free_stat_buffers(asd);
 
-	err = atomisp_alloc_css_stat_bufs(asd, stream_id);
+	err = atomisp_alloc_css_stat_bufs(asd, ATOMISP_INPUT_STREAM_GENERAL);
 	if (err) {
 		dev_err(isp->dev, "stat_buf allocate error\n");
 		goto err;
@@ -4431,8 +4430,6 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 	const struct atomisp_format_bridge *fmt;
 	struct atomisp_input_stream_info *stream_info =
 	    (struct atomisp_input_stream_info *)snr_mbus_fmt->reserved;
-	u16 stream_index;
-	int source_pad = atomisp_subdev_source_pad(vdev);
 	int ret;
 
 	if (!asd) {
@@ -4444,7 +4441,6 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 	if (!isp->inputs[asd->input_curr].camera)
 		return -EINVAL;
 
-	stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
 	fmt = atomisp_get_format_bridge(f->pixelformat);
 	if (!fmt) {
 		dev_err(isp->dev, "unsupported pixelformat!\n");
@@ -4458,7 +4454,7 @@ int atomisp_try_fmt(struct video_device *vdev, struct v4l2_pix_format *f,
 	snr_mbus_fmt->width = f->width;
 	snr_mbus_fmt->height = f->height;
 
-	__atomisp_init_stream_info(stream_index, stream_info);
+	__atomisp_init_stream_info(ATOMISP_INPUT_STREAM_GENERAL, stream_info);
 
 	dev_dbg(isp->dev, "try_mbus_fmt: asking for %ux%u\n",
 		snr_mbus_fmt->width, snr_mbus_fmt->height);
@@ -4743,7 +4739,6 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	int (*configure_pp_input)(struct atomisp_sub_device *asd,
 				  unsigned int width, unsigned int height) =
 				      configure_pp_input_nop;
-	u16 stream_index;
 	const struct atomisp_in_fmt_conv *fc;
 	int ret, i;
 
@@ -4752,7 +4747,6 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 			__func__, vdev->name);
 		return -EINVAL;
 	}
-	stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
 
 	v4l2_fh_init(&fh.vfh, vdev);
 
@@ -4772,7 +4766,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 			dev_err(isp->dev, "mipi_info is NULL\n");
 			return -EINVAL;
 		}
-		if (atomisp_set_sensor_mipi_to_isp(asd, stream_index,
+		if (atomisp_set_sensor_mipi_to_isp(asd, ATOMISP_INPUT_STREAM_GENERAL,
 						   mipi_info))
 			return -EINVAL;
 		fc = atomisp_find_in_fmt_conv_by_atomisp_in_fmt(
@@ -4856,7 +4850,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 	/* ISP2401 new input system need to use copy pipe */
 	if (asd->copy_mode) {
 		pipe_id = IA_CSS_PIPE_ID_COPY;
-		atomisp_css_capture_enable_online(asd, stream_index, false);
+		atomisp_css_capture_enable_online(asd, ATOMISP_INPUT_STREAM_GENERAL, false);
 	} else if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
 		/* video same in continuouscapture and online modes */
 		configure_output = atomisp_css_video_configure_output;
@@ -4888,7 +4882,8 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 				pipe_id = IA_CSS_PIPE_ID_CAPTURE;
 
 				atomisp_update_capture_mode(asd);
-				atomisp_css_capture_enable_online(asd, stream_index, false);
+				atomisp_css_capture_enable_online(
+					asd, ATOMISP_INPUT_STREAM_GENERAL, false);
 			}
 		}
 	} else if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW) {
@@ -4913,7 +4908,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 
 		if (!asd->continuous_mode->val)
 			/* in case of ANR, force capture pipe to offline mode */
-			atomisp_css_capture_enable_online(asd, stream_index,
+			atomisp_css_capture_enable_online(asd, ATOMISP_INPUT_STREAM_GENERAL,
 							  asd->params.low_light ?
 							  false : asd->params.online_process);
 
@@ -4944,7 +4939,7 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 		pipe_id = IA_CSS_PIPE_ID_YUVPP;
 
 	if (asd->copy_mode)
-		ret = atomisp_css_copy_configure_output(asd, stream_index,
+		ret = atomisp_css_copy_configure_output(asd, ATOMISP_INPUT_STREAM_GENERAL,
 							pix->width, pix->height,
 							format->planar ? pix->bytesperline :
 							pix->bytesperline * 8 / format->depth,
@@ -4968,8 +4963,8 @@ static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 		return -EINVAL;
 	}
 	if (asd->copy_mode)
-		ret = atomisp_css_copy_get_output_frame_info(asd, stream_index,
-			output_info);
+		ret = atomisp_css_copy_get_output_frame_info(
+			asd, ATOMISP_INPUT_STREAM_GENERAL, output_info);
 	else
 		ret = get_frame_info(asd, output_info);
 	if (ret) {
@@ -5061,7 +5056,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 	struct atomisp_device *isp;
 	struct atomisp_input_stream_info *stream_info =
 	    (struct atomisp_input_stream_info *)ffmt->reserved;
-	u16 stream_index = ATOMISP_INPUT_STREAM_GENERAL;
 	int source_pad = atomisp_subdev_source_pad(vdev);
 	struct v4l2_subdev_fh fh;
 	int ret;
@@ -5076,8 +5070,6 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 
 	v4l2_fh_init(&fh.vfh, vdev);
 
-	stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
-
 	format = atomisp_get_format_bridge(pixelformat);
 	if (!format)
 		return -EINVAL;
@@ -5090,7 +5082,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 		ffmt->width, ffmt->height, padding_w, padding_h,
 		dvs_env_w, dvs_env_h);
 
-	__atomisp_init_stream_info(stream_index, stream_info);
+	__atomisp_init_stream_info(ATOMISP_INPUT_STREAM_GENERAL, stream_info);
 
 	req_ffmt = ffmt;
 
@@ -5122,7 +5114,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev,
 	if (ret)
 		return ret;
 
-	__atomisp_update_stream_env(asd, stream_index, stream_info);
+	__atomisp_update_stream_env(asd, ATOMISP_INPUT_STREAM_GENERAL, stream_info);
 
 	dev_dbg(isp->dev, "sensor width: %d, height: %d\n",
 		ffmt->width, ffmt->height);
@@ -5841,12 +5833,6 @@ int atomisp_flash_enable(struct atomisp_sub_device *asd, int num_frames)
 	return 0;
 }
 
-int atomisp_source_pad_to_stream_id(struct atomisp_sub_device *asd,
-				    uint16_t source_pad)
-{
-	return ATOMISP_INPUT_STREAM_GENERAL;
-}
-
 bool atomisp_is_vf_pipe(struct atomisp_video_pipe *pipe)
 {
 	struct atomisp_sub_device *asd = pipe->asd;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.h b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
index ba3433a63595..c9f92f1326b6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.h
@@ -297,8 +297,6 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
 		      bool q_buffers, enum atomisp_input_stream_id stream_id);
 
 void atomisp_css_flush(struct atomisp_device *isp);
-int atomisp_source_pad_to_stream_id(struct atomisp_sub_device *asd,
-				    uint16_t source_pad);
 
 /* Events. Only one event has to be exported for now. */
 void atomisp_eof_event(struct atomisp_sub_device *asd, uint8_t exp_id);
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index 64dd63ddc29c..fdc05548d972 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -1427,7 +1427,6 @@ int atomisp_css_get_grid_info(struct atomisp_sub_device *asd,
 	struct ia_css_pipe_info p_info;
 	struct ia_css_grid_info old_info;
 	struct atomisp_device *isp = asd->isp;
-	int stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
 	int md_width = asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].
 		       stream_config.metadata_config.resolution.width;
 
@@ -1435,7 +1434,7 @@ int atomisp_css_get_grid_info(struct atomisp_sub_device *asd,
 	memset(&old_info, 0, sizeof(struct ia_css_grid_info));
 
 	if (ia_css_pipe_get_info(
-		asd->stream_env[stream_index].pipes[pipe_id],
+		asd->stream_env[ATOMISP_INPUT_STREAM_GENERAL].pipes[pipe_id],
 		&p_info) != 0) {
 		dev_err(isp->dev, "ia_css_pipe_get_info failed\n");
 		return -EINVAL;
@@ -2680,11 +2679,11 @@ int atomisp_get_css_frame_info(struct atomisp_sub_device *asd,
 	struct atomisp_device *isp = asd->isp;
 
 	if (ATOMISP_SOC_CAMERA(asd)) {
-		stream_index = atomisp_source_pad_to_stream_id(asd, source_pad);
+		stream_index = ATOMISP_INPUT_STREAM_GENERAL;
 	} else {
 		stream_index = (pipe_index == IA_CSS_PIPE_ID_YUVPP) ?
 			       ATOMISP_INPUT_STREAM_VIDEO :
-			       atomisp_source_pad_to_stream_id(asd, source_pad);
+			       ATOMISP_INPUT_STREAM_GENERAL;
 	}
 
 	if (0 != ia_css_pipe_get_info(asd->stream_env[stream_index]
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index cbbb25d3e5fe..aefa7c07242a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1112,11 +1112,8 @@ int atomisp_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *req
 	struct ia_css_frame *frame;
 	struct videobuf_vmalloc_memory *vm_mem;
 	u16 source_pad = atomisp_subdev_source_pad(vdev);
-	u16 stream_id;
 	int ret = 0, i = 0;
 
-	stream_id = atomisp_source_pad_to_stream_id(asd, source_pad);
-
 	if (req->count == 0) {
 		mutex_lock(&pipe->capq.vb_lock);
 		if (!list_empty(&pipe->capq.stream))
@@ -1137,7 +1134,7 @@ int atomisp_reqbufs(struct file *file, void *fh, struct v4l2_requestbuffers *req
 	if (ret)
 		return ret;
 
-	atomisp_alloc_css_stat_bufs(asd, stream_id);
+	atomisp_alloc_css_stat_bufs(asd, ATOMISP_INPUT_STREAM_GENERAL);
 
 	/*
 	 * for user pointer type, buffers are not really allocated here,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 88bf693f4c50..27a7cd98f935 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -373,16 +373,12 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	struct atomisp_sub_device *isp_sd = v4l2_get_subdevdata(sd);
 	struct atomisp_device *isp = isp_sd->isp;
 	struct v4l2_mbus_framefmt *ffmt[ATOMISP_SUBDEV_PADS_NUM];
-	u16 vdev_pad = atomisp_subdev_source_pad(sd->devnode);
 	struct v4l2_rect *crop[ATOMISP_SUBDEV_PADS_NUM],
 		       *comp[ATOMISP_SUBDEV_PADS_NUM];
-	enum atomisp_input_stream_id stream_id;
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
 
-	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
-
 	isp_get_fmt_rect(sd, sd_state, which, ffmt, crop, comp);
 
 	dev_dbg(isp->dev,
@@ -478,7 +474,8 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 			dvs_w = dvs_h = 0;
 		}
 		atomisp_css_video_set_dis_envelope(isp_sd, dvs_w, dvs_h);
-		atomisp_css_input_set_effective_resolution(isp_sd, stream_id,
+		atomisp_css_input_set_effective_resolution(
+			isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
 			crop[pad]->width, crop[pad]->height);
 
 		break;
@@ -523,14 +520,14 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		if (r->width * crop[ATOMISP_SUBDEV_PAD_SINK]->height <
 		    crop[ATOMISP_SUBDEV_PAD_SINK]->width * r->height)
 			atomisp_css_input_set_effective_resolution(isp_sd,
-				stream_id,
+				ATOMISP_INPUT_STREAM_GENERAL,
 				rounddown(crop[ATOMISP_SUBDEV_PAD_SINK]->
 					  height * r->width / r->height,
 					  ATOM_ISP_STEP_WIDTH),
 				crop[ATOMISP_SUBDEV_PAD_SINK]->height);
 		else
 			atomisp_css_input_set_effective_resolution(isp_sd,
-				stream_id,
+				ATOMISP_INPUT_STREAM_GENERAL,
 				crop[ATOMISP_SUBDEV_PAD_SINK]->width,
 				rounddown(crop[ATOMISP_SUBDEV_PAD_SINK]->
 					  width * r->height / r->width,
@@ -620,16 +617,12 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
 	struct atomisp_device *isp = isp_sd->isp;
 	struct v4l2_mbus_framefmt *__ffmt =
 	    atomisp_subdev_get_ffmt(sd, sd_state, which, pad);
-	u16 vdev_pad = atomisp_subdev_source_pad(sd->devnode);
-	enum atomisp_input_stream_id stream_id;
 
 	dev_dbg(isp->dev, "ffmt: pad %s w %d h %d code 0x%8.8x which %s\n",
 		atomisp_pad_str(pad), ffmt->width, ffmt->height, ffmt->code,
 		which == V4L2_SUBDEV_FORMAT_TRY ? "V4L2_SUBDEV_FORMAT_TRY"
 		: "V4L2_SUBDEV_FORMAT_ACTIVE");
 
-	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
-
 	switch (pad) {
 	case ATOMISP_SUBDEV_PAD_SINK: {
 		const struct atomisp_in_fmt_conv *fc =
@@ -649,15 +642,15 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
 
 		if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 			atomisp_css_input_set_resolution(isp_sd,
-							 stream_id, ffmt);
+							 ATOMISP_INPUT_STREAM_GENERAL, ffmt);
 			atomisp_css_input_set_binning_factor(isp_sd,
-							     stream_id,
+							     ATOMISP_INPUT_STREAM_GENERAL,
 							     atomisp_get_sensor_bin_factor(isp_sd));
-			atomisp_css_input_set_bayer_order(isp_sd, stream_id,
+			atomisp_css_input_set_bayer_order(isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
 							  fc->bayer_order);
-			atomisp_css_input_set_format(isp_sd, stream_id,
+			atomisp_css_input_set_format(isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
 						     fc->atomisp_in_fmt);
-			atomisp_css_set_default_isys_config(isp_sd, stream_id,
+			atomisp_css_set_default_isys_config(isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
 							    ffmt);
 		}
 
-- 
2.37.3

Re: [PATCH 16/17] media: atomisp: Remove atomisp_source_pad_to_stream_id()

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 07:16:52PM +0200, Hans de Goede wrote:
> atomisp_source_pad_to_stream_id() returns ATOMISP_INPUT_STREAM_GENERAL
> unconditionally now. Drop it and directly use ATOMISP_INPUT_STREAM_GENERAL
> in its callers.

...

> -				atomisp_css_capture_enable_online(asd, stream_index, false);
> +				atomisp_css_capture_enable_online(
> +					asd, ATOMISP_INPUT_STREAM_GENERAL, false);

asd can be left on the same line. no?


...

> +		ret = atomisp_css_copy_get_output_frame_info(
> +			asd, ATOMISP_INPUT_STREAM_GENERAL, output_info);

Ditto.

...

> -		atomisp_css_input_set_effective_resolution(isp_sd, stream_id,
> +		atomisp_css_input_set_effective_resolution(
> +			isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
>  			crop[pad]->width, crop[pad]->height);

In the similar way...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 16/17] media: atomisp: Remove atomisp_source_pad_to_stream_id()

[Hans de Goede]

Hi,

On 9/12/22 13:43, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 07:16:52PM +0200, Hans de Goede wrote:
>> atomisp_source_pad_to_stream_id() returns ATOMISP_INPUT_STREAM_GENERAL
>> unconditionally now. Drop it and directly use ATOMISP_INPUT_STREAM_GENERAL
>> in its callers.
> 
> ...
> 
>> -				atomisp_css_capture_enable_online(asd, stream_index, false);
>> +				atomisp_css_capture_enable_online(
>> +					asd, ATOMISP_INPUT_STREAM_GENERAL, false);
> 
> asd can be left on the same line. no?

Right, I changed this to make checkpatch happy about the arguments not being aligned
the same way as the first argument. But then it complained about a '(' at the end of
the line ... so this was not really better (nor worse).

I have changed this to:

				atomisp_css_capture_enable_online(asd,
								  AOMISP_INPUT_STREAM_GENERAL,
								  false);

now which does make checkpatch fully happy. I will change other 2 blocks in the same way.

Regards,

Hans


> 
> 
> ...
> 
>> +		ret = atomisp_css_copy_get_output_frame_info(
>> +			asd, ATOMISP_INPUT_STREAM_GENERAL, output_info);
> 
> Ditto.
> 
> ...
> 
>> -		atomisp_css_input_set_effective_resolution(isp_sd, stream_id,
>> +		atomisp_css_input_set_effective_resolution(
>> +			isp_sd, ATOMISP_INPUT_STREAM_GENERAL,
>>  			crop[pad]->width, crop[pad]->height);
> 
> In the similar way...
> 

[PATCH 17/17] media: atomisp_gmin_platform: Unexport and split camera_sensor_csi()

[Hans de Goede]

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

The camera_sensor_csi() is not used outside the module, hence make it
static. While at it, split it to _alloc() and _free() to clearly show
the idea behind the last parameter @flag that is passed to
gmin_csi_cfg().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Link: https://lore.kernel.org/r/20220730162027.1011-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>
---
 .../include/linux/atomisp_gmin_platform.h     |  2 -
 .../media/atomisp/pci/atomisp_gmin_platform.c | 68 ++++++++++---------
 2 files changed, 37 insertions(+), 33 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
index 58e0ea5355a3..5463d11d4295 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_gmin_platform.h
@@ -26,8 +26,6 @@ struct v4l2_subdev *atomisp_gmin_find_subdev(struct i2c_adapter *adapter,
 int atomisp_gmin_remove_subdev(struct v4l2_subdev *sd);
 int gmin_get_var_int(struct device *dev, bool is_gmin,
 		     const char *var, int def);
-int camera_sensor_csi(struct v4l2_subdev *sd, u32 port,
-		      u32 lanes, u32 format, u32 bayer_order, int flag);
 struct camera_sensor_platform_data *
 gmin_camera_platform_data(
     struct v4l2_subdev *subdev,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
index 254e8c97f71f..3d41fab661cf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c
@@ -1048,6 +1048,38 @@ static int gmin_flisclk_ctrl(struct v4l2_subdev *subdev, int on)
 	return ret;
 }
 
+static int camera_sensor_csi_alloc(struct v4l2_subdev *sd, u32 port, u32 lanes,
+				   u32 format, u32 bayer_order)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct camera_mipi_info *csi;
+
+	csi = kzalloc(sizeof(*csi), GFP_KERNEL);
+	if (!csi)
+		return -ENOMEM;
+
+	csi->port = port;
+	csi->num_lanes = lanes;
+	csi->input_format = format;
+	csi->raw_bayer_order = bayer_order;
+	v4l2_set_subdev_hostdata(sd, csi);
+	csi->metadata_format = ATOMISP_INPUT_FORMAT_EMBEDDED;
+	csi->metadata_effective_width = NULL;
+	dev_info(&client->dev,
+		 "camera pdata: port: %d lanes: %d order: %8.8x\n",
+		 port, lanes, bayer_order);
+
+	return 0;
+}
+
+static void camera_sensor_csi_free(struct v4l2_subdev *sd)
+{
+	struct camera_mipi_info *csi;
+
+	csi = v4l2_get_subdev_hostdata(sd);
+	kfree(csi);
+}
+
 static int gmin_csi_cfg(struct v4l2_subdev *sd, int flag)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
@@ -1056,8 +1088,11 @@ static int gmin_csi_cfg(struct v4l2_subdev *sd, int flag)
 	if (!client || !gs)
 		return -ENODEV;
 
-	return camera_sensor_csi(sd, gs->csi_port, gs->csi_lanes,
-				 gs->csi_fmt, gs->csi_bayer, flag);
+	if (flag)
+		return camera_sensor_csi_alloc(sd, gs->csi_port, gs->csi_lanes,
+					       gs->csi_fmt, gs->csi_bayer);
+	camera_sensor_csi_free(sd);
+	return 0;
 }
 
 static struct camera_vcm_control *gmin_get_vcm_ctrl(struct v4l2_subdev *subdev,
@@ -1340,35 +1375,6 @@ int gmin_get_var_int(struct device *dev, bool is_gmin, const char *var, int def)
 }
 EXPORT_SYMBOL_GPL(gmin_get_var_int);
 
-int camera_sensor_csi(struct v4l2_subdev *sd, u32 port,
-		      u32 lanes, u32 format, u32 bayer_order, int flag)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
-	struct camera_mipi_info *csi = NULL;
-
-	if (flag) {
-		csi = kzalloc(sizeof(*csi), GFP_KERNEL);
-		if (!csi)
-			return -ENOMEM;
-		csi->port = port;
-		csi->num_lanes = lanes;
-		csi->input_format = format;
-		csi->raw_bayer_order = bayer_order;
-		v4l2_set_subdev_hostdata(sd, (void *)csi);
-		csi->metadata_format = ATOMISP_INPUT_FORMAT_EMBEDDED;
-		csi->metadata_effective_width = NULL;
-		dev_info(&client->dev,
-			 "camera pdata: port: %d lanes: %d order: %8.8x\n",
-			 port, lanes, bayer_order);
-	} else {
-		csi = v4l2_get_subdev_hostdata(sd);
-		kfree(csi);
-	}
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(camera_sensor_csi);
-
 /* PCI quirk: The BYT ISP advertises PCI runtime PM but it doesn't
  * work.  Disable so the kernel framework doesn't hang the device
  * trying.  The driver itself does direct calls to the PUNIT to manage
-- 
2.37.3

Re: [PATCH 00/17] media: atomisp: further cleanups / unwanted code removal

[Andy Shevchenko]

On Sun, Sep 11, 2022 at 07:16:36PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is another atomisp patch-series with further cleanups / unwanted code
> removal. Note this mostly cleaning up things which I hit while I'm still
> working towards converting the driver to videobuf2.
> 
> The main goal of this series was to make the atomisp locking more like
> that of other v4l2 drivers, which is acomplished in the
> "media: atomisp: Use video_dev.lock for ioctl locking" patch.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
for non-commented/non-authored-by-me patches.

> Andy Shevchenko (1):
>   media: atomisp_gmin_platform: Unexport and split camera_sensor_csi()
> 
> Hans de Goede (16):
>   media: atomisp: Use a normal mutex for the main lock
>   media: atomisp: Remove unused lock member from struct
>     atomisp_sub_device
>   media: atomisp: Fix locking around asd->streaming read/write
>   media: atomisp: Remove asd == NULL checks from ioctl handling
>   media: atomisp: Add atomisp_pipe_check() helper
>   media: atomisp: Remove watchdog timer
>   media: atomisp: Move atomisp_streaming_count() check into
>     __atomisp_css_recover()
>   media: atomisp: Rework asd->streaming state update in
>     __atomisp_streamoff()
>   media: atomisp: Drop streamoff_mutex
>   media: atomisp: Use video_dev.lock for ioctl locking
>   media: atomisp: Remove a couple of not useful function wrappers
>   media: atomisp: Drop unnecessary first_streamoff check
>   media: atomisp: Make atomisp_set_raw_buffer_bitmap() static
>   media: atomisp: Remove unused atomisp_css_get_dis_statistics()
>   media: atomisp: Remove const/fixed camera_caps
>   media: atomisp: Remove atomisp_source_pad_to_stream_id()
> 
>  .../include/linux/atomisp_gmin_platform.h     |   2 -
>  .../atomisp/include/linux/atomisp_platform.h  |  18 -
>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 581 ++----------------
>  .../staging/media/atomisp/pci/atomisp_cmd.h   |   9 +-
>  .../media/atomisp/pci/atomisp_compat.h        |   4 -
>  .../media/atomisp/pci/atomisp_compat_css20.c  |  74 +--
>  .../staging/media/atomisp/pci/atomisp_fops.c  |  87 +--
>  .../media/atomisp/pci/atomisp_gmin_platform.c |  86 ++-
>  .../media/atomisp/pci/atomisp_internal.h      |  39 +-
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 567 ++++-------------
>  .../staging/media/atomisp/pci/atomisp_ioctl.h |  10 +-
>  .../media/atomisp/pci/atomisp_subdev.c        |  34 +-
>  .../media/atomisp/pci/atomisp_subdev.h        |  24 +-
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |  65 +-
>  14 files changed, 267 insertions(+), 1333 deletions(-)
> 
> -- 
> 2.37.3
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/17] media: atomisp: further cleanups / unwanted code removal

[Hans de Goede]

Hi,

On 9/12/22 13:44, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 07:16:36PM +0200, Hans de Goede wrote:
>> Hi All,
>>
>> Here is another atomisp patch-series with further cleanups / unwanted code
>> removal. Note this mostly cleaning up things which I hit while I'm still
>> working towards converting the driver to videobuf2.
>>
>> The main goal of this series was to make the atomisp locking more like
>> that of other v4l2 drivers, which is acomplished in the
>> "media: atomisp: Use video_dev.lock for ioctl locking" patch.
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> for non-commented/non-authored-by-me patches.

Thank you for all the reviews.

I'm currently preparing a pull-req for atomisp for Mauro. You can find
this series here now:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp

For the indentation fixes you asked about for 16/17, see:
https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/commit/?h=media-atomisp&id=f9c9e7db01caaa1c1fa5a18d1505676c1bdcaeb0

Note I'm going to do a test-build + some quick local tests for this
and then I'll put a signed-tag on it and send out a pull-req.

So if you are still going to give your Reviewed-by for patch 1 + 16
it will likely be too late for me to process the tags, sorry.

Regards,

Hans


> 
>> Andy Shevchenko (1):
>>   media: atomisp_gmin_platform: Unexport and split camera_sensor_csi()
>>
>> Hans de Goede (16):
>>   media: atomisp: Use a normal mutex for the main lock
>>   media: atomisp: Remove unused lock member from struct
>>     atomisp_sub_device
>>   media: atomisp: Fix locking around asd->streaming read/write
>>   media: atomisp: Remove asd == NULL checks from ioctl handling
>>   media: atomisp: Add atomisp_pipe_check() helper
>>   media: atomisp: Remove watchdog timer
>>   media: atomisp: Move atomisp_streaming_count() check into
>>     __atomisp_css_recover()
>>   media: atomisp: Rework asd->streaming state update in
>>     __atomisp_streamoff()
>>   media: atomisp: Drop streamoff_mutex
>>   media: atomisp: Use video_dev.lock for ioctl locking
>>   media: atomisp: Remove a couple of not useful function wrappers
>>   media: atomisp: Drop unnecessary first_streamoff check
>>   media: atomisp: Make atomisp_set_raw_buffer_bitmap() static
>>   media: atomisp: Remove unused atomisp_css_get_dis_statistics()
>>   media: atomisp: Remove const/fixed camera_caps
>>   media: atomisp: Remove atomisp_source_pad_to_stream_id()
>>
>>  .../include/linux/atomisp_gmin_platform.h     |   2 -
>>  .../atomisp/include/linux/atomisp_platform.h  |  18 -
>>  .../staging/media/atomisp/pci/atomisp_cmd.c   | 581 ++----------------
>>  .../staging/media/atomisp/pci/atomisp_cmd.h   |   9 +-
>>  .../media/atomisp/pci/atomisp_compat.h        |   4 -
>>  .../media/atomisp/pci/atomisp_compat_css20.c  |  74 +--
>>  .../staging/media/atomisp/pci/atomisp_fops.c  |  87 +--
>>  .../media/atomisp/pci/atomisp_gmin_platform.c |  86 ++-
>>  .../media/atomisp/pci/atomisp_internal.h      |  39 +-
>>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 567 ++++-------------
>>  .../staging/media/atomisp/pci/atomisp_ioctl.h |  10 +-
>>  .../media/atomisp/pci/atomisp_subdev.c        |  34 +-
>>  .../media/atomisp/pci/atomisp_subdev.h        |  24 +-
>>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |  65 +-
>>  14 files changed, 267 insertions(+), 1333 deletions(-)
>>
>> -- 
>> 2.37.3
>>
>