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

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

[Mikko Perttunen]

On Monday, May 4, 2026 3:36 AM Svyatoslav Ryhel wrote:
> нд, 3 трав. 2026 р. о 19:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> >
> > From: Ion Agorria <ion@agorria.com>
> >
> > 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@nvidia.com>
> > Signed-off-by: Ion Agorria <ion@agorria.com>
> > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > ---
> >  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.

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

Re: [PATCH v2 1/2] drm/tegra: gr2d/gr3d: Initialize address register map before HOST1X client is registered

[Mikko Perttunen]

On Monday, May 4, 2026 1:38 AM Svyatoslav Ryhel wrote:
> The host1x_client_register() function is called just prior to register map
> initialization loop, making the device available to userspace. This may
> result in userspace attempting to submits a job before the register map is
> initialized. Address this by moving register initialization before host1x
> client registration.
> 
> 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..e4148b034af7 100644
> --- a/drivers/gpu/drm/tegra/gr2d.c
> +++ b/drivers/gpu/drm/tegra/gr2d.c
> @@ -276,16 +276,16 @@ static int gr2d_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	/* initialize address register map */
> +	for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
> +		set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
> +
>  	err = host1x_client_register(&gr2d->client.base);
>  	if (err < 0) {
>  		dev_err(dev, "failed to register host1x client: %d\n", err);
>  		return err;
>  	}
>  
> -	/* initialize address register map */
> -	for (i = 0; i < ARRAY_SIZE(gr2d_addr_regs); i++)
> -		set_bit(gr2d_addr_regs[i], gr2d->addr_regs);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/tegra/gr3d.c b/drivers/gpu/drm/tegra/gr3d.c
> index 42e9656ab80c..47b0c6c56bfd 100644
> --- a/drivers/gpu/drm/tegra/gr3d.c
> +++ b/drivers/gpu/drm/tegra/gr3d.c
> @@ -506,6 +506,10 @@ static int gr3d_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	/* initialize address register map */
> +	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
> +		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
> +
>  	err = host1x_client_register(&gr3d->client.base);
>  	if (err < 0) {
>  		dev_err(&pdev->dev, "failed to register host1x client: %d\n",
> @@ -513,10 +517,6 @@ static int gr3d_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	/* initialize address register map */
> -	for (i = 0; i < ARRAY_SIZE(gr3d_addr_regs); i++)
> -		set_bit(gr3d_addr_regs[i], gr3d->addr_regs);
> -
>  	return 0;
>  }
>  
> -- 
> 2.51.0
> 
> 

Acked-by: Mikko Perttunen <mperttunen@nvidia.com>

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@nvidia.com> пише:
>
> On Monday, May 4, 2026 3:36 AM Svyatoslav Ryhel wrote:
> > нд, 3 трав. 2026 р. о 19:38 Svyatoslav Ryhel <clamor95@gmail.com> пише:
> > >
> > > From: Ion Agorria <ion@agorria.com>
> > >
> > > 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@nvidia.com>
> > > Signed-off-by: Ion Agorria <ion@agorria.com>
> > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > ---
> > >  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
> > >
>
>
>
>