| Message ID | 1582271336-3708-3-git-send-email-kevin3.tang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Unisoc's drm kms module | expand |
Hi Kevin. On Fri, Feb 21, 2020 at 03:48:52PM +0800, Kevin Tang wrote: > From: Kevin Tang <kevin.tang@unisoc.com> > > 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 <leon.he@unisoc.com>"); > +MODULE_AUTHOR("Kevin Tang <kevin.tang@unisoc.com>"); > +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 > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Sat, Feb 22, 2020 at 5:36 AM Sam Ravnborg <sam@ravnborg.org> wrote: > Hi Kevin. > > On Fri, Feb 21, 2020 at 03:48:52PM +0800, Kevin Tang wrote: > > From: Kevin Tang <kevin.tang@unisoc.com> > > > > 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. > Yes, copy from drm/Kconfig, but we have tested ok on android hwcomposer. It is indeed copied from outdated content, need to fix it? > > > > +++ 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. > Ok, i will fix it. > > > > \ 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. > Ok, this can be delete, thanks for reminding. > > > + > > +subdir-ccflags-y += -I$(src) > Not required - delete. > This maybe can't, because our dpu subdir need it to found parent dir header file. > > + > > +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. > 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. > > > + > > + 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 > modern drm drivers need drm_xxx to replace 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. > Our display controller isr is been register on crtc driver(sprd_dpu.c), not here. > > > + > > + /* 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(). > Ok, i will fix it > > > +} > > + > > +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? > Ok, i will drop useless debug log. > > > + > > + 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. > It's right mask for us. > > > + 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. > Ok, i will fix it > > > +}; > > +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 <leon.he@unisoc.com>"); > > +MODULE_AUTHOR("Kevin Tang <kevin.tang@unisoc.com>"); > > +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 > > dri-devel@lists.freedesktop.org > > 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
On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <sam@ravnborg.org> 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. > + > + /* 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 > >
On Sun, Feb 23, 2020 at 12:26 PM tang pengchuan <kevin3.tang@gmail.com> wrote: > > > On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <sam@ravnborg.org> 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. > But I've always been confused, because the irq is initialized in the sub-device code, but the device state is set by drm_drv. Everyone else seems to do like this. why can't for me? Can you help to tell the reason in detail, looking forward to your answers. BR > > 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 >> >>
Hi all, On Fri, 21 Feb 2020 at 11:15, Kevin Tang <kevin3.tang@gmail.com> wrote: > > From: Kevin Tang <kevin.tang@unisoc.com> > > 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 <orsonzhai@gmail.com> > Cc: Baolin Wang <baolin.wang@linaro.org> > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > Signed-off-by: Kevin Tang <kevin.tang@unisoc.com> > --- > 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 <emil.velikov@collabora.com> -Emil
Got it, thanks! On Tue, Feb 25, 2020 at 12:43 AM Emil Velikov <emil.l.velikov@gmail.com> wrote: > Hi all, > > On Fri, 21 Feb 2020 at 11:15, Kevin Tang <kevin3.tang@gmail.com> wrote: > > > > From: Kevin Tang <kevin.tang@unisoc.com> > > > > 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 <orsonzhai@gmail.com> > > Cc: Baolin Wang <baolin.wang@linaro.org> > > Cc: Chunyan Zhang <zhang.lyra@gmail.com> > > Signed-off-by: Kevin Tang <kevin.tang@unisoc.com> > > --- > > 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 <emil.velikov@collabora.com> > > -Emil >
Hi Am 23.02.20 um 05:26 schrieb tang pengchuan: > > > On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <sam@ravnborg.org > <mailto:sam@ravnborg.org>> 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 > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, Feb 25, 2020 at 3:38 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Hi > > Am 23.02.20 um 05:26 schrieb tang pengchuan: > > > > > > On Sun, Feb 23, 2020 at 5:27 AM Sam Ravnborg <sam@ravnborg.org > > <mailto:sam@ravnborg.org>> 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? > Hi Thomas, The the irq is initialized in the sub-device code, but the device state is set on kms driver. E.g Here is the device state set on kms driver: 206 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#206> *static* *int* mtk_drm_kms_init <http://10.0.1.79:8081/s?refs=mtk_drm_kms_init&project=sprdroidr_trunk>(*struct* drm_device <http://10.0.1.79:8081/s?defs=drm_device&project=sprdroidr_trunk> *drm <http://10.0.1.79:8081/s?refs=drm&project=sprdroidr_trunk>) ...... 298 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#298> /*299 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#299> * We don't use the drm_irq_install() helpers provided by the DRM300 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#300> * core, so we need to set this manually in order to allow the301 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#301> * DRM_IOCTL_WAIT_VBLANK to operate correctly.302 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#302> */303 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_drm_drv.c#303> drm <http://10.0.1.79:8081/s?defs=drm&project=sprdroidr_trunk>->irq_enabled <http://10.0.1.79:8081/s?defs=irq_enabled&project=sprdroidr_trunk> = *true*; Here is irq install on subdev: 265 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#265> *static* *int* mtk_disp_rdma_probe <http://10.0.1.79:8081/s?refs=mtk_disp_rdma_probe&project=sprdroidr_trunk>(*struct* platform_device <http://10.0.1.79:8081/s?defs=platform_device&project=sprdroidr_trunk> *pdev <http://10.0.1.79:8081/s?refs=pdev&project=sprdroidr_trunk>) ...... 298 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#298> ret <http://10.0.1.79:8081/s?defs=ret&project=sprdroidr_trunk> = devm_request_irq <http://10.0.1.79:8081/s?defs=devm_request_irq&project=sprdroidr_trunk>(dev <http://10.0.1.79:8081/s?defs=dev&project=sprdroidr_trunk>, irq <http://10.0.1.79:8081/s?defs=irq&project=sprdroidr_trunk>, mtk_disp_rdma_irq_handler <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#mtk_disp_rdma_irq_handler>,299 <http://10.0.1.79:8081/xref/sprdroidr_trunk/bsp/kernel/kernel5.4/drivers/gpu/drm/mediatek/mtk_disp_rdma.c#299> IRQF_TRIGGER_NONE <http://10.0.1.79:8081/s?defs=IRQF_TRIGGER_NONE&project=sprdroidr_trunk>, dev_name <http://10.0.1.79:8081/s?defs=dev_name&project=sprdroidr_trunk>(dev <http://10.0.1.79:8081/s?defs=dev&project=sprdroidr_trunk>), priv <http://10.0.1.79:8081/s?defs=priv&project=sprdroidr_trunk>); I'm not sure if my understanding is wrong... > 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 > > dri-devel@lists.freedesktop.org > > 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 > >
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 <leon.he@unisoc.com>"); +MODULE_AUTHOR("Kevin Tang <kevin.tang@unisoc.com>"); +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_ */