linux-kernel - Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware version Titan 780

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f31c2b15-d1ae-41fa-952b-eab806b0e15d@quicinc.com>
Date: Mon, 30 Sep 2024 13:37:43 +0800
From: Depeng Shao <quic_depengs@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>, <rfoss@...nel.org>,
        <todor.too@...il.com>, <mchehab@...nel.org>, <robh@...nel.org>,
        <krzk+dt@...nel.org>, <conor+dt@...nel.org>
CC: <linux-arm-msm@...r.kernel.org>, <linux-media@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...cinc.com>, Yongsheng Li <quic_yon@...cinc.com>
Subject: Re: [PATCH 13/13] media: qcom: camss: Add support for VFE hardware
 version Titan 780

Hi Bryan,

On 9/30/2024 7:57 AM, Bryan O'Donoghue wrote:
> On 29/09/2024 02:28, Depeng Shao wrote:
>>>>
>>>
>>> Thanks for catching this, I forget to add the rup irq, so this logic 
>>> is also missed. I have tried it just now, the logic works good, will 
>>> add it in next version patch.
>>>
>>
>> I go through the code again, and find we don't do the wait for 
>> completion in VFE 480 driver, this is just used in VFE gen1 driver and 
>> just during disabling port.
> 
> Right but, we _should_ wait for completion there, the fact we don't is a 
> bug.
> 
> One context issues a command to take an action and another context in 
> this case an ISR has to fire for that action to be complete.
> 
> Therefore we _should_ wait_for_completion() in the initiating context 
> and timeout if it exceeds a reasonable timeout.
> 
> Granted, we've "dropped the ball" in 480 you're right, it needs to be 
> fixed and will be but, please in your submission do the right thing.
> 

Qualcomm downstream camera driver use the rup to move the req to a list 
to maintenance a state machine. If we don't get rup then we will enter 
bubble state.
But we are downplaying this process now due to AUP, and the bubble 
processing has been disabled in latest code base, since we think the 
buffer must be filled to the given address if we have configured the AUP 
and got buf done irq.


And this per frame wait_for_completion flow isn't exist in whole camss 
code, and current camss driver just use buf done irq to trigger the per 
frame flow.

E.g.,
irqreturn_t vfe_irq()
{
	if (rup_irq)
		reg_update_clear();

	if (buf_done_irq) {
		vfe_wm_update();
		reg_update();    --> We can't do wait_for_completion at here in irq 
context
		vb2_buffer_done();
	}
}

Just VFE gen1 driver use this wait_for_complete in vfe_disable_output, 
and this flow has been removed in vfe gen2(camss-vfe.c), so looks like
we don't need to add this wait_for_completion support and also can 
remove below code in camss-vfe-480.c

vfe_isr_reg_update()
{
	if (output->wait_reg_update) {
		output->wait_reg_update = 0;
		complete(&output->reg_update);
	}
}

Thanks,
Depeng