Hi,

On 26 February 2016 at 16:54, Archit Taneja [off-list ref] wrote:

Hi,

I had some minor comments. Sorry about sharing this late. Otherwise,
the looks good to me.

Hi Archit,  you are welcome :-)
Thanks for review again.

On 02/26/2016 02:10 PM, Xinliang Liu wrote:

quoted

Add kirin DRM master driver for hi6220 SoC which used in HiKey board.
Add dumb buffer feature.
Add prime dmabuf feature.

v6: None.
v5: None.
v4: None.
v3:
- Move and rename all the files to kirin sub-directory.
   So that we could separate different seires SoCs' driver.
- Replace drm_platform_init, load, unload implementation.
v2:
- Remove abtraction layer.

Signed-off-by: Xinliang Liu <xinliang.liu@linaro.org>
---
  drivers/gpu/drm/Kconfig                         |   2 +
  drivers/gpu/drm/Makefile                        |   1 +
  drivers/gpu/drm/hisilicon/Kconfig               |   5 +
  drivers/gpu/drm/hisilicon/Makefile              |   5 +
  drivers/gpu/drm/hisilicon/kirin/Kconfig         |   9 +
  drivers/gpu/drm/hisilicon/kirin/Makefile        |   3 +
  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 321
++++++++++++++++++++++++
  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h |  20 ++
  8 files changed, 366 insertions(+)
  create mode 100644 drivers/gpu/drm/hisilicon/Kconfig
  create mode 100644 drivers/gpu/drm/hisilicon/Makefile
  create mode 100644 drivers/gpu/drm/hisilicon/kirin/Kconfig
  create mode 100644 drivers/gpu/drm/hisilicon/kirin/Makefile
  create mode 100644 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
  create mode 100644 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index b50ae60f5f50..f5c5656e2547 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig

@@ -277,3 +277,5 @@ source "drivers/gpu/drm/imx/Kconfig"
  source "drivers/gpu/drm/vc4/Kconfig"

  source "drivers/gpu/drm/etnaviv/Kconfig"
+
+source "drivers/gpu/drm/hisilicon/Kconfig"

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 61766dec6a8d..60554832079c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile

@@ -74,3 +74,4 @@ obj-y                 += panel/
  obj-y                 += bridge/
  obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
  obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
+obj-y                  += hisilicon/

diff --git a/drivers/gpu/drm/hisilicon/Kconfig

b/drivers/gpu/drm/hisilicon/Kconfig
new file mode 100644
index 000000000000..558c61b1b8e8

--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/Kconfig

@@ -0,0 +1,5 @@
+#
+# hisilicon drm device configuration.
+# Please keep this list sorted alphabetically
+
+source "drivers/gpu/drm/hisilicon/kirin/Kconfig"

diff --git a/drivers/gpu/drm/hisilicon/Makefile

b/drivers/gpu/drm/hisilicon/Makefile
new file mode 100644
index 000000000000..e3f6d493c996

--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/Makefile

@@ -0,0 +1,5 @@
+#
+# Makefile for hisilicon drm drivers.
+# Please keep this list sorted alphabetically
+
+obj-$(CONFIG_DRM_HISI_KIRIN) += kirin/

diff --git a/drivers/gpu/drm/hisilicon/kirin/Kconfig

b/drivers/gpu/drm/hisilicon/kirin/Kconfig
new file mode 100644
index 000000000000..3ac4b8edeac1

--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/kirin/Kconfig

@@ -0,0 +1,9 @@
+config DRM_HISI_KIRIN
+       tristate "DRM Support for Hisilicon Kirin series SoCs Platform"
+       depends on DRM
+       select DRM_KMS_HELPER
+       select DRM_GEM_CMA_HELPER
+       select DRM_KMS_CMA_HELPER
+       help
+         Choose this option if you have a hisilicon Kirin

chipsets(hi6220).
+         If M is selected the module will be called kirin-drm.

diff --git a/drivers/gpu/drm/hisilicon/kirin/Makefile

b/drivers/gpu/drm/hisilicon/kirin/Makefile
new file mode 100644
index 000000000000..cb346de47d48

--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/kirin/Makefile

@@ -0,0 +1,3 @@
+kirin-drm-y := kirin_drm_drv.o
+
+obj-$(CONFIG_DRM_HISI_KIRIN) += kirin-drm.o

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c

b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
new file mode 100644
index 000000000000..789ebd1f5922

--- /dev/null
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c

@@ -0,0 +1,321 @@
+/*
+ * Hisilicon Kirin SoCs drm master driver
+ *
+ * Copyright (c) 2016 Linaro Limited.
+ * Copyright (c) 2014-2016 Hisilicon Limited.
+ *
+ * Author:
+ *     Xinliang Liu <z.liuxinliang@hisilicon.com>
+ *     Xinliang Liu <xinliang.liu@linaro.org>
+ *     Xinwei Kong <kong.kongxinwei@hisilicon.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/of_platform.h>
+#include <linux/component.h>
+#include <linux/of_graph.h>
+
+#include <drm/drmP.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_atomic_helper.h>
+
+#include "kirin_drm_drv.h"
+
+static struct kirin_dc_ops *dc_ops;
+
+static int kirin_drm_kms_cleanup(struct drm_device *dev)
+{
+       dc_ops->cleanup(dev);
+       drm_mode_config_cleanup(dev);
+
+       return 0;
+}
+
+static const struct drm_mode_config_funcs kirin_drm_mode_config_funcs = {
+       .fb_create = drm_fb_cma_create,
+       .atomic_check = drm_atomic_helper_check,
+       .atomic_commit = drm_atomic_helper_commit,
+};
+
+static void kirin_drm_mode_config_init(struct drm_device *dev)
+{
+       dev->mode_config.min_width = 0;
+       dev->mode_config.min_height = 0;
+
+       dev->mode_config.max_width = 2048;
+       dev->mode_config.max_height = 2048;
+
+       dev->mode_config.funcs = &kirin_drm_mode_config_funcs;
+}
+
+static int kirin_drm_kms_init(struct drm_device *dev)
+{
+       int ret;
+
+       dev_set_drvdata(dev->dev, dev);
+
+       /* dev->mode_config initialization */
+       drm_mode_config_init(dev);
+       kirin_drm_mode_config_init(dev);
+
+       /* display controller init */
+       ret = dc_ops->init(dev);
+       if (ret)
+               goto err_mode_config_cleanup;
+
+       /* bind and init sub drivers */
+       ret = component_bind_all(dev->dev, dev);
+       if (ret) {
+               DRM_ERROR("failed to bind all component.\n");
+               goto err_dc_cleanup;
+       }
+
+       /* reset all the states of crtc/plane/encoder/connector */
+       drm_mode_config_reset(dev);
+
+       return 0;
+
+err_dc_cleanup:
+       dc_ops->cleanup(dev);
+err_mode_config_cleanup:
+       drm_mode_config_cleanup(dev);
+
+       return ret;
+}
+
+static const struct file_operations kirin_drm_fops = {
+       .owner          = THIS_MODULE,
+       .open           = drm_open,
+       .release        = drm_release,
+       .unlocked_ioctl = drm_ioctl,
+#ifdef CONFIG_COMPAT
+       .compat_ioctl   = drm_compat_ioctl,
+#endif
+       .poll           = drm_poll,
+       .read           = drm_read,
+       .llseek         = no_llseek,
+       .mmap           = drm_gem_cma_mmap,
+};
+
+static int kirin_gem_cma_dumb_create(struct drm_file *file,
+                                    struct drm_device *dev,
+                                    struct drm_mode_create_dumb *args)
+{
+       return drm_gem_cma_dumb_create_internal(file, dev, args);
+}
+
+static struct drm_driver kirin_drm_driver = {
+       .driver_features        = DRIVER_GEM | DRIVER_MODESET |

DRIVER_PRIME |
+                                 DRIVER_ATOMIC,
+       .fops                   = &kirin_drm_fops,
+       .set_busid              = drm_platform_set_busid,
+
+       .gem_free_object        = drm_gem_cma_free_object,
+       .gem_vm_ops             = &drm_gem_cma_vm_ops,
+       .dumb_create            = kirin_gem_cma_dumb_create,
+       .dumb_map_offset        = drm_gem_cma_dumb_map_offset,
+       .dumb_destroy           = drm_gem_dumb_destroy,
+
+       .prime_handle_to_fd     = drm_gem_prime_handle_to_fd,
+       .prime_fd_to_handle     = drm_gem_prime_fd_to_handle,
+       .gem_prime_export       = drm_gem_prime_export,
+       .gem_prime_import       = drm_gem_prime_import,
+       .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
+       .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
+       .gem_prime_vmap         = drm_gem_cma_prime_vmap,
+       .gem_prime_vunmap       = drm_gem_cma_prime_vunmap,
+       .gem_prime_mmap         = drm_gem_cma_prime_mmap,
+
+       .name                   = "kirin",
+       .desc                   = "Hisilicon Kirin SoCs' DRM Driver",
+       .date                   = "20150718",
+       .major                  = 1,
+       .minor                  = 0,
+};
+
+static int compare_of(struct device *dev, void *data)
+{
+       return dev->of_node == data;
+}
+
+static int kirin_drm_connectors_register(struct drm_device *dev)
+{
+       struct drm_connector *connector;
+       struct drm_connector *failed_connector;
+       int ret;
+

This access needs to be protected by the dev->mode_config.mutex

thanks for pointing out this. will fix in next version.

quoted

+       list_for_each_entry(connector, &dev->mode_config.connector_list,
head) {

The above can be replaced with:

drm_for_each_connector(connector, dev) {

Great, this makes things more clear. will change in next version.

quoted

+               ret = drm_connector_register(connector);
+               if (ret) {
+                       failed_connector = connector;
+                       goto err;
+               }
+       }
+
+       return 0;
+
+err:
+       list_for_each_entry(connector, &dev->mode_config.connector_list,
head) {
+               if (failed_connector == connector)
+                       break;
+               drm_connector_unregister(connector);
+       }

Same points as above.

quoted

+
+       return ret;
+}
+
+static int kirin_drm_bind(struct device *dev)
+{
+       struct drm_driver *driver = &kirin_drm_driver;
+       struct drm_device *drm_dev;
+       int ret;
+
+       drm_dev = drm_dev_alloc(driver, dev);
+       if (!drm_dev)
+               return -ENOMEM;
+
+       drm_dev->platformdev = to_platform_device(dev);
+
+       ret = kirin_drm_kms_init(drm_dev);
+       if (ret)
+               goto err_drm_dev_unref;
+
+       ret = drm_dev_register(drm_dev, 0);
+       if (ret)
+               goto err_kms_cleanup;
+
+       /* connectors should be registered after drm device register */
+       ret = kirin_drm_connectors_register(drm_dev);
+       if (ret)
+               goto err_drm_dev_unregister;
+
+       DRM_INFO("Initialized %s %d.%d.%d %s on minor %d\n",
+                driver->name, driver->major, driver->minor,
driver->patchlevel,
+                driver->date, drm_dev->primary->index);
+
+       return 0;
+
+err_drm_dev_unregister:
+       drm_dev_unregister(drm_dev);
+err_kms_cleanup:
+       kirin_drm_kms_cleanup(drm_dev);
+err_drm_dev_unref:
+       drm_dev_unref(drm_dev);
+
+       return ret;
+}
+
+static void kirin_drm_unbind(struct device *dev)
+{
+       drm_put_dev(dev_get_drvdata(dev));
+}
+
+static const struct component_master_ops kirin_drm_ops = {
+       .bind = kirin_drm_bind,
+       .unbind = kirin_drm_unbind,
+};
+
+static const struct of_device_id kirin_drm_dt_ids[] = {
+       { /* end node */ },
+};
+MODULE_DEVICE_TABLE(of, kirin_drm_dt_ids);
+
+static int kirin_get_dc_ops(struct device_node *np)
+{
+       const struct of_device_id *match_id;
+
+       match_id = of_match_node(kirin_drm_dt_ids, np);
+       if (!match_id) {
+               DRM_ERROR("failed to match dt id\n");
+               return -EINVAL;
+       }
+
+       dc_ops = (struct kirin_dc_ops *)match_id->data;
+       if (!dc_ops) {
+               DRM_ERROR("dt id data is null\n");
+               return -EINVAL;
+       }

We can use of_device_get_match_data to get the match data,
and move the kirin_drm_dt_ids table down where the driver is
defined.

Same, it is better to do like you said. will change in next version.

Thanks,
-xinliang

<snip>

Thanks,
Archit

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation