Hi,
On 26 February 2016 at 16:54, Archit Taneja <architt@xxxxxxxxxxxxxx> 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:
>>
>> 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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxx>
>> + * Xinliang Liu <xinliang.liu@xxxxxxxxxx>
>> + * Xinwei Kong <kong.kongxinwei@xxxxxxxxxxxxx>
>> + *
>> + * 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.
>
>> + 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.
>
>> + 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.
>
>
>> +
>> + 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
