Re: [PATCH v4 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode
From: Jagadeesh Kona
Date: Tue Feb 13 2024 - 08:07:34 EST
On 1/31/2024 6:35 AM, Bjorn Andersson wrote:
On Mon, Jan 22, 2024 at 10:47:05AM +0200, Abel Vesa wrote:
From: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
Use dev_pm_genpd_set_hwmode API to switch the vcodec gdsc to SW/HW
modes at runtime based on requirement for venus V6 variants.
Before the GDSC HWCTL was available to the consumer, the venus driver
needed to somehow keep the power from collapsing while under the driver
control. The only way to do that was to clear the CORE_PWR_DISABLE bit
(in wrapper POWER_CONTROL register) and, respectively, set it back after
the driver control was completed. Now, that there is a way to switch the
GDSC HW/SW control back and forth, the CORE_PWR_DISABLE toggling in
vcodec_control_v4() can be dropped for V6 variants.
The purpose of this commit is to warrant the need of this new mechanism,
but I don't find that it actually describes a problem to be solved.
With newer implementation, the mode of vcodec gdsc gets switched only in
Does "With newer implementation" mean "after these patches are applied"?
Thanks Bjorn for your review!
Yes, after all these patches are applied, will update the commit text to
be bit more precise.
set_hwmode API and the GDSC should not be switched to HW control mode
before turning off the GDSC, else subsequent GDSC enable may fail, hence
add check to avoid switching the GDSC to HW mode before powering off the
GDSC on V6 variants.
Is this saying that "if we return the GDSC to HW control after turning
off the clocks, it might not be possible to turn it on again"?
Yes, if the GDSC is left in HW control mode before GDSC disable, the
subsequent GDSC enable callback may fail while polling for GDSC status,
since HW can keep the GDSC in disabled state.
How come? Today this GDSC is operating in HW control mode, before,
during and after the clock operation.
Currently once GDSC is moved to HW control mode, Venus driver is using
it's POWER_CONTROL register to keep the GDSC ON before the clock
operations and reset it back after clock operations to handover control
back to HW.
And these venus POWER_CONTROL register addresses are not constant and
vary from one venus variant to other. With this new API Venus driver can
avoid these register writes and use this standard API everywhere to
switch the GDSC mode as required.
Signed-off-by: Jagadeesh Kona <quic_jkona@xxxxxxxxxxx>
Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
---
drivers/media/platform/qcom/venus/pm_helpers.c | 23 +++++++++++++----------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
index a1b127caa90a..55e8ec3f4ee9 100644
--- a/drivers/media/platform/qcom/venus/pm_helpers.c
+++ b/drivers/media/platform/qcom/venus/pm_helpers.c
@@ -412,10 +412,9 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
u32 val;
int ret;
- if (IS_V6(core)) {
- ctrl = core->wrapper_base + WRAPPER_CORE_POWER_CONTROL_V6;
- stat = core->wrapper_base + WRAPPER_CORE_POWER_STATUS_V6;
- } else if (coreid == VIDC_CORE_ID_1) {
+ if (IS_V6(core))
+ return dev_pm_genpd_set_hwmode(core->pmdomains[coreid], !enable);
+ else if (coreid == VIDC_CORE_ID_1) {
ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS;
} else {
@@ -451,9 +450,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
vcodec_clks_disable(core, core->vcodec0_clks);
- ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
- if (ret)
- return ret;
+ if (!IS_V6(core)) {
+ ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
First I had this expectation that the GDSC will always be in SW control
when the GDSC turns on - like the downstream implementation.
In this case I felt we should have a similar condition in
poweron_coreid() - as there's no point in switching to SW mode when we
know we're in SW mode already.
But as I finally realized that this is not the case, I now see that by
skipping the transition to HW mode here, dev_pm_genpd_set_hwmode() will
find the domain in SW mode, and through
if (dev_gpd_data(dev)->hw_mode == enable)
Will turn the vcodec_control_v4(, true) into a nop.
So, my first first instinct of feeling that this should be symmetric
between poweron/poweroff was reasonable...I think...
Yes, we can add similar check in poweron_coreid() also to be symmetric
but since it will be nop haven't added it. Shall I add similar check in
poweron_coreid() as well?
I find that this interface does not match the expectations that people
will bring from downstream and this example isn't helpful in explaining
how to use the new interface.
There are 3 consumers that currently use this HW control mode for
GDSC's:- display, camera and display.
Display driver is able to operate with GDSC always in HW mode. Camera
drivers don't have power saving features enabled on upstream yet and
hence not using the HW control mode of GDSC's currently, but will need
this API support to enable camera power saving features on upstream.
Currently on upstream, only venus driver requires GDSC HW and SW modes
switching, and hence added support in this driver to use the new interface.
PS. I trust there's no case whre legacy_binding = true, or that that
code path does not need similar workaround?
This change is applicable only to sc7280 and sm8250 targets for which
legacy_binding will be false.
Thanks,
Jagadeesh
Regards,
Bjorn
+ if (ret)
+ return ret;
+ }
ret = pm_runtime_put_sync(core->pmdomains[1]);
if (ret < 0)
@@ -467,9 +468,11 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
vcodec_clks_disable(core, core->vcodec1_clks);
- ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
- if (ret)
- return ret;
+ if (!IS_V6(core)) {
+ ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
+ if (ret)
+ return ret;
+ }
ret = pm_runtime_put_sync(core->pmdomains[2]);
if (ret < 0)
--
2.34.1