Message-ID: <c41df0f5-b2b5-49f1-a49e-8750e55975e1@amd.com>
Date: Mon, 10 Nov 2025 17:46:28 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
 laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org,
 sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 mario.limonciello@....com, richard.gong@....com, anson.tsao@....com
Subject: Re: [PATCH v5 0/7] Add AMD ISP4 driver

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

Powered by blists - more mailing lists