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

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

[Marc Gonzalez]

From: Pierre-Hugues Husson <phhusson@freebox.fr>

On our msm8998-based device, calling venus_sys_set_power_control()
breaks playback. Since the vendor kernel never calls it, we assume
it should not be called for this device/FW combo.

Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
TODO in v2: split the patch in 2
Is "qcom,no-low-power" a proper name for the property?
Is a boolean property the right approach?
---
 .../devicetree/bindings/media/qcom,venus-common.yaml     | 3 +++
 drivers/media/platform/qcom/venus/hfi_venus.c            | 9 +++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/media/qcom,venus-common.yaml b/Documentation/devicetree/bindings/media/qcom,venus-common.yaml
index 3153d91f9d18a..69cb16dc4852c 100644
--- a/Documentation/devicetree/bindings/media/qcom,venus-common.yaml
+++ b/Documentation/devicetree/bindings/media/qcom,venus-common.yaml
@@ -62,6 +62,9 @@ properties:
     required:
       - iommus
 
+  qcom,no-low-power:
+    type: boolean
+
 required:
   - reg
   - clocks
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b91..2cd85a8cd837e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -945,10 +945,11 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
 			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
 	}
 
-	ret = venus_sys_set_power_control(hdev, venus_fw_low_power_mode);
-	if (ret)
-		dev_warn(dev, "setting hw power collapse ON failed (%d)\n",
-			 ret);
+	if (!of_property_read_bool(dev->of_node, "qcom,no-low-power")) {
+		ret = venus_sys_set_power_control(hdev, venus_fw_low_power_mode);
+		if (ret)
+			dev_warn(dev, "setting hw power collapse ON failed (%d)\n", ret);
+	}
 
 	/* For specific venus core, it is mandatory to set the UBWC configuration */
 	if (res->ubwc_conf) {
-- 
2.34.1

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

[Konrad Dybcio]

On 19.02.2024 18:18, Marc Gonzalez wrote:
> From: Pierre-Hugues Husson <phhusson@freebox.fr>
> 
> On our msm8998-based device, calling venus_sys_set_power_control()
> breaks playback. Since the vendor kernel never calls it, we assume
> it should not be called for this device/FW combo.
> 
> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---

FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
to name a couple.

Konrad

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

[Dmitry Baryshkov]

On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 19.02.2024 18:18, Marc Gonzalez wrote:
> > From: Pierre-Hugues Husson <phhusson@freebox.fr>
> >
> > On our msm8998-based device, calling venus_sys_set_power_control()
> > breaks playback. Since the vendor kernel never calls it, we assume
> > it should not be called for this device/FW combo.
> >
> > Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
> > Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> > ---
>
> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
> to name a couple.

Then let's just disable it until it gets unbroken?


-- 
With best wishes
Dmitry

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

[Bryan O'Donoghue]

On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>> From: Pierre-Hugues Husson <phhusson@freebox.fr>
>>>
>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>> breaks playback. Since the vendor kernel never calls it, we assume
>>> it should not be called for this device/FW combo.
>>>
>>> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
>>> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
>>> ---
>>
>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>> to name a couple.
> 
> Then let's just disable it until it gets unbroken?

Its functional on most of our upstream stuff though, why switch if off 
unless necessary ?

Maybe it should be an opt-in instead of an opt-out, TBH my own feeling 
is its better to minimize the amount of work and opt as per the proposed 
patch.

Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we 
come to tackling new HFI6XX and later SoCs ...

---
bod

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

[Marc Gonzalez]

On 19/02/2024 20:24, Bryan O'Donoghue wrote:

> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>
>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>
>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>
>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>> it should not be called for this device/FW combo.
>>>
>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>> to name a couple.
>>
>> Then let's just disable it until it gets unbroken?
> 
> Its functional on most of our upstream stuff though, why switch if off 
> unless necessary ?
> 
> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling 
> is its better to minimize the amount of work and opt as per the proposed 
> patch.
> 
> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we 
> come to tackling new HFI6XX and later SoCs ...

I was wondering if the chosen property name might cause issues later...

Thinking "qcom,no-low-power" might be a bit too general?
Perhaps would need to mention venus somewhere in the name,
to limit this to the video decoder?

Regards

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

[Bryan O'Donoghue]

On 20/02/2024 10:56 a.m., Marc Gonzalez wrote:
> On 19/02/2024 20:24, Bryan O'Donoghue wrote:
> 
>> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>>
>>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>>
>>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>>
>>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>>> it should not be called for this device/FW combo.
>>>>
>>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>>> to name a couple.
>>>
>>> Then let's just disable it until it gets unbroken?
>>
>> Its functional on most of our upstream stuff though, why switch if off
>> unless necessary ?
>>
>> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling
>> is its better to minimize the amount of work and opt as per the proposed
>> patch.
>>
>> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we
>> come to tackling new HFI6XX and later SoCs ...
> 
> I was wondering if the chosen property name might cause issues later...
> 
> Thinking "qcom,no-low-power" might be a bit too general?
> Perhaps would need to mention venus somewhere in the name,
> to limit this to the video decoder?
> 
> Regards
> 

Yep, the word venus should probably appear in the property name.

---
bod

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

[Krzysztof Kozlowski]

On 20/02/2024 12:21, Bryan O'Donoghue wrote:
> On 20/02/2024 10:56 a.m., Marc Gonzalez wrote:
>> On 19/02/2024 20:24, Bryan O'Donoghue wrote:
>>
>>> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>>>
>>>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>>>
>>>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>>>
>>>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>>>> it should not be called for this device/FW combo.
>>>>>
>>>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>>>> to name a couple.
>>>>
>>>> Then let's just disable it until it gets unbroken?
>>>
>>> Its functional on most of our upstream stuff though, why switch if off
>>> unless necessary ?
>>>
>>> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling
>>> is its better to minimize the amount of work and opt as per the proposed
>>> patch.
>>>
>>> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we
>>> come to tackling new HFI6XX and later SoCs ...
>>
>> I was wondering if the chosen property name might cause issues later...
>>
>> Thinking "qcom,no-low-power" might be a bit too general?
>> Perhaps would need to mention venus somewhere in the name,
>> to limit this to the video decoder?
>>
>> Regards
>>
> 
> Yep, the word venus should probably appear in the property name.

This is RFC, so I am ignoring it, but just in case before you send v2
with the same:

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.

Best regards,
Krzysztof

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

[Marc Gonzalez]

On 20/02/2024 12:37, Krzysztof Kozlowski wrote:

> On 20/02/2024 12:21, Bryan O'Donoghue wrote:
>
>> On 20/02/2024 10:56 a.m., Marc Gonzalez wrote:
>>
>>> On 19/02/2024 20:24, Bryan O'Donoghue wrote:
>>>
>>>> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>>>>
>>>>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>>>>
>>>>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>>>>
>>>>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>>>>> it should not be called for this device/FW combo.
>>>>>>
>>>>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>>>>> to name a couple.
>>>>>
>>>>> Then let's just disable it until it gets unbroken?
>>>>
>>>> Its functional on most of our upstream stuff though, why switch if off
>>>> unless necessary ?
>>>>
>>>> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling
>>>> is its better to minimize the amount of work and opt as per the proposed
>>>> patch.
>>>>
>>>> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we
>>>> come to tackling new HFI6XX and later SoCs ...
>>>
>>> I was wondering if the chosen property name might cause issues later...
>>>
>>> Thinking "qcom,no-low-power" might be a bit too general?
>>> Perhaps would need to mention venus somewhere in the name,
>>> to limit this to the video decoder?
>>
>> Yep, the word venus should probably appear in the property name.
> 
> This is RFC, so I am ignoring it, but just in case before you send v2
> with the same:
> 
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.

I added the RFC tag explicitly because I was hoping for the DT folks
and msm maintainers to comment on the property name ;)

Thanks for your comment!

Here's the proposal for v2:

qcom,venus-broken-low-power-mode

Description:
This property is defined for devices where playback does not work
when the video decoder is in low power mode.

Regards

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

[Krzysztof Kozlowski]

On 20/02/2024 13:34, Marc Gonzalez wrote:
> On 20/02/2024 12:37, Krzysztof Kozlowski wrote:
> 
>> On 20/02/2024 12:21, Bryan O'Donoghue wrote:
>>
>>> On 20/02/2024 10:56 a.m., Marc Gonzalez wrote:
>>>
>>>> On 19/02/2024 20:24, Bryan O'Donoghue wrote:
>>>>
>>>>> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>>>>>
>>>>>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>>>>>
>>>>>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>>>>>
>>>>>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>>>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>>>>>> it should not be called for this device/FW combo.
>>>>>>>
>>>>>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>>>>>> to name a couple.
>>>>>>
>>>>>> Then let's just disable it until it gets unbroken?
>>>>>
>>>>> Its functional on most of our upstream stuff though, why switch if off
>>>>> unless necessary ?
>>>>>
>>>>> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling
>>>>> is its better to minimize the amount of work and opt as per the proposed
>>>>> patch.
>>>>>
>>>>> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we
>>>>> come to tackling new HFI6XX and later SoCs ...
>>>>
>>>> I was wondering if the chosen property name might cause issues later...
>>>>
>>>> Thinking "qcom,no-low-power" might be a bit too general?
>>>> Perhaps would need to mention venus somewhere in the name,
>>>> to limit this to the video decoder?
>>>
>>> Yep, the word venus should probably appear in the property name.
>>
>> This is RFC, so I am ignoring it, but just in case before you send v2
>> with the same:
>>
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
> 
> I added the RFC tag explicitly because I was hoping for the DT folks
> and msm maintainers to comment on the property name ;)

And for the PATCH we would not comment? RFC means not ready and you
gather opinion before doing more work. Some maintainers ignore entirely
RFC patches.

> 
> Thanks for your comment!
> 
> Here's the proposal for v2:
> 
> qcom,venus-broken-low-power-mode
> 
> Description:
> This property is defined for devices where playback does not work
> when the video decoder is in low power mode.

Would be nice to know what's broken but if that's tricky to get, then
sounds fine.

Best regards,
Krzysztof

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

[Vikash Garodia]

Hi,

On 2/20/2024 6:57 PM, Krzysztof Kozlowski wrote:
> On 20/02/2024 13:34, Marc Gonzalez wrote:
>> On 20/02/2024 12:37, Krzysztof Kozlowski wrote:
>>
>>> On 20/02/2024 12:21, Bryan O'Donoghue wrote:
>>>
>>>> On 20/02/2024 10:56 a.m., Marc Gonzalez wrote:
>>>>
>>>>> On 19/02/2024 20:24, Bryan O'Donoghue wrote:
>>>>>
>>>>>> On 19/02/2024 5:44 p.m., Dmitry Baryshkov wrote:
>>>>>>
>>>>>>> On Mon, 19 Feb 2024 at 19:29, Konrad Dybcio wrote:
>>>>>>>>
>>>>>>>> On 19.02.2024 18:18, Marc Gonzalez wrote:
>>>>>>>>
>>>>>>>>> On our msm8998-based device, calling venus_sys_set_power_control()
>>>>>>>>> breaks playback. Since the vendor kernel never calls it, we assume
>>>>>>>>> it should not be called for this device/FW combo.
>>>>>>>>
>>>>>>>> FWIW, this is also broken on other SoCs.. 8280/8350 and 6115
>>>>>>>> to name a couple.
>>>>>>>
>>>>>>> Then let's just disable it until it gets unbroken?
>>>>>>
>>>>>> Its functional on most of our upstream stuff though, why switch if off
>>>>>> unless necessary ?
>>>>>>
>>>>>> Maybe it should be an opt-in instead of an opt-out, TBH my own feeling
>>>>>> is its better to minimize the amount of work and opt as per the proposed
>>>>>> patch.
>>>>>>
>>>>>> Perhaps the qcom vidc team can give insights on 8280xp and 8350 when we
>>>>>> come to tackling new HFI6XX and later SoCs ...
>>>>>
>>>>> I was wondering if the chosen property name might cause issues later...
>>>>>
>>>>> Thinking "qcom,no-low-power" might be a bit too general?
>>>>> Perhaps would need to mention venus somewhere in the name,
>>>>> to limit this to the video decoder?
>>>>
>>>> Yep, the word venus should probably appear in the property name.
>>>
>>> This is RFC, so I am ignoring it, but just in case before you send v2
>>> with the same:
>>>
>>> You described the desired Linux feature or behavior, not the actual
>>> hardware. The bindings are about the latter, so instead you need to
>>> rephrase the property and its description to match actual hardware
>>> capabilities/features/configuration etc.
>>
>> I added the RFC tag explicitly because I was hoping for the DT folks
>> and msm maintainers to comment on the property name ;)
> 
> And for the PATCH we would not comment? RFC means not ready and you
> gather opinion before doing more work. Some maintainers ignore entirely
> RFC patches.
> 
>>
>> Thanks for your comment!
>>
>> Here's the proposal for v2:
>>
>> qcom,venus-broken-low-power-mode
>>
>> Description:
>> This property is defined for devices where playback does not work
>> when the video decoder is in low power mode.
> 
> Would be nice to know what's broken but if that's tricky to get, then
> sounds fine.

msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
mode. Could you please check and confirm if the driver is configuring only the
VCodec GDSC and not the venus GDSC. Look for the attribute
"qcom,support-hw-trigger" in vendor dt file.

Regards,
Vikash

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

[Marc Gonzalez]

On 20/02/2024 14:53, Vikash Garodia wrote:

> On 2/20/2024 6:57 PM, Krzysztof Kozlowski wrote:
>
>> On 20/02/2024 13:34, Marc Gonzalez wrote:
>>
>>> Here's the proposal for v2:
>>>
>>> qcom,venus-broken-low-power-mode
>>>
>>> Description:
>>> This property is defined for devices where playback does not work
>>> when the video decoder is in low power mode.
>>
>> Would be nice to know what's broken but if that's tricky to get, then
>> sounds fine.
> 
> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
> mode. Could you please check and confirm if the driver is configuring only the
> VCodec GDSC and not the venus GDSC. Look for the attribute
> "qcom,support-hw-trigger" in vendor dt file.

[ Vendor DTS for easy reference: ]
[ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]

In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
(It is using the previously proposed "qcom,no-low-power" mechanism, but that
might not be necessary, if I understand correctly?)


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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
+
+			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, },



@Vikash, are you saying that perhaps the DTS for video-codec@cc00000
needs to be written slightly differently?

Regards

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

[Vikash Garodia]

On 2/20/2024 8:15 PM, Marc Gonzalez wrote:
> On 20/02/2024 14:53, Vikash Garodia wrote:
> 
>> On 2/20/2024 6:57 PM, Krzysztof Kozlowski wrote:
>>
>>> On 20/02/2024 13:34, Marc Gonzalez wrote:
>>>
>>>> Here's the proposal for v2:
>>>>
>>>> qcom,venus-broken-low-power-mode
>>>>
>>>> Description:
>>>> This property is defined for devices where playback does not work
>>>> when the video decoder is in low power mode.
>>>
>>> Would be nice to know what's broken but if that's tricky to get, then
>>> sounds fine.
>>
>> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
>> mode. Could you please check and confirm if the driver is configuring only the
>> VCodec GDSC and not the venus GDSC. Look for the attribute
>> "qcom,support-hw-trigger" in vendor dt file.
> 
> [ Vendor DTS for easy reference: ]
> [ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]
> 
> In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
> (It is using the previously proposed "qcom,no-low-power" mechanism, but that
> might not be necessary, if I understand correctly?)
> 
> 
> 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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
> +
> +			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, },
> 
> 
> 
> @Vikash, are you saying that perhaps the DTS for video-codec@cc00000
> needs to be written slightly differently?
Certainly yes. For ex, in the clock list, i do not see the core clocks listed
i.e clk_mmss_video_subcore0_clk and clk_mmss_video_subcore1_clk. You can refer
the downstream video DT node [1] and then align it as per venus driver
[1]
https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi

Regards,
Vikash

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

[Marc Gonzalez]

On 23/02/2024 14:48, Vikash Garodia wrote:

> On 2/20/2024 8:15 PM, Marc Gonzalez wrote:
> 
>> On 20/02/2024 14:53, Vikash Garodia wrote:
>>
>>> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
>>> mode. Could you please check and confirm if the driver is configuring only the
>>> VCodec GDSC and not the venus GDSC. Look for the attribute
>>> "qcom,support-hw-trigger" in vendor dt file.
>>
>> [ Vendor DTS for easy reference: ]
>> [ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]
>>
>> In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
>> (It is using the previously proposed "qcom,no-low-power" mechanism, but that
>> might not be necessary, if I understand correctly?)
>>
>>
>> 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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
>> +
>> +			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, },
>>
>>
>>
>> @Vikash, are you saying that perhaps the DTS for video-codec@cc00000
>> needs to be written slightly differently?
>
>
> Certainly yes. For ex, in the clock list, i do not see the core clocks listed
> i.e clk_mmss_video_subcore0_clk and clk_mmss_video_subcore1_clk. You can refer
> the downstream video DT node [1] and then align it as per venus driver
> [1]
> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi


If I understand correctly (which is far from certain),
we should base the "qcom,msm8998-venus" DT node on
"qcom,sdm845-venus-v2" rather than "qcom,msm8996-venus" ?

Regards

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

[Vikash Garodia]

On 2/26/2024 6:39 PM, Marc Gonzalez wrote:
> On 23/02/2024 14:48, Vikash Garodia wrote:
> 
>> On 2/20/2024 8:15 PM, Marc Gonzalez wrote:
>>
>>> On 20/02/2024 14:53, Vikash Garodia wrote:
>>>
>>>> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
>>>> mode. Could you please check and confirm if the driver is configuring only the
>>>> VCodec GDSC and not the venus GDSC. Look for the attribute
>>>> "qcom,support-hw-trigger" in vendor dt file.
>>>
>>> [ Vendor DTS for easy reference: ]
>>> [ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]
>>>
>>> In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
>>> (It is using the previously proposed "qcom,no-low-power" mechanism, but that
>>> might not be necessary, if I understand correctly?)
>>>
>>>
>>> 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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
>>> +
>>> +			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, },
>>>
>>>
>>>
>>> @Vikash, are you saying that perhaps the DTS for video-codec@cc00000
>>> needs to be written slightly differently?
>>
>>
>> Certainly yes. For ex, in the clock list, i do not see the core clocks listed
>> i.e clk_mmss_video_subcore0_clk and clk_mmss_video_subcore1_clk. You can refer
>> the downstream video DT node [1] and then align it as per venus driver
>> [1]
>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
> 
> 
> If I understand correctly (which is far from certain),
> we should base the "qcom,msm8998-venus" DT node on
> "qcom,sdm845-venus-v2" rather than "qcom,msm8996-venus" ?
Thats correct, but thats just another way to do the configuration. With the
existing node, is video decode as well as encode working ?

Regards,
Vikash

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

[Marc Gonzalez]

On 26/02/2024 15:30, Vikash Garodia wrote:

> On 2/26/2024 6:39 PM, Marc Gonzalez wrote:
>
>> On 23/02/2024 14:48, Vikash Garodia wrote:
>>
>>> On 2/20/2024 8:15 PM, Marc Gonzalez wrote:
>>>
>>>> On 20/02/2024 14:53, Vikash Garodia wrote:
>>>>
>>>>> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
>>>>> mode. Could you please check and confirm if the driver is configuring only the
>>>>> VCodec GDSC and not the venus GDSC. Look for the attribute
>>>>> "qcom,support-hw-trigger" in vendor dt file.
>>>>
>>>> [ Vendor DTS for easy reference: ]
>>>> [ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]
>>>>
>>>> In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
>>>> (It is using the previously proposed "qcom,no-low-power" mechanism, but that
>>>> might not be necessary, if I understand correctly?)
>>>>
>>>>
>>>> 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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
>>>> +
>>>> +			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, },
>>>>
>>>>
>>>>
>>>> @Vikash, are you saying that perhaps the DTS for video-codec@cc00000
>>>> needs to be written slightly differently?
>>>
>>>
>>> Certainly yes. For ex, in the clock list, i do not see the core clocks listed
>>> i.e clk_mmss_video_subcore0_clk and clk_mmss_video_subcore1_clk. You can refer
>>> the downstream video DT node [1] and then align it as per venus driver
>>> [1]
>>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
>>
>> If I understand correctly (which is far from certain),
>> we should base the "qcom,msm8998-venus" DT node on
>> "qcom,sdm845-venus-v2" rather than "qcom,msm8996-venus" ?
>
> That's correct, but that's just another way to do the configuration. With the
> existing node, is video decode as well as encode working ?

Errr, there is currently no existing node for msm8998-venus?

With the proposed node above (based on msm8996-venus)
AND the proposed work-around disabling low-power mode,
decoding works correctly.

Encoding does not work, but it has never been used/tested on our device.

[h264_v4l2m2m @ 0xaaaaec9c44a0] Using device /dev/video1
[h264_v4l2m2m @ 0xaaaaec9c44a0] driver 'qcom-venus' on card 'Qualcomm Venus video encoder' in mplane mode
[h264_v4l2m2m @ 0xaaaaec9c44a0] requesting formats: output=NV12/yuv420p capture=H264/none
[h264_v4l2m2m @ 0xaaaaec9c44a0] output VIDIOC_REQBUFS failed: Invalid argument
[h264_v4l2m2m @ 0xaaaaec9c44a0] no v4l2 output context's buffers
[h264_v4l2m2m @ 0xaaaaec9c44a0] can't configure encoder
[vost#0:0/h264_v4l2m2m @ 0xaaaaec9c4160] Error while opening encoder - maybe incorrect parameters such as bit_rate, rate, width or height.

Regards

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

[Vikash Garodia]

On 2/26/2024 9:25 PM, Marc Gonzalez wrote:
> On 26/02/2024 15:30, Vikash Garodia wrote:
> 
>> On 2/26/2024 6:39 PM, Marc Gonzalez wrote:
>>
>>> On 23/02/2024 14:48, Vikash Garodia wrote:
>>>
>>>> On 2/20/2024 8:15 PM, Marc Gonzalez wrote:
>>>>
>>>>> On 20/02/2024 14:53, Vikash Garodia wrote:
>>>>>
>>>>>> msm8998 supports configuring the VCodec (venus core0) GDSC in HW power control
>>>>>> mode. Could you please check and confirm if the driver is configuring only the
>>>>>> VCodec GDSC and not the venus GDSC. Look for the attribute
>>>>>> "qcom,support-hw-trigger" in vendor dt file.
>>>>>
>>>>> [ Vendor DTS for easy reference: ]
>>>>> [ https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998.dtsi ]
>>>>>
>>>>> In the queue, we have a patch enabling the Venus Decoder (VDEC) in mainline.
>>>>> (It is using the previously proposed "qcom,no-low-power" mechanism, but that
>>>>> might not be necessary, if I understand correctly?)
>>>>>
>>>>>
>>>>> 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,no-low-power; /*** WORK AROUND LOW-POWER ISSUE ***/
>>>>> +
>>>>> +			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, },
>>>>>
>>>>>
>>>>>
>>>>> @Vikash, are you saying that perhaps the DTS for video-codec@cc00000
>>>>> needs to be written slightly differently?
>>>>
>>>>
>>>> Certainly yes. For ex, in the clock list, i do not see the core clocks listed
>>>> i.e clk_mmss_video_subcore0_clk and clk_mmss_video_subcore1_clk. You can refer
>>>> the downstream video DT node [1] and then align it as per venus driver
>>>> [1]
>>>> https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
>>>
>>> If I understand correctly (which is far from certain),
>>> we should base the "qcom,msm8998-venus" DT node on
>>> "qcom,sdm845-venus-v2" rather than "qcom,msm8996-venus" ?
>>
>> That's correct, but that's just another way to do the configuration. With the
>> existing node, is video decode as well as encode working ?
> 
> 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

> Encoding does not work, but it has never been used/tested on our device.
> 
> [h264_v4l2m2m @ 0xaaaaec9c44a0] Using device /dev/video1
> [h264_v4l2m2m @ 0xaaaaec9c44a0] driver 'qcom-venus' on card 'Qualcomm Venus video encoder' in mplane mode
> [h264_v4l2m2m @ 0xaaaaec9c44a0] requesting formats: output=NV12/yuv420p capture=H264/none
> [h264_v4l2m2m @ 0xaaaaec9c44a0] output VIDIOC_REQBUFS failed: Invalid argument
> [h264_v4l2m2m @ 0xaaaaec9c44a0] no v4l2 output context's buffers
> [h264_v4l2m2m @ 0xaaaaec9c44a0] can't configure encoder
> [vost#0:0/h264_v4l2m2m @ 0xaaaaec9c4160] Error while opening encoder - maybe incorrect parameters such as bit_rate, rate, width or height.

We can revisit the encoder failure once we have decode fixed without any workaround.

Regards,
Vikash

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

[Marc Gonzalez]

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?


Regards

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

[Vikash Garodia]

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.

Regards,
Vikash

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

[Marc Gonzalez]

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?

And why the HW_CTRL bit is not set?

In 96893e101eb294bced8358fbd48cbac175977aa4
"clk: qcom: Put venus core0/1 gdscs to hw control mode"
Sricharan set the HW_CTRL bit in venus_core0_gdsc & venus_core1_gdsc
for msm8996. A priori, it is required for msm8998 as well.

Same deal with 196eb928525579
"clk: qcom: mmcc-sdm660: Add hw_ctrl flag to venus_core0_gdsc"

Regards

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

[Jeffrey Hugo]

On 2/29/2024 9:24 AM, 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?
> 
> And why the HW_CTRL bit is not set?

Not really.  Looks like this was 4-5 years ago.  I'm guessing I looked 
at either 845 or 8996 to figure out which one was closer to 8998, and 
then copied that implementation with a quick sanity check against 
downstream and any hardware documentation I could find.  I think the 
GDSC implementation has drastically improved since then, so OFF_ON might 
have been correct then, but is not correct now.

Sorry, probably not much help.

-Jeff

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

[Konrad Dybcio]

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@linaro.org/

Konrad

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

[Vikash Garodia]

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@linaro.org/
> 
> Konrad

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

[Marc Gonzalez]

On 29/02/2024 16:32, Vikash Garodia wrote:

> 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.

Hello,

Still trying to land support for venus decoder on msm8998 in mainline.

This is the patch I have at the moment:

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 4dfe2d09ac285..67b8374ddf02f 100644
--- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
@@ -3010,6 +3010,55 @@ 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";
+
+			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 ce206b7097541..42e0c580e093d 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -587,6 +587,47 @@ 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 */
+};
+
+/*
+ * https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
+ */
+static const struct reg_val msm8998_reg_preset[] = {
+	{ 0x80124, 0x00000003 },
+	{ 0x80550, 0x01111111 },
+	{ 0x80560, 0x01111111 },
+	{ 0x80568, 0x01111111 },
+	{ 0x80570, 0x01111111 },
+	{ 0x80580, 0x01111111 },
+	{ 0x80588, 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 },
@@ -893,6 +934,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, },
diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index f9437b6412b91..abdc578ce988e 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -945,6 +945,7 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
 			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
 	}
 
+	venus_fw_low_power_mode = false;
 	ret = venus_sys_set_power_control(hdev, venus_fw_low_power_mode);
 	if (ret)
 		dev_warn(dev, "setting hw power collapse ON failed (%d)\n",



With the quick and dirty hack in hfi_venus.c
I am able to correctly decode using venus with:

# mpv --hwdec=v4l2m2m-copy --vo=tct --quiet demo-480.webm
 (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
 (+) Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl



Without the hack, HW decoding fails (and falls back to SW decode)

# cd /home && mpv --hwdec=v4l2m2m-copy --vo=tct --quiet demo-480.webm
 (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
 (+) Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: output VIDIOC_REQBUFS failed: Connection timed out
[ffmpeg/video] vp9_v4l2m2m: no v4l2 output context's buffers
[ffmpeg/video] vp9_v4l2m2m: can't configure decoder
Could not open codec.


Not sure where to go from here.
Vikash, do you have any guidance?
(I think you were not a fan of the DT-based work-around?)


Regards

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

[Bryan O'Donoghue]

On 08/04/2024 16:39, Marc Gonzalez wrote:
> On 29/02/2024 16:32, Vikash Garodia wrote:
> 
>> 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.
> 
> Hello,
> 
> Still trying to land support for venus decoder on msm8998 in mainline.
> 
> This is the patch I have at the moment:
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 4dfe2d09ac285..67b8374ddf02f 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> @@ -3010,6 +3010,55 @@ 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";
> +
> +			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 ce206b7097541..42e0c580e093d 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -587,6 +587,47 @@ 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 */
> +};
> +
> +/*
> + * https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
> + */
> +static const struct reg_val msm8998_reg_preset[] = {
> +	{ 0x80124, 0x00000003 },
> +	{ 0x80550, 0x01111111 },
> +	{ 0x80560, 0x01111111 },
> +	{ 0x80568, 0x01111111 },
> +	{ 0x80570, 0x01111111 },
> +	{ 0x80580, 0x01111111 },
> +	{ 0x80588, 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 },
> @@ -893,6 +934,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, },
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index f9437b6412b91..abdc578ce988e 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -945,6 +945,7 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
>   			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
>   	}
>   
> +	venus_fw_low_power_mode = false;

So instead of this workaround, @Vikash is asking for HW_CTRL similar to 
what we have in 8996

8996 has a top-level "magic" GDSC which 8998 doesn't appear to have.

I think the CXC stuff would still be valid.

diff --git a/drivers/clk/qcom/mmcc-msm8998.c 
b/drivers/clk/qcom/mmcc-msm8998.c
index 1180e48c687ac..275fb3b71ede4 100644
--- a/drivers/clk/qcom/mmcc-msm8998.c
+++ b/drivers/clk/qcom/mmcc-msm8998.c
@@ -2535,6 +2535,8 @@ static struct clk_branch vmem_ahb_clk = {

  static struct gdsc video_top_gdsc = {
         .gdscr = 0x1024,
+       .cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
+       .cxc_count = 3,
         .pd = {
                 .name = "video_top",
         },
@@ -2543,20 +2545,26 @@ static struct gdsc video_top_gdsc = {

  static struct gdsc video_subcore0_gdsc = {
         .gdscr = 0x1040,
+       .cxcs = (unsigned int []){ 0x1048 },
+       .cxc_count = 1,
         .pd = {
                 .name = "video_subcore0",
         },
         .parent = &video_top_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
+       .flags = HW_CTRL,
  };

  static struct gdsc video_subcore1_gdsc = {
         .gdscr = 0x1044,
+       .cxcs = (unsigned int []){ 0x104c },
+       .cxc_count = 1,
         .pd = {
                 .name = "video_subcore1",
         },
         .parent = &video_top_gdsc.pd,
         .pwrsts = PWRSTS_OFF_ON,
+       .flags = HW_CTRL,
  };

Can you give it a try ?

---
bod

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

[Marc Gonzalez]

On 09/04/2024 13:27, Bryan O'Donoghue wrote:

> On 08/04/2024 16:39, Marc Gonzalez wrote:
> 
>> On 29/02/2024 16:32, Vikash Garodia wrote:
>> 
>>> 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.
>>
>> Still trying to land support for venus decoder on msm8998 in mainline.
>>
>> This is the patch I have at the moment:
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> index 4dfe2d09ac285..67b8374ddf02f 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>> @@ -3010,6 +3010,55 @@ 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";
>> +
>> +			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 ce206b7097541..42e0c580e093d 100644
>> --- a/drivers/media/platform/qcom/venus/core.c
>> +++ b/drivers/media/platform/qcom/venus/core.c
>> @@ -587,6 +587,47 @@ 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 */
>> +};
>> +
>> +/*
>> + * https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/arch/arm/boot/dts/qcom/msm8998-vidc.dtsi
>> + */
>> +static const struct reg_val msm8998_reg_preset[] = {
>> +	{ 0x80124, 0x00000003 },
>> +	{ 0x80550, 0x01111111 },
>> +	{ 0x80560, 0x01111111 },
>> +	{ 0x80568, 0x01111111 },
>> +	{ 0x80570, 0x01111111 },
>> +	{ 0x80580, 0x01111111 },
>> +	{ 0x80588, 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 },
>> @@ -893,6 +934,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, },
>> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
>> index f9437b6412b91..abdc578ce988e 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
>> @@ -945,6 +945,7 @@ static int venus_sys_set_default_properties(struct venus_hfi_device *hdev)
>>   			dev_warn(dev, "setting idle response ON failed (%d)\n", ret);
>>   	}
>>   
>> +	venus_fw_low_power_mode = false;
> 
> So instead of this workaround, @Vikash is asking for HW_CTRL similar to 
> what we have in 8996
> 
> 8996 has a top-level "magic" GDSC which 8998 doesn't appear to have.
> 
> I think the CXC stuff would still be valid.
> 
> diff --git a/drivers/clk/qcom/mmcc-msm8998.c 
> b/drivers/clk/qcom/mmcc-msm8998.c
> index 1180e48c687ac..275fb3b71ede4 100644
> --- a/drivers/clk/qcom/mmcc-msm8998.c
> +++ b/drivers/clk/qcom/mmcc-msm8998.c
> @@ -2535,6 +2535,8 @@ static struct clk_branch vmem_ahb_clk = {
> 
>   static struct gdsc video_top_gdsc = {
>          .gdscr = 0x1024,
> +       .cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },
> +       .cxc_count = 3,
>          .pd = {
>                  .name = "video_top",
>          },
> @@ -2543,20 +2545,26 @@ static struct gdsc video_top_gdsc = {
> 
>   static struct gdsc video_subcore0_gdsc = {
>          .gdscr = 0x1040,
> +       .cxcs = (unsigned int []){ 0x1048 },
> +       .cxc_count = 1,
>          .pd = {
>                  .name = "video_subcore0",
>          },
>          .parent = &video_top_gdsc.pd,
>          .pwrsts = PWRSTS_OFF_ON,
> +       .flags = HW_CTRL,
>   };
> 
>   static struct gdsc video_subcore1_gdsc = {
>          .gdscr = 0x1044,
> +       .cxcs = (unsigned int []){ 0x104c },
> +       .cxc_count = 1,
>          .pd = {
>                  .name = "video_subcore1",
>          },
>          .parent = &video_top_gdsc.pd,
>          .pwrsts = PWRSTS_OFF_ON,
> +       .flags = HW_CTRL,
>   };
> 
> Can you give it a try ?


Looks like your patch DOES fix the issue!


References (mostly) for myself


static struct gdsc video_top_gdsc = {
	.gdscr = 0x1024,
	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },

static struct gdsc video_subcore0_gdsc = {
	.gdscr = 0x1040,
	.cxcs = (unsigned int []){ 0x1048 },

static struct gdsc video_subcore1_gdsc = {
	.gdscr = 0x1044,
	.cxcs = (unsigned int []){ 0x104c },


GDSCR = Globally Distributed Switch Controller Register

https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8998.h

0x1024 = undocumented = MMSS_VIDEO GDSCR
0x1028 = MMSS_VIDEO_CORE_CBCR
0x1030 = MMSS_VIDEO_AHB_CBCR
0x1034 = MMSS_VIDEO_AXI_CBCR
0x1038 = MMSS_VIDEO_MAXI_CBCR
0x1040 = undocumented = MMSS_VIDEO_SUBCORE0 GDSCR
0x1044 = undocumented = MMSS_VIDEO_SUBCORE1 GDSCR
0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
0x104c = MMSS_VIDEO_SUBCORE1_CBCR



On msm8996

static struct gdsc venus_gdsc = {
	.gdscr = 0x1024,
	.cxcs = (unsigned int []){ 0x1028, 0x1034, 0x1038 },

static struct gdsc venus_core0_gdsc = {
	.gdscr = 0x1040,
	.cxcs = (unsigned int []){ 0x1048 },

static struct gdsc venus_core1_gdsc = {
	.gdscr = 0x1044,
	.cxcs = (unsigned int []){ 0x104c },


https://git.codelinaro.org/clo/la/kernel/msm-4.4/-/blob/caf_migration/kernel.lnx.4.4.r38-rel/include/dt-bindings/clock/msm-clocks-hwio-8996.h

0x1028 = MMSS_VIDEO_CORE_CBCR = MMSS_VIDEO_CXO_CBCR
0x1030 = MMSS_VIDEO_AHB_CBCR
0x1034 = MMSS_VIDEO_AXI_CBCR
0x1038 = MMSS_VIDEO_MAXI_CBCR
0x1048 = MMSS_VIDEO_SUBCORE0_CBCR
0x104c = MMSS_VIDEO_SUBCORE1_CBCR


Registers of interest are mapped identically in msm8996 & msm8998.
Therefore, it makes sense for the code to be identical.


Regards

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

[Marc Gonzalez]

On 09/04/2024 13:27, Bryan O'Donoghue wrote:

> Can you give it a try ?

Random notes

For easy reference, I've used this command to test:

$ mpv --hwdec=v4l2m2m-copy --vo=tct --quiet --no-audio demo-480.webm

And it displays the video directly in the terminal :)
(Rendering speed depends on terminal size)

I'd never played the video to the end.
I notice I get:

[  397.410006] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d7c000
[  397.410114] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001

How bad is that?


Sometimes, decoding simply fails immediately.
Must quit & restart.
Will have to script a 100 starts and check frequency of failures.


Will test with higher-resolution video.

# time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed demo-480.webm
 (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
     Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
Using hardware decoding (v4l2m2m-copy).
VO: [null] 854x480 nv12
[ffmpeg/video] vp9_v4l2m2m: capture POLLERR
Exiting... (Quit)
/*** HANGS UNTIL CTRL-C ***/

real	0m21.467s
user	0m3.795s
sys	0m1.914s


# time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed --length=30 demo-1440.webm 
 (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
     Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
Using hardware decoding (v4l2m2m-copy).
VO: [null] 2560x1440 nv12
Exiting... (End of file)

real	0m16.433s
user	0m1.764s
sys	0m1.118s


Regards

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

[Bryan O'Donoghue]

On 09/04/2024 17:53, Marc Gonzalez wrote:
> On 09/04/2024 13:27, Bryan O'Donoghue wrote:
> 
>> Can you give it a try ?
> 
> Random notes
> 
> For easy reference, I've used this command to test:
> 
> $ mpv --hwdec=v4l2m2m-copy --vo=tct --quiet --no-audio demo-480.webm
> 
> And it displays the video directly in the terminal :)
> (Rendering speed depends on terminal size)
> 
> I'd never played the video to the end.
> I notice I get:
> 
> [  397.410006] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d7c000
> [  397.410114] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001
> 
> How bad is that?
> 
> 
> Sometimes, decoding simply fails immediately.
> Must quit & restart.
> Will have to script a 100 starts and check frequency of failures.
> 
> 
> Will test with higher-resolution video.
> 
> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed demo-480.webm
>   (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> Using hardware decoding (v4l2m2m-copy).
> VO: [null] 854x480 nv12
> [ffmpeg/video] vp9_v4l2m2m: capture POLLERR
> Exiting... (Quit)
> /*** HANGS UNTIL CTRL-C ***/

I think there are a number of different resolutions across SoCs that 
will exhibit this behavior, I've seen it with lower resolutions on 8250.

Its a bug that we need to drill into in Venus but I don't think is a bug 
that is specific to your setup.

> 
> real	0m21.467s
> user	0m3.795s
> sys	0m1.914s
> 
> 
> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed --length=30 demo-1440.webm
>   (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> Using hardware decoding (v4l2m2m-copy).
> VO: [null] 2560x1440 nv12
> Exiting... (End of file)
> 
> real	0m16.433s
> user	0m1.764s
> sys	0m1.118s

If this higher resolution is stable for you, I'd say this is about baseline.

1. The GDSC change should make no impact on playback or available resolution

2. Higher more "normal" use cases like 1080p should be fine.

If so, then file "low resolution is broken" under a "known unknown" and 
scrub your patches for submission.

If not, we need to do more 8998 specific debug.

---
bod

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

[Vikash Garodia]

On 4/10/2024 1:47 PM, Bryan O'Donoghue wrote:
> On 09/04/2024 17:53, Marc Gonzalez wrote:
>> On 09/04/2024 13:27, Bryan O'Donoghue wrote:
>>
>>> Can you give it a try ?
>>
>> Random notes
>>
>> For easy reference, I've used this command to test:
>>
>> $ mpv --hwdec=v4l2m2m-copy --vo=tct --quiet --no-audio demo-480.webm
>>
>> And it displays the video directly in the terminal :)
>> (Rendering speed depends on terminal size)
>>
>> I'd never played the video to the end.
>> I notice I get:
>>
>> [  397.410006] qcom-venus cc00000.video-codec: session error: event id:1001
>> (deadb000), session id:79d7c000
>> [  397.410114] qcom-venus-decoder cc00000.video-codec:video-decoder: dec:
>> event session error 1001
>>
>> How bad is that?
>>
>>
>> Sometimes, decoding simply fails immediately.
>> Must quit & restart.
>> Will have to script a 100 starts and check frequency of failures.
>>
>>
>> Will test with higher-resolution video.
We were writing sample test application [1] to test video usecase and more
specifically with user driven inputs. Incase this help in your trial. I haven't
yet tried this on venus driver and used it mainly for the some other v4l video
driver, but i can expect it to work given it is v4l interface based. It has few
sample config which can dump the decoded output to validate. Ignore if this adds
to your effort to validate from this app, will publish once we try on venus driver.

Regards,
Vikash
[1] https://github.com/quic/v4l-video-test-app

>>
>> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed
>> demo-480.webm
>>   (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
>>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> Using hardware decoding (v4l2m2m-copy).
>> VO: [null] 854x480 nv12
>> [ffmpeg/video] vp9_v4l2m2m: capture POLLERR
>> Exiting... (Quit)
>> /*** HANGS UNTIL CTRL-C ***/
> 
> I think there are a number of different resolutions across SoCs that will
> exhibit this behavior, I've seen it with lower resolutions on 8250.
> 
> Its a bug that we need to drill into in Venus but I don't think is a bug that is
> specific to your setup.
> 
>>
>> real    0m21.467s
>> user    0m3.795s
>> sys    0m1.914s
>>
>>
>> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed
>> --length=30 demo-1440.webm
>>   (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
>>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> Using hardware decoding (v4l2m2m-copy).
>> VO: [null] 2560x1440 nv12
>> Exiting... (End of file)
>>
>> real    0m16.433s
>> user    0m1.764s
>> sys    0m1.118s
> 
> If this higher resolution is stable for you, I'd say this is about baseline.
> 
> 1. The GDSC change should make no impact on playback or available resolution
> 
> 2. Higher more "normal" use cases like 1080p should be fine.
> 
> If so, then file "low resolution is broken" under a "known unknown" and scrub
> your patches for submission.
> 
> If not, we need to do more 8998 specific debug.
> 
> ---
> bod

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

[Marc Gonzalez]

On 10/04/2024 10:17, Bryan O'Donoghue wrote:

> On 09/04/2024 17:53, Marc Gonzalez wrote:
>
>> On 09/04/2024 13:27, Bryan O'Donoghue wrote:
>>
>>> Can you give it a try ?
>>
>> Random notes
>>
>> For easy reference, I've used this command to test:
>>
>> $ mpv --hwdec=v4l2m2m-copy --vo=tct --quiet --no-audio demo-480.webm
>>
>> And it displays the video directly in the terminal :)
>> (Rendering speed depends on terminal size)
>>
>> I'd never played the video to the end.
>> I notice I get:
>>
>> [  397.410006] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d7c000
>> [  397.410114] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001
>>
>> How bad is that?
>>
>>
>> Sometimes, decoding simply fails immediately.
>> Must quit & restart.
>> Will have to script a 100 starts and check frequency of failures.
>>
>>
>> Will test with higher-resolution video.
>>
>> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed demo-480.webm
>>   (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
>>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> Using hardware decoding (v4l2m2m-copy).
>> VO: [null] 854x480 nv12
>> [ffmpeg/video] vp9_v4l2m2m: capture POLLERR
>> Exiting... (Quit)
>> /*** HANGS UNTIL CTRL-C ***/
> 
> I think there are a number of different resolutions across SoCs that 
> will exhibit this behavior, I've seen it with lower resolutions on 8250.
> 
> Its a bug that we need to drill into in Venus but I don't think is a bug 
> that is specific to your setup.
> 
>>
>> real	0m21.467s
>> user	0m3.795s
>> sys	0m1.914s
>>
>>
>> # time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed --length=30 demo-1440.webm
>>   (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
>>       Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
>> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
>> Using hardware decoding (v4l2m2m-copy).
>> VO: [null] 2560x1440 nv12
>> Exiting... (End of file)
>>
>> real	0m16.433s
>> user	0m1.764s
>> sys	0m1.118s
> 
> If this higher resolution is stable for you, I'd say this is about baseline.
> 
> 1. The GDSC change should make no impact on playback or available resolution
> 
> 2. Higher more "normal" use cases like 1080p should be fine.
> 
> If so, then file "low resolution is broken" under a "known unknown" and 
> scrub your patches for submission.
> 
> If not, we need to do more 8998 specific debug.

FWIW, I get the same behavior with 854x480 and 2560x1440:

1) If mpv runs with '--length=N' (play only part of the file)
then mpv exits cleanly, with no kernel messages.

2) If mpv plays the entire file, then mpv hangs at the end
(needs CTRL-C to exit) and driver prints to kernel:
[68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
[68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001


# time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed demo-1440.webm 
 (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
     Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
Using hardware decoding (v4l2m2m-copy).
VO: [null] 2560x1440 nv12
[ffmpeg/video] vp9_v4l2m2m: capture POLLERR
Exiting... (Quit)

real	2m43.292s
user	0m9.204s
sys	0m4.591s


# time mpv --hwdec=v4l2m2m-copy --vo=null --quiet --no-audio --untimed --length=300 demo-1440.webm 
 (+) Video --vid=1 (*) (vp9 2560x1440 59.940fps)
     Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
Using hardware decoding (v4l2m2m-copy).
VO: [null] 2560x1440 nv12
Exiting... (End of file)

real	2m32.412s
user	0m8.401s
sys	0m4.917s


300 seconds decoded in 152 seconds = 2x for 1440p.

Will send the patch series in a few hours.

Regards

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

[Bryan O'Donoghue]

On 10/04/2024 13:23, Marc Gonzalez wrote:
> FWIW, I get the same behavior with 854x480 and 2560x1440:
> 
> 1) If mpv runs with '--length=N' (play only part of the file)
> then mpv exits cleanly, with no kernel messages.

On -next ?

I think you mentioned before you were doing your work on an older kernel 
and forward porting patches ?

> 2) If mpv plays the entire file, then mpv hangs at the end
> (needs CTRL-C to exit) and driver prints to kernel:
> [68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event : 1001

Hmm

#define HFI_ERR_SESSION_FATAL				0x1001

Something is causing the firmware to return this packet to you.

Probably worth tracing the last five messages sent by the firmware prior 
to that to see if we can root-cause.

---
bod

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

[Marc Gonzalez]

On 10/04/2024 15:14, Bryan O'Donoghue wrote:

> On 10/04/2024 13:23, Marc Gonzalez wrote:
>
>> FWIW, I get the same behavior with 854x480 and 2560x1440:
>>
>> 1) If mpv runs with '--length=N' (play only part of the file)
>> then mpv exits cleanly, with no kernel messages.
> 
> On -next ?
> 
> I think you mentioned before you were doing your work on an older kernel 
> and forward porting patches ?

I work on v6.9-rc1
Is -next that much different in that area?

>> 2) If mpv plays the entire file, then mpv hangs at the end
>> (needs CTRL-C to exit) and driver prints to kernel:
>> [68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
>> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event : 1001
> 
> Hmm
> 
> #define HFI_ERR_SESSION_FATAL				0x1001
> 
> Something is causing the firmware to return this packet to you.
> 
> Probably worth tracing the last five messages sent by the firmware prior 
> to that to see if we can root-cause.

I'm not seeing anything from the FW in dmesg.
I suppose I need to enable DEBUG in various places?

Regards

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

[Bryan O'Donoghue]

On 10/04/2024 14:18, Marc Gonzalez wrote:
> On 10/04/2024 15:14, Bryan O'Donoghue wrote:
> 
>> On 10/04/2024 13:23, Marc Gonzalez wrote:
>>
>>> FWIW, I get the same behavior with 854x480 and 2560x1440:
>>>
>>> 1) If mpv runs with '--length=N' (play only part of the file)
>>> then mpv exits cleanly, with no kernel messages.
>>
>> On -next ?
>>
>> I think you mentioned before you were doing your work on an older kernel
>> and forward porting patches ?
> 
> I work on v6.9-rc1
> Is -next that much different in that area?

No, I thought you were on a 4.x kernel for some reason.

6.9.x is fine

> 
>>> 2) If mpv plays the entire file, then mpv hangs at the end
>>> (needs CTRL-C to exit) and driver prints to kernel:
>>> [68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
>>> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event : 1001
>>
>> Hmm
>>
>> #define HFI_ERR_SESSION_FATAL				0x1001
>>
>> Something is causing the firmware to return this packet to you.
>>
>> Probably worth tracing the last five messages sent by the firmware prior
>> to that to see if we can root-cause.
> 
> I'm not seeing anything from the FW in dmesg.
> I suppose I need to enable DEBUG in various places?

--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -129,7 +129,8 @@ struct venus_hfi_device {
         u8 dbg_buf[IFACEQ_VAR_HUGE_PKT_SIZE];
  };

-static bool venus_pkt_debug;
+#define DEBUG
+static bool venus_pkt_debug = true;

We can add additional flags - it'd be nice if these could be controlled 
by a module prameter or debugfs trigger - to this

venus_fw_debug = 0x2f;

But start with the default mask = int venus_fw_debug = 
HFI_DEBUG_MSG_ERROR | HFI_DEBUG_MSG_FATAL

---
bod

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

[Bryan O'Donoghue]

On 10/04/2024 14:29, Bryan O'Donoghue wrote:
> venus_fw_debug = 0x2f;

* 0x3f;

---
bod

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

[Bryan O'Donoghue]

On 10/04/2024 14:14, Bryan O'Donoghue wrote:
> On 10/04/2024 13:23, Marc Gonzalez wrote:
>> FWIW, I get the same behavior with 854x480 and 2560x1440:
>>
>> 1) If mpv runs with '--length=N' (play only part of the file)
>> then mpv exits cleanly, with no kernel messages.
> 
> On -next ?
> 
> I think you mentioned before you were doing your work on an older kernel 
> and forward porting patches ?
> 
>> 2) If mpv plays the entire file, then mpv hangs at the end
>> (needs CTRL-C to exit) and driver prints to kernel:
>> [68643.935888] qcom-venus cc00000.video-codec: session error: event 
>> id:1001 (deadb000), session id:79d42000
>> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: 
>> dec: event : 1001
> 
> Hmm
> 
> #define HFI_ERR_SESSION_FATAL                0x1001
> 
> Something is causing the firmware to return this packet to you.
> 
> Probably worth tracing the last five messages sent by the firmware prior 
> to that to see if we can root-cause.
> 
> ---
> bod

BTW I think you've found two bugs here

1. When we receive a fatal error from firmware we don't "bug out"
    properly. So we hang forever waiting for more events which
    won't come.
    We should fix the handling of SESSION_FATAL to => termination.
    Clearly after HFI_ERR_SESSION_FATAL we should be completely done
    not hanging around waiting for additional inputs.

2. Why do we get a fatal error for the session ?
    Are we continuing to send commands to the firmware after
    termination maybe - so is there a incongruous context
    between firmware and Linux ?

I don't think either is a blocker specifically for your DTS submission 
so I think you should go ahead with your DTS stuff.

Also though, I think 1) we don't exit properly on HFI_ERR_SESSION_FATAL 
and 2) we should root-cause why HFI_ERR_SESSION_FATAL is generated at all.

---
bod

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

[Marc Gonzalez]

On 10/04/2024 15:14, Bryan O'Donoghue wrote:

> On 10/04/2024 13:23, Marc Gonzalez wrote:
>
>> FWIW, I get the same behavior with 854x480 and 2560x1440:
>>
>> 1) If mpv runs with '--length=N' (play only part of the file)
>> then mpv exits cleanly, with no kernel messages.
> 
> On -next ?
> 
> I think you mentioned before you were doing your work on an older kernel 
> and forward porting patches ?
> 
>> 2) If mpv plays the entire file, then mpv hangs at the end
>> (needs CTRL-C to exit) and driver prints to kernel:
>> [68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
>> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event : 1001
> 
> Hmm
> 
> #define HFI_ERR_SESSION_FATAL				0x1001
> 
> Something is causing the firmware to return this packet to you.
> 
> Probably worth tracing the last five messages sent by the firmware prior 
> to that to see if we can root-cause.

On kernel command line: log_buf_len=16777216
Before decode: dmesg -n 1

# time mpv --hwdec=v4l2m2m-copy --vd-lavc-software-fallback=no --vo=null --no-audio --untimed --quiet demo-480.webm
 (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
     Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
[ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
Using hardware decoding (v4l2m2m-copy).
VO: [null] 854x480 nv12
[ffmpeg/video] vp9_v4l2m2m: capture POLLERR
Exiting... (Quit)
real	0m26.810s
user	0m4.168s
sys	0m3.690s



Do you see anything relevant?


[  107.135902] tutu: 00000000: 24 00 00 00 01 10 02 00 00 b0 0c 7a 06 00 00 01  $..........z....
[  107.135908] tutu: 00000010: 00 00 00 00 00 00 00 00 00 00 70 d8 00 00 00 00  ..........p.....
[  107.135913] tutu: 00000020: 08 00 00 00                                      ....
[  107.135933] tutu: 00000000: 68 00 00 00 08 10 22 00 00 b0 0c 7a 00 00 00 00  h....."....z....
[  107.135939] tutu: 00000010: 4d 04 00 00 00 00 00 00 00 00 00 00 60 bf aa 12  M...........`...
[  107.135943] tutu: 00000020: 00 02 00 00 00 00 00 00 00 00 00 00 7e 1b 43 75  ............~.Cu
[  107.135948] tutu: 00000030: 00 10 0a 00 00 d8 09 00 00 00 00 00 56 03 00 00  ............V...
[  107.135953] tutu: 00000040: e0 01 00 00 00 00 00 00 00 00 00 00 0a 00 00 00  ................
[  107.135958] tutu: 00000050: 00 00 00 00 05 00 00 00 02 00 00 00 00 00 a0 d8  ................
[  107.135963] tutu: 00000060: 00 00 00 00 00 00 00 00                          ........
[  107.136433] tutu: 00000000: 2c 00 00 00 05 10 21 00 00 b0 0c 7a 00 00 00 00  ,.....!....z....
[  107.136464] tutu: 00000010: 00 00 00 00 00 10 0a 00 00 00 00 00 07 00 00 00  ................
[  107.136469] tutu: 00000020: 00 00 80 d8 00 00 00 00 00 00 00 00              ............
[  107.136614] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
[  107.136661] tutu: 00000010: 50 e5 b2 12 00 00 00 00 00 00 00 00 00 00 00 00  P...............
[  107.136667] tutu: 00000020: 00 00 00 00 00 00 26 00 9f 00 00 00 0a 00 00 00  ......&.........
[  107.136672] tutu: 00000030: 00 00 c0 da 00 00 00 00 28 3d fa 85              ........(=..
[  107.137791] tutu: 00000000: 28 00 00 00 07 10 22 00 00 b0 0c 7a 00 00 00 00  (....."....z....
[  107.137799] tutu: 00000010: 9c 00 10 00 18 17 11 00 0b 00 00 00 00 00 80 da  ................
[  107.137804] tutu: 00000020: 00 00 00 00 00 00 00 00                          ........
[  107.137815] tutu: 00000000: 88 00 00 00 04 00 02 00 04 00 00 00 6c 00 00 00  ............l...
[  107.137820] tutu: 00000010: 00 00 00 00 c8 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.137826] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.137832] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 39 20 48 65 61 64  b000:0> VP9 Head
[  107.137837] tutu: 00000040: 65 72 20 28 49 4e 54 45 52 20 46 52 41 4d 45 20  er (INTER FRAME 
[  107.137842] tutu: 00000050: 2d 20 66 72 6f 6d 20 61 63 74 69 76 65 5f 72 65  - from active_re
[  107.137847] tutu: 00000060: 66 5f 69 64 78 5f 65 6e 61 62 6c 65 64 29 3a 57  f_idx_enabled):W
[  107.137851] tutu: 00000070: 69 64 74 68 3d 38 35 34 20 48 65 69 67 68 74 3d  idth=854 Height=
[  107.137857] tutu: 00000080: 34 38 30 00 20 4b 65 79                          480. Key
[  107.137863] tutu: 00000000: 76 00 00 00 04 00 02 00 08 00 00 00 5a 00 00 00  v...........Z...
[  107.137868] tutu: 00000010: 00 00 00 00 c9 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
[  107.137874] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.137878] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
[  107.137883] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
[  107.137888] tutu: 00000050: 65 28 38 33 32 35 29 3a 20 6d 61 72 6b 65 72 20  e(8325): marker 
[  107.137893] tutu: 00000060: 76 61 6c 75 65 20 62 65 66 6f 72 65 20 3d 20 30  value before = 0
[  107.137898] tutu: 00000070: 0a 00 46 52 3d 38                                ..FR=8
[  107.137905] tutu: 00000000: 75 00 00 00 04 00 02 00 08 00 00 00 59 00 00 00  u...........Y...
[  107.137909] tutu: 00000010: 00 00 00 00 ca 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
[  107.137915] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.137919] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
[  107.137924] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
[  107.137929] tutu: 00000050: 65 28 38 33 33 30 29 3a 20 6d 61 72 6b 65 72 20  e(8330): marker 
[  107.137934] tutu: 00000060: 76 61 6c 75 65 20 61 66 74 65 72 20 3d 20 30 0a  value after = 0.
[  107.137940] tutu: 00000070: 00 00 46 52 3d                                   ..FR=
[  107.137946] tutu: 00000000: 7c 00 00 00 04 00 02 00 04 00 00 00 60 00 00 00  |...........`...
[  107.137952] tutu: 00000010: 00 00 00 00 cb 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.137957] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
[  107.137962] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
[  107.137967] tutu: 00000040: 64 65 72 3a 20 28 44 69 72 65 63 74 20 4d 6f 64  der: (Direct Mod
[  107.137971] tutu: 00000050: 65 29 20 53 50 44 2d 41 50 53 50 20 46 72 61 6d  e) SPD-APSP Fram
[  107.137976] tutu: 00000060: 65 20 31 30 30 36 37 20 44 6f 6e 65 20 53 65 73  e 10067 Done Ses
[  107.137981] tutu: 00000070: 73 69 6f 6e 20 30 0a 00 6e 20 30 0a              sion 0..n 0.
[  107.137987] tutu: 00000000: 53 00 00 00 04 00 02 00 04 00 00 00 37 00 00 00  S...........7...
[  107.137992] tutu: 00000010: 00 00 00 00 cc 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.137997] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
[  107.138002] tutu: 00000030: 62 30 30 30 3a 30 3e 20 50 72 6f 63 65 73 73 69  b000:0> Processi
[  107.138007] tutu: 00000040: 6e 67 20 44 69 72 65 63 74 20 4d 6f 64 65 00 64  ng Direct Mode.d
[  107.138012] tutu: 00000050: 65 29 20                                         e) 
[  107.138018] tutu: 00000000: 68 00 00 00 04 00 02 00 04 00 00 00 4c 00 00 00  h...........L...
[  107.138024] tutu: 00000010: 00 00 00 00 cd 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.138028] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
[  107.138033] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
[  107.138038] tutu: 00000040: 64 65 72 3a 56 50 50 2d 53 44 45 20 50 72 6f 63  der:VPP-SDE Proc
[  107.138043] tutu: 00000050: 65 73 73 69 6e 67 20 46 72 61 6d 65 20 31 30 30  essing Frame 100
[  107.138048] tutu: 00000060: 36 38 0a 00 30 36 37 20                          68..067 
[  107.138054] tutu: 00000000: 81 00 00 00 04 00 02 00 04 00 00 00 65 00 00 00  ............e...
[  107.138059] tutu: 00000010: 00 00 00 00 ce 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.138064] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
[  107.138069] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
[  107.138074] tutu: 00000040: 64 65 72 3a 20 28 44 69 72 65 63 74 20 4d 6f 64  der: (Direct Mod
[  107.138079] tutu: 00000050: 65 29 20 53 65 6e 64 69 6e 67 20 46 72 61 6d 65  e) Sending Frame
[  107.138083] tutu: 00000060: 20 31 30 30 36 38 20 74 6f 20 56 50 50 2d 53 44   10068 to VPP-SD
[  107.138089] tutu: 00000070: 45 20 53 65 73 73 69 6f 6e 20 30 0a 00 00 00 00  E Session 0.....
[  107.138093] tutu: 00000080: 34                                               4
[  107.138111] tutu: 00000000: 24 00 00 00 01 10 02 00 00 b0 0c 7a 06 00 00 01  $..........z....
[  107.138117] tutu: 00000010: 00 00 00 00 00 00 00 00 00 00 a0 d8 00 00 00 00  ................
[  107.138121] tutu: 00000020: 05 00 00 00                                      ....
[  107.138137] tutu: 00000000: 68 00 00 00 08 10 22 00 00 b0 0c 7a 00 00 00 00  h....."....z....
[  107.138142] tutu: 00000010: 4d 04 00 00 00 00 00 00 00 00 00 00 48 40 ab 12  M...........H@..
[  107.138147] tutu: 00000020: 00 02 00 00 00 00 00 00 00 00 00 00 7e 1b 43 75  ............~.Cu
[  107.138152] tutu: 00000030: 00 10 0a 00 00 d8 09 00 00 00 00 00 56 03 00 00  ............V...
[  107.138157] tutu: 00000040: e0 01 00 00 00 00 00 00 00 00 00 00 0b 00 00 00  ................
[  107.138162] tutu: 00000050: 00 00 00 00 01 00 00 00 02 00 00 00 00 00 e0 d8  ................
[  107.138167] tutu: 00000060: 00 00 00 00 00 00 00 00                          ........
[  107.138173] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
[  107.138180] tutu: 00000010: 38 66 b3 12 00 00 00 00 00 00 00 00 00 00 00 00  8f..............
[  107.138186] tutu: 00000020: 00 00 00 00 00 00 26 00 32 00 00 00 0b 00 00 00  ......&.2.......
[  107.138191] tutu: 00000030: 00 00 80 da 00 00 00 00 28 3d fa 85              ........(=..
[  107.138699] tutu: 00000000: 2c 00 00 00 05 10 21 00 00 b0 0c 7a 00 00 00 00  ,.....!....z....
[  107.138712] tutu: 00000010: 00 00 00 00 00 10 0a 00 00 00 00 00 06 00 00 00  ................
[  107.138717] tutu: 00000020: 00 00 90 d8 00 00 00 00 00 00 00 00              ............
[  107.138947] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
[  107.138955] tutu: 00000010: 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  ................
[  107.138961] tutu: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
[  107.138966] tutu: 00000030: 00 b0 ad de 00 00 00 00 00 00 00 00              ............
[  107.139622] tutu: 00000000: 1c 00 00 00 01 10 02 00 00 b0 0c 7a 02 00 00 00  ...........z....
[  107.139661] tutu: 00000010: 01 10 00 00 00 b0 ad de 03 00 01 00              ............

[  107.139670] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:7a0cb000
[  107.139695] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001

[  107.139703] tutu: 00000000: 28 00 00 00 07 10 22 00 00 b0 0c 7a 03 10 00 00  (....."....z....
[  107.139708] tutu: 00000010: 10 00 00 00 0c 00 00 00 00 00 00 00 00 b0 ad de  ................
[  107.139713] tutu: 00000020: 20 00 00 00 c0 46 02 00                           ....F..
[  107.139726] tutu: 00000000: 88 00 00 00 04 00 02 00 04 00 00 00 6c 00 00 00  ............l...
[  107.139732] tutu: 00000010: 00 00 00 00 cf 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
[  107.139737] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.139742] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 39 20 48 65 61 64  b000:0> VP9 Head
[  107.139747] tutu: 00000040: 65 72 20 28 49 4e 54 45 52 20 46 52 41 4d 45 20  er (INTER FRAME 
[  107.139752] tutu: 00000050: 2d 20 66 72 6f 6d 20 61 63 74 69 76 65 5f 72 65  - from active_re
[  107.139757] tutu: 00000060: 66 5f 69 64 78 5f 65 6e 61 62 6c 65 64 29 3a 57  f_idx_enabled):W
[  107.139762] tutu: 00000070: 69 64 74 68 3d 38 35 34 20 48 65 69 67 68 74 3d  idth=854 Height=
[  107.139772] tutu: 00000080: 34 38 30 00 20 4b 65 79                          480. Key
[  107.139778] tutu: 00000000: 76 00 00 00 04 00 02 00 08 00 00 00 5a 00 00 00  v...........Z...
[  107.139783] tutu: 00000010: 00 00 00 00 d0 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
[  107.139788] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.139793] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
[  107.139798] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
[  107.139803] tutu: 00000050: 65 28 38 33 32 35 29 3a 20 6d 61 72 6b 65 72 20  e(8325): marker 
[  107.139807] tutu: 00000060: 76 61 6c 75 65 20 62 65 66 6f 72 65 20 3d 20 30  value before = 0
[  107.139813] tutu: 00000070: 0a 00 46 52 3d 38                                ..FR=8
[  107.139819] tutu: 00000000: 75 00 00 00 04 00 02 00 08 00 00 00 59 00 00 00  u...........Y...
[  107.139824] tutu: 00000010: 00 00 00 00 d1 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
[  107.139829] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
[  107.139834] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
[  107.139839] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
[  107.139843] tutu: 00000050: 65 28 38 33 33 30 29 3a 20 6d 61 72 6b 65 72 20  e(8330): marker 
[  107.139848] tutu: 00000060: 76 61 6c 75 65 20 61 66 74 65 72 20 3d 20 30 0a  value after = 0.
[  107.139853] tutu: 00000070: 00 00 46 52 3d                                   ..FR=


Regards

Re: venus decoder hangs at EOF

[Marc Gonzalez]

On 25/04/2024 18:43, Marc Gonzalez wrote:

> On 10/04/2024 15:14, Bryan O'Donoghue wrote:
> 
>> On 10/04/2024 13:23, Marc Gonzalez wrote:
>>
>>> FWIW, I get the same behavior with 854x480 and 2560x1440:
>>>
>>> 2) If mpv plays the entire file, then mpv hangs at the end
>>> (needs CTRL-C to exit) and driver prints to kernel:
>>> [68643.935888] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:79d42000
>>> [68643.935995] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event : 1001
>>
>> Hmm
>>
>> #define HFI_ERR_SESSION_FATAL				0x1001
>>
>> Something is causing the firmware to return this packet to you.
>>
>> Probably worth tracing the last five messages sent by the firmware prior 
>> to that to see if we can root-cause.
> 
> On kernel command line: log_buf_len=16777216
> Before decode: dmesg -n 1
> 
> # time mpv --hwdec=v4l2m2m-copy --vd-lavc-software-fallback=no --vo=null --no-audio --untimed --quiet demo-480.webm
>  (+) Video --vid=1 (*) (vp9 854x480 29.970fps)
>      Audio --aid=1 --alang=eng (*) (opus 2ch 48000Hz)
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> [ffmpeg/video] vp9_v4l2m2m: VIDIOC_G_FMT ioctl
> Using hardware decoding (v4l2m2m-copy).
> VO: [null] 854x480 nv12
> [ffmpeg/video] vp9_v4l2m2m: capture POLLERR
> Exiting... (Quit)
> real	0m26.810s
> user	0m4.168s
> sys	0m3.690s
> 
> 
> 
> Do you see anything relevant?
> 
> 
> [  107.135902] tutu: 00000000: 24 00 00 00 01 10 02 00 00 b0 0c 7a 06 00 00 01  $..........z....
> [  107.135908] tutu: 00000010: 00 00 00 00 00 00 00 00 00 00 70 d8 00 00 00 00  ..........p.....
> [  107.135913] tutu: 00000020: 08 00 00 00                                      ....
> [  107.135933] tutu: 00000000: 68 00 00 00 08 10 22 00 00 b0 0c 7a 00 00 00 00  h....."....z....
> [  107.135939] tutu: 00000010: 4d 04 00 00 00 00 00 00 00 00 00 00 60 bf aa 12  M...........`...
> [  107.135943] tutu: 00000020: 00 02 00 00 00 00 00 00 00 00 00 00 7e 1b 43 75  ............~.Cu
> [  107.135948] tutu: 00000030: 00 10 0a 00 00 d8 09 00 00 00 00 00 56 03 00 00  ............V...
> [  107.135953] tutu: 00000040: e0 01 00 00 00 00 00 00 00 00 00 00 0a 00 00 00  ................
> [  107.135958] tutu: 00000050: 00 00 00 00 05 00 00 00 02 00 00 00 00 00 a0 d8  ................
> [  107.135963] tutu: 00000060: 00 00 00 00 00 00 00 00                          ........
> [  107.136433] tutu: 00000000: 2c 00 00 00 05 10 21 00 00 b0 0c 7a 00 00 00 00  ,.....!....z....
> [  107.136464] tutu: 00000010: 00 00 00 00 00 10 0a 00 00 00 00 00 07 00 00 00  ................
> [  107.136469] tutu: 00000020: 00 00 80 d8 00 00 00 00 00 00 00 00              ............
> [  107.136614] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
> [  107.136661] tutu: 00000010: 50 e5 b2 12 00 00 00 00 00 00 00 00 00 00 00 00  P...............
> [  107.136667] tutu: 00000020: 00 00 00 00 00 00 26 00 9f 00 00 00 0a 00 00 00  ......&.........
> [  107.136672] tutu: 00000030: 00 00 c0 da 00 00 00 00 28 3d fa 85              ........(=..
> [  107.137791] tutu: 00000000: 28 00 00 00 07 10 22 00 00 b0 0c 7a 00 00 00 00  (....."....z....
> [  107.137799] tutu: 00000010: 9c 00 10 00 18 17 11 00 0b 00 00 00 00 00 80 da  ................
> [  107.137804] tutu: 00000020: 00 00 00 00 00 00 00 00                          ........
> [  107.137815] tutu: 00000000: 88 00 00 00 04 00 02 00 04 00 00 00 6c 00 00 00  ............l...
> [  107.137820] tutu: 00000010: 00 00 00 00 c8 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.137826] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.137832] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 39 20 48 65 61 64  b000:0> VP9 Head
> [  107.137837] tutu: 00000040: 65 72 20 28 49 4e 54 45 52 20 46 52 41 4d 45 20  er (INTER FRAME 
> [  107.137842] tutu: 00000050: 2d 20 66 72 6f 6d 20 61 63 74 69 76 65 5f 72 65  - from active_re
> [  107.137847] tutu: 00000060: 66 5f 69 64 78 5f 65 6e 61 62 6c 65 64 29 3a 57  f_idx_enabled):W
> [  107.137851] tutu: 00000070: 69 64 74 68 3d 38 35 34 20 48 65 69 67 68 74 3d  idth=854 Height=
> [  107.137857] tutu: 00000080: 34 38 30 00 20 4b 65 79                          480. Key
> [  107.137863] tutu: 00000000: 76 00 00 00 04 00 02 00 08 00 00 00 5a 00 00 00  v...........Z...
> [  107.137868] tutu: 00000010: 00 00 00 00 c9 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
> [  107.137874] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.137878] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
> [  107.137883] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
> [  107.137888] tutu: 00000050: 65 28 38 33 32 35 29 3a 20 6d 61 72 6b 65 72 20  e(8325): marker 
> [  107.137893] tutu: 00000060: 76 61 6c 75 65 20 62 65 66 6f 72 65 20 3d 20 30  value before = 0
> [  107.137898] tutu: 00000070: 0a 00 46 52 3d 38                                ..FR=8
> [  107.137905] tutu: 00000000: 75 00 00 00 04 00 02 00 08 00 00 00 59 00 00 00  u...........Y...
> [  107.137909] tutu: 00000010: 00 00 00 00 ca 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
> [  107.137915] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.137919] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
> [  107.137924] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
> [  107.137929] tutu: 00000050: 65 28 38 33 33 30 29 3a 20 6d 61 72 6b 65 72 20  e(8330): marker 
> [  107.137934] tutu: 00000060: 76 61 6c 75 65 20 61 66 74 65 72 20 3d 20 30 0a  value after = 0.
> [  107.137940] tutu: 00000070: 00 00 46 52 3d                                   ..FR=
> [  107.137946] tutu: 00000000: 7c 00 00 00 04 00 02 00 04 00 00 00 60 00 00 00  |...........`...
> [  107.137952] tutu: 00000010: 00 00 00 00 cb 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.137957] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
> [  107.137962] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
> [  107.137967] tutu: 00000040: 64 65 72 3a 20 28 44 69 72 65 63 74 20 4d 6f 64  der: (Direct Mod
> [  107.137971] tutu: 00000050: 65 29 20 53 50 44 2d 41 50 53 50 20 46 72 61 6d  e) SPD-APSP Fram
> [  107.137976] tutu: 00000060: 65 20 31 30 30 36 37 20 44 6f 6e 65 20 53 65 73  e 10067 Done Ses
> [  107.137981] tutu: 00000070: 73 69 6f 6e 20 30 0a 00 6e 20 30 0a              sion 0..n 0.
> [  107.137987] tutu: 00000000: 53 00 00 00 04 00 02 00 04 00 00 00 37 00 00 00  S...........7...
> [  107.137992] tutu: 00000010: 00 00 00 00 cc 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.137997] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
> [  107.138002] tutu: 00000030: 62 30 30 30 3a 30 3e 20 50 72 6f 63 65 73 73 69  b000:0> Processi
> [  107.138007] tutu: 00000040: 6e 67 20 44 69 72 65 63 74 20 4d 6f 64 65 00 64  ng Direct Mode.d
> [  107.138012] tutu: 00000050: 65 29 20                                         e) 
> [  107.138018] tutu: 00000000: 68 00 00 00 04 00 02 00 04 00 00 00 4c 00 00 00  h...........L...
> [  107.138024] tutu: 00000010: 00 00 00 00 cd 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.138028] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
> [  107.138033] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
> [  107.138038] tutu: 00000040: 64 65 72 3a 56 50 50 2d 53 44 45 20 50 72 6f 63  der:VPP-SDE Proc
> [  107.138043] tutu: 00000050: 65 73 73 69 6e 67 20 46 72 61 6d 65 20 31 30 30  essing Frame 100
> [  107.138048] tutu: 00000060: 36 38 0a 00 30 36 37 20                          68..067 
> [  107.138054] tutu: 00000000: 81 00 00 00 04 00 02 00 04 00 00 00 65 00 00 00  ............e...
> [  107.138059] tutu: 00000010: 00 00 00 00 ce 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.138064] tutu: 00000020: 56 70 70 44 65 63 3a 76 70 39 64 3a 37 61 30 63  VppDec:vp9d:7a0c
> [  107.138069] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 78 20 44 65 63 6f  b000:0> VPx Deco
> [  107.138074] tutu: 00000040: 64 65 72 3a 20 28 44 69 72 65 63 74 20 4d 6f 64  der: (Direct Mod
> [  107.138079] tutu: 00000050: 65 29 20 53 65 6e 64 69 6e 67 20 46 72 61 6d 65  e) Sending Frame
> [  107.138083] tutu: 00000060: 20 31 30 30 36 38 20 74 6f 20 56 50 50 2d 53 44   10068 to VPP-SD
> [  107.138089] tutu: 00000070: 45 20 53 65 73 73 69 6f 6e 20 30 0a 00 00 00 00  E Session 0.....
> [  107.138093] tutu: 00000080: 34                                               4
> [  107.138111] tutu: 00000000: 24 00 00 00 01 10 02 00 00 b0 0c 7a 06 00 00 01  $..........z....
> [  107.138117] tutu: 00000010: 00 00 00 00 00 00 00 00 00 00 a0 d8 00 00 00 00  ................
> [  107.138121] tutu: 00000020: 05 00 00 00                                      ....
> [  107.138137] tutu: 00000000: 68 00 00 00 08 10 22 00 00 b0 0c 7a 00 00 00 00  h....."....z....
> [  107.138142] tutu: 00000010: 4d 04 00 00 00 00 00 00 00 00 00 00 48 40 ab 12  M...........H@..
> [  107.138147] tutu: 00000020: 00 02 00 00 00 00 00 00 00 00 00 00 7e 1b 43 75  ............~.Cu
> [  107.138152] tutu: 00000030: 00 10 0a 00 00 d8 09 00 00 00 00 00 56 03 00 00  ............V...
> [  107.138157] tutu: 00000040: e0 01 00 00 00 00 00 00 00 00 00 00 0b 00 00 00  ................
> [  107.138162] tutu: 00000050: 00 00 00 00 01 00 00 00 02 00 00 00 00 00 e0 d8  ................
> [  107.138167] tutu: 00000060: 00 00 00 00 00 00 00 00                          ........
> [  107.138173] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
> [  107.138180] tutu: 00000010: 38 66 b3 12 00 00 00 00 00 00 00 00 00 00 00 00  8f..............
> [  107.138186] tutu: 00000020: 00 00 00 00 00 00 26 00 32 00 00 00 0b 00 00 00  ......&.2.......
> [  107.138191] tutu: 00000030: 00 00 80 da 00 00 00 00 28 3d fa 85              ........(=..
> [  107.138699] tutu: 00000000: 2c 00 00 00 05 10 21 00 00 b0 0c 7a 00 00 00 00  ,.....!....z....
> [  107.138712] tutu: 00000010: 00 00 00 00 00 10 0a 00 00 00 00 00 06 00 00 00  ................
> [  107.138717] tutu: 00000020: 00 00 90 d8 00 00 00 00 00 00 00 00              ............
> [  107.138947] tutu: 00000000: 3c 00 00 00 04 10 21 00 00 b0 0c 7a 00 00 00 00  <.....!....z....
> [  107.138955] tutu: 00000010: 00 00 00 00 01 00 00 00 00 00 00 00 00 00 00 00  ................
> [  107.138961] tutu: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
> [  107.138966] tutu: 00000030: 00 b0 ad de 00 00 00 00 00 00 00 00              ............
> [  107.139622] tutu: 00000000: 1c 00 00 00 01 10 02 00 00 b0 0c 7a 02 00 00 00  ...........z....
> [  107.139661] tutu: 00000010: 01 10 00 00 00 b0 ad de 03 00 01 00              ............
> 
> [  107.139670] qcom-venus cc00000.video-codec: session error: event id:1001 (deadb000), session id:7a0cb000
> [  107.139695] qcom-venus-decoder cc00000.video-codec:video-decoder: dec: event session error 1001
> 
> [  107.139703] tutu: 00000000: 28 00 00 00 07 10 22 00 00 b0 0c 7a 03 10 00 00  (....."....z....
> [  107.139708] tutu: 00000010: 10 00 00 00 0c 00 00 00 00 00 00 00 00 b0 ad de  ................
> [  107.139713] tutu: 00000020: 20 00 00 00 c0 46 02 00                           ....F..
> [  107.139726] tutu: 00000000: 88 00 00 00 04 00 02 00 04 00 00 00 6c 00 00 00  ............l...
> [  107.139732] tutu: 00000010: 00 00 00 00 cf 11 01 00 0a 3c 56 46 57 5f 48 3a  .........<VFW_H:
> [  107.139737] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.139742] tutu: 00000030: 62 30 30 30 3a 30 3e 20 56 50 39 20 48 65 61 64  b000:0> VP9 Head
> [  107.139747] tutu: 00000040: 65 72 20 28 49 4e 54 45 52 20 46 52 41 4d 45 20  er (INTER FRAME 
> [  107.139752] tutu: 00000050: 2d 20 66 72 6f 6d 20 61 63 74 69 76 65 5f 72 65  - from active_re
> [  107.139757] tutu: 00000060: 66 5f 69 64 78 5f 65 6e 61 62 6c 65 64 29 3a 57  f_idx_enabled):W
> [  107.139762] tutu: 00000070: 69 64 74 68 3d 38 35 34 20 48 65 69 67 68 74 3d  idth=854 Height=
> [  107.139772] tutu: 00000080: 34 38 30 00 20 4b 65 79                          480. Key
> [  107.139778] tutu: 00000000: 76 00 00 00 04 00 02 00 08 00 00 00 5a 00 00 00  v...........Z...
> [  107.139783] tutu: 00000010: 00 00 00 00 d0 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
> [  107.139788] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.139793] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
> [  107.139798] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
> [  107.139803] tutu: 00000050: 65 28 38 33 32 35 29 3a 20 6d 61 72 6b 65 72 20  e(8325): marker 
> [  107.139807] tutu: 00000060: 76 61 6c 75 65 20 62 65 66 6f 72 65 20 3d 20 30  value before = 0
> [  107.139813] tutu: 00000070: 0a 00 46 52 3d 38                                ..FR=8
> [  107.139819] tutu: 00000000: 75 00 00 00 04 00 02 00 08 00 00 00 59 00 00 00  u...........Y...
> [  107.139824] tutu: 00000010: 00 00 00 00 d1 11 01 00 0a 3c 56 46 57 5f 45 3a  .........<VFW_E:
> [  107.139829] tutu: 00000020: 57 6f 72 6b 65 72 3a 76 70 39 64 3a 37 61 30 63  Worker:vp9d:7a0c
> [  107.139834] tutu: 00000030: 62 30 30 30 3a 30 3e 20 76 70 78 44 65 63 5f 50  b000:0> vpxDec_P
> [  107.139839] tutu: 00000040: 61 72 73 65 46 6f 72 53 75 70 65 72 46 72 61 6d  arseForSuperFram
> [  107.139843] tutu: 00000050: 65 28 38 33 33 30 29 3a 20 6d 61 72 6b 65 72 20  e(8330): marker 
> [  107.139848] tutu: 00000060: 76 61 6c 75 65 20 61 66 74 65 72 20 3d 20 30 0a  value after = 0.
> [  107.139853] tutu: 00000070: 00 00 46 52 3d                                   ..FR=


Bryan suggested adding Dikshita.

Is there any useful information in the debug output above?

Regards

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

[Rob Herring]

On Mon, 19 Feb 2024 18:18:55 +0100, Marc Gonzalez wrote:
> From: Pierre-Hugues Husson <phhusson@freebox.fr>
> 
> On our msm8998-based device, calling venus_sys_set_power_control()
> breaks playback. Since the vendor kernel never calls it, we assume
> it should not be called for this device/FW combo.
> 
> Signed-off-by: Pierre-Hugues Husson <phhusson@freebox.fr>
> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
> TODO in v2: split the patch in 2
> Is "qcom,no-low-power" a proper name for the property?
> Is a boolean property the right approach?
> ---
>  .../devicetree/bindings/media/qcom,venus-common.yaml     | 3 +++
>  drivers/media/platform/qcom/venus/hfi_venus.c            | 9 +++++----
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/media/qcom,venus-common.yaml: properties:qcom,no-low-power: 'description' is a dependency of 'type'
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/0843621b-386b-4173-9e3c-9538cdb4641d@freebox.fr

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.