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

[Hugues FRUCHET <hugues.fruchet@foss.st.com>]

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.

>> 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.