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

[Sultan Alsawaf <sultan@kerneltoast.com>]

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. :)

> 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