>From 80619e49f1d94e22fd2677709bc3bc0048793d9e Mon Sep 17 00:00:00 2001 From: Sultan Alsawaf Date: Fri, 21 Nov 2025 19:10:05 -0800 Subject: [PATCH v2] media: platform: amd: Don't wait for cmd_done under isp4if_mutex This fixes a regression in "[PATCH v2] media: platform: amd: Big squash of fixes/cleanup on top of v5 patchset" (and in the v1 version) where waiting for completion of a command was accidentally moved underneath isp4if_mutex, causing the following error every time when closing the camera: [ 56.136720] amd_isp_capture amd_isp_capture: fail to disable stream [ 56.643383] amd_isp_capture amd_isp_capture: fail to stop steam [ 56.643531] amd_isp_capture amd_isp_capture: disabling streaming failed (-110) Fix it by moving the wait outside of isp4if_mutex, and also moving unrelated operations outside of the isp4if_mutex scope as an optimization. Signed-off-by: Sultan Alsawaf --- v1 -> v2 changes: -Rebased onto v2-media-platform-amd-Big-squash-of-fixes-cleanup-on.patch, since this patch was accidentally based on v1 of the big squash patch --- .../media/platform/amd/isp4/isp4_interface.c | 97 +++++++++---------- 1 file changed, 48 insertions(+), 49 deletions(-) diff --git a/drivers/media/platform/amd/isp4/isp4_interface.c b/drivers/media/platform/amd/isp4/isp4_interface.c index 21055af96602..90585b8a4e8c 100644 --- a/drivers/media/platform/amd/isp4/isp4_interface.c +++ b/drivers/media/platform/amd/isp4/isp4_interface.c @@ -339,34 +339,6 @@ static int isp4if_send_fw_cmd(struct isp4_interface *ispif, u32 cmd_id, const vo return -EINVAL; } - /* Allocate the sync command object early and outside of the lock */ - if (sync) { - ele = kmalloc(sizeof(*ele), GFP_KERNEL); - if (!ele) - return -ENOMEM; - - /* Get two references: one for the resp thread, one for us */ - atomic_set(&ele->refcnt, 2); - init_completion(&ele->cmd_done); - } - - guard(mutex)(&ispif->isp4if_mutex); - - ret = read_poll_timeout(isp4if_is_cmdq_rb_full, ret, !ret, ISP4IF_RB_FULL_SLEEP_US, - ISP4IF_RB_FULL_TIMEOUT_US, false, ispif, stream); - if (ret) { - struct isp4if_rb_config *rb_config = &isp4if_resp_rb_config[stream]; - u32 rd_ptr = isp4hw_rreg(ispif->mmio, rb_config->reg_rptr); - u32 wr_ptr = isp4hw_rreg(ispif->mmio, rb_config->reg_wptr); - - dev_err(dev, - "failed to get free cmdq slot, stream %s(%d),rd %u, wr %u\n", - isp4dbg_get_if_stream_str(stream), - stream, rd_ptr, wr_ptr); - ret = -ETIMEDOUT; - goto free_ele; - } - /* * The struct will be shared with ISP FW, use memset() to guarantee padding bits are * zeroed, since this is not guaranteed on all compilers. @@ -382,34 +354,61 @@ static int isp4if_send_fw_cmd(struct isp4_interface *ispif, u32 cmd_id, const vo break; default: dev_err(dev, "fail bad stream id %d\n", stream); - ret = -EINVAL; - goto free_ele; + return -EINVAL; + } + + /* Allocate the sync command object early and outside of the lock */ + if (sync) { + ele = kmalloc(sizeof(*ele), GFP_KERNEL); + if (!ele) + return -ENOMEM; + + /* Get two references: one for the resp thread, one for us */ + atomic_set(&ele->refcnt, 2); + init_completion(&ele->cmd_done); } if (package && package_size) memcpy(cmd.cmd_param, package, package_size); - seq_num = ispif->host2fw_seq_num++; - cmd.cmd_seq_num = seq_num; - cmd.cmd_check_sum = isp4if_compute_check_sum(&cmd, sizeof(cmd) - sizeof(u32)); + scoped_guard(mutex, &ispif->isp4if_mutex) { + ret = read_poll_timeout(isp4if_is_cmdq_rb_full, ret, !ret, ISP4IF_RB_FULL_SLEEP_US, + ISP4IF_RB_FULL_TIMEOUT_US, false, ispif, stream); + if (ret) { + struct isp4if_rb_config *rb_config = &isp4if_resp_rb_config[stream]; + u32 rd_ptr = isp4hw_rreg(ispif->mmio, rb_config->reg_rptr); + u32 wr_ptr = isp4hw_rreg(ispif->mmio, rb_config->reg_wptr); + + dev_err(dev, + "failed to get free cmdq slot, stream %s(%d),rd %u, wr %u\n", + isp4dbg_get_if_stream_str(stream), + stream, rd_ptr, wr_ptr); + ret = -ETIMEDOUT; + goto free_ele; + } - /* - * only append the fw cmd to queue when its response needs to be waited for, - * currently there are only two such commands, disable channel and stop stream - * which are only sent after close camera - */ - if (ele) { - ele->seq_num = seq_num; - ele->cmd_id = cmd_id; - scoped_guard(spinlock, &ispif->cmdq_lock) - list_add_tail(&ele->list, &ispif->cmdq); - } + seq_num = ispif->host2fw_seq_num++; + cmd.cmd_seq_num = seq_num; + cmd.cmd_check_sum = isp4if_compute_check_sum(&cmd, sizeof(cmd) - sizeof(u32)); + + /* + * only append the fw cmd to queue when its response needs to be waited for, + * currently there are only two such commands, disable channel and stop stream + * which are only sent after close camera + */ + if (ele) { + ele->seq_num = seq_num; + ele->cmd_id = cmd_id; + scoped_guard(spinlock, &ispif->cmdq_lock) + list_add_tail(&ele->list, &ispif->cmdq); + } - ret = isp4if_insert_isp_fw_cmd(ispif, stream, &cmd); - if (ret) { - dev_err(dev, "fail for insert_isp_fw_cmd cmd_id %s(0x%08x)\n", - isp4dbg_get_cmd_str(cmd_id), cmd_id); - goto err_dequeue_ele; + ret = isp4if_insert_isp_fw_cmd(ispif, stream, &cmd); + if (ret) { + dev_err(dev, "fail for insert_isp_fw_cmd cmd_id %s(0x%08x)\n", + isp4dbg_get_cmd_str(cmd_id), cmd_id); + goto err_dequeue_ele; + } } if (ele) { -- 2.52.0