Hi,
This is a follow-up of the discussion here:
https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
This implements a mechanism to raise and lower clock rates based on consumer
workloads, with an example of such an implementation for the RaspberryPi4 HDMI
controller.
There's a couple of things worth discussing:
- The name is in conflict with clk_request_rate, and even though it feels
like the right name to me, we should probably avoid any confusion
- The code so far implements a policy of always going for the lowest rate
possible. While we don't have an use-case for something else, this should
maybe be made more flexible?
Let me know what you think
Maxime
Changes from v1:
- Return NULL in clk_request_start if clk pointer is NULL
- Test for clk_req pointer in clk_request_done
- Add another user in vc4
- Rebased on top of v5.15-rc1
Dom Cobley (1):
drm/vc4: hvs: Convert to the new clock request API
Maxime Ripard (2):
clk: Introduce a clock request API
drm/vc4: hdmi: Convert to the new clock request API
drivers/clk/clk.c | 126 +++++++++++++++++++++++++++++++++
drivers/gpu/drm/vc4/vc4_hdmi.c | 15 ++--
drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +
drivers/gpu/drm/vc4/vc4_kms.c | 5 +-
include/linux/clk.h | 4 ++
5 files changed, 146 insertions(+), 7 deletions(-)
--
2.31.1
It's not unusual to find clocks being shared across multiple devices
that need to change the rate depending on what the device is doing at a
given time.
The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
between its two HDMI controllers that share a clock that needs to be
raised depending on the output resolution of each controller.
The current clk_set_rate API doesn't really allow to support that case
since there's really no synchronisation between multiple users, it's
essentially a fire-and-forget solution.
clk_set_min_rate does allow for such a synchronisation, but has another
drawback: it doesn't allow to reduce the clock rate once the work is
over.
In our previous example, this means that if we were to raise the
resolution of one HDMI controller to the largest resolution and then
changing for a smaller one, we would still have the clock running at the
largest resolution rate resulting in a poor power-efficiency.
In order to address both issues, let's create an API that allows user to
create temporary requests to increase the rate to a minimum, before
going back to the initial rate once the request is done.
This introduces mainly two side-effects:
* There's an interaction between clk_set_rate and requests. This has
been addressed by having clk_set_rate increasing the rate if it's
greater than what the requests asked for, and in any case changing
the rate the clock will return to once all the requests are done.
* Similarly, clk_round_rate has been adjusted to take the requests
into account and return a rate that will be greater or equal to the
requested rates.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/clk/clk.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/clk.h | 4 ++
2 files changed, 130 insertions(+)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 65508eb89ec9..9ab0927e7ed6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -77,12 +77,14 @@ struct clk_core {
unsigned int protect_count;
unsigned long min_rate;
unsigned long max_rate;
+ unsigned long default_request_rate;
unsigned long accuracy;
int phase;
struct clk_duty duty;
struct hlist_head children;
struct hlist_node child_node;
struct hlist_head clks;
+ struct list_head pending_requests;
unsigned int notifier_count;
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
@@ -105,6 +107,12 @@ struct clk {
struct hlist_node clks_node;
};
+struct clk_request {
+ struct list_head list;
+ struct clk *clk;
+ unsigned long rate;
+};
+
/*** runtime pm ***/
static int clk_pm_runtime_get(struct clk_core *core)
{
@@ -1434,10 +1442,14 @@ unsigned long clk_hw_round_rate(struct clk_hw *hw, unsigned long rate)
{
int ret;
struct clk_rate_request req;
+ struct clk_request *clk_req;
clk_core_get_boundaries(hw->core, &req.min_rate, &req.max_rate);
req.rate = rate;
+ list_for_each_entry(clk_req, &hw->core->pending_requests, list)
+ req.min_rate = max(clk_req->rate, req.min_rate);
+
ret = clk_core_round_rate_nolock(hw->core, &req);
if (ret)
return 0;
@@ -1458,6 +1470,7 @@ EXPORT_SYMBOL_GPL(clk_hw_round_rate);
long clk_round_rate(struct clk *clk, unsigned long rate)
{
struct clk_rate_request req;
+ struct clk_request *clk_req;
int ret;
if (!clk)
@@ -1471,6 +1484,9 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
clk_core_get_boundaries(clk->core, &req.min_rate, &req.max_rate);
req.rate = rate;
+ list_for_each_entry(clk_req, &clk->core->pending_requests, list)
+ req.min_rate = max(clk_req->rate, req.min_rate);
+
ret = clk_core_round_rate_nolock(clk->core, &req);
if (clk->exclusive_count)
@@ -1938,6 +1954,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
unsigned long new_rate;
unsigned long min_rate;
unsigned long max_rate;
+ struct clk_request *req;
int p_index = 0;
long ret;
@@ -1952,6 +1969,9 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
clk_core_get_boundaries(core, &min_rate, &max_rate);
+ list_for_each_entry(req, &core->pending_requests, list)
+ min_rate = max(req->rate, min_rate);
+
/* find the closest rate and parent clk/rate */
if (clk_core_can_round(core)) {
struct clk_rate_request req;
@@ -2148,6 +2168,7 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
{
int ret, cnt;
struct clk_rate_request req;
+ struct clk_request *clk_req;
lockdep_assert_held(&prepare_lock);
@@ -2162,6 +2183,9 @@ static unsigned long clk_core_req_round_rate_nolock(struct clk_core *core,
clk_core_get_boundaries(core, &req.min_rate, &req.max_rate);
req.rate = req_rate;
+ list_for_each_entry(clk_req, &core->pending_requests, list)
+ req.min_rate = max(clk_req->rate, req.min_rate);
+
ret = clk_core_round_rate_nolock(core, &req);
/* restore the protection */
@@ -2255,6 +2279,9 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
ret = clk_core_set_rate_nolock(clk->core, rate);
+ if (!list_empty(&clk->core->pending_requests))
+ clk->core->default_request_rate = rate;
+
if (clk->exclusive_count)
clk_core_rate_protect(clk->core);
@@ -2420,6 +2447,104 @@ int clk_set_max_rate(struct clk *clk, unsigned long rate)
}
EXPORT_SYMBOL_GPL(clk_set_max_rate);
+/**
+ * clk_request_start - Request a rate to be enforced temporarily
+ * @clk: the clk to act on
+ * @rate: the new rate asked for
+ *
+ * This function will create a request to temporarily increase the rate
+ * of the clock to a given rate to a certain minimum.
+ *
+ * This is meant as a best effort mechanism and while the rate of the
+ * clock will be guaranteed to be equal or higher than the requested
+ * rate, there's none on what the actual rate will be due to other
+ * factors (other requests previously set, clock boundaries, etc.).
+ *
+ * Once the request is marked as done through clk_request_done(), the
+ * rate will be reverted back to what the rate was before the request.
+ *
+ * The reported boundaries of the clock will also be adjusted so that
+ * clk_round_rate() take those requests into account. A call to
+ * clk_set_rate() during a request will affect the rate the clock will
+ * return to after the requests on that clock are done.
+ *
+ * Returns the request on success, NULL if clk was NULL, or an ERR_PTR
+ * otherwise.
+ */
+struct clk_request *clk_request_start(struct clk *clk, unsigned long rate)
+{
+ struct clk_request *req;
+ int ret;
+
+ if (!clk)
+ return NULL;
+
+ req = kzalloc(sizeof(*req), GFP_KERNEL);
+ if (!req)
+ return ERR_PTR(-ENOMEM);
+
+ clk_prepare_lock();
+
+ req->clk = clk;
+ req->rate = rate;
+
+ if (list_empty(&clk->core->pending_requests))
+ clk->core->default_request_rate = clk_core_get_rate_recalc(clk->core);
+
+ ret = clk_core_set_rate_nolock(clk->core, rate);
+ if (ret) {
+ clk_prepare_unlock();
+ kfree(req);
+ return ERR_PTR(ret);
+ }
+
+ list_add_tail(&req->list, &clk->core->pending_requests);
+ clk_prepare_unlock();
+
+ return req;
+}
+EXPORT_SYMBOL_GPL(clk_request_start);
+
+/**
+ * clk_request_done - Mark a clk_request as done
+ * @req: the request to mark done
+ *
+ * This function will remove the rate request from the clock and adjust
+ * the clock rate back to either to what it was before the request
+ * started, or if there's any other request on that clock to a proper
+ * rate for them.
+ */
+void clk_request_done(struct clk_request *req)
+{
+ struct clk_core *core;
+
+ if (!req)
+ return;
+
+ clk_prepare_lock();
+
+ list_del(&req->list);
+ core = req->clk->core;
+
+ if (list_empty(&core->pending_requests)) {
+ clk_core_set_rate_nolock(core, core->default_request_rate);
+ core->default_request_rate = 0;
+ } else {
+ struct clk_request *cur_req;
+ unsigned long new_rate = 0;
+
+ list_for_each_entry(cur_req, &core->pending_requests, list)
+ new_rate = max(new_rate, cur_req->rate);
+
+ clk_core_set_rate_nolock(core, new_rate);
+ }
+
+ clk_prepare_unlock();
+
+ kfree(req);
+}
+EXPORT_SYMBOL_GPL(clk_request_done);
+
/**
* clk_get_parent - return the parent of a clk
* @clk: the clk whose parent gets returned
@@ -3851,6 +3976,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
goto fail_parents;
INIT_HLIST_HEAD(&core->clks);
+ INIT_LIST_HEAD(&core->pending_requests);
/*
* Don't call clk_hw_create_clk() here because that would pin the
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 266e8de3cb51..2aa52140d8a9 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -15,6 +15,7 @@
struct device;
struct clk;
+struct clk_request;
struct device_node;
struct of_phandle_args;
@@ -783,6 +784,9 @@ int clk_save_context(void);
*/
void clk_restore_context(void);
+struct clk_request *clk_request_start(struct clk *clk, unsigned long rate);
+void clk_request_done(struct clk_request *req);
+
#else /* !CONFIG_HAVE_CLK */
static inline struct clk *clk_get(struct device *dev, const char *id)
--
2.31.1
The new clock request API allows us to increase the rate of the HSM
clock to match our pixel rate requirements while decreasing it when
we're done, resulting in a better power-efficiency.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 15 ++++++++++-----
drivers/gpu/drm/vc4/vc4_hdmi.h | 3 +++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 4a1115043114..099a94570e86 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -635,6 +635,8 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
vc4_hdmi->variant->phy_disable(vc4_hdmi);
clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
+ clk_request_done(vc4_hdmi->bvb_req);
+ clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
ret = pm_runtime_put(&vc4_hdmi->pdev->dev);
@@ -941,9 +943,9 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
* pixel clock, but HSM ends up being the limiting factor.
*/
hsm_rate = max_t(unsigned long, 120000000, (pixel_rate / 100) * 101);
- ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
- if (ret) {
- DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
+ vc4_hdmi->hsm_req = clk_request_start(vc4_hdmi->hsm_clock, hsm_rate);
+ if (IS_ERR(vc4_hdmi->hsm_req)) {
+ DRM_ERROR("Failed to set HSM clock rate: %ld\n", PTR_ERR(vc4_hdmi->hsm_req));
return;
}
@@ -956,9 +958,10 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
else
bvb_rate = 75000000;
- ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
+ vc4_hdmi->bvb_req = clk_request_start(vc4_hdmi->pixel_bvb_clock, bvb_rate);
if (ret) {
- DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
+ DRM_ERROR("Failed to set pixel bvb clock rate: %ld\n", PTR_ERR(vc4_hdmi->bvb_req));
+ clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
return;
}
@@ -966,6 +969,8 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
if (ret) {
DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
+ clk_request_done(vc4_hdmi->bvb_req);
+ clk_request_done(vc4_hdmi->hsm_req);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
return;
}
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 33e9f665ab8e..683b2d8a3dca 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -176,6 +176,9 @@ struct vc4_hdmi {
struct reset_control *reset;
+ struct clk_request *bvb_req;
+ struct clk_request *hsm_req;
+
struct debugfs_regset32 hdmi_regset;
struct debugfs_regset32 hd_regset;
};
--
2.31.1
From: Dom Cobley <[email protected]>
The new clock request API allows us to increase the rate of the
core clock as required during mode set while decreasing it when
we're done, resulting in a better power-efficiency.
Signed-off-by: Dom Cobley <[email protected]>
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/vc4/vc4_kms.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f0b3e4cf5bce..3550ae9f782e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -341,6 +341,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
struct vc4_hvs_state *old_hvs_state;
+ struct clk_request *core_req;
int i;
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -354,7 +355,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
}
if (vc4->hvs->hvs5)
- clk_set_min_rate(hvs->core_clk, 500000000);
+ core_req = clk_request_start(hvs->core_clk, 500000000);
old_hvs_state = vc4_hvs_get_old_global_state(state);
if (!old_hvs_state)
@@ -399,7 +400,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state *state)
drm_atomic_helper_cleanup_planes(dev, state);
if (vc4->hvs->hvs5)
- clk_set_min_rate(hvs->core_clk, 0);
+ clk_request_done(core_req);
}
static int vc4_atomic_commit_setup(struct drm_atomic_state *state)
--
2.31.1
Hi Stephen, Mike,
On Tue, Sep 14, 2021 at 11:35:12AM +0200, Maxime Ripard wrote:
> Hi,
>
> This is a follow-up of the discussion here:
> https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
>
> This implements a mechanism to raise and lower clock rates based on consumer
> workloads, with an example of such an implementation for the RaspberryPi4 HDMI
> controller.
>
> There's a couple of things worth discussing:
>
> - The name is in conflict with clk_request_rate, and even though it feels
> like the right name to me, we should probably avoid any confusion
>
> - The code so far implements a policy of always going for the lowest rate
> possible. While we don't have an use-case for something else, this should
> maybe be made more flexible?
This has been posted around 8 monthes ago now:
https://lore.kernel.org/all/[email protected]/
I haven't had any review on this, and I'm struggling to see how we can
move forward. Given your initial reaction, I'm guessing you were a bit
reluctant at first with the approach, if so, can you share *any*
direction in which I should amend that series to support similar
features?
Maxime
Sorry for being super delayed on response here. I'm buried in other
work. +Jerome for exclusive clk API.
Quoting Maxime Ripard (2021-09-14 02:35:13)
> It's not unusual to find clocks being shared across multiple devices
> that need to change the rate depending on what the device is doing at a
> given time.
>
> The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
> between its two HDMI controllers that share a clock that needs to be
> raised depending on the output resolution of each controller.
>
> The current clk_set_rate API doesn't really allow to support that case
> since there's really no synchronisation between multiple users, it's
> essentially a fire-and-forget solution.
I'd also say a "last caller wins"
>
> clk_set_min_rate does allow for such a synchronisation, but has another
> drawback: it doesn't allow to reduce the clock rate once the work is
> over.
What does "work over" mean specifically? Does it mean one of the clk
consumers has decided to stop using the clk?
Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined
with clk_set_rate_exclusive()?
>
> In our previous example, this means that if we were to raise the
> resolution of one HDMI controller to the largest resolution and then
> changing for a smaller one, we would still have the clock running at the
> largest resolution rate resulting in a poor power-efficiency.
Does this example have two HDMI controllers where they share one clk and
want to use the most efficient frequency for both of the HDMI devices? I
think I'm following along but it's hard. It would be clearer if there
was some psuedo-code explaining how it is both non-workable with current
APIs and workable with the new APIs.
>
> In order to address both issues, let's create an API that allows user to
> create temporary requests to increase the rate to a minimum, before
> going back to the initial rate once the request is done.
>
> This introduces mainly two side-effects:
>
> * There's an interaction between clk_set_rate and requests. This has
> been addressed by having clk_set_rate increasing the rate if it's
> greater than what the requests asked for, and in any case changing
> the rate the clock will return to once all the requests are done.
>
> * Similarly, clk_round_rate has been adjusted to take the requests
> into account and return a rate that will be greater or equal to the
> requested rates.
>
I believe clk_set_rate_range() is broken but it can be fixed. I'm
forgetting the details though. If the intended user of this new API
can't use that range API then it would be good to understand why it
can't be used. I imagine it would be something like
struct clk *clk_hdmi1, *clk_hdmi2;
clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX);
clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX);
clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX);
and then the goal would be for HDMI1_MIN to be used, or at the least for
the last call to clk_set_rate_range() to drop the rate constraint and
re-evaluate the frequency of the clk again based on hdmi1's rate range.
We could have a macro for range requests to drop their frequency
constraint like clk_drop_rate_range() that's a simple wrapper around 0,
UINT_MAX if that makes it easier to read.
Hi Stephen,
Thanks for your answer
On Tue, Jan 11, 2022 at 07:37:15PM -0800, Stephen Boyd wrote:
> Sorry for being super delayed on response here. I'm buried in other
> work. +Jerome for exclusive clk API.
>
> Quoting Maxime Ripard (2021-09-14 02:35:13)
> > It's not unusual to find clocks being shared across multiple devices
> > that need to change the rate depending on what the device is doing at a
> > given time.
> >
> > The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
> > between its two HDMI controllers that share a clock that needs to be
> > raised depending on the output resolution of each controller.
> >
> > The current clk_set_rate API doesn't really allow to support that case
> > since there's really no synchronisation between multiple users, it's
> > essentially a fire-and-forget solution.
>
> I'd also say a "last caller wins"
>
> >
> > clk_set_min_rate does allow for such a synchronisation, but has another
> > drawback: it doesn't allow to reduce the clock rate once the work is
> > over.
>
> What does "work over" mean specifically? Does it mean one of the clk
> consumers has decided to stop using the clk?
That, or it doesn't need to enforce that minimum anymore. We have
several cases like this on the RPi. For example, during a change of
display mode a (shared) clock needs to be raised to a minimum, but
another shared one needs to raise its minimum based on the resolution.
In the former case, we only need the minimum to be enforced during the
resolution change, so it's fairly quick, but the latter requires its
minimum for as long as the display is on.
> Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined
> with clk_set_rate_exclusive()?
clk_set_rate_range could work (it's what we have right now in mainline
after all), but it's suboptimal since the clock is never scaled down.
It's especially showing in my first example where we need to raise the
clock only for the duration of the resolution change. Using
clk_set_min_rate works but we end up with that fairly high clock (at
least 500MHz) for the rest of the system life even though we usually can
get away with using a clock around 200MHz outside of that (short) window.
This is fairly inefficient, and is mostly what I'm trying to address.
> > In our previous example, this means that if we were to raise the
> > resolution of one HDMI controller to the largest resolution and then
> > changing for a smaller one, we would still have the clock running at the
> > largest resolution rate resulting in a poor power-efficiency.
>
> Does this example have two HDMI controllers where they share one clk and
> want to use the most efficient frequency for both of the HDMI devices? I
> think I'm following along but it's hard. It would be clearer if there
> was some psuedo-code explaining how it is both non-workable with current
> APIs and workable with the new APIs.
The fact that we have two HDMI controllers that share one clock is why
we use clk_set_min_rate in the first place, but you can have that
behavior with clk_set_min_rate only with a single user.
With pseudo-code, if you do something like
clk = clk_get(NULL);
clk_set_min_rate(600 * 1000 * 1000);
clk_set_min_rate(1000);
The clock will still remain at 600MHz, even though you would be totally
fine with the clock running at 1kHz.
If you really wanted to make the clock run at 1kHz, you'd need to have:
clk = clk_get(NULL);
clk_set_min_rate(600 * 1000 * 1000);
clk_set_min_rate(1000);
clk_set_rate(1000);
And that works fine for a single user.
If you have a clock shared by multiple drivers though, things get
tricky. Indeed, you can't really find out what the minimum for that
clock is, so figuring out the rate to pass to the clk_set_rate call
would be difficult already. And it wouldn't be atomic anyway.
It's made even more difficult since in clk_calc_new_rates the core
checks that the rate is within the boundaries and will error out if it
isn't, so even using clk_set_rate(0) wouldn't work.
It could work if the clock driver makes sure in round/determine_rate
that the rate passed in within the boundaries of the clock, but then you
start working around the core and relying on the behavior of clock
drivers, which is a fairly significant abstraction violation.
> > In order to address both issues, let's create an API that allows user to
> > create temporary requests to increase the rate to a minimum, before
> > going back to the initial rate once the request is done.
> >
> > This introduces mainly two side-effects:
> >
> > * There's an interaction between clk_set_rate and requests. This has
> > been addressed by having clk_set_rate increasing the rate if it's
> > greater than what the requests asked for, and in any case changing
> > the rate the clock will return to once all the requests are done.
> >
> > * Similarly, clk_round_rate has been adjusted to take the requests
> > into account and return a rate that will be greater or equal to the
> > requested rates.
> >
>
> I believe clk_set_rate_range() is broken but it can be fixed. I'm
> forgetting the details though. If the intended user of this new API
> can't use that range API then it would be good to understand why it
> can't be used. I imagine it would be something like
>
> struct clk *clk_hdmi1, *clk_hdmi2;
>
> clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX);
> clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX);
> clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX);
>
> and then the goal would be for HDMI1_MIN to be used, or at the least for
> the last call to clk_set_rate_range() to drop the rate constraint and
> re-evaluate the frequency of the clk again based on hdmi1's rate range.
This is pretty much what this series was doing. I was being conservative
and didn't really want to modify the behavior of existing functions, but
that will work fine.
Thanks!
Maxime
14.09.2021 12:35, Maxime Ripard пишет:
> Hi,
>
> This is a follow-up of the discussion here:
> https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
>
> This implements a mechanism to raise and lower clock rates based on consumer
> workloads, with an example of such an implementation for the RaspberryPi4 HDMI
> controller.
>
> There's a couple of things worth discussing:
>
> - The name is in conflict with clk_request_rate, and even though it feels
> like the right name to me, we should probably avoid any confusion
>
> - The code so far implements a policy of always going for the lowest rate
> possible. While we don't have an use-case for something else, this should
> maybe be made more flexible?
Hello Maxime,
On NVIDIA Tegra we use interconnect framework for converting of
workload-based memory bandwidth requirement to the memory clock rate
[1]. All Tegra SoCs have two display controllers and other memory
clients, ICC takes care of summing and updating memory bandwidth for us,
which in the end results in a freq change of the shared memory controller.
[1] https://git.kernel.org/linus/04d5d5df9
Not so long time ago me and Thierry Reding were looking at yours v1 and
back then Thierry suggested that the same ICC approach might work for
yours case. I'm now looking at the v2 and yours discussion with Stephen
Boyd, and it appears that ICC is indeed what you really need. Have you
considered to use ICC?
Hi Dmitry,
On Wed, Jan 12, 2022 at 04:28:41PM +0300, Dmitry Osipenko wrote:
> 14.09.2021 12:35, Maxime Ripard пишет:
> > Hi,
> >
> > This is a follow-up of the discussion here:
> > https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
> >
> > This implements a mechanism to raise and lower clock rates based on consumer
> > workloads, with an example of such an implementation for the RaspberryPi4 HDMI
> > controller.
> >
> > There's a couple of things worth discussing:
> >
> > - The name is in conflict with clk_request_rate, and even though it feels
> > like the right name to me, we should probably avoid any confusion
> >
> > - The code so far implements a policy of always going for the lowest rate
> > possible. While we don't have an use-case for something else, this should
> > maybe be made more flexible?
>
> Hello Maxime,
>
> On NVIDIA Tegra we use interconnect framework for converting of
> workload-based memory bandwidth requirement to the memory clock rate
> [1]. All Tegra SoCs have two display controllers and other memory
> clients, ICC takes care of summing and updating memory bandwidth for us,
> which in the end results in a freq change of the shared memory controller.
>
> [1] https://git.kernel.org/linus/04d5d5df9
>
> Not so long time ago me and Thierry Reding were looking at yours v1 and
> back then Thierry suggested that the same ICC approach might work for
> yours case. I'm now looking at the v2 and yours discussion with Stephen
> Boyd, and it appears that ICC is indeed what you really need. Have you
> considered to use ICC?
The goals seem to be similar indeed, but most of these clocks feed some
internal state machine in those devices and are not related to the
memory bandwidth at all. So there's no real interconnect to model there :/
Maxime
12.01.2022 16:51, Maxime Ripard пишет:
> Hi Dmitry,
>
> On Wed, Jan 12, 2022 at 04:28:41PM +0300, Dmitry Osipenko wrote:
>> 14.09.2021 12:35, Maxime Ripard пишет:
>>> Hi,
>>>
>>> This is a follow-up of the discussion here:
>>> https://lore.kernel.org/linux-clk/20210319150355.xzw7ikwdaga2dwhv@gilmour/
>>>
>>> This implements a mechanism to raise and lower clock rates based on consumer
>>> workloads, with an example of such an implementation for the RaspberryPi4 HDMI
>>> controller.
>>>
>>> There's a couple of things worth discussing:
>>>
>>> - The name is in conflict with clk_request_rate, and even though it feels
>>> like the right name to me, we should probably avoid any confusion
>>>
>>> - The code so far implements a policy of always going for the lowest rate
>>> possible. While we don't have an use-case for something else, this should
>>> maybe be made more flexible?
>>
>> Hello Maxime,
>>
>> On NVIDIA Tegra we use interconnect framework for converting of
>> workload-based memory bandwidth requirement to the memory clock rate
>> [1]. All Tegra SoCs have two display controllers and other memory
>> clients, ICC takes care of summing and updating memory bandwidth for us,
>> which in the end results in a freq change of the shared memory controller.
>>
>> [1] https://git.kernel.org/linus/04d5d5df9
>>
>> Not so long time ago me and Thierry Reding were looking at yours v1 and
>> back then Thierry suggested that the same ICC approach might work for
>> yours case. I'm now looking at the v2 and yours discussion with Stephen
>> Boyd, and it appears that ICC is indeed what you really need. Have you
>> considered to use ICC?
>
> The goals seem to be similar indeed, but most of these clocks feed some
> internal state machine in those devices and are not related to the
> memory bandwidth at all. So there's no real interconnect to model there :/
If you could convert resolution/pclk to BW and BW to the clock rates,
then it should be possible to model ICC. BW doesn't necessarily need to
be "memory" bandwidth, bandwidth is abstract value expressed in kbytes/sec.
The state machine will be ICC provider then, although you'll need to
model that machine as a separate device somehow. For example, on Tegra
we needed to specify clocks as separate devices to model GENPD [2][3].
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=b1bc04a2ac5
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/tegra30.dtsi?h=next-20220112#n394
Quoting Maxime Ripard (2022-01-12 03:46:52)
> Hi Stephen,
>
> Thanks for your answer
>
> On Tue, Jan 11, 2022 at 07:37:15PM -0800, Stephen Boyd wrote:
> > Sorry for being super delayed on response here. I'm buried in other
> > work. +Jerome for exclusive clk API.
> >
> > Quoting Maxime Ripard (2021-09-14 02:35:13)
> > > It's not unusual to find clocks being shared across multiple devices
> > > that need to change the rate depending on what the device is doing at a
> > > given time.
> > >
> > > The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
> > > between its two HDMI controllers that share a clock that needs to be
> > > raised depending on the output resolution of each controller.
> > >
> > > The current clk_set_rate API doesn't really allow to support that case
> > > since there's really no synchronisation between multiple users, it's
> > > essentially a fire-and-forget solution.
> >
> > I'd also say a "last caller wins"
> >
> > >
> > > clk_set_min_rate does allow for such a synchronisation, but has another
> > > drawback: it doesn't allow to reduce the clock rate once the work is
> > > over.
> >
> > What does "work over" mean specifically? Does it mean one of the clk
> > consumers has decided to stop using the clk?
>
> That, or it doesn't need to enforce that minimum anymore. We have
> several cases like this on the RPi. For example, during a change of
> display mode a (shared) clock needs to be raised to a minimum, but
> another shared one needs to raise its minimum based on the resolution.
>
> In the former case, we only need the minimum to be enforced during the
> resolution change, so it's fairly quick, but the latter requires its
> minimum for as long as the display is on.
>
> > Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined
> > with clk_set_rate_exclusive()?
>
> clk_set_rate_range could work (it's what we have right now in mainline
> after all), but it's suboptimal since the clock is never scaled down.
Alright, I didn't see any mention of clk_set_rate_range() in the commit
text so did I miss it? Maybe it's used interchangeably with
clk_set_min_rate()?
>
> It's especially showing in my first example where we need to raise the
> clock only for the duration of the resolution change. Using
> clk_set_min_rate works but we end up with that fairly high clock (at
> least 500MHz) for the rest of the system life even though we usually can
> get away with using a clock around 200MHz outside of that (short) window.
>
> This is fairly inefficient, and is mostly what I'm trying to address.
Got it!
>
> > > In our previous example, this means that if we were to raise the
> > > resolution of one HDMI controller to the largest resolution and then
> > > changing for a smaller one, we would still have the clock running at the
> > > largest resolution rate resulting in a poor power-efficiency.
> >
> > Does this example have two HDMI controllers where they share one clk and
> > want to use the most efficient frequency for both of the HDMI devices? I
> > think I'm following along but it's hard. It would be clearer if there
> > was some psuedo-code explaining how it is both non-workable with current
> > APIs and workable with the new APIs.
>
> The fact that we have two HDMI controllers that share one clock is why
> we use clk_set_min_rate in the first place, but you can have that
> behavior with clk_set_min_rate only with a single user.
>
> With pseudo-code, if you do something like
>
> clk = clk_get(NULL);
> clk_set_min_rate(600 * 1000 * 1000);
> clk_set_min_rate(1000);
>
> The clock will still remain at 600MHz, even though you would be totally
> fine with the clock running at 1kHz.
That looks like a bug. While we could happily ignore the rate floor
being lowered because we're still within constraints, it looks like we
should always re-evaluate the constraints when they change.
>
> If you really wanted to make the clock run at 1kHz, you'd need to have:
>
> clk = clk_get(NULL);
> clk_set_min_rate(600 * 1000 * 1000);
> clk_set_min_rate(1000);
> clk_set_rate(1000);
>
> And that works fine for a single user.
>
> If you have a clock shared by multiple drivers though, things get
> tricky. Indeed, you can't really find out what the minimum for that
> clock is, so figuring out the rate to pass to the clk_set_rate call
> would be difficult already. And it wouldn't be atomic anyway.
Right.
>
> It's made even more difficult since in clk_calc_new_rates the core
> checks that the rate is within the boundaries and will error out if it
> isn't, so even using clk_set_rate(0) wouldn't work.
clk_set_rate(0) is pretty gross!
>
> It could work if the clock driver makes sure in round/determine_rate
> that the rate passed in within the boundaries of the clock, but then you
> start working around the core and relying on the behavior of clock
> drivers, which is a fairly significant abstraction violation.
>
> > > In order to address both issues, let's create an API that allows user to
> > > create temporary requests to increase the rate to a minimum, before
> > > going back to the initial rate once the request is done.
> > >
> > > This introduces mainly two side-effects:
> > >
> > > * There's an interaction between clk_set_rate and requests. This has
> > > been addressed by having clk_set_rate increasing the rate if it's
> > > greater than what the requests asked for, and in any case changing
> > > the rate the clock will return to once all the requests are done.
> > >
> > > * Similarly, clk_round_rate has been adjusted to take the requests
> > > into account and return a rate that will be greater or equal to the
> > > requested rates.
> > >
> >
> > I believe clk_set_rate_range() is broken but it can be fixed. I'm
> > forgetting the details though. If the intended user of this new API
> > can't use that range API then it would be good to understand why it
> > can't be used. I imagine it would be something like
> >
> > struct clk *clk_hdmi1, *clk_hdmi2;
> >
> > clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX);
> > clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX);
> > clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX);
> >
> > and then the goal would be for HDMI1_MIN to be used, or at the least for
> > the last call to clk_set_rate_range() to drop the rate constraint and
> > re-evaluate the frequency of the clk again based on hdmi1's rate range.
>
> This is pretty much what this series was doing. I was being conservative
> and didn't really want to modify the behavior of existing functions, but
> that will work fine.
>
I don't see a problem with re-evaluating the rate every time we call
clk_set_rate_range(). That's probably the bug that I can't recall. Can
you fix the API so it works that way?
On Thu, Jan 13, 2022 at 01:44:25PM -0800, Stephen Boyd wrote:
> Quoting Maxime Ripard (2022-01-12 03:46:52)
> > On Tue, Jan 11, 2022 at 07:37:15PM -0800, Stephen Boyd wrote:
> > > Sorry for being super delayed on response here. I'm buried in other
> > > work. +Jerome for exclusive clk API.
> > >
> > > Quoting Maxime Ripard (2021-09-14 02:35:13)
> > > > It's not unusual to find clocks being shared across multiple devices
> > > > that need to change the rate depending on what the device is doing at a
> > > > given time.
> > > >
> > > > The SoC found on the RaspberryPi4 (BCM2711) is in such a situation
> > > > between its two HDMI controllers that share a clock that needs to be
> > > > raised depending on the output resolution of each controller.
> > > >
> > > > The current clk_set_rate API doesn't really allow to support that case
> > > > since there's really no synchronisation between multiple users, it's
> > > > essentially a fire-and-forget solution.
> > >
> > > I'd also say a "last caller wins"
> > >
> > > >
> > > > clk_set_min_rate does allow for such a synchronisation, but has another
> > > > drawback: it doesn't allow to reduce the clock rate once the work is
> > > > over.
> > >
> > > What does "work over" mean specifically? Does it mean one of the clk
> > > consumers has decided to stop using the clk?
> >
> > That, or it doesn't need to enforce that minimum anymore. We have
> > several cases like this on the RPi. For example, during a change of
> > display mode a (shared) clock needs to be raised to a minimum, but
> > another shared one needs to raise its minimum based on the resolution.
> >
> > In the former case, we only need the minimum to be enforced during the
> > resolution change, so it's fairly quick, but the latter requires its
> > minimum for as long as the display is on.
> >
> > > Why doesn't clk_set_rate_range() work? Or clk_set_rate_range() combined
> > > with clk_set_rate_exclusive()?
> >
> > clk_set_rate_range could work (it's what we have right now in mainline
> > after all), but it's suboptimal since the clock is never scaled down.
>
> Alright, I didn't see any mention of clk_set_rate_range() in the commit
> text so did I miss it? Maybe it's used interchangeably with
> clk_set_min_rate()?
Yeah, we were interested in only the minimum so I've used
clk_set_min_rate, but it's backed by clk_set_rate_range() so it can be
used interchangeably.
> > It's especially showing in my first example where we need to raise the
> > clock only for the duration of the resolution change. Using
> > clk_set_min_rate works but we end up with that fairly high clock (at
> > least 500MHz) for the rest of the system life even though we usually can
> > get away with using a clock around 200MHz outside of that (short) window.
> >
> > This is fairly inefficient, and is mostly what I'm trying to address.
>
> Got it!
>
> >
> > > > In our previous example, this means that if we were to raise the
> > > > resolution of one HDMI controller to the largest resolution and then
> > > > changing for a smaller one, we would still have the clock running at the
> > > > largest resolution rate resulting in a poor power-efficiency.
> > >
> > > Does this example have two HDMI controllers where they share one clk and
> > > want to use the most efficient frequency for both of the HDMI devices? I
> > > think I'm following along but it's hard. It would be clearer if there
> > > was some psuedo-code explaining how it is both non-workable with current
> > > APIs and workable with the new APIs.
> >
> > The fact that we have two HDMI controllers that share one clock is why
> > we use clk_set_min_rate in the first place, but you can have that
> > behavior with clk_set_min_rate only with a single user.
> >
> > With pseudo-code, if you do something like
> >
> > clk = clk_get(NULL);
> > clk_set_min_rate(600 * 1000 * 1000);
> > clk_set_min_rate(1000);
> >
> > The clock will still remain at 600MHz, even though you would be totally
> > fine with the clock running at 1kHz.
>
> That looks like a bug. While we could happily ignore the rate floor
> being lowered because we're still within constraints, it looks like we
> should always re-evaluate the constraints when they change.
>
> >
> > If you really wanted to make the clock run at 1kHz, you'd need to have:
> >
> > clk = clk_get(NULL);
> > clk_set_min_rate(600 * 1000 * 1000);
> > clk_set_min_rate(1000);
> > clk_set_rate(1000);
> >
> > And that works fine for a single user.
> >
> > If you have a clock shared by multiple drivers though, things get
> > tricky. Indeed, you can't really find out what the minimum for that
> > clock is, so figuring out the rate to pass to the clk_set_rate call
> > would be difficult already. And it wouldn't be atomic anyway.
>
> Right.
>
> >
> > It's made even more difficult since in clk_calc_new_rates the core
> > checks that the rate is within the boundaries and will error out if it
> > isn't, so even using clk_set_rate(0) wouldn't work.
>
> clk_set_rate(0) is pretty gross!
>
> >
> > It could work if the clock driver makes sure in round/determine_rate
> > that the rate passed in within the boundaries of the clock, but then you
> > start working around the core and relying on the behavior of clock
> > drivers, which is a fairly significant abstraction violation.
> >
> > > > In order to address both issues, let's create an API that allows user to
> > > > create temporary requests to increase the rate to a minimum, before
> > > > going back to the initial rate once the request is done.
> > > >
> > > > This introduces mainly two side-effects:
> > > >
> > > > * There's an interaction between clk_set_rate and requests. This has
> > > > been addressed by having clk_set_rate increasing the rate if it's
> > > > greater than what the requests asked for, and in any case changing
> > > > the rate the clock will return to once all the requests are done.
> > > >
> > > > * Similarly, clk_round_rate has been adjusted to take the requests
> > > > into account and return a rate that will be greater or equal to the
> > > > requested rates.
> > > >
> > >
> > > I believe clk_set_rate_range() is broken but it can be fixed. I'm
> > > forgetting the details though. If the intended user of this new API
> > > can't use that range API then it would be good to understand why it
> > > can't be used. I imagine it would be something like
> > >
> > > struct clk *clk_hdmi1, *clk_hdmi2;
> > >
> > > clk_set_rate_range(&clk_hdmi1, HDMI1_MIN, HDMI1_MAX);
> > > clk_set_rate_range(&clk_hdmi2, HDMI2_MIN, HDMI2_MAX);
> > > clk_set_rate_range(&clk_hdmi2, 0, UINT_MAX);
> > >
> > > and then the goal would be for HDMI1_MIN to be used, or at the least for
> > > the last call to clk_set_rate_range() to drop the rate constraint and
> > > re-evaluate the frequency of the clk again based on hdmi1's rate range.
> >
> > This is pretty much what this series was doing. I was being conservative
> > and didn't really want to modify the behavior of existing functions, but
> > that will work fine.
>
> I don't see a problem with re-evaluating the rate every time we call
> clk_set_rate_range(). That's probably the bug that I can't recall. Can
> you fix the API so it works that way?
Yep, I'll work on it next week. I started to think about it this week,
and there's two things I'm not entirely sure about:
- Keeping the clock at its minimum rate is essentially policy. Do we
want to guard that behavior by a flag, and do we want to do the same
thing if we want to max it?
- How should we deal with a clk_set_rate call while we have a range?
Something like:
clk_set_min_rate(hdmi1_clk, 1000);
clk_set_min_rate(hdmi2_clk, 2000)
clk_set_rate(hdmi1_clk, 3000);
clk_drop_range(hdmi2_clk);
If we just try to minimize the rate all the time, we'll set the rate
back to 1000Hz, but I think it would make more sense to keep the
rate at 3000?
Maybe we can test if the range is still at the boundary we remove,
and only if it is, drop it to whatever boundary we have left?
Maxime
Quoting Maxime Ripard (2022-01-14 08:15:56)
> On Thu, Jan 13, 2022 at 01:44:25PM -0800, Stephen Boyd wrote:
> > >
> > I don't see a problem with re-evaluating the rate every time we call
> > clk_set_rate_range(). That's probably the bug that I can't recall. Can
> > you fix the API so it works that way?
>
> Yep, I'll work on it next week. I started to think about it this week,
> and there's two things I'm not entirely sure about:
>
> - Keeping the clock at its minimum rate is essentially policy. Do we
> want to guard that behavior by a flag, and do we want to do the same
> thing if we want to max it?
The policy should be to re-evaluate the rate based on constraints each
time the constraints or rate is changed. The clk provider can decide if
it wants to max it out or keep it minimized in the determine_rate
clk_op. If you're worried about breaking somebody, there's only one
other user I see in the tree, Tegra, so the risk seems low. If Tegra is
relying on the existing behavior (very possible) then we can hopefully
change that clk provider to keep the rate where it is if it's within the
constraints vs. round it down to the minimum acceptable.
>
> - How should we deal with a clk_set_rate call while we have a range?
> Something like:
>
> clk_set_min_rate(hdmi1_clk, 1000);
> clk_set_min_rate(hdmi2_clk, 2000)
> clk_set_rate(hdmi1_clk, 3000);
> clk_drop_range(hdmi2_clk);
>
> If we just try to minimize the rate all the time, we'll set the rate
> back to 1000Hz, but I think it would make more sense to keep the
> rate at 3000?
This assumes the rate after clk_set_rate() is 3000. It all depends on
the provider what the rate actually is.
>
> Maybe we can test if the range is still at the boundary we remove,
> and only if it is, drop it to whatever boundary we have left?
>
From the consumer API perspective it looks like hdmi1 is saying it can
accept a frequency as low as 1000 but would like 3000. The clk provider
will look at the constraints and decide to try to get close to 3000, or
exceed 3000, or set it lower than 3000 but not lower than 2000. I don't
want to enforce anything in the framework here. Let the clk provider
decide what frequency it should set the rate to based on the
constraints. When there are constraints mixed with a clk_set_rate() we
should give the provider all the facts, i.e. the rate and the
constraints and let it decide what to do.
Quoting Maxime Ripard (2022-01-14 08:15:56)
> On Thu, Jan 13, 2022 at 01:44:25PM -0800, Stephen Boyd wrote:
> >
> > I don't see a problem with re-evaluating the rate every time we call
> > clk_set_rate_range(). That's probably the bug that I can't recall. Can
> > you fix the API so it works that way?
>
> Yep, I'll work on it next week.
BTW, this is an area that's easily tested with kunit. I'm going to
re-post kunit tests for clk-gate.c today. We should add unit tests for
this and other parts of clk.c so that future changes don't break this.