All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend
@ 2016-01-06  1:44 Matt Roper
  2016-01-06  1:47 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2) Matt Roper
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Matt Roper @ 2016-01-06  1:44 UTC (permalink / raw)
  To: intel-gfx; +Cc: drm-intel-fixes

Our attempts save/restore panel power state in i915_suspend.c are
causing unclaimed register warnings on BXT since the registers for this
platform differ from older platforms.

The big hammer suspend/resume shouldn't actually be necessary for PP
since the connector/encoder hooks should already handle this, so let's
just add BXT to the list of platforms that we don't try to save/restore
these on.

Cc: Vandana Kannan <vandana.kannan@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a2aa09c..f7787a5 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
 		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
 		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
@@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
 		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && !IS_BROXTON(dev)) {
 		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
 		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2)
  2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
@ 2016-01-06  1:47 ` Matt Roper
  2016-01-06  8:20   ` Daniel Vetter
  2016-01-06  2:06 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend kbuild test robot
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2016-01-06  1:47 UTC (permalink / raw)
  To: intel-gfx; +Cc: drm-intel-fixes

Our attempts save/restore panel power state in i915_suspend.c are
causing unclaimed register warnings on BXT since the registers for this
platform differ from older platforms.

The big hammer suspend/resume shouldn't actually be necessary for PP
since the connector/encoder hooks should already handle this, so let's
just add BXT to the list of platforms that we don't try to save/restore
these on.

v2: Typo fix: s/||/&&/

Cc: Vandana Kannan <vandana.kannan@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: drm-intel-fixes@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a2aa09c..41625d6 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
 		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && !IS_BROXTON(dev)) {
 		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
@@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
 		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && !IS_BROXTON(dev)) {
 		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
 		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend
  2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
  2016-01-06  1:47 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2) Matt Roper
@ 2016-01-06  2:06 ` kbuild test robot
  2016-01-06  6:04 ` kbuild test robot
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2016-01-06  2:06 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, drm-intel-fixes, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2422 bytes --]

Hi Matt,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160105]
[cannot apply to v4.4-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/drm-i915-bxt-Don-t-save-restore-eDP-panel-power-during-suspend/20160106-094646
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x017-01041832 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/i915_suspend.c: In function 'i915_save_display':
>> drivers/gpu/drm/i915/i915_suspend.c:52:33: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
                                    ^

vim +52 drivers/gpu/drm/i915/i915_suspend.c

    36		/* Display arbitration control */
    37		if (INTEL_INFO(dev)->gen <= 4)
    38			dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
    39	
    40		/* LVDS state */
    41		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
    42			dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
    43		else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
    44			dev_priv->regfile.saveLVDS = I915_READ(LVDS);
    45	
    46		/* Panel power sequencer */
    47		if (HAS_PCH_SPLIT(dev)) {
    48			dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
    49			dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
    50			dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
    51			dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
  > 52		} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
    53			dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
    54			dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
    55			dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
    56			dev_priv->regfile.savePP_DIVISOR = I915_READ(PP_DIVISOR);
    57		}
    58	
    59		/* save FBC interval */
    60		if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev))

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22771 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend
  2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
  2016-01-06  1:47 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2) Matt Roper
  2016-01-06  2:06 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend kbuild test robot
@ 2016-01-06  6:04 ` kbuild test robot
  2016-01-06 12:20 ` ✓ success: Fi.CI.BAT Patchwork
  2016-01-07  7:20 ` Patchwork
  4 siblings, 0 replies; 13+ messages in thread
From: kbuild test robot @ 2016-01-06  6:04 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, drm-intel-fixes, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]

Hi Matt,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on next-20160105]
[cannot apply to v4.4-rc8]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Roper/drm-i915-bxt-Don-t-save-restore-eDP-panel-power-during-suspend/20160106-094646
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-s5-01061315 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/stddef.h:1:0,
                    from include/linux/stddef.h:4,
                    from include/uapi/linux/posix_types.h:4,
                    from include/uapi/linux/types.h:13,
                    from include/linux/types.h:5,
                    from include/linux/list.h:4,
                    from include/linux/agp_backend.h:33,
                    from include/drm/drmP.h:35,
                    from drivers/gpu/drm/i915/i915_suspend.c:27:
   drivers/gpu/drm/i915/i915_suspend.c: In function 'i915_save_display':
   drivers/gpu/drm/i915/i915_suspend.c:52:33: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
                                    ^
   include/linux/compiler.h:147:28: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                               ^
>> drivers/gpu/drm/i915/i915_suspend.c:52:9: note: in expansion of macro 'if'
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
            ^
   drivers/gpu/drm/i915/i915_suspend.c:52:33: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
                                    ^
   include/linux/compiler.h:147:40: note: in definition of macro '__trace_if'
     if (__builtin_constant_p((cond)) ? !!(cond) :   \
                                           ^
>> drivers/gpu/drm/i915/i915_suspend.c:52:9: note: in expansion of macro 'if'
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
            ^
   drivers/gpu/drm/i915/i915_suspend.c:52:33: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
                                    ^
   include/linux/compiler.h:158:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> drivers/gpu/drm/i915/i915_suspend.c:52:9: note: in expansion of macro 'if'
     } else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
            ^

vim +/if +52 drivers/gpu/drm/i915/i915_suspend.c

    21	 * IN NO EVENT SHALL TUNGSTEN GRAPHICS AND/OR ITS SUPPLIERS BE LIABLE FOR
    22	 * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
    23	 * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
    24	 * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
    25	 */
    26	
  > 27	#include <drm/drmP.h>
    28	#include <drm/i915_drm.h>
    29	#include "intel_drv.h"
    30	#include "i915_reg.h"
    31	
    32	static void i915_save_display(struct drm_device *dev)
    33	{
    34		struct drm_i915_private *dev_priv = dev->dev_private;
    35	
    36		/* Display arbitration control */
    37		if (INTEL_INFO(dev)->gen <= 4)
    38			dev_priv->regfile.saveDSPARB = I915_READ(DSPARB);
    39	
    40		/* LVDS state */
    41		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
    42			dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
    43		else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
    44			dev_priv->regfile.saveLVDS = I915_READ(LVDS);
    45	
    46		/* Panel power sequencer */
    47		if (HAS_PCH_SPLIT(dev)) {
    48			dev_priv->regfile.savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
    49			dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
    50			dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
    51			dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
  > 52		} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) || !IS_BROXTON(dev)) {
    53			dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
    54			dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
    55			dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 20155 bytes --]

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2)
  2016-01-06  1:47 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2) Matt Roper
@ 2016-01-06  8:20   ` Daniel Vetter
  2016-01-06 17:53     ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3) Matt Roper
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2016-01-06  8:20 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, drm-intel-fixes

On Tue, Jan 05, 2016 at 05:47:55PM -0800, Matt Roper wrote:
> Our attempts save/restore panel power state in i915_suspend.c are
> causing unclaimed register warnings on BXT since the registers for this
> platform differ from older platforms.
> 
> The big hammer suspend/resume shouldn't actually be necessary for PP
> since the connector/encoder hooks should already handle this, so let's
> just add BXT to the list of platforms that we don't try to save/restore
> these on.
> 
> v2: Typo fix: s/||/&&/
> 
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Could we just nuke this instead? From orbit, just to be sure? At least I
hope Jani's work on panel/backlight would make this all unecessary.

> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index a2aa09c..41625d6 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
>  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
>  		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
> -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> +	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && !IS_BROXTON(dev)) {

Or at least do a gen < 5 check here to future proof this forever.
-Daniel

>  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
>  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
>  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
> @@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
>  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>  		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
> -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> +	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev) && !IS_BROXTON(dev)) {
>  		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>  		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>  		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* ✓ success: Fi.CI.BAT
  2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
                   ` (2 preceding siblings ...)
  2016-01-06  6:04 ` kbuild test robot
@ 2016-01-06 12:20 ` Patchwork
  2016-01-07  7:20 ` Patchwork
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2016-01-06 12:20 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

== Summary ==

Built on 89d0d1b6f0e9c3a6b90476bd115cfe1881646fd6 drm-intel-nightly: 2016y-01m-06d-10h-37m-17s UTC integration manifest

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (byt-nuc) UNSTABLE

bdw-nuci7        total:132  pass:0    dwarn:0   dfail:0   fail:0   skip:132
bsw-nuc-2        total:135  pass:115  dwarn:0   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:121  dwarn:1   dfail:0   fail:0   skip:13 
skl-i5k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:123  dwarn:0   dfail:0   fail:0   skip:12 

Results at /archive/results/CI_IGT_test/Patchwork_1096/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-06  8:20   ` Daniel Vetter
@ 2016-01-06 17:53     ` Matt Roper
  2016-01-07  8:15       ` Jani Nikula
  0 siblings, 1 reply; 13+ messages in thread
From: Matt Roper @ 2016-01-06 17:53 UTC (permalink / raw)
  To: intel-gfx; +Cc: drm-intel-fixes

Our attempts save/restore panel power state in i915_suspend.c are
causing unclaimed register warnings on BXT since the registers for this
platform differ from older platforms.

The big hammer suspend/resume shouldn't be necessary for PP since the
connector/encoder hooks should already handle this.  In theory we could
remove this for all platforms, but in practice it's likely that would
cause some regressions since older platforms with LVDS may have
incomplete PP handling.  For now we'll leave the PCH save/restore alone
and change the non-PCH branch to only operate on gen <= 4 so that BXT
and future platforms aren't included.

v2: Typo fix: s/||/&&/

v3: Change non-PCH condition to a gen <= 4 test rather than listing
    VLV/CHV/BXT as specific platforms to exclude; should be more
    future-proof as we add new platforms.  (Daniel)

Cc: Vandana Kannan <vandana.kannan@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: drm-intel-fixes@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
Although it's the direction we want to move, I'm not brave enough to blow away
the entire PP save/restore for all platforms since I don't have the HW
necessary to deal with the regressions that might pop up.  The consensus on IRC
is that there probably will be a few regressions when we do that, so I'd rather
have people with appropriate platform access make those changes.

 drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a2aa09c..a8af594 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
 		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (INTEL_INFO(dev)->gen <= 4) {
 		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
 		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
 		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
@@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
 		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
-	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
+	} else if (INTEL_INFO(dev)->gen <= 4) {
 		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
 		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
 		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* ✓ success: Fi.CI.BAT
  2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
                   ` (3 preceding siblings ...)
  2016-01-06 12:20 ` ✓ success: Fi.CI.BAT Patchwork
@ 2016-01-07  7:20 ` Patchwork
  4 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2016-01-07  7:20 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx

== Summary ==

Built on 532a438d16e609a4b8f161c0a18b34f24001ed8f drm-intel-nightly: 2016y-01m-06d-15h-38m-17s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2) UNSTABLE
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a-frame-sequence:
                pass       -> DMESG-WARN (byt-nuc) UNSTABLE
        Subgroup read-crc-pipe-c:
                pass       -> SKIP       (bdw-nuci7)

bdw-nuci7        total:132  pass:1    dwarn:0   dfail:0   fail:0   skip:131
bsw-nuc-2        total:135  pass:115  dwarn:0   dfail:0   fail:0   skip:20 
byt-nuc          total:135  pass:120  dwarn:2   dfail:0   fail:0   skip:13 
skl-i5k-2        total:135  pass:126  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:123  dwarn:0   dfail:0   fail:0   skip:12 

Results at /archive/results/CI_IGT_test/Patchwork_1106/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-06 17:53     ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3) Matt Roper
@ 2016-01-07  8:15       ` Jani Nikula
  2016-01-07  9:28         ` Daniel Vetter
  2016-02-02 18:14         ` Imre Deak
  0 siblings, 2 replies; 13+ messages in thread
From: Jani Nikula @ 2016-01-07  8:15 UTC (permalink / raw)
  To: Matt Roper, intel-gfx; +Cc: drm-intel-fixes

On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> Our attempts save/restore panel power state in i915_suspend.c are
> causing unclaimed register warnings on BXT since the registers for this
> platform differ from older platforms.
>
> The big hammer suspend/resume shouldn't be necessary for PP since the
> connector/encoder hooks should already handle this.  In theory we could
> remove this for all platforms, but in practice it's likely that would
> cause some regressions since older platforms with LVDS may have
> incomplete PP handling.  For now we'll leave the PCH save/restore alone
> and change the non-PCH branch to only operate on gen <= 4 so that BXT
> and future platforms aren't included.
>
> v2: Typo fix: s/||/&&/
>
> v3: Change non-PCH condition to a gen <= 4 test rather than listing
>     VLV/CHV/BXT as specific platforms to exclude; should be more
>     future-proof as we add new platforms.  (Daniel)
>
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> Although it's the direction we want to move, I'm not brave enough to blow away
> the entire PP save/restore for all platforms since I don't have the HW
> necessary to deal with the regressions that might pop up.  The consensus on IRC
> is that there probably will be a few regressions when we do that, so I'd rather
> have people with appropriate platform access make those changes.

This is the first step we want to take anyway.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



>
>  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index a2aa09c..a8af594 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
>  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
>  		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
> -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> +	} else if (INTEL_INFO(dev)->gen <= 4) {
>  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
>  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
>  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
> @@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
>  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>  		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
> -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> +	} else if (INTEL_INFO(dev)->gen <= 4) {
>  		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>  		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>  		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-07  8:15       ` Jani Nikula
@ 2016-01-07  9:28         ` Daniel Vetter
  2016-01-07  9:40           ` Jani Nikula
  2016-02-02 18:14         ` Imre Deak
  1 sibling, 1 reply; 13+ messages in thread
From: Daniel Vetter @ 2016-01-07  9:28 UTC (permalink / raw)
  To: Jani Nikula; +Cc: drm-intel-fixes, intel-gfx

On Thu, Jan 07, 2016 at 10:15:00AM +0200, Jani Nikula wrote:
> On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> > Our attempts save/restore panel power state in i915_suspend.c are
> > causing unclaimed register warnings on BXT since the registers for this
> > platform differ from older platforms.
> >
> > The big hammer suspend/resume shouldn't be necessary for PP since the
> > connector/encoder hooks should already handle this.  In theory we could
> > remove this for all platforms, but in practice it's likely that would
> > cause some regressions since older platforms with LVDS may have
> > incomplete PP handling.  For now we'll leave the PCH save/restore alone
> > and change the non-PCH branch to only operate on gen <= 4 so that BXT
> > and future platforms aren't included.
> >
> > v2: Typo fix: s/||/&&/
> >
> > v3: Change non-PCH condition to a gen <= 4 test rather than listing
> >     VLV/CHV/BXT as specific platforms to exclude; should be more
> >     future-proof as we add new platforms.  (Daniel)
> >
> > Cc: Vandana Kannan <vandana.kannan@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > Although it's the direction we want to move, I'm not brave enough to blow away
> > the entire PP save/restore for all platforms since I don't have the HW
> > necessary to deal with the regressions that might pop up.  The consensus on IRC
> > is that there probably will be a few regressions when we do that, so I'd rather
> > have people with appropriate platform access make those changes.
> 
> This is the first step we want to take anyway.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Just looked at this some more, and the code here is the only code that
restores PP registers. Except for vlv/chv, where we have a call to
intel_dp_init_panel_power_sequencer_registers in vlv_power_sequencer_pipe.

I think we first need to sprinkle more calls to restore PP registers
around before we can disable this here for bxt. This patch here just
papers over a legit bug, without fixing it really.
-Daniel

> 
> 
> 
> >
> >  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index a2aa09c..a8af594 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
> >  		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
> >  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
> > @@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
> >  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
> >  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
> >  		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
> >  		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
> >  		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
> >  		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-07  9:28         ` Daniel Vetter
@ 2016-01-07  9:40           ` Jani Nikula
  2016-01-07 14:39             ` Daniel Vetter
  0 siblings, 1 reply; 13+ messages in thread
From: Jani Nikula @ 2016-01-07  9:40 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: drm-intel-fixes, intel-gfx

On Thu, 07 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jan 07, 2016 at 10:15:00AM +0200, Jani Nikula wrote:
>> On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
>> > Our attempts save/restore panel power state in i915_suspend.c are
>> > causing unclaimed register warnings on BXT since the registers for this
>> > platform differ from older platforms.
>> >
>> > The big hammer suspend/resume shouldn't be necessary for PP since the
>> > connector/encoder hooks should already handle this.  In theory we could
>> > remove this for all platforms, but in practice it's likely that would
>> > cause some regressions since older platforms with LVDS may have
>> > incomplete PP handling.  For now we'll leave the PCH save/restore alone
>> > and change the non-PCH branch to only operate on gen <= 4 so that BXT
>> > and future platforms aren't included.
>> >
>> > v2: Typo fix: s/||/&&/
>> >
>> > v3: Change non-PCH condition to a gen <= 4 test rather than listing
>> >     VLV/CHV/BXT as specific platforms to exclude; should be more
>> >     future-proof as we add new platforms.  (Daniel)
>> >
>> > Cc: Vandana Kannan <vandana.kannan@intel.com>
>> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Cc: drm-intel-fixes@lists.freedesktop.org
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> > Although it's the direction we want to move, I'm not brave enough to blow away
>> > the entire PP save/restore for all platforms since I don't have the HW
>> > necessary to deal with the regressions that might pop up.  The consensus on IRC
>> > is that there probably will be a few regressions when we do that, so I'd rather
>> > have people with appropriate platform access make those changes.
>> 
>> This is the first step we want to take anyway.
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
> Just looked at this some more, and the code here is the only code that
> restores PP registers. Except for vlv/chv, where we have a call to
> intel_dp_init_panel_power_sequencer_registers in vlv_power_sequencer_pipe.
>
> I think we first need to sprinkle more calls to restore PP registers
> around before we can disable this here for bxt. This patch here just
> papers over a legit bug, without fixing it really.

No, this patch doesn't paper over anything. The code that gets run for
Broxton before this patch doesn't save/restore any meaningful registers
that could possibly help with PP. This fixes unclaimed register
read/writes, and it's a valid fix as such.

The commit message should probably be fixed to say that we're not there
yet for Broxton with the hooks. But IMO this patch is a valid fix and
orthogonal to the issue of fixing the hooks.


BR,
Jani.


> -Daniel
>
>> 
>> 
>> 
>> >
>> >  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
>> > index a2aa09c..a8af594 100644
>> > --- a/drivers/gpu/drm/i915/i915_suspend.c
>> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
>> > @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device *dev)
>> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PCH_PP_ON_DELAYS);
>> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PCH_PP_OFF_DELAYS);
>> >  		dev_priv->regfile.savePP_DIVISOR = I915_READ(PCH_PP_DIVISOR);
>> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
>> >  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
>> >  		dev_priv->regfile.savePP_ON_DELAYS = I915_READ(PP_ON_DELAYS);
>> >  		dev_priv->regfile.savePP_OFF_DELAYS = I915_READ(PP_OFF_DELAYS);
>> > @@ -84,7 +84,7 @@ static void i915_restore_display(struct drm_device *dev)
>> >  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>> >  		I915_WRITE(PCH_PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>> >  		I915_WRITE(PCH_PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
>> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
>> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
>> >  		I915_WRITE(PP_ON_DELAYS, dev_priv->regfile.savePP_ON_DELAYS);
>> >  		I915_WRITE(PP_OFF_DELAYS, dev_priv->regfile.savePP_OFF_DELAYS);
>> >  		I915_WRITE(PP_DIVISOR, dev_priv->regfile.savePP_DIVISOR);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-07  9:40           ` Jani Nikula
@ 2016-01-07 14:39             ` Daniel Vetter
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Vetter @ 2016-01-07 14:39 UTC (permalink / raw)
  To: Jani Nikula; +Cc: annie.j.matheson, intel-gfx, drm-intel-fixes

On Thu, Jan 07, 2016 at 11:40:22AM +0200, Jani Nikula wrote:
> On Thu, 07 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jan 07, 2016 at 10:15:00AM +0200, Jani Nikula wrote:
> >> On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> >> > Our attempts save/restore panel power state in i915_suspend.c are
> >> > causing unclaimed register warnings on BXT since the registers for this
> >> > platform differ from older platforms.
> >> >
> >> > The big hammer suspend/resume shouldn't be necessary for PP since the
> >> > connector/encoder hooks should already handle this.  In theory we could
> >> > remove this for all platforms, but in practice it's likely that would
> >> > cause some regressions since older platforms with LVDS may have
> >> > incomplete PP handling.  For now we'll leave the PCH save/restore alone
> >> > and change the non-PCH branch to only operate on gen <= 4 so that BXT
> >> > and future platforms aren't included.
> >> >
> >> > v2: Typo fix: s/||/&&/
> >> >
> >> > v3: Change non-PCH condition to a gen <= 4 test rather than listing
> >> >     VLV/CHV/BXT as specific platforms to exclude; should be more
> >> >     future-proof as we add new platforms.  (Daniel)
> >> >
> >> > Cc: Vandana Kannan <vandana.kannan@intel.com>
> >> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> > Cc: Daniel Vetter <daniel@ffwll.ch>
> >> > Cc: drm-intel-fixes@lists.freedesktop.org
> >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> > ---
> >> > Although it's the direction we want to move, I'm not brave enough to blow away
> >> > the entire PP save/restore for all platforms since I don't have the HW
> >> > necessary to deal with the regressions that might pop up.  The consensus on IRC
> >> > is that there probably will be a few regressions when we do that, so I'd rather
> >> > have people with appropriate platform access make those changes.
> >>
> >> This is the first step we want to take anyway.
> >>
> >> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> >
> > Just looked at this some more, and the code here is the only code that
> > restores PP registers. Except for vlv/chv, where we have a call to
> > intel_dp_init_panel_power_sequencer_registers in vlv_power_sequencer_pipe.
> >
> > I think we first need to sprinkle more calls to restore PP registers
> > around before we can disable this here for bxt. This patch here just
> > papers over a legit bug, without fixing it really.
>
> No, this patch doesn't paper over anything. The code that gets run for
> Broxton before this patch doesn't save/restore any meaningful registers
> that could possibly help with PP. This fixes unclaimed register
> read/writes, and it's a valid fix as such.
>
> The commit message should probably be fixed to say that we're not there
> yet for Broxton with the hooks. But IMO this patch is a valid fix and
> orthogonal to the issue of fixing the hooks.

The hooks are good enough, at least they work for vlv/chv. If we can't fix
up PP restoring for bxt right away because it's too much work I think we
should at least put int a FIXME. Otherwise we'll promptly forget about
this again, invalidating the value unclaimed register access provides as a
hint that we haven't yet completed some crucial bxt enabling work.

Also, we need a jira to keep track of this. Adding Imre/Annie for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3)
  2016-01-07  8:15       ` Jani Nikula
  2016-01-07  9:28         ` Daniel Vetter
@ 2016-02-02 18:14         ` Imre Deak
  1 sibling, 0 replies; 13+ messages in thread
From: Imre Deak @ 2016-02-02 18:14 UTC (permalink / raw)
  To: Jani Nikula, Matt Roper, intel-gfx; +Cc: drm-intel-fixes

On to, 2016-01-07 at 10:15 +0200, Jani Nikula wrote:
> On Wed, 06 Jan 2016, Matt Roper <matthew.d.roper@intel.com> wrote:
> > Our attempts save/restore panel power state in i915_suspend.c are
> > causing unclaimed register warnings on BXT since the registers for
> > this
> > platform differ from older platforms.
> > 
> > The big hammer suspend/resume shouldn't be necessary for PP since
> > the
> > connector/encoder hooks should already handle this.  In theory we
> > could
> > remove this for all platforms, but in practice it's likely that
> > would
> > cause some regressions since older platforms with LVDS may have
> > incomplete PP handling.  For now we'll leave the PCH save/restore
> > alone
> > and change the non-PCH branch to only operate on gen <= 4 so that
> > BXT
> > and future platforms aren't included.
> > 
> > v2: Typo fix: s/||/&&/
> > 
> > v3: Change non-PCH condition to a gen <= 4 test rather than listing
> >     VLV/CHV/BXT as specific platforms to exclude; should be more
> >     future-proof as we add new platforms.  (Daniel)
> > 
> > Cc: Vandana Kannan <vandana.kannan@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> > Although it's the direction we want to move, I'm not brave enough
> > to blow away
> > the entire PP save/restore for all platforms since I don't have the
> > HW
> > necessary to deal with the regressions that might pop up.  The
> > consensus on IRC
> > is that there probably will be a few regressions when we do that,
> > so I'd rather
> > have people with appropriate platform access make those changes.
> 
> This is the first step we want to take anyway.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks for the patch and review, I pushed the patch to dinq.

--Imre

> 
> 
> 
> > 
> >  drivers/gpu/drm/i915/i915_suspend.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c
> > b/drivers/gpu/drm/i915/i915_suspend.c
> > index a2aa09c..a8af594 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -49,7 +49,7 @@ static void i915_save_display(struct drm_device
> > *dev)
> >  		dev_priv->regfile.savePP_ON_DELAYS =
> > I915_READ(PCH_PP_ON_DELAYS);
> >  		dev_priv->regfile.savePP_OFF_DELAYS =
> > I915_READ(PCH_PP_OFF_DELAYS);
> >  		dev_priv->regfile.savePP_DIVISOR =
> > I915_READ(PCH_PP_DIVISOR);
> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
> >  		dev_priv->regfile.savePP_CONTROL =
> > I915_READ(PP_CONTROL);
> >  		dev_priv->regfile.savePP_ON_DELAYS =
> > I915_READ(PP_ON_DELAYS);
> >  		dev_priv->regfile.savePP_OFF_DELAYS =
> > I915_READ(PP_OFF_DELAYS);
> > @@ -84,7 +84,7 @@ static void i915_restore_display(struct
> > drm_device *dev)
> >  		I915_WRITE(PCH_PP_OFF_DELAYS, dev_priv-
> > >regfile.savePP_OFF_DELAYS);
> >  		I915_WRITE(PCH_PP_DIVISOR, dev_priv-
> > >regfile.savePP_DIVISOR);
> >  		I915_WRITE(PCH_PP_CONTROL, dev_priv-
> > >regfile.savePP_CONTROL);
> > -	} else if (!IS_VALLEYVIEW(dev) && !IS_CHERRYVIEW(dev)) {
> > +	} else if (INTEL_INFO(dev)->gen <= 4) {
> >  		I915_WRITE(PP_ON_DELAYS, dev_priv-
> > >regfile.savePP_ON_DELAYS);
> >  		I915_WRITE(PP_OFF_DELAYS, dev_priv-
> > >regfile.savePP_OFF_DELAYS);
> >  		I915_WRITE(PP_DIVISOR, dev_priv-
> > >regfile.savePP_DIVISOR);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-02-02 18:14 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-06  1:44 [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend Matt Roper
2016-01-06  1:47 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v2) Matt Roper
2016-01-06  8:20   ` Daniel Vetter
2016-01-06 17:53     ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend (v3) Matt Roper
2016-01-07  8:15       ` Jani Nikula
2016-01-07  9:28         ` Daniel Vetter
2016-01-07  9:40           ` Jani Nikula
2016-01-07 14:39             ` Daniel Vetter
2016-02-02 18:14         ` Imre Deak
2016-01-06  2:06 ` [PATCH] drm/i915/bxt: Don't save/restore eDP panel power during suspend kbuild test robot
2016-01-06  6:04 ` kbuild test robot
2016-01-06 12:20 ` ✓ success: Fi.CI.BAT Patchwork
2016-01-07  7:20 ` Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.