Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 21 Dec 2023 15:18:56 +0100 | Subject | Re: [PATCH v5 0/5] Add support for video hardware codec of STMicroelectronics STM32 SoC series | From | Alex Bee <> |
| |
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.
| ![\](/images/icornerr.gif) |