Re: [PATCH v1 2/4] arm64: dts: qcom: sm8550: Add JPEG encoder node

[Atanas Filipov <atanas.filipov@oss.qualcomm.com>]

Thank you for the detailed explanation. Let me share my understanding of 
the shared upper-level blocks. They are exactly the reason we have 
frameworks like ICC with aggregate bandwidth voting, reference counting 
in the clock framework, and so on — the same applies to power domains. I 
do not think using shared resources is a problem when the drivers are 
correctly designed.

We have actually validated this: we got CAMSS working alongside the 
Qualcomm downstream camera stack after fixing the shared resource 
management — something everyone considered nearly impossible at the time.

On the CAMNOC and CPAS concern: if that coordination becomes necessary, 
the right fix is to address the resource management in both drivers 
independently, using the aggregate capabilities of the existing 
frameworks — not to introduce a
hierarchical dependency between them. Moving JPEG under CAMSS does not 
solve the CAMNOC, clock and power domain coordination problems, it just 
papers over them.

IMO the problem you are pointing at is more general than just CAMNOC — I 
would add priorities, QoS and other shared resources to the list as 
well. The answer to all of them is the same: correct use of the existing 
frameworks, not driver
merging.

On the idea of putting JPEG inside CAMSS with an external API: CAMSS has 
no engine or pipeline that produces YUV output, which is what the JPEG 
encoder needs as input. If JPEG moves into CAMSS without an external 
API, it becomes
inaccessible to userspace. If it does expose one, we end up with a 
standalone interface anyway, just with an extra layer of indirection on top.

afilipov

On 6/13/2026 12:52 PM, Bryan O'Donoghue wrote:
> On 13/06/2026 10:24, Atanas Filipov wrote:
>> Thank you for the feedback. I understand the reasoning, but I
>> respectfully disagree with this approach for the following reasons.
>>
>> While it is true that the JPEG encoder shares the same camera NOC and
>> power domain infrastructure as CAMSS, that is a hardware topology detail
>> — not a sufficient justification for imposing a software dependency. The
>> driver is a fully
>> self-contained V4L2 mem2mem encoder, implemented like every other JPEG
>> encoder driver currently in the kernel (imx-jpeg, s5p-jpeg, mtk-jpeg,
>> nxp-jpeg). None of those are sub-nodes of a parent ISP or camera
>> subsystem driver.
> 
> That's a backwards understanding of the ethos of DT, which is to 
> describe hardware architecture, to describe hardware, not to subscribe 
> to or proscribe a particular software architecture.
> 
> Those jpeg blocks are standalone, whereas the CAMSS jpeg encoder lives 
> inside of the CAMSS power-island.
>> Making the JPEG encoder a sub-node of camss would introduce an
>> unnecessary and artificial coupling: the JPEG encoder cannot be probed,
>> built, or used independently of the CAMSS driver, even on platforms
>> where CAMSS is disabled. This directly contradicts the kernel's
>> principle of independent, single-purpose drivers.
> 
> - Probed true
> - Built true
> - Used untrue
> 
> Once probed your current driver can chug along pretty much unperturbed, 
> however I don't believe that statement can hold true as more of the 
> camera hardware gets enabled.
>> The shared hardware resources (clocks, interconnects, IOMMU stream IDs,
>> power domain) are already fully described in the device tree node and
>> handled by the standard kernel frameworks — there is no functional
>> reason to nest the node under camss.
> 
> Except that it is a real description of the hardware. "We can model it 
> separately != we have modeled it correctly".
> 
> And at least one thing you are leaving out here is the cam noc - which 
> eventually we will have to start to enable and will almost certainly 
> have to be controlled by the core driver which also owns the power- 
> collapse and muxes, the thing that will also program CPAs - the core 
> CAMSS driver.
> 
> Perhaps we choose to model that NOC as a separate driver or perhaps we 
> expose an API in CAMSS to vote, either way its an intrinsic part of the 
> voltage and clocks in this block.
> 
> Either way sure we could model it as a fully separate node but, that is 
> not really how/where the block lives. It lives inside of a defined CAMSS 
> block, which is its own power-island.
> 
> Switching on the JPEG part of it by inference switches on the top-level 
> of the island so, its not separate at all.
>> For these reasons I would prefer to keep the JPEG encoder as a
>> standalone platform device with its own DT node, consistent with how all
>> comparable JPEG encoder drivers are structured in the kernel today.
>>
>> afilipov
>>
>> On 6/13/2026 2:14 AM, Bryan O'Donoghue wrote:
>>> On 12/06/2026 20:44, Atanas Filipov wrote:
>>>> +        qcom_jpeg_enc: jpeg-encoder@ac4e000 {
>>>
>>> One key bit of review feedback I gave in the previous leaked version of
>>> this driver is that since the jpeg-encoder is part of the CAMSS block it
>>> should be a sub-node of camss as OPE, CSIPHY and other blocks will be.
>>>
>>> Please take that feedback onboard in your v2.
>>>
>>> ---
>>> bod
>>
>>
> 
> And please no top posting !
> 
> ---
> bod
>