Re: [PATCH v1 1/1] drm/tegra: gr2d/gr3d: Move pm_runtime_enable to gr*d_probe

[Mikko Perttunen <mperttunen@nvidia.com>]

On Thursday, April 30, 2026 5:04 PM Svyatoslav Ryhel wrote:
> чт, 30 квіт. 2026 р. о 09:23 Mikko Perttunen <mperttunen@nvidia.com> пише:
> >
> > On Tuesday, April 28, 2026 1:57 PM Svyatoslav Ryhel wrote:
> > > вт, 28 квіт. 2026 р. о 04:53 Mikko Perttunen <mperttunen@nvidia.com> пише:
> > > >
> > > > On Monday, April 27, 2026 4:58 PM Svyatoslav Ryhel wrote:
> > > > > From: Ion Agorria <ion@agorria.com>
> > > > >
> > > > > The gr*d_remove() has pm_runtime_disable, this indicates it should be
> > > > > paired with pm_runtime_enable in the probe instead of being inside
> > > > > gr*d_runtime_resume().
> > > > >
> > > > > Signed-off-by: Ion Agorria <ion@agorria.com>
> > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > > ---
> > > > >  drivers/gpu/drm/tegra/gr2d.c | 8 ++++----
> > > > >  drivers/gpu/drm/tegra/gr3d.c | 8 ++++----
> > > > >  2 files changed, 8 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> > > > > index 21f4dd0fa6af..71f092d59d65 100644
> > > > > --- a/drivers/gpu/drm/tegra/gr2d.c
> > > > > +++ b/drivers/gpu/drm/tegra/gr2d.c
> > > > > @@ -286,6 +286,10 @@ static int gr2d_probe(struct platform_device *pdev)
> > > > >       for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
> > > > >               set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
> > > > >
> > > > > +     pm_runtime_enable(dev);
> > > > > +     pm_runtime_use_autosuspend(dev);
> > > > > +     pm_runtime_set_autosuspend_delay(dev, 500);
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -367,10 +371,6 @@ static int __maybe_unused gr2d_runtime_resume(struct device *dev)
> > > > >               goto disable_clk;
> > > > >       }
> > > > >
> > > > > -     pm_runtime_enable(dev);
> > > > > -     pm_runtime_use_autosuspend(dev);
> > > > > -     pm_runtime_set_autosuspend_delay(dev, 500);
> > > > > -
> > > > >       return 0;
> > > > >
> > > > >  disable_clk:
> > > > > diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> > > > > index 42e9656ab80c..33e88ca4d4c5 100644
> > > > > --- a/drivers/gpu/drm/tegra/gr3d.c
> > > > > +++ b/drivers/gpu/drm/tegra/gr3d.c
> > > > > @@ -517,6 +517,10 @@ static int gr3d_probe(struct platform_device *pdev)
> > > > >       for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
> > > > >               set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
> > > > >
> > > > > +     pm_runtime_enable(&pdev->dev);
> > > > > +     pm_runtime_use_autosuspend(&pdev->dev);
> > > > > +     pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
> > > > > +
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -578,10 +582,6 @@ static int __maybe_unused gr3d_runtime_resume(struct device *dev)
> > > > >               goto disable_clk;
> > > > >       }
> > > > >
> > > > > -     pm_runtime_enable(dev);
> > > > > -     pm_runtime_use_autosuspend(dev);
> > > > > -     pm_runtime_set_autosuspend_delay(dev, 500);
> > > > > -
> > > > >       return 0;
> > > > >
> > > > >  disable_clk:
> > > > > --
> > > > > 2.51.0
> > > > >
> > > > >
> > > >
> > > > Oof, looks like I had managed to really bungle this with my earlier
> > > > patch. Thanks for fixing it!
> > > >
> > >
> > > Hello Mikko!
> > >
> > > Thank you for taking time and looking into this patch. Don't be so
> > > harsh to yourself, PM is easy to mess and hard to set properly. This
> > > patch does fix gr*d access but it does not resolve the issue itself.
> > > PM should be set in the init/exit rather then probe/remove. I have v2
> > > which fixes this and one more minor issue and I will send them later
> > > on.
> >
> > Thanks! Why do you think it's necessary to enable runtime PM in init?
> > If you look at the commit I referenced below (in 'Fixes'), we've had
> > some issues in the past with doing pm_runtime_enable outside of probe,
> > where the engine's power domain would be left enabled even when it is
> > idle.
> >
> > gr2d/gr3d I suppose wouldn't be in practice affected by that issue
> > though given they aren't in their own power domains.
> 
> WDYM, gr2d and gr3d have their own power domains.

Ah, my bad, didn't look far enough in the device tree.

> 
> If the master device is unbound and rebound, gr2d_init() will run again, but
> pm configuration is only located in this probe function which will not
> run again, while pm disable are both in exit and remove. This results
> in pm issue we are observing.
> 
> Solution would be either do everyting in probe/remove, or init/exit.
> Probe/remove will lead to domain being turned on even if engines are
> idle. Init/exit seems to me more suitable and we have tasted this
> configuration for a quite while in the grate DRM version.

Yes, we should only do it in one place. Where are you observing the
domain being turned on when idle?

Genpd turns on the domain before probe is called, and it should turn
it off again after probe if probe had enabled runtime PM and the usage
count is zero. That's what we have currently for the newer engines like
VIC and it's working.

We used to have it in init/exit, but then we were seeing the domain
sometimes being left on at boot, which makes some sense since as probe
didn't enable runtime PM, genpd would leave the domain powered after
probe. The domain would then also never get powered off when init
enabled runtime PM. I assume the reason this only happened sometimes
was that it was dependent on if the init function or the genpd
post-probe code ran first.

Based on that we should have the runtime PM handling in probe/remove,
to ensure genpd turns off the domain after probe when there are no
users.

Mikko

> 
> Best regards,
> Svyatoslav R.
> 
> >
> > Cheers
> > Mikko
> >
> > >
> > > So for now this patch should not be picked.
> > >
> > > Best regards,
> > > Svyatoslav R.
> > >
> > > > FWIW, I've been working on adding some nightly testing for Host1x/
> > > > TegraDRM, so hopefully we'll be able to catch such things easier
> > > > in the future.
> > > >
> > > > Fixes: 62fa0a985e2c ("drm/tegra: Enable runtime PM during probe")
> > > > Acked-by: Mikko Perttunen <mperttunen@nvidia.com>
> > > >
> > > >
> > > >
> >
> >
> >
> >