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
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index bfdadc3..cead12c 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -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
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9f1c7c4..85ca211 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -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/
diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
new file mode 100644
index 0000000..79f286b
--- /dev/null
+++ b/drivers/gpu/drm/sprd/Kconfig
@@ -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
diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile
new file mode 100644
index 0000000..63b8751
--- /dev/null
+++ b/drivers/gpu/drm/sprd/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+ccflags-y += -Iinclude/drm
+
+subdir-ccflags-y += -I$(src)
+
+obj-y := sprd_drm.o
diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
new file mode 100644
index 0000000..7cac098
--- /dev/null
+++ b/drivers/gpu/drm/sprd/sprd_drm.c
@@ -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);
+ continue;
+ }
+
+ component_match_add(dev, &match, compare_of, remote);
+ 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);
+ 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);
+}
+
+static const struct of_device_id drm_match_table[] = {
+ { .compatible = "sprd,display-subsystem", },
+ {},
+};
+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");
diff --git a/drivers/gpu/drm/sprd/sprd_drm.h b/drivers/gpu/drm/sprd/sprd_drm.h
new file mode 100644
index 0000000..137cb27
--- /dev/null
+++ b/drivers/gpu/drm/sprd/sprd_drm.h
@@ -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
Hi Kevin.
On Fri, Feb 21, 2020 at 03:48:52PM +0800, Kevin Tang wrote:
> 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
>
> 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.
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -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.
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.
> \ No newline at end of file
Please fix.
> diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile
> new file mode 100644
> index 0000000..63b8751
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-y += -Iinclude/drm
Not required - delete.
> +
> +subdir-ccflags-y += -I$(src)
Not required - delete.
> +
> +obj-y := sprd_drm.o
> diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
> new file mode 100644
> index 0000000..7cac098
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c
> @@ -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"
We are in 2020 now..
> +#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);
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.
> +
> + 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;
> + }
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
> +
> + /* 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;
I cannot see any irq being installed. This looks wrong.
> +
> + /* 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;
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().
> +}
> +
> +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);
Leftover debugging that can be dropped?
> +
> + 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);
> + }
> +
> + 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.
> + 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);
> +}
> +
> +static const struct of_device_id drm_match_table[] = {
> + { .compatible = "sprd,display-subsystem", },
> + {},
Sometimes we use { /* sentinel */ },
here.
> +};
> +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");
> diff --git a/drivers/gpu/drm/sprd/sprd_drm.h b/drivers/gpu/drm/sprd/sprd_drm.h
> new file mode 100644
> index 0000000..137cb27
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.h
> @@ -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
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
Sam
Hi all,
On Fri, 21 Feb 2020 at 11:15, Kevin Tang <[email protected]> wrote:
>
> 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
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index bfdadc3..cead12c 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -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
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f1c7c4..85ca211 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -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/
> diff --git a/drivers/gpu/drm/sprd/Kconfig b/drivers/gpu/drm/sprd/Kconfig
> new file mode 100644
> index 0000000..79f286b
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Kconfig
> @@ -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
> diff --git a/drivers/gpu/drm/sprd/Makefile b/drivers/gpu/drm/sprd/Makefile
> new file mode 100644
> index 0000000..63b8751
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ccflags-y += -Iinclude/drm
> +
> +subdir-ccflags-y += -I$(src)
> +
> +obj-y := sprd_drm.o
> diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
> new file mode 100644
> index 0000000..7cac098
> --- /dev/null
> +++ b/drivers/gpu/drm/sprd/sprd_drm.c
> @@ -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);
> + continue;
> + }
> +
> + component_match_add(dev, &match, compare_of, remote);
> + 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
Hi
Am 23.02.20 um 05:26 schrieb tang pengchuan:
>
>
> 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.
I have been busy last week. Sorry for not getting back to you.
>
> 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
> 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.
>
>> +
>> + /* 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
>
--
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