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:

quoted

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!

quoted

Signed-off-by: Diogo Ivo <redacted>
---
  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.

quoted

+
+    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.

quoted

+    if (IS_ERR(nvjpg->regs))
+        return PTR_ERR(nvjpg->regs);
+
+    nvjpg->rst = devm_reset_control_get_exclusive_released(&pdev- 

quoted

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.

quoted

+
+    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.

quoted

+
+    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