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

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

On 10/11/2025 3:18 PM, Sultan Alsawaf wrote:
> On Fri, Oct 10, 2025 at 06:25:48PM +0800, Du, Bin wrote:
>> Thanks, Sultan. sorry for the delayed response due to the long public
>> holiday here.
> 
> No worries, hope you had a good holiday. :)
> 

Thank you!

>> 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:
>>>>>> +	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.
> 
> My thinking was that it's possible for duplicate wakeups to occur when the
> stream IRQs are affined to different CPUs and they fire around the same time in
> parallel.
> 
> But now that I see how the ISP interrupts are actually GPU interrupts, it means
> that the stream IRQs will always be affined to the same CPU as each other.
> 
> So my concern does not apply here, and you should disregard it. :)
> 
> Sultan

Thanks for the further clarification.

-- 
Regards,
Bin