Re: [PATCH v3 0/3] Use APIs in gdsc genpd to switch gdsc mode for venus v4 core

From: Dmitry Baryshkov
Date: Wed Jan 15 2025 - 05:00:04 EST


On Wed, Jan 15, 2025 at 03:00:51PM +0530, Renjiang Han 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.
>
> If only the clock patch or the venus driver patch is merged, the venus
> driver will not work properly. Although it does not affect other system
> functions, it is still recommended to wait until both the clock patch
> and the venus driver patch are reviewed and passed, and then merge them
> into the same release by their respective maintainers.

NO! This will not work, as both -media and arm-soc branches will be
broken. Please read about the git-bisect before making such suggestions.

A proper plan would be:
- implement a function which allows one to check that hwmode is
supported by the genpd driver
- Change Venus to use hwmode for those platforms if it is enabled
- Enable HWMODE support in the clock driver. Clearly identify that this
patch should be merged together and after Venus changes if all
maintainers agree with that
- Clean up now-dead code.

Doing it in any other way would result in the broken kernels.

>
> Validated this series on QCS615 and SC7180.
>
> Signed-off-by: Renjiang Han <quic_renjiang@xxxxxxxxxxx>
> ---
> Changes in v3:
> - 1. Update commit message.
> - 2. Add a patch to clean up the dead code for the venus driver.
> - 3. Remove vcodec_control_v4() function.
> - 4. Directly call dev_pm_genpd_set_hwmode() without vcodec_control_v4().
> - Link to v2: https://lore.kernel.org/r/20241223-switch_gdsc_mode-v2-0-eb5c96aee662@xxxxxxxxxxx
>
> Changes in v2:
> - 1. Add the HW_CTRL_TRIGGER flag for the targets SM7150/SM8150 and SM8450
> video GDSCs supporting movement between HW and SW mode of the GDSC.
> (Suggested by Dmitry Baryshkov)
> - 2. There is a dependency of the clock driver introducing the new flag
> and the video driver adapting to this new API. Missing either the clock
> and video driver could potentially break the video driver.
> - Link to v1: https://lore.kernel.org/r/20241122-switch_gdsc_mode-v1-0-365f097ecbb0@xxxxxxxxxxx
>
> ---
> Renjiang Han (2):
> venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4
> venus: pm_helpers: Remove dead code and simplify power management
>
> Taniya Das (1):
> clk: qcom: videocc: Use HW_CTRL_TRIGGER flag for video GDSC's
>
> drivers/clk/qcom/videocc-sc7180.c | 2 +-
> drivers/clk/qcom/videocc-sdm845.c | 4 +-
> drivers/clk/qcom/videocc-sm7150.c | 4 +-
> drivers/clk/qcom/videocc-sm8150.c | 4 +-
> drivers/clk/qcom/videocc-sm8450.c | 4 +-
> drivers/media/platform/qcom/venus/pm_helpers.c | 73 +++-----------------------
> 6 files changed, 17 insertions(+), 74 deletions(-)
> ---
> base-commit: 63b3ff03d91ae8f875fe8747c781a521f78cde17
> change-id: 20250115-switch_gdsc_mode-a9c14fad9a36
>
> Best regards,
> --
> Renjiang Han <quic_renjiang@xxxxxxxxxxx>
>

--
With best wishes
Dmitry