* [GIT PULL FOR v4.11] New st-delta driver
@ 2017-01-09 13:23 Hans Verkuil
2017-01-30 19:15 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2017-01-09 13:23 UTC (permalink / raw)
To: Linux Media Mailing List
See the v4 series for details:
https://www.spinics.net/lists/linux-media/msg108737.html
Regards,
Hans
The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a:
[media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200)
are available in the git repository at:
git://linuxtv.org/hverkuil/media_tree.git delta
for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340:
st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100)
----------------------------------------------------------------
Hugues Fruchet (10):
Documentation: DT: add bindings for ST DELTA
ARM: dts: STiH410: add DELTA dt node
ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support
MAINTAINERS: add st-delta driver
st-delta: STiH4xx multi-format video decoder v4l2 driver
st-delta: add memory allocator helper functions
st-delta: rpmsg ipc support
st-delta: EOS (End Of Stream) support
st-delta: add mjpeg support
st-delta: debug: trace stream/frame information & summary
Documentation/devicetree/bindings/media/st,st-delta.txt | 17 +
MAINTAINERS | 8 +
arch/arm/boot/dts/stih410.dtsi | 10 +
arch/arm/configs/multi_v7_defconfig | 1 +
drivers/media/platform/Kconfig | 27 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/sti/delta/Makefile | 6 +
drivers/media/platform/sti/delta/delta-cfg.h | 63 ++
drivers/media/platform/sti/delta/delta-debug.c | 72 ++
drivers/media/platform/sti/delta/delta-debug.h | 18 +
drivers/media/platform/sti/delta/delta-ipc.c | 591 +++++++++++++
drivers/media/platform/sti/delta/delta-ipc.h | 76 ++
drivers/media/platform/sti/delta/delta-mem.c | 51 ++
drivers/media/platform/sti/delta/delta-mem.h | 14 +
drivers/media/platform/sti/delta/delta-mjpeg-dec.c | 454 ++++++++++
drivers/media/platform/sti/delta/delta-mjpeg-fw.h | 221 +++++
drivers/media/platform/sti/delta/delta-mjpeg-hdr.c | 150 ++++
drivers/media/platform/sti/delta/delta-mjpeg.h | 35 +
drivers/media/platform/sti/delta/delta-v4l2.c | 1977 +++++++++++++++++++++++++++++++++++++++++++
drivers/media/platform/sti/delta/delta.h | 566 +++++++++++++
20 files changed, 4359 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/st,st-delta.txt
create mode 100644 drivers/media/platform/sti/delta/Makefile
create mode 100644 drivers/media/platform/sti/delta/delta-cfg.h
create mode 100644 drivers/media/platform/sti/delta/delta-debug.c
create mode 100644 drivers/media/platform/sti/delta/delta-debug.h
create mode 100644 drivers/media/platform/sti/delta/delta-ipc.c
create mode 100644 drivers/media/platform/sti/delta/delta-ipc.h
create mode 100644 drivers/media/platform/sti/delta/delta-mem.c
create mode 100644 drivers/media/platform/sti/delta/delta-mem.h
create mode 100644 drivers/media/platform/sti/delta/delta-mjpeg-dec.c
create mode 100644 drivers/media/platform/sti/delta/delta-mjpeg-fw.h
create mode 100644 drivers/media/platform/sti/delta/delta-mjpeg-hdr.c
create mode 100644 drivers/media/platform/sti/delta/delta-mjpeg.h
create mode 100644 drivers/media/platform/sti/delta/delta-v4l2.c
create mode 100644 drivers/media/platform/sti/delta/delta.h
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [GIT PULL FOR v4.11] New st-delta driver 2017-01-09 13:23 [GIT PULL FOR v4.11] New st-delta driver Hans Verkuil @ 2017-01-30 19:15 ` Mauro Carvalho Chehab 2017-01-30 19:18 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 6+ messages in thread From: Mauro Carvalho Chehab @ 2017-01-30 19:15 UTC (permalink / raw) To: Hans Verkuil; +Cc: Linux Media Mailing List Em Mon, 9 Jan 2017 14:23:33 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > See the v4 series for details: > > https://www.spinics.net/lists/linux-media/msg108737.html > > Regards, > > Hans > > The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a: > > [media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200) > > are available in the git repository at: > > git://linuxtv.org/hverkuil/media_tree.git delta > > for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340: > > st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100) > > ---------------------------------------------------------------- > Hugues Fruchet (10): > Documentation: DT: add bindings for ST DELTA > ARM: dts: STiH410: add DELTA dt node > ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support > MAINTAINERS: add st-delta driver > st-delta: STiH4xx multi-format video decoder v4l2 driver > st-delta: add memory allocator helper functions > st-delta: rpmsg ipc support > st-delta: EOS (End Of Stream) support > st-delta: add mjpeg support > st-delta: debug: trace stream/frame information & summary There is something wrong on this driver... even after applying all patches, it complains that there's a for there that does nothing: drivers/media/platform/sti/delta/delta-v4l2.c:322 register_decoders() warn: we never enter this loop drivers/media/platform/sti/delta/delta-v4l2.c: In function 'register_decoders': drivers/media/platform/sti/delta/delta-v4l2.c:322:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] for (i = 0; i < ARRAY_SIZE(delta_decoders); i++) { ^ On a first glance, it seems that the register_decoders() function is reponsible to register the format decoders that the hardware recognizes. If so, I suspect that this driver is deadly broken. Please be sure that the upstream driver works properly before submitting it upstream. Also, please fix the comments to match the Kernel standard. E. g. instead of: /* guard output frame count: * - at least 1 frame needed for display * - at worst 21 * ( max h264 dpb (16) + * decoding peak smoothing (2) + * user display pipeline (3) ) */ It should be: /* * guard output frame count: * - at least 1 frame needed for display * - at worst 21 * ( max h264 dpb (16) + * decoding peak smoothing (2) + * user display pipeline (3) ) */ There are several similar occurrences among this patch series. Thanks, Mauro Thanks, Mauro ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL FOR v4.11] New st-delta driver 2017-01-30 19:15 ` Mauro Carvalho Chehab @ 2017-01-30 19:18 ` Mauro Carvalho Chehab 2017-01-31 15:16 ` Hugues FRUCHET 0 siblings, 1 reply; 6+ messages in thread From: Mauro Carvalho Chehab @ 2017-01-30 19:18 UTC (permalink / raw) To: Hans Verkuil, Hugues Fruchet Cc: Linux Media Mailing List, Benjamin Gaignard, Hugues Fruchet, Jean-Christophe Trotin Em Mon, 30 Jan 2017 17:15:36 -0200 Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu: > Em Mon, 9 Jan 2017 14:23:33 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > See the v4 series for details: > > > > https://www.spinics.net/lists/linux-media/msg108737.html > > > > Regards, > > > > Hans > > > > The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a: > > > > [media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200) > > > > are available in the git repository at: > > > > git://linuxtv.org/hverkuil/media_tree.git delta > > > > for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340: > > > > st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100) > > > > ---------------------------------------------------------------- > > Hugues Fruchet (10): > > Documentation: DT: add bindings for ST DELTA > > ARM: dts: STiH410: add DELTA dt node > > ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support > > MAINTAINERS: add st-delta driver > > st-delta: STiH4xx multi-format video decoder v4l2 driver > > st-delta: add memory allocator helper functions > > st-delta: rpmsg ipc support > > st-delta: EOS (End Of Stream) support > > st-delta: add mjpeg support > > st-delta: debug: trace stream/frame information & summary > > There is something wrong on this driver... even after applying all > patches, it complains that there's a for there that does nothing: > > drivers/media/platform/sti/delta/delta-v4l2.c:322 register_decoders() warn: we never enter this loop > drivers/media/platform/sti/delta/delta-v4l2.c: In function 'register_decoders': > drivers/media/platform/sti/delta/delta-v4l2.c:322:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > for (i = 0; i < ARRAY_SIZE(delta_decoders); i++) { > ^ > > On a first glance, it seems that the register_decoders() function is > reponsible to register the format decoders that the hardware > recognizes. If so, I suspect that this driver is deadly broken. > > Please be sure that the upstream driver works properly before > submitting it upstream. > > Also, please fix the comments to match the Kernel standard. E. g. > instead of: > > /* guard output frame count: > * - at least 1 frame needed for display > * - at worst 21 > * ( max h264 dpb (16) + > * decoding peak smoothing (2) + > * user display pipeline (3) ) > */ > > It should be: > > /* > * guard output frame count: > * - at least 1 frame needed for display > * - at worst 21 > * ( max h264 dpb (16) + > * decoding peak smoothing (2) + > * user display pipeline (3) ) > */ > > There are several similar occurrences among this patch series. Ah, forgot to comment, but it mentions a firmware. Does such firmware reside on some RAM memory? If so, how such firmware is loaded? > > Thanks, > Mauro > > Thanks, > Mauro Thanks, Mauro ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL FOR v4.11] New st-delta driver 2017-01-30 19:18 ` Mauro Carvalho Chehab @ 2017-01-31 15:16 ` Hugues FRUCHET 2017-02-01 10:39 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 6+ messages in thread From: Hugues FRUCHET @ 2017-01-31 15:16 UTC (permalink / raw) To: Mauro Carvalho Chehab, Hans Verkuil Cc: Linux Media Mailing List, Benjamin Gaignard, Jean Christophe TROTIN On 01/30/2017 08:18 PM, Mauro Carvalho Chehab wrote: > Em Mon, 30 Jan 2017 17:15:36 -0200 > Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu: > >> Em Mon, 9 Jan 2017 14:23:33 +0100 >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >> >>> See the v4 series for details: >>> >>> https://www.spinics.net/lists/linux-media/msg108737.html >>> >>> Regards, >>> >>> Hans >>> >>> The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a: >>> >>> [media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200) >>> >>> are available in the git repository at: >>> >>> git://linuxtv.org/hverkuil/media_tree.git delta >>> >>> for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340: >>> >>> st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100) >>> >>> ---------------------------------------------------------------- >>> Hugues Fruchet (10): >>> Documentation: DT: add bindings for ST DELTA >>> ARM: dts: STiH410: add DELTA dt node >>> ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support >>> MAINTAINERS: add st-delta driver >>> st-delta: STiH4xx multi-format video decoder v4l2 driver >>> st-delta: add memory allocator helper functions >>> st-delta: rpmsg ipc support >>> st-delta: EOS (End Of Stream) support >>> st-delta: add mjpeg support >>> st-delta: debug: trace stream/frame information & summary >> >> There is something wrong on this driver... even after applying all >> patches, it complains that there's a for there that does nothing: >> >> drivers/media/platform/sti/delta/delta-v4l2.c:322 register_decoders() warn: we never enter this loop >> drivers/media/platform/sti/delta/delta-v4l2.c: In function 'register_decoders': >> drivers/media/platform/sti/delta/delta-v4l2.c:322:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] >> for (i = 0; i < ARRAY_SIZE(delta_decoders); i++) { >> ^ Hi Mauro, It's strange that you face this warning, code is like that: /* registry of available decoders */ static const struct delta_dec *delta_decoders[] = { #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG &mjpegdec, #endif }; and MJPEG config is enabled by default: config VIDEO_STI_DELTA_MJPEG bool "STMicroelectronics DELTA MJPEG support" default y so you should not encounter this warning. On the other hand, you face issue on line 322 of delta-v4l2.c but in my codebase, and also in Hans' git tree (git://linuxtv.org/hverkuil/media_tree.git delta), this code is at line 323. Anyway, in order to prevent such warning even if no decoder are selected in config, I have reworked the code in v5 adding a "NULL" element at the end of decoder array out of any config switch: static const struct delta_dec *delta_decoders[] = { #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG &mjpegdec, #endif NULL, }; >> >> On a first glance, it seems that the register_decoders() function is >> reponsible to register the format decoders that the hardware >> recognizes. If so, I suspect that this driver is deadly broken. >> >> Please be sure that the upstream driver works properly before >> submitting it upstream. >> >> Also, please fix the comments to match the Kernel standard. E. g. >> instead of: >> >> /* guard output frame count: >> * - at least 1 frame needed for display >> * - at worst 21 >> * ( max h264 dpb (16) + >> * decoding peak smoothing (2) + >> * user display pipeline (3) ) >> */ >> >> It should be: >> >> /* >> * guard output frame count: >> * - at least 1 frame needed for display >> * - at worst 21 >> * ( max h264 dpb (16) + >> * decoding peak smoothing (2) + >> * user display pipeline (3) ) >> */ >> >> There are several similar occurrences among this patch series. I apologize for this -unfortunately not raised by checkpatch, I will have a look to fix it- Multiple lines comments are now fixed in v5. > > Ah, forgot to comment, but it mentions a firmware. Does such firmware > reside on some RAM memory? If so, how such firmware is loaded? Firmware is loaded in coprocessor at system startup by remoteproc framework: From "[GIT PULL] STi DT update for v4.11 round 1" https://lkml.org/lkml/2017/1/12/525: https://kernel.googlesource.com/pub/scm/linux/kernel/git/pchotard/sti/+/sti-dt-for-v4.11/arch/arm/boot/dts/stih407-family.dtsi st231_delta: remote-processor { compatible = "st,st231-rproc"; memory-region = <&delta_reserved>; resets = <&softreset STIH407_ST231_DMU_SOFTRESET>; reset-names = "sw_reset"; clocks = <&clk_s_c0_flexgen CLK_ST231_DMU>; clock-frequency = <600000000>; st,syscfg = <&syscfg_core 0x224>; #mbox-cells = <1>; mbox-names = "vq0_rx", "vq0_tx", "vq1_rx", "vq1_tx"; mboxes = <&mailbox0 0 0>, <&mailbox3 0 1>, <&mailbox0 0 1>, <&mailbox3 0 0>; }; > >> >> Thanks, >> Mauro >> >> Thanks, >> Mauro > > > > Thanks, > Mauro > Thanks for all, Hugues. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL FOR v4.11] New st-delta driver 2017-01-31 15:16 ` Hugues FRUCHET @ 2017-02-01 10:39 ` Mauro Carvalho Chehab 2017-02-01 15:33 ` Hugues FRUCHET 0 siblings, 1 reply; 6+ messages in thread From: Mauro Carvalho Chehab @ 2017-02-01 10:39 UTC (permalink / raw) To: Hugues FRUCHET Cc: Hans Verkuil, Linux Media Mailing List, Benjamin Gaignard, Jean Christophe TROTIN Em Tue, 31 Jan 2017 15:16:10 +0000 Hugues FRUCHET <hugues.fruchet@st.com> escreveu: > On 01/30/2017 08:18 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 30 Jan 2017 17:15:36 -0200 > > Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu: > > > >> Em Mon, 9 Jan 2017 14:23:33 +0100 > >> Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> > >>> See the v4 series for details: > >>> > >>> https://www.spinics.net/lists/linux-media/msg108737.html > >>> > >>> Regards, > >>> > >>> Hans > >>> > >>> The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a: > >>> > >>> [media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200) > >>> > >>> are available in the git repository at: > >>> > >>> git://linuxtv.org/hverkuil/media_tree.git delta > >>> > >>> for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340: > >>> > >>> st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100) > >>> > >>> ---------------------------------------------------------------- > >>> Hugues Fruchet (10): > >>> Documentation: DT: add bindings for ST DELTA > >>> ARM: dts: STiH410: add DELTA dt node > >>> ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support > >>> MAINTAINERS: add st-delta driver > >>> st-delta: STiH4xx multi-format video decoder v4l2 driver > >>> st-delta: add memory allocator helper functions > >>> st-delta: rpmsg ipc support > >>> st-delta: EOS (End Of Stream) support > >>> st-delta: add mjpeg support > >>> st-delta: debug: trace stream/frame information & summary > >> > >> There is something wrong on this driver... even after applying all > >> patches, it complains that there's a for there that does nothing: > >> > >> drivers/media/platform/sti/delta/delta-v4l2.c:322 register_decoders() warn: we never enter this loop > >> drivers/media/platform/sti/delta/delta-v4l2.c: In function 'register_decoders': > >> drivers/media/platform/sti/delta/delta-v4l2.c:322:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] > >> for (i = 0; i < ARRAY_SIZE(delta_decoders); i++) { > >> ^ > > Hi Mauro, > > It's strange that you face this warning, code is like that: > /* registry of available decoders */ > static const struct delta_dec *delta_decoders[] = { > #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG > &mjpegdec, > #endif > }; > > and MJPEG config is enabled by default: > config VIDEO_STI_DELTA_MJPEG > bool "STMicroelectronics DELTA MJPEG support" > default y > > so you should not encounter this warning. > > On the other hand, you face issue on line 322 of delta-v4l2.c but in my > codebase, and also in Hans' git tree > (git://linuxtv.org/hverkuil/media_tree.git delta), this code is at line 323. Well, here I compile everything patch per patch. > > Anyway, in order to prevent such warning even if no decoder are selected > in config, I have reworked the code in v5 adding a "NULL" > element at the end of decoder array out of any config switch: > static const struct delta_dec *delta_decoders[] = { > #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG > &mjpegdec, > #endif > NULL, > }; That just hides the warning. The real problem here is that, if someone compiles just the main driver with no decoder drivers, it will get an useless driver. It only makes sense to build VIDEO_STI_DELTA if at least one of the "daughter" drivers is built. Assuming that, on some future, you add a MPEG decoder, I guess the best way to address it would be to have something like this at the Kconfig: config VIDEO_STI_DELTA tristate "STMicroelectronics STiH4xx DELTA multi-format video decoder V4L2 driver options" depends on VIDEO_DEV && VIDEO_V4L2 depends on ARCH_STI || COMPILE_TEST depends on HAS_DMA help This V4L2 DELTA multi-format video decoder driver of STMicroelectronics STiH4xx SoC series allow hardware decoding of various compressed video bitstream format in raw uncompressed format. Use this option to see the decoders available for such hardware. Please notice that the driver will only be built if at least one of the delta codecs below is selected. if VIDEO_STI_DELTA config VIDEO_STI_DELTA_MJPEG bool "STMicroelectronics DELTA MJPEG support" help Enables the DELTA driver with MJPEG hardware support. To compile this driver as a module, choose M here: the module will be called st-delta. config VIDEO_STI_DELTA_MPEG bool "STMicroelectronics DELTA MPEG support" help Enables the DELTA driver with MPEG hardware support. To compile this driver as a module, choose M here: the module will be called st-delta. config VIDEO_STI_DELTA_DRIVER tristate depends on VIDEO_STI_DELTA depends on VIDEO_STI_DELTA_MJPEG | VIDEO_STI_DELTA_MPEG default VIDEO_STI_DELTA_MJPEG | VIDEO_STI_DELTA_MPEG select VIDEOBUF2_DMA_CONTIG select V4L2_MEM2MEM_DEV select RPMSG endif # VIDEO_STI_DELTA and change the sti/delta/Makefile to compile the delta driver using the VIDEO_STI_DELTA_DRIVER symbol: obj-$(CONFIG_VIDEO_STI_DELTA_DRIVER) := st-delta.o st-delta-y := delta-v4l2.o delta-mem.o delta-ipc.o delta-debug.o # MJPEG support st-delta-$(CONFIG_VIDEO_STI_DELTA_MJPEG) += delta-mjpeg-hdr.o st-delta-$(CONFIG_VIDEO_STI_DELTA_MJPEG) += delta-mjpeg-dec.o # MPEG support st-delta-$(CONFIG_VIDEO_STI_DELTA_MPEG) += delta-mpeg-hdr.o st-delta-$(CONFIG_VIDEO_STI_DELTA_MPEG) += delta-mpeg-dec.o > > >> > >> On a first glance, it seems that the register_decoders() function is > >> reponsible to register the format decoders that the hardware > >> recognizes. If so, I suspect that this driver is deadly broken. > >> > >> Please be sure that the upstream driver works properly before > >> submitting it upstream. > >> > >> Also, please fix the comments to match the Kernel standard. E. g. > >> instead of: > >> > >> /* guard output frame count: > >> * - at least 1 frame needed for display > >> * - at worst 21 > >> * ( max h264 dpb (16) + > >> * decoding peak smoothing (2) + > >> * user display pipeline (3) ) > >> */ > >> > >> It should be: > >> > >> /* > >> * guard output frame count: > >> * - at least 1 frame needed for display > >> * - at worst 21 > >> * ( max h264 dpb (16) + > >> * decoding peak smoothing (2) + > >> * user display pipeline (3) ) > >> */ > >> > >> There are several similar occurrences among this patch series. > > I apologize for this -unfortunately not raised by checkpatch, I will > have a look to fix it- > Multiple lines comments are now fixed in v5. You need to enable checkpatch in pedantic mode in order to see those warnings. > > > > > Ah, forgot to comment, but it mentions a firmware. Does such firmware > > reside on some RAM memory? If so, how such firmware is loaded? > > Firmware is loaded in coprocessor at system startup by remoteproc framework: > From "[GIT PULL] STi DT update for v4.11 round 1" > https://lkml.org/lkml/2017/1/12/525: > https://kernel.googlesource.com/pub/scm/linux/kernel/git/pchotard/sti/+/sti-dt-for-v4.11/arch/arm/boot/dts/stih407-family.dtsi > st231_delta: remote-processor { > compatible = "st,st231-rproc"; > memory-region = <&delta_reserved>; > resets = <&softreset STIH407_ST231_DMU_SOFTRESET>; > reset-names = "sw_reset"; > clocks = <&clk_s_c0_flexgen CLK_ST231_DMU>; > clock-frequency = <600000000>; > st,syscfg = <&syscfg_core 0x224>; > #mbox-cells = <1>; > mbox-names = "vq0_rx", "vq0_tx", "vq1_rx", "vq1_tx"; > mboxes = <&mailbox0 0 0>, <&mailbox3 0 1>, <&mailbox0 0 1>, > <&mailbox3 0 0>; > }; Ok. > > > > >> > >> Thanks, > >> Mauro > >> > >> Thanks, > >> Mauro > > > > > > > > Thanks, > > Mauro > > > > Thanks for all, > Hugues. Thanks, Mauro ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [GIT PULL FOR v4.11] New st-delta driver 2017-02-01 10:39 ` Mauro Carvalho Chehab @ 2017-02-01 15:33 ` Hugues FRUCHET 0 siblings, 0 replies; 6+ messages in thread From: Hugues FRUCHET @ 2017-02-01 15:33 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Hans Verkuil, Linux Media Mailing List, Benjamin Gaignard, Jean Christophe TROTIN On 02/01/2017 11:39 AM, Mauro Carvalho Chehab wrote: > Em Tue, 31 Jan 2017 15:16:10 +0000 > Hugues FRUCHET <hugues.fruchet@st.com> escreveu: > >> On 01/30/2017 08:18 PM, Mauro Carvalho Chehab wrote: >>> Em Mon, 30 Jan 2017 17:15:36 -0200 >>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu: >>> >>>> Em Mon, 9 Jan 2017 14:23:33 +0100 >>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu: >>>> >>>>> See the v4 series for details: >>>>> >>>>> https://www.spinics.net/lists/linux-media/msg108737.html >>>>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>> The following changes since commit 40eca140c404505c09773d1c6685d818cb55ab1a: >>>>> >>>>> [media] mn88473: add DVB-T2 PLP support (2016-12-27 14:00:15 -0200) >>>>> >>>>> are available in the git repository at: >>>>> >>>>> git://linuxtv.org/hverkuil/media_tree.git delta >>>>> >>>>> for you to fetch changes up to e6f199d01e7b8bc4436738b6c666fda31b9f3340: >>>>> >>>>> st-delta: debug: trace stream/frame information & summary (2017-01-09 14:16:45 +0100) >>>>> >>>>> ---------------------------------------------------------------- >>>>> Hugues Fruchet (10): >>>>> Documentation: DT: add bindings for ST DELTA >>>>> ARM: dts: STiH410: add DELTA dt node >>>>> ARM: multi_v7_defconfig: enable STMicroelectronics DELTA Support >>>>> MAINTAINERS: add st-delta driver >>>>> st-delta: STiH4xx multi-format video decoder v4l2 driver >>>>> st-delta: add memory allocator helper functions >>>>> st-delta: rpmsg ipc support >>>>> st-delta: EOS (End Of Stream) support >>>>> st-delta: add mjpeg support >>>>> st-delta: debug: trace stream/frame information & summary >>>> >>>> There is something wrong on this driver... even after applying all >>>> patches, it complains that there's a for there that does nothing: >>>> >>>> drivers/media/platform/sti/delta/delta-v4l2.c:322 register_decoders() warn: we never enter this loop >>>> drivers/media/platform/sti/delta/delta-v4l2.c: In function 'register_decoders': >>>> drivers/media/platform/sti/delta/delta-v4l2.c:322:16: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] >>>> for (i = 0; i < ARRAY_SIZE(delta_decoders); i++) { >>>> ^ >> >> Hi Mauro, >> >> It's strange that you face this warning, code is like that: >> /* registry of available decoders */ >> static const struct delta_dec *delta_decoders[] = { >> #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG >> &mjpegdec, >> #endif >> }; >> >> and MJPEG config is enabled by default: >> config VIDEO_STI_DELTA_MJPEG >> bool "STMicroelectronics DELTA MJPEG support" >> default y >> >> so you should not encounter this warning. >> >> On the other hand, you face issue on line 322 of delta-v4l2.c but in my >> codebase, and also in Hans' git tree >> (git://linuxtv.org/hverkuil/media_tree.git delta), this code is at line 323. > > Well, here I compile everything patch per patch. > >> >> Anyway, in order to prevent such warning even if no decoder are selected >> in config, I have reworked the code in v5 adding a "NULL" >> element at the end of decoder array out of any config switch: >> static const struct delta_dec *delta_decoders[] = { >> #ifdef CONFIG_VIDEO_STI_DELTA_MJPEG >> &mjpegdec, >> #endif >> NULL, >> }; > > That just hides the warning. The real problem here is that, if > someone compiles just the main driver with no decoder drivers, it > will get an useless driver. > > It only makes sense to build VIDEO_STI_DELTA if at least one of > the "daughter" drivers is built. > > Assuming that, on some future, you add a MPEG decoder, I guess the > best way to address it would be to have something like this at the > Kconfig: > > config VIDEO_STI_DELTA > tristate "STMicroelectronics STiH4xx DELTA multi-format video decoder V4L2 driver options" > depends on VIDEO_DEV && VIDEO_V4L2 > depends on ARCH_STI || COMPILE_TEST > depends on HAS_DMA > help > This V4L2 DELTA multi-format video decoder driver > of STMicroelectronics STiH4xx SoC series allow hardware > decoding of various compressed video bitstream format in > raw uncompressed format. > > Use this option to see the decoders available for > such hardware. > > Please notice that the driver will only be built if > at least one of the delta codecs below is selected. > > if VIDEO_STI_DELTA > > config VIDEO_STI_DELTA_MJPEG > bool "STMicroelectronics DELTA MJPEG support" > help > Enables the DELTA driver with MJPEG hardware support. > > To compile this driver as a module, choose M here: > the module will be called st-delta. > > config VIDEO_STI_DELTA_MPEG > bool "STMicroelectronics DELTA MPEG support" > help > Enables the DELTA driver with MPEG hardware support. > > To compile this driver as a module, choose M here: > the module will be called st-delta. > > config VIDEO_STI_DELTA_DRIVER > tristate > depends on VIDEO_STI_DELTA > depends on VIDEO_STI_DELTA_MJPEG | VIDEO_STI_DELTA_MPEG > default VIDEO_STI_DELTA_MJPEG | VIDEO_STI_DELTA_MPEG > select VIDEOBUF2_DMA_CONTIG > select V4L2_MEM2MEM_DEV > select RPMSG > > endif # VIDEO_STI_DELTA > > and change the sti/delta/Makefile to compile the delta driver using > the VIDEO_STI_DELTA_DRIVER symbol: > > obj-$(CONFIG_VIDEO_STI_DELTA_DRIVER) := st-delta.o > st-delta-y := delta-v4l2.o delta-mem.o delta-ipc.o delta-debug.o > > # MJPEG support > st-delta-$(CONFIG_VIDEO_STI_DELTA_MJPEG) += delta-mjpeg-hdr.o > st-delta-$(CONFIG_VIDEO_STI_DELTA_MJPEG) += delta-mjpeg-dec.o > > # MPEG support > st-delta-$(CONFIG_VIDEO_STI_DELTA_MPEG) += delta-mpeg-hdr.o > st-delta-$(CONFIG_VIDEO_STI_DELTA_MPEG) += delta-mpeg-dec.o > Hi Mauro, Thanks for the code, I have implemented it in v6 and reverted the v5 change around adding NULL in delta_decoders array. Many thanks, Hugues. >> >>>> >>>> On a first glance, it seems that the register_decoders() function is >>>> reponsible to register the format decoders that the hardware >>>> recognizes. If so, I suspect that this driver is deadly broken. >>>> >>>> Please be sure that the upstream driver works properly before >>>> submitting it upstream. >>>> >>>> Also, please fix the comments to match the Kernel standard. E. g. >>>> instead of: >>>> >>>> /* guard output frame count: >>>> * - at least 1 frame needed for display >>>> * - at worst 21 >>>> * ( max h264 dpb (16) + >>>> * decoding peak smoothing (2) + >>>> * user display pipeline (3) ) >>>> */ >>>> >>>> It should be: >>>> >>>> /* >>>> * guard output frame count: >>>> * - at least 1 frame needed for display >>>> * - at worst 21 >>>> * ( max h264 dpb (16) + >>>> * decoding peak smoothing (2) + >>>> * user display pipeline (3) ) >>>> */ >>>> >>>> There are several similar occurrences among this patch series. >> >> I apologize for this -unfortunately not raised by checkpatch, I will >> have a look to fix it- >> Multiple lines comments are now fixed in v5. > > You need to enable checkpatch in pedantic mode in order to see those > warnings. > >> >>> >>> Ah, forgot to comment, but it mentions a firmware. Does such firmware >>> reside on some RAM memory? If so, how such firmware is loaded? >> >> Firmware is loaded in coprocessor at system startup by remoteproc framework: >> From "[GIT PULL] STi DT update for v4.11 round 1" >> https://lkml.org/lkml/2017/1/12/525: >> https://kernel.googlesource.com/pub/scm/linux/kernel/git/pchotard/sti/+/sti-dt-for-v4.11/arch/arm/boot/dts/stih407-family.dtsi >> st231_delta: remote-processor { >> compatible = "st,st231-rproc"; >> memory-region = <&delta_reserved>; >> resets = <&softreset STIH407_ST231_DMU_SOFTRESET>; >> reset-names = "sw_reset"; >> clocks = <&clk_s_c0_flexgen CLK_ST231_DMU>; >> clock-frequency = <600000000>; >> st,syscfg = <&syscfg_core 0x224>; >> #mbox-cells = <1>; >> mbox-names = "vq0_rx", "vq0_tx", "vq1_rx", "vq1_tx"; >> mboxes = <&mailbox0 0 0>, <&mailbox3 0 1>, <&mailbox0 0 1>, >> <&mailbox3 0 0>; >> }; > > > Ok. > >> >>> >>>> >>>> Thanks, >>>> Mauro >>>> >>>> Thanks, >>>> Mauro >>> >>> >>> >>> Thanks, >>> Mauro >>> >> >> Thanks for all, >> Hugues. > > > Thanks, > Mauro > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-02-01 15:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-01-09 13:23 [GIT PULL FOR v4.11] New st-delta driver Hans Verkuil 2017-01-30 19:15 ` Mauro Carvalho Chehab 2017-01-30 19:18 ` Mauro Carvalho Chehab 2017-01-31 15:16 ` Hugues FRUCHET 2017-02-01 10:39 ` Mauro Carvalho Chehab 2017-02-01 15:33 ` Hugues FRUCHET
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox