Add Unisoc's drm kms module

From: Kevin Tang <[email protected]>

The Unisoc DRM master device is a virtual device needed to list all DPU devices or other display interface nodes that comprise the graphics subsystem

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/drm.yaml      | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sprd/drm.yaml

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +3 -0

new file mode 100644
index 0000000..1614ca6
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---

Hi Kevin.

Quoted 24 lines · Kevin Tang

From: Kevin Tang <[email protected]>

The Unisoc DRM master device is a virtual device needed to list all DPU devices or other display interface nodes that comprise the graphics subsystem

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/drm.yaml      | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sprd/drm.yaml

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +3 -0

new file mode 100644
index 0000000..1614ca6
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---

Documentation/devicetree/bindings/display/sprd/drm.yaml line 13

Quoted 10 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +10 -0

+$id: http://devicetree.org/schemas/display/sprd/drm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc DRM master device
+
+maintainers:
+  - David Airlie <[email protected]>
+  - Daniel Vetter <[email protected]>
+  - Rob Herring <[email protected]>
+  - Mark Rutland <[email protected]>

Rob is king of a super-maintainer.
He should not be listed unless he has special
relations to sprd.
David + Daniel - likewise. Unless they are closely related to sprd drop them.

Documentation/devicetree/bindings/display/sprd/drm.yaml line 18

Quoted 5 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +5 -0

+
+description: |
+  The Unisoc DRM master device is a virtual device needed to list all
+  DPU devices or other display interface nodes that comprise the
+  graphics subsystem.

I wonder why you name it "Unisoc" when all other places references sprd.

Quoted 5 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +3 -0

+
+properties:
+  compatible:

Documentation/devicetree/bindings/display/sprd/drm.yaml line 31

Quoted 10 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +10 -0

+    const: sprd,display-subsystem
+
+  ports:
+    description:
+      Should contain a list of phandles pointing to display interface port
+      of DPU devices.
+
+required:
+  - compatible
+  - ports

So you want to force the driver to support ports - and no panel referenced directly?

Documentation/devicetree/bindings/display/sprd/drm.yaml line 38

Quoted 8 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +7 -0

+
+examples:
+  - |
+    display-subsystem {
+        compatible = "sprd,display-subsystem";
+        ports = <&dpu_out>;
+    };
\ No newline at end of file

Please fix.

Sam

Hi Kevin.

Quoted 28 lines · Kevin Tang

From: Kevin Tang <[email protected]>

The Unisoc DRM master device is a virtual device needed to list all DPU devices or other display interface nodes that comprise the graphics subsystem

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/drm.yaml      | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sprd/drm.yaml

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +7 -0

new file mode 100644
index 0000000..1614ca6
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sprd/drm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc DRM master device

Documentation/devicetree/bindings/display/sprd/drm.yaml line 13

Quoted 6 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +6 -0

+
+maintainers:
+  - David Airlie <[email protected]>
+  - Daniel Vetter <[email protected]>
+  - Rob Herring <[email protected]>
+  - Mark Rutland <[email protected]>

Rob is king of a super-maintainer.

Hi Kevin.

Quoted 6 lines · Kevin Tang

From: Kevin Tang <[email protected]>

The Unisoc DRM master device is a virtual device needed to list all DPU devices or other display interface nodes that comprise the graphics subsystem

Quoted 6 lines

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/drm.yaml      | 38

++++++++++++++++++++++

Quoted 2 lines

 1 file changed, 38 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/sprd/drm.yaml

Review

Quoted 2 lines

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/display/sprd/drm.yaml

Quoted 16 lines

new file mode 100644
index 0000000..1614ca6

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +11 -0

@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sprd/drm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc DRM master device
+
+maintainers:
+  - David Airlie <[email protected]>
+  - Daniel Vetter <[email protected]>

Documentation/devicetree/bindings/display/sprd/drm.yaml line 13

Quoted 2 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +2 -0

+  - Rob Herring <[email protected]>
+  - Mark Rutland <[email protected]>

Rob is king of a super-maintainer.
He should not be listed unless he has special
relations to sprd.
David + Daniel - likewise. Unless they are closely related to sprd drop them.

Documentation/devicetree/bindings/display/sprd/drm.yaml line 18

Quoted 6 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +5 -0

+
+description: |
+  The Unisoc DRM master device is a virtual device needed to list all
+  DPU devices or other display interface nodes that comprise the
+  graphics subsystem.

I wonder why you name it "Unisoc" when all other places references sprd.

Documentation/devicetree/bindings/display/sprd/drm.yaml

Quoted 10 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +8 -0

+
+properties:
+  compatible:
+    const: sprd,display-subsystem
+
+  ports:
+    description:
+      Should contain a list of phandles pointing to display interface

port

Documentation/devicetree/bindings/display/sprd/drm.yaml line 31

Quoted 5 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +5 -0

+      of DPU devices.
+
+required:
+  - compatible
+  - ports

So you want to force the driver to support ports - and no panel referenced directly?

The ports is related to our display controller, not panel.

Documentation/devicetree/bindings/display/sprd/drm.yaml line 38

Quoted 8 lines

Documentation/devicetree/bindings/display/sprd/drm.yaml new file +7 -0

+
+examples:
+  - |
+    display-subsystem {
+        compatible = "sprd,display-subsystem";
+        ports = <&dpu_out>;
+    };
\ No newline at end of file

Please fix.

Sam

From: Kevin Tang <[email protected]>

Adds drm support for the Unisoc's display subsystem.

This is drm device and gem driver. This driver provides support for the Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

+++ b/drivers/gpu/drm/sprd/Kconfig

drivers/gpu/drm/sprd/Kconfig new file +4 -0

@@ -0,0 +1,14 @@
+config DRM_SPRD
+	tristate "DRM Support for Unisoc SoCs Platform"
+	depends on ARCH_SPRD
+	depends on DRM && OF

\ No newline at end of file

drivers/gpu/drm/sprd/Makefile new file +2 -0

+
+obj-y := sprd_drm.o

drivers/gpu/drm/sprd/sprd_drm.c new file +15 -0

new file mode 100644
index 0000000..7cac098
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/component.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>

drivers/gpu/drm/sprd/sprd_drm.c new file +42 -0

+#define DRIVER_MAJOR	1
+#define DRIVER_MINOR	0
+
+static const struct drm_mode_config_helper_funcs sprd_drm_mode_config_helper = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct drm_mode_config_funcs sprd_drm_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static void sprd_drm_mode_config_init(struct drm_device *drm)
+{
+	drm_mode_config_init(drm);
+
+	drm->mode_config.min_width = 0;
+	drm->mode_config.min_height = 0;
+	drm->mode_config.max_width = 8192;
+	drm->mode_config.max_height = 8192;
+	drm->mode_config.allow_fb_modifiers = true;
+
+	drm->mode_config.funcs = &sprd_drm_mode_config_funcs;
+	drm->mode_config.helper_private = &sprd_drm_mode_config_helper;
+}
+
+DEFINE_DRM_GEM_CMA_FOPS(sprd_drm_fops);
+
+static struct drm_driver sprd_drm_drv = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &sprd_drm_fops,
+
+	/* GEM Operations */
+  	DRM_GEM_CMA_VMAP_DRIVER_OPS,
+
+	.name			= DRIVER_NAME,
+	.desc			= DRIVER_DESC,
+	.date			= DRIVER_DATE,
+	.major			= DRIVER_MAJOR,
+	.minor			= DRIVER_MINOR,
+};

drivers/gpu/drm/sprd/sprd_drm.c new file +8 -0

+
+	dev_set_drvdata(dev, drm);
+
+	sprd = devm_kzalloc(drm->dev, sizeof(*sprd), GFP_KERNEL);
+	if (!sprd) {
+		err = -ENOMEM;
+		goto err_free_drm;
+	}

drivers/gpu/drm/sprd/sprd_drm.c new file +12 -0

+
+	/* reset all the states of crtc/plane/encoder/connector */
+	drm_mode_config_reset(drm);
+
+	/* init kms poll for handling hpd */
+	drm_kms_helper_poll_init(drm);
+
+	err = drm_dev_register(drm, 0);
+	if (err < 0)
+		goto err_kms_helper_poll_fini;
+
+	return 0;

drivers/gpu/drm/sprd/sprd_drm.c new file +7 -0

+}
+
+static void sprd_drm_unbind(struct device *dev)
+{
+	drm_put_dev(dev_get_drvdata(dev));
+}
+

drivers/gpu/drm/sprd/sprd_drm.c new file +71 -0

+
+	return dev->of_node == np;
+}
+
+static int sprd_drm_component_probe(struct device *dev,
+			   const struct component_master_ops *m_ops)
+{
+	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
+	int i;
+
+	if (!dev->of_node)
+		return -EINVAL;
+
+	/*
+	 * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+	 * called from encoder's .bind callbacks works as expected
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		component_match_add(dev, &match, compare_of, port->parent);
+		of_node_put(port);
+	}
+
+	if (i == 0) {
+		dev_err(dev, "missing 'ports' property\n");
+		return -ENODEV;
+	}
+
+	if (!match) {
+		dev_err(dev, "no available port\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * For bound crtcs, bind the encoders attached to their remote endpoint
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		for_each_child_of_node(port, ep) {
+			remote = of_graph_get_remote_port_parent(ep);
+			if (!remote || !of_device_is_available(remote)) {
+				of_node_put(remote);
+				continue;
+			} else if (!of_device_is_available(remote->parent)) {
+				dev_warn(dev, "parent device of %s is not available\n",
+					 remote->full_name);
+				of_node_put(remote);
+				continue;
+			}
+
+			component_match_add(dev, &match, compare_of, remote);
+			of_node_put(remote);
+		}
+		of_node_put(port);

drivers/gpu/drm/sprd/sprd_drm.c new file +19 -0

+	if (ret) {
+		DRM_ERROR("dma_set_mask_and_coherent failed (%d)\n", ret);
+		return ret;
+	}
+
+	return sprd_drm_component_probe(&pdev->dev, &drm_component_ops);
+}
+
+static int sprd_drm_remove(struct platform_device *pdev)
+{
+	component_master_del(&pdev->dev, &drm_component_ops);
+	return 0;
+}
+
+static void sprd_drm_shutdown(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	if (!drm) {

drivers/gpu/drm/sprd/sprd_drm.c new file +39 -0

+};
+MODULE_DEVICE_TABLE(of, drm_match_table);
+
+static struct platform_driver sprd_drm_driver = {
+	.probe = sprd_drm_probe,
+	.remove = sprd_drm_remove,
+	.shutdown = sprd_drm_shutdown,
+	.driver = {
+		.name = "sprd-drm-drv",
+		.of_match_table = drm_match_table,
+	},
+};
+
+static struct platform_driver *sprd_drm_drivers[]  = {
+	&sprd_drm_driver,
+};
+
+static int __init sprd_drm_init(void)
+{
+	int ret;
+
+	ret = platform_register_drivers(sprd_drm_drivers,
+					ARRAY_SIZE(sprd_drm_drivers));
+	return ret;
+}
+
+static void __exit sprd_drm_exit(void)
+{
+	platform_unregister_drivers(sprd_drm_drivers,
+				    ARRAY_SIZE(sprd_drm_drivers));
+}
+
+module_init(sprd_drm_init);
+module_exit(sprd_drm_exit);
+
+MODULE_AUTHOR("Leon He <[email protected]>");
+MODULE_AUTHOR("Kevin Tang <[email protected]>");
+MODULE_DESCRIPTION("Unisoc DRM KMS Master Driver");
+MODULE_LICENSE("GPL v2");

drivers/gpu/drm/sprd/sprd_drm.h new file +16 -0

new file mode 100644
index 0000000..137cb27
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#ifndef _SPRD_DRM_H_
+#define _SPRD_DRM_H_
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_print.h>
+
+struct sprd_drm {
+	struct drm_device *drm;
+};
+
+#endif /* _SPRD_DRM_H_ */

Signature · 6 lines

2.7.4

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Kevin.

Quoted 3 lines · Kevin Tang

From: Kevin Tang <[email protected]>

Adds drm support for the Unisoc's display subsystem.

Thanks for the updated driver patch.
Good split of patches.
A few comments in the following.
Please filter in the comments as some may not apply to this driver.

Sam

Quoted 3 lines

This is drm device and gem driver. This driver provides support for the Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

Hmm, hopefully we can use this without XFree86? Looks like the above was copied from something outdated.

Quoted 15 lines

+++ b/drivers/gpu/drm/sprd/Kconfig

drivers/gpu/drm/sprd/Kconfig new file +11 -0

@@ -0,0 +1,14 @@
+config DRM_SPRD
+     tristate "DRM Support for Unisoc SoCs Platform"
+     depends on ARCH_SPRD
+     depends on DRM && OF
+     select DRM_KMS_HELPER
+     select DRM_GEM_CMA_HELPER
+     select DRM_KMS_CMA_HELPER
+     select DRM_MIPI_DSI
+     select DRM_PANEL
+     select VIDEOMODE_HELPERS
+     select BACKLIGHT_CLASS_DEVICE

drivers/gpu/drm/sprd/Kconfig line 14

Quoted 3 lines

drivers/gpu/drm/sprd/Kconfig new file +3 -0

+     help
+       Choose this option if you have a Unisoc chipsets.
+       If M is selected the module will be called sprd-drm.

Please check that VIDEOMODE_HELPERS is really needed. Please add COMPILE_TEST - so we will build this driver with allmodconfig/allyesconfig.
This is how we ofthe verify refactoring work.

Quoted 3 lines

\ No newline at end of file

Please fix.

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/Makefile

drivers/gpu/drm/sprd/Makefile line 3

Quoted 8 lines

new file mode 100644
index 0000000..63b8751

drivers/gpu/drm/sprd/Makefile new file +3 -0

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-y += -Iinclude/drm

Not required - delete.

drivers/gpu/drm/sprd/Makefile line 5

Quoted 3 lines

drivers/gpu/drm/sprd/Makefile new file +2 -0

+
+subdir-ccflags-y += -I$(src)

Not required - delete.

Review

Quoted 4 lines

drivers/gpu/drm/sprd/Makefile new file +2 -0

+
+obj-y := sprd_drm.o

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/sprd_drm.c

Quoted 20 lines

new file mode 100644
index 0000000..7cac098

drivers/gpu/drm/sprd/sprd_drm.c new file +15 -0

@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/component.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>

drivers/gpu/drm/sprd/sprd_drm.c line 25

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>
+
+#include "sprd_drm.h"
+
+#define DRIVER_NAME  "sprd"
+#define DRIVER_DESC  "Spreadtrum SoCs' DRM Driver"
+#define DRIVER_DATE  "20191101"

We are in 2020 now..

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 4 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +4 -0

+#define DRIVER_MAJOR 1
+#define DRIVER_MINOR 0
+
+static const struct drm_mode_config_helper_funcs

sprd_drm_mode_config_helper = {

Quoted 17 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +17 -0

+     .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct drm_mode_config_funcs sprd_drm_mode_config_funcs = {
+     .fb_create = drm_gem_fb_create,
+     .atomic_check = drm_atomic_helper_check,
+     .atomic_commit = drm_atomic_helper_commit,
+};
+
+static void sprd_drm_mode_config_init(struct drm_device *drm)
+{
+     drm_mode_config_init(drm);
+
+     drm->mode_config.min_width = 0;
+     drm->mode_config.min_height = 0;
+     drm->mode_config.max_width = 8192;
+     drm->mode_config.max_height = 8192;

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+     drm->mode_config.allow_fb_modifiers = true;
+
+     drm->mode_config.funcs = &sprd_drm_mode_config_funcs;
+     drm->mode_config.helper_private = &sprd_drm_mode_config_helper;
+}
+
+DEFINE_DRM_GEM_CMA_FOPS(sprd_drm_fops);
+
+static struct drm_driver sprd_drm_drv = {
+     .driver_features        = DRIVER_GEM | DRIVER_MODESET |

DRIVER_ATOMIC,

Quoted 11 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +11 -0

+     .fops                   = &sprd_drm_fops,
+
+     /* GEM Operations */
+     DRM_GEM_CMA_VMAP_DRIVER_OPS,
+
+     .name                   = DRIVER_NAME,
+     .desc                   = DRIVER_DESC,
+     .date                   = DRIVER_DATE,
+     .major                  = DRIVER_MAJOR,
+     .minor                  = DRIVER_MINOR,
+};

drivers/gpu/drm/sprd/sprd_drm.c line 77

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+
+static int sprd_drm_bind(struct device *dev)
+{
+     struct drm_device *drm;
+     struct sprd_drm *sprd;
+     int err;
+
+     drm = drm_dev_alloc(&sprd_drm_drv, dev);
+     if (IS_ERR(drm))
+             return PTR_ERR(drm);

You should embed drm_device in struct sprd_drm.
See example code in drm/drm_drv.c

This is what modern drm drivers do.

I *think* you can drop the component framework if you do this.

Quoted 14 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +13 -0

+
+     dev_set_drvdata(dev, drm);
+
+     sprd = devm_kzalloc(drm->dev, sizeof(*sprd), GFP_KERNEL);
+     if (!sprd) {
+             err = -ENOMEM;
+             goto err_free_drm;
+     }
+     drm->dev_private = sprd;
+
+     sprd_drm_mode_config_init(drm);
+
+     /* bind and init sub drivers */

drivers/gpu/drm/sprd/sprd_drm.c line 95

Quoted 5 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +5 -0

+     err = component_bind_all(drm->dev, drm);
+     if (err) {
+             DRM_ERROR("failed to bind all component.\n");
+             goto err_dc_cleanup;
+     }

When you have a drm_device - which you do here. Then please use drm_err() and friends for error messages. Please verify all uses of DRM_XXX

Quoted 8 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +7 -0

+
+     /* vblank init */
+     err = drm_vblank_init(drm, drm->mode_config.num_crtc);
+     if (err) {
+             DRM_ERROR("failed to initialize vblank.\n");
+             goto err_unbind_all;
+     }

drivers/gpu/drm/sprd/sprd_drm.c line 104

Quoted 2 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +2 -0

+     /* with irq_enabled = true, we can use the vblank feature. */
+     drm->irq_enabled = true;

I cannot see any irq being installed. This looks wrong.

Quoted 18 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +17 -0

+
+     /* reset all the states of crtc/plane/encoder/connector */
+     drm_mode_config_reset(drm);
+
+     /* init kms poll for handling hpd */
+     drm_kms_helper_poll_init(drm);
+
+     err = drm_dev_register(drm, 0);
+     if (err < 0)
+             goto err_kms_helper_poll_fini;
+
+     return 0;
+
+err_kms_helper_poll_fini:
+     drm_kms_helper_poll_fini(drm);
+err_unbind_all:
+     component_unbind_all(drm->dev, drm);

drivers/gpu/drm/sprd/sprd_drm.c line 126

Quoted 5 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +5 -0

+err_dc_cleanup:
+     drm_mode_config_cleanup(drm);
+err_free_drm:
+     drm_dev_put(drm);
+     return err;

Please see the example in drm/drm_drv.c - following the example will simpligy error handling a bit.
Ad you will learn not to use drm_dev_put().

Quoted 11 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+}
+
+static void sprd_drm_unbind(struct device *dev)
+{
+     drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops drm_component_ops = {
+     .bind = sprd_drm_bind,
+     .unbind = sprd_drm_unbind,

drivers/gpu/drm/sprd/sprd_drm.c line 143

Quoted 7 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +7 -0

+};
+
+static int compare_of(struct device *dev, void *data)
+{
+     struct device_node *np = data;
+
+     DRM_DEBUG("compare %s\n", np->full_name);

Leftover debugging that can be dropped?

Quoted 7 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+
+     return dev->of_node == np;
+}
+
+static int sprd_drm_component_probe(struct device *dev,
+                        const struct component_master_ops *m_ops)

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+{
+     struct device_node *ep, *port, *remote;
+     struct component_match *match = NULL;
+     int i;
+
+     if (!dev->of_node)
+             return -EINVAL;
+
+     /*
+      * Bind the crtc's ports first, so that

drm_of_find_possible_crtcs()

Quoted 18 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +18 -0

+      * called from encoder's .bind callbacks works as expected
+      */
+     for (i = 0; ; i++) {
+             port = of_parse_phandle(dev->of_node, "ports", i);
+             if (!port)
+                     break;
+
+             if (!of_device_is_available(port->parent)) {
+                     of_node_put(port);
+                     continue;
+             }
+
+             component_match_add(dev, &match, compare_of, port->parent);
+             of_node_put(port);
+     }
+
+     if (i == 0) {
+             dev_err(dev, "missing 'ports' property\n");

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+             return -ENODEV;
+     }
+
+     if (!match) {
+             dev_err(dev, "no available port\n");
+             return -ENODEV;
+     }
+
+     /*
+      * For bound crtcs, bind the encoders attached to their remote

endpoint

Quoted 7 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +7 -0

+      */
+     for (i = 0; ; i++) {
+             port = of_parse_phandle(dev->of_node, "ports", i);
+             if (!port)
+                     break;
+
+             if (!of_device_is_available(port->parent)) {

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+                     of_node_put(port);
+                     continue;
+             }
+
+             for_each_child_of_node(port, ep) {
+                     remote = of_graph_get_remote_port_parent(ep);
+                     if (!remote || !of_device_is_available(remote)) {
+                             of_node_put(remote);
+                             continue;
+                     } else if

(!of_device_is_available(remote->parent)) {

drivers/gpu/drm/sprd/sprd_drm.c line 205

Quoted 1 line

drivers/gpu/drm/sprd/sprd_drm.c new file +1 -0

+                             dev_warn(dev, "parent device of %s is not

available\n",

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+                                      remote->full_name);
+                             of_node_put(remote);
+                             continue;
+                     }
+
+                     component_match_add(dev, &match, compare_of,

remote);

drivers/gpu/drm/sprd/sprd_drm.c line 224

Quoted 13 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +13 -0

+                     of_node_put(remote);
+             }
+             of_node_put(port);
+     }
+
+     return component_master_add_with_match(dev, m_ops, match);
+}
+
+static int sprd_drm_probe(struct platform_device *pdev)
+{
+     int ret;
+
+     ret = dma_set_mask_and_coherent(&pdev->dev, ~0);

I do not thing ~0 is always the right mask.
Please verify.

Quoted 25 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +24 -0

+     if (ret) {
+             DRM_ERROR("dma_set_mask_and_coherent failed (%d)\n", ret);
+             return ret;
+     }
+
+     return sprd_drm_component_probe(&pdev->dev, &drm_component_ops);
+}
+
+static int sprd_drm_remove(struct platform_device *pdev)
+{
+     component_master_del(&pdev->dev, &drm_component_ops);
+     return 0;
+}
+
+static void sprd_drm_shutdown(struct platform_device *pdev)
+{
+     struct drm_device *drm = platform_get_drvdata(pdev);
+
+     if (!drm) {
+             DRM_WARN("drm device is not available, no shutdown\n");
+             return;
+     }
+
+     drm_atomic_helper_shutdown(drm);

drivers/gpu/drm/sprd/sprd_drm.c line 253

Quoted 5 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +5 -0

+}
+
+static const struct of_device_id drm_match_table[] = {
+     { .compatible = "sprd,display-subsystem", },
+     {},

Sometimes we use { /* sentinel */ },
here.

Review

Quoted 41 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +39 -0

+};
+MODULE_DEVICE_TABLE(of, drm_match_table);
+
+static struct platform_driver sprd_drm_driver = {
+     .probe = sprd_drm_probe,
+     .remove = sprd_drm_remove,
+     .shutdown = sprd_drm_shutdown,
+     .driver = {
+             .name = "sprd-drm-drv",
+             .of_match_table = drm_match_table,
+     },
+};
+
+static struct platform_driver *sprd_drm_drivers[]  = {
+     &sprd_drm_driver,
+};
+
+static int __init sprd_drm_init(void)
+{
+     int ret;
+
+     ret = platform_register_drivers(sprd_drm_drivers,
+                                     ARRAY_SIZE(sprd_drm_drivers));
+     return ret;
+}
+
+static void __exit sprd_drm_exit(void)
+{
+     platform_unregister_drivers(sprd_drm_drivers,
+                                 ARRAY_SIZE(sprd_drm_drivers));
+}
+
+module_init(sprd_drm_init);
+module_exit(sprd_drm_exit);
+
+MODULE_AUTHOR("Leon He <[email protected]>");
+MODULE_AUTHOR("Kevin Tang <[email protected]>");
+MODULE_DESCRIPTION("Unisoc DRM KMS Master Driver");
+MODULE_LICENSE("GPL v2");

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/sprd_drm.h

Quoted 24 lines

new file mode 100644
index 0000000..137cb27

drivers/gpu/drm/sprd/sprd_drm.h new file +16 -1

@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#ifndef _SPRD_DRM_H_
+#define _SPRD_DRM_H_
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_print.h>
+
+struct sprd_drm {
+     struct drm_device *drm;
+};
+
+#endif /* _SPRD_DRM_H_ */
--

2.7.4

Quoted 4 lines

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoted 12 lines

drivers/gpu/drm/sprd/sprd_drm.c line 77

Quoted 9 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +9 -0

+static int sprd_drm_bind(struct device *dev)
+{
+     struct drm_device *drm;
+     struct sprd_drm *sprd;
+     int err;
+
+     drm = drm_dev_alloc(&sprd_drm_drv, dev);
+     if (IS_ERR(drm))
+             return PTR_ERR(drm);

You should embed drm_device in struct sprd_drm. See example code in drm/drm_drv.c

Quoted 4 lines

This is what modern drm drivers do.

I *think* you can drop the component framework if you do this.

I have read it(drm/drm_drv.c) carefully, if drop the component framework, the whole our drm driver maybe need to redesign, so i still want to keep current design.

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +8 -0

+     sprd_drm_mode_config_init(drm);
+
+     /* bind and init sub drivers */
+     err = component_bind_all(drm->dev, drm);
+     if (err) {
+             DRM_ERROR("failed to bind all component.\n");
+             goto err_dc_cleanup;
+     }

Quoted 4 lines

When you have a drm_device - which you do here. Then please use drm_err() and friends for error messages. Please verify all uses of DRM_XXX

modern drm drivers need drm_xxx to replace DRM_XXX?

Quoted 5 lines

drivers/gpu/drm/sprd/sprd_drm.c line 104

Quoted 3 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +2 -0

+     /* with irq_enabled = true, we can use the vblank feature. */
+     drm->irq_enabled = true;

I cannot see any irq being installed. This looks wrong.

Our display controller isr is been register on crtc driver(sprd_dpu.c), not here.

Hi Kevin/tang.

Thanks for the quick and detailed feedback.
Your questions are addressed below.

Sam

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/sprd/sprd_drm.c line 77

Quoted 9 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +9 -0

+static int sprd_drm_bind(struct device *dev)
+{
+     struct drm_device *drm;
+     struct sprd_drm *sprd;
+     int err;
+
+     drm = drm_dev_alloc(&sprd_drm_drv, dev);
+     if (IS_ERR(drm))
+             return PTR_ERR(drm);

You should embed drm_device in struct sprd_drm. See example code in drm/drm_drv.c

Quoted 7 lines

Quoted 4 lines

This is what modern drm drivers do.

I *think* you can drop the component framework if you do this.

I have read it(drm/drm_drv.c) carefully, if drop the component framework, the whole our drm driver maybe need to redesign, so i still want to keep current design.

OK, fine.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +8 -0

+     sprd_drm_mode_config_init(drm);
+
+     /* bind and init sub drivers */
+     err = component_bind_all(drm->dev, drm);
+     if (err) {
+             DRM_ERROR("failed to bind all component.\n");
+             goto err_dc_cleanup;
+     }

Quoted 5 lines

Quoted 4 lines

When you have a drm_device - which you do here. Then please use drm_err() and friends for error messages. Please verify all uses of DRM_XXX

modern drm drivers need drm_xxx to replace DRM_XXX?

Yes, use of DRM_XXX is deprecated - when you have a drm_device.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +2 -0

+     /* with irq_enabled = true, we can use the vblank feature. */
+     drm->irq_enabled = true;

Quoted 3 lines

Quoted 2 lines

I cannot see any irq being installed. This looks wrong.

Our display controller isr is been register on crtc driver(sprd_dpu.c),

not

Quoted 1 line

here.

I think you just need to move this to next patch and then it is fine.

Sam

Quoted 48 lines · Sam Ravnborg

Hi Kevin/tang.

Thanks for the quick and detailed feedback.
Your questions are addressed below.

Sam

Quoted 12 lines

Quoted 12 lines

drivers/gpu/drm/sprd/sprd_drm.c line 77

Quoted 9 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +9 -0

+static int sprd_drm_bind(struct device *dev)
+{
+     struct drm_device *drm;
+     struct sprd_drm *sprd;
+     int err;
+
+     drm = drm_dev_alloc(&sprd_drm_drv, dev);
+     if (IS_ERR(drm))
+             return PTR_ERR(drm);

You should embed drm_device in struct sprd_drm. See example code in drm/drm_drv.c

Quoted 5 lines

Quoted 4 lines

This is what modern drm drivers do.

I *think* you can drop the component framework if you do this.

I have read it(drm/drm_drv.c) carefully, if drop the component

framework,

Quoted 2 lines

the whole our drm driver maybe need to redesign, so i still want to keep current design.

OK, fine.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +8 -0

+     sprd_drm_mode_config_init(drm);
+
+     /* bind and init sub drivers */
+     err = component_bind_all(drm->dev, drm);
+     if (err) {
+             DRM_ERROR("failed to bind all component.\n");
+             goto err_dc_cleanup;
+     }

Quoted 5 lines

Quoted 4 lines

When you have a drm_device - which you do here. Then please use drm_err() and friends for error messages. Please verify all uses of DRM_XXX

modern drm drivers need drm_xxx to replace DRM_XXX?

Yes, use of DRM_XXX is deprecated - when you have a drm_device.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +2 -0

+     /* with irq_enabled = true, we can use the vblank feature. */
+     drm->irq_enabled = true;

Quoted 8 lines

Quoted 3 lines

Quoted 2 lines

I cannot see any irq being installed. This looks wrong.

Our display controller isr is been register on crtc driver(sprd_dpu.c),

not

Quoted 1 line

here.

I think you just need to move this to next patch and then it is fine.

Here is the advice given by Thomas Zimmermann, similar to the advice you gave.
I have given thomas feedback about my questions, maybe thomas didn't see my email, so there is no reply.

But I've always been confused, because irq is initialized in drm driver for other guys, why not for me?
Can you help to tell the reason in detail, looking forward to your answers.

Thomas's suggestion：

-------------------------------------------------------------------------------------------
This line indicates the problem's design. The irq is initialized in the sub-device code, but the device state is set here. Instead both should be set in the same place.

drivers/gpu/drm/sprd/sprd_drm.c line 110

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+
+     /* reset all the states of crtc/plane/encoder/connector */
+     drm_mode_config_reset(drm);
+
+     /* init kms poll for handling hpd */
+     drm_kms_helper_poll_init(drm);

Most of this function's code should be moved into the sub-device bind function.

Here, maybe do:

* allocate device structures
* call component_bind_all()
* on success, register device

The sub-device function should then do

* init modesetting, crtc, planes, etc.
* do drm_mode_config_reset()
* init vblanking
* init the irq
* do drm_kms_helper_poll_init()

roughtly in that order. It makes sense to call drm_vblank_init() after drm_mode_config_reset(), as vblanking uses some of the mode-config fields.

------------------------------------------------------------------------------------------------------

Quoted 4 lines

Sam

On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <[email protected] <mailto:[email protected]>> wrote:

    Hi Kevin/tang.

    Thanks for the quick and detailed feedback.
    Your questions are addressed below.

            Sam


    > > > +static int sprd_drm_bind(struct device *dev)
    > > > +{
    > > > +     struct drm_device *drm;
    > > > +     struct sprd_drm *sprd;
    > > > +     int err;
    > > > +
    > > > +     drm = drm_dev_alloc(&sprd_drm_drv, dev);
    > > > +     if (IS_ERR(drm))
    > > > +             return PTR_ERR(drm);
    > > You should embed drm_device in struct sprd_drm.
    > > See example code in drm/drm_drv.c
    > >
    > > This is what modern drm drivers do.
    > >
    > > I *think* you can drop the component framework if you do this.
    > >
    > I have read it(drm/drm_drv.c) carefully, if drop the component
    framework,
    > the whole our drm driver maybe need to redesign, so i still want
    to keep
    > current design.
    OK, fine.

    > > > +     sprd_drm_mode_config_init(drm);
    > > > +
    > > > +     /* bind and init sub drivers */
    > > > +     err = component_bind_all(drm->dev, drm);
    > > > +     if (err) {
    > > > +             DRM_ERROR("failed to bind all component.\n");
    > > > +             goto err_dc_cleanup;
    > > > +     }
    > > When you have a drm_device - which you do here.
    > > Then please use drm_err() and friends for error messages.
    > > Please verify all uses of DRM_XXX
    > >
    >   modern drm drivers need drm_xxx to replace DRM_XXX?
    Yes, use of DRM_XXX is deprecated - when you have a drm_device.

    > >
    > > > +     /* with irq_enabled = true, we can use the vblank
    feature. */
    > > > +     drm->irq_enabled = true;
    > > I cannot see any irq being installed. This looks wrong.
    > >
    > Our display controller isr is been register on crtc
    driver(sprd_dpu.c), not
    > here.

    I think you just need to move this to next patch and then it is fine.

Here is the advice given by Thomas Zimmermann, similar to the advice you gave.
I have given thomas feedback about my questions, maybe thomas didn't see my email, so there is no reply.

But I've always been confused, because irq is initialized in drm driver for other guys, why not for me?

Can you help to tell the reason in detail, looking forward to your answers.

Thomas's suggestion：
-------------------------------------------------------------------------------------------

This line indicates the problem's design. The irq is initialized in the sub-device code, but the device state is set here. Instead both should be set in the same place.

drivers/gpu/drm/sprd/sprd_drm.c line 110

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+
+     /* reset all the states of crtc/plane/encoder/connector */
+     drm_mode_config_reset(drm);
+
+     /* init kms poll for handling hpd */
+     drm_kms_helper_poll_init(drm);

Most of this function's code should be moved into the sub-device bind function.

Here, maybe do:

 * allocate device structures
 * call component_bind_all()
 * on success, register device

The sub-device function should then do

 * init modesetting, crtc, planes, etc.
 * do drm_mode_config_reset()
 * init vblanking
 * init the irq
 * do drm_kms_helper_poll_init()

roughtly in that order. It makes sense to call drm_vblank_init() after drm_mode_config_reset(), as vblanking uses some of the mode-config fields.
------------------------------------------------------------------------------------------------------

        Sam

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi

Am 23.02.20 um 05:26 schrieb tang pengchuan:

Quoted 63 lines

On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <[email protected] <mailto:[email protected]>> wrote:

    Hi Kevin/tang.

    Thanks for the quick and detailed feedback.
    Your questions are addressed below.

            Sam


    > > > +static int sprd_drm_bind(struct device *dev)
    > > > +{
    > > > +     struct drm_device *drm;
    > > > +     struct sprd_drm *sprd;
    > > > +     int err;
    > > > +
    > > > +     drm = drm_dev_alloc(&sprd_drm_drv, dev);
    > > > +     if (IS_ERR(drm))
    > > > +             return PTR_ERR(drm);
    > > You should embed drm_device in struct sprd_drm.
    > > See example code in drm/drm_drv.c
    > >
    > > This is what modern drm drivers do.
    > >
    > > I *think* you can drop the component framework if you do this.
    > >
    > I have read it(drm/drm_drv.c) carefully, if drop the component
    framework,
    > the whole our drm driver maybe need to redesign, so i still want
    to keep
    > current design.
    OK, fine.

    > > > +     sprd_drm_mode_config_init(drm);
    > > > +
    > > > +     /* bind and init sub drivers */
    > > > +     err = component_bind_all(drm->dev, drm);
    > > > +     if (err) {
    > > > +             DRM_ERROR("failed to bind all component.\n");
    > > > +             goto err_dc_cleanup;
    > > > +     }
    > > When you have a drm_device - which you do here.
    > > Then please use drm_err() and friends for error messages.
    > > Please verify all uses of DRM_XXX
    > >
    >   modern drm drivers need drm_xxx to replace DRM_XXX?
    Yes, use of DRM_XXX is deprecated - when you have a drm_device.

    > >
    > > > +     /* with irq_enabled = true, we can use the vblank
    feature. */
    > > > +     drm->irq_enabled = true;
    > > I cannot see any irq being installed. This looks wrong.
    > >
    > Our display controller isr is been register on crtc
    driver(sprd_dpu.c), not
    > here.

    I think you just need to move this to next patch and then it is fine.

Quoted 4 lines

Here is the advice given by Thomas Zimmermann, similar to the advice you gave.
I have given thomas feedback about my questions, maybe thomas didn't see my email, so there is no reply.

I have been busy last week. Sorry for not getting back to you.

Quoted 3 lines

But I've always been confused, because irq is initialized in drm driver for other guys, why not for me?

Do you have an example?

Best regards
Thomas

Quoted 1 line

Can you help to tell the reason in detail, looking forward to your

answers.

Quoted 3 lines

Thomas's suggestion：

-------------------------------------------------------------------------------------------

Quoted 23 lines

This line indicates the problem's design. The irq is initialized in the sub-device code, but the device state is set here. Instead both should be set in the same place.

drivers/gpu/drm/sprd/sprd_drm.c line 110

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+
+     /* reset all the states of crtc/plane/encoder/connector */
+     drm_mode_config_reset(drm);
+
+     /* init kms poll for handling hpd */
+     drm_kms_helper_poll_init(drm);

Most of this function's code should be moved into the sub-device bind function.

Here, maybe do:

* allocate device structures
* call component_bind_all()
* on success, register device

The sub-device function should then do

Quoted 8 lines

* init modesetting, crtc, planes, etc.
* do drm_mode_config_reset()
* init vblanking
* init the irq
* do drm_kms_helper_poll_init()

roughtly in that order. It makes sense to call drm_vblank_init() after drm_mode_config_reset(), as vblanking uses some of the mode-config

fields.

Quoted 1 line

------------------------------------------------------------------------------------------------------

Quoted 10 lines

Sam

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

From: Kevin Tang <[email protected]>

Adds drm support for the Unisoc's display subsystem.

This is drm device and gem driver. This driver provides support for the Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 drivers/gpu/drm/Kconfig         |   2 +
 drivers/gpu/drm/Makefile        |   1 +
 drivers/gpu/drm/sprd/Kconfig    |  14 ++
 drivers/gpu/drm/sprd/Makefile   |   7 +
 drivers/gpu/drm/sprd/sprd_drm.c | 292 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/sprd/sprd_drm.h |  16 +++
 6 files changed, 332 insertions(+)
 create mode 100644 drivers/gpu/drm/sprd/Kconfig
 create mode 100644 drivers/gpu/drm/sprd/Makefile
 create mode 100644 drivers/gpu/drm/sprd/sprd_drm.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_drm.h

drivers/gpu/drm/Kconfig +2 -0

index bfdadc3..cead12c 100644
@@ -387,6 +387,8 @@ source "drivers/gpu/drm/aspeed/Kconfig"
 
 source "drivers/gpu/drm/mcde/Kconfig"
 
+source "drivers/gpu/drm/sprd/Kconfig"
+
 # Keep legacy drivers last
 
 menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 9f1c7c4..85ca211 100644
@@ -122,3 +122,4 @@ obj-$(CONFIG_DRM_LIMA)  += lima/
 obj-$(CONFIG_DRM_PANFROST) += panfrost/
 obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
 obj-$(CONFIG_DRM_MCDE) += mcde/
+obj-$(CONFIG_DRM_SPRD) += sprd/

drivers/gpu/drm/sprd/Kconfig new file +14 -0

new file mode 100644
index 0000000..79f286b
@@ -0,0 +1,14 @@
+config DRM_SPRD
+       tristate "DRM Support for Unisoc SoCs Platform"
+       depends on ARCH_SPRD
+       depends on DRM && OF
+       select DRM_KMS_HELPER
+       select DRM_GEM_CMA_HELPER
+       select DRM_KMS_CMA_HELPER
+       select DRM_MIPI_DSI
+       select DRM_PANEL
+       select VIDEOMODE_HELPERS
+       select BACKLIGHT_CLASS_DEVICE
+       help
+         Choose this option if you have a Unisoc chipsets.
+         If M is selected the module will be called sprd-drm.
\ No newline at end of file

drivers/gpu/drm/sprd/Makefile new file +7 -0

new file mode 100644
index 0000000..63b8751
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-y += -Iinclude/drm
+
+subdir-ccflags-y += -I$(src)
+
+obj-y := sprd_drm.o

drivers/gpu/drm/sprd/sprd_drm.c new file +207 -0

new file mode 100644
index 0000000..7cac098
@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/component.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>
+
+#include "sprd_drm.h"
+
+#define DRIVER_NAME    "sprd"
+#define DRIVER_DESC    "Spreadtrum SoCs' DRM Driver"
+#define DRIVER_DATE    "20191101"
+#define DRIVER_MAJOR   1
+#define DRIVER_MINOR   0
+
+static const struct drm_mode_config_helper_funcs sprd_drm_mode_config_helper = {
+       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct drm_mode_config_funcs sprd_drm_mode_config_funcs = {
+       .fb_create = drm_gem_fb_create,
+       .atomic_check = drm_atomic_helper_check,
+       .atomic_commit = drm_atomic_helper_commit,
+};
+
+static void sprd_drm_mode_config_init(struct drm_device *drm)
+{
+       drm_mode_config_init(drm);
+
+       drm->mode_config.min_width = 0;
+       drm->mode_config.min_height = 0;
+       drm->mode_config.max_width = 8192;
+       drm->mode_config.max_height = 8192;
+       drm->mode_config.allow_fb_modifiers = true;
+
+       drm->mode_config.funcs = &sprd_drm_mode_config_funcs;
+       drm->mode_config.helper_private = &sprd_drm_mode_config_helper;
+}
+
+DEFINE_DRM_GEM_CMA_FOPS(sprd_drm_fops);
+
+static struct drm_driver sprd_drm_drv = {
+       .driver_features        = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+       .fops                   = &sprd_drm_fops,
+
+       /* GEM Operations */
+       DRM_GEM_CMA_VMAP_DRIVER_OPS,
+
+       .name                   = DRIVER_NAME,
+       .desc                   = DRIVER_DESC,
+       .date                   = DRIVER_DATE,
+       .major                  = DRIVER_MAJOR,
+       .minor                  = DRIVER_MINOR,
+};
+
+static int sprd_drm_bind(struct device *dev)
+{
+       struct drm_device *drm;
+       struct sprd_drm *sprd;
+       int err;
+
+       drm = drm_dev_alloc(&sprd_drm_drv, dev);
+       if (IS_ERR(drm))
+               return PTR_ERR(drm);
+
+       dev_set_drvdata(dev, drm);
+
+       sprd = devm_kzalloc(drm->dev, sizeof(*sprd), GFP_KERNEL);
+       if (!sprd) {
+               err = -ENOMEM;
+               goto err_free_drm;
+       }
+       drm->dev_private = sprd;
+
+       sprd_drm_mode_config_init(drm);
+
+       /* bind and init sub drivers */
+       err = component_bind_all(drm->dev, drm);
+       if (err) {
+               DRM_ERROR("failed to bind all component.\n");
+               goto err_dc_cleanup;
+       }
+
+       /* vblank init */
+       err = drm_vblank_init(drm, drm->mode_config.num_crtc);
+       if (err) {
+               DRM_ERROR("failed to initialize vblank.\n");
+               goto err_unbind_all;
+       }
+       /* with irq_enabled = true, we can use the vblank feature. */
+       drm->irq_enabled = true;
+
+       /* reset all the states of crtc/plane/encoder/connector */
+       drm_mode_config_reset(drm);
+
+       /* init kms poll for handling hpd */
+       drm_kms_helper_poll_init(drm);
+
+       err = drm_dev_register(drm, 0);
+       if (err < 0)
+               goto err_kms_helper_poll_fini;
+
+       return 0;
+
+err_kms_helper_poll_fini:
+       drm_kms_helper_poll_fini(drm);
+err_unbind_all:
+       component_unbind_all(drm->dev, drm);
+err_dc_cleanup:
+       drm_mode_config_cleanup(drm);
+err_free_drm:
+       drm_dev_put(drm);
+       return err;
+}
+
+static void sprd_drm_unbind(struct device *dev)
+{
+       drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops drm_component_ops = {
+       .bind = sprd_drm_bind,
+       .unbind = sprd_drm_unbind,
+};
+
+static int compare_of(struct device *dev, void *data)
+{
+       struct device_node *np = data;
+
+       DRM_DEBUG("compare %s\n", np->full_name);
+
+       return dev->of_node == np;
+}
+
+static int sprd_drm_component_probe(struct device *dev,
+                          const struct component_master_ops *m_ops)
+{
+       struct device_node *ep, *port, *remote;
+       struct component_match *match = NULL;
+       int i;
+
+       if (!dev->of_node)
+               return -EINVAL;
+
+       /*
+        * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+        * called from encoder's .bind callbacks works as expected
+        */
+       for (i = 0; ; i++) {
+               port = of_parse_phandle(dev->of_node, "ports", i);
+               if (!port)
+                       break;
+
+               if (!of_device_is_available(port->parent)) {
+                       of_node_put(port);
+                       continue;
+               }
+
+               component_match_add(dev, &match, compare_of, port->parent);
+               of_node_put(port);
+       }
+
+       if (i == 0) {
+               dev_err(dev, "missing 'ports' property\n");
+               return -ENODEV;
+       }
+
+       if (!match) {
+               dev_err(dev, "no available port\n");
+               return -ENODEV;
+       }
+
+       /*
+        * For bound crtcs, bind the encoders attached to their remote endpoint
+        */
+       for (i = 0; ; i++) {
+               port = of_parse_phandle(dev->of_node, "ports", i);
+               if (!port)
+                       break;
+
+               if (!of_device_is_available(port->parent)) {
+                       of_node_put(port);
+                       continue;
+               }
+
+               for_each_child_of_node(port, ep) {
+                       remote = of_graph_get_remote_port_parent(ep);
+                       if (!remote || !of_device_is_available(remote)) {
+                               of_node_put(remote);
+                               continue;
+                       } else if (!of_device_is_available(remote->parent)) {
+                               dev_warn(dev, "parent device of %s is not available\n",
+                                        remote->full_name);
+                               of_node_put(remote);

Hi all,

Quoted 12 lines · Kevin Tang

From: Kevin Tang <[email protected]>

Adds drm support for the Unisoc's display subsystem.

This is drm device and gem driver. This driver provides support for the Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

Quoted 6 lines

---
 drivers/gpu/drm/Kconfig         |   2 +
 drivers/gpu/drm/Makefile        |   1 +
 drivers/gpu/drm/sprd/Kconfig    |  14 ++
 drivers/gpu/drm/sprd/Makefile   |   7 +
 drivers/gpu/drm/sprd/sprd_drm.c | 292

++++++++++++++++++++++++++++++++++++++++

Review

Quoted 51 lines

 drivers/gpu/drm/sprd/sprd_drm.h |  16 +++
 6 files changed, 332 insertions(+)
 create mode 100644 drivers/gpu/drm/sprd/Kconfig
 create mode 100644 drivers/gpu/drm/sprd/Makefile
 create mode 100644 drivers/gpu/drm/sprd/sprd_drm.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_drm.h

drivers/gpu/drm/Kconfig +2 -0

index bfdadc3..cead12c 100644
@@ -387,6 +387,8 @@ source "drivers/gpu/drm/aspeed/Kconfig"
 
 source "drivers/gpu/drm/mcde/Kconfig"
 
+source "drivers/gpu/drm/sprd/Kconfig"
+
 # Keep legacy drivers last
 
 menuconfig DRM_LEGACY

drivers/gpu/drm/Makefile +1 -0

index 9f1c7c4..85ca211 100644
@@ -122,3 +122,4 @@ obj-$(CONFIG_DRM_LIMA)  += lima/
 obj-$(CONFIG_DRM_PANFROST) += panfrost/
 obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/
 obj-$(CONFIG_DRM_MCDE) += mcde/
+obj-$(CONFIG_DRM_SPRD) += sprd/

drivers/gpu/drm/sprd/Kconfig new file +14 -0

new file mode 100644
index 0000000..79f286b
@@ -0,0 +1,14 @@
+config DRM_SPRD
+       tristate "DRM Support for Unisoc SoCs Platform"
+       depends on ARCH_SPRD
+       depends on DRM && OF
+       select DRM_KMS_HELPER
+       select DRM_GEM_CMA_HELPER
+       select DRM_KMS_CMA_HELPER
+       select DRM_MIPI_DSI
+       select DRM_PANEL
+       select VIDEOMODE_HELPERS
+       select BACKLIGHT_CLASS_DEVICE
+       help
+         Choose this option if you have a Unisoc chipsets.
+         If M is selected the module will be called sprd-drm.
\ No newline at end of file

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/Makefile

Review

Quoted 13 lines

new file mode 100644
index 0000000..63b8751

drivers/gpu/drm/sprd/Makefile new file +7 -0

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-y += -Iinclude/drm
+
+subdir-ccflags-y += -I$(src)
+
+obj-y := sprd_drm.o

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/sprd_drm.c

Quoted 24 lines

new file mode 100644
index 0000000..7cac098

drivers/gpu/drm/sprd/sprd_drm.c new file +19 -0

@@ -0,0 +1,292 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/component.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_graph.h>
+#include <linux/of_platform.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>

drivers/gpu/drm/sprd/sprd_drm.c line 29

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+
+#include "sprd_drm.h"
+
+#define DRIVER_NAME    "sprd"
+#define DRIVER_DESC    "Spreadtrum SoCs' DRM Driver"
+#define DRIVER_DATE    "20191101"
+#define DRIVER_MAJOR   1
+#define DRIVER_MINOR   0
+
+static const struct drm_mode_config_helper_funcs

sprd_drm_mode_config_helper = {

Quoted 17 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +17 -0

+       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static const struct drm_mode_config_funcs sprd_drm_mode_config_funcs = {
+       .fb_create = drm_gem_fb_create,
+       .atomic_check = drm_atomic_helper_check,
+       .atomic_commit = drm_atomic_helper_commit,
+};
+
+static void sprd_drm_mode_config_init(struct drm_device *drm)
+{
+       drm_mode_config_init(drm);
+
+       drm->mode_config.min_width = 0;
+       drm->mode_config.min_height = 0;
+       drm->mode_config.max_width = 8192;
+       drm->mode_config.max_height = 8192;

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+       drm->mode_config.allow_fb_modifiers = true;
+
+       drm->mode_config.funcs = &sprd_drm_mode_config_funcs;
+       drm->mode_config.helper_private = &sprd_drm_mode_config_helper;
+}
+
+DEFINE_DRM_GEM_CMA_FOPS(sprd_drm_fops);
+
+static struct drm_driver sprd_drm_drv = {
+       .driver_features        = DRIVER_GEM | DRIVER_MODESET |

DRIVER_ATOMIC,

Quoted 93 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +93 -0

+       .fops                   = &sprd_drm_fops,
+
+       /* GEM Operations */
+       DRM_GEM_CMA_VMAP_DRIVER_OPS,
+
+       .name                   = DRIVER_NAME,
+       .desc                   = DRIVER_DESC,
+       .date                   = DRIVER_DATE,
+       .major                  = DRIVER_MAJOR,
+       .minor                  = DRIVER_MINOR,
+};
+
+static int sprd_drm_bind(struct device *dev)
+{
+       struct drm_device *drm;
+       struct sprd_drm *sprd;
+       int err;
+
+       drm = drm_dev_alloc(&sprd_drm_drv, dev);
+       if (IS_ERR(drm))
+               return PTR_ERR(drm);
+
+       dev_set_drvdata(dev, drm);
+
+       sprd = devm_kzalloc(drm->dev, sizeof(*sprd), GFP_KERNEL);
+       if (!sprd) {
+               err = -ENOMEM;
+               goto err_free_drm;
+       }
+       drm->dev_private = sprd;
+
+       sprd_drm_mode_config_init(drm);
+
+       /* bind and init sub drivers */
+       err = component_bind_all(drm->dev, drm);
+       if (err) {
+               DRM_ERROR("failed to bind all component.\n");
+               goto err_dc_cleanup;
+       }
+
+       /* vblank init */
+       err = drm_vblank_init(drm, drm->mode_config.num_crtc);
+       if (err) {
+               DRM_ERROR("failed to initialize vblank.\n");
+               goto err_unbind_all;
+       }
+       /* with irq_enabled = true, we can use the vblank feature. */
+       drm->irq_enabled = true;
+
+       /* reset all the states of crtc/plane/encoder/connector */
+       drm_mode_config_reset(drm);
+
+       /* init kms poll for handling hpd */
+       drm_kms_helper_poll_init(drm);
+
+       err = drm_dev_register(drm, 0);
+       if (err < 0)
+               goto err_kms_helper_poll_fini;
+
+       return 0;
+
+err_kms_helper_poll_fini:
+       drm_kms_helper_poll_fini(drm);
+err_unbind_all:
+       component_unbind_all(drm->dev, drm);
+err_dc_cleanup:
+       drm_mode_config_cleanup(drm);
+err_free_drm:
+       drm_dev_put(drm);
+       return err;
+}
+
+static void sprd_drm_unbind(struct device *dev)
+{
+       drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops drm_component_ops = {
+       .bind = sprd_drm_bind,
+       .unbind = sprd_drm_unbind,
+};
+
+static int compare_of(struct device *dev, void *data)
+{
+       struct device_node *np = data;
+
+       DRM_DEBUG("compare %s\n", np->full_name);
+
+       return dev->of_node == np;
+}
+
+static int sprd_drm_component_probe(struct device *dev,
+                          const struct component_master_ops *m_ops)

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+{
+       struct device_node *ep, *port, *remote;
+       struct component_match *match = NULL;
+       int i;
+
+       if (!dev->of_node)
+               return -EINVAL;
+
+       /*
+        * Bind the crtc's ports first, so that

drm_of_find_possible_crtcs()

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 13 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +13 -0

+        * called from encoder's .bind callbacks works as expected
+        */
+       for (i = 0; ; i++) {
+               port = of_parse_phandle(dev->of_node, "ports", i);
+               if (!port)
+                       break;
+
+               if (!of_device_is_available(port->parent)) {
+                       of_node_put(port);
+                       continue;
+               }
+
+               component_match_add(dev, &match, compare_of,

port->parent);

Quoted 5 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +5 -0

+               of_node_put(port);
+       }
+
+       if (i == 0) {
+               dev_err(dev, "missing 'ports' property\n");

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+               return -ENODEV;
+       }
+
+       if (!match) {
+               dev_err(dev, "no available port\n");
+               return -ENODEV;
+       }
+
+       /*
+        * For bound crtcs, bind the encoders attached to their remote

endpoint

Quoted 7 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +7 -0

+        */
+       for (i = 0; ; i++) {
+               port = of_parse_phandle(dev->of_node, "ports", i);
+               if (!port)
+                       break;
+
+               if (!of_device_is_available(port->parent)) {

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 10 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +10 -0

+                       of_node_put(port);
+                       continue;
+               }
+
+               for_each_child_of_node(port, ep) {
+                       remote = of_graph_get_remote_port_parent(ep);
+                       if (!remote || !of_device_is_available(remote)) {
+                               of_node_put(remote);
+                               continue;
+                       } else if

(!of_device_is_available(remote->parent)) {

drivers/gpu/drm/sprd/sprd_drm.c line 205

Quoted 1 line

drivers/gpu/drm/sprd/sprd_drm.c new file +1 -0

+                               dev_warn(dev, "parent device of %s is

not available\n",

drivers/gpu/drm/sprd/sprd_drm.c

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+                                        remote->full_name);
+                               of_node_put(remote);
+                               continue;
+                       }
+
+                       component_match_add(dev, &match, compare_of,

remote);

drivers/gpu/drm/sprd/sprd_drm.c line 217

Quoted 6 lines

drivers/gpu/drm/sprd/sprd_drm.c new file +6 -0

+                       of_node_put(remote);
+               }
+               of_node_put(port);
+       }
+
+       return component_master_add_with_match(dev, m_ops, match);

This whole function is effectively a copy of drm_of_component_probe(). Reuse that instead.

With that + comments from Sam addressed this patch is:

Reviewed-by Emil Velikov <[email protected]>

-Emil

From: Kevin Tang <[email protected]>

DPU (Display Processor Unit) is the Display Controller for the Unisoc SoCs which transfers the image data from a video memory buffer to an internal LCD interface.

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/dpu.yaml      | 85 ++++++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sprd/dpu.yaml

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +25 -0

new file mode 100644
index 0000000..7695d94
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sprd/dpu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc SoC Display Processor Unit (DPU)
+
+maintainers:
+  - David Airlie <[email protected]>
+  - Daniel Vetter <[email protected]>
+  - Rob Herring <[email protected]>
+  - Mark Rutland <[email protected]>
+
+description: |
+  DPU (Display Processor Unit) is the Display Controller for the Unisoc SoCs
+  which transfers the image data from a video memory buffer to an internal
+  LCD interface.
+
+properties:
+  compatible:
+    const: sprd,sharkl3-dpu
+
+  reg:
+    maxItems: 1

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +40 -0

+
+  clock-names:
+    items:
+      - const: clk_src_128m
+      - const: clk_src_384m
+
+  power-domains:
+    description: A phandle to DPU power domain node.
+
+  iommus:
+    description: A phandle to DPU iommu node.
+
+  port:
+    type: object
+    description:
+      A port node with endpoint definitions as defined in
+      Documentation/devicetree/bindings/media/video-interfaces.txt.
+      That port should be the output endpoint, usually output to
+      the associated DSI.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/sprd,sc9860-clk.h>
+    dpu: dpu@0x63000000 {
+          compatible = "sprd,sharkl3-dpu";
+          reg = <0x0 0x63000000 0x0 0x1000>;
+          interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+          clock-names = "clk_src_128m",
+      	        "clk_src_384m";

Hi Kevin.

Quoted 3 lines · Kevin Tang

From: Kevin Tang <[email protected]>

DPU (Display Processor Unit) is the Display Controller for the Unisoc

SoCs

Quoted 9 lines

which transfers the image data from a video memory buffer to an internal LCD interface.

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 .../devicetree/bindings/display/sprd/dpu.yaml      | 85

++++++++++++++++++++++

Quoted 2 lines

 1 file changed, 85 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/sprd/dpu.yaml

Review

Quoted 2 lines

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/display/sprd/dpu.yaml

Quoted 11 lines

new file mode 100644
index 0000000..7695d94

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +6 -0

@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/sprd/dpu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+

Documentation/devicetree/bindings/display/sprd/dpu.yaml line 16

Quoted 10 lines

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +10 -0

+title: Unisoc SoC Display Processor Unit (DPU)
+
+maintainers:
+  - David Airlie <[email protected]>
+  - Daniel Vetter <[email protected]>
+  - Rob Herring <[email protected]>
+  - Mark Rutland <[email protected]>
+
+description: |
+  DPU (Display Processor Unit) is the Display Controller for the Unisoc

SoCs

Documentation/devicetree/bindings/display/sprd/dpu.yaml line 17

Quoted 1 line

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +1 -0

+  which transfers the image data from a video memory buffer to an

internal

Quoted 12 lines

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +12 -0

+  LCD interface.
+
+properties:
+  compatible:
+    const: sprd,sharkl3-dpu
+
+  reg:
+    maxItems: 1
+    description:
+      Physical base address and length of the DPU registers set
+
+  interrupts:

Documentation/devicetree/bindings/display/sprd/dpu.yaml line 35

Quoted 6 lines

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +6 -0

+    maxItems: 1
+    description:
+      The interrupt signal from DPU
+
+  clocks:
+    minItems: 2

Should this be maxItems: 2?
That would imply minItems: 2.

Quoted 47 lines

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +45 -0

+
+  clock-names:
+    items:
+      - const: clk_src_128m
+      - const: clk_src_384m
+
+  power-domains:
+    description: A phandle to DPU power domain node.
+
+  iommus:
+    description: A phandle to DPU iommu node.
+
+  port:
+    type: object
+    description:
+      A port node with endpoint definitions as defined in
+      Documentation/devicetree/bindings/media/video-interfaces.txt.
+      That port should be the output endpoint, usually output to
+      the associated DSI.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/sprd,sc9860-clk.h>
+    dpu: dpu@0x63000000 {
+          compatible = "sprd,sharkl3-dpu";
+          reg = <0x0 0x63000000 0x0 0x1000>;
+          interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
+          clock-names = "clk_src_128m",
+                     "clk_src_384m";
+
+          clocks = <&pll CLK_TWPLL_128M>,
+                <&pll CLK_TWPLL_384M>;
+
+          dpu_port: port {

Documentation/devicetree/bindings/display/sprd/dpu.yaml line 85

Quoted 5 lines

Documentation/devicetree/bindings/display/sprd/dpu.yaml new file +5 -0

+                  dpu_out: endpoint {
+                          remote-endpoint = <&dsi_in>;
+                  };
+          };
+    };

Did this example pass dt_binding_check with no warnings? I wonder how the reg property could avoid generating warnings as the upper node do not have #address_cells, #node_cells

Sam

Adds DPU(Display Processor Unit) support for the Unisoc's display subsystem. It's support multi planes, scaler, rotation, PQ(Picture Quality) and more.

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 drivers/gpu/drm/sprd/Makefile       |   6 +-
 drivers/gpu/drm/sprd/disp_lib.c     |  59 +++
 drivers/gpu/drm/sprd/disp_lib.h     |  21 +
 drivers/gpu/drm/sprd/dpu/Makefile   |   7 +
 drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 787 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/sprd/sprd_dpu.c     | 678 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/sprd/sprd_dpu.h     | 130 ++++++
 drivers/gpu/drm/sprd/sprd_drm.c     |   1 +
 drivers/gpu/drm/sprd/sprd_drm.h     |   2 +
 9 files changed, 1690 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/sprd/disp_lib.c
 create mode 100644 drivers/gpu/drm/sprd/disp_lib.h
 create mode 100644 drivers/gpu/drm/sprd/dpu/Makefile
 create mode 100644 drivers/gpu/drm/sprd/dpu/dpu_r2p0.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.h

drivers/gpu/drm/sprd/Makefile +5 -1

index 63b8751..c94c8ac 100644
@@ -4,4 +4,8 @@ ccflags-y += -Iinclude/drm
 
 subdir-ccflags-y += -I$(src)
 
-obj-y := sprd_drm.o
+obj-y := sprd_drm.o \
+       sprd_dpu.o
+
+obj-y += disp_lib.o
+obj-y += dpu/

drivers/gpu/drm/sprd/disp_lib.c new file +0 -0

new file mode 100644
index 0000000..c887822

drivers/gpu/drm/sprd/dpu/Makefile new file +1 -0

+obj-y += dpu_r2p0.o

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +26 -0

new file mode 100644
index 0000000..b96e2e2
@@ -0,0 +1,787 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+#include "sprd_dpu.h"
+
+#define DISPC_INT_FBC_PLD_ERR_MASK     BIT(8)
+#define DISPC_INT_FBC_HDR_ERR_MASK     BIT(9)
+
+#define DISPC_INT_MMU_INV_WR_MASK      BIT(19)
+#define DISPC_INT_MMU_INV_RD_MASK      BIT(18)
+#define DISPC_INT_MMU_VAOR_WR_MASK     BIT(17)
+#define DISPC_INT_MMU_VAOR_RD_MASK     BIT(16)
+
+struct layer_reg {
+       u32 addr[4];
+       u32 ctrl;
+       u32 size;
+       u32 pitch;
+       u32 pos;
+       u32 alpha;
+       u32 ck;

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +12 -0

+enum {
+       DPU_LAYER_FORMAT_YUV422_2PLANE,
+       DPU_LAYER_FORMAT_YUV420_2PLANE,
+       DPU_LAYER_FORMAT_YUV420_3PLANE,
+       DPU_LAYER_FORMAT_ARGB8888,
+       DPU_LAYER_FORMAT_RGB565,
+       DPU_LAYER_FORMAT_XFBC_ARGB8888 = 8,
+       DPU_LAYER_FORMAT_XFBC_RGB565,
+       DPU_LAYER_FORMAT_MAX_TYPES,
+};
+
+enum {

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +2 -0

+static void dpu_dpi_init(struct dpu_context *ctx)
+{

+struct dpu_core_ops sharkl3_dpu_core_ops = {

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +1 -0

+};

drivers/gpu/drm/sprd/sprd_dpu.c new file +0 -0

new file mode 100644
index 0000000..331f236

+ if (dpu->core && dpu->core->init)

Quoted 2 lines · Kevin Tang

Adds DPU(Display Processor Unit) support for the Unisoc's display

subsystem.

Quoted 1 line

It's support multi planes, scaler, rotation, PQ(Picture Quality) and

more.

Quoted 11 lines

Cc Orson Zhai <[email protected]>

Cc Baolin Wang <[email protected]>

Cc Chunyan Zhang <[email protected]>

Signed-off-by Kevin Tang <[email protected]>

---
 drivers/gpu/drm/sprd/Makefile       |   6 +-
 drivers/gpu/drm/sprd/disp_lib.c     |  59 +++
 drivers/gpu/drm/sprd/disp_lib.h     |  21 +
 drivers/gpu/drm/sprd/dpu/Makefile   |   7 +
 drivers/gpu/drm/sprd/dpu/dpu_r2p0.c | 787

++++++++++++++++++++++++++++++++++++

Quoted 1 line

 drivers/gpu/drm/sprd/sprd_dpu.c     | 678

+++++++++++++++++++++++++++++++

Quoted 11 lines

 drivers/gpu/drm/sprd/sprd_dpu.h     | 130 ++++++
 drivers/gpu/drm/sprd/sprd_drm.c     |   1 +
 drivers/gpu/drm/sprd/sprd_drm.h     |   2 +
 9 files changed, 1690 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/sprd/disp_lib.c
 create mode 100644 drivers/gpu/drm/sprd/disp_lib.h
 create mode 100644 drivers/gpu/drm/sprd/dpu/Makefile
 create mode 100644 drivers/gpu/drm/sprd/dpu/dpu_r2p0.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.c
 create mode 100644 drivers/gpu/drm/sprd/sprd_dpu.h

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/Makefile

Review

Quoted 14 lines

index 63b8751..c94c8ac 100644

drivers/gpu/drm/sprd/Makefile +5 -1

@@ -4,4 +4,8 @@ ccflags-y += -Iinclude/drm
 
 subdir-ccflags-y += -I$(src)
 
-obj-y := sprd_drm.o
+obj-y := sprd_drm.o \
+       sprd_dpu.o
+
+obj-y += disp_lib.o
+obj-y += dpu/

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/disp_lib.c

Quoted 4 lines

new file mode 100644
index 0000000..c887822
+++ b/drivers/gpu/drm/sprd/disp_lib.c +++ b/drivers/gpu/drm/sprd/disp_lib.h

These are completely unused. Drop them for now as well as their Makefile hunk.

drivers/gpu/drm/sprd/dpu/Makefile line 6

Quoted 11 lines

drivers/gpu/drm/sprd/dpu/Makefile new file +6 -0

@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ifdef CONFIG_ARM64
+KBUILD_CFLAGS += -mstrict-align
+endif
+

There is only a single case of this in the whole kernel. This is a strong indication that there is something wrong, perhaps. Why do we need this?

Review

Quoted 4 lines

drivers/gpu/drm/sprd/dpu/Makefile new file +1 -0

+obj-y += dpu_r2p0.o

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/dpu/dpu_r2p0.c

Quoted 31 lines

new file mode 100644
index 0000000..b96e2e2

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +26 -0

@@ -0,0 +1,787 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Unisoc Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+#include "sprd_dpu.h"
+
+#define DISPC_INT_FBC_PLD_ERR_MASK     BIT(8)
+#define DISPC_INT_FBC_HDR_ERR_MASK     BIT(9)
+
+#define DISPC_INT_MMU_INV_WR_MASK      BIT(19)
+#define DISPC_INT_MMU_INV_RD_MASK      BIT(18)
+#define DISPC_INT_MMU_VAOR_WR_MASK     BIT(17)
+#define DISPC_INT_MMU_VAOR_RD_MASK     BIT(16)
+
+struct layer_reg {
+       u32 addr[4];
+       u32 ctrl;
+       u32 size;
+       u32 pitch;
+       u32 pos;
+       u32 alpha;
+       u32 ck;

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 36

Quoted 10 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +10 -0

+       u32 pallete;
+       u32 crop_start;
+};
+
+struct wb_region_reg {
+       u32 pos;
+       u32 size;
+};
+
+struct dpu_reg {

...

There are actual HW registers and we get the base via ioremap_nocache(). Please add a small comment.

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 140

Quoted 5 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +5 -0

+static DECLARE_WAIT_QUEUE_HEAD(wait_queue);
+static bool panel_ready = true;
+static bool evt_update;
+static bool evt_stop;
+

Using static variables tend to be an example of badly designed code.

...

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 348

Quoted 1 line

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +1 -0

+static void dpu_uninit(struct dpu_context *ctx)

Normally people use init - for initialization and fini for "uninitialization"

Quoted 19 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +19 -0

+enum {
+       DPU_LAYER_FORMAT_YUV422_2PLANE,
+       DPU_LAYER_FORMAT_YUV420_2PLANE,
+       DPU_LAYER_FORMAT_YUV420_3PLANE,
+       DPU_LAYER_FORMAT_ARGB8888,
+       DPU_LAYER_FORMAT_RGB565,
+       DPU_LAYER_FORMAT_XFBC_ARGB8888 = 8,
+       DPU_LAYER_FORMAT_XFBC_RGB565,
+       DPU_LAYER_FORMAT_MAX_TYPES,
+};
+
+enum {
+       DPU_LAYER_ROTATION_0,
+       DPU_LAYER_ROTATION_90,
+       DPU_LAYER_ROTATION_180,
+       DPU_LAYER_ROTATION_270,
+       DPU_LAYER_ROTATION_0_M,
+       DPU_LAYER_ROTATION_90_M,
+       DPU_LAYER_ROTATION_180_M,

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 379

Quoted 3 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +3 -0

+       DPU_LAYER_ROTATION_270_M,
+};
+

Starting with one format and rotation is good. Others can be added as follow-up.
Especially since FBC is something that is just becoming fashionable.

Quoted 3 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +2 -0

+static void dpu_dpi_init(struct dpu_context *ctx)
+{

...

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 712

Quoted 9 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +9 -0

+       } else if (ctx->if_type == SPRD_DISPC_IF_EDPI) {
+               /* use edpi as interface */
+               reg->dpu_cfg0 |= BIT(0);
+
+               /* use external te */
+               reg->dpi_ctrl |= BIT(10);
+
+               /* enable te */
+               reg->dpi_ctrl |= BIT(8);

Try to avoid using BIT() randomly. A well chosen name, removes the need for a
comment. Perhaps it's just me but the "enable foo" comments do not help much.

...

Quoted 1 line

+struct dpu_core_ops sharkl3_dpu_core_ops = {

In general ops should be const.

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c line 786

Quoted 12 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +12 -0

+       .version = dpu_get_version,
+       .init = dpu_init,
+       .uninit = dpu_uninit,
+       .run = dpu_run,
+       .stop = dpu_stop,
+       .isr = dpu_isr,
+       .ifconfig = dpu_dpi_init,
+       .capability = dpu_capability,
+       .flip = dpu_flip,
+       .bg_color = dpu_bgcolor,
+       .enable_vsync = enable_vsync,
+       .disable_vsync = disable_vsync,

... then again, we have only a single set of dpu_core_ops. So the whole abstraction layer seems unnessesary.

Review

Quoted 6 lines

drivers/gpu/drm/sprd/dpu/dpu_r2p0.c new file +1 -0

+};

(quoted diff) +0 -0

b/drivers/gpu/drm/sprd/sprd_dpu.c

drivers/gpu/drm/sprd/sprd_dpu.c line 42

Quoted 12 lines

new file mode 100644
index 0000000..331f236

drivers/gpu/drm/sprd/sprd_dpu.c new file +5 -0

 
 
 
+static int sprd_plane_atomic_check(struct drm_plane *plane,
+                                 struct drm_plane_state *state)
+{
+       DRM_DEBUG("%s()\n", __func__);
+

This does not feel right. Here the driver must ensure that the state given will
always work.

Is the hardware so versatile that any permutation (given by userspace) will work?

Same goes for the other atomic_check functions that are no-op.

drivers/gpu/drm/sprd/sprd_dpu.c

Quoted 11 lines

drivers/gpu/drm/sprd/sprd_dpu.c new file +9 -0

+static void sprd_plane_atomic_disable(struct drm_plane *plane,
+                                    struct drm_plane_state *old_state)
+{
+       struct sprd_plane *p = to_sprd_plane(plane);
+
+       /*
+        * NOTE:
+        * The dpu->core->flip() will disable all the planes each time.
+        * So there is no need to impliment the atomic_disable()

function.

drivers/gpu/drm/sprd/sprd_dpu.c line 101

Quoted 4 lines

drivers/gpu/drm/sprd/sprd_dpu.c new file +4 -0

+        * But this function can not be removed, because it will change
+        * to call atomic_update() callback instead. Which will cause
+        * kernel panic in sprd_plane_atomic_update().
+        *

Why do we disable all the planes on flip? This is the first time I see such driver decision.