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

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
> 

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
> 

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

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
> 

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