Messages in this thread | | | Date | Wed, 24 Apr 2024 01:16:26 +0100 | Subject | Re: [PATCH V5 RESEND 5/5] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V6 | From | Bryan O'Donoghue <> |
| |
On 14/04/2024 15:52, Jagadeesh Kona wrote: > > > On 4/14/2024 6:09 PM, Bryan O'Donoghue wrote: >> On 13/04/2024 16:20, Jagadeesh Kona wrote: >>> The Venus driver requires vcodec GDSC to be ON in SW mode for clock >>> operations and move it back to HW mode to gain power benefits. Earlier, >>> as there is no interface to switch the GDSC mode from GenPD framework, >>> the GDSC is moved to HW control mode as part of GDSC enable callback and >>> venus driver is writing to its POWER_CONTROL register to keep the >>> GDSC ON >>> from SW whereever required. But the POWER_CONTROL register addresses >>> are not constant and can vary across the variants. >>> >>> Also as per the HW recommendation, the GDSC mode switching needs to be >>> controlled from respective GDSC register and this is a uniform approach >>> across all the targets. Hence use dev_pm_genpd_set_hwmode() API which >>> controls GDSC mode switching using its respective GDSC register. >>> >>> In venus V6 variants, the vcodec gdsc gets enabled in SW mode by default >>> with new HW_CTRL_TRIGGER flag and there is no need to switch it to SW >>> mode again after enable, hence add check to avoid switching gdsc to >>> SW mode >>> again after gdsc enable. Similarly add check to avoid switching GDSC >>> to HW >>> mode before disabling the GDSC, so GDSC gets enabled in SW mode in >>> the next >>> enable. >>> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>> --- >> >> When I tested this out on sm8250 a few months ago it was broken. >> >> I don't quite see in your commit logs, how the breakage was addressed. >> >> Can you provide some details ? >> > > Thanks Bryan for your review! > > In earlier series, venus driver is switching the vcodec GDSC to HW > control mode before disabling the GDSC by invoking > vcodec_control_v4(..., false) in poweroff_coreid(). Due to this, the > subsequent GDSC enable from venus driver is failing while polling for > GDSC power ON status, since GDSC is under HW control mode and HW can > keep the GDSC in disabled state. > > Now a check is added in poweroff_coreid() to avoid switching the GDSC to > HW control mode before disabling the GDSC for Venus V6 variants that use > this new API. Hence during the next GDSC enable, GDSC will be in SW mode > and GDSC will powerup properly.
Right so the intention is to have HW GDSC control during playback only - and then revert to SW control when no stream is active, right ?
I tried your series on today's -next.
Here is -next without your changes
https://drive.google.com/file/d/1PFuLOlEp582rBQUvuwc9PNZUBxn1ioYf/view?usp=sharing
and here is -next with your changes
https://drive.google.com/file/d/1PHR4rZnWUH9Wp2B-itT5yCUXIMOMZrwM/view?usp=sharing
The first time I tried that test the stopping/stuttering was worse.
So yes the original crash was fixed but, this looks like a performance regression to me.
Here's the tree I tested with.
https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-24-05-23-review?ref_type=heads
--- bod
| |