| Message ID | 20241125-add-venus-for-qcs615-v3-0-5a376b97a68e@quicinc.com |
|---|---|
| Headers | show |
| Series | media: venus: enable venus on qcs615 | expand |
On 11/25/2024 11:55 PM, Krzysztof Kozlowski wrote: > On 25/11/2024 16:49, Renjiang Han (QUIC) wrote: >>>> + video-decoder: >>>> + type: object >>>> + >>>> + additionalProperties: false >>>> + >>>> + properties: >>>> + compatible: >>>> + const: venus-decoder >>>> + >>>> + required: >>>> + - compatible >>>> + >>>> + video-encoder: >>>> + type: object >>> Both nodes are useless - no resources here, nothing to control. >>> Do not add nodes just to instantiate Linux drivers. Drop them. >> Do you mean I should remove video-decoder and video-encoder from here? > Yes, that's my suggestion. > >> If so, do I also need to remove these two nodes from the dtsi file and add > Yes > >> them in the qcs615-ride.dts file? > Well, no, how would it pass dtbs_check? > > Don't add nodes purely for Linux driver instantiation. OK, I got it. I'll update like this. If video-decoder and video-encoder are removed from dtsi file and not added to qcs615-ride.dts file, then the video decoder and encoder functions will not be available on the qcs615 platform. So I think these two nodes should be added to the qcs615-ride.dts file to ensure that the qcs615 platform can enable the video decoder and encoder functions. > Best regards, > Krzysztof
On 26/11/2024 10:39, Renjiang Han wrote: >>>>> If so, do I also need to remove these two nodes from the dtsi file and add >>>> Yes >>>> >>>>> them in the qcs615-ride.dts file? >>>> Well, no, how would it pass dtbs_check? >>>> >>>> Don't add nodes purely for Linux driver instantiation. >>> OK, I got it. I'll update like this. If video-decoder and video-encoder are >>> >>> removed from dtsi file and not added to qcs615-ride.dts file, then the >>> >>> video decoder and encoder functions will not be available on the qcs615 >>> >>> platform. So I think these two nodes should be added to the >>> >>> qcs615-ride.dts file to ensure that the qcs615 platform can enable the >>> >>> video decoder and encoder functions. >> You just repeated the same sentences. Address my comment instead - empty >> device nodes should not be used just to instantiate Linux device drivers. > > Thanks for your reply. I agree with your comment. The two nodes > video-decoder and > > video-encoder should not be placed in the devicetree. But this is > affected by the venus > > driver. On the old platform, some only need to enable the video-decoder > function or > > only enable the video-encoder function. So these two nodes were added to the > > devicetree at that time. For new platforms, the iris driver will be used > in the future, > > and this situation will not occur. These are new bindings, for new device, so please fix your driver. We had similar talk long time ago and answer was that it's a legacy driver which won't be developed. This means also no new devices. If you bring new devices to old driver, instead of to new iris, then it means you still develop old driver. Fix the old driver. Best regards, Krzysztof
On 26/11/2024 10:58, Renjiang Han wrote: > > On 11/26/2024 5:34 PM, Krzysztof Kozlowski wrote: >> On 26/11/2024 09:57, Renjiang Han wrote: >>>>> +description: >>>>> + The Venus IP is a video encode and decode accelerator present >>>>> + on Qualcomm platforms >>>>> + >>>>> +allOf: >>>>> + - $ref: qcom,venus-common.yaml# >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: qcom,qcs615-venus >>>> Please extend sc7180-venus.yaml instead. No need to duplicate >>>> unnecessary things. >>> Thanks for your review. But I'm sorry I can't get it. The devicetree for >>> >>> qcs615-venus is in qcs615.dtsi. I'm not sure how to use sc7180-venus.yaml >>> >>> instead. >> DTSI is not relevant here to the bindings. I don't understand the >> problem, so not sure what you are asking here about. > The opp-table parameters are different in devicetree. Can we also use > the same yaml file? Please look at existing bindings for other devices. Best regards, Krzysztof
On 11/25/2024 9:42 PM, Dmitry Baryshkov wrote: > On Mon, Nov 25, 2024 at 11:04:49AM +0530, Renjiang Han wrote: >> Add support for Qualcomm video acceleration hardware used for video >> stream decoding and encoding on QCOM QCS615. >> >> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> >> --- >> .../bindings/media/qcom,qcs615-venus.yaml | 182 +++++++++++++++++++++ >> 1 file changed, 182 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/media/qcom,qcs615-venus.yaml b/Documentation/devicetree/bindings/media/qcom,qcs615-venus.yaml >> new file mode 100644 >> index 0000000000000000000000000000000000000000..7a3a01ff06d8b62bc2424a0a24857c86c6865f89 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/qcom,qcs615-venus.yaml >> @@ -0,0 +1,182 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/qcom,qcs615-venus.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm QCS615 Venus video encode and decode accelerators >> + >> +maintainers: >> + - Stanimir Varbanov <stanimir.k.varbanov@gmail.com> >> + - Vikash Garodia <quic_vgarodia@quicinc.com> >> + >> +description: >> + The Venus IP is a video encode and decode accelerator present >> + on Qualcomm platforms >> + >> +allOf: >> + - $ref: qcom,venus-common.yaml# >> + >> +properties: >> + compatible: >> + const: qcom,qcs615-venus > > Please extend sc7180-venus.yaml instead. No need to duplicate > unnecessary things. Exactly, we should reuse the sc7180 bindings for this. Regards, Vikash > >> + >> + power-domains: >> + minItems: 2 >> + maxItems: 3 > > So, is it 2 or 3? You don't have legacy here, so you should know an > exact number. > >> + >> + power-domain-names: >> + minItems: 2 > > And this one also can go away. > >> + items: >> + - const: venus >> + - const: vcodec0 >> + - const: cx >> + >> + clocks: >> + maxItems: 5 >> + >> + clock-names: >> + items: >> + - const: core >> + - const: iface >> + - const: bus >> + - const: vcodec0_core >> + - const: vcodec0_bus >> + >> + iommus: >> + maxItems: 1 >> + >> + memory-region: >> + maxItems: 1 >> + >> + interconnects: >> + maxItems: 2 >> + >> + interconnect-names: >> + items: >> + - const: video-mem >> + - const: cpu-cfg >> + >> + operating-points-v2: true >> + >> + opp-table: >> + type: object >> + >> + video-decoder: >> + type: object >> + >> + additionalProperties: false >> + >> + properties: >> + compatible: >> + const: venus-decoder >> + >> + required: >> + - compatible >> + >> + video-encoder: >> + type: object >> + >> + additionalProperties: false >> + >> + properties: >> + compatible: >> + const: venus-encoder >> + >> + required: >> + - compatible >> + >> +required: >> + - compatible >> + - power-domain-names >> + - iommus >> + - video-decoder >> + - video-encoder >> + >> +unevaluatedProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/clock/qcom,qcs615-videocc.h> >> + #include <dt-bindings/interconnect/qcom,qcs615-rpmh.h> >> + #include <dt-bindings/power/qcom,rpmhpd.h> >> + >> + venus: video-codec@aa00000 { >> + compatible = "qcom,qcs615-venus"; >> + reg = <0xaa00000 0x100000>; >> + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>; >> + >> + clocks = <&videocc VIDEO_CC_VENUS_CTL_CORE_CLK>, >> + <&videocc VIDEO_CC_VENUS_AHB_CLK>, >> + <&videocc VIDEO_CC_VENUS_CTL_AXI_CLK>, >> + <&videocc VIDEO_CC_VCODEC0_CORE_CLK>, >> + <&videocc VIDEO_CC_VCODEC0_AXI_CLK>; >> + clock-names = "core", >> + "iface", >> + "bus", >> + "vcodec0_core", >> + "vcodec0_bus"; >> + >> + power-domains = <&videocc VENUS_GDSC>, >> + <&videocc VCODEC0_GDSC>, >> + <&rpmhpd RPMHPD_CX>; >> + power-domain-names = "venus", >> + "vcodec0", >> + "cx"; >> + >> + operating-points-v2 = <&venus_opp_table>; >> + >> + interconnects = <&mmss_noc MASTER_VIDEO_P0 0 >> + &mc_virt SLAVE_EBI1 0>, >> + <&gem_noc MASTER_APPSS_PROC 0 >> + &config_noc SLAVE_VENUS_CFG 0>; >> + interconnect-names = "video-mem", >> + "cpu-cfg"; >> + >> + iommus = <&apps_smmu 0xe40 0x20>; >> + >> + memory-region = <&pil_video_mem>; >> + >> + video-decoder { >> + compatible = "venus-decoder"; >> + }; >> + >> + video-encoder { >> + compatible = "venus-encoder"; >> + }; >> + >> + venus_opp_table: opp-table { >> + compatible = "operating-points-v2"; >> + >> + opp-133330000 { >> + opp-hz = /bits/ 64 <133330000>; >> + required-opps = <&rpmhpd_opp_low_svs>; >> + }; >> + >> + opp-240000000 { >> + opp-hz = /bits/ 64 <240000000>; >> + required-opps = <&rpmhpd_opp_svs>; >> + }; >> + >> + opp-300000000 { >> + opp-hz = /bits/ 64 <300000000>; >> + required-opps = <&rpmhpd_opp_svs_l1>; >> + }; >> + >> + opp-380000000 { >> + opp-hz = /bits/ 64 <380000000>; >> + required-opps = <&rpmhpd_opp_nom>; >> + }; >> + >> + opp-410000000 { >> + opp-hz = /bits/ 64 <410000000>; >> + required-opps = <&rpmhpd_opp_turbo>; >> + }; >> + >> + opp-460000000 { >> + opp-hz = /bits/ 64 <460000000>; >> + required-opps = <&rpmhpd_opp_turbo_l1>; >> + }; >> + }; >> + }; >> >> -- >> 2.34.1 >> >
On 25.11.2024 6:34 AM, Renjiang Han wrote: > Add venus node into devicetree for the qcs615 video. > > Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> > --- > arch/arm64/boot/dts/qcom/qcs615.dtsi | 86 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 86 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi > index 06deb5c499fe83f0eb20d7957ca14948de7aab34..18ad4da5ed194458aded424560f45a3a9f3163dc 100644 > --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi > +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi > @@ -394,6 +394,11 @@ smem_region: smem@86000000 { > no-map; > hwlocks = <&tcsr_mutex 3>; > }; > + > + pil_video_mem: pil-video@93400000 { > + reg = <0x0 0x93400000 0x0 0x500000>; > + no-map; > + }; > }; > > soc: soc@0 { > @@ -530,6 +535,87 @@ gem_noc: interconnect@9680000 { > qcom,bcm-voters = <&apps_bcm_voter>; > }; > > + venus: video-codec@aa00000 { > + compatible = "qcom,qcs615-venus"; > + reg = <0x0 0xaa00000 0x0 0x100000>; Please pad the address part to 8 hex digits with leading zeroes > + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>; > + > + clocks = <&videocc VIDEO_CC_VENUS_CTL_CORE_CLK>, > + <&videocc VIDEO_CC_VENUS_AHB_CLK>, > + <&videocc VIDEO_CC_VENUS_CTL_AXI_CLK>, > + <&videocc VIDEO_CC_VCODEC0_CORE_CLK>, > + <&videocc VIDEO_CC_VCODEC0_AXI_CLK>; > + clock-names = "core", > + "iface", > + "bus", > + "vcodec0_core", > + "vcodec0_bus"; > + > + power-domains = <&videocc VENUS_GDSC>, > + <&videocc VCODEC0_GDSC>, > + <&rpmhpd RPMHPD_CX>; > + power-domain-names = "venus", > + "vcodec0", > + "cx"; > + > + operating-points-v2 = <&venus_opp_table>; > + > + interconnects = <&mmss_noc MASTER_VIDEO_P0 0 QCOM_ICC_TAG_ALWAYS > + &mc_virt SLAVE_EBI1 0>, > + <&gem_noc MASTER_APPSS_PROC 0 > + &config_noc SLAVE_VENUS_CFG 0>; QCOM_ICC_TAG_ACTIVE_ONLY Konrad
On 25/11/2024 08:50, Renjiang Han (QUIC) wrote: > On Mon 11/25/2024 1:35 PM, Renjiang Han wrote: >> Add venus node into devicetree for the qcs615 video. > Forgot to add Reviewed-by, next version will add Reviewed-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> You'll need to drop the video-encoder and video-decoder nodes per Krzysztof's feeback. https://lore.kernel.org/linux-arm-msm/436145fd-d65f-44ec-b950-c434775187ca@kernel.org You could do that with this series: https://lore.kernel.org/linux-media/20241128-media-staging-24-11-25-rb3-hw-compat-string-v4-0-fd062b399374@linaro.org/T/#m55b26747af4692da928ec9b531c4288c4e45c4d2 And the following change to your resource structure. static const struct venus_resources qcs615_res = { + .dec_nodename = "video-decoder", + .enc_nodename = "video-encoder", }; --- bod
This series enables 4xx venus encode/decode on qcs615 for BU. Configure venus node in the devicetree, add resource data in the venus core. And binding node to the venus core with compatible. This DT change is dependent on [1]dts and [2] videocc dts, [3]videocc driver and [4]smmu. depends on: [1] https://lore.kernel.org/all/20241104-add_initial_support_for_qcs615-v5-0-9dde8d7b80b0@quicinc.com/ [2] https://lore.kernel.org/lkml/20241108-qcs615-mm-dt-nodes-v1-0-b2669cac0624@quicinc.com/ [3] https://lore.kernel.org/all/20241108-qcs615-mm-clockcontroller-v3-0-7d3b2d235fdf@quicinc.com/ [4] https://lore.kernel.org/all/20241105032107.9552-1-quic_qqzhou@quicinc.com/ Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com> --- Changes in v3: - 1. Remove the ‘|’ after 'description' in the qcom,qcs615-venus.yaml. - 2. Add a blank line before 'opp-table' in the qcom,qcs615-venus.yaml. - 3. Put ‘additionalProperties’ before ‘properties’ in the qcom,qcs615-venus.yaml. - 4. Update the subject of qcom,qcs615-venus.yaml patch. - Link to v2: https://lore.kernel.org/r/20241112-add-venus-for-qcs615-v2-0-e67947f957af@quicinc.com Changes in v2: - 1. The change-id of DT and driver are removed. - 2. Add qcom,qcs615-venus.yaml files to explain DT. - 3. The order of driver's commit and DT's commit is adjusted. Place the driver's commit before the DT's commit. - 4. Extends driver's commit message. - 5. Split DT's commit into two commits. Add the venus node to the qcs615.dtsi file. Then in the qcs615-ride.dts file enable the venus node. - 6. Modify alignment, sort, upper and lower case letters issue. - 7. Update cover letter message description. - Link to v1: https://lore.kernel.org/r/20241008-add_qcs615_video-v1-0-436ce07bfc63@quicinc.com --- Renjiang Han (4): dt-bindings: qcom,qcs615-venus: add support for video hardware media: venus: core: add qcs615 platform data arm64: dts: qcom: add venus node for the qcs615 arm64: dts: qcom: qcs615-ride: enable venus node .../bindings/media/qcom,qcs615-venus.yaml | 182 +++++++++++++++++++++ arch/arm64/boot/dts/qcom/qcs615-ride.dts | 4 + arch/arm64/boot/dts/qcom/qcs615.dtsi | 86 ++++++++++ drivers/media/platform/qcom/venus/core.c | 50 ++++++ 4 files changed, 322 insertions(+) --- base-commit: a39230ecf6b3057f5897bc4744a790070cfbe7a8 change-id: 20241112-add-venus-for-qcs615-686235e64fdd prerequisite-message-id: <20241104-add_initial_support_for_qcs615-v5-0-9dde8d7b80b0@quicinc.com> prerequisite-patch-id: 09782474af7eecf1013425fd34f9d2f082fb3616 prerequisite-patch-id: 04ca722967256efddc402b7bab94136a5174b0b9 prerequisite-patch-id: 82481c82a20345548e2cb292d3098ed51843b809 prerequisite-patch-id: 3bd8edd83297815fcb1b81fcd891d3c14908442f prerequisite-patch-id: fc1cfec4ecd56e669c161c4d2c3797fc0abff0ae prerequisite-message-id: <20241108-qcs615-mm-dt-nodes-v1-0-b2669cac0624@quicinc.com> prerequisite-patch-id: bcb1328b70868bb9c87c0e4c48e5c9d38853bc60 prerequisite-patch-id: 8844a4661902eb44406639a3b7344416a0c88ed9 prerequisite-message-id: <20241108-qcs615-mm-clockcontroller-v3-0-7d3b2d235fdf@quicinc.com> prerequisite-patch-id: 748a4e51bbedae9c6ebdbd642b2fd1badf958788 prerequisite-patch-id: 72a894a3b19fdbd431e1cec9397365bc5b27abfe prerequisite-patch-id: da2b7a74f1afd58833c6a9a4544a0e271720641f prerequisite-patch-id: 40b79fe0b9101f5db3bddad23551c1123572aee5 prerequisite-patch-id: cb93e5798f6bfe8cc3044c4ce973e3ae5f20dc6b prerequisite-patch-id: 13b0dbf97ac1865d241791afb4b46a28ca499523 prerequisite-patch-id: 807019bedabd47c04f7ac78e9461d0b5a6e9131b prerequisite-patch-id: 8e2e841401fefbd96d78dd4a7c47514058c83bf2 prerequisite-patch-id: 125bb8cb367109ba22cededf6e78754579e1ed03 prerequisite-patch-id: b3cc42570d5826a4704f7702e7b26af9a0fe57b0 prerequisite-patch-id: df8e2fdd997cbf6c0a107f1871ed9e2caaa97582 prerequisite-message-id: <20241105032107.9552-1-quic_qqzhou@quicinc.com> prerequisite-patch-id: aaa7214fe86fade46ae5c245e0a44625fae1bad3 prerequisite-patch-id: 4db9f55207af45c6b64fff4f8929648a7fb44669 prerequisite-patch-id: 89ce719a863bf5e909989877f15f82b51552e449 Best regards,