| Message ID | 20231101-gdsc-hwctrl-v3-4-0740ae6b2b04@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PM: domains: Add control for switching back and forth to HW control | expand |
On Wed, 1 Nov 2023 at 11:06, Abel Vesa <abel.vesa@linaro.org> wrote: > > From: Jagadeesh Kona <quic_jkona@quicinc.com> > > The current HW_CTRL flag switches the video GDSC to HW control mode as > part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to > give consumer drivers more control and switch the GDSC mode as and when > required. > > HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to > HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. Will existing venus driver continue to work with this patch being applied? > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/videocc-sc7180.c | 2 +- > drivers/clk/qcom/videocc-sc7280.c | 2 +- > drivers/clk/qcom/videocc-sdm845.c | 4 ++-- > drivers/clk/qcom/videocc-sm8250.c | 4 ++-- > drivers/clk/qcom/videocc-sm8550.c | 4 ++-- > 5 files changed, 8 insertions(+), 8 deletions(-)
On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote: > From: Jagadeesh Kona <quic_jkona@quicinc.com> > > The current HW_CTRL flag switches the video GDSC to HW control mode as > part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to > give consumer drivers more control and switch the GDSC mode as and when > required. > > HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to > HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. > This states what the code currently does, and what the new code will do. But I don't find that it adequately describes _why_ this is done. In the current implementation, the hardware is might collapse the GDSC anytime between gdsc_enable() and gdsc_disable(). By giving "drivers more control" the time spent in this state is reduced to some fraction of that span, which to me implies higher power consumption. Under the assumption that we don't want to consume more power without reason, I'm forced to guess that there might be some scenarios that we want this feature to keep the GDSC non-collapsed against the indication of the hardware - to avoid some instability somewhere, perhaps? Regards, Bjorn > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/videocc-sc7180.c | 2 +- > drivers/clk/qcom/videocc-sc7280.c | 2 +- > drivers/clk/qcom/videocc-sdm845.c | 4 ++-- > drivers/clk/qcom/videocc-sm8250.c | 4 ++-- > drivers/clk/qcom/videocc-sm8550.c | 4 ++-- > 5 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c > index 5b9b54f616b8..51439f7ba70c 100644 > --- a/drivers/clk/qcom/videocc-sc7180.c > +++ b/drivers/clk/qcom/videocc-sc7180.c > @@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = { > .pd = { > .name = "vcodec0_gdsc", > }, > - .flags = HW_CTRL, > + .flags = HW_CTRL_TRIGGER, > .pwrsts = PWRSTS_OFF_ON, > }; > > diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c > index 615695d82319..3d07b1e95986 100644 > --- a/drivers/clk/qcom/videocc-sc7280.c > +++ b/drivers/clk/qcom/videocc-sc7280.c > @@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = { > .name = "mvs0_gdsc", > }, > .pwrsts = PWRSTS_OFF_ON, > - .flags = HW_CTRL | RETAIN_FF_ENABLE, > + .flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE, > }; > > static struct gdsc mvsc_gdsc = { > diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c > index c77a4dd5d39c..dad011c48973 100644 > --- a/drivers/clk/qcom/videocc-sdm845.c > +++ b/drivers/clk/qcom/videocc-sdm845.c > @@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = { > }, > .cxcs = (unsigned int []){ 0x890, 0x930 }, > .cxc_count = 2, > - .flags = HW_CTRL | POLL_CFG_GDSCR, > + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > }; > > @@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = { > }, > .cxcs = (unsigned int []){ 0x8d0, 0x950 }, > .cxc_count = 2, > - .flags = HW_CTRL | POLL_CFG_GDSCR, > + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, > .pwrsts = PWRSTS_OFF_ON, > }; > > diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c > index ad46c4014a40..c1b73d852f1c 100644 > --- a/drivers/clk/qcom/videocc-sm8250.c > +++ b/drivers/clk/qcom/videocc-sm8250.c > @@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = { > .pd = { > .name = "mvs0_gdsc", > }, > - .flags = HW_CTRL, > + .flags = HW_CTRL_TRIGGER, > .pwrsts = PWRSTS_OFF_ON, > }; > > @@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = { > .pd = { > .name = "mvs1_gdsc", > }, > - .flags = HW_CTRL, > + .flags = HW_CTRL_TRIGGER, > .pwrsts = PWRSTS_OFF_ON, > }; > > diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c > index f3c9dfaee968..404c6600edae 100644 > --- a/drivers/clk/qcom/videocc-sm8550.c > +++ b/drivers/clk/qcom/videocc-sm8550.c > @@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = { > }, > .pwrsts = PWRSTS_OFF_ON, > .parent = &video_cc_mvs0c_gdsc.pd, > - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, > + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, > }; > > static struct gdsc video_cc_mvs1c_gdsc = { > @@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = { > }, > .pwrsts = PWRSTS_OFF_ON, > .parent = &video_cc_mvs1c_gdsc.pd, > - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, > + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, > }; > > static struct clk_regmap *video_cc_sm8550_clocks[] = { > > -- > 2.34.1 >
On 11/7/23 14:05, Bryan O'Donoghue wrote: > On 01/11/2023 09:04, Abel Vesa wrote: >> From: Jagadeesh Kona <quic_jkona@quicinc.com> >> >> The current HW_CTRL flag switches the video GDSC to HW control mode as >> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to >> give consumer drivers more control and switch the GDSC mode as and when >> required. >> >> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to >> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> drivers/clk/qcom/videocc-sc7180.c | 2 +- >> drivers/clk/qcom/videocc-sc7280.c | 2 +- >> drivers/clk/qcom/videocc-sdm845.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8250.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8550.c | 4 ++-- >> 5 files changed, 8 insertions(+), 8 deletions(-) > > So. > > I'm assuming the rest of this series works however for sc8250 at least this is a NAK, breaks venus on rb5. Are you saying that applying this patch alone causes the attached crash? Konrad
On 07/11/2023 21:21, Konrad Dybcio wrote: >> >> So. >> >> I'm assuming the rest of this series works however for sc8250 at least >> this is a NAK, breaks venus on rb5. > Are you saying that applying this patch alone causes the attached crash? > > Konrad I tried to revert this patch in isolation and got -> [ 157.083287] qcom-venus aa00000.video-codec: Failed to switch power-domain:1 to SW mode [ 162.004630] qcom-venus aa00000.video-codec: Failed to switch power-domain:1 to SW mode I stopped debugging there. --- bod
On 11/7/2023 6:35 PM, Bryan O'Donoghue wrote: > On 01/11/2023 09:04, Abel Vesa wrote: >> From: Jagadeesh Kona <quic_jkona@quicinc.com> >> >> The current HW_CTRL flag switches the video GDSC to HW control mode as >> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to >> give consumer drivers more control and switch the GDSC mode as and when >> required. >> >> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to >> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> drivers/clk/qcom/videocc-sc7180.c | 2 +- >> drivers/clk/qcom/videocc-sc7280.c | 2 +- >> drivers/clk/qcom/videocc-sdm845.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8250.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8550.c | 4 ++-- >> 5 files changed, 8 insertions(+), 8 deletions(-) > > So. > > I'm assuming the rest of this series works however for sc8250 at least > this is a NAK, breaks venus on rb5. > > Reproduction: > > - Debian trixie rb5 > - Linux 6.6-rc3 + this patch > - Attached defconfig > - This file: > https://download.samplelib.com/mp4/sample-30s.mp4 > - This command: > ffplay -loglevel debug -code:video h264_v4l2m2m -i sample-30s.mp4 > > Second play of file shows stuck gdsc as below > > I didn't try on rb3, I'd expect similar results. Does this work on 8550 ? > > [ 1601.581204] ------------[ cut here ]------------ > [ 1601.585983] mvs0_gdsc status stuck at 'off' > [ 1601.586015] WARNING: CPU: 1 PID: 13372 at drivers/clk/qcom/gdsc.c:178 > gdsc_toggle_logic+0x16c/0x174 > [ 1601.599627] Modules linked in: nf_tables libcrc32c nfnetlink > snd_soc_hdmi_codec q6asm_dai q6routing q6afe_dai q6asm q6adm > q6afe_clocks snd_q6dsp_common q6afe q6core apr pdr_interface venus_enc > venus_dec qcom_camss videobuf2_dma_contig mcp251xfd imx412 > videobuf2_dma_sg venus_core xhci_plat_hcd v4l2_fwnode fastrpc xhci_hcd > can_dev qrtr_smd lontium_lt9611uxc msm v4l2_async v4l2_mem2mem > qcom_spmi_adc_tm5 rtc_pm8xxx qcom_spmi_adc5 qcom_pon videobuf2_memops > crct10dif_ce qcom_spmi_temp_alarm videobuf2_v4l2 qcom_vadc_common > gpu_sched drm_dp_aux_bus videodev snd_soc_sm8250 drm_display_helper > snd_soc_qcom_sdw videobuf2_common snd_soc_qcom_common qrtr i2c_qcom_cci > soundwire_qcom mc i2c_qcom_geni spi_geni_qcom phy_qcom_qmp_combo > qcom_q6v5_pas qcom_rng soundwire_bus qcom_pil_info > snd_soc_lpass_va_macro qcom_q6v5 slimbus snd_soc_lpass_macro_common > qcom_sysmon snd_soc_lpass_wsa_macro display_connector qcom_common > socinfo qcom_glink_smem qmi_helpers drm_kms_helper mdt_loader qcom_wdt > icc_osm_l3 qnoc_sm8250 fuse drm backlight dm_mod > [ 1601.599859] ip_tables x_tables > [ 1601.695314] CPU: 1 PID: 13372 Comm: video_decoder Not tainted > 6.6.0-rc3-00396-gdbc0d9fa7641-dirty #1 > [ 1601.704694] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT) > [ 1601.711582] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS > BTYPE=--) > [ 1601.718740] pc : gdsc_toggle_logic+0x16c/0x174 > [ 1601.723317] lr : gdsc_toggle_logic+0x16c/0x174 > [ 1601.727888] sp : ffff80008adab800 > [ 1601.731296] x29: ffff80008adab800 x28: ffffb661e8596210 x27: > ffffb661e855ad88 > [ 1601.738629] x26: 0000000000000000 x25: ffff100c0b5a0d28 x24: > ffffb6620fd92118 > [ 1601.745960] x23: ffffb6620fe1d3d8 x22: 0000000000000000 x21: > 0000000000000001 > [ 1601.753292] x20: 00000000ffffff92 x19: ffffb6620fd92118 x18: > ffffffffffc0d3e8 > [ 1601.760631] x17: 0000000000000000 x16: ffffb6620e269e14 x15: > 0000000000000028 > [ 1601.767973] x14: 0000000000000000 x13: ffff100d75c00000 x12: > 0000000000000894 > [ 1601.775304] x11: 00000000000002dc x10: ffff100d767044a0 x9 : > ffff100d75c00000 > [ 1601.782636] x8 : 00000000fffdffff x7 : ffff100d76700000 x6 : > 00000000000002dc > [ 1601.789976] x5 : ffff100d7ef40d48 x4 : 40000000fffe02dc x3 : > ffff59ab6fa21000 > [ 1601.797306] x2 : 0000000000000000 x1 : 0000000000000000 x0 : > ffff100cdacdec80 > [ 1601.804638] Call trace: > [ 1601.807161] gdsc_toggle_logic+0x16c/0x174 > [ 1601.811383] gdsc_enable+0x60/0x27c > [ 1601.814982] _genpd_power_on+0x94/0x184 > [ 1601.818931] genpd_power_on+0xa8/0x16c > [ 1601.822791] genpd_runtime_resume+0xd4/0x2a4 > [ 1601.827184] __rpm_callback+0x48/0x1dc > [ 1601.831045] rpm_callback+0x6c/0x78 > [ 1601.834638] rpm_resume+0x45c/0x6c8 > [ 1601.838231] __pm_runtime_resume+0x4c/0x90 > [ 1601.842443] coreid_power_v4+0x378/0x58c [venus_core] > [ 1601.847695] vdec_start_streaming+0xc0/0x4e8 [venus_dec] > [ 1601.853169] vb2_start_streaming+0x68/0x15c [videobuf2_common] > [ 1601.859199] vb2_core_streamon+0xf8/0x1bc [videobuf2_common] > [ 1601.865032] vb2_streamon+0x18/0x64 [videobuf2_v4l2] > [ 1601.870174] v4l2_m2m_ioctl_streamon+0x38/0x98 [v4l2_mem2mem] > [ 1601.876134] v4l_streamon+0x24/0x30 [videodev] > [ 1601.880759] __video_do_ioctl+0x15c/0x3c0 [videodev] > [ 1601.885905] video_usercopy+0x1f0/0x658 [videodev] > [ 1601.890868] video_ioctl2+0x18/0x28 [videodev] > [ 1601.895481] v4l2_ioctl+0x40/0x60 [videodev] > [ 1601.899911] __arm64_sys_ioctl+0xac/0xf0 > [ 1601.903958] invoke_syscall+0x48/0x114 > [ 1601.907829] el0_svc_common.constprop.0+0x40/0xe0 > [ 1601.912672] do_el0_svc+0x1c/0x28 > [ 1601.916085] el0_svc+0x40/0xe8 > [ 1601.919243] el0t_64_sync_handler+0x100/0x12c > [ 1601.923730] el0t_64_sync+0x190/0x194 > [ 1601.927505] ---[ end trace 0000000000000000 ]--- > [ 1608.121533] ------------[ cut here ]------------ > > And just reverting the one patch - yields > > [ 157.083287] qcom-venus aa00000.video-codec: Failed to switch > power-domain:1 to SW mode > [ 162.004630] qcom-venus aa00000.video-codec: Failed to switch > power-domain:1 to SW mode > > I'll leave the testing here. Please fix ! > Thanks Bryan for reporting this, this could be happening since GDSC might be left in HW control mode during power off sequence, so the subsequent GDSC enable is failing since GDSC is still in HW mode. I am checking internally on this and will get back. Thanks, Jagadeesh > --- > bod
On 11/4/2023 1:45 AM, Bjorn Andersson wrote: > On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote: >> From: Jagadeesh Kona <quic_jkona@quicinc.com> >> >> The current HW_CTRL flag switches the video GDSC to HW control mode as >> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to >> give consumer drivers more control and switch the GDSC mode as and when >> required. >> >> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to >> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. >> > > This states what the code currently does, and what the new code will do. > But I don't find that it adequately describes _why_ this is done. > > > In the current implementation, the hardware is might collapse the GDSC > anytime between gdsc_enable() and gdsc_disable(). By giving "drivers > more control" the time spent in this state is reduced to some fraction > of that span, which to me implies higher power consumption. > > Under the assumption that we don't want to consume more power without > reason, I'm forced to guess that there might be some scenarios that we > want this feature to keep the GDSC non-collapsed against the indication > of the hardware - to avoid some instability somewhere, perhaps? > Thanks Bjorn for your review. Sure, will update commit text with details in next series. Normally, consumers will enable the GDSC and then the required clocks. If GDSC is moved to HW mode in gdsc_enable() itself, the subsequent clocks enablement that are dependent on GDSC might fail since GDSC could be turned off by HW. The consumers can still switch the GDSC to HW mode with new API right after the clocks are enabled and the control will be taken back to SW mode just before disabling the GDSC, so even with the newer implementation, HW can collapse the GDSC anytime for most of the duration between gdsc_enable() and gdsc_disable(). This API adds more flexibility for consumer drivers to control the GDSC mode as per their requirements. Thanks, Jagadeesh > Regards, > Bjorn > >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >> --- >> drivers/clk/qcom/videocc-sc7180.c | 2 +- >> drivers/clk/qcom/videocc-sc7280.c | 2 +- >> drivers/clk/qcom/videocc-sdm845.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8250.c | 4 ++-- >> drivers/clk/qcom/videocc-sm8550.c | 4 ++-- >> 5 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c >> index 5b9b54f616b8..51439f7ba70c 100644 >> --- a/drivers/clk/qcom/videocc-sc7180.c >> +++ b/drivers/clk/qcom/videocc-sc7180.c >> @@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = { >> .pd = { >> .name = "vcodec0_gdsc", >> }, >> - .flags = HW_CTRL, >> + .flags = HW_CTRL_TRIGGER, >> .pwrsts = PWRSTS_OFF_ON, >> }; >> >> diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c >> index 615695d82319..3d07b1e95986 100644 >> --- a/drivers/clk/qcom/videocc-sc7280.c >> +++ b/drivers/clk/qcom/videocc-sc7280.c >> @@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = { >> .name = "mvs0_gdsc", >> }, >> .pwrsts = PWRSTS_OFF_ON, >> - .flags = HW_CTRL | RETAIN_FF_ENABLE, >> + .flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE, >> }; >> >> static struct gdsc mvsc_gdsc = { >> diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c >> index c77a4dd5d39c..dad011c48973 100644 >> --- a/drivers/clk/qcom/videocc-sdm845.c >> +++ b/drivers/clk/qcom/videocc-sdm845.c >> @@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = { >> }, >> .cxcs = (unsigned int []){ 0x890, 0x930 }, >> .cxc_count = 2, >> - .flags = HW_CTRL | POLL_CFG_GDSCR, >> + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, >> .pwrsts = PWRSTS_OFF_ON, >> }; >> >> @@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = { >> }, >> .cxcs = (unsigned int []){ 0x8d0, 0x950 }, >> .cxc_count = 2, >> - .flags = HW_CTRL | POLL_CFG_GDSCR, >> + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, >> .pwrsts = PWRSTS_OFF_ON, >> }; >> >> diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c >> index ad46c4014a40..c1b73d852f1c 100644 >> --- a/drivers/clk/qcom/videocc-sm8250.c >> +++ b/drivers/clk/qcom/videocc-sm8250.c >> @@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = { >> .pd = { >> .name = "mvs0_gdsc", >> }, >> - .flags = HW_CTRL, >> + .flags = HW_CTRL_TRIGGER, >> .pwrsts = PWRSTS_OFF_ON, >> }; >> >> @@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = { >> .pd = { >> .name = "mvs1_gdsc", >> }, >> - .flags = HW_CTRL, >> + .flags = HW_CTRL_TRIGGER, >> .pwrsts = PWRSTS_OFF_ON, >> }; >> >> diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c >> index f3c9dfaee968..404c6600edae 100644 >> --- a/drivers/clk/qcom/videocc-sm8550.c >> +++ b/drivers/clk/qcom/videocc-sm8550.c >> @@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = { >> }, >> .pwrsts = PWRSTS_OFF_ON, >> .parent = &video_cc_mvs0c_gdsc.pd, >> - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, >> + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, >> }; >> >> static struct gdsc video_cc_mvs1c_gdsc = { >> @@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = { >> }, >> .pwrsts = PWRSTS_OFF_ON, >> .parent = &video_cc_mvs1c_gdsc.pd, >> - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, >> + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, >> }; >> >> static struct clk_regmap *video_cc_sm8550_clocks[] = { >> >> -- >> 2.34.1 >>
On 10/11/2023 08:29, Jagadeesh Kona wrote: > > > On 11/7/2023 6:35 PM, Bryan O'Donoghue wrote: >> On 01/11/2023 09:04, Abel Vesa wrote: >>> From: Jagadeesh Kona <quic_jkona@quicinc.com> >>> >>> The current HW_CTRL flag switches the video GDSC to HW control mode as >>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to >>> give consumer drivers more control and switch the GDSC mode as and when >>> required. >>> >>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to >>> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. >>> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>> --- >>> drivers/clk/qcom/videocc-sc7180.c | 2 +- >>> drivers/clk/qcom/videocc-sc7280.c | 2 +- >>> drivers/clk/qcom/videocc-sdm845.c | 4 ++-- >>> drivers/clk/qcom/videocc-sm8250.c | 4 ++-- >>> drivers/clk/qcom/videocc-sm8550.c | 4 ++-- >>> 5 files changed, 8 insertions(+), 8 deletions(-) >> >> So. >> >> I'm assuming the rest of this series works however for sc8250 at least >> this is a NAK, breaks venus on rb5. >> >> Reproduction: >> >> - Debian trixie rb5 >> - Linux 6.6-rc3 + this patch >> - Attached defconfig >> - This file: >> https://download.samplelib.com/mp4/sample-30s.mp4 >> - This command: >> ffplay -loglevel debug -code:video h264_v4l2m2m -i sample-30s.mp4 >> >> Second play of file shows stuck gdsc as below >> >> I didn't try on rb3, I'd expect similar results. Does this work on 8550 ? >> >> [ 1601.581204] ------------[ cut here ]------------ >> [ 1601.585983] mvs0_gdsc status stuck at 'off' >> [ 1601.586015] WARNING: CPU: 1 PID: 13372 at >> drivers/clk/qcom/gdsc.c:178 gdsc_toggle_logic+0x16c/0x174 >> [ 1601.599627] Modules linked in: nf_tables libcrc32c nfnetlink >> snd_soc_hdmi_codec q6asm_dai q6routing q6afe_dai q6asm q6adm >> q6afe_clocks snd_q6dsp_common q6afe q6core apr pdr_interface venus_enc >> venus_dec qcom_camss videobuf2_dma_contig mcp251xfd imx412 >> videobuf2_dma_sg venus_core xhci_plat_hcd v4l2_fwnode fastrpc xhci_hcd >> can_dev qrtr_smd lontium_lt9611uxc msm v4l2_async v4l2_mem2mem >> qcom_spmi_adc_tm5 rtc_pm8xxx qcom_spmi_adc5 qcom_pon videobuf2_memops >> crct10dif_ce qcom_spmi_temp_alarm videobuf2_v4l2 qcom_vadc_common >> gpu_sched drm_dp_aux_bus videodev snd_soc_sm8250 drm_display_helper >> snd_soc_qcom_sdw videobuf2_common snd_soc_qcom_common qrtr >> i2c_qcom_cci soundwire_qcom mc i2c_qcom_geni spi_geni_qcom >> phy_qcom_qmp_combo qcom_q6v5_pas qcom_rng soundwire_bus qcom_pil_info >> snd_soc_lpass_va_macro qcom_q6v5 slimbus snd_soc_lpass_macro_common >> qcom_sysmon snd_soc_lpass_wsa_macro display_connector qcom_common >> socinfo qcom_glink_smem qmi_helpers drm_kms_helper mdt_loader qcom_wdt >> icc_osm_l3 qnoc_sm8250 fuse drm backlight dm_mod >> [ 1601.599859] ip_tables x_tables >> [ 1601.695314] CPU: 1 PID: 13372 Comm: video_decoder Not tainted >> 6.6.0-rc3-00396-gdbc0d9fa7641-dirty #1 >> [ 1601.704694] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 >> (DT) >> [ 1601.711582] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS >> BTYPE=--) >> [ 1601.718740] pc : gdsc_toggle_logic+0x16c/0x174 >> [ 1601.723317] lr : gdsc_toggle_logic+0x16c/0x174 >> [ 1601.727888] sp : ffff80008adab800 >> [ 1601.731296] x29: ffff80008adab800 x28: ffffb661e8596210 x27: >> ffffb661e855ad88 >> [ 1601.738629] x26: 0000000000000000 x25: ffff100c0b5a0d28 x24: >> ffffb6620fd92118 >> [ 1601.745960] x23: ffffb6620fe1d3d8 x22: 0000000000000000 x21: >> 0000000000000001 >> [ 1601.753292] x20: 00000000ffffff92 x19: ffffb6620fd92118 x18: >> ffffffffffc0d3e8 >> [ 1601.760631] x17: 0000000000000000 x16: ffffb6620e269e14 x15: >> 0000000000000028 >> [ 1601.767973] x14: 0000000000000000 x13: ffff100d75c00000 x12: >> 0000000000000894 >> [ 1601.775304] x11: 00000000000002dc x10: ffff100d767044a0 x9 : >> ffff100d75c00000 >> [ 1601.782636] x8 : 00000000fffdffff x7 : ffff100d76700000 x6 : >> 00000000000002dc >> [ 1601.789976] x5 : ffff100d7ef40d48 x4 : 40000000fffe02dc x3 : >> ffff59ab6fa21000 >> [ 1601.797306] x2 : 0000000000000000 x1 : 0000000000000000 x0 : >> ffff100cdacdec80 >> [ 1601.804638] Call trace: >> [ 1601.807161] gdsc_toggle_logic+0x16c/0x174 >> [ 1601.811383] gdsc_enable+0x60/0x27c >> [ 1601.814982] _genpd_power_on+0x94/0x184 >> [ 1601.818931] genpd_power_on+0xa8/0x16c >> [ 1601.822791] genpd_runtime_resume+0xd4/0x2a4 >> [ 1601.827184] __rpm_callback+0x48/0x1dc >> [ 1601.831045] rpm_callback+0x6c/0x78 >> [ 1601.834638] rpm_resume+0x45c/0x6c8 >> [ 1601.838231] __pm_runtime_resume+0x4c/0x90 >> [ 1601.842443] coreid_power_v4+0x378/0x58c [venus_core] >> [ 1601.847695] vdec_start_streaming+0xc0/0x4e8 [venus_dec] >> [ 1601.853169] vb2_start_streaming+0x68/0x15c [videobuf2_common] >> [ 1601.859199] vb2_core_streamon+0xf8/0x1bc [videobuf2_common] >> [ 1601.865032] vb2_streamon+0x18/0x64 [videobuf2_v4l2] >> [ 1601.870174] v4l2_m2m_ioctl_streamon+0x38/0x98 [v4l2_mem2mem] >> [ 1601.876134] v4l_streamon+0x24/0x30 [videodev] >> [ 1601.880759] __video_do_ioctl+0x15c/0x3c0 [videodev] >> [ 1601.885905] video_usercopy+0x1f0/0x658 [videodev] >> [ 1601.890868] video_ioctl2+0x18/0x28 [videodev] >> [ 1601.895481] v4l2_ioctl+0x40/0x60 [videodev] >> [ 1601.899911] __arm64_sys_ioctl+0xac/0xf0 >> [ 1601.903958] invoke_syscall+0x48/0x114 >> [ 1601.907829] el0_svc_common.constprop.0+0x40/0xe0 >> [ 1601.912672] do_el0_svc+0x1c/0x28 >> [ 1601.916085] el0_svc+0x40/0xe8 >> [ 1601.919243] el0t_64_sync_handler+0x100/0x12c >> [ 1601.923730] el0t_64_sync+0x190/0x194 >> [ 1601.927505] ---[ end trace 0000000000000000 ]--- >> [ 1608.121533] ------------[ cut here ]------------ >> >> And just reverting the one patch - yields >> >> [ 157.083287] qcom-venus aa00000.video-codec: Failed to switch >> power-domain:1 to SW mode >> [ 162.004630] qcom-venus aa00000.video-codec: Failed to switch >> power-domain:1 to SW mode >> >> I'll leave the testing here. Please fix ! >> > > Thanks Bryan for reporting this, this could be happening since GDSC > might be left in HW control mode during power off sequence, so the > subsequent GDSC enable is failing since GDSC is still in HW mode. I am > checking internally on this and will get back. Great, Please remember to check for rb3 / sdm845 too. --- bod
On 11/10/2023 2:02 PM, Jagadeesh Kona wrote: > > > On 11/4/2023 1:45 AM, Bjorn Andersson wrote: >> On Wed, Nov 01, 2023 at 11:04:10AM +0200, Abel Vesa wrote: >>> From: Jagadeesh Kona <quic_jkona@quicinc.com> >>> >>> The current HW_CTRL flag switches the video GDSC to HW control mode as >>> part of GDSC enable itself, instead of that use HW_CTRL_TRIGGER flag to >>> give consumer drivers more control and switch the GDSC mode as and when >>> required. >>> >>> HW_CTRL_TRIGGER flag allows consumer drivers to switch the video GDSC to >>> HW/SW control modes at runtime using dev_pm_genpd_set_hwmode API. >>> >> >> This states what the code currently does, and what the new code will do. >> But I don't find that it adequately describes _why_ this is done. >> >> >> In the current implementation, the hardware is might collapse the GDSC >> anytime between gdsc_enable() and gdsc_disable(). By giving "drivers >> more control" the time spent in this state is reduced to some fraction >> of that span, which to me implies higher power consumption. >> >> Under the assumption that we don't want to consume more power without >> reason, I'm forced to guess that there might be some scenarios that we >> want this feature to keep the GDSC non-collapsed against the indication >> of the hardware - to avoid some instability somewhere, perhaps? >> > > Thanks Bjorn for your review. Sure, will update commit text with details in next > series. > > Normally, consumers will enable the GDSC and then the required clocks. If GDSC > is moved to HW mode in gdsc_enable() itself, the subsequent clocks enablement > that are dependent on GDSC might fail since GDSC could be turned off by HW. The > consumers can still switch the GDSC to HW mode with new API right after the > clocks are enabled and the control will be taken back to SW mode just before > disabling the GDSC, so even with the newer implementation, HW can collapse the > GDSC anytime for most of the duration between gdsc_enable() and gdsc_disable(). > This API adds more flexibility for consumer drivers to control the GDSC mode as > per their > requirements. There is one more scenario where the driver would like GDSC in driver controlled. Let say video hardware, which is under vcodec0_gdsc, have registers to be programmed by TZ. In such scenario, the GDSC should be non collapsed, while TZ programs those registers precisely while loading the firmware and bringing hardware out of reset. Regards, Vikash
diff --git a/drivers/clk/qcom/videocc-sc7180.c b/drivers/clk/qcom/videocc-sc7180.c index 5b9b54f616b8..51439f7ba70c 100644 --- a/drivers/clk/qcom/videocc-sc7180.c +++ b/drivers/clk/qcom/videocc-sc7180.c @@ -166,7 +166,7 @@ static struct gdsc vcodec0_gdsc = { .pd = { .name = "vcodec0_gdsc", }, - .flags = HW_CTRL, + .flags = HW_CTRL_TRIGGER, .pwrsts = PWRSTS_OFF_ON, }; diff --git a/drivers/clk/qcom/videocc-sc7280.c b/drivers/clk/qcom/videocc-sc7280.c index 615695d82319..3d07b1e95986 100644 --- a/drivers/clk/qcom/videocc-sc7280.c +++ b/drivers/clk/qcom/videocc-sc7280.c @@ -236,7 +236,7 @@ static struct gdsc mvs0_gdsc = { .name = "mvs0_gdsc", }, .pwrsts = PWRSTS_OFF_ON, - .flags = HW_CTRL | RETAIN_FF_ENABLE, + .flags = HW_CTRL_TRIGGER | RETAIN_FF_ENABLE, }; static struct gdsc mvsc_gdsc = { diff --git a/drivers/clk/qcom/videocc-sdm845.c b/drivers/clk/qcom/videocc-sdm845.c index c77a4dd5d39c..dad011c48973 100644 --- a/drivers/clk/qcom/videocc-sdm845.c +++ b/drivers/clk/qcom/videocc-sdm845.c @@ -260,7 +260,7 @@ static struct gdsc vcodec0_gdsc = { }, .cxcs = (unsigned int []){ 0x890, 0x930 }, .cxc_count = 2, - .flags = HW_CTRL | POLL_CFG_GDSCR, + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, }; @@ -271,7 +271,7 @@ static struct gdsc vcodec1_gdsc = { }, .cxcs = (unsigned int []){ 0x8d0, 0x950 }, .cxc_count = 2, - .flags = HW_CTRL | POLL_CFG_GDSCR, + .flags = HW_CTRL_TRIGGER | POLL_CFG_GDSCR, .pwrsts = PWRSTS_OFF_ON, }; diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c index ad46c4014a40..c1b73d852f1c 100644 --- a/drivers/clk/qcom/videocc-sm8250.c +++ b/drivers/clk/qcom/videocc-sm8250.c @@ -293,7 +293,7 @@ static struct gdsc mvs0_gdsc = { .pd = { .name = "mvs0_gdsc", }, - .flags = HW_CTRL, + .flags = HW_CTRL_TRIGGER, .pwrsts = PWRSTS_OFF_ON, }; @@ -302,7 +302,7 @@ static struct gdsc mvs1_gdsc = { .pd = { .name = "mvs1_gdsc", }, - .flags = HW_CTRL, + .flags = HW_CTRL_TRIGGER, .pwrsts = PWRSTS_OFF_ON, }; diff --git a/drivers/clk/qcom/videocc-sm8550.c b/drivers/clk/qcom/videocc-sm8550.c index f3c9dfaee968..404c6600edae 100644 --- a/drivers/clk/qcom/videocc-sm8550.c +++ b/drivers/clk/qcom/videocc-sm8550.c @@ -322,7 +322,7 @@ static struct gdsc video_cc_mvs0_gdsc = { }, .pwrsts = PWRSTS_OFF_ON, .parent = &video_cc_mvs0c_gdsc.pd, - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, }; static struct gdsc video_cc_mvs1c_gdsc = { @@ -347,7 +347,7 @@ static struct gdsc video_cc_mvs1_gdsc = { }, .pwrsts = PWRSTS_OFF_ON, .parent = &video_cc_mvs1c_gdsc.pd, - .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL, + .flags = POLL_CFG_GDSCR | RETAIN_FF_ENABLE | HW_CTRL_TRIGGER, }; static struct clk_regmap *video_cc_sm8550_clocks[] = {