Re: [PATCH v5 0/7] Add AMD ISP4 driver

[Sultan Alsawaf <sultan@kerneltoast.com>]

[-- Attachment #1: Type: text/plain, Size: 1866 bytes --]

Hi Bin,

To expedite review, I've attached a patch containing a bunch of fixes I've made
on top of v5. Most of my changes should be self-explanatory; feel free to ask
further about specific changes if you have any questions.

I haven't had a chance to review all of the v4 -> v5 changes yet, but I figured
I should send what I've got so far.

FYI, there is a regression in isp4if_dequeue_buffer() where the bufq lock isn't
protecting the list_del() anymore. I also checked the compiler output when using
guard() versus scoped_guard() in that function and there is no difference:

  sha1sum:
  5652a0306da22ea741d80a9e03a787d0f71758a8  isp4_interface.o // guard()
  5652a0306da22ea741d80a9e03a787d0f71758a8  isp4_interface.o // scoped_guard()

So guard() should be used there again, which I've done in my patch.

I also have a few questions:

1. Does ISP FW provide a register interface to disable the IRQ? If so, it is
   faster to use that than using disable_irq_nosync() to disable the IRQ from
   the CPU's side.

2. When the IRQ is re-enabled in isp4sd_fw_resp_func(), is there anything
   beforehand to mask all pending interrupts from the ISP so that there isn't a
   bunch of stale interrupts firing as soon the IRQ is re-enabled?

3. Is there any risk of deadlock due to the stream kthread racing with the ISP
   when the ISP posts a new response _after_ the kthread determines there are no
   more new responses but _before_ the enable_irq() in isp4sd_fw_resp_func()?

4. Why are some lines much longer than before? It seems inconsistent that now
   there is a mix of several lines wrapped to 80 cols and many lines going
   beyond 80 cols. And there are multiple places where code is wrapped before
   reaching 80 cols even with lots of room left, specifically for cases where it
   wouldn't hurt readability to put more characters onto each line.

Sultan

[-- Attachment #2: amd-isp4-v5-fixes-round-1.patch --]
[-- Type: text/plain, Size: 22370 bytes --]

From 83fd6cffdc34204495044c28f384488c3a8ae08b Mon Sep 17 00:00:00 2001
From: Sultan Alsawaf <sultan@kerneltoast.com>
Date: Wed, 5 Nov 2025 00:57:55 -0800
Subject: [PATCH] amd isp4 v5 fixes round 1

---
 drivers/media/platform/amd/isp4/isp4.c        |   5 +-
 drivers/media/platform/amd/isp4/isp4_debug.c  |   6 +-
 .../media/platform/amd/isp4/isp4_interface.c  |  33 ++--
 .../media/platform/amd/isp4/isp4_interface.h  |   6 +-
 drivers/media/platform/amd/isp4/isp4_subdev.c | 168 +++++++-----------
 drivers/media/platform/amd/isp4/isp4_video.c  |  26 +--
 drivers/media/platform/amd/isp4/isp4_video.h  |  23 +--
 7 files changed, 96 insertions(+), 171 deletions(-)

diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/platform/amd/isp4/isp4.c
index 78a7a998d86e..d136da830a2f 100644
--- a/drivers/media/platform/amd/isp4/isp4.c
+++ b/drivers/media/platform/amd/isp4/isp4.c
@@ -115,9 +115,8 @@ static int isp4_capture_probe(struct platform_device *pdev)
 			return dev_err_probe(dev, irq[i], "fail to get irq %d\n",
 					     isp4_ringbuf_interrupt_num[i]);
 
-		irq_set_status_flags(irq[i], IRQ_NOAUTOEN);
-		ret = devm_request_irq(dev, irq[i], isp4_irq_handler, 0, isp4_irq_name[i],
-				       isp_dev);
+		ret = devm_request_irq(dev, irq[i], isp4_irq_handler,
+				       IRQF_NO_AUTOEN, isp4_irq_name[i], isp_dev);
 		if (ret)
 			return dev_err_probe(dev, ret, "fail to req irq %d\n", irq[i]);
 	}
diff --git a/drivers/media/platform/amd/isp4/isp4_debug.c b/drivers/media/platform/amd/isp4/isp4_debug.c
index 746a92707e54..86f34ada78ac 100644
--- a/drivers/media/platform/amd/isp4/isp4_debug.c
+++ b/drivers/media/platform/amd/isp4/isp4_debug.c
@@ -56,11 +56,11 @@ static u32 isp_fw_fill_rb_log(struct isp4_subdev *isp, u8 *sys, u32 rb_size)
 		else if (wr_ptr < rd_ptr)
 			cnt = rb_size - rd_ptr;
 		else
-			goto unlock_and_quit;
+			goto quit;
 
 		if (cnt > rb_size) {
 			dev_err(dev, "fail bad fw log size %u\n", cnt);
-			goto unlock_and_quit;
+			goto quit;
 		}
 
 		memcpy(buf + offset, (u8 *)(sys + rd_ptr), cnt);
@@ -72,7 +72,7 @@ static u32 isp_fw_fill_rb_log(struct isp4_subdev *isp, u8 *sys, u32 rb_size)
 
 	isp4hw_wreg(ISP4_GET_ISP_REG_BASE(isp), ISP_LOG_RB_RPTR0, rd_ptr);
 
-unlock_and_quit:
+quit:
 	return total_cnt;
 }
 
diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c
index 1852bd56a947..b92df92f8827 100644
--- a/drivers/media/platform/amd/isp4/isp4_interface.c
+++ b/drivers/media/platform/amd/isp4/isp4_interface.c
@@ -16,6 +16,8 @@
 	 ISP_SYS_INT0_EN__SYS_INT_RINGBUFFER_WPT11_EN_MASK | \
 	 ISP_SYS_INT0_EN__SYS_INT_RINGBUFFER_WPT12_EN_MASK)
 
+#define ISP4IF_FW_CMD_TIMEOUT (HZ / 2)
+
 struct isp4if_rb_config {
 	const char *name;
 	u32 index;
@@ -281,10 +283,9 @@ struct isp4if_cmd_element *isp4if_rm_cmd_from_cmdq(struct isp4_interface *ispif,
 						   u32 cmd_id)
 {
 	struct isp4if_cmd_element *buf_node;
-	struct isp4if_cmd_element *tmp_node;
 
 	scoped_guard(spinlock, &ispif->cmdq_lock)
-		list_for_each_entry_safe(buf_node, tmp_node, &ispif->cmdq, list) {
+		list_for_each_entry(buf_node, &ispif->cmdq, list) {
 			if (buf_node->seq_num == seq_num &&
 			    buf_node->cmd_id == cmd_id) {
 				list_del(&buf_node->list);
@@ -372,14 +373,14 @@ static inline enum isp4if_stream_id isp4if_get_fw_stream(u32 cmd_id)
 	return ISP4IF_STREAM_ID_1;
 }
 
-static int isp4if_send_fw_cmd(struct isp4_interface *ispif, u32 cmd_id, void *package,
+static int isp4if_send_fw_cmd(struct isp4_interface *ispif, u32 cmd_id, const void *package,
 			      u32 package_size, struct completion *cmd_complete, u32 *seq)
 {
 	enum isp4if_stream_id stream = isp4if_get_fw_stream(cmd_id);
 	struct isp4if_cmd_element *cmd_ele = NULL;
 	struct isp4if_rb_config *rb_config;
 	struct device *dev = ispif->dev;
-	struct isp4fw_cmd cmd = {};
+	struct isp4fw_cmd cmd;
 	u32 seq_num;
 	u32 rreg;
 	u32 wreg;
@@ -393,7 +394,7 @@ static int isp4if_send_fw_cmd(struct isp4_interface *ispif, u32 cmd_id, void *pa
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&cmd, 0, sizeof(cmd));
 	rb_config = &isp4if_resp_rb_config[stream];
@@ -477,7 +478,7 @@ static int isp4if_send_buffer(struct isp4_interface *ispif, struct isp4if_img_bu
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&cmd, 0, sizeof(cmd));
 	cmd.buffer_type = BUFFER_TYPE_PREVIEW;
@@ -778,13 +779,13 @@ int isp4if_f2h_resp(struct isp4_interface *ispif, enum isp4if_stream_id stream,
 	return 0;
 }
 
-int isp4if_send_command(struct isp4_interface *ispif, u32 cmd_id, void *package, u32 package_size)
+int isp4if_send_command(struct isp4_interface *ispif, u32 cmd_id, const void *package, u32 package_size)
 {
 	return isp4if_send_fw_cmd(ispif, cmd_id, package, package_size, NULL, NULL);
 }
 
-int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, void *package,
-			     u32 package_size, u32 timeout)
+int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, const void *package,
+			     u32 package_size)
 {
 	DECLARE_COMPLETION_ONSTACK(cmd_completion);
 	struct device *dev = ispif->dev;
@@ -798,12 +799,9 @@ int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, void *pac
 		return ret;
 	}
 
-	ret = wait_for_completion_timeout(&cmd_completion, msecs_to_jiffies(timeout));
-	if (ret == 0) {
-		struct isp4if_cmd_element *ele;
-
-		ele = isp4if_rm_cmd_from_cmdq(ispif, seq, cmd_id);
-		kfree(ele);
+	ret = wait_for_completion_timeout(&cmd_completion, ISP4IF_FW_CMD_TIMEOUT);
+	if (!ret) {
+		kfree(isp4if_rm_cmd_from_cmdq(ispif, seq, cmd_id));
 		return -ETIMEDOUT;
 	}
 
@@ -842,8 +840,9 @@ struct isp4if_img_buf_node *isp4if_dequeue_buffer(struct isp4_interface *ispif)
 {
 	struct isp4if_img_buf_node *buf_node;
 
-	scoped_guard(spinlock, &ispif->bufq_lock)
-		buf_node = list_first_entry_or_null(&ispif->bufq, typeof(*buf_node), node);
+	guard(spinlock)(&ispif->bufq_lock);
+
+	buf_node = list_first_entry_or_null(&ispif->bufq, typeof(*buf_node), node);
 	if (buf_node)
 		list_del(&buf_node->node);
 
diff --git a/drivers/media/platform/amd/isp4/isp4_interface.h b/drivers/media/platform/amd/isp4/isp4_interface.h
index 688a4ea84dc6..1f589c31dc43 100644
--- a/drivers/media/platform/amd/isp4/isp4_interface.h
+++ b/drivers/media/platform/amd/isp4/isp4_interface.h
@@ -115,11 +115,11 @@ static inline u64 isp4if_join_addr64(u32 lo, u32 hi)
 
 int isp4if_f2h_resp(struct isp4_interface *ispif, enum isp4if_stream_id stream, void *response);
 
-int isp4if_send_command(struct isp4_interface *ispif, u32 cmd_id, void *package,
+int isp4if_send_command(struct isp4_interface *ispif, u32 cmd_id, const void *package,
 			u32 package_size);
 
-int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, void *package,
-			     u32 package_size, u32 timeout);
+int isp4if_send_command_sync(struct isp4_interface *ispif, u32 cmd_id, const void *package,
+			     u32 package_size);
 
 struct isp4if_cmd_element *isp4if_rm_cmd_from_cmdq(struct isp4_interface *ispif, u32 seq_num,
 						   u32 cmd_id);
diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.c b/drivers/media/platform/amd/isp4/isp4_subdev.c
index 17480ae5150d..0e9e6ec59ed8 100644
--- a/drivers/media/platform/amd/isp4/isp4_subdev.c
+++ b/drivers/media/platform/amd/isp4/isp4_subdev.c
@@ -17,8 +17,6 @@
 #define ISP4SD_PERFORMANCE_STATE_LOW 0
 #define ISP4SD_PERFORMANCE_STATE_HIGH 1
 
-#define ISP4SD_FW_CMD_TIMEOUT_IN_MS  500
-
 /* align 32KB */
 #define ISP4SD_META_BUF_SIZE ALIGN(sizeof(struct isp4fw_meta_info), 0x8000)
 
@@ -55,7 +53,7 @@ static int isp4sd_setup_fw_mem_pool(struct isp4_subdev *isp_subdev)
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&buf_type, 0, sizeof(buf_type));
 	buf_type.buffer_type = BUFFER_TYPE_MEM_POOL;
@@ -92,7 +90,7 @@ static int isp4sd_set_stream_path(struct isp4_subdev *isp_subdev)
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&cmd, 0, sizeof(cmd));
 	cmd.stream_cfg.mipi_pipe_path_cfg.isp4fw_sensor_id = SENSOR_ID_ON_MIPI0;
@@ -113,16 +111,14 @@ static int isp4sd_send_meta_buf(struct isp4_subdev *isp_subdev)
 {
 	struct isp4_interface *ispif = &isp_subdev->ispif;
 	struct isp4fw_cmd_send_buffer buf_type;
-	struct isp4sd_sensor_info *sensor_info;
 	struct device *dev = isp_subdev->dev;
-	u32 i;
+	int i;
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&buf_type, 0, sizeof(buf_type));
-	sensor_info = &isp_subdev->sensor_info;
 	for (i = 0; i < ISP4IF_MAX_STREAM_BUF_COUNT; i++) {
 		int ret;
 
@@ -140,8 +136,7 @@ static int isp4sd_send_meta_buf(struct isp4_subdev *isp_subdev)
 		buf_type.buffer.buf_size_a =
 			(u32)isp_subdev->ispif.meta_info_buf[i]->mem_size;
 		ret = isp4if_send_command(ispif, CMD_ID_SEND_BUFFER,
-					  &buf_type,
-					  sizeof(buf_type));
+					  &buf_type, sizeof(buf_type));
 		if (ret) {
 			dev_err(dev, "send meta info(%u) fail\n", i);
 			return ret;
@@ -158,7 +153,6 @@ static bool isp4sd_get_str_out_prop(struct isp4_subdev *isp_subdev,
 {
 	struct device *dev = isp_subdev->dev;
 	struct v4l2_mbus_framefmt *format;
-	bool ret;
 
 	format = v4l2_subdev_state_get_format(state, pad, 0);
 	if (!format) {
@@ -173,7 +167,6 @@ static bool isp4sd_get_str_out_prop(struct isp4_subdev *isp_subdev,
 		out_prop->height = format->height;
 		out_prop->luma_pitch = format->width;
 		out_prop->chroma_pitch = out_prop->width;
-		ret = true;
 		break;
 	case MEDIA_BUS_FMT_YUYV8_1X16:
 		out_prop->image_format = IMAGE_FORMAT_YUV422INTERLEAVED;
@@ -181,18 +174,17 @@ static bool isp4sd_get_str_out_prop(struct isp4_subdev *isp_subdev,
 		out_prop->height = format->height;
 		out_prop->luma_pitch = format->width * 2;
 		out_prop->chroma_pitch = 0;
-		ret = true;
 		break;
 	default:
 		dev_err(dev, "fail for bad image format:0x%x\n",
 			format->code);
-		ret = false;
-		break;
+		return false;
 	}
 
 	if (!out_prop->width || !out_prop->height)
-		ret = false;
-	return ret;
+		return false;
+
+	return true;
 }
 
 static int isp4sd_kickoff_stream(struct isp4_subdev *isp_subdev, u32 w, u32 h)
@@ -249,7 +241,6 @@ static int isp4sd_setup_output(struct isp4_subdev *isp_subdev,
 	struct isp4fw_cmd_set_out_ch_prop cmd_ch_prop;
 	struct isp4fw_cmd_enable_out_ch cmd_ch_en;
 	struct device *dev = isp_subdev->dev;
-	struct isp4fw_image_prop *out_prop;
 	int ret;
 
 	if (output_info->start_status == ISP4SD_START_STATUS_STARTED)
@@ -262,20 +253,12 @@ static int isp4sd_setup_output(struct isp4_subdev *isp_subdev,
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&cmd_ch_prop, 0, sizeof(cmd_ch_prop));
-	/*
-	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
-	 */
-	memset(&cmd_ch_en, 0, sizeof(cmd_ch_en));
-	out_prop = &cmd_ch_prop.image_prop;
 	cmd_ch_prop.ch = ISP_PIPE_OUT_CH_PREVIEW;
-	cmd_ch_en.ch = ISP_PIPE_OUT_CH_PREVIEW;
-	cmd_ch_en.is_enable = true;
 
-	if (!isp4sd_get_str_out_prop(isp_subdev, out_prop, state, pad)) {
+	if (!isp4sd_get_str_out_prop(isp_subdev, &cmd_ch_prop.image_prop, state, pad)) {
 		dev_err(dev, "fail to get out prop\n");
 		return -EINVAL;
 	}
@@ -288,17 +271,23 @@ static int isp4sd_setup_output(struct isp4_subdev *isp_subdev,
 		cmd_ch_prop.image_prop.chroma_pitch);
 
 	ret = isp4if_send_command(ispif, CMD_ID_SET_OUT_CHAN_PROP,
-				  &cmd_ch_prop,
-				  sizeof(cmd_ch_prop));
+				  &cmd_ch_prop, sizeof(cmd_ch_prop));
 	if (ret) {
 		output_info->start_status = ISP4SD_START_STATUS_START_FAIL;
 		dev_err(dev, "fail to set out prop\n");
 		return ret;
-	};
+	}
+
+	/*
+	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
+	 * zeroed, since this is not guaranteed on all compilers.
+	 */
+	memset(&cmd_ch_en, 0, sizeof(cmd_ch_en));
+	cmd_ch_en.ch = ISP_PIPE_OUT_CH_PREVIEW;
+	cmd_ch_en.is_enable = true;
 
 	ret = isp4if_send_command(ispif, CMD_ID_ENABLE_OUT_CHAN,
 				  &cmd_ch_en, sizeof(cmd_ch_en));
-
 	if (ret) {
 		output_info->start_status = ISP4SD_START_STATUS_START_FAIL;
 		dev_err(dev, "fail to enable channel\n");
@@ -309,8 +298,9 @@ static int isp4sd_setup_output(struct isp4_subdev *isp_subdev,
 		isp4dbg_get_out_ch_str(cmd_ch_en.ch));
 
 	if (!sensor_info->start_stream_cmd_sent) {
-		ret = isp4sd_kickoff_stream(isp_subdev, out_prop->width,
-					    out_prop->height);
+		ret = isp4sd_kickoff_stream(isp_subdev,
+					    cmd_ch_prop.image_prop.width,
+					    cmd_ch_prop.image_prop.height);
 		if (ret) {
 			dev_err(dev, "kickoff stream fail %d\n", ret);
 			return ret;
@@ -463,36 +453,6 @@ static struct isp4fw_meta_info *isp4sd_get_meta_by_mc(struct isp4_subdev *isp_su
 	return NULL;
 }
 
-static struct isp4if_img_buf_node *
-isp4sd_preview_done(struct isp4_subdev *isp_subdev,
-		    struct isp4fw_meta_info *meta,
-		    struct isp4vid_framedone_param *pcb)
-{
-	struct isp4_interface *ispif = &isp_subdev->ispif;
-	struct isp4if_img_buf_node *prev = NULL;
-	struct device *dev = isp_subdev->dev;
-
-	pcb->preview.status = ISP4VID_BUF_DONE_STATUS_ABSENT;
-	if (meta->preview.enabled &&
-	    (meta->preview.status == BUFFER_STATUS_SKIPPED ||
-	     meta->preview.status == BUFFER_STATUS_DONE ||
-	     meta->preview.status == BUFFER_STATUS_DIRTY)) {
-		prev = isp4if_dequeue_buffer(ispif);
-		if (!prev) {
-			dev_err(dev, "fail null prev buf\n");
-		} else {
-			pcb->preview.buf = prev->buf_info;
-			pcb->preview.status = ISP4VID_BUF_DONE_STATUS_SUCCESS;
-		}
-	} else if (meta->preview.enabled) {
-		dev_err(dev, "fail bad preview status %u(%s)\n",
-			meta->preview.status,
-			isp4dbg_get_buf_done_str(meta->preview.status));
-	}
-
-	return prev;
-}
-
 static void isp4sd_send_meta_info(struct isp4_subdev *isp_subdev,
 				  u64 meta_info_mc)
 {
@@ -508,38 +468,35 @@ static void isp4sd_send_meta_info(struct isp4_subdev *isp_subdev,
 
 	/*
 	 * The struct will be shared with ISP FW, use memset() to guarantee padding bits are
-	 * zeroed, since this may not guarantee on all compilers.
+	 * zeroed, since this is not guaranteed on all compilers.
 	 */
 	memset(&buf_type, 0, sizeof(buf_type));
-	if (meta_info_mc) {
-		buf_type.buffer_type = BUFFER_TYPE_META_INFO;
-		buf_type.buffer.buf_tags = 0;
-		buf_type.buffer.vmid_space.bit.vmid = 0;
-		buf_type.buffer.vmid_space.bit.space = ADDR_SPACE_TYPE_GPU_VA;
-		isp4if_split_addr64(meta_info_mc,
-				    &buf_type.buffer.buf_base_a_lo,
-				    &buf_type.buffer.buf_base_a_hi);
+	buf_type.buffer_type = BUFFER_TYPE_META_INFO;
+	buf_type.buffer.buf_tags = 0;
+	buf_type.buffer.vmid_space.bit.vmid = 0;
+	buf_type.buffer.vmid_space.bit.space = ADDR_SPACE_TYPE_GPU_VA;
+	isp4if_split_addr64(meta_info_mc,
+			    &buf_type.buffer.buf_base_a_lo,
+			    &buf_type.buffer.buf_base_a_hi);
+	buf_type.buffer.buf_size_a = ISP4SD_META_BUF_SIZE;
 
-		buf_type.buffer.buf_size_a = ISP4SD_META_BUF_SIZE;
-		if (isp4if_send_command(ispif, CMD_ID_SEND_BUFFER,
-					&buf_type, sizeof(buf_type))) {
-			dev_err(dev, "fail send meta_info 0x%llx\n",
-				meta_info_mc);
-		} else {
-			dev_dbg(dev, "resend meta_info 0x%llx\n", meta_info_mc);
-		}
-	}
+	if (isp4if_send_command(ispif, CMD_ID_SEND_BUFFER,
+				&buf_type, sizeof(buf_type)))
+		dev_err(dev, "fail send meta_info 0x%llx\n",
+			meta_info_mc);
+	else
+		dev_dbg(dev, "resend meta_info 0x%llx\n", meta_info_mc);
 }
 
 static void isp4sd_fw_resp_frame_done(struct isp4_subdev *isp_subdev,
 				      enum isp4if_stream_id stream_id,
 				      struct isp4fw_resp_param_package *para)
 {
-	struct isp4vid_framedone_param pcb = {};
+	struct isp4_interface *ispif = &isp_subdev->ispif;
 	struct device *dev = isp_subdev->dev;
 	struct isp4if_img_buf_node *prev;
 	struct isp4fw_meta_info *meta;
-	u64 mc = 0;
+	u64 mc;
 
 	mc = isp4if_join_addr64(para->package_addr_lo, para->package_addr_hi);
 	meta = isp4sd_get_meta_by_mc(isp_subdev, mc);
@@ -548,24 +505,32 @@ static void isp4sd_fw_resp_frame_done(struct isp4_subdev *isp_subdev,
 		return;
 	}
 
-	pcb.poc = meta->poc;
-	pcb.cam_id = 0;
-
 	dev_dbg(dev, "ts:%llu,streamId:%d,poc:%u,preview_en:%u,%s(%i)\n",
 		ktime_get_ns(), stream_id, meta->poc,
 		meta->preview.enabled,
 		isp4dbg_get_buf_done_str(meta->preview.status),
 		meta->preview.status);
 
-	prev = isp4sd_preview_done(isp_subdev, meta, &pcb);
-	if (pcb.preview.status != ISP4VID_BUF_DONE_STATUS_ABSENT) {
-		isp4dbg_show_bufmeta_info(dev, "prev", &meta->preview,
-					  &pcb.preview.buf);
-		isp4vid_notify(&isp_subdev->isp_vdev, &pcb);
+	if (meta->preview.enabled &&
+	    (meta->preview.status == BUFFER_STATUS_SKIPPED ||
+	     meta->preview.status == BUFFER_STATUS_DONE ||
+	     meta->preview.status == BUFFER_STATUS_DIRTY)) {
+		prev = isp4if_dequeue_buffer(ispif);
+		if (prev) {
+			isp4dbg_show_bufmeta_info(dev, "prev", &meta->preview,
+						  &prev->buf_info);
+			isp4vid_handle_frame_done(&isp_subdev->isp_vdev,
+						  &prev->buf_info);
+			isp4if_dealloc_buffer_node(prev);
+		} else {
+			dev_err(dev, "fail null prev buf\n");
+		}
+	} else if (meta->preview.enabled) {
+		dev_err(dev, "fail bad preview status %u(%s)\n",
+			meta->preview.status,
+			isp4dbg_get_buf_done_str(meta->preview.status));
 	}
 
-	isp4if_dealloc_buffer_node(prev);
-
 	if (isp_subdev->sensor_info.status == ISP4SD_START_STATUS_STARTED)
 		isp4sd_send_meta_info(isp_subdev, mc);
 
@@ -834,22 +799,23 @@ static int isp4sd_stop_stream(struct isp4_subdev *isp_subdev,
 	if (output_info->start_status == ISP4SD_START_STATUS_STARTED) {
 		struct isp4fw_cmd_enable_out_ch cmd_ch_disable;
 
+		/*
+		 * The struct will be shared with ISP FW, use memset() to guarantee
+		 * padding bits are zeroed, since this is not guaranteed on all compilers.
+		 */
+		memset(&cmd_ch_disable, 0, sizeof(cmd_ch_disable));
 		cmd_ch_disable.ch = ISP_PIPE_OUT_CH_PREVIEW;
 		cmd_ch_disable.is_enable = false;
-		ret = isp4if_send_command_sync(ispif,
-					       CMD_ID_ENABLE_OUT_CHAN,
+		ret = isp4if_send_command_sync(ispif, CMD_ID_ENABLE_OUT_CHAN,
 					       &cmd_ch_disable,
-					       sizeof(cmd_ch_disable),
-					       ISP4SD_FW_CMD_TIMEOUT_IN_MS);
+					       sizeof(cmd_ch_disable));
 		if (ret)
 			dev_err(dev, "fail to disable stream\n");
 		else
 			dev_dbg(dev, "wait disable stream suc\n");
 
 		ret = isp4if_send_command_sync(ispif, CMD_ID_STOP_STREAM,
-					       NULL,
-					       0,
-					       ISP4SD_FW_CMD_TIMEOUT_IN_MS);
+					       NULL, 0);
 		if (ret)
 			dev_err(dev, "fail to stop steam\n");
 		else
diff --git a/drivers/media/platform/amd/isp4/isp4_video.c b/drivers/media/platform/amd/isp4/isp4_video.c
index 456435f6e771..ca876cb72154 100644
--- a/drivers/media/platform/amd/isp4/isp4_video.c
+++ b/drivers/media/platform/amd/isp4/isp4_video.c
@@ -60,12 +60,10 @@ static const struct v4l2_fract isp4vid_tpfs[] = {
 	{ 1, ISP4VID_MAX_PREVIEW_FPS }
 };
 
-static void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
-				      const struct isp4if_img_buf_info *img_buf,
-				      bool done_suc)
+void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
+			       const struct isp4if_img_buf_info *img_buf)
 {
 	struct isp4vid_capture_buffer *isp4vid_buf;
-	enum vb2_buffer_state state;
 	void *vbuf;
 
 	scoped_guard(mutex, &isp_vdev->buf_list_lock) {
@@ -95,30 +93,12 @@ static void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
 	vb2_set_plane_payload(&isp4vid_buf->vb2.vb2_buf,
 			      0, isp_vdev->format.sizeimage);
 
-	state = done_suc ? VB2_BUF_STATE_DONE : VB2_BUF_STATE_ERROR;
-	vb2_buffer_done(&isp4vid_buf->vb2.vb2_buf, state);
+	vb2_buffer_done(&isp4vid_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
 
 	dev_dbg(isp_vdev->dev, "call vb2_buffer_done(size=%u)\n",
 		isp_vdev->format.sizeimage);
 }
 
-s32 isp4vid_notify(void *cb_ctx, struct isp4vid_framedone_param *evt_param)
-{
-	struct isp4vid_dev *isp4vid_vdev = cb_ctx;
-
-	if (evt_param->preview.status != ISP4VID_BUF_DONE_STATUS_ABSENT) {
-		bool suc;
-
-		suc = (evt_param->preview.status ==
-		       ISP4VID_BUF_DONE_STATUS_SUCCESS);
-		isp4vid_handle_frame_done(isp4vid_vdev,
-					  &evt_param->preview.buf,
-					  suc);
-	}
-
-	return 0;
-}
-
 static unsigned int isp4vid_vb2_num_users(void *buf_priv)
 {
 	struct isp4vid_vb2_buf *buf = buf_priv;
diff --git a/drivers/media/platform/amd/isp4/isp4_video.h b/drivers/media/platform/amd/isp4/isp4_video.h
index d925f67567e7..b87316d2a2e5 100644
--- a/drivers/media/platform/amd/isp4/isp4_video.h
+++ b/drivers/media/platform/amd/isp4/isp4_video.h
@@ -11,26 +11,6 @@
 
 #include "isp4_interface.h"
 
-enum isp4vid_buf_done_status {
-	/* It means no corresponding image buf in fw response */
-	ISP4VID_BUF_DONE_STATUS_ABSENT,
-	ISP4VID_BUF_DONE_STATUS_SUCCESS,
-	ISP4VID_BUF_DONE_STATUS_FAILED
-};
-
-struct isp4vid_buf_done_info {
-	enum isp4vid_buf_done_status status;
-	struct isp4if_img_buf_info buf;
-};
-
-/* call back parameter for CB_EVT_ID_FRAME_DONE */
-struct isp4vid_framedone_param {
-	s32 poc;
-	s32 cam_id;
-	s64 time_stamp;
-	struct isp4vid_buf_done_info preview;
-};
-
 struct isp4vid_capture_buffer {
 	/*
 	 * struct vb2_v4l2_buffer must be the first element
@@ -79,6 +59,7 @@ int isp4vid_dev_init(struct isp4vid_dev *isp_vdev,
 
 void isp4vid_dev_deinit(struct isp4vid_dev *isp_vdev);
 
-s32 isp4vid_notify(void *cb_ctx, struct isp4vid_framedone_param *evt_param);
+void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
+			       const struct isp4if_img_buf_info *img_buf);
 
 #endif /* _ISP4_VIDEO_H_ */
-- 
2.51.2