From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.15]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9262C312830; Mon, 22 Dec 2025 10:11:17 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.15 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766398281; cv=none; b=Cn9+xDvZmY7Hhe8m49Czfjn0epvZGieruclxOGCZw/PWACJUVMMCs8KD0RrXJSUAj7QWjREDoIA7glRqSIoinIpQ7YlOnNT19iRIcsQ0XU9CWy0clNA5nxNAVQvPudmpSqpeyEYxvWt+n/VSDikYC2b2wVANpBh7zaWJD+E/P/g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766398281; c=relaxed/simple; bh=QRuGyTYBP4ysWYy6ijrGM1AQC+TrHmh1N4On8SFaryw=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZHhStYVgGpudfpqvGLfzFlimWrWMEWe5X7Lr2vwTemVQ6UG051wGOOG9nIOFHWl9ZGBKwS/z5fg1+ijXPnINjMi93BIH0FGjnitj1kkfl3UsIoa/kA5qkk8ay1Ia4P6jLUp6034I4lFNx3t2Y5ermnvXVhL1JLAGIyfd/hchXNg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=mpFKvIhp; arc=none smtp.client-ip=198.175.65.15 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="mpFKvIhp" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1766398277; x=1797934277; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=QRuGyTYBP4ysWYy6ijrGM1AQC+TrHmh1N4On8SFaryw=; b=mpFKvIhpsmO2IYOfnYsa6LtHCRS0hSPu2c295LhwRdK6v60dUW3IorjV buAtHXc8boa1N42p6T/n7b12CACDbQdGHB53ZweMo8YYiMVWdSszfQfYY fTrhu+dA28pmfipEYYAAiNiBg3dfZt7IJgF3hV0hnPlOVPS+TSdL4pJxY xiFUQFAlfCeQERdpDSTeanK1IppBtpsLdjwzMcRjvhbu9HYP8s3ltTSLi tl4pgol1DPB1jvieOkOfE6zt+46r2xc2Jv8u4aqKvs9qXBogpxNvnNq9u 5dWf4c1chvV+x+2nK3zDa6CXXDepvbdMcwjbpgD3MgTYDIE7n/siAp+Lr A==; X-CSE-ConnectionGUID: b5YPUVopR1G+9LtP9ciPEg== X-CSE-MsgGUID: fBJFg2aNRbWJS8+CnZklWA== X-IronPort-AV: E=McAfee;i="6800,10657,11649"; a="71883505" X-IronPort-AV: E=Sophos;i="6.21,167,1763452800"; d="scan'208";a="71883505" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by orvoesa107.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Dec 2025 02:11:16 -0800 X-CSE-ConnectionGUID: iVpVxdFhSAKcHwZHiwQZww== X-CSE-MsgGUID: PcOMoRtGQfyTtDC5VrK6LA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.21,167,1763452800"; d="scan'208";a="222978689" Received: from ijarvine-mobl1.ger.corp.intel.com (HELO kekkonen.fi.intel.com) ([10.245.245.187]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Dec 2025 02:11:11 -0800 Received: from kekkonen.localdomain (localhost [IPv6:::1]) by kekkonen.fi.intel.com (Postfix) with SMTP id 042FE12063F; Mon, 22 Dec 2025 12:11:12 +0200 (EET) Date: Mon, 22 Dec 2025 12:11:11 +0200 Organization: Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs, Bertel Jungin Aukio 5, 02600 Espoo From: Sakari Ailus To: Bin Du Cc: mchehab@kernel.org, hverkuil@xs4all.nl, laurent.pinchart+renesas@ideasonboard.com, bryan.odonoghue@linaro.org, prabhakar.mahadev-lad.rj@bp.renesas.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, sultan@kerneltoast.com, pratap.nirujogi@amd.com, benjamin.chan@amd.com, king.li@amd.com, gjorgji.rosikopulos@amd.com, Phil.Jawich@amd.com, Dominic.Antony@amd.com, mario.limonciello@amd.com, richard.gong@amd.com, anson.tsao@amd.com, Svetoslav Stoilov , Alexey Zagorodnikov Subject: Re: [PATCH v7 4/7] media: platform: amd: isp4 subdev and firmware loading handling added Message-ID: References: <20251216091326.111977-1-Bin.Du@amd.com> <20251216091326.111977-5-Bin.Du@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20251216091326.111977-5-Bin.Du@amd.com> Hi Bin, On Tue, Dec 16, 2025 at 05:13:23PM +0800, Bin Du wrote: > Isp4 sub-device is implementing v4l2 sub-device interface. It has one > capture video node, and supports only preview stream. It manages firmware > states, stream configuration. Add interrupt handling and notification for > isp firmware to isp-subdevice. > > Co-developed-by: Sultan Alsawaf > Signed-off-by: Sultan Alsawaf > Co-developed-by: Svetoslav Stoilov > Signed-off-by: Svetoslav Stoilov > Signed-off-by: Bin Du > Tested-by: Alexey Zagorodnikov > --- > MAINTAINERS | 2 + > drivers/media/platform/amd/isp4/Makefile | 3 +- > drivers/media/platform/amd/isp4/isp4.c | 99 +- > drivers/media/platform/amd/isp4/isp4.h | 7 +- > drivers/media/platform/amd/isp4/isp4_subdev.c | 975 ++++++++++++++++++ > drivers/media/platform/amd/isp4/isp4_subdev.h | 124 +++ > 6 files changed, 1202 insertions(+), 8 deletions(-) > create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.c > create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index cccae369c876..48ffc8bbdcee 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1149,6 +1149,8 @@ F: drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h > F: drivers/media/platform/amd/isp4/isp4_hw_reg.h > F: drivers/media/platform/amd/isp4/isp4_interface.c > F: drivers/media/platform/amd/isp4/isp4_interface.h > +F: drivers/media/platform/amd/isp4/isp4_subdev.c > +F: drivers/media/platform/amd/isp4/isp4_subdev.h > > AMD KFD > M: Felix Kuehling > diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile > index a2a5bf98e912..6d4e6d6ac7f5 100644 > --- a/drivers/media/platform/amd/isp4/Makefile > +++ b/drivers/media/platform/amd/isp4/Makefile > @@ -4,4 +4,5 @@ > > obj-$(CONFIG_AMD_ISP4) += amd_capture.o > amd_capture-objs := isp4.o \ > - isp4_interface.o > + isp4_interface.o \ > + isp4_subdev.o > diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/platform/amd/isp4/isp4.c > index ad95e7f89189..bcd7cad32afd 100644 > --- a/drivers/media/platform/amd/isp4/isp4.c > +++ b/drivers/media/platform/amd/isp4/isp4.c > @@ -3,15 +3,19 @@ > * Copyright (C) 2025 Advanced Micro Devices, Inc. > */ > > +#include > #include > #include > +#include I don't think you need this one. > #include > > #include "isp4.h" > - > -#define VIDEO_BUF_NUM 5 What happened with this one? Is it no longer needed? > +#include "isp4_hw_reg.h" > > #define ISP4_DRV_NAME "amd_isp_capture" > +#define ISP4_FW_RESP_RB_IRQ_STATUS_MASK \ > + (ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT9_INT_MASK | \ > + ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT12_INT_MASK) > > static const struct { > const char *name; > @@ -19,27 +23,97 @@ static const struct { > u32 en_mask; > u32 ack_mask; > u32 rb_int_num; > -} isp4_irq[] = { > +} isp4_irq[ISP4SD_MAX_FW_RESP_STREAM_NUM] = { > /* The IRQ order is aligned with the isp4_subdev.fw_resp_thread order */ > { > .name = "isp_irq_global", > + .status_mask = ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT12_INT_MASK, > + .en_mask = ISP_SYS_INT0_EN__SYS_INT_RINGBUFFER_WPT12_EN_MASK, > + .ack_mask = ISP_SYS_INT0_ACK__SYS_INT_RINGBUFFER_WPT12_ACK_MASK, > .rb_int_num = 4, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT12 */ > }, > { > .name = "isp_irq_stream1", > + .status_mask = ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT9_INT_MASK, > + .en_mask = ISP_SYS_INT0_EN__SYS_INT_RINGBUFFER_WPT9_EN_MASK, > + .ack_mask = ISP_SYS_INT0_ACK__SYS_INT_RINGBUFFER_WPT9_ACK_MASK, > .rb_int_num = 0, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT9 */ > }, > }; > > +void isp4_intr_enable(struct isp4_subdev *isp_subdev, u32 index, bool enable) > +{ > + u32 intr_en; > + > + /* Synchronize ISP_SYS_INT0_EN writes with the IRQ handler's writes */ > + spin_lock_irq(&isp_subdev->irq_lock); > + intr_en = isp4hw_rreg(isp_subdev->mmio, ISP_SYS_INT0_EN); > + if (enable) > + intr_en |= isp4_irq[index].en_mask; > + else > + intr_en &= ~isp4_irq[index].en_mask; > + > + isp4hw_wreg(isp_subdev->mmio, ISP_SYS_INT0_EN, intr_en); > + spin_unlock_irq(&isp_subdev->irq_lock); > +} > + > +static void isp4_wake_up_resp_thread(struct isp4_subdev *isp_subdev, u32 index) > +{ > + struct isp4sd_thread_handler *thread_ctx = &isp_subdev->fw_resp_thread[index]; > + > + thread_ctx->resp_ready = true; > + wake_up_interruptible(&thread_ctx->waitq); > +} > + > static irqreturn_t isp4_irq_handler(int irq, void *arg) > { > + struct isp4_subdev *isp_subdev = arg; > + u32 intr_ack = 0, intr_en = 0, intr_status; > + int seen = 0; Is int appropriate here? Should this be u32 or u64? > + > + /* Get the ISP_SYS interrupt status */ > + intr_status = isp4hw_rreg(isp_subdev->mmio, ISP_SYS_INT0_STATUS); > + intr_status &= ISP4_FW_RESP_RB_IRQ_STATUS_MASK; > + > + /* Find which ISP_SYS interrupts fired */ > + for (size_t i = 0; i < ARRAY_SIZE(isp4_irq); i++) { > + if (intr_status & isp4_irq[i].status_mask) { > + intr_ack |= isp4_irq[i].ack_mask; > + intr_en |= isp4_irq[i].en_mask; > + seen |= BIT(i); > + } > + } > + > + /* > + * Disable the ISP_SYS interrupts that fired. Must be done before waking > + * the response threads, since they re-enable interrupts when finished. > + * The lock synchronizes RMW of INT0_EN with isp4_enable_interrupt(). > + */ > + spin_lock(&isp_subdev->irq_lock); > + intr_en = isp4hw_rreg(isp_subdev->mmio, ISP_SYS_INT0_EN) & ~intr_en; > + isp4hw_wreg(isp_subdev->mmio, ISP_SYS_INT0_EN, intr_en); > + spin_unlock(&isp_subdev->irq_lock); > + > + /* > + * Clear the ISP_SYS interrupts. This must be done after the interrupts > + * are disabled, so that ISP FW won't flag any new interrupts on these > + * streams, and thus we don't need to clear interrupts again before > + * re-enabling them in the response thread. > + */ > + isp4hw_wreg(isp_subdev->mmio, ISP_SYS_INT0_ACK, intr_ack); > + > + /* Wake up the response threads */ > + for (int i; (i = ffs(seen)); seen = (seen >> i) << i) unsigned int, please. The parentheses around ffs() appear redundant. The increment could probably be expressed as seen &= ~BIT(i). > + isp4_wake_up_resp_thread(isp_subdev, i - 1); > + > return IRQ_HANDLED; > } > > static int isp4_capture_probe(struct platform_device *pdev) > { > + int irq[ISP4SD_MAX_FW_RESP_STREAM_NUM]; > struct device *dev = &pdev->dev; > - int irq[ARRAY_SIZE(isp4_irq)]; > + struct isp4_subdev *isp_subdev; > struct isp4_device *isp_dev; > size_t i; > int ret; > @@ -50,6 +124,11 @@ static int isp4_capture_probe(struct platform_device *pdev) > > dev->init_name = ISP4_DRV_NAME; > > + isp_subdev = &isp_dev->isp_subdev; > + isp_subdev->mmio = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(isp_subdev->mmio)) > + return dev_err_probe(dev, PTR_ERR(isp_subdev->mmio), "isp ioremap fail\n"); > + > for (i = 0; i < ARRAY_SIZE(isp4_irq); i++) { > irq[i] = platform_get_irq(pdev, isp4_irq[i].rb_int_num); > if (irq[i] < 0) > @@ -57,7 +136,7 @@ static int isp4_capture_probe(struct platform_device *pdev) > isp4_irq[i].rb_int_num); > > ret = devm_request_irq(dev, irq[i], isp4_irq_handler, > - IRQF_NO_AUTOEN, isp4_irq[i].name, dev); > + IRQF_NO_AUTOEN, isp4_irq[i].name, isp_subdev); > if (ret) > return dev_err_probe(dev, ret, "fail to req irq %d\n", irq[i]); > } > @@ -83,6 +162,13 @@ static int isp4_capture_probe(struct platform_device *pdev) > > pm_runtime_set_suspended(dev); > pm_runtime_enable(dev); > + spin_lock_init(&isp_subdev->irq_lock); > + ret = isp4sd_init(&isp_dev->isp_subdev, &isp_dev->v4l2_dev, irq); > + if (ret) { > + dev_err_probe(dev, ret, "fail init isp4 sub dev\n"); > + goto err_pm_disable; > + } > + > ret = media_device_register(&isp_dev->mdev); > if (ret) { > dev_err_probe(dev, ret, "fail to register media device\n"); > @@ -94,6 +180,8 @@ static int isp4_capture_probe(struct platform_device *pdev) > return 0; > > err_isp4_deinit: > + isp4sd_deinit(&isp_dev->isp_subdev); > +err_pm_disable: > pm_runtime_disable(dev); > v4l2_device_unregister(&isp_dev->v4l2_dev); > err_clean_media: > @@ -108,6 +196,7 @@ static void isp4_capture_remove(struct platform_device *pdev) > struct device *dev = &pdev->dev; > > media_device_unregister(&isp_dev->mdev); > + isp4sd_deinit(&isp_dev->isp_subdev); > pm_runtime_disable(dev); > v4l2_device_unregister(&isp_dev->v4l2_dev); > media_device_cleanup(&isp_dev->mdev); > diff --git a/drivers/media/platform/amd/isp4/isp4.h b/drivers/media/platform/amd/isp4/isp4.h > index 7f2db0dfa2d9..2db6683d6d8b 100644 > --- a/drivers/media/platform/amd/isp4/isp4.h > +++ b/drivers/media/platform/amd/isp4/isp4.h > @@ -6,12 +6,15 @@ > #ifndef _ISP4_H_ > #define _ISP4_H_ > > -#include > -#include > +#include > +#include "isp4_subdev.h" > > struct isp4_device { > struct v4l2_device v4l2_dev; > + struct isp4_subdev isp_subdev; > struct media_device mdev; > }; > > +void isp4_intr_enable(struct isp4_subdev *isp_subdev, u32 index, bool enable); > + > #endif /* _ISP4_H_ */ > diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.c b/drivers/media/platform/amd/isp4/isp4_subdev.c > new file mode 100644 > index 000000000000..3a25d1fc49ce > --- /dev/null > +++ b/drivers/media/platform/amd/isp4/isp4_subdev.c > @@ -0,0 +1,975 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2025 Advanced Micro Devices, Inc. > + */ > + > +#include > +#include > + > +#include "isp4_fw_cmd_resp.h" > +#include "isp4_interface.h" > +#include "isp4.h" > + > +#define ISP4SD_MIN_BUF_CNT_BEF_START_STREAM 4 > + > +#define ISP4SD_PERFORMANCE_STATE_LOW 0 > +#define ISP4SD_PERFORMANCE_STATE_HIGH 1 > + > +/* align 32KB */ > +#define ISP4SD_META_BUF_SIZE ALIGN(sizeof(struct isp4fw_meta_info), 0x8000) > + > +#define to_isp4_subdev(v4l2_sdev) \ Virtually always variables referring to a sub-device are called either sd or subdev (or variants of these). Up to you. > + container_of(v4l2_sdev, struct isp4_subdev, sdev) > + > +static const char *isp4sd_entity_name = "amd isp4"; > + > +static const char *isp4sd_thread_name[ISP4SD_MAX_FW_RESP_STREAM_NUM] = { > + "amd_isp4_thread_global", > + "amd_isp4_thread_stream1", > +}; > + > +static void isp4sd_module_enable(struct isp4_subdev *isp_subdev, bool enable) > +{ > + if (isp_subdev->enable_gpio) { > + gpiod_set_value(isp_subdev->enable_gpio, enable ? 1 : 0); > + dev_dbg(isp_subdev->dev, "%s isp_subdev module\n", > + enable ? "enable" : "disable"); > + } > +} > + > +static int isp4sd_setup_fw_mem_pool(struct isp4_subdev *isp_subdev) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct isp4fw_cmd_send_buffer buf_type; > + struct device *dev = isp_subdev->dev; > + int ret; > + > + if (!ispif->fw_mem_pool) { > + dev_err(dev, "fail to alloc mem pool\n"); > + return -ENOMEM; > + } > + > + /* > + * 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(&buf_type, 0, sizeof(buf_type)); > + buf_type.buffer_type = BUFFER_TYPE_MEM_POOL; > + buf_type.buffer.vmid_space.bit.space = ADDR_SPACE_TYPE_GPU_VA; > + isp4if_split_addr64(ispif->fw_mem_pool->gpu_mc_addr, > + &buf_type.buffer.buf_base_a_lo, > + &buf_type.buffer.buf_base_a_hi); > + buf_type.buffer.buf_size_a = ispif->fw_mem_pool->mem_size; > + > + ret = isp4if_send_command(ispif, CMD_ID_SEND_BUFFER, > + &buf_type, sizeof(buf_type)); > + if (ret) { > + dev_err(dev, "send fw mem pool 0x%llx(%u) fail %d\n", > + ispif->fw_mem_pool->gpu_mc_addr, buf_type.buffer.buf_size_a, ret); > + return ret; > + } > + > + dev_dbg(dev, "send fw mem pool 0x%llx(%u) suc\n", > + ispif->fw_mem_pool->gpu_mc_addr, buf_type.buffer.buf_size_a); > + > + return 0; > +} > + > +static int isp4sd_set_stream_path(struct isp4_subdev *isp_subdev) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct isp4fw_cmd_set_stream_cfg cmd; > + struct device *dev = isp_subdev->dev; > + > + /* > + * 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, 0, sizeof(cmd)); You could assign assign all these in the declaration and avoid zeroing the memory explicitly at the same time. I presume possibly leaking some information from memory to the firmware in case there are holes in the struct isn't an issue. > + cmd.stream_cfg.mipi_pipe_path_cfg.isp4fw_sensor_id = SENSOR_ID_ON_MIPI0; > + cmd.stream_cfg.mipi_pipe_path_cfg.b_enable = true; > + cmd.stream_cfg.isp_pipe_path_cfg.isp_pipe_id = MIPI0_ISP_PIPELINE_ID; > + > + cmd.stream_cfg.b_enable_tnr = true; > + dev_dbg(dev, "isp4fw_sensor_id %d, pipeId 0x%x EnableTnr %u\n", > + cmd.stream_cfg.mipi_pipe_path_cfg.isp4fw_sensor_id, > + cmd.stream_cfg.isp_pipe_path_cfg.isp_pipe_id, > + cmd.stream_cfg.b_enable_tnr); > + > + return isp4if_send_command(ispif, CMD_ID_SET_STREAM_CONFIG, > + &cmd, sizeof(cmd)); > +} > + > +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 device *dev = isp_subdev->dev; > + int i; unsigned int, please. You can also declare this within the loop as you do elsewhere. Consistency is nice. > + > + /* > + * 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(&buf_type, 0, sizeof(buf_type)); > + for (i = 0; i < ISP4IF_MAX_STREAM_BUF_COUNT; i++) { > + struct isp4if_gpu_mem_info *meta_info_buf = > + isp_subdev->ispif.meta_info_buf[i]; > + int ret; > + > + if (!meta_info_buf) { > + dev_err(dev, "fail for no meta info buf(%u)\n", i); > + return -ENOMEM; > + } > + > + buf_type.buffer_type = BUFFER_TYPE_META_INFO; > + buf_type.buffer.vmid_space.bit.space = ADDR_SPACE_TYPE_GPU_VA; > + isp4if_split_addr64(meta_info_buf->gpu_mc_addr, > + &buf_type.buffer.buf_base_a_lo, > + &buf_type.buffer.buf_base_a_hi); > + buf_type.buffer.buf_size_a = meta_info_buf->mem_size; > + ret = isp4if_send_command(ispif, CMD_ID_SEND_BUFFER, > + &buf_type, sizeof(buf_type)); > + if (ret) { > + dev_err(dev, "send meta info(%u) fail\n", i); > + return ret; > + } > + } > + > + dev_dbg(dev, "send meta info suc\n"); > + return 0; > +} > + > +static bool isp4sd_get_str_out_prop(struct isp4_subdev *isp_subdev, > + struct isp4fw_image_prop *out_prop, > + struct v4l2_subdev_state *state, u32 pad) > +{ > + struct device *dev = isp_subdev->dev; > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_subdev_state_get_format(state, pad, 0); > + if (!format) { > + dev_err(dev, "fail get subdev state format\n"); > + return false; > + } > + > + switch (format->code) { > + case MEDIA_BUS_FMT_YUYV8_1_5X8: > + out_prop->image_format = IMAGE_FORMAT_NV12; > + out_prop->width = format->width; > + out_prop->height = format->height; > + out_prop->luma_pitch = format->width; > + out_prop->chroma_pitch = out_prop->width; > + break; > + case MEDIA_BUS_FMT_YUYV8_1X16: > + out_prop->image_format = IMAGE_FORMAT_YUV422INTERLEAVED; > + out_prop->width = format->width; > + out_prop->height = format->height; > + out_prop->luma_pitch = format->width * 2; > + out_prop->chroma_pitch = 0; > + break; > + default: > + dev_err(dev, "fail for bad image format:0x%x\n", > + format->code); > + return false; > + } > + > + if (!out_prop->width || !out_prop->height) > + return false; > + > + return true; > +} > + > +static int isp4sd_kickoff_stream(struct isp4_subdev *isp_subdev, u32 w, u32 h) > +{ > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + > + if (sensor_info->status == ISP4SD_START_STATUS_STARTED) > + return 0; > + > + if (sensor_info->status == ISP4SD_START_STATUS_START_FAIL) { > + dev_err(dev, "fail for previous start fail\n"); > + return -EINVAL; > + } > + > + dev_dbg(dev, "w:%u,h:%u\n", w, h); > + > + if (isp4sd_send_meta_buf(isp_subdev)) { > + dev_err(dev, "fail to send meta buf\n"); > + sensor_info->status = ISP4SD_START_STATUS_START_FAIL; > + return -EINVAL; > + } > + > + sensor_info->status = ISP4SD_START_STATUS_OFF; > + > + if (!sensor_info->start_stream_cmd_sent && > + sensor_info->buf_sent_cnt >= ISP4SD_MIN_BUF_CNT_BEF_START_STREAM) { > + int ret = isp4if_send_command(ispif, CMD_ID_START_STREAM, > + NULL, 0); > + if (ret) { > + dev_err(dev, "fail to start stream\n"); > + return ret; > + } > + > + sensor_info->start_stream_cmd_sent = true; > + } else { > + dev_dbg(dev, > + "no send START_STREAM, start_sent %u, buf_sent %u\n", > + sensor_info->start_stream_cmd_sent, > + sensor_info->buf_sent_cnt); > + } > + > + return 0; > +} > + > +static int isp4sd_setup_output(struct isp4_subdev *isp_subdev, > + struct v4l2_subdev_state *state, u32 pad) > +{ > + struct isp4sd_output_info *output_info = &isp_subdev->sensor_info.output_info; > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + 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; > + int ret; > + > + if (output_info->start_status == ISP4SD_START_STATUS_STARTED) > + return 0; > + > + if (output_info->start_status == ISP4SD_START_STATUS_START_FAIL) { > + dev_err(dev, "fail for previous start fail\n"); > + return -EINVAL; > + } > + > + /* > + * 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_prop, 0, sizeof(cmd_ch_prop)); > + cmd_ch_prop.ch = ISP_PIPE_OUT_CH_PREVIEW; > + > + 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; > + } > + > + dev_dbg(dev, "channel:%d,fmt %d,w:h=%u:%u,lp:%u,cp%u\n", > + cmd_ch_prop.ch, > + cmd_ch_prop.image_prop.image_format, > + cmd_ch_prop.image_prop.width, cmd_ch_prop.image_prop.height, > + cmd_ch_prop.image_prop.luma_pitch, > + 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)); > + 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. You should have explicit padding fields in any case and not rely on ABI in this case. > + */ > + 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"); > + return ret; > + } > + > + dev_dbg(dev, "enable channel %d\n", cmd_ch_en.ch); > + > + if (!sensor_info->start_stream_cmd_sent) { > + 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; > + } > + /* > + * sensor_info->start_stream_cmd_sent will be set to true > + * 1. in isp4sd_kickoff_stream, if app first send buffer then > + * start stream > + * 2. in isp_set_stream_buf, if app first start stream, then > + * send buffer > + * because ISP FW has the requirement, host needs to send buffer > + * before send start stream cmd > + */ > + if (sensor_info->start_stream_cmd_sent) { > + sensor_info->status = ISP4SD_START_STATUS_STARTED; > + output_info->start_status = ISP4SD_START_STATUS_STARTED; > + dev_dbg(dev, "kickoff stream suc,start cmd sent\n"); > + } > + } else { > + dev_dbg(dev, "stream running, no need kickoff\n"); > + output_info->start_status = ISP4SD_START_STATUS_STARTED; > + } > + > + dev_dbg(dev, "setup output suc\n"); > + return 0; > +} > + > +static int isp4sd_init_stream(struct isp4_subdev *isp_subdev) > +{ > + struct device *dev = isp_subdev->dev; > + int ret; > + > + ret = isp4sd_setup_fw_mem_pool(isp_subdev); > + if (ret) { > + dev_err(dev, "fail to setup fw mem pool\n"); > + return ret; > + } > + > + ret = isp4sd_set_stream_path(isp_subdev); > + if (ret) { > + dev_err(dev, "fail to setup stream path\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void isp4sd_uninit_stream(struct isp4_subdev *isp_subdev, > + struct v4l2_subdev_state *state, u32 pad) > +{ > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + struct isp4sd_output_info *output_info = &sensor_info->output_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_subdev_state_get_format(state, pad, 0); > + if (!format) { > + dev_err(isp_subdev->dev, "fail to get v4l2 format\n"); > + } else { > + memset(format, 0, sizeof(*format)); > + format->code = MEDIA_BUS_FMT_YUYV8_1_5X8; > + } > + > + isp4if_clear_bufq(ispif); > + isp4if_clear_cmdq(ispif); > + > + sensor_info->start_stream_cmd_sent = false; > + sensor_info->buf_sent_cnt = 0; > + > + sensor_info->status = ISP4SD_START_STATUS_OFF; > + output_info->start_status = ISP4SD_START_STATUS_OFF; > +} > + > +static void isp4sd_fw_resp_cmd_done(struct isp4_subdev *isp_subdev, > + enum isp4if_stream_id stream_id, > + struct isp4fw_resp_cmd_done *para) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct isp4if_cmd_element *ele = > + isp4if_rm_cmd_from_cmdq(ispif, para->cmd_seq_num, para->cmd_id); > + struct device *dev = isp_subdev->dev; > + > + dev_dbg(dev, "stream %d,cmd (0x%08x)(%d),seq %u, ele %p\n", > + stream_id, > + para->cmd_id, para->cmd_status, para->cmd_seq_num, > + ele); > + > + if (ele) { > + complete(&ele->cmd_done); > + if (atomic_dec_and_test(&ele->refcnt)) > + kfree(ele); > + } > +} > + > +static struct isp4fw_meta_info *isp4sd_get_meta_by_mc(struct isp4_subdev *isp_subdev, > + u64 mc) > +{ > + for (int i = 0; i < ISP4IF_MAX_STREAM_BUF_COUNT; i++) { unsigned int, please. Similarly for the rest. > + struct isp4if_gpu_mem_info *meta_info_buf = > + isp_subdev->ispif.meta_info_buf[i]; > + > + if (meta_info_buf->gpu_mc_addr == mc) > + return meta_info_buf->sys_addr; > + } > + > + return NULL; > +} > + > +static void isp4sd_send_meta_info(struct isp4_subdev *isp_subdev, > + u64 meta_info_mc) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct isp4fw_cmd_send_buffer buf_type; > + struct device *dev = isp_subdev->dev; > + > + if (isp_subdev->sensor_info.status != ISP4SD_START_STATUS_STARTED) { > + dev_warn(dev, "not working status %i, meta_info 0x%llx\n", > + isp_subdev->sensor_info.status, meta_info_mc); > + return; > + } > + > + /* > + * 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(&buf_type, 0, sizeof(buf_type)); > + buf_type.buffer_type = BUFFER_TYPE_META_INFO; > + 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; > + > + 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 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; > + > + mc = isp4if_join_addr64(para->package_addr_lo, para->package_addr_hi); > + meta = isp4sd_get_meta_by_mc(isp_subdev, mc); > + if (!meta) { > + dev_err(dev, "fail to get meta from mc %llx\n", mc); > + return; > + } > + > + dev_dbg(dev, "ts:%llu,streamId:%d,poc:%u,preview_en:%u(%i)\n", > + ktime_get_ns(), stream_id, meta->poc, > + meta->preview.enabled, > + meta->preview.status); > + > + 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) > + 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\n", > + meta->preview.status); > + } > + > + if (isp_subdev->sensor_info.status == ISP4SD_START_STATUS_STARTED) > + isp4sd_send_meta_info(isp_subdev, mc); > + > + dev_dbg(dev, "stream_id:%d, status:%d\n", stream_id, > + isp_subdev->sensor_info.status); > +} > + > +static void isp4sd_fw_resp_func(struct isp4_subdev *isp_subdev, > + enum isp4if_stream_id stream_id) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + struct isp4fw_resp resp; > + > + while (true) { > + if (isp4if_f2h_resp(ispif, stream_id, &resp)) { > + /* Re-enable the interrupt */ > + isp4_intr_enable(isp_subdev, stream_id, true); > + /* > + * Recheck to see if there is a new response. > + * To ensure that an in-flight interrupt is not lost, > + * enabling the interrupt must occur _before_ checking > + * for a new response, hence a memory barrier is needed. > + * Disable the interrupt again if there was a new response. > + */ > + mb(); > + if (likely(isp4if_f2h_resp(ispif, stream_id, &resp))) > + break; > + > + isp4_intr_enable(isp_subdev, stream_id, false); > + } > + > + switch (resp.resp_id) { > + case RESP_ID_CMD_DONE: > + isp4sd_fw_resp_cmd_done(isp_subdev, stream_id, > + &resp.param.cmd_done); > + break; > + case RESP_ID_NOTI_FRAME_DONE: > + isp4sd_fw_resp_frame_done(isp_subdev, stream_id, > + &resp.param.frame_done); > + break; > + default: > + dev_err(dev, "-><- fail respid (0x%x)\n", resp.resp_id); > + break; > + } > + } > +} > + > +static s32 isp4sd_fw_resp_thread(void *context) > +{ > + struct isp4_subdev_thread_param *para = context; > + struct isp4_subdev *isp_subdev = para->isp_subdev; > + struct isp4sd_thread_handler *thread_ctx = > + &isp_subdev->fw_resp_thread[para->idx]; > + struct device *dev = isp_subdev->dev; > + > + dev_dbg(dev, "[%u] fw resp thread started\n", para->idx); > + while (true) { > + wait_event_interruptible(thread_ctx->waitq, thread_ctx->resp_ready); > + thread_ctx->resp_ready = false; > + > + if (kthread_should_stop()) { > + dev_dbg(dev, "[%u] fw resp thread quit\n", para->idx); > + break; > + } > + > + isp4sd_fw_resp_func(isp_subdev, para->idx); > + } > + > + return 0; > +} > + > +static int isp4sd_stop_resp_proc_threads(struct isp4_subdev *isp_subdev) > +{ > + int i; > + > + for (i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++) { > + struct isp4sd_thread_handler *thread_ctx = > + &isp_subdev->fw_resp_thread[i]; > + > + if (thread_ctx->thread) { > + kthread_stop(thread_ctx->thread); > + thread_ctx->thread = NULL; > + } > + } > + > + return 0; > +} > + > +static int isp4sd_start_resp_proc_threads(struct isp4_subdev *isp_subdev) > +{ > + struct device *dev = isp_subdev->dev; > + int i; > + > + for (i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++) { > + struct isp4sd_thread_handler *thread_ctx = &isp_subdev->fw_resp_thread[i]; > + > + isp_subdev->isp_resp_para[i].idx = i; > + isp_subdev->isp_resp_para[i].isp_subdev = isp_subdev; > + init_waitqueue_head(&thread_ctx->waitq); > + thread_ctx->resp_ready = false; > + > + thread_ctx->thread = kthread_run(isp4sd_fw_resp_thread, > + &isp_subdev->isp_resp_para[i], > + isp4sd_thread_name[i]); > + if (IS_ERR(thread_ctx->thread)) { > + dev_err(dev, "create thread [%d] fail\n", i); > + thread_ctx->thread = NULL; > + isp4sd_stop_resp_proc_threads(isp_subdev); > + return -EINVAL; > + } > + } > + > + return 0; > +} > + > +static int isp4sd_pwroff_and_deinit(struct isp4_subdev *isp_subdev) > +{ > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + unsigned int perf_state = ISP4SD_PERFORMANCE_STATE_LOW; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + int ret; > + > + if (sensor_info->status == ISP4SD_START_STATUS_STARTED) { > + dev_err(dev, "fail for stream still running\n"); > + return -EINVAL; > + } > + > + sensor_info->status = ISP4SD_START_STATUS_OFF; > + > + if (isp_subdev->irq_enabled) { > + for (int i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++) > + disable_irq(isp_subdev->irq[i]); > + isp_subdev->irq_enabled = false; > + } > + > + isp4sd_stop_resp_proc_threads(isp_subdev); > + dev_dbg(dev, "isp_subdev stop resp proc streads suc"); > + > + isp4if_stop(ispif); > + > + ret = dev_pm_genpd_set_performance_state(dev, perf_state); > + if (ret) > + dev_err(dev, "fail to set isp_subdev performance state %u,ret %d\n", > + perf_state, ret); > + > + /* hold ccpu reset */ > + isp4hw_wreg(isp_subdev->mmio, ISP_SOFT_RESET, 0); > + isp4hw_wreg(isp_subdev->mmio, ISP_POWER_STATUS, 0); > + ret = pm_runtime_put_sync(dev); > + if (ret) > + dev_err(dev, "power off isp_subdev fail %d\n", ret); > + else > + dev_dbg(dev, "power off isp_subdev suc\n"); > + > + ispif->status = ISP4IF_STATUS_PWR_OFF; > + isp4if_clear_cmdq(ispif); > + isp4sd_module_enable(isp_subdev, false); > + > + /* > + * When opening the camera, isp4sd_module_enable(isp_subdev, true) is called. > + * Hardware requires at least a 20ms delay between disabling and enabling the module, > + * so a sleep is added to ensure ISP stability during quick reopen scenarios. > + */ > + msleep(20); > + > + return 0; > +} > + > +static int isp4sd_pwron_and_init(struct isp4_subdev *isp_subdev) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + int ret; > + > + if (ispif->status == ISP4IF_STATUS_FW_RUNNING) { > + dev_dbg(dev, "camera already opened, do nothing\n"); > + return 0; > + } > + > + isp4sd_module_enable(isp_subdev, true); > + > + if (ispif->status < ISP4IF_STATUS_PWR_ON) { > + unsigned int perf_state = ISP4SD_PERFORMANCE_STATE_HIGH; > + > + ret = pm_runtime_resume_and_get(dev); > + if (ret) { > + dev_err(dev, "fail to power on isp_subdev ret %d\n", > + ret); > + goto err_deinit; > + } > + > + /* ISPPG ISP Power Status */ > + isp4hw_wreg(isp_subdev->mmio, ISP_POWER_STATUS, 0x7FF); > + ret = dev_pm_genpd_set_performance_state(dev, perf_state); > + if (ret) { > + dev_err(dev, > + "fail to set performance state %u, ret %d\n", > + perf_state, ret); > + goto err_deinit; > + } > + > + ispif->status = ISP4IF_STATUS_PWR_ON; > + } > + > + isp_subdev->sensor_info.start_stream_cmd_sent = false; > + isp_subdev->sensor_info.buf_sent_cnt = 0; > + > + ret = isp4if_start(ispif); > + if (ret) { > + dev_err(dev, "fail to start isp_subdev interface\n"); > + goto err_deinit; > + } > + > + if (isp4sd_start_resp_proc_threads(isp_subdev)) { > + dev_err(dev, "isp_start_resp_proc_threads fail"); > + goto err_deinit; > + } > + > + dev_dbg(dev, "create resp threads ok"); > + > + for (int i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++) > + enable_irq(isp_subdev->irq[i]); > + isp_subdev->irq_enabled = true; > + > + return 0; > +err_deinit: > + isp4sd_pwroff_and_deinit(isp_subdev); > + return -EINVAL; > +} > + > +static int isp4sd_stop_stream(struct isp4_subdev *isp_subdev, > + struct v4l2_subdev_state *state, u32 pad) > +{ > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + struct isp4sd_output_info *output_info = &sensor_info->output_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + > + guard(mutex)(&isp_subdev->ops_mutex); > + dev_dbg(dev, "status %i\n", output_info->start_status); > + > + if (output_info->start_status == ISP4SD_START_STATUS_STARTED) { > + struct isp4fw_cmd_enable_out_ch cmd_ch_disable; > + int 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_disable, 0, sizeof(cmd_ch_disable)); > + cmd_ch_disable.ch = ISP_PIPE_OUT_CH_PREVIEW; > + /* `cmd_ch_disable.is_enable` is already false */ > + ret = isp4if_send_command_sync(ispif, CMD_ID_ENABLE_OUT_CHAN, > + &cmd_ch_disable, > + 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); > + if (ret) > + dev_err(dev, "fail to stop steam\n"); > + else > + dev_dbg(dev, "wait stop stream suc\n"); > + } > + > + isp4sd_uninit_stream(isp_subdev, state, pad); > + > + /* > + * Return success to ensure the stop process proceeds, > + * and disregard any errors since they are not fatal. > + */ > + return 0; > +} > + > +static int isp4sd_start_stream(struct isp4_subdev *isp_subdev, > + struct v4l2_subdev_state *state, u32 pad) > +{ > + struct isp4sd_output_info *output_info = > + &isp_subdev->sensor_info.output_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = isp_subdev->dev; > + int ret; > + > + guard(mutex)(&isp_subdev->ops_mutex); > + > + if (ispif->status != ISP4IF_STATUS_FW_RUNNING) { > + dev_err(dev, "fail, bad fsm %d", ispif->status); > + return -EINVAL; > + } > + > + switch (output_info->start_status) { > + case ISP4SD_START_STATUS_OFF: > + break; > + case ISP4SD_START_STATUS_STARTED: > + dev_dbg(dev, "stream already started, do nothing\n"); > + return 0; > + case ISP4SD_START_STATUS_START_FAIL: > + dev_err(dev, "stream previously failed to start\n"); > + return -EINVAL; > + } > + > + ret = isp4sd_init_stream(isp_subdev); > + if (ret) { > + dev_err(dev, "fail to init isp_subdev stream\n"); > + goto err_stop_stream; > + } > + > + ret = isp4sd_setup_output(isp_subdev, state, pad); > + if (ret) { > + dev_err(dev, "fail to setup output\n"); > + goto err_stop_stream; > + } > + > + return 0; > + > +err_stop_stream: > + isp4sd_stop_stream(isp_subdev, state, pad); > + return ret; > +} > + > +static int isp4sd_set_power(struct v4l2_subdev *sd, int on) > +{ > + struct isp4_subdev *isp_subdev = to_isp4_subdev(sd); > + > + guard(mutex)(&isp_subdev->ops_mutex); > + if (on) > + return isp4sd_pwron_and_init(isp_subdev); > + else > + return isp4sd_pwroff_and_deinit(isp_subdev); The s_power() callback is deprecated, please rely on runtime PM. > +} > + > +static const struct v4l2_subdev_core_ops isp4sd_core_ops = { > + .s_power = isp4sd_set_power, > +}; > + > +static const struct v4l2_subdev_video_ops isp4sd_video_ops = { > + .s_stream = v4l2_subdev_s_stream_helper, > +}; > + > +static int isp4sd_set_pad_format(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_format *fmt) > +{ > + struct isp4sd_output_info *steam_info = Steam? > + &(to_isp4_subdev(sd)->sensor_info.output_info); > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_subdev_state_get_format(sd_state, fmt->pad); > + > + if (!format) { > + dev_err(sd->dev, "fail to get state format\n"); > + return -EINVAL; > + } > + > + *format = fmt->format; > + switch (format->code) { > + case MEDIA_BUS_FMT_YUYV8_1_5X8: > + steam_info->image_size = format->width * format->height * 3 / 2; > + break; > + case MEDIA_BUS_FMT_YUYV8_1X16: > + steam_info->image_size = format->width * format->height * 2; > + break; > + default: > + steam_info->image_size = 0; Please stick to formats the driver supports i.e. make either of the above the default. > + break; > + } > + if (!steam_info->image_size) { > + dev_err(sd->dev, "fail set pad format,code 0x%x,width %u, height %u\n", > + format->code, format->width, format->height); > + return -EINVAL; > + } > + > + dev_dbg(sd->dev, "set pad format suc, code:%x w:%u h:%u size:%u\n", > + format->code, format->width, format->height, steam_info->image_size); > + > + return 0; > +} > + > +static int isp4sd_enable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct isp4_subdev *isp_subdev = to_isp4_subdev(sd); > + > + return isp4sd_start_stream(isp_subdev, state, pad); > +} > + > +static int isp4sd_disable_streams(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, u32 pad, > + u64 streams_mask) > +{ > + struct isp4_subdev *isp_subdev = to_isp4_subdev(sd); > + > + return isp4sd_stop_stream(isp_subdev, state, pad); > +} > + > +static const struct v4l2_subdev_pad_ops isp4sd_pad_ops = { > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = isp4sd_set_pad_format, > + .enable_streams = isp4sd_enable_streams, > + .disable_streams = isp4sd_disable_streams, > +}; > + > +static const struct v4l2_subdev_ops isp4sd_subdev_ops = { > + .core = &isp4sd_core_ops, > + .video = &isp4sd_video_ops, > + .pad = &isp4sd_pad_ops, > +}; > + > +static int isp4sd_sdev_link_validate(struct media_link *link) > +{ > + return 0; Uh-oh. What is actually being configured via the sub-device? There is no device node either, is there? Are there plans for future developments, apart from possibly making the ISP and the sensor controllable by the host? > +} > + > +static const struct media_entity_operations isp4sd_sdev_ent_ops = { > + .link_validate = isp4sd_sdev_link_validate, > +}; > + > +int isp4sd_init(struct isp4_subdev *isp_subdev, struct v4l2_device *v4l2_dev, > + int irq[ISP4SD_MAX_FW_RESP_STREAM_NUM]) > +{ > + struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info; > + struct isp4_interface *ispif = &isp_subdev->ispif; > + struct device *dev = v4l2_dev->dev; > + int ret; > + > + isp_subdev->dev = dev; > + v4l2_subdev_init(&isp_subdev->sdev, &isp4sd_subdev_ops); > + isp_subdev->sdev.owner = THIS_MODULE; > + isp_subdev->sdev.dev = dev; > + snprintf(isp_subdev->sdev.name, sizeof(isp_subdev->sdev.name), "%s", > + dev_name(dev)); > + > + isp_subdev->sdev.entity.name = isp4sd_entity_name; > + isp_subdev->sdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_ISP; > + isp_subdev->sdev.entity.ops = &isp4sd_sdev_ent_ops; > + isp_subdev->sdev_pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&isp_subdev->sdev.entity, 1, > + &isp_subdev->sdev_pad); > + if (ret) { > + dev_err(dev, "fail to init isp4 subdev entity pad %d\n", ret); > + return ret; > + } > + > + ret = v4l2_subdev_init_finalize(&isp_subdev->sdev); > + if (ret < 0) { > + dev_err(dev, "fail to init finalize isp4 subdev %d\n", > + ret); > + return ret; > + } > + > + ret = v4l2_device_register_subdev(v4l2_dev, &isp_subdev->sdev); > + if (ret) { > + dev_err(dev, "fail to register isp4 subdev to V4L2 device %d\n", ret); > + goto err_media_clean_up; > + } > + > + isp4if_init(ispif, dev, isp_subdev->mmio); > + > + mutex_init(&isp_subdev->ops_mutex); > + sensor_info->status = ISP4SD_START_STATUS_OFF; > + > + /* create ISP enable gpio control */ > + isp_subdev->enable_gpio = devm_gpiod_get(isp_subdev->dev, > + "enable_isp", > + GPIOD_OUT_LOW); > + if (IS_ERR(isp_subdev->enable_gpio)) { > + ret = PTR_ERR(isp_subdev->enable_gpio); > + dev_err(dev, "fail to get gpiod %d\n", ret); > + goto err_subdev_unreg; > + } > + > + for (int i = 0; i < ISP4SD_MAX_FW_RESP_STREAM_NUM; i++) > + isp_subdev->irq[i] = irq[i]; > + > + isp_subdev->host2fw_seq_num = 1; > + ispif->status = ISP4IF_STATUS_PWR_OFF; > + > + return 0; > + > +err_subdev_unreg: > + v4l2_device_unregister_subdev(&isp_subdev->sdev); > +err_media_clean_up: > + v4l2_subdev_cleanup(&isp_subdev->sdev); > + media_entity_cleanup(&isp_subdev->sdev.entity); > + return ret; > +} > + > +void isp4sd_deinit(struct isp4_subdev *isp_subdev) > +{ > + struct isp4_interface *ispif = &isp_subdev->ispif; > + > + v4l2_device_unregister_subdev(&isp_subdev->sdev); > + media_entity_cleanup(&isp_subdev->sdev.entity); > + isp4if_deinit(ispif); > + isp4sd_module_enable(isp_subdev, false); > + > + ispif->status = ISP4IF_STATUS_PWR_OFF; > +} > diff --git a/drivers/media/platform/amd/isp4/isp4_subdev.h b/drivers/media/platform/amd/isp4/isp4_subdev.h > new file mode 100644 > index 000000000000..70007b9de3f3 > --- /dev/null > +++ b/drivers/media/platform/amd/isp4/isp4_subdev.h > @@ -0,0 +1,124 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (C) 2025 Advanced Micro Devices, Inc. > + */ > + > +#ifndef _ISP4_SUBDEV_H_ > +#define _ISP4_SUBDEV_H_ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "isp4_fw_cmd_resp.h" > +#include "isp4_hw_reg.h" > +#include "isp4_interface.h" > + > +/* > + * one is for none sesnor specefic response which is not used now > + * another is for sensor specific response > + */ > +#define ISP4SD_MAX_FW_RESP_STREAM_NUM 2 > + > +/* > + * cmd used to register frame done callback, parameter is > + * struct isp4sd_register_framedone_cb_param * > + * when a image buffer is filled by ISP, ISP will call the registered callback. > + * callback func prototype is isp4sd_framedone_cb, cb_ctx can be anything > + * provided by caller which will be provided back as the first parameter of the > + * callback function. > + * both cb_func and cb_ctx are provide by caller, set cb_func to NULL to > + * unregister the callback These comments would benefit from grammatical and spelling corrections. Same below. > + */ > + > +/* used to indicate the ISP status */ > +enum isp4sd_status { > + ISP4SD_STATUS_PWR_OFF, > + ISP4SD_STATUS_PWR_ON, > + ISP4SD_STATUS_FW_RUNNING, > + ISP4SD_STATUS_MAX > +}; > + > +/* used to indicate the status of sensor, output stream */ > +enum isp4sd_start_status { > + ISP4SD_START_STATUS_OFF, > + ISP4SD_START_STATUS_STARTED, > + ISP4SD_START_STATUS_START_FAIL, > +}; > + > +struct isp4sd_img_buf_node { > + struct list_head node; > + struct isp4if_img_buf_info buf_info; > +}; > + > +/* this is isp output after processing bayer raw input from sensor */ > +struct isp4sd_output_info { > + enum isp4sd_start_status start_status; > + u32 image_size; > +}; > + > +/* > + * This struct represents the sensor info which is input or source of ISP, > + * status is the sensor status > + * output_info is the isp output info after ISP processing the sensor input, > + * start_stream_cmd_sent mean if CMD_ID_START_STREAM has sent to fw. > + * buf_sent_cnt is buffer count app has sent to receive the images > + */ > +struct isp4sd_sensor_info { > + struct isp4sd_output_info output_info; > + enum isp4sd_start_status status; > + bool start_stream_cmd_sent; > + u32 buf_sent_cnt; > +}; > + > +/* > + * Thread created by driver to receive fw response > + * thread will be wakeup by fw to driver response interrupt > + */ > +struct isp4sd_thread_handler { > + struct task_struct *thread; > + wait_queue_head_t waitq; > + bool resp_ready; > +}; > + > +struct isp4_subdev_thread_param { > + u32 idx; > + struct isp4_subdev *isp_subdev; > +}; > + > +struct isp4_subdev { > + struct v4l2_subdev sdev; > + struct isp4_interface ispif; > + > + struct media_pad sdev_pad; > + > + enum isp4sd_status isp_status; > + struct mutex ops_mutex; /* ops_mutex */ > + > + /* Used to store fw cmds sent to FW whose response driver needs to wait for */ > + struct isp4sd_thread_handler fw_resp_thread[ISP4SD_MAX_FW_RESP_STREAM_NUM]; > + > + u32 host2fw_seq_num; > + > + struct isp4sd_sensor_info sensor_info; > + > + /* gpio descriptor */ > + struct gpio_desc *enable_gpio; > + struct device *dev; > + void __iomem *mmio; > + struct isp4_subdev_thread_param isp_resp_para[ISP4SD_MAX_FW_RESP_STREAM_NUM]; > + int irq[ISP4SD_MAX_FW_RESP_STREAM_NUM]; > + bool irq_enabled; > + /* spin lock to access ISP_SYS_INT0_EN exclusively */ > + spinlock_t irq_lock; > +}; > + > +int isp4sd_init(struct isp4_subdev *isp_subdev, struct v4l2_device *v4l2_dev, > + int irq[ISP4SD_MAX_FW_RESP_STREAM_NUM]); > +void isp4sd_deinit(struct isp4_subdev *isp_subdev); > + > +#endif /* _ISP4_SUBDEV_H_ */ -- Regards, Sakari Ailus