diff mbox series

[v3,2/2] media: mtk-vcodec: fix build breakage when one of VPU or SCP is enabled

Message ID 20201012053557.4102148-3-acourbot@chromium.org (mailing list archive)
State New, archived
Headers show
Series media: mtk-vcodec: fix builds when remoteproc is disabled | expand

Commit Message

Alexandre Courbot Oct. 12, 2020, 5:35 a.m. UTC
The addition of MT8183 support added a dependency on the SCP remoteproc
module. However the initial patch used the "select" Kconfig directive,
which may result in the SCP module to not be compiled if remoteproc was
disabled. In such a case, mtk-vcodec would try to link against
non-existent SCP symbols. "select" was clearly misused here as explained
in kconfig-language.txt.

Replace this by a "depends" directive on at least one of the VPU and
SCP modules, to allow the driver to be compiled as long as one of these
is enabled, and adapt the code to support this new scenario.

Also adapt the Kconfig text to explain the extra requirements for MT8173
and MT8183.

Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/Kconfig                | 22 +++++++++++++++----
 drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
 .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
 3 files changed, 44 insertions(+), 6 deletions(-)

Comments

Mauro Carvalho Chehab Oct. 12, 2020, 7:43 a.m. UTC | #1
Em Mon, 12 Oct 2020 14:35:57 +0900
Alexandre Courbot <acourbot@chromium.org> escreveu:

> The addition of MT8183 support added a dependency on the SCP remoteproc
> module. However the initial patch used the "select" Kconfig directive,
> which may result in the SCP module to not be compiled if remoteproc was
> disabled. In such a case, mtk-vcodec would try to link against
> non-existent SCP symbols. "select" was clearly misused here as explained
> in kconfig-language.txt.
> 
> Replace this by a "depends" directive on at least one of the VPU and
> SCP modules, to allow the driver to be compiled as long as one of these
> is enabled, and adapt the code to support this new scenario.
> 
> Also adapt the Kconfig text to explain the extra requirements for MT8173
> and MT8183.
> 
> Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/Kconfig                | 22 +++++++++++++++----
>  drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
>  .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..457b6c39ddc0 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,18 +253,32 @@ config VIDEO_MEDIATEK_VCODEC
>  	depends on MTK_IOMMU || COMPILE_TEST
>  	depends on VIDEO_DEV && VIDEO_V4L2
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on VIDEO_MEDIATEK_VPU || MTK_SCP
> +	# The two following lines ensure we have the same state ("m" or "y") as
> +	# our dependencies, to avoid missing symbols during link.
> +	depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> +	depends on MTK_SCP || !MTK_SCP
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select VIDEO_MEDIATEK_VPU
> -	select MTK_SCP
> +	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> +	select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
>  	help
>  	    Mediatek video codec driver provides HW capability to
> -	    encode and decode in a range of video formats
> -	    This driver rely on VPU driver to communicate with VPU.
> +	    encode and decode in a range of video formats on MT8173
> +	    and MT8183.
> +
> +	    Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
> +	    also be selected. Support for MT8183 depends on MTK_SCP.
>  
>  	    To compile this driver as modules, choose M here: the
>  	    modules will be called mtk-vcodec-dec and mtk-vcodec-enc.

Just my 2 cents here, and to complement my last e-mail, the helper message
here IMO is a lot more confusing than if you do this, instead:

	config VIDEO_MEDIATEK_CODEC
	         depends on VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU
		 default y

	config VIDEO_MEDIATEK_VPU
            depends on VIDEO_DEV && VIDEO_V4L2
            depends on ARCH_MEDIATEK || COMPILE_TEST
            tristate "Enable Mediatek Video Processor Unit for MT8173"
	    help
		Select this option to enable Mediatek VPU on MT8173.

	config VIDEO_MEDIATEK_VPU_SCP
            depends on VIDEO_DEV && VIDEO_V4L2
            depends on ARCH_MEDIATEK || COMPILE_TEST
            tristate "Enable Mediatek Video Processor Unit for MT8183"
	    help
		Select this option to enable Mediatek VPU on MT8183.

To be clear, from my side, I can live with either one of the alternatives,
but, IMHO, the above is a lot clearer for anyone wanting to use
VPU, as, if MTK_SCP is disabled, the MT8183 Kconfig prompt will
disappear.


Thanks,
Mauro
Randy Dunlap Oct. 12, 2020, 2:59 p.m. UTC | #2
On 10/11/20 10:35 PM, Alexandre Courbot wrote:
> The addition of MT8183 support added a dependency on the SCP remoteproc
> module. However the initial patch used the "select" Kconfig directive,
> which may result in the SCP module to not be compiled if remoteproc was
> disabled. In such a case, mtk-vcodec would try to link against
> non-existent SCP symbols. "select" was clearly misused here as explained
> in kconfig-language.txt.
> 
> Replace this by a "depends" directive on at least one of the VPU and
> SCP modules, to allow the driver to be compiled as long as one of these
> is enabled, and adapt the code to support this new scenario.
> 
> Also adapt the Kconfig text to explain the extra requirements for MT8173
> and MT8183.
> 
> Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested

That Ack applied to v2. I have not tested nor acked this version of the patch.

> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/Kconfig                | 22 +++++++++++++++----
>  drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
>  .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..457b6c39ddc0 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,18 +253,32 @@ config VIDEO_MEDIATEK_VCODEC
>  	depends on MTK_IOMMU || COMPILE_TEST
>  	depends on VIDEO_DEV && VIDEO_V4L2
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on VIDEO_MEDIATEK_VPU || MTK_SCP
> +	# The two following lines ensure we have the same state ("m" or "y") as
> +	# our dependencies, to avoid missing symbols during link.
> +	depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> +	depends on MTK_SCP || !MTK_SCP
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select VIDEO_MEDIATEK_VPU
> -	select MTK_SCP
> +	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> +	select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
>  	help
>  	    Mediatek video codec driver provides HW capability to
> -	    encode and decode in a range of video formats
> -	    This driver rely on VPU driver to communicate with VPU.
> +	    encode and decode in a range of video formats on MT8173
> +	    and MT8183.
> +
> +	    Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
> +	    also be selected. Support for MT8183 depends on MTK_SCP.
>  
>  	    To compile this driver as modules, choose M here: the
>  	    modules will be called mtk-vcodec-dec and mtk-vcodec-enc.
>  
> +config VIDEO_MEDIATEK_VCODEC_VPU
> +	bool
> +
> +config VIDEO_MEDIATEK_VCODEC_SCP
> +	bool
> +
>  config VIDEO_MEM2MEM_DEINTERLACE
>  	tristate "Deinterlace support"
>  	depends on VIDEO_DEV && VIDEO_V4L2
> diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
> index 6e1ea3a9f052..4618d43dbbc8 100644
> --- a/drivers/media/platform/mtk-vcodec/Makefile
> +++ b/drivers/media/platform/mtk-vcodec/Makefile
> @@ -25,5 +25,11 @@ mtk-vcodec-enc-y := venc/venc_vp8_if.o \
>  mtk-vcodec-common-y := mtk_vcodec_intr.o \
>  		mtk_vcodec_util.o \
>  		mtk_vcodec_fw.o \
> -		mtk_vcodec_fw_vpu.o \
> -		mtk_vcodec_fw_scp.o
> +
> +ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU),)
> +mtk-vcodec-common-y += mtk_vcodec_fw_vpu.o
> +endif
> +
> +ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP),)
> +mtk-vcodec-common-y += mtk_vcodec_fw_scp.o
> +endif
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> index 51f1694a7c7d..b41e66185cec 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> @@ -27,8 +27,26 @@ struct mtk_vcodec_fw_ops {
>  	void (*release)(struct mtk_vcodec_fw *fw);
>  };
>  
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU)
>  struct mtk_vcodec_fw *mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
>  					     enum mtk_vcodec_fw_use fw_use);
> +#else
> +static inline struct mtk_vcodec_fw *
> +mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
> +		       enum mtk_vcodec_fw_use fw_use)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_VPU */
> +
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP)
>  struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev);
> +#else
> +static inline struct mtk_vcodec_fw *
> +mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_SCP */
>  
>  #endif /* _MTK_VCODEC_FW_PRIV_H_ */
>
Randy Dunlap Oct. 12, 2020, 5:57 p.m. UTC | #3
Hi,

For the Kconfig entries, specifically the help text:


On 10/11/20 10:35 PM, Alexandre Courbot wrote:
> ---
>  drivers/media/platform/Kconfig                | 22 +++++++++++++++----
>  drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
>  .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
>  3 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..457b6c39ddc0 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,18 +253,32 @@ config VIDEO_MEDIATEK_VCODEC
>  	depends on MTK_IOMMU || COMPILE_TEST
>  	depends on VIDEO_DEV && VIDEO_V4L2
>  	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	depends on VIDEO_MEDIATEK_VPU || MTK_SCP
> +	# The two following lines ensure we have the same state ("m" or "y") as
> +	# our dependencies, to avoid missing symbols during link.
> +	depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> +	depends on MTK_SCP || !MTK_SCP
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select VIDEO_MEDIATEK_VPU
> -	select MTK_SCP
> +	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> +	select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
>  	help
>  	    Mediatek video codec driver provides HW capability to
> -	    encode and decode in a range of video formats
> -	    This driver rely on VPU driver to communicate with VPU.
> +	    encode and decode in a range of video formats on MT8173
> +	    and MT8183.
> +
> +	    Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
> +	    also be selected. Support for MT8183 depends on MTK_SCP.
>  
>  	    To compile this driver as modules, choose M here: the
>  	    modules will be called mtk-vcodec-dec and mtk-vcodec-enc.

Please follow coding-style for Kconfig files:

from Documentation/process/coding-style.rst, section 10):

For all of the Kconfig* configuration files throughout the source tree,
the indentation is somewhat different.  Lines under a ``config`` definition
are indented with one tab, while help text is indented an additional two
spaces.


thanks.
Alexandre Courbot Oct. 13, 2020, 12:57 a.m. UTC | #4
On Tue, Oct 13, 2020 at 12:00 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 10/11/20 10:35 PM, Alexandre Courbot wrote:
> > The addition of MT8183 support added a dependency on the SCP remoteproc
> > module. However the initial patch used the "select" Kconfig directive,
> > which may result in the SCP module to not be compiled if remoteproc was
> > disabled. In such a case, mtk-vcodec would try to link against
> > non-existent SCP symbols. "select" was clearly misused here as explained
> > in kconfig-language.txt.
> >
> > Replace this by a "depends" directive on at least one of the VPU and
> > SCP modules, to allow the driver to be compiled as long as one of these
> > is enabled, and adapt the code to support this new scenario.
> >
> > Also adapt the Kconfig text to explain the extra requirements for MT8173
> > and MT8183.
> >
> > Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
>
> That Ack applied to v2. I have not tested nor acked this version of the patch.

Sorry about that - I was careless and left it in the log.

>
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/platform/Kconfig                | 22 +++++++++++++++----
> >  drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
> >  .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
> >  3 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index a3cb104956d5..457b6c39ddc0 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -253,18 +253,32 @@ config VIDEO_MEDIATEK_VCODEC
> >       depends on MTK_IOMMU || COMPILE_TEST
> >       depends on VIDEO_DEV && VIDEO_V4L2
> >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > +     depends on VIDEO_MEDIATEK_VPU || MTK_SCP
> > +     # The two following lines ensure we have the same state ("m" or "y") as
> > +     # our dependencies, to avoid missing symbols during link.
> > +     depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> > +     depends on MTK_SCP || !MTK_SCP
> >       select VIDEOBUF2_DMA_CONTIG
> >       select V4L2_MEM2MEM_DEV
> > -     select VIDEO_MEDIATEK_VPU
> > -     select MTK_SCP
> > +     select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> > +     select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
> >       help
> >           Mediatek video codec driver provides HW capability to
> > -         encode and decode in a range of video formats
> > -         This driver rely on VPU driver to communicate with VPU.
> > +         encode and decode in a range of video formats on MT8173
> > +         and MT8183.
> > +
> > +         Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
> > +         also be selected. Support for MT8183 depends on MTK_SCP.
> >
> >           To compile this driver as modules, choose M here: the
> >           modules will be called mtk-vcodec-dec and mtk-vcodec-enc.
> >
> > +config VIDEO_MEDIATEK_VCODEC_VPU
> > +     bool
> > +
> > +config VIDEO_MEDIATEK_VCODEC_SCP
> > +     bool
> > +
> >  config VIDEO_MEM2MEM_DEINTERLACE
> >       tristate "Deinterlace support"
> >       depends on VIDEO_DEV && VIDEO_V4L2
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
> > index 6e1ea3a9f052..4618d43dbbc8 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -25,5 +25,11 @@ mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> >  mtk-vcodec-common-y := mtk_vcodec_intr.o \
> >               mtk_vcodec_util.o \
> >               mtk_vcodec_fw.o \
> > -             mtk_vcodec_fw_vpu.o \
> > -             mtk_vcodec_fw_scp.o
> > +
> > +ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU),)
> > +mtk-vcodec-common-y += mtk_vcodec_fw_vpu.o
> > +endif
> > +
> > +ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP),)
> > +mtk-vcodec-common-y += mtk_vcodec_fw_scp.o
> > +endif
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> > index 51f1694a7c7d..b41e66185cec 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
> > @@ -27,8 +27,26 @@ struct mtk_vcodec_fw_ops {
> >       void (*release)(struct mtk_vcodec_fw *fw);
> >  };
> >
> > +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU)
> >  struct mtk_vcodec_fw *mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
> >                                            enum mtk_vcodec_fw_use fw_use);
> > +#else
> > +static inline struct mtk_vcodec_fw *
> > +mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
> > +                    enum mtk_vcodec_fw_use fw_use)
> > +{
> > +     return ERR_PTR(-ENODEV);
> > +}
> > +#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_VPU */
> > +
> > +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP)
> >  struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev);
> > +#else
> > +static inline struct mtk_vcodec_fw *
> > +mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev)
> > +{
> > +     return ERR_PTR(-ENODEV);
> > +}
> > +#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_SCP */
> >
> >  #endif /* _MTK_VCODEC_FW_PRIV_H_ */
> >
>
>
> --
> ~Randy
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
Alexandre Courbot Oct. 13, 2020, 12:38 p.m. UTC | #5
Hi Mauro,

On Mon, Oct 12, 2020 at 4:43 PM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Mon, 12 Oct 2020 14:35:57 +0900
> Alexandre Courbot <acourbot@chromium.org> escreveu:
>
> > The addition of MT8183 support added a dependency on the SCP remoteproc
> > module. However the initial patch used the "select" Kconfig directive,
> > which may result in the SCP module to not be compiled if remoteproc was
> > disabled. In such a case, mtk-vcodec would try to link against
> > non-existent SCP symbols. "select" was clearly misused here as explained
> > in kconfig-language.txt.
> >
> > Replace this by a "depends" directive on at least one of the VPU and
> > SCP modules, to allow the driver to be compiled as long as one of these
> > is enabled, and adapt the code to support this new scenario.
> >
> > Also adapt the Kconfig text to explain the extra requirements for MT8173
> > and MT8183.
> >
> > Reported-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> > Acked-by: Randy Dunlap <rdunlap@infradead.org> # build-tested
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/platform/Kconfig                | 22 +++++++++++++++----
> >  drivers/media/platform/mtk-vcodec/Makefile    | 10 +++++++--
> >  .../platform/mtk-vcodec/mtk_vcodec_fw_priv.h  | 18 +++++++++++++++
> >  3 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index a3cb104956d5..457b6c39ddc0 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -253,18 +253,32 @@ config VIDEO_MEDIATEK_VCODEC
> >       depends on MTK_IOMMU || COMPILE_TEST
> >       depends on VIDEO_DEV && VIDEO_V4L2
> >       depends on ARCH_MEDIATEK || COMPILE_TEST
> > +     depends on VIDEO_MEDIATEK_VPU || MTK_SCP
> > +     # The two following lines ensure we have the same state ("m" or "y") as
> > +     # our dependencies, to avoid missing symbols during link.
> > +     depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
> > +     depends on MTK_SCP || !MTK_SCP
> >       select VIDEOBUF2_DMA_CONTIG
> >       select V4L2_MEM2MEM_DEV
> > -     select VIDEO_MEDIATEK_VPU
> > -     select MTK_SCP
> > +     select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
> > +     select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
> >       help
> >           Mediatek video codec driver provides HW capability to
> > -         encode and decode in a range of video formats
> > -         This driver rely on VPU driver to communicate with VPU.
> > +         encode and decode in a range of video formats on MT8173
> > +         and MT8183.
> > +
> > +         Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
> > +         also be selected. Support for MT8183 depends on MTK_SCP.
> >
> >           To compile this driver as modules, choose M here: the
> >           modules will be called mtk-vcodec-dec and mtk-vcodec-enc.
>
> Just my 2 cents here, and to complement my last e-mail, the helper message
> here IMO is a lot more confusing than if you do this, instead:
>
>         config VIDEO_MEDIATEK_CODEC
>                  depends on VIDEO_MEDIATEK_VPU_SCP || VIDEO_MEDIATEK_VPU
>                  default y
>
>         config VIDEO_MEDIATEK_VPU
>             depends on VIDEO_DEV && VIDEO_V4L2
>             depends on ARCH_MEDIATEK || COMPILE_TEST
>             tristate "Enable Mediatek Video Processor Unit for MT8173"
>             help
>                 Select this option to enable Mediatek VPU on MT8173.
>
>         config VIDEO_MEDIATEK_VPU_SCP
>             depends on VIDEO_DEV && VIDEO_V4L2
>             depends on ARCH_MEDIATEK || COMPILE_TEST
>             tristate "Enable Mediatek Video Processor Unit for MT8183"
>             help
>                 Select this option to enable Mediatek VPU on MT8183.
>
> To be clear, from my side, I can live with either one of the alternatives,
> but, IMHO, the above is a lot clearer for anyone wanting to use
> VPU, as, if MTK_SCP is disabled, the MT8183 Kconfig prompt will
> disappear.

So I have experimented a bit and it turns out that even after adding
these new entries and using the "default" option that you suggested,
one still needs to perform the

    depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
    depends on MTK_SCP || !MTK_SCP

magic to prevent the VCODEC module from being built-in while having
the SCP and/or VPU support as modules (which would trigger the same
link error as before).

Also from experiment, the additional menu entries for SCP and VPU
won't bring any improvement in visibility since e.g. we cannot make
VIDEO_MEDIATEK_VPU_SCP visible unless MTK_SCP (the remoteproc option)
is also enabled. And this would leave us with the possibility of
having MTK_SCP enabled while leaving VIDEO_MEDIATEK_VPU_SCP
unselected, which is not a configuration one would want in practice.

Maybe I am missing something again, but I cannot find a benefit over
the current way - I'm completely open to revisit this in the future,
maybe by restructuring the driver some more, but for now I suggest we
fix the build breakage quickly. :)

I will send a v4 with some minor fixes.
diff mbox series

Patch

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index a3cb104956d5..457b6c39ddc0 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -253,18 +253,32 @@  config VIDEO_MEDIATEK_VCODEC
 	depends on MTK_IOMMU || COMPILE_TEST
 	depends on VIDEO_DEV && VIDEO_V4L2
 	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on VIDEO_MEDIATEK_VPU || MTK_SCP
+	# The two following lines ensure we have the same state ("m" or "y") as
+	# our dependencies, to avoid missing symbols during link.
+	depends on VIDEO_MEDIATEK_VPU || !VIDEO_MEDIATEK_VPU
+	depends on MTK_SCP || !MTK_SCP
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
-	select VIDEO_MEDIATEK_VPU
-	select MTK_SCP
+	select VIDEO_MEDIATEK_VCODEC_VPU if VIDEO_MEDIATEK_VPU
+	select VIDEO_MEDIATEK_VCODEC_SCP if MTK_SCP
 	help
 	    Mediatek video codec driver provides HW capability to
-	    encode and decode in a range of video formats
-	    This driver rely on VPU driver to communicate with VPU.
+	    encode and decode in a range of video formats on MT8173
+	    and MT8183.
+
+	    Note that support for MT8173 requires VIDEO_MEDIATEK_VPU to
+	    also be selected. Support for MT8183 depends on MTK_SCP.
 
 	    To compile this driver as modules, choose M here: the
 	    modules will be called mtk-vcodec-dec and mtk-vcodec-enc.
 
+config VIDEO_MEDIATEK_VCODEC_VPU
+	bool
+
+config VIDEO_MEDIATEK_VCODEC_SCP
+	bool
+
 config VIDEO_MEM2MEM_DEINTERLACE
 	tristate "Deinterlace support"
 	depends on VIDEO_DEV && VIDEO_V4L2
diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
index 6e1ea3a9f052..4618d43dbbc8 100644
--- a/drivers/media/platform/mtk-vcodec/Makefile
+++ b/drivers/media/platform/mtk-vcodec/Makefile
@@ -25,5 +25,11 @@  mtk-vcodec-enc-y := venc/venc_vp8_if.o \
 mtk-vcodec-common-y := mtk_vcodec_intr.o \
 		mtk_vcodec_util.o \
 		mtk_vcodec_fw.o \
-		mtk_vcodec_fw_vpu.o \
-		mtk_vcodec_fw_scp.o
+
+ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU),)
+mtk-vcodec-common-y += mtk_vcodec_fw_vpu.o
+endif
+
+ifneq ($(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP),)
+mtk-vcodec-common-y += mtk_vcodec_fw_scp.o
+endif
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
index 51f1694a7c7d..b41e66185cec 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw_priv.h
@@ -27,8 +27,26 @@  struct mtk_vcodec_fw_ops {
 	void (*release)(struct mtk_vcodec_fw *fw);
 };
 
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_VPU)
 struct mtk_vcodec_fw *mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
 					     enum mtk_vcodec_fw_use fw_use);
+#else
+static inline struct mtk_vcodec_fw *
+mtk_vcodec_fw_vpu_init(struct mtk_vcodec_dev *dev,
+		       enum mtk_vcodec_fw_use fw_use)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_VPU */
+
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VCODEC_SCP)
 struct mtk_vcodec_fw *mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev);
+#else
+static inline struct mtk_vcodec_fw *
+mtk_vcodec_fw_scp_init(struct mtk_vcodec_dev *dev)
+{
+	return ERR_PTR(-ENODEV);
+}
+#endif /* CONFIG_VIDEO_MEDIATEK_VCODEC_SCP */
 
 #endif /* _MTK_VCODEC_FW_PRIV_H_ */