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

[Loic Poulain]

On Mon, Oct 26, 2026 at 4:24 PM Bryan O'Donoghue
<bryan.odonoghue@xxxxxxxxxx> wrote:
>
> 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.

Yes, and we're adding one more in this series (AREG), so I saw this
patch as a good opportunity to raise the issue and complain ;-)

>
> 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.

Yes, although the PIX path clock requirement will likely always win here.

>
> > 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.

Fair enough, it's already an improvement.

>
> 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