Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Mon, 12 Oct 2020 13:58:51 +0900
Alexandre Courbot <acourbot@chromium.org> escreveu:

> Hi Mauro,
> 
> On Fri, Oct 9, 2020 at 3:34 PM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Fri, 9 Oct 2020 13:30:06 +0900
> > Alexandre Courbot <acourbot@chromium.org> escreveu:
> >  
> > > On Fri, Oct 9, 2020 at 1:13 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:  
> >  
> > > > >>> If VIDEO_MEDIATEK_VPU=y and MTK_SCP=m, then VIDEO_MEDIATEK_VCODEC can be configured
> > > > >>> to y, and then it won't be able to find the scp_ functions.
> > > > >>>
> > > > >>> To be honest, I'm not sure how to solve this.  
> > > > >>
> > > > >> Found it. Add this:
> > > > >>
> > > > >>         depends on MTK_SCP || !MTK_SCP
> > > > >>         depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> > > > >>
> > > > >> Ugly as hell, but it appears to be the correct incantation for this.  
> >
> > While the above does the job, I'm wondering if the better wouldn't
> > be to have this spit into 3 config dependencies. E. g. something like:
> >
> > config VIDEO_MEDIATEK_CODEC
> >         depends on VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU
> >
> > config VIDEO_MEDIATEK_VPU
> >         depends on VIDEO_DEV && VIDEO_V4L2
> >         depends on ARCH_MEDIATEK || COMPILE_TEST
> >         tristate "support for Mediatek Video Processor Unit without SCP"
> >         help
> >             ...
> >
> > config VIDEO_MEDIATEK_VPU_SCP
> >         depends on VIDEO_DEV && VIDEO_V4L2
> >         depends on ARCH_MEDIATEK || COMPILE_TEST
> >         tristate "support for Mediatek Video Processor Unit with SCP"
> >         help
> >             ...  
> 
> Doing so would introduce two extra choices to enable the driver, so
> I'm a bit concerned this may be a bit confusing?

The Kconfig name for "SCP" is already confusing:

	config MTK_SCP
		tristate "Mediatek SCP support"

Only looking at the helper messages one would understand what SCP
actually means ;-)

	help
	  Say y here to support Mediatek's System Companion Processor (SCP) via
	  the remote processor framework.

IMO, the way to make it less confusing would be to change the Kconfig
message (probably both here and at remoteproc) to make it easier for
people to understand.

For example, I would use something similar to this for MTK_SCP prompt:

	tristate "Use remoteproc with Mediatek companion processor (SCP)"

There would be other ways of producing the same result using multiple
config entries and just one that would be prompted, but, IMHO, with
multiple entries, it t is clearer for the user to understand what
what kind of support was selected. 

This also allows one to look at the produced .config in order to 
check if SCP was enabled for Mediatek VPU or not.

> Also I have experimented with this, and it appears that
> VIDEO_MEDIATEK_CODEC won't be automatically enabled if one of the new
> options is selected. So this means that after setting e.g.
> VIDEO_MEDIATEK_VPU_SCP, one still needs to manually enable
> VIDEO_MEDIATEK_CODEC otherwise the driver won't be compiled at all.

Actually, the codec config option would need a default line too,
e. g. either:

	config VIDEO_MEDIATEK_CODEC
	         depends on VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU
		 default y

or:

	config VIDEO_MEDIATEK_CODEC
	         depends on VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU
		 default VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU

Both should produce exactly the same result. I usually prefer the
first, as it is easier to read.

> 
> >
> > And split the board-specific data for each variant on separate files,
> > doing something like this at the Makefile:
> >
> >         obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> >                                        mtk-vcodec-enc.o \
> >                                        mtk-vcodec-common.o
> >
> >         ifneq ($(VIDEO_MEDIATEK_VPU_SCP),)
> >         obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-fw-scp.o
> >         endif
> >
> >         ifneq ($(VIDEO_MEDIATEK_VPU),)
> >         obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-fw-vpu.o
> >         endif
> >
> > This will avoid the ugly ifdefs in the middle of mtk_vcodec_fw.c,
> > and the ugly "depends on FOO || !FOO" usage.
> >
> > It should also be simpler to add future variants of it in the
> > future, if needed.  
> 
> Indeed, the split makes sense regardless of the selection mechanism
> adopted. I will try to do it in the next revision.

Agreed.

Thanks,
Mauro