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

[Svyatoslav Ryhel]

чт, 30 квіт. 2026 р. о 11:38 Mikko Perttunen <mperttunen@xxxxxxxxxx> пише:
>
> On Thursday, April 30, 2026 5:04 PM Svyatoslav Ryhel wrote:
> > чт, 30 квіт. 2026 р. о 09:23 Mikko Perttunen <mperttunen@xxxxxxxxxx> пише:
> > >
> > > On Tuesday, April 28, 2026 1:57 PM Svyatoslav Ryhel wrote:
> > > > вт, 28 квіт. 2026 р. о 04:53 Mikko Perttunen <mperttunen@xxxxxxxxxx> пише:
> > > > >
> > > > > On Monday, April 27, 2026 4:58 PM Svyatoslav Ryhel wrote:
> > > > > > From: Ion Agorria <ion@xxxxxxxxxxx>
> > > > > >
> > > > > > 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@xxxxxxxxxxx>
> > > > > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > > > > ---
> > > > > >  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
>

Thank you for an explanation, I will adjust v2 accordingly (configure
PM in probe/remove).

> >
> > 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@xxxxxxxxxx>
> > > > >
> > > > >
> > > > >
> > >
> > >
> > >
> > >
>
>
>
>