Re: [PATCH v4 4/7] media: platform: amd: isp4 subdev and firmware loading handling added

["Du, Bin" <bin.du@amd.com>]

Thanks, Sultan. sorry for the delayed response due to the long public 
holiday here.

On 10/1/2025 3:24 PM, Sultan Alsawaf wrote:
> On Tue, Sep 30, 2025 at 03:30:49PM +0800, Du, Bin wrote:
>> On 9/23/2025 3:23 PM, Sultan Alsawaf wrote:
>>> On Thu, Sep 11, 2025 at 06:08:44PM +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: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Tested-by: Alexey Zagorodnikov <xglooom@gmail.com>
>>>
>>> [snip]
>>>
>>>> +++ b/drivers/media/platform/amd/isp4/isp4.c
>>>> @@ -5,13 +5,19 @@
>>>>    #include <linux/pm_runtime.h>
>>>>    #include <linux/vmalloc.h>
>>>> +
>>>> +#include <media/v4l2-fwnode.h>
>>>>    #include <media/v4l2-ioctl.h>
>>>>    #include "isp4.h"
>>>> -
>>>> -#define VIDEO_BUF_NUM 5
>>>> +#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_WPT10_INT_MASK | \
>>>> +	 ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT11_INT_MASK | \
>>>> +	 ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT12_INT_MASK)
>>>>    /* interrupt num */
>>>>    static const u32 isp4_ringbuf_interrupt_num[] = {
>>>> @@ -21,19 +27,95 @@ static const u32 isp4_ringbuf_interrupt_num[] = {
>>>>    	4, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT12 */
>>>>    };
>>>> -#define to_isp4_device(dev) \
>>>> -	((struct isp4_device *)container_of(dev, struct isp4_device, v4l2_dev))
>>>> +#define to_isp4_device(dev) container_of(dev, struct isp4_device, v4l2_dev)
>>>> +
>>>> +static void isp4_wake_up_resp_thread(struct isp4_subdev *isp, u32 index)
>>>> +{
>>>> +	if (isp && index < ISP4SD_MAX_FW_RESP_STREAM_NUM) {
>>>> +		struct isp4sd_thread_handler *thread_ctx =
>>>> +				&isp->fw_resp_thread[index];
>>>> +
>>>> +		thread_ctx->wq_cond = 1;
>>>> +		wake_up_interruptible(&thread_ctx->waitq);
>>>> +	}
>>>> +}
>>>> +
>>>> +static void isp4_resp_interrupt_notify(struct isp4_subdev *isp, u32 intr_status)
>>>> +{
>>>> +	bool wake = (isp->ispif.status == ISP4IF_STATUS_FW_RUNNING);
>>>> +
>>>> +	u32 intr_ack = 0;
>>>> +
>>>> +	/* global response */
>>>> +	if (intr_status &
>>>> +	    ISP_SYS_INT0_STATUS__SYS_INT_RINGBUFFER_WPT12_INT_MASK) {
>>>> +		if (wake)
>>>> +			isp4_wake_up_resp_thread(isp, 0);
>>>> +
>>>> +		intr_ack |= ISP_SYS_INT0_ACK__SYS_INT_RINGBUFFER_WPT12_ACK_MASK;
>>>
>>> The INT_MASKs and ACK_MASKs are the same; perhaps the ACK_MASKs can just be
>>> removed so you can just write intr_status to ISP_SYS_INT0_ACK instead?
>>>
>>
>> These macro definitions are automatically generated from the IP design by
>> the hardware team. INT_MASK and ACK_MASK represent specific bits in
>> different registers—the status and acknowledgment registers, respectively.
>> While their values are currently the same, they could differ depending on
>> the IP design. I prefer to keep both definitions to maintain clarity.
> 
> Sure, no problem.
> 
>>>> +
>>>> +	/* clear ISP_SYS interrupts */
>>>> +	isp4hw_wreg(ISP4_GET_ISP_REG_BASE(isp), ISP_SYS_INT0_ACK, intr_ack);
>>>> +}
>>>>    static irqreturn_t isp4_irq_handler(int irq, void *arg)
>>>>    {
>>>> +	struct isp4_device *isp_dev = dev_get_drvdata(arg);
>>>
>>> This is technically a data race because setting drvdata and reading drvdata do
>>> not use WRITE_ONCE() and READ_ONCE(), respectively. And enabling the IRQ before
>>> the handler is allowed to do anything is why that `if (!isp_dev)` check exists,
>>> because that is another race.
>>>
>>> Instead, pass the isp_dev pointer through the private pointer of
>>> devm_request_irq() and add IRQ_NOAUTOEN so the IRQ is enabled by default. Then,
>>> when it is safe for the IRQ to run, enable it with enable_irq().
>>>
>>> That way you can delete the `if (!isp_dev)` check and resolve the two races.
>>>
>>
>> Good deep insight, suppose you mean use IRQ_NOAUTOEN to make irq default
>> disabled. Sure, will add support to dynamically enable/disable IRQ during
>> camera open/close and remove unnecessary check.
> 
> Sorry for the typo, meant to say default disabled indeed. :)
> 

No worries, got your point :)

>>>> +	u32 r1;
>>>> +
>>>> +	if (!isp_dev)
>>>> +		goto error_drv_data;
>>>> +
>>>> +	isp = &isp_dev->isp_sdev;
>>>> +	/* check ISP_SYS interrupts status */
>>>> +	r1 = isp4hw_rreg(ISP4_GET_ISP_REG_BASE(isp), ISP_SYS_INT0_STATUS);
>>>> +
>>>> +	isp_sys_irq_status = r1 & ISP4_FW_RESP_RB_IRQ_STATUS_MASK;
>>>
>>> There are four IRQs (one for each stream) but any one of the IRQs can result in
>>> a notification for _all_ streams. Each IRQ should only do the work of its own
>>> stream.
>>>
>>> You can do this by passing devm_request_irq() a private pointer to indicate the
>>> mapping between a stream and its IRQ, so that isp4_irq_handler() can know which
>>> stream it should look at.
>>>
>>
>> Will do optimization to remove unused IRQs and keep only necessary ones
>> (reduce from 4 to 2), actually an IRQ won't result in notification to all
>> streams, please check the implementation of isp4_resp_interrupt_notify, it
>> will only wake up IRQ corresponding stream processing thread.
> 
> What I mean is that the IRQ for one stream can wake a different stream if it is
> also ready at the same time according to the interrupt status register.
> 

Yes, you are correct, besides its own stream, the IRQ may wake a 
different stream if it is ready too in the IRQ status register. But i 
believe the shared irq handler can improve the performance without 
negative effects. The peseudo code of isp4_irq_handler works like this 
(take your below example)
irqreturn_t isp4_irq_handler(...)
{
	status = read_irq_status();
	if(status & WPT9)
		isp4_wake_up_resp_thread(isp, 1);
	if(status & WPT10)
		isp4_wake_up_resp_thread(isp, 2)
         ack_irq_status(status);
	return IRQ_HANDLED;
}
Which means the first isp4_irq_handler can process all IRQs at that 
time. For the second isp4_irq_handler, because the irq status is cleared 
by the first isp4_irq_handler, it just does nothing and quit. So even if 
isp4_irq_handler doen't know exactly which IRQ triggers it, there's no 
harm as far as I can tell, not sure if I missed something.

> Assume we have ISP_IRQ 0 and 1 for streams 1 (WPT9) and 2 (WPT10), respectively.
> Consider the following sequence of events:
> 
>      ISP_IRQ0 (WPT9)			ISP_IRQ1 (WPT10)
>      ---------------			----------------
>      <interrupt fires>			<interrupt fires>
>      isp4_irq_handler()			isp4_irq_handler()
>      isp_sys_irq_status = WPT9|WPT10	isp_sys_irq_status = WPT9|WPT10
> 
>      isp4_wake_up_resp_thread(isp, 1)	isp4_wake_up_resp_thread(isp, 1)
> 					// ^ woke up WPT9 from WPT10 IRQ!
> 
>      isp4_wake_up_resp_thread(isp, 2)	isp4_wake_up_resp_thread(isp, 2)
>      // ^ woke up WPT10 from WPT9 IRQ!
> 
> The problem is that isp4_irq_handler() doesn't know which IRQ triggered the call
> into isp4_irq_handler().
> 
>>>> +static int isp4sd_init_meta_buf(struct isp4_subdev *isp_subdev)
>>>> +{
>>>> +	struct isp4sd_sensor_info *sensor_info = &isp_subdev->sensor_info;
>>>> +	struct isp4_interface *ispif = &isp_subdev->ispif;
>>>> +	struct device *dev = isp_subdev->dev;
>>>> +	u32 i;
>>>> +
>>>> +	for (i = 0; i < ISP4IF_MAX_STREAM_BUF_COUNT; i++) {
>>>> +		if (!sensor_info->meta_info_buf[i]) {
>>>> +			sensor_info->meta_info_buf[i] = ispif->metainfo_buf_pool[i];
>>>> +			if (!sensor_info->meta_info_buf[i]) {
>>>> +				dev_err(dev, "invalid %u meta_info_buf fail\n", i);
>>>> +				return -ENOMEM;
>>>> +			}
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> What is the point of metainfo_buf_pool? Especially since metainfo_buf_pool[i] is
>>> not set to NULL after this "allocation" occurs.
>>>
>>> I think isp4sd_init_meta_buf() and metainfo_buf_pool are unnecessary and can be
>>> factored out.
>>>
>>
>> I suppose you mean meta_info_buf, will remove it together with
>> isp4sd_init_meta_buf() and use metainfo_buf_pool from ispif directly which
>> is vital for ISP FW to carry response info.
> 
> I was thinking that metainfo_buf_pool could be renamed to meta_info_buf and then
> the old meta_info_buf could be deleted. Same result either way. :)
> 

Sure, will do that in the next version.

>>>> +	init_waitqueue_head(&thread_ctx->waitq);
>>>> +	timeout = msecs_to_jiffies(ISP4SD_WAIT_RESP_IRQ_TIMEOUT);
>>>> +
>>>> +	dev_dbg(dev, "[%u] started\n", para->idx);
>>>> +
>>>> +	while (true) {
>>>> +		wait_event_interruptible_timeout(thread_ctx->waitq,
>>>> +						 thread_ctx->wq_cond != 0,
>>>> +						 timeout);
>>>
>>> Why is there a timeout? What does the timeout even do since the return value of
>>> wait_event_interruptible_timeout() is not checked? Doesn't that mean that once
>>> the timeout is hit, isp4sd_fw_resp_func() will be called for nothing?
>>>
>>> I observe that most of the time spent by these kthreads is due to the constant
>>> wake-ups from the very short 5 ms timeout. This is bad for energy efficiency and
>>> creates needless overhead.
>>>
>>
>> Good catch, previouly before IRQ is really enabled, this is to make sure ISP
>> can work normally even for 120fps sensor, since now IRQ is enabled, we can
>> increase the timeout value to like 200ms to avoid the unwanted timeout
>> caused wake-ups.
> 
> What should the kthread do when there is a timeout though? Is the timeout
> necessary to detect when FW is no longer responding? If so, shouldn't there be
> error handling?
> 
> If the timeout isn't used to check for error then I think it should be removed.
> 

Yes, good suggestion, will remove the timeout in the next version

>>>> +		thread_ctx->wq_cond = 0;
>>>> +
>>>> +		if (kthread_should_stop()) {
>>>> +			dev_dbg(dev, "[%u] quit\n", para->idx);
>>>> +			break;
>>>> +		}
>>>> +
>>>> +		guard(mutex)(&thread_ctx->mutex);
>>>> +		isp4sd_fw_resp_func(isp_subdev, stream_id);
>>>> +	}
>>>> +
>>>> +	mutex_destroy(&thread_ctx->mutex);
>>>> +
>>>> +	return 0;
>>>> +}
> 
> [snip]
> 
>>>> +
>>>> +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;
>>>> +	u32 cnt;
>>>> +	int ret;
>>>> +
>>>> +	mutex_lock(&isp_subdev->ops_mutex);
>>>> +
>>>> +	if (sensor_info->status == ISP4SD_START_STATUS_STARTED) {
>>>> +		dev_err(dev, "fail for stream still running\n");
>>>> +		mutex_unlock(&isp_subdev->ops_mutex);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	sensor_info->status = ISP4SD_START_STATUS_NOT_START;
>>>> +	cnt = isp4sd_get_started_stream_count(isp_subdev);
>>>> +	if (cnt > 0) {
>>>> +		dev_dbg(dev, "no need power off isp_subdev\n");
>>>> +		mutex_unlock(&isp_subdev->ops_mutex);
>>>> +		return 0;
>>>> +	}
>>>> +
>>>> +	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);
>>>> +	isp4sd_stop_resp_proc_threads(isp_subdev);
>>>> +	dev_dbg(dev, "isp_subdev stop resp proc streads suc");
>>>> +	/* hold ccpu reset */
>>>> +	isp4hw_wreg(isp_subdev->mmio, ISP_SOFT_RESET, 0x0);
>>>> +	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);
>>>> +
>>>> +	msleep(20);
>>>
>>> What is this msleep for?
>>>
>>
>> This is the HW requirement, at least 20ms is needed for the possible quickly
>> open followed.
> 
> Add a comment explaining the HW requirement for this msleep.
> 

Sure, will add comments to make it clear

> Sultan

-- 
Regards,
Bin