[PATCH v2,0/9] Support jpeg encode for MT8195

[PATCH v2,0/9] Support jpeg encode for MT8195

[kyrie.wu]

add component framework to using multi-HW for MT8195 jpeg encode.

kyrie.wu (9):
  dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
  media: mtk-jpegenc: Add MT8195 JPEG venc driver
  media: mtk-jpegenc: remove redundant code of irq
  media: mtk-jpegenc: Refactor jpeg clock interface
  media: mtk-jpegenc: Generalize jpeg encode irq interfaces
  media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces
  media: mtk-jpegenc: Use component framework to manage each hardware
    information
  media: mtk-jpegenc: Generalize jpegenc HW operations interfaces
  media: mtk-jpegenc: Refactor jpegenc device run interface

 .../bindings/media/mediatek-jpeg-encoder.yaml      |   3 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c    | 600 +++++++++++++++++----
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h    |  69 ++-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c  | 208 +++++++
 4 files changed, 786 insertions(+), 94 deletions(-)

---
This patch dependents on "dt-bindings: mediatek: convert mtk jpeg decoder/encoder to yaml"[1]

Please also accept this patch together with [1].

[1]https://lore.kernel.org/patchwork/patch/1445298/
---

-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 1/9] 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>
---
 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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2,2/9] media: mtk-jpegenc: Add MT8195 JPEG venc driver

[kyrie.wu]

Add MT8195 JPEG venc driver's compatible and device private data.
compatible = "mediatek,mt8195-jpgenc": this node would only register
jpgenc device node;
compatible = "mediatek,mt8195-jpgenc0": HW0 node, this node would not
register jpgenc device node, but register irq, init clk and power,
remmap register base and do other resource options;
compatible = "mediatek,mt8195-jpgenc1": HW1 node, just like HW0 node;

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 11 +++++------
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  1 +
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 12 ++++++++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 88a23bc..908aa1f 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1524,13 +1524,13 @@ static const struct mtk_jpeg_variant mt8173_jpeg_drvdata = {
 	.cap_q_default_fourcc = V4L2_PIX_FMT_YUV420M,
 };
 
-static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
+static const struct mtk_jpeg_variant mtk_jpegenc_drvdata = {
+	.is_encoder	= true,
 	.clks = mtk_jpeg_clocks,
 	.num_clks = ARRAY_SIZE(mtk_jpeg_clocks),
 	.formats = mtk_jpeg_enc_formats,
 	.num_formats = MTK_JPEG_ENC_NUM_FORMATS,
 	.qops = &mtk_jpeg_enc_qops,
-	.irq_handler = mtk_jpeg_enc_irq,
 	.hw_reset = mtk_jpeg_enc_reset,
 	.m2m_ops = &mtk_jpeg_enc_m2m_ops,
 	.dev_name = "mtk-jpeg-enc",
@@ -1540,6 +1540,9 @@ static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
 };
 
 static const struct of_device_id mtk_jpeg_match[] = {
+	{	.compatible = "mediatek,mt8195-jpgenc",
+		.data = &mtk_jpegenc_drvdata,
+	},
 	{
 		.compatible = "mediatek,mt8173-jpgdec",
 		.data = &mt8173_jpeg_drvdata,
@@ -1548,10 +1551,6 @@ static const struct of_device_id mtk_jpeg_match[] = {
 		.compatible = "mediatek,mt2701-jpgdec",
 		.data = &mt8173_jpeg_drvdata,
 	},
-	{
-		.compatible = "mediatek,mtk-jpgenc",
-		.data = &mtk_jpeg_drvdata,
-	},
 	{},
 };
 
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 595f7f1..bdbd768 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;
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..3da1011 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -152,3 +152,15 @@ void mtk_jpeg_set_enc_params(struct mtk_jpeg_ctx *ctx,  void __iomem *base)
 
 	writel(ctx->restart_interval, base + JPEG_ENC_RST_MCU_NUM);
 }
+
+#if defined(CONFIG_OF)
+static const struct of_device_id mtk_jpegenc_hw_ids[] = {
+	{
+		.compatible = "mediatek,mt8195-jpgenc0",
+	},
+	{	.compatible = "mediatek,mt8195-jpgenc1",
+	},
+	{},
+};
+MODULE_DEVICE_TABLE(of, mtk_jpegenc_hw_ids);
+#endif
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,2/9] media: mtk-jpegenc: Add MT8195 JPEG venc driver

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:32 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Add MT8195 JPEG venc driver's compatible and device private data.
> compatible = "mediatek,mt8195-jpgenc": this node would only register
> jpgenc device node;
> compatible = "mediatek,mt8195-jpgenc0": HW0 node, this node would not
> register jpgenc device node, but register irq, init clk and power,
> remmap register base and do other resource options;
> compatible = "mediatek,mt8195-jpgenc1": HW1 node, just like HW0 node;
The commit message is not easy to read.  Please rephrase the sentences.

What does "venc" stand for?  I believe it is a copy-n-paste typo.

The commit title "support MT8195 JPEG encoder" looks better to me.

> -static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
> +static const struct mtk_jpeg_variant mtk_jpegenc_drvdata = {
Why remove mtk_jpeg_drvdata?

> -       .irq_handler = mtk_jpeg_enc_irq,
Why remove the IRQ handler?

> @@ -1548,10 +1551,6 @@ static const struct of_device_id mtk_jpeg_match[] = {
>                 .compatible = "mediatek,mt2701-jpgdec",
>                 .data = &mt8173_jpeg_drvdata,
>         },
> -       {
> -               .compatible = "mediatek,mtk-jpgenc",
> -               .data = &mtk_jpeg_drvdata,
> -       },
Why remove "mediatek,mtk-jpgenc"?

> +#if defined(CONFIG_OF)
> +static const struct of_device_id mtk_jpegenc_hw_ids[] = {
> +       {
> +               .compatible = "mediatek,mt8195-jpgenc0",
> +       },
> +       {       .compatible = "mediatek,mt8195-jpgenc1",
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_jpegenc_hw_ids);
> +#endif
Would expect somewhere to reference mtk_jpegenc_hw_ids but failed to find it.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2,3/9] media: mtk-jpegenc: remove redundant code of irq

[kyrie.wu]

the func of jpgenc irq handler would not compatible, remove those
code.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 48 -------------------------
 1 file changed, 48 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 908aa1f..24edd87 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1072,54 +1072,6 @@ static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
 	mtk_smi_larb_put(jpeg->larb);
 }
 
-static irqreturn_t mtk_jpeg_enc_done(struct mtk_jpeg_dev *jpeg)
-{
-	struct mtk_jpeg_ctx *ctx;
-	struct vb2_v4l2_buffer *src_buf, *dst_buf;
-	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
-	u32 result_size;
-
-	ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
-	if (!ctx) {
-		v4l2_err(&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(jpeg->m2m_dev, ctx->fh.m2m_ctx);
-	pm_runtime_put(ctx->jpeg->dev);
-	return IRQ_HANDLED;
-}
-
-static irqreturn_t mtk_jpeg_enc_irq(int irq, void *priv)
-{
-	struct mtk_jpeg_dev *jpeg = priv;
-	u32 irq_status;
-	irqreturn_t ret = IRQ_NONE;
-
-	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)
-		writel(0, jpeg->reg_base + JPEG_ENC_INT_STS);
-
-	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE))
-		return ret;
-
-	ret = mtk_jpeg_enc_done(jpeg);
-	return ret;
-}
-
 static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
 {
 	struct mtk_jpeg_dev *jpeg = priv;
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,3/9] media: mtk-jpegenc: remove redundant code of irq

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> the func of jpgenc irq handler would not compatible, remove those
> code.
Need more explanation about why as I believe it is non-backward compatible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,3/9] media: mtk-jpegenc: remove redundant code of irq

[Tomasz Figa]

On Tue, Jul 06, 2021 at 07:00:33PM +0800, Tzung-Bi Shih wrote:
> On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> > the func of jpgenc irq handler would not compatible, remove those
> > code.
> Need more explanation about why as I believe it is non-backward compatible.

Right. And it breaks bisection, which is not acceptable.

Kyrie, please structure your series in a way that none of the patches
break any existing functionality.

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface

[kyrie.wu]

Using the needed param for lock on/off function.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 46 ++++++++++++++++++++++++-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 28 +++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 24edd87..7c053e3 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1053,7 +1053,32 @@ static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq,
 
 static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
 {
-	int ret;
+	struct mtk_jpeg_dev *comp_dev;
+	struct mtk_jpegenc_pm *pm;
+	struct mtk_jpegenc_clk *jpegclk;
+	struct mtk_jpegenc_clk_info *clk_info;
+	int ret, i;
+
+	if (jpeg->variant->is_encoder) {
+		for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+			comp_dev = jpeg->hw_dev[i];
+			if (!comp_dev) {
+				dev_err(jpeg->dev, "Failed to get hw dev\n");
+				return;
+			}
+
+			pm = &comp_dev->pm;
+			jpegclk = &pm->venc_clk;
+			clk_info = jpegclk->clk_info;
+			ret = clk_prepare_enable(clk_info->jpegenc_clk);
+			if (ret) {
+				dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n",
+				       i, jpegclk->clk_info->clk_name);
+				return;
+			}
+		}
+		return;
+	}
 
 	ret = mtk_smi_larb_get(jpeg->larb);
 	if (ret)
@@ -1067,6 +1092,25 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
 
 static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
 {
+	struct mtk_jpeg_dev *comp_dev;
+	struct mtk_jpegenc_pm *pm;
+	struct mtk_jpegenc_clk *jpegclk;
+	int i;
+
+	if (jpeg->variant->is_encoder) {
+		for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+			comp_dev = jpeg->hw_dev[i];
+			if (!comp_dev) {
+				dev_err(jpeg->dev, "Failed to get hw dev\n");
+				return;
+			}
+
+			pm = &comp_dev->pm;
+			jpegclk = &pm->venc_clk;
+			clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk);
+		}
+		return;
+	}
 	clk_bulk_disable_unprepare(jpeg->variant->num_clks,
 				   jpeg->variant->clks);
 	mtk_smi_larb_put(jpeg->larb);
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index bdbd768..93ea71c 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -75,6 +75,31 @@ 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_jpeg_dev	*mtkdev;
+};
+
 /**
  * struct mtk_jpeg_dev - JPEG IP abstraction
  * @lock:		the mutex protecting this structure
@@ -103,6 +128,9 @@ struct mtk_jpeg_dev {
 	struct device		*larb;
 	struct delayed_work job_timeout_work;
 	const struct mtk_jpeg_variant *variant;
+
+	struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];
+	struct mtk_jpegenc_pm pm;
 };
 
 /**
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Using the needed param for lock on/off function.
The description makes less sense.

The patch is more than a "refactor".  Please change the title accordingly.

>  static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
>  {
> -       int ret;
> +       struct mtk_jpeg_dev *comp_dev;
> +       struct mtk_jpegenc_pm *pm;
> +       struct mtk_jpegenc_clk *jpegclk;
> +       struct mtk_jpegenc_clk_info *clk_info;
> +       int ret, i;
> +
> +       if (jpeg->variant->is_encoder) {
> +               for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +                       comp_dev = jpeg->hw_dev[i];
> +                       if (!comp_dev) {
> +                               dev_err(jpeg->dev, "Failed to get hw dev\n");
> +                               return;
> +                       }
> +
> +                       pm = &comp_dev->pm;
> +                       jpegclk = &pm->venc_clk;
> +                       clk_info = jpegclk->clk_info;
> +                       ret = clk_prepare_enable(clk_info->jpegenc_clk);
> +                       if (ret) {
> +                               dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n",
> +                                      i, jpegclk->clk_info->clk_name);
> +                               return;
> +                       }
> +               }
> +               return;
> +       }
Doesn't it need to call clk_disable_unprepare() in the error paths?

> +                       pm = &comp_dev->pm;
> +                       jpegclk = &pm->venc_clk;
> +                       clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk);
Consistency issue: mtk_jpeg_clk_on() has struct mtk_jpegenc_clk_info
*clk_info.  Why not also have the local variable here?

Is it a good idea to just separate functions for ->is_encoder for
mtk_jpeg_clk_on() and mtk_jpeg_clk_off()?  For example,
mtk_jpegenc_clk_on() and mtk_jpegdec_clk_on().

> +/** * 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_jpeg_dev     *mtkdev;
> +};
> +
>  /**
>   * struct mtk_jpeg_dev - JPEG IP abstraction
>   * @lock:              the mutex protecting this structure
> @@ -103,6 +128,9 @@ struct mtk_jpeg_dev {
>         struct device           *larb;
>         struct delayed_work job_timeout_work;
>         const struct mtk_jpeg_variant *variant;
> +
> +       struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];
> +       struct mtk_jpegenc_pm pm;
>  };
If the declaration cannot align, using 1-space is sufficient.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,4/9] media: mtk-jpegenc: Refactor jpeg clock interface

[Tomasz Figa]

Hi Kyrie,

On Wed, Jun 30, 2021 at 03:27:54PM +0800, kyrie.wu wrote:
> Using the needed param for lock on/off function.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 46 ++++++++++++++++++++++++-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 28 +++++++++++++++
>  2 files changed, 73 insertions(+), 1 deletion(-)
>

Thanks for the patch. Please see my comments inline.

Also, how does this patch refactor anything? I only see new code being
added. Does the subject and/or commit message need some adjustment?

> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index 24edd87..7c053e3 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -1053,7 +1053,32 @@ static int mtk_jpeg_queue_init(void *priv, struct vb2_queue *src_vq,
>  
>  static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
>  {
> -	int ret;
> +	struct mtk_jpeg_dev *comp_dev;
> +	struct mtk_jpegenc_pm *pm;
> +	struct mtk_jpegenc_clk *jpegclk;
> +	struct mtk_jpegenc_clk_info *clk_info;
> +	int ret, i;
> +
> +	if (jpeg->variant->is_encoder) {
> +		for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {

Why do we need to enable clocks for all hardware instances? Wouldn't it
make more sense to only enable the clock for the instance that is
selected for given encode job?

> +			comp_dev = jpeg->hw_dev[i];
> +			if (!comp_dev) {
> +				dev_err(jpeg->dev, "Failed to get hw dev\n");
> +				return;
> +			}
> +
> +			pm = &comp_dev->pm;
> +			jpegclk = &pm->venc_clk;
> +			clk_info = jpegclk->clk_info;
> +			ret = clk_prepare_enable(clk_info->jpegenc_clk);
> +			if (ret) {
> +				dev_err(jpeg->dev, "jpegenc clk enable %d %s fail\n",
> +				       i, jpegclk->clk_info->clk_name);

Missing undo. (But the suggestion below would take care of it.)

> +				return;
> +			}
> +		}

How about using the clk_bulk_ API instead of the open coded loop?

> +		return;
> +	}

Rather than multiple if/else variants in one function, it's a common
practice to have two separate functions and then a function pointer in a
hardware variant descriptor struct pointing to the right function. It
makes the code more readable.

>  
>  	ret = mtk_smi_larb_get(jpeg->larb);
>  	if (ret)
> @@ -1067,6 +1092,25 @@ static void mtk_jpeg_clk_on(struct mtk_jpeg_dev *jpeg)
>  
>  static void mtk_jpeg_clk_off(struct mtk_jpeg_dev *jpeg)
>  {
> +	struct mtk_jpeg_dev *comp_dev;
> +	struct mtk_jpegenc_pm *pm;
> +	struct mtk_jpegenc_clk *jpegclk;
> +	int i;
> +
> +	if (jpeg->variant->is_encoder) {
> +		for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +			comp_dev = jpeg->hw_dev[i];
> +			if (!comp_dev) {
> +				dev_err(jpeg->dev, "Failed to get hw dev\n");
> +				return;
> +			}
> +
> +			pm = &comp_dev->pm;
> +			jpegclk = &pm->venc_clk;
> +			clk_disable_unprepare(jpegclk->clk_info->jpegenc_clk);
> +		}
> +		return;
> +	}

Same comments here as for the clk_on function.

>  	clk_bulk_disable_unprepare(jpeg->variant->num_clks,
>  				   jpeg->variant->clks);
>  	mtk_smi_larb_put(jpeg->larb);
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index bdbd768..93ea71c 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,31 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +enum mtk_jpegenc_hw_id {
> +	MTK_JPEGENC_HW0,
> +	MTK_JPEGENC_HW1,
> +	MTK_JPEGENC_HW_MAX,
> +};

There is no added value from the enum above. Just use integer index,

> +
> +/** * 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;
> +};

This looks like the generic clk_bulk_data struct.

> +
> +/** * struct mtk_vcodec_pm - Power management data structure */

vcodec?

> +struct mtk_jpegenc_pm {
> +	struct mtk_jpegenc_clk	venc_clk;

venc?

> +	struct device	*dev;
> +	struct mtk_jpeg_dev	*mtkdev;
> +};
> +
>  /**
>   * struct mtk_jpeg_dev - JPEG IP abstraction
>   * @lock:		the mutex protecting this structure
> @@ -103,6 +128,9 @@ struct mtk_jpeg_dev {
>  	struct device		*larb;
>  	struct delayed_work job_timeout_work;
>  	const struct mtk_jpeg_variant *variant;
> +
> +	struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];

Why is this recursively having the same struct as its children?
Should we have a separate struct that describes a hardware instance
(core?)?

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 5/9] media: mtk-jpegenc: Generalize jpeg encode irq interfaces

[kyrie.wu]

Generalizes jpeg encode irq interfaces to support different hardware.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 124 +++++++++++++++++++++++-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h |  24 ++++-
 2 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 7c053e3..062feab 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -106,10 +106,40 @@ 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)
 
+/*
+ * mtk_jpeg_enc_param:  General jpeg encoding parameters
+ * @enc_w:		image width
+ * @enc_h:		image height
+ * @enable_exif:	EXIF enable for jpeg encode mode
+ * @enc_quality:	destination image quality in encode mode
+ * @enc_format:		input image format
+ * @restart_interval:	JPEG restart interval for JPEG encoding
+ * @img_stride:		jpeg encoder image stride
+ * @mem_stride:		jpeg encoder memory stride
+ * @total_encdu:	total 8x8 block number
+ */
+struct mtk_jpeg_enc_param {
+	u32 enc_w;
+	u32 enc_h;
+	u32 enable_exif;
+	u32 enc_quality;
+	u32 enc_format;
+	u32 restart_interval;
+	u32 img_stride;
+	u32 mem_stride;
+	u32 total_encdu;
+};
+
 struct mtk_jpeg_src_buf {
 	struct vb2_v4l2_buffer b;
 	struct list_head list;
+	u32 frame_num;
+	u32 bs_size;
+	int flags;
 	struct mtk_jpeg_dec_param dec_param;
+
+	struct mtk_jpeg_enc_param enc_param;
+	struct mtk_jpeg_ctx *curr_ctx;
 };
 
 static int debug;
@@ -1163,6 +1193,98 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+void mtk_jpeg_put_buf(struct mtk_jpeg_dev *jpeg)
+{
+	struct mtk_jpeg_ctx *ctx;
+	struct vb2_v4l2_buffer *dst_buffer;
+	struct list_head *temp_entry;
+	struct list_head *pos;
+	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 = mtk_jpeg_vb2_to_srcbuf(&dst_buffer->vb2_buf);
+
+	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);
+}
+
+irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
+{
+	struct mtk_jpeg_dev *jpeg = priv;
+	struct mtk_jpeg_ctx *ctx;
+	struct mtk_jpeg_dev *master_jpeg;
+	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+
+	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
+	u32 result_size;
+	u32 irq_status;
+
+	cancel_delayed_work(&jpeg->job_timeout_work);
+
+	src_buf = jpeg->hw_param.src_buffer;
+	dst_buf = jpeg->hw_param.dst_buffer;
+	ctx = jpeg->hw_param.curr_ctx;
+	master_jpeg = ctx->jpeg;
+	irq_status = readl(jpeg->reg_base[MTK_JPEGENC_HW0] + JPEG_ENC_INT_STS) &
+		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
+	if (irq_status)
+		writel(0, jpeg->reg_base[MTK_JPEGENC_HW0] + JPEG_ENC_INT_STS);
+	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE)) {
+		dev_err(jpeg->dev, "jpeg encode failed !!!\n");
+		goto irq_end;
+	}
+
+	result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base[0]);
+	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
+
+	buf_state = VB2_BUF_STATE_DONE;
+
+irq_end:
+	v4l2_m2m_buf_done(src_buf, buf_state);
+	mtk_jpeg_put_buf(jpeg);
+	pm_runtime_put(jpeg->pm.dev);
+	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
+	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->hw_wq);
+	atomic_inc(&jpeg->hw_rdy);
+	return IRQ_HANDLED;
+}
+
 static void mtk_jpeg_set_default_params(struct mtk_jpeg_ctx *ctx)
 {
 	struct mtk_jpeg_q_data *q = &ctx->out_q;
@@ -1352,7 +1474,7 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 	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);
+	jpeg->reg_base[0] = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(jpeg->reg_base)) {
 		ret = PTR_ERR(jpeg->reg_base);
 		return ret;
diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
index 93ea71c..03ff060 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -75,6 +75,17 @@ 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_jpeg_hw_state {
+	MTK_JPEG_HW_IDLE = 0,
+	MTK_JPEG_HW_BUSY = 1,
+};
+
 enum mtk_jpegenc_hw_id {
 	MTK_JPEGENC_HW0,
 	MTK_JPEGENC_HW1,
@@ -124,13 +135,18 @@ struct mtk_jpeg_dev {
 	struct v4l2_m2m_dev	*m2m_dev;
 	void			*alloc_ctx;
 	struct video_device	*vdev;
-	void __iomem		*reg_base;
 	struct device		*larb;
 	struct delayed_work job_timeout_work;
 	const struct mtk_jpeg_variant *variant;
 
+	void __iomem *reg_base[MTK_JPEGENC_HW_MAX];
+	int jpegenc_irq;
 	struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];
 	struct mtk_jpegenc_pm pm;
+	enum mtk_jpeg_hw_state hw_state;
+	struct mtk_jpeg_hw_param hw_param;
+	wait_queue_head_t hw_wq;
+	atomic_t hw_rdy;
 };
 
 /**
@@ -189,6 +205,12 @@ struct mtk_jpeg_ctx {
 	u8 enc_quality;
 	u8 restart_interval;
 	struct v4l2_ctrl_handler ctrl_hdl;
+
+	struct list_head dst_done_queue;
+	spinlock_t done_queue_lock;	/* spinlock protecting done queue */
+	u32 total_frame_num;
+	u32 last_done_frame_num;
+	struct work_struct jpeg_work;
 };
 
 #endif /* _MTK_JPEG_CORE_H */
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2, 5/9] media: mtk-jpegenc: Generalize jpeg encode irq interfaces

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Generalizes jpeg encode irq interfaces to support different hardware.
There are some missing pieces for using the code.  I guess the patch
needs to be submitted with other patches or needs to further be
divided.

> + * mtk_jpeg_enc_param:  General jpeg encoding parameters
> + * @enc_w:             image width
> + * @enc_h:             image height
> + * @enable_exif:       EXIF enable for jpeg encode mode
> + * @enc_quality:       destination image quality in encode mode
> + * @enc_format:                input image format
> + * @restart_interval:  JPEG restart interval for JPEG encoding
> + * @img_stride:                jpeg encoder image stride
> + * @mem_stride:                jpeg encoder memory stride
> + * @total_encdu:       total 8x8 block number
They are not well-aligned.

> +struct mtk_jpeg_enc_param {
> +       u32 enc_w;
> +       u32 enc_h;
> +       u32 enable_exif;
> +       u32 enc_quality;
> +       u32 enc_format;
> +       u32 restart_interval;
> +       u32 img_stride;
> +       u32 mem_stride;
> +       u32 total_encdu;
> +};
They are not used.

> +       u32 bs_size;
> +       int flags;
They are not used.

> +       struct mtk_jpeg_enc_param enc_param;
> +       struct mtk_jpeg_ctx *curr_ctx;
They are not used.

> +void mtk_jpeg_put_buf(struct mtk_jpeg_dev *jpeg)
> +{
> +       struct mtk_jpeg_ctx *ctx;
> +       struct vb2_v4l2_buffer *dst_buffer;
> +       struct list_head *temp_entry;
> +       struct list_head *pos;
> +       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;
> +       }
The caller "mtk_jpegenc_hw_irq_handler()" doesn't even check ctx and
dst_buffer.  Does mtk_jpeg_put_buf() need to validate them?

> +       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)) {
Why does it need to compare `pos != &ctx->dst_done_queue`?  On a
related note, at the first time, pos will be some garbage data from
stack.

> +irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
No code is using mtk_jpegenc_hw_irq_handler.  Have no enough context
to review the code.

> +       src_buf = jpeg->hw_param.src_buffer;
> +       dst_buf = jpeg->hw_param.dst_buffer;
> +       ctx = jpeg->hw_param.curr_ctx;
> +       master_jpeg = ctx->jpeg;
Could they be inlined to above where the variables are declared?

> +enum mtk_jpeg_hw_state {
> +       MTK_JPEG_HW_IDLE = 0,
> +       MTK_JPEG_HW_BUSY = 1,
MTK_JPEG_HW_BUSY is not used.

> @@ -124,13 +135,18 @@ struct mtk_jpeg_dev {
>         struct v4l2_m2m_dev     *m2m_dev;
>         void                    *alloc_ctx;
>         struct video_device     *vdev;
> -       void __iomem            *reg_base;
>         struct device           *larb;
>         struct delayed_work job_timeout_work;
>         const struct mtk_jpeg_variant *variant;
>
> +       void __iomem *reg_base[MTK_JPEGENC_HW_MAX];
> +       int jpegenc_irq;
jpegenc_irq is not used.

> @@ -189,6 +205,12 @@ struct mtk_jpeg_ctx {
>         u8 enc_quality;
>         u8 restart_interval;
>         struct v4l2_ctrl_handler ctrl_hdl;
> +
> +       struct list_head dst_done_queue;
> +       spinlock_t done_queue_lock;     /* spinlock protecting done queue */
> +       u32 total_frame_num;
total_frame_num is not used.


Need to double confirm: why sometimes the code uses
jpeg->reg_base[MTK_JPEGENC_HW0] but sometimes jpeg->reg_base[0]?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,5/9] media: mtk-jpegenc: Generalize jpeg encode irq interfaces

[Tomasz Figa]

On Wed, Jun 30, 2021 at 03:27:55PM +0800, kyrie.wu wrote:
> Generalizes jpeg encode irq interfaces to support different hardware.
> 
> Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 124 +++++++++++++++++++++++-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h |  24 ++++-
>  2 files changed, 146 insertions(+), 2 deletions(-)
> 

This again doesn't look like a refactor, because there is only new code
added.

Also see my comments inline.

> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index 7c053e3..062feab 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -106,10 +106,40 @@ 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)
>  
> +/*
> + * mtk_jpeg_enc_param:  General jpeg encoding parameters
> + * @enc_w:		image width
> + * @enc_h:		image height
> + * @enable_exif:	EXIF enable for jpeg encode mode
> + * @enc_quality:	destination image quality in encode mode
> + * @enc_format:		input image format
> + * @restart_interval:	JPEG restart interval for JPEG encoding
> + * @img_stride:		jpeg encoder image stride
> + * @mem_stride:		jpeg encoder memory stride
> + * @total_encdu:	total 8x8 block number
> + */
> +struct mtk_jpeg_enc_param {
> +	u32 enc_w;
> +	u32 enc_h;
> +	u32 enable_exif;
> +	u32 enc_quality;
> +	u32 enc_format;
> +	u32 restart_interval;
> +	u32 img_stride;
> +	u32 mem_stride;

What is the difference between these two strides?

> +	u32 total_encdu;

Is it really necessary to store this? Isn't it a trivial computation
from width and height.

> +};
> +
>  struct mtk_jpeg_src_buf {
>  	struct vb2_v4l2_buffer b;
>  	struct list_head list;
> +	u32 frame_num;

Isn't this equivalent to the sequence field inside vb2_v4l2_buffer?

> +	u32 bs_size;
> +	int flags;

What are these 2? I don't see them used in the code added by this patch.
Please add new fields in the same patch that adds the code using them.

>  	struct mtk_jpeg_dec_param dec_param;
> +
> +	struct mtk_jpeg_enc_param enc_param;

We can put these two into an union to reduce the size of the struct.

> +	struct mtk_jpeg_ctx *curr_ctx;
>  };
>  
>  static int debug;
> @@ -1163,6 +1193,98 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
>  	return IRQ_HANDLED;
>  }
>  
> +void mtk_jpeg_put_buf(struct mtk_jpeg_dev *jpeg)
> +{
> +	struct mtk_jpeg_ctx *ctx;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct list_head *temp_entry;
> +	struct list_head *pos;
> +	struct mtk_jpeg_src_buf *dst_done_buf, *tmp_dst_done_buf;

Wait, is the buffer src or dst?

> +	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");

When can this happen?

> +		return;
> +	}
> +
> +	dst_done_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buffer->vb2_buf);
> +
> +	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++;
> +			}
> +		}
> +	}

Do we need the outer while loop here?

Also, is the prev variant of list_for_each needed here? Wouldn't
list_for_each_entry_safe() be enough?

> +	spin_unlock_irqrestore(&ctx->done_queue_lock, flags);
> +}
> +
> +irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
> +{
> +	struct mtk_jpeg_dev *jpeg = priv;
> +	struct mtk_jpeg_ctx *ctx;
> +	struct mtk_jpeg_dev *master_jpeg;
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +
> +	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +	u32 result_size;
> +	u32 irq_status;
> +
> +	cancel_delayed_work(&jpeg->job_timeout_work);
> +
> +	src_buf = jpeg->hw_param.src_buffer;
> +	dst_buf = jpeg->hw_param.dst_buffer;
> +	ctx = jpeg->hw_param.curr_ctx;
> +	master_jpeg = ctx->jpeg;
> +	irq_status = readl(jpeg->reg_base[MTK_JPEGENC_HW0] + JPEG_ENC_INT_STS) &

Why is this always HW0?

> +		JPEG_ENC_INT_STATUS_MASK_ALLIRQ;
> +	if (irq_status)
> +		writel(0, jpeg->reg_base[MTK_JPEGENC_HW0] + JPEG_ENC_INT_STS);
> +	if (!(irq_status & JPEG_ENC_INT_STATUS_DONE)) {
> +		dev_err(jpeg->dev, "jpeg encode failed !!!\n");
> +		goto irq_end;
> +	}
> +
> +	result_size = mtk_jpeg_enc_get_file_size(jpeg->reg_base[0]);
> +	vb2_set_plane_payload(&dst_buf->vb2_buf, 0, result_size);
> +
> +	buf_state = VB2_BUF_STATE_DONE;
> +
> +irq_end:
> +	v4l2_m2m_buf_done(src_buf, buf_state);
> +	mtk_jpeg_put_buf(jpeg);
> +	pm_runtime_put(jpeg->pm.dev);
> +	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);

Shouldn't this be the other way around? I.e. first clock and then
runtime PM? Otherwise the power domain could be powered off while the
clock is still running.

> +	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);

This patch is missing the initialization of jpeg_work, so I have no idea
what the work actually does. Please reorganize your patches, so that it
adds all the interdependent pieces together. (As is, the code wouldn't
even compile if you checked out your tree to have the series up to this
patch but not the next ones, but it's a requirement for patch submission
to the kernel.)

I suspect that there is no need for a workqueue in this driver, but
let's see after you reorganize the patches.

> +	}
> +
> +	jpeg->hw_state = MTK_JPEG_HW_IDLE;

Do we need this hw_state?

> +	wake_up(&master_jpeg->hw_wq);
> +	atomic_inc(&jpeg->hw_rdy);

Do we really need these? (Again, it's not used by any code in this
patch.)

> +	return IRQ_HANDLED;
> +}
> +
>  static void mtk_jpeg_set_default_params(struct mtk_jpeg_ctx *ctx)
>  {
>  	struct mtk_jpeg_q_data *q = &ctx->out_q;
> @@ -1352,7 +1474,7 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  	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);
> +	jpeg->reg_base[0] = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(jpeg->reg_base)) {
>  		ret = PTR_ERR(jpeg->reg_base);
>  		return ret;
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> index 93ea71c..03ff060 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -75,6 +75,17 @@ struct mtk_jpeg_variant {
>  	u32 cap_q_default_fourcc;
>  };
>  
> +struct mtk_jpeg_hw_param {

Could perhaps mtk_jpeg_hw_job be a better name?

> +	struct vb2_v4l2_buffer *src_buffer;
> +	struct vb2_v4l2_buffer *dst_buffer;
> +	struct mtk_jpeg_ctx *curr_ctx;
> +};
> +
> +enum mtk_jpeg_hw_state {
> +	MTK_JPEG_HW_IDLE = 0,
> +	MTK_JPEG_HW_BUSY = 1,
> +};

Wouldn't "bool busy" be good enough? (In case we really need to track
the busy state at all.)

Best regards,
Tomasz

> +
>  enum mtk_jpegenc_hw_id {
>  	MTK_JPEGENC_HW0,
>  	MTK_JPEGENC_HW1,
> @@ -124,13 +135,18 @@ struct mtk_jpeg_dev {
>  	struct v4l2_m2m_dev	*m2m_dev;
>  	void			*alloc_ctx;
>  	struct video_device	*vdev;
> -	void __iomem		*reg_base;
>  	struct device		*larb;
>  	struct delayed_work job_timeout_work;
>  	const struct mtk_jpeg_variant *variant;
>  
> +	void __iomem *reg_base[MTK_JPEGENC_HW_MAX];
> +	int jpegenc_irq;
>  	struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];
>  	struct mtk_jpegenc_pm pm;
> +	enum mtk_jpeg_hw_state hw_state;
> +	struct mtk_jpeg_hw_param hw_param;
> +	wait_queue_head_t hw_wq;
> +	atomic_t hw_rdy;
>  };
>  
>  /**
> @@ -189,6 +205,12 @@ struct mtk_jpeg_ctx {
>  	u8 enc_quality;
>  	u8 restart_interval;
>  	struct v4l2_ctrl_handler ctrl_hdl;
> +
> +	struct list_head dst_done_queue;
> +	spinlock_t done_queue_lock;	/* spinlock protecting done queue */
> +	u32 total_frame_num;
> +	u32 last_done_frame_num;
> +	struct work_struct jpeg_work;
>  };
>  
>  #endif /* _MTK_JPEG_CORE_H */
> -- 
> 2.6.4
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 6/9] media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces

[kyrie.wu]

Generalizes jpegenc HW timeout interfaces to handle HW timeout.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 37 +++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 062feab..39101d1 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -1285,6 +1285,43 @@ irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+void mtk_jpegenc_timeout_work(struct work_struct *work)
+{
+	struct mtk_jpeg_dev *jpeg = container_of(work, struct mtk_jpeg_dev,
+		job_timeout_work.work);
+	struct mtk_jpeg_ctx *ctx;
+	struct mtk_jpeg_dev *master_jpeg;
+	struct vb2_v4l2_buffer *src_buf, *dst_buf;
+	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
+	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
+
+	src_buf = jpeg->hw_param.src_buffer;
+	dst_buf = jpeg->hw_param.dst_buffer;
+	ctx = jpeg->hw_param.curr_ctx;
+	if (!ctx) {
+		v4l2_err(&jpeg->v4l2_dev, "Context is NULL\n");
+		return;
+	}
+
+	master_jpeg = ctx->jpeg;
+	if (!master_jpeg) {
+		v4l2_err(&jpeg->v4l2_dev, "master_jpeg is NULL\n");
+		return;
+	}
+
+	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(&src_buf->vb2_buf);
+	jpeg_dst_buf = mtk_jpeg_vb2_to_srcbuf(&dst_buf->vb2_buf);
+
+	mtk_jpeg_enc_reset(jpeg->reg_base[MTK_JPEGENC_HW0]);
+	pm_runtime_put(jpeg->pm.dev);
+	clk_disable_unprepare(jpeg->pm.venc_clk.clk_info->jpegenc_clk);
+	jpeg->hw_state = MTK_JPEG_HW_IDLE;
+	atomic_inc(&jpeg->hw_rdy);
+	wake_up(&master_jpeg->hw_wq);
+	v4l2_m2m_buf_done(src_buf, buf_state);
+	mtk_jpeg_put_buf(jpeg);
+}
+
 static void mtk_jpeg_set_default_params(struct mtk_jpeg_ctx *ctx)
 {
 	struct mtk_jpeg_q_data *q = &ctx->out_q;
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2, 6/9] media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:31 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> +void mtk_jpegenc_timeout_work(struct work_struct *work)
No code is using mtk_jpegenc_timeout_work.  Have no enough context to
review the code.

> +{
> +       struct mtk_jpeg_dev *jpeg = container_of(work, struct mtk_jpeg_dev,
> +               job_timeout_work.work);
> +       struct mtk_jpeg_ctx *ctx;
> +       struct mtk_jpeg_dev *master_jpeg;
> +       struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +       struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
> +       enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
> +
> +       src_buf = jpeg->hw_param.src_buffer;
> +       dst_buf = jpeg->hw_param.dst_buffer;
> +       ctx = jpeg->hw_param.curr_ctx;
Could they be inlined to above where the variables are declared?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 7/9] media: mtk-jpegenc: Use component framework to manage each hardware information

[kyrie.wu]

Uses component framework to manage each hardware information which
includes irq/power/clk. The hardware includes jpegenc0, jpegenc1.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c   | 185 +++++++++++++++++---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h   |  16 +-
 drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c | 196 ++++++++++++++++++++++
 3 files changed, 372 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 39101d1..bf519ba 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -7,6 +7,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -1493,12 +1494,99 @@ static inline void mtk_jpeg_clk_release(struct mtk_jpeg_dev *jpeg)
 	put_device(jpeg->larb);
 }
 
+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,
+	},
+	{},
+};
+
+static inline int mtk_vdec_compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static inline void mtk_vdec_release_of(struct device *dev, void *data)
+{
+	of_node_put(data);
+}
+
+static inline int mtk_jpegenc_bind(struct device *dev)
+{
+	struct mtk_jpeg_dev *data = dev_get_drvdata(dev);
+
+	return component_bind_all(dev, data);
+}
+
+static inline void mtk_jpegenc_unbind(struct device *dev)
+{
+	struct mtk_jpeg_dev *data = dev_get_drvdata(dev);
+
+	component_unbind_all(dev, data);
+}
+
+static const struct component_master_ops mtk_jpegenc_ops = {
+	.bind = mtk_jpegenc_bind,
+	.unbind = mtk_jpegenc_unbind,
+};
+
+static struct component_match *mtk_jpegenc_match_add(struct mtk_jpeg_dev *jpeg)
+{
+	struct device *dev = jpeg->dev;
+	struct component_match *match = NULL;
+	int i;
+	char compatible[128] = {0};
+
+	for (i = 0; i < ARRAY_SIZE(mtk_jpegenc_drv_ids); i++) {
+		struct device_node *comp_node;
+		enum mtk_jpegenc_hw_id comp_idx;
+		const struct of_device_id *of_id;
+
+		memcpy(compatible, mtk_jpegenc_drv_ids[i].compatible,
+		       sizeof(mtk_jpegenc_drv_ids[i].compatible));
+
+		comp_node = of_find_compatible_node(NULL, NULL,
+						    compatible);
+		if (!comp_node)
+			continue;
+
+		if (!of_device_is_available(comp_node)) {
+			of_node_put(comp_node);
+			v4l2_err(&jpeg->v4l2_dev, "Fail to get jpeg enc HW node\n");
+			continue;
+		}
+
+		of_id = of_match_node(mtk_jpegenc_drv_ids, comp_node);
+		if (!of_id) {
+			v4l2_err(&jpeg->v4l2_dev, "Failed to get match node\n");
+			return ERR_PTR(-EINVAL);
+		}
+
+		comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
+		v4l2_info(&jpeg->v4l2_dev, "Get component:hw_id(%d),jpeg_dev(0x%p),comp_node(0x%p)\n",
+			  comp_idx, jpeg, comp_node);
+
+		jpeg->component_node[comp_idx] = comp_node;
+
+		component_match_add_release(dev, &match, mtk_vdec_release_of,
+					    mtk_vdec_compare_of, comp_node);
+	}
+
+	return match;
+}
+
 static int mtk_jpeg_probe(struct platform_device *pdev)
 {
 	struct mtk_jpeg_dev *jpeg;
 	struct resource *res;
 	int jpeg_irq;
 	int ret;
+	struct component_match *match;
 
 	jpeg = devm_kzalloc(&pdev->dev, sizeof(*jpeg), GFP_KERNEL);
 	if (!jpeg)
@@ -1510,31 +1598,47 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 	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[0] = 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) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+		jpeg->reg_base[MTK_JPEGENC_HW0] =
+			devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(jpeg->reg_base[MTK_JPEGENC_HW0])) {
+			ret = PTR_ERR(jpeg->reg_base[MTK_JPEGENC_HW0]);
+			return ret;
+		}
 
-	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;
-	}
+		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;
+		}
 
-	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 = 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, err %d\n", ret);
-		goto err_clk_init;
+		ret = mtk_jpeg_clk_init(jpeg);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to init clk\n");
+			goto err_clk_init;
+		}
+	} else {
+		init_waitqueue_head(&jpeg->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);
@@ -1583,7 +1687,20 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, jpeg);
 
-	pm_runtime_enable(&pdev->dev);
+	if (jpeg->variant->is_encoder) {
+		match = mtk_jpegenc_match_add(jpeg);
+		if (IS_ERR_OR_NULL(match))
+			goto err_vfd_jpeg_register;
+
+		video_set_drvdata(jpeg->vdev, jpeg);
+		platform_set_drvdata(pdev, jpeg);
+		ret = component_master_add_with_match(&pdev->dev,
+						      &mtk_jpegenc_ops, match);
+		if (ret < 0)
+			goto err_vfd_jpeg_register;
+	} else {
+		pm_runtime_enable(&pdev->dev);
+	}
 
 	return 0;
 
@@ -1601,6 +1718,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
 
 err_clk_init:
 
+err_alloc_workqueue:
+
 err_req_irq:
 
 	return ret;
@@ -1721,7 +1840,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 03ff060..03ef719 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
@@ -125,6 +125,8 @@ struct mtk_jpegenc_pm {
  * @larb:		SMI device
  * @job_timeout_work:	IRQ timeout structure
  * @variant:		driver variant to be used
+ * @irqlock:		spinlock protecting for irq
+ * @dev_mutex:		the mutex protecting for device
  */
 struct mtk_jpeg_dev {
 	struct mutex		lock;
@@ -136,12 +138,18 @@ struct mtk_jpeg_dev {
 	void			*alloc_ctx;
 	struct video_device	*vdev;
 	struct device		*larb;
-	struct delayed_work job_timeout_work;
 	const struct mtk_jpeg_variant *variant;
 
+	struct clk		*clk_jpeg;
+	struct delayed_work job_timeout_work;
+	struct platform_device *plat_dev;
+	spinlock_t irqlock;	/* spinlock protecting for irq */
+	struct mutex dev_mutex;	/* the mutex protecting for device */
 	void __iomem *reg_base[MTK_JPEGENC_HW_MAX];
 	int jpegenc_irq;
 	struct mtk_jpeg_dev *hw_dev[MTK_JPEGENC_HW_MAX];
+	struct device_node *component_node[MTK_JPEGENC_HW_MAX];
+	int comp_idx;
 	struct mtk_jpegenc_pm pm;
 	enum mtk_jpeg_hw_state hw_state;
 	struct mtk_jpeg_hw_param hw_param;
@@ -149,6 +157,8 @@ struct mtk_jpeg_dev {
 	atomic_t hw_rdy;
 };
 
+extern struct platform_driver mtk_jpegenc_hw_driver;
+
 /**
  * struct mtk_jpeg_fmt - driver's internal color format data
  * @fourcc:	the fourcc code, 0 if not applicable
@@ -194,6 +204,7 @@ struct mtk_jpeg_q_data {
  * @enc_quality:	jpeg encoder quality
  * @restart_interval:	jpeg encoder restart interval
  * @ctrl_hdl:		controls handler
+ * @done_queue_lock:	spinlock protecting for buffer done queue
  */
 struct mtk_jpeg_ctx {
 	struct mtk_jpeg_dev		*jpeg;
@@ -213,4 +224,7 @@ struct mtk_jpeg_ctx {
 	struct work_struct jpeg_work;
 };
 
+void mtk_jpegenc_timeout_work(struct work_struct *work);
+irqreturn_t mtk_jpegenc_hw_irq_handler(int irq, void *priv);
+
 #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 3da1011..9dc6d99 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c
@@ -5,11 +5,21 @@
  *
  */
 
+#include <linux/clk.h>
+#include <linux/component.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/videobuf2-core.h>
 #include <media/videobuf2-dma-contig.h>
 
+#include "mtk_jpeg_core.h"
 #include "mtk_jpeg_enc_hw.h"
 
 static const struct mtk_jpeg_enc_qlt mtk_jpeg_enc_quality[] = {
@@ -153,6 +163,182 @@ 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 int mtk_jpegenc_hw_bind(struct device *dev,
+			       struct device *master, void *data)
+{
+	struct mtk_jpeg_dev *comp_priv = dev_get_drvdata(dev);
+	struct mtk_jpeg_dev *master_priv = data;
+	int i;
+
+	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+		if (dev->of_node != master_priv->component_node[i])
+			continue;
+		master_priv->hw_dev[i] = comp_priv;
+		comp_priv->comp_idx = i;
+		master_priv->reg_base[i] = comp_priv->reg_base[MTK_JPEGENC_HW0];
+		break;
+	}
+	if (i == MTK_JPEGENC_HW_MAX) {
+		dev_err(dev, "Failed to get component node\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static void mtk_jpegenc_hw_unbind(struct device *dev,
+				  struct device *master, void *data)
+{
+	struct mtk_jpeg_dev *comp_priv = dev_get_drvdata(dev);
+
+	comp_priv->reg_base[MTK_JPEGENC_HW0] = 0;
+}
+
+static const struct component_ops mtk_jpegenc_hw_component_ops = {
+	.bind = mtk_jpegenc_hw_bind,
+	.unbind = mtk_jpegenc_hw_unbind,
+};
+
+int mtk_jpegenc_init_pm(struct mtk_jpeg_dev *mtkdev)
+{
+	struct platform_device *pdev;
+	struct mtk_jpegenc_pm *pm;
+	struct mtk_jpegenc_clk *jpegenc_clk;
+	struct mtk_jpegenc_clk_info *clk_info;
+	int i, ret;
+
+	pdev = mtkdev->plat_dev;
+	pm->dev = &pdev->dev;
+	pm = &mtkdev->pm;
+	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_jpeg_dev *mtkdev)
+{
+	struct platform_device *pdev = mtkdev->plat_dev;
+
+	pm_runtime_disable(&pdev->dev);
+}
+
+static int mtk_jpegenc_hw_init_irq(struct mtk_jpeg_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;
+}
+
+static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
+{
+	struct mtk_jpeg_dev *dev;
+	int ret;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+	dev->plat_dev = pdev;
+	spin_lock_init(&dev->irqlock);
+	mutex_init(&dev->dev_mutex);
+	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);
+
+	ret = mtk_jpegenc_init_pm(dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get jpeg enc clock source");
+		return ret;
+	}
+
+	dev->reg_base[MTK_JPEGENC_HW0] =
+		devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR((__force void *)dev->reg_base[MTK_JPEGENC_HW0])) {
+		ret = PTR_ERR((__force void *)dev->reg_base[MTK_JPEGENC_HW0]);
+		goto err;
+	}
+
+	ret = mtk_jpegenc_hw_init_irq(dev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to register JPEGENC irq handler.\n");
+		goto err;
+	}
+
+	platform_set_drvdata(pdev, dev);
+
+	ret = component_add(&pdev->dev, &mtk_jpegenc_hw_component_ops);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to component_add: %d\n", ret);
+		goto err;
+	}
+
+	return 0;
+
+err:
+	mtk_jpegenc_release_pm(dev);
+	return ret;
+}
+
+static int mtk_jpegenc_remove(struct platform_device *pdev)
+{
+	struct mtk_jpeg_dev *dev = platform_get_drvdata(pdev);
+
+	mtk_jpegenc_release_pm(dev);
+
+	return 0;
+}
+
 #if defined(CONFIG_OF)
 static const struct of_device_id mtk_jpegenc_hw_ids[] = {
 	{
@@ -164,3 +350,13 @@ static const struct of_device_id mtk_jpegenc_hw_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_jpegenc_hw_ids);
 #endif
+
+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_hw_ids),
+	},
+};
+
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,7/9] media: mtk-jpegenc: Use component framework to manage each hardware information

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> +static  const struct of_device_id mtk_jpegenc_drv_ids[] = {
Remove the extra space between "static" and "const".

> +       {
> +               .compatible = "mediatek,mt8195-jpgenc0",
> +               .data = (void *)MTK_JPEGENC_HW0,
> +       },
> +       {
> +               .compatible = "mediatek,mt8195-jpgenc1",
> +               .data = (void *)MTK_JPEGENC_HW1,
> +       },
> +       {},
> +};
Should be guarded by CONFIG_OF.

> +static struct component_match *mtk_jpegenc_match_add(struct mtk_jpeg_dev *jpeg)
> +{
> +       struct device *dev = jpeg->dev;
> +       struct component_match *match = NULL;
> +       int i;
> +       char compatible[128] = {0};
It doesn't need to be initialized.

> +
> +       for (i = 0; i < ARRAY_SIZE(mtk_jpegenc_drv_ids); i++) {
> +               struct device_node *comp_node;
> +               enum mtk_jpegenc_hw_id comp_idx;
> +               const struct of_device_id *of_id;
> +
> +               memcpy(compatible, mtk_jpegenc_drv_ids[i].compatible,
> +                      sizeof(mtk_jpegenc_drv_ids[i].compatible));
Shouldn't rely on the source length.  Also needs to use strcpy-family
for better handling the NULL terminator.

> +               if (!of_device_is_available(comp_node)) {
> +                       of_node_put(comp_node);
> +                       v4l2_err(&jpeg->v4l2_dev, "Fail to get jpeg enc HW node\n");
To be consistent, use "Failed".

> +               of_id = of_match_node(mtk_jpegenc_drv_ids, comp_node);
> +               if (!of_id) {
> +                       v4l2_err(&jpeg->v4l2_dev, "Failed to get match node\n");
> +                       return ERR_PTR(-EINVAL);
Shouldn't it call of_node_put() before returning?

> +               comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
> +               v4l2_info(&jpeg->v4l2_dev, "Get component:hw_id(%d),jpeg_dev(0x%p),comp_node(0x%p)\n",
> +                         comp_idx, jpeg, comp_node);
> +
> +               jpeg->component_node[comp_idx] = comp_node;
> +
> +               component_match_add_release(dev, &match, mtk_vdec_release_of,
> +                                           mtk_vdec_compare_of, comp_node);
Shouldn't it need to break if it already found?

> +       if (!jpeg->variant->is_encoder) {
> +               res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +               jpeg->reg_base[MTK_JPEGENC_HW0] =
> +                       devm_ioremap_resource(&pdev->dev, res);
If !is_encoder, why is it still using MTK_JPEGENC_HW0?

> +               if (IS_ERR(jpeg->reg_base[MTK_JPEGENC_HW0])) {
> +                       ret = PTR_ERR(jpeg->reg_base[MTK_JPEGENC_HW0]);
> +                       return ret;
Just return the PTR_ERR if it doesn't need to goto.

> -       pm_runtime_enable(&pdev->dev);
> +       if (jpeg->variant->is_encoder) {
> +               match = mtk_jpegenc_match_add(jpeg);
> +               if (IS_ERR_OR_NULL(match))
> +                       goto err_vfd_jpeg_register;
> +
> +               video_set_drvdata(jpeg->vdev, jpeg);
> +               platform_set_drvdata(pdev, jpeg);
> +               ret = component_master_add_with_match(&pdev->dev,
> +                                                     &mtk_jpegenc_ops, match);
> +               if (ret < 0)
> +                       goto err_vfd_jpeg_register;
Shouldn't it call of_node_put() for un-doing mtk_jpegenc_match_add()?

> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -125,6 +125,8 @@ struct mtk_jpegenc_pm {
>   * @larb:              SMI device
>   * @job_timeout_work:  IRQ timeout structure
>   * @variant:           driver variant to be used
> + * @irqlock:           spinlock protecting for irq
> + * @dev_mutex:         the mutex protecting for device
The patch adds more than 2 fields in the struct.  They also need short
descriptions here.

>   */
>  struct mtk_jpeg_dev {
>         struct mutex            lock;
> @@ -136,12 +138,18 @@ struct mtk_jpeg_dev {
>         void                    *alloc_ctx;
>         struct video_device     *vdev;
>         struct device           *larb;
> -       struct delayed_work job_timeout_work;
>         const struct mtk_jpeg_variant *variant;
>
> +       struct clk              *clk_jpeg;
It is not used.

>  /**
>   * struct mtk_jpeg_fmt - driver's internal color format data
>   * @fourcc:    the fourcc code, 0 if not applicable
> @@ -194,6 +204,7 @@ struct mtk_jpeg_q_data {
>   * @enc_quality:       jpeg encoder quality
>   * @restart_interval:  jpeg encoder restart interval
>   * @ctrl_hdl:          controls handler
> + * @done_queue_lock:   spinlock protecting for buffer done queue
Probably put in the wrong patch?

> +int mtk_jpegenc_init_pm(struct mtk_jpeg_dev *mtkdev)
> +{
> +       struct platform_device *pdev;
> +       struct mtk_jpegenc_pm *pm;
> +       struct mtk_jpegenc_clk *jpegenc_clk;
> +       struct mtk_jpegenc_clk_info *clk_info;
> +       int i, ret;
> +
> +       pdev = mtkdev->plat_dev;
> +       pm->dev = &pdev->dev;
> +       pm = &mtkdev->pm;
> +       pm->mtkdev = mtkdev;
> +       jpegenc_clk = &pm->venc_clk;
Could they be inlined to above where the variables are declared.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 8/9] media: mtk-jpegenc: Generalize jpegenc HW operations interfaces

[kyrie.wu]

Generalizes jpegenc select/deselect HW and set params interfaces.

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 46 +++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index bf519ba..0143baf8 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -939,6 +939,52 @@ static int mtk_jpeg_set_dec_dst(struct mtk_jpeg_ctx *ctx,
 	return 0;
 }
 
+static int mtk_jpeg_select_hw(struct mtk_jpeg_ctx *ctx)
+{
+	int hw_id = -1;
+	int i;
+	unsigned long flags;
+	struct mtk_jpeg_dev *jpeg = ctx->jpeg, *comp_jpeg = NULL;
+
+	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+		comp_jpeg = jpeg->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_jpeg_deselect_hw(struct mtk_jpeg_dev *jpeg, int hw_id)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	jpeg->hw_dev[hw_id]->hw_state = MTK_JPEG_HW_IDLE;
+	spin_unlock_irqrestore(&jpeg->hw_lock, flags);
+
+	return 0;
+}
+
+static int mtk_jpeg_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_jpeg_dev *jpeg = ctx->jpeg->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 void mtk_jpeg_enc_device_run(void *priv)
 {
 	struct mtk_jpeg_ctx *ctx = priv;
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,8/9] media: mtk-jpegenc: Generalize jpegenc HW operations interfaces

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Generalizes jpegenc select/deselect HW and set params interfaces.
No code is using the functions.  The patch needs to be submitted with
other patches.

> +static int mtk_jpeg_select_hw(struct mtk_jpeg_ctx *ctx)
> +{
> +       int hw_id = -1;
> +       int i;
> +       unsigned long flags;
> +       struct mtk_jpeg_dev *jpeg = ctx->jpeg, *comp_jpeg = NULL;
comp_jpeg doesn't need to be initialized.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2, 9/9] media: mtk-jpegenc: Refactor jpegenc device run interface

[kyrie.wu]

modify jpegenc device run func interface and add worker for encode

Signed-off-by: kyrie.wu <kyrie.wu@mediatek.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 109 ++++++++++++++++++++----
 1 file changed, 94 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 0143baf8..190e405 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -985,38 +985,99 @@ static int mtk_jpeg_set_hw_param(struct mtk_jpeg_ctx *ctx,
 	return 0;
 }
 
-static void mtk_jpeg_enc_device_run(void *priv)
+static void mtk_jpegenc_worker(struct work_struct *work)
 {
-	struct mtk_jpeg_ctx *ctx = priv;
+	struct mtk_jpeg_ctx *ctx = container_of(work, struct mtk_jpeg_ctx,
+		jpeg_work);
 	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
+	struct mtk_jpeg_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
 	struct vb2_v4l2_buffer *src_buf, *dst_buf;
 	enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
 	unsigned long flags;
-	int ret;
+	struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
+	int ret, i, hw_id = 0;
+	atomic_t *hw_rdy[MTK_JPEGENC_HW_MAX];
+	struct clk *jpegenc_clk;
+
+	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
+		comp_jpeg[i] = jpeg->hw_dev[i];
+		hw_rdy[i] = &comp_jpeg[i]->hw_rdy;
+	}
+
+retry_select:
+	hw_id = mtk_jpeg_select_hw(ctx);
+	if (hw_id < 0) {
+		ret = wait_event_interruptible(jpeg->hw_wq,
+				(atomic_read(hw_rdy[0]) ||
+				atomic_read(hw_rdy[1])) > 0);
+		if (ret != 0) {
+			dev_err(jpeg->dev, "all HW are busy\n");
+			v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
+			return;
+		}
+		dev_err(jpeg->dev, "NEW HW IDLE, please retry selcet!!!\n");
+		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, "get src_buf fail !!!\n");
+		goto getbuf_fail;
+	}
+
 	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+	if (!dst_buf) {
+		dev_err(jpeg->dev, "get dst_buf fail !!!\n");
+		goto getbuf_fail;
+	}
 
-	ret = pm_runtime_get_sync(jpeg->dev);
-	if (ret < 0)
+	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++;
+
+	v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
+	v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
+
+	mtk_jpeg_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, "pm_runtime_get_sync fail !!!\n");
 		goto enc_end;
+	}
 
-	schedule_delayed_work(&jpeg->job_timeout_work,
-			      msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
+	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, "jpegenc clk_prepare_enable fail !!!\n");
+		goto enc_end;
+	}
 
-	spin_lock_irqsave(&jpeg->hw_lock, flags);
+	schedule_delayed_work(&comp_jpeg[hw_id]->job_timeout_work,
+			msecs_to_jiffies(MTK_JPEG_HW_TIMEOUT_MSEC));
 
 	/*
 	 * 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);
-
-	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);
+	spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
+	mtk_jpeg_enc_reset(comp_jpeg[hw_id]->reg_base[0]);
+	mtk_jpeg_set_enc_dst(ctx,
+			     comp_jpeg[hw_id]->reg_base[0],
+			     &dst_buf->vb2_buf);
+	mtk_jpeg_set_enc_src(ctx,
+			     comp_jpeg[hw_id]->reg_base[0],
+			     &src_buf->vb2_buf);
+	mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base[0]);
+	mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base[0]);
+	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
+	spin_unlock_irqrestore(&comp_jpeg[hw_id]->hw_lock, flags);
 	return;
 
 enc_end:
@@ -1024,9 +1085,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_jpeg_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;
@@ -1419,6 +1491,11 @@ static int mtk_jpeg_open(struct file *file)
 		goto free;
 	}
 
+	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);
@@ -1781,6 +1858,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;
 }
-- 
2.6.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2, 9/9] media: mtk-jpegenc: Refactor jpegenc device run interface

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:32 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> modify jpegenc device run func interface and add worker for encode
The description makes less sense.

> +static void mtk_jpegenc_worker(struct work_struct *work)
>  {
> -       struct mtk_jpeg_ctx *ctx = priv;
> +       struct mtk_jpeg_ctx *ctx = container_of(work, struct mtk_jpeg_ctx,
> +               jpeg_work);
>         struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> +       struct mtk_jpeg_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
>         struct vb2_v4l2_buffer *src_buf, *dst_buf;
>         enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
>         unsigned long flags;
> -       int ret;
> +       struct mtk_jpeg_src_buf *jpeg_src_buf, *jpeg_dst_buf;
> +       int ret, i, hw_id = 0;
hw_id doesn't need to be initialized.

> +retry_select:
> +       hw_id = mtk_jpeg_select_hw(ctx);
> +       if (hw_id < 0) {
> +               ret = wait_event_interruptible(jpeg->hw_wq,
> +                               (atomic_read(hw_rdy[0]) ||
> +                               atomic_read(hw_rdy[1])) > 0);
Hard-coded refers to hw_rdy[0] and hw_rdy[1] here?

> -       mtk_jpeg_enc_reset(jpeg->reg_base);
> -
> -       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);
> +       spin_lock_irqsave(&comp_jpeg[hw_id]->hw_lock, flags);
> +       mtk_jpeg_enc_reset(comp_jpeg[hw_id]->reg_base[0]);
> +       mtk_jpeg_set_enc_dst(ctx,
> +                            comp_jpeg[hw_id]->reg_base[0],
> +                            &dst_buf->vb2_buf);
> +       mtk_jpeg_set_enc_src(ctx,
> +                            comp_jpeg[hw_id]->reg_base[0],
> +                            &src_buf->vb2_buf);
> +       mtk_jpeg_set_enc_params(ctx, comp_jpeg[hw_id]->reg_base[0]);
> +       mtk_jpeg_enc_start(comp_jpeg[hw_id]->reg_base[0]);
Why it uses reg_base[0]?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,0/9] Support jpeg encode for MT8195

[Tzung-Bi Shih]

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> add component framework to using multi-HW for MT8195 jpeg encode.
Could you add some summary for each patch for getting an overview of the series?

>   dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
>   media: mtk-jpegenc: Add MT8195 JPEG venc driver
>   media: mtk-jpegenc: remove redundant code of irq
>   media: mtk-jpegenc: Refactor jpeg clock interface
>   media: mtk-jpegenc: Generalize jpeg encode irq interfaces
>   media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces
>   media: mtk-jpegenc: Use component framework to manage each hardware
>     information
>   media: mtk-jpegenc: Generalize jpegenc HW operations interfaces
>   media: mtk-jpegenc: Refactor jpegenc device run interface
The series has some consistency issues which would make readers feel
uncomfortable.

For example:
- Whether to capitalize the first characters in the commit messages/titles.
- Whether to add a period at the end of English sentences.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,0/9] Support jpeg encode for MT8195

[Tomasz Figa]

On Tue, Jul 06, 2021 at 07:00:17PM +0800, Tzung-Bi Shih wrote:
> On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> > add component framework to using multi-HW for MT8195 jpeg encode.
> Could you add some summary for each patch for getting an overview of the series?
> 
> >   dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
> >   media: mtk-jpegenc: Add MT8195 JPEG venc driver
> >   media: mtk-jpegenc: remove redundant code of irq
> >   media: mtk-jpegenc: Refactor jpeg clock interface
> >   media: mtk-jpegenc: Generalize jpeg encode irq interfaces
> >   media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces
> >   media: mtk-jpegenc: Use component framework to manage each hardware
> >     information
> >   media: mtk-jpegenc: Generalize jpegenc HW operations interfaces
> >   media: mtk-jpegenc: Refactor jpegenc device run interface
> The series has some consistency issues which would make readers feel
> uncomfortable.
> 
> For example:
> - Whether to capitalize the first characters in the commit messages/titles.
> - Whether to add a period at the end of English sentences.

FWIW, it's not customary to add a period at the end of a patch subject.

Best regards,
Tomasz


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2,0/9] Support jpeg encode for MT8195

[Tomasz Figa]

Hi Kyrie,

On Wed, Jun 30, 2021 at 4:31 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
>
> add component framework to using multi-HW for MT8195 jpeg encode.
>
> kyrie.wu (9):
>   dt-bindings: mediatek: Add mediatek, mt8195-jpgenc compatible
>   media: mtk-jpegenc: Add MT8195 JPEG venc driver
>   media: mtk-jpegenc: remove redundant code of irq
>   media: mtk-jpegenc: Refactor jpeg clock interface
>   media: mtk-jpegenc: Generalize jpeg encode irq interfaces
>   media: mtk-jpegenc: Generalize jpegenc HW timeout interfaces
>   media: mtk-jpegenc: Use component framework to manage each hardware
>     information
>   media: mtk-jpegenc: Generalize jpegenc HW operations interfaces
>   media: mtk-jpegenc: Refactor jpegenc device run interface
>
>  .../bindings/media/mediatek-jpeg-encoder.yaml      |   3 +
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c    | 600 +++++++++++++++++----
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h    |  69 ++-
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_enc_hw.c  | 208 +++++++
>  4 files changed, 786 insertions(+), 94 deletions(-)
>
> ---
> This patch dependents on "dt-bindings: mediatek: convert mtk jpeg decoder/encoder to yaml"[1]
>
> Please also accept this patch together with [1].
>
> [1]https://lore.kernel.org/patchwork/patch/1445298/

Thank you for the series. However, I gave reviewing it a try and
unfortunately had a very hard time following it, because of the way
the patches are organized. Please make sure to read and understand the
kernel patch submission guide[1], adjust the series appropriately and
send a new version which I'll review.

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel