diff mbox series

[5/6] drm/i915/pm: Do pci_restore_state() in switcheroo resume hook

Message ID 20240925144526.2482-6-ville.syrjala@linux.intel.com (mailing list archive)
State Changes Requested
Headers show
Series drm/i915/pm: Clean up the hibernate vs. PCI D3 quirk | expand

Commit Message

Ville Syrjälä Sept. 25, 2024, 2:45 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since this switcheroo stuff bypasses all the core pm we
have to manually manage the pci state. To that end add the
missing pci_restore_state() to the switcheroo resume hook.
We already have the pci_save_state() counterpart on the
suspend side.

I suppose this might not matter in practice as the
integrated GPU probably won't lose any state in D3,
and I presume there are no machines where this code
would come into play with an Intel discrete GPU.

Arguably none of this code should exist in the driver
in the first place, and instead the entire switcheroo
mechanism should be rewritten and properly integrated into
core pm code...

Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: linux-pci@vger.kernel.org
Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Rodrigo Vivi Sept. 26, 2024, 2:48 p.m. UTC | #1 
On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since this switcheroo stuff bypasses all the core pm we
> have to manually manage the pci state. To that end add the
> missing pci_restore_state() to the switcheroo resume hook.
> We already have the pci_save_state() counterpart on the
> suspend side.
> 
> I suppose this might not matter in practice as the
> integrated GPU probably won't lose any state in D3,
> and I presume there are no machines where this code
> would come into play with an Intel discrete GPU.
> 
> Arguably none of this code should exist in the driver
> in the first place, and instead the entire switcheroo
> mechanism should be rewritten and properly integrated into
> core pm code...
> 
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index fe7c34045794..c3e7225ea1ba 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
>  	if (ret)
>  		return ret;
>  
> +	pci_restore_state(pdev);

then why not simply call that inside the resume, for a better alignment
with the save counterpart?

> +
>  	ret = i915_drm_resume_early(&i915->drm);
>  	if (ret)
>  		return ret;
> -- 
> 2.44.2
>
Ville Syrjälä Sept. 26, 2024, 3:36 p.m. UTC | #2 
On Thu, Sep 26, 2024 at 10:48:41AM -0400, Rodrigo Vivi wrote:
> On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Since this switcheroo stuff bypasses all the core pm we
> > have to manually manage the pci state. To that end add the
> > missing pci_restore_state() to the switcheroo resume hook.
> > We already have the pci_save_state() counterpart on the
> > suspend side.
> > 
> > I suppose this might not matter in practice as the
> > integrated GPU probably won't lose any state in D3,
> > and I presume there are no machines where this code
> > would come into play with an Intel discrete GPU.
> > 
> > Arguably none of this code should exist in the driver
> > in the first place, and instead the entire switcheroo
> > mechanism should be rewritten and properly integrated into
> > core pm code...
> > 
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: linux-pci@vger.kernel.org
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_driver.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > index fe7c34045794..c3e7225ea1ba 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
> >  	if (ret)
> >  		return ret;
> >  
> > +	pci_restore_state(pdev);
> 
> then why not simply call that inside the resume, for a better alignment
> with the save counterpart?

This is switcheroo resume. And the counterpart is in switcheroo suspend.

For the core pm hooks I'm getting rid of both save and restore.

> 
> > +
> >  	ret = i915_drm_resume_early(&i915->drm);
> >  	if (ret)
> >  		return ret;
> > -- 
> > 2.44.2
> >
Rodrigo Vivi Sept. 26, 2024, 4:27 p.m. UTC | #3 
On Thu, Sep 26, 2024 at 06:36:13PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 26, 2024 at 10:48:41AM -0400, Rodrigo Vivi wrote:
> > On Wed, Sep 25, 2024 at 05:45:25PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Since this switcheroo stuff bypasses all the core pm we
> > > have to manually manage the pci state. To that end add the
> > > missing pci_restore_state() to the switcheroo resume hook.
> > > We already have the pci_save_state() counterpart on the
> > > suspend side.
> > > 
> > > I suppose this might not matter in practice as the
> > > integrated GPU probably won't lose any state in D3,
> > > and I presume there are no machines where this code
> > > would come into play with an Intel discrete GPU.
> > > 
> > > Arguably none of this code should exist in the driver
> > > in the first place, and instead the entire switcheroo
> > > mechanism should be rewritten and properly integrated into
> > > core pm code...
> > > 
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: linux-pci@vger.kernel.org
> > > Cc: intel-gfx@lists.freedesktop.org
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_driver.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> > > index fe7c34045794..c3e7225ea1ba 100644
> > > --- a/drivers/gpu/drm/i915/i915_driver.c
> > > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > > @@ -1311,6 +1311,8 @@ int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	pci_restore_state(pdev);
> > 
> > then why not simply call that inside the resume, for a better alignment
> > with the save counterpart?
> 
> This is switcheroo resume. And the counterpart is in switcheroo suspend.
> 
> For the core pm hooks I'm getting rid of both save and restore.

With this I totally agree. I probably missed something when just
reading the patches... I had to apply them all to see the final version.

So,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> > > +
> > >  	ret = i915_drm_resume_early(&i915->drm);
> > >  	if (ret)
> > >  		return ret;
> > > -- 
> > > 2.44.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index fe7c34045794..c3e7225ea1ba 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1311,6 +1311,8 @@  int i915_driver_resume_switcheroo(struct drm_i915_private *i915)
 	if (ret)
 		return ret;
 
+	pci_restore_state(pdev);
+
 	ret = i915_drm_resume_early(&i915->drm);
 	if (ret)
 		return ret;