Hi Jyri. Took a quick look - looks like a very nice driver. Some drive by comments below. I was left with the impression that there is a lot of code to support vblank - and I wonder if there is some general code that can help you here that you have missed. Sam On Tue, Nov 26, 2019 at 11:53:06AM +0200, Jyri Sarha wrote: > This patch adds a new DRM driver for Texas Instruments DSS IPs used on > Texas Instruments Keystone K2G, AM65x, and J721e SoCs. The new DSS IP is > a major change to the older DSS IP versions, which are supported by > the omapdrm driver. While on higher level the Keystone DSS resembles > the older DSS versions, the registers are completely different and the > internal pipelines differ a lot. > > DSS IP found on K2G is an "ultra-light" version, and has only a single > plane and a single output. The Keystone 3 DSS IPs are found on AM65x > and J721E SoCs. AM65x DSS has two video ports, one full video plane, > and another "lite" plane without scaling support. J721E has 4 video > ports, 2 video planes and 2 lite planes. AM65x DSS has also integrated > OLDI (LVDS) output. > > Co-developed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > Signed-off-by: Jyri Sarha <jsarha@xxxxxx> > +++ b/drivers/gpu/drm/tidss/Kconfig > @@ -0,0 +1,15 @@ > +config DRM_TIDSS > + tristate "DRM Support for TI Keystone" > + depends on DRM && OF > + depends on ARM || ARM64 || COMPILE_TEST > + select DRM_KMS_HELPER > + select DRM_KMS_CMA_HELPER > + select DRM_GEM_CMA_HELPER > + select VIDEOMODE_HELPERS VIDEOMODE_HELPERS seems unused. > + help > + The TI Keystone family SoCs introduced a new generation of > + Display SubSystem. There is currently three Keystone family > + SoCs released with DSS. Each with somewhat different version > + of it. The SoCs are 66AK2Gx, AM65x, and J721E. Set this to Y > + or M to add display support for TI Keystone family > + platforms. > +++ b/drivers/gpu/drm/tidss/tidss_crtc.c > @@ -0,0 +1,373 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#include <drm/drmP.h> This file is no longer present in the tree. > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_fb_cma_helper.h> > +#include <drm/drm_gem_cma_helper.h> > +#include <drm/drm_plane_helper.h> > + > +#include "tidss_crtc.h" > +#include "tidss_dispc.h" > +#include "tidss_drv.h" > +#include "tidss_irq.h" > + > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_crtc.h > @@ -0,0 +1,45 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#ifndef __TIDSS_CRTC_H__ > +#define __TIDSS_CRTC_H__ > + > +#include <drm/drm_crtc.h> > +#include <linux/completion.h> > +#include <linux/wait.h> The normal way to structure header files are: #include <linux/*.h> #include <drm/*.h> with each block sorted alphabetically. > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_dispc.c > @@ -0,0 +1,2645 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2016-2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Jyri Sarha <jsarha@xxxxxx> > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/dma-mapping.h> > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/of_graph.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > + > +#include <drm/drm_fourcc.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_fb_cma_helper.h> > +#include <drm/drm_gem_cma_helper.h> Blocks are fine, but sort the include files. > + > +#include "tidss_drv.h" > +#include "tidss_crtc.h" > +#include "tidss_plane.h" > +#include "tidss_irq.h" > + > +#include "tidss_scale_coefs.h" > +#include "tidss_dispc_regs.h" > + > +#include "tidss_dispc.h" > + > + > +enum c8_to_c12_mode { C8_TO_C12_REPLICATE, C8_TO_C12_MAX, C8_TO_C12_MIN }; > + > +static u16 c8_to_c12(u8 c8, enum c8_to_c12_mode mode) > +{ > + u16 c12; > + > + c12 = c8 << 4; > + > + switch (mode) { > + case C8_TO_C12_REPLICATE: > + /* Copy c8 4 MSB to 4 LSB for full scale c12 */ > + c12 |= c8 >> 4; > + break; > + case C8_TO_C12_MAX: > + c12 |= 0xF; > + break; > + default: > + case C8_TO_C12_MIN: > + break; > + } > + > + return c12; > +} > + > +static u64 argb8888_to_argb12121212(u32 argb8888, enum c8_to_c12_mode m) > +{ > + u8 a, r, g, b; > + u64 v; > + > + a = (argb8888 >> 24) & 0xff; > + r = (argb8888 >> 16) & 0xff; > + g = (argb8888 >> 8) & 0xff; > + b = (argb8888 >> 0) & 0xff; > + > + v = ((u64)c8_to_c12(a, m) << 36) | ((u64)c8_to_c12(r, m) << 24) | > + ((u64)c8_to_c12(g, m) << 12) | (u64)c8_to_c12(b, m); > + > + return v; > +} This looks like a helper that could be useful for others. Maybe move up one level? > diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c > new file mode 100644 > index 000000000000..ba13b530214f > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_drv.c > @@ -0,0 +1,292 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#include <linux/console.h> > +#include <linux/of_device.h> > +#include <linux/pm_runtime.h> > + > +#include <drm/drmP.h> This file is no longer present. > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_fb_helper.h> > +#include <drm/drm_gem_cma_helper.h> > +#include <drm/drm_probe_helper.h> > + > +#include "tidss_dispc.h" > +#include "tidss_drv.h" > +#include "tidss_kms.h" > +#include "tidss_irq.h" > + > +/* Power management */ > + > +int tidss_runtime_get(struct tidss_device *tidss) > +{ > + int r; > + > + dev_dbg(tidss->dev, "%s\n", __func__); > + > + r = pm_runtime_get_sync(tidss->dev); > + WARN_ON(r < 0); > + return r < 0 ? r : 0; > +} > + > +void tidss_runtime_put(struct tidss_device *tidss) > +{ > + int r; > + > + dev_dbg(tidss->dev, "%s\n", __func__); > + > + r = pm_runtime_put_sync(tidss->dev); > + WARN_ON(r < 0); > +} > + > +#ifdef CONFIG_PM Use __maybe_unused here - then you can drop the #ifdef > + > +static int tidss_pm_runtime_suspend(struct device *dev) > +{ > + struct tidss_device *tidss = dev_get_drvdata(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + return dispc_runtime_suspend(tidss->dispc); > +} > + > +static int tidss_pm_runtime_resume(struct device *dev) > +{ > + struct tidss_device *tidss = dev_get_drvdata(dev); > + int r; > + > + dev_dbg(dev, "%s\n", __func__); > + > + r = dispc_runtime_resume(tidss->dispc); > + if (r) > + return r; > + > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int tidss_suspend(struct device *dev) > +{ > + struct tidss_device *tidss = dev_get_drvdata(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + return drm_mode_config_helper_suspend(tidss->ddev); > +} > + > +static int tidss_resume(struct device *dev) > +{ > + struct tidss_device *tidss = dev_get_drvdata(dev); > + > + dev_dbg(dev, "%s\n", __func__); > + > + return drm_mode_config_helper_resume(tidss->ddev); > +} > +#endif /* CONFIG_PM_SLEEP */ > + > +static const struct dev_pm_ops tidss_pm_ops = { > + .runtime_suspend = tidss_pm_runtime_suspend, > + .runtime_resume = tidss_pm_runtime_resume, > + SET_SYSTEM_SLEEP_PM_OPS(tidss_suspend, tidss_resume) > +}; > + > +#endif /* CONFIG_PM */ > + > +/* Platform driver */ > + > +/* DRM device Information */ > +DEFINE_DRM_GEM_CMA_FOPS(tidss_fops); > + > +static struct drm_driver tidss_driver = { > + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC | > + DRIVER_HAVE_IRQ, DRIVER_HAVE_IRQ is only for legacy drivers. In general - look through the comments in drm_drv.h - and remove use of all obsoleted callbacks. I checked .gem_free_object_unlocked - it is deprecated. I guess there is more. > + .gem_free_object_unlocked = drm_gem_cma_free_object, > + .gem_vm_ops = &drm_gem_cma_vm_ops, > + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > + .gem_prime_import = drm_gem_prime_import, > + .gem_prime_export = drm_gem_prime_export, > + .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, > + .dumb_create = drm_gem_cma_dumb_create, > + .fops = &tidss_fops, > + .name = "tidss", > + .desc = "TI Keystone DSS", > + .date = "20180215", > + .major = 1, > + .minor = 0, > + > + .irq_preinstall = tidss_irq_preinstall, > + .irq_postinstall = tidss_irq_postinstall, > + .irq_handler = tidss_irq_handler, > + .irq_uninstall = tidss_irq_uninstall, > +}; > + > +static int tidss_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct tidss_device *tidss; > + struct drm_device *ddev; See code example in drm_drv.c how to embed drm_device. This is the preferred approach these days. > + int ret; > + int irq; > + > + dev_dbg(dev, "%s\n", __func__); > + > + tidss = devm_kzalloc(dev, sizeof(*tidss), GFP_KERNEL); > + if (tidss == NULL) > + return -ENOMEM; > + > + tidss->dev = dev; > + tidss->feat = of_device_get_match_data(dev); > + > + platform_set_drvdata(pdev, tidss); > + > + ddev = drm_dev_alloc(&tidss_driver, dev); > + if (IS_ERR(ddev)) > + return PTR_ERR(ddev); > + > + tidss->ddev = ddev; > + ddev->dev_private = tidss; > + > + pm_runtime_enable(dev); > + > + ret = dispc_init(tidss); > + if (ret) { > + dev_err(dev, "failed to initialize dispc: %d\n", ret); > + goto err_disable_pm; > + } > + > +#ifndef CONFIG_PM > + /* If we don't have PM, we need to call resume manually */ > + dispc_runtime_resume(tidss->dispc); > +#endif > + > + ret = tidss_modeset_init(tidss); > + if (ret < 0) { > + if (ret != -EPROBE_DEFER) > + dev_err(dev, "failed to init DRM/KMS (%d)\n", ret); > + goto err_runtime_suspend; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) { > + ret = irq; > + goto err_modeset_cleanup; > + } > + > + ret = drm_irq_install(ddev, irq); > + if (ret) { > + dev_err(dev, "drm_irq_install failed: %d\n", ret); > + goto err_modeset_cleanup; > + } > + > + drm_kms_helper_poll_init(ddev); > + > + ret = drm_dev_register(ddev, 0); > + if (ret) { > + dev_err(dev, "failed to register DRM device\n"); > + goto err_poll_fini; > + } > + > + drm_fbdev_generic_setup(ddev, 32); > + > + dev_dbg(dev, "%s done\n", __func__); > + > + return 0; > + > +err_poll_fini: > + drm_kms_helper_poll_fini(ddev); > + > + drm_atomic_helper_shutdown(ddev); > + > + drm_irq_uninstall(ddev); > + > +err_modeset_cleanup: > + tidss_modeset_cleanup(tidss); > + > +err_runtime_suspend: > +#ifndef CONFIG_PM > + /* If we don't have PM, we need to call suspend manually */ > + dispc_runtime_suspend(tidss->dispc); > +#endif > + > + dispc_remove(tidss); > + > +err_disable_pm: > + pm_runtime_disable(dev); > + > + drm_dev_put(ddev); > + > + return ret; > +} > + > +static int tidss_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct tidss_device *tidss = platform_get_drvdata(pdev); > + struct drm_device *ddev = tidss->ddev; > + > + dev_dbg(dev, "%s\n", __func__); > + > + drm_dev_unregister(ddev); > + > + drm_kms_helper_poll_fini(ddev); > + > + drm_atomic_helper_shutdown(ddev); > + > + drm_irq_uninstall(ddev); > + > + tidss_modeset_cleanup(tidss); > + > +#ifndef CONFIG_PM > + /* If we don't have PM, we need to call suspend manually */ > + dispc_runtime_suspend(tidss->dispc); > +#endif > + > + dispc_remove(tidss); > + > + pm_runtime_disable(dev); > + > + drm_dev_put(ddev); > + > + dev_dbg(dev, "%s done\n", __func__); > + > + return 0; > +} > + > +static const struct of_device_id tidss_of_table[] = { > + { .compatible = "ti,k2g-dss", .data = &dispc_k2g_feats, }, > + { .compatible = "ti,am65x-dss", .data = &dispc_am65x_feats, }, > + { .compatible = "ti,j721e-dss", .data = &dispc_j721e_feats, }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, tidss_of_table); > + > +static struct platform_driver tidss_platform_driver = { > + .probe = tidss_probe, > + .remove = tidss_remove, > + .driver = { > + .name = "tidss", > +#ifdef CONFIG_PM > + .pm = &tidss_pm_ops, > +#endif > + .of_match_table = tidss_of_table, > + .suppress_bind_attrs = true, > + }, > +}; > + > +module_platform_driver(tidss_platform_driver); > + > +MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@xxxxxx>"); > +MODULE_DESCRIPTION("TI Keystone DSS Driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h > new file mode 100644 > index 000000000000..ef815c840b44 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_drv.h > @@ -0,0 +1,42 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#ifndef __TIDSS_DRV_H__ > +#define __TIDSS_DRV_H__ > + > +#include <linux/spinlock.h> > + > +#define TIDSS_MAX_PORTS 4 > +#define TIDSS_MAX_PLANES 4 > + > +typedef u32 dispc_irq_t; > + > +struct tidss_device { > + struct device *dev; /* Underlying DSS device */ > + struct drm_device *ddev; /* DRM device for DSS */ > + > + struct drm_fbdev_cma *fbdev; Unused, can be removed. > + > + const struct dispc_features *feat; > + struct dispc_device *dispc; > + > + unsigned int num_crtcs; > + struct drm_crtc *crtcs[TIDSS_MAX_PORTS]; > + > + unsigned int num_planes; > + struct drm_plane *planes[TIDSS_MAX_PLANES]; > + > + spinlock_t wait_lock; /* protects the irq masks */ > + dispc_irq_t irq_mask; /* enabled irqs in addition to wait_list */ > + dispc_irq_t irq_uf_mask; /* underflow irq bits for all planes */ > + > + struct drm_atomic_state *saved_state; > +}; > + > +int tidss_runtime_get(struct tidss_device *tidss); > +void tidss_runtime_put(struct tidss_device *tidss); > + > +#endif > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c > new file mode 100644 > index 000000000000..7390c056b257 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_encoder.c > @@ -0,0 +1,89 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#include <linux/export.h> > + > +#include <drm/drmP.h> File no longer present in the tree. > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_of.h> Sort include files. > + > +#include "tidss_drv.h" > +#include "tidss_encoder.h" > +#include "tidss_crtc.h" > + > +static int tidss_encoder_atomic_check(struct drm_encoder *encoder, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct drm_device *ddev = encoder->dev; > + struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state); > + struct drm_display_info *di = &conn_state->connector->display_info; > + struct drm_bridge *bridge; > + bool bus_flags_set = false; > + > + dev_dbg(ddev->dev, "%s\n", __func__); > + > + /* > + * Take the bus_flags from the first bridge that defines > + * bridge timings, or from the connector's display_info if no > + * bridge defines the timings. > + */ > + for (bridge = encoder->bridge; bridge; bridge = bridge->next) { > + if (!bridge->timings) > + continue; > + > + tcrtc_state->bus_flags = bridge->timings->input_bus_flags; > + bus_flags_set = true; > + break; > + } > + > + if (!di->bus_formats || di->num_bus_formats == 0) { > + dev_err(ddev->dev, "%s: No bus_formats in connected display\n", > + __func__); > + return -EINVAL; > + } > + > + // XXX any cleaner way to set bus format and flags? > + tcrtc_state->bus_format = di->bus_formats[0]; > + if (!bus_flags_set) > + tcrtc_state->bus_flags = di->bus_flags; > + > + return 0; > +} > + > +static const struct drm_encoder_helper_funcs encoder_helper_funcs = { > + .atomic_check = tidss_encoder_atomic_check, > +}; > + > +static const struct drm_encoder_funcs encoder_funcs = { > + .destroy = drm_encoder_cleanup, > +}; > + > +struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, > + u32 encoder_type, u32 possible_crtcs) > +{ > + struct drm_encoder *enc; > + int ret; > + > + enc = devm_kzalloc(tidss->dev, sizeof(*enc), GFP_KERNEL); > + if (!enc) > + return ERR_PTR(-ENOMEM); > + > + enc->possible_crtcs = possible_crtcs; > + > + ret = drm_encoder_init(tidss->ddev, enc, &encoder_funcs, > + encoder_type, NULL); > + if (ret < 0) > + return ERR_PTR(ret); > + > + drm_encoder_helper_add(enc, &encoder_helper_funcs); > + > + dev_dbg(tidss->dev, "Encoder create done\n"); > + > + return enc; > +} > diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h > new file mode 100644 > index 000000000000..06854d66e7e6 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_encoder.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#ifndef __TIDSS_ENCODER_H__ > +#define __TIDSS_ENCODER_H__ > + > +#include <drm/drm_encoder.h> > + > +struct tidss_device; > + > +struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss, > + u32 encoder_type, u32 possible_crtcs); > + > +#endif > diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c > new file mode 100644 > index 000000000000..001cac7c1355 > --- /dev/null > +++ b/drivers/gpu/drm/tidss/tidss_irq.c > @@ -0,0 +1,185 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ > + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxx> > + */ > + > +#include <drm/drmP.h> No longer present in the tree. > + > +#include "tidss_irq.h" > +#include "tidss_drv.h" > +#include "tidss_dispc.h" > +#include "tidss_crtc.h" > +#include "tidss_plane.h" _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
