Message-ID: <da94459b-1e06-4e07-936c-51aac41c0d26@tecnico.ulisboa.pt>
Date: Wed, 11 Jun 2025 12:47:20 +0100
From: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
To: Mikko Perttunen <cyndis@...si.fi>,
 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


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

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

Powered by blists - more mailing lists