On 05.12.2016 21:35, Thierry Reding wrote:
On Thu, Nov 10, 2016 at 08:23:41PM +0200, Mikko Perttunen wrote:
From: Arto Merilainen <amerilainen@xxxxxxxxxx>
This patch adds support for Video Image Compositor engine which
can be used for 2d operations.
Signed-off-by: Andrew Chew <achew@xxxxxxxxxx>
Signed-off-by: Arto Merilainen <amerilainen@xxxxxxxxxx>
Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
---
drivers/gpu/drm/tegra/Makefile | 3 +-
drivers/gpu/drm/tegra/drm.c | 3 +
drivers/gpu/drm/tegra/drm.h | 1 +
drivers/gpu/drm/tegra/vic.c | 400 +++++++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/tegra/vic.h | 31 ++++
include/linux/host1x.h | 1 +
6 files changed, 438 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 ecfe7ba..707404d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1151,11 +1151,13 @@ static const struct of_device_id host1x_drm_subdevs[] = {
{ .compatible = "nvidia,tegra124-sor", },
{ .compatible = "nvidia,tegra124-hdmi", },
{ .compatible = "nvidia,tegra124-dsi", },
+ { .compatible = "nvidia,tegra124-vic", },
{ .compatible = "nvidia,tegra132-dsi", },
{ .compatible = "nvidia,tegra210-dc", },
{ .compatible = "nvidia,tegra210-dsi", },
{ .compatible = "nvidia,tegra210-sor", },
{ .compatible = "nvidia,tegra210-sor1", },
+ { .compatible = "nvidia,tegra210-vic", },
{ /* sentinel */ }
};
@@ -1177,6 +1179,7 @@ static struct platform_driver * const drivers[] = {
&tegra_sor_driver,
&tegra_gr2d_driver,
&tegra_gr3d_driver,
+ &tegra_vic_driver,
};
static int __init host1x_drm_init(void)
diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index f75b505..8efaaa4 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -292,5 +292,6 @@ extern struct platform_driver tegra_dpaux_driver;
extern struct platform_driver tegra_sor_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..9eda5e5
--- /dev/null
+++ b/drivers/gpu/drm/tegra/vic.c
@@ -0,0 +1,400 @@
+/*
+ * 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;
Maybe just "firmware"? That way you could remove the comment.
Fixed.
+};
+
+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;
+
+ /* Platform configuration */
+ const struct vic_config *config;
+};
+
+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);
+
+ return clk_prepare_enable(vic->clk);
+}
+
+static int vic_runtime_suspend(struct device *dev)
+{
+ struct vic *vic = dev_get_drvdata(dev);
+
+ clk_disable_unprepare(vic->clk);
+
+ 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;
+ }
+
+ /* 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;
Why do we need this? We can get at struct vic * from struct falcon * via
an upcast, and once we have struct vic, we have struct tegra_drm_client.
But now I realize that apparently there's no way to get from that to the
struct drm_device. I guess we could go via vic->client.base.parent and a
call to dev_get_drvdata(), but that's rather far. It's still a little
less confusing than stashing the struct tegra_drm * in it, since it is
not at all driver-specific data.
I don't know, falcon.data is just supposed to be "priv pointer" for
falcon ops callbacks so it could be anything, in principle. I can change
this but I somewhat prefer the current version.
+ vic->falcon.ops = &vic_falcon_ops;
+ err = falcon_init(&vic->falcon);
Could use a blank line between the above.
Fixed.
+ 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);
+
+ 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.
+ */
That sounds like a hack. Typically you're supposed to have the firmware
on the same medium as the module that requests it. That is, if the
driver is built-in, then you need to make the firmware available as
built-in as well or stash it into an initrd. If the driver is built as a
loadable module and is on the root filesystem, that's where the firmware
should be as well. There's also the possibility to put the loadable
module into an initrd, in which case that's where the firmware should go
as well.
That said, I think it makes sense to defer the actual booting until this
point. Loading the firmware seems unappropriate to me. It really should
be done in ->probe().
Fixed. The way it's now done is that the firmware is read in probe and
loaded in vic_init, as we cannot call tegra_drm functions from probe.
+ 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 const struct tegra_drm_client_ops vic_ops = {
+ .open_channel = vic_open_channel,
+ .close_channel = vic_close_channel,
+ .submit = tegra_drm_submit,
+};
+
+static const struct vic_config vic_t124_config = {
+ .ucode_name = "nvidia/tegra124/vic03_ucode.bin",
+};
+
+static const struct vic_config vic_t210_config = {
+ .ucode_name = "nvidia/tegra210/vic04_ucode.bin",
+};
+
+static const struct of_device_id vic_match[] = {
+ { .compatible = "nvidia,tegra124-vic", .data = &vic_t124_config },
+ { .compatible = "nvidia,tegra210-vic", .data = &vic_t210_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);
+ }
+
+ 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;
+ }
That's a slightly ugly construct, but I can't think of an easy way to
get rid of it (other than to depend on PM, which actually might be the
right thing to do here, it's already selected by ARCH_TEGRA on 64-bit
ARM). I can't think of a scenario where we'd want to run without runtime
PM.
Kept this for now.
+
+ dev_info(&pdev->dev, "initialized");
Again, no need for this.
Removed.
Thierry
Thanks,
Mikko.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
