Re: [PATCH v5 0/7] Add AMD ISP4 driver

[Du, Bin]

Thank you, Sultan, for your time, big effort, and constant support. 
Apologies for my delayed reply for being occupied a little with other 
matters.

On 11/10/2025 4:33 PM, Sultan Alsawaf wrote:
> Hi Bin,
>
> On Wed, Nov 05, 2025 at 01:25:58AM -0800, Sultan Alsawaf wrote:
> > Hi Bin,
> >
> > To expedite review, I've attached a patch containing a bunch of fixes I've made
> > on top of v5. Most of my changes should be self-explanatory; feel free to ask
> > further about specific changes if you have any questions.
> >
> > I haven't had a chance to review all of the v4 -> v5 changes yet, but I figured
> > I should send what I've got so far.
> >
> > FYI, there is a regression in isp4if_dequeue_buffer() where the bufq lock isn't
> > protecting the list_del() anymore. I also checked the compiler output when using
> > guard() versus scoped_guard() in that function and there is no difference:
> >
> >    sha1sum:
> >    5652a0306da22ea741d80a9e03a787d0f71758a8  isp4_interface.o // guard()
> >    5652a0306da22ea741d80a9e03a787d0f71758a8  isp4_interface.o // scoped_guard()
> >
> > So guard() should be used there again, which I've done in my patch.
> >
> > I also have a few questions:
> >
> > 1. Does ISP FW provide a register interface to disable the IRQ? If so, it is
> >     faster to use that than using disable_irq_nosync() to disable the IRQ from
> >     the CPU's side.
> >
> > 2. When the IRQ is re-enabled in isp4sd_fw_resp_func(), is there anything
> >     beforehand to mask all pending interrupts from the ISP so that there isn't a
> >     bunch of stale interrupts firing as soon the IRQ is re-enabled?
> >
> > 3. Is there any risk of deadlock due to the stream kthread racing with the ISP
> >     when the ISP posts a new response _after_ the kthread determines there are no
> >     more new responses but _before_ the enable_irq() in isp4sd_fw_resp_func()?
> >
> > 4. Why are some lines much longer than before? It seems inconsistent that now
> >     there is a mix of several lines wrapped to 80 cols and many lines going
> >     beyond 80 cols. And there are multiple places where code is wrapped before
> >     reaching 80 cols even with lots of room left, specifically for cases where it
> >     wouldn't hurt readability to put more characters onto each line.
>
> I've attached a new, complete patch of changes to apply on top of v5. You may
> ignore the incomplete patch from my previous email and use the new one instead.
>
> I made many changes and also answered questions 1-3 myself.
>
> Please apply this on top of v5 and let me know if you have any questions.

Sure, will review, apply and test your patch accordingly. Your 
contribution is greatly appreciated, will let you know if there is any 
question or problem.

> BTW, I noticed a strange regression in v5 even without any of my changes: every
> time you use cheese after using it one time, the video will freeze after 30-60
> seconds with this message printed to dmesg:
>    [ 2032.716559] amd_isp_capture amd_isp_capture: -><- fail respid unknown respid(0x30002)
>
> And the video never unfreezes. I haven't found the cause for the regression yet;
> can you try to reproduce it?

Really weird, we don't see this issue either in dev or QA test. Is it 
100% reproducible and any other fail or err in the log?

> Thanks,
> Sultan

--
Regards,
Bin