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

[Svyatoslav Ryhel]

чт, 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.

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.

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