Message-ID: <811ad406-4afb-45c5-9783-683779f874cc@kapsi.fi>
Date: Tue, 10 Jun 2025 12:26:07 +0900
From: Mikko Perttunen <cyndis@...si.fi>
To: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>,
 Thierry Reding <thierry.reding@...il.com>,
 Mikko Perttunen <mperttunen@...dia.com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Jonathan Hunter <jonathanh@...dia.com>,
 Philipp Zabel <p.zabel@...gutronix.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 linux-tegra@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/3] drm/tegra: Add NVJPG driver

On 6/6/25 7:45 PM, Diogo Ivo wrote:
> Add support for booting and using NVJPG on Tegra210 to the Host1x
> and TegraDRM drivers. This driver only supports the new TegraDRM uAPI.

Hello Diogo -- I'm happy to see this driver!

> 
> Signed-off-by: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
> ---
>   drivers/gpu/drm/tegra/Makefile |   1 +
>   drivers/gpu/drm/tegra/drm.c    |   2 +
>   drivers/gpu/drm/tegra/drm.h    |   1 +
>   drivers/gpu/drm/tegra/nvjpg.c  | 379 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 383 insertions(+)
> ...
> +
> +static __maybe_unused int nvjpg_runtime_resume(struct device *dev)
> +{
> +	struct nvjpg *nvjpg = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = clk_prepare_enable(nvjpg->clk);
> +	if (err < 0)
> +		return err;
> +
> +	usleep_range(20, 30);
> +
> +	if (nvjpg->rst) {
> +		err = reset_control_acquire(nvjpg->rst);
> +		if (err < 0) {
> +			dev_err(dev, "failed to acquire reset: %d\n", err);
> +			goto disable_clk;
> +		}
> +
> +		err = reset_control_deassert(nvjpg->rst);
> +		if (err < 0) {
> +			dev_err(dev, "failed to deassert reset: %d\n", err);
> +			goto release_reset;
> +		}
> +
> +		usleep_range(20, 30);
> +	}

Do we need this manual reset handling? NVJPG is only on T210+ where the 
power domain code handles the reset as well. Did you run into any issues 
with it?

(As a note, the reset_control_* functions are no-ops on a NULL reset. So 
the 'if' here is unnecessary.)

> +
> +	err = nvjpg_load_falcon_firmware(nvjpg);
> +	if (err < 0)
> +		goto assert_reset;
> +
> +	err = falcon_boot(&nvjpg->falcon);
> +	if (err < 0)
> +		goto assert_reset;
> +
> +	return 0;
> +
> +assert_reset:
> +	reset_control_assert(nvjpg->rst);
> +release_reset:
> +	reset_control_release(nvjpg->rst);
> +disable_clk:
> +	clk_disable_unprepare(nvjpg->clk);
> +	return err;
> +}
> ...
> +
> +static int nvjpg_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct nvjpg *nvjpg;
> +	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;
> +	}
> +
> +	nvjpg = devm_kzalloc(dev, sizeof(*nvjpg), GFP_KERNEL);
> +	if (!nvjpg)
> +		return -ENOMEM;
> +
> +	nvjpg->config = of_device_get_match_data(dev);
> +
> +	nvjpg->regs = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);

This can be devm_platform_ioremap_resource -- slightly simpler.

> +	if (IS_ERR(nvjpg->regs))
> +		return PTR_ERR(nvjpg->regs);
> +
> +	nvjpg->rst = devm_reset_control_get_exclusive_released(&pdev->dev, "nvjpg");
> +	if (IS_ERR(nvjpg->rst)) {
> +		err = PTR_ERR(nvjpg->rst);
> +
> +		if (err != -EBUSY || WARN_ON(!pdev->dev.pm_domain)) {
> +			dev_err(&pdev->dev, "failed to get reset control: %d\n",
> +				err);
> +			return err;
> +		}
> +
> +		/*
> +		 * At this point, the reset control is most likely being used
> +		 * by the generic power domain implementation. With any luck
> +		 * the power domain will have taken care of resetting the SOR
> +		 * and we don't have to do anything.
> +		 */
> +		nvjpg->rst = NULL;
> +	}

I see you've taken this from sor.c, but I think it should be 
unnecessary. I imagine the code in sor.c is overcomplicated as well, 
maybe because we used not to have the power domain implementation.

> +
> +	nvjpg->clk = devm_clk_get(dev, "nvjpg");
> +	if (IS_ERR(nvjpg->clk)) {
> +		dev_err(&pdev->dev, "failed to get clock\n");
> +		return PTR_ERR(nvjpg->clk);
> +	}

Probably a good idea to set the clock rate to max (see vic.c).

> +
> +	nvjpg->falcon.dev = dev;
> +	nvjpg->falcon.regs = nvjpg->regs;
> +
> +	err = falcon_init(&nvjpg->falcon);
> +	if (err < 0)
> +		return err;
> +
> +	platform_set_drvdata(pdev, nvjpg);
> +
> +	INIT_LIST_HEAD(&nvjpg->client.base.list);
> +	nvjpg->client.base.ops = &nvjpg_client_ops;
> +	nvjpg->client.base.dev = dev;
> +	nvjpg->client.base.class = HOST1X_CLASS_NVJPG;
> +	nvjpg->dev = dev;
> +
> +	INIT_LIST_HEAD(&nvjpg->client.list);
> +	nvjpg->client.version = nvjpg->config->version;
> +	nvjpg->client.ops = &nvjpg_ops;
> +
> +	err = host1x_client_register(&nvjpg->client.base);
> +	if (err < 0) {
> +		dev_err(dev, "failed to register host1x client: %d\n", err);
> +		goto exit_falcon;
> +	}
> +
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 500);
> +	devm_pm_runtime_enable(dev);
> +
> +	return 0;
> +
> +exit_falcon:
> +	falcon_exit(&nvjpg->falcon);
> +
> +	return err;
> +}
> +
> +static void nvjpg_remove(struct platform_device *pdev)
> +{
> +	struct nvjpg *nvjpg = platform_get_drvdata(pdev);
> +
> +	host1x_client_unregister(&nvjpg->client.base);
> +	falcon_exit(&nvjpg->falcon);
> +}
> +
> +static const struct dev_pm_ops nvjpg_pm_ops = {
> +	SET_RUNTIME_PM_OPS(nvjpg_runtime_suspend, nvjpg_runtime_resume, NULL)
> +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +				pm_runtime_force_resume)
> +};

There are modern, improved variants with no SET_ prefix.

Thanks,
Mikko

> +
> +struct platform_driver tegra_nvjpg_driver = {
> +	.driver = {
> +		.name = "tegra-nvjpg",
> +		.of_match_table = tegra_nvjpg_of_match,
> +		.pm = &nvjpg_pm_ops
> +	},
> +	.probe = nvjpg_probe,
> +	.remove = nvjpg_remove,
> +};
> +
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_210_SOC)
> +MODULE_FIRMWARE(NVIDIA_TEGRA_210_NVJPG_FIRMWARE);
> +#endif
> 

Powered by blists - more mailing lists