Re: [PATCH v2 3/3] drm/tegra: Add NVDEC driver

On Fri, Aug 06, 2021 at 03:34:50PM +0300, Mikko Perttunen wrote:

Add support for booting and using NVDEC on Tegra210, Tegra186 and Tegra194 to the Host1x and TegraDRM drivers. Booting in secure mode is not currently supported.

Signed-off-by: Mikko Perttunen mperttunen@nvidia.com

v2:

• Use devm_platform_get_and_ioremap_resource
• Remove reset handling, done by power domain code
• Assume runtime PM is enabled

---

drivers/gpu/drm/tegra/Makefile | 3 +- drivers/gpu/drm/tegra/drm.c | 4 + drivers/gpu/drm/tegra/drm.h | 1 + drivers/gpu/drm/tegra/nvdec.c | 473 +++++++++++++++++++++++++++++++++ drivers/gpu/host1x/dev.c | 18 ++ include/linux/host1x.h | 2 + 6 files changed, 500 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/tegra/nvdec.c

diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile index 5d2039f0c734..b248c631f790 100644 --- a/drivers/gpu/drm/tegra/Makefile +++ b/drivers/gpu/drm/tegra/Makefile @@ -24,7 +24,8 @@ tegra-drm-y := \ gr2d.o \ gr3d.o \ falcon.o \

• vic.o

• vic.o \
• nvdec.o

tegra-drm-y += trace.o

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index b20fd0833661..5f5afd7ba37e 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -1337,15 +1337,18 @@ static const struct of_device_id host1x_drm_subdevs[] = { { .compatible = "nvidia,tegra210-sor", }, { .compatible = "nvidia,tegra210-sor1", }, { .compatible = "nvidia,tegra210-vic", },

• { .compatible = "nvidia,tegra210-nvdec", }, { .compatible = "nvidia,tegra186-display", }, { .compatible = "nvidia,tegra186-dc", }, { .compatible = "nvidia,tegra186-sor", }, { .compatible = "nvidia,tegra186-sor1", }, { .compatible = "nvidia,tegra186-vic", },
• { .compatible = "nvidia,tegra186-nvdec", }, { .compatible = "nvidia,tegra194-display", }, { .compatible = "nvidia,tegra194-dc", }, { .compatible = "nvidia,tegra194-sor", }, { .compatible = "nvidia,tegra194-vic", },
• { .compatible = "nvidia,tegra194-nvdec", }, { /* sentinel */ }

};

@@ -1369,6 +1372,7 @@ static struct platform_driver * const drivers[] = { &tegra_gr2d_driver, &tegra_gr3d_driver, &tegra_vic_driver,

• &tegra_nvdec_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 8b28327c931c..fc0a19554eac 100644 --- a/drivers/gpu/drm/tegra/drm.h +++ b/drivers/gpu/drm/tegra/drm.h @@ -202,5 +202,6 @@ 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; +extern struct platform_driver tegra_nvdec_driver;

#endif /* HOST1X_DRM_H */ diff --git a/drivers/gpu/drm/tegra/nvdec.c b/drivers/gpu/drm/tegra/nvdec.c new file mode 100644 index 000000000000..4a58b5357473 --- /dev/null +++ b/drivers/gpu/drm/tegra/nvdec.c @@ -0,0 +1,473 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*

• • Copyright (c) 2015-2021, NVIDIA Corporation.
• */

+#include <linux/clk.h> +#include <linux/delay.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 nvdec_config {

• const char *firmware;
• unsigned int version;
• bool supports_sid;
• int num_instances;

This can be unsigned int.

+};

+struct nvdec {

• struct falcon falcon;
• void __iomem *regs;
• struct tegra_drm_client client;

Traditionally this goes first to make the to_nvdec() cast helper a no-op. But I see that we also got this wrong for VIC, and that's probably where you copied this from. So nevermind, we can fix that in a later patch.

• struct host1x_channel *channel;
• struct device *dev;
• struct clk *clk;
• /* Platform configuration */
• const struct nvdec_config *config;

+};

+static inline struct nvdec *to_nvdec(struct tegra_drm_client *client) +{

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

+}

+static void nvdec_writel(struct nvdec *nvdec, u32 value, unsigned int offset) +{

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

+}

+static int nvdec_boot(struct nvdec *nvdec) +{ +#ifdef CONFIG_IOMMU_API

• struct iommu_fwspec *spec = dev_iommu_fwspec_get(nvdec->dev);

+#endif

• int err = 0;

Why does this need to be initialized?

+#ifdef CONFIG_IOMMU_API

• if (nvdec->config->supports_sid && spec) {

• u32 value;
  

• value = TRANSCFG_ATT(1, TRANSCFG_SID_FALCON) |
  

• 	TRANSCFG_ATT(0, TRANSCFG_SID_HW);
  

This fits on a single line. The limit of characters per line was recently bumped to 100.

• nvdec_writel(nvdec, value, VIC_TFBIF_TRANSCFG);
  

• if (spec->num_ids > 0) {
  

• 	value = spec->ids[0] & 0xffff;
  

• 	nvdec_writel(nvdec, value, VIC_THI_STREAMID0);
  

• 	nvdec_writel(nvdec, value, VIC_THI_STREAMID1);
  

• }
  

• }

+#endif

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

• return err;
  

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

• dev_err(nvdec->dev,
  

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

Same here.

• return err;
  

• }
• return 0;

+}

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

• struct tegra_drm_client *drm = host1x_to_drm_client(client);
• struct drm_device *dev = dev_get_drvdata(client->host);
• struct tegra_drm *tegra = dev->dev_private;
• struct nvdec *nvdec = to_nvdec(drm);
• int err;
• err = host1x_client_iommu_attach(client);
• if (err < 0 && err != -ENODEV) {

• dev_err(nvdec->dev, "failed to attach to domain: %d\n", err);
  

• return err;
  

• }
• nvdec->channel = host1x_channel_request(client);
• if (!nvdec->channel) {

• err = -ENOMEM;
  

• goto detach;
  

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

• err = -ENOMEM;
  

• goto free_channel;
  

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

• goto free_syncpt;
  

• /*

• * Inherit the DMA parameters (such as maximum segment size) from the
  

• * parent host1x device.
  

• */
  

• client->dev->dma_parms = client->host->dma_parms;
• return 0;

+free_syncpt:

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

+free_channel:

• host1x_channel_put(nvdec->channel);

+detach:

• host1x_client_iommu_detach(client);
• return err;

+}

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

• struct tegra_drm_client *drm = host1x_to_drm_client(client);
• struct drm_device *dev = dev_get_drvdata(client->host);
• struct tegra_drm *tegra = dev->dev_private;
• struct nvdec *nvdec = to_nvdec(drm);
• int err;
• /* avoid a dangling pointer just in case this disappears */
• client->dev->dma_parms = NULL;
• err = tegra_drm_unregister_client(tegra, drm);
• if (err < 0)

• return err;
  

• host1x_syncpt_put(client->syncpts[0]);
• host1x_channel_put(nvdec->channel);
• host1x_client_iommu_detach(client);
• if (client->group) {

• dma_unmap_single(nvdec->dev, nvdec->falcon.firmware.phys,
  

• 		 nvdec->falcon.firmware.size, DMA_TO_DEVICE);
  

• tegra_drm_free(tegra, nvdec->falcon.firmware.size,
  

• 	       nvdec->falcon.firmware.virt,
  

• 	       nvdec->falcon.firmware.iova);
  

• } else {

• dma_free_coherent(nvdec->dev, nvdec->falcon.firmware.size,
  

• 		  nvdec->falcon.firmware.virt,
  

• 		  nvdec->falcon.firmware.iova);
  

• }
• return 0;

+}

+static const struct host1x_client_ops nvdec_client_ops = {

• .init = nvdec_init,
• .exit = nvdec_exit,

+};

+static int nvdec_load_firmware(struct nvdec *nvdec) +{

• struct host1x_client *client = &nvdec->client.base;
• struct tegra_drm *tegra = nvdec->client.drm;
• dma_addr_t iova;
• size_t size;
• void *virt;
• int err;
• if (nvdec->falcon.firmware.virt)

• return 0;
  

• err = falcon_read_firmware(&nvdec->falcon, nvdec->config->firmware);
• if (err < 0)

• return err;
  

• size = nvdec->falcon.firmware.size;
• if (!client->group) {

• virt = dma_alloc_coherent(nvdec->dev, size, &iova, GFP_KERNEL);
  

• err = dma_mapping_error(nvdec->dev, iova);
  

• if (err < 0)
  

• 	return err;
  

• } else {

• virt = tegra_drm_alloc(tegra, size, &iova);
  

• }
• nvdec->falcon.firmware.virt = virt;
• nvdec->falcon.firmware.iova = iova;
• err = falcon_load_firmware(&nvdec->falcon);
• if (err < 0)

• goto cleanup;
  

• /*

• * In this case we have received an IOVA from the shared domain, so we
  

• * need to make sure to get the physical address so that the DMA API
  

• * knows what memory pages to flush the cache for.
  

• */
  

• if (client->group) {

• dma_addr_t phys;
  

• phys = dma_map_single(nvdec->dev, virt, size, DMA_TO_DEVICE);
  

• err = dma_mapping_error(nvdec->dev, phys);
  

• if (err < 0)
  

• 	goto cleanup;
  

• nvdec->falcon.firmware.phys = phys;
  

• }
• return 0;

+cleanup:

• if (!client->group)

• dma_free_coherent(nvdec->dev, size, virt, iova);
  

• else

• tegra_drm_free(tegra, size, virt, iova);
  

• return err;

+}

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

• struct nvdec *nvdec = dev_get_drvdata(dev);
• int err;
• err = clk_prepare_enable(nvdec->clk);
• if (err < 0)

• return err;
  

• usleep_range(10, 20);
• err = nvdec_load_firmware(nvdec);
• if (err < 0)

• goto disable;
  

• err = nvdec_boot(nvdec);
• if (err < 0)

• goto disable;
  

• return 0;

+disable:

• clk_disable_unprepare(nvdec->clk);
• return err;

+}

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

• struct nvdec *nvdec = dev_get_drvdata(dev);
• clk_disable_unprepare(nvdec->clk);
• return 0;

+}

+static int nvdec_open_channel(struct tegra_drm_client *client,

• 	    struct tegra_drm_context *context)
  

+{

• struct nvdec *nvdec = to_nvdec(client);
• int err;
• err = pm_runtime_get_sync(nvdec->dev);
• if (err < 0) {

• pm_runtime_put(nvdec->dev);
  

• return err;
  

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

• pm_runtime_put(nvdec->dev);
  

• return -ENOMEM;
  

• }
• return 0;

+}

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

• struct nvdec *nvdec = to_nvdec(context->client);
• host1x_channel_put(context->channel);
• pm_runtime_put(nvdec->dev);

+}

+static const struct tegra_drm_client_ops nvdec_ops = {

• .open_channel = nvdec_open_channel,
• .close_channel = nvdec_close_channel,
• .submit = tegra_drm_submit,

+};

+#define NVIDIA_TEGRA_210_NVDEC_FIRMWARE "nvidia/tegra210/nvdec.bin"

+static const struct nvdec_config nvdec_t210_config = {

• .firmware = NVIDIA_TEGRA_210_NVDEC_FIRMWARE,
• .version = 0x21,
• .supports_sid = false,

+};

+#define NVIDIA_TEGRA_186_NVDEC_FIRMWARE "nvidia/tegra186/nvdec.bin"

+static const struct nvdec_config nvdec_t186_config = {

• .firmware = NVIDIA_TEGRA_186_NVDEC_FIRMWARE,
• .version = 0x18,
• .supports_sid = true,

+};

Shouldn't the above both have .num_instances = 1?

+#define NVIDIA_TEGRA_194_NVDEC_FIRMWARE "nvidia/tegra194/nvdec.bin"

+static const struct nvdec_config nvdec_t194_config = {

• .firmware = NVIDIA_TEGRA_194_NVDEC_FIRMWARE,
• .version = 0x19,
• .supports_sid = true,

And shouldn't this have .num_instances = 2?

+};

+static const struct of_device_id tegra_nvdec_of_match[] = {

• { .compatible = "nvidia,tegra210-nvdec", .data = &nvdec_t210_config },
• { .compatible = "nvidia,tegra186-nvdec", .data = &nvdec_t186_config },
• { .compatible = "nvidia,tegra194-nvdec", .data = &nvdec_t194_config },
• { },

+}; +MODULE_DEVICE_TABLE(of, tegra_nvdec_of_match);

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

• struct device *dev = &pdev->dev;
• struct host1x_syncpt **syncpts;
• struct nvdec *nvdec;
• u32 instance;
• int err;
• /* inherit DMA mask from host1x parent */
• err = dma_coerce_mask_and_coherent(dev, *dev->parent->dma_mask);
• if (err < 0) {

• dev_err(&pdev->dev, "failed to set DMA mask: %d\n", err);
  

• return err;
  

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

• return -ENOMEM;
  

• nvdec->config = of_device_get_match_data(dev);
• syncpts = devm_kzalloc(dev, sizeof(*syncpts), GFP_KERNEL);
• if (!syncpts)

• return -ENOMEM;
  

• nvdec->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
• if (IS_ERR(nvdec->regs))

• return PTR_ERR(nvdec->regs);
  

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

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

• return PTR_ERR(nvdec->clk);
  

• }
• err = of_property_read_u32(dev->of_node, "nvidia,instance", &instance);
• if (err < 0)

• instance = 0;
  

• if (instance > nvdec->config->num_instances)

• return -EINVAL;
  

I assume nvidia,instance is zero-based? Shouldn't this then be:

if (instance >= nvdec->config->num_instances)

instead? With the current code, a second instance (nvidia,instance = <1>) would be accepted, even if the SoC only supported a single instance.

Thierry