[PATCH V6, 0/5] Support multi-hardware jpeg encoding using of_platform_populate

[PATCH V6, 0/5] Support multi-hardware jpeg encoding using of_platform_populate

[kyrie.wu]

This series adds support for multi hardware jpeg encoding, by first
adding use of_platform_populate to manage each hardware information:
interrupt, clock, register bases and power. Secondly add encoding 
work queue to deal with the encoding requestsof multi-hardware
at the same time. Lastly, add output picture reorder function
interface to eliminate the out of order images.

This series has been tested with both MT8195.
Encoding worked for this chip.

Patches 1~2 use of_platform_populate to replace component framework
to manage multi-hardware.

Patch 3 add jpeg encoding timeout function to judge hardware timeout.

Patch 4 add encoding work queue to deal with multi-hardware encoding
at the same time.

Patch 5 add output picture reorder function to order images.
---
Changes compared with v5:
- use of_platform_populate to replace component framework to
manage multi-hardware in patch 2.

Changes compared with v4:
--No change compaered with v4

Changes compared with v3:
--Structure patches for consistency, non-backward
  compatible and do not break any existing functionality

Changes compared with v2:
--Split the last two patches into several patches
  to enhance readability
--Correct some syntax errors
--Explain why the component framework is used

Changes compared with v1:
--Add jpeg encoder dt-bindings for MT8195
--Use component framework to manage jpegenc HW
--Add jpegenc output pic reorder function interface

kyrie.wu (5):
  dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
  media: mtk-jpegenc: manage jpegenc multi-hardware
  media: mtk-jpegenc: add jpegenc timeout func interface
  media: mtk-jpegenc: add jpeg encode worker interface
  media: mtk-jpegenc: add output pic reorder interface

 .../bindings/media/mediatek-jpeg-encoder.yaml      |   3 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c    | 287 +++++++++++++++----
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h    |  91 +++++-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c  |   1 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h  |   3 +-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c  | 316 ++++++++++++++++++++-
 6 files changed, 644 insertions(+), 57 deletions(-)

-- 
2.6.4


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

[PATCH V6, 1/5] dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible

[kyrie.wu]

Add mediatek, mt8195-jpgenc compatible to binding document

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
V6: no change
---
 Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
index fcd9b82..e93ccf5 100644
--- a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
+++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
@@ -18,6 +18,9 @@ properties:
       - enum:
           - mediatek,mt2701-jpgenc
           - mediatek,mt8183-jpgenc
+          - mediatek,mt8195-jpgenc
+          - mediatek,mt8195-jpgenc0
+          - mediatek,mt8195-jpgenc1
       - const: mediatek,mtk-jpgenc
   reg:
     maxItems: 1
-- 
2.6.4


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

Re: [PATCH V6, 1/5] dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible

[Ricardo Ribalda]

kyrie.wu wrote:

> Add mediatek, mt8195-jpgenc compatible to binding document
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change

Hi Kyrie

Your patch does not seem to apply on top of linus or linus/next.

In linus/next they still have the .txt file:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt?id=7afeac307a9561e3a93682c1e7eb22f918aa1187

What are you using?

Thanks


> ---
>  Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> index fcd9b82..e93ccf5 100644
> --- a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml
> @@ -18,6 +18,9 @@ properties:
>        - enum:
>            - mediatek,mt2701-jpgenc
>            - mediatek,mt8183-jpgenc
> +          - mediatek,mt8195-jpgenc
> +          - mediatek,mt8195-jpgenc0
> +          - mediatek,mt8195-jpgenc1
>        - const: mediatek,mtk-jpgenc
>    reg:
>      maxItems: 1
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

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

Re: [PATCH V6, 1/5] dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible

[kyrie.wu]

On Fri, 2021-12-03 at 14:43 +0100, Ricardo Ribalda wrote:
> kyrie.wu wrote:
> 
> > Add mediatek, mt8195-jpgenc compatible to binding document
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: no change
> 
> Hi Kyrie
> 
> Your patch does not seem to apply on top of linus or linus/next.
> 
> In linus/next they still have the .txt file:
> 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt?id=7afeac307a9561e3a93682c1e7eb22f918aa1187
> 
> What are you using?
> 
> Thanks
I will fix this issue in the next code version, thanks.
> 
> 
> > ---
> >  Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.yaml 
> > | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-
> > encoder.yaml b/Documentation/devicetree/bindings/media/mediatek-
> > jpeg-encoder.yaml
> > index fcd9b82..e93ccf5 100644
> > --- a/Documentation/devicetree/bindings/media/mediatek-jpeg-
> > encoder.yaml
> > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-
> > encoder.yaml
> > @@ -18,6 +18,9 @@ properties:
> >        - enum:
> >            - mediatek,mt2701-jpgenc
> >            - mediatek,mt8183-jpgenc
> > +          - mediatek,mt8195-jpgenc
> > +          - mediatek,mt8195-jpgenc0
> > +          - mediatek,mt8195-jpgenc1
> >        - const: mediatek,mtk-jpgenc
> >    reg:
> >      maxItems: 1
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

[kyrie.wu]

manage each hardware information, including irq/clk/power.
the hardware includes HW0 and HW1.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
V6: use of_platform_populate to replace component framework
to manage multi-hardware
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232 +++++++++++++++++++++-
 3 files changed, 362 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index a89c7b2..da7eb84 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 	spin_lock_init(&jpeg->hw_lock);
 	jpeg->dev = &pdev->dev;
 	jpeg->variant = of_device_get_match_data(jpeg->dev);
-	INIT_DELAYED_WORK(&jpeg->job_timeout_work, mtk_jpeg_job_timeout_work);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(jpeg->reg_base)) {
-		ret = PTR_ERR(jpeg->reg_base);
-		return ret;
-	}
+	if (!jpeg->variant->is_encoder) {
+		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
+				mtk_jpeg_job_timeout_work);
 
-	jpeg_irq = platform_get_irq(pdev, 0);
-	if (jpeg_irq < 0) {
-		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n", jpeg_irq);
-		return jpeg_irq;
-	}
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(jpeg->reg_base)) {
+			ret = PTR_ERR(jpeg->reg_base);
+			return ret;
+		}
 
-	ret = devm_request_irq(&pdev->dev, jpeg_irq,
-			       jpeg->variant->irq_handler, 0, pdev->name, jpeg);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
-			jpeg_irq, ret);
-		goto err_req_irq;
-	}
+		jpeg_irq = platform_get_irq(pdev, 0);
+		if (jpeg_irq < 0) {
+			dev_err(&pdev->dev, "Failed to get jpeg_irq.\n");
+			return jpeg_irq;
+		}
 
-	ret = mtk_jpeg_clk_init(jpeg);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to init clk, err %d\n", ret);
-		goto err_clk_init;
+		ret = devm_request_irq(&pdev->dev, jpeg_irq,
+				       jpeg->variant->irq_handler,
+					   0, pdev->name, jpeg);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
+				jpeg_irq, ret);
+			goto err_req_irq;
+		}
+
+		ret = mtk_jpeg_clk_init(jpeg);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to init clk(%d)\n", ret);
+			goto err_clk_init;
+		}
+
+		pm_runtime_enable(&pdev->dev);
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
@@ -1426,9 +1433,16 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 		  jpeg->variant->dev_name, jpeg->vdev->num,
 		  VIDEO_MAJOR, jpeg->vdev->minor);
 
-	platform_set_drvdata(pdev, jpeg);
+	if (jpeg->variant->is_encoder) {
+		ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
+			&pdev->dev);
+		if (ret) {
+			v4l2_err(&jpeg->v4l2_dev, "Master of platform populate failed.");
+			goto err_vfd_jpeg_register;
+		}
+	}
 
-	pm_runtime_enable(&pdev->dev);
+	platform_set_drvdata(pdev, jpeg);
 
 	return 0;
 
@@ -1539,6 +1553,19 @@ static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
 	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
 };
 
+static const struct mtk_jpeg_variant mtk_jpegenc_drvdata = {
+	.is_encoder = true,
+	.formats = mtk_jpeg_enc_formats,
+	.num_formats = MTK_JPEG_ENC_NUM_FORMATS,
+	.qops = &mtk_jpeg_enc_qops,
+	.m2m_ops = &mtk_jpeg_enc_m2m_ops,
+	.dev_name = "mtk-jpeg-enc",
+	.ioctl_ops = &mtk_jpeg_enc_ioctl_ops,
+	.out_q_default_fourcc = V4L2_PIX_FMT_YUYV,
+	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
+};
+
+#if defined(CONFIG_OF)
 static const struct of_device_id mtk_jpeg_match[] = {
 	{
 		.compatible = "mediatek,mt8173-jpgdec",
@@ -1552,10 +1579,14 @@ static const struct of_device_id mtk_jpeg_match[] = {
 		.compatible = "mediatek,mtk-jpgenc",
 		.data = &mtk_jpeg_drvdata,
 	},
+	{
+		.compatible = "mediatek,mt8195-jpgenc",
+		.data = &mtk_jpegenc_drvdata,
+	},
 	{},
 };
-
 MODULE_DEVICE_TABLE(of, mtk_jpeg_match);
+#endif
 
 static struct platform_driver mtk_jpeg_driver = {
 	.probe = mtk_jpeg_probe,
@@ -1567,7 +1598,25 @@ static struct platform_driver mtk_jpeg_driver = {
 	},
 };
 
-module_platform_driver(mtk_jpeg_driver);
+static struct platform_driver * const mtk_jpeg_source_drivers[] = {
+	&mtk_jpegenc_hw_driver,
+	&mtk_jpeg_driver,
+};
+static int __init mtk_jpeg_init(void)
+{
+	return platform_register_drivers(mtk_jpeg_source_drivers,
+		ARRAY_SIZE(mtk_jpeg_source_drivers));
+}
+
+static void __exit mtk_jpeg_exit(void)
+{
+	platform_unregister_drivers(mtk_jpeg_source_drivers,
+		ARRAY_SIZE(mtk_jpeg_source_drivers));
+}
+
+module_init(mtk_jpeg_init);
+module_exit(mtk_jpeg_exit);
+
 
 MODULE_DESCRIPTION("MediaTek JPEG codec driver");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 595f7f1..7d013de 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -60,6 +60,7 @@ enum mtk_jpeg_ctx_state {
  * @cap_q_default_fourcc:	capture queue default fourcc
  */
 struct mtk_jpeg_variant {
+	bool is_encoder;
 	struct clk_bulk_data *clks;
 	int num_clks;
 	struct mtk_jpeg_fmt *formats;
@@ -74,6 +75,55 @@ struct mtk_jpeg_variant {
 	u32 cap_q_default_fourcc;
 };
 
+enum mtk_jpegenc_hw_id {
+	MTK_JPEGENC_HW0,
+	MTK_JPEGENC_HW1,
+	MTK_JPEGENC_HW_MAX,
+};
+
+/**
+ * struct mtk_jpegenc_clk_info - Structure used to store clock name
+ */
+struct mtk_jpegenc_clk_info {
+	const char	*clk_name;
+	struct clk	*jpegenc_clk;
+};
+
+/**
+ * struct mtk_vcodec_clk - Structure used to store vcodec clock information
+ */
+struct mtk_jpegenc_clk {
+	struct mtk_jpegenc_clk_info	*clk_info;
+	int	clk_num;
+};
+
+/**
+ * struct mtk_vcodec_pm - Power management data structure
+ */
+struct mtk_jpegenc_pm {
+	struct mtk_jpegenc_clk	venc_clk;
+	struct device	*dev;
+	struct mtk_jpegenc_comp_dev	*mtkdev;
+};
+
+/**
+ * struct mtk_jpegenc_comp_dev - JPEG COREX abstraction
+ * @dev:		        JPEG device
+ * @plat_dev:		    platform device data
+ * @reg_base:		    JPEG registers mapping
+ * @master_dev:		    mtk_jpeg_dev device
+ * @pm:	                mtk_jpegenc_pm
+ * @jpegenc_irq:	    jpeg encode irq num
+ */
+struct mtk_jpegenc_comp_dev {
+	struct device		*dev;
+	struct platform_device *plat_dev;
+	void __iomem		*reg_base;
+	struct mtk_jpeg_dev *master_dev;
+	struct mtk_jpegenc_pm pm;
+	int jpegenc_irq;
+};
+
 /**
  * struct mtk_jpeg_dev - JPEG IP abstraction
  * @lock:		the mutex protecting this structure
@@ -102,6 +152,9 @@ struct mtk_jpeg_dev {
 	struct device		*larb;
 	struct delayed_work job_timeout_work;
 	const struct mtk_jpeg_variant *variant;
+
+	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
+	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
 };
 
 /**
@@ -162,4 +215,6 @@ struct mtk_jpeg_ctx {
 	struct v4l2_ctrl_handler ctrl_hdl;
 };
 
+extern struct platform_driver mtk_jpegenc_hw_driver;
+
 #endif /* _MTK_JPEG_CORE_H */
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
index 1cf037b..4ccda1d 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -4,12 +4,27 @@
  * Author: Xia Jiang <xia.jiang@mediatek.com>
  *
  */
-
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <media/media-device.h>
 #include <media/videobuf2-core.h>
 #include <media/videobuf2-dma-contig.h>
+#include <media/videobuf2-v4l2.h>
+#include <media/v4l2-mem2mem.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
+#include "mtk_jpeg_core.h"
 #include "mtk_jpeg_enc_hw.h"
 
 static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
@@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
 	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
 };
 
+#if defined(CONFIG_OF)
+static const struct of_device_id mtk_jpegenc_drv_ids[] = {
+	{
+		.compatible = "mediatek,mt8195-jpgenc0",
+		.data = (void *)MTK_JPEGENC_HW0,
+	},
+	{
+		.compatible = "mediatek,mt8195-jpgenc1",
+		.data = (void *)MTK_JPEGENC_HW1,
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
+#endif
+
 void mtk_jpeg_enc_reset(void __iomem *base)
 {
 	writel(0, base + JPEG_ENC_RSTB);
@@ -152,3 +182,203 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
 
 	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
 }
+
+static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
+{
+	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+	enum vb2_buffer_state buf_state;
+	struct mtk_jpeg_ctx *ctx;
+	u32 result_size;
+	u32 irq_status;
+
+	struct mtk_jpegenc_comp_dev *jpeg = priv;
+	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
+
+	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
+		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
+	if (irq_status)
+		writel(0, jpeg->reg_base + JPEG_ENC_INT_STS);
+	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE))
+		return IRQ_NONE;
+
+	ctx = v4l2_m2m_get_curr_priv(master_jpeg->m2m_dev);
+	if (!ctx) {
+		v4l2_err(&master_jpeg->v4l2_dev, "Context is NULL\n");
+		return IRQ_HANDLED;
+	}
+
+	src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
+	result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base);
+	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
+	buf_state = VB2_BUF_STATE_DONE;
+	v4l2_m2m_buf_done(src_buf, buf_state);
+	v4l2_m2m_buf_done(dst_buf, buf_state);
+	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
+	pm_runtime_put(ctx->jpeg->dev);
+
+	return IRQ_HANDLED;
+}
+
+static int mtk_jpegenc_hw_init_irq(struct mtk_jpegenc_comp_dev *dev)
+{
+	struct platform_device *pdev = dev->plat_dev;
+	int ret;
+
+	dev->jpegenc_irq = platform_get_irq(pdev, 0);
+	if (dev->jpegenc_irq < 0) {
+		dev_err(&pdev->dev, "Failed to get irq resource");
+		return dev->jpegenc_irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, dev->jpegenc_irq,
+		mtk_jpegenc_hw_irq_handler, 0, pdev->name, dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to devm_request_irq %d (%d)",
+			dev->jpegenc_irq, ret);
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
+int mtk_jpegenc_init_pm(struct mtk_jpegenc_comp_dev *mtkdev)
+{
+	struct mtk_jpegenc_clk_info *clk_info;
+	struct mtk_jpegenc_clk *jpegenc_clk;
+	struct platform_device *pdev;
+	struct mtk_jpegenc_pm *pm;
+	int i, ret;
+
+	pdev = mtkdev->plat_dev;
+	pm = &mtkdev->pm;
+	pm->dev = &pdev->dev;
+	pm->mtkdev = mtkdev;
+	jpegenc_clk = &pm->venc_clk;
+	jpegenc_clk->clk_num =
+		of_property_count_strings(pdev->dev.of_node, "clock-names");
+	if (!jpegenc_clk->clk_num) {
+		dev_err(&pdev->dev, "Failed to get jpegenc clock count\n");
+		return -EINVAL;
+	}
+
+	jpegenc_clk->clk_info = devm_kcalloc(&pdev->dev,
+		jpegenc_clk->clk_num,
+		sizeof(*clk_info),
+		GFP_KERNEL);
+	if (!jpegenc_clk->clk_info)
+		return -ENOMEM;
+
+	for (i = 0; i < jpegenc_clk->clk_num; i++) {
+		clk_info = &jpegenc_clk->clk_info[i];
+		ret = of_property_read_string_index(pdev->dev.of_node,
+			"clock-names", i,
+			&clk_info->clk_name);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to get jpegenc clock name\n");
+			return ret;
+		}
+
+		clk_info->jpegenc_clk = devm_clk_get(&pdev->dev,
+			clk_info->clk_name);
+		if (IS_ERR(clk_info->jpegenc_clk)) {
+			dev_err(&pdev->dev, "devm_clk_get (%d)%s fail",
+				i, clk_info->clk_name);
+			return PTR_ERR(clk_info->jpegenc_clk);
+		}
+	}
+
+	pm_runtime_enable(&pdev->dev);
+
+	return ret;
+}
+
+void mtk_jpegenc_release_pm(struct mtk_jpegenc_comp_dev *mtkdev)
+{
+	struct platform_device *pdev = mtkdev->plat_dev;
+
+	pm_runtime_disable(&pdev->dev);
+}
+
+static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
+{
+	struct mtk_jpeg_dev *master_dev;
+	const struct of_device_id *of_id;
+	struct mtk_jpegenc_comp_dev *dev;
+	int ret, comp_idx;
+
+	struct device *decs = &pdev->dev;
+
+	if (!decs->parent)
+		return -EPROBE_DEFER;
+
+	master_dev = dev_get_drvdata(decs->parent);
+	if (!master_dev)
+		return -EPROBE_DEFER;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->plat_dev = pdev;
+
+	ret = mtk_jpegenc_init_pm(dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
+		return ret;
+	}
+
+	dev->reg_base =
+		devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(dev->reg_base)) {
+		ret = PTR_ERR(dev->reg_base);
+		goto err;
+	}
+
+	ret = mtk_jpegenc_hw_init_irq(dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register JPEGENC irq handler.\n");
+		goto err;
+	}
+
+	of_id = of_match_device(mtk_jpegenc_drv_ids, decs);
+	if (!of_id) {
+		dev_err(&pdev->dev, "Can't get vdec comp device id.\n");
+		ret = -EINVAL;
+		goto err;
+	}
+
+	comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
+	if (comp_idx < MTK_JPEGENC_HW_MAX) {
+		master_dev->enc_hw_dev[comp_idx] = dev;
+		master_dev->reg_encbase[comp_idx] = dev->reg_base;
+		dev->master_dev = master_dev;
+	}
+
+	platform_set_drvdata(pdev, dev);
+
+	return 0;
+
+err:
+	mtk_jpegenc_release_pm(dev);
+	return ret;
+}
+
+
+static int mtk_jpegenc_remove(struct platform_device *pdev)
+{
+	struct mtk_jpegenc_comp_dev *dev = platform_get_drvdata(pdev);
+
+	mtk_jpegenc_release_pm(dev);
+
+	return 0;
+}
+
+struct platform_driver mtk_jpegenc_hw_driver = {
+	.probe = mtk_jpegenc_hw_probe,
+	.remove = mtk_jpegenc_remove,
+	.driver = {
+		.name = "mtk-jpegenc-hw",
+		.of_match_table = of_match_ptr(mtk_jpegenc_drv_ids),
+	},
+};
-- 
2.6.4


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

Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

[Ricardo Ribalda]

kyrie.wu wrote:

> manage each hardware information, including irq/clk/power.
> the hardware includes HW0 and HW1.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: use of_platform_populate to replace component framework
> to manage multi-hardware
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232 +++++++++++++++++++++-
>  3 files changed, 362 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index a89c7b2..da7eb84 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  	spin_lock_init(&jpeg->hw_lock);
>  	jpeg->dev = &pdev->dev;
>  	jpeg->variant = of_device_get_match_data(jpeg->dev);
> -	INIT_DELAYED_WORK(&jpeg->job_timeout_work, mtk_jpeg_job_timeout_work);
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(jpeg->reg_base)) {
> -		ret = PTR_ERR(jpeg->reg_base);
> -		return ret;
> -	}
> +	if (!jpeg->variant->is_encoder) {
> +		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> +				mtk_jpeg_job_timeout_work);
>  
> -	jpeg_irq = platform_get_irq(pdev, 0);
> -	if (jpeg_irq < 0) {
> -		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n", jpeg_irq);
> -		return jpeg_irq;
> -	}
Maybe better use  devm_platform_ioremap_resource()
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(jpeg->reg_base)) {
> +			ret = PTR_ERR(jpeg->reg_base);
> +			return ret;
> +		}
>  
> -	ret = devm_request_irq(&pdev->dev, jpeg_irq,
> -			       jpeg->variant->irq_handler, 0, pdev->name, jpeg);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
> -			jpeg_irq, ret);
> -		goto err_req_irq;
> -	}
> +		jpeg_irq = platform_get_irq(pdev, 0);

> +		if (jpeg_irq < 0) {
> +			dev_err(&pdev->dev, "Failed to get jpeg_irq.\n");
> +			return jpeg_irq;
> +		}
>  
> -	ret = mtk_jpeg_clk_init(jpeg);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to init clk, err %d\n", ret);
> -		goto err_clk_init;
> +		ret = devm_request_irq(&pdev->dev, jpeg_irq,
> +				       jpeg->variant->irq_handler,
> +					   0, pdev->name, jpeg);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
> +				jpeg_irq, ret);
> +			goto err_req_irq;
> +		}
> +
> +		ret = mtk_jpeg_clk_init(jpeg);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to init clk(%d)\n", ret);
> +			goto err_clk_init;
> +		}
> +
> +		pm_runtime_enable(&pdev->dev);
>  	}
>  
>  	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> @@ -1426,9 +1433,16 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  		  jpeg->variant->dev_name, jpeg->vdev->num,
>  		  VIDEO_MAJOR, jpeg->vdev->minor);
>  
> -	platform_set_drvdata(pdev, jpeg);
> +	if (jpeg->variant->is_encoder) {
Can you share a device tree that uses this? 

You can't you have all the device in same level, instead of making them
childs of this device?
> +		ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
> +			&pdev->dev);
Maybe you want devm_of_platform_populate()?
> +		if (ret) {
> +			v4l2_err(&jpeg->v4l2_dev, "Master of platform populate failed.");
> +			goto err_vfd_jpeg_register;
> +		}
> +	}
>  
> -	pm_runtime_enable(&pdev->dev);
> +	platform_set_drvdata(pdev, jpeg);
>  
>  	return 0;
>  
> @@ -1539,6 +1553,19 @@ static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
>  	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
>  };
>  
> +static const struct mtk_jpeg_variant mtk_jpegenc_drvdata = {
> +	.is_encoder = true,
> +	.formats = mtk_jpeg_enc_formats,
> +	.num_formats = MTK_JPEG_ENC_NUM_FORMATS,
> +	.qops = &mtk_jpeg_enc_qops,
> +	.m2m_ops = &mtk_jpeg_enc_m2m_ops,
> +	.dev_name = "mtk-jpeg-enc",
> +	.ioctl_ops = &mtk_jpeg_enc_ioctl_ops,
> +	.out_q_default_fourcc = V4L2_PIX_FMT_YUYV,
> +	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
> +};
> +
> +#if defined(CONFIG_OF)
>  static const struct of_device_id mtk_jpeg_match[] = {
>  	{
>  		.compatible = "mediatek,mt8173-jpgdec",
> @@ -1552,10 +1579,14 @@ static const struct of_device_id mtk_jpeg_match[] = {
>  		.compatible = "mediatek,mtk-jpgenc",
>  		.data = &mtk_jpeg_drvdata,
>  	},
> +	{
> +		.compatible = "mediatek,mt8195-jpgenc",
> +		.data = &mtk_jpegenc_drvdata,
> +	},
>  	{},
>  };
> -
>  MODULE_DEVICE_TABLE(of, mtk_jpeg_match);
> +#endif
>  
>  static struct platform_driver mtk_jpeg_driver = {
>  	.probe = mtk_jpeg_probe,
> @@ -1567,7 +1598,25 @@ static struct platform_driver mtk_jpeg_driver = {
>  	},
>  };

This is not very clear. It would be better to refactor the change and
have a module_platform_driver() macro for jpegenc_hw_driver, where its
probe and remove exists.
> 
> -module_platform_driver(mtk_jpeg_driver);
> +static struct platform_driver * const mtk_jpeg_source_drivers[] = {
> +	&mtk_jpegenc_hw_driver,
> +	&mtk_jpeg_driver,
> +};
> +static int __init mtk_jpeg_init(void)
> +{
> +	return platform_register_drivers(mtk_jpeg_source_drivers,
> +		ARRAY_SIZE(mtk_jpeg_source_drivers));
> +}
> +
> +static void __exit mtk_jpeg_exit(void)
> +{
> +	platform_unregister_drivers(mtk_jpeg_source_drivers,
> +		ARRAY_SIZE(mtk_jpeg_source_drivers));
> +}
> +
> +module_init(mtk_jpeg_init);
> +module_exit(mtk_jpeg_exit);
> +
>  
>  MODULE_DESCRIPTION("MediaTek JPEG codec driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 595f7f1..7d013de 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -60,6 +60,7 @@ enum mtk_jpeg_ctx_state {
>   * @cap_q_default_fourcc:	capture queue default fourcc
>   */
>  struct mtk_jpeg_variant {
> +	bool is_encoder;
>  	struct clk_bulk_data *clks;
>  	int num_clks;
>  	struct mtk_jpeg_fmt *formats;
> @@ -74,6 +75,55 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +enum mtk_jpegenc_hw_id {
> +	MTK_JPEGENC_HW0,
> +	MTK_JPEGENC_HW1,
> +	MTK_JPEGENC_HW_MAX,
> +};
> +
> +/**
> + * struct mtk_jpegenc_clk_info - Structure used to store clock name
> + */
> +struct mtk_jpegenc_clk_info {
> +	const char	*clk_name;
> +	struct clk	*jpegenc_clk;
> +};
> +
> +/**
> + * struct mtk_vcodec_clk - Structure used to store vcodec clock information
> + */
> +struct mtk_jpegenc_clk {
> +	struct mtk_jpegenc_clk_info	*clk_info;
> +	int	clk_num;
> +};
> +
> +/**
> + * struct mtk_vcodec_pm - Power management data structure
> + */
> +struct mtk_jpegenc_pm {
> +	struct mtk_jpegenc_clk	venc_clk;
> +	struct device	*dev;
> +	struct mtk_jpegenc_comp_dev	*mtkdev;
> +};
> +
> +/**
> + * struct mtk_jpegenc_comp_dev - JPEG COREX abstraction
> + * @dev:		        JPEG device
> + * @plat_dev:		    platform device data
> + * @reg_base:		    JPEG registers mapping
> + * @master_dev:		    mtk_jpeg_dev device
> + * @pm:	                mtk_jpegenc_pm
> + * @jpegenc_irq:	    jpeg encode irq num
> + */
> +struct mtk_jpegenc_comp_dev {
> +	struct device		*dev;
> +	struct platform_device *plat_dev;
> +	void __iomem		*reg_base;
> +	struct mtk_jpeg_dev *master_dev;
> +	struct mtk_jpegenc_pm pm;
> +	int jpegenc_irq;
> +};
> +
>  /**
>   * struct mtk_jpeg_dev - JPEG IP abstraction
>   * @lock:		the mutex protecting this structure
> @@ -102,6 +152,9 @@ struct mtk_jpeg_dev {
>  	struct device		*larb;
>  	struct delayed_work job_timeout_work;
>  	const struct mtk_jpeg_variant *variant;
> +
> +	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
> +	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
>  };
>  
>  /**
> @@ -162,4 +215,6 @@ struct mtk_jpeg_ctx {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  };
> 
Please no extern here:
> +extern struct platform_driver mtk_jpegenc_hw_driver;
> +
>  #endif /* _MTK_JPEG_CORE_H */
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 1cf037b..4ccda1d 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -4,12 +4,27 @@
>   * Author: Xia Jiang <xia.jiang@mediatek.com>
>   *
>   */
> -
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <media/media-device.h>
>  #include <media/videobuf2-core.h>
>  #include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  
> +#include "mtk_jpeg_core.h"
>  #include "mtk_jpeg_enc_hw.h"
>  
>  static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> @@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
>  	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
>  };
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mtk_jpegenc_drv_ids[] = {
> +	{
> +		.compatible = "mediatek,mt8195-jpgenc0",
> +		.data = (void *)MTK_JPEGENC_HW0,
> +	},
> +	{
> +		.compatible = "mediatek,mt8195-jpgenc1",
> +		.data = (void *)MTK_JPEGENC_HW1,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
> +#endif
> +
>  void mtk_jpeg_enc_reset(void __iomem *base)
>  {
>  	writel(0, base + JPEG_ENC_RSTB);
> @@ -152,3 +182,203 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
>  
>  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
>  }
> +
> +static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> +{
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +	enum vb2_buffer_state buf_state;
> +	struct mtk_jpeg_ctx *ctx;
> +	u32 result_size;
> +	u32 irq_status;
> +
> +	struct mtk_jpegenc_comp_dev *jpeg = priv;
> +	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
> +
> +	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
> +		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> +	if (irq_status)
> +		writel(0, jpeg->reg_base + JPEG_ENC_INT_STS);
> +	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE))
> +		return IRQ_NONE;
> +
> +	ctx = v4l2_m2m_get_curr_priv(master_jpeg->m2m_dev);
> +	if (!ctx) {
> +		v4l2_err(&master_jpeg->v4l2_dev, "Context is NULL\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +	result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base);
> +	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> +	buf_state = VB2_BUF_STATE_DONE;
> +	v4l2_m2m_buf_done(src_buf, buf_state);
> +	v4l2_m2m_buf_done(dst_buf, buf_state);
> +	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +	pm_runtime_put(ctx->jpeg->dev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mtk_jpegenc_hw_init_irq(struct mtk_jpegenc_comp_dev *dev)
> +{
> +	struct platform_device *pdev = dev->plat_dev;
> +	int ret;
> +
> +	dev->jpegenc_irq = platform_get_irq(pdev, 0);
> +	if (dev->jpegenc_irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq resource");
> +		return dev->jpegenc_irq;
> +	}
> +
Have you tried with a threaded_irq?
> +	ret = devm_request_irq(&pdev->dev, dev->jpegenc_irq,
> +		mtk_jpegenc_hw_irq_handler, 0, pdev->name, dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to devm_request_irq %d (%d)",
> +			dev->jpegenc_irq, ret);
> +		return -ENOENT;
> +	}
> +
> +	return 0;
> +}
> +
> +int mtk_jpegenc_init_pm(struct mtk_jpegenc_comp_dev *mtkdev)
> +{
> +	struct mtk_jpegenc_clk_info *clk_info;
> +	struct mtk_jpegenc_clk *jpegenc_clk;
> +	struct platform_device *pdev;
> +	struct mtk_jpegenc_pm *pm;
> +	int i, ret;
> +
> +	pdev = mtkdev->plat_dev;
> +	pm = &mtkdev->pm;
> +	pm->dev = &pdev->dev;
> +	pm->mtkdev = mtkdev;

Can't you simplify with devm_clk_bulk_get_all
> +	jpegenc_clk = &pm->venc_clk;
> +	jpegenc_clk->clk_num =
> +		of_property_count_strings(pdev->dev.of_node, "clock-names");
> +	if (!jpegenc_clk->clk_num) {
> +		dev_err(&pdev->dev, "Failed to get jpegenc clock count\n");
> +		return -EINVAL;
> +	}
> +
> +	jpegenc_clk->clk_info = devm_kcalloc(&pdev->dev,
> +		jpegenc_clk->clk_num,
> +		sizeof(*clk_info),
> +		GFP_KERNEL);
> +	if (!jpegenc_clk->clk_info)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < jpegenc_clk->clk_num; i++) {
> +		clk_info = &jpegenc_clk->clk_info[i];
> +		ret = of_property_read_string_index(pdev->dev.of_node,
> +			"clock-names", i,
> +			&clk_info->clk_name);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to get jpegenc clock name\n");
> +			return ret;
> +		}
> +
> +		clk_info->jpegenc_clk = devm_clk_get(&pdev->dev,
> +			clk_info->clk_name);
> +		if (IS_ERR(clk_info->jpegenc_clk)) {
> +			dev_err(&pdev->dev, "devm_clk_get (%d)%s fail",
> +				i, clk_info->clk_name);
> +			return PTR_ERR(clk_info->jpegenc_clk);
> +		}
> +	}
> +
Can't you enable the pm after all the probing is completed to simplify
the error handling?
> +	pm_runtime_enable(&pdev->dev);
> +
> +	return ret;
> +}
> +
> +void mtk_jpegenc_release_pm(struct mtk_jpegenc_comp_dev *mtkdev)
> +{
> +	struct platform_device *pdev = mtkdev->plat_dev;
> +
> +	pm_runtime_disable(&pdev->dev);
> +}
> +
> +static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
> +{
> +	struct mtk_jpeg_dev *master_dev;
> +	const struct of_device_id *of_id;
> +	struct mtk_jpegenc_comp_dev *dev;
> +	int ret, comp_idx;
> +
> +	struct device *decs = &pdev->dev;
> +
When is the parent NULL?
> +	if (!decs->parent)
> +		return -EPROBE_DEFER;
> +
> +	master_dev = dev_get_drvdata(decs->parent);
> +	if (!master_dev)
> +		return -EPROBE_DEFER;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->plat_dev = pdev;
> +
> +	ret = mtk_jpegenc_init_pm(dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
> +		return ret;
> +	}
> +
> +	dev->reg_base =
> +		devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(dev->reg_base)) {
> +		ret = PTR_ERR(dev->reg_base);
> +		goto err;
> +	}
> +
> +	ret = mtk_jpegenc_hw_init_irq(dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register JPEGENC irq handler.\n");
> +		goto err;
> +	}
> +
please use of_device_get_match_data()
> +	of_id = of_match_device(mtk_jpegenc_drv_ids, decs);
> +	if (!of_id) {
> +		dev_err(&pdev->dev, "Can't get vdec comp device id.\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
What if the drvdata is freed before you set it?
> +	comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
> +	if (comp_idx < MTK_JPEGENC_HW_MAX) {
> +		master_dev->enc_hw_dev[comp_idx] = dev;
> +		master_dev->reg_encbase[comp_idx] = dev->reg_base;
> +		dev->master_dev = master_dev;
> +	}
> +
> +	platform_set_drvdata(pdev, dev);
> +
> +	return 0;
> +
> +err:
> +	mtk_jpegenc_release_pm(dev);
> +	return ret;
> +}
> +
> +
> +static int mtk_jpegenc_remove(struct platform_device *pdev)
> +{
> +	struct mtk_jpegenc_comp_dev *dev = platform_get_drvdata(pdev);
> +
> +	mtk_jpegenc_release_pm(dev);
> +
> +	return 0;
> +}
> +
> +struct platform_driver mtk_jpegenc_hw_driver = {
> +	.probe = mtk_jpegenc_hw_probe,
> +	.remove = mtk_jpegenc_remove,
> +	.driver = {
> +		.name = "mtk-jpegenc-hw",
> +		.of_match_table = of_match_ptr(mtk_jpegenc_drv_ids),
> +	},
> +};
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

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

Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

[kyrie.wu]

On Fri, 2021-12-03 at 15:54 +0100, Ricardo Ribalda wrote:
> kyrie.wu wrote:
> 
> > manage each hardware information, including irq/clk/power.
> > the hardware includes HW0 and HW1.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: use of_platform_populate to replace component framework
> > to manage multi-hardware
> > ---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232
> > +++++++++++++++++++++-
> >  3 files changed, 362 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index a89c7b2..da7eb84 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >  	spin_lock_init(&jpeg->hw_lock);
> >  	jpeg->dev = &pdev->dev;
> >  	jpeg->variant = of_device_get_match_data(jpeg->dev);
> > -	INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > mtk_jpeg_job_timeout_work);
> >  
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> > -	if (IS_ERR(jpeg->reg_base)) {
> > -		ret = PTR_ERR(jpeg->reg_base);
> > -		return ret;
> > -	}
> > +	if (!jpeg->variant->is_encoder) {
> > +		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > +				mtk_jpeg_job_timeout_work);
> >  
> > -	jpeg_irq = platform_get_irq(pdev, 0);
> > -	if (jpeg_irq < 0) {
> > -		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n",
> > jpeg_irq);
> > -		return jpeg_irq;
> > -	}
> 
> Maybe better use  devm_platform_ioremap_resource()
It would used in the next code version.
thanks.
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		jpeg->reg_base = devm_ioremap_resource(&pdev->dev,
> > res);
> > +		if (IS_ERR(jpeg->reg_base)) {
> > +			ret = PTR_ERR(jpeg->reg_base);
> > +			return ret;
> > +		}
> >  
> > -	ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > -			       jpeg->variant->irq_handler, 0, pdev-
> > >name, jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to request jpeg_irq %d
> > (%d)\n",
> > -			jpeg_irq, ret);
> > -		goto err_req_irq;
> > -	}
> > +		jpeg_irq = platform_get_irq(pdev, 0);
> > +		if (jpeg_irq < 0) {
> > +			dev_err(&pdev->dev, "Failed to get
> > jpeg_irq.\n");
> > +			return jpeg_irq;
> > +		}
> >  
> > -	ret = mtk_jpeg_clk_init(jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to init clk, err %d\n",
> > ret);
> > -		goto err_clk_init;
> > +		ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > +				       jpeg->variant->irq_handler,
> > +					   0, pdev->name, jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to request jpeg_irq
> > %d (%d)\n",
> > +				jpeg_irq, ret);
> > +			goto err_req_irq;
> > +		}
> > +
> > +		ret = mtk_jpeg_clk_init(jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to init clk(%d)\n",
> > ret);
> > +			goto err_clk_init;
> > +		}
> > +
> > +		pm_runtime_enable(&pdev->dev);
> >  	}
> >  
> >  	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> > @@ -1426,9 +1433,16 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >  		  jpeg->variant->dev_name, jpeg->vdev->num,
> >  		  VIDEO_MAJOR, jpeg->vdev->minor);
> >  
> > -	platform_set_drvdata(pdev, jpeg);
> > +	if (jpeg->variant->is_encoder) {
> 
> Can you share a device tree that uses this?
I will supply in the next code version.
thanks.
>  
> 
> You can't you have all the device in same level, instead of making
> them
> childs of this device?
MT8195 has two encode hw, we wanna use one device node to use them, so
they can't set in same level.
> > +		ret = of_platform_populate(pdev->dev.of_node, NULL,
> > NULL,
> > +			&pdev->dev);
> 
> Maybe you want devm_of_platform_populate()?
It would fixed in the next code version.
thanks.
> > +		if (ret) {
> > +			v4l2_err(&jpeg->v4l2_dev, "Master of platform
> > populate failed.");
> > +			goto err_vfd_jpeg_register;
> > +		}
> > +	}
> >  
> > -	pm_runtime_enable(&pdev->dev);
> > +	platform_set_drvdata(pdev, jpeg);
> >  
> >  	return 0;
> >  
> > @@ -1539,6 +1553,19 @@ static const struct mtk_jpeg_variant
> > mtk_jpeg_drvdata = {
> >  	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
> >  };
> >  
> > +static const struct mtk_jpeg_variant mtk_jpegenc_drvdata = {
> > +	.is_encoder = true,
> > +	.formats = mtk_jpeg_enc_formats,
> > +	.num_formats = MTK_JPEG_ENC_NUM_FORMATS,
> > +	.qops = &mtk_jpeg_enc_qops,
> > +	.m2m_ops = &mtk_jpeg_enc_m2m_ops,
> > +	.dev_name = "mtk-jpeg-enc",
> > +	.ioctl_ops = &mtk_jpeg_enc_ioctl_ops,
> > +	.out_q_default_fourcc = V4L2_PIX_FMT_YUYV,
> > +	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
> > +};
> > +
> > +#if defined(CONFIG_OF)
> >  static const struct of_device_id mtk_jpeg_match[] = {
> >  	{
> >  		.compatible = "mediatek,mt8173-jpgdec",
> > @@ -1552,10 +1579,14 @@ static const struct of_device_id
> > mtk_jpeg_match[] = {
> >  		.compatible = "mediatek,mtk-jpgenc",
> >  		.data = &mtk_jpeg_drvdata,
> >  	},
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc",
> > +		.data = &mtk_jpegenc_drvdata,
> > +	},
> >  	{},
> >  };
> > -
> >  MODULE_DEVICE_TABLE(of, mtk_jpeg_match);
> > +#endif
> >  
> >  static struct platform_driver mtk_jpeg_driver = {
> >  	.probe = mtk_jpeg_probe,
> > @@ -1567,7 +1598,25 @@ static struct platform_driver
> > mtk_jpeg_driver = {
> >  	},
> >  };
> 
> This is not very clear. It would be better to refactor the change and
> have a module_platform_driver() macro for jpegenc_hw_driver, where
> its
> probe and remove exists.
This suggestion would used in the next code version.
thanks.
> > 
> > -module_platform_driver(mtk_jpeg_driver);
> > +static struct platform_driver * const mtk_jpeg_source_drivers[] =
> > {
> > +	&mtk_jpegenc_hw_driver,
> > +	&mtk_jpeg_driver,
> > +};
> > +static int __init mtk_jpeg_init(void)
> > +{
> > +	return platform_register_drivers(mtk_jpeg_source_drivers,
> > +		ARRAY_SIZE(mtk_jpeg_source_drivers));
> > +}
> > +
> > +static void __exit mtk_jpeg_exit(void)
> > +{
> > +	platform_unregister_drivers(mtk_jpeg_source_drivers,
> > +		ARRAY_SIZE(mtk_jpeg_source_drivers));
> > +}
> > +
> > +module_init(mtk_jpeg_init);
> > +module_exit(mtk_jpeg_exit);
> > +
> >  
> >  MODULE_DESCRIPTION("MediaTek JPEG codec driver");
> >  MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 595f7f1..7d013de 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -60,6 +60,7 @@ enum mtk_jpeg_ctx_state {
> >   * @cap_q_default_fourcc:	capture queue default fourcc
> >   */
> >  struct mtk_jpeg_variant {
> > +	bool is_encoder;
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> >  	struct mtk_jpeg_fmt *formats;
> > @@ -74,6 +75,55 @@ struct mtk_jpeg_variant {
> >  	u32 cap_q_default_fourcc;
> >  };
> >  
> > +enum mtk_jpegenc_hw_id {
> > +	MTK_JPEGENC_HW0,
> > +	MTK_JPEGENC_HW1,
> > +	MTK_JPEGENC_HW_MAX,
> > +};
> > +
> > +/**
> > + * struct mtk_jpegenc_clk_info - Structure used to store clock
> > name
> > + */
> > +struct mtk_jpegenc_clk_info {
> > +	const char	*clk_name;
> > +	struct clk	*jpegenc_clk;
> > +};
> > +
> > +/**
> > + * struct mtk_vcodec_clk - Structure used to store vcodec clock
> > information
> > + */
> > +struct mtk_jpegenc_clk {
> > +	struct mtk_jpegenc_clk_info	*clk_info;
> > +	int	clk_num;
> > +};
> > +
> > +/**
> > + * struct mtk_vcodec_pm - Power management data structure
> > + */
> > +struct mtk_jpegenc_pm {
> > +	struct mtk_jpegenc_clk	venc_clk;
> > +	struct device	*dev;
> > +	struct mtk_jpegenc_comp_dev	*mtkdev;
> > +};
> > +
> > +/**
> > + * struct mtk_jpegenc_comp_dev - JPEG COREX abstraction
> > + * @dev:		        JPEG device
> > + * @plat_dev:		    platform device data
> > + * @reg_base:		    JPEG registers mapping
> > + * @master_dev:		    mtk_jpeg_dev device
> > + * @pm:	                mtk_jpegenc_pm
> > + * @jpegenc_irq:	    jpeg encode irq num
> > + */
> > +struct mtk_jpegenc_comp_dev {
> > +	struct device		*dev;
> > +	struct platform_device *plat_dev;
> > +	void __iomem		*reg_base;
> > +	struct mtk_jpeg_dev *master_dev;
> > +	struct mtk_jpegenc_pm pm;
> > +	int jpegenc_irq;
> > +};
> > +
> >  /**
> >   * struct mtk_jpeg_dev - JPEG IP abstraction
> >   * @lock:		the mutex protecting this structure
> > @@ -102,6 +152,9 @@ struct mtk_jpeg_dev {
> >  	struct device		*larb;
> >  	struct delayed_work job_timeout_work;
> >  	const struct mtk_jpeg_variant *variant;
> > +
> > +	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
> > +	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
> >  };
> >  
> >  /**
> > @@ -162,4 +215,6 @@ struct mtk_jpeg_ctx {
> >  	struct v4l2_ctrl_handler ctrl_hdl;
> >  };
> > 
> 
> Please no extern here:
It would fixed in the next code version.
thanks.
> > +extern struct platform_driver mtk_jpegenc_hw_driver;
> > +
> >  #endif /* _MTK_JPEG_CORE_H */
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 1cf037b..4ccda1d 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -4,12 +4,27 @@
> >   * Author: Xia Jiang <xia.jiang@mediatek.com>
> >   *
> >   */
> > -
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> >  #include <linux/io.h>
> >  #include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <media/media-device.h>
> >  #include <media/videobuf2-core.h>
> >  #include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/v4l2-dev.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fh.h>
> > +#include <media/v4l2-event.h>
> >  
> > +#include "mtk_jpeg_core.h"
> >  #include "mtk_jpeg_enc_hw.h"
> >  
> >  static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> > @@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt
> > mtk_jpeg_enc_quality[] = {
> >  	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
> >  };
> >  
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mtk_jpegenc_drv_ids[] = {
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc0",
> > +		.data = (void *)MTK_JPEGENC_HW0,
> > +	},
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc1",
> > +		.data = (void *)MTK_JPEGENC_HW1,
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
> > +#endif
> > +
> >  void mtk_jpeg_enc_reset(void __iomem *base)
> >  {
> >  	writel(0, base + JPEG_ENC_RSTB);
> > @@ -152,3 +182,203 @@ void mtk_jpeg_set_enc_params(struct
> > mtk_jpeg_ctx *ctx,  void __iomem *base)
> >  
> >  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
> >  }
> > +
> > +static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> > +{
> > +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +	enum vb2_buffer_state buf_state;
> > +	struct mtk_jpeg_ctx *ctx;
> > +	u32 result_size;
> > +	u32 irq_status;
> > +
> > +	struct mtk_jpegenc_comp_dev *jpeg = priv;
> > +	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
> > +
> > +	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
> > +		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> > +	if (irq_status)
> > +		writel(0, jpeg->reg_base + JPEG_ENC_INT_STS);
> > +	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE))
> > +		return IRQ_NONE;
> > +
> > +	ctx = v4l2_m2m_get_curr_priv(master_jpeg->m2m_dev);
> > +	if (!ctx) {
> > +		v4l2_err(&master_jpeg->v4l2_dev, "Context is NULL\n");
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +	dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +	result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base);
> > +	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> > +	buf_state = VB2_BUF_STATE_DONE;
> > +	v4l2_m2m_buf_done(src_buf, buf_state);
> > +	v4l2_m2m_buf_done(dst_buf, buf_state);
> > +	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +	pm_runtime_put(ctx->jpeg->dev);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_jpegenc_hw_init_irq(struct mtk_jpegenc_comp_dev
> > *dev)
> > +{
> > +	struct platform_device *pdev = dev->plat_dev;
> > +	int ret;
> > +
> > +	dev->jpegenc_irq = platform_get_irq(pdev, 0);
> > +	if (dev->jpegenc_irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to get irq resource");
> > +		return dev->jpegenc_irq;
> > +	}
> > +
> 
> Have you tried with a threaded_irq?
> > +	ret = devm_request_irq(&pdev->dev, dev->jpegenc_irq,
> > +		mtk_jpegenc_hw_irq_handler, 0, pdev->name, dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to devm_request_irq %d
> > (%d)",
> > +			dev->jpegenc_irq, ret);
> > +		return -ENOENT;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int mtk_jpegenc_init_pm(struct mtk_jpegenc_comp_dev *mtkdev)
> > +{
> > +	struct mtk_jpegenc_clk_info *clk_info;
> > +	struct mtk_jpegenc_clk *jpegenc_clk;
> > +	struct platform_device *pdev;
> > +	struct mtk_jpegenc_pm *pm;
> > +	int i, ret;
> > +
> > +	pdev = mtkdev->plat_dev;
> > +	pm = &mtkdev->pm;
> > +	pm->dev = &pdev->dev;
> > +	pm->mtkdev = mtkdev;
> 
> Can't you simplify with devm_clk_bulk_get_all
It would fixed in the next code version.
thanks.
> > +	jpegenc_clk = &pm->venc_clk;
> > +	jpegenc_clk->clk_num =
> > +		of_property_count_strings(pdev->dev.of_node, "clock-
> > names");
> > +	if (!jpegenc_clk->clk_num) {
> > +		dev_err(&pdev->dev, "Failed to get jpegenc clock
> > count\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	jpegenc_clk->clk_info = devm_kcalloc(&pdev->dev,
> > +		jpegenc_clk->clk_num,
> > +		sizeof(*clk_info),
> > +		GFP_KERNEL);
> > +	if (!jpegenc_clk->clk_info)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < jpegenc_clk->clk_num; i++) {
> > +		clk_info = &jpegenc_clk->clk_info[i];
> > +		ret = of_property_read_string_index(pdev->dev.of_node,
> > +			"clock-names", i,
> > +			&clk_info->clk_name);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to get jpegenc
> > clock name\n");
> > +			return ret;
> > +		}
> > +
> > +		clk_info->jpegenc_clk = devm_clk_get(&pdev->dev,
> > +			clk_info->clk_name);
> > +		if (IS_ERR(clk_info->jpegenc_clk)) {
> > +			dev_err(&pdev->dev, "devm_clk_get (%d)%s fail",
> > +				i, clk_info->clk_name);
> > +			return PTR_ERR(clk_info->jpegenc_clk);
> > +		}
> > +	}
> > +
> 
> Can't you enable the pm after all the probing is completed to
> simplify
> the error handling?
It would fixed in the next code version.
thanks.
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return ret;
> > +}
> > +
> > +void mtk_jpegenc_release_pm(struct mtk_jpegenc_comp_dev *mtkdev)
> > +{
> > +	struct platform_device *pdev = mtkdev->plat_dev;
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +}
> > +
> > +static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
> > +{
> > +	struct mtk_jpeg_dev *master_dev;
> > +	const struct of_device_id *of_id;
> > +	struct mtk_jpegenc_comp_dev *dev;
> > +	int ret, comp_idx;
> > +
> > +	struct device *decs = &pdev->dev;
> > +
> 
> When is the parent NULL?
parent delegates the master_dev, if mtk_jpeg_probe is called after
mtk_jpegenc_hw_probe, the parent is NULL. If return -EPROBE_DEFER,
the system will call mtk_jpegenc_hw_probe later.
> > +	if (!decs->parent)
> > +		return -EPROBE_DEFER;
> > +
> > +	master_dev = dev_get_drvdata(decs->parent);
> > +	if (!master_dev)
> > +		return -EPROBE_DEFER;
> > +
> > +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > +	if (!dev)
> > +		return -ENOMEM;
> > +
> > +	dev->plat_dev = pdev;
> > +
> > +	ret = mtk_jpegenc_init_pm(dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to get jpeg enc clock
> > source");
> > +		return ret;
> > +	}
> > +
> > +	dev->reg_base =
> > +		devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(dev->reg_base)) {
> > +		ret = PTR_ERR(dev->reg_base);
> > +		goto err;
> > +	}
> > +
> > +	ret = mtk_jpegenc_hw_init_irq(dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to register JPEGENC irq
> > handler.\n");
> > +		goto err;
> > +	}
> > +
> 
> please use of_device_get_match_data()
It would fixed in the next code version.
thanks.
> > +	of_id = of_match_device(mtk_jpegenc_drv_ids, decs);
> > +	if (!of_id) {
> > +		dev_err(&pdev->dev, "Can't get vdec comp device
> > id.\n");
> > +		ret = -EINVAL;
> > +		goto err;
> > +	}
> > +
> 
> What if the drvdata is freed before you set it?
I will supply a device tree to explain it in the next version.
thanks.
> > +	comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
> > +	if (comp_idx < MTK_JPEGENC_HW_MAX) {
> > +		master_dev->enc_hw_dev[comp_idx] = dev;
> > +		master_dev->reg_encbase[comp_idx] = dev->reg_base;
> > +		dev->master_dev = master_dev;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, dev);
> > +
> > +	return 0;
> > +
> > +err:
> > +	mtk_jpegenc_release_pm(dev);
> > +	return ret;
> > +}
> > +
> > +
> > +static int mtk_jpegenc_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_jpegenc_comp_dev *dev = platform_get_drvdata(pdev);
> > +
> > +	mtk_jpegenc_release_pm(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +struct platform_driver mtk_jpegenc_hw_driver = {
> > +	.probe = mtk_jpegenc_hw_probe,
> > +	.remove = mtk_jpegenc_remove,
> > +	.driver = {
> > +		.name = "mtk-jpegenc-hw",
> > +		.of_match_table = of_match_ptr(mtk_jpegenc_drv_ids),
> > +	},
> > +};
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

[AngeloGioacchino Del Regno]

Il 03/12/21 04:13, kyrie.wu ha scritto:
> manage each hardware information, including irq/clk/power.
> the hardware includes HW0 and HW1.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>

Hello Kyrie,
thanks for the patch!

However, there are a few things to improve...

> ---
> V6: use of_platform_populate to replace component framework
> to manage multi-hardware
> ---
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++---
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232 +++++++++++++++++++++-
>   3 files changed, 362 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index a89c7b2..da7eb84 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>   	spin_lock_init(&jpeg->hw_lock);
>   	jpeg->dev = &pdev->dev;
>   	jpeg->variant = of_device_get_match_data(jpeg->dev);
> -	INIT_DELAYED_WORK(&jpeg->job_timeout_work, mtk_jpeg_job_timeout_work);
>   
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);

This has to be rebased... new versions are using devm_platform_ioremap_resource(),
which is shorter and cleaner...

> -	if (IS_ERR(jpeg->reg_base)) {
> -		ret = PTR_ERR(jpeg->reg_base);
> -		return ret;
> -	}
> +	if (!jpeg->variant->is_encoder) {

What about mediatek,mtk-jpgenc? That's also an encoder... this "is_encoder"
variable is a bit confusing... Is that a newer version of the IP?
Please, let's find a better name for this variable to avoid confusion.


> +		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> +				mtk_jpeg_job_timeout_work);
>   
> -	jpeg_irq = platform_get_irq(pdev, 0);
> -	if (jpeg_irq < 0) {
> -		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n", jpeg_irq);
> -		return jpeg_irq;
> -	}
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +		jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(jpeg->reg_base)) {
> +			ret = PTR_ERR(jpeg->reg_base);
> +			return ret;
> +		}
>   
> -	ret = devm_request_irq(&pdev->dev, jpeg_irq,
> -			       jpeg->variant->irq_handler, 0, pdev->name, jpeg);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
> -			jpeg_irq, ret);
> -		goto err_req_irq;
> -	}
> +		jpeg_irq = platform_get_irq(pdev, 0);
> +		if (jpeg_irq < 0) {
> +			dev_err(&pdev->dev, "Failed to get jpeg_irq.\n");
> +			return jpeg_irq;
> +		}
>   
> -	ret = mtk_jpeg_clk_init(jpeg);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to init clk, err %d\n", ret);
> -		goto err_clk_init;
> +		ret = devm_request_irq(&pdev->dev, jpeg_irq,
> +				       jpeg->variant->irq_handler,
> +					   0, pdev->name, jpeg);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to request jpeg_irq %d (%d)\n",
> +				jpeg_irq, ret);
> +			goto err_req_irq;
> +		}
> +
> +		ret = mtk_jpeg_clk_init(jpeg);
> +		if (ret) {
> +			dev_err(&pdev->dev, "Failed to init clk(%d)\n", ret);
> +			goto err_clk_init;
> +		}
> +
> +		pm_runtime_enable(&pdev->dev);
>   	}
>   
>   	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);

...snip...

/
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 1cf037b..4ccda1d 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -4,12 +4,27 @@
>    * Author: Xia Jiang <xia.jiang@mediatek.com>
>    *
>    */
> -
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>   #include <linux/io.h>
>   #include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +#include <media/media-device.h>
>   #include <media/videobuf2-core.h>
>   #include <media/videobuf2-dma-contig.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>   
> +#include "mtk_jpeg_core.h"
>   #include "mtk_jpeg_enc_hw.h"
>   
>   static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> @@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
>   	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
>   };
>   
> +#if defined(CONFIG_OF)
> +static const struct of_device_id mtk_jpegenc_drv_ids[] = {

There's nothing special in jpgenc1 or, at least, nothing that really needs
us to differentiate between jpgenc0 and jpgenc1, apart knowing which core is
the "main" one, hence, you don't need a special compatible string for each core.

Here's my proposal:
- Use one compatible "mediatek,mt8195-jpgenc-hw"
- Add either of:
   - A bool "mediatek,hw-leader" on core 0; or
   - A u8 "mediatek,instance" (0, 1, 2 ... for core number)

A comment is required on "mediatek,instance" though... this way should only be
chosen if it is expected, in the future, to have the following situation on
newer SoCs:
- More than two cores, and
- non-interchangeable cores (meaning that, for example, frame 1 *shall* go to
   core 1, frame 2 shall go to core 2, frame 3 to core 3, BUT core 2/3 are not
   interchangeable, as in there are reasons to never process frame 2 on core 3),
   so this means that it's not important if Linux labels core 3 as core 2.

Otherwise, if it's not expected to have non-interchangeable "hw-follower" cores,
or if more than two cores are not ever expected, "mediatek,hw-leader" is the best
choice for this.

Example:

jpegenc@address {
	compatible = "mediatek,mt8195-jpgenc";
	reg = < .... >;
	ranges = < ....... >;
	.... other properties here ....

	jpegenc-hw0@relative-address {
		compatible = "mediatek,mt8195-jpgenc-hw", "mediatek,jpgenc-hw";
		iommus, interrupts, other properties here ...
		mediatek,hw-leader;
	};

	jpegenc-hw1@relative-address {
		compatible = "mediatek,mt8195-jpgenc-hw", "mediatek,jpgenc-hw";
		iommus, interrupts, etc.....
	};
};

> +	{
> +		.compatible = "mediatek,mt8195-jpgenc0",
> +		.data = (void *)MTK_JPEGENC_HW0,
> +	},
> +	{
> +		.compatible = "mediatek,mt8195-jpgenc1",
> +		.data = (void *)MTK_JPEGENC_HW1,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
> +#endif
> +
>   void mtk_jpeg_enc_reset(void __iomem *base)
>   {
>   	writel(0, base + JPEG_ENC_RSTB);

Thanks,
Angelo


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

Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware

[kyrie.wu]

On Mon, 2022-02-07 at 15:50 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/12/21 04:13, kyrie.wu ha scritto:
> > manage each hardware information, including irq/clk/power.
> > the hardware includes HW0 and HW1.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> 
> Hello Kyrie,
> thanks for the patch!
> 
> However, there are a few things to improve...
> 
> > ---
> > V6: use of_platform_populate to replace component framework
> > to manage multi-hardware
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 103 +++++++
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  55 +++++
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 232
> > +++++++++++++++++++++-
> >   3 files changed, 362 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index a89c7b2..da7eb84 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -1353,33 +1353,40 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >   	spin_lock_init(&jpeg->hw_lock);
> >   	jpeg->dev = &pdev->dev;
> >   	jpeg->variant = of_device_get_match_data(jpeg->dev);
> > -	INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > mtk_jpeg_job_timeout_work);
> >   
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > -	jpeg->reg_base = devm_ioremap_resource(&pdev->dev, res);
> 
> This has to be rebased... new versions are using
> devm_platform_ioremap_resource(),
> which is shorter and cleaner...
Hi AngeloGioacchino,
Thanks for your kindly reply, I will fix this problem in the next
version.

> 
> > -	if (IS_ERR(jpeg->reg_base)) {
> > -		ret = PTR_ERR(jpeg->reg_base);
> > -		return ret;
> > -	}
> > +	if (!jpeg->variant->is_encoder) {
> 
> What about mediatek,mtk-jpgenc? That's also an encoder... this
> "is_encoder"
> variable is a bit confusing... Is that a newer version of the IP?
> Please, let's find a better name for this variable to avoid
> confusion.
The confusion would modify in the next version.
Thanks.

> 
> 
> > +		INIT_DELAYED_WORK(&jpeg->job_timeout_work,
> > +				mtk_jpeg_job_timeout_work);
> >   
> > -	jpeg_irq = platform_get_irq(pdev, 0);
> > -	if (jpeg_irq < 0) {
> > -		dev_err(&pdev->dev, "Failed to get jpeg_irq %d.\n",
> > jpeg_irq);
> > -		return jpeg_irq;
> > -	}
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +		jpeg->reg_base = devm_ioremap_resource(&pdev->dev,
> > res);
> > +		if (IS_ERR(jpeg->reg_base)) {
> > +			ret = PTR_ERR(jpeg->reg_base);
> > +			return ret;
> > +		}
> >   
> > -	ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > -			       jpeg->variant->irq_handler, 0, pdev-
> > >name, jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to request jpeg_irq %d
> > (%d)\n",
> > -			jpeg_irq, ret);
> > -		goto err_req_irq;
> > -	}
> > +		jpeg_irq = platform_get_irq(pdev, 0);
> > +		if (jpeg_irq < 0) {
> > +			dev_err(&pdev->dev, "Failed to get
> > jpeg_irq.\n");
> > +			return jpeg_irq;
> > +		}
> >   
> > -	ret = mtk_jpeg_clk_init(jpeg);
> > -	if (ret) {
> > -		dev_err(&pdev->dev, "Failed to init clk, err %d\n",
> > ret);
> > -		goto err_clk_init;
> > +		ret = devm_request_irq(&pdev->dev, jpeg_irq,
> > +				       jpeg->variant->irq_handler,
> > +					   0, pdev->name, jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to request jpeg_irq
> > %d (%d)\n",
> > +				jpeg_irq, ret);
> > +			goto err_req_irq;
> > +		}
> > +
> > +		ret = mtk_jpeg_clk_init(jpeg);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "Failed to init clk(%d)\n",
> > ret);
> > +			goto err_clk_init;
> > +		}
> > +
> > +		pm_runtime_enable(&pdev->dev);
> >   	}
> >   
> >   	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> 
> ...snip...
> 
> /
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 1cf037b..4ccda1d 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -4,12 +4,27 @@
> >    * Author: Xia Jiang <xia.jiang@mediatek.com>
> >    *
> >    */
> > -
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> >   #include <linux/io.h>
> >   #include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +#include <media/media-device.h>
> >   #include <media/videobuf2-core.h>
> >   #include <media/videobuf2-dma-contig.h>
> > +#include <media/videobuf2-v4l2.h>
> > +#include <media/v4l2-mem2mem.h>
> > +#include <media/v4l2-dev.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-fh.h>
> > +#include <media/v4l2-event.h>
> >   
> > +#include "mtk_jpeg_core.h"
> >   #include "mtk_jpeg_enc_hw.h"
> >   
> >   static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
> > @@ -30,6 +45,21 @@ static const struct mtk_jpeg_enc_qlt
> > mtk_jpeg_enc_quality[] = {
> >   	{.quality_param = 97, .hardware_value = JPEG_ENC_QUALITY_Q97},
> >   };
> >   
> > +#if defined(CONFIG_OF)
> > +static const struct of_device_id mtk_jpegenc_drv_ids[] = {
> 
> There's nothing special in jpgenc1 or, at least, nothing that really
> needs
> us to differentiate between jpgenc0 and jpgenc1, apart knowing which
> core is
> the "main" one, hence, you don't need a special compatible string for
> each core.
> 
> Here's my proposal:
> - Use one compatible "mediatek,mt8195-jpgenc-hw"
> - Add either of:
>    - A bool "mediatek,hw-leader" on core 0; or
>    - A u8 "mediatek,instance" (0, 1, 2 ... for core number)
> 
> A comment is required on "mediatek,instance" though... this way
> should only be
> chosen if it is expected, in the future, to have the following
> situation on
> newer SoCs:
> - More than two cores, and
> - non-interchangeable cores (meaning that, for example, frame 1
> *shall* go to
>    core 1, frame 2 shall go to core 2, frame 3 to core 3, BUT core
> 2/3 are not
>    interchangeable, as in there are reasons to never process frame 2
> on core 3),
>    so this means that it's not important if Linux labels core 3 as
> core 2.
> 
> Otherwise, if it's not expected to have non-interchangeable "hw-
> follower" cores,
> or if more than two cores are not ever expected, "mediatek,hw-leader" 
> is the best
> choice for this.
> 
> Example:
> 
> jpegenc@address {
> 	compatible = "mediatek,mt8195-jpgenc";
> 	reg = < .... >;
> 	ranges = < ....... >;
> 	.... other properties here ....
> 
> 	jpegenc-hw0@relative-address {
> 		compatible = "mediatek,mt8195-jpgenc-hw",
> "mediatek,jpgenc-hw";
> 		iommus, interrupts, other properties here ...
> 		mediatek,hw-leader;
> 	};
> 
> 	jpegenc-hw1@relative-address {
> 		compatible = "mediatek,mt8195-jpgenc-hw",
> "mediatek,jpgenc-hw";
> 		iommus, interrupts, etc.....
> 	};
> };
I will take your suggestion in the next version.
Thanks.
> 
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc0",
> > +		.data = (void *)MTK_JPEGENC_HW0,
> > +	},
> > +	{
> > +		.compatible = "mediatek,mt8195-jpgenc1",
> > +		.data = (void *)MTK_JPEGENC_HW1,
> > +	},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_jpegenc_drv_ids);
> > +#endif
> > +
> >   void mtk_jpeg_enc_reset(void __iomem *base)
> >   {
> >   	writel(0, base + JPEG_ENC_RSTB);
> 
> Thanks,
> Angelo
> 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

[kyrie.wu]

Generalizes jpegenc timeout func interfaces to handle HW timeout.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
V6: no change
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 7d013de..baab305 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
 	u32 cap_q_default_fourcc;
 };
 
+struct mtk_jpeg_hw_param {
+	struct vb2_v4l2_buffer *src_buffer;
+	struct vb2_v4l2_buffer *dst_buffer;
+	struct mtk_jpeg_ctx *curr_ctx;
+};
+
 enum mtk_jpegenc_hw_id {
 	MTK_JPEGENC_HW0,
 	MTK_JPEGENC_HW1,
@@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
 	struct mtk_jpeg_dev *master_dev;
 	struct mtk_jpegenc_pm pm;
 	int jpegenc_irq;
+	struct delayed_work job_timeout_work;
+	struct mtk_jpeg_hw_param hw_param;
 };
 
 /**
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
index 4ccda1d..e62b875 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
 	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
 }
 
+static void mtk_jpegenc_timeout_work(struct work_struct *work)
+{
+	struct delayed_work *Pwork =
+		container_of(work, struct delayed_work, work);
+	struct mtk_jpegenc_comp_dev *cjpeg =
+		container_of(Pwork, struct mtk_jpegenc_comp_dev,
+		job_timeout_work);
+	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
+	struct vb2_v4l2_buffer *src_buf;
+
+	src_buf = cjpeg->hw_param.src_buffer;
+
+	mtk_jpeg_enc_reset(cjpeg->reg_base);
+	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);
+	pm_runtime_put(cjpeg->pm.dev);
+	v4l2_m2m_buf_done(src_buf, buf_state);
+}
+
 static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
 {
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
@@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
 	struct mtk_jpegenc_comp_dev *jpeg = priv;
 	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
 
+	cancel_delayed_work(&jpeg->job_timeout_work);
+
 	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
 		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
 	if (irq_status)
@@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
 
 	dev->plat_dev = pdev;
 
+	INIT_DELAYED_WORK(&dev->job_timeout_work,
+		mtk_jpegenc_timeout_work);
+
 	ret = mtk_jpegenc_init_pm(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
-- 
2.6.4


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

Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

[Ricardo Ribalda]

kyrie.wu wrote:

> Generalizes jpegenc timeout func interfaces to handle HW timeout.

Where do you call schedule_delayed_work() and init hw_param?
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 7d013de..baab305 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +struct mtk_jpeg_hw_param {
> +	struct vb2_v4l2_buffer *src_buffer;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct mtk_jpeg_ctx *curr_ctx;
> +};
> +
>  enum mtk_jpegenc_hw_id {
>  	MTK_JPEGENC_HW0,
>  	MTK_JPEGENC_HW1,
> @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
>  	struct mtk_jpeg_dev *master_dev;
>  	struct mtk_jpegenc_pm pm;
>  	int jpegenc_irq;
> +	struct delayed_work job_timeout_work;
> +	struct mtk_jpeg_hw_param hw_param;
>  };
>  
>  /**
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 4ccda1d..e62b875 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
>  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
>  }
>  
> +static void mtk_jpegenc_timeout_work(struct work_struct *work)
> +{
> +	struct delayed_work *Pwork =
> +		container_of(work, struct delayed_work, work);
Please use to_delayed_work()
> +	struct mtk_jpegenc_comp_dev *cjpeg =
> +		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> +		job_timeout_work);
> +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +	struct vb2_v4l2_buffer *src_buf;
> +
> +	src_buf = cjpeg->hw_param.src_buffer;
> +
> +	mtk_jpeg_enc_reset(cjpeg->reg_base);
> +	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);
> +	pm_runtime_put(cjpeg->pm.dev);
> +	v4l2_m2m_buf_done(src_buf, buf_state);
> +}
> +
>  static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>  {
>  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>  	struct mtk_jpegenc_comp_dev *jpeg = priv;
>  	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
>  
> +	cancel_delayed_work(&jpeg->job_timeout_work);
Isn't here a race condition is the delayed work is being exectuted?
> +
>  	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
>  		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
>  	if (irq_status)
> @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  
>  	dev->plat_dev = pdev;
>  
> +	INIT_DELAYED_WORK(&dev->job_timeout_work,
> +		mtk_jpegenc_timeout_work);
> +
>  	ret = mtk_jpegenc_init_pm(dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

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

Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

[kyrie.wu]

On Fri, 2021-12-03 at 16:07 +0100, Ricardo Ribalda wrote:
> kyrie.wu wrote:
> 
> > Generalizes jpegenc timeout func interfaces to handle HW timeout.
> 
> Where do you call schedule_delayed_work() and init hw_param?
the work of call schedule_delayed_work() and init hw_param would
execute in the function of mtk_jpegenc_worker in [PATCH V6, 4/5]
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: no change
> > ---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23
> > +++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 7d013de..baab305 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
> >  	u32 cap_q_default_fourcc;
> >  };
> >  
> > +struct mtk_jpeg_hw_param {
> > +	struct vb2_v4l2_buffer *src_buffer;
> > +	struct vb2_v4l2_buffer *dst_buffer;
> > +	struct mtk_jpeg_ctx *curr_ctx;
> > +};
> > +
> >  enum mtk_jpegenc_hw_id {
> >  	MTK_JPEGENC_HW0,
> >  	MTK_JPEGENC_HW1,
> > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
> >  	struct mtk_jpeg_dev *master_dev;
> >  	struct mtk_jpegenc_pm pm;
> >  	int jpegenc_irq;
> > +	struct delayed_work job_timeout_work;
> > +	struct mtk_jpeg_hw_param hw_param;
> >  };
> >  
> >  /**
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 4ccda1d..e62b875 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct
> > mtk_jpeg_ctx *ctx,  void __iomem *base)
> >  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
> >  }
> >  
> > +static void mtk_jpegenc_timeout_work(struct work_struct *work)
> > +{
> > +	struct delayed_work *Pwork =
> > +		container_of(work, struct delayed_work, work);
> 
> Please use to_delayed_work()
It would be fixed in the next code version.
Thanks.
> > +	struct mtk_jpegenc_comp_dev *cjpeg =
> > +		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> > +		job_timeout_work);
> > +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > +	struct vb2_v4l2_buffer *src_buf;
> > +
> > +	src_buf = cjpeg->hw_param.src_buffer;
> > +
> > +	mtk_jpeg_enc_reset(cjpeg->reg_base);
> > +	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info-
> > >jpegenc_clk);
> > +	pm_runtime_put(cjpeg->pm.dev);
> > +	v4l2_m2m_buf_done(src_buf, buf_state);
> > +}
> > +
> >  static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> >  {
> >  	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > @@ -194,6 +212,8 @@ static irqreturn_t
> > mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> >  	struct mtk_jpegenc_comp_dev *jpeg = priv;
> >  	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
> >  
> > +	cancel_delayed_work(&jpeg->job_timeout_work);
> 
> Isn't here a race condition is the delayed work is being exectuted?
If hw worked beyond 1 second, the delayed work is called. This setting 
is shown in the function of mtk_jpegenc_worker in [PATCH V6, 4/5].
> > +
> >  	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
> >  		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> >  	if (irq_status)
> > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct
> > platform_device *pdev)
> >  
> >  	dev->plat_dev = pdev;
> >  
> > +	INIT_DELAYED_WORK(&dev->job_timeout_work,
> > +		mtk_jpegenc_timeout_work);
> > +
> >  	ret = mtk_jpegenc_init_pm(dev);
> >  	if (ret) {
> >  		dev_err(&pdev->dev, "Failed to get jpeg enc clock
> > source");
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

[AngeloGioacchino Del Regno]

Il 03/12/21 04:13, kyrie.wu ha scritto:
> Generalizes jpegenc timeout func interfaces to handle HW timeout.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change
> ---
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
>   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23 +++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 7d013de..baab305 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
>   	u32 cap_q_default_fourcc;
>   };
>   
> +struct mtk_jpeg_hw_param {
> +	struct vb2_v4l2_buffer *src_buffer;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct mtk_jpeg_ctx *curr_ctx;
> +};
> +
>   enum mtk_jpegenc_hw_id {
>   	MTK_JPEGENC_HW0,
>   	MTK_JPEGENC_HW1,
> @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
>   	struct mtk_jpeg_dev *master_dev;
>   	struct mtk_jpegenc_pm pm;
>   	int jpegenc_irq;
> +	struct delayed_work job_timeout_work;
> +	struct mtk_jpeg_hw_param hw_param;
>   };
>   
>   /**
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 4ccda1d..e62b875 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
>   	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
>   }
>   
> +static void mtk_jpegenc_timeout_work(struct work_struct *work)
> +{
> +	struct delayed_work *Pwork =
> +		container_of(work, struct delayed_work, work);
> +	struct mtk_jpegenc_comp_dev *cjpeg =
> +		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> +		job_timeout_work);
> +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +	struct vb2_v4l2_buffer *src_buf;
> +
> +	src_buf = cjpeg->hw_param.src_buffer;
> +
> +	mtk_jpeg_enc_reset(cjpeg->reg_base);
> +	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);

You disable and unprepare the clock, but you never turn it back on?

This will lead to unbalanced disable on module removal but, more importantly,
will lead to unclocked access after a timeout, for which the platform may or
may not crash, but the jpgenc hardware will be unrecoverable until performing
a full system reboot.

Please fix this.

> +	pm_runtime_put(cjpeg->pm.dev);
> +	v4l2_m2m_buf_done(src_buf, buf_state);
> +}
> +
>   static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>   {
>   	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> @@ -194,6 +212,8 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>   	struct mtk_jpegenc_comp_dev *jpeg = priv;
>   	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
>   
> +	cancel_delayed_work(&jpeg->job_timeout_work);
> +
>   	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
>   		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
>   	if (irq_status)
> @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>   
>   	dev->plat_dev = pdev;
>   
> +	INIT_DELAYED_WORK(&dev->job_timeout_work,
> +		mtk_jpegenc_timeout_work);
> +
>   	ret = mtk_jpegenc_init_pm(dev);
>   	if (ret) {
>   		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");



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

Re: [PATCH V6, 3/5] media: mtk-jpegenc: add jpegenc timeout func interface

[kyrie.wu]

On Mon, 2022-02-07 at 15:50 +0100, AngeloGioacchino Del Regno wrote:
> Il 03/12/21 04:13, kyrie.wu ha scritto:
> > Generalizes jpegenc timeout func interfaces to handle HW timeout.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: no change
> > ---
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  8 ++++++++
> >   drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 23
> > +++++++++++++++++++++++
> >   2 files changed, 31 insertions(+)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 7d013de..baab305 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -75,6 +75,12 @@ struct mtk_jpeg_variant {
> >   	u32 cap_q_default_fourcc;
> >   };
> >   
> > +struct mtk_jpeg_hw_param {
> > +	struct vb2_v4l2_buffer *src_buffer;
> > +	struct vb2_v4l2_buffer *dst_buffer;
> > +	struct mtk_jpeg_ctx *curr_ctx;
> > +};
> > +
> >   enum mtk_jpegenc_hw_id {
> >   	MTK_JPEGENC_HW0,
> >   	MTK_JPEGENC_HW1,
> > @@ -122,6 +128,8 @@ struct mtk_jpegenc_comp_dev {
> >   	struct mtk_jpeg_dev *master_dev;
> >   	struct mtk_jpegenc_pm pm;
> >   	int jpegenc_irq;
> > +	struct delayed_work job_timeout_work;
> > +	struct mtk_jpeg_hw_param hw_param;
> >   };
> >   
> >   /**
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 4ccda1d..e62b875 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -183,6 +183,24 @@ void mtk_jpeg_set_enc_params(struct
> > mtk_jpeg_ctx *ctx,  void __iomem *base)
> >   	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
> >   }
> >   
> > +static void mtk_jpegenc_timeout_work(struct work_struct *work)
> > +{
> > +	struct delayed_work *Pwork =
> > +		container_of(work, struct delayed_work, work);
> > +	struct mtk_jpegenc_comp_dev *cjpeg =
> > +		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> > +		job_timeout_work);
> > +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > +	struct vb2_v4l2_buffer *src_buf;
> > +
> > +	src_buf = cjpeg->hw_param.src_buffer;
> > +
> > +	mtk_jpeg_enc_reset(cjpeg->reg_base);
> > +	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info-
> > >jpegenc_clk);
> 
> You disable and unprepare the clock, but you never turn it back on?
> 
> This will lead to unbalanced disable on module removal but, more
> importantly,
> will lead to unclocked access after a timeout, for which the platform
> may or
> may not crash, but the jpgenc hardware will be unrecoverable until
> performing
> a full system reboot.
> 
> Please fix this.
Dear AngeloGioacchino,
The colck enabled in the function of mtk_jpegenc_worker in [V6,4/5]
media: mtk-jpegenc: add jpeg encode worker interface. And it disabled
in mtk_jpegenc_timeout_work if encode error, or in
mtk_jpegenc_hw_irq_handler if encode success.
> 
> > +	pm_runtime_put(cjpeg->pm.dev);
> > +	v4l2_m2m_buf_done(src_buf, buf_state);
> > +}
> > +
> >   static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void
> > *priv)
> >   {
> >   	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > @@ -194,6 +212,8 @@ static irqreturn_t
> > mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> >   	struct mtk_jpegenc_comp_dev *jpeg = priv;
> >   	struct mtk_jpeg_dev *master_jpeg = jpeg->master_dev;
> >   
> > +	cancel_delayed_work(&jpeg->job_timeout_work);
> > +
> >   	irq_status = readl(jpeg->reg_base + JPEG_ENC_INT_STS) &
> >   		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> >   	if (irq_status)
> > @@ -322,6 +342,9 @@ static int mtk_jpegenc_hw_probe(struct
> > platform_device *pdev)
> >   
> >   	dev->plat_dev = pdev;
> >   
> > +	INIT_DELAYED_WORK(&dev->job_timeout_work,
> > +		mtk_jpegenc_timeout_work);
> > +
> >   	ret = mtk_jpegenc_init_pm(dev);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "Failed to get jpeg enc clock
> > source");
> 
> 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH V6, 4/5] media: mtk-jpegenc: add jpeg encode worker interface

[kyrie.wu]

Add jpeg encoding worker to ensure that two HWs
run in parallel in MT8195.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
V6: no change
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 180 +++++++++++++++++++---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  11 ++
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c |  17 ++
 3 files changed, 188 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index da7eb84..80a6c1a 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -110,6 +110,9 @@ struct mtk_jpeg_src_buf {
 	struct vb2_v4l2_buffer b;
 	struct list_head list;
 	struct mtk_jpeg_dec_param dec_param;
+
+	struct mtk_jpeg_ctx *curr_ctx;
+	u32 frame_num;
 };
 
 static int debug;
@@ -908,38 +911,148 @@ static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx,
 	return 0;
 }
 
-static void mtk_jpeg_enc_device_run(void *priv)
+static int mtk_jpegenc_select_hw(struct mtk_jpeg_ctx *ctx)
 {
-	struct mtk_jpeg_ctx *ctx = priv;
+	struct mtk_jpegenc_comp_dev *comp_jpeg;
 	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
-	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+	unsigned long flags;
+	int hw_id = -1;
+	int i;
+
+	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+		comp_jpeg = jpeg->enc_hw_dev[i];
+		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
+			hw_id = i;
+			comp_jpeg->hw_state = MTK_JPEG_HW_BUSY;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
+
+	return hw_id;
+}
+
+static int mtk_jpegenc_set_hw_param(struct mtk_jpeg_ctx *ctx,
+	int hw_id,
+	struct vb2_v4l2_buffer *src_buf,
+	struct vb2_v4l2_buffer *dst_buf)
+{
+	struct mtk_jpegenc_comp_dev *jpeg = ctx->jpeg->enc_hw_dev[hw_id];
+
+	jpeg->hw_param.curr_ctx = ctx;
+	jpeg->hw_param.src_buffer = src_buf;
+	jpeg->hw_param.dst_buffer = dst_buf;
+
+	return 0;
+}
+
+static int mtk_jpegenc_deselect_hw(struct mtk_jpeg_dev *jpeg,
+	int hw_id)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	jpeg->enc_hw_dev[hw_id]->hw_state = MTK_JPEG_HW_IDLE;
+	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
+
+	return 0;
+}
+
+static void mtk_jpegenc_worker(struct work_struct *work)
+{
+	struct mtk_jpegenc_comp_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
 	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
+	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
+	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+	atomic_t *hw_rdy[MTK_JPEGENC_HW_MAX];
+	struct clk *jpegenc_clk;
+	int ret, i, hw_id = 0;
 	unsigned long flags;
-	int ret;
 
+	struct mtk_jpeg_ctx *ctx = container_of(work,
+		struct mtk_jpeg_ctx,
+		jpeg_work);
+	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
+
+	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+		comp_jpeg[i] = jpeg->enc_hw_dev[i];
+		hw_rdy[i] = &comp_jpeg[i]->hw_rdy;
+	}
+
+retry_select:
+	hw_id = mtk_jpegenc_select_hw(ctx);
+	if (hw_id < 0) {
+		ret = wait_event_interruptible(jpeg->enc_hw_wq,
+			(atomic_read(hw_rdy[0]) ||
+			atomic_read(hw_rdy[1])) > 0);
+		if (ret != 0) {
+			dev_err(jpeg->dev, "%s : %d, all HW are busy\n",
+				__func__, __LINE__);
+			v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
+			return;
+		}
+
+		goto retry_select;
+	}
+
+	atomic_dec(&comp_jpeg[hw_id]->hw_rdy);
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+	if (!src_buf) {
+		dev_err(jpeg->dev, "%s : %d, get src_buf fail !!!\n",
+			__func__, __LINE__);
+		goto getbuf_fail;
+	}
+
 	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+	if (!dst_buf) {
+		pr_info("%s : %d, get dst_buf fail !!!\n",
+			__func__, __LINE__);
+		goto getbuf_fail;
+	}
 
-	ret = pm_runtime_resume_and_get(jpeg->dev);
-	if (ret < 0)
-		goto enc_end;
+	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
+	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
+	jpeg_src_buf->curr_ctx = ctx;
+	jpeg_src_buf->frame_num = ctx->total_frame_num;
+	jpeg_dst_buf->curr_ctx = ctx;
+	jpeg_dst_buf->frame_num = ctx->total_frame_num;
+	ctx->total_frame_num++;
 
-	schedule_delayed_work(&jpeg->job_timeout_work,
-			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
+	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
+	mtk_jpegenc_set_hw_param(ctx, hw_id, src_buf, dst_buf);
+	ret = pm_runtime_get_sync(comp_jpeg[hw_id]->pm.dev);
+	if (ret < 0) {
+		dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail !!!\n",
+			__func__, __LINE__);
+		goto enc_end;
+	}
 
-	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	jpegenc_clk = comp_jpeg[hw_id]->pm.venc_clk.clk_info->jpegenc_clk;
+	ret = clk_prepare_enable(jpegenc_clk);
+	if (ret) {
+		dev_err(jpeg->dev, "%s : %d, jpegenc clk_prepare_enable fail\n",
+			__func__, __LINE__);
+		goto enc_end;
+	}
 
-	/*
-	 * Resetting the hardware every frame is to ensure that all the
-	 * registers are cleared. This is a hardware requirement.
-	 */
-	mtk_jpeg_enc_reset(jpeg->reg_base);
+	schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work,
+		msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
+
+	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
+	mtk_jpeg_enc_reset(comp_jpeg[hw_id]->reg_base);
+	mtk_jpeg_set_enc_dst(ctx,
+		comp_jpeg[hw_id]->reg_base,
+		&dst_buf->vb2_buf);
+	mtk_jpeg_set_enc_src(ctx,
+		comp_jpeg[hw_id]->reg_base,
+		&src_buf->vb2_buf);
+	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base);
+	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
+	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
+	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);
 
-	mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf);
-	mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf);
-	mtk_jpeg_set_enc_params(ctx, jpeg->reg_base);
-	mtk_jpeg_enc_start(jpeg->reg_base);
-	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
 	return;
 
 enc_end:
@@ -947,9 +1060,20 @@ static void mtk_jpeg_enc_device_run(void *priv)
 	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
 	v4l2_m2m_buf_done(src_buf, buf_state);
 	v4l2_m2m_buf_done(dst_buf, buf_state);
+getbuf_fail:
+	atomic_inc(&comp_jpeg[hw_id]->hw_rdy);
+	mtk_jpegenc_deselect_hw(jpeg, hw_id);
 	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
 }
 
+static void mtk_jpeg_enc_device_run(void *priv)
+{
+	struct mtk_jpeg_ctx *ctx = priv;
+	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
+
+	queue_work(jpeg->workqueue, &ctx->jpeg_work);
+}
+
 static void mtk_jpeg_dec_device_run(void *priv)
 {
 	struct mtk_jpeg_ctx *ctx = priv;
@@ -1217,6 +1341,9 @@ static int mtk_jpeg_open(struct file *file)
 		goto free;
 	}
 
+	if (jpeg->variant->is_encoder)
+		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
+
 	v4l2_fh_init(&ctx->fh, vfd);
 	file->private_data = &ctx->fh;
 	v4l2_fh_add(&ctx->fh);
@@ -1387,6 +1514,15 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 		}
 
 		pm_runtime_enable(&pdev->dev);
+	} else {
+		init_waitqueue_head(&jpeg->enc_hw_wq);
+		jpeg->workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME,
+			WQ_MEM_RECLAIM | WQ_FREEZABLE);
+		if (!jpeg->workqueue) {
+			dev_err(&pdev->dev, "Failed to create jpeg workqueue!\n");
+			ret = -EINVAL;
+			goto err_alloc_workqueue;
+		}
 	}
 
 	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
@@ -1460,6 +1596,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 
 err_clk_init:
 
+err_alloc_workqueue:
+
 err_req_irq:
 
 	return ret;
@@ -1475,6 +1613,8 @@ static int mtk_jpeg_remove(struct platform_device *pdev)
 	v4l2_m2m_release(jpeg->m2m_dev);
 	v4l2_device_unregister(&jpeg->v4l2_dev);
 	mtk_jpeg_clk_release(jpeg);
+	flush_workqueue(jpeg->workqueue);
+	destroy_workqueue(jpeg->workqueue);
 
 	return 0;
 }
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index baab305..4c669af 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -75,6 +75,11 @@ struct mtk_jpeg_variant {
 	u32 cap_q_default_fourcc;
 };
 
+enum mtk_jpeg_hw_state {
+	MTK_JPEG_HW_IDLE = 0,
+	MTK_JPEG_HW_BUSY = 1,
+};
+
 struct mtk_jpeg_hw_param {
 	struct vb2_v4l2_buffer *src_buffer;
 	struct vb2_v4l2_buffer *dst_buffer;
@@ -130,6 +135,9 @@ struct mtk_jpegenc_comp_dev {
 	int jpegenc_irq;
 	struct delayed_work job_timeout_work;
 	struct mtk_jpeg_hw_param hw_param;
+	atomic_t hw_rdy;
+	enum mtk_jpeg_hw_state hw_state;
+	spinlock_t hw_lock;
 };
 
 /**
@@ -163,6 +171,7 @@ struct mtk_jpeg_dev {
 
 	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
 	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
+	wait_queue_head_t enc_hw_wq;
 };
 
 /**
@@ -221,6 +230,8 @@ struct mtk_jpeg_ctx {
 	u8 enc_quality;
 	u8 restart_interval;
 	struct v4l2_ctrl_handler ctrl_hdl;
+	struct work_struct jpeg_work;
+	u32 total_frame_num;
 };
 
 extern struct platform_driver mtk_jpegenc_hw_driver;
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
index e62b875..244f9f7 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -190,6 +190,7 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
 	struct mtk_jpegenc_comp_dev *cjpeg =
 		container_of(Pwork, struct mtk_jpegenc_comp_dev,
 		job_timeout_work);
+	struct mtk_jpeg_dev *master_jpeg = cjpeg->master_dev;
 	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
 	struct vb2_v4l2_buffer *src_buf;
 
@@ -198,6 +199,9 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
 	mtk_jpeg_enc_reset(cjpeg->reg_base);
 	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);
 	pm_runtime_put(cjpeg->pm.dev);
+	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
+	atomic_inc(&cjpeg->hw_rdy);
+	wake_up(&master_jpeg->enc_hw_wq);
 	v4l2_m2m_buf_done(src_buf, buf_state);
 }
 
@@ -235,7 +239,17 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
 	v4l2_m2m_buf_done(src_buf, buf_state);
 	v4l2_m2m_buf_done(dst_buf, buf_state);
 	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
+	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
 	pm_runtime_put(ctx->jpeg->dev);
+	if (ctx->fh.m2m_ctx &&
+		(!list_empty(&ctx->fh.m2m_ctx->out_q_ctx.rdy_queue) ||
+		!list_empty(&ctx->fh.m2m_ctx->cap_q_ctx.rdy_queue))) {
+		queue_work(master_jpeg->workqueue, &ctx->jpeg_work);
+	}
+
+	jpeg->hw_state = MTK_JPEG_HW_IDLE;
+	wake_up(&master_jpeg->enc_hw_wq);
+	atomic_inc(&jpeg->hw_rdy);
 
 	return IRQ_HANDLED;
 }
@@ -341,6 +355,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	dev->plat_dev = pdev;
+	atomic_set(&dev->hw_rdy, 1U);
+	spin_lock_init(&dev->hw_lock);
+	dev->hw_state = MTK_JPEG_HW_IDLE;
 
 	INIT_DELAYED_WORK(&dev->job_timeout_work,
 		mtk_jpegenc_timeout_work);
-- 
2.6.4


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

Re: [PATCH V6, 4/5] media: mtk-jpegenc: add jpeg encode worker interface

[Ricardo Ribalda]

kyrie.wu wrote:

> Add jpeg encoding worker to ensure that two HWs
> run in parallel in MT8195.

Have you considered using a semaphore initialized to two to arbitrate this?

> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 180 +++++++++++++++++++---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  11 ++
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c |  17 ++
>  3 files changed, 188 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index da7eb84..80a6c1a 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -110,6 +110,9 @@ struct mtk_jpeg_src_buf {
>  	struct vb2_v4l2_buffer b;
>  	struct list_head list;
>  	struct mtk_jpeg_dec_param dec_param;
> +
> +	struct mtk_jpeg_ctx *curr_ctx;
> +	u32 frame_num;
>  };
>  
>  static int debug;
> @@ -908,38 +911,148 @@ static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx,
>  	return 0;
>  }
>  
> -static void mtk_jpeg_enc_device_run(void *priv)
> +static int mtk_jpegenc_select_hw(struct mtk_jpeg_ctx *ctx)
>  {
> -	struct mtk_jpeg_ctx *ctx = priv;
> +	struct mtk_jpegenc_comp_dev *comp_jpeg;
>  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> -	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +	unsigned long flags;
> +	int hw_id = -1;
> +	int i;
> +
> +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> +	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +		comp_jpeg = jpeg->enc_hw_dev[i];
> +		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
> +			hw_id = i;
> +			comp_jpeg->hw_state = MTK_JPEG_HW_BUSY;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> +
> +	return hw_id;
> +}
> +
> +static int mtk_jpegenc_set_hw_param(struct mtk_jpeg_ctx *ctx,
> +	int hw_id,
> +	struct vb2_v4l2_buffer *src_buf,
> +	struct vb2_v4l2_buffer *dst_buf)
> +{
> +	struct mtk_jpegenc_comp_dev *jpeg = ctx->jpeg->enc_hw_dev[hw_id];
> +
> +	jpeg->hw_param.curr_ctx = ctx;
> +	jpeg->hw_param.src_buffer = src_buf;
> +	jpeg->hw_param.dst_buffer = dst_buf;
> +
> +	return 0;
> +}
> +
> +static int mtk_jpegenc_deselect_hw(struct mtk_jpeg_dev *jpeg,
> +	int hw_id)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> +	jpeg->enc_hw_dev[hw_id]->hw_state = MTK_JPEG_HW_IDLE;
> +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> +
> +	return 0;
> +}
> +
> +static void mtk_jpegenc_worker(struct work_struct *work)
> +{
> +	struct mtk_jpegenc_comp_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
>  	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +	atomic_t *hw_rdy[MTK_JPEGENC_HW_MAX];
> +	struct clk *jpegenc_clk;
> +	int ret, i, hw_id = 0;
>  	unsigned long flags;
> -	int ret;
>  
> +	struct mtk_jpeg_ctx *ctx = container_of(work,
> +		struct mtk_jpeg_ctx,
> +		jpeg_work);
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +
> +	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +		comp_jpeg[i] = jpeg->enc_hw_dev[i];
> +		hw_rdy[i] = &comp_jpeg[i]->hw_rdy;
> +	}
> +
> +retry_select:
> +	hw_id = mtk_jpegenc_select_hw(ctx);
> +	if (hw_id < 0) {
> +		ret = wait_event_interruptible(jpeg->enc_hw_wq,
> +			(atomic_read(hw_rdy[0]) ||
> +			atomic_read(hw_rdy[1])) > 0);
> +		if (ret != 0) {
> +			dev_err(jpeg->dev, "%s : %d, all HW are busy\n",
> +				__func__, __LINE__);
> +			v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +			return;
> +		}
> +
> +		goto retry_select;
> +	}
> +
> +	atomic_dec(&comp_jpeg[hw_id]->hw_rdy);
>  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +	if (!src_buf) {
> +		dev_err(jpeg->dev, "%s : %d, get src_buf fail !!!\n",
> +			__func__, __LINE__);
> +		goto getbuf_fail;
> +	}
> +
>  	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> +	if (!dst_buf) {
> +		pr_info("%s : %d, get dst_buf fail !!!\n",
> +			__func__, __LINE__);
> +		goto getbuf_fail;
> +	}
>  
> -	ret = pm_runtime_resume_and_get(jpeg->dev);
> -	if (ret < 0)
> -		goto enc_end;
> +	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> +	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
> +	jpeg_src_buf->curr_ctx = ctx;
> +	jpeg_src_buf->frame_num = ctx->total_frame_num;
> +	jpeg_dst_buf->curr_ctx = ctx;
> +	jpeg_dst_buf->frame_num = ctx->total_frame_num;
> +	ctx->total_frame_num++;
>  
> -	schedule_delayed_work(&jpeg->job_timeout_work,
> -			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
> +	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> +	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> +	mtk_jpegenc_set_hw_param(ctx, hw_id, src_buf, dst_buf);
> +	ret = pm_runtime_get_sync(comp_jpeg[hw_id]->pm.dev);
> +	if (ret < 0) {
> +		dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail !!!\n",
> +			__func__, __LINE__);
> +		goto enc_end;
> +	}
>  
> -	spin_lock_irqsave(&jpeg->hw_lock, flags);
> +	jpegenc_clk = comp_jpeg[hw_id]->pm.venc_clk.clk_info->jpegenc_clk;
> +	ret = clk_prepare_enable(jpegenc_clk);
> +	if (ret) {
> +		dev_err(jpeg->dev, "%s : %d, jpegenc clk_prepare_enable fail\n",
> +			__func__, __LINE__);
> +		goto enc_end;
> +	}
>  
> -	/*
> -	 * Resetting the hardware every frame is to ensure that all the
> -	 * registers are cleared. This is a hardware requirement.
> -	 */
> -	mtk_jpeg_enc_reset(jpeg->reg_base);
> +	schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work,
> +		msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
> +
> +	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
> +	mtk_jpeg_enc_reset(comp_jpeg[hw_id]->reg_base);
> +	mtk_jpeg_set_enc_dst(ctx,
> +		comp_jpeg[hw_id]->reg_base,
> +		&dst_buf->vb2_buf);
> +	mtk_jpeg_set_enc_src(ctx,
> +		comp_jpeg[hw_id]->reg_base,
> +		&src_buf->vb2_buf);
> +	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base);
> +	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
> +	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);
>  
> -	mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf);
> -	mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf);
> -	mtk_jpeg_set_enc_params(ctx, jpeg->reg_base);
> -	mtk_jpeg_enc_start(jpeg->reg_base);
> -	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
>  	return;
>  
>  enc_end:
> @@ -947,9 +1060,20 @@ static void mtk_jpeg_enc_device_run(void *priv)
>  	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>  	v4l2_m2m_buf_done(src_buf, buf_state);
>  	v4l2_m2m_buf_done(dst_buf, buf_state);
> +getbuf_fail:
> +	atomic_inc(&comp_jpeg[hw_id]->hw_rdy);
> +	mtk_jpegenc_deselect_hw(jpeg, hw_id);
>  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>  }
>  
> +static void mtk_jpeg_enc_device_run(void *priv)
> +{
> +	struct mtk_jpeg_ctx *ctx = priv;
> +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;

You are reusing the jpeg_work structure, so if you call multiple times
mtk_jpeg_enc_device_run(), before the old one is completed, you would
overwrite the old values. Is this what you want?
> +
> +	queue_work(jpeg->workqueue, &ctx->jpeg_work);
> +}
> +
>  static void mtk_jpeg_dec_device_run(void *priv)
>  {
>  	struct mtk_jpeg_ctx *ctx = priv;
> @@ -1217,6 +1341,9 @@ static int mtk_jpeg_open(struct file *file)
>  		goto free;
>  	}
>  
> +	if (jpeg->variant->is_encoder)
> +		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
> +
>  	v4l2_fh_init(&ctx->fh, vfd);
>  	file->private_data = &ctx->fh;
>  	v4l2_fh_add(&ctx->fh);
> @@ -1387,6 +1514,15 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  		}
>  
>  		pm_runtime_enable(&pdev->dev);
> +	} else {
> +		init_waitqueue_head(&jpeg->enc_hw_wq);
> +		jpeg->workqueue = alloc_ordered_workqueue(MTK_JPEG_NAME,
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE);
> +		if (!jpeg->workqueue) {
> +			dev_err(&pdev->dev, "Failed to create jpeg workqueue!\n");
> +			ret = -EINVAL;
> +			goto err_alloc_workqueue;
> +		}
>  	}
>  
>  	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> @@ -1460,6 +1596,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  
>  err_clk_init:
>  
> +err_alloc_workqueue:
> +
>  err_req_irq:
>  
>  	return ret;
> @@ -1475,6 +1613,8 @@ static int mtk_jpeg_remove(struct platform_device *pdev)
>  	v4l2_m2m_release(jpeg->m2m_dev);
>  	v4l2_device_unregister(&jpeg->v4l2_dev);
>  	mtk_jpeg_clk_release(jpeg);
> +	flush_workqueue(jpeg->workqueue);
> +	destroy_workqueue(jpeg->workqueue);
>  
>  	return 0;
>  }
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index baab305..4c669af 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,11 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +enum mtk_jpeg_hw_state {
> +	MTK_JPEG_HW_IDLE = 0,
> +	MTK_JPEG_HW_BUSY = 1,
> +};
> +
>  struct mtk_jpeg_hw_param {
>  	struct vb2_v4l2_buffer *src_buffer;
>  	struct vb2_v4l2_buffer *dst_buffer;
> @@ -130,6 +135,9 @@ struct mtk_jpegenc_comp_dev {
>  	int jpegenc_irq;
>  	struct delayed_work job_timeout_work;
>  	struct mtk_jpeg_hw_param hw_param;
> +	atomic_t hw_rdy;
> +	enum mtk_jpeg_hw_state hw_state;
> +	spinlock_t hw_lock;
>  };
>  
>  /**
> @@ -163,6 +171,7 @@ struct mtk_jpeg_dev {
>  
>  	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
>  	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
> +	wait_queue_head_t enc_hw_wq;
>  };
>  
>  /**
> @@ -221,6 +230,8 @@ struct mtk_jpeg_ctx {
>  	u8 enc_quality;
>  	u8 restart_interval;
>  	struct v4l2_ctrl_handler ctrl_hdl;
> +	struct work_struct jpeg_work;
> +	u32 total_frame_num;
>  };
>  
>  extern struct platform_driver mtk_jpegenc_hw_driver;
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index e62b875..244f9f7 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -190,6 +190,7 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
>  	struct mtk_jpegenc_comp_dev *cjpeg =
>  		container_of(Pwork, struct mtk_jpegenc_comp_dev,
>  		job_timeout_work);
> +	struct mtk_jpeg_dev *master_jpeg = cjpeg->master_dev;
>  	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
>  	struct vb2_v4l2_buffer *src_buf;
>  
> @@ -198,6 +199,9 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
>  	mtk_jpeg_enc_reset(cjpeg->reg_base);
>  	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info->jpegenc_clk);
>  	pm_runtime_put(cjpeg->pm.dev);
> +	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
> +	atomic_inc(&cjpeg->hw_rdy);
> +	wake_up(&master_jpeg->enc_hw_wq);
>  	v4l2_m2m_buf_done(src_buf, buf_state);
>  }
>  
> @@ -235,7 +239,17 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>  	v4l2_m2m_buf_done(src_buf, buf_state);
>  	v4l2_m2m_buf_done(dst_buf, buf_state);
>  	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
>  	pm_runtime_put(ctx->jpeg->dev);
> +	if (ctx->fh.m2m_ctx &&
> +		(!list_empty(&ctx->fh.m2m_ctx->out_q_ctx.rdy_queue) ||
> +		!list_empty(&ctx->fh.m2m_ctx->cap_q_ctx.rdy_queue))) {
> +		queue_work(master_jpeg->workqueue, &ctx->jpeg_work);
> +	}
> +
> +	jpeg->hw_state = MTK_JPEG_HW_IDLE;
> +	wake_up(&master_jpeg->enc_hw_wq);
> +	atomic_inc(&jpeg->hw_rdy);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -341,6 +355,9 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	dev->plat_dev = pdev;
> +	atomic_set(&dev->hw_rdy, 1U);
> +	spin_lock_init(&dev->hw_lock);
> +	dev->hw_state = MTK_JPEG_HW_IDLE;
>  
>  	INIT_DELAYED_WORK(&dev->job_timeout_work,
>  		mtk_jpegenc_timeout_work);
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [PATCH V6, 4/5] media: mtk-jpegenc: add jpeg encode worker interface

[kyrie.wu]

On Fri, 2021-12-03 at 16:29 +0100, Ricardo Ribalda wrote:
> kyrie.wu wrote:
> 
> > Add jpeg encoding worker to ensure that two HWs
> > run in parallel in MT8195.
> 
> Have you considered using a semaphore initialized to two to arbitrate
> this?
I will consider your suggestion. Thanks.
> 
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: no change
> > ---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 180
> > +++++++++++++++++++---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  11 ++
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c |  17 ++
> >  3 files changed, 188 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index da7eb84..80a6c1a 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -110,6 +110,9 @@ struct mtk_jpeg_src_buf {
> >  	struct vb2_v4l2_buffer b;
> >  	struct list_head list;
> >  	struct mtk_jpeg_dec_param dec_param;
> > +
> > +	struct mtk_jpeg_ctx *curr_ctx;
> > +	u32 frame_num;
> >  };
> >  
> >  static int debug;
> > @@ -908,38 +911,148 @@ static int mtk_jpeg_set_dec_dst(struct
> > mtk_jpeg_ctx *ctx,
> >  	return 0;
> >  }
> >  
> > -static void mtk_jpeg_enc_device_run(void *priv)
> > +static int mtk_jpegenc_select_hw(struct mtk_jpeg_ctx *ctx)
> >  {
> > -	struct mtk_jpeg_ctx *ctx = priv;
> > +	struct mtk_jpegenc_comp_dev *comp_jpeg;
> >  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > -	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +	unsigned long flags;
> > +	int hw_id = -1;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> > +		comp_jpeg = jpeg->enc_hw_dev[i];
> > +		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {
> > +			hw_id = i;
> > +			comp_jpeg->hw_state = MTK_JPEG_HW_BUSY;
> > +			break;
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > +
> > +	return hw_id;
> > +}
> > +
> > +static int mtk_jpegenc_set_hw_param(struct mtk_jpeg_ctx *ctx,
> > +	int hw_id,
> > +	struct vb2_v4l2_buffer *src_buf,
> > +	struct vb2_v4l2_buffer *dst_buf)
> > +{
> > +	struct mtk_jpegenc_comp_dev *jpeg = ctx->jpeg-
> > >enc_hw_dev[hw_id];
> > +
> > +	jpeg->hw_param.curr_ctx = ctx;
> > +	jpeg->hw_param.src_buffer = src_buf;
> > +	jpeg->hw_param.dst_buffer = dst_buf;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_jpegenc_deselect_hw(struct mtk_jpeg_dev *jpeg,
> > +	int hw_id)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +	jpeg->enc_hw_dev[hw_id]->hw_state = MTK_JPEG_HW_IDLE;
> > +	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_jpegenc_worker(struct work_struct *work)
> > +{
> > +	struct mtk_jpegenc_comp_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
> >  	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> > +	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
> > +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> > +	atomic_t *hw_rdy[MTK_JPEGENC_HW_MAX];
> > +	struct clk *jpegenc_clk;
> > +	int ret, i, hw_id = 0;
> >  	unsigned long flags;
> > -	int ret;
> >  
> > +	struct mtk_jpeg_ctx *ctx = container_of(work,
> > +		struct mtk_jpeg_ctx,
> > +		jpeg_work);
> > +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> > +
> > +	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> > +		comp_jpeg[i] = jpeg->enc_hw_dev[i];
> > +		hw_rdy[i] = &comp_jpeg[i]->hw_rdy;
> > +	}
> > +
> > +retry_select:
> > +	hw_id = mtk_jpegenc_select_hw(ctx);
> > +	if (hw_id < 0) {
> > +		ret = wait_event_interruptible(jpeg->enc_hw_wq,
> > +			(atomic_read(hw_rdy[0]) ||
> > +			atomic_read(hw_rdy[1])) > 0);
> > +		if (ret != 0) {
> > +			dev_err(jpeg->dev, "%s : %d, all HW are
> > busy\n",
> > +				__func__, __LINE__);
> > +			v4l2_m2m_job_finish(jpeg->m2m_dev, ctx-
> > >fh.m2m_ctx);
> > +			return;
> > +		}
> > +
> > +		goto retry_select;
> > +	}
> > +
> > +	atomic_dec(&comp_jpeg[hw_id]->hw_rdy);
> >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> > +	if (!src_buf) {
> > +		dev_err(jpeg->dev, "%s : %d, get src_buf fail !!!\n",
> > +			__func__, __LINE__);
> > +		goto getbuf_fail;
> > +	}
> > +
> >  	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > +	if (!dst_buf) {
> > +		pr_info("%s : %d, get dst_buf fail !!!\n",
> > +			__func__, __LINE__);
> > +		goto getbuf_fail;
> > +	}
> >  
> > -	ret = pm_runtime_resume_and_get(jpeg->dev);
> > -	if (ret < 0)
> > -		goto enc_end;
> > +	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
> > +	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
> > +	jpeg_src_buf->curr_ctx = ctx;
> > +	jpeg_src_buf->frame_num = ctx->total_frame_num;
> > +	jpeg_dst_buf->curr_ctx = ctx;
> > +	jpeg_dst_buf->frame_num = ctx->total_frame_num;
> > +	ctx->total_frame_num++;
> >  
> > -	schedule_delayed_work(&jpeg->job_timeout_work,
> > -			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC
> > ));
> > +	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> > +	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> > +	mtk_jpegenc_set_hw_param(ctx, hw_id, src_buf, dst_buf);
> > +	ret = pm_runtime_get_sync(comp_jpeg[hw_id]->pm.dev);
> > +	if (ret < 0) {
> > +		dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail
> > !!!\n",
> > +			__func__, __LINE__);
> > +		goto enc_end;
> > +	}
> >  
> > -	spin_lock_irqsave(&jpeg->hw_lock, flags);
> > +	jpegenc_clk = comp_jpeg[hw_id]->pm.venc_clk.clk_info-
> > >jpegenc_clk;
> > +	ret = clk_prepare_enable(jpegenc_clk);
> > +	if (ret) {
> > +		dev_err(jpeg->dev, "%s : %d, jpegenc clk_prepare_enable
> > fail\n",
> > +			__func__, __LINE__);
> > +		goto enc_end;
> > +	}
> >  
> > -	/*
> > -	 * Resetting the hardware every frame is to ensure that all the
> > -	 * registers are cleared. This is a hardware requirement.
> > -	 */
> > -	mtk_jpeg_enc_reset(jpeg->reg_base);
> > +	schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work,
> > +		msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
> > +
> > +	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
> > +	mtk_jpeg_enc_reset(comp_jpeg[hw_id]->reg_base);
> > +	mtk_jpeg_set_enc_dst(ctx,
> > +		comp_jpeg[hw_id]->reg_base,
> > +		&dst_buf->vb2_buf);
> > +	mtk_jpeg_set_enc_src(ctx,
> > +		comp_jpeg[hw_id]->reg_base,
> > +		&src_buf->vb2_buf);
> > +	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base);
> > +	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base);
> > +	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);
> >  
> > -	mtk_jpeg_set_enc_src(ctx, jpeg->reg_base, &src_buf->vb2_buf);
> > -	mtk_jpeg_set_enc_dst(ctx, jpeg->reg_base, &dst_buf->vb2_buf);
> > -	mtk_jpeg_set_enc_params(ctx, jpeg->reg_base);
> > -	mtk_jpeg_enc_start(jpeg->reg_base);
> > -	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
> >  	return;
> >  
> >  enc_end:
> > @@ -947,9 +1060,20 @@ static void mtk_jpeg_enc_device_run(void
> > *priv)
> >  	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> >  	v4l2_m2m_buf_done(dst_buf, buf_state);
> > +getbuf_fail:
> > +	atomic_inc(&comp_jpeg[hw_id]->hw_rdy);
> > +	mtk_jpegenc_deselect_hw(jpeg, hw_id);
> >  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> >  }
> >  
> > +static void mtk_jpeg_enc_device_run(void *priv)
> > +{
> > +	struct mtk_jpeg_ctx *ctx = priv;
> > +	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> 
> You are reusing the jpeg_work structure, so if you call multiple
> times
> mtk_jpeg_enc_device_run(), before the old one is completed, you would
> overwrite the old values. Is this what you want?
queue_work only queue jpeg_work to workqueue, and the worker function
will schedule to encode.The encode source data is set in output buffer,
and it would not overwrite.
> > +
> > +	queue_work(jpeg->workqueue, &ctx->jpeg_work);
> > +}
> > +
> >  static void mtk_jpeg_dec_device_run(void *priv)
> >  {
> >  	struct mtk_jpeg_ctx *ctx = priv;
> > @@ -1217,6 +1341,9 @@ static int mtk_jpeg_open(struct file *file)
> >  		goto free;
> >  	}
> >  
> > +	if (jpeg->variant->is_encoder)
> > +		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
> > +
> >  	v4l2_fh_init(&ctx->fh, vfd);
> >  	file->private_data = &ctx->fh;
> >  	v4l2_fh_add(&ctx->fh);
> > @@ -1387,6 +1514,15 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >  		}
> >  
> >  		pm_runtime_enable(&pdev->dev);
> > +	} else {
> > +		init_waitqueue_head(&jpeg->enc_hw_wq);
> > +		jpeg->workqueue =
> > alloc_ordered_workqueue(MTK_JPEG_NAME,
> > +			WQ_MEM_RECLAIM | WQ_FREEZABLE);
> > +		if (!jpeg->workqueue) {
> > +			dev_err(&pdev->dev, "Failed to create jpeg
> > workqueue!\n");
> > +			ret = -EINVAL;
> > +			goto err_alloc_workqueue;
> > +		}
> >  	}
> >  
> >  	ret = v4l2_device_register(&pdev->dev, &jpeg->v4l2_dev);
> > @@ -1460,6 +1596,8 @@ static int mtk_jpeg_probe(struct
> > platform_device *pdev)
> >  
> >  err_clk_init:
> >  
> > +err_alloc_workqueue:
> > +
> >  err_req_irq:
> >  
> >  	return ret;
> > @@ -1475,6 +1613,8 @@ static int mtk_jpeg_remove(struct
> > platform_device *pdev)
> >  	v4l2_m2m_release(jpeg->m2m_dev);
> >  	v4l2_device_unregister(&jpeg->v4l2_dev);
> >  	mtk_jpeg_clk_release(jpeg);
> > +	flush_workqueue(jpeg->workqueue);
> > +	destroy_workqueue(jpeg->workqueue);
> >  
> >  	return 0;
> >  }
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index baab305..4c669af 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -75,6 +75,11 @@ struct mtk_jpeg_variant {
> >  	u32 cap_q_default_fourcc;
> >  };
> >  
> > +enum mtk_jpeg_hw_state {
> > +	MTK_JPEG_HW_IDLE = 0,
> > +	MTK_JPEG_HW_BUSY = 1,
> > +};
> > +
> >  struct mtk_jpeg_hw_param {
> >  	struct vb2_v4l2_buffer *src_buffer;
> >  	struct vb2_v4l2_buffer *dst_buffer;
> > @@ -130,6 +135,9 @@ struct mtk_jpegenc_comp_dev {
> >  	int jpegenc_irq;
> >  	struct delayed_work job_timeout_work;
> >  	struct mtk_jpeg_hw_param hw_param;
> > +	atomic_t hw_rdy;
> > +	enum mtk_jpeg_hw_state hw_state;
> > +	spinlock_t hw_lock;
> >  };
> >  
> >  /**
> > @@ -163,6 +171,7 @@ struct mtk_jpeg_dev {
> >  
> >  	void __iomem *reg_encbase[MTK_JPEGENC_HW_MAX];
> >  	struct mtk_jpegenc_comp_dev *enc_hw_dev[MTK_JPEGENC_HW_MAX];
> > +	wait_queue_head_t enc_hw_wq;
> >  };
> >  
> >  /**
> > @@ -221,6 +230,8 @@ struct mtk_jpeg_ctx {
> >  	u8 enc_quality;
> >  	u8 restart_interval;
> >  	struct v4l2_ctrl_handler ctrl_hdl;
> > +	struct work_struct jpeg_work;
> > +	u32 total_frame_num;
> >  };
> >  
> >  extern struct platform_driver mtk_jpegenc_hw_driver;
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index e62b875..244f9f7 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -190,6 +190,7 @@ static void mtk_jpegenc_timeout_work(struct
> > work_struct *work)
> >  	struct mtk_jpegenc_comp_dev *cjpeg =
> >  		container_of(Pwork, struct mtk_jpegenc_comp_dev,
> >  		job_timeout_work);
> > +	struct mtk_jpeg_dev *master_jpeg = cjpeg->master_dev;
> >  	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> >  	struct vb2_v4l2_buffer *src_buf;
> >  
> > @@ -198,6 +199,9 @@ static void mtk_jpegenc_timeout_work(struct
> > work_struct *work)
> >  	mtk_jpeg_enc_reset(cjpeg->reg_base);
> >  	clk_disable_unprepare(cjpeg->pm.venc_clk.clk_info-
> > >jpegenc_clk);
> >  	pm_runtime_put(cjpeg->pm.dev);
> > +	cjpeg->hw_state = MTK_JPEG_HW_IDLE;
> > +	atomic_inc(&cjpeg->hw_rdy);
> > +	wake_up(&master_jpeg->enc_hw_wq);
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> >  }
> >  
> > @@ -235,7 +239,17 @@ static irqreturn_t
> > mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> >  	v4l2_m2m_buf_done(dst_buf, buf_state);
> >  	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
> >  	pm_runtime_put(ctx->jpeg->dev);
> > +	if (ctx->fh.m2m_ctx &&
> > +		(!list_empty(&ctx->fh.m2m_ctx->out_q_ctx.rdy_queue) ||
> > +		!list_empty(&ctx->fh.m2m_ctx->cap_q_ctx.rdy_queue))) {
> > +		queue_work(master_jpeg->workqueue, &ctx->jpeg_work);
> > +	}
> > +
> > +	jpeg->hw_state = MTK_JPEG_HW_IDLE;
> > +	wake_up(&master_jpeg->enc_hw_wq);
> > +	atomic_inc(&jpeg->hw_rdy);
> >  
> >  	return IRQ_HANDLED;
> >  }
> > @@ -341,6 +355,9 @@ static int mtk_jpegenc_hw_probe(struct
> > platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	dev->plat_dev = pdev;
> > +	atomic_set(&dev->hw_rdy, 1U);
> > +	spin_lock_init(&dev->hw_lock);
> > +	dev->hw_state = MTK_JPEG_HW_IDLE;
> >  
> >  	INIT_DELAYED_WORK(&dev->job_timeout_work,
> >  		mtk_jpegenc_timeout_work);
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH V6, 5/5] media: mtk-jpegenc: add output pic reorder interface

[kyrie.wu]

There are two HWs in mt8195. Since the two HWs run
in parallel, it is necessary to reorder the output images
to ensure that the order is consistent with the input images.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
V6: no change
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 10 +----
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   | 17 +++++++-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c |  1 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h |  3 +-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 48 ++++++++++++++++++++++-
 5 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 80a6c1a..9e89629 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -106,15 +106,6 @@ static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = {
 #define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats)
 #define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats)
 
-struct mtk_jpeg_src_buf {
-	struct vb2_v4l2_buffer b;
-	struct list_head list;
-	struct mtk_jpeg_dec_param dec_param;
-
-	struct mtk_jpeg_ctx *curr_ctx;
-	u32 frame_num;
-};
-
 static int debug;
 module_param(debug, int, 0644);
 
@@ -1344,6 +1335,7 @@ static int mtk_jpeg_open(struct file *file)
 	if (jpeg->variant->is_encoder)
 		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
 
+	INIT_LIST_HEAD(&ctx->dst_done_queue);
 	v4l2_fh_init(&ctx->fh, vfd);
 	file->private_data = &ctx->fh;
 	v4l2_fh_add(&ctx->fh);
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 4c669af..f276221 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -13,10 +13,11 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fh.h>
+#include <media/videobuf2-v4l2.h>
 
-#define MTK_JPEG_NAME		"mtk-jpeg"
+#include "mtk_jpeg_dec_hw.h"
 
-#define MTK_JPEG_COMP_MAX		3
+#define MTK_JPEG_NAME		"mtk-jpeg"
 
 #define MTK_JPEG_FMT_FLAG_OUTPUT	BIT(0)
 #define MTK_JPEG_FMT_FLAG_CAPTURE	BIT(1)
@@ -75,6 +76,15 @@ struct mtk_jpeg_variant {
 	u32 cap_q_default_fourcc;
 };
 
+struct mtk_jpeg_src_buf {
+	struct vb2_v4l2_buffer b;
+	struct list_head list;
+	struct mtk_jpeg_dec_param dec_param;
+
+	struct mtk_jpeg_ctx *curr_ctx;
+	u32 frame_num;
+};
+
 enum mtk_jpeg_hw_state {
 	MTK_JPEG_HW_IDLE = 0,
 	MTK_JPEG_HW_BUSY = 1,
@@ -232,6 +242,9 @@ struct mtk_jpeg_ctx {
 	struct v4l2_ctrl_handler ctrl_hdl;
 	struct work_struct jpeg_work;
 	u32 total_frame_num;
+	struct list_head dst_done_queue;
+	spinlock_t done_queue_lock;
+	u32 last_done_frame_num;
 };
 
 extern struct platform_driver mtk_jpegenc_hw_driver;
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
index afbbfd5..1e38522 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
@@ -9,6 +9,7 @@
 #include <linux/kernel.h>
 #include <media/videobuf2-core.h>
 
+#include "mtk_jpeg_core.h"
 #include "mtk_jpeg_dec_hw.h"
 
 #define MTK_JPEG_DUNUM_MASK(val)	(((val) - 1) & 0x3)
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
index fa0d45f..87aaa5c 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
@@ -11,9 +11,10 @@
 
 #include <media/videobuf2-core.h>
 
-#include "mtk_jpeg_core.h"
 #include "mtk_jpeg_dec_reg.h"
 
+#define MTK_JPEG_COMP_MAX		3
+
 enum {
 	MTK_JPEG_DEC_RESULT_EOF_DONE		= 0,
 	MTK_JPEG_DEC_RESULT_PAUSE		= 1,
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
index 244f9f7..3383dc0 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -183,6 +183,50 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
 	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
 }
 
+void mtk_jpegenc_put_buf(struct mtk_jpegenc_comp_dev *jpeg)
+{
+	struct mtk_jpeg_ctx *ctx;
+	struct vb2_v4l2_buffer *dst_buffer;
+	struct list_head *temp_entry;
+	struct list_head *pos = NULL;
+	struct mtk_jpeg_src_buf *dst_done_buf, *tmp_dst_done_buf;
+	unsigned long flags;
+
+	ctx = jpeg->hw_param.curr_ctx;
+	if (!ctx) {
+		dev_err(jpeg->dev, "comp_jpeg ctx fail !!!\n");
+		return;
+	}
+
+	dst_buffer = jpeg->hw_param.dst_buffer;
+	if (!dst_buffer) {
+		dev_err(jpeg->dev, "comp_jpeg dst_buffer fail !!!\n");
+		return;
+	}
+
+	dst_done_buf = container_of(dst_buffer,
+		struct mtk_jpeg_src_buf, b);
+
+	spin_lock_irqsave(&ctx->done_queue_lock, flags);
+	list_add_tail(&dst_done_buf->list, &ctx->dst_done_queue);
+	while (!list_empty(&ctx->dst_done_queue) &&
+		(pos != &ctx->dst_done_queue)) {
+		list_for_each_prev_safe(pos, temp_entry,
+			(&ctx->dst_done_queue)) {
+			tmp_dst_done_buf = list_entry(pos,
+				struct mtk_jpeg_src_buf, list);
+			if (tmp_dst_done_buf->frame_num ==
+				ctx->last_done_frame_num) {
+				list_del(&tmp_dst_done_buf->list);
+				v4l2_m2m_buf_done(&tmp_dst_done_buf->b,
+					VB2_BUF_STATE_DONE);
+				ctx->last_done_frame_num++;
+			}
+		}
+	}
+	spin_unlock_irqrestore(&ctx->done_queue_lock, flags);
+}
+
 static void mtk_jpegenc_timeout_work(struct work_struct *work)
 {
 	struct delayed_work *Pwork =
@@ -203,6 +247,7 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
 	atomic_inc(&cjpeg->hw_rdy);
 	wake_up(&master_jpeg->enc_hw_wq);
 	v4l2_m2m_buf_done(src_buf, buf_state);
+	mtk_jpegenc_put_buf(cjpeg);
 }
 
 static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
@@ -237,8 +282,7 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
 	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
 	buf_state = VB2_BUF_STATE_DONE;
 	v4l2_m2m_buf_done(src_buf, buf_state);
-	v4l2_m2m_buf_done(dst_buf, buf_state);
-	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
+	mtk_jpegenc_put_buf(jpeg);
 	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
 	pm_runtime_put(ctx->jpeg->dev);
 	if (ctx->fh.m2m_ctx &&
-- 
2.6.4


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

Re: [PATCH V6, 5/5] media: mtk-jpegenc: add output pic reorder interface

[Ricardo Ribalda]

kyrie.wu wrote:

> There are two HWs in mt8195. Since the two HWs run
> in parallel, it is necessary to reorder the output images
> to ensure that the order is consistent with the input images.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
> V6: no change
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 10 +----
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   | 17 +++++++-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c |  1 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h |  3 +-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 48 ++++++++++++++++++++++-
>  5 files changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index 80a6c1a..9e89629 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -106,15 +106,6 @@ static struct mtk_jpeg_fmt mtk_jpeg_dec_formats[] = {
>  #define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats)
>  #define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats)
>  
> -struct mtk_jpeg_src_buf {
> -	struct vb2_v4l2_buffer b;
> -	struct list_head list;
> -	struct mtk_jpeg_dec_param dec_param;
> -
> -	struct mtk_jpeg_ctx *curr_ctx;
> -	u32 frame_num;
> -};
> -
>  static int debug;
>  module_param(debug, int, 0644);
>  
> @@ -1344,6 +1335,7 @@ static int mtk_jpeg_open(struct file *file)
>  	if (jpeg->variant->is_encoder)
>  		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
>  
> +	INIT_LIST_HEAD(&ctx->dst_done_queue);
>  	v4l2_fh_init(&ctx->fh, vfd);
>  	file->private_data = &ctx->fh;
>  	v4l2_fh_add(&ctx->fh);
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 4c669af..f276221 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -13,10 +13,11 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fh.h>
> +#include <media/videobuf2-v4l2.h>
>  
> -#define MTK_JPEG_NAME		"mtk-jpeg"
> +#include "mtk_jpeg_dec_hw.h"
>  
> -#define MTK_JPEG_COMP_MAX		3
> +#define MTK_JPEG_NAME		"mtk-jpeg"
>  
>  #define MTK_JPEG_FMT_FLAG_OUTPUT	BIT(0)
>  #define MTK_JPEG_FMT_FLAG_CAPTURE	BIT(1)
> @@ -75,6 +76,15 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +struct mtk_jpeg_src_buf {
> +	struct vb2_v4l2_buffer b;
> +	struct list_head list;
> +	struct mtk_jpeg_dec_param dec_param;
> +
> +	struct mtk_jpeg_ctx *curr_ctx;
> +	u32 frame_num;
> +};
> +
>  enum mtk_jpeg_hw_state {
>  	MTK_JPEG_HW_IDLE = 0,
>  	MTK_JPEG_HW_BUSY = 1,
> @@ -232,6 +242,9 @@ struct mtk_jpeg_ctx {
>  	struct v4l2_ctrl_handler ctrl_hdl;
>  	struct work_struct jpeg_work;
>  	u32 total_frame_num;
> +	struct list_head dst_done_queue;
> +	spinlock_t done_queue_lock;
> +	u32 last_done_frame_num;
>  };
>  
>  extern struct platform_driver mtk_jpegenc_hw_driver;
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> index afbbfd5..1e38522 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> @@ -9,6 +9,7 @@
>  #include <linux/kernel.h>
>  #include <media/videobuf2-core.h>
>  
> +#include "mtk_jpeg_core.h"
>  #include "mtk_jpeg_dec_hw.h"
>  
>  #define MTK_JPEG_DUNUM_MASK(val)	(((val) - 1) & 0x3)
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> index fa0d45f..87aaa5c 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> @@ -11,9 +11,10 @@
>  
>  #include <media/videobuf2-core.h>
>  
> -#include "mtk_jpeg_core.h"
>  #include "mtk_jpeg_dec_reg.h"
>  
Why are you moving around this define?
> +#define MTK_JPEG_COMP_MAX		3
> +
>  enum {
>  	MTK_JPEG_DEC_RESULT_EOF_DONE		= 0,
>  	MTK_JPEG_DEC_RESULT_PAUSE		= 1,
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> index 244f9f7..3383dc0 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> @@ -183,6 +183,50 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
>  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
>  }
>  
> +void mtk_jpegenc_put_buf(struct mtk_jpegenc_comp_dev *jpeg)
> +{
> +	struct mtk_jpeg_ctx *ctx;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct list_head *temp_entry;
> +	struct list_head *pos = NULL;
> +	struct mtk_jpeg_src_buf *dst_done_buf, *tmp_dst_done_buf;
> +	unsigned long flags;
> +
> +	ctx = jpeg->hw_param.curr_ctx;
> +	if (!ctx) {
> +		dev_err(jpeg->dev, "comp_jpeg ctx fail !!!\n");
> +		return;
> +	}
> +
> +	dst_buffer = jpeg->hw_param.dst_buffer;
> +	if (!dst_buffer) {
> +		dev_err(jpeg->dev, "comp_jpeg dst_buffer fail !!!\n");
> +		return;
> +	}
> +
> +	dst_done_buf = container_of(dst_buffer,
> +		struct mtk_jpeg_src_buf, b);
> +
> +	spin_lock_irqsave(&ctx->done_queue_lock, flags);
> +	list_add_tail(&dst_done_buf->list, &ctx->dst_done_queue);
You want to run this until there are only 0 or 1 elements? Did I got it
right?

So this means that when you enter put_buf there is max one element in
the list. Do you need a list for that or only save one buffer in case
they have come out of order?
> +	while (!list_empty(&ctx->dst_done_queue) &&
> +		(pos != &ctx->dst_done_queue)) {
> +		list_for_each_prev_safe(pos, temp_entry,
> +			(&ctx->dst_done_queue)) {
> +			tmp_dst_done_buf = list_entry(pos,
> +				struct mtk_jpeg_src_buf, list);
> +			if (tmp_dst_done_buf->frame_num ==
> +				ctx->last_done_frame_num) {
> +				list_del(&tmp_dst_done_buf->list);
> +				v4l2_m2m_buf_done(&tmp_dst_done_buf->b,
> +					VB2_BUF_STATE_DONE);
> +				ctx->last_done_frame_num++;
> +			}
> +		}
> +	}
> +	spin_unlock_irqrestore(&ctx->done_queue_lock, flags);
> +}
> +
>  static void mtk_jpegenc_timeout_work(struct work_struct *work)
>  {
>  	struct delayed_work *Pwork =
> @@ -203,6 +247,7 @@ static void mtk_jpegenc_timeout_work(struct work_struct *work)
>  	atomic_inc(&cjpeg->hw_rdy);
>  	wake_up(&master_jpeg->enc_hw_wq);
>  	v4l2_m2m_buf_done(src_buf, buf_state);
> +	mtk_jpegenc_put_buf(cjpeg);
>  }
>  
>  static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> @@ -237,8 +282,7 @@ static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
>  	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
>  	buf_state = VB2_BUF_STATE_DONE;
>  	v4l2_m2m_buf_done(src_buf, buf_state);
> -	v4l2_m2m_buf_done(dst_buf, buf_state);
> -	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> +	mtk_jpegenc_put_buf(jpeg);
>  	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
>  	pm_runtime_put(ctx->jpeg->dev);
>  	if (ctx->fh.m2m_ctx &&
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

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

Re: [PATCH V6, 5/5] media: mtk-jpegenc: add output pic reorder interface

[kyrie.wu]

On Fri, 2021-12-03 at 16:50 +0100, Ricardo Ribalda wrote:
> kyrie.wu wrote:
> 
> > There are two HWs in mt8195. Since the two HWs run
> > in parallel, it is necessary to reorder the output images
> > to ensure that the order is consistent with the input images.
> > 
> > Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> > ---
> > V6: no change
> > ---
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 10 +----
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   | 17 +++++++-
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c |  1 +
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h |  3 +-
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 48
> > ++++++++++++++++++++++-
> >  5 files changed, 65 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index 80a6c1a..9e89629 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -106,15 +106,6 @@ static struct mtk_jpeg_fmt
> > mtk_jpeg_dec_formats[] = {
> >  #define MTK_JPEG_ENC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_enc_formats)
> >  #define MTK_JPEG_DEC_NUM_FORMATS ARRAY_SIZE(mtk_jpeg_dec_formats)
> >  
> > -struct mtk_jpeg_src_buf {
> > -	struct vb2_v4l2_buffer b;
> > -	struct list_head list;
> > -	struct mtk_jpeg_dec_param dec_param;
> > -
> > -	struct mtk_jpeg_ctx *curr_ctx;
> > -	u32 frame_num;
> > -};
> > -
> >  static int debug;
> >  module_param(debug, int, 0644);
> >  
> > @@ -1344,6 +1335,7 @@ static int mtk_jpeg_open(struct file *file)
> >  	if (jpeg->variant->is_encoder)
> >  		INIT_WORK(&ctx->jpeg_work, mtk_jpegenc_worker);
> >  
> > +	INIT_LIST_HEAD(&ctx->dst_done_queue);
> >  	v4l2_fh_init(&ctx->fh, vfd);
> >  	file->private_data = &ctx->fh;
> >  	v4l2_fh_add(&ctx->fh);
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > index 4c669af..f276221 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> > @@ -13,10 +13,11 @@
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-fh.h>
> > +#include <media/videobuf2-v4l2.h>
> >  
> > -#define MTK_JPEG_NAME		"mtk-jpeg"
> > +#include "mtk_jpeg_dec_hw.h"
> >  
> > -#define MTK_JPEG_COMP_MAX		3
> > +#define MTK_JPEG_NAME		"mtk-jpeg"
> >  
> >  #define MTK_JPEG_FMT_FLAG_OUTPUT	BIT(0)
> >  #define MTK_JPEG_FMT_FLAG_CAPTURE	BIT(1)
> > @@ -75,6 +76,15 @@ struct mtk_jpeg_variant {
> >  	u32 cap_q_default_fourcc;
> >  };
> >  
> > +struct mtk_jpeg_src_buf {
> > +	struct vb2_v4l2_buffer b;
> > +	struct list_head list;
> > +	struct mtk_jpeg_dec_param dec_param;
> > +
> > +	struct mtk_jpeg_ctx *curr_ctx;
> > +	u32 frame_num;
> > +};
> > +
> >  enum mtk_jpeg_hw_state {
> >  	MTK_JPEG_HW_IDLE = 0,
> >  	MTK_JPEG_HW_BUSY = 1,
> > @@ -232,6 +242,9 @@ struct mtk_jpeg_ctx {
> >  	struct v4l2_ctrl_handler ctrl_hdl;
> >  	struct work_struct jpeg_work;
> >  	u32 total_frame_num;
> > +	struct list_head dst_done_queue;
> > +	spinlock_t done_queue_lock;
> > +	u32 last_done_frame_num;
> >  };
> >  
> >  extern struct platform_driver mtk_jpegenc_hw_driver;
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> > index afbbfd5..1e38522 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/kernel.h>
> >  #include <media/videobuf2-core.h>
> >  
> > +#include "mtk_jpeg_core.h"
> >  #include "mtk_jpeg_dec_hw.h"
> >  
> >  #define MTK_JPEG_DUNUM_MASK(val)	(((val) - 1) & 0x3)
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> > index fa0d45f..87aaa5c 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h
> > @@ -11,9 +11,10 @@
> >  
> >  #include <media/videobuf2-core.h>
> >  
> > -#include "mtk_jpeg_core.h"
> >  #include "mtk_jpeg_dec_reg.h"
> >  
> 
> Why are you moving around this define?
this micro defination is only used for jpeg decode, not for common
using, move to it.
> > +#define MTK_JPEG_COMP_MAX		3
> > +
> >  enum {
> >  	MTK_JPEG_DEC_RESULT_EOF_DONE		= 0,
> >  	MTK_JPEG_DEC_RESULT_PAUSE		= 1,
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > index 244f9f7..3383dc0 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
> > @@ -183,6 +183,50 @@ void mtk_jpeg_set_enc_params(struct
> > mtk_jpeg_ctx *ctx,  void __iomem *base)
> >  	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
> >  }
> >  
> > +void mtk_jpegenc_put_buf(struct mtk_jpegenc_comp_dev *jpeg)
> > +{
> > +	struct mtk_jpeg_ctx *ctx;
> > +	struct vb2_v4l2_buffer *dst_buffer;
> > +	struct list_head *temp_entry;
> > +	struct list_head *pos = NULL;
> > +	struct mtk_jpeg_src_buf *dst_done_buf, *tmp_dst_done_buf;
> > +	unsigned long flags;
> > +
> > +	ctx = jpeg->hw_param.curr_ctx;
> > +	if (!ctx) {
> > +		dev_err(jpeg->dev, "comp_jpeg ctx fail !!!\n");
> > +		return;
> > +	}
> > +
> > +	dst_buffer = jpeg->hw_param.dst_buffer;
> > +	if (!dst_buffer) {
> > +		dev_err(jpeg->dev, "comp_jpeg dst_buffer fail !!!\n");
> > +		return;
> > +	}
> > +
> > +	dst_done_buf = container_of(dst_buffer,
> > +		struct mtk_jpeg_src_buf, b);
> > +
> > +	spin_lock_irqsave(&ctx->done_queue_lock, flags);
> > +	list_add_tail(&dst_done_buf->list, &ctx->dst_done_queue);
> 
> You want to run this until there are only 0 or 1 elements? Did I got
> it
> right?
Yes.
> 
> So this means that when you enter put_buf there is max one element in
> the list. Do you need a list for that or only save one buffer in case
> they have come out of order?
The explaination is shown as below:
1. the put_buf is used to reorder output buffer;
2. the ctx->last_done_frame_num will increase when encode finished
everytime;
3. if the output buffer's frame_num(tmp_dst_done_buf->frame_num) is
equal to ctx->last_done_frame_num, call v4l2_m2m_buf_done to finish
this time encoding.
4. if there is no or only one element, the reordering is not required.

> > +	while (!list_empty(&ctx->dst_done_queue) &&
> > +		(pos != &ctx->dst_done_queue)) {
> > +		list_for_each_prev_safe(pos, temp_entry,
> > +			(&ctx->dst_done_queue)) {
> > +			tmp_dst_done_buf = list_entry(pos,
> > +				struct mtk_jpeg_src_buf, list);
> > +			if (tmp_dst_done_buf->frame_num ==
> > +				ctx->last_done_frame_num) {
> > +				list_del(&tmp_dst_done_buf->list);
> > +				v4l2_m2m_buf_done(&tmp_dst_done_buf->b,
> > +					VB2_BUF_STATE_DONE);
> > +				ctx->last_done_frame_num++;
> > +			}
> > +		}
> > +	}
> > +	spin_unlock_irqrestore(&ctx->done_queue_lock, flags);
> > +}
> > +
> >  static void mtk_jpegenc_timeout_work(struct work_struct *work)
> >  {
> >  	struct delayed_work *Pwork =
> > @@ -203,6 +247,7 @@ static void mtk_jpegenc_timeout_work(struct
> > work_struct *work)
> >  	atomic_inc(&cjpeg->hw_rdy);
> >  	wake_up(&master_jpeg->enc_hw_wq);
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> > +	mtk_jpegenc_put_buf(cjpeg);
> >  }
> >  
> >  static irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> > @@ -237,8 +282,7 @@ static irqreturn_t
> > mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> >  	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> >  	buf_state = VB2_BUF_STATE_DONE;
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> > -	v4l2_m2m_buf_done(dst_buf, buf_state);
> > -	v4l2_m2m_job_finish(master_jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +	mtk_jpegenc_put_buf(jpeg);
> >  	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
> >  	pm_runtime_put(ctx->jpeg->dev);
> >  	if (ctx->fh.m2m_ctx &&
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH V6, 0/5] Support multi-hardware jpeg encoding using of_platform_populate

[Ricardo Ribalda]

Hi

Any idea why this series is not available at
https://patchwork.linuxtv.org/ but it exists in 
https://lore.kernel.org/all/1638501230-13417-1-git-send-email-kyrie.wu@mediatek.com/#r

thanks!

kyrie.wu wrote:

> This series adds support for multi hardware jpeg encoding, by first
> adding use of_platform_populate to manage each hardware information:
> interrupt, clock, register bases and power. Secondly add encoding 
> work queue to deal with the encoding requestsof multi-hardware
> at the same time. Lastly, add output picture reorder function
> interface to eliminate the out of order images.
> 
> This series has been tested with both MT8195.
> Encoding worked for this chip.
> 
> Patches 1~2 use of_platform_populate to replace component framework
> to manage multi-hardware.
> 
> Patch 3 add jpeg encoding timeout function to judge hardware timeout.
> 
> Patch 4 add encoding work queue to deal with multi-hardware encoding
> at the same time.
> 
> Patch 5 add output picture reorder function to order images.
> ---
> Changes compared with v5:
> - use of_platform_populate to replace component framework to
> manage multi-hardware in patch 2.
> 
> Changes compared with v4:
> --No change compaered with v4
> 
> Changes compared with v3:
> --Structure patches for consistency, non-backward
>   compatible and do not break any existing functionality
> 
> Changes compared with v2:
> --Split the last two patches into several patches
>   to enhance readability
> --Correct some syntax errors
> --Explain why the component framework is used
> 
> Changes compared with v1:
> --Add jpeg encoder dt-bindings for MT8195
> --Use component framework to manage jpegenc HW
> --Add jpegenc output pic reorder function interface
> 
> kyrie.wu (5):
>   dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
>   media: mtk-jpegenc: manage jpegenc multi-hardware
>   media: mtk-jpegenc: add jpegenc timeout func interface
>   media: mtk-jpegenc: add jpeg encode worker interface
>   media: mtk-jpegenc: add output pic reorder interface
> 
>  .../bindings/media/mediatek-jpeg-encoder.yaml      |   3 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c    | 287 +++++++++++++++----
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h    |  91 +++++-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c  |   1 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h  |   3 +-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c  | 316 ++++++++++++++++++++-
>  6 files changed, 644 insertions(+), 57 deletions(-)
> 
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
> 

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

Re: [PATCH V6, 0/5] Support multi-hardware jpeg encoding using of_platform_populate

[kyrie.wu]

On Fri, 2021-12-03 at 14:38 +0100, Ricardo Ribalda wrote:
> Hi
> 
> Any idea why this series is not available at
> https://patchwork.linuxtv.org/ but it exists in 
> 
https://lore.kernel.org/all/1638501230-13417-1-git-send-email-kyrie.wu@mediatek.com/#r
> 
> thanks!
I will fix this issue in the next code version, thanks.
> 
> kyrie.wu wrote:
> 
> > This series adds support for multi hardware jpeg encoding, by first
> > adding use of_platform_populate to manage each hardware
> > information:
> > interrupt, clock, register bases and power. Secondly add encoding 
> > work queue to deal with the encoding requestsof multi-hardware
> > at the same time. Lastly, add output picture reorder function
> > interface to eliminate the out of order images.
> > 
> > This series has been tested with both MT8195.
> > Encoding worked for this chip.
> > 
> > Patches 1~2 use of_platform_populate to replace component framework
> > to manage multi-hardware.
> > 
> > Patch 3 add jpeg encoding timeout function to judge hardware
> > timeout.
> > 
> > Patch 4 add encoding work queue to deal with multi-hardware
> > encoding
> > at the same time.
> > 
> > Patch 5 add output picture reorder function to order images.
> > ---
> > Changes compared with v5:
> > - use of_platform_populate to replace component framework to
> > manage multi-hardware in patch 2.
> > 
> > Changes compared with v4:
> > --No change compaered with v4
> > 
> > Changes compared with v3:
> > --Structure patches for consistency, non-backward
> >   compatible and do not break any existing functionality
> > 
> > Changes compared with v2:
> > --Split the last two patches into several patches
> >   to enhance readability
> > --Correct some syntax errors
> > --Explain why the component framework is used
> > 
> > Changes compared with v1:
> > --Add jpeg encoder dt-bindings for MT8195
> > --Use component framework to manage jpegenc HW
> > --Add jpegenc output pic reorder function interface
> > 
> > kyrie.wu (5):
> >   dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
> >   media: mtk-jpegenc: manage jpegenc multi-hardware
> >   media: mtk-jpegenc: add jpegenc timeout func interface
> >   media: mtk-jpegenc: add jpeg encode worker interface
> >   media: mtk-jpegenc: add output pic reorder interface
> > 
> >  .../bindings/media/mediatek-jpeg-encoder.yaml      |   3 +
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c    | 287
> > +++++++++++++++----
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h    |  91 +++++-
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.c  |   1 +
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_dec_hw.h  |   3 +-
> >  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c  | 316
> > ++++++++++++++++++++-
> >  6 files changed, 644 insertions(+), 57 deletions(-)
> > 
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > Linux-mediatek mailing list
> > Linux-mediatek@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > 
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek