[1/6] media: dt-bindings: Document SC8280XP/SM8350 Venus

  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(+)
  

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