diff mbox series

[2/4] media: dt-binding: media: Document rk3588’s vepu121

Message ID 20240320173736.2720778-3-linkmauve@linkmauve.fr (mailing list archive)
State New, archived
Headers show
Series Enable JPEG encoding on rk3588 | expand

Commit Message

Link Mauve March 20, 2024, 5:37 p.m. UTC
This encoder-only device is present four times on this SoC, and should
support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
encoding).

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml          | 1 +
 1 file changed, 1 insertion(+)

Comments

Sebastian Reichel March 20, 2024, 8:16 p.m. UTC | #1
Hi,

On Wed, Mar 20, 2024 at 06:37:31PM +0100, Emmanuel Gil Peyrot wrote:
> This encoder-only device is present four times on this SoC, and should
> support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
> encoding).
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> index 9d90d8d0565a..947ad699cc5e 100644
> --- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      enum:
>        - rockchip,rk3568-vepu
> +      - rockchip,rk3588-vepu121

Looks like they are fully compatible. In that case it's better to
use a fallback compatible (i.e. like the iommu binding), which does
not need any drivers changes. So binding should be like this:

compatible:
  oneOf:
    - const: rockchip,rk3568-vepu
    - items:
      - enum:
          - rockchip,rk3588-vepu121
      - const: rockchip,rk3568-vepu

Then in DT (i.e. the following patch) you use

compatible = "rockchip,rk3588-vepu121", "rockchip,rk3568-vepu";

And patch 4/4 can be dropped.

Greetings,

-- Sebastian
Krzysztof Kozlowski March 21, 2024, 8:14 a.m. UTC | #2
On 20/03/2024 18:37, Emmanuel Gil Peyrot wrote:
> This encoder-only device is present four times on this SoC, and should
> support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
> encoding).
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml          | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> index 9d90d8d0565a..947ad699cc5e 100644
> --- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> @@ -17,6 +17,7 @@ properties:
>    compatible:
>      enum:
>        - rockchip,rk3568-vepu
> +      - rockchip,rk3588-vepu121

What is 121?

Best regards,
Krzysztof
Heiko Stübner March 21, 2024, 8:47 a.m. UTC | #3
Am Donnerstag, 21. März 2024, 09:14:51 CET schrieb Krzysztof Kozlowski:
> On 20/03/2024 18:37, Emmanuel Gil Peyrot wrote:
> > This encoder-only device is present four times on this SoC, and should
> > support everything the rk3568 vepu supports (so JPEG, H.264 and VP8
> > encoding).
> > 
> > Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> > ---
> >  .../devicetree/bindings/media/rockchip,rk3568-vepu.yaml          | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> > index 9d90d8d0565a..947ad699cc5e 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
> > @@ -17,6 +17,7 @@ properties:
> >    compatible:
> >      enum:
> >        - rockchip,rk3568-vepu
> > +      - rockchip,rk3588-vepu121
> 
> What is 121?

That is the strange naming of the ip block inside the soc.

I.e. the rk3588 TRM lists a number of different video encoders and decoders:
- VDPU121 is decoding h.263 and mpeg1,2,4
- VDPU381 is decoding h.265, h.264 and some more
- VDPU720 is decoding jpeg
- VDPU981 decodes AV1
- VEPU121 is the jpeg encoder above
- VEPU580 encodes h.264 and h.265

Each of those are separate IP blocks with their own io-memory, their own
interrupts and their own iommus, etc.


Heiko
Krzysztof Kozlowski March 21, 2024, 9:19 a.m. UTC | #4
On 21/03/2024 09:47, Heiko Stübner wrote:
>>>      enum:
>>>        - rockchip,rk3568-vepu
>>> +      - rockchip,rk3588-vepu121
>>
>> What is 121?
> 
> That is the strange naming of the ip block inside the soc.
> 
> I.e. the rk3588 TRM lists a number of different video encoders and decoders:
> - VDPU121 is decoding h.263 and mpeg1,2,4
> - VDPU381 is decoding h.265, h.264 and some more
> - VDPU720 is decoding jpeg
> - VDPU981 decodes AV1
> - VEPU121 is the jpeg encoder above
> - VEPU580 encodes h.264 and h.265
> 
> Each of those are separate IP blocks with their own io-memory, their own
> interrupts and their own iommus, etc.

Thanks for explanation. Short introduction in commit msg would be nice
(e.g. VEPU121, one of two VEPU encoders). OTOH, why not documenting all
of them? Bindings are supposed to be as complete as possible.

Best regards,
Krzysztof
Heiko Stübner March 21, 2024, 9:32 a.m. UTC | #5
Am Donnerstag, 21. März 2024, 10:19:54 CET schrieb Krzysztof Kozlowski:
> On 21/03/2024 09:47, Heiko Stübner wrote:
> >>>      enum:
> >>>        - rockchip,rk3568-vepu
> >>> +      - rockchip,rk3588-vepu121
> >>
> >> What is 121?
> > 
> > That is the strange naming of the ip block inside the soc.
> > 
> > I.e. the rk3588 TRM lists a number of different video encoders and decoders:
> > - VDPU121 is decoding h.263 and mpeg1,2,4
> > - VDPU381 is decoding h.265, h.264 and some more
> > - VDPU720 is decoding jpeg
> > - VDPU981 decodes AV1
> > - VEPU121 is the jpeg encoder above
> > - VEPU580 encodes h.264 and h.265
> > 
> > Each of those are separate IP blocks with their own io-memory, their own
> > interrupts and their own iommus, etc.
> 
> Thanks for explanation. Short introduction in commit msg would be nice
> (e.g. VEPU121, one of two VEPU encoders). OTOH, why not documenting all
> of them? Bindings are supposed to be as complete as possible.

We have a concurrent series for the vdpu121 running at
  https://lore.kernel.org/all/20240316071100.2419369-1-liujianfeng1994@gmail.com

I think not all of those encoders/decoders are based on the Hantro IP
or at least at the moment we don't know this yet.
Hence people adding compatibles for the blocks they have actually
managed to run on their hardware.

Bindings are supposed to be as complete as possible, but revising a wrong
binding later is very hard. And the whole media part is full of binary libraries
in the vendor kernel and has not the best documentation.

So I guess people are just cautious ;-)


Heiko
Nicolas Dufresne March 22, 2024, 2:57 p.m. UTC | #6
Le jeudi 21 mars 2024 à 10:32 +0100, Heiko Stübner a écrit :
> Am Donnerstag, 21. März 2024, 10:19:54 CET schrieb Krzysztof Kozlowski:
> > On 21/03/2024 09:47, Heiko Stübner wrote:
> > > > >      enum:
> > > > >        - rockchip,rk3568-vepu
> > > > > +      - rockchip,rk3588-vepu121
> > > > 
> > > > What is 121?
> > > 
> > > That is the strange naming of the ip block inside the soc.
> > > 
> > > I.e. the rk3588 TRM lists a number of different video encoders and decoders:
> > > - VDPU121 is decoding h.263 and mpeg1,2,4
> > > - VDPU381 is decoding h.265, h.264 and some more
> > > - VDPU720 is decoding jpeg
> > > - VDPU981 decodes AV1
> > > - VEPU121 is the jpeg encoder above
> > > - VEPU580 encodes h.264 and h.265
> > > 
> > > Each of those are separate IP blocks with their own io-memory, their own
> > > interrupts and their own iommus, etc.
> > 
> > Thanks for explanation. Short introduction in commit msg would be nice
> > (e.g. VEPU121, one of two VEPU encoders). OTOH, why not documenting all
> > of them? Bindings are supposed to be as complete as possible.
> 
> We have a concurrent series for the vdpu121 running at
>   https://lore.kernel.org/all/20240316071100.2419369-1-liujianfeng1994@gmail.com
> 
> I think not all of those encoders/decoders are based on the Hantro IP
> or at least at the moment we don't know this yet.
> Hence people adding compatibles for the blocks they have actually
> managed to run on their hardware.

Correct, on top of this legacy core, only VDPU981 (AV1 decode) is a chip from
Hantro / Verisilicon. Everything else looks like their own design and derived
from their original rkvdec codec pack. We didn't name this AV1 decoder after VSI
brand as we didn't have the proper version information available, but we suspect
that is variant of their VSI9000D cores. In short, we tried to avoid documenting
our speculation, even if we believe we are right.

> 
> Bindings are supposed to be as complete as possible, but revising a wrong
> binding later is very hard. And the whole media part is full of binary libraries
> in the vendor kernel and has not the best documentation.

I agree with this, but I must give to RK that despite the lack of documentation,
their CODEC software is fully open-source and blob free on this platform.

> 
> So I guess people are just cautious ;-)
> 

exactly!

Nicolas
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
index 9d90d8d0565a..947ad699cc5e 100644
--- a/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
+++ b/Documentation/devicetree/bindings/media/rockchip,rk3568-vepu.yaml
@@ -17,6 +17,7 @@  properties:
   compatible:
     enum:
       - rockchip,rk3568-vepu
+      - rockchip,rk3588-vepu121
 
   reg:
     maxItems: 1