diff mbox series

[05/30] media: atomisp: Replace source-pad checks with run-mode checks

Message ID 20230513123159.33234-6-hdegoede@redhat.com (mailing list archive)
State Accepted
Headers
Series media: atomisp: Register only 1 /dev/video# node + cleanups |

Commit Message

Hans de Goede May 13, 2023, 12:31 p.m. UTC 
Currently atomisp behavior is determined by a mix of which /dev/video# node
(which isp-subdev source-pad) is opened + which run-mode is set.
With various combinations not being allowed and likely leading to crashes
due to lack of error checking.

Now that we no longer support continuous mode and thus no longer support
streaming from 2 /dev/video# nodes at the same time, there is no need
to have a separate /dev/video# node for each run-mode. Instead the plan is
to support the 3 different run-modes on a single /dev/video# node.
Since we are moving to a single isp-subdev source-pad, the behavior should
then be solely and consistently be defined by the run-mode.

Replace various source-pad checks with run-mode checks in preparation for
moving to a single source-pad. In some places the new run-mode checks
overlap with existing run-mode checks and the checks are folded together
into a single check.

This removes handling of the ATOMISP_SUBDEV_PAD_SOURCE_VF source-pad,
this source-pad was only useful for continuous mode, for which support has
been removed.

Note that currently the only run-mode which we actually have been able to
get to work is the video-capture with scaler aka preview mode and as such
that is also the only run-mode tested. This patch is intended to preserve
the current (known to not work 100%) behavior of the other run-modes, so
that those maybe can be enabled later.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/atomisp_cmd.c   | 37 +++-------
 .../media/atomisp/pci/atomisp_compat_css20.c  | 72 ++++---------------
 .../media/atomisp/pci/atomisp_subdev.c        | 23 ++----
 3 files changed, 29 insertions(+), 103 deletions(-)

Comments

Andy Shevchenko May 13, 2023, 8:07 p.m. UTC | #1 
On Sat, May 13, 2023 at 3:32 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Currently atomisp behavior is determined by a mix of which /dev/video# node
> (which isp-subdev source-pad) is opened + which run-mode is set.
> With various combinations not being allowed and likely leading to crashes
> due to lack of error checking.
>
> Now that we no longer support continuous mode and thus no longer support
> streaming from 2 /dev/video# nodes at the same time, there is no need
> to have a separate /dev/video# node for each run-mode. Instead the plan is
> to support the 3 different run-modes on a single /dev/video# node.
> Since we are moving to a single isp-subdev source-pad, the behavior should
> then be solely and consistently be defined by the run-mode.
>
> Replace various source-pad checks with run-mode checks in preparation for
> moving to a single source-pad. In some places the new run-mode checks
> overlap with existing run-mode checks and the checks are folded together
> into a single check.
>
> This removes handling of the ATOMISP_SUBDEV_PAD_SOURCE_VF source-pad,
> this source-pad was only useful for continuous mode, for which support has
> been removed.
>
> Note that currently the only run-mode which we actually have been able to
> get to work is the video-capture with scaler aka preview mode and as such
> that is also the only run-mode tested. This patch is intended to preserve
> the current (known to not work 100%) behavior of the other run-modes, so
> that those maybe can be enabled later.

...

> @@ -5005,16 +4993,11 @@ static int atomisp_get_pipe_id(struct atomisp_video_pipe *pipe)

It seems to me that all 'else':s in this function are redundant.
At least you can probably drop them either here or in a separate change.

>                 return IA_CSS_PIPE_ID_VIDEO;
>         } else if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
>                 return IA_CSS_PIPE_ID_CAPTURE;
> -       } else if (pipe == &asd->video_out_video_capture) {
> +       } else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
>                 return IA_CSS_PIPE_ID_VIDEO;
> -       } else if (pipe == &asd->video_out_vf) {
> -               return IA_CSS_PIPE_ID_CAPTURE;
> -       } else if (pipe == &asd->video_out_preview) {
> -               if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO)
> -                       return IA_CSS_PIPE_ID_VIDEO;
> -               else
> -                       return IA_CSS_PIPE_ID_PREVIEW;
> -       } else if (pipe == &asd->video_out_capture) {
> +       } else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
> +               return IA_CSS_PIPE_ID_PREVIEW;
> +       } else if (asd->run_mode->val == ATOMISP_RUN_MODE_STILL_CAPTURE) {
>                 if (asd->copy_mode)
>                         return IA_CSS_PIPE_ID_COPY;
>                 else
diff mbox series

Patch

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index ed1a534b7c05..710e97a492c6 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -4215,21 +4215,12 @@  static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 					ATOMISP_SUBDEV_PAD_SOURCE_VF, &vf_ffmt);
 		asd->video_out_vf.sh_fmt = IA_CSS_FRAME_FORMAT_NV12;
 
-		if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
+		if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO ||
+		    asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER) {
 			atomisp_css_video_configure_viewfinder(asd,
 							       vf_size.width, vf_size.height, 0,
 							       asd->video_out_vf.sh_fmt);
-		} else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
-			if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW ||
-			    source_pad == ATOMISP_SUBDEV_PAD_SOURCE_VIDEO)
-				atomisp_css_video_configure_viewfinder(asd,
-								       vf_size.width, vf_size.height, 0,
-								       asd->video_out_vf.sh_fmt);
-			else
-				atomisp_css_capture_configure_viewfinder(asd,
-					vf_size.width, vf_size.height, 0,
-					asd->video_out_vf.sh_fmt);
-		} else if (source_pad != ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW ||
+		} else if (asd->run_mode->val == ATOMISP_RUN_MODE_STILL_CAPTURE ||
 			   asd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
 			atomisp_css_capture_configure_viewfinder(asd,
 				vf_size.width, vf_size.height, 0,
@@ -4255,7 +4246,7 @@  static int atomisp_set_fmt_to_isp(struct video_device *vdev,
 		configure_output = atomisp_css_video_configure_output;
 		get_frame_info = atomisp_css_video_get_output_frame_info;
 		pipe_id = IA_CSS_PIPE_ID_VIDEO;
-	} else if (source_pad == ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW) {
+	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
 		configure_output = atomisp_css_preview_configure_output;
 		get_frame_info = atomisp_css_preview_get_output_frame_info;
 		configure_pp_input = atomisp_css_preview_configure_pp_input;
@@ -4386,7 +4377,6 @@  static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	struct atomisp_device *isp;
 	struct atomisp_input_stream_info *stream_info =
 	    (struct atomisp_input_stream_info *)ffmt->reserved;
-	int source_pad = atomisp_subdev_source_pad(vdev);
 	struct v4l2_subdev_fh fh;
 	int ret;
 
@@ -4417,8 +4407,7 @@  static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	req_ffmt = ffmt;
 
 	/* Disable dvs if resolution can't be supported by sensor */
-	if (asd->params.video_dis_en &&
-	    source_pad == ATOMISP_SUBDEV_PAD_SOURCE_VIDEO) {
+	if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
 		vformat.which = V4L2_SUBDEV_FORMAT_TRY;
 		ret = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
 				       pad, set_fmt, &pad_state, &vformat);
@@ -4453,8 +4442,7 @@  static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 	    ffmt->height < ATOM_ISP_STEP_HEIGHT)
 		return -EINVAL;
 
-	if (asd->params.video_dis_en &&
-	    source_pad == ATOMISP_SUBDEV_PAD_SOURCE_VIDEO &&
+	if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO &&
 	    (ffmt->width < req_ffmt->width || ffmt->height < req_ffmt->height)) {
 		dev_warn(isp->dev,
 			 "can not enable video dis due to sensor limitation.");
@@ -5005,16 +4993,11 @@  static int atomisp_get_pipe_id(struct atomisp_video_pipe *pipe)
 		return IA_CSS_PIPE_ID_VIDEO;
 	} else if (asd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
 		return IA_CSS_PIPE_ID_CAPTURE;
-	} else if (pipe == &asd->video_out_video_capture) {
+	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
 		return IA_CSS_PIPE_ID_VIDEO;
-	} else if (pipe == &asd->video_out_vf) {
-		return IA_CSS_PIPE_ID_CAPTURE;
-	} else if (pipe == &asd->video_out_preview) {
-		if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO)
-			return IA_CSS_PIPE_ID_VIDEO;
-		else
-			return IA_CSS_PIPE_ID_PREVIEW;
-	} else if (pipe == &asd->video_out_capture) {
+	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_PREVIEW) {
+		return IA_CSS_PIPE_ID_PREVIEW;
+	} else if (asd->run_mode->val == ATOMISP_RUN_MODE_STILL_CAPTURE) {
 		if (asd->copy_mode)
 			return IA_CSS_PIPE_ID_COPY;
 		else
diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
index e8c26e66ae7e..dfed7f1fbf6b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
@@ -2416,41 +2416,21 @@  static int __get_frame_info(struct atomisp_sub_device *asd,
 	return -EINVAL;
 }
 
-static unsigned int atomisp_get_pipe_index(struct atomisp_sub_device *asd,
-	uint16_t source_pad)
+static unsigned int atomisp_get_pipe_index(struct atomisp_sub_device *asd)
 {
-	struct atomisp_device *isp = asd->isp;
-
-	switch (source_pad) {
-	case ATOMISP_SUBDEV_PAD_SOURCE_VIDEO:
-		if (asd->copy_mode)
-			return IA_CSS_PIPE_ID_COPY;
-		if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO
-		    || asd->vfpp->val == ATOMISP_VFPP_DISABLE_SCALER)
-			return IA_CSS_PIPE_ID_VIDEO;
+	if (asd->copy_mode)
+		return IA_CSS_PIPE_ID_COPY;
 
+	switch (asd->run_mode->val) {
+	case ATOMISP_RUN_MODE_VIDEO:
+		return IA_CSS_PIPE_ID_VIDEO;
+	case ATOMISP_RUN_MODE_STILL_CAPTURE:
 		return IA_CSS_PIPE_ID_CAPTURE;
-	case ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE:
-		if (asd->copy_mode)
-			return IA_CSS_PIPE_ID_COPY;
-
-		return IA_CSS_PIPE_ID_CAPTURE;
-	case ATOMISP_SUBDEV_PAD_SOURCE_VF:
-		if (!atomisp_is_mbuscode_raw(asd->fmt[asd->capture_pad].fmt.code)) {
-			return IA_CSS_PIPE_ID_CAPTURE;
-		}
-		fallthrough;
-	case ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW:
-		if (asd->copy_mode)
-			return IA_CSS_PIPE_ID_COPY;
-		if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO)
-			return IA_CSS_PIPE_ID_VIDEO;
-
+	case ATOMISP_RUN_MODE_PREVIEW:
 		return IA_CSS_PIPE_ID_PREVIEW;
 	}
-	dev_warn(isp->dev,
-		 "invalid source pad:%d, return default preview pipe index.\n",
-		 source_pad);
+
+	dev_warn(asd->isp->dev, "cannot determine pipe-index return default preview pipe\n");
 	return IA_CSS_PIPE_ID_PREVIEW;
 }
 
@@ -2459,7 +2439,7 @@  int atomisp_get_css_frame_info(struct atomisp_sub_device *asd,
 			       struct ia_css_frame_info *frame_info)
 {
 	struct ia_css_pipe_info info;
-	int pipe_index = atomisp_get_pipe_index(asd, source_pad);
+	int pipe_index = atomisp_get_pipe_index(asd);
 	int stream_index;
 	struct atomisp_device *isp = asd->isp;
 
@@ -2473,34 +2453,8 @@  int atomisp_get_css_frame_info(struct atomisp_sub_device *asd,
 		return -EINVAL;
 	}
 
-	switch (source_pad) {
-	case ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE:
-		*frame_info = info.output_info[0];
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_VIDEO:
-		*frame_info = info.output_info[ATOMISP_CSS_OUTPUT_DEFAULT_INDEX];
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_VF:
-		if (stream_index == ATOMISP_INPUT_STREAM_POSTVIEW)
-			*frame_info = info.output_info[0];
-		else
-			*frame_info = info.vf_output_info[0];
-		break;
-	case ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW:
-		if (asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO &&
-		    (pipe_index == IA_CSS_PIPE_ID_VIDEO ||
-		     pipe_index == IA_CSS_PIPE_ID_YUVPP))
-			*frame_info = info.vf_output_info[ATOMISP_CSS_OUTPUT_DEFAULT_INDEX];
-		else
-			*frame_info =
-			    info.output_info[ATOMISP_CSS_OUTPUT_DEFAULT_INDEX];
-
-		break;
-	default:
-		frame_info = NULL;
-		break;
-	}
-	return frame_info ? 0 : -EINVAL;
+	*frame_info = info.output_info[0];
+	return 0;
 }
 
 int atomisp_css_copy_configure_output(struct atomisp_sub_device *asd,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index cb8ab0e47d1a..ae57aef377b4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -393,8 +393,7 @@  int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	r->width = rounddown(r->width, ATOM_ISP_STEP_WIDTH);
 	r->height = rounddown(r->height, ATOM_ISP_STEP_HEIGHT);
 
-	switch (pad) {
-	case ATOMISP_SUBDEV_PAD_SINK: {
+	if (pad == ATOMISP_SUBDEV_PAD_SINK) {
 		/* Only crop target supported on sink pad. */
 		unsigned int dvs_w, dvs_h;
 
@@ -445,7 +444,7 @@  int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		}
 
 		if (which == V4L2_SUBDEV_FORMAT_TRY)
-			break;
+			goto get_rect;
 
 		if (isp_sd->params.video_dis_en &&
 		    isp_sd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
@@ -468,12 +467,8 @@  int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 							   ATOMISP_INPUT_STREAM_GENERAL,
 							   crop[pad]->width,
 							   crop[pad]->height);
-		break;
-	}
-	case ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE:
-	case ATOMISP_SUBDEV_PAD_SOURCE_VIDEO: {
+	} else if (isp_sd->run_mode->val != ATOMISP_RUN_MODE_PREVIEW) {
 		/* Only compose target is supported on source pads. */
-
 		if (isp_sd->vfpp->val == ATOMISP_VFPP_DISABLE_LOWLAT) {
 			/* Scaling is disabled in this mode */
 			r->width = crop[ATOMISP_SUBDEV_PAD_SINK]->width;
@@ -492,7 +487,7 @@  int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		if (r->width == 0 || r->height == 0 ||
 		    crop[ATOMISP_SUBDEV_PAD_SINK]->width == 0 ||
 		    crop[ATOMISP_SUBDEV_PAD_SINK]->height == 0)
-			break;
+			goto get_rect;
 		/*
 		 * do cropping on sensor input if ratio of required resolution
 		 * is different with sensor output resolution ratio:
@@ -522,18 +517,12 @@  int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 				rounddown(crop[ATOMISP_SUBDEV_PAD_SINK]->
 					  width * r->height / r->width,
 					  ATOM_ISP_STEP_WIDTH));
-
-		break;
-	}
-	case ATOMISP_SUBDEV_PAD_SOURCE_VF:
-	case ATOMISP_SUBDEV_PAD_SOURCE_PREVIEW:
+	} else {
 		comp[pad]->width = r->width;
 		comp[pad]->height = r->height;
-		break;
-	default:
-		return -EINVAL;
 	}
 
+get_rect:
 	/* Set format dimensions on non-sink pads as well. */
 	if (pad != ATOMISP_SUBDEV_PAD_SINK) {
 		ffmt[pad]->width = comp[pad]->width;