| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4e1b549b-748e-94f3-befb-59ceab950297@collabora.com>
Date: Mon, 7 Feb 2022 15:50:32 +0100
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: "kyrie.wu" <kyrie.wu@...iatek.com>,
Hans Verkuil <hverkuil-cisco@...all.nl>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Tomasz Figa <tfiga@...omium.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Tzung-Bi Shih <tzungbi@...omium.org>
Cc: Project_Global_Chrome_Upstream_Group@...iatek.com,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, xia.jiang@...iatek.com,
maoguang.meng@...iatek.com, srv_heupstream@...iatek.com,
irui.wang@...iatek.com
Subject: Re: [PATCH V6, 2/5] media: mtk-jpegenc: manage jpegenc multi-hardware
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@...iatek.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@...iatek.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@...ress {
compatible = "mediatek,mt8195-jpgenc";
reg = < .... >;
ranges = < ....... >;
.... other properties here ....
jpegenc-hw0@...ative-address {
compatible = "mediatek,mt8195-jpgenc-hw", "mediatek,jpgenc-hw";
iommus, interrupts, other properties here ...
mediatek,hw-leader;
};
jpegenc-hw1@...ative-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
Powered by blists - more mailing lists