[PATCH 4/5] drm/tidss: New driver for TI Keystone platform Display SubSystem
Sam Ravnborg
sam at ravnborg.org
Tue Nov 26 21:26:34 UTC 2019
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 at ti.com>
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> +++ 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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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 at ti.com>");
> +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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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 at ti.com>
> + */
> +
> +#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"
More information about the dri-devel
mailing list