| Message ID | 0843621b-386b-4173-9e3c-9538cdb4641d@freebox.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC,WIP] venus: add qcom,no-low-power property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 24 lines checked |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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
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?
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.
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
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
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
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
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
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
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
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
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
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
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
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
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) {