Re: [RFC PATCH 01/11] dt-bindings: media: qcom,sm8550-iris: Add vpu sub nodes

[Bryan O'Donoghue <bod@kernel.org>]

On 09/07/2026 13:35, Vikash Garodia wrote:
> VPU hardwares have a limitation where VPU streams are associated with

VPU hardware has

> dedicated addressable address range, as illustrated below
> 
>      +-----------------------------------------------------------+
>      | Stream A reserved region (600 MB)                         |
>      | 0x00000000 - 0x25800000                                   |
>      +-----------------------------------------------------------+
>      | Stream B reserved region (3.5 GB)                         |
>      | 0x00000000 - 0xe0000000                                   |
>      +-----------------------------------------------------------+
>      | Other reserved regions                                    |
>      +-----------------------------------------------------------+
> 
> Mapping a stream outside its expected range can cause unintended
> behavior, including device crashes, as reported at:
> https://gitlab.freedesktop.org/drm/msm/-/work_items/100
> 
> To address this limitation, the subset of stream/s are now represented as

To address this bug surely - to me this is a bugfix we are introducing a 
change to stop a crash that leads to a hard reset. Its not a limitation 
its a bug.

> sub nodes, so that they can be associated to the respective addressable
> range.

> The limitation could be exposed when running usecase like concurrent
> video sessions. 

"The bug is readily reproduced when running concurrent video sessions"

The binding have been validated with higher concurrent
> sessions across the SOCs supported under this schema.

The bindings have been

or

The binding has been

> 
> Co-developed-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> Signed-off-by: Vishnu Reddy <busanna.reddy@oss.qualcomm.com>
> Signed-off-by: Vikash Garodia <vikash.garodia@oss.qualcomm.com>
> ---
>   .../bindings/media/qcom,sm8550-iris.yaml           | 66 +++++++++++++++++++++-
>   1 file changed, 63 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> index 0400ca1bff05dcef6b742c3fbf77e38adca9f280..bf4d24ce90bd38666704274390b98be450f708c0 100644
> --- a/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> +++ b/Documentation/devicetree/bindings/media/qcom,sm8550-iris.yaml
> @@ -91,6 +91,51 @@ properties:
>     opp-table:
>       type: object
> 
> +  '#address-cells':
> +    const: 2
> +
> +  '#size-cells':
> +    const: 2
> +
> +  non-pixel:
> +    type: object
> +    description:
> +      Non pixel context bank is needed when video hardware have distinct iommus for non pixel
> +      buffers. Non pixel buffers are compressed and internal buffers.

Don't describe when it is needed - describe what it is and what it does.

> +    properties:
> +      iommus:
> +        maxItems: 1
> +      memory-region:
> +        maxItems: 1
> +    required:
> +      - iommus
> +      - memory-region
> +    additionalProperties: false
> +
> +  pixel:
> +    type: object
> +    description:
> +      Pixel context bank is needed when video hardware have distinct iommus for pixel buffers.
> +      Pixel buffers are uncompressed buffers.

Same comment on the necessity of the binding. You're in "justification" 
space here but should be in "information" space i.e. inform the reader 
what as opposed to justify to the reviewer why, which is how these two 
descriptions read.

> +    properties:
> +      iommus:
> +        maxItems: 1
> +    required:
> +      - iommus
> +    additionalProperties: false
> +
> +  firmware:
> +    type: object
> +    description:
> +      Firmware context bank represents the firmware processing domain of the VPU. Required to boot
> +      VPU when no hypervisor is present.

Yes, this is information not justification.

> +    properties:
> +      iommus:
> +        maxItems: 1
> +    required:
> +      - iommus
> +    additionalProperties: false
> +
>   required:
>     - compatible
>     - power-domain-names
> @@ -98,9 +143,15 @@ required:
>     - interconnect-names
>     - resets
>     - reset-names
> -  - iommus
>     - dma-coherent
> 
> +oneOf:
> +  - required:
> +      - iommus
> +  - required:
> +      - non-pixel
> +      - pixel
> +
>   allOf:
>     - if:
>         properties:
> @@ -177,12 +228,21 @@ examples:
>           resets = <&gcc GCC_VIDEO_AXI0_CLK_ARES>;
>           reset-names = "bus";
> 
> -        iommus = <&apps_smmu 0x1940 0x0000>,
> -                 <&apps_smmu 0x1947 0x0000>;
>           dma-coherent;
> 
>           operating-points-v2 = <&iris_opp_table>;
> 
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        iris_non_pixel: non-pixel {
> +            iommus = <&apps_smmu 0x1940 0x0000>;
> +            memory-region = <&iris_resv>;
> +        };
> +
> +        iris_pixel: pixel {
> +            iommus = <&apps_smmu 0x1947 0x0000>;
> +        };
> +
>           iris_opp_table: opp-table {
>               compatible = "operating-points-v2";
> 
> 
> --
> 2.34.1
> 

Personally I think this describes a real system behaviour - the question 
is, is it a fixed behaviour - i.e. we _always_ expect to see firmware 
operate this way - if so it should be represented in DT, if not it 
should be represented in a platform descriptor in the driver.

The SIDs are fixed and mean specific things.

It would be possible to encode the SIDs in platform code but, that would 
be unusual.

I think this is a good solution TBH.

---
bod