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

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

[Frederic Chen]

Hello,

This is the first version of the RFC patch series adding Digital Image
Processing (DIP) driver on Mediatek mt8183 SoC, which will be used in camera
features on CrOS application. It belongs to the first 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 on
the overall structure of the driver.

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.

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 image 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-dev-ctx-core.c: Implements common software flow of DIP driver.
  DIP driver supports two or more software contexts. For example, context 0 is
  created for preview path and context 1 is for capture path. Both the two
  contexts share the same DIP hardware to process the images.
* mtk_dip-v4l2.c: Static DIP contexts configuration.
* mtk_dip.c: Controls the hardware flow.
* mtk_dip-dev.c: Implements context-independent flow.
* mtk_dip-ctrl.c: Handles the HW ctrl request from userspace.
* mtk_dip-smem-drv.c: Provides the shared memory management required operation.
  We reserved a memory region for the co-processor and DIP to exchange the
  tuning and hardware configuration data.
* mtk_dip-v4l2-util.c: Implements V4L2 and vb2 ops.

Frederic Chen (7):
  [media] dt-bindings: mt8183: Add binding for DIP shared memory
  dts: arm64: mt8183: Add DIP shared memory node
  [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings
  [media] dt-bindings: mt8183: Added DIP dt-bindings
  dts: arm64: mt8183: Add DIP nodes
  media: platform: Add Mediatek DIP driver KConfig
  [media] platform: mtk-isp: Add Mediatek DIP driver

 .../bindings/media/mediatek,dip_smem.txt           |   29 +
 .../bindings/media/mediatek,mt8183-dip.txt         |   35 +
 .../mediatek,reserve-memory-dip_smem.txt           |   45 +
 arch/arm64/boot/dts/mediatek/mt8183.dtsi           |   36 +
 drivers/media/platform/Kconfig                     |    2 +
 drivers/media/platform/mtk-isp/Kconfig             |   21 +
 drivers/media/platform/mtk-isp/Makefile            |   18 +
 drivers/media/platform/mtk-isp/isp_50/Makefile     |   17 +
 drivers/media/platform/mtk-isp/isp_50/dip/Makefile |   35 +
 .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h     |  188 +++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c     |  173 +++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h     |   43 +
 .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h      |  319 ++++
 .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c      | 1643 ++++++++++++++++++++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c      |  374 +++++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h      |  191 +++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c |  452 ++++++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h     |   25 +
 .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c         | 1000 ++++++++++++
 .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h         |   38 +
 .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c     |  292 ++++
 .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h     |   60 +
 .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c    | 1385 +++++++++++++++++
 .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h    |   93 ++
 24 files changed, 6514 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
 create mode 100644 drivers/media/platform/mtk-isp/Kconfig
 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-core.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
 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-smem-drv.c
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
 create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h

-- 
1.9.1


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

[RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Frederic Chen]

This patch adds the binding for describing the shared memory
used to exchange configuration and tuning data between the
co-processor and Digital Image Processing (DIP) unit of the
camera ISP system on Mediatek SoCs.

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt

diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
new file mode 100644
index 0000000..0ded478
--- /dev/null
+++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
@@ -0,0 +1,45 @@
+Mediatek DIP Shared Memory binding
+
+This binding describes the shared memory, which serves the purpose of
+describing the shared memory region used to exchange data between Digital
+Image Processing (DIP) and co-processor in Mediatek SoCs.
+
+The co-processor doesn't have the iommu so we need to use the physical
+address to access the shared buffer in the firmware.
+
+The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
+it can use dma address to access the memory region.
+(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
+
+
+Required properties:
+
+- compatible: must be "mediatek,reserve-memory-dip_smem"
+
+- reg: required for static allocation (see reserved-memory.txt for
+  the detailed usage)
+
+- alloc-range: required for dynamic allocation. The range must
+  between 0x00000400 and 0x100000000 due to the co-processer's
+  addressing limitation
+
+- size: required for dynamic allocation. The unit is bytes.
+  If you want to enable the full feature of Digital Processing Unit,
+  you need 20 MB at least.
+
+
+Example:
+
+The following example shows the DIP shared memory setup for MT8183.
+
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		reserve-memory-isp_smem {
+			compatible = "mediatek,reserve-memory-dip_smem";
+			size = <0 0x1400000>;
+			alignment = <0 0x1000>;
+			alloc-ranges = <0 0x40000000 0 0x50000000>;
+		};
+	};
-- 
1.9.1


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

[RFC PATCH V0 2/7] dts: arm64: mt8183: Add DIP shared memory node

[Frederic Chen]

This patch adds a shared memory region used on mt8183 for
exchanging tuning data between co-processor and
Digital Image Processing (DIP) unit.

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

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 63db9cc..fff67c4 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -133,6 +133,19 @@
 		clock-output-names = "clk26m";
 	};
 
+	reserved-memory {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		reserve-memory-dip_smem {
+			compatible = "mediatek,reserve-memory-dip_smem";
+			no-map;
+			size = <0 0x01400000>;  /*20 MB share mem size */
+			alignment = <0 0x1000>;
+			alloc-ranges = <0 0x40000000 0 0x10000000>;
+		};
+	};
+
 	timer {
 		compatible = "arm,armv8-timer";
 		interrupt-parent = <&gic>;
-- 
1.9.1


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

[RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings

[Frederic Chen]

This patch adds the DT binding documentation for the shared memory
between DIP (Digital Image Processing) unit of the camera ISP system
and the co-processor in Mediatek SoCs.

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

diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
new file mode 100644
index 0000000..5533721
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
@@ -0,0 +1,29 @@
+Mediatek ISP Shared Memory Device
+
+Mediatek ISP Shared Memory Device is used to manage shared memory
+among CPU, ISP IPs and coprocessor. It is associated with a reserved
+memory region (Please see Documentation\devicetree\bindings\
+reserved-memory\mediatek,reserve-memory-isp_smem.txt) and
+and provide the context to allocate memory with dma addresses.
+
+Required properties:
+- compatible: Should be "mediatek,isp_smem"
+
+- iommus: should point to the respective IOMMU block with master port
+  as argument. Please set the ports which may be accessed
+  through the common path. You can see
+  Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
+  for the detail.
+
+- mediatek,larb: must contain the local arbiters in the current Socs.
+  Please set the larb of camsys for Pass 1 and imgsys for DIP, or both
+  if you are using all the camera function. You can see
+  Documentation/devicetree/bindings/memory-controllers/
+  mediatek,smi-larb.txt for the detail.
+
+Example:
+	isp_smem: isp_smem {
+		compatible = "mediatek,isp_smem";
+		mediatek,larb = <&larb5>;
+		iommus = <&iommu M4U_PORT_CAM_IMGI>;
+	};
-- 
1.9.1


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

[RFC PATCH V0 4/7] [media] dt-bindings: mt8183: Added DIP dt-bindings

[Frederic Chen]

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

Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
---
 .../bindings/media/mediatek,mt8183-dip.txt         | 35 ++++++++++++++++++++++
 1 file changed, 35 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 0000000..0e1994b
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
@@ -0,0 +1,35 @@
+* 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,larb: must contain the local arbiters in the current Socs, see
+  Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
+  for details.
+- clocks: must contain the local arbiters 5 (LARB5) and DIP clock
+- clock-names: must contain DIP_CG_IMG_LARB5 and DIP_CG_IMG_DIP
+
+Example:
+	dip: dip@15022000 {
+		compatible = "mediatek,mt8183-dip";
+		mediatek,larb = <&larb5>;
+		mediatek,mdp3 = <&mdp_rdma0>;
+		mediatek,vpu = <&vpu>;
+		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 = "DIP_CG_IMG_LARB5",
+			      "DIP_CG_IMG_DIP";
+	};
-- 
1.9.1


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

[RFC PATCH V0 5/7] dts: arm64: mt8183: Add DIP nodes

[Frederic Chen]

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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index fff67c4..19b2c13 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -377,6 +377,29 @@
 		#clock-cells = <1>;
 	};
 
+	dip_smem: dip_smem {
+		compatible = "mediatek,dip_smem";
+		mediatek,larb = <&larb5>;
+		iommus = <&iommu M4U_PORT_CAM_IMGI>;
+	};
+
+	dip: dip@15022000 {
+		compatible = "mediatek,mt8183-dip";
+		mediatek,larb = <&larb5>;
+		mediatek,mdp3 = <&mdp_rdma0>;
+		mediatek,vpu = <&vpu>;
+		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 =
+				"DIP_CG_IMG_LARB5",
+				"DIP_CG_IMG_DIP";
+		smem_device = <&dip_smem>;
+	};
+
 	vdecsys: syscon@16000000 {
 		compatible = "mediatek,mt8183-vdecsys", "syscon";
 		reg = <0 0x16000000 0 0x1000>;
-- 
1.9.1


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

[RFC PATCH V0 6/7] media: platform: Add Mediatek DIP driver KConfig

[Frederic Chen]

This patch adds KConfig for Mediatek Digital Image Processing
driver (DIP). DIP is embedded in Mediatek SoCs. It can provide
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 | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)
 create mode 100644 drivers/media/platform/mtk-isp/Kconfig

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index a505e9f..ef08d48 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 0000000..e5f36d1
--- /dev/null
+++ b/drivers/media/platform/mtk-isp/Kconfig
@@ -0,0 +1,21 @@
+config VIDEO_MEDIATEK_ISP_DIP_SUPPORT
+	bool "Mediatek Digital Image Processing function"
+	select DMA_SHARED_BUFFER
+	select VIDEO_V4L2_SUBDEV_API
+	select VIDEOBUF2_DMA_CONTIG
+	select VIDEOBUF2_CORE
+	select VIDEOBUF2_V4L2
+	select VIDEOBUF2_MEMOPS
+	select VIDEOBUF2_VMALLOC
+	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.
+
-- 
1.9.1


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

Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver

[Brian Norris]

Hi,

On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote:
> 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            |   18 +
>  drivers/media/platform/mtk-isp/isp_50/Makefile     |   17 +
>  drivers/media/platform/mtk-isp/isp_50/dip/Makefile |   35 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h     |  188 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c     |  173 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h     |   43 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h      |  319 ++++
>  .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c      | 1643 ++++++++++++++++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c      |  374 +++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h      |  191 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c |  452 ++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h     |   25 +
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c         | 1000 ++++++++++++
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h         |   38 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c     |  292 ++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h     |   60 +
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c    | 1385 +++++++++++++++++
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h    |   93 ++
>  18 files changed, 6346 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-core.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
>  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-smem-drv.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h
> 

...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> new file mode 100644
> index 0000000..9d29507
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> @@ -0,0 +1,173 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Frederic Chen <frederic.chen@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include "mtk_dip-dev.h"
> +#include "mtk_dip-ctrl.h"
> +
> +#define CONFIG_MTK_DIP_COMMON_UT

Please don't do this. You're pretending to have configurability that you
don't actually have.

> +
> +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl);
> +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl);
> +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl);
> +
> +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx_queue *queue =
> +		container_of(ctrl->handler,
> +			     struct mtk_dip_ctx_queue, ctrl_handler);
> +
> +	if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT ||
> +	    ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) {
> +		pr_err("Invalid buffer usage id %d", ctrl->val);
> +		return;
> +	}
> +	queue->buffer_usage = ctrl->val;
> +}
> +
> +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx_queue *queue =
> +		container_of(ctrl->handler,
> +			     struct mtk_dip_ctx_queue, ctrl_handler);
> +
> +	if (ctrl->val != 0 || ctrl->val != 90 ||
> +	    ctrl->val != 180 || ctrl->val != 270) {
> +		pr_err("Invalid buffer rotation %d", ctrl->val);
> +		return;
> +	}
> +	queue->rotation = ctrl->val;
> +}
> +
> +static const struct v4l2_ctrl_ops mtk_dip_ctx_ctrl_ops = {
> +	.s_ctrl = mtk_dip_ctx_s_ctrl,
> +};
> +
> +#ifdef CONFIG_MTK_DIP_COMMON_UT

Kill the #ifdef if you're not actually going to make this a Kconfig.
Same elsewhere.

> +
> +static void handle_ctrl_common_util_ut_set_debug_mode
> +	(struct v4l2_ctrl *ctrl)
> +{
> +	struct mtk_dip_ctx *dev_ctx =
> +		container_of(ctrl->handler, struct mtk_dip_ctx, ctrl_handler);
> +	dev_ctx->mode = ctrl->val;
> +	dev_dbg(&dev_ctx->pdev->dev, "Set ctx(id = %d) mode to %d\n",
> +		dev_ctx->ctx_id, dev_ctx->mode);
> +}
> +
> +static const struct v4l2_ctrl_config mtk_dip_mode_config = {
> +	.ops	= &mtk_dip_ctx_ctrl_ops,
> +	.id	= V4L2_CID_PRIVATE_SET_CTX_MODE_NUM,
> +	.name	= "MTK ISP UNIT TEST CASE",
> +	.type	= V4L2_CTRL_TYPE_INTEGER,
> +	.min	= 0,
> +	.max	= 65535,
> +	.step	= 1,
> +	.def	= 0,
> +	.flags	= V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE,
> +};
> +#endif /* CONFIG_MTK_DIP_COMMON_UT */
> +
> +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	switch (ctrl->id) {
> +	#ifdef CONFIG_MTK_DIP_COMMON_UT
> +	case V4L2_CID_PRIVATE_SET_CTX_MODE_NUM:
> +		handle_ctrl_common_util_ut_set_debug_mode(ctrl);
> +		break;
> +	#endif /* CONFIG_MTK_DIP_COMMON_UT */
> +	default:
> +			break;
> +	}
> +	return 0;
> +}
> +

...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> new file mode 100644
> index 0000000..c735919
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> @@ -0,0 +1,1643 @@
...

> +#ifdef MTK_DIP_CTX_DIP_V4L2_UT

What is this #ifdef'ery? I don't see MTK_DIP_CTX_DIP_V4L2_UT anywhere.

> +static int check_and_refill_dip_ut_start_ipi_param
> +	(struct img_ipi_frameparam *ipi_param,
> +	 struct mtk_dip_ctx_buffer *ctx_buf_in,
> +	 struct mtk_dip_ctx_buffer *ctx_buf_out)
> +{
> +	/* Check the buffer size information from user space */
> +	int ret = 0;
> +	unsigned char *buffer_ptr = NULL;
> +	const unsigned int src_width = 3264;
...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> new file mode 100644
> index 0000000..b425031
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> @@ -0,0 +1,1000 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 Mediatek Corporation.
> + * Copyright (c) 2017 Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License version
> + * 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * MTK_DIP-v4l2 is highly based on Intel IPU 3 chrome driver
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-dma-contig.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-event.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +
> +#include "mtk_dip-dev.h"
> +#include "mtk_dip-v4l2-util.h"
> +
> +static u32 mtk_dip_node_get_v4l2_cap
> +	(struct mtk_dip_ctx_queue *node_ctx);
> +
> +static int mtk_dip_videoc_s_meta_fmt(struct file *file,
> +				     void *fh, struct v4l2_format *f);
> +
> +static int mtk_dip_subdev_open(struct v4l2_subdev *sd,
> +			       struct v4l2_subdev_fh *fh)
> +{
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	isp_dev->ctx.fh = fh;
> +
> +	return mtk_dip_ctx_open(&isp_dev->ctx);
> +}
> +
> +static int mtk_dip_subdev_close(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_fh *fh)
> +{
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	return mtk_dip_ctx_release(&isp_dev->ctx);
> +}
> +
> +static int mtk_dip_subdev_s_stream(struct v4l2_subdev *sd,
> +				   int enable)
> +{
> +	int ret = 0;
> +
> +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> +
> +	if (enable) {
> +		ret = mtk_dip_ctx_streamon(&isp_dev->ctx);
> +
> +		if (!ret)
> +			ret = mtk_dip_dev_queue_buffers
> +				(mtk_dip_ctx_to_dev(&isp_dev->ctx), 1);
> +		if (ret)
> +			pr_err("failed to queue initial buffers (%d)", ret);
> +	}	else {
> +		ret = mtk_dip_ctx_streamoff(&isp_dev->ctx);
> +	}
> +
> +	if (!ret)
> +		isp_dev->mem2mem2.streaming = enable;
> +
> +	return ret;
> +}
> +
> +int mtk_dip_subdev_subscribe_event(struct v4l2_subdev *subdev,
> +				   struct v4l2_fh *fh,
> +				   struct v4l2_event_subscription *sub)

Should be static.

> +{
> +	pr_info("sub type(%x)", sub->type);

I feel like you have this problem in other places too: this definitely
shouldn't be at KERN_INFO level. It seems a bit excessive anyway.

> +	if (sub->type != V4L2_EVENT_PRIVATE_START &&
> +	    sub->type != V4L2_EVENT_MTK_DIP_FRAME_DONE)
> +		return -EINVAL;
> +
> +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> +}
> +
> +int mtk_dip_subdev_unsubscribe_event(struct v4l2_subdev *subdev,
> +				     struct v4l2_fh *fh,

Static.

> +	struct v4l2_event_subscription *sub)
> +{
> +	return v4l2_event_unsubscribe(fh, sub);
> +}
> +
> +static int mtk_dip_link_setup(struct media_entity *entity,
> +			      const struct media_pad *local,
> +	const struct media_pad *remote, u32 flags)
> +{
> +	struct mtk_dip_mem2mem2_device *m2m2 =
> +			container_of(entity,
> +				     struct mtk_dip_mem2mem2_device,
> +				     subdev.entity);
> +	struct mtk_dip_dev *isp_dev =
> +		container_of(m2m2, struct mtk_dip_dev, mem2mem2);
> +
> +	u32 pad = local->index;
> +
> +	dev_dbg(&isp_dev->pdev->dev,
> +		"link setup: %d --> %d\n", pad, remote->index);
> +
> +	WARN_ON(entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV);
> +
> +	WARN_ON(pad >= m2m2->num_nodes);
> +
> +	m2m2->nodes[pad].enabled = !!(flags & MEDIA_LNK_FL_ENABLED);
> +
> +	/* queue_enable can be phase out in the future since */
> +	/* we don't have internal queue of each node in */
> +	/* v4l2 common module */
> +	isp_dev->queue_enabled[pad] = m2m2->nodes[pad].enabled;
> +
> +	return 0;
> +}
> +
> +static void mtk_dip_vb2_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mtk_dip_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue);
> +
> +	struct mtk_dip_dev *mtk_dip_dev = mtk_dip_m2m_to_dev(m2m2);
> +
> +	struct device *dev = &mtk_dip_dev->pdev->dev;
> +
> +	struct mtk_dip_dev_buffer *buf = NULL;
> +
> +	struct vb2_v4l2_buffer *v4l2_buf = NULL;
> +
> +	struct mtk_dip_dev_video_device *node =
> +		mtk_dip_vbq_to_isp_node(vb->vb2_queue);
> +
> +	int queue = mtk_dip_dev_get_queue_id_of_dev_node(mtk_dip_dev, node);

You've got a lot of extra blank lines in here.

> +
> +	dev_dbg(dev,
> +		"queue vb2_buf: Node(%s) queue id(%d)\n",
> +		node->name,
> +		queue);
> +
> +	if (queue < 0) {
> +		dev_err(m2m2->dev, "Invalid mtk_dip_dev node.\n");
> +		return;
> +	}
> +
> +	if (mtk_dip_dev->ctx.mode == MTK_DIP_CTX_MODE_DEBUG_BYPASS_ALL) {
> +		dev_dbg(m2m2->dev, "By pass mode, just loop back the buffer\n");
> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		return;
> +	}
> +
> +	if (!vb)
> +		pr_err("VB can't be null\n");
> +
> +	buf = mtk_dip_vb2_buf_to_dev_buf(vb);
> +
> +	if (!buf)
> +		pr_err("buf can't be null\n");
> +
> +	v4l2_buf = to_vb2_v4l2_buffer(vb);
> +
> +	if (!v4l2_buf)
> +		pr_err("v4l2_buf can't be null\n");
> +
> +	mutex_lock(&mtk_dip_dev->lock);
> +
> +	/* the dma address will be filled in later frame buffer handling*/
> +	mtk_dip_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0);
> +
> +	/* Added the buffer into the tracking list */
> +	list_add_tail(&buf->m2m2_buf.list,
> +		      &m2m2->nodes[node - m2m2->nodes].buffers);
> +	mutex_unlock(&mtk_dip_dev->lock);
> +
> +	/* Enqueue the buffer */
> +	if (mtk_dip_dev->mem2mem2.streaming) {
> +		dev_dbg(dev, "%s: mtk_dip_dev_queue_buffers\n",
> +			node->name);
> +		mtk_dip_dev_queue_buffers(mtk_dip_dev, 0);
> +	}
> +}
...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> new file mode 100644
> index 0000000..ffdc45e
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Frederic Chen <frederic.chen@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include "mtk_dip-ctx.h"
> +#include "mtk_dip.h"
> +#include "mtk_dip-v4l2.h"
> +#include "mtk-mdp3-regs.h"
> +
> +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2"
> +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \
> +	MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME
> +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2"
> +
> +#ifdef MTK_DIP_CTX_DIP_V4L2_UT
> +#include "mtk_dip-dev-ctx-dip-test.h"

The above macros was never defined, and this header doesn't exist.
Please remove.

> +#endif
> +

...

> +int mtk_dip_ctx_dip_v4l2_init(struct platform_device *pdev,
> +			      struct mtk_dip_dev *isp_preview_dev,
> +	struct mtk_dip_dev *isp_capture_dev)
> +{
> +	struct media_device *media_dev;
> +	struct v4l2_device *v4l2_dev;
> +	struct v4l2_ctrl_handler *ctrl_handler;
> +	int ret = 0;
> +
> +	/* initialize the v4l2 common part */
> +	dev_info(&pdev->dev, "init v4l2 common part: dev=%llx\n",
> +		 (unsigned long long)&pdev->dev);
> +
> +	media_dev = &isp_preview_dev->media_dev;
> +	v4l2_dev = &isp_preview_dev->v4l2_dev;
> +	ctrl_handler = &isp_preview_dev->ctx.ctrl_handler;
> +
> +	ret = mtk_dip_media_register(&pdev->dev,
> +				     media_dev,
> +		MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME);
> +
> +	ret = mtk_dip_v4l2_register(&pdev->dev,
> +				    media_dev,
> +		v4l2_dev,
> +		ctrl_handler);
> +
> +	dev_info(&pdev->dev, "init v4l2 preview part\n");
> +	ret = mtk_dip_dev_core_init_ext(pdev,
> +					isp_preview_dev,
> +					&mtk_dip_ctx_desc_dip_preview,
> +		media_dev, v4l2_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "Preview v4l2 part init failed: %d\n", ret);
> +
> +	dev_info(&pdev->dev, "init v4l2 capture part\n");
> +
> +	ret = mtk_dip_dev_core_init_ext(pdev,
> +					isp_capture_dev,
> +					&mtk_dip_ctx_desc_dip_capture,
> +		media_dev, v4l2_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "Capture v4l2 part init failed: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +/* MTK ISP context initialization */
> +int mtk_dip_ctx_dip_preview_init(struct mtk_dip_ctx *preview_ctx)
> +{
> +	preview_ctx->ctx_id = MTK_DIP_CTX_P2_ID_PREVIEW;
> +
> +	/* Initialize main data structure */
> +	mtk_dip_ctx_core_queue_setup(preview_ctx, &preview_queues_setting);
> +
> +	return mtk_dip_ctx_core_steup(preview_ctx,
> +		&mtk_dip_ctx_dip_preview_setting);
> +}
> +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_preview_init);
> +
> +/* MTK ISP context initialization */
> +int mtk_dip_ctx_dip_capture_init(struct mtk_dip_ctx *capture_ctx)
> +{
> +	capture_ctx->ctx_id =  MTK_DIP_CTX_P2_ID_CAPTURE;
> +	/* Initialize main data structure */
> +	mtk_dip_ctx_core_queue_setup(capture_ctx, &capture_queues_setting);
> +
> +	return mtk_dip_ctx_core_steup(capture_ctx,
> +		&mtk_dip_ctx_dip_capture_setting);
> +}
> +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_capture_init);

...

> diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> new file mode 100644
> index 0000000..3569c7c
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> @@ -0,0 +1,1385 @@
> +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Holmes Chiou <holmes.chiou@mediatek.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/kthread.h> /* thread functions */
> +#include <linux/pm_runtime.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h> /* kzalloc and kfree */
> +
> +#include "mtk_vpu.h"
> +#include "mtk-mdp3-cmdq.h"
> +
> +#include "mtk_dip-dev.h"
> +#include "mtk_dip.h"
> +#include "mtk_dip-core.h"
> +#include "mtk_dip-v4l2.h"
> +
> +#define DIP_DEV_NAME		"camera-dip"
> +
> +#define DIP_COMPOSER_THREAD_TIMEOUT     (16U)
> +#define DIP_COMPOSING_WQ_TIMEOUT	(16U)
> +#define DIP_COMPOSING_MAX_NUM		(3)
> +#define DIP_FLUSHING_WQ_TIMEOUT		(16U)
> +
> +#define DIP_MAX_ERR_COUNT		(188U)
> +
> +#define DIP_FRM_SZ		(76 * 1024)
> +#define DIP_SUB_FRM_SZ		(16 * 1024)
> +#define DIP_TUNING_SZ		(32 * 1024)
> +#define DIP_COMP_SZ		(24 * 1024)
> +#define DIP_FRAMEPARAM_SZ	(4 * 1024)
> +
> +#define DIP_TUNING_OFFSET	(DIP_SUB_FRM_SZ)
> +#define DIP_COMP_OFFSET		(DIP_TUNING_OFFSET + DIP_TUNING_SZ)
> +#define DIP_FRAMEPARAM_OFFSET	(DIP_COMP_OFFSET + DIP_COMP_SZ)
> +
> +#define DIP_SUB_FRM_DATA_NUM	(32)
> +
> +#define DIP_SCP_WORKINGBUF_OFFSET	(5 * 1024 * 1024)
> +
> +#define DIP_GET_ID(x)			(((x) & 0xffff0000) >> 16)
> +
> +static const struct of_device_id dip_of_ids[] = {
> +	/* Remider: Add this device node manually in .dtsi */
> +	{ .compatible = "mediatek,mt8183-dip", },
> +	{}
> +};

Please add:

MODULE_DEVICE_TABLE(of, dip_of_ids);

> +
> +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> +				    struct img_ipi_frameparam *iparam);
> +
> +static struct img_frameparam *dip_create_framejob(int sequence)
> +{
> +	struct dip_frame_job *fjob = NULL;
> +
> +	fjob = kzalloc(sizeof(*fjob), GFP_ATOMIC);
> +
> +	if (!fjob)
> +		return NULL;
> +
> +	fjob->sequence = sequence;
> +
> +	return &fjob->fparam;
> +}
> +
> +static void dip_free_framejob(struct img_frameparam *fparam)
> +{
> +	struct dip_frame_job *fjob = NULL;
> +
> +	fjob = mtk_dip_fparam_to_job(fparam);
> +
> +	/* to avoid use after free issue */
> +	fjob->sequence = -1;
> +
> +	kfree(fjob);
> +}
> +
> +static void dip_enable_ccf_clock(struct dip_device *dip_dev)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(dip_dev->larb_dev);
> +	if (ret < 0)
> +		dev_err(&dip_dev->pdev->dev, "cannot get smi larb clock\n");
> +
> +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	if (ret)
> +		dev_err(&dip_dev->pdev->dev,
> +			"cannot prepare and enable DIP_IMG_LARB5 clock\n");
> +
> +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_DIP);
> +	if (ret)
> +		dev_err(&dip_dev->pdev->dev,
> +			"cannot prepare and enable DIP_IMG_DIP clock\n");
> +}
> +
> +static void dip_disable_ccf_clock(struct dip_device *dip_dev)
> +{
> +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_DIP);
> +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	pm_runtime_put_sync(dip_dev->larb_dev);
> +}
> +
> +static int dip_send(struct platform_device *pdev, enum ipi_id id,
> +		    void *buf, unsigned int  len, unsigned int wait)
> +{
> +	vpu_ipi_send_sync_async(pdev, id, buf, len, wait);
> +	return 0;
> +}
> +
> +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> +				    struct img_ipi_frameparam *iparam)
> +{
> +	struct mtk_dip_ctx_finish_param fparam;
> +	struct mtk_isp_dip_drv_data *drv_data;
> +	struct mtk_dip_ctx *dev_ctx;
> +	int ctx_id = 0;
> +	int r = 0;
> +
> +	if (!dip_dev) {
> +		pr_err("Can't update buffer status, dip_dev can't be NULL\n");
> +		return;
> +	}
> +
> +	if (!iparam) {
> +		dev_err(&dip_dev->pdev->dev,
> +			"%s: iparam can't be NULL\n", __func__);
> +		return;
> +	}
> +
> +	drv_data = dip_dev_to_drv(dip_dev);
> +
> +	frame_param_ipi_to_ctx(iparam, &fparam);
> +	ctx_id = MTK_DIP_GET_CTX_ID_FROM_SEQUENCE(fparam.frame_id);
> +
> +	if (ctx_id == MTK_DIP_CTX_P2_ID_PREVIEW) {
> +		dev_ctx = &drv_data->isp_preview_dev.ctx;
> +	} else if (ctx_id == MTK_DIP_CTX_P2_ID_CAPTURE) {
> +		dev_ctx = &drv_data->isp_capture_dev.ctx;
> +	} else {
> +		dev_err(&dip_dev->pdev->dev,
> +			"unknown ctx id: %d\n", ctx_id);
> +		return;
> +	}
> +
> +	r = mtk_dip_ctx_core_job_finish(dev_ctx, &fparam);
> +
> +	if (r)
> +		dev_err(&dip_dev->pdev->dev, "finish op failed: %d\n",
> +			r);
> +	dev_dbg(&dip_dev->pdev->dev, "Ready to return buffers: CTX(%d), Frame(%d)\n",
> +		ctx_id, fparam.frame_id);
> +}
> +
> +static void mtk_dip_notify(void *data)
> +{
> +	struct dip_device	*dip_dev;
> +	struct mtk_dip_hw_ctx	*dip_ctx;
> +	struct img_frameparam	*framejob;
> +	struct dip_user_id	*user_id;
> +	struct dip_subframe	*buf, *tmpbuf;
> +	struct img_ipi_frameparam	*frameparam;
> +	u32 num;
> +	bool found = false;
> +
> +	frameparam = (struct img_ipi_frameparam *)data;
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +	framejob = container_of(frameparam,
> +				struct img_frameparam,
> +				frameparam);
> +
> +	if (frameparam->state == FRAME_STATE_HW_TIMEOUT) {
> +		dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> +			 (void *)frameparam, sizeof(*frameparam), 0);
> +		dev_err(&dip_dev->pdev->dev, "frame no(%d) HW timeout\n",
> +			frameparam->frame_no);
> +	}
> +
> +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> +	list_for_each_entry_safe(buf, tmpbuf,
> +				 &dip_ctx->dip_usedbufferlist.queue,
> +				 list_entry) {
> +		if (buf->buffer.pa == frameparam->subfrm_data.pa) {
> +			list_del(&buf->list_entry);
> +			dip_ctx->dip_usedbufferlist.queue_cnt--;
> +			found = true;
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"Find used buffer (%x)\n", buf->buffer.pa);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> +
> +	if (!found) {
> +		dev_err(&dip_dev->pdev->dev,
> +			"frame_no(%d) buffer(%x) used buffer count(%d)\n",
> +			frameparam->frame_no, frameparam->subfrm_data.pa,
> +			dip_ctx->dip_usedbufferlist.queue_cnt);
> +
> +		frameparam->state = FRAME_STATE_ERROR;
> +
> +	} else {
> +		mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> +		list_add_tail(&buf->list_entry,
> +			      &dip_ctx->dip_freebufferlist.queue);
> +		dip_ctx->dip_freebufferlist.queue_cnt++;
> +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +		frameparam->state = FRAME_STATE_DONE;
> +	}
> +
> +	call_mtk_dip_ctx_finish(dip_dev, frameparam);
> +
> +	found = false;
> +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +	list_for_each_entry(user_id,
> +			    &dip_ctx->dip_useridlist.queue,
> +			    list_entry) {
> +		if (DIP_GET_ID(frameparam->index) == user_id->id) {
> +			user_id->num--;
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"user_id(%x) is found, and cnt: %d\n",
> +				user_id->id, user_id->num);
> +			found = true;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +	wake_up(&dip_ctx->flushing_wq);
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) is finished\n", framejob->frameparam.frame_no);
> +	dip_free_framejob(framejob);
> +
> +	num = atomic_dec_return(&dip_ctx->num_running);
> +	dev_dbg(&dip_dev->pdev->dev, "Running count: %d\n", num);
> +}
> +
> +static void mdp_cb_worker(struct work_struct *work)
> +{
> +	struct mtk_mdpcb_work *mdpcb_work;
> +
> +	mdpcb_work = container_of(work, struct mtk_mdpcb_work, frame_work);
> +	mtk_dip_notify(mdpcb_work->frameparams);
> +	kfree(mdpcb_work);
> +}
> +
> +static struct img_ipi_frameparam *convert_to_fparam(struct cmdq_cb_data *data)
> +{
> +	struct device *dev = NULL;

Every use of dev_err() in this function is wrong, since you're
guaranteed to be NULL. Either come up with some better way to report
device errors using the pointers you have, or else just switch to
pr_err().

> +	struct dip_device *dip_dev = NULL;
> +	struct dip_frame_job *fjob = NULL;
> +	struct img_ipi_frameparam *ipi_fparam = NULL;
> +
> +	if (!data) {
> +		dev_err(dev, "DIP got NULL in cmdq_cb_data,%s\n",
> +			__func__);
> +		return NULL;
> +	}
> +
> +	if (data->sta != CMDQ_CB_NORMAL) {
> +		dev_warn(dev, "DIP got CMDQ CB (%d) without CMDQ_CB_NORMAL\n",
> +			 data->sta);
> +	}
> +
> +	if (!data->data) {
> +		dev_err(dev, "DIP got NULL data in cmdq_cb_data,%s\n",
> +			__func__);
> +		return NULL;
> +	}
> +
> +	fjob = mtk_dip_ipi_fparam_to_job(data->data);
> +
> +	if (fjob->sequence == -1) {
> +		dev_err(dev, "Invalid cmdq_cb_data(%llx)\n",
> +			(unsigned long long)data);
> +		ipi_fparam = NULL;
> +	} else {
> +		ipi_fparam = &fjob->fparam.frameparam;
> +		dip_dev = dip_hw_ctx_to_dev((void *)ipi_fparam->drv_data);
> +		dev = &dip_dev->pdev->dev;
> +	}
> +
> +	dev_dbg(dev, "framejob(0x%llx,seq:%d):\n",
> +		(unsigned long long)fjob, fjob->sequence);
> +	dev_dbg(dev, "idx(%d),no(%d),s(%d),n_in(%d),n_out(%d),drv(%llx)\n",
> +		fjob->fparam.frameparam.index,
> +		fjob->fparam.frameparam.frame_no,
> +		fjob->fparam.frameparam.state,
> +		fjob->fparam.frameparam.num_inputs,
> +		fjob->fparam.frameparam.num_outputs,
> +		(unsigned long long)fjob->fparam.frameparam.drv_data
> +	);
> +
> +	return ipi_fparam;
> +}
> +
> +/* Maybe in IRQ context of cmdq */
> +void dip_mdp_cb_func(struct cmdq_cb_data data)

Make this static.

> +{
> +	struct img_ipi_frameparam *frameparam;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +	struct mtk_mdpcb_work *mdpcb_work;
> +
> +	frameparam = convert_to_fparam(&data);
> +
> +	if (!frameparam) {
> +		dev_err(NULL, "%s return due to invalid cmdq_cb_data(%llx)",

Don't directly pass NULL to dev_err(). Just use pr_err() or similar.

> +			__func__, &data);
> +		return;
> +	}
> +
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +
> +	mdpcb_work = kzalloc(sizeof(*mdpcb_work), GFP_ATOMIC);
> +	WARN_ONCE(!mdpcb_work, "frame_no(%d) is lost", frameparam->frame_no);
> +	if (!mdpcb_work)
> +		return;
> +
> +	INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker);
> +	mdpcb_work->frameparams = frameparam;
> +	if (data.sta != CMDQ_CB_NORMAL)
> +		mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT;
> +
> +	queue_work(dip_ctx->mdpcb_workqueue, &mdpcb_work->frame_work);
> +}
> +
> +static void dip_vpu_handler(void *data, unsigned int len, void *priv)
> +{
> +	struct img_frameparam *framejob;
> +	struct img_ipi_frameparam *frameparam;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +	struct dip_device *dip_dev;
> +	unsigned long flags;
> +	u32 num;
> +
> +	WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__);
> +	if (!data)

You can combine these lines:

	
	if (WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__))
		return;

> +		return;
> +
> +	frameparam = (struct img_ipi_frameparam *)data;
> +
> +	framejob = dip_create_framejob(frameparam->index);
> +	WARN_ONCE(!framejob, "frame_no(%d) is lost", frameparam->frame_no);
> +	if (!framejob)

Same here.

> +		return;
> +
> +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +
> +	wake_up(&dip_ctx->composing_wq);
> +	memcpy(&framejob->frameparam, data, sizeof(framejob->frameparam));
> +	num = atomic_dec_return(&dip_ctx->num_composing);
> +
> +	spin_lock_irqsave(&dip_ctx->dip_gcejoblist.queuelock, flags);
> +	list_add_tail(&framejob->list_entry, &dip_ctx->dip_gcejoblist.queue);
> +	dip_ctx->dip_gcejoblist.queue_cnt++;
> +	spin_unlock_irqrestore(&dip_ctx->dip_gcejoblist.queuelock, flags);
> +
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) is back, composing num: %d\n",
> +		frameparam->frame_no, num);
> +
> +	wake_up(&dip_ctx->dip_runner_thread.wq);
> +}
> +

...
> +static int dip_runner_func(void *data)
> +{

...

> +
> +			mdp_cmdq_sendtask

I don't see this defined anywhere?

> +				(dip_ctx->mdp_pdev,
> +				 (struct img_config *)
> +					framejob->frameparam.config_data.va,
> +				 &framejob->frameparam, NULL, false,
> +				 dip_mdp_cb_func,
> +				 (void *)&framejob->frameparam);
> +
...
> +	return 0;
> +}
> +
> +static void dip_submit_worker(struct work_struct *work)
> +{
> +	struct mtk_dip_submit_work *dip_submit_work =
> +		container_of(work, struct mtk_dip_submit_work, frame_work);
> +
> +	struct mtk_dip_hw_ctx  *dip_ctx = dip_submit_work->dip_ctx;
> +	struct mtk_dip_work *dip_work;
> +	struct dip_device *dip_dev;
> +	struct dip_subframe *buf;
> +	u32 len, num;
> +	int ret;
> +
> +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> +	num  = atomic_read(&dip_ctx->num_composing);
> +
> +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> +	dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
> +				    struct mtk_dip_work, list_entry);
> +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);

I see you grab the head of the list here, but then you release the lock.
Then you later assume that reference is still valid, throughout this
function.

That's usually true, because you only remove/delete entries from this
list within this same workqueue (at the end of this function). But it's
not true in dip_release_context() (which doesn't even grab the lock,
BTW).

I think there could be several ways to solve this, but judging by how
this list entry is used...couldn't you just remove it from the list
here, while holding the lock? Then you only have to kfree() it when
you're done under the free_work_list label.

> +
> +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +	if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
> +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> +
> +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +		dip_work->user_id->num--;
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n",
> +			dip_work->user_id->id, dip_work->user_id->num,
> +			dip_work->frameparams.frame_no,
> +			dip_work->frameparams.index);
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		goto free_work_list;
> +	}
> +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +	while (num >= DIP_COMPOSING_MAX_NUM) {
> +		ret = wait_event_interruptible_timeout
> +			(dip_ctx->composing_wq,
> +			 (num < DIP_COMPOSING_MAX_NUM),
> +			 msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT));
> +
> +		if (ret == -ERESTARTSYS)
> +			dev_err(&dip_dev->pdev->dev,
> +				"interrupted by a signal!\n");
> +		else if (ret == 0)
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"timeout frame_no(%d), num: %d\n",
> +				dip_work->frameparams.frame_no, num);
> +		else
> +			dev_dbg(&dip_dev->pdev->dev,
> +				"wakeup frame_no(%d), num: %d\n",
> +				dip_work->frameparams.frame_no, num);
> +
> +		num = atomic_read(&dip_ctx->num_composing);
> +	};
> +
> +	mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> +	if (list_empty(&dip_ctx->dip_freebufferlist.queue)) {
> +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +		dev_err(&dip_dev->pdev->dev,
> +			"frame_no(%d) index: %x no free buffer: %d\n",
> +			dip_work->frameparams.frame_no,
> +			dip_work->frameparams.index,
> +			dip_ctx->dip_freebufferlist.queue_cnt);
> +
> +		/* Call callback to notify V4L2 common framework
> +		 * for failure of enqueue
> +		 */
> +		dip_work->frameparams.state = FRAME_STATE_ERROR;
> +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> +
> +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> +		dip_work->user_id->num--;
> +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> +
> +		goto free_work_list;
> +	}
> +
> +	buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue,
> +			       struct dip_subframe,
> +			       list_entry);
> +	list_del(&buf->list_entry);
> +	dip_ctx->dip_freebufferlist.queue_cnt--;
> +	mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> +	list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue);
> +	dip_ctx->dip_usedbufferlist.queue_cnt++;
> +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> +
> +	memcpy(&dip_work->frameparams.subfrm_data,
> +	       &buf->buffer, sizeof(buf->buffer));
> +
> +	memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ);
> +
> +	memcpy(&dip_work->frameparams.config_data,
> +	       &buf->config_data, sizeof(buf->config_data));
> +
> +	memset((char *)buf->config_data.va, 0, DIP_COMP_SZ);
> +
> +	if (dip_work->frameparams.tuning_data.pa == 0) {
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"frame_no(%d) has no tuning_data\n",
> +			dip_work->frameparams.frame_no);
> +
> +		memcpy(&dip_work->frameparams.tuning_data,
> +		       &buf->tuning_buf, sizeof(buf->tuning_buf));
> +
> +		memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> +		/* When user enqueued without tuning buffer,
> +		 * it would use driver internal buffer.
> +		 * So, tuning_data.va should be 0
> +		 */
> +		dip_work->frameparams.tuning_data.va = 0;
> +	}
> +
> +	dip_work->frameparams.drv_data = (u64)dip_ctx;
> +	dip_work->frameparams.state = FRAME_STATE_COMPOSING;
> +
> +	memcpy((void *)buf->frameparam.va, &dip_work->frameparams,
> +	       sizeof(dip_work->frameparams));
> +
> +	dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> +		 (void *)&dip_work->frameparams,
> +		 sizeof(dip_work->frameparams), 0);
> +	num = atomic_inc_return(&dip_ctx->num_composing);
> +
> +free_work_list:
> +
> +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> +	list_del(&dip_work->list_entry);
> +	dip_ctx->dip_worklist.queue_cnt--;
> +	len = dip_ctx->dip_worklist.queue_cnt;
> +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> +
> +	dev_dbg(&dip_dev->pdev->dev,
> +		"frame_no(%d) index: %x, worklist count: %d, composing num: %d\n",
> +		dip_work->frameparams.frame_no, dip_work->frameparams.index,
> +		len, num);
> +
> +	kfree(dip_work);
> +}

...

> +int dip_open_context(struct dip_device *dip_dev)

Should be static.

> +{

...

> +}
> +
> +int dip_release_context(struct dip_device *dip_dev)

Should be static.

> +{
> +	u32 i = 0;
> +	struct dip_subframe *buf, *tmpbuf;
> +	struct mtk_dip_work *dip_work, *tmp_work;
> +	struct dip_user_id  *dip_userid, *tmp_id;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +
> +	dip_ctx = &dip_dev->dip_ctx;
> +	dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n",
> +		dip_ctx->dip_worklist.queue_cnt);
> +
> +	list_for_each_entry_safe(dip_work, tmp_work,
> +				 &dip_ctx->dip_worklist.queue,
> +				 list_entry) {

Shouldn't you be holding the mutex for this? Or alternatively, cancel
any outstanding work and move the flush_workqueue()/destroy_workqueue()
up.

Similar questions for the other lists we're going through here.

> +		list_del(&dip_work->list_entry);
> +		dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n",
> +			dip_work->frameparams.frame_no);
> +		kfree(dip_work);
> +		dip_ctx->dip_worklist.queue_cnt--;
> +	}
> +
> +	if (dip_ctx->dip_worklist.queue_cnt != 0)
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"dip_worklist is not empty (%d)\n",
> +			dip_ctx->dip_worklist.queue_cnt);
> +
> +	list_for_each_entry_safe(dip_userid, tmp_id,
> +				 &dip_ctx->dip_useridlist.queue,
> +				 list_entry) {
> +		list_del(&dip_userid->list_entry);
> +		dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n",
> +			dip_userid->id);
> +		kfree(dip_userid);
> +		dip_ctx->dip_useridlist.queue_cnt--;
> +	}
> +
> +	if (dip_ctx->dip_useridlist.queue_cnt != 0)
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"dip_useridlist is not empty (%d)\n",
> +			dip_ctx->dip_useridlist.queue_cnt);
> +
> +	flush_workqueue(dip_ctx->mdpcb_workqueue);
> +	destroy_workqueue(dip_ctx->mdpcb_workqueue);
> +	dip_ctx->mdpcb_workqueue = NULL;
> +
> +	flush_workqueue(dip_ctx->composer_wq);
> +	destroy_workqueue(dip_ctx->composer_wq);
> +	dip_ctx->composer_wq = NULL;
> +
> +	atomic_set(&dip_ctx->num_composing, 0);
> +	atomic_set(&dip_ctx->num_running, 0);
> +
> +	kthread_stop(dip_ctx->dip_runner_thread.thread);
> +	dip_ctx->dip_runner_thread.thread = NULL;
> +
> +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> +
> +	/* All the buffer should be in the freebufferlist when release */
> +	list_for_each_entry_safe(buf, tmpbuf,
> +				 &dip_ctx->dip_freebufferlist.queue,
> +				 list_entry) {
> +		struct sg_table *sgt = &buf->table;
> +
> +		dev_dbg(&dip_dev->pdev->dev,
> +			"buf pa (%d): %x\n", i, buf->buffer.pa);
> +		dip_ctx->dip_freebufferlist.queue_cnt--;
> +		dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl,
> +				   sgt->orig_nents,
> +				   DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
> +		sg_free_table(sgt);
> +		list_del(&buf->list_entry);
> +		kfree(buf);
> +		buf = NULL;
> +		i++;
> +	}
> +
> +	if (dip_ctx->dip_freebufferlist.queue_cnt != 0 &&
> +	    i != DIP_SUB_FRM_DATA_NUM)
> +		dev_err(&dip_dev->pdev->dev,
> +			"dip_freebufferlist is not empty (%d/%d)\n",
> +			dip_ctx->dip_freebufferlist.queue_cnt, i);
> +
> +	mutex_destroy(&dip_ctx->dip_useridlist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_worklist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock);
> +	mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock);
> +
> +	return 0;
> +}
> +

...

> +static int mtk_dip_probe(struct platform_device *pdev)
> +{
> +	struct mtk_isp_dip_drv_data *dip_drv;
> +	struct dip_device *dip_dev;
> +	struct mtk_dip_hw_ctx *dip_ctx;
> +	struct device_node *node;
> +	struct platform_device *larb_pdev;
> +
> +	int ret = 0;
> +
> +	dev_info(&pdev->dev, "E. DIP driver probe.\n");
> +
> +	dip_drv = devm_kzalloc(&pdev->dev, sizeof(*dip_drv), GFP_KERNEL);

Need to check for NULL.

> +	dev_set_drvdata(&pdev->dev, dip_drv);
> +	dip_dev = &dip_drv->dip_dev;
> +
> +	if (!dip_dev)
> +		return -ENOMEM;
> +
> +	dev_info(&pdev->dev, "Created dip_dev = 0x%p\n", dip_dev);
> +
> +	dip_dev->pdev = pdev;
> +	dip_ctx = &dip_dev->dip_ctx;
> +
> +	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> +	if (!node) {
> +		dev_err(&pdev->dev, "no mediatek,larb found");
> +		return -EINVAL;
> +	}
> +	larb_pdev = of_find_device_by_node(node);
> +	if (!larb_pdev) {
> +		dev_err(&pdev->dev, "no mediatek,larb device found");
> +		return -EINVAL;
> +	}
> +	dip_dev->larb_dev = &larb_pdev->dev;
> +
> +	/*CCF: Grab clock pointer (struct clk*) */

Add a space before 'CCF'.

> +	dip_dev->dip_clk.DIP_IMG_LARB5 = devm_clk_get(&pdev->dev,
> +						      "DIP_CG_IMG_LARB5");
> +	dip_dev->dip_clk.DIP_IMG_DIP = devm_clk_get(&pdev->dev,
> +						    "DIP_CG_IMG_DIP");
> +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_LARB5)) {
> +		dev_err(&pdev->dev, "cannot get DIP_IMG_LARB5 clock\n");
> +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_LARB5);
> +	}
> +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_DIP)) {
> +		dev_err(&pdev->dev, "cannot get DIP_IMG_DIP clock\n");
> +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_DIP);
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> +
> +	atomic_set(&dip_ctx->num_composing, 0);
> +	atomic_set(&dip_ctx->num_running, 0);
> +
> +	dip_ctx->dip_worklist.queue_cnt = 0;
> +
> +	ret = mtk_dip_ctx_dip_v4l2_init(pdev,
> +					&dip_drv->isp_preview_dev,
> +		&dip_drv->isp_capture_dev);
> +
> +	if (ret)
> +		dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret);
> +
> +	dev_info(&pdev->dev, "X. DIP driver probe.\n");
> +
> +	return ret;
> +}
> +
> +static int mtk_dip_remove(struct platform_device *pdev)
> +{
> +	struct mtk_isp_dip_drv_data *drv_data =
> +		dev_get_drvdata(&pdev->dev);
> +
> +	/*  */

What's with the empty comments? Here and below.

> +	if (drv_data) {
> +		mtk_dip_dev_core_release(pdev, &drv_data->isp_preview_dev);
> +		mtk_dip_dev_core_release(pdev, &drv_data->isp_capture_dev);
> +		dev_info(&pdev->dev, "E. %s\n", __func__);

Remove this line.

> +	}
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	/*  */
> +	return 0;
> +}
> +

...

> +static struct platform_driver mtk_dip_driver = {
> +	.probe   = mtk_dip_probe,
> +	.remove  = mtk_dip_remove,
> +	.driver  = {
> +		.name  = DIP_DEV_NAME,
> +		.owner = THIS_MODULE,

You don't need the .owner line. module_platform_driver() /
platform_driver_register() takes care of this for you.

Brian

> +		.of_match_table = dip_of_ids,
> +		.pm     = &mtk_dip_pm_ops,
> +	}
> +};
> +
> +module_platform_driver(mtk_dip_driver);
> +
> +MODULE_DESCRIPTION("Camera DIP driver");
> +MODULE_LICENSE("GPL");
...

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

Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Sakari Ailus]

Hi Frederic,

Could you cc the devicetree list, please?

On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> This patch adds the binding for describing the shared memory
> used to exchange configuration and tuning data between the
> co-processor and Digital Image Processing (DIP) unit of the
> camera ISP system on Mediatek SoCs.
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> 
> diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> new file mode 100644
> index 0000000..0ded478
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> @@ -0,0 +1,45 @@
> +Mediatek DIP Shared Memory binding
> +
> +This binding describes the shared memory, which serves the purpose of
> +describing the shared memory region used to exchange data between Digital
> +Image Processing (DIP) and co-processor in Mediatek SoCs.
> +
> +The co-processor doesn't have the iommu so we need to use the physical
> +address to access the shared buffer in the firmware.
> +
> +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> +it can use dma address to access the memory region.
> +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)

What kind of purpose is the memory used for? Buffers containing video data,
or something else? Could the buffer objects be mapped on the devices
based on the need instead?

> +
> +
> +Required properties:
> +
> +- compatible: must be "mediatek,reserve-memory-dip_smem"
> +
> +- reg: required for static allocation (see reserved-memory.txt for
> +  the detailed usage)
> +
> +- alloc-range: required for dynamic allocation. The range must
> +  between 0x00000400 and 0x100000000 due to the co-processer's
> +  addressing limitation
> +
> +- size: required for dynamic allocation. The unit is bytes.
> +  If you want to enable the full feature of Digital Processing Unit,
> +  you need 20 MB at least.
> +
> +
> +Example:
> +
> +The following example shows the DIP shared memory setup for MT8183.
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		reserve-memory-isp_smem {
> +			compatible = "mediatek,reserve-memory-dip_smem";
> +			size = <0 0x1400000>;
> +			alignment = <0 0x1000>;
> +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> +		};
> +	};

-- 
Kind regards,

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 V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings

[Sakari Ailus]

Hi Frederic,

Thanks for the patchset.

Could you also cc the devicetree list, please?

On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote:
> This patch adds the DT binding documentation for the shared memory
> between DIP (Digital Image Processing) unit of the camera ISP system
> and the co-processor in Mediatek SoCs.
> 
> Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> ---
>  .../bindings/media/mediatek,dip_smem.txt           | 29 ++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> new file mode 100644
> index 0000000..5533721
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> @@ -0,0 +1,29 @@
> +Mediatek ISP Shared Memory Device
> +
> +Mediatek ISP Shared Memory Device is used to manage shared memory
> +among CPU, ISP IPs and coprocessor. It is associated with a reserved
> +memory region (Please see Documentation\devicetree\bindings\
> +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and

s/\\/\//g;

> +and provide the context to allocate memory with dma addresses.
> +
> +Required properties:
> +- compatible: Should be "mediatek,isp_smem"

s/Should/Shall/

> +
> +- iommus: should point to the respective IOMMU block with master port

s/should/shall/

> +  as argument. Please set the ports which may be accessed
> +  through the common path. You can see
> +  Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> +  for the detail.
> +
> +- mediatek,larb: must contain the local arbiters in the current Socs.

Perhaps "SoCs"?

> +  Please set the larb of camsys for Pass 1 and imgsys for DIP, or both
> +  if you are using all the camera function. You can see
> +  Documentation/devicetree/bindings/memory-controllers/
> +  mediatek,smi-larb.txt for the detail.
> +
> +Example:
> +	isp_smem: isp_smem {
> +		compatible = "mediatek,isp_smem";
> +		mediatek,larb = <&larb5>;
> +		iommus = <&iommu M4U_PORT_CAM_IMGI>;
> +	};

-- 
Kind regards,

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 V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Laurent Pinchart]

On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote:
> Hi Frederic,
> 
> Could you cc the devicetree list, please?
> 
> On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> > This patch adds the binding for describing the shared memory
> > used to exchange configuration and tuning data between the
> > co-processor and Digital Image Processing (DIP) unit of the
> > camera ISP system on Mediatek SoCs.
> > 
> > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > ---
> >  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > new file mode 100644
> > index 0000000..0ded478
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > @@ -0,0 +1,45 @@
> > +Mediatek DIP Shared Memory binding
> > +
> > +This binding describes the shared memory, which serves the purpose of
> > +describing the shared memory region used to exchange data between Digital
> > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > +
> > +The co-processor doesn't have the iommu so we need to use the physical
> > +address to access the shared buffer in the firmware.
> > +
> > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > +it can use dma address to access the memory region.
> > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> 
> What kind of purpose is the memory used for? Buffers containing video data,
> or something else? Could the buffer objects be mapped on the devices
> based on the need instead?

And could CMA be used when physically contiguous memory is needed ?

> > +
> > +
> > +Required properties:
> > +
> > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > +
> > +- reg: required for static allocation (see reserved-memory.txt for
> > +  the detailed usage)
> > +
> > +- alloc-range: required for dynamic allocation. The range must
> > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > +  addressing limitation
> > +
> > +- size: required for dynamic allocation. The unit is bytes.
> > +  If you want to enable the full feature of Digital Processing Unit,
> > +  you need 20 MB at least.
> > +
> > +
> > +Example:
> > +
> > +The following example shows the DIP shared memory setup for MT8183.
> > +
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +		reserve-memory-isp_smem {
> > +			compatible = "mediatek,reserve-memory-dip_smem";
> > +			size = <0 0x1400000>;
> > +			alignment = <0 0x1000>;
> > +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> > +		};
> > +	};

-- 
Regards,

Laurent Pinchart

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

Re: [RFC PATCH V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings

[Laurent Pinchart]

Hello Frederic,

On Sat, Feb 09, 2019 at 05:59:35PM +0200, Sakari Ailus wrote:
> Hi Frederic,
> 
> Thanks for the patchset.
> 
> Could you also cc the devicetree list, please?
> 
> On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote:
> > This patch adds the DT binding documentation for the shared memory
> > between DIP (Digital Image Processing) unit of the camera ISP system
> > and the co-processor in Mediatek SoCs.
> > 
> > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > ---
> >  .../bindings/media/mediatek,dip_smem.txt           | 29 ++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > new file mode 100644
> > index 0000000..5533721
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > @@ -0,0 +1,29 @@
> > +Mediatek ISP Shared Memory Device
> > +
> > +Mediatek ISP Shared Memory Device is used to manage shared memory
> > +among CPU, ISP IPs and coprocessor. It is associated with a reserved
> > +memory region (Please see Documentation\devicetree\bindings\
> > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and
> 
> s/\\/\//g;
> 
> > +and provide the context to allocate memory with dma addresses.

Does this represent a real device (as in IP core) in the SoC ? There
seems to be no driver associated with the compatible string defined
herein in this patch series, what is this node used for ?

> > +Required properties:
> > +- compatible: Should be "mediatek,isp_smem"
> 
> s/Should/Shall/
> 
> > +
> > +- iommus: should point to the respective IOMMU block with master port
> 
> s/should/shall/
> 
> > +  as argument. Please set the ports which may be accessed
> > +  through the common path. You can see
> > +  Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> > +  for the detail.
> > +
> > +- mediatek,larb: must contain the local arbiters in the current Socs.
> 
> Perhaps "SoCs"?
> 
> > +  Please set the larb of camsys for Pass 1 and imgsys for DIP, or both
> > +  if you are using all the camera function. You can see
> > +  Documentation/devicetree/bindings/memory-controllers/
> > +  mediatek,smi-larb.txt for the detail.
> > +
> > +Example:
> > +	isp_smem: isp_smem {
> > +		compatible = "mediatek,isp_smem";
> > +		mediatek,larb = <&larb5>;
> > +		iommus = <&iommu M4U_PORT_CAM_IMGI>;
> > +	};

-- 
Regards,

Laurent Pinchart

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

Re: [RFC PATCH V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Frederic Chen]

Dear Laurent and Sakari,

I appreciate your messages.

On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote:
> On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote:
> > Hi Frederic,
> > 
> > Could you cc the devicetree list, please?
> > 
> > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> > > This patch adds the binding for describing the shared memory
> > > used to exchange configuration and tuning data between the
> > > co-processor and Digital Image Processing (DIP) unit of the
> > > camera ISP system on Mediatek SoCs.
> > > 
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > new file mode 100644
> > > index 0000000..0ded478
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > @@ -0,0 +1,45 @@
> > > +Mediatek DIP Shared Memory binding
> > > +
> > > +This binding describes the shared memory, which serves the purpose of
> > > +describing the shared memory region used to exchange data between Digital
> > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > +
> > > +The co-processor doesn't have the iommu so we need to use the physical
> > > +address to access the shared buffer in the firmware.
> > > +
> > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > +it can use dma address to access the memory region.
> > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > 
> > What kind of purpose is the memory used for? Buffers containing video data,
> > or something else? Could the buffer objects be mapped on the devices
> > based on the need instead?

The memory buffers contain camera 3A tuning data, which are used by the
co-processor and DIP IP. About mapping the buffer based on the need
instead, I’m not sure I understand this point. Do you mean that
allocating and mapping the memory dynamically?

> 
> And could CMA be used when physically contiguous memory is needed ?

DIP driver does not use CMA now, because the first version will be used
by CrOS but CrOS won’t enable CMA.

> 
> > > +
> > > +
> > > +Required properties:
> > > +
> > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > > +
> > > +- reg: required for static allocation (see reserved-memory.txt for
> > > +  the detailed usage)
> > > +
> > > +- alloc-range: required for dynamic allocation. The range must
> > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > +  addressing limitation
> > > +
> > > +- size: required for dynamic allocation. The unit is bytes.
> > > +  If you want to enable the full feature of Digital Processing Unit,
> > > +  you need 20 MB at least.
> > > +
> > > +
> > > +Example:
> > > +
> > > +The following example shows the DIP shared memory setup for MT8183.
> > > +
> > > +	reserved-memory {
> > > +		#address-cells = <2>;
> > > +		#size-cells = <2>;
> > > +		ranges;
> > > +		reserve-memory-isp_smem {
> > > +			compatible = "mediatek,reserve-memory-dip_smem";
> > > +			size = <0 0x1400000>;
> > > +			alignment = <0 0x1000>;
> > > +			alloc-ranges = <0 0x40000000 0 0x50000000>;
> > > +		};
> > > +	};
> 

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 V0 3/7] [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings

[Frederic Chen]

Dear Laurent and Sakari,


On Sat, 2019-02-09 at 20:20 +0200, Laurent Pinchart wrote:
> Hello Frederic,
> 
> On Sat, Feb 09, 2019 at 05:59:35PM +0200, Sakari Ailus wrote:
> > Hi Frederic,
> > 
> > Thanks for the patchset.
> > 
> > Could you also cc the devicetree list, please?
> > 
> > On Fri, Feb 01, 2019 at 07:21:27PM +0800, Frederic Chen wrote:
> > > This patch adds the DT binding documentation for the shared memory
> > > between DIP (Digital Image Processing) unit of the camera ISP system
> > > and the co-processor in Mediatek SoCs.
> > > 
> > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > ---
> > >  .../bindings/media/mediatek,dip_smem.txt           | 29 ++++++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > new file mode 100644
> > > index 0000000..5533721
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
> > > @@ -0,0 +1,29 @@
> > > +Mediatek ISP Shared Memory Device
> > > +
> > > +Mediatek ISP Shared Memory Device is used to manage shared memory
> > > +among CPU, ISP IPs and coprocessor. It is associated with a reserved
> > > +memory region (Please see Documentation\devicetree\bindings\
> > > +reserved-memory\mediatek,reserve-memory-isp_smem.txt) and
> > 
> > s/\\/\//g;
> > 
> > > +and provide the context to allocate memory with dma addresses.
> 
> Does this represent a real device (as in IP core) in the SoC ? There
> seems to be no driver associated with the compatible string defined
> herein in this patch series, what is this node used for ?

It does not represent a real device. It is used for creating the
DIP-specific vb2 buffer allocation context (implemented
in /drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c).
The compatible string has been renamed as “mediatek,dip_smem” in this
patch series and I will correct it in this binding document.

> 
> > > +Required properties:
> > > +- compatible: Should be "mediatek,isp_smem"
> > 
> > s/Should/Shall/
> > 
> > > +
> > > +- iommus: should point to the respective IOMMU block with master port
> > 
> > s/should/shall/
> > 
> > > +  as argument. Please set the ports which may be accessed
> > > +  through the common path. You can see
> > > +  Documentation/devicetree/bindings/iommu/mediatek,iommu.txt
> > > +  for the detail.
> > > +
> > > +- mediatek,larb: must contain the local arbiters in the current Socs.
> > 
> > Perhaps "SoCs"?
> > 
> > > +  Please set the larb of camsys for Pass 1 and imgsys for DIP, or both
> > > +  if you are using all the camera function. You can see
> > > +  Documentation/devicetree/bindings/memory-controllers/
> > > +  mediatek,smi-larb.txt for the detail.
> > > +
> > > +Example:
> > > +	isp_smem: isp_smem {
> > > +		compatible = "mediatek,isp_smem";
> > > +		mediatek,larb = <&larb5>;
> > > +		iommus = <&iommu M4U_PORT_CAM_IMGI>;
> > > +	};
> 


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 V0 1/7] [media] dt-bindings: mt8183: Add binding for DIP shared memory

[Tomasz Figa]

On Tue, Feb 12, 2019 at 6:37 PM Frederic Chen
<frederic.chen@mediatek.com> wrote:
>
> Dear Laurent and Sakari,
>
> I appreciate your messages.
>
> On Sat, 2019-02-09 at 20:17 +0200, Laurent Pinchart wrote:
> > On Sat, Feb 09, 2019 at 05:59:07PM +0200, Sakari Ailus wrote:
> > > Hi Frederic,
> > >
> > > Could you cc the devicetree list, please?
> > >
> > > On Fri, Feb 01, 2019 at 07:21:25PM +0800, Frederic Chen wrote:
> > > > This patch adds the binding for describing the shared memory
> > > > used to exchange configuration and tuning data between the
> > > > co-processor and Digital Image Processing (DIP) unit of the
> > > > camera ISP system on Mediatek SoCs.
> > > >
> > > > Signed-off-by: Frederic Chen <frederic.chen@mediatek.com>
> > > > ---
> > > >  .../mediatek,reserve-memory-dip_smem.txt           | 45 ++++++++++++++++++++++
> > > >  1 file changed, 45 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > new file mode 100644
> > > > index 0000000..0ded478
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
> > > > @@ -0,0 +1,45 @@
> > > > +Mediatek DIP Shared Memory binding
> > > > +
> > > > +This binding describes the shared memory, which serves the purpose of
> > > > +describing the shared memory region used to exchange data between Digital
> > > > +Image Processing (DIP) and co-processor in Mediatek SoCs.
> > > > +
> > > > +The co-processor doesn't have the iommu so we need to use the physical
> > > > +address to access the shared buffer in the firmware.
> > > > +
> > > > +The Digital Image Processing (DIP) can access memory through mt8183 IOMMU so
> > > > +it can use dma address to access the memory region.
> > > > +(See iommu/mediatek,iommu.txt for the detailed description of Mediatek IOMMU)
> > >
> > > What kind of purpose is the memory used for? Buffers containing video data,
> > > or something else? Could the buffer objects be mapped on the devices
> > > based on the need instead?
>
> The memory buffers contain camera 3A tuning data, which are used by the
> co-processor and DIP IP. About mapping the buffer based on the need
> instead, I’m not sure I understand this point. Do you mean that
> allocating and mapping the memory dynamically?
>
> >
> > And could CMA be used when physically contiguous memory is needed ?
>
> DIP driver does not use CMA now, because the first version will be used
> by CrOS but CrOS won’t enable CMA.
>

Thanks Frederic for replying. Let me further clarify what's the problem here.

The co-processor is behind a simple MPU (Memory Protection Unit),
which does not have any mapping capabilities, but can only allow or
deny access to particular parts of the physical address space. That
means that we have to either build some scatter gather capability
inside of the firmware or just do with contiguous allocation.

There is also a security aspect here. The MPU can be accessed from
both the co-processor and CPU, but it has a lockdown mode, which makes
it read only until the SoC is reset. If we allocate the memory
dynamically, we need to keep the MPU unlocked, which automatically
grants the firmware access to all the address space.

For security reasons we decided to go with preallocated memory pool,
which lets us pre-program the MPU and lock it down.

Best regards,
Tomasz

> >
> > > > +
> > > > +
> > > > +Required properties:
> > > > +
> > > > +- compatible: must be "mediatek,reserve-memory-dip_smem"
> > > > +
> > > > +- reg: required for static allocation (see reserved-memory.txt for
> > > > +  the detailed usage)
> > > > +
> > > > +- alloc-range: required for dynamic allocation. The range must
> > > > +  between 0x00000400 and 0x100000000 due to the co-processer's
> > > > +  addressing limitation
> > > > +
> > > > +- size: required for dynamic allocation. The unit is bytes.
> > > > +  If you want to enable the full feature of Digital Processing Unit,
> > > > +  you need 20 MB at least.
> > > > +
> > > > +
> > > > +Example:
> > > > +
> > > > +The following example shows the DIP shared memory setup for MT8183.
> > > > +
> > > > + reserved-memory {
> > > > +         #address-cells = <2>;
> > > > +         #size-cells = <2>;
> > > > +         ranges;
> > > > +         reserve-memory-isp_smem {
> > > > +                 compatible = "mediatek,reserve-memory-dip_smem";
> > > > +                 size = <0 0x1400000>;
> > > > +                 alignment = <0 0x1000>;
> > > > +                 alloc-ranges = <0 0x40000000 0 0x50000000>;
> > > > +         };
> > > > + };
> >
>
> 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 V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver

[Frederic Chen]

Dear Brian,

I appreciate your comments. I'm really sorry for the delay in responding
to the comments due to some mail subscribing failed issue inside my company.

On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote:
> Re-sent to Frederic due to company Mail system issue.
> 
> On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote:
> > Hi,
> > 
> > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote:
> > > 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            |   18 +
> > >  drivers/media/platform/mtk-isp/isp_50/Makefile     |   17 +
> > >  drivers/media/platform/mtk-isp/isp_50/dip/Makefile |   35 +
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h     |  188 +++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c     |  173 +++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h     |   43 +
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h      |  319 ++++
> > >  .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c      | 1643 ++++++++++++++++++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c      |  374 +++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h      |  191 +++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c |  452 ++++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h     |   25 +
> > >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c         | 1000 ++++++++++++
> > >  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h         |   38 +
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c     |  292 ++++
> > >  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h     |   60 +
> > >  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c    | 1385 +++++++++++++++++
> > >  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h    |   93 ++
> > >  18 files changed, 6346 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-core.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> > >  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-smem-drv.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> > >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h
> > > 
> > 
> > ...
> > 
> > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> > > new file mode 100644
> > > index 0000000..9d29507
> > > --- /dev/null
> > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
> > > @@ -0,0 +1,173 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 MediaTek Inc.
> > > + * Author: Frederic Chen <frederic.chen@mediatek.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/platform_device.h>
> > > +#include "mtk_dip-dev.h"
> > > +#include "mtk_dip-ctrl.h"
> > > +
> > > +#define CONFIG_MTK_DIP_COMMON_UT
> > 
> > Please don't do this. You're pretending to have configurability that you
> > don't actually have.
> > 

I got it. I will remove CONFIG_MTK_DIP_COMMON_UT and other similar macro in
the next patch.

> > > +
> > > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl);
> > > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl);
> > > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl);
> > > +
> > > +static void handle_buf_usage_config(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct mtk_dip_ctx_queue *queue =
> > > +		container_of(ctrl->handler,
> > > +			     struct mtk_dip_ctx_queue, ctrl_handler);
> > > +
> > > +	if (ctrl->val < MTK_DIP_V4l2_BUF_USAGE_DEFAULT ||
> > > +	    ctrl->val >= MTK_DIP_V4l2_BUF_USAGE_NONE) {
> > > +		pr_err("Invalid buffer usage id %d", ctrl->val);
> > > +		return;
> > > +	}
> > > +	queue->buffer_usage = ctrl->val;
> > > +}
> > > +
> > > +static void handle_buf_rotate_config(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct mtk_dip_ctx_queue *queue =
> > > +		container_of(ctrl->handler,
> > > +			     struct mtk_dip_ctx_queue, ctrl_handler);
> > > +
> > > +	if (ctrl->val != 0 || ctrl->val != 90 ||
> > > +	    ctrl->val != 180 || ctrl->val != 270) {
> > > +		pr_err("Invalid buffer rotation %d", ctrl->val);
> > > +		return;
> > > +	}
> > > +	queue->rotation = ctrl->val;
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_ops mtk_dip_ctx_ctrl_ops = {
> > > +	.s_ctrl = mtk_dip_ctx_s_ctrl,
> > > +};
> > > +
> > > +#ifdef CONFIG_MTK_DIP_COMMON_UT
> > 
> > Kill the #ifdef if you're not actually going to make this a Kconfig.
> > Same elsewhere.
> > 

I will remove CONFIG_MTK_DIP_COMMON_UT and the related codes in the next patch.

> > > +
> > > +static void handle_ctrl_common_util_ut_set_debug_mode
> > > +	(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	struct mtk_dip_ctx *dev_ctx =
> > > +		container_of(ctrl->handler, struct mtk_dip_ctx, ctrl_handler);
> > > +	dev_ctx->mode = ctrl->val;
> > > +	dev_dbg(&dev_ctx->pdev->dev, "Set ctx(id = %d) mode to %d\n",
> > > +		dev_ctx->ctx_id, dev_ctx->mode);
> > > +}
> > > +
> > > +static const struct v4l2_ctrl_config mtk_dip_mode_config = {
> > > +	.ops	= &mtk_dip_ctx_ctrl_ops,
> > > +	.id	= V4L2_CID_PRIVATE_SET_CTX_MODE_NUM,
> > > +	.name	= "MTK ISP UNIT TEST CASE",
> > > +	.type	= V4L2_CTRL_TYPE_INTEGER,
> > > +	.min	= 0,
> > > +	.max	= 65535,
> > > +	.step	= 1,
> > > +	.def	= 0,
> > > +	.flags	= V4L2_CTRL_FLAG_SLIDER | V4L2_CTRL_FLAG_EXECUTE_ON_WRITE,
> > > +};
> > > +#endif /* CONFIG_MTK_DIP_COMMON_UT */
> > > +
> > > +static int mtk_dip_ctx_s_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > +	switch (ctrl->id) {
> > > +	#ifdef CONFIG_MTK_DIP_COMMON_UT
> > > +	case V4L2_CID_PRIVATE_SET_CTX_MODE_NUM:
> > > +		handle_ctrl_common_util_ut_set_debug_mode(ctrl);
> > > +		break;
> > > +	#endif /* CONFIG_MTK_DIP_COMMON_UT */
> > > +	default:
> > > +			break;
> > > +	}
> > > +	return 0;
> > > +}
> > > +
> > 
> > ...
> > 
> > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> > > new file mode 100644
> > > index 0000000..c735919
> > > --- /dev/null
> > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
> > > @@ -0,0 +1,1643 @@
> > ...
> > 
> > > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT
> > 
> > What is this #ifdef'ery? I don't see MTK_DIP_CTX_DIP_V4L2_UT anywhere.
> > 

MTK_DIP_CTX_DIP_V4L2_UT is only used for our internal test. I will also
remove it in the next patch.


> > > +static int check_and_refill_dip_ut_start_ipi_param
> > > +	(struct img_ipi_frameparam *ipi_param,
> > > +	 struct mtk_dip_ctx_buffer *ctx_buf_in,
> > > +	 struct mtk_dip_ctx_buffer *ctx_buf_out)
> > > +{
> > > +	/* Check the buffer size information from user space */
> > > +	int ret = 0;
> > > +	unsigned char *buffer_ptr = NULL;
> > > +	const unsigned int src_width = 3264;
> > ...
> > 
> > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> > > new file mode 100644
> > > index 0000000..b425031
> > > --- /dev/null
> > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
> > > @@ -0,0 +1,1000 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 Mediatek Corporation.
> > > + * Copyright (c) 2017 Intel Corporation.
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > + * modify it under the terms of the GNU General Public License version
> > > + * 2 as published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * MTK_DIP-v4l2 is highly based on Intel IPU 3 chrome driver
> > > + *
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/videodev2.h>
> > > +#include <media/v4l2-ioctl.h>
> > > +#include <media/videobuf2-dma-contig.h>
> > > +#include <media/v4l2-subdev.h>
> > > +#include <media/v4l2-event.h>
> > > +#include <linux/device.h>
> > > +#include <linux/platform_device.h>
> > > +
> > > +#include "mtk_dip-dev.h"
> > > +#include "mtk_dip-v4l2-util.h"
> > > +
> > > +static u32 mtk_dip_node_get_v4l2_cap
> > > +	(struct mtk_dip_ctx_queue *node_ctx);
> > > +
> > > +static int mtk_dip_videoc_s_meta_fmt(struct file *file,
> > > +				     void *fh, struct v4l2_format *f);
> > > +
> > > +static int mtk_dip_subdev_open(struct v4l2_subdev *sd,
> > > +			       struct v4l2_subdev_fh *fh)
> > > +{
> > > +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> > > +
> > > +	isp_dev->ctx.fh = fh;
> > > +
> > > +	return mtk_dip_ctx_open(&isp_dev->ctx);
> > > +}
> > > +
> > > +static int mtk_dip_subdev_close(struct v4l2_subdev *sd,
> > > +				struct v4l2_subdev_fh *fh)
> > > +{
> > > +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> > > +
> > > +	return mtk_dip_ctx_release(&isp_dev->ctx);
> > > +}
> > > +
> > > +static int mtk_dip_subdev_s_stream(struct v4l2_subdev *sd,
> > > +				   int enable)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	struct mtk_dip_dev *isp_dev = mtk_dip_subdev_to_dev(sd);
> > > +
> > > +	if (enable) {
> > > +		ret = mtk_dip_ctx_streamon(&isp_dev->ctx);
> > > +
> > > +		if (!ret)
> > > +			ret = mtk_dip_dev_queue_buffers
> > > +				(mtk_dip_ctx_to_dev(&isp_dev->ctx), 1);
> > > +		if (ret)
> > > +			pr_err("failed to queue initial buffers (%d)", ret);
> > > +	}	else {
> > > +		ret = mtk_dip_ctx_streamoff(&isp_dev->ctx);
> > > +	}
> > > +
> > > +	if (!ret)
> > > +		isp_dev->mem2mem2.streaming = enable;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +int mtk_dip_subdev_subscribe_event(struct v4l2_subdev *subdev,
> > > +				   struct v4l2_fh *fh,
> > > +				   struct v4l2_event_subscription *sub)
> > 
> > Should be static.

I got it.

> > 
> > > +{
> > > +	pr_info("sub type(%x)", sub->type);
> > 
> > I feel like you have this problem in other places too: this definitely
> > shouldn't be at KERN_INFO level. It seems a bit excessive anyway.

I will use KERN_DEBUG instead and check the same problem in other places.

> > 
> > > +	if (sub->type != V4L2_EVENT_PRIVATE_START &&
> > > +	    sub->type != V4L2_EVENT_MTK_DIP_FRAME_DONE)
> > > +		return -EINVAL;
> > > +
> > > +	return v4l2_event_subscribe(fh, sub, 0, NULL);
> > > +}
> > > +
> > > +int mtk_dip_subdev_unsubscribe_event(struct v4l2_subdev *subdev,
> > > +				     struct v4l2_fh *fh,
> > 
> > Static.
> > 

I got it.

> > > +	struct v4l2_event_subscription *sub)
> > > +{
> > > +	return v4l2_event_unsubscribe(fh, sub);
> > > +}
> > > +
> > > +static int mtk_dip_link_setup(struct media_entity *entity,
> > > +			      const struct media_pad *local,
> > > +	const struct media_pad *remote, u32 flags)
> > > +{
> > > +	struct mtk_dip_mem2mem2_device *m2m2 =
> > > +			container_of(entity,
> > > +				     struct mtk_dip_mem2mem2_device,
> > > +				     subdev.entity);
> > > +	struct mtk_dip_dev *isp_dev =
> > > +		container_of(m2m2, struct mtk_dip_dev, mem2mem2);
> > > +
> > > +	u32 pad = local->index;
> > > +
> > > +	dev_dbg(&isp_dev->pdev->dev,
> > > +		"link setup: %d --> %d\n", pad, remote->index);
> > > +
> > > +	WARN_ON(entity->obj_type != MEDIA_ENTITY_TYPE_V4L2_SUBDEV);
> > > +
> > > +	WARN_ON(pad >= m2m2->num_nodes);
> > > +
> > > +	m2m2->nodes[pad].enabled = !!(flags & MEDIA_LNK_FL_ENABLED);
> > > +
> > > +	/* queue_enable can be phase out in the future since */
> > > +	/* we don't have internal queue of each node in */
> > > +	/* v4l2 common module */
> > > +	isp_dev->queue_enabled[pad] = m2m2->nodes[pad].enabled;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void mtk_dip_vb2_buf_queue(struct vb2_buffer *vb)
> > > +{
> > > +	struct mtk_dip_mem2mem2_device *m2m2 = vb2_get_drv_priv(vb->vb2_queue);
> > > +
> > > +	struct mtk_dip_dev *mtk_dip_dev = mtk_dip_m2m_to_dev(m2m2);
> > > +
> > > +	struct device *dev = &mtk_dip_dev->pdev->dev;
> > > +
> > > +	struct mtk_dip_dev_buffer *buf = NULL;
> > > +
> > > +	struct vb2_v4l2_buffer *v4l2_buf = NULL;
> > > +
> > > +	struct mtk_dip_dev_video_device *node =
> > > +		mtk_dip_vbq_to_isp_node(vb->vb2_queue);
> > > +
> > > +	int queue = mtk_dip_dev_get_queue_id_of_dev_node(mtk_dip_dev, node);
> > 
> > You've got a lot of extra blank lines in here.
> > 

I will remove them in the next patch.

> > > +
> > > +	dev_dbg(dev,
> > > +		"queue vb2_buf: Node(%s) queue id(%d)\n",
> > > +		node->name,
> > > +		queue);
> > > +
> > > +	if (queue < 0) {
> > > +		dev_err(m2m2->dev, "Invalid mtk_dip_dev node.\n");
> > > +		return;
> > > +	}
> > > +
> > > +	if (mtk_dip_dev->ctx.mode == MTK_DIP_CTX_MODE_DEBUG_BYPASS_ALL) {
> > > +		dev_dbg(m2m2->dev, "By pass mode, just loop back the buffer\n");
> > > +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> > > +		return;
> > > +	}
> > > +
> > > +	if (!vb)
> > > +		pr_err("VB can't be null\n");
> > > +
> > > +	buf = mtk_dip_vb2_buf_to_dev_buf(vb);
> > > +
> > > +	if (!buf)
> > > +		pr_err("buf can't be null\n");
> > > +
> > > +	v4l2_buf = to_vb2_v4l2_buffer(vb);
> > > +
> > > +	if (!v4l2_buf)
> > > +		pr_err("v4l2_buf can't be null\n");
> > > +
> > > +	mutex_lock(&mtk_dip_dev->lock);
> > > +
> > > +	/* the dma address will be filled in later frame buffer handling*/
> > > +	mtk_dip_ctx_buf_init(&buf->ctx_buf, queue, (dma_addr_t)0);
> > > +
> > > +	/* Added the buffer into the tracking list */
> > > +	list_add_tail(&buf->m2m2_buf.list,
> > > +		      &m2m2->nodes[node - m2m2->nodes].buffers);
> > > +	mutex_unlock(&mtk_dip_dev->lock);
> > > +
> > > +	/* Enqueue the buffer */
> > > +	if (mtk_dip_dev->mem2mem2.streaming) {
> > > +		dev_dbg(dev, "%s: mtk_dip_dev_queue_buffers\n",
> > > +			node->name);
> > > +		mtk_dip_dev_queue_buffers(mtk_dip_dev, 0);
> > > +	}
> > > +}
> > ...
> > 
> > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > > new file mode 100644
> > > index 0000000..ffdc45e
> > > --- /dev/null
> > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
> > > @@ -0,0 +1,292 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (c) 2018 MediaTek Inc.
> > > + * Author: Frederic Chen <frederic.chen@mediatek.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/platform_device.h>
> > > +#include "mtk_dip-ctx.h"
> > > +#include "mtk_dip.h"
> > > +#include "mtk_dip-v4l2.h"
> > > +#include "mtk-mdp3-regs.h"
> > > +
> > > +#define MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME "MTK-ISP-DIP-V4L2"
> > > +#define MTK_DIP_DEV_DIP_PREVIEW_NAME \
> > > +	MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME
> > > +#define MTK_DIP_DEV_DIP_CAPTURE_NAME "MTK-ISP-DIP-CAP-V4L2"
> > > +
> > > +#ifdef MTK_DIP_CTX_DIP_V4L2_UT
> > > +#include "mtk_dip-dev-ctx-dip-test.h"
> > 
> > The above macros was never defined, and this header doesn't exist.
> > Please remove.
> > 

I will remove them in the next patch.

> > > +#endif
> > > +
> > 
> > ...
> > 
> > > +int mtk_dip_ctx_dip_v4l2_init(struct platform_device *pdev,
> > > +			      struct mtk_dip_dev *isp_preview_dev,
> > > +	struct mtk_dip_dev *isp_capture_dev)
> > > +{
> > > +	struct media_device *media_dev;
> > > +	struct v4l2_device *v4l2_dev;
> > > +	struct v4l2_ctrl_handler *ctrl_handler;
> > > +	int ret = 0;
> > > +
> > > +	/* initialize the v4l2 common part */
> > > +	dev_info(&pdev->dev, "init v4l2 common part: dev=%llx\n",
> > > +		 (unsigned long long)&pdev->dev);
> > > +
> > > +	media_dev = &isp_preview_dev->media_dev;
> > > +	v4l2_dev = &isp_preview_dev->v4l2_dev;
> > > +	ctrl_handler = &isp_preview_dev->ctx.ctrl_handler;
> > > +
> > > +	ret = mtk_dip_media_register(&pdev->dev,
> > > +				     media_dev,
> > > +		MTK_DIP_DEV_DIP_MEDIA_MODEL_NAME);
> > > +
> > > +	ret = mtk_dip_v4l2_register(&pdev->dev,
> > > +				    media_dev,
> > > +		v4l2_dev,
> > > +		ctrl_handler);
> > > +
> > > +	dev_info(&pdev->dev, "init v4l2 preview part\n");
> > > +	ret = mtk_dip_dev_core_init_ext(pdev,
> > > +					isp_preview_dev,
> > > +					&mtk_dip_ctx_desc_dip_preview,
> > > +		media_dev, v4l2_dev);
> > > +
> > > +	if (ret)
> > > +		dev_err(&pdev->dev, "Preview v4l2 part init failed: %d\n", ret);
> > > +
> > > +	dev_info(&pdev->dev, "init v4l2 capture part\n");
> > > +
> > > +	ret = mtk_dip_dev_core_init_ext(pdev,
> > > +					isp_capture_dev,
> > > +					&mtk_dip_ctx_desc_dip_capture,
> > > +		media_dev, v4l2_dev);
> > > +
> > > +	if (ret)
> > > +		dev_err(&pdev->dev, "Capture v4l2 part init failed: %d\n", ret);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/* MTK ISP context initialization */
> > > +int mtk_dip_ctx_dip_preview_init(struct mtk_dip_ctx *preview_ctx)
> > > +{
> > > +	preview_ctx->ctx_id = MTK_DIP_CTX_P2_ID_PREVIEW;
> > > +
> > > +	/* Initialize main data structure */
> > > +	mtk_dip_ctx_core_queue_setup(preview_ctx, &preview_queues_setting);
> > > +
> > > +	return mtk_dip_ctx_core_steup(preview_ctx,
> > > +		&mtk_dip_ctx_dip_preview_setting);
> > > +}
> > > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_preview_init);
> > > +
> > > +/* MTK ISP context initialization */
> > > +int mtk_dip_ctx_dip_capture_init(struct mtk_dip_ctx *capture_ctx)
> > > +{
> > > +	capture_ctx->ctx_id =  MTK_DIP_CTX_P2_ID_CAPTURE;
> > > +	/* Initialize main data structure */
> > > +	mtk_dip_ctx_core_queue_setup(capture_ctx, &capture_queues_setting);
> > > +
> > > +	return mtk_dip_ctx_core_steup(capture_ctx,
> > > +		&mtk_dip_ctx_dip_capture_setting);
> > > +}
> > > +EXPORT_SYMBOL_GPL(mtk_dip_ctx_dip_capture_init);
> > 
> > ...
> > 
> > > diff --git a/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> > > new file mode 100644
> > > index 0000000..3569c7c
> > > --- /dev/null
> > > +++ b/drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
> > > @@ -0,0 +1,1385 @@
> > > +// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
> > > +/*
> > > + * Copyright (c) 2018 MediaTek Inc.
> > > + * Author: Holmes Chiou <holmes.chiou@mediatek.com>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + */
> > > +
> > > +#include <linux/of_device.h>
> > > +#include <linux/module.h>
> > > +#include <linux/device.h>
> > > +#include <linux/kthread.h> /* thread functions */
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/dma-iommu.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/slab.h> /* kzalloc and kfree */
> > > +
> > > +#include "mtk_vpu.h"
> > > +#include "mtk-mdp3-cmdq.h"
> > > +
> > > +#include "mtk_dip-dev.h"
> > > +#include "mtk_dip.h"
> > > +#include "mtk_dip-core.h"
> > > +#include "mtk_dip-v4l2.h"
> > > +
> > > +#define DIP_DEV_NAME		"camera-dip"
> > > +
> > > +#define DIP_COMPOSER_THREAD_TIMEOUT     (16U)
> > > +#define DIP_COMPOSING_WQ_TIMEOUT	(16U)
> > > +#define DIP_COMPOSING_MAX_NUM		(3)
> > > +#define DIP_FLUSHING_WQ_TIMEOUT		(16U)
> > > +
> > > +#define DIP_MAX_ERR_COUNT		(188U)
> > > +
> > > +#define DIP_FRM_SZ		(76 * 1024)
> > > +#define DIP_SUB_FRM_SZ		(16 * 1024)
> > > +#define DIP_TUNING_SZ		(32 * 1024)
> > > +#define DIP_COMP_SZ		(24 * 1024)
> > > +#define DIP_FRAMEPARAM_SZ	(4 * 1024)
> > > +
> > > +#define DIP_TUNING_OFFSET	(DIP_SUB_FRM_SZ)
> > > +#define DIP_COMP_OFFSET		(DIP_TUNING_OFFSET + DIP_TUNING_SZ)
> > > +#define DIP_FRAMEPARAM_OFFSET	(DIP_COMP_OFFSET + DIP_COMP_SZ)
> > > +
> > > +#define DIP_SUB_FRM_DATA_NUM	(32)
> > > +
> > > +#define DIP_SCP_WORKINGBUF_OFFSET	(5 * 1024 * 1024)
> > > +
> > > +#define DIP_GET_ID(x)			(((x) & 0xffff0000) >> 16)
> > > +
> > > +static const struct of_device_id dip_of_ids[] = {
> > > +	/* Remider: Add this device node manually in .dtsi */
> > > +	{ .compatible = "mediatek,mt8183-dip", },
> > > +	{}
> > > +};
> > 
> > Please add:
> > 
> > MODULE_DEVICE_TABLE(of, dip_of_ids);
> > 

I see. I will add this line in the next patch.

> > > +
> > > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> > > +				    struct img_ipi_frameparam *iparam);
> > > +
> > > +static struct img_frameparam *dip_create_framejob(int sequence)
> > > +{
> > > +	struct dip_frame_job *fjob = NULL;
> > > +
> > > +	fjob = kzalloc(sizeof(*fjob), GFP_ATOMIC);
> > > +
> > > +	if (!fjob)
> > > +		return NULL;
> > > +
> > > +	fjob->sequence = sequence;
> > > +
> > > +	return &fjob->fparam;
> > > +}
> > > +
> > > +static void dip_free_framejob(struct img_frameparam *fparam)
> > > +{
> > > +	struct dip_frame_job *fjob = NULL;
> > > +
> > > +	fjob = mtk_dip_fparam_to_job(fparam);
> > > +
> > > +	/* to avoid use after free issue */
> > > +	fjob->sequence = -1;
> > > +
> > > +	kfree(fjob);
> > > +}
> > > +
> > > +static void dip_enable_ccf_clock(struct dip_device *dip_dev)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = pm_runtime_get_sync(dip_dev->larb_dev);
> > > +	if (ret < 0)
> > > +		dev_err(&dip_dev->pdev->dev, "cannot get smi larb clock\n");
> > > +
> > > +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_LARB5);
> > > +	if (ret)
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"cannot prepare and enable DIP_IMG_LARB5 clock\n");
> > > +
> > > +	ret = clk_prepare_enable(dip_dev->dip_clk.DIP_IMG_DIP);
> > > +	if (ret)
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"cannot prepare and enable DIP_IMG_DIP clock\n");
> > > +}
> > > +
> > > +static void dip_disable_ccf_clock(struct dip_device *dip_dev)
> > > +{
> > > +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_DIP);
> > > +	clk_disable_unprepare(dip_dev->dip_clk.DIP_IMG_LARB5);
> > > +	pm_runtime_put_sync(dip_dev->larb_dev);
> > > +}
> > > +
> > > +static int dip_send(struct platform_device *pdev, enum ipi_id id,
> > > +		    void *buf, unsigned int  len, unsigned int wait)
> > > +{
> > > +	vpu_ipi_send_sync_async(pdev, id, buf, len, wait);
> > > +	return 0;
> > > +}
> > > +
> > > +static void call_mtk_dip_ctx_finish(struct dip_device *dip_dev,
> > > +				    struct img_ipi_frameparam *iparam)
> > > +{
> > > +	struct mtk_dip_ctx_finish_param fparam;
> > > +	struct mtk_isp_dip_drv_data *drv_data;
> > > +	struct mtk_dip_ctx *dev_ctx;
> > > +	int ctx_id = 0;
> > > +	int r = 0;
> > > +
> > > +	if (!dip_dev) {
> > > +		pr_err("Can't update buffer status, dip_dev can't be NULL\n");
> > > +		return;
> > > +	}
> > > +
> > > +	if (!iparam) {
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"%s: iparam can't be NULL\n", __func__);
> > > +		return;
> > > +	}
> > > +
> > > +	drv_data = dip_dev_to_drv(dip_dev);
> > > +
> > > +	frame_param_ipi_to_ctx(iparam, &fparam);
> > > +	ctx_id = MTK_DIP_GET_CTX_ID_FROM_SEQUENCE(fparam.frame_id);
> > > +
> > > +	if (ctx_id == MTK_DIP_CTX_P2_ID_PREVIEW) {
> > > +		dev_ctx = &drv_data->isp_preview_dev.ctx;
> > > +	} else if (ctx_id == MTK_DIP_CTX_P2_ID_CAPTURE) {
> > > +		dev_ctx = &drv_data->isp_capture_dev.ctx;
> > > +	} else {
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"unknown ctx id: %d\n", ctx_id);
> > > +		return;
> > > +	}
> > > +
> > > +	r = mtk_dip_ctx_core_job_finish(dev_ctx, &fparam);
> > > +
> > > +	if (r)
> > > +		dev_err(&dip_dev->pdev->dev, "finish op failed: %d\n",
> > > +			r);
> > > +	dev_dbg(&dip_dev->pdev->dev, "Ready to return buffers: CTX(%d), Frame(%d)\n",
> > > +		ctx_id, fparam.frame_id);
> > > +}
> > > +
> > > +static void mtk_dip_notify(void *data)
> > > +{
> > > +	struct dip_device	*dip_dev;
> > > +	struct mtk_dip_hw_ctx	*dip_ctx;
> > > +	struct img_frameparam	*framejob;
> > > +	struct dip_user_id	*user_id;
> > > +	struct dip_subframe	*buf, *tmpbuf;
> > > +	struct img_ipi_frameparam	*frameparam;
> > > +	u32 num;
> > > +	bool found = false;
> > > +
> > > +	frameparam = (struct img_ipi_frameparam *)data;
> > > +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> > > +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> > > +	framejob = container_of(frameparam,
> > > +				struct img_frameparam,
> > > +				frameparam);
> > > +
> > > +	if (frameparam->state == FRAME_STATE_HW_TIMEOUT) {
> > > +		dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> > > +			 (void *)frameparam, sizeof(*frameparam), 0);
> > > +		dev_err(&dip_dev->pdev->dev, "frame no(%d) HW timeout\n",
> > > +			frameparam->frame_no);
> > > +	}
> > > +
> > > +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > +	list_for_each_entry_safe(buf, tmpbuf,
> > > +				 &dip_ctx->dip_usedbufferlist.queue,
> > > +				 list_entry) {
> > > +		if (buf->buffer.pa == frameparam->subfrm_data.pa) {
> > > +			list_del(&buf->list_entry);
> > > +			dip_ctx->dip_usedbufferlist.queue_cnt--;
> > > +			found = true;
> > > +			dev_dbg(&dip_dev->pdev->dev,
> > > +				"Find used buffer (%x)\n", buf->buffer.pa);
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > +
> > > +	if (!found) {
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"frame_no(%d) buffer(%x) used buffer count(%d)\n",
> > > +			frameparam->frame_no, frameparam->subfrm_data.pa,
> > > +			dip_ctx->dip_usedbufferlist.queue_cnt);
> > > +
> > > +		frameparam->state = FRAME_STATE_ERROR;
> > > +
> > > +	} else {
> > > +		mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> > > +		list_add_tail(&buf->list_entry,
> > > +			      &dip_ctx->dip_freebufferlist.queue);
> > > +		dip_ctx->dip_freebufferlist.queue_cnt++;
> > > +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > +
> > > +		frameparam->state = FRAME_STATE_DONE;
> > > +	}
> > > +
> > > +	call_mtk_dip_ctx_finish(dip_dev, frameparam);
> > > +
> > > +	found = false;
> > > +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > +	list_for_each_entry(user_id,
> > > +			    &dip_ctx->dip_useridlist.queue,
> > > +			    list_entry) {
> > > +		if (DIP_GET_ID(frameparam->index) == user_id->id) {
> > > +			user_id->num--;
> > > +			dev_dbg(&dip_dev->pdev->dev,
> > > +				"user_id(%x) is found, and cnt: %d\n",
> > > +				user_id->id, user_id->num);
> > > +			found = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > +	wake_up(&dip_ctx->flushing_wq);
> > > +	dev_dbg(&dip_dev->pdev->dev,
> > > +		"frame_no(%d) is finished\n", framejob->frameparam.frame_no);
> > > +	dip_free_framejob(framejob);
> > > +
> > > +	num = atomic_dec_return(&dip_ctx->num_running);
> > > +	dev_dbg(&dip_dev->pdev->dev, "Running count: %d\n", num);
> > > +}
> > > +
> > > +static void mdp_cb_worker(struct work_struct *work)
> > > +{
> > > +	struct mtk_mdpcb_work *mdpcb_work;
> > > +
> > > +	mdpcb_work = container_of(work, struct mtk_mdpcb_work, frame_work);
> > > +	mtk_dip_notify(mdpcb_work->frameparams);
> > > +	kfree(mdpcb_work);
> > > +}
> > > +
> > > +static struct img_ipi_frameparam *convert_to_fparam(struct cmdq_cb_data *data)
> > > +{
> > > +	struct device *dev = NULL;
> > 
> > Every use of dev_err() in this function is wrong, since you're
> > guaranteed to be NULL. Either come up with some better way to report
> > device errors using the pointers you have, or else just switch to
> > pr_err().
> > 

I see. I would like to switch to pr_err().

> > > +	struct dip_device *dip_dev = NULL;
> > > +	struct dip_frame_job *fjob = NULL;
> > > +	struct img_ipi_frameparam *ipi_fparam = NULL;
> > > +
> > > +	if (!data) {
> > > +		dev_err(dev, "DIP got NULL in cmdq_cb_data,%s\n",
> > > +			__func__);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	if (data->sta != CMDQ_CB_NORMAL) {
> > > +		dev_warn(dev, "DIP got CMDQ CB (%d) without CMDQ_CB_NORMAL\n",
> > > +			 data->sta);
> > > +	}
> > > +
> > > +	if (!data->data) {
> > > +		dev_err(dev, "DIP got NULL data in cmdq_cb_data,%s\n",
> > > +			__func__);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	fjob = mtk_dip_ipi_fparam_to_job(data->data);
> > > +
> > > +	if (fjob->sequence == -1) {
> > > +		dev_err(dev, "Invalid cmdq_cb_data(%llx)\n",
> > > +			(unsigned long long)data);
> > > +		ipi_fparam = NULL;
> > > +	} else {
> > > +		ipi_fparam = &fjob->fparam.frameparam;
> > > +		dip_dev = dip_hw_ctx_to_dev((void *)ipi_fparam->drv_data);
> > > +		dev = &dip_dev->pdev->dev;
> > > +	}
> > > +
> > > +	dev_dbg(dev, "framejob(0x%llx,seq:%d):\n",
> > > +		(unsigned long long)fjob, fjob->sequence);
> > > +	dev_dbg(dev, "idx(%d),no(%d),s(%d),n_in(%d),n_out(%d),drv(%llx)\n",
> > > +		fjob->fparam.frameparam.index,
> > > +		fjob->fparam.frameparam.frame_no,
> > > +		fjob->fparam.frameparam.state,
> > > +		fjob->fparam.frameparam.num_inputs,
> > > +		fjob->fparam.frameparam.num_outputs,
> > > +		(unsigned long long)fjob->fparam.frameparam.drv_data
> > > +	);
> > > +
> > > +	return ipi_fparam;
> > > +}
> > > +
> > > +/* Maybe in IRQ context of cmdq */
> > > +void dip_mdp_cb_func(struct cmdq_cb_data data)
> > 
> > Make this static.
> > 

I got it.

> > > +{
> > > +	struct img_ipi_frameparam *frameparam;
> > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > +	struct mtk_mdpcb_work *mdpcb_work;
> > > +
> > > +	frameparam = convert_to_fparam(&data);
> > > +
> > > +	if (!frameparam) {
> > > +		dev_err(NULL, "%s return due to invalid cmdq_cb_data(%llx)",
> > 
> > Don't directly pass NULL to dev_err(). Just use pr_err() or similar.
> >

I will use pr_err() in the next patch.


> > > +			__func__, &data);
> > > +		return;
> > > +	}
> > > +
> > > +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> > > +
> > > +	mdpcb_work = kzalloc(sizeof(*mdpcb_work), GFP_ATOMIC);
> > > +	WARN_ONCE(!mdpcb_work, "frame_no(%d) is lost", frameparam->frame_no);
> > > +	if (!mdpcb_work)
> > > +		return;
> > > +
> > > +	INIT_WORK(&mdpcb_work->frame_work, mdp_cb_worker);
> > > +	mdpcb_work->frameparams = frameparam;
> > > +	if (data.sta != CMDQ_CB_NORMAL)
> > > +		mdpcb_work->frameparams->state = FRAME_STATE_HW_TIMEOUT;
> > > +
> > > +	queue_work(dip_ctx->mdpcb_workqueue, &mdpcb_work->frame_work);
> > > +}
> > > +
> > > +static void dip_vpu_handler(void *data, unsigned int len, void *priv)
> > > +{
> > > +	struct img_frameparam *framejob;
> > > +	struct img_ipi_frameparam *frameparam;
> > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > +	struct dip_device *dip_dev;
> > > +	unsigned long flags;
> > > +	u32 num;
> > > +
> > > +	WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__);
> > > +	if (!data)
> > 
> > You can combine these lines:
> > 
> > 	
> > 	if (WARN_ONCE(!data, "%s is failed due to NULL data\n", __func__))
> > 		return;
> > 

I got it. I will combine them.

> > > +		return;
> > > +
> > > +	frameparam = (struct img_ipi_frameparam *)data;
> > > +
> > > +	framejob = dip_create_framejob(frameparam->index);
> > > +	WARN_ONCE(!framejob, "frame_no(%d) is lost", frameparam->frame_no);
> > > +	if (!framejob)
> > 
> > Same here.
> > 

I will also combine them here. 

> > > +		return;
> > > +
> > > +	dip_ctx = (struct mtk_dip_hw_ctx *)frameparam->drv_data;
> > > +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> > > +
> > > +	wake_up(&dip_ctx->composing_wq);
> > > +	memcpy(&framejob->frameparam, data, sizeof(framejob->frameparam));
> > > +	num = atomic_dec_return(&dip_ctx->num_composing);
> > > +
> > > +	spin_lock_irqsave(&dip_ctx->dip_gcejoblist.queuelock, flags);
> > > +	list_add_tail(&framejob->list_entry, &dip_ctx->dip_gcejoblist.queue);
> > > +	dip_ctx->dip_gcejoblist.queue_cnt++;
> > > +	spin_unlock_irqrestore(&dip_ctx->dip_gcejoblist.queuelock, flags);
> > > +
> > > +	dev_dbg(&dip_dev->pdev->dev,
> > > +		"frame_no(%d) is back, composing num: %d\n",
> > > +		frameparam->frame_no, num);
> > > +
> > > +	wake_up(&dip_ctx->dip_runner_thread.wq);
> > > +}
> > > +
> > 
> > ...
> > > +static int dip_runner_func(void *data)
> > > +{
> > 
> > ...
> > 
> > > +
> > > +			mdp_cmdq_sendtask
> > 
> > I don't see this defined anywhere?
> > 

mdp_cmdq_sendtask() is defined in MDP 3 driver. We will send
the RFC patch for Mediatek MDP 3 driver by 2/28.

> > > +				(dip_ctx->mdp_pdev,
> > > +				 (struct img_config *)
> > > +					framejob->frameparam.config_data.va,
> > > +				 &framejob->frameparam, NULL, false,
> > > +				 dip_mdp_cb_func,
> > > +				 (void *)&framejob->frameparam);
> > > +
> > ...
> > > +	return 0;
> > > +}
> > > +
> > > +static void dip_submit_worker(struct work_struct *work)
> > > +{
> > > +	struct mtk_dip_submit_work *dip_submit_work =
> > > +		container_of(work, struct mtk_dip_submit_work, frame_work);
> > > +
> > > +	struct mtk_dip_hw_ctx  *dip_ctx = dip_submit_work->dip_ctx;
> > > +	struct mtk_dip_work *dip_work;
> > > +	struct dip_device *dip_dev;
> > > +	struct dip_subframe *buf;
> > > +	u32 len, num;
> > > +	int ret;
> > > +
> > > +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> > > +	num  = atomic_read(&dip_ctx->num_composing);
> > > +
> > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > +	dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
> > > +				    struct mtk_dip_work, list_entry);
> > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > 
> > I see you grab the head of the list here, but then you release the lock.
> > Then you later assume that reference is still valid, throughout this
> > function.
> > 
> > That's usually true, because you only remove/delete entries from this
> > list within this same workqueue (at the end of this function). But it's
> > not true in dip_release_context() (which doesn't even grab the lock,
> > BTW).
> > 
> > I think there could be several ways to solve this, but judging by how
> > this list entry is used...couldn't you just remove it from the list
> > here, while holding the lock? Then you only have to kfree() it when
> > you're done under the free_work_list label.
> > 

I see. I would like to modify the codes as following:

mutex_lock(&dip_ctx->dip_useridlist.queuelock);
dip_work->user_id->num--;
list_del(&dip_work->list_entry);
dip_ctx->dip_worklist.queue_cnt--;
len = dip_ctx->dip_worklist.queue_cnt;
mutex_unlock(&dip_ctx->dip_useridlist.queuelock);

goto free_work_list;

/* ...... */

free_work_list:
	kfree(dip_work);

> > > +
> > > +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > +	if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
> > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > +
> > > +		dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
> > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > +
> > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > +		dip_work->user_id->num--;
> > > +		dev_dbg(&dip_dev->pdev->dev,
> > > +			"user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n",
> > > +			dip_work->user_id->id, dip_work->user_id->num,
> > > +			dip_work->frameparams.frame_no,
> > > +			dip_work->frameparams.index);
> > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > +
> > > +		goto free_work_list;
> > > +	}
> > > +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > +
> > > +	while (num >= DIP_COMPOSING_MAX_NUM) {
> > > +		ret = wait_event_interruptible_timeout
> > > +			(dip_ctx->composing_wq,
> > > +			 (num < DIP_COMPOSING_MAX_NUM),
> > > +			 msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT));
> > > +
> > > +		if (ret == -ERESTARTSYS)
> > > +			dev_err(&dip_dev->pdev->dev,
> > > +				"interrupted by a signal!\n");
> > > +		else if (ret == 0)
> > > +			dev_dbg(&dip_dev->pdev->dev,
> > > +				"timeout frame_no(%d), num: %d\n",
> > > +				dip_work->frameparams.frame_no, num);
> > > +		else
> > > +			dev_dbg(&dip_dev->pdev->dev,
> > > +				"wakeup frame_no(%d), num: %d\n",
> > > +				dip_work->frameparams.frame_no, num);
> > > +
> > > +		num = atomic_read(&dip_ctx->num_composing);
> > > +	};
> > > +
> > > +	mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> > > +	if (list_empty(&dip_ctx->dip_freebufferlist.queue)) {
> > > +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > +
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"frame_no(%d) index: %x no free buffer: %d\n",
> > > +			dip_work->frameparams.frame_no,
> > > +			dip_work->frameparams.index,
> > > +			dip_ctx->dip_freebufferlist.queue_cnt);
> > > +
> > > +		/* Call callback to notify V4L2 common framework
> > > +		 * for failure of enqueue
> > > +		 */
> > > +		dip_work->frameparams.state = FRAME_STATE_ERROR;
> > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > +
> > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > +		dip_work->user_id->num--;
> > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > +
> > > +		goto free_work_list;
> > > +	}
> > > +
> > > +	buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue,
> > > +			       struct dip_subframe,
> > > +			       list_entry);
> > > +	list_del(&buf->list_entry);
> > > +	dip_ctx->dip_freebufferlist.queue_cnt--;
> > > +	mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > +
> > > +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > +	list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue);
> > > +	dip_ctx->dip_usedbufferlist.queue_cnt++;
> > > +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > +
> > > +	memcpy(&dip_work->frameparams.subfrm_data,
> > > +	       &buf->buffer, sizeof(buf->buffer));
> > > +
> > > +	memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ);
> > > +
> > > +	memcpy(&dip_work->frameparams.config_data,
> > > +	       &buf->config_data, sizeof(buf->config_data));
> > > +
> > > +	memset((char *)buf->config_data.va, 0, DIP_COMP_SZ);
> > > +
> > > +	if (dip_work->frameparams.tuning_data.pa == 0) {
> > > +		dev_dbg(&dip_dev->pdev->dev,
> > > +			"frame_no(%d) has no tuning_data\n",
> > > +			dip_work->frameparams.frame_no);
> > > +
> > > +		memcpy(&dip_work->frameparams.tuning_data,
> > > +		       &buf->tuning_buf, sizeof(buf->tuning_buf));
> > > +
> > > +		memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> > > +		/* When user enqueued without tuning buffer,
> > > +		 * it would use driver internal buffer.
> > > +		 * So, tuning_data.va should be 0
> > > +		 */
> > > +		dip_work->frameparams.tuning_data.va = 0;
> > > +	}
> > > +
> > > +	dip_work->frameparams.drv_data = (u64)dip_ctx;
> > > +	dip_work->frameparams.state = FRAME_STATE_COMPOSING;
> > > +
> > > +	memcpy((void *)buf->frameparam.va, &dip_work->frameparams,
> > > +	       sizeof(dip_work->frameparams));
> > > +
> > > +	dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> > > +		 (void *)&dip_work->frameparams,
> > > +		 sizeof(dip_work->frameparams), 0);
> > > +	num = atomic_inc_return(&dip_ctx->num_composing);
> > > +
> > > +free_work_list:
> > > +
> > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > +	list_del(&dip_work->list_entry);
> > > +	dip_ctx->dip_worklist.queue_cnt--;
> > > +	len = dip_ctx->dip_worklist.queue_cnt;
> > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > > +
> > > +	dev_dbg(&dip_dev->pdev->dev,
> > > +		"frame_no(%d) index: %x, worklist count: %d, composing num: %d\n",
> > > +		dip_work->frameparams.frame_no, dip_work->frameparams.index,
> > > +		len, num);
> > > +
> > > +	kfree(dip_work);
> > > +}
> > 
> > ...
> > 
> > > +int dip_open_context(struct dip_device *dip_dev)
> > 
> > Should be static.
> >

I will change it to static.

> > > +{
> > 
> > ...
> > 
> > > +}
> > > +
> > > +int dip_release_context(struct dip_device *dip_dev)
> > 
> > Should be static.
> > 

I will change it to static.

> > > +{
> > > +	u32 i = 0;
> > > +	struct dip_subframe *buf, *tmpbuf;
> > > +	struct mtk_dip_work *dip_work, *tmp_work;
> > > +	struct dip_user_id  *dip_userid, *tmp_id;
> > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > +
> > > +	dip_ctx = &dip_dev->dip_ctx;
> > > +	dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n",
> > > +		dip_ctx->dip_worklist.queue_cnt);
> > > +
> > > +	list_for_each_entry_safe(dip_work, tmp_work,
> > > +				 &dip_ctx->dip_worklist.queue,
> > > +				 list_entry) {
> > 
> > Shouldn't you be holding the mutex for this? Or alternatively, cancel
> > any outstanding work and move the flush_workqueue()/destroy_workqueue()
> > up.
> > 
> > Similar questions for the other lists we're going through here.
> > 

We missed the mutex holding here. I would like to change the codes as following:

mutex_lock(&dip_ctx->dip_worklist.queuelock);
list_for_each_entry_safe(dip_work, tmp_work,
			 &dip_ctx->dip_worklist.queue,
			 list_entry) {
	list_del(&dip_work->list_entry);
	dip_ctx->dip_worklist.queue_cnt--;
	kfree(dip_work);
}
mutex_unlock(&dip_ctx->dip_worklist.queuelock);

I will also modify dip_useridlist and dip_ctx->dip_freebufferlist 
parts like dip_ctx->dip_worklist.

> > > +		list_del(&dip_work->list_entry);
> > > +		dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n",
> > > +			dip_work->frameparams.frame_no);
> > > +		kfree(dip_work);
> > > +		dip_ctx->dip_worklist.queue_cnt--;
> > > +	}
> > > +
> > > +	if (dip_ctx->dip_worklist.queue_cnt != 0)
> > > +		dev_dbg(&dip_dev->pdev->dev,
> > > +			"dip_worklist is not empty (%d)\n",
> > > +			dip_ctx->dip_worklist.queue_cnt);
> > > +
> > > +	list_for_each_entry_safe(dip_userid, tmp_id,
> > > +				 &dip_ctx->dip_useridlist.queue,
> > > +				 list_entry) {
> > > +		list_del(&dip_userid->list_entry);
> > > +		dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n",
> > > +			dip_userid->id);
> > > +		kfree(dip_userid);
> > > +		dip_ctx->dip_useridlist.queue_cnt--;
> > > +	}
> > > +
> > > +	if (dip_ctx->dip_useridlist.queue_cnt != 0)
> > > +		dev_dbg(&dip_dev->pdev->dev,
> > > +			"dip_useridlist is not empty (%d)\n",
> > > +			dip_ctx->dip_useridlist.queue_cnt);
> > > +
> > > +	flush_workqueue(dip_ctx->mdpcb_workqueue);
> > > +	destroy_workqueue(dip_ctx->mdpcb_workqueue);
> > > +	dip_ctx->mdpcb_workqueue = NULL;
> > > +
> > > +	flush_workqueue(dip_ctx->composer_wq);
> > > +	destroy_workqueue(dip_ctx->composer_wq);
> > > +	dip_ctx->composer_wq = NULL;
> > > +
> > > +	atomic_set(&dip_ctx->num_composing, 0);
> > > +	atomic_set(&dip_ctx->num_running, 0);
> > > +
> > > +	kthread_stop(dip_ctx->dip_runner_thread.thread);
> > > +	dip_ctx->dip_runner_thread.thread = NULL;
> > > +
> > > +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> > > +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> > > +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> > > +
> > > +	/* All the buffer should be in the freebufferlist when release */
> > > +	list_for_each_entry_safe(buf, tmpbuf,
> > > +				 &dip_ctx->dip_freebufferlist.queue,
> > > +				 list_entry) {
> > > +		struct sg_table *sgt = &buf->table;
> > > +
> > > +		dev_dbg(&dip_dev->pdev->dev,
> > > +			"buf pa (%d): %x\n", i, buf->buffer.pa);
> > > +		dip_ctx->dip_freebufferlist.queue_cnt--;
> > > +		dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl,
> > > +				   sgt->orig_nents,
> > > +				   DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
> > > +		sg_free_table(sgt);
> > > +		list_del(&buf->list_entry);
> > > +		kfree(buf);
> > > +		buf = NULL;
> > > +		i++;
> > > +	}
> > > +
> > > +	if (dip_ctx->dip_freebufferlist.queue_cnt != 0 &&
> > > +	    i != DIP_SUB_FRM_DATA_NUM)
> > > +		dev_err(&dip_dev->pdev->dev,
> > > +			"dip_freebufferlist is not empty (%d/%d)\n",
> > > +			dip_ctx->dip_freebufferlist.queue_cnt, i);
> > > +
> > > +	mutex_destroy(&dip_ctx->dip_useridlist.queuelock);
> > > +	mutex_destroy(&dip_ctx->dip_worklist.queuelock);
> > > +	mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock);
> > > +	mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > 
> > ...
> > 
> > > +static int mtk_dip_probe(struct platform_device *pdev)
> > > +{
> > > +	struct mtk_isp_dip_drv_data *dip_drv;
> > > +	struct dip_device *dip_dev;
> > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > +	struct device_node *node;
> > > +	struct platform_device *larb_pdev;
> > > +
> > > +	int ret = 0;
> > > +
> > > +	dev_info(&pdev->dev, "E. DIP driver probe.\n");
> > > +
> > > +	dip_drv = devm_kzalloc(&pdev->dev, sizeof(*dip_drv), GFP_KERNEL);
> > 
> > Need to check for NULL.
> >

I got it.

> > > +	dev_set_drvdata(&pdev->dev, dip_drv);
> > > +	dip_dev = &dip_drv->dip_dev;
> > > +
> > > +	if (!dip_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_info(&pdev->dev, "Created dip_dev = 0x%p\n", dip_dev);
> > > +
> > > +	dip_dev->pdev = pdev;
> > > +	dip_ctx = &dip_dev->dip_ctx;
> > > +
> > > +	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> > > +	if (!node) {
> > > +		dev_err(&pdev->dev, "no mediatek,larb found");
> > > +		return -EINVAL;
> > > +	}
> > > +	larb_pdev = of_find_device_by_node(node);
> > > +	if (!larb_pdev) {
> > > +		dev_err(&pdev->dev, "no mediatek,larb device found");
> > > +		return -EINVAL;
> > > +	}
> > > +	dip_dev->larb_dev = &larb_pdev->dev;
> > > +
> > > +	/*CCF: Grab clock pointer (struct clk*) */
> > 
> > Add a space before 'CCF'.
> > 

I got it.

> > > +	dip_dev->dip_clk.DIP_IMG_LARB5 = devm_clk_get(&pdev->dev,
> > > +						      "DIP_CG_IMG_LARB5");
> > > +	dip_dev->dip_clk.DIP_IMG_DIP = devm_clk_get(&pdev->dev,
> > > +						    "DIP_CG_IMG_DIP");
> > > +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_LARB5)) {
> > > +		dev_err(&pdev->dev, "cannot get DIP_IMG_LARB5 clock\n");
> > > +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_LARB5);
> > > +	}
> > > +	if (IS_ERR(dip_dev->dip_clk.DIP_IMG_DIP)) {
> > > +		dev_err(&pdev->dev, "cannot get DIP_IMG_DIP clock\n");
> > > +		return PTR_ERR(dip_dev->dip_clk.DIP_IMG_DIP);
> > > +	}
> > > +
> > > +	pm_runtime_enable(&pdev->dev);
> > > +
> > > +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> > > +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> > > +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> > > +
> > > +	atomic_set(&dip_ctx->num_composing, 0);
> > > +	atomic_set(&dip_ctx->num_running, 0);
> > > +
> > > +	dip_ctx->dip_worklist.queue_cnt = 0;
> > > +
> > > +	ret = mtk_dip_ctx_dip_v4l2_init(pdev,
> > > +					&dip_drv->isp_preview_dev,
> > > +		&dip_drv->isp_capture_dev);
> > > +
> > > +	if (ret)
> > > +		dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret);
> > > +
> > > +	dev_info(&pdev->dev, "X. DIP driver probe.\n");
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int mtk_dip_remove(struct platform_device *pdev)
> > > +{
> > > +	struct mtk_isp_dip_drv_data *drv_data =
> > > +		dev_get_drvdata(&pdev->dev);
> > > +
> > > +	/*  */
> > 
> > What's with the empty comments? Here and below.
> > 

I will remove them.

> > > +	if (drv_data) {
> > > +		mtk_dip_dev_core_release(pdev, &drv_data->isp_preview_dev);
> > > +		mtk_dip_dev_core_release(pdev, &drv_data->isp_capture_dev);
> > > +		dev_info(&pdev->dev, "E. %s\n", __func__);
> > 
> > Remove this line.
> > 

I will remove this line.

> > > +	}
> > > +
> > > +	pm_runtime_disable(&pdev->dev);
> > > +
> > > +	/*  */
> > > +	return 0;
> > > +}
> > > +
> > 
> > ...
> > 
> > > +static struct platform_driver mtk_dip_driver = {
> > > +	.probe   = mtk_dip_probe,
> > > +	.remove  = mtk_dip_remove,
> > > +	.driver  = {
> > > +		.name  = DIP_DEV_NAME,
> > > +		.owner = THIS_MODULE,
> > 
> > You don't need the .owner line. module_platform_driver() /
> > platform_driver_register() takes care of this for you.
> > 

I got it. I will remove the .owner line

> > Brian
> > 
> > > +		.of_match_table = dip_of_ids,
> > > +		.pm     = &mtk_dip_pm_ops,
> > > +	}
> > > +};
> > > +
> > > +module_platform_driver(mtk_dip_driver);
> > > +
> > > +MODULE_DESCRIPTION("Camera DIP driver");
> > > +MODULE_LICENSE("GPL");
> > ...
> 
> 


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 V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver

[Brian Norris]

Hi Frederic,

On Sat, Feb 23, 2019 at 02:18:54PM +0800, Frederic Chen wrote:
> Dear Brian,
> 
> I appreciate your comments. I'm really sorry for the delay in responding
> to the comments due to some mail subscribing failed issue inside my company.

No problem.

> On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote:
> > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote:
> > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote:

> > > > +static void dip_submit_worker(struct work_struct *work)
> > > > +{
> > > > +	struct mtk_dip_submit_work *dip_submit_work =
> > > > +		container_of(work, struct mtk_dip_submit_work, frame_work);
> > > > +
> > > > +	struct mtk_dip_hw_ctx  *dip_ctx = dip_submit_work->dip_ctx;
> > > > +	struct mtk_dip_work *dip_work;
> > > > +	struct dip_device *dip_dev;
> > > > +	struct dip_subframe *buf;
> > > > +	u32 len, num;
> > > > +	int ret;
> > > > +
> > > > +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> > > > +	num  = atomic_read(&dip_ctx->num_composing);
> > > > +
> > > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > > +	dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
> > > > +				    struct mtk_dip_work, list_entry);
> > > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > > 
> > > I see you grab the head of the list here, but then you release the lock.
> > > Then you later assume that reference is still valid, throughout this
> > > function.
> > > 
> > > That's usually true, because you only remove/delete entries from this
> > > list within this same workqueue (at the end of this function). But it's
> > > not true in dip_release_context() (which doesn't even grab the lock,
> > > BTW).
> > > 
> > > I think there could be several ways to solve this, but judging by how
> > > this list entry is used...couldn't you just remove it from the list
> > > here, while holding the lock? Then you only have to kfree() it when
> > > you're done under the free_work_list label.
> > > 
> 
> I see. I would like to modify the codes as following:
> 
> mutex_lock(&dip_ctx->dip_useridlist.queuelock);

You missed the part where you get the head of the list:

	dip_work = list_first_entry(...);

But otherwise mostly looks OK.

> dip_work->user_id->num--;

Why do you need to do that with the queuelock held? Once you remove this
work item from the list (safely under the lock), shouldn't you be the
only one accessing it?

(Note, I don't actually know what that 'num' really means. I'm just
looking at basic driver mechanics.)

> list_del(&dip_work->list_entry);
> dip_ctx->dip_worklist.queue_cnt--;
> len = dip_ctx->dip_worklist.queue_cnt;
> mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> 
> goto free_work_list;
> 
> /* ...... */
> 
> free_work_list:
> 	kfree(dip_work);
> 
> > > > +
> > > > +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > +	if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
> > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > +
> > > > +		dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
> > > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > > +
> > > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > +		dip_work->user_id->num--;
> > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > +			"user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n",
> > > > +			dip_work->user_id->id, dip_work->user_id->num,
> > > > +			dip_work->frameparams.frame_no,
> > > > +			dip_work->frameparams.index);
> > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > +
> > > > +		goto free_work_list;
> > > > +	}
> > > > +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > +
> > > > +	while (num >= DIP_COMPOSING_MAX_NUM) {
> > > > +		ret = wait_event_interruptible_timeout
> > > > +			(dip_ctx->composing_wq,
> > > > +			 (num < DIP_COMPOSING_MAX_NUM),
> > > > +			 msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT));
> > > > +
> > > > +		if (ret == -ERESTARTSYS)
> > > > +			dev_err(&dip_dev->pdev->dev,
> > > > +				"interrupted by a signal!\n");
> > > > +		else if (ret == 0)
> > > > +			dev_dbg(&dip_dev->pdev->dev,
> > > > +				"timeout frame_no(%d), num: %d\n",
> > > > +				dip_work->frameparams.frame_no, num);
> > > > +		else
> > > > +			dev_dbg(&dip_dev->pdev->dev,
> > > > +				"wakeup frame_no(%d), num: %d\n",
> > > > +				dip_work->frameparams.frame_no, num);
> > > > +
> > > > +		num = atomic_read(&dip_ctx->num_composing);
> > > > +	};
> > > > +
> > > > +	mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > +	if (list_empty(&dip_ctx->dip_freebufferlist.queue)) {
> > > > +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > +
> > > > +		dev_err(&dip_dev->pdev->dev,
> > > > +			"frame_no(%d) index: %x no free buffer: %d\n",
> > > > +			dip_work->frameparams.frame_no,
> > > > +			dip_work->frameparams.index,
> > > > +			dip_ctx->dip_freebufferlist.queue_cnt);
> > > > +
> > > > +		/* Call callback to notify V4L2 common framework
> > > > +		 * for failure of enqueue
> > > > +		 */
> > > > +		dip_work->frameparams.state = FRAME_STATE_ERROR;
> > > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > > +
> > > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > +		dip_work->user_id->num--;
> > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > +
> > > > +		goto free_work_list;
> > > > +	}
> > > > +
> > > > +	buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue,
> > > > +			       struct dip_subframe,
> > > > +			       list_entry);
> > > > +	list_del(&buf->list_entry);
> > > > +	dip_ctx->dip_freebufferlist.queue_cnt--;
> > > > +	mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > +
> > > > +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > +	list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue);
> > > > +	dip_ctx->dip_usedbufferlist.queue_cnt++;
> > > > +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > +
> > > > +	memcpy(&dip_work->frameparams.subfrm_data,
> > > > +	       &buf->buffer, sizeof(buf->buffer));
> > > > +
> > > > +	memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ);
> > > > +
> > > > +	memcpy(&dip_work->frameparams.config_data,
> > > > +	       &buf->config_data, sizeof(buf->config_data));
> > > > +
> > > > +	memset((char *)buf->config_data.va, 0, DIP_COMP_SZ);
> > > > +
> > > > +	if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > +			"frame_no(%d) has no tuning_data\n",
> > > > +			dip_work->frameparams.frame_no);
> > > > +
> > > > +		memcpy(&dip_work->frameparams.tuning_data,
> > > > +		       &buf->tuning_buf, sizeof(buf->tuning_buf));
> > > > +
> > > > +		memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> > > > +		/* When user enqueued without tuning buffer,
> > > > +		 * it would use driver internal buffer.
> > > > +		 * So, tuning_data.va should be 0
> > > > +		 */
> > > > +		dip_work->frameparams.tuning_data.va = 0;
> > > > +	}
> > > > +
> > > > +	dip_work->frameparams.drv_data = (u64)dip_ctx;
> > > > +	dip_work->frameparams.state = FRAME_STATE_COMPOSING;
> > > > +
> > > > +	memcpy((void *)buf->frameparam.va, &dip_work->frameparams,
> > > > +	       sizeof(dip_work->frameparams));
> > > > +
> > > > +	dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> > > > +		 (void *)&dip_work->frameparams,
> > > > +		 sizeof(dip_work->frameparams), 0);
> > > > +	num = atomic_inc_return(&dip_ctx->num_composing);
> > > > +
> > > > +free_work_list:
> > > > +
> > > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > > +	list_del(&dip_work->list_entry);
> > > > +	dip_ctx->dip_worklist.queue_cnt--;
> > > > +	len = dip_ctx->dip_worklist.queue_cnt;
> > > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > > > +
> > > > +	dev_dbg(&dip_dev->pdev->dev,
> > > > +		"frame_no(%d) index: %x, worklist count: %d, composing num: %d\n",
> > > > +		dip_work->frameparams.frame_no, dip_work->frameparams.index,
> > > > +		len, num);
> > > > +
> > > > +	kfree(dip_work);
> > > > +}


> > > > +int dip_release_context(struct dip_device *dip_dev)
> > > 
> > > Should be static.
> > > 
> 
> I will change it to static.
> 
> > > > +{
> > > > +	u32 i = 0;
> > > > +	struct dip_subframe *buf, *tmpbuf;
> > > > +	struct mtk_dip_work *dip_work, *tmp_work;
> > > > +	struct dip_user_id  *dip_userid, *tmp_id;
> > > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > > +
> > > > +	dip_ctx = &dip_dev->dip_ctx;
> > > > +	dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n",
> > > > +		dip_ctx->dip_worklist.queue_cnt);
> > > > +
> > > > +	list_for_each_entry_safe(dip_work, tmp_work,
> > > > +				 &dip_ctx->dip_worklist.queue,
> > > > +				 list_entry) {
> > > 
> > > Shouldn't you be holding the mutex for this? Or alternatively, cancel
> > > any outstanding work and move the flush_workqueue()/destroy_workqueue()
> > > up.
> > > 
> > > Similar questions for the other lists we're going through here.
> > > 
> 
> We missed the mutex holding here. I would like to change the codes as following:
> 
> mutex_lock(&dip_ctx->dip_worklist.queuelock);
> list_for_each_entry_safe(dip_work, tmp_work,
> 			 &dip_ctx->dip_worklist.queue,
> 			 list_entry) {
> 	list_del(&dip_work->list_entry);
> 	dip_ctx->dip_worklist.queue_cnt--;
> 	kfree(dip_work);
> }
> mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> 
> I will also modify dip_useridlist and dip_ctx->dip_freebufferlist 
> parts like dip_ctx->dip_worklist.

Seems about right.

Brian

> > > > +		list_del(&dip_work->list_entry);
> > > > +		dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n",
> > > > +			dip_work->frameparams.frame_no);
> > > > +		kfree(dip_work);
> > > > +		dip_ctx->dip_worklist.queue_cnt--;
> > > > +	}
> > > > +
> > > > +	if (dip_ctx->dip_worklist.queue_cnt != 0)
> > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > +			"dip_worklist is not empty (%d)\n",
> > > > +			dip_ctx->dip_worklist.queue_cnt);
> > > > +
> > > > +	list_for_each_entry_safe(dip_userid, tmp_id,
> > > > +				 &dip_ctx->dip_useridlist.queue,
> > > > +				 list_entry) {
> > > > +		list_del(&dip_userid->list_entry);
> > > > +		dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n",
> > > > +			dip_userid->id);
> > > > +		kfree(dip_userid);
> > > > +		dip_ctx->dip_useridlist.queue_cnt--;
> > > > +	}
> > > > +
> > > > +	if (dip_ctx->dip_useridlist.queue_cnt != 0)
> > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > +			"dip_useridlist is not empty (%d)\n",
> > > > +			dip_ctx->dip_useridlist.queue_cnt);
> > > > +
> > > > +	flush_workqueue(dip_ctx->mdpcb_workqueue);
> > > > +	destroy_workqueue(dip_ctx->mdpcb_workqueue);
> > > > +	dip_ctx->mdpcb_workqueue = NULL;
> > > > +
> > > > +	flush_workqueue(dip_ctx->composer_wq);
> > > > +	destroy_workqueue(dip_ctx->composer_wq);
> > > > +	dip_ctx->composer_wq = NULL;
> > > > +
> > > > +	atomic_set(&dip_ctx->num_composing, 0);
> > > > +	atomic_set(&dip_ctx->num_running, 0);
> > > > +
> > > > +	kthread_stop(dip_ctx->dip_runner_thread.thread);
> > > > +	dip_ctx->dip_runner_thread.thread = NULL;
> > > > +
> > > > +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> > > > +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> > > > +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> > > > +
> > > > +	/* All the buffer should be in the freebufferlist when release */
> > > > +	list_for_each_entry_safe(buf, tmpbuf,
> > > > +				 &dip_ctx->dip_freebufferlist.queue,
> > > > +				 list_entry) {
> > > > +		struct sg_table *sgt = &buf->table;
> > > > +
> > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > +			"buf pa (%d): %x\n", i, buf->buffer.pa);
> > > > +		dip_ctx->dip_freebufferlist.queue_cnt--;
> > > > +		dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl,
> > > > +				   sgt->orig_nents,
> > > > +				   DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
> > > > +		sg_free_table(sgt);
> > > > +		list_del(&buf->list_entry);
> > > > +		kfree(buf);
> > > > +		buf = NULL;
> > > > +		i++;
> > > > +	}
> > > > +
> > > > +	if (dip_ctx->dip_freebufferlist.queue_cnt != 0 &&
> > > > +	    i != DIP_SUB_FRM_DATA_NUM)
> > > > +		dev_err(&dip_dev->pdev->dev,
> > > > +			"dip_freebufferlist is not empty (%d/%d)\n",
> > > > +			dip_ctx->dip_freebufferlist.queue_cnt, i);
> > > > +
> > > > +	mutex_destroy(&dip_ctx->dip_useridlist.queuelock);
> > > > +	mutex_destroy(&dip_ctx->dip_worklist.queuelock);
> > > > +	mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > +	mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +

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

Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek DIP driver

[Frederic Chen]

Dear Brian,

I appreciate your comments.


On Wed, 2019-02-27 at 19:24 -0800, Brian Norris wrote:
> Hi Frederic,
> 
> On Sat, Feb 23, 2019 at 02:18:54PM +0800, Frederic Chen wrote:
> > Dear Brian,
> > 
> > I appreciate your comments. I'm really sorry for the delay in responding
> > to the comments due to some mail subscribing failed issue inside my company.
> 
> No problem.
> 
> > On Thu, 2019-02-21 at 21:36 +0800, Jungo Lin wrote:
> > > On Thu, 2019-02-07 at 11:08 -0800, Brian Norris wrote:
> > > > On Fri, Feb 01, 2019 at 07:21:31PM +0800, Frederic Chen wrote:
> 
> > > > > +static void dip_submit_worker(struct work_struct *work)
> > > > > +{
> > > > > +	struct mtk_dip_submit_work *dip_submit_work =
> > > > > +		container_of(work, struct mtk_dip_submit_work, frame_work);
> > > > > +
> > > > > +	struct mtk_dip_hw_ctx  *dip_ctx = dip_submit_work->dip_ctx;
> > > > > +	struct mtk_dip_work *dip_work;
> > > > > +	struct dip_device *dip_dev;
> > > > > +	struct dip_subframe *buf;
> > > > > +	u32 len, num;
> > > > > +	int ret;
> > > > > +
> > > > > +	dip_dev = container_of(dip_ctx, struct dip_device, dip_ctx);
> > > > > +	num  = atomic_read(&dip_ctx->num_composing);
> > > > > +
> > > > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > > > +	dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
> > > > > +				    struct mtk_dip_work, list_entry);
> > > > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > > > 
> > > > I see you grab the head of the list here, but then you release the lock.
> > > > Then you later assume that reference is still valid, throughout this
> > > > function.
> > > > 
> > > > That's usually true, because you only remove/delete entries from this
> > > > list within this same workqueue (at the end of this function). But it's
> > > > not true in dip_release_context() (which doesn't even grab the lock,
> > > > BTW).
> > > > 
> > > > I think there could be several ways to solve this, but judging by how
> > > > this list entry is used...couldn't you just remove it from the list
> > > > here, while holding the lock? Then you only have to kfree() it when
> > > > you're done under the free_work_list label.
> > > > 
> > 
> > I see. I would like to modify the codes as following:
> > 
> > mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> 
> You missed the part where you get the head of the list:
> 
> 	dip_work = list_first_entry(...);
> 
> But otherwise mostly looks OK.
> 
> > dip_work->user_id->num--;
> 
> Why do you need to do that with the queuelock held? Once you remove this
> work item from the list (safely under the lock), shouldn't you be the
> only one accessing it?
> 
> (Note, I don't actually know what that 'num' really means. I'm just
> looking at basic driver mechanics.)
> 

Yes, there is only one user of the dip work at that time.

I made a mistake on the usage of dip_useridlist.queuelock and 
dip_worklist.queuelock here. What I would like to do is to decrease the
total number of the frames of the user, which is protected by
dip_useridlist.queuelock. (user_id->num saves the total number of the
dip frames belongs to a user; the user may be the preview or capture
context.)

On the other hand, the list of dip work is protected by another lock, 
dip_worklist.queuelock.

In regarding to that point, I would like change the codes as following:

mutex_lock(&dip_ctx->dip_worklist.queuelock);
dip_work = list_first_entry(&dip_ctx->dip_worklist.queue,
			    struct mtk_dip_work, list_entry);
list_del(&dip_work->list_entry);
dip_ctx->dip_worklist.queue_cnt--;
len = dip_ctx->dip_worklist.queue_cnt;
mutex_unlock(&dip_ctx->dip_worklist.queuelock);

/* If the frame's user (preview or capture device) */
/* is in stream off state, */
/* return and release the buffers of the frame */
mutex_lock(&dip_ctx->dip_useridlist.queuelock);
if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
	dip_work->user_id->num--;
	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);

	dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
	call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);

	goto free_work_list;
mutex_unlock(&dip_ctx->dip_useridlist.queuelock);

> > list_del(&dip_work->list_entry);
> > dip_ctx->dip_worklist.queue_cnt--;
> > len = dip_ctx->dip_worklist.queue_cnt;
> > mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > 
> > goto free_work_list;
> > 
> > /* ...... */
> > 
> > free_work_list:
> > 	kfree(dip_work);
> > 
> > > > > +
> > > > > +	mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +	if (dip_work->user_id->state == DIP_STATE_STREAMOFF) {
> > > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +
> > > > > +		dip_work->frameparams.state = FRAME_STATE_STREAMOFF;
> > > > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > > > +
> > > > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +		dip_work->user_id->num--;
> > > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > > +			"user_id(%x) is streamoff and num: %d, frame_no(%d) index: %x\n",
> > > > > +			dip_work->user_id->id, dip_work->user_id->num,
> > > > > +			dip_work->frameparams.frame_no,
> > > > > +			dip_work->frameparams.index);
> > > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +
> > > > > +		goto free_work_list;
> > > > > +	}
> > > > > +	mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +
> > > > > +	while (num >= DIP_COMPOSING_MAX_NUM) {
> > > > > +		ret = wait_event_interruptible_timeout
> > > > > +			(dip_ctx->composing_wq,
> > > > > +			 (num < DIP_COMPOSING_MAX_NUM),
> > > > > +			 msecs_to_jiffies(DIP_COMPOSING_WQ_TIMEOUT));
> > > > > +
> > > > > +		if (ret == -ERESTARTSYS)
> > > > > +			dev_err(&dip_dev->pdev->dev,
> > > > > +				"interrupted by a signal!\n");
> > > > > +		else if (ret == 0)
> > > > > +			dev_dbg(&dip_dev->pdev->dev,
> > > > > +				"timeout frame_no(%d), num: %d\n",
> > > > > +				dip_work->frameparams.frame_no, num);
> > > > > +		else
> > > > > +			dev_dbg(&dip_dev->pdev->dev,
> > > > > +				"wakeup frame_no(%d), num: %d\n",
> > > > > +				dip_work->frameparams.frame_no, num);
> > > > > +
> > > > > +		num = atomic_read(&dip_ctx->num_composing);
> > > > > +	};
> > > > > +
> > > > > +	mutex_lock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > > +	if (list_empty(&dip_ctx->dip_freebufferlist.queue)) {
> > > > > +		mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > > +
> > > > > +		dev_err(&dip_dev->pdev->dev,
> > > > > +			"frame_no(%d) index: %x no free buffer: %d\n",
> > > > > +			dip_work->frameparams.frame_no,
> > > > > +			dip_work->frameparams.index,
> > > > > +			dip_ctx->dip_freebufferlist.queue_cnt);
> > > > > +
> > > > > +		/* Call callback to notify V4L2 common framework
> > > > > +		 * for failure of enqueue
> > > > > +		 */
> > > > > +		dip_work->frameparams.state = FRAME_STATE_ERROR;
> > > > > +		call_mtk_dip_ctx_finish(dip_dev, &dip_work->frameparams);
> > > > > +
> > > > > +		mutex_lock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +		dip_work->user_id->num--;
> > > > > +		mutex_unlock(&dip_ctx->dip_useridlist.queuelock);
> > > > > +
> > > > > +		goto free_work_list;
> > > > > +	}
> > > > > +
> > > > > +	buf = list_first_entry(&dip_ctx->dip_freebufferlist.queue,
> > > > > +			       struct dip_subframe,
> > > > > +			       list_entry);
> > > > > +	list_del(&buf->list_entry);
> > > > > +	dip_ctx->dip_freebufferlist.queue_cnt--;
> > > > > +	mutex_unlock(&dip_ctx->dip_freebufferlist.queuelock);
> > > > > +
> > > > > +	mutex_lock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > > +	list_add_tail(&buf->list_entry, &dip_ctx->dip_usedbufferlist.queue);
> > > > > +	dip_ctx->dip_usedbufferlist.queue_cnt++;
> > > > > +	mutex_unlock(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > > +
> > > > > +	memcpy(&dip_work->frameparams.subfrm_data,
> > > > > +	       &buf->buffer, sizeof(buf->buffer));
> > > > > +
> > > > > +	memset((char *)buf->buffer.va, 0, DIP_SUB_FRM_SZ);
> > > > > +
> > > > > +	memcpy(&dip_work->frameparams.config_data,
> > > > > +	       &buf->config_data, sizeof(buf->config_data));
> > > > > +
> > > > > +	memset((char *)buf->config_data.va, 0, DIP_COMP_SZ);
> > > > > +
> > > > > +	if (dip_work->frameparams.tuning_data.pa == 0) {
> > > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > > +			"frame_no(%d) has no tuning_data\n",
> > > > > +			dip_work->frameparams.frame_no);
> > > > > +
> > > > > +		memcpy(&dip_work->frameparams.tuning_data,
> > > > > +		       &buf->tuning_buf, sizeof(buf->tuning_buf));
> > > > > +
> > > > > +		memset((char *)buf->tuning_buf.va, 0, DIP_TUNING_SZ);
> > > > > +		/* When user enqueued without tuning buffer,
> > > > > +		 * it would use driver internal buffer.
> > > > > +		 * So, tuning_data.va should be 0
> > > > > +		 */
> > > > > +		dip_work->frameparams.tuning_data.va = 0;
> > > > > +	}
> > > > > +
> > > > > +	dip_work->frameparams.drv_data = (u64)dip_ctx;
> > > > > +	dip_work->frameparams.state = FRAME_STATE_COMPOSING;
> > > > > +
> > > > > +	memcpy((void *)buf->frameparam.va, &dip_work->frameparams,
> > > > > +	       sizeof(dip_work->frameparams));
> > > > > +
> > > > > +	dip_send(dip_ctx->vpu_pdev, IPI_DIP_FRAME,
> > > > > +		 (void *)&dip_work->frameparams,
> > > > > +		 sizeof(dip_work->frameparams), 0);
> > > > > +	num = atomic_inc_return(&dip_ctx->num_composing);
> > > > > +
> > > > > +free_work_list:
> > > > > +
> > > > > +	mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > > > > +	list_del(&dip_work->list_entry);
> > > > > +	dip_ctx->dip_worklist.queue_cnt--;
> > > > > +	len = dip_ctx->dip_worklist.queue_cnt;
> > > > > +	mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > > > > +
> > > > > +	dev_dbg(&dip_dev->pdev->dev,
> > > > > +		"frame_no(%d) index: %x, worklist count: %d, composing num: %d\n",
> > > > > +		dip_work->frameparams.frame_no, dip_work->frameparams.index,
> > > > > +		len, num);
> > > > > +
> > > > > +	kfree(dip_work);
> > > > > +}
> 
> 
> > > > > +int dip_release_context(struct dip_device *dip_dev)
> > > > 
> > > > Should be static.
> > > > 
> > 
> > I will change it to static.
> > 
> > > > > +{
> > > > > +	u32 i = 0;
> > > > > +	struct dip_subframe *buf, *tmpbuf;
> > > > > +	struct mtk_dip_work *dip_work, *tmp_work;
> > > > > +	struct dip_user_id  *dip_userid, *tmp_id;
> > > > > +	struct mtk_dip_hw_ctx *dip_ctx;
> > > > > +
> > > > > +	dip_ctx = &dip_dev->dip_ctx;
> > > > > +	dev_dbg(&dip_dev->pdev->dev, "composer work queue = %d\n",
> > > > > +		dip_ctx->dip_worklist.queue_cnt);
> > > > > +
> > > > > +	list_for_each_entry_safe(dip_work, tmp_work,
> > > > > +				 &dip_ctx->dip_worklist.queue,
> > > > > +				 list_entry) {
> > > > 
> > > > Shouldn't you be holding the mutex for this? Or alternatively, cancel
> > > > any outstanding work and move the flush_workqueue()/destroy_workqueue()
> > > > up.
> > > > 
> > > > Similar questions for the other lists we're going through here.
> > > > 
> > 
> > We missed the mutex holding here. I would like to change the codes as following:
> > 
> > mutex_lock(&dip_ctx->dip_worklist.queuelock);
> > list_for_each_entry_safe(dip_work, tmp_work,
> > 			 &dip_ctx->dip_worklist.queue,
> > 			 list_entry) {
> > 	list_del(&dip_work->list_entry);
> > 	dip_ctx->dip_worklist.queue_cnt--;
> > 	kfree(dip_work);
> > }
> > mutex_unlock(&dip_ctx->dip_worklist.queuelock);
> > 
> > I will also modify dip_useridlist and dip_ctx->dip_freebufferlist 
> > parts like dip_ctx->dip_worklist.
> 
> Seems about right.
> 
> Brian
> 
> > > > > +		list_del(&dip_work->list_entry);
> > > > > +		dev_dbg(&dip_dev->pdev->dev, "dip work frame no: %d\n",
> > > > > +			dip_work->frameparams.frame_no);
> > > > > +		kfree(dip_work);
> > > > > +		dip_ctx->dip_worklist.queue_cnt--;
> > > > > +	}
> > > > > +
> > > > > +	if (dip_ctx->dip_worklist.queue_cnt != 0)
> > > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > > +			"dip_worklist is not empty (%d)\n",
> > > > > +			dip_ctx->dip_worklist.queue_cnt);
> > > > > +
> > > > > +	list_for_each_entry_safe(dip_userid, tmp_id,
> > > > > +				 &dip_ctx->dip_useridlist.queue,
> > > > > +				 list_entry) {
> > > > > +		list_del(&dip_userid->list_entry);
> > > > > +		dev_dbg(&dip_dev->pdev->dev, "dip user id: %x\n",
> > > > > +			dip_userid->id);
> > > > > +		kfree(dip_userid);
> > > > > +		dip_ctx->dip_useridlist.queue_cnt--;
> > > > > +	}
> > > > > +
> > > > > +	if (dip_ctx->dip_useridlist.queue_cnt != 0)
> > > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > > +			"dip_useridlist is not empty (%d)\n",
> > > > > +			dip_ctx->dip_useridlist.queue_cnt);
> > > > > +
> > > > > +	flush_workqueue(dip_ctx->mdpcb_workqueue);
> > > > > +	destroy_workqueue(dip_ctx->mdpcb_workqueue);
> > > > > +	dip_ctx->mdpcb_workqueue = NULL;
> > > > > +
> > > > > +	flush_workqueue(dip_ctx->composer_wq);
> > > > > +	destroy_workqueue(dip_ctx->composer_wq);
> > > > > +	dip_ctx->composer_wq = NULL;
> > > > > +
> > > > > +	atomic_set(&dip_ctx->num_composing, 0);
> > > > > +	atomic_set(&dip_ctx->num_running, 0);
> > > > > +
> > > > > +	kthread_stop(dip_ctx->dip_runner_thread.thread);
> > > > > +	dip_ctx->dip_runner_thread.thread = NULL;
> > > > > +
> > > > > +	atomic_set(&dip_ctx->dip_user_cnt, 0);
> > > > > +	atomic_set(&dip_ctx->dip_stream_cnt, 0);
> > > > > +	atomic_set(&dip_ctx->dip_enque_cnt, 0);
> > > > > +
> > > > > +	/* All the buffer should be in the freebufferlist when release */
> > > > > +	list_for_each_entry_safe(buf, tmpbuf,
> > > > > +				 &dip_ctx->dip_freebufferlist.queue,
> > > > > +				 list_entry) {
> > > > > +		struct sg_table *sgt = &buf->table;
> > > > > +
> > > > > +		dev_dbg(&dip_dev->pdev->dev,
> > > > > +			"buf pa (%d): %x\n", i, buf->buffer.pa);
> > > > > +		dip_ctx->dip_freebufferlist.queue_cnt--;
> > > > > +		dma_unmap_sg_attrs(&dip_dev->pdev->dev, sgt->sgl,
> > > > > +				   sgt->orig_nents,
> > > > > +				   DMA_BIDIRECTIONAL, DMA_ATTR_SKIP_CPU_SYNC);
> > > > > +		sg_free_table(sgt);
> > > > > +		list_del(&buf->list_entry);
> > > > > +		kfree(buf);
> > > > > +		buf = NULL;
> > > > > +		i++;
> > > > > +	}
> > > > > +
> > > > > +	if (dip_ctx->dip_freebufferlist.queue_cnt != 0 &&
> > > > > +	    i != DIP_SUB_FRM_DATA_NUM)
> > > > > +		dev_err(&dip_dev->pdev->dev,
> > > > > +			"dip_freebufferlist is not empty (%d/%d)\n",
> > > > > +			dip_ctx->dip_freebufferlist.queue_cnt, i);
> > > > > +
> > > > > +	mutex_destroy(&dip_ctx->dip_useridlist.queuelock);
> > > > > +	mutex_destroy(&dip_ctx->dip_worklist.queuelock);
> > > > > +	mutex_destroy(&dip_ctx->dip_usedbufferlist.queuelock);
> > > > > +	mutex_destroy(&dip_ctx->dip_freebufferlist.queuelock);
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +

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 V0 0/7] media: platform: Add support for Digital Image Processing (DIP) on mt8183 SoC

[Hans Verkuil]

Hi Frederic Chen,

On 2/1/19 12:21 PM, Frederic Chen wrote:
> Hello,
> 
> This is the first version of the RFC patch series adding Digital Image
> Processing (DIP) driver on Mediatek mt8183 SoC, which will be used in camera
> features on CrOS application. It belongs to the first 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 on
> the overall structure of the driver.
> 
> 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.
> 
> 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 image 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.

Just a high-level comment before you post the next version of this series
and for your "platform: Add support for ISP Pass 1 on mt8183 SoC" series

Please compile the latest version of v4l2-compliance (part of
git://linuxtv.org/v4l-utils.git) and run it against your driver:

v4l2-compliance -m /dev/mediaX

Whenever you post a new version of this series, please do a 'git pull' of
the v4l-utils repo, recompile and retest with v4l2-compliance and post the
test results in the cover letter.

Obviously, there should be no FAILs and probably no warnings.

I suspect that streaming (e.g. adding the -s10 option to v4l2-compliance)
might not work since v4l2-compliance doesn't know about the meta data
formats. But give it a try and see what happens :-)

Regards,

	Hans

> 
> The overall file structure of the DIP driver is as following:
> 
> * mtk_dip-dev-ctx-core.c: Implements common software flow of DIP driver.
>   DIP driver supports two or more software contexts. For example, context 0 is
>   created for preview path and context 1 is for capture path. Both the two
>   contexts share the same DIP hardware to process the images.
> * mtk_dip-v4l2.c: Static DIP contexts configuration.
> * mtk_dip.c: Controls the hardware flow.
> * mtk_dip-dev.c: Implements context-independent flow.
> * mtk_dip-ctrl.c: Handles the HW ctrl request from userspace.
> * mtk_dip-smem-drv.c: Provides the shared memory management required operation.
>   We reserved a memory region for the co-processor and DIP to exchange the
>   tuning and hardware configuration data.
> * mtk_dip-v4l2-util.c: Implements V4L2 and vb2 ops.
> 
> Frederic Chen (7):
>   [media] dt-bindings: mt8183: Add binding for DIP shared memory
>   dts: arm64: mt8183: Add DIP shared memory node
>   [media] dt-bindings: mt8183: Added DIP-SMEM dt-bindings
>   [media] dt-bindings: mt8183: Added DIP dt-bindings
>   dts: arm64: mt8183: Add DIP nodes
>   media: platform: Add Mediatek DIP driver KConfig
>   [media] platform: mtk-isp: Add Mediatek DIP driver
> 
>  .../bindings/media/mediatek,dip_smem.txt           |   29 +
>  .../bindings/media/mediatek,mt8183-dip.txt         |   35 +
>  .../mediatek,reserve-memory-dip_smem.txt           |   45 +
>  arch/arm64/boot/dts/mediatek/mt8183.dtsi           |   36 +
>  drivers/media/platform/Kconfig                     |    2 +
>  drivers/media/platform/mtk-isp/Kconfig             |   21 +
>  drivers/media/platform/mtk-isp/Makefile            |   18 +
>  drivers/media/platform/mtk-isp/isp_50/Makefile     |   17 +
>  drivers/media/platform/mtk-isp/isp_50/dip/Makefile |   35 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-core.h     |  188 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c     |  173 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h     |   43 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h      |  319 ++++
>  .../mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c      | 1643 ++++++++++++++++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.c      |  374 +++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-dev.h      |  191 +++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem-drv.c |  452 ++++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-smem.h     |   25 +
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c         | 1000 ++++++++++++
>  .../mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h         |   38 +
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c     |  292 ++++
>  .../platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h     |   60 +
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.c    | 1385 +++++++++++++++++
>  .../media/platform/mtk-isp/isp_50/dip/mtk_dip.h    |   93 ++
>  24 files changed, 6514 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,dip_smem.txt
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-dip.txt
>  create mode 100644 Documentation/devicetree/bindings/reserved-memory/mediatek,reserve-memory-dip_smem.txt
>  create mode 100644 drivers/media/platform/mtk-isp/Kconfig
>  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-core.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctrl.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-ctx.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-dev-ctx-core.c
>  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-smem-drv.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-smem.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2-util.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip-v4l2.h
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.c
>  create mode 100644 drivers/media/platform/mtk-isp/isp_50/dip/mtk_dip.h
> 


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