[PATCH 4/5] drm/tidss: New driver for TI Keystone platform Display SubSystem

Wed Nov 27 15:01:08 UTC 2019

Hi Sam,
And thank you for the prompt review comments!

On 26/11/2019 23:26, Sam Ravnborg wrote:
> 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.
> 

I am not so sure if the DSS go-bit functionality (hw support to commit
changes to display during next vertical blank) so widely used concept
that it would make sense to make drm wide helpers to handle it.

I applied already the simple to fix comments. There is couple of
comments and questions bellow about the more complex ones.

> 	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
...
>> --- /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>
>> + */
...>> +
>> +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?
> 

I feel the function should probably be a bit more generic and support
other formats too, but for the moment DRM does not even recognize
argb12121212 format used here. So there is some plumbing to do before
that would make sense. I happy to help with that (K3 DSS has support for
some other 64-bit formats too), but first I need to get this driver in
shape for merging.

>> 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
...>> +
>> +/* 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.
> 

Thanks, I will do that. I just got the driver working on top of
drm-next-2019-11-27 with the old probe approach, but it looks like the
things will have to changed quite a bit for the new approach. It will
probably take some time before I get module load-unload cycles to go
smoothly again.

One generic question about that. What will happen if the probe fails
after successful devm_drm_dev_init()?

The release callback gets called, and it should survive releasing the
resources from the unfinished driver object?

>> +	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
...

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki