Re: [PATCH v2 2/2] drm/tegra: gr2d/gr3d: Contain PM in the gr*d_probe/gr*d_remove

[Svyatoslav Ryhel]

чт, 14 трав. 2026 р. о 07:07 Mikko Perttunen <mperttunen@xxxxxxxxxx> пише:
>
> On Monday, May 4, 2026 3:36 AM Svyatoslav Ryhel wrote:
> > нд, 3 трав. 2026 р. о 19:38 Svyatoslav Ryhel <clamor95@xxxxxxxxx> пише:
> > >
> > > From: Ion Agorria <ion@xxxxxxxxxxx>
> > >
> > > The current power management configuration causes GR2G/GR3D to malfunction
> > > after resume. Reconfigure all PM actions to be handled within the GR*D
> > > probe and remove operations to address this.
> > >
> > > Fixes: 62fa0a985e2c ("drm/tegra: Enable runtime PM during probe")
> > > Acked-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > > Signed-off-by: Ion Agorria <ion@xxxxxxxxxxx>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/tegra/gr2d.c | 11 ++++-------
> > >  drivers/gpu/drm/tegra/gr3d.c | 11 ++++-------
> > >  2 files changed, 8 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/tegra/gr2d.c b/drivers/gpu/drm/tegra/gr2d.c
> > > index e4148b034af7..ffcd076b5831 100644
> > > --- a/drivers/gpu/drm/tegra/gr2d.c
> > > +++ b/drivers/gpu/drm/tegra/gr2d.c
> > > @@ -100,9 +100,6 @@ static int gr2d_exit(struct host1x_client *client)
> > >         if (err < 0)
> > >                 return err;
> > >
> > > -       pm_runtime_dont_use_autosuspend(client->dev);
> > > -       pm_runtime_force_suspend(client->dev);
> > > -
> > >         host1x_client_iommu_detach(client);
> > >         host1x_syncpt_put(client->syncpts[0]);
> > >         host1x_channel_put(gr2d->channel);
> > > @@ -286,6 +283,10 @@ static int gr2d_probe(struct platform_device *pdev)
> > >                 return err;
> > >         }
> > >
> > > +       pm_runtime_enable(dev);
> > > +       pm_runtime_use_autosuspend(dev);
> > > +       pm_runtime_set_autosuspend_delay(dev, 500);
> > > +
> >
> > Hello Mikko!
>
> Hello!
>
> >
> > I have used same setup as in VIC. May you please take a look to sashiko's check
> > https://sashiko.dev/#/patchset/20260502124055.22475-1-clamor95%40gmail.com
> >
> > I do agree with statement that pm_runtime_enable should be before
> > host1x_client_register since this same approach is widely used in the
> > media subsystem too.
>
> Yep, I agree as well.
>
> >
> > But I am more interested in your thoughts regarding sashiko's
> > gr2d_exit situation reasoning.
>
> It sounds accurate to me, there is probably a real issue. I expect
> there to be in general some driver unbind related issues, since it
> (particularly when the device is in use) isn't really being tested.
>
> FWIW, in an ideal world, it would probably make more sense to do the
> IOMMU handling on the platform device side, since the device's memory
> accesses are not related to host1x. But we can't really do that right
> now.
>
> For the time being, I don't think fixing this is particularly critical
> since other things will go wrong as well if root causes .exit to happen
> while the engine is executing. I can't say offhand what the proper full
> solution would be.
>

Thank you for sharing your thoughts on this topic. I will adjust
accordingly in v3

> Thanks
> Mikko
>
> >
> > Thank you!
> >
> > >         return 0;
> > >  }
> > >
> > > @@ -367,10 +368,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 47b0c6c56bfd..cd5554e2117f 100644
> > > --- a/drivers/gpu/drm/tegra/gr3d.c
> > > +++ b/drivers/gpu/drm/tegra/gr3d.c
> > > @@ -109,9 +109,6 @@ static int gr3d_exit(struct host1x_client *client)
> > >         if (err < 0)
> > >                 return err;
> > >
> > > -       pm_runtime_dont_use_autosuspend(client->dev);
> > > -       pm_runtime_force_suspend(client->dev);
> > > -
> > >         host1x_client_iommu_detach(client);
> > >         host1x_syncpt_put(client->syncpts[0]);
> > >         host1x_channel_put(gr3d->channel);
> > > @@ -517,6 +514,10 @@ static int gr3d_probe(struct platform_device *pdev)
> > >                 return err;
> > >         }
> > >
> > > +       pm_runtime_enable(&pdev->dev);
> > > +       pm_runtime_use_autosuspend(&pdev->dev);
> > > +       pm_runtime_set_autosuspend_delay(&pdev->dev, 500);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -578,10 +579,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
> > >
>
>
>
>