On 3/13/2024 12:09 AM, Konrad Dybcio wrote: > > > On 2/29/24 17:24, Marc Gonzalez wrote: >> On 29/02/2024 16:32, Vikash Garodia wrote: >> >>> On 2/27/2024 9:41 PM, Marc Gonzalez wrote: >>> >>>> On 27/02/2024 07:55, Vikash Garodia wrote: >>>> >>>>> On 2/26/2024 9:25 PM, Marc Gonzalez wrote: >>>>> >>>>>> Errr, there is currently no existing node for msm8998-venus? >>>>> >>>>> My bad, i meant your initial node msm8998-venus, without migrating to v2. >>>>> >>>>>> With the proposed node above (based on msm8996-venus) >>>>>> AND the proposed work-around disabling low-power mode, >>>>>> decoding works correctly. >>>>> >>>>> Nice, lets fix the work-around part before migrating to v2. Could you share >>>>> the >>>>> configurations for VIDEO_SUBCORE0_GDSC and VIDEO_SUBCORE1_GDSC ? >>>>> >>>>> If we see vendor code[1], sys power plane control is very much supported, so >>>>> ideally we should get it working without the workaround >>>>> [1] >>>>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/drivers/media/platform/msm/vidc/venus_hfi.c#L2223 >>>> >>>> OK, for easy reference, here are the proposed changes again: >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi >>>> b/arch/arm64/boot/dts/qcom/msm8998.dtsi >>>> index 2793cc22d381a..5084191be1446 100644 >>>> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi >>>> @@ -3000,6 +3000,56 @@ mdss_dsi1_phy: phy@c996400 { >>>> }; >>>> }; >>>> + venus: video-codec@cc00000 { >>>> + compatible = "qcom,msm8998-venus"; >>>> + reg = <0x0cc00000 0xff000>; >>>> + interrupts = <GIC_SPI 287 IRQ_TYPE_LEVEL_HIGH>; >>>> + power-domains = <&mmcc VIDEO_TOP_GDSC>; >>>> + clocks = <&mmcc VIDEO_CORE_CLK>, >>>> + <&mmcc VIDEO_AHB_CLK>, >>>> + <&mmcc VIDEO_AXI_CLK>, >>>> + <&mmcc VIDEO_MAXI_CLK>; >>>> + clock-names = "core", "iface", "bus", "mbus"; >>>> + iommus = <&mmss_smmu 0x400>, >>>> + <&mmss_smmu 0x401>, >>>> + <&mmss_smmu 0x40a>, >>>> + <&mmss_smmu 0x407>, >>>> + <&mmss_smmu 0x40e>, >>>> + <&mmss_smmu 0x40f>, >>>> + <&mmss_smmu 0x408>, >>>> + <&mmss_smmu 0x409>, >>>> + <&mmss_smmu 0x40b>, >>>> + <&mmss_smmu 0x40c>, >>>> + <&mmss_smmu 0x40d>, >>>> + <&mmss_smmu 0x410>, >>>> + <&mmss_smmu 0x411>, >>>> + <&mmss_smmu 0x421>, >>>> + <&mmss_smmu 0x428>, >>>> + <&mmss_smmu 0x429>, >>>> + <&mmss_smmu 0x42b>, >>>> + <&mmss_smmu 0x42c>, >>>> + <&mmss_smmu 0x42d>, >>>> + <&mmss_smmu 0x411>, >>>> + <&mmss_smmu 0x431>; >>>> + memory-region = <&venus_mem>; >>>> + status = "disabled"; >>>> + qcom,venus-broken-low-power-mode; >>>> + >>>> + video-decoder { >>>> + compatible = "venus-decoder"; >>>> + clocks = <&mmcc VIDEO_SUBCORE0_CLK>; >>>> + clock-names = "core"; >>>> + power-domains = <&mmcc VIDEO_SUBCORE0_GDSC>; >>>> + }; >>>> + >>>> + video-encoder { >>>> + compatible = "venus-encoder"; >>>> + clocks = <&mmcc VIDEO_SUBCORE1_CLK>; >>>> + clock-names = "core"; >>>> + power-domains = <&mmcc VIDEO_SUBCORE1_GDSC>; >>>> + }; >>>> + }; >>>> + >>>> mmss_smmu: iommu@cd00000 { >>>> compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2"; >>>> reg = <0x0cd00000 0x40000>; >>>> diff --git a/drivers/media/platform/qcom/venus/core.c >>>> b/drivers/media/platform/qcom/venus/core.c >>>> index a712dd4f02a5b..ad1705e510312 100644 >>>> --- a/drivers/media/platform/qcom/venus/core.c >>>> +++ b/drivers/media/platform/qcom/venus/core.c >>>> @@ -585,6 +585,43 @@ static const struct venus_resources msm8996_res = { >>>> .fwname = "qcom/venus-4.2/venus.mbn", >>>> }; >>>> +static const struct freq_tbl msm8998_freq_table[] = { >>>> + { 1944000, 520000000 }, /* 4k UHD @ 60 (decode only) */ >>>> + { 972000, 520000000 }, /* 4k UHD @ 30 */ >>>> + { 489600, 346666667 }, /* 1080p @ 60 */ >>>> + { 244800, 150000000 }, /* 1080p @ 30 */ >>>> + { 108000, 75000000 }, /* 720p @ 30 */ >>>> +}; >>>> + >>>> +static const struct reg_val msm8998_reg_preset[] = { >>>> + { 0x80124, 0x00000003 }, >>>> + { 0x80550, 0x01111111 }, >>>> + { 0x80560, 0x01111111 }, >>>> + { 0x80568, 0x01111111 }, >>>> + { 0x80570, 0x01111111 }, >>>> + { 0x80580, 0x01111111 }, >>>> + { 0xe2010, 0x00000000 }, >>>> +}; >>>> + >>>> +static const struct venus_resources msm8998_res = { >>>> + .freq_tbl = msm8998_freq_table, >>>> + .freq_tbl_size = ARRAY_SIZE(msm8998_freq_table), >>>> + .reg_tbl = msm8998_reg_preset, >>>> + .reg_tbl_size = ARRAY_SIZE(msm8998_reg_preset), >>>> + .clks = {"core", "iface", "bus", "mbus"}, >>>> + .clks_num = 4, >>>> + .vcodec0_clks = { "core" }, >>>> + .vcodec1_clks = { "core" }, >>>> + .vcodec_clks_num = 1, >>>> + .max_load = 2563200, >>>> + .hfi_version = HFI_VERSION_3XX, >>>> + .vmem_id = VIDC_RESOURCE_NONE, >>>> + .vmem_size = 0, >>>> + .vmem_addr = 0, >>>> + .dma_mask = 0xddc00000 - 1, >>>> + .fwname = "qcom/venus-4.4/venus.mbn", >>>> +}; >>>> + >>>> static const struct freq_tbl sdm660_freq_table[] = { >>>> { 979200, 518400000 }, >>>> { 489600, 441600000 }, >>>> @@ -891,6 +928,7 @@ static const struct venus_resources sc7280_res = { >>>> static const struct of_device_id venus_dt_match[] = { >>>> { .compatible = "qcom,msm8916-venus", .data = &msm8916_res, }, >>>> { .compatible = "qcom,msm8996-venus", .data = &msm8996_res, }, >>>> + { .compatible = "qcom,msm8998-venus", .data = &msm8998_res, }, >>>> { .compatible = "qcom,sdm660-venus", .data = &sdm660_res, }, >>>> { .compatible = "qcom,sdm845-venus", .data = &sdm845_res, }, >>>> { .compatible = "qcom,sdm845-venus-v2", .data = &sdm845_res_v2, }, >>>> >>>> >>>> >>>> This patch is on top of v6.8-rc1 >>>> so the configurations for VIDEO_SUBCOREx_GDSC >>>> are as defined in mainline. >>>> >>>> #define VIDEO_SUBCORE0_CLK_SRC 51 >>>> #define VIDEO_SUBCORE1_CLK_SRC 52 >>>> >>>> #define VIDEO_TOP_GDSC 1 >>>> #define VIDEO_SUBCORE0_GDSC 2 >>>> #define VIDEO_SUBCORE1_GDSC 3 >>>> >>>> https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/mmcc-msm8998.c#L2536-L2561 >>>> >>>> static struct gdsc video_top_gdsc = { >>>> .gdscr = 0x1024, >>>> .pd = { >>>> .name = "video_top", >>>> }, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> }; >>>> >>>> static struct gdsc video_subcore0_gdsc = { >>>> .gdscr = 0x1040, >>>> .pd = { >>>> .name = "video_subcore0", >>>> }, >>>> .parent = &video_top_gdsc.pd, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> }; >>>> >>>> static struct gdsc video_subcore1_gdsc = { >>>> .gdscr = 0x1044, >>>> .pd = { >>>> .name = "video_subcore1", >>>> }, >>>> .parent = &video_top_gdsc.pd, >>>> .pwrsts = PWRSTS_OFF_ON, >>>> }; >>>> >>>> >>>> const u8 pwrsts; >>>> /* Powerdomain allowable state bitfields */ >>>> #define PWRSTS_OFF BIT(0) >>>> /* >>>> * There is no SW control to transition a GDSC into >>>> * PWRSTS_RET. This happens in HW when the parent >>>> * domain goes down to a low power state >>>> */ >>>> #define PWRSTS_RET BIT(1) >>>> #define PWRSTS_ON BIT(2) >>>> #define PWRSTS_OFF_ON (PWRSTS_OFF | PWRSTS_ON) >>>> #define PWRSTS_RET_ON (PWRSTS_RET | PWRSTS_ON) >>>> const u16 flags; >>>> #define VOTABLE BIT(0) >>>> #define CLAMP_IO BIT(1) >>>> #define HW_CTRL BIT(2) >>>> #define SW_RESET BIT(3) >>>> #define AON_RESET BIT(4) >>>> #define POLL_CFG_GDSCR BIT(5) >>>> #define ALWAYS_ON BIT(6) >>>> #define RETAIN_FF_ENABLE BIT(7) >>>> #define NO_RET_PERIPH BIT(8) >>>> >>>> >>>> Should .pwrsts be PWRSTS_RET_ON instead of PWRSTS_OFF_ON? >>>> >>>> Should .flags be HW_CTRL instead of 0? >>> >>> Not completely sure on these configurations, but certainly both the >>> video_subcore0_gdsc and video_subcore1_gdsc should be configured in hardware >>> control mode in the gdsc configuration. >> >> Jeffrey, Bjorn, >> >> I'm trying to get mainline support for the msm8998 video decoder (venus). >> Apparently, there appears to be an issue with the multimedia clocks. >> >> Do you remember why you chose PWRSTS_OFF_ON instead of PWRSTS_RET_ON? > > Doing a quick reconnaissance against msm-4.4, PWRSTS_OFF_ON looks > very sane. > > Moreover, PWRSTS_RET_ON very much only looks useful as a hack for > non-fully-implemented drivers (and that would indeed match the state > of our usb and pcie driver today) - it prevents GDSC shutdown, but > tells the PM framework that the registered power domain has collapsed > >> >> And why the HW_CTRL bit is not set? > > HW_CTRL means "totally hand over the control of this GDSC to the > hardware" where "hardware" is very loosely defined.. > > Reading msm-4.4 again, it seems like we want HW_CTRL mode to be > enabled if and only if the low power property has been set through > the venus firmware interface. For video hardware, there are 2 steps for this: 1. Inform video firmware that the GDSC is in HW control. This is done via HFI 2. Switch GDSC mode runtime during the usecase as SW or HW mode. This is currently done via programming few power control video hardware registers directly. AFAIU, if the GDSC flag configuration has HW_CTRL (in file mmcc-msm8998.c) and the HFI in #1 above indicates the same, then all should be good. Regards, Vikash > > More particularly, we don't want it to be there once we wanna shut > down Venus for good. This is being worked on by folks over at [1]. > > https://lore.kernel.org/linux-arm-msm/20240122-gdsc-hwctrl-v4-0-9061e8a7aa07@xxxxxxxxxx/ > > Konrad
