Hi Mikko, thanks for the quick review!
On 6/10/25 4:26 AM, Mikko Perttunen wrote:
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 <[email protected]>
---
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?
Everything works just fine without the manual reset handling. I was
unsure if I should add it to the driver and in the end I decided to do
it because I thought that it would be better if the driver had explicit
control over it. However if delegating this to the power domain code is
enough I'll delete it for v2.
+
+ 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.
Thanks, I'll update that for v2.
+ 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.
Following my comment above I'll delete this block for v2.
+
+ 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).
Sounds good, I'll update it for v2.
+
+ 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.
I'll update it for v2 as well.
Thank you for your time!
Diogo
