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

[Mikko Perttunen]

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.

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