unicam clock changes

[naushir]

Add control of the vpu clock when streaming from unicam.

[naushir]

@pelwell and @6by9 comments please.

[pelwell]

I'm not wild about the magic number 250 * 1000 * 1000 in the code as opposed to a macro constant, but it is only used once. Is it possible that this value might ever change? Should it be a DT parameter?

I was going to raise some concerns about the SET_CLOCK_RATE mechanism, but I see that the firmware does the right thing and uses the "boost" mechanism, meaning that you won't reduce the clock if something else wants it to be higher. There appears to be no way to relinquish a claim on the clock minimum frequency, but is 0 a better minimum than 200MHz?

My only remaining concern is over whether firmware-clocks needs a similar "boost" accounting mechanism as the firmware - what happens when one stream stops while the other is still going? What stops the core frequency from being reduced?

[naushir]

I'm not wild about the magic number 250 * 1000 * 1000 in the code as opposed to a macro constant, but it is only used once. Is it possible that this value might ever change? Should it be a DT parameter?

Like the LP clock, this will (likely) never change. Having it as a DT parameter is reasonable, but perhaps overkill?

I was going to raise some concerns about the SET_CLOCK_RATE mechanism, but I see that the firmware does the right thing and uses the "boost" mechanism, meaning that you won't reduce the clock if something else wants it to be higher. There appears to be no way to relinquish a claim on the clock minimum frequency, but is 0 a better minimum than 200MHz?

Dave and me discussed this very point, and we ended up setting it back to 200MHz like the HVS does.

My only remaining concern is over whether firmware-clocks needs a similar "boost" accounting mechanism as the firmware - what happens when one stream stops while the other is still going? What stops the core frequency from being reduced?

Yes, this is a good point. I had actually assumed that firmware-clock already did this, will need to go look at the code in more detail.

[popcornmix]

Max/min rates can be read from firmware, but I don't think the clock driver exposes that conveniently:
https://github.com/raspberrypi/linux/blob/rpi-5.4.y/drivers/clk/bcm/clk-raspberrypi.c#L215-L232

Overlapping calls to control the same clock are not safe. Returning a cookie when boosting that is passed back in when unboosted is how the firmware deals with this but I don't know how that can be done though the clock interface.

I don't know if something unique to the caller could be passed into the (extended) RPI_FIRMWARE_SET_CLOCK_RATE call. Similar to the pid passed into vchiq messages. The firmware could then keep track of multiple requests.

[6by9]

I did look at the max/min clock queries that can be done, but currently it's only there for PLLB (ARM clock). All the other clocks just get added.
It looks like it's used in determine_rate for PLLB to clip the range, but I don't see a way of querying the it externally.

@mripard may be able to advise if there is a range check within the clock APIs, and confirm whether the clock framework deals with refcounting clock requests (I seem to recall him saying it does).

[mripard]

I did look at the max/min clock queries that can be done, but currently it's only there for PLLB (ARM clock). All the other clocks just get added.

I didn't add it since I wasn't aware of any use case for it before, but it's pretty trivial to add it if we need it.

It looks like it's used in determine_rate for PLLB to clip the range, but I don't see a way of querying the it externally.

Indeed it's only used for clipping at the moment and you can't retrieve it from the user API. It could be added I think if we have a use case for it. I'm not sure what the issue addressed in your patches is though. You need to have the VPU clock running at least at 250MHz while the unicam is running, and 200MHz otherwise?

@mripard may be able to advise if there is a range check within the clock APIs, and confirm whether the clock framework deals with refcounting clock requests (I seem to recall him saying it does).

The clock framework has some refcounting (on enable / disable for example), but not on all operations (set_rate doesn't for example). What kind of requests are we talking about here?

[pelwell]

The aim is address my question above:

My only remaining concern is over whether firmware-clocks needs a similar "boost" accounting mechanism as the firmware - what happens when one stream stops while the other is still going? What stops the core frequency from being reduced?

The clock driver is a single client of the firmware but represents multiple clients within the kernel. Since the aim is to guarantee a minimum clock speed then something in either unicam or the clock driver needs to track how many streams are active and request the higher clock when the count is non-zero.

My suggestion would be to add aliases of (at least) the core clock and get each of the unicam instances to use different aliases, allowing multiple uses to be tracked independently, unless you feel like making a more sweeping change.

[popcornmix]

vc4 driver also boosts core clock during a mode set. I think that's the only other "official" user. User apps could also poke the mailbox directly.

[pelwell]

User apps could also poke the mailbox directly.

Those users should expect bad stuff to happen occasionally.

[popcornmix]

Looks like:

min_clock = clk_round_rate(clk, 0);
max_clock = clk_round_rate(clk, ULONG_MAX);

if what's wanted rather than hardd coded numbers.
Request max_clock when busy and min_clock when idle.

[mripard]

The aim is address my question above:

My only remaining concern is over whether firmware-clocks needs a similar "boost" accounting mechanism as the firmware - what happens when one stream stops while the other is still going? What stops the core frequency from being reduced?

The clock driver is a single client of the firmware but represents multiple clients within the kernel. Since the aim is to guarantee a minimum clock speed then something in either unicam or the clock driver needs to track how many streams are active and request the higher clock when the count is non-zero.

My suggestion would be to add aliases of (at least) the core clock and get each of the unicam instances to use different aliases, allowing multiple uses to be tracked independently, unless you feel like making a more sweeping change.

Does that minimum clock speed change from one user to the other, or is it the same across all users?

If it's the same rate for any user, then we could use the prepare / unprepare callbacks in the clock driver that are called any time a first / last user enables / disables the clock, and use them to raise it to the needed minimum rate, and back to the lower frequency once no-one enables it anymore.

[naushir]

Does that minimum clock speed change from one user to the other, or is it the same across all users?

I expect it could be different per user. Unicam needs 250 MHz, where as HVS currently uses 500 MHz.

[mripard]

Does that minimum clock speed change from one user to the other, or is it the same across all users?

I expect it could be different per user. Unicam needs 250 MHz, where as HVS currently uses 500 MHz.

In the HVS case, it's only when changing a mode, so it's not really the same thing than raising the clock when it starts to have a user?

[pelwell]

I don't think there's a way to get a satisfactory solution without looking at the clock rates requested at runtime from all users and calculating the maximum.

[mripard]

Then I guess each user should call clk_set_min_rate with its requirement, and the clock framework will figure it out on its own between the hardware min and max (reported by the firmware in our case), and what has been set by each user.

[pelwell]

Is there really that extra level between the clock provider and the clock consumer? Do multiple callers to of_clk_get* get back different struct clk *s so the framework can distinguish between them?

[mripard]

Yes, struct clk is only the customer facing side of the clock representation, and each user will have a different struct clk affected to it. The core uses the structs clk_core and clk_hw (clk_core for the generic parts, clk_hw is what's passed to the drivers) to represent each clocks.

You can assign min and max rates on clk_core (to reflect boundaries of the hardware), or each user can do so on its struct clk (to reflect boundaries on what that particular user needs to be enforced).

[pelwell]

OK - found the per-caller struct clk allocation (with shared clk_hw), but clk_set_rate_range (the implementation of clk_set_min_rate) doesn't appear to share the limit changes, i.e. although applying a minimum might immediately change the clock rate, I don't see how it could prevent somebody else from changing it.

Anyway, I'll let you worry about that.

[mripard]

So I took some time to confirm this this morning, and it's indeed pretty hard to see how it's occuring, but I believe it should work as expected.

When calling clk_set_rate_range, if the current rate of the clock is outside of the range we want to enforce, it will call clk_core_set_rate_nolock to force the rate into the range:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2323

clk_set_rate_nolock will then call clk_core_req_round_rate_nolock on that new rate:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2158

And clk_core_req_round_rate_nolock will call clk_core_get_boundaries
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2137

clk_core_get_boundaries is the function that will retrieve the range for all the users, and the clock provider, and will find the common range
https://elixir.bootlin.com/linux/v5.7-rc7/source/drivers/clk/clk.c#L618

clk_core_req_round_rate_nolock will then call clk_core_round_rate_nolock using the new rate we want to enforce and the boundaries it discovered, but will not clamp it itself.

It might be a bug there, but at least there's an attempt to do what we were discussing :)

[naushir]

So I took some time to confirm this this morning, and it's indeed pretty hard to see how it's occuring, but I believe it should work as expected.

When calling clk_set_rate_range, if the current rate of the clock is outside of the range we want to enforce, it will call clk_core_set_rate_nolock to force the rate into the range:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2323

clk_set_rate_nolock will then call clk_core_req_round_rate_nolock on that new rate:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2158

And clk_core_req_round_rate_nolock will call clk_core_get_boundaries
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2137

clk_core_get_boundaries is the function that will retrieve the range for all the users, and the clock provider, and will find the common range
https://elixir.bootlin.com/linux/v5.7-rc7/source/drivers/clk/clk.c#L618

clk_core_req_round_rate_nolock will then call clk_core_round_rate_nolock using the new rate we want to enforce and the boundaries it discovered, but will not clamp it itself.

It might be a bug there, but at least there's an attempt to do what we were discussing :)

So with all of that, is the correct thing to do as follows:

old_min_rate = clk_round_rate(clk, 0);
clk_set_min_rate(250Mhz);
/* Do stuff in Unicam */
clk_set_min_rate(old_min_rate);

?

[naushir]

So with all of that, is the correct thing to do as follows:

old_min_rate = clk_round_rate(clk, 0);
clk_set_min_rate(250Mhz);
/* Do stuff in Unicam */
clk_set_min_rate(old_min_rate);

?

Sadly this does not work as expected. clk_round_rate(clk, 0); will always return 0. Need to understand what it does exactly.

[popcornmix]

I believe it works for arm clock in cpufreq driver.
I wonder if it works any better with @mripard 's patches. Specifically patch 12 here:
https://lkml.org/lkml/2020/5/27/957

[mripard]

So I took some time to confirm this this morning, and it's indeed pretty hard to see how it's occuring, but I believe it should work as expected.
When calling clk_set_rate_range, if the current rate of the clock is outside of the range we want to enforce, it will call clk_core_set_rate_nolock to force the rate into the range:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2323
clk_set_rate_nolock will then call clk_core_req_round_rate_nolock on that new rate:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2158
And clk_core_req_round_rate_nolock will call clk_core_get_boundaries
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2137
clk_core_get_boundaries is the function that will retrieve the range for all the users, and the clock provider, and will find the common range
https://elixir.bootlin.com/linux/v5.7-rc7/source/drivers/clk/clk.c#L618
clk_core_req_round_rate_nolock will then call clk_core_round_rate_nolock using the new rate we want to enforce and the boundaries it discovered, but will not clamp it itself.
It might be a bug there, but at least there's an attempt to do what we were discussing :)

So with all of that, is the correct thing to do as follows:

old_min_rate = clk_round_rate(clk, 0);
clk_set_min_rate(250Mhz);
/* Do stuff in Unicam */
clk_set_min_rate(old_min_rate);

?

I guess you can even simplify that by just doing something like

clk_set_min_rate(250000000);
...
clk_set_min_rate(0);

But yeah, like @popcornmix was mentionning, this was not properly reported to the CCF at the moment. I can send a PR for the additional fixes I sent to the firmware clocks on that last series if you want?

[popcornmix]

@mripard a PR for upstream clock fixes would be welcome.

[naushir]

@mripard a PR for upstream clock fixes would be welcome.

@mripard, could you ping me when you get this done, and I can finish off the unicam bit.

[mripard]

I just did that PR here:
#3664

[pelwell]

What's the status of this?

[naushir]

I've made the changes, but still have not tested them. One for next week I'm afraid.

[naushir]

@pelwell, after a rebase, I get a compile error on the dtsi:

Building branch unicam_clock
make[1]: Entering directory '/home/naushir/projects/linux_build/unicam_clock'
  GEN     Makefile
  DTC     arch/arm/boot/dts/bcm2708-rpi-b.dtb
/home/naushir/projects/linux/arch/arm/boot/dts/bcm270x.dtsi:86.22-100.5: ERROR (phandle_references): /soc/csi@7e800000: Reference to non-existent node or label "firmware_clocks"

/home/naushir/projects/linux/arch/arm/boot/dts/bcm270x.dtsi:102.22-116.5: ERROR (phandle_references): /soc/csi@7e801000: Reference to non-existent node or label "firmware_clocks"

  also defined at /home/naushir/projects/linux/arch/arm/boot/dts/bcm283x-rpi-csi1-2lane.dtsi:2.7-4.3
ERROR: Input tree has errors, aborting (use -f to force output)
make[2]: *** [scripts/Makefile.lib:286: arch/arm/boot/dts/bcm2708-rpi-b.dtb] Error 2
make[1]: *** [/home/naushir/projects/linux/Makefile:1249: dtbs] Error 2
make[1]: *** Waiting for unfinished jobs....
make[1]: Leaving directory '/home/naushir/projects/linux_build/unicam_clock'
make: *** [Makefile:179: sub-make] Error 2

I tried adding the following block to the end of the file (which is probably incorrect), but no luck:

&firmware {
	firmware_clocks: clocks {
		compatible = "raspberrypi,firmware-clocks";
		#clock-cells = <1>;
	};
};

My device tree knowledge is very limited, any idea what I should be doing go get firmware_clock recognised again?

[pelwell]

It was broken by @mripard's change "clk: rpi: Adjust DT binding to match upstream", and then not fixed by my followup "SQUASH: dts: Fix firmware clocks support" because nothing other than raspberrypi-cpufreq was using firmware_clocks, and raspberrypi-cpufreq isn't using on BCM2835.

Try this patch: pelwell@ab0ef79

[naushir]

Building with pelwell/linux@ab0ef79 seems to go further. Will do a run-test shortly. @pelwell, shall I include that patch in this PR, or did you intend to merge that separately?

[pelwell]

I've merged it to rpi-5.4.y.

[naushir]

I think this is doing the right thing now.

[popcornmix]

Cool - that looks pretty simple. I guess we want to make the vc4 display code use the same mechanism (unrelated to this PR).

[mripard]

It does in mainline already, @6by9 wanted to do a merge soon, so I guess it will come to 5.4 as well