Re: [PATCH v5 0/5] Add support for video hardware codec of STMicroelectronics STM32 SoC series

[Alex Bee <knaerzche@gmail.com>]

Hi Hugues,

Am 21.12.23 um 14:55 schrieb Hugues FRUCHET:
> Hi Alex,
>
> On 12/21/23 14:46, Alex Bee wrote:
>>
>> Am 21.12.23 um 14:38 schrieb Adam Ford:
>>> On Thu, Dec 21, 2023 at 7:31 AM Alex Bee <knaerzche@gmail.com> wrote:
>>>> Hi Hugues,
>>>>
>>>> Am 21.12.23 um 14:08 schrieb Hugues FRUCHET:
>>>>> Hi Alex,
>>>>>
>>>>> This is because VDEC and VENC are two separated IPs with their own
>>>>> hardware resources and no links between both.
>>>>> On future SoCs, VDEC can ship on its own, same for VENC.
>>>>>
>>>> I think that's what the driver is/was designed for :)
>>>>
>>>> I don't  think there _has_ to be a link between variants in the 
>>>> same file.
>>>> For Rockchip we only had the issue that there _is_ a link (shared
>>>> resources) between encoder and decoder and they had (for that 
>>>> reason) to be
>>>> defined has a _single_ variant. And there is no reason you can ship 
>>>> decoder
>>>> and encoder seperated when you have two variants (with different
>>>> compatibles).
>>>> For Rockchip and iMX those files are even containing variants for 
>>>> completly
>>>> different generations / different SoCs. I had to cleanup this mess for
>>> The i.MX8M Mini and Plus have different power domains for encoder and
>>> decoders as well as different clocks.  Keeping them separate would
>>> almost be necessary.
>> I guess there is missunderstanding: I didn't say the two STM variants
>> should be merged in one variant, but the two variants should be 
>> within the
>> same _file_, like the other platforms are doing :)
>
> I have two separated hardware: VDEC and VENC, not a single block like 
> "VPU" for example. So what name should have this file ?
> Other platforms had a common file because there was a common block 
> embedding both decoder and encoder, sometimes with links/dependencies 
> between both.
> SAMA5D4 has only a decoder, only a single file called "_vdec_hw.c"...
> so it is quite logical for me to have one file per independent IP.
>
Maybe - but that's not way the driver is currently organzied.
rockchip_vpu_hw.c also holds variants which have only have a decoder and
also some which only have a encoder. So "vpu" is quite neutral, I guess. It
doesn't say anything if it belongs to encoder or decoder.
When I was adding the RK3066 variant a I was even asked to add a explicit
comment, why this integration can't be splitted in encoder and decoder
variant.

We were having a a lot of these split-ups in the early days of hantro
driver and it was no fun to clean them up.

Alex

>>> adam
>>>
>>>> Rockchip once - and it was no fun :) Anyways: It's up to the 
>>>> maintainers I
>>>> guess - I just wanted to ask if I missunderstand something here.
>>>>
>>>> Greetings,
>>>>
>>>> Alex
>>>>
>>>>> Hoping that this clarify.
>>>>>
>>>>> Best regards,
>>>>> Hugues.
>>>>>
>>>>> On 12/21/23 13:40, Alex Bee wrote:
>>>>>> Hi Hugues, Hi Nicolas,
>>>>>>
>>>>>> is there any specific reason I'm not understanding / seeing why this
>>>>>> is added in two seperate vdec* / venc* files and not a single vpu*
>>>>>> file? Is it only for the seperate clocks (-names) / irqs (-names) /
>>>>>> callbacks? Those are defined per variant and perfectly fit in a
>>>>>> single file holding one vdec and one venc variant.
>>>>>>
>>>>>> Alex
>>>>>>
>>>>>> Am 21.12.23 um 09:47 schrieb Hugues Fruchet:
>>>>>>> This patchset introduces support for VDEC video hardware decoder
>>>>>>> and VENC video hardware encoder of STMicroelectronics STM32MP25
>>>>>>> SoC series.
>>>>>>>
>>>>>>> This initial support implements H264 decoding, VP8 decoding and
>>>>>>> JPEG encoding.
>>>>>>>
>>>>>>> This has been tested on STM32MP257F-EV1 evaluation board.
>>>>>>>
>>>>>>> ===========
>>>>>>> = history =
>>>>>>> ===========
>>>>>>> version 5:
>>>>>>>      - Precise that video decoding as been successfully tested 
>>>>>>> up to
>>>>>>> full HD
>>>>>>>      - Add Nicolas Dufresne reviewed-by
>>>>>>>
>>>>>>> version 4:
>>>>>>>      - Fix comments from Nicolas about dropping encoder raw steps
>>>>>>>
>>>>>>> version 3:
>>>>>>>      - Fix remarks from Krzysztof Kozlowski:
>>>>>>>       - drop "items", we keep simple enum in such case
>>>>>>>       - drop second example - it is the same as the first
>>>>>>>      - Drop unused node labels as suggested by Conor Dooley
>>>>>>>      - Revisit min/max resolutions as suggested by Nicolas Dufresne
>>>>>>>
>>>>>>> version 2:
>>>>>>>      - Fix remarks from Krzysztof Kozlowski on v1:
>>>>>>>       - single video-codec binding for both VDEC/VENC
>>>>>>>       - get rid of "-names"
>>>>>>>       - use of generic node name "video-codec"
>>>>>>>
>>>>>>> version 1:
>>>>>>>     - Initial submission
>>>>>>>
>>>>>>> Hugues Fruchet (5):
>>>>>>>     dt-bindings: media: Document STM32MP25 VDEC & VENC video codecs
>>>>>>>     media: hantro: add support for STM32MP25 VDEC
>>>>>>>     media: hantro: add support for STM32MP25 VENC
>>>>>>>     arm64: dts: st: add video decoder support to stm32mp255
>>>>>>>     arm64: dts: st: add video encoder support to stm32mp255
>>>>>>>
>>>>>>>    .../media/st,stm32mp25-video-codec.yaml       |  50 ++++++++
>>>>>>>    arch/arm64/boot/dts/st/stm32mp251.dtsi        |  12 ++
>>>>>>>    arch/arm64/boot/dts/st/stm32mp255.dtsi        |  17 +++
>>>>>>>    drivers/media/platform/verisilicon/Kconfig    |  14 ++-
>>>>>>>    drivers/media/platform/verisilicon/Makefile   |   4 +
>>>>>>>    .../media/platform/verisilicon/hantro_drv.c   |   4 +
>>>>>>>    .../media/platform/verisilicon/hantro_hw.h    |   2 +
>>>>>>>    .../platform/verisilicon/stm32mp25_vdec_hw.c  |  92 
>>>>>>> ++++++++++++++
>>>>>>>    .../platform/verisilicon/stm32mp25_venc_hw.c  | 115
>>>>>>> ++++++++++++++++++
>>>>>>>    9 files changed, 307 insertions(+), 3 deletions(-)
>>>>>>>    create mode 100644
>>>>>>> Documentation/devicetree/bindings/media/st,stm32mp25-video-codec.yaml 
>>>>>>>
>>>>>>>    create mode 100644
>>>>>>> drivers/media/platform/verisilicon/stm32mp25_vdec_hw.c
>>>>>>>    create mode 100644
>>>>>>> drivers/media/platform/verisilicon/stm32mp25_venc_hw.c
>>>>>>>
>
> Best regards,
> Hugues.