Received-SPF: pass (google.com: domain of linux-kernel+bounces-8563-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <5095f695-6dad-45e2-9cff-1b79003355c9@gmail.com>
Date: Thu, 21 Dec 2023 15:18:56 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 0/5] Add support for video hardware codec of
 STMicroelectronics STM32 SoC series
Content-Language: en-US, de-DE
To: Hugues FRUCHET <hugues.fruchet@foss.st.com>
Cc: Nicolas Dufresne <nicolas.dufresne@collabora.com>,
 Marco Felsch <m.felsch@pengutronix.de>,
 Philipp Zabel <p.zabel@pengutronix.de>,
 Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
 Sakari Ailus <sakari.ailus@linux.intel.com>,
 Benjamin Gaignard <benjamin.gaignard@collabora.com>,
 Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
 Benjamin Mugnier <benjamin.mugnier@foss.st.com>,
 Mauro Carvalho Chehab <mchehab@kernel.org>, linux-kernel@vger.kernel.org,
 Daniel Almeida <daniel.almeida@collabora.com>,
 Heiko Stuebner <heiko@sntech.de>, Hans Verkuil <hverkuil@xs4all.nl>,
 Rob Herring <robh+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>,
 linux-stm32@st-md-mailman.stormreply.com,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 devicetree@vger.kernel.org, Maxime Coquelin <mcoquelin.stm32@gmail.com>,
 linux-media@vger.kernel.org, Alexandre Torgue
 <alexandre.torgue@foss.st.com>,
 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
 Adam Ford <aford173@gmail.com>
References: <20231221084723.2152034-1-hugues.fruchet@foss.st.com>
 <769a1510-f8d2-4095-9879-42f413141dee@gmail.com>
 <a240d2ac-db0e-481b-8d13-3ae76cfd2fe7@foss.st.com>
 <e5ba1e14-4bbf-43e3-933a-fee6d4b90641@gmail.com>
 <CAHCN7xJ3Ktn+TnoOYdnNvKTddGCfLp4OQ+gM0WonWj-aqnsGuA@mail.gmail.com>
 <b03a7ddc-65c5-44c3-a563-d52ee938148a@gmail.com>
 <6d26d307-eb7a-43ad-b4f3-57f8ac7ce8f0@foss.st.com>
From: Alex Bee <knaerzche@gmail.com>
In-Reply-To: <6d26d307-eb7a-43ad-b4f3-57f8ac7ce8f0@foss.st.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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.