| Message ID | 20210305183924.1754026-8-emil.l.velikov@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] media: hantro: use G1_REG_INTERRUPT directly for the mpeg2 | expand |
Hi Ezequiel, Thanks for the prompt reply On Sat, 6 Mar 2021 at 11:25, Ezequiel Garcia <ezequiel@collabora.com> wrote: > > (adding another Nicolas) > > Hi Emil, > > Thanks a lot for the patch. > > On Fri, 2021-03-05 at 18:39 +0000, Emil Velikov wrote: > > From: Emil Velikov <emil.velikov@collabora.com> > > > > The SoC features a Hantro G1 compatible video decoder. > > > > Cc: Ezequiel Garcia <ezequiel@collabora.com> > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > Cc: linux-media@vger.kernel.org > > Cc: linux-rockchip@lists.infradead.org > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> > > --- > > arch/arm/boot/dts/sama5d4.dtsi | 9 ++ > > Usually device-tree changes go to their own patch. > > The new compatible string "atmel,sama5d4-vdec" needs > an update to the DT bindings, see Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml > for an example. > > DT bindings change should also go to a separate > patch, see Documentation/devicetree/bindings/submitting-patches.rst. > Will do. Thanks > > arch/arm/configs/sama5_defconfig | 3 + > > Better if config changes are a separate patch. > > But also, the driver is in staging and we haven't enabled > in ARM64 defconfig. Let's leave that decision to the machine > maintainer to decide. > Makes sense. Will keep it separate patch for completeness sake, with explicit note. ARM/Microchip (AT91) SoC maintainers will be in CC list and will defer the decision to them. > > +static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = { > > + { > > + .fourcc = V4L2_PIX_FMT_YUYV, > > + .codec_mode = HANTRO_MODE_NONE, > > + }, > > +}; > > + > > I haven't found information on how the series > was tested in the cover letter, could you add that to the next > iteration? > > Please test the YUYV post-processed output and MPEG-2 decoding as well. > Any recommendations for MPEG-2 and post-processing testing? For the former I could use gstreamer on Big Buck Bunny or other media, yet not sure about the latter. > Also add the fluster score on this platform, and while here you could > give a pass at v4l2-compliance, which should pass without failures. > Note that you need to use v4l2-compliance HEAD from git. > > https://git.linuxtv.org/v4l-utils.git > Ack, will do. Fwiw I did not see any results in the i.MX8M series so I followed suit :-P > > +static int sama5d4_hw_init(struct hantro_dev *vpu) > > +{ > > + return 0; > > Ah, the hantro_variant.init ops is not optional, but > if this VPU has no hw-specific init, then it should be. > > In any case, we might get rid of it soon: Benjamin's work > will hopefully remove the i.MX8M need for ctrl_base. > > And then the static clk_set_rate() in Rockchip variants could > be replaced with some dynamic rate using devfreq. > Neat looking forward to it. Thanks Emil
Hi Emil, So nice to see this support! Thank you so much for handling that. Little comments below... On 08/03/2021 at 14:07, Emil Velikov wrote: > Hi Ezequiel, > > Thanks for the prompt reply > > On Sat, 6 Mar 2021 at 11:25, Ezequiel Garcia <ezequiel@collabora.com> wrote: >> >> (adding another Nicolas) >> >> Hi Emil, >> >> Thanks a lot for the patch. >> >> On Fri, 2021-03-05 at 18:39 +0000, Emil Velikov wrote: >>> From: Emil Velikov <emil.velikov@collabora.com> >>> >>> The SoC features a Hantro G1 compatible video decoder. >>> >>> Cc: Ezequiel Garcia <ezequiel@collabora.com> >>> Cc: Philipp Zabel <p.zabel@pengutronix.de> >>> Cc: linux-media@vger.kernel.org >>> Cc: linux-rockchip@lists.infradead.org >>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com> >>> --- >>> arch/arm/boot/dts/sama5d4.dtsi | 9 ++ >> >> Usually device-tree changes go to their own patch. >> >> The new compatible string "atmel,sama5d4-vdec" needs Nitpicking: I would use "microchip,sama5d4-vdec". We tend to use the microchip name for new DT bidings and compatibility strings. >> an update to the DT bindings, see Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml >> for an example. >> >> DT bindings change should also go to a separate >> patch, see Documentation/devicetree/bindings/submitting-patches.rst. >> > Will do. Thanks > >>> arch/arm/configs/sama5_defconfig | 3 + >> >> Better if config changes are a separate patch. >> >> But also, the driver is in staging and we haven't enabled >> in ARM64 defconfig. Let's leave that decision to the machine >> maintainer to decide. >> > Makes sense. Will keep it separate patch for completeness sake, with > explicit note. > ARM/Microchip (AT91) SoC maintainers will be in CC list and will defer > the decision to them. I'm fine with having a "staging" component. Maybe add the hantro vdec as a module instead. Best regards, -- Nicolas Ferre
On Mon, 2021-03-08 at 13:07 +0000, Emil Velikov wrote: > Hi Ezequiel, > > Thanks for the prompt reply > > On Sat, 6 Mar 2021 at 11:25, Ezequiel Garcia <ezequiel@collabora.com> wrote: > > > > (adding another Nicolas) > > > > Hi Emil, > > > > Thanks a lot for the patch. > > > > On Fri, 2021-03-05 at 18:39 +0000, Emil Velikov wrote: > > > From: Emil Velikov <emil.velikov@collabora.com> > > > > > > The SoC features a Hantro G1 compatible video decoder. > > > > > > Cc: Ezequiel Garcia <ezequiel@collabora.com> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > > Cc: linux-media@vger.kernel.org > > > Cc: linux-rockchip@lists.infradead.org > > > Signed-off-by: Emil Velikov <emil.velikov@collabora.com> [..] > > > +static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = { > > > + { > > > + .fourcc = V4L2_PIX_FMT_YUYV, > > > + .codec_mode = HANTRO_MODE_NONE, > > > + }, > > > +}; > > > + > > > > I haven't found information on how the series > > was tested in the cover letter, could you add that to the next > > iteration? > > > > Please test the YUYV post-processed output and MPEG-2 decoding as well. > > > Any recommendations for MPEG-2 and post-processing testing? For the > former I could use gstreamer on Big Buck Bunny or other media, yet not > sure about the latter. > The post-processed YUYV output can be requested like this in GStreamer: gst-launch-1.0 -v filesrc location=$something ! parsebin ! decodebin ! video/x-raw,format=YUY2 ! ... For MPEG-2 testing, I'm afraid there's no GStreamer yet for this, but Jonas' ffmpeg should work https://github.com/Kwiboo/FFmpeg/commits/v4l2-request-hwaccel-4.3. Thanks, Ezequiel
On Mon, 8 Mar 2021 at 13:21, Nicolas Ferre <nicolas.ferre@microchip.com> wrote: > > Hi Emil, > Greetings Nicolas, > So nice to see this support! Thank you so much for handling that. > > Little comments below... > > Nitpicking: I would use "microchip,sama5d4-vdec". We tend to use the > microchip name for new DT bidings and compatibility strings. > Should i use Microchip (instead of Atmel) only for the DT bindings or throughout the series? > I'm fine with having a "staging" component. Maybe add the hantro vdec as > a module instead. > Ack, will do for v2. Thanks Emil
On 08/03/2021 at 16:57, Emil Velikov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, 8 Mar 2021 at 13:21, Nicolas Ferre <nicolas.ferre@microchip.com> wrote: >> >> Hi Emil, >> > Greetings Nicolas, > >> So nice to see this support! Thank you so much for handling that. >> >> Little comments below... >> > >> Nitpicking: I would use "microchip,sama5d4-vdec". We tend to use the >> microchip name for new DT bidings and compatibility strings. >> > Should i use Microchip (instead of Atmel) only for the DT bindings or > throughout the series? Yes, everywhere you can (Kconfig, explanation text, ...). Only keep Atmel/atmel where you cannot do differently or if it would require to modify code or move file just for this purpose. Regards, Nicolas >> I'm fine with having a "staging" component. Maybe add the hantro vdec as >> a module instead. >> > Ack, will do for v2. > > Thanks > Emil > -- Nicolas Ferre
diff --git a/arch/arm/boot/dts/sama5d4.dtsi b/arch/arm/boot/dts/sama5d4.dtsi index 05c55875835d..deaf1f6cc784 100644 --- a/arch/arm/boot/dts/sama5d4.dtsi +++ b/arch/arm/boot/dts/sama5d4.dtsi @@ -101,6 +101,15 @@ nfc_sram: sram@100000 { ranges = <0 0x100000 0x2400>; }; + vdec0: vdec@00300000 { + compatible = "atmel,sama5d4-vdec"; + reg = <0x00300000 0x100000>; + interrupts = <19 IRQ_TYPE_LEVEL_HIGH 4>; + interrupt-names = "vdec"; + clocks = <&pmc PMC_TYPE_PERIPHERAL 19>; + clock-names = "vdec_clk"; + }; + usb0: gadget@400000 { compatible = "atmel,sama5d3-udc"; reg = <0x00400000 0x100000 diff --git a/arch/arm/configs/sama5_defconfig b/arch/arm/configs/sama5_defconfig index 0dca50c64503..10806f38abfb 100644 --- a/arch/arm/configs/sama5_defconfig +++ b/arch/arm/configs/sama5_defconfig @@ -200,6 +200,9 @@ CONFIG_RTC_DRV_AT91RM9200=y CONFIG_DMADEVICES=y CONFIG_AT_HDMAC=y CONFIG_AT_XDMAC=y +CONFIG_STAGING=y +CONFIG_STAGING_MEDIA=y +CONFIG_VIDEO_HANTRO=y # CONFIG_IOMMU_SUPPORT is not set CONFIG_IIO=y CONFIG_AT91_ADC=y diff --git a/drivers/staging/media/hantro/Kconfig b/drivers/staging/media/hantro/Kconfig index 5b6cf9f62b1a..43762c8164e0 100644 --- a/drivers/staging/media/hantro/Kconfig +++ b/drivers/staging/media/hantro/Kconfig @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 config VIDEO_HANTRO tristate "Hantro VPU driver" - depends on ARCH_MXC || ARCH_ROCKCHIP || COMPILE_TEST + depends on ARCH_MXC || ARCH_ROCKCHIP || ARCH_AT91 || COMPILE_TEST depends on VIDEO_DEV && VIDEO_V4L2 select MEDIA_CONTROLLER select MEDIA_CONTROLLER_REQUEST_API @@ -24,6 +24,14 @@ config VIDEO_HANTRO_IMX8M help Enable support for i.MX8M SoCs. +config VIDEO_HANTRO_SAMA5D4 + bool "Hantro VDEC SAMA5D4 support" + depends on VIDEO_HANTRO + depends on ARCH_AT91 || COMPILE_TEST + default y + help + Enable support for Atmel SAMA5D4 SoCs. + config VIDEO_HANTRO_ROCKCHIP bool "Hantro VPU Rockchip support" depends on VIDEO_HANTRO diff --git a/drivers/staging/media/hantro/Makefile b/drivers/staging/media/hantro/Makefile index 3747a32799b2..f4b99901eeee 100644 --- a/drivers/staging/media/hantro/Makefile +++ b/drivers/staging/media/hantro/Makefile @@ -22,6 +22,9 @@ hantro-vpu-y += \ hantro-vpu-$(CONFIG_VIDEO_HANTRO_IMX8M) += \ imx8m_vpu_hw.o +hantro-vpu-$(CONFIG_VIDEO_HANTRO_SAMA5D4) += \ + sama5d4_vdec_hw.o + hantro-vpu-$(CONFIG_VIDEO_HANTRO_ROCKCHIP) += \ rk3288_vpu_hw.o \ rk3399_vpu_hw.o diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index e5f200e64993..19f1202574a2 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -478,6 +478,9 @@ static const struct of_device_id of_hantro_match[] = { #endif #ifdef CONFIG_VIDEO_HANTRO_IMX8M { .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, }, +#endif +#ifdef CONFIG_VIDEO_HANTRO_SAMA5D4 + { .compatible = "atmel,sama5d4-vdec", .data = &sama5d4_vdec_variant, }, #endif { /* sentinel */ } }; diff --git a/drivers/staging/media/hantro/hantro_hw.h b/drivers/staging/media/hantro/hantro_hw.h index 73c71bb2320c..4d39da1d1581 100644 --- a/drivers/staging/media/hantro/hantro_hw.h +++ b/drivers/staging/media/hantro/hantro_hw.h @@ -152,6 +152,7 @@ extern const struct hantro_variant rk3399_vpu_variant; extern const struct hantro_variant rk3328_vpu_variant; extern const struct hantro_variant rk3288_vpu_variant; extern const struct hantro_variant imx8mq_vpu_variant; +extern const struct hantro_variant sama5d4_vdec_variant; extern const struct hantro_postproc_regs hantro_g1_postproc_regs; diff --git a/drivers/staging/media/hantro/sama5d4_vdec_hw.c b/drivers/staging/media/hantro/sama5d4_vdec_hw.c new file mode 100644 index 000000000000..9cf1068d986b --- /dev/null +++ b/drivers/staging/media/hantro/sama5d4_vdec_hw.c @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Hantro VDEC driver + * + * Copyright (C) 2021 Collabora Ltd, Emil Velikov <emil.velikov@collabora.com> + */ + +#include "hantro.h" + + +/* + * Supported formats. + */ + +static const struct hantro_fmt sama5d4_vdec_postproc_fmts[] = { + { + .fourcc = V4L2_PIX_FMT_YUYV, + .codec_mode = HANTRO_MODE_NONE, + }, +}; + +static const struct hantro_fmt sama5d4_vdec_fmts[] = { + { + .fourcc = V4L2_PIX_FMT_NV12, + .codec_mode = HANTRO_MODE_NONE, + }, + { + .fourcc = V4L2_PIX_FMT_MPEG2_SLICE, + .codec_mode = HANTRO_MODE_MPEG2_DEC, + .max_depth = 2, + .frmsize = { + .min_width = 48, + .max_width = 1280, + .step_width = MB_DIM, + .min_height = 48, + .max_height = 720, + .step_height = MB_DIM, + }, + }, + { + .fourcc = V4L2_PIX_FMT_VP8_FRAME, + .codec_mode = HANTRO_MODE_VP8_DEC, + .max_depth = 2, + .frmsize = { + .min_width = 48, + .max_width = 1280, + .step_width = MB_DIM, + .min_height = 48, + .max_height = 720, + .step_height = MB_DIM, + }, + }, + { + .fourcc = V4L2_PIX_FMT_H264_SLICE, + .codec_mode = HANTRO_MODE_H264_DEC, + .max_depth = 2, + .frmsize = { + .min_width = 48, + .max_width = 1280, + .step_width = MB_DIM, + .min_height = 48, + .max_height = 720, + .step_height = MB_DIM, + }, + }, +}; + +static int sama5d4_hw_init(struct hantro_dev *vpu) +{ + return 0; +} + +/* + * Supported codec ops. + */ + +static const struct hantro_codec_ops sama5d4_vdec_codec_ops[] = { + [HANTRO_MODE_MPEG2_DEC] = { + .run = hantro_g1_mpeg2_dec_run, + .reset = hantro_g1_reset, + .init = hantro_mpeg2_dec_init, + .exit = hantro_mpeg2_dec_exit, + }, + [HANTRO_MODE_VP8_DEC] = { + .run = hantro_g1_vp8_dec_run, + .reset = hantro_g1_reset, + .init = hantro_vp8_dec_init, + .exit = hantro_vp8_dec_exit, + }, + [HANTRO_MODE_H264_DEC] = { + .run = hantro_g1_h264_dec_run, + .reset = hantro_g1_reset, + .init = hantro_h264_dec_init, + .exit = hantro_h264_dec_exit, + }, +}; + +static const struct hantro_irq sama5d4_irqs[] = { + { "vdec", hantro_g1_irq }, +}; + +static const char * const sama5d4_clk_names[] = { "vdec_clk" }; + +const struct hantro_variant sama5d4_vdec_variant = { + .dec_fmts = sama5d4_vdec_fmts, + .num_dec_fmts = ARRAY_SIZE(sama5d4_vdec_fmts), + .postproc_fmts = sama5d4_vdec_postproc_fmts, + .num_postproc_fmts = ARRAY_SIZE(sama5d4_vdec_postproc_fmts), + .postproc_regs = &hantro_g1_postproc_regs, + .codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER | + HANTRO_H264_DECODER, + .codec_ops = sama5d4_vdec_codec_ops, + .init = sama5d4_hw_init, + .irqs = sama5d4_irqs, + .num_irqs = ARRAY_SIZE(sama5d4_irqs), + .clk_names = sama5d4_clk_names, + .num_clocks = ARRAY_SIZE(sama5d4_clk_names), +};