[RFC PATCH V3 1/5] dt-bindings: mt8183: Added DIP dt-bindings

[RFC PATCH V3 1/5] dt-bindings: mt8183: Added DIP dt-bindings

[frederic.chen]

From: Frederic Chen <frederic.chen@mediatek.com>

This patch adds DT binding documentation for the Digital Image
Processing (DIP) unit of camera ISP system on Mediatek's SoCs.

It depends on the SCP and MDP 3 patch as following:

1. dt-bindings: Add a binding for Mediatek SCP
   https://patchwork.kernel.org/patch/11027247/
2. dt-binding: mt8183: Add Mediatek MDP3 dt-bindings
   https://patchwork.kernel.org/patch/10945603/

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 .../bindings/media/mediatek,mt8183-dip.txt    | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt

diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt b/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
new file mode 100644
index 000000000000..3a0435513089
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
@@ -0,0 +1,40 @@
+* Mediatek Digital Image Processor (DIP)
+
+Digital Image Processor (DIP) unit in Mediatek ISP system is responsible for
+image content adjustment according to the tuning parameters. DIP can process
+the image form memory buffer and output the processed image to multiple output
+buffers. Furthermore, it can support demosaicing and noise reduction on the
+images.
+
+Required properties:
+- compatible: "mediatek,mt8183-dip"
+- reg: Physical base address and length of the function block register space
+- interrupts: interrupt number to the cpu
+- iommus: should point to the respective IOMMU block with master port as
+  argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
+  for details.
+- mediatek,mdp3: should point to the respective mdp block. DIP hardware
+  connects to MDP and we can get the processed image with both effect of the
+  two blocks.
+- mediatek,larb: must contain the local arbiters in the current SoCs, see
+  Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
+  for details.
+- mediatek,scp: should point to the scp node since the we use SCP
+  coprocessor to control DIP hardware
+- clocks: must contain the local arbiters 5 (LARB5) and DIP clock
+- clock-names: must contain "larb5" and "dip"
+
+Example:
+	dip: dip@15022000 {
+		compatible = "mediatek,mt8183-dip";
+		mediatek,larb = <&larb5>;
+		mediatek,mdp3 = <&mdp_rdma0>;
+		mediatek,scp = <&scp>;
+		iommus = <&iommu M4U_PORT_CAM_IMGI>;
+		reg = <0 0x15022000 0 0x6000>;
+		interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_LOW>;
+		clocks = <&imgsys CLK_IMG_LARB5>,
+			 <&imgsys CLK_IMG_DIP>;
+		clock-names = "larb5",
+			      "dip";
+		};
-- 
2.18.0


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

Re: [RFC PATCH V3 1/5] dt-bindings: mt8183: Added DIP dt-bindings

[Rob Herring]

On Tue, 10 Sep 2019 03:22:40 +0800, <frederic.chen@mediatek.com> wrote:
> From: Frederic Chen <frederic.chen@mediatek.com>
> 
> This patch adds DT binding documentation for the Digital Image
> Processing (DIP) unit of camera ISP system on Mediatek's SoCs.
> 
> It depends on the SCP and MDP 3 patch as following:
> 
> 1. dt-bindings: Add a binding for Mediatek SCP
>    https://patchwork.kernel.org/patch/11027247/
> 2. dt-binding: mt8183: Add Mediatek MDP3 dt-bindings
>    https://patchwork.kernel.org/patch/10945603/
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  .../bindings/media/mediatek,mt8183-dip.txt    | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
> 

Please add Acked-by/Reviewed-by tags when posting new versions. However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for acks received on the version they apply.

If a tag was not added on purpose, please state why and what changed.


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

Re: [RFC PATCH V3 1/5] dt-bindings: mt8183: Added DIP dt-bindings

[Sakari Ailus]

Hi Frederic,

On Tue, Sep 10, 2019 at 03:22:40AM +0800, frederic.chen@mediatek.com wrote:
> From: Frederic Chen <frederic.chen@mediatek.com>
> 
> This patch adds DT binding documentation for the Digital Image
> Processing (DIP) unit of camera ISP system on Mediatek's SoCs.
> 
> It depends on the SCP and MDP 3 patch as following:
> 
> 1. dt-bindings: Add a binding for Mediatek SCP
>    https://patchwork.kernel.org/patch/11027247/
> 2. dt-binding: mt8183: Add Mediatek MDP3 dt-bindings
>    https://patchwork.kernel.org/patch/10945603/
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  .../bindings/media/mediatek,mt8183-dip.txt    | 40 +++++++++++++++++++

Could you add a MAINTAINERS entry for this, please? Same for the driver.

-- 
Sakari Ailus

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

[RFC PATCH V3 2/5] dts: arm64: mt8183: Add DIP nodes

[frederic.chen]

From: Frederic Chen <frederic.chen@mediatek.com>

This patch adds nodes for Digital Image Processing (DIP). DIP is
embedded in Mediatek SoCs and works with the co-processor to
adjust image content according to tuning input data. It also
provides image format conversion, resizing, and rotation
features.

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 0f2646c9eb65..d7b0fb8230f0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -513,6 +513,22 @@
 			#clock-cells = <1>;
 		};
 
+		dip: dip@15022000 {
+			compatible = "mediatek,mt8183-dip";
+			mediatek,larb = <&larb5>;
+			mediatek,mdp3 = <&mdp_rdma0>;
+			mediatek,scp = <&scp>;
+			iommus = <&iommu M4U_PORT_CAM_IMGI>;
+			reg = <0 0x15022000 0 0x6000>;
+			interrupts = <GIC_SPI 268 IRQ_TYPE_LEVEL_LOW>;
+			clocks =
+					<&imgsys CLK_IMG_LARB5>,
+					<&imgsys CLK_IMG_DIP>;
+			clock-names =
+					"larb5",
+					"dip";
+		};
+
 		vdecsys: syscon@16000000 {
 			compatible = "mediatek,mt8183-vdecsys", "syscon";
 			reg = <0 0x16000000 0 0x1000>;
-- 
2.18.0


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

[RFC PATCH V3 3/5] media: platform: Add Mediatek DIP driver KConfig

[frederic.chen]

From: Frederic Chen <frederic.chen@mediatek.com>

This patch adds KConfig for Mediatek Digital Image Processing
driver(DIP). DIP is embedded in Mediatek SoCs. It provides
image format conversion, resizing, and rotation function.

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 drivers/media/platform/Kconfig         |  2 ++
 drivers/media/platform/mtk-isp/Kconfig | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)
 create mode 100644 drivers/media/platform/mtk-isp/Kconfig

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 0c725d4dcf80..b8501e1b134f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -32,6 +32,8 @@ source "drivers/media/platform/davinci/Kconfig"
 
 source "drivers/media/platform/omap/Kconfig"
 
+source "drivers/media/platform/mtk-isp/Kconfig"
+
 config VIDEO_ASPEED
 	tristate "Aspeed AST2400 and AST2500 Video Engine driver"
 	depends on VIDEO_V4L2
diff --git a/drivers/media/platform/mtk-isp/Kconfig b/drivers/media/platform/mtk-isp/Kconfig
new file mode 100644
index 000000000000..a4267db70c24
--- /dev/null
+++ b/drivers/media/platform/mtk-isp/Kconfig
@@ -0,0 +1,19 @@
+config VIDEO_MEDIATEK_ISP_DIP
+	bool "Mediatek Digital Image Processing function"
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_V4L2
+	select VIDEOBUF2_MEMOPS
+	select MEDIA_CONTROLLER
+
+	default n
+	help
+	    Support the basic Digital Image Processing (DIP) driver.
+
+	    DIP driver provides image format conversion, resizing,
+	    and rotation function through the low power hardware.
+	    DIP also supports multiple output feature. It can
+	    generate two or more output image with different effect
+	    based on a single input image at the same time.
+
-- 
2.18.0


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

Re: [RFC PATCH V3 3/5] media: platform: Add Mediatek DIP driver KConfig

[Sakari Ailus]

Hi Frederic,

On Tue, Sep 10, 2019 at 03:22:42AM +0800, frederic.chen@mediatek.com wrote:
> From: Frederic Chen <frederic.chen@mediatek.com>
> 
> This patch adds KConfig for Mediatek Digital Image Processing
> driver(DIP). DIP is embedded in Mediatek SoCs. It provides
> image format conversion, resizing, and rotation function.
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>

Please merge this to the patch adding the driver.

-- 
Sakari Ailus

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

[RFC PATCH V3 5/5] media: platform: mtk-mdp3: Add struct tuning_addr and img_sw_buffer

[frederic.chen]

From: Frederic Chen <frederic.chen@mediatek.com>

We added a struct tuning_addr which contains a field "present"
so that the driver can tell the firmware if we have user tuning
dataor not.

The strcut img_sw_buffer is also added. This struct has no cpu address
field and uses a handle instead so that we don't pass a cpu address
to co-processor.

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 drivers/media/platform/mtk-mdp3/mtk-img-ipi.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp3/mtk-img-ipi.h b/drivers/media/platform/mtk-mdp3/mtk-img-ipi.h
index 9fabe7e8b71d..f61e61faf636 100644
--- a/drivers/media/platform/mtk-mdp3/mtk-img-ipi.h
+++ b/drivers/media/platform/mtk-mdp3/mtk-img-ipi.h
@@ -38,6 +38,12 @@ struct img_addr {
 	u32	iova;	/* Used by IOMMU HW access */
 } __attribute__ ((__packed__));
 
+struct tuning_addr {
+	u32	present;
+	u32	pa;	/* Used by CM4 access */
+	u32	iova;	/* Used by IOMMU HW access */
+} __attribute__ ((__packed__));
+
 struct img_sw_addr {
 	u64	va;	/* Used by APMCU access */
 	u32	pa;	/* Used by CM4 access */
@@ -105,16 +111,21 @@ struct img_ipi_frameparam {
 	u64		drv_data;
 	struct img_input	inputs[IMG_MAX_HW_INPUTS];
 	struct img_output	outputs[IMG_MAX_HW_OUTPUTS];
-	struct img_addr		tuning_data;
+	struct tuning_addr	tuning_data;
 	struct img_addr		subfrm_data;
 	struct img_sw_addr	config_data;
 	struct img_sw_addr  self_data;
 	/* u8		pq_data[]; */
 } __attribute__ ((__packed__));
 
+struct img_sw_buffer {
+	u64	handle;		/* Used by APMCU access */
+	u32	scp_addr;	/* Used by CM4 access */
+} __attribute__ ((__packed__));
+
 struct img_ipi_param {
 	u8	usage;
-	struct	img_sw_addr frm_param;
+	struct img_sw_buffer frm_param;
 } __attribute__ ((__packed__));
 
 struct img_frameparam {
-- 
2.18.0


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

Re: [RFC PATCH V3 5/5] media: platform: mtk-mdp3: Add struct tuning_addr and img_sw_buffer

[Sakari Ailus]

On Tue, Sep 10, 2019 at 03:22:44AM +0800, frederic.chen@mediatek.com wrote:
> From: Frederic Chen <frederic.chen@mediatek.com>
> 
> We added a struct tuning_addr which contains a field "present"
> so that the driver can tell the firmware if we have user tuning
> dataor not.
> 
> The strcut img_sw_buffer is also added. This struct has no cpu address
> field and uses a handle instead so that we don't pass a cpu address
> to co-processor.
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>

If the driver depends on this patch, it needs to come before the driver.

-- 
Sakari Ailus

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

Re: [RFC PATCH V3 4/5] platform: mtk-isp: Add Mediatek DIP driver

[Tomasz Figa]

Hi Frederic,

On Tue, Sep 10, 2019 at 4:23 AM <frederic.chen@mediatek.com> wrote:
>
> From: Frederic Chen <frederic.chen@mediatek.com>
>
> This patch adds the driver of Digital Image Processing (DIP)
> unit in Mediatek ISP system, providing image format
> conversion, resizing, and rotation features.
>
> The mtk-isp directory will contain drivers for multiple IP
> blocks found in Mediatek ISP system. It will include ISP
> Pass 1 driver(CAM), sensor interface driver, DIP driver and
> face detection driver.
>
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  drivers/media/platform/mtk-isp/Makefile       |    7 +
>  .../media/platform/mtk-isp/isp_50/Makefile    |    7 +
>  .../platform/mtk-isp/isp_50/dip/Makefile      |   18 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c |  650 +++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h |  566 +++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h  |  156 ++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c |  521 ++++
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c         | 2255 +++++++++++++++++
>  8 files changed, 4180 insertions(+)
>  create mode 100644 drivers/media/platform/mtk-isp/Makefile
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
>

Thanks for sending v3!

I'm going to do a full review a bit later, but please check one
comment about power handling below.

Other than that one comment, from a quick look, I think we only have a
number of style issues left. Thanks for the hard work!

[snip]
> +static void dip_runner_func(struct work_struct *work)
> +{
> +       struct mtk_dip_request *req = mtk_dip_hw_mdp_work_to_req(work);
> +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> +       struct img_config *config_data =
> +               (struct img_config *)req->working_buf->config_data.vaddr;
> +
> +       /*
> +        * Call MDP/GCE API to do HW excecution
> +        * Pass the framejob to MDP driver
> +        */
> +       pm_runtime_get_sync(dip_dev->dev);
> +       mdp_cmdq_sendtask(dip_dev->mdp_pdev, config_data,
> +                         &req->img_fparam.frameparam, NULL, false,
> +                         dip_mdp_cb_func, req);
> +}
[snip]
> +static void dip_composer_workfunc(struct work_struct *work)
> +{
> +       struct mtk_dip_request *req = mtk_dip_hw_fw_work_to_req(work);
> +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> +       struct img_ipi_param ipi_param;
> +       struct mtk_dip_hw_subframe *buf;
> +       int ret;
> +
> +       down(&dip_dev->sem);
> +
> +       buf = mtk_dip_hw_working_buf_alloc(req->dip_pipe->dip_dev);
> +       if (!buf) {
> +               dev_err(req->dip_pipe->dip_dev->dev,
> +                       "%s:%s:req(%p): no free working buffer available\n",
> +                       __func__, req->dip_pipe->desc->name, req);
> +       }
> +
> +       req->working_buf = buf;
> +       mtk_dip_wbuf_to_ipi_img_addr(&req->img_fparam.frameparam.subfrm_data,
> +                                    &buf->buffer);
> +       memset(buf->buffer.vaddr, 0, DIP_SUB_FRM_SZ);
> +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.config_data,
> +                                       &buf->config_data);
> +       memset(buf->config_data.vaddr, 0, DIP_COMP_SZ);
> +
> +       if (!req->img_fparam.frameparam.tuning_data.present) {
> +               /*
> +                * When user enqueued without tuning buffer,
> +                * it would use driver internal buffer.
> +                */
> +               dev_dbg(dip_dev->dev,
> +                       "%s: frame_no(%d) has no tuning_data\n",
> +                       __func__, req->img_fparam.frameparam.frame_no);
> +
> +               mtk_dip_wbuf_to_ipi_tuning_addr
> +                               (&req->img_fparam.frameparam.tuning_data,
> +                                &buf->tuning_buf);
> +               memset(buf->tuning_buf.vaddr, 0, DIP_TUNING_SZ);
> +       }
> +
> +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.self_data,
> +                                       &buf->frameparam);
> +       memcpy(buf->frameparam.vaddr, &req->img_fparam.frameparam,
> +              sizeof(req->img_fparam.frameparam));
> +       ipi_param.usage = IMG_IPI_FRAME;
> +       ipi_param.frm_param.handle = req->id;
> +       ipi_param.frm_param.scp_addr = (u32)buf->frameparam.scp_daddr;
> +
> +       mutex_lock(&dip_dev->hw_op_lock);
> +       atomic_inc(&dip_dev->num_composing);
> +       ret = scp_ipi_send(dip_dev->scp_pdev, SCP_IPI_DIP, &ipi_param,
> +                          sizeof(ipi_param), 0);

We're not holding the pm_runtime enable count here
(pm_runtime_get_sync() wasn't called), so rproc_shutdown() might have
been called. Wouldn't that affect the ability for this IPI to run?

We had a related discussion with Jerry on the FD series and I think
the conclusion is:
a) if there is any state that needs to be preserved between jobs, that
would be cleared by rproc_shutdown() then we need to call
rproc_boot/shutdown() when we start/stop streaming.
b) it there is no such state, we can keep them inside runtime PM
callbacks, but we need to call pm_runtime_get_sync() before sending an
IPI and pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend()
after the SCP signals completion. In this case the runtime PM
autosuspend delay should be set to around 2-3 times the delay needed
for rproc_shutdown() + rproc_boot() to complete.

Best regards,
Tomasz

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

Re: [RFC PATCH V3 4/5] platform: mtk-isp: Add Mediatek DIP driver

[Frederic Chen]

Hi Tomasz,

I appreciate your helpful comments.


On Tue, 2019-09-10 at 13:04 +0900, Tomasz Figa wrote:
> Hi Frederic,
> 
> On Tue, Sep 10, 2019 at 4:23 AM <frederic.chen@mediatek.com> wrote:
> >
> > From: Frederic Chen <frederic.chen@mediatek.com>
> >
> > This patch adds the driver of Digital Image Processing (DIP)
> > unit in Mediatek ISP system, providing image format
> > conversion, resizing, and rotation features.
> >
> > The mtk-isp directory will contain drivers for multiple IP
> > blocks found in Mediatek ISP system. It will include ISP
> > Pass 1 driver(CAM), sensor interface driver, DIP driver and
> > face detection driver.
> >
> > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > ---
> >  drivers/media/platform/mtk-isp/Makefile       |    7 +
> >  .../media/platform/mtk-isp/isp_50/Makefile    |    7 +
> >  .../platform/mtk-isp/isp_50/dip/Makefile      |   18 +
> >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c |  650 +++++
> >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h |  566 +++++
> >  .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h  |  156 ++
> >  .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c |  521 ++++
> >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c         | 2255 +++++++++++++++++
> >  8 files changed, 4180 insertions(+)
> >  create mode 100644 drivers/media/platform/mtk-isp/Makefile
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> >
> 
> Thanks for sending v3!
> 
> I'm going to do a full review a bit later, but please check one
> comment about power handling below.
> 
> Other than that one comment, from a quick look, I think we only have a
> number of style issues left. Thanks for the hard work!
> 
> [snip]
> > +static void dip_runner_func(struct work_struct *work)
> > +{
> > +       struct mtk_dip_request *req = mtk_dip_hw_mdp_work_to_req(work);
> > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > +       struct img_config *config_data =
> > +               (struct img_config *)req->working_buf->config_data.vaddr;
> > +
> > +       /*
> > +        * Call MDP/GCE API to do HW excecution
> > +        * Pass the framejob to MDP driver
> > +        */
> > +       pm_runtime_get_sync(dip_dev->dev);
> > +       mdp_cmdq_sendtask(dip_dev->mdp_pdev, config_data,
> > +                         &req->img_fparam.frameparam, NULL, false,
> > +                         dip_mdp_cb_func, req);
> > +}
> [snip]
> > +static void dip_composer_workfunc(struct work_struct *work)
> > +{
> > +       struct mtk_dip_request *req = mtk_dip_hw_fw_work_to_req(work);
> > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > +       struct img_ipi_param ipi_param;
> > +       struct mtk_dip_hw_subframe *buf;
> > +       int ret;
> > +
> > +       down(&dip_dev->sem);
> > +
> > +       buf = mtk_dip_hw_working_buf_alloc(req->dip_pipe->dip_dev);
> > +       if (!buf) {
> > +               dev_err(req->dip_pipe->dip_dev->dev,
> > +                       "%s:%s:req(%p): no free working buffer available\n",
> > +                       __func__, req->dip_pipe->desc->name, req);
> > +       }
> > +
> > +       req->working_buf = buf;
> > +       mtk_dip_wbuf_to_ipi_img_addr(&req->img_fparam.frameparam.subfrm_data,
> > +                                    &buf->buffer);
> > +       memset(buf->buffer.vaddr, 0, DIP_SUB_FRM_SZ);
> > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.config_data,
> > +                                       &buf->config_data);
> > +       memset(buf->config_data.vaddr, 0, DIP_COMP_SZ);
> > +
> > +       if (!req->img_fparam.frameparam.tuning_data.present) {
> > +               /*
> > +                * When user enqueued without tuning buffer,
> > +                * it would use driver internal buffer.
> > +                */
> > +               dev_dbg(dip_dev->dev,
> > +                       "%s: frame_no(%d) has no tuning_data\n",
> > +                       __func__, req->img_fparam.frameparam.frame_no);
> > +
> > +               mtk_dip_wbuf_to_ipi_tuning_addr
> > +                               (&req->img_fparam.frameparam.tuning_data,
> > +                                &buf->tuning_buf);
> > +               memset(buf->tuning_buf.vaddr, 0, DIP_TUNING_SZ);
> > +       }
> > +
> > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.self_data,
> > +                                       &buf->frameparam);
> > +       memcpy(buf->frameparam.vaddr, &req->img_fparam.frameparam,
> > +              sizeof(req->img_fparam.frameparam));
> > +       ipi_param.usage = IMG_IPI_FRAME;
> > +       ipi_param.frm_param.handle = req->id;
> > +       ipi_param.frm_param.scp_addr = (u32)buf->frameparam.scp_daddr;
> > +
> > +       mutex_lock(&dip_dev->hw_op_lock);
> > +       atomic_inc(&dip_dev->num_composing);
> > +       ret = scp_ipi_send(dip_dev->scp_pdev, SCP_IPI_DIP, &ipi_param,
> > +                          sizeof(ipi_param), 0);
> 
> We're not holding the pm_runtime enable count here
> (pm_runtime_get_sync() wasn't called), so rproc_shutdown() might have
> been called. Wouldn't that affect the ability for this IPI to run?
> 
> We had a related discussion with Jerry on the FD series and I think
> the conclusion is:
> a) if there is any state that needs to be preserved between jobs, that
> would be cleared by rproc_shutdown() then we need to call
> rproc_boot/shutdown() when we start/stop streaming.
> b) it there is no such state, we can keep them inside runtime PM
> callbacks, but we need to call pm_runtime_get_sync() before sending an
> IPI and pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend()
> after the SCP signals completion. In this case the runtime PM
> autosuspend delay should be set to around 2-3 times the delay needed
> for rproc_shutdown() + rproc_boot() to complete.

Since each IMG_IPI_FRAME command is stateless, I would like to
use pm_runtime_get_sync()/ pm_runtime_mark_last_busy()/
pm_runtime_put_autosuspend() to fix this issue (solution b).

> 
> Best regards,
> Tomasz


Sincerely,

Frederic Chen



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

Re: [RFC PATCH V3 4/5] platform: mtk-isp: Add Mediatek DIP driver

[Tomasz Figa]

On Thu, Sep 12, 2019 at 2:41 AM Frederic Chen
<frederic.chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> I appreciate your helpful comments.
>
>
> On Tue, 2019-09-10 at 13:04 +0900, Tomasz Figa wrote:
> > Hi Frederic,
> >
> > On Tue, Sep 10, 2019 at 4:23 AM <frederic.chen@mediatek.com> wrote:
> > >
> > > From: Frederic Chen <frederic.chen@mediatek.com>
> > >
> > > This patch adds the driver of Digital Image Processing (DIP)
> > > unit in Mediatek ISP system, providing image format
> > > conversion, resizing, and rotation features.
> > >
> > > The mtk-isp directory will contain drivers for multiple IP
> > > blocks found in Mediatek ISP system. It will include ISP
> > > Pass 1 driver(CAM), sensor interface driver, DIP driver and
> > > face detection driver.
> > >
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  drivers/media/platform/mtk-isp/Makefile       |    7 +
> > >  .../media/platform/mtk-isp/isp_50/Makefile    |    7 +
> > >  .../platform/mtk-isp/isp_50/dip/Makefile      |   18 +
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c |  650 +++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h |  566 +++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h  |  156 ++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c |  521 ++++
> > >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c         | 2255 +++++++++++++++++
> > >  8 files changed, 4180 insertions(+)
> > >  create mode 100644 drivers/media/platform/mtk-isp/Makefile
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > >
> >
> > Thanks for sending v3!
> >
> > I'm going to do a full review a bit later, but please check one
> > comment about power handling below.
> >
> > Other than that one comment, from a quick look, I think we only have a
> > number of style issues left. Thanks for the hard work!
> >
> > [snip]
> > > +static void dip_runner_func(struct work_struct *work)
> > > +{
> > > +       struct mtk_dip_request *req = mtk_dip_hw_mdp_work_to_req(work);
> > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > +       struct img_config *config_data =
> > > +               (struct img_config *)req->working_buf->config_data.vaddr;
> > > +
> > > +       /*
> > > +        * Call MDP/GCE API to do HW excecution
> > > +        * Pass the framejob to MDP driver
> > > +        */
> > > +       pm_runtime_get_sync(dip_dev->dev);
> > > +       mdp_cmdq_sendtask(dip_dev->mdp_pdev, config_data,
> > > +                         &req->img_fparam.frameparam, NULL, false,
> > > +                         dip_mdp_cb_func, req);
> > > +}
> > [snip]
> > > +static void dip_composer_workfunc(struct work_struct *work)
> > > +{
> > > +       struct mtk_dip_request *req = mtk_dip_hw_fw_work_to_req(work);
> > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > +       struct img_ipi_param ipi_param;
> > > +       struct mtk_dip_hw_subframe *buf;
> > > +       int ret;
> > > +
> > > +       down(&dip_dev->sem);
> > > +
> > > +       buf = mtk_dip_hw_working_buf_alloc(req->dip_pipe->dip_dev);
> > > +       if (!buf) {
> > > +               dev_err(req->dip_pipe->dip_dev->dev,
> > > +                       "%s:%s:req(%p): no free working buffer available\n",
> > > +                       __func__, req->dip_pipe->desc->name, req);
> > > +       }
> > > +
> > > +       req->working_buf = buf;
> > > +       mtk_dip_wbuf_to_ipi_img_addr(&req->img_fparam.frameparam.subfrm_data,
> > > +                                    &buf->buffer);
> > > +       memset(buf->buffer.vaddr, 0, DIP_SUB_FRM_SZ);
> > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.config_data,
> > > +                                       &buf->config_data);
> > > +       memset(buf->config_data.vaddr, 0, DIP_COMP_SZ);
> > > +
> > > +       if (!req->img_fparam.frameparam.tuning_data.present) {
> > > +               /*
> > > +                * When user enqueued without tuning buffer,
> > > +                * it would use driver internal buffer.
> > > +                */
> > > +               dev_dbg(dip_dev->dev,
> > > +                       "%s: frame_no(%d) has no tuning_data\n",
> > > +                       __func__, req->img_fparam.frameparam.frame_no);
> > > +
> > > +               mtk_dip_wbuf_to_ipi_tuning_addr
> > > +                               (&req->img_fparam.frameparam.tuning_data,
> > > +                                &buf->tuning_buf);
> > > +               memset(buf->tuning_buf.vaddr, 0, DIP_TUNING_SZ);
> > > +       }
> > > +
> > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.self_data,
> > > +                                       &buf->frameparam);
> > > +       memcpy(buf->frameparam.vaddr, &req->img_fparam.frameparam,
> > > +              sizeof(req->img_fparam.frameparam));
> > > +       ipi_param.usage = IMG_IPI_FRAME;
> > > +       ipi_param.frm_param.handle = req->id;
> > > +       ipi_param.frm_param.scp_addr = (u32)buf->frameparam.scp_daddr;
> > > +
> > > +       mutex_lock(&dip_dev->hw_op_lock);
> > > +       atomic_inc(&dip_dev->num_composing);
> > > +       ret = scp_ipi_send(dip_dev->scp_pdev, SCP_IPI_DIP, &ipi_param,
> > > +                          sizeof(ipi_param), 0);
> >
> > We're not holding the pm_runtime enable count here
> > (pm_runtime_get_sync() wasn't called), so rproc_shutdown() might have
> > been called. Wouldn't that affect the ability for this IPI to run?
> >
> > We had a related discussion with Jerry on the FD series and I think
> > the conclusion is:
> > a) if there is any state that needs to be preserved between jobs, that
> > would be cleared by rproc_shutdown() then we need to call
> > rproc_boot/shutdown() when we start/stop streaming.
> > b) it there is no such state, we can keep them inside runtime PM
> > callbacks, but we need to call pm_runtime_get_sync() before sending an
> > IPI and pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend()
> > after the SCP signals completion. In this case the runtime PM
> > autosuspend delay should be set to around 2-3 times the delay needed
> > for rproc_shutdown() + rproc_boot() to complete.
>
> Since each IMG_IPI_FRAME command is stateless, I would like to
> use pm_runtime_get_sync()/ pm_runtime_mark_last_busy()/
> pm_runtime_put_autosuspend() to fix this issue (solution b).

What does IMG_IPI_INIT do then? Do we need it at all?

I'm worried about the fact that we call rproc_boot(), IMG_IPI_INIT and
then rproc_shutdown(). The latter can completely shutdown the SCP and
clear any state there. How would the effects of IMG_IPI_INIT be
preserved until IMG_IPI_FRAME is called?

Best regards,
Tomasz

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

Re: [RFC PATCH V3 4/5] platform: mtk-isp: Add Mediatek DIP driver

[Frederic Chen]

Dear Tomasz,


On Thu, 2019-09-12 at 14:58 +0900, Tomasz Figa wrote:
> On Thu, Sep 12, 2019 at 2:41 AM Frederic Chen
> <frederic.chen@mediatek.com> wrote:
> >
> > Hi Tomasz,
> >
> > I appreciate your helpful comments.
> >
> >
> > On Tue, 2019-09-10 at 13:04 +0900, Tomasz Figa wrote:
> > > Hi Frederic,
> > >
> > > On Tue, Sep 10, 2019 at 4:23 AM <frederic.chen@mediatek.com> wrote:
> > > >
> > > > From: Frederic Chen <frederic.chen@mediatek.com>
> > > >
> > > > This patch adds the driver of Digital Image Processing (DIP)
> > > > unit in Mediatek ISP system, providing image format
> > > > conversion, resizing, and rotation features.
> > > >
> > > > The mtk-isp directory will contain drivers for multiple IP
> > > > blocks found in Mediatek ISP system. It will include ISP
> > > > Pass 1 driver(CAM), sensor interface driver, DIP driver and
> > > > face detection driver.
> > > >
> > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > ---
> > > >  drivers/media/platform/mtk-isp/Makefile       |    7 +
> > > >  .../media/platform/mtk-isp/isp_50/Makefile    |    7 +
> > > >  .../platform/mtk-isp/isp_50/dip/Makefile      |   18 +
> > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c |  650 +++++
> > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h |  566 +++++
> > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h  |  156 ++
> > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c |  521 ++++
> > > >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c         | 2255 +++++++++++++++++
> > > >  8 files changed, 4180 insertions(+)
> > > >  create mode 100644 drivers/media/platform/mtk-isp/Makefile
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > > >
> > >
> > > Thanks for sending v3!
> > >
> > > I'm going to do a full review a bit later, but please check one
> > > comment about power handling below.
> > >
> > > Other than that one comment, from a quick look, I think we only have a
> > > number of style issues left. Thanks for the hard work!
> > >
> > > [snip]
> > > > +static void dip_runner_func(struct work_struct *work)
> > > > +{
> > > > +       struct mtk_dip_request *req = mtk_dip_hw_mdp_work_to_req(work);
> > > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > > +       struct img_config *config_data =
> > > > +               (struct img_config *)req->working_buf->config_data.vaddr;
> > > > +
> > > > +       /*
> > > > +        * Call MDP/GCE API to do HW excecution
> > > > +        * Pass the framejob to MDP driver
> > > > +        */
> > > > +       pm_runtime_get_sync(dip_dev->dev);
> > > > +       mdp_cmdq_sendtask(dip_dev->mdp_pdev, config_data,
> > > > +                         &req->img_fparam.frameparam, NULL, false,
> > > > +                         dip_mdp_cb_func, req);
> > > > +}
> > > [snip]
> > > > +static void dip_composer_workfunc(struct work_struct *work)
> > > > +{
> > > > +       struct mtk_dip_request *req = mtk_dip_hw_fw_work_to_req(work);
> > > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > > +       struct img_ipi_param ipi_param;
> > > > +       struct mtk_dip_hw_subframe *buf;
> > > > +       int ret;
> > > > +
> > > > +       down(&dip_dev->sem);
> > > > +
> > > > +       buf = mtk_dip_hw_working_buf_alloc(req->dip_pipe->dip_dev);
> > > > +       if (!buf) {
> > > > +               dev_err(req->dip_pipe->dip_dev->dev,
> > > > +                       "%s:%s:req(%p): no free working buffer available\n",
> > > > +                       __func__, req->dip_pipe->desc->name, req);
> > > > +       }
> > > > +
> > > > +       req->working_buf = buf;
> > > > +       mtk_dip_wbuf_to_ipi_img_addr(&req->img_fparam.frameparam.subfrm_data,
> > > > +                                    &buf->buffer);
> > > > +       memset(buf->buffer.vaddr, 0, DIP_SUB_FRM_SZ);
> > > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.config_data,
> > > > +                                       &buf->config_data);
> > > > +       memset(buf->config_data.vaddr, 0, DIP_COMP_SZ);
> > > > +
> > > > +       if (!req->img_fparam.frameparam.tuning_data.present) {
> > > > +               /*
> > > > +                * When user enqueued without tuning buffer,
> > > > +                * it would use driver internal buffer.
> > > > +                */
> > > > +               dev_dbg(dip_dev->dev,
> > > > +                       "%s: frame_no(%d) has no tuning_data\n",
> > > > +                       __func__, req->img_fparam.frameparam.frame_no);
> > > > +
> > > > +               mtk_dip_wbuf_to_ipi_tuning_addr
> > > > +                               (&req->img_fparam.frameparam.tuning_data,
> > > > +                                &buf->tuning_buf);
> > > > +               memset(buf->tuning_buf.vaddr, 0, DIP_TUNING_SZ);
> > > > +       }
> > > > +
> > > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.self_data,
> > > > +                                       &buf->frameparam);
> > > > +       memcpy(buf->frameparam.vaddr, &req->img_fparam.frameparam,
> > > > +              sizeof(req->img_fparam.frameparam));
> > > > +       ipi_param.usage = IMG_IPI_FRAME;
> > > > +       ipi_param.frm_param.handle = req->id;
> > > > +       ipi_param.frm_param.scp_addr = (u32)buf->frameparam.scp_daddr;
> > > > +
> > > > +       mutex_lock(&dip_dev->hw_op_lock);
> > > > +       atomic_inc(&dip_dev->num_composing);
> > > > +       ret = scp_ipi_send(dip_dev->scp_pdev, SCP_IPI_DIP, &ipi_param,
> > > > +                          sizeof(ipi_param), 0);
> > >
> > > We're not holding the pm_runtime enable count here
> > > (pm_runtime_get_sync() wasn't called), so rproc_shutdown() might have
> > > been called. Wouldn't that affect the ability for this IPI to run?
> > >
> > > We had a related discussion with Jerry on the FD series and I think
> > > the conclusion is:
> > > a) if there is any state that needs to be preserved between jobs, that
> > > would be cleared by rproc_shutdown() then we need to call
> > > rproc_boot/shutdown() when we start/stop streaming.
> > > b) it there is no such state, we can keep them inside runtime PM
> > > callbacks, but we need to call pm_runtime_get_sync() before sending an
> > > IPI and pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend()
> > > after the SCP signals completion. In this case the runtime PM
> > > autosuspend delay should be set to around 2-3 times the delay needed
> > > for rproc_shutdown() + rproc_boot() to complete.
> >
> > Since each IMG_IPI_FRAME command is stateless, I would like to
> > use pm_runtime_get_sync()/ pm_runtime_mark_last_busy()/
> > pm_runtime_put_autosuspend() to fix this issue (solution b).
> 
> What does IMG_IPI_INIT do then? Do we need it at all?
> 
> I'm worried about the fact that we call rproc_boot(), IMG_IPI_INIT and
> then rproc_shutdown(). The latter can completely shutdown the SCP and
> clear any state there. How would the effects of IMG_IPI_INIT be
> preserved until IMG_IPI_FRAME is called?
> 

The command IMG_IPI_INIT is to initialize the DIP hardware. Although the
DIP hardware status is not cleared when SCP is powered off, it can still
be cleared after mtk_dip_runtime_suspend() is called (it means that DIP
is going to be powered off).

I would like to re-initialize the DIP with IMG_IPI_INIT in
mtk_dip_runtime_resume() to handle this case. Is is OK?


> Best regards,
> Tomasz


Sincerely,

Frederic Chen


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

Re: [RFC PATCH V3 4/5] platform: mtk-isp: Add Mediatek DIP driver

[Tomasz Figa]

On Thu, Sep 19, 2019 at 6:41 PM Frederic Chen
<frederic.chen@mediatek.com> wrote:
>
> Dear Tomasz,
>
>
> On Thu, 2019-09-12 at 14:58 +0900, Tomasz Figa wrote:
> > On Thu, Sep 12, 2019 at 2:41 AM Frederic Chen
> > <frederic.chen@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > I appreciate your helpful comments.
> > >
> > >
> > > On Tue, 2019-09-10 at 13:04 +0900, Tomasz Figa wrote:
> > > > Hi Frederic,
> > > >
> > > > On Tue, Sep 10, 2019 at 4:23 AM <frederic.chen@mediatek.com> wrote:
> > > > >
> > > > > From: Frederic Chen <frederic.chen@mediatek.com>
> > > > >
> > > > > This patch adds the driver of Digital Image Processing (DIP)
> > > > > unit in Mediatek ISP system, providing image format
> > > > > conversion, resizing, and rotation features.
> > > > >
> > > > > The mtk-isp directory will contain drivers for multiple IP
> > > > > blocks found in Mediatek ISP system. It will include ISP
> > > > > Pass 1 driver(CAM), sensor interface driver, DIP driver and
> > > > > face detection driver.
> > > > >
> > > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > > ---
> > > > >  drivers/media/platform/mtk-isp/Makefile       |    7 +
> > > > >  .../media/platform/mtk-isp/isp_50/Makefile    |    7 +
> > > > >  .../platform/mtk-isp/isp_50/dip/Makefile      |   18 +
> > > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c |  650 +++++
> > > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h |  566 +++++
> > > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-hw.h  |  156 ++
> > > > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-sys.c |  521 ++++
> > > > >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2.c         | 2255 +++++++++++++++++
> > > > >  8 files changed, 4180 insertions(+)
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/Makefile
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/Makefile
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/Makefile
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.c
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev.h
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-hw.h
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-sys.c
> > > > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > > > >
> > > >
> > > > Thanks for sending v3!
> > > >
> > > > I'm going to do a full review a bit later, but please check one
> > > > comment about power handling below.
> > > >
> > > > Other than that one comment, from a quick look, I think we only have a
> > > > number of style issues left. Thanks for the hard work!
> > > >
> > > > [snip]
> > > > > +static void dip_runner_func(struct work_struct *work)
> > > > > +{
> > > > > +       struct mtk_dip_request *req = mtk_dip_hw_mdp_work_to_req(work);
> > > > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > > > +       struct img_config *config_data =
> > > > > +               (struct img_config *)req->working_buf->config_data.vaddr;
> > > > > +
> > > > > +       /*
> > > > > +        * Call MDP/GCE API to do HW excecution
> > > > > +        * Pass the framejob to MDP driver
> > > > > +        */
> > > > > +       pm_runtime_get_sync(dip_dev->dev);
> > > > > +       mdp_cmdq_sendtask(dip_dev->mdp_pdev, config_data,
> > > > > +                         &req->img_fparam.frameparam, NULL, false,
> > > > > +                         dip_mdp_cb_func, req);
> > > > > +}
> > > > [snip]
> > > > > +static void dip_composer_workfunc(struct work_struct *work)
> > > > > +{
> > > > > +       struct mtk_dip_request *req = mtk_dip_hw_fw_work_to_req(work);
> > > > > +       struct mtk_dip_dev *dip_dev = req->dip_pipe->dip_dev;
> > > > > +       struct img_ipi_param ipi_param;
> > > > > +       struct mtk_dip_hw_subframe *buf;
> > > > > +       int ret;
> > > > > +
> > > > > +       down(&dip_dev->sem);
> > > > > +
> > > > > +       buf = mtk_dip_hw_working_buf_alloc(req->dip_pipe->dip_dev);
> > > > > +       if (!buf) {
> > > > > +               dev_err(req->dip_pipe->dip_dev->dev,
> > > > > +                       "%s:%s:req(%p): no free working buffer available\n",
> > > > > +                       __func__, req->dip_pipe->desc->name, req);
> > > > > +       }
> > > > > +
> > > > > +       req->working_buf = buf;
> > > > > +       mtk_dip_wbuf_to_ipi_img_addr(&req->img_fparam.frameparam.subfrm_data,
> > > > > +                                    &buf->buffer);
> > > > > +       memset(buf->buffer.vaddr, 0, DIP_SUB_FRM_SZ);
> > > > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.config_data,
> > > > > +                                       &buf->config_data);
> > > > > +       memset(buf->config_data.vaddr, 0, DIP_COMP_SZ);
> > > > > +
> > > > > +       if (!req->img_fparam.frameparam.tuning_data.present) {
> > > > > +               /*
> > > > > +                * When user enqueued without tuning buffer,
> > > > > +                * it would use driver internal buffer.
> > > > > +                */
> > > > > +               dev_dbg(dip_dev->dev,
> > > > > +                       "%s: frame_no(%d) has no tuning_data\n",
> > > > > +                       __func__, req->img_fparam.frameparam.frame_no);
> > > > > +
> > > > > +               mtk_dip_wbuf_to_ipi_tuning_addr
> > > > > +                               (&req->img_fparam.frameparam.tuning_data,
> > > > > +                                &buf->tuning_buf);
> > > > > +               memset(buf->tuning_buf.vaddr, 0, DIP_TUNING_SZ);
> > > > > +       }
> > > > > +
> > > > > +       mtk_dip_wbuf_to_ipi_img_sw_addr(&req->img_fparam.frameparam.self_data,
> > > > > +                                       &buf->frameparam);
> > > > > +       memcpy(buf->frameparam.vaddr, &req->img_fparam.frameparam,
> > > > > +              sizeof(req->img_fparam.frameparam));
> > > > > +       ipi_param.usage = IMG_IPI_FRAME;
> > > > > +       ipi_param.frm_param.handle = req->id;
> > > > > +       ipi_param.frm_param.scp_addr = (u32)buf->frameparam.scp_daddr;
> > > > > +
> > > > > +       mutex_lock(&dip_dev->hw_op_lock);
> > > > > +       atomic_inc(&dip_dev->num_composing);
> > > > > +       ret = scp_ipi_send(dip_dev->scp_pdev, SCP_IPI_DIP, &ipi_param,
> > > > > +                          sizeof(ipi_param), 0);
> > > >
> > > > We're not holding the pm_runtime enable count here
> > > > (pm_runtime_get_sync() wasn't called), so rproc_shutdown() might have
> > > > been called. Wouldn't that affect the ability for this IPI to run?
> > > >
> > > > We had a related discussion with Jerry on the FD series and I think
> > > > the conclusion is:
> > > > a) if there is any state that needs to be preserved between jobs, that
> > > > would be cleared by rproc_shutdown() then we need to call
> > > > rproc_boot/shutdown() when we start/stop streaming.
> > > > b) it there is no such state, we can keep them inside runtime PM
> > > > callbacks, but we need to call pm_runtime_get_sync() before sending an
> > > > IPI and pm_runtime_mark_last_busy() + pm_runtime_put_autosuspend()
> > > > after the SCP signals completion. In this case the runtime PM
> > > > autosuspend delay should be set to around 2-3 times the delay needed
> > > > for rproc_shutdown() + rproc_boot() to complete.
> > >
> > > Since each IMG_IPI_FRAME command is stateless, I would like to
> > > use pm_runtime_get_sync()/ pm_runtime_mark_last_busy()/
> > > pm_runtime_put_autosuspend() to fix this issue (solution b).
> >
> > What does IMG_IPI_INIT do then? Do we need it at all?
> >
> > I'm worried about the fact that we call rproc_boot(), IMG_IPI_INIT and
> > then rproc_shutdown(). The latter can completely shutdown the SCP and
> > clear any state there. How would the effects of IMG_IPI_INIT be
> > preserved until IMG_IPI_FRAME is called?
> >
>
> The command IMG_IPI_INIT is to initialize the DIP hardware. Although the
> DIP hardware status is not cleared when SCP is powered off, it can still
> be cleared after mtk_dip_runtime_suspend() is called (it means that DIP
> is going to be powered off).

Great, that's exactly what I wanted to confirm. Thanks. :)

>
> I would like to re-initialize the DIP with IMG_IPI_INIT in
> mtk_dip_runtime_resume() to handle this case. Is is OK?
>

Makes sense.

Best regards,
Tomasz

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

Re: [RFC PATCH V3 0/5] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC

[Sakari Ailus]

Hi Frederic,

On Tue, Sep 10, 2019 at 03:22:39AM +0800, frederic.chen@mediatek.com wrote:
> Hello,
> 
> This RFC patch series added Digital Image Processing (DIP) driver on Mediatek
> mt8183 SoC. It belongs to the Mediatek's ISP driver series based on V4L2 and
> media controller framework. I posted the main part of the DIP driver as RFC to
> discuss first and would like some review comments.

Could you post media-ctl -p and media-ctl --print-dot outputs for this
driver, please?

-- 
Sakari Ailus

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

Re: [RFC PATCH V3 0/5] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC

[Paul Menzel]

[Cc: +Sakari, +Guenter]


Dear Frederic, dear Linux folks,


Am 09.09.19 um 21:22 schrieb frederic.chen@mediatek.com:

> This RFC patch series added Digital Image Processing (DIP) driver on Mediatek
> mt8183 SoC. It belongs to the Mediatek's ISP driver series based on V4L2 and
> media controller framework. I posted the main part of the DIP driver as RFC to
> discuss first and would like some review comments.
> 
> I appreciate the comment of Tomasz in RFC V2. The RFC V3 patch addressed on all
> issues reviewed in V2 except the one about Mediatek proprietary MDP stride,
> depth and raw depth usage which is still under discussion. I will refactor
> the related parts once we come to the conclusion.
> 
> You can check the following URL for the detail.
> http://lists.infradead.org/pipermail/linux-mediatek/2019-September/023254.html
> 
> 
> In V3, I also removed all workaround solution about the following V4L2
> compliance tool issues so that we got the related failed result.
> 
> 1. Request API test doesn't know which buffers of the video devices are
> required so we got failed in testRequests()
> 
> 2. V4L2 compliance test check if the driver return error when passing an
> invalid image size, but in vb2_create_bufs() case, we don't know if the
> size check is required or not.
> 
> Please see the following URL for the detail.
> http://lists.infradead.org/pipermail/linux-mediatek/2019-June/020884.html
> 
> 
> Besides that, we got a new issue about the test case. When receiving the
> VIDIOC_SUBDEV_G_FMT ioctl on a DIP sub device's pad which connects with a
> meta video device, we return -EINVEL since it doesn't represent an image
> data flow (no width and height information), but the test case expects
> that the driver return some media format information.
> 
> 	Sub-Device ioctls (Sink Pad 1):
> 	fail: v4l2-test-subdevs.cpp(352): doioctl(node, VIDIOC_SUBDEV_G_FMT, &fmt)
> 	test Try VIDIOC_SUBDEV_G/S_FMT: FAIL
> 
> 
> ==============
>   Introduction
> ==============
> 
> Digital Image Processing (DIP) unit can accept the tuning parameters and
> adjust the image content in Mediatek ISP system. Furthermore, it performs
> demosaicing and noise reduction on the image to support the advanced camera
> features of the application. The DIP driver also support image format
> conversion, resizing and rotation with its hardware path.
> 
> The driver is implemented with V4L2 and media controller framework. We
> have the following entities describing the DIP path. Since a DIP frame has
> multiple buffers, the driver uses Request API to control the multiple
> buffer's enqueue flow.
> 
> 1. Meta (output video device): connects to DIP sub device. It accepts the
> input tuning buffer from userspace. The metadata interface used currently
> is only a temporary solution to kick off driver development and is not
> ready for reviewed yet.
> 
> 2. RAW (output video device): connects to DIP sub device. It accepts input
> image buffer from userspace.
> 
> 3. DIP (sub device): connects to MDP-0 and MDP-1. When processing an image,
> DIP hardware support multiple output images with different size and format
> so it needs two capture video devices to return the streaming data to the
> user.
> 
> 4. MDP-0 (capture video device): return the processed image data.
> 
> 5. MDP-1 (capture video device): return the processed image data, the
> image size and format can be different from the ones of MDP-0.
> 
> The overall file structure of the DIP driver is as following:
> 
> * mtk_dip-v4l2.c: implements DIP platform driver, V4L2 and vb2 operations.
> 
> * mtk_dip-sys.c: implements the hardware job handling flow including the part of
> interaction with the SCP and MDP.
> 
> * mtk_dip-dev.c: implements dip pipe utilities. DIP driver supports 3 software
> pipes (preview, capture and reprocessing) at the same time. All
> the pipes share the same DIP hardware to process the images.

Thank you for your work. I use the Lenovo IdeaPad Duet Chromebook 
(google/kukui variant of google/krane), and noticed the messages below 
using the camera in the browser with recent ChromeOS:

     [    0.000000] Linux version 5.10.180-22631-gc8e37fc5f0ab 
(chrome-bot@chromeos-release-builder-us-central1-b-x32-66-okmh) 
(Chromium OS 17.0_pre496208_p20230501-r6 clang version 17.0.0 
(/mnt/host/source/src/third_party/llvm-project 
98f5a340975bc00197c57e39eb4ca26e2da0e8a2), LLD 17.0.0) #1 SMP PREEMPT 
Wed Jul 26 19:01:55 PDT 2023
     […]
     [ 2766.733517] mtk-cam-dip 15022000.dip: req(0xffffff8e5fdc9800), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.737034] req(0xffffff8e5fdc9800), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.772352] mtk-cam-dip 15022000.dip: req(0xffffff8d88002000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.775906] req(0xffffff8d88002000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.788790] mtk-cam-dip 15022000.dip: req(0xffffff8d88000000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.792327] req(0xffffff8d88000000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.830257] mtk-cam-dip 15022000.dip: req(0xffffff8e5ff46000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.833806] req(0xffffff8e5ff46000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.869589] mtk-cam-dip 15022000.dip: req(0xffffff8e5ff44000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.873104] req(0xffffff8e5ff44000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.889804] mtk-cam-dip 15022000.dip: req(0xffffff8e5ff41000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.893351] req(0xffffff8e5ff41000), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.939293] mtk-cam-dip 15022000.dip: req(0xffffff8e5ff43800), 
req->dip_pipe(0xffffff8d829a0398)
     [ 2766.942827] req(0xffffff8e5ff43800), 
req->dip_pipe(0xffffff8d829a0398)

Search for that in the upstream Linux kernel, I found out, the the DIP 
support is not upstreamed yet, and also has not been added to the 
Chromium OS Linux kernel branches chromeos-5.15 and chromeos-6.1 [1].

Were you able to come to a conclusion regarding the two(?) issues 
mentioned in your cover letter, so this series can be re-posted as 
non-RFC? The driver seems to work well on millions(?) of devices, so 
it’d be great to have it upstream.

[…]


Kind regards,

Paul


[1]: 
https://chromium-review.googlesource.com/q/I1d1ba58cbdcdcc161b140398fc26b24ec2134cdb

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