[RFC v2 0/4] media: mediatek: support mdp3 on mt8183 platform

[RFC v2 0/4] media: mediatek: support mdp3 on mt8183 platform

[Daoyuan Huang]

Hi,

This is the first version of RFC patch for Media Data Path 3 (MDP3),
MDP3 is used for scaling and color format conversion.
support using GCE to write register in critical time limitation.
support V4L2 m2m device control.

Changes since v1:
- modify code for CMDQ v3 API support
- EC ipi cmd migration
- fix compliance test fail item (m2m cmd with -f)
due to there is two problem in runing all format(-f) cmd:
1. out of memory before test complete
	Due to capture buffer mmap (refcount + 1) after reqbuf but seems
	no corresponding munmap called before device close.
	There are total 12XX items(formats) in format test and each format
	alloc 8 capture/output buffers.
2. unceasingly captureBufs() (randomly)
	Seems the break statement didn't catch the count == 0 situation:
	In v4l2-test-buffers.cpp, function: captureBufs()
			...
			count--;
			if (!node->is_m2m && !count)
				break;
	Log is as attachment

I will paste the test result with problem part in another e-mail



---
Based on v5.0-rc1 and these series:
device tree:
http://lists.infradead.org/pipermail/linux-mediatek/2019-February/017570.html
clock control:
http://lists.infradead.org/pipermail/linux-mediatek/2019-February/017320.html
system control processor (SCP):
http://lists.infradead.org/pipermail/linux-mediatek/2019-February/017774.html
global command engine (GCE):
http://lists.infradead.org/pipermail/linux-mediatek/2019-January/017143.html
---

daoyuan huang (4):
  dt-binding: mt8183: Add Mediatek MDP3 dt-bindings
  dts: arm64: mt8183: Add Mediatek MDP3 nodes
  media: platform: Add Mediatek MDP3 driver KConfig
  media: platform: mtk-mdp3: Add Mediatek MDP3 driver

 .../bindings/media/mediatek,mt8183-mdp3.txt   |  217 +++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      |  173 +++
 drivers/media/platform/Kconfig                |   18 +
 drivers/media/platform/Makefile               |    2 +
 drivers/media/platform/mtk-mdp3/Makefile      |    9 +
 drivers/media/platform/mtk-mdp3/isp_reg.h     |   38 +
 .../media/platform/mtk-mdp3/mdp-platform.h    |   67 +
 .../media/platform/mtk-mdp3/mdp_reg_ccorr.h   |   76 +
 .../media/platform/mtk-mdp3/mdp_reg_rdma.h    |  207 +++
 drivers/media/platform/mtk-mdp3/mdp_reg_rsz.h |  110 ++
 .../media/platform/mtk-mdp3/mdp_reg_wdma.h    |  126 ++
 .../media/platform/mtk-mdp3/mdp_reg_wrot.h    |  116 ++
 .../media/platform/mtk-mdp3/mmsys_config.h    |  189 +++
 drivers/media/platform/mtk-mdp3/mmsys_mutex.h |   36 +
 .../media/platform/mtk-mdp3/mmsys_reg_base.h  |   39 +
 drivers/media/platform/mtk-mdp3/mtk-img-ipi.h |  282 ++++
 .../media/platform/mtk-mdp3/mtk-mdp3-cmdq.c   |  442 ++++++
 .../media/platform/mtk-mdp3/mtk-mdp3-cmdq.h   |   57 +
 .../media/platform/mtk-mdp3/mtk-mdp3-comp.c   | 1325 +++++++++++++++++
 .../media/platform/mtk-mdp3/mtk-mdp3-comp.h   |  177 +++
 .../media/platform/mtk-mdp3/mtk-mdp3-core.c   |  256 ++++
 .../media/platform/mtk-mdp3/mtk-mdp3-core.h   |   88 ++
 .../media/platform/mtk-mdp3/mtk-mdp3-m2m.c    |  823 ++++++++++
 .../media/platform/mtk-mdp3/mtk-mdp3-m2m.h    |   52 +
 .../media/platform/mtk-mdp3/mtk-mdp3-regs.c   |  757 ++++++++++
 .../media/platform/mtk-mdp3/mtk-mdp3-regs.h   |  386 +++++
 .../media/platform/mtk-mdp3/mtk-mdp3-vpu.c    |  277 ++++
 .../media/platform/mtk-mdp3/mtk-mdp3-vpu.h    |   90 ++
 28 files changed, 6435 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
 create mode 100644 drivers/media/platform/mtk-mdp3/Makefile
 create mode 100644 drivers/media/platform/mtk-mdp3/isp_reg.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp-platform.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_ccorr.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_rdma.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_rsz.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_wdma.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_wrot.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_config.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_mutex.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_reg_base.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-img-ipi.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-comp.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-comp.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-core.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-core.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-m2m.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-m2m.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-regs.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-regs.h
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-vpu.c
 create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-vpu.h

-- 
2.18.0




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

[RFC v2 1/4] dt-binding: mt8183: Add Mediatek MDP3 dt-bindings

[Daoyuan Huang]

From: daoyuan huang <daoyuan.huang@mediatek.com>

This patch adds DT binding document for Media Data Path 3 (MDP3)
a unit in multimedia system used for scaling and color format convert.

Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
---
 .../bindings/media/mediatek,mt8183-mdp3.txt   | 217 ++++++++++++++++++
 1 file changed, 217 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt

diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt b/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
new file mode 100644
index 000000000000..cf3e808b7146
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
@@ -0,0 +1,217 @@
+* Mediatek Media Data Path 3
+
+Media Data Path 3 (MDP3) is used for scaling and color space conversion.
+
+Required properties (controller node):
+- compatible: "mediatek,mt8183-mdp"
+- mediatek,scp: the node of system control processor (SCP), using the
+  remoteproc & rpmsg framework, see
+  Documentation/devicetree/bindings/remoteproc/mtk,scp.txt for details.
+- mediatek,mmsys: the node of mux(multiplexer) controller for HW connections.
+- mediatek,mm-mutex: the node of sof(start of frame) signal controller.
+- mediatek,mailbox-gce: the node of global command engine (GCE), used to
+  read/write registers with critical time limitation, see
+  Documentation/devicetree/bindings/mailbox/mtk-gce.txt for details.
+- mboxes: mailbox number used to communicate with GCE.
+- gce-subsys: sub-system id corresponding to the register address.
+- gce-event-names: in use event name list, used to correspond to event IDs.
+- gce-events: in use event IDs list, all IDs are defined in
+  'dt-bindings/gce/mt8183-gce.h'.
+
+Required properties (all function blocks, child node):
+- compatible: Should be one of
+        "mediatek,mt8183-mdp-rdma"  - read DMA
+        "mediatek,mt8183-mdp-rsz"   - resizer
+        "mediatek,mt8183-mdp-wdma"  - write DMA
+        "mediatek,mt8183-mdp-wrot"  - write DMA with rotation
+        "mediatek,mt8183-mdp-ccorr" - color correction with 3X3 matrix
+- reg: Physical base address and length of the function block register space
+- clocks: device clocks, see
+  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- power-domains: a phandle to the power domain, see
+  Documentation/devicetree/bindings/power/power_domain.txt for details.
+- mediatek,mdp-id: HW index to distinguish same functionality modules.
+
+Required properties (DMA function blocks, child node):
+- compatible: Should be one of
+        "mediatek,mt8183-mdp-rdma"
+        "mediatek,mt8183-mdp-wdma"
+        "mediatek,mt8183-mdp-wrot"
+- 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.
+
+Required properties (input path selection node):
+- compatible:
+        "mediatek,mt8183-mdp-dl"    - MDP direct link input source selection
+- reg: Physical base address and length of the function block register space
+- clocks: device clocks, see
+  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
+- mediatek,mdp-id: HW index to distinguish same functionality modules.
+
+Required properties (ISP PASS2 (DIP) module path selection node):
+- compatible:
+        "mediatek,mt8183-mdp-imgi"  - input DMA of ISP PASS2 (DIP) module for raw image input
+- reg: Physical base address and length of the function block register space
+- mediatek,mdp-id: HW index to distinguish same functionality modules.
+
+Required properties (SW node):
+- compatible: Should be one of
+        "mediatek,mt8183-mdp-exto"  - output DMA of ISP PASS2 (DIP) module for yuv image output
+        "mediatek,mt8183-mdp-path"  - MDP output path selection
+- mediatek,mdp-id: HW index to distinguish same functionality modules.
+
+Example:
+		mdp_camin@14000000 {
+			compatible = "mediatek,mt8183-mdp-dl";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14000000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_DL_TXCK>,
+				<&mmsys CLK_MM_MDP_DL_RX>;
+		};
+
+		mdp_camin2@14000000 {
+			compatible = "mediatek,mt8183-mdp-dl";
+			mediatek,mdp-id = <1>;
+			reg = <0 0x14000000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_IPU_DL_TXCK>,
+				<&mmsys CLK_MM_IPU_DL_RX>;
+		};
+
+		mdp_rdma0: mdp_rdma0@14001000 {
+			compatible = "mediatek,mt8183-mdp-rdma", "mediatek,mt8183-mdp3";
+			mediatek,scp = <&scp>;
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14001000 0 0x1000>;
+			power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>;
+			clocks = <&mmsys CLK_MM_MDP_RDMA0>,
+				<&mmsys CLK_MM_MDP_RSZ1>;
+			iommus = <&iommu M4U_PORT_MDP_RDMA0>;
+			mediatek,larb = <&larb0>;
+			mediatek,mmsys = <&mmsys>;
+			mediatek,mm-mutex = <&mutex>;
+			mediatek,mailbox-gce = <&gce>;
+			mboxes = <&gce 20 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 21 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 22 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 23 0 CMDQ_THR_PRIO_LOWEST>;
+			gce-subsys = <&gce 0x14000000 SUBSYS_1400XXXX>,
+				<&gce 0x14010000 SUBSYS_1401XXXX>,
+				<&gce 0x14020000 SUBSYS_1402XXXX>,
+				<&gce 0x15020000 SUBSYS_1502XXXX>;
+			gce-event-names = "rdma0_sof",
+				"rsz0_sof",
+				"rsz1_sof",
+				"tdshp0_sof",
+				"wrot0_sof",
+				"wdma0_sof",
+				"rdma0_done",
+				"wrot0_done",
+				"wdma0_done",
+				"isp_p2_0_done",
+				"isp_p2_1_done",
+				"isp_p2_2_done",
+				"isp_p2_3_done",
+				"isp_p2_4_done",
+				"isp_p2_5_done",
+				"isp_p2_6_done",
+				"isp_p2_7_done",
+				"isp_p2_8_done",
+				"isp_p2_9_done",
+				"isp_p2_10_done",
+				"isp_p2_11_done",
+				"isp_p2_12_done",
+				"isp_p2_13_done",
+				"isp_p2_14_done",
+				"wpe_done",
+				"wpe_b_done";
+			gce-events = <&gce CMDQ_EVENT_MDP_RDMA0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RSZ0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RSZ1_SOF>,
+				<&gce CMDQ_EVENT_MDP_TDSHP_SOF>,
+				<&gce CMDQ_EVENT_MDP_WROT0_SOF>,
+				<&gce CMDQ_EVENT_MDP_WDMA0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RDMA0_EOF>,
+				<&gce CMDQ_EVENT_MDP_WROT0_EOF>,
+				<&gce CMDQ_EVENT_MDP_WDMA0_EOF>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_0>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_1>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_2>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_3>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_4>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_5>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_6>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_7>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_8>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_9>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_10>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_11>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_12>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_13>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_14>,
+				<&gce CMDQ_EVENT_WPE_A_DONE>,
+				<&gce CMDQ_EVENT_SPE_B_DONE>;
+		};
+
+		mdp_imgi@15020000 {
+			compatible = "mediatek,mt8183-mdp-imgi";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x15020000 0 0x1000>;
+		};
+
+		mdp_img2o@15020000 {
+			compatible = "mediatek,mt8183-mdp-exto";
+			mediatek,mdp-id = <1>;
+		};
+
+		mdp_rsz0: mdp_rsz0@14003000 {
+			compatible = "mediatek,mt8183-mdp-rsz";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14003000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_RSZ0>;
+		};
+
+		mdp_rsz1: mdp_rsz1@14004000 {
+			compatible = "mediatek,mt8183-mdp-rsz";
+			mediatek,mdp-id = <1>;
+			reg = <0 0x14004000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_RSZ1>;
+		};
+
+		mdp_wrot0: mdp_wrot0@14005000 {
+			compatible = "mediatek,mt8183-mdp-wrot";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14005000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_WROT0>;
+			iommus = <&iommu M4U_PORT_MDP_WROT0>;
+			mediatek,larb = <&larb0>;
+		};
+
+		mdp_path0_sout@14005000 {
+			compatible = "mediatek,mt8183-mdp-path";
+			mediatek,mdp-id = <0>;
+		};
+
+		mdp_wdma: mdp_wdma@14006000 {
+			compatible = "mediatek,mt8183-mdp-wdma";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14006000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_WDMA0>;
+			iommus = <&iommu M4U_PORT_MDP_WDMA0>;
+			mediatek,larb = <&larb0>;
+		};
+
+		mdp_path1_sout@14006000 {
+			compatible = "mediatek,mt8183-mdp-path";
+			mediatek,mdp-id = <1>;
+		};
+
+		mdp_ccorr: mdp_ccorr@1401c000 {
+			compatible = "mediatek,mt8183-mdp-ccorr";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x1401c000 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_CCORR>;
+		};
-- 
2.18.0


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

[RFC v2 2/4] dts: arm64: mt8183: Add Mediatek MDP3 nodes

[Daoyuan Huang]

From: daoyuan huang <daoyuan.huang@mediatek.com>

Add device nodes for Media Data Path 3 (MDP3) modules.

Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8183.dtsi | 173 +++++++++++++++++++++++
 1 file changed, 173 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index c3a516e63141..e3e4c3bcd7b6 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -418,14 +418,187 @@
 		mmsys: syscon@14000000 {
 			compatible = "mediatek,mt8183-mmsys", "syscon";
 			reg = <0 0x14000000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0 0x1000>;
 			#clock-cells = <1>;
 		};
 
+		mdp_camin@14000000 {
+			compatible = "mediatek,mt8183-mdp-dl";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14000000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_DL_TXCK>,
+				<&mmsys CLK_MM_MDP_DL_RX>;
+		};
+
+		mdp_camin2@14000000 {
+			compatible = "mediatek,mt8183-mdp-dl";
+			mediatek,mdp-id = <1>;
+			reg = <0 0x14000000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0 0x1000>;
+			clocks = <&mmsys CLK_MM_IPU_DL_TXCK>,
+				<&mmsys CLK_MM_IPU_DL_RX>;
+		};
+
+		mdp_rdma0: mdp_rdma0@14001000 {
+			compatible = "mediatek,mt8183-mdp-rdma",
+				     "mediatek,mt8183-mdp3";
+			mediatek,vpu = <&vpu>;
+			mediatek,scp = <&scp>;
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14001000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0x1000 0x1000>;
+			power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>;
+			clocks = <&mmsys CLK_MM_MDP_RDMA0>,
+				<&mmsys CLK_MM_MDP_RSZ1>;
+			iommus = <&iommu M4U_PORT_MDP_RDMA0>;
+			mediatek,larb = <&larb0>;
+			mediatek,mmsys = <&mmsys>;
+			mediatek,mm-mutex = <&mutex>;
+			mediatek,mailbox-gce = <&gce>;
+			mboxes = <&gce 20 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 21 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 22 0 CMDQ_THR_PRIO_LOWEST>,
+				<&gce 23 0 CMDQ_THR_PRIO_LOWEST>;
+			gce-subsys = <&gce 0x14000000 SUBSYS_1400XXXX>,
+				<&gce 0x14010000 SUBSYS_1401XXXX>,
+				<&gce 0x14020000 SUBSYS_1402XXXX>,
+				<&gce 0x15020000 SUBSYS_1502XXXX>;
+			mediatek,gce-event-names = "rdma0_sof",
+				"rsz0_sof",
+				"rsz1_sof",
+				"tdshp0_sof",
+				"wrot0_sof",
+				"wdma0_sof",
+				"rdma0_done",
+				"wrot0_done",
+				"wdma0_done",
+				"isp_p2_0_done",
+				"isp_p2_1_done",
+				"isp_p2_2_done",
+				"isp_p2_3_done",
+				"isp_p2_4_done",
+				"isp_p2_5_done",
+				"isp_p2_6_done",
+				"isp_p2_7_done",
+				"isp_p2_8_done",
+				"isp_p2_9_done",
+				"isp_p2_10_done",
+				"isp_p2_11_done",
+				"isp_p2_12_done",
+				"isp_p2_13_done",
+				"isp_p2_14_done",
+				"wpe_done",
+				"wpe_b_done";
+			mediatek,gce-events = <&gce CMDQ_EVENT_MDP_RDMA0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RSZ0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RSZ1_SOF>,
+				<&gce CMDQ_EVENT_MDP_TDSHP_SOF>,
+				<&gce CMDQ_EVENT_MDP_WROT0_SOF>,
+				<&gce CMDQ_EVENT_MDP_WDMA0_SOF>,
+				<&gce CMDQ_EVENT_MDP_RDMA0_EOF>,
+				<&gce CMDQ_EVENT_MDP_WROT0_EOF>,
+				<&gce CMDQ_EVENT_MDP_WDMA0_EOF>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_0>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_1>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_2>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_3>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_4>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_5>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_6>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_7>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_8>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_9>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_10>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_11>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_12>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_13>,
+				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_14>,
+				<&gce CMDQ_EVENT_WPE_A_DONE>,
+				<&gce CMDQ_EVENT_SPE_B_DONE>;
+		};
+
+		mdp_imgi@15020000 {
+			compatible = "mediatek,mt8183-mdp-imgi";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x15020000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1502XXXX 0 0x1000>;
+		};
+
+		mdp_img2o@15020000 {
+			compatible = "mediatek,mt8183-mdp-exto";
+			mediatek,mdp-id = <1>;
+		};
+
+		mdp_rsz0: mdp_rsz0@14003000 {
+			compatible = "mediatek,mt8183-mdp-rsz";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14003000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0x3000 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_RSZ0>;
+		};
+
+		mdp_rsz1: mdp_rsz1@14004000 {
+			compatible = "mediatek,mt8183-mdp-rsz";
+			mediatek,mdp-id = <1>;
+			reg = <0 0x14004000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0x4000 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_RSZ1>;
+		};
+
+		mdp_wrot0: mdp_wrot0@14005000 {
+			compatible = "mediatek,mt8183-mdp-wrot";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14005000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0x5000 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_WROT0>;
+			iommus = <&iommu M4U_PORT_MDP_WROT0>;
+			mediatek,larb = <&larb0>;
+		};
+
+		mdp_path0_sout@14005000 {
+			compatible = "mediatek,mt8183-mdp-path";
+			mediatek,mdp-id = <0>;
+		};
+
+		mdp_wdma: mdp_wdma@14006000 {
+			compatible = "mediatek,mt8183-mdp-wdma";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x14006000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1400XXXX 0x6000 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_WDMA0>;
+			iommus = <&iommu M4U_PORT_MDP_WDMA0>;
+			mediatek,larb = <&larb0>;
+		};
+
+		mdp_path1_sout@14006000 {
+			compatible = "mediatek,mt8183-mdp-path";
+			mediatek,mdp-id = <1>;
+		};
+
 		smi_common: smi@14019000 {
 			compatible = "mediatek,mt8183-smi-common", "syscon";
 			reg = <0 0x14019000 0 0x1000>;
 		};
 
+		mdp_ccorr: mdp_ccorr@1401c000 {
+			compatible = "mediatek,mt8183-mdp-ccorr";
+			mediatek,mdp-id = <0>;
+			reg = <0 0x1401c000 0 0x1000>;
+			mediatek,gce-client-reg =
+					<&gce SUBSYS_1401XXXX 0xc000 0x1000>;
+			clocks = <&mmsys CLK_MM_MDP_CCORR>;
+		};
+
 		imgsys: syscon@15020000 {
 			compatible = "mediatek,mt8183-imgsys", "syscon";
 			reg = <0 0x15020000 0 0x1000>;
-- 
2.18.0


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

[RFC v2 3/4] media: platform: Add Mediatek MDP3 driver KConfig

[Daoyuan Huang]

From: daoyuan huang <daoyuan.huang@mediatek.com>

This patch adds Kconfig for Mediatek Media Data Path 3 (MDP3)
driver. MDP3 is used to do scaling and color format conversion.

Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
---
 drivers/media/platform/Kconfig  | 18 ++++++++++++++++++
 drivers/media/platform/Makefile |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index a505e9f5a1e2..6e8b594a1569 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -244,6 +244,24 @@ config VIDEO_MEDIATEK_MDP
 	    To compile this driver as a module, choose M here: the
 	    module will be called mtk-mdp.
 
+config VIDEO_MEDIATEK_MDP3
+	tristate "Mediatek MDP v3 driver"
+	depends on MTK_IOMMU || COMPILE_TEST
+	depends on VIDEO_DEV && VIDEO_V4L2
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	depends on HAS_DMA
+	select VIDEOBUF2_DMA_CONTIG
+	select V4L2_MEM2MEM_DEV
+	select VIDEO_MEDIATEK_VPU
+	select MTK_CMDQ
+	default n
+	help
+	    It is a v4l2 driver and present in Mediatek MT8183 SoC.
+	    The driver supports for scaling and color space conversion.
+
+	    To compile this driver as a module, choose M here: the
+	    module will be called mtk-mdp3.
+
 config VIDEO_MEDIATEK_VCODEC
 	tristate "Mediatek Video Codec driver"
 	depends on MTK_IOMMU || COMPILE_TEST
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index e6deb2597738..1e2cb92bc6da 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -92,6 +92,8 @@ obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC)	+= mtk-vcodec/
 
 obj-$(CONFIG_VIDEO_MEDIATEK_MDP)	+= mtk-mdp/
 
+obj-$(CONFIG_VIDEO_MEDIATEK_MDP3)	+= mtk-mdp3/
+
 obj-$(CONFIG_VIDEO_MEDIATEK_JPEG)	+= mtk-jpeg/
 
 obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss/
-- 
2.18.0


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

Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Tomasz Figa]

Hi Daoyuan,

On Thu, May 16, 2019 at 11:23:32AM +0800, Daoyuan Huang wrote:
> From: daoyuan huang <daoyuan.huang@mediatek.com>
> 
> This patch adds driver for Media Data Path 3 (MDP3).
> Each modules' related operation control is sited in mtk-mdp3-comp.c
> Each modules' register table is defined in file with "mdp_reg_"
> and "mmsys_" prefix
> GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> Probe, power, suspend/resume, system level functions are defined in
> mtk-mdp3-core.c

Thanks for the patch. Some initial comments inline.

[snip]
> +void mdp_comp_clock_on(struct device *dev, struct mdp_comp *comp)
> +{
> +	int i, err;
> +
> +	if (comp->larb_dev) {
> +		err = pm_runtime_get_sync(comp->larb_dev);
> +		if (err < 0)
> +			dev_err(dev,
> +				"Failed to get larb, err %d. type:%d id:%d\n",
> +				err, comp->type, comp->id);

There is no need to make this conditional. Actually, we always need to grab
a runtime PM enable count, otherwise the power domain would power off (if
this SoC has gate'able power domains).

> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> +		if (IS_ERR(comp->clks[i]))
> +			break;
> +		err = clk_prepare_enable(comp->clks[i]);
> +		if (err)
> +			dev_err(dev,
> +				"Failed to enable clock %d, err %d. type:%d id:%d\n",
> +				i, err, comp->type, comp->id);
> +	}
> +}
> +
> +void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> +		if (IS_ERR(comp->clks[i]))
> +			break;
> +		clk_disable_unprepare(comp->clks[i]);
> +	}
> +
> +	if (comp->larb_dev)
> +		pm_runtime_put_sync(comp->larb_dev);

Ditto. Also, it doesn't make sense to use the _sync variant here, we don't
care if it powers off before the function returns or a bit after.

[snip]

> +static int mdp_comp_init(struct device *dev, struct mdp_dev *mdp,
> +			 struct device_node *node, struct mdp_comp *comp,
> +			 enum mdp_comp_id id)
> +{
> +	struct device_node *larb_node;
> +	struct platform_device *larb_pdev;
> +	int i;
> +
> +	if (id < 0 || id >= MDP_MAX_COMP_COUNT) {
> +		dev_err(dev, "Invalid component id %d\n", id);
> +		return -EINVAL;
> +	}
> +
> +	__mdp_comp_init(mdp, node, comp);
> +	comp->type = mdp_comp_matches[id].type;
> +	comp->id = id;
> +	comp->alias_id = mdp_comp_matches[id].alias_id;
> +	comp->ops = mdp_comp_ops[comp->type];
> +
> +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> +		comp->clks[i] = of_clk_get(node, i);
> +		if (IS_ERR(comp->clks[i]))
> +			break;
> +	}
> +
> +	mdp_get_subsys_id(dev, node, comp);
> +
> +	/* Only DMA capable components need the LARB property */
> +	comp->larb_dev = NULL;
> +	if (comp->type != MDP_COMP_TYPE_RDMA &&
> +	    comp->type != MDP_COMP_TYPE_WROT &&
> +		comp->type != MDP_COMP_TYPE_WDMA)
> +		return 0;
> +
> +	larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> +	if (!larb_node) {
> +		dev_err(dev, "Missing mediatek,larb phandle in %pOF node\n",
> +			node);
> +		return -EINVAL;
> +	}
> +
> +	larb_pdev = of_find_device_by_node(larb_node);
> +	if (!larb_pdev) {
> +		dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
> +		of_node_put(larb_node);
> +		return -EPROBE_DEFER;
> +	}
> +	of_node_put(larb_node);
> +
> +	comp->larb_dev = &larb_pdev->dev;

Why do we need this larb_dev? I believe we already made the handling
transparent by using device links, so as long as the driver handles runtime
PM properly, it should automatically handle larbs.

> +
> +	return 0;
> +}
> +
> +static void mdp_comp_deinit(struct device *dev, struct mdp_comp *comp)
> +{
> +	iounmap(comp->regs);
> +	/* of_node_put(comp->dev_node); */
> +}
> +
> +void mdp_component_deinit(struct device *dev, struct mdp_dev *mdp)
> +{
> +	int i;
> +
> +	mdp_comp_deinit(dev, &mdp->mmsys);
> +	mdp_comp_deinit(dev, &mdp->mm_mutex);
> +	for (i = 0; i < ARRAY_SIZE(mdp->comp); i++) {
> +		if (mdp->comp[i]) {
> +			mdp_comp_deinit(dev, mdp->comp[i]);
> +			kfree(mdp->comp[i]);
> +		}
> +	}
> +}
> +
> +int mdp_component_init(struct device *dev, struct mdp_dev *mdp)
> +{
> +	struct device_node *node, *parent;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(gce_event_names); i++) {
> +		s32 event_id;
> +
> +		event_id = cmdq_dev_get_event(dev, gce_event_names[i]);
> +		mdp->event[i] = (event_id < 0) ? -i : event_id;
> +		dev_info(dev, "Get event %s id:%d\n",
> +			 gce_event_names[i], mdp->event[i]);
> +	}
> +
> +	ret = mdp_mm_init(dev, mdp, &mdp->mmsys, "mediatek,mmsys");
> +	if (ret)
> +		goto err_init_mm;
> +
> +	ret = mdp_mm_init(dev, mdp, &mdp->mm_mutex, "mediatek,mm-mutex");
> +	if (ret)
> +		goto err_init_mm;
> +
> +	parent = dev->of_node->parent;
> +	/* Iterate over sibling MDP function blocks */
> +	for_each_child_of_node(parent, node) {
> +		const struct of_device_id *of_id;
> +		enum mdp_comp_type type;
> +		int id;
> +		struct mdp_comp *comp;
> +
> +		of_id = of_match_node(mdp_comp_dt_ids, node);
> +		if (!of_id)
> +			continue;
> +
> +		if (!of_device_is_available(node)) {
> +			dev_err(dev, "Skipping disabled component %pOF\n",
> +				node);

The function doesn't fail, so this doesn't look like error. Perhaps
dev_info()?

> +			continue;
> +		}
> +
> +		type = (enum mdp_comp_type)of_id->data;
> +		id = mdp_comp_get_id(dev, node, type);
> +		if (id < 0) {
> +			dev_warn(dev, "Skipping unknown component %pOF\n",
> +				 node);
> +			continue;
> +		}
> +
> +		comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
> +		if (!comp) {
> +			ret = -ENOMEM;
> +			goto err_init_comps;
> +		}
> +		mdp->comp[id] = comp;
> +
> +		ret = mdp_comp_init(dev, mdp, node, comp, id);
> +		if (ret)
> +			goto err_init_comps;
> +
> +		dev_info(dev, "%s type:%d alias:%d id:%d base:%#x regs:%p\n",
> +			 of_id->compatible, type, comp->alias_id, id,
> +			(u32)comp->reg_base, comp->regs);
> +	}
> +	return 0;
> +
> +err_init_comps:
> +	mdp_component_deinit(dev, mdp);
> +err_init_mm:
> +	return ret;
> +}
[snip]
> +static int mdp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mdp_dev *mdp;
> +	phandle rproc_phandle;
> +	int ret;
> +
> +	mdp = devm_kzalloc(dev, sizeof(*mdp), GFP_KERNEL);
> +	if (!mdp)
> +		return -ENOMEM;
> +
> +	mdp->pdev = pdev;
> +	ret = mdp_component_init(dev, mdp);
> +	if (ret) {
> +		dev_err(dev, "Failed to initialize mdp components\n");
> +		goto err_init_comp;

There should be no cleanup needed here.

Please always name the labels after the first cleanup step that is needed
after the jump. That helps avoiding such mistakes.

> +	}
> +
> +	mdp->job_wq = create_singlethread_workqueue(MDP_MODULE_NAME);

We should make it freezable.

> +	if (!mdp->job_wq) {
> +		dev_err(dev, "Unable to create job workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_create_job_wq;
> +	}
> +
> +	mdp->vpu_dev = scp_get_pdev(pdev);
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> +				 &rproc_phandle))
> +		dev_err(&pdev->dev, "Could not get scp device\n");

This should fail the probe.

> +	else
> +		dev_info(&pdev->dev, "Find mediatek,scp phandle:%llx\n",
> +			 (unsigned long long)rproc_phandle);

No need for this positive log.

> +
> +	mdp->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> +
> +	dev_info(&pdev->dev, "MDP rproc_handle: %llx",
> +		 (unsigned long long)mdp->rproc_handle);
> +
> +	if (!mdp->rproc_handle)
> +		dev_err(&pdev->dev, "Could not get MDP's rproc_handle\n");

This should fail the probe too.

> +
> +	/* vpu_wdt_reg_handler(mdp->vpu_dev, mdp_reset_handler, mdp,
> +	 *		       VPU_RST_MDP);
> +	 */

Please remove unnecessary code.

> +	mutex_init(&mdp->vpu_lock);
> +	mdp->vpu_count = 0;
> +	mdp->id_count = 0;

No need to explicitly initialize to 0, because we just allocated a zeroed
struct earlier in this function.

> +
> +	mdp->cmdq_clt = cmdq_mbox_create(dev, 0, 1200);
> +	if (IS_ERR(mdp->cmdq_clt))
> +		goto err_mbox_create;
> +
> +	ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register v4l2 device\n");
> +		ret = -EINVAL;
> +		goto err_v4l2_register;
> +	}
> +
> +	ret = mdp_m2m_device_register(mdp);
> +	if (ret) {
> +		v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
> +		goto err_m2m_register;
> +	}
> +	mutex_init(&mdp->m2m_lock);
> +
> +	platform_set_drvdata(pdev, mdp);
> +
> +	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> +	pm_runtime_enable(dev);

This is racy, because mdp_m2m_device_register() already exposed the video
node to the userspace, before we ended the initialization. Please reorder the
initialization so that the device node is registered at the end, when the
driver is fully ready to accept userspace requests.

> +	dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
> +	return 0;
> +
> +err_m2m_register:
> +	v4l2_device_unregister(&mdp->v4l2_dev);
> +err_v4l2_register:

Shouldn't we destroy the cmdq_mbox?

> +err_mbox_create:
> +	destroy_workqueue(mdp->job_wq);
> +err_create_job_wq:
> +err_init_comp:
> +	kfree(mdp);

This was allocated using devm_kzalloc(), so no need to free it explicitly.

> +
> +	dev_dbg(dev, "Errno %d\n", ret);
> +	return ret;
> +}
> +
> +static int mdp_remove(struct platform_device *pdev)
> +{
> +	struct mdp_dev *mdp = platform_get_drvdata(pdev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> +	mdp_m2m_device_unregister(mdp);

Same as in probe, removing must start with unregistering the userspace
interfaces.

> +	v4l2_device_unregister(&mdp->v4l2_dev);
> +
> +	flush_workqueue(mdp->job_wq);
> +	destroy_workqueue(mdp->job_wq);

destroy_workqueue() includes a flush (or drain specifically, which is
stricter than flush).

> +
> +	mdp_component_deinit(&pdev->dev, mdp);
> +	kfree(mdp);

Allocated by devm_kzalloc(), so no need to free here.

> +
> +	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> +	return 0;
> +}
> +
> +static int __maybe_unused mdp_pm_suspend(struct device *dev)
> +{
> +	// TODO: mdp clock off

Is MDP inside a power domain that is controlled by genpd? If yes, runtime
suspend would be cover for the power domain sequencing latency, which would
cause the clock to be running unnecessarily for that time. To avoid that,
one would control the clocks separately, outside of runtime PM callbacks.

> +	return 0;
> +}
> +
> +static int __maybe_unused mdp_pm_resume(struct device *dev)
> +{
> +	// TODO: mdp clock on
> +	return 0;
> +}
> +
> +static int __maybe_unused mdp_suspend(struct device *dev)
> +{
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +

We need to ensure here that the hardware is not doing any processing
anymore.

Hint: Looks like mdp_m2m_worker() is synchronous, so we could make the
workqueue freezable and the PM core would wait for the current pending work
to complete and then freeze the workqueue.

> +	return mdp_pm_suspend(dev);
> +}
> +
> +static int __maybe_unused mdp_resume(struct device *dev)
> +{
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	return mdp_pm_resume(dev);

We need to resume any processing that was queued to the driver before the
system suspended. Should be possible to handle by switching the workqueue to
freezable too.

> +}
> +
> +static const struct dev_pm_ops mdp_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(mdp_suspend, mdp_resume)
> +	SET_RUNTIME_PM_OPS(mdp_pm_suspend, mdp_pm_resume, NULL)
> +};
> +
> +static struct platform_driver mdp_driver = {
> +	.probe		= mdp_probe,
> +	.remove		= mdp_remove,
> +	.driver = {
> +		.name	= MDP_MODULE_NAME,
> +		.pm	= &mdp_pm_ops,
> +		.of_match_table = mdp_of_ids,

Please use the of_match_ptr() wrapper.

[snip]
> +static int mdp_m2m_ctrls_create(struct mdp_m2m_ctx *ctx)
> +{
> +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, MDP_MAX_CTRLS);
> +	ctx->ctrls.hflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					     &mdp_m2m_ctrl_ops, V4L2_CID_HFLIP,
> +					     0, 1, 1, 0);
> +	ctx->ctrls.vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					     &mdp_m2m_ctrl_ops, V4L2_CID_VFLIP,
> +					     0, 1, 1, 0);
> +	ctx->ctrls.rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +					      &mdp_m2m_ctrl_ops,
> +					      V4L2_CID_ROTATE, 0, 270, 90, 0);
> +
> +	if (ctx->ctrl_handler.error) {
> +		int err = ctx->ctrl_handler.error;
> +
> +		v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +		dev_err(&ctx->mdp_dev->pdev->dev,
> +			"Failed to create control handler\n");

Perhaps "Failed to register controls"?

> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static int mdp_m2m_open(struct file *file)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	struct mdp_dev *mdp = video_get_drvdata(vdev);
> +	struct mdp_m2m_ctx *ctx;
> +	int ret;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	if (mutex_lock_interruptible(&mdp->m2m_lock)) {
> +		ret = -ERESTARTSYS;
> +		goto err_lock;
> +	}
> +
> +	ctx->id = mdp->id_count++;

Hmm, wouldn't this leave holes in the id space after we close? Should we use
something like ida?
See: https://www.kernel.org/doc/htmldocs/kernel-api/idr.html

> +	ctx->mdp_dev = mdp;
> +
> +	v4l2_fh_init(&ctx->fh, vdev);
> +	file->private_data = &ctx->fh;
> +	ret = mdp_m2m_ctrls_create(ctx);
> +	if (ret)
> +		goto err_ctrls_create;
> +
> +	/* Use separate control handler per file handle */
> +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> +	v4l2_fh_add(&ctx->fh);
> +
> +	ctx->m2m_ctx = v4l2_m2m_ctx_init(mdp->m2m_dev, ctx, mdp_m2m_queue_init);
> +	if (IS_ERR(ctx->m2m_ctx)) {
> +		dev_err(&mdp->pdev->dev, "Failed to initialize m2m context\n");
> +		ret = PTR_ERR(ctx->m2m_ctx);
> +		goto err_m2m_ctx;
> +	}
> +	ctx->fh.m2m_ctx = ctx->m2m_ctx;
> +
> +	INIT_WORK(&ctx->work, mdp_m2m_worker);
> +	ctx->frame_count = 0;

No need to explicitly initialize fields to 0, because we just allocated the
struct using kzalloc().

> +
> +	ctx->curr_param = mdp_frameparam_init();
> +	if (IS_ERR(ctx->curr_param)) {
> +		dev_err(&mdp->pdev->dev,
> +			"Failed to initialize mdp parameter\n");
> +		ret = PTR_ERR(ctx->curr_param);
> +		goto err_param_init;
> +	}
> +	ctx->curr_param->type = MDP_STREAM_TYPE_BITBLT;

Why not initialize this in mdp_frameparam_init()? We only seem to be using
this value in this driver.

> +
> +	INIT_LIST_HEAD(&ctx->param_list);
> +	list_add_tail(&ctx->curr_param->list, &ctx->param_list);

Why do we need this list? We only seem to have ctx->curr_param in this
driver.

> +
> +	ret = mdp_vpu_get_locked(mdp);
> +	if (ret < 0)
> +		goto err_load_vpu;

This shouldn't happen in open(), but rather the latest possible point in
time. If one needs to keep the VPU running for the time of streaming, then
it should be start_streaming. If one can safely turn the VPU off if there is
no frame queued for long time, it should be just in m2m job_run.

Generally the userspace should be able to
just open an m2m device for querying it, without any side effects like
actually powering on the hardware or grabbing a hardware instance (which
could block some other processes, trying to grab one too).

> +
> +	mutex_unlock(&mdp->m2m_lock);
> +
> +	mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id);
> +
> +	return 0;
> +
> +err_load_vpu:
> +	mdp_frameparam_release(ctx->curr_param);
> +err_param_init:
> +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +err_m2m_ctx:
> +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +	v4l2_fh_del(&ctx->fh);
> +err_ctrls_create:
> +	v4l2_fh_exit(&ctx->fh);
> +	mutex_unlock(&mdp->m2m_lock);
> +err_lock:

Incorrect naming of all the error labels here.

> +	kfree(ctx);
> +
> +	return ret;
> +}
[snip]
> +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> +						 u32 mdp_color)
> +{
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> +
> +	if (MDP_COLOR_IS_RGB(mdp_color))
> +		return MDP_YCBCR_PROFILE_FULL_BT601;
> +
> +	switch (pix_mp->colorspace) {
> +	case V4L2_COLORSPACE_JPEG:
> +		return MDP_YCBCR_PROFILE_JPEG;
> +	case V4L2_COLORSPACE_REC709:
> +	case V4L2_COLORSPACE_DCI_P3:
> +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +			return MDP_YCBCR_PROFILE_FULL_BT709;
> +		return MDP_YCBCR_PROFILE_BT709;
> +	case V4L2_COLORSPACE_BT2020:
> +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +			return MDP_YCBCR_PROFILE_FULL_BT2020;
> +		return MDP_YCBCR_PROFILE_BT2020;
> +	}
> +	/* V4L2_COLORSPACE_SRGB or else */
> +	if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> +		return MDP_YCBCR_PROFILE_FULL_BT601;
> +	return MDP_YCBCR_PROFILE_BT601;

Putting this under the default clause of the switch statement would be
cleaner and the comment wouldn't be needed.

[snip]
> +struct mdp_frameparam *mdp_frameparam_init(void)
> +{
> +	struct mdp_frameparam *param;
> +	struct mdp_frame *frame;
> +
> +	param = kzalloc(sizeof(*param), GFP_KERNEL);
> +	if (!param)
> +		return ERR_PTR(-ENOMEM);

We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then
wouldn't need any dynamic allocation here anymore. And as a side effect, the
function could be just made void, because it couldn't fail.

> +
> +	INIT_LIST_HEAD(&param->list);
> +	mutex_init(&param->lock);
> +	param->state = 0;
> +	param->limit = &mdp_def_limit;
> +	param->type = MDP_STREAM_TYPE_UNKNOWN;

We always seem to use MDP_STREAM_TYPE_BITBLT in this driver.

> +	param->frame_no = 0;

No need for explicit initialization to 0.

Best regards,
Tomasz


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

Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Daoyuan Huang]

hi Tomasz:

Thanks for your review comments, the corresponding modification
& explanation is under preparation, will update soon.

Thanks.

BR
Daoyuan

On Tue, 2019-06-04 at 20:20 +0900, Tomasz Figa wrote:
> Hi Daoyuan,
> 
> On Thu, May 16, 2019 at 11:23:32AM +0800, Daoyuan Huang wrote:
> > From: daoyuan huang <daoyuan.huang@mediatek.com>
> > 
> > This patch adds driver for Media Data Path 3 (MDP3).
> > Each modules' related operation control is sited in mtk-mdp3-comp.c
> > Each modules' register table is defined in file with "mdp_reg_"
> > and "mmsys_" prefix
> > GCE related API, operation control  sited in mtk-mdp3-cmdq.c
> > V4L2 m2m device functions are implemented in mtk-mdp3-m2m.c
> > Probe, power, suspend/resume, system level functions are defined in
> > mtk-mdp3-core.c
> 
> Thanks for the patch. Some initial comments inline.
> 
> [snip]
> > +void mdp_comp_clock_on(struct device *dev, struct mdp_comp *comp)
> > +{
> > +	int i, err;
> > +
> > +	if (comp->larb_dev) {
> > +		err = pm_runtime_get_sync(comp->larb_dev);
> > +		if (err < 0)
> > +			dev_err(dev,
> > +				"Failed to get larb, err %d. type:%d id:%d\n",
> > +				err, comp->type, comp->id);
> 
> There is no need to make this conditional. Actually, we always need to grab
> a runtime PM enable count, otherwise the power domain would power off (if
> this SoC has gate'able power domains).
> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > +		if (IS_ERR(comp->clks[i]))
> > +			break;
> > +		err = clk_prepare_enable(comp->clks[i]);
> > +		if (err)
> > +			dev_err(dev,
> > +				"Failed to enable clock %d, err %d. type:%d id:%d\n",
> > +				i, err, comp->type, comp->id);
> > +	}
> > +}
> > +
> > +void mdp_comp_clock_off(struct device *dev, struct mdp_comp *comp)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > +		if (IS_ERR(comp->clks[i]))
> > +			break;
> > +		clk_disable_unprepare(comp->clks[i]);
> > +	}
> > +
> > +	if (comp->larb_dev)
> > +		pm_runtime_put_sync(comp->larb_dev);
> 
> Ditto. Also, it doesn't make sense to use the _sync variant here, we don't
> care if it powers off before the function returns or a bit after.
> 
> [snip]
> 
> > +static int mdp_comp_init(struct device *dev, struct mdp_dev *mdp,
> > +			 struct device_node *node, struct mdp_comp *comp,
> > +			 enum mdp_comp_id id)
> > +{
> > +	struct device_node *larb_node;
> > +	struct platform_device *larb_pdev;
> > +	int i;
> > +
> > +	if (id < 0 || id >= MDP_MAX_COMP_COUNT) {
> > +		dev_err(dev, "Invalid component id %d\n", id);
> > +		return -EINVAL;
> > +	}
> > +
> > +	__mdp_comp_init(mdp, node, comp);
> > +	comp->type = mdp_comp_matches[id].type;
> > +	comp->id = id;
> > +	comp->alias_id = mdp_comp_matches[id].alias_id;
> > +	comp->ops = mdp_comp_ops[comp->type];
> > +
> > +	for (i = 0; i < ARRAY_SIZE(comp->clks); i++) {
> > +		comp->clks[i] = of_clk_get(node, i);
> > +		if (IS_ERR(comp->clks[i]))
> > +			break;
> > +	}
> > +
> > +	mdp_get_subsys_id(dev, node, comp);
> > +
> > +	/* Only DMA capable components need the LARB property */
> > +	comp->larb_dev = NULL;
> > +	if (comp->type != MDP_COMP_TYPE_RDMA &&
> > +	    comp->type != MDP_COMP_TYPE_WROT &&
> > +		comp->type != MDP_COMP_TYPE_WDMA)
> > +		return 0;
> > +
> > +	larb_node = of_parse_phandle(node, "mediatek,larb", 0);
> > +	if (!larb_node) {
> > +		dev_err(dev, "Missing mediatek,larb phandle in %pOF node\n",
> > +			node);
> > +		return -EINVAL;
> > +	}
> > +
> > +	larb_pdev = of_find_device_by_node(larb_node);
> > +	if (!larb_pdev) {
> > +		dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
> > +		of_node_put(larb_node);
> > +		return -EPROBE_DEFER;
> > +	}
> > +	of_node_put(larb_node);
> > +
> > +	comp->larb_dev = &larb_pdev->dev;
> 
> Why do we need this larb_dev? I believe we already made the handling
> transparent by using device links, so as long as the driver handles runtime
> PM properly, it should automatically handle larbs.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void mdp_comp_deinit(struct device *dev, struct mdp_comp *comp)
> > +{
> > +	iounmap(comp->regs);
> > +	/* of_node_put(comp->dev_node); */
> > +}
> > +
> > +void mdp_component_deinit(struct device *dev, struct mdp_dev *mdp)
> > +{
> > +	int i;
> > +
> > +	mdp_comp_deinit(dev, &mdp->mmsys);
> > +	mdp_comp_deinit(dev, &mdp->mm_mutex);
> > +	for (i = 0; i < ARRAY_SIZE(mdp->comp); i++) {
> > +		if (mdp->comp[i]) {
> > +			mdp_comp_deinit(dev, mdp->comp[i]);
> > +			kfree(mdp->comp[i]);
> > +		}
> > +	}
> > +}
> > +
> > +int mdp_component_init(struct device *dev, struct mdp_dev *mdp)
> > +{
> > +	struct device_node *node, *parent;
> > +	int i, ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(gce_event_names); i++) {
> > +		s32 event_id;
> > +
> > +		event_id = cmdq_dev_get_event(dev, gce_event_names[i]);
> > +		mdp->event[i] = (event_id < 0) ? -i : event_id;
> > +		dev_info(dev, "Get event %s id:%d\n",
> > +			 gce_event_names[i], mdp->event[i]);
> > +	}
> > +
> > +	ret = mdp_mm_init(dev, mdp, &mdp->mmsys, "mediatek,mmsys");
> > +	if (ret)
> > +		goto err_init_mm;
> > +
> > +	ret = mdp_mm_init(dev, mdp, &mdp->mm_mutex, "mediatek,mm-mutex");
> > +	if (ret)
> > +		goto err_init_mm;
> > +
> > +	parent = dev->of_node->parent;
> > +	/* Iterate over sibling MDP function blocks */
> > +	for_each_child_of_node(parent, node) {
> > +		const struct of_device_id *of_id;
> > +		enum mdp_comp_type type;
> > +		int id;
> > +		struct mdp_comp *comp;
> > +
> > +		of_id = of_match_node(mdp_comp_dt_ids, node);
> > +		if (!of_id)
> > +			continue;
> > +
> > +		if (!of_device_is_available(node)) {
> > +			dev_err(dev, "Skipping disabled component %pOF\n",
> > +				node);
> 
> The function doesn't fail, so this doesn't look like error. Perhaps
> dev_info()?
> 
> > +			continue;
> > +		}
> > +
> > +		type = (enum mdp_comp_type)of_id->data;
> > +		id = mdp_comp_get_id(dev, node, type);
> > +		if (id < 0) {
> > +			dev_warn(dev, "Skipping unknown component %pOF\n",
> > +				 node);
> > +			continue;
> > +		}
> > +
> > +		comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
> > +		if (!comp) {
> > +			ret = -ENOMEM;
> > +			goto err_init_comps;
> > +		}
> > +		mdp->comp[id] = comp;
> > +
> > +		ret = mdp_comp_init(dev, mdp, node, comp, id);
> > +		if (ret)
> > +			goto err_init_comps;
> > +
> > +		dev_info(dev, "%s type:%d alias:%d id:%d base:%#x regs:%p\n",
> > +			 of_id->compatible, type, comp->alias_id, id,
> > +			(u32)comp->reg_base, comp->regs);
> > +	}
> > +	return 0;
> > +
> > +err_init_comps:
> > +	mdp_component_deinit(dev, mdp);
> > +err_init_mm:
> > +	return ret;
> > +}
> [snip]
> > +static int mdp_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct mdp_dev *mdp;
> > +	phandle rproc_phandle;
> > +	int ret;
> > +
> > +	mdp = devm_kzalloc(dev, sizeof(*mdp), GFP_KERNEL);
> > +	if (!mdp)
> > +		return -ENOMEM;
> > +
> > +	mdp->pdev = pdev;
> > +	ret = mdp_component_init(dev, mdp);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to initialize mdp components\n");
> > +		goto err_init_comp;
> 
> There should be no cleanup needed here.
> 
> Please always name the labels after the first cleanup step that is needed
> after the jump. That helps avoiding such mistakes.
> 
> > +	}
> > +
> > +	mdp->job_wq = create_singlethread_workqueue(MDP_MODULE_NAME);
> 
> We should make it freezable.
> 
> > +	if (!mdp->job_wq) {
> > +		dev_err(dev, "Unable to create job workqueue\n");
> > +		ret = -ENOMEM;
> > +		goto err_create_job_wq;
> > +	}
> > +
> > +	mdp->vpu_dev = scp_get_pdev(pdev);
> > +
> > +	if (of_property_read_u32(pdev->dev.of_node, "mediatek,scp",
> > +				 &rproc_phandle))
> > +		dev_err(&pdev->dev, "Could not get scp device\n");
> 
> This should fail the probe.
> 
> > +	else
> > +		dev_info(&pdev->dev, "Find mediatek,scp phandle:%llx\n",
> > +			 (unsigned long long)rproc_phandle);
> 
> No need for this positive log.
> 
> > +
> > +	mdp->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > +
> > +	dev_info(&pdev->dev, "MDP rproc_handle: %llx",
> > +		 (unsigned long long)mdp->rproc_handle);
> > +
> > +	if (!mdp->rproc_handle)
> > +		dev_err(&pdev->dev, "Could not get MDP's rproc_handle\n");
> 
> This should fail the probe too.
> 
> > +
> > +	/* vpu_wdt_reg_handler(mdp->vpu_dev, mdp_reset_handler, mdp,
> > +	 *		       VPU_RST_MDP);
> > +	 */
> 
> Please remove unnecessary code.
> 
> > +	mutex_init(&mdp->vpu_lock);
> > +	mdp->vpu_count = 0;
> > +	mdp->id_count = 0;
> 
> No need to explicitly initialize to 0, because we just allocated a zeroed
> struct earlier in this function.
> 
> > +
> > +	mdp->cmdq_clt = cmdq_mbox_create(dev, 0, 1200);
> > +	if (IS_ERR(mdp->cmdq_clt))
> > +		goto err_mbox_create;
> > +
> > +	ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register v4l2 device\n");
> > +		ret = -EINVAL;
> > +		goto err_v4l2_register;
> > +	}
> > +
> > +	ret = mdp_m2m_device_register(mdp);
> > +	if (ret) {
> > +		v4l2_err(&mdp->v4l2_dev, "Failed to register m2m device\n");
> > +		goto err_m2m_register;
> > +	}
> > +	mutex_init(&mdp->m2m_lock);
> > +
> > +	platform_set_drvdata(pdev, mdp);
> > +
> > +	vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> > +	pm_runtime_enable(dev);
> 
> This is racy, because mdp_m2m_device_register() already exposed the video
> node to the userspace, before we ended the initialization. Please reorder the
> initialization so that the device node is registered at the end, when the
> driver is fully ready to accept userspace requests.
> 
> > +	dev_dbg(dev, "mdp-%d registered successfully\n", pdev->id);
> > +	return 0;
> > +
> > +err_m2m_register:
> > +	v4l2_device_unregister(&mdp->v4l2_dev);
> > +err_v4l2_register:
> 
> Shouldn't we destroy the cmdq_mbox?
> 
> > +err_mbox_create:
> > +	destroy_workqueue(mdp->job_wq);
> > +err_create_job_wq:
> > +err_init_comp:
> > +	kfree(mdp);
> 
> This was allocated using devm_kzalloc(), so no need to free it explicitly.
> 
> > +
> > +	dev_dbg(dev, "Errno %d\n", ret);
> > +	return ret;
> > +}
> > +
> > +static int mdp_remove(struct platform_device *pdev)
> > +{
> > +	struct mdp_dev *mdp = platform_get_drvdata(pdev);
> > +
> > +	pm_runtime_disable(&pdev->dev);
> > +	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> > +	mdp_m2m_device_unregister(mdp);
> 
> Same as in probe, removing must start with unregistering the userspace
> interfaces.
> 
> > +	v4l2_device_unregister(&mdp->v4l2_dev);
> > +
> > +	flush_workqueue(mdp->job_wq);
> > +	destroy_workqueue(mdp->job_wq);
> 
> destroy_workqueue() includes a flush (or drain specifically, which is
> stricter than flush).
> 
> > +
> > +	mdp_component_deinit(&pdev->dev, mdp);
> > +	kfree(mdp);
> 
> Allocated by devm_kzalloc(), so no need to free here.
> 
> > +
> > +	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_pm_suspend(struct device *dev)
> > +{
> > +	// TODO: mdp clock off
> 
> Is MDP inside a power domain that is controlled by genpd? If yes, runtime
> suspend would be cover for the power domain sequencing latency, which would
> cause the clock to be running unnecessarily for that time. To avoid that,
> one would control the clocks separately, outside of runtime PM callbacks.
> 
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_pm_resume(struct device *dev)
> > +{
> > +	// TODO: mdp clock on
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused mdp_suspend(struct device *dev)
> > +{
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> > +
> 
> We need to ensure here that the hardware is not doing any processing
> anymore.
> 
> Hint: Looks like mdp_m2m_worker() is synchronous, so we could make the
> workqueue freezable and the PM core would wait for the current pending work
> to complete and then freeze the workqueue.
> 
> > +	return mdp_pm_suspend(dev);
> > +}
> > +
> > +static int __maybe_unused mdp_resume(struct device *dev)
> > +{
> > +	if (pm_runtime_suspended(dev))
> > +		return 0;
> > +
> > +	return mdp_pm_resume(dev);
> 
> We need to resume any processing that was queued to the driver before the
> system suspended. Should be possible to handle by switching the workqueue to
> freezable too.
> 
> > +}
> > +
> > +static const struct dev_pm_ops mdp_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(mdp_suspend, mdp_resume)
> > +	SET_RUNTIME_PM_OPS(mdp_pm_suspend, mdp_pm_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mdp_driver = {
> > +	.probe		= mdp_probe,
> > +	.remove		= mdp_remove,
> > +	.driver = {
> > +		.name	= MDP_MODULE_NAME,
> > +		.pm	= &mdp_pm_ops,
> > +		.of_match_table = mdp_of_ids,
> 
> Please use the of_match_ptr() wrapper.
> 
> [snip]
> > +static int mdp_m2m_ctrls_create(struct mdp_m2m_ctx *ctx)
> > +{
> > +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, MDP_MAX_CTRLS);
> > +	ctx->ctrls.hflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					     &mdp_m2m_ctrl_ops, V4L2_CID_HFLIP,
> > +					     0, 1, 1, 0);
> > +	ctx->ctrls.vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					     &mdp_m2m_ctrl_ops, V4L2_CID_VFLIP,
> > +					     0, 1, 1, 0);
> > +	ctx->ctrls.rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> > +					      &mdp_m2m_ctrl_ops,
> > +					      V4L2_CID_ROTATE, 0, 270, 90, 0);
> > +
> > +	if (ctx->ctrl_handler.error) {
> > +		int err = ctx->ctrl_handler.error;
> > +
> > +		v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +		dev_err(&ctx->mdp_dev->pdev->dev,
> > +			"Failed to create control handler\n");
> 
> Perhaps "Failed to register controls"?
> 
> > +		return err;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mdp_m2m_open(struct file *file)
> > +{
> > +	struct video_device *vdev = video_devdata(file);
> > +	struct mdp_dev *mdp = video_get_drvdata(vdev);
> > +	struct mdp_m2m_ctx *ctx;
> > +	int ret;
> > +
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx)
> > +		return -ENOMEM;
> > +
> > +	if (mutex_lock_interruptible(&mdp->m2m_lock)) {
> > +		ret = -ERESTARTSYS;
> > +		goto err_lock;
> > +	}
> > +
> > +	ctx->id = mdp->id_count++;
> 
> Hmm, wouldn't this leave holes in the id space after we close? Should we use
> something like ida?
> See: https://www.kernel.org/doc/htmldocs/kernel-api/idr.html
> 
> > +	ctx->mdp_dev = mdp;
> > +
> > +	v4l2_fh_init(&ctx->fh, vdev);
> > +	file->private_data = &ctx->fh;
> > +	ret = mdp_m2m_ctrls_create(ctx);
> > +	if (ret)
> > +		goto err_ctrls_create;
> > +
> > +	/* Use separate control handler per file handle */
> > +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> > +	v4l2_fh_add(&ctx->fh);
> > +
> > +	ctx->m2m_ctx = v4l2_m2m_ctx_init(mdp->m2m_dev, ctx, mdp_m2m_queue_init);
> > +	if (IS_ERR(ctx->m2m_ctx)) {
> > +		dev_err(&mdp->pdev->dev, "Failed to initialize m2m context\n");
> > +		ret = PTR_ERR(ctx->m2m_ctx);
> > +		goto err_m2m_ctx;
> > +	}
> > +	ctx->fh.m2m_ctx = ctx->m2m_ctx;
> > +
> > +	INIT_WORK(&ctx->work, mdp_m2m_worker);
> > +	ctx->frame_count = 0;
> 
> No need to explicitly initialize fields to 0, because we just allocated the
> struct using kzalloc().
> 
> > +
> > +	ctx->curr_param = mdp_frameparam_init();
> > +	if (IS_ERR(ctx->curr_param)) {
> > +		dev_err(&mdp->pdev->dev,
> > +			"Failed to initialize mdp parameter\n");
> > +		ret = PTR_ERR(ctx->curr_param);
> > +		goto err_param_init;
> > +	}
> > +	ctx->curr_param->type = MDP_STREAM_TYPE_BITBLT;
> 
> Why not initialize this in mdp_frameparam_init()? We only seem to be using
> this value in this driver.
> 
> > +
> > +	INIT_LIST_HEAD(&ctx->param_list);
> > +	list_add_tail(&ctx->curr_param->list, &ctx->param_list);
> 
> Why do we need this list? We only seem to have ctx->curr_param in this
> driver.
> 
> > +
> > +	ret = mdp_vpu_get_locked(mdp);
> > +	if (ret < 0)
> > +		goto err_load_vpu;
> 
> This shouldn't happen in open(), but rather the latest possible point in
> time. If one needs to keep the VPU running for the time of streaming, then
> it should be start_streaming. If one can safely turn the VPU off if there is
> no frame queued for long time, it should be just in m2m job_run.
> 
> Generally the userspace should be able to
> just open an m2m device for querying it, without any side effects like
> actually powering on the hardware or grabbing a hardware instance (which
> could block some other processes, trying to grab one too).
> 
> > +
> > +	mutex_unlock(&mdp->m2m_lock);
> > +
> > +	mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id);
> > +
> > +	return 0;
> > +
> > +err_load_vpu:
> > +	mdp_frameparam_release(ctx->curr_param);
> > +err_param_init:
> > +	v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > +err_m2m_ctx:
> > +	v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +	v4l2_fh_del(&ctx->fh);
> > +err_ctrls_create:
> > +	v4l2_fh_exit(&ctx->fh);
> > +	mutex_unlock(&mdp->m2m_lock);
> > +err_lock:
> 
> Incorrect naming of all the error labels here.
> 
> > +	kfree(ctx);
> > +
> > +	return ret;
> > +}
> [snip]
> > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> > +						 u32 mdp_color)
> > +{
> > +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +
> > +	if (MDP_COLOR_IS_RGB(mdp_color))
> > +		return MDP_YCBCR_PROFILE_FULL_BT601;
> > +
> > +	switch (pix_mp->colorspace) {
> > +	case V4L2_COLORSPACE_JPEG:
> > +		return MDP_YCBCR_PROFILE_JPEG;
> > +	case V4L2_COLORSPACE_REC709:
> > +	case V4L2_COLORSPACE_DCI_P3:
> > +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +			return MDP_YCBCR_PROFILE_FULL_BT709;
> > +		return MDP_YCBCR_PROFILE_BT709;
> > +	case V4L2_COLORSPACE_BT2020:
> > +		if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +			return MDP_YCBCR_PROFILE_FULL_BT2020;
> > +		return MDP_YCBCR_PROFILE_BT2020;
> > +	}
> > +	/* V4L2_COLORSPACE_SRGB or else */
> > +	if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +		return MDP_YCBCR_PROFILE_FULL_BT601;
> > +	return MDP_YCBCR_PROFILE_BT601;
> 
> Putting this under the default clause of the switch statement would be
> cleaner and the comment wouldn't be needed.
> 
> [snip]
> > +struct mdp_frameparam *mdp_frameparam_init(void)
> > +{
> > +	struct mdp_frameparam *param;
> > +	struct mdp_frame *frame;
> > +
> > +	param = kzalloc(sizeof(*param), GFP_KERNEL);
> > +	if (!param)
> > +		return ERR_PTR(-ENOMEM);
> 
> We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then
> wouldn't need any dynamic allocation here anymore. And as a side effect, the
> function could be just made void, because it couldn't fail.
> 
> > +
> > +	INIT_LIST_HEAD(&param->list);
> > +	mutex_init(&param->lock);
> > +	param->state = 0;
> > +	param->limit = &mdp_def_limit;
> > +	param->type = MDP_STREAM_TYPE_UNKNOWN;
> 
> We always seem to use MDP_STREAM_TYPE_BITBLT in this driver.
> 
> > +	param->frame_no = 0;
> 
> No need for explicit initialization to 0.
> 
> Best regards,
> Tomasz
> 



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

Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Tomasz Figa]

Hi Daoyuan,

On Tue, Jun 11, 2019 at 6:20 PM Daoyuan Huang
<daoyuan.huang@mediatek.com> wrote:
>
> hi Tomasz:
>
> Thanks for your review comments, the corresponding modification
> & explanation is under preparation, will update soon.
>
> Thanks.

Thanks.

Note that Alexandre may already be reviewing the rest of this patch,
so I'd consult with him if sending a next revision or waiting for his
review is preferred.

Best regards,
Tomasz

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

Re: [RFC v2 1/4] dt-binding: mt8183: Add Mediatek MDP3 dt-bindings

[Rob Herring]

On Thu, May 16, 2019 at 11:23:29AM +0800, Daoyuan Huang wrote:
> From: daoyuan huang <daoyuan.huang@mediatek.com>
> 
> This patch adds DT binding document for Media Data Path 3 (MDP3)
> a unit in multimedia system used for scaling and color format convert.
> 
> Signed-off-by: Ping-Hsun Wu <ping-hsun.wu@mediatek.com>
> Signed-off-by: daoyuan huang <daoyuan.huang@mediatek.com>
> ---
>  .../bindings/media/mediatek,mt8183-mdp3.txt   | 217 ++++++++++++++++++
>  1 file changed, 217 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt b/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
> new file mode 100644
> index 000000000000..cf3e808b7146
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/mediatek,mt8183-mdp3.txt
> @@ -0,0 +1,217 @@
> +* Mediatek Media Data Path 3
> +
> +Media Data Path 3 (MDP3) is used for scaling and color space conversion.
> +
> +Required properties (controller node):
> +- compatible: "mediatek,mt8183-mdp"
> +- mediatek,scp: the node of system control processor (SCP), using the
> +  remoteproc & rpmsg framework, see
> +  Documentation/devicetree/bindings/remoteproc/mtk,scp.txt for details.
> +- mediatek,mmsys: the node of mux(multiplexer) controller for HW connections.
> +- mediatek,mm-mutex: the node of sof(start of frame) signal controller.
> +- mediatek,mailbox-gce: the node of global command engine (GCE), used to
> +  read/write registers with critical time limitation, see
> +  Documentation/devicetree/bindings/mailbox/mtk-gce.txt for details.
> +- mboxes: mailbox number used to communicate with GCE.
> +- gce-subsys: sub-system id corresponding to the register address.
> +- gce-event-names: in use event name list, used to correspond to event IDs.
> +- gce-events: in use event IDs list, all IDs are defined in
> +  'dt-bindings/gce/mt8183-gce.h'.
> +
> +Required properties (all function blocks, child node):
> +- compatible: Should be one of
> +        "mediatek,mt8183-mdp-rdma"  - read DMA
> +        "mediatek,mt8183-mdp-rsz"   - resizer
> +        "mediatek,mt8183-mdp-wdma"  - write DMA
> +        "mediatek,mt8183-mdp-wrot"  - write DMA with rotation
> +        "mediatek,mt8183-mdp-ccorr" - color correction with 3X3 matrix
> +- reg: Physical base address and length of the function block register space
> +- clocks: device clocks, see
> +  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> +- power-domains: a phandle to the power domain, see
> +  Documentation/devicetree/bindings/power/power_domain.txt for details.
> +- mediatek,mdp-id: HW index to distinguish same functionality modules.
> +
> +Required properties (DMA function blocks, child node):
> +- compatible: Should be one of
> +        "mediatek,mt8183-mdp-rdma"
> +        "mediatek,mt8183-mdp-wdma"
> +        "mediatek,mt8183-mdp-wrot"
> +- 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.
> +
> +Required properties (input path selection node):
> +- compatible:
> +        "mediatek,mt8183-mdp-dl"    - MDP direct link input source selection
> +- reg: Physical base address and length of the function block register space
> +- clocks: device clocks, see
> +  Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> +- mediatek,mdp-id: HW index to distinguish same functionality modules.
> +
> +Required properties (ISP PASS2 (DIP) module path selection node):
> +- compatible:
> +        "mediatek,mt8183-mdp-imgi"  - input DMA of ISP PASS2 (DIP) module for raw image input
> +- reg: Physical base address and length of the function block register space
> +- mediatek,mdp-id: HW index to distinguish same functionality modules.
> +
> +Required properties (SW node):
> +- compatible: Should be one of
> +        "mediatek,mt8183-mdp-exto"  - output DMA of ISP PASS2 (DIP) module for yuv image output
> +        "mediatek,mt8183-mdp-path"  - MDP output path selection
> +- mediatek,mdp-id: HW index to distinguish same functionality modules.
> +
> +Example:
> +		mdp_camin@14000000 {

s/_/-/ in node names

> +			compatible = "mediatek,mt8183-mdp-dl";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x14000000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_DL_TXCK>,
> +				<&mmsys CLK_MM_MDP_DL_RX>;
> +		};
> +
> +		mdp_camin2@14000000 {
> +			compatible = "mediatek,mt8183-mdp-dl";
> +			mediatek,mdp-id = <1>;
> +			reg = <0 0x14000000 0 0x1000>;

You've got 2 nodes at the same address. You can't do that.

Build your dtb with W=1 and dtc will warn on this. And fix any other 
warnings you get.

> +			clocks = <&mmsys CLK_MM_IPU_DL_TXCK>,
> +				<&mmsys CLK_MM_IPU_DL_RX>;
> +		};
> +
> +		mdp_rdma0: mdp_rdma0@14001000 {
> +			compatible = "mediatek,mt8183-mdp-rdma", "mediatek,mt8183-mdp3";
> +			mediatek,scp = <&scp>;
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x14001000 0 0x1000>;
> +			power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>;
> +			clocks = <&mmsys CLK_MM_MDP_RDMA0>,
> +				<&mmsys CLK_MM_MDP_RSZ1>;
> +			iommus = <&iommu M4U_PORT_MDP_RDMA0>;
> +			mediatek,larb = <&larb0>;
> +			mediatek,mmsys = <&mmsys>;
> +			mediatek,mm-mutex = <&mutex>;
> +			mediatek,mailbox-gce = <&gce>;
> +			mboxes = <&gce 20 0 CMDQ_THR_PRIO_LOWEST>,
> +				<&gce 21 0 CMDQ_THR_PRIO_LOWEST>,
> +				<&gce 22 0 CMDQ_THR_PRIO_LOWEST>,
> +				<&gce 23 0 CMDQ_THR_PRIO_LOWEST>;
> +			gce-subsys = <&gce 0x14000000 SUBSYS_1400XXXX>,
> +				<&gce 0x14010000 SUBSYS_1401XXXX>,
> +				<&gce 0x14020000 SUBSYS_1402XXXX>,
> +				<&gce 0x15020000 SUBSYS_1502XXXX>;
> +			gce-event-names = "rdma0_sof",
> +				"rsz0_sof",
> +				"rsz1_sof",
> +				"tdshp0_sof",
> +				"wrot0_sof",
> +				"wdma0_sof",
> +				"rdma0_done",
> +				"wrot0_done",
> +				"wdma0_done",
> +				"isp_p2_0_done",
> +				"isp_p2_1_done",
> +				"isp_p2_2_done",
> +				"isp_p2_3_done",
> +				"isp_p2_4_done",
> +				"isp_p2_5_done",
> +				"isp_p2_6_done",
> +				"isp_p2_7_done",
> +				"isp_p2_8_done",
> +				"isp_p2_9_done",
> +				"isp_p2_10_done",
> +				"isp_p2_11_done",
> +				"isp_p2_12_done",
> +				"isp_p2_13_done",
> +				"isp_p2_14_done",
> +				"wpe_done",
> +				"wpe_b_done";
> +			gce-events = <&gce CMDQ_EVENT_MDP_RDMA0_SOF>,
> +				<&gce CMDQ_EVENT_MDP_RSZ0_SOF>,
> +				<&gce CMDQ_EVENT_MDP_RSZ1_SOF>,
> +				<&gce CMDQ_EVENT_MDP_TDSHP_SOF>,
> +				<&gce CMDQ_EVENT_MDP_WROT0_SOF>,
> +				<&gce CMDQ_EVENT_MDP_WDMA0_SOF>,
> +				<&gce CMDQ_EVENT_MDP_RDMA0_EOF>,
> +				<&gce CMDQ_EVENT_MDP_WROT0_EOF>,
> +				<&gce CMDQ_EVENT_MDP_WDMA0_EOF>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_0>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_1>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_2>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_3>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_4>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_5>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_6>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_7>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_8>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_9>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_10>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_11>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_12>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_13>,
> +				<&gce CMDQ_EVENT_ISP_FRAME_DONE_P2_14>,
> +				<&gce CMDQ_EVENT_WPE_A_DONE>,
> +				<&gce CMDQ_EVENT_SPE_B_DONE>;
> +		};
> +
> +		mdp_imgi@15020000 {
> +			compatible = "mediatek,mt8183-mdp-imgi";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x15020000 0 0x1000>;
> +		};
> +
> +		mdp_img2o@15020000 {
> +			compatible = "mediatek,mt8183-mdp-exto";
> +			mediatek,mdp-id = <1>;

Missing reg? Again, looks like 2 nodes at the same address.

> +		};
> +
> +		mdp_rsz0: mdp_rsz0@14003000 {
> +			compatible = "mediatek,mt8183-mdp-rsz";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x14003000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_RSZ0>;
> +		};
> +
> +		mdp_rsz1: mdp_rsz1@14004000 {
> +			compatible = "mediatek,mt8183-mdp-rsz";
> +			mediatek,mdp-id = <1>;
> +			reg = <0 0x14004000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_RSZ1>;
> +		};
> +
> +		mdp_wrot0: mdp_wrot0@14005000 {
> +			compatible = "mediatek,mt8183-mdp-wrot";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x14005000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_WROT0>;
> +			iommus = <&iommu M4U_PORT_MDP_WROT0>;
> +			mediatek,larb = <&larb0>;
> +		};
> +
> +		mdp_path0_sout@14005000 {
> +			compatible = "mediatek,mt8183-mdp-path";
> +			mediatek,mdp-id = <0>;

Missing reg? Again, looks like 2 nodes at the same address.

> +		};
> +
> +		mdp_wdma: mdp_wdma@14006000 {
> +			compatible = "mediatek,mt8183-mdp-wdma";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x14006000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_WDMA0>;
> +			iommus = <&iommu M4U_PORT_MDP_WDMA0>;
> +			mediatek,larb = <&larb0>;
> +		};
> +
> +		mdp_path1_sout@14006000 {
> +			compatible = "mediatek,mt8183-mdp-path";
> +			mediatek,mdp-id = <1>;
> +		};
> +
> +		mdp_ccorr: mdp_ccorr@1401c000 {
> +			compatible = "mediatek,mt8183-mdp-ccorr";
> +			mediatek,mdp-id = <0>;
> +			reg = <0 0x1401c000 0 0x1000>;
> +			clocks = <&mmsys CLK_MM_MDP_CCORR>;
> +		};
> -- 
> 2.18.0
> 

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

Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Alexandre Courbot]

On Tue, Jun 4, 2019 at 8:20 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > +
> > +     ret = mdp_vpu_get_locked(mdp);
> > +     if (ret < 0)
> > +             goto err_load_vpu;
>
> This shouldn't happen in open(), but rather the latest possible point in
> time. If one needs to keep the VPU running for the time of streaming, then
> it should be start_streaming. If one can safely turn the VPU off if there is
> no frame queued for long time, it should be just in m2m job_run.
>
> Generally the userspace should be able to
> just open an m2m device for querying it, without any side effects like
> actually powering on the hardware or grabbing a hardware instance (which
> could block some other processes, trying to grab one too).

OTOH looking at the code of mdp_vpu_get_locked(), we do the whole
rproc_boot and VPU init procedure if we were the only user. So I can
understand we want to avoid doing this too often.

Maybe mdp_vpu_get_locked() can be reorganized in a better way. I feel
like the call to mdp_vpu_register() should be done in probe, and maybe
we can use runtime PM (with a reasonable timeout) to control the rproc
and VPU init?


>
> > +
> > +     mutex_unlock(&mdp->m2m_lock);
> > +
> > +     mdp_dbg(0, "%s [%d]", dev_name(&mdp->pdev->dev), ctx->id);
> > +
> > +     return 0;
> > +
> > +err_load_vpu:
> > +     mdp_frameparam_release(ctx->curr_param);
> > +err_param_init:
> > +     v4l2_m2m_ctx_release(ctx->m2m_ctx);
> > +err_m2m_ctx:
> > +     v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> > +     v4l2_fh_del(&ctx->fh);
> > +err_ctrls_create:
> > +     v4l2_fh_exit(&ctx->fh);
> > +     mutex_unlock(&mdp->m2m_lock);
> > +err_lock:
>
> Incorrect naming of all the error labels here.
>
> > +     kfree(ctx);
> > +
> > +     return ret;
> > +}
> [snip]
> > +enum mdp_ycbcr_profile mdp_map_ycbcr_prof_mplane(struct v4l2_format *f,
> > +                                              u32 mdp_color)
> > +{
> > +     struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> > +
> > +     if (MDP_COLOR_IS_RGB(mdp_color))
> > +             return MDP_YCBCR_PROFILE_FULL_BT601;
> > +
> > +     switch (pix_mp->colorspace) {
> > +     case V4L2_COLORSPACE_JPEG:
> > +             return MDP_YCBCR_PROFILE_JPEG;
> > +     case V4L2_COLORSPACE_REC709:
> > +     case V4L2_COLORSPACE_DCI_P3:
> > +             if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +                     return MDP_YCBCR_PROFILE_FULL_BT709;
> > +             return MDP_YCBCR_PROFILE_BT709;
> > +     case V4L2_COLORSPACE_BT2020:
> > +             if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +                     return MDP_YCBCR_PROFILE_FULL_BT2020;
> > +             return MDP_YCBCR_PROFILE_BT2020;
> > +     }
> > +     /* V4L2_COLORSPACE_SRGB or else */
> > +     if (pix_mp->quantization == V4L2_QUANTIZATION_FULL_RANGE)
> > +             return MDP_YCBCR_PROFILE_FULL_BT601;
> > +     return MDP_YCBCR_PROFILE_BT601;
>
> Putting this under the default clause of the switch statement would be
> cleaner and the comment wouldn't be needed.
>
> [snip]
> > +struct mdp_frameparam *mdp_frameparam_init(void)
> > +{
> > +     struct mdp_frameparam *param;
> > +     struct mdp_frame *frame;
> > +
> > +     param = kzalloc(sizeof(*param), GFP_KERNEL);
> > +     if (!param)
> > +             return ERR_PTR(-ENOMEM);
>
> We could just embed mdp_frameparam into the mdp_m2m_ctx struct and then
> wouldn't need any dynamic allocation here anymore. And as a side effect, the
> function could be just made void, because it couldn't fail.
>
> > +
> > +     INIT_LIST_HEAD(&param->list);
> > +     mutex_init(&param->lock);
> > +     param->state = 0;
> > +     param->limit = &mdp_def_limit;
> > +     param->type = MDP_STREAM_TYPE_UNKNOWN;
>
> We always seem to use MDP_STREAM_TYPE_BITBLT in this driver.
>
> > +     param->frame_no = 0;
>
> No need for explicit initialization to 0.
>
> Best regards,
> Tomasz
>

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

Re: [RFC v2 4/4] media: platform: mtk-mdp3: Add Mediatek MDP3 driver

[Tomasz Figa]

On Thu, Jun 20, 2019 at 1:48 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Tue, Jun 4, 2019 at 8:20 PM Tomasz Figa <tfiga@chromium.org> wrote:
> > > +
> > > +     ret = mdp_vpu_get_locked(mdp);
> > > +     if (ret < 0)
> > > +             goto err_load_vpu;
> >
> > This shouldn't happen in open(), but rather the latest possible point in
> > time. If one needs to keep the VPU running for the time of streaming, then
> > it should be start_streaming. If one can safely turn the VPU off if there is
> > no frame queued for long time, it should be just in m2m job_run.
> >
> > Generally the userspace should be able to
> > just open an m2m device for querying it, without any side effects like
> > actually powering on the hardware or grabbing a hardware instance (which
> > could block some other processes, trying to grab one too).
>
> OTOH looking at the code of mdp_vpu_get_locked(), we do the whole
> rproc_boot and VPU init procedure if we were the only user. So I can
> understand we want to avoid doing this too often.
>
> Maybe mdp_vpu_get_locked() can be reorganized in a better way. I feel
> like the call to mdp_vpu_register() should be done in probe, and maybe
> we can use runtime PM (with a reasonable timeout) to control the rproc
> and VPU init?

I think it depends on when exactly the rproc and VPU need stay
initialized. In general, we want to turn off as much as possible as
quickly as possible, but keeping in mind any turn on latencies.

For example. if it takes 10 ms to boot rproc/VPU, we probably
shouldn't turn it off unless we already spent 20-30 ms idling, which
could be handled with runtime PM with (delayed) autosuspend. However,
things like clock gating are normally very fast, so we could just stop
any clocks as soon as frame processing ends and restart when next
frame is getting scheduled and if we use autosuspend, we wouldn't be
able to do it using PM runtime.

My point was that just open() is not the right place for doing this.
Any later stage should be okay, as long as it suits the hardware
architecture.

Best regards,
Tomasz

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