Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Imre Deak]

On to, 2016-10-13 at 15:21 +0300, Jani Nikula wrote:
> On Thu, 13 Oct 2016, Imre Deak  wrote:
> > Currently the display INIT power domain disabling/enabling happens in a
> > mismatched way in the suspend/resume_early hooks respectively. This can
> > leave display power wells incorrectly disabled in the resume hook if the
> > suspend sequence is aborted for some reason resulting in the
> > suspend/resume hooks getting called but the suspend_late/resume_early
> > hooks being skipped. In particular this change fixes "Unclaimed read
> > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
> > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
> > to devices.
> > 
> > Cc: Ville Syrjälä 
> > Cc: David Weinehall 
> > Signed-off-by: Imre Deak 
> 
> Fixes: ?

Fixes: 85e90679335f ("drm/i915: disable power wells on suspend")

> Cc: sta...@vger.kernel.org ?

It matters only for an error path which is rare (another device needs
to fail suspending between its suspend and suspend_late steps) and
pm_test. So I'd say this isn't for fixes or stable.

--Imre

> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index e9b3bfc..5adabac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device
> > *dev)
> >  
> >     dev_priv->suspend_count++;
> >  
> > -   intel_display_set_init_power(dev_priv, false);
> > -
> >     intel_csr_ucode_suspend(dev_priv);
> >  
> >  out:
> > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct
> > drm_device *dev, bool hibernation)
> >  
> >     disable_rpm_wakeref_asserts(dev_priv);
> >  
> > +   intel_display_set_init_power(dev_priv, false);
> > +
> >     fw_csr = !IS_BROXTON(dev_priv) &&
> >     suspend_to_idle(dev_priv) && dev_priv-
> > >csr.dmc_payload;
> >     /*
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Imre Deak]

On to, 2016-10-13 at 15:43 +0300, Ville Syrjälä wrote:
> On Thu, Oct 13, 2016 at 02:34:06PM +0300, Imre Deak wrote:
> > Currently the display INIT power domain disabling/enabling happens in a
> > mismatched way in the suspend/resume_early hooks respectively. This can
> > leave display power wells incorrectly disabled in the resume hook if the
> > suspend sequence is aborted for some reason resulting in the
> > suspend/resume hooks getting called but the suspend_late/resume_early
> > hooks being skipped. In particular this change fixes "Unclaimed read
> > from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
> > intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
> > to devices.
> > 
> > Cc: Ville Syrjälä 
> > Cc: David Weinehall 
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c 
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index e9b3bfc..5adabac 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  
> >     dev_priv->suspend_count++;
> >  
> > -   intel_display_set_init_power(dev_priv, false);
> > -
> >     intel_csr_ucode_suspend(dev_priv);
> >  
> >  out:
> > @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device 
> > *dev, bool hibernation)
> >  
> >     disable_rpm_wakeref_asserts(dev_priv);
> >  
> > +   intel_display_set_init_power(dev_priv, false);
> > +
> 
> Makes sense in most cases.
> Reviewed-by: Ville Syrjälä 
> 
> >     fw_csr = !IS_BROXTON(dev_priv) &&
> >     suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
> 
> However this part is quite confusing. We do the init_power_put
> unconditionally, but the init_power_get is done from the init_hw()
> which is conditional on this supend_to_idle special casing. So it seems
> quite assymmetrical, and I can no longer claim to understand it.

Yep, that's a problem of its own for the suspend-to-idle case, enabling the
INIT domain should be also unconditional and happen from this function.
I suppose this happened to work so far since it only applies to SKL where
nothing depended on the INIT domain, everything takes its own domain ref.
Will look into fixing this.

--Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Ville Syrjälä]

On Thu, Oct 13, 2016 at 02:34:06PM +0300, Imre Deak wrote:
> Currently the display INIT power domain disabling/enabling happens in a
> mismatched way in the suspend/resume_early hooks respectively. This can
> leave display power wells incorrectly disabled in the resume hook if the
> suspend sequence is aborted for some reason resulting in the
> suspend/resume hooks getting called but the suspend_late/resume_early
> hooks being skipped. In particular this change fixes "Unclaimed read
> from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
> intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
> to devices.
> 
> Cc: Ville Syrjälä 
> Cc: David Weinehall 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9b3bfc..5adabac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>   dev_priv->suspend_count++;
>  
> - intel_display_set_init_power(dev_priv, false);
> -
>   intel_csr_ucode_suspend(dev_priv);
>  
>  out:
> @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device 
> *dev, bool hibernation)
>  
>   disable_rpm_wakeref_asserts(dev_priv);
>  
> + intel_display_set_init_power(dev_priv, false);
> +

Makes sense in most cases.
Reviewed-by: Ville Syrjälä 

>   fw_csr = !IS_BROXTON(dev_priv) &&
>   suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;

However this part is quite confusing. We do the init_power_put
unconditionally, but the init_power_get is done from the init_hw()
which is conditional on this supend_to_idle special casing. So it seems
quite assymmetrical, and I can no longer claim to understand it.

>   /*
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Jani Nikula]

On Thu, 13 Oct 2016, Imre Deak  wrote:
> Currently the display INIT power domain disabling/enabling happens in a
> mismatched way in the suspend/resume_early hooks respectively. This can
> leave display power wells incorrectly disabled in the resume hook if the
> suspend sequence is aborted for some reason resulting in the
> suspend/resume hooks getting called but the suspend_late/resume_early
> hooks being skipped. In particular this change fixes "Unclaimed read
> from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
> intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
> to devices.
>
> Cc: Ville Syrjälä 
> Cc: David Weinehall 
> Signed-off-by: Imre Deak 

Fixes: ?
Cc: sta...@vger.kernel.org ?

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e9b3bfc..5adabac 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>   dev_priv->suspend_count++;
>  
> - intel_display_set_init_power(dev_priv, false);
> -
>   intel_csr_ucode_suspend(dev_priv);
>  
>  out:
> @@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device 
> *dev, bool hibernation)
>  
>   disable_rpm_wakeref_asserts(dev_priv);
>  
> + intel_display_set_init_power(dev_priv, false);
> +
>   fw_csr = !IS_BROXTON(dev_priv) &&
>   suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
>   /*

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Fix mismatched INIT power domain disabling during suspend

[Imre Deak]

Currently the display INIT power domain disabling/enabling happens in a
mismatched way in the suspend/resume_early hooks respectively. This can
leave display power wells incorrectly disabled in the resume hook if the
suspend sequence is aborted for some reason resulting in the
suspend/resume hooks getting called but the suspend_late/resume_early
hooks being skipped. In particular this change fixes "Unclaimed read
from register 0x1e1204" on BYT/BSW triggered from i915_drm_resume()->
intel_pps_unlock_regs_wa() when suspending with /sys/power/pm_test set
to devices.

Cc: Ville Syrjälä 
Cc: David Weinehall 
Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e9b3bfc..5adabac 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1430,8 +1430,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 
dev_priv->suspend_count++;
 
-   intel_display_set_init_power(dev_priv, false);
-
intel_csr_ucode_suspend(dev_priv);
 
 out:
@@ -1449,6 +1447,8 @@ static int i915_drm_suspend_late(struct drm_device *dev, 
bool hibernation)
 
disable_rpm_wakeref_asserts(dev_priv);
 
+   intel_display_set_init_power(dev_priv, false);
+
fw_csr = !IS_BROXTON(dev_priv) &&
suspend_to_idle(dev_priv) && dev_priv->csr.dmc_payload;
/*
-- 
2.5.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx