Re: [GIT PULL FOR v4.11] New st-delta driver

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

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