Re: [RFC WIP PATCH] venus: add qcom,no-low-power property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.

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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux