Re: [PATCH v2 2/4] drm/tegra: Add VIC support

Hi Mikko,

On 20/07/15 08:54, Mikko Perttunen wrote:

From: Arto Merilainen amerilainen@nvidia.com

This patch adds support for Video Image Compositor engine which can be used for 2d operations.

Signed-off-by: Andrew Chew achew@nvidia.com Signed-off-by: Arto Merilainen amerilainen@nvidia.com Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

---

drivers/gpu/drm/tegra/Makefile | 3 +- drivers/gpu/drm/tegra/drm.c | 7 + drivers/gpu/drm/tegra/drm.h | 1 + drivers/gpu/drm/tegra/vic.c | 456 +++++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/vic.h | 35 ++++ include/linux/host1x.h | 1 + 6 files changed, 502 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/tegra/vic.c create mode 100644 drivers/gpu/drm/tegra/vic.h

diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile index 93e9a4a..6af3a9a 100644 --- a/drivers/gpu/drm/tegra/Makefile +++ b/drivers/gpu/drm/tegra/Makefile @@ -14,6 +14,7 @@ tegra-drm-y := \ dpaux.o \ gr2d.o \ gr3d.o \

• falcon.o

• falcon.o \
• vic.o

obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index af4ff86..bc8cc2a 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1145,6 +1145,7 @@ static const struct of_device_id host1x_drm_subdevs[] = { { .compatible = "nvidia,tegra124-dc", }, { .compatible = "nvidia,tegra124-sor", }, { .compatible = "nvidia,tegra124-hdmi", },

• { .compatible = "nvidia,tegra124-vic", }, { /* sentinel */ }

};

@@ -1194,8 +1195,14 @@ static int __init host1x_drm_init(void) if (err < 0) goto unregister_gr2d;

• err = platform_driver_register(&tegra_vic_driver);
• if (err < 0)

• goto unregister_gr3d;
  

• return 0;

+unregister_gr3d:

• platform_driver_unregister(&tegra_gr3d_driver);

unregister_gr2d: platform_driver_unregister(&tegra_gr2d_driver); unregister_dpaux: diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h index 58c83b11..2fc7e42 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -284,5 +284,6 @@ extern struct platform_driver tegra_hdmi_driver; extern struct platform_driver tegra_dpaux_driver; extern struct platform_driver tegra_gr2d_driver; extern struct platform_driver tegra_gr3d_driver; +extern struct platform_driver tegra_vic_driver;

#endif /* HOST1X_DRM_H */ diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c new file mode 100644 index 0000000..fce7c04 --- /dev/null +++ b/drivers/gpu/drm/tegra/vic.c @@ -0,0 +1,456 @@ +/*

• • Copyright (c) 2015, NVIDIA Corporation.
• • This program is free software; you can redistribute it and/or modify
• • it under the terms of the GNU General Public License version 2 as
• • published by the Free Software Foundation.
• */

+#include <linux/clk.h> +#include <linux/host1x.h> +#include <linux/iommu.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/reset.h>

+#include <soc/tegra/pmc.h>

+#include "drm.h" +#include "falcon.h" +#include "vic.h"

+struct vic_config {

• /* Firmware name */
• const char *ucode_name;

+};

+struct vic {

• struct falcon falcon;
• bool booted;
• void __iomem *regs;
• struct tegra_drm_client client;
• struct host1x_channel *channel;
• struct iommu_domain *domain;
• struct device *dev;
• struct clk *clk;
• struct reset_control *rst;
• /* Platform configuration */
• const struct vic_config *config;
• /* for firewall - this determines if method 1 should be regarded

• * as an address register */
  

• bool method_data_is_addr_reg;

+};

+static inline struct vic *to_vic(struct tegra_drm_client *client) +{

• return container_of(client, struct vic, client);

+}

+static void vic_writel(struct vic *vic, u32 value, unsigned int offset) +{

• writel(value, vic->regs + offset);

+}

+static int vic_runtime_resume(struct device *dev) +{

• struct vic *vic = dev_get_drvdata(dev);
• int err;
• err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VIC,

• 				vic->clk, vic->rst);
  

I would like to deprecate use of the above function [1]. I have been trying to migrate driver to use a new API instead of the above.

• if (err < 0)

• dev_err(dev, "failed to power up device\n");
  

• return err;

+}

+static int vic_runtime_suspend(struct device *dev) +{

• struct vic *vic = dev_get_drvdata(dev);
• clk_disable_unprepare(vic->clk);
• reset_control_assert(vic->rst);
• tegra_powergate_power_off(TEGRA_POWERGATE_VIC);

Similarly here. I would like to get rid of the above API in favour of a different API to help migrate to generic power domain support.

• vic->booted = false;
• return 0;

+}

+static int vic_boot(struct vic *vic) +{

• u32 fce_ucode_size, fce_bin_data_offset;
• void *hdr;
• int err = 0;
• if (vic->booted)

• return 0;
  

• if (!vic->falcon.firmware.valid) {

• err = falcon_read_firmware(&vic->falcon,
  

• 			   vic->config->ucode_name);
  

• if (err < 0)
  

• 	return err;
  

• }
• /* ensure that the engine is in sane state */
• reset_control_assert(vic->rst);
• usleep_range(10, 100);
• reset_control_deassert(vic->rst);
• /* setup clockgating registers */
• vic_writel(vic, CG_IDLE_CG_DLY_CNT(4) |

• 	CG_IDLE_CG_EN |
  

• 	CG_WAKEUP_DLY_CNT(4),
  

•    NV_PVIC_MISC_PRI_VIC_CG);
  

• err = falcon_boot(&vic->falcon);
• if (err < 0)

• return err;
  

• hdr = vic->falcon.firmware.vaddr;
• fce_bin_data_offset = *(u32 *)(hdr + VIC_UCODE_FCE_DATA_OFFSET);
• hdr = vic->falcon.firmware.vaddr +

• *(u32 *)(hdr + VIC_UCODE_FCE_HEADER_OFFSET);
  

• fce_ucode_size = *(u32 *)(hdr + FCE_UCODE_SIZE_OFFSET);
• falcon_execute_method(&vic->falcon, VIC_SET_APPLICATION_ID, 1);
• falcon_execute_method(&vic->falcon, VIC_SET_FCE_UCODE_SIZE,

• 	      fce_ucode_size);
  

• falcon_execute_method(&vic->falcon, VIC_SET_FCE_UCODE_OFFSET,

• 	      (vic->falcon.firmware.paddr + fce_bin_data_offset)
  

• 		>> 8);
  

• err = falcon_wait_idle(&vic->falcon);
• if (err < 0) {

• dev_err(vic->dev,
  

• 	"failed to set application ID and FCE base\n");
  

• return err;
  

• }
• vic->booted = true;
• return 0;

+}

+static void *vic_falcon_alloc(struct falcon *falcon, size_t size,

• 	       dma_addr_t *iova)
  

+{

• struct tegra_drm *tegra = falcon->data;
• return tegra_drm_alloc(tegra, size, iova);

+}

+static void vic_falcon_free(struct falcon *falcon, size_t size,

• 	    dma_addr_t iova, void *va)
  

+{

• struct tegra_drm *tegra = falcon->data;
• return tegra_drm_free(tegra, size, va, iova);

+}

+static const struct falcon_ops vic_falcon_ops = {

• .alloc = vic_falcon_alloc,
• .free = vic_falcon_free

+};

+static int vic_init(struct host1x_client *client) +{

• struct tegra_drm_client *drm = host1x_to_drm_client(client);
• struct drm_device *dev = dev_get_drvdata(client->parent);
• struct tegra_drm *tegra = dev->dev_private;
• struct vic *vic = to_vic(drm);
• int err;
• if (tegra->domain) {

• err = iommu_attach_device(tegra->domain, vic->dev);
  

• if (err < 0) {
  

• 	dev_err(vic->dev, "failed to attach to domain: %d\n",
  

• 		err);
  

• 	return err;
  

• }
  

• vic->domain = tegra->domain;
  

• }
• vic->falcon.dev = vic->dev;
• vic->falcon.regs = vic->regs;
• vic->falcon.data = tegra;
• vic->falcon.ops = &vic_falcon_ops;
• err = falcon_init(&vic->falcon);
• if (err < 0)

• goto detach_device;
  

• vic->channel = host1x_channel_request(client->dev);
• if (!vic->channel) {

• err = -ENOMEM;
  

• goto exit_falcon;
  

• }
• client->syncpts[0] = host1x_syncpt_request(client->dev, 0);
• if (!client->syncpts[0]) {

• err = -ENOMEM;
  

• goto free_channel;
  

• }
• err = tegra_drm_register_client(tegra, drm);
• if (err < 0)

• goto free_syncpt;
  

• return 0;

+free_syncpt:

• host1x_syncpt_free(client->syncpts[0]);

+free_channel:

• host1x_channel_free(vic->channel);

+exit_falcon:

• falcon_exit(&vic->falcon);

+detach_device:

• if (tegra->domain)

• iommu_detach_device(tegra->domain, vic->dev);
  

• return err;

+}

+static int vic_exit(struct host1x_client *client) +{

• struct tegra_drm_client *drm = host1x_to_drm_client(client);
• struct drm_device *dev = dev_get_drvdata(client->parent);
• struct tegra_drm *tegra = dev->dev_private;
• struct vic *vic = to_vic(drm);
• int err;
• err = tegra_drm_unregister_client(tegra, drm);
• if (err < 0)

• return err;
  

• host1x_syncpt_free(client->syncpts[0]);
• host1x_channel_free(vic->channel);
• if (vic->booted) {

• reset_control_assert(vic->rst);
  

• usleep_range(10, 100);
  

• reset_control_deassert(vic->rst);
  

• }
• falcon_exit(&vic->falcon);
• if (vic->domain) {

• iommu_detach_device(vic->domain, vic->dev);
  

• vic->domain = NULL;
  

• }
• return 0;

+}

+static const struct host1x_client_ops vic_client_ops = {

• .init = vic_init,
• .exit = vic_exit,

+};

+static int vic_open_channel(struct tegra_drm_client *client,

• 	    struct tegra_drm_context *context)
  

+{

• struct vic *vic = to_vic(client);
• int err;
• err = pm_runtime_get_sync(vic->dev);
• if (err < 0)

• return err;
  

• /*

• * Try to boot the Falcon microcontroller. Booting is deferred until
  

• * here because the firmware might not yet be available during system
  

• * boot, for example if it's on remote storage.
  

• */
  

• err = vic_boot(vic);
• if (err < 0) {

• pm_runtime_put(vic->dev);
  

• return err;
  

• }
• context->channel = host1x_channel_get(vic->channel);
• if (!context->channel) {

• pm_runtime_put(vic->dev);
  

• return -ENOMEM;
  

• }
• return 0;

+}

+static void vic_close_channel(struct tegra_drm_context *context) +{

• struct vic *vic = to_vic(context->client);
• host1x_channel_put(context->channel);
• pm_runtime_put(vic->dev);

+}

+static int vic_is_addr_reg(struct device *dev, u32 class, u32 offset, u32 val) +{

• struct vic *vic = dev_get_drvdata(dev);
• if (class != HOST1X_CLASS_VIC)

• return false;
  

• /*

• * Method call parameter. Check stored value to see if set method uses
  

• * parameter as memory address.
  

• */
  

• if (offset == FALCON_UCLASS_METHOD_DATA >> 2)

• return vic->method_data_is_addr_reg;
  

• /* Method call number store. */
• if (offset == FALCON_UCLASS_METHOD_OFFSET >> 2) {

• u32 method = val << 2;
  

• if ((method >= VIC_SET_SURFACE0_SLOT0_LUMA_OFFSET &&
  

•      method <= VIC_SET_SURFACE7_SLOT4_CHROMAV_OFFSET) ||
  

•     (method >= VIC_SET_CONFIG_STRUCT_OFFSET &&
  

•      method <= VIC_SET_OUTPUT_SURFACE_CHROMAV_OFFSET))
  

• 	vic->method_data_is_addr_reg = true;
  

• else
  

• 	vic->method_data_is_addr_reg = false;
  

• }
• return false;

+}

+static const struct tegra_drm_client_ops vic_ops = {

• .open_channel = vic_open_channel,
• .close_channel = vic_close_channel,
• .is_addr_reg = vic_is_addr_reg,
• .submit = tegra_drm_submit,

+};

+static const struct vic_config vic_t124_config = {

• .ucode_name = "vic03_ucode.bin",

+};

+static const struct of_device_id vic_match[] = {

• { .compatible = "nvidia,tegra124-vic", .data = &vic_t124_config },
• { },

+};

+static int vic_probe(struct platform_device *pdev) +{

• struct vic_config *vic_config = NULL;
• struct device *dev = &pdev->dev;
• struct host1x_syncpt **syncpts;
• struct resource *regs;
• const struct of_device_id *match;
• struct vic *vic;
• int err;
• match = of_match_device(vic_match, dev);
• vic_config = (struct vic_config *)match->data;
• vic = devm_kzalloc(dev, sizeof(*vic), GFP_KERNEL);
• if (!vic)

• return -ENOMEM;
  

• syncpts = devm_kzalloc(dev, sizeof(*syncpts), GFP_KERNEL);
• if (!syncpts)

• return -ENOMEM;
  

• regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
• if (!regs) {

• dev_err(&pdev->dev, "failed to get registers\n");
  

• return -ENXIO;
  

• }
• vic->regs = devm_ioremap_resource(dev, regs);
• if (IS_ERR(vic->regs))

• return PTR_ERR(vic->regs);
  

• vic->clk = devm_clk_get(dev, NULL);
• if (IS_ERR(vic->clk)) {

• dev_err(&pdev->dev, "failed to get clock\n");
  

• return PTR_ERR(vic->clk);
  

• }
• vic->rst = devm_reset_control_get(dev, "vic");
• if (IS_ERR(vic->rst)) {

• dev_err(&pdev->dev, "cannot get reset\n");
  

• return PTR_ERR(vic->rst);
  

• }
• platform_set_drvdata(pdev, vic);
• INIT_LIST_HEAD(&vic->client.base.list);
• vic->client.base.ops = &vic_client_ops;
• vic->client.base.dev = dev;
• vic->client.base.class = HOST1X_CLASS_VIC;
• vic->client.base.syncpts = syncpts;
• vic->client.base.num_syncpts = 1;
• vic->dev = dev;
• vic->config = vic_config;
• INIT_LIST_HEAD(&vic->client.list);
• vic->client.ops = &vic_ops;
• err = host1x_client_register(&vic->client.base);
• if (err < 0) {

• dev_err(dev, "failed to register host1x client: %d\n", err);
  

• platform_set_drvdata(pdev, NULL);
  

• return err;
  

• }
• pm_runtime_enable(&pdev->dev);
• if (!pm_runtime_enabled(&pdev->dev)) {

• err = vic_runtime_resume(&pdev->dev);
  

• if (err < 0)
  

• 	goto unregister_client;
  

• }

I don't see why pm_runtime_enable() should ever fail to enable pm_runtime here. Hence, the if-statement appears to be redundant. If you are trying to determine whether rpm is supported for the power-domains then you should simply check to see if the DT node for the device has the "power-domains" property. See my series [1].

Cheers Jon

[1] http://www.spinics.net/lists/arm-kernel/msg431051.html