Re: [PATCH v3 2/6] media: qcom: camss: clamp VFE clock rates to the highest level

[Bryan O'Donoghue]

On 26/10/2026 15:03, Loic Poulain wrote:
> On Mon, Oct 26, 2026 at 2:37 PM Atanas Filipov
> <atanas.filipov@xxxxxxxxxxxxxxxx> wrote:
> > The VFE fails to power on when the pixel rate exceeds the highest rate
> > listed for any of its clocks. The rate tables of the VFE clocks are
> > independent and need not have the same number of levels, so a clock
> > with a lower top rate than the VFE core clock, such as the SC7280
> > camnoc_axi clock, makes the VFE reject pixel rates the core clock
> > still supports.
> >
> > Clamp each clock to its highest listed rate instead, and warn that the
> > pixel rate exceeds it when the clock lists more than one rate. Take the
> > clamp into account when another line powers on an already running VFE,
> > so that a clock running at its highest rate does not reject the
> > request.
>
> Again, we retrieve the pixel clock to derive the required VFE **core**
> clock rate. If the VFE cannot sustain that rate, failing here seems
> like the correct and sane behavior.
>
> What I don't agree with is applying the same logic to other clock
> domains, such as AXI/CAMNOC clocks. Their frequency is not directly
> tied to the pixel clock, but rather to the aggregated bandwidth
> requirements of all active clients and data paths. As such, deriving
> their rate from the pixel clock requirement feels inaccurate and, in
> my opinion, is an abuse of the existing logic.

camnoc_axi and camnoc_rt_axi are matched by vfe_match_clock_names() 
today and have been for a long time so excluding them would be a 
behaviour change.

Perhaps warranted but that would be a different patch for different reasons.

Upstream:
return (!strcmp(clock->name, vfe_name) ||
		!strcmp(clock->name, vfe_lite_name) ||
		!strcmp(clock->name, "vfe_lite") ||
		!strcmp(clock->name, "camnoc_axi") ||
		!strcmp(clock->name, "camnoc_rt_axi"));

Also - RDI path computes
pixel_clock * bpp / 64, which is a bandwidth estimate, not a pixel rate.

> So I think the implementation should either handle or delegate to
> separate functions depending on the clock type:
> - Set the VFE core clock based on the pixel clock requirements
> (typically vfe_clk_rate >= pixel_rate).
> - For all other clocks (like camnoc_axi), and until we have proper
> aggregation or voting support, a simpler and safer approach would be
> to select the highest available frequency.

That's a bigger change - this patch only changes the top of the table. 
Setting to the max clock by default would burn power for no reason IMO.

On the core clock though I think you're right; if the sensor exceeds 
what vfeX can do then failing is the correct thing to do.

@Atanas could you keep -EINVAL for the VFE core clock and clamp only for 
the rest.

cheers.

---
bod