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

[Sultan Alsawaf]

On Mon, Nov 10, 2025 at 05:46:28PM +0800, Du, Bin wrote:
> 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?

Yes, it's 100% reproducible. There's no other message in dmesg, only that one.

Sometimes there is a stop stream error when I close cheese after it froze:

  [  656.540307] amd_isp_capture amd_isp_capture: fail to disable stream
  [  657.046633] amd_isp_capture amd_isp_capture: fail to stop steam
  [  657.047224] amd_isp_capture amd_isp_capture: disabling streaming failed (-110)

When I revert to v4 I cannot reproduce it at all. It seems to be something in
v4 -> v5 that is not fixed by any of my changes.

I will try to identify the cause this week.

Sultan