| Message ID | 20230731-topic-8280_venus-v1-1-8c8bbe1983a5@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | SM8350 and SC8280XP venus support | expand |
On 04/08/2023 22:09, Konrad Dybcio wrote: > Both of these SoCs implement an IRIS2 block, with SC8280XP being able > to clock it a bit higher. > ... > + > + iommus: > + maxItems: 1 > + > + video-decoder: > + type: object > + > + properties: > + compatible: > + const: venus-decoder That's not how compatibles are constructed... missing vendor prefix, SoC or IP block name. > + > + required: > + - compatible > + > + additionalProperties: false Why do you need this child node? Child nodes without properties are usually useless. > + > + video-encoder: > + type: object > + > + properties: > + compatible: > + const: venus-encoder > + > + required: > + - compatible > + > + additionalProperties: false > + > +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,gcc-sm8350.h> > + #include <dt-bindings/clock/qcom,sm8350-videocc.h> > + #include <dt-bindings/interconnect/qcom,sm8350.h> > + #include <dt-bindings/power/qcom-rpmpd.h> > + > + venus: video-codec@aa00000 { > + compatible = "qcom,sm8350-venus"; > + reg = <0x0aa00000 0x100000>; > + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>; > + > + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, > + <&videocc VIDEO_CC_MVS0C_CLK>, > + <&videocc VIDEO_CC_MVS0_CLK>; > + clock-names = "iface", > + "core", > + "vcodec0_core"; > + > + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>; > + reset-names = "core"; > + > + power-domains = <&videocc MVS0C_GDSC>, > + <&videocc MVS0_GDSC>, > + <&rpmhpd SM8350_MX>; > + power-domain-names = "venus", > + "vcodec0", > + "mx"; > + > + interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>, > + <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>, > + <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>; > + interconnect-names = "cpu-cfg", > + "video-mem", > + "video-llcc"; > + > + operating-points-v2 = <&venus_opp_table>; > + iommus = <&apps_smmu 0x2100 0x400>; > + memory-region = <&pil_video_mem>; > + > + status = "disabled"; Drop status. > + > + video-decoder { Best regards, Krzysztof
On 5.08.2023 21:29, Krzysztof Kozlowski wrote: > On 04/08/2023 22:09, Konrad Dybcio wrote: >> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >> to clock it a bit higher. >> > > ... > >> + >> + iommus: >> + maxItems: 1 >> + >> + video-decoder: >> + type: object >> + >> + properties: >> + compatible: >> + const: venus-decoder > > That's not how compatibles are constructed... missing vendor prefix, SoC > or IP block name. > >> + >> + required: >> + - compatible >> + >> + additionalProperties: false > > Why do you need this child node? Child nodes without properties are > usually useless. For both comments: I aligned with what was there.. The driver abuses these compats to probe enc/dec submodules, even though every Venus implementation (to my knowledge) is implicitly enc/dec capable.. Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec devices from the venus core probe and get rid of this fake stuff? Konrad
On 07/08/2023 14:41, Konrad Dybcio wrote: > On 5.08.2023 21:29, Krzysztof Kozlowski wrote: >> On 04/08/2023 22:09, Konrad Dybcio wrote: >>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >>> to clock it a bit higher. >>> >> >> ... >> >>> + >>> + iommus: >>> + maxItems: 1 >>> + >>> + video-decoder: >>> + type: object >>> + >>> + properties: >>> + compatible: >>> + const: venus-decoder >> >> That's not how compatibles are constructed... missing vendor prefix, SoC >> or IP block name. >> >>> + >>> + required: >>> + - compatible >>> + >>> + additionalProperties: false >> >> Why do you need this child node? Child nodes without properties are >> usually useless. > For both comments: I aligned with what was there.. > > The driver abuses these compats to probe enc/dec submodules, even though > every Venus implementation (to my knowledge) is implicitly enc/dec capable.. Holy crap, I see... > > Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec > devices from the venus core probe and get rid of this fake stuff? Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there, so we actually could stay with these subnodes, just correct the compatibles to a list with correct prefixes: qcom,sc8280xp-venus-decoder + qcom,venus-decoder Best regards, Krzysztof
On 7.08.2023 16:04, Krzysztof Kozlowski wrote: > On 07/08/2023 14:41, Konrad Dybcio wrote: >> On 5.08.2023 21:29, Krzysztof Kozlowski wrote: >>> On 04/08/2023 22:09, Konrad Dybcio wrote: >>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >>>> to clock it a bit higher. >>>> >>> >>> ... >>> >>>> + >>>> + iommus: >>>> + maxItems: 1 >>>> + >>>> + video-decoder: >>>> + type: object >>>> + >>>> + properties: >>>> + compatible: >>>> + const: venus-decoder >>> >>> That's not how compatibles are constructed... missing vendor prefix, SoC >>> or IP block name. >>> >>>> + >>>> + required: >>>> + - compatible >>>> + >>>> + additionalProperties: false >>> >>> Why do you need this child node? Child nodes without properties are >>> usually useless. >> For both comments: I aligned with what was there.. >> >> The driver abuses these compats to probe enc/dec submodules, even though >> every Venus implementation (to my knowledge) is implicitly enc/dec capable.. > > Holy crap, I see... > >> >> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec >> devices from the venus core probe and get rid of this fake stuff? > > Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there, > so we actually could stay with these subnodes, just correct the > compatibles to a list with correct prefixes: > > qcom,sc8280xp-venus-decoder + qcom,venus-decoder Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not "v2 chip" or "v2 hardware") these were used to look up clocks but then they were moved to the root node. I am not quite sure if it makes sense to distinguish e.g. sc8280xp-venus-decoder within sc8280xp-venus.. Perhaps deprecating the "8916 way" (clocks under subnodes), adding some boilerplate to look up clocks/pds in both places and converting everybody to the "7180 way" way of doing things (clocks under venus), and then getting rid of venus encoder/decoder completely (by calling device creation from venus probe) would be better. WDYT? Konrad
On 07/08/2023 17:02, Konrad Dybcio wrote: > On 7.08.2023 16:04, Krzysztof Kozlowski wrote: >> On 07/08/2023 14:41, Konrad Dybcio wrote: >>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote: >>>> On 04/08/2023 22:09, Konrad Dybcio wrote: >>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >>>>> to clock it a bit higher. >>>>> >>>> >>>> ... >>>> >>>>> + >>>>> + iommus: >>>>> + maxItems: 1 >>>>> + >>>>> + video-decoder: >>>>> + type: object >>>>> + >>>>> + properties: >>>>> + compatible: >>>>> + const: venus-decoder >>>> >>>> That's not how compatibles are constructed... missing vendor prefix, SoC >>>> or IP block name. >>>> >>>>> + >>>>> + required: >>>>> + - compatible >>>>> + >>>>> + additionalProperties: false >>>> >>>> Why do you need this child node? Child nodes without properties are >>>> usually useless. >>> For both comments: I aligned with what was there.. >>> >>> The driver abuses these compats to probe enc/dec submodules, even though >>> every Venus implementation (to my knowledge) is implicitly enc/dec capable.. >> >> Holy crap, I see... >> >>> >>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec >>> devices from the venus core probe and get rid of this fake stuff? >> >> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there, >> so we actually could stay with these subnodes, just correct the >> compatibles to a list with correct prefixes: >> >> qcom,sc8280xp-venus-decoder + qcom,venus-decoder > Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not > "v2 chip" or "v2 hardware") these were used to look up clocks but > then they were moved to the root node. > > I am not quite sure if it makes sense to distinguish e.g. > sc8280xp-venus-decoder within sc8280xp-venus..> > Perhaps deprecating the "8916 way" (clocks under subnodes), adding > some boilerplate to look up clocks/pds in both places and converting > everybody to the "7180 way" way of doing things (clocks under venus), > and then getting rid of venus encoder/decoder completely (by calling > device creation from venus probe) would be better. WDYT? Yes, this makes more sense. I think this is controlled by "legacy_binding" variable (see drivers/media/platform/qcom/venus/pm_helpers.c). Once all bindings are converted to new one (clocks in main/parent node), then we can drop the children and instantiate devices as MFD. Best regards, Krzysztof
On 07/08/2023 16:02, Konrad Dybcio wrote: > On 7.08.2023 16:04, Krzysztof Kozlowski wrote: >> On 07/08/2023 14:41, Konrad Dybcio wrote: >>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote: >>>> On 04/08/2023 22:09, Konrad Dybcio wrote: >>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >>>>> to clock it a bit higher. >>>>> >>>> >>>> ... >>>> >>>>> + >>>>> + iommus: >>>>> + maxItems: 1 >>>>> + >>>>> + video-decoder: >>>>> + type: object >>>>> + >>>>> + properties: >>>>> + compatible: >>>>> + const: venus-decoder >>>> >>>> That's not how compatibles are constructed... missing vendor prefix, SoC >>>> or IP block name. >>>> >>>>> + >>>>> + required: >>>>> + - compatible >>>>> + >>>>> + additionalProperties: false >>>> >>>> Why do you need this child node? Child nodes without properties are >>>> usually useless. >>> For both comments: I aligned with what was there.. >>> >>> The driver abuses these compats to probe enc/dec submodules, even though >>> every Venus implementation (to my knowledge) is implicitly enc/dec capable.. >> >> Holy crap, I see... >> >>> >>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec >>> devices from the venus core probe and get rid of this fake stuff? >> >> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there, >> so we actually could stay with these subnodes, just correct the >> compatibles to a list with correct prefixes: >> >> qcom,sc8280xp-venus-decoder + qcom,venus-decoder > Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not > "v2 chip" or "v2 hardware") these were used to look up clocks but > then they were moved to the root node. > > I am not quite sure if it makes sense to distinguish e.g. > sc8280xp-venus-decoder within sc8280xp-venus.. > > Perhaps deprecating the "8916 way" (clocks under subnodes), adding > some boilerplate to look up clocks/pds in both places and converting > everybody to the "7180 way" way of doing things (clocks under venus), > and then getting rid of venus encoder/decoder completely (by calling > device creation from venus probe) would be better. WDYT? > > Konrad As I understand it though, for some classes of venus hardware - earlier, it was possible to have two encoders or two decoders and it really didn't - perhaps still doesn't matter which order they are declared in. That's the logic behind having a compat string that assigns either encoder or decoder to one of the logical blocks. You can have any mixture of - encoder - decoder - encoder - encoder - decoder - decoder - decoder - encoder - encoder - decoder I think it should *still* be the case - whether it is a practical reality or not, that any of those mapping can be selected and supported. --- bod
On 7.08.2023 20:44, Bryan O'Donoghue wrote: > On 07/08/2023 16:02, Konrad Dybcio wrote: >> On 7.08.2023 16:04, Krzysztof Kozlowski wrote: >>> On 07/08/2023 14:41, Konrad Dybcio wrote: >>>> On 5.08.2023 21:29, Krzysztof Kozlowski wrote: >>>>> On 04/08/2023 22:09, Konrad Dybcio wrote: >>>>>> Both of these SoCs implement an IRIS2 block, with SC8280XP being able >>>>>> to clock it a bit higher. >>>>>> >>>>> >>>>> ... >>>>> >>>>>> + >>>>>> + iommus: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + video-decoder: >>>>>> + type: object >>>>>> + >>>>>> + properties: >>>>>> + compatible: >>>>>> + const: venus-decoder >>>>> >>>>> That's not how compatibles are constructed... missing vendor prefix, SoC >>>>> or IP block name. >>>>> >>>>>> + >>>>>> + required: >>>>>> + - compatible >>>>>> + >>>>>> + additionalProperties: false >>>>> >>>>> Why do you need this child node? Child nodes without properties are >>>>> usually useless. >>>> For both comments: I aligned with what was there.. >>>> >>>> The driver abuses these compats to probe enc/dec submodules, even though >>>> every Venus implementation (to my knowledge) is implicitly enc/dec capable.. >>> >>> Holy crap, I see... >>> >>>> >>>> Perhaps a bigger clean-up is due. I guess I could just create the venc/vdec >>>> devices from the venus core probe and get rid of this fake stuff? >>> >>> Few devices (qcom,msm8996-venus.yaml, sdm660, sdm845) have clocks there, >>> so we actually could stay with these subnodes, just correct the >>> compatibles to a list with correct prefixes: >>> >>> qcom,sc8280xp-venus-decoder + qcom,venus-decoder >> Hm.. looks like pre-845-v2 (with the v2 being "v2 binding" and not >> "v2 chip" or "v2 hardware") these were used to look up clocks but >> then they were moved to the root node. >> >> I am not quite sure if it makes sense to distinguish e.g. >> sc8280xp-venus-decoder within sc8280xp-venus.. >> >> Perhaps deprecating the "8916 way" (clocks under subnodes), adding >> some boilerplate to look up clocks/pds in both places and converting >> everybody to the "7180 way" way of doing things (clocks under venus), >> and then getting rid of venus encoder/decoder completely (by calling >> device creation from venus probe) would be better. WDYT? >> >> Konrad > > As I understand it though, for some classes of venus hardware - earlier, it was possible to have two encoders or two decoders and it really didn't - perhaps still doesn't matter which order they are declared in. > > That's the logic behind having a compat string that assigns either encoder or decoder to one of the logical blocks. > > You can have any mixture of > - encoder > - decoder > > - encoder > - encoder > > - decoder > - decoder > > - decoder > - encoder > > - encoder > > - decoder > > I think it should *still* be the case - whether it is a practical reality or not, that any of those mapping can be selected and supported. That can be taken care of with match data. Konrad
On 07/08/2023 19:45, Konrad Dybcio wrote: > That can be taken care of with match data. > > Konrad Well perhaps. I'm just sticking my oar in, to elucidate. The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof. A functionality we want to maintain. --- bod
On 7.08.2023 20:49, Bryan O'Donoghue wrote: > On 07/08/2023 19:45, Konrad Dybcio wrote: >> That can be taken care of with match data. >> >> Konrad > > Well perhaps. > > I'm just sticking my oar in, to elucidate. > > The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof. > > A functionality we want to maintain. Surely something like a modparam would be more suitable here? Konrad
On 07/08/2023 19:55, Konrad Dybcio wrote: > On 7.08.2023 20:49, Bryan O'Donoghue wrote: >> On 07/08/2023 19:45, Konrad Dybcio wrote: >>> That can be taken care of with match data. >>> >>> Konrad >> >> Well perhaps. >> >> I'm just sticking my oar in, to elucidate. >> >> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof. >> >> A functionality we want to maintain. > Surely something like a modparam would be more suitable here? > > Konrad Hmm. Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned. If there's a cogent argument _still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility. Though my own €0.02 is that a module parameter is more of a PITA than a compat string. OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility. @stan probably has the right idea what to do. --- bod
On 7.08.2023 21:05, Bryan O'Donoghue wrote: > On 07/08/2023 19:55, Konrad Dybcio wrote: >> On 7.08.2023 20:49, Bryan O'Donoghue wrote: >>> On 07/08/2023 19:45, Konrad Dybcio wrote: >>>> That can be taken care of with match data. >>>> >>>> Konrad >>> >>> Well perhaps. >>> >>> I'm just sticking my oar in, to elucidate. >>> >>> The compat sub-nodes aren't just a random choice with no logic. They exist to select between what you assign the blocks to be, encoder, decoder or any admixture thereof. >>> >>> A functionality we want to maintain. >> Surely something like a modparam would be more suitable here? >> >> Konrad > > Hmm. > > Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned. > > If there's a cogent argument _still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility. > > Though my own €0.02 is that a module parameter is more of a PITA than a compat string. > > OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility. > > @stan probably has the right idea what to do. Actually.. Has anybody tested this, ever, with the mainline driver? Do we have anyone using this? Is anybody willing to maintain that, test for regressions and fix them in a reasonable amount of time? If we don't have at least 2x "yes" here, I don't think it makes sense to worry about it.. Konrad
On 09/08/2023 13:15, Konrad Dybcio wrote: >> Hmm. >> >> Well from earlier in the thread the question "why do we have these compat strings" is because we can have any combination of encoder/decoder assigned. >> >> If there's a cogent argument_still_ to be made to transition to some new way of assignment then fine so long as we don't break that basic flexibility. >> >> Though my own €0.02 is that a module parameter is more of a PITA than a compat string. >> >> OTOH I could make the argument, that the high probability is most people - probably all, just instantiate a single encoder and decoder and aren't aware of or using the inbuilt flexibility. >> >> @stan probably has the right idea what to do. > Actually.. > > Has anybody tested this, ever, with the mainline driver? I assume Stan has. > Do we have anyone using this? Can't say. > Is anybody willing to maintain that, test for regressions and > fix them in a reasonable amount of time? > > > If we don't have at least 2x "yes" here, I don't think it makes sense > to worry about it.. Hmm. We decide if we are encoding or decoding when we init a session and the blocks are symmetrical. The hw blocks themselves are not bound to a particular encode/decode mode. Having two parallel encoders or decoders is exactly the same effort as having a parallel encoder/decoder. We don't test parallel encoding/decoding but we should. I'd not be surprised to find there are bugs but, that's not a reason to exclude rather to find and fix bugs. --- bod
diff --git a/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml new file mode 100644 index 000000000000..8a31bce27c18 --- /dev/null +++ b/Documentation/devicetree/bindings/media/qcom,sm8350-venus.yaml @@ -0,0 +1,149 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/qcom,sm8350-venus.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm SM8350 Venus video encode and decode accelerators + +maintainers: + - Konrad Dybcio <konradybcio@kernel.org> + +description: | + The Venus Iris2 IP is a video encode and decode accelerator present + on Qualcomm platforms + +allOf: + - $ref: qcom,venus-common.yaml# + +properties: + compatible: + enum: + - qcom,sc8280xp-venus + - qcom,sm8350-venus + + clocks: + maxItems: 3 + + clock-names: + items: + - const: iface + - const: core + - const: vcodec0_core + + resets: + maxItems: 1 + + reset-names: + items: + - const: core + + power-domains: + maxItems: 3 + + power-domain-names: + items: + - const: venus + - const: vcodec0 + - const: mx + + interconnects: + maxItems: 3 + + interconnect-names: + items: + - const: cpu-cfg + - const: video-mem + - const: video-llcc + + operating-points-v2: true + opp-table: + type: object + + iommus: + maxItems: 1 + + video-decoder: + type: object + + properties: + compatible: + const: venus-decoder + + required: + - compatible + + additionalProperties: false + + video-encoder: + type: object + + properties: + compatible: + const: venus-encoder + + required: + - compatible + + additionalProperties: false + +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,gcc-sm8350.h> + #include <dt-bindings/clock/qcom,sm8350-videocc.h> + #include <dt-bindings/interconnect/qcom,sm8350.h> + #include <dt-bindings/power/qcom-rpmpd.h> + + venus: video-codec@aa00000 { + compatible = "qcom,sm8350-venus"; + reg = <0x0aa00000 0x100000>; + interrupts = <GIC_SPI 174 IRQ_TYPE_LEVEL_HIGH>; + + clocks = <&gcc GCC_VIDEO_AXI0_CLK>, + <&videocc VIDEO_CC_MVS0C_CLK>, + <&videocc VIDEO_CC_MVS0_CLK>; + clock-names = "iface", + "core", + "vcodec0_core"; + + resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>; + reset-names = "core"; + + power-domains = <&videocc MVS0C_GDSC>, + <&videocc MVS0_GDSC>, + <&rpmhpd SM8350_MX>; + power-domain-names = "venus", + "vcodec0", + "mx"; + + interconnects = <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_VENUS_CFG 0>, + <&mmss_noc MASTER_VIDEO_P0 0 &mc_virt SLAVE_EBI1 0>, + <&mmss_noc MASTER_VIDEO_P0 0 &gem_noc SLAVE_LLCC 0>; + interconnect-names = "cpu-cfg", + "video-mem", + "video-llcc"; + + operating-points-v2 = <&venus_opp_table>; + iommus = <&apps_smmu 0x2100 0x400>; + memory-region = <&pil_video_mem>; + + status = "disabled"; + + video-decoder { + compatible = "venus-decoder"; + }; + + video-encoder { + compatible = "venus-encoder"; + }; + };
Both of these SoCs implement an IRIS2 block, with SC8280XP being able to clock it a bit higher. Document it. Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- .../bindings/media/qcom,sm8350-venus.yaml | 149 +++++++++++++++++++++ 1 file changed, 149 insertions(+)