All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-04 12:22 ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-04 12:22 UTC (permalink / raw)
  To: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa
  Cc: Hans Verkuil, linux-media, linux-kernel, linux-mediatek,
	Alexandre Courbot, Randy Dunlap

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
---
 drivers/media/platform/Kconfig                | 10 +--
 .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
 2 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index a3cb104956d5..98eb62e49ec2 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -253,14 +253,16 @@ 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
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
-	select VIDEO_MEDIATEK_VPU
-	select 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.
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
index 6c2a2568d844..23a80027a8fb 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
@@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
 			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
 	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
 			unsigned int len, unsigned int wait);
+	void (*release)(struct mtk_vcodec_fw *fw);
 };
 
 struct mtk_vcodec_fw {
@@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
 	struct mtk_scp *scp;
 };
 
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
+
 static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
 {
 	return vpu_load_firmware(fw->pdev);
@@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
 	return vpu_ipi_send(fw->pdev, id, buf, len);
 }
 
+static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
+{
+	put_device(&fw->pdev->dev);
+}
+
+static void mtk_vcodec_vpu_reset_handler(void *priv)
+{
+	struct mtk_vcodec_dev *dev = priv;
+	struct mtk_vcodec_ctx *ctx;
+
+	mtk_v4l2_err("Watchdog timeout!!");
+
+	mutex_lock(&dev->dev_mutex);
+	list_for_each_entry(ctx, &dev->ctx_list, list) {
+		ctx->state = MTK_STATE_ABORT;
+		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
+			       ctx->id);
+	}
+	mutex_unlock(&dev->dev_mutex);
+}
+
 static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
 	.load_firmware = mtk_vcodec_vpu_load_firmware,
 	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
@@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
 	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
 	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
 	.ipi_send = mtk_vcodec_vpu_ipi_send,
+	.release = mtk_vcodec_vpu_release,
 };
 
+#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
+
+#if IS_ENABLED(CONFIG_MTK_SCP)
+
 static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
 {
 	return rproc_boot(scp_get_rproc(fw->scp));
@@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
 	return scp_ipi_send(fw->scp, id, buf, len, wait);
 }
 
+static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
+{
+	scp_put(fw->scp);
+}
+
 static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
 	.load_firmware = mtk_vcodec_scp_load_firmware,
 	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
@@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
 	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
 	.ipi_register = mtk_vcodec_scp_set_ipi_register,
 	.ipi_send = mtk_vcodec_scp_ipi_send,
+	.release = mtk_vcodec_scp_release,
 };
 
-static void mtk_vcodec_reset_handler(void *priv)
-{
-	struct mtk_vcodec_dev *dev = priv;
-	struct mtk_vcodec_ctx *ctx;
-
-	mtk_v4l2_err("Watchdog timeout!!");
-
-	mutex_lock(&dev->dev_mutex);
-	list_for_each_entry(ctx, &dev->ctx_list, list) {
-		ctx->state = MTK_STATE_ABORT;
-		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
-			       ctx->id);
-	}
-	mutex_unlock(&dev->dev_mutex);
-}
+#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
 
 struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 					   enum mtk_vcodec_fw_type type,
@@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 
 	switch (type) {
 	case VPU:
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
 		ops = &mtk_vcodec_vpu_msg;
 		fw_pdev = vpu_get_plat_device(dev->plat_dev);
 		if (!fw_pdev) {
 			mtk_v4l2_err("firmware device is not ready");
 			return ERR_PTR(-EINVAL);
 		}
-		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
+		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
 				    dev, rst_id);
 		break;
+#else
+		mtk_v4l2_err("no VPU support in this kernel");
+		return ERR_PTR(-ENODEV);
+#endif
 	case SCP:
+#if IS_ENABLED(CONFIG_MTK_SCP)
 		ops = &mtk_vcodec_rproc_msg;
 		scp = scp_get(dev->plat_dev);
 		if (!scp) {
@@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 		break;
+#else
+		mtk_v4l2_err("no SCP support in this kernel");
+		return ERR_PTR(-ENODEV);
+#endif
 	default:
 		mtk_v4l2_err("invalid vcodec fw type");
 		return ERR_PTR(-EINVAL);
@@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
 
 void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
 {
-	switch (fw->type) {
-	case VPU:
-		put_device(&fw->pdev->dev);
-		break;
-	case SCP:
-		scp_put(fw->scp);
-		break;
-	}
+	fw->ops->release(fw);
 }
 EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
 
-- 
2.28.0.806.g8561365e88-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-04 12:22 ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-04 12:22 UTC (permalink / raw)
  To: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa
  Cc: Alexandre Courbot, Randy Dunlap, linux-kernel, linux-mediatek,
	Hans Verkuil, linux-media

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
---
 drivers/media/platform/Kconfig                | 10 +--
 .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
 2 files changed, 54 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index a3cb104956d5..98eb62e49ec2 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -253,14 +253,16 @@ 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
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
-	select VIDEO_MEDIATEK_VPU
-	select 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.
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
index 6c2a2568d844..23a80027a8fb 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
@@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
 			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
 	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
 			unsigned int len, unsigned int wait);
+	void (*release)(struct mtk_vcodec_fw *fw);
 };
 
 struct mtk_vcodec_fw {
@@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
 	struct mtk_scp *scp;
 };
 
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
+
 static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
 {
 	return vpu_load_firmware(fw->pdev);
@@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
 	return vpu_ipi_send(fw->pdev, id, buf, len);
 }
 
+static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
+{
+	put_device(&fw->pdev->dev);
+}
+
+static void mtk_vcodec_vpu_reset_handler(void *priv)
+{
+	struct mtk_vcodec_dev *dev = priv;
+	struct mtk_vcodec_ctx *ctx;
+
+	mtk_v4l2_err("Watchdog timeout!!");
+
+	mutex_lock(&dev->dev_mutex);
+	list_for_each_entry(ctx, &dev->ctx_list, list) {
+		ctx->state = MTK_STATE_ABORT;
+		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
+			       ctx->id);
+	}
+	mutex_unlock(&dev->dev_mutex);
+}
+
 static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
 	.load_firmware = mtk_vcodec_vpu_load_firmware,
 	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
@@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
 	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
 	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
 	.ipi_send = mtk_vcodec_vpu_ipi_send,
+	.release = mtk_vcodec_vpu_release,
 };
 
+#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
+
+#if IS_ENABLED(CONFIG_MTK_SCP)
+
 static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
 {
 	return rproc_boot(scp_get_rproc(fw->scp));
@@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
 	return scp_ipi_send(fw->scp, id, buf, len, wait);
 }
 
+static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
+{
+	scp_put(fw->scp);
+}
+
 static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
 	.load_firmware = mtk_vcodec_scp_load_firmware,
 	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
@@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
 	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
 	.ipi_register = mtk_vcodec_scp_set_ipi_register,
 	.ipi_send = mtk_vcodec_scp_ipi_send,
+	.release = mtk_vcodec_scp_release,
 };
 
-static void mtk_vcodec_reset_handler(void *priv)
-{
-	struct mtk_vcodec_dev *dev = priv;
-	struct mtk_vcodec_ctx *ctx;
-
-	mtk_v4l2_err("Watchdog timeout!!");
-
-	mutex_lock(&dev->dev_mutex);
-	list_for_each_entry(ctx, &dev->ctx_list, list) {
-		ctx->state = MTK_STATE_ABORT;
-		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
-			       ctx->id);
-	}
-	mutex_unlock(&dev->dev_mutex);
-}
+#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
 
 struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 					   enum mtk_vcodec_fw_type type,
@@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 
 	switch (type) {
 	case VPU:
+#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
 		ops = &mtk_vcodec_vpu_msg;
 		fw_pdev = vpu_get_plat_device(dev->plat_dev);
 		if (!fw_pdev) {
 			mtk_v4l2_err("firmware device is not ready");
 			return ERR_PTR(-EINVAL);
 		}
-		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
+		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
 				    dev, rst_id);
 		break;
+#else
+		mtk_v4l2_err("no VPU support in this kernel");
+		return ERR_PTR(-ENODEV);
+#endif
 	case SCP:
+#if IS_ENABLED(CONFIG_MTK_SCP)
 		ops = &mtk_vcodec_rproc_msg;
 		scp = scp_get(dev->plat_dev);
 		if (!scp) {
@@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
 			return ERR_PTR(-EPROBE_DEFER);
 		}
 		break;
+#else
+		mtk_v4l2_err("no SCP support in this kernel");
+		return ERR_PTR(-ENODEV);
+#endif
 	default:
 		mtk_v4l2_err("invalid vcodec fw type");
 		return ERR_PTR(-EINVAL);
@@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
 
 void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
 {
-	switch (fw->type) {
-	case VPU:
-		put_device(&fw->pdev->dev);
-		break;
-	case SCP:
-		scp_put(fw->scp);
-		break;
-	}
+	fw->ops->release(fw);
 }
 EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
 
-- 
2.28.0.806.g8561365e88-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-04 12:22 ` Alexandre Courbot
@ 2020-10-05  3:32   ` Alexandre Courbot
  -1 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-05  3:32 UTC (permalink / raw)
  To: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa
  Cc: Hans Verkuil, Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support, Randy Dunlap

On Sun, Oct 4, 2020 at 9:22 PM Alexandre Courbot <acourbot@chromium.org> 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

Forgot to send the changelog, so here it is:

Changes since v1:
* Added Acked-by from Randy.
* Fixed typo in Kconfig description.

> ---
>  drivers/media/platform/Kconfig                | 10 +--
>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>  2 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..98eb62e49ec2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,14 +253,16 @@ 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
>         select VIDEOBUF2_DMA_CONTIG
>         select V4L2_MEM2MEM_DEV
> -       select VIDEO_MEDIATEK_VPU
> -       select 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.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> index 6c2a2568d844..23a80027a8fb 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>                             mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>         int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>                         unsigned int len, unsigned int wait);
> +       void (*release)(struct mtk_vcodec_fw *fw);
>  };
>
>  struct mtk_vcodec_fw {
> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>         struct mtk_scp *scp;
>  };
>
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
> +
>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>         return vpu_load_firmware(fw->pdev);
> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>         return vpu_ipi_send(fw->pdev, id, buf, len);
>  }
>
> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
> +{
> +       put_device(&fw->pdev->dev);
> +}
> +
> +static void mtk_vcodec_vpu_reset_handler(void *priv)
> +{
> +       struct mtk_vcodec_dev *dev = priv;
> +       struct mtk_vcodec_ctx *ctx;
> +
> +       mtk_v4l2_err("Watchdog timeout!!");
> +
> +       mutex_lock(&dev->dev_mutex);
> +       list_for_each_entry(ctx, &dev->ctx_list, list) {
> +               ctx->state = MTK_STATE_ABORT;
> +               mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> +                              ctx->id);
> +       }
> +       mutex_unlock(&dev->dev_mutex);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>         .load_firmware = mtk_vcodec_vpu_load_firmware,
>         .get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>         .map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>         .ipi_register = mtk_vcodec_vpu_set_ipi_register,
>         .ipi_send = mtk_vcodec_vpu_ipi_send,
> +       .release = mtk_vcodec_vpu_release,
>  };
>
> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
> +
> +#if IS_ENABLED(CONFIG_MTK_SCP)
> +
>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>         return rproc_boot(scp_get_rproc(fw->scp));
> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>         return scp_ipi_send(fw->scp, id, buf, len, wait);
>  }
>
> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
> +{
> +       scp_put(fw->scp);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>         .load_firmware = mtk_vcodec_scp_load_firmware,
>         .get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>         .map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>         .ipi_register = mtk_vcodec_scp_set_ipi_register,
>         .ipi_send = mtk_vcodec_scp_ipi_send,
> +       .release = mtk_vcodec_scp_release,
>  };
>
> -static void mtk_vcodec_reset_handler(void *priv)
> -{
> -       struct mtk_vcodec_dev *dev = priv;
> -       struct mtk_vcodec_ctx *ctx;
> -
> -       mtk_v4l2_err("Watchdog timeout!!");
> -
> -       mutex_lock(&dev->dev_mutex);
> -       list_for_each_entry(ctx, &dev->ctx_list, list) {
> -               ctx->state = MTK_STATE_ABORT;
> -               mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> -                              ctx->id);
> -       }
> -       mutex_unlock(&dev->dev_mutex);
> -}
> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>
>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>                                            enum mtk_vcodec_fw_type type,
> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>
>         switch (type) {
>         case VPU:
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>                 ops = &mtk_vcodec_vpu_msg;
>                 fw_pdev = vpu_get_plat_device(dev->plat_dev);
>                 if (!fw_pdev) {
>                         mtk_v4l2_err("firmware device is not ready");
>                         return ERR_PTR(-EINVAL);
>                 }
> -               vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> +               vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>                                     dev, rst_id);
>                 break;
> +#else
> +               mtk_v4l2_err("no VPU support in this kernel");
> +               return ERR_PTR(-ENODEV);
> +#endif
>         case SCP:
> +#if IS_ENABLED(CONFIG_MTK_SCP)
>                 ops = &mtk_vcodec_rproc_msg;
>                 scp = scp_get(dev->plat_dev);
>                 if (!scp) {
> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>                         return ERR_PTR(-EPROBE_DEFER);
>                 }
>                 break;
> +#else
> +               mtk_v4l2_err("no SCP support in this kernel");
> +               return ERR_PTR(-ENODEV);
> +#endif
>         default:
>                 mtk_v4l2_err("invalid vcodec fw type");
>                 return ERR_PTR(-EINVAL);
> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>
>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>  {
> -       switch (fw->type) {
> -       case VPU:
> -               put_device(&fw->pdev->dev);
> -               break;
> -       case SCP:
> -               scp_put(fw->scp);
> -               break;
> -       }
> +       fw->ops->release(fw);
>  }
>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>
> --
> 2.28.0.806.g8561365e88-goog
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-05  3:32   ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-05  3:32 UTC (permalink / raw)
  To: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa
  Cc: Hans Verkuil, Randy Dunlap,
	moderated list:ARM/Mediatek SoC support, LKML,
	Linux Media Mailing List

On Sun, Oct 4, 2020 at 9:22 PM Alexandre Courbot <acourbot@chromium.org> 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

Forgot to send the changelog, so here it is:

Changes since v1:
* Added Acked-by from Randy.
* Fixed typo in Kconfig description.

> ---
>  drivers/media/platform/Kconfig                | 10 +--
>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>  2 files changed, 54 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..98eb62e49ec2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,14 +253,16 @@ 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
>         select VIDEOBUF2_DMA_CONTIG
>         select V4L2_MEM2MEM_DEV
> -       select VIDEO_MEDIATEK_VPU
> -       select 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.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> index 6c2a2568d844..23a80027a8fb 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>                             mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>         int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>                         unsigned int len, unsigned int wait);
> +       void (*release)(struct mtk_vcodec_fw *fw);
>  };
>
>  struct mtk_vcodec_fw {
> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>         struct mtk_scp *scp;
>  };
>
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
> +
>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>         return vpu_load_firmware(fw->pdev);
> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>         return vpu_ipi_send(fw->pdev, id, buf, len);
>  }
>
> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
> +{
> +       put_device(&fw->pdev->dev);
> +}
> +
> +static void mtk_vcodec_vpu_reset_handler(void *priv)
> +{
> +       struct mtk_vcodec_dev *dev = priv;
> +       struct mtk_vcodec_ctx *ctx;
> +
> +       mtk_v4l2_err("Watchdog timeout!!");
> +
> +       mutex_lock(&dev->dev_mutex);
> +       list_for_each_entry(ctx, &dev->ctx_list, list) {
> +               ctx->state = MTK_STATE_ABORT;
> +               mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> +                              ctx->id);
> +       }
> +       mutex_unlock(&dev->dev_mutex);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>         .load_firmware = mtk_vcodec_vpu_load_firmware,
>         .get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>         .map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>         .ipi_register = mtk_vcodec_vpu_set_ipi_register,
>         .ipi_send = mtk_vcodec_vpu_ipi_send,
> +       .release = mtk_vcodec_vpu_release,
>  };
>
> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
> +
> +#if IS_ENABLED(CONFIG_MTK_SCP)
> +
>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>         return rproc_boot(scp_get_rproc(fw->scp));
> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>         return scp_ipi_send(fw->scp, id, buf, len, wait);
>  }
>
> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
> +{
> +       scp_put(fw->scp);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>         .load_firmware = mtk_vcodec_scp_load_firmware,
>         .get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>         .map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>         .ipi_register = mtk_vcodec_scp_set_ipi_register,
>         .ipi_send = mtk_vcodec_scp_ipi_send,
> +       .release = mtk_vcodec_scp_release,
>  };
>
> -static void mtk_vcodec_reset_handler(void *priv)
> -{
> -       struct mtk_vcodec_dev *dev = priv;
> -       struct mtk_vcodec_ctx *ctx;
> -
> -       mtk_v4l2_err("Watchdog timeout!!");
> -
> -       mutex_lock(&dev->dev_mutex);
> -       list_for_each_entry(ctx, &dev->ctx_list, list) {
> -               ctx->state = MTK_STATE_ABORT;
> -               mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> -                              ctx->id);
> -       }
> -       mutex_unlock(&dev->dev_mutex);
> -}
> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>
>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>                                            enum mtk_vcodec_fw_type type,
> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>
>         switch (type) {
>         case VPU:
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>                 ops = &mtk_vcodec_vpu_msg;
>                 fw_pdev = vpu_get_plat_device(dev->plat_dev);
>                 if (!fw_pdev) {
>                         mtk_v4l2_err("firmware device is not ready");
>                         return ERR_PTR(-EINVAL);
>                 }
> -               vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> +               vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>                                     dev, rst_id);
>                 break;
> +#else
> +               mtk_v4l2_err("no VPU support in this kernel");
> +               return ERR_PTR(-ENODEV);
> +#endif
>         case SCP:
> +#if IS_ENABLED(CONFIG_MTK_SCP)
>                 ops = &mtk_vcodec_rproc_msg;
>                 scp = scp_get(dev->plat_dev);
>                 if (!scp) {
> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>                         return ERR_PTR(-EPROBE_DEFER);
>                 }
>                 break;
> +#else
> +               mtk_v4l2_err("no SCP support in this kernel");
> +               return ERR_PTR(-ENODEV);
> +#endif
>         default:
>                 mtk_v4l2_err("invalid vcodec fw type");
>                 return ERR_PTR(-EINVAL);
> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>
>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>  {
> -       switch (fw->type) {
> -       case VPU:
> -               put_device(&fw->pdev->dev);
> -               break;
> -       case SCP:
> -               scp_put(fw->scp);
> -               break;
> -       }
> +       fw->ops->release(fw);
>  }
>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>
> --
> 2.28.0.806.g8561365e88-goog
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-04 12:22 ` Alexandre Courbot
@ 2020-10-05  8:48   ` Sakari Ailus
  -1 siblings, 0 replies; 20+ messages in thread
From: Sakari Ailus @ 2020-10-05  8:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Tiffany Lin, Andrew-CT Chen, Tomasz Figa, Hans Verkuil,
	linux-media, linux-kernel, linux-mediatek, Randy Dunlap

Hi Alexandre,

On Sun, Oct 04, 2020 at 09:22:34PM +0900, 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

Thanks for the patch!

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

I wonder if this driver suffers from similar object lifetime management
issues than V4L2 and MC do, albeit not related to either. Say, what happens
if you unbind the other device while mtk-vcodec is in use?

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-05  8:48   ` Sakari Ailus
  0 siblings, 0 replies; 20+ messages in thread
From: Sakari Ailus @ 2020-10-05  8:48 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Andrew-CT Chen, Randy Dunlap, linux-kernel, Tomasz Figa,
	linux-mediatek, Hans Verkuil, Tiffany Lin, linux-media

Hi Alexandre,

On Sun, Oct 04, 2020 at 09:22:34PM +0900, 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

Thanks for the patch!

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

I wonder if this driver suffers from similar object lifetime management
issues than V4L2 and MC do, albeit not related to either. Say, what happens
if you unbind the other device while mtk-vcodec is in use?

-- 
Regards,

Sakari Ailus

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-05  8:48   ` Sakari Ailus
@ 2020-10-05 11:29     ` Alexandre Courbot
  -1 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-05 11:29 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Tiffany Lin, Andrew-CT Chen, Tomasz Figa, Hans Verkuil,
	Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support, Randy Dunlap

On Mon, Oct 5, 2020 at 5:49 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Alexandre,
>
> On Sun, Oct 04, 2020 at 09:22:34PM +0900, 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
>
> Thanks for the patch!
>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thanks!

>
> I wonder if this driver suffers from similar object lifetime management
> issues than V4L2 and MC do, albeit not related to either. Say, what happens
> if you unbind the other device while mtk-vcodec is in use?

That's a question that maybe the driver maintainers can answer, but
from my experience during development I have been able to unload one
of the two mtk-vcodec-* modules while keeping the other one active.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-05 11:29     ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-05 11:29 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Andrew-CT Chen, Randy Dunlap, LKML, Tomasz Figa,
	moderated list:ARM/Mediatek SoC support, Hans Verkuil,
	Tiffany Lin, Linux Media Mailing List

On Mon, Oct 5, 2020 at 5:49 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Alexandre,
>
> On Sun, Oct 04, 2020 at 09:22:34PM +0900, 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
>
> Thanks for the patch!
>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Thanks!

>
> I wonder if this driver suffers from similar object lifetime management
> issues than V4L2 and MC do, albeit not related to either. Say, what happens
> if you unbind the other device while mtk-vcodec is in use?

That's a question that maybe the driver maintainers can answer, but
from my experience during development I have been able to unload one
of the two mtk-vcodec-* modules while keeping the other one active.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-04 12:22 ` Alexandre Courbot
@ 2020-10-08 13:07   ` Hans Verkuil
  -1 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 13:07 UTC (permalink / raw)
  To: Alexandre Courbot, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa
  Cc: linux-media, linux-kernel, linux-mediatek, Randy Dunlap

Hi Alexandre,

On 04/10/2020 14:22, 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
> ---
>  drivers/media/platform/Kconfig                | 10 +--
>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>  2 files changed, 54 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..98eb62e49ec2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,14 +253,16 @@ 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

Close, but no cigar.

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.

Regards,

	Hans

>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select VIDEO_MEDIATEK_VPU
> -	select 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.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> index 6c2a2568d844..23a80027a8fb 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>  			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>  	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>  			unsigned int len, unsigned int wait);
> +	void (*release)(struct mtk_vcodec_fw *fw);
>  };
>  
>  struct mtk_vcodec_fw {
> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>  	struct mtk_scp *scp;
>  };
>  
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
> +
>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>  	return vpu_load_firmware(fw->pdev);
> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>  	return vpu_ipi_send(fw->pdev, id, buf, len);
>  }
>  
> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
> +{
> +	put_device(&fw->pdev->dev);
> +}
> +
> +static void mtk_vcodec_vpu_reset_handler(void *priv)
> +{
> +	struct mtk_vcodec_dev *dev = priv;
> +	struct mtk_vcodec_ctx *ctx;
> +
> +	mtk_v4l2_err("Watchdog timeout!!");
> +
> +	mutex_lock(&dev->dev_mutex);
> +	list_for_each_entry(ctx, &dev->ctx_list, list) {
> +		ctx->state = MTK_STATE_ABORT;
> +		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> +			       ctx->id);
> +	}
> +	mutex_unlock(&dev->dev_mutex);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>  	.load_firmware = mtk_vcodec_vpu_load_firmware,
>  	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>  	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>  	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
>  	.ipi_send = mtk_vcodec_vpu_ipi_send,
> +	.release = mtk_vcodec_vpu_release,
>  };
>  
> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
> +
> +#if IS_ENABLED(CONFIG_MTK_SCP)
> +
>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>  	return rproc_boot(scp_get_rproc(fw->scp));
> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>  	return scp_ipi_send(fw->scp, id, buf, len, wait);
>  }
>  
> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
> +{
> +	scp_put(fw->scp);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>  	.load_firmware = mtk_vcodec_scp_load_firmware,
>  	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>  	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>  	.ipi_register = mtk_vcodec_scp_set_ipi_register,
>  	.ipi_send = mtk_vcodec_scp_ipi_send,
> +	.release = mtk_vcodec_scp_release,
>  };
>  
> -static void mtk_vcodec_reset_handler(void *priv)
> -{
> -	struct mtk_vcodec_dev *dev = priv;
> -	struct mtk_vcodec_ctx *ctx;
> -
> -	mtk_v4l2_err("Watchdog timeout!!");
> -
> -	mutex_lock(&dev->dev_mutex);
> -	list_for_each_entry(ctx, &dev->ctx_list, list) {
> -		ctx->state = MTK_STATE_ABORT;
> -		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> -			       ctx->id);
> -	}
> -	mutex_unlock(&dev->dev_mutex);
> -}
> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>  
>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  					   enum mtk_vcodec_fw_type type,
> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  
>  	switch (type) {
>  	case VPU:
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>  		ops = &mtk_vcodec_vpu_msg;
>  		fw_pdev = vpu_get_plat_device(dev->plat_dev);
>  		if (!fw_pdev) {
>  			mtk_v4l2_err("firmware device is not ready");
>  			return ERR_PTR(-EINVAL);
>  		}
> -		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> +		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>  				    dev, rst_id);
>  		break;
> +#else
> +		mtk_v4l2_err("no VPU support in this kernel");
> +		return ERR_PTR(-ENODEV);
> +#endif
>  	case SCP:
> +#if IS_ENABLED(CONFIG_MTK_SCP)
>  		ops = &mtk_vcodec_rproc_msg;
>  		scp = scp_get(dev->plat_dev);
>  		if (!scp) {
> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  			return ERR_PTR(-EPROBE_DEFER);
>  		}
>  		break;
> +#else
> +		mtk_v4l2_err("no SCP support in this kernel");
> +		return ERR_PTR(-ENODEV);
> +#endif
>  	default:
>  		mtk_v4l2_err("invalid vcodec fw type");
>  		return ERR_PTR(-EINVAL);
> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>  
>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>  {
> -	switch (fw->type) {
> -	case VPU:
> -		put_device(&fw->pdev->dev);
> -		break;
> -	case SCP:
> -		scp_put(fw->scp);
> -		break;
> -	}
> +	fw->ops->release(fw);
>  }
>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>  
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-08 13:07   ` Hans Verkuil
  0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 13:07 UTC (permalink / raw)
  To: Alexandre Courbot, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa
  Cc: Randy Dunlap, linux-mediatek, linux-kernel, linux-media

Hi Alexandre,

On 04/10/2020 14:22, 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
> ---
>  drivers/media/platform/Kconfig                | 10 +--
>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>  2 files changed, 54 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index a3cb104956d5..98eb62e49ec2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -253,14 +253,16 @@ 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

Close, but no cigar.

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.

Regards,

	Hans

>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
> -	select VIDEO_MEDIATEK_VPU
> -	select 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.
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> index 6c2a2568d844..23a80027a8fb 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>  			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>  	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>  			unsigned int len, unsigned int wait);
> +	void (*release)(struct mtk_vcodec_fw *fw);
>  };
>  
>  struct mtk_vcodec_fw {
> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>  	struct mtk_scp *scp;
>  };
>  
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
> +
>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>  	return vpu_load_firmware(fw->pdev);
> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>  	return vpu_ipi_send(fw->pdev, id, buf, len);
>  }
>  
> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
> +{
> +	put_device(&fw->pdev->dev);
> +}
> +
> +static void mtk_vcodec_vpu_reset_handler(void *priv)
> +{
> +	struct mtk_vcodec_dev *dev = priv;
> +	struct mtk_vcodec_ctx *ctx;
> +
> +	mtk_v4l2_err("Watchdog timeout!!");
> +
> +	mutex_lock(&dev->dev_mutex);
> +	list_for_each_entry(ctx, &dev->ctx_list, list) {
> +		ctx->state = MTK_STATE_ABORT;
> +		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> +			       ctx->id);
> +	}
> +	mutex_unlock(&dev->dev_mutex);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>  	.load_firmware = mtk_vcodec_vpu_load_firmware,
>  	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>  	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>  	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
>  	.ipi_send = mtk_vcodec_vpu_ipi_send,
> +	.release = mtk_vcodec_vpu_release,
>  };
>  
> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
> +
> +#if IS_ENABLED(CONFIG_MTK_SCP)
> +
>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>  {
>  	return rproc_boot(scp_get_rproc(fw->scp));
> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>  	return scp_ipi_send(fw->scp, id, buf, len, wait);
>  }
>  
> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
> +{
> +	scp_put(fw->scp);
> +}
> +
>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>  	.load_firmware = mtk_vcodec_scp_load_firmware,
>  	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>  	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>  	.ipi_register = mtk_vcodec_scp_set_ipi_register,
>  	.ipi_send = mtk_vcodec_scp_ipi_send,
> +	.release = mtk_vcodec_scp_release,
>  };
>  
> -static void mtk_vcodec_reset_handler(void *priv)
> -{
> -	struct mtk_vcodec_dev *dev = priv;
> -	struct mtk_vcodec_ctx *ctx;
> -
> -	mtk_v4l2_err("Watchdog timeout!!");
> -
> -	mutex_lock(&dev->dev_mutex);
> -	list_for_each_entry(ctx, &dev->ctx_list, list) {
> -		ctx->state = MTK_STATE_ABORT;
> -		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> -			       ctx->id);
> -	}
> -	mutex_unlock(&dev->dev_mutex);
> -}
> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>  
>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  					   enum mtk_vcodec_fw_type type,
> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  
>  	switch (type) {
>  	case VPU:
> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>  		ops = &mtk_vcodec_vpu_msg;
>  		fw_pdev = vpu_get_plat_device(dev->plat_dev);
>  		if (!fw_pdev) {
>  			mtk_v4l2_err("firmware device is not ready");
>  			return ERR_PTR(-EINVAL);
>  		}
> -		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> +		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>  				    dev, rst_id);
>  		break;
> +#else
> +		mtk_v4l2_err("no VPU support in this kernel");
> +		return ERR_PTR(-ENODEV);
> +#endif
>  	case SCP:
> +#if IS_ENABLED(CONFIG_MTK_SCP)
>  		ops = &mtk_vcodec_rproc_msg;
>  		scp = scp_get(dev->plat_dev);
>  		if (!scp) {
> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>  			return ERR_PTR(-EPROBE_DEFER);
>  		}
>  		break;
> +#else
> +		mtk_v4l2_err("no SCP support in this kernel");
> +		return ERR_PTR(-ENODEV);
> +#endif
>  	default:
>  		mtk_v4l2_err("invalid vcodec fw type");
>  		return ERR_PTR(-EINVAL);
> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>  
>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>  {
> -	switch (fw->type) {
> -	case VPU:
> -		put_device(&fw->pdev->dev);
> -		break;
> -	case SCP:
> -		scp_put(fw->scp);
> -		break;
> -	}
> +	fw->ops->release(fw);
>  }
>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>  
> 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-08 13:07   ` Hans Verkuil
@ 2020-10-08 13:12     ` Hans Verkuil
  -1 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 13:12 UTC (permalink / raw)
  To: Alexandre Courbot, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa
  Cc: linux-media, linux-kernel, linux-mediatek, Randy Dunlap

On 08/10/2020 15:07, Hans Verkuil wrote:
> Hi Alexandre,
> 
> On 04/10/2020 14:22, 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
>> ---
>>  drivers/media/platform/Kconfig                | 10 +--
>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>>  2 files changed, 54 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index a3cb104956d5..98eb62e49ec2 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -253,14 +253,16 @@ 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
> 
> Close, but no cigar.
> 
> 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.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>  	select VIDEOBUF2_DMA_CONTIG
>>  	select V4L2_MEM2MEM_DEV
>> -	select VIDEO_MEDIATEK_VPU
>> -	select 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.
>> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> index 6c2a2568d844..23a80027a8fb 100644
>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>>  			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>>  	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  			unsigned int len, unsigned int wait);
>> +	void (*release)(struct mtk_vcodec_fw *fw);
>>  };
>>  
>>  struct mtk_vcodec_fw {
>> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>>  	struct mtk_scp *scp;
>>  };
>>  
>> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>> +
>>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>>  {
>>  	return vpu_load_firmware(fw->pdev);
>> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  	return vpu_ipi_send(fw->pdev, id, buf, len);
>>  }
>>  
>> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
>> +{
>> +	put_device(&fw->pdev->dev);
>> +}
>> +
>> +static void mtk_vcodec_vpu_reset_handler(void *priv)
>> +{
>> +	struct mtk_vcodec_dev *dev = priv;
>> +	struct mtk_vcodec_ctx *ctx;
>> +
>> +	mtk_v4l2_err("Watchdog timeout!!");
>> +
>> +	mutex_lock(&dev->dev_mutex);
>> +	list_for_each_entry(ctx, &dev->ctx_list, list) {
>> +		ctx->state = MTK_STATE_ABORT;
>> +		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
>> +			       ctx->id);
>> +	}
>> +	mutex_unlock(&dev->dev_mutex);
>> +}
>> +
>>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>>  	.load_firmware = mtk_vcodec_vpu_load_firmware,
>>  	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
>> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>>  	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>>  	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
>>  	.ipi_send = mtk_vcodec_vpu_ipi_send,
>> +	.release = mtk_vcodec_vpu_release,
>>  };
>>  
>> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
>> +
>> +#if IS_ENABLED(CONFIG_MTK_SCP)
>> +
>>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>>  {
>>  	return rproc_boot(scp_get_rproc(fw->scp));
>> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  	return scp_ipi_send(fw->scp, id, buf, len, wait);
>>  }
>>  
>> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
>> +{
>> +	scp_put(fw->scp);
>> +}
>> +
>>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>>  	.load_firmware = mtk_vcodec_scp_load_firmware,
>>  	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
>> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>>  	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>>  	.ipi_register = mtk_vcodec_scp_set_ipi_register,
>>  	.ipi_send = mtk_vcodec_scp_ipi_send,
>> +	.release = mtk_vcodec_scp_release,
>>  };
>>  
>> -static void mtk_vcodec_reset_handler(void *priv)
>> -{
>> -	struct mtk_vcodec_dev *dev = priv;
>> -	struct mtk_vcodec_ctx *ctx;
>> -
>> -	mtk_v4l2_err("Watchdog timeout!!");
>> -
>> -	mutex_lock(&dev->dev_mutex);
>> -	list_for_each_entry(ctx, &dev->ctx_list, list) {
>> -		ctx->state = MTK_STATE_ABORT;
>> -		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
>> -			       ctx->id);
>> -	}
>> -	mutex_unlock(&dev->dev_mutex);
>> -}
>> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>>  
>>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  					   enum mtk_vcodec_fw_type type,
>> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  
>>  	switch (type) {
>>  	case VPU:
>> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>>  		ops = &mtk_vcodec_vpu_msg;
>>  		fw_pdev = vpu_get_plat_device(dev->plat_dev);
>>  		if (!fw_pdev) {
>>  			mtk_v4l2_err("firmware device is not ready");
>>  			return ERR_PTR(-EINVAL);
>>  		}
>> -		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
>> +		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>>  				    dev, rst_id);
>>  		break;
>> +#else
>> +		mtk_v4l2_err("no VPU support in this kernel");
>> +		return ERR_PTR(-ENODEV);
>> +#endif
>>  	case SCP:
>> +#if IS_ENABLED(CONFIG_MTK_SCP)
>>  		ops = &mtk_vcodec_rproc_msg;
>>  		scp = scp_get(dev->plat_dev);
>>  		if (!scp) {
>> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  			return ERR_PTR(-EPROBE_DEFER);
>>  		}
>>  		break;
>> +#else
>> +		mtk_v4l2_err("no SCP support in this kernel");
>> +		return ERR_PTR(-ENODEV);
>> +#endif
>>  	default:
>>  		mtk_v4l2_err("invalid vcodec fw type");
>>  		return ERR_PTR(-EINVAL);
>> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>>  
>>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>>  {
>> -	switch (fw->type) {
>> -	case VPU:
>> -		put_device(&fw->pdev->dev);
>> -		break;
>> -	case SCP:
>> -		scp_put(fw->scp);
>> -		break;
>> -	}
>> +	fw->ops->release(fw);
>>  }
>>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>>  
>>
> 


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-08 13:12     ` Hans Verkuil
  0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 13:12 UTC (permalink / raw)
  To: Alexandre Courbot, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa
  Cc: Randy Dunlap, linux-mediatek, linux-kernel, linux-media

On 08/10/2020 15:07, Hans Verkuil wrote:
> Hi Alexandre,
> 
> On 04/10/2020 14:22, 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
>> ---
>>  drivers/media/platform/Kconfig                | 10 +--
>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>>  2 files changed, 54 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index a3cb104956d5..98eb62e49ec2 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -253,14 +253,16 @@ 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
> 
> Close, but no cigar.
> 
> 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.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>  	select VIDEOBUF2_DMA_CONTIG
>>  	select V4L2_MEM2MEM_DEV
>> -	select VIDEO_MEDIATEK_VPU
>> -	select 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.
>> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> index 6c2a2568d844..23a80027a8fb 100644
>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
>> @@ -13,6 +13,7 @@ struct mtk_vcodec_fw_ops {
>>  			    mtk_vcodec_ipi_handler handler, const char *name, void *priv);
>>  	int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  			unsigned int len, unsigned int wait);
>> +	void (*release)(struct mtk_vcodec_fw *fw);
>>  };
>>  
>>  struct mtk_vcodec_fw {
>> @@ -22,6 +23,8 @@ struct mtk_vcodec_fw {
>>  	struct mtk_scp *scp;
>>  };
>>  
>> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>> +
>>  static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
>>  {
>>  	return vpu_load_firmware(fw->pdev);
>> @@ -64,6 +67,27 @@ static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  	return vpu_ipi_send(fw->pdev, id, buf, len);
>>  }
>>  
>> +static void mtk_vcodec_vpu_release(struct mtk_vcodec_fw *fw)
>> +{
>> +	put_device(&fw->pdev->dev);
>> +}
>> +
>> +static void mtk_vcodec_vpu_reset_handler(void *priv)
>> +{
>> +	struct mtk_vcodec_dev *dev = priv;
>> +	struct mtk_vcodec_ctx *ctx;
>> +
>> +	mtk_v4l2_err("Watchdog timeout!!");
>> +
>> +	mutex_lock(&dev->dev_mutex);
>> +	list_for_each_entry(ctx, &dev->ctx_list, list) {
>> +		ctx->state = MTK_STATE_ABORT;
>> +		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
>> +			       ctx->id);
>> +	}
>> +	mutex_unlock(&dev->dev_mutex);
>> +}
>> +
>>  static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>>  	.load_firmware = mtk_vcodec_vpu_load_firmware,
>>  	.get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
>> @@ -71,8 +95,13 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
>>  	.map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
>>  	.ipi_register = mtk_vcodec_vpu_set_ipi_register,
>>  	.ipi_send = mtk_vcodec_vpu_ipi_send,
>> +	.release = mtk_vcodec_vpu_release,
>>  };
>>  
>> +#endif  /* IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU) */
>> +
>> +#if IS_ENABLED(CONFIG_MTK_SCP)
>> +
>>  static int mtk_vcodec_scp_load_firmware(struct mtk_vcodec_fw *fw)
>>  {
>>  	return rproc_boot(scp_get_rproc(fw->scp));
>> @@ -107,6 +136,11 @@ static int mtk_vcodec_scp_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
>>  	return scp_ipi_send(fw->scp, id, buf, len, wait);
>>  }
>>  
>> +static void mtk_vcodec_scp_release(struct mtk_vcodec_fw *fw)
>> +{
>> +	scp_put(fw->scp);
>> +}
>> +
>>  static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>>  	.load_firmware = mtk_vcodec_scp_load_firmware,
>>  	.get_vdec_capa = mtk_vcodec_scp_get_vdec_capa,
>> @@ -114,23 +148,10 @@ static const struct mtk_vcodec_fw_ops mtk_vcodec_rproc_msg = {
>>  	.map_dm_addr = mtk_vcodec_vpu_scp_dm_addr,
>>  	.ipi_register = mtk_vcodec_scp_set_ipi_register,
>>  	.ipi_send = mtk_vcodec_scp_ipi_send,
>> +	.release = mtk_vcodec_scp_release,
>>  };
>>  
>> -static void mtk_vcodec_reset_handler(void *priv)
>> -{
>> -	struct mtk_vcodec_dev *dev = priv;
>> -	struct mtk_vcodec_ctx *ctx;
>> -
>> -	mtk_v4l2_err("Watchdog timeout!!");
>> -
>> -	mutex_lock(&dev->dev_mutex);
>> -	list_for_each_entry(ctx, &dev->ctx_list, list) {
>> -		ctx->state = MTK_STATE_ABORT;
>> -		mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
>> -			       ctx->id);
>> -	}
>> -	mutex_unlock(&dev->dev_mutex);
>> -}
>> +#endif  /* IS_ENABLED(CONFIG_MTK_SCP) */
>>  
>>  struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  					   enum mtk_vcodec_fw_type type,
>> @@ -143,16 +164,22 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  
>>  	switch (type) {
>>  	case VPU:
>> +#if IS_ENABLED(CONFIG_VIDEO_MEDIATEK_VPU)
>>  		ops = &mtk_vcodec_vpu_msg;
>>  		fw_pdev = vpu_get_plat_device(dev->plat_dev);
>>  		if (!fw_pdev) {
>>  			mtk_v4l2_err("firmware device is not ready");
>>  			return ERR_PTR(-EINVAL);
>>  		}
>> -		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
>> +		vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_vpu_reset_handler,
>>  				    dev, rst_id);
>>  		break;
>> +#else
>> +		mtk_v4l2_err("no VPU support in this kernel");
>> +		return ERR_PTR(-ENODEV);
>> +#endif
>>  	case SCP:
>> +#if IS_ENABLED(CONFIG_MTK_SCP)
>>  		ops = &mtk_vcodec_rproc_msg;
>>  		scp = scp_get(dev->plat_dev);
>>  		if (!scp) {
>> @@ -160,6 +187,10 @@ struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
>>  			return ERR_PTR(-EPROBE_DEFER);
>>  		}
>>  		break;
>> +#else
>> +		mtk_v4l2_err("no SCP support in this kernel");
>> +		return ERR_PTR(-ENODEV);
>> +#endif
>>  	default:
>>  		mtk_v4l2_err("invalid vcodec fw type");
>>  		return ERR_PTR(-EINVAL);
>> @@ -180,14 +211,7 @@ EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
>>  
>>  void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
>>  {
>> -	switch (fw->type) {
>> -	case VPU:
>> -		put_device(&fw->pdev->dev);
>> -		break;
>> -	case SCP:
>> -		scp_put(fw->scp);
>> -		break;
>> -	}
>> +	fw->ops->release(fw);
>>  }
>>  EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
>>  
>>
> 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-08 13:12     ` Hans Verkuil
@ 2020-10-08 14:02       ` Alexandre Courbot
  -1 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-08 14:02 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa,
	Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support, Randy Dunlap

Hi Hans, thanks for taking the time to look at this!

On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 08/10/2020 15:07, Hans Verkuil wrote:
> > Hi Alexandre,
> >
> > On 04/10/2020 14:22, 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
> >> ---
> >>  drivers/media/platform/Kconfig                | 10 +--
> >>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
> >>  2 files changed, 54 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >> index a3cb104956d5..98eb62e49ec2 100644
> >> --- a/drivers/media/platform/Kconfig
> >> +++ b/drivers/media/platform/Kconfig
> >> @@ -253,14 +253,16 @@ 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
> >
> > Close, but no cigar.
> >
> > 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.

But doesn't it mean that the driver can be compiled if !MTK_SCP and
!VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-08 14:02       ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-08 14:02 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Andrew-CT Chen, Randy Dunlap, LKML, Tomasz Figa,
	moderated list:ARM/Mediatek SoC support, Sakari Ailus,
	Tiffany Lin, Linux Media Mailing List

Hi Hans, thanks for taking the time to look at this!

On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 08/10/2020 15:07, Hans Verkuil wrote:
> > Hi Alexandre,
> >
> > On 04/10/2020 14:22, 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
> >> ---
> >>  drivers/media/platform/Kconfig                | 10 +--
> >>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
> >>  2 files changed, 54 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >> index a3cb104956d5..98eb62e49ec2 100644
> >> --- a/drivers/media/platform/Kconfig
> >> +++ b/drivers/media/platform/Kconfig
> >> @@ -253,14 +253,16 @@ 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
> >
> > Close, but no cigar.
> >
> > 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.

But doesn't it mean that the driver can be compiled if !MTK_SCP and
!VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-08 14:02       ` Alexandre Courbot
@ 2020-10-08 16:13         ` Hans Verkuil
  -1 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 16:13 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa,
	Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support, Randy Dunlap

On 08/10/2020 16:02, Alexandre Courbot wrote:
> Hi Hans, thanks for taking the time to look at this!
> 
> On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 08/10/2020 15:07, Hans Verkuil wrote:
>>> Hi Alexandre,
>>>
>>> On 04/10/2020 14:22, 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
>>>> ---
>>>>  drivers/media/platform/Kconfig                | 10 +--
>>>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>>>>  2 files changed, 54 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index a3cb104956d5..98eb62e49ec2 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -253,14 +253,16 @@ 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
>>>
>>> Close, but no cigar.
>>>
>>> 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.
> 
> But doesn't it mean that the driver can be compiled if !MTK_SCP and
> !VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.

No, because you still have:

	depends on VIDEO_MEDIATEK_VPU || MTK_SCP

So at least one of these must be set.

Just try it :-)

Regards,

	Hans

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-08 16:13         ` Hans Verkuil
  0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-10-08 16:13 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Andrew-CT Chen, Randy Dunlap, LKML, Tomasz Figa,
	moderated list:ARM/Mediatek SoC support, Sakari Ailus,
	Tiffany Lin, Linux Media Mailing List

On 08/10/2020 16:02, Alexandre Courbot wrote:
> Hi Hans, thanks for taking the time to look at this!
> 
> On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 08/10/2020 15:07, Hans Verkuil wrote:
>>> Hi Alexandre,
>>>
>>> On 04/10/2020 14:22, 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
>>>> ---
>>>>  drivers/media/platform/Kconfig                | 10 +--
>>>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
>>>>  2 files changed, 54 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index a3cb104956d5..98eb62e49ec2 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -253,14 +253,16 @@ 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
>>>
>>> Close, but no cigar.
>>>
>>> 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.
> 
> But doesn't it mean that the driver can be compiled if !MTK_SCP and
> !VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.

No, because you still have:

	depends on VIDEO_MEDIATEK_VPU || MTK_SCP

So at least one of these must be set.

Just try it :-)

Regards,

	Hans

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-08 16:13         ` Hans Verkuil
@ 2020-10-09  4:30           ` Alexandre Courbot
  -1 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-09  4:30 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Tiffany Lin, Andrew-CT Chen, Sakari Ailus, Tomasz Figa,
	Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support, Randy Dunlap

On Fri, Oct 9, 2020 at 1:13 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 08/10/2020 16:02, Alexandre Courbot wrote:
> > Hi Hans, thanks for taking the time to look at this!
> >
> > On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 08/10/2020 15:07, Hans Verkuil wrote:
> >>> Hi Alexandre,
> >>>
> >>> On 04/10/2020 14:22, 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
> >>>> ---
> >>>>  drivers/media/platform/Kconfig                | 10 +--
> >>>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
> >>>>  2 files changed, 54 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >>>> index a3cb104956d5..98eb62e49ec2 100644
> >>>> --- a/drivers/media/platform/Kconfig
> >>>> +++ b/drivers/media/platform/Kconfig
> >>>> @@ -253,14 +253,16 @@ 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
> >>>
> >>> Close, but no cigar.
> >>>
> >>> 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.
> >
> > But doesn't it mean that the driver can be compiled if !MTK_SCP and
> > !VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.
>
> No, because you still have:
>
>         depends on VIDEO_MEDIATEK_VPU || MTK_SCP
>
> So at least one of these must be set.
>
> Just try it :-)

Aha, I misread your message and thought you suggested replacing the
dependencies with these two lines. In this case it would certainly
work! Thanks for the suggestion, I'll send a v3 soon.

>
> Regards,
>
>         Hans

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
@ 2020-10-09  4:30           ` Alexandre Courbot
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-09  4:30 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Andrew-CT Chen, Randy Dunlap, LKML, Tomasz Figa,
	moderated list:ARM/Mediatek SoC support, Sakari Ailus,
	Tiffany Lin, Linux Media Mailing List

On Fri, Oct 9, 2020 at 1:13 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 08/10/2020 16:02, Alexandre Courbot wrote:
> > Hi Hans, thanks for taking the time to look at this!
> >
> > On Thu, Oct 8, 2020 at 10:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> On 08/10/2020 15:07, Hans Verkuil wrote:
> >>> Hi Alexandre,
> >>>
> >>> On 04/10/2020 14:22, 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
> >>>> ---
> >>>>  drivers/media/platform/Kconfig                | 10 +--
> >>>>  .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 72 ++++++++++++-------
> >>>>  2 files changed, 54 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >>>> index a3cb104956d5..98eb62e49ec2 100644
> >>>> --- a/drivers/media/platform/Kconfig
> >>>> +++ b/drivers/media/platform/Kconfig
> >>>> @@ -253,14 +253,16 @@ 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
> >>>
> >>> Close, but no cigar.
> >>>
> >>> 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.
> >
> > But doesn't it mean that the driver can be compiled if !MTK_SCP and
> > !VIDEO_MEDIATEK_VPU? That's the one case we want to avoid.
>
> No, because you still have:
>
>         depends on VIDEO_MEDIATEK_VPU || MTK_SCP
>
> So at least one of these must be set.
>
> Just try it :-)

Aha, I misread your message and thought you suggested replacing the
dependencies with these two lines. In this case it would certainly
work! Thanks for the suggestion, I'll send a v3 soon.

>
> Regards,
>
>         Hans

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
       [not found]           ` <20201009083350.6c2e5a6a@coco.lan>
@ 2020-10-12  4:58             ` Alexandre Courbot
  2020-10-12  7:28               ` Mauro Carvalho Chehab
  0 siblings, 1 reply; 20+ messages in thread
From: Alexandre Courbot @ 2020-10-12  4:58 UTC (permalink / raw)
  To: Mauro Carvalho Chehab
  Cc: Hans Verkuil, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa, Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support
	<linux-mediatek@lists.infradead.org>,
	Randy Dunlap

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?

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.

>
> 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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled
  2020-10-12  4:58             ` Alexandre Courbot
@ 2020-10-12  7:28               ` Mauro Carvalho Chehab
  0 siblings, 0 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2020-10-12  7:28 UTC (permalink / raw)
  To: Alexandre Courbot
  Cc: Hans Verkuil, Tiffany Lin, Andrew-CT Chen, Sakari Ailus,
	Tomasz Figa, Linux Media Mailing List, LKML,
	moderated list:ARM/Mediatek SoC support
	<linux-mediatek@lists.infradead.org>,
	Randy Dunlap

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-10-12  7:29 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-04 12:22 [PATCH v2] media: mtk-vcodec: fix builds when remoteproc is disabled Alexandre Courbot
2020-10-04 12:22 ` Alexandre Courbot
2020-10-05  3:32 ` Alexandre Courbot
2020-10-05  3:32   ` Alexandre Courbot
2020-10-05  8:48 ` Sakari Ailus
2020-10-05  8:48   ` Sakari Ailus
2020-10-05 11:29   ` Alexandre Courbot
2020-10-05 11:29     ` Alexandre Courbot
2020-10-08 13:07 ` Hans Verkuil
2020-10-08 13:07   ` Hans Verkuil
2020-10-08 13:12   ` Hans Verkuil
2020-10-08 13:12     ` Hans Verkuil
2020-10-08 14:02     ` Alexandre Courbot
2020-10-08 14:02       ` Alexandre Courbot
2020-10-08 16:13       ` Hans Verkuil
2020-10-08 16:13         ` Hans Verkuil
2020-10-09  4:30         ` Alexandre Courbot
2020-10-09  4:30           ` Alexandre Courbot
     [not found]           ` <20201009083350.6c2e5a6a@coco.lan>
2020-10-12  4:58             ` Alexandre Courbot
2020-10-12  7:28               ` Mauro Carvalho Chehab

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.