diff mbox series

[2/3] drm/i915: Disallow D3Cold.

Message ID 20210910174447.289750-2-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/i915/runtime_pm: Consolidate runtime_pm functions | expand

Commit Message

Rodrigo Vivi Sept. 10, 2021, 5:44 p.m. UTC 
During runtime or s2idle suspend and resume cases on discrete cards,
if D3Cold is really achieved, we will blow everything up and
freeze the machine because we are not yet handling the pci states
properly.

On Integrated it simply doesn't matter because D3hot is the maximum
that we will get anyway, unless the system is on S3/S4 and our power
is cut.

Let's put this hammer for now everywhere. So we can work to enable
the auto-suspend by default without blowing up the world.

Then, this should be removed when we finally fix the D3Cold flow.

Cc: Tilak Tangudu <tilak.tangudu@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Acked-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Gupta, Anshuman Sept. 13, 2021, 10:52 a.m. UTC | #1 
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Rodrigo
> Vivi
> Sent: Friday, September 10, 2021 11:15 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Tangudu, Tilak
> <tilak.tangudu@intel.com>; Deak, Imre <imre.deak@intel.com>
> Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Disallow D3Cold.
> 
> During runtime or s2idle suspend and resume cases on discrete cards, if D3Cold
> is really achieved, we will blow everything up and freeze the machine because
> we are not yet handling the pci states properly.
> 
> On Integrated it simply doesn't matter because D3hot is the maximum that we
> will get anyway, unless the system is on S3/S4 and our power is cut.
> 
> Let's put this hammer for now everywhere. So we can work to enable the auto-
> suspend by default without blowing up the world.
> 
> Then, this should be removed when we finally fix the D3Cold flow.
> 
> Cc: Tilak Tangudu <tilak.tangudu@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Acked-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a40b5d806321..086a9a475ce8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -301,6 +301,7 @@ static void sanitize_gpu(struct drm_i915_private *i915)
>   */
>  static int i915_driver_early_probe(struct drm_i915_private *dev_priv)  {
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
>  	int ret = 0;
> 
>  	if (i915_inject_probe_failure(dev_priv))
> @@ -331,6 +332,13 @@ static int i915_driver_early_probe(struct
> drm_i915_private *dev_priv)
>  	if (ret < 0)
>  		return ret;
> 
> +	/*
> +	 * FIXME: Temporary hammer to avoid freezing the machine on our
> DGFX
> +	 * This should be totally removed when we handle the pci states
> properly
> +	 * on runtime PM and on s2idle cases.
> +	 */
> +	pci_d3cold_disable(pdev);
	This still doesn't protect, if user space enables d3cold via sys-fs.
	d3cold_allowed_store() may call pci_d3cold_enable()
	Is it possible to disable it via PCI PM Caps at early probe?
	Otherwise it should done in respective suspend callback to make sure
	d3cold is disabled.

Thanks,
Anshuman Gupta.
> +
>  	ret = vlv_suspend_init(dev_priv);
>  	if (ret < 0)
>  		goto err_workqueues;
> --
> 2.31.1
Rodrigo Vivi Sept. 13, 2021, 6:48 p.m. UTC | #2 
On Mon, 2021-09-13 at 16:22 +0530, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > Of Rodrigo
> > Vivi
> > Sent: Friday, September 10, 2021 11:15 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Tangudu, Tilak
> > <tilak.tangudu@intel.com>; Deak, Imre <imre.deak@intel.com>
> > Subject: [Intel-gfx] [PATCH 2/3] drm/i915: Disallow D3Cold.
> > 
> > During runtime or s2idle suspend and resume cases on discrete
> > cards, if D3Cold
> > is really achieved, we will blow everything up and freeze the
> > machine because
> > we are not yet handling the pci states properly.
> > 
> > On Integrated it simply doesn't matter because D3hot is the maximum
> > that we
> > will get anyway, unless the system is on S3/S4 and our power is
> > cut.
> > 
> > Let's put this hammer for now everywhere. So we can work to enable
> > the auto-
> > suspend by default without blowing up the world.
> > 
> > Then, this should be removed when we finally fix the D3Cold flow.
> > 
> > Cc: Tilak Tangudu <tilak.tangudu@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Acked-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index a40b5d806321..086a9a475ce8 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -301,6 +301,7 @@ static void sanitize_gpu(struct
> > drm_i915_private *i915)
> >   */
> >  static int i915_driver_early_probe(struct drm_i915_private
> > *dev_priv)  {
> > +       struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> >         int ret = 0;
> > 
> >         if (i915_inject_probe_failure(dev_priv))
> > @@ -331,6 +332,13 @@ static int i915_driver_early_probe(struct
> > drm_i915_private *dev_priv)
> >         if (ret < 0)
> >                 return ret;
> > 
> > +       /*
> > +        * FIXME: Temporary hammer to avoid freezing the machine on
> > our
> > DGFX
> > +        * This should be totally removed when we handle the pci
> > states
> > properly
> > +        * on runtime PM and on s2idle cases.
> > +        */
> > +       pci_d3cold_disable(pdev);
>         This still doesn't protect, if user space enables d3cold via
> sys-fs.
>         d3cold_allowed_store() may call pci_d3cold_enable()
>         Is it possible to disable it via PCI PM Caps at early probe?
>         Otherwise it should done in respective suspend callback to
> make sure
>         d3cold is disabled.

Well, I'm not too concerned about the case that user goes and tweak
something that he shouldn't. In especial a temporary hack like this,
and with us actively working to remove that.

However you have a point and Tilak also is trying to convince me that
his version which disables it on the relevant suspend paths and re-
enables it on the resume is safer for s3 and s4.

I'm also not fully convinced there becasue on s3/s4 we lose power
ayways, however I'm not going to be stubborn and listen to both of you
;)

Please let's go with Tilak's version.

I just believe that we need a way with a temporary kernel parameter to
be able to allow d3cold on rpm and s2idle so while we are working to
fix d3cold paths properly.

Thanks,
Rodrigo.

> 
> Thanks,
> Anshuman Gupta.
> > +
> >         ret = vlv_suspend_init(dev_priv);
> >         if (ret < 0)
> >                 goto err_workqueues;
> > --
> > 2.31.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a40b5d806321..086a9a475ce8 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -301,6 +301,7 @@  static void sanitize_gpu(struct drm_i915_private *i915)
  */
 static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 {
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
 	int ret = 0;
 
 	if (i915_inject_probe_failure(dev_priv))
@@ -331,6 +332,13 @@  static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * FIXME: Temporary hammer to avoid freezing the machine on our DGFX
+	 * This should be totally removed when we handle the pci states properly
+	 * on runtime PM and on s2idle cases.
+	 */
+	pci_d3cold_disable(pdev);
+
 	ret = vlv_suspend_init(dev_priv);
 	if (ret < 0)
 		goto err_workqueues;