Date: Thu, 21 Dec 2023 15:18:56 +0100
From: Alex Bee <knaerzche@...il.com>
To: Hugues FRUCHET <hugues.fruchet@...s.st.com>
Cc: Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Marco Felsch <m.felsch@...gutronix.de>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Andrzej Pietrasiewicz <andrzej.p@...labora.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Benjamin Gaignard <benjamin.gaignard@...labora.com>,
 Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
 Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, linux-kernel@...r.kernel.org,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Heiko Stuebner <heiko@...ech.de>, Hans Verkuil <hverkuil@...all.nl>,
 Rob Herring <robh+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 linux-stm32@...md-mailman.stormreply.com,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 devicetree@...r.kernel.org, Maxime Coquelin <mcoquelin.stm32@...il.com>,
 linux-media@...r.kernel.org, Alexandre Torgue
 <alexandre.torgue@...s.st.com>,
 Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
 Adam Ford <aford173@...il.com>
Subject: Re: [PATCH v5 0/5] Add support for video hardware codec of
 STMicroelectronics STM32 SoC series

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

Powered by blists - more mailing lists