All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms
@ 2018-07-25  7:22 Dhinakaran Pandiyan
  2018-07-25  8:20 ` ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-25  7:22 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodigo Vivi

We have merged several fixes, re-written some tests and improved debug
capability in the past several months, so this is a good time to give PSR1
another try. PSR1 has not been tested on HSW and BDW recently, so let's
enable only on gen9+ now.

Cc: Rodigo Vivi <rodrigo.vivi@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4bd5768731ee..942db85da6a1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -471,10 +471,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 	if (!CAN_PSR(dev_priv))
 		return;
 
-	if (!i915_modparams.enable_psr) {
-		DRM_DEBUG_KMS("PSR disable by flag\n");
+	if (!i915_modparams.enable_psr)
 		return;
-	}
 
 	/*
 	 * HSW spec explicitly says PSR is tied to port A.
@@ -516,7 +514,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
 	}
 
 	crtc_state->has_psr = true;
-	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
+
+	/* Enable only PSR 1 by default for now */
+	crtc_state->has_psr2 = i915_modparams.enable_psr == 1 &&
+			       intel_psr2_config_valid(intel_dp, crtc_state);
+
 	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
 }
 
@@ -956,12 +958,10 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 	if (!dev_priv->psr.sink_support)
 		return;
 
-	if (i915_modparams.enable_psr == -1) {
-		i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
-
-		/* Per platform default: all disabled. */
-		i915_modparams.enable_psr = 0;
-	}
+	/* Enable PSR 1 default only on gen9+ */
+	if (i915_modparams.enable_psr == -1)
+		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
+			i915_modparams.enable_psr = 0;
 
 	/* Set link_standby x link_off defaults */
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-- 
2.17.1

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

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

* ✓ Fi.CI.BAT: success for drm/i915/psr: Enable PSR1 by default on gen9+ platforms
  2018-07-25  7:22 [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms Dhinakaran Pandiyan
@ 2018-07-25  8:20 ` Patchwork
  2018-07-25  9:31 ` ✓ Fi.CI.IGT: " Patchwork
  2018-07-25 16:12 ` [PATCH] " Rodrigo Vivi
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-07-25  8:20 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Enable PSR1 by default on gen9+ platforms
URL   : https://patchwork.freedesktop.org/series/47188/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4538 -> Patchwork_9761 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/47188/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9761:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_hangcheck:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL

    igt@drv_selftest@live_hugepages:
      {fi-cfl-8109u}:     PASS -> DMESG-WARN +9

    
== Known issues ==

  Here are the changes found in Patchwork_9761 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_selftest@live_workarounds:
      {fi-cfl-8109u}:     PASS -> DMESG-FAIL (fdo#107292)

    igt@gem_ctx_create@basic-files:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#105719) +1

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       PASS -> DMESG-WARN (fdo#105128, fdo#107139)

    igt@kms_chamelium@hdmi-hpd-fast:
      fi-kbl-7500u:       SKIP -> FAIL (fdo#102672, fdo#103841)

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       PASS -> DMESG-FAIL (fdo#106103, fdo#102614)

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2520m:       NOTRUN -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@drv_module_reload@basic-no-display:
      fi-glk-j4005:       DMESG-WARN (fdo#106725) -> PASS

    igt@drv_selftest@live_workarounds:
      fi-kbl-x1275:       DMESG-FAIL (fdo#107292) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#102672 https://bugs.freedesktop.org/show_bug.cgi?id=102672
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103841 https://bugs.freedesktop.org/show_bug.cgi?id=103841
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128
  fdo#105719 https://bugs.freedesktop.org/show_bug.cgi?id=105719
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725
  fdo#107139 https://bugs.freedesktop.org/show_bug.cgi?id=107139
  fdo#107292 https://bugs.freedesktop.org/show_bug.cgi?id=107292


== Participating hosts (51 -> 44) ==

  Missing    (7): fi-ilk-m540 fi-bxt-dsi fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


== Build changes ==

    * Linux: CI_DRM_4538 -> Patchwork_9761

  CI_DRM_4538: 0233efd326b819549668b71be377d405266120ba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4573: 2884f91dd6d7682ea738ef6f0943cc591643dda2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9761: 0b8dc507cfca99610648a2eaf67da0da0bfdf4be @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0b8dc507cfca drm/i915/psr: Enable PSR1 by default on gen9+ platforms

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9761/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* ✓ Fi.CI.IGT: success for drm/i915/psr: Enable PSR1 by default on gen9+ platforms
  2018-07-25  7:22 [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms Dhinakaran Pandiyan
  2018-07-25  8:20 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2018-07-25  9:31 ` Patchwork
  2018-07-25 16:12 ` [PATCH] " Rodrigo Vivi
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2018-07-25  9:31 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/psr: Enable PSR1 by default on gen9+ platforms
URL   : https://patchwork.freedesktop.org/series/47188/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4538_full -> Patchwork_9761_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9761_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9761_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9761_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-vebox:
      shard-kbl:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9761_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_exec_flush@basic-wb-rw-default:
      shard-snb:          PASS -> INCOMPLETE (fdo#105411)

    igt@gem_ppgtt@blt-vs-render-ctx0:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          FAIL (fdo#105189) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-hsw:          FAIL (fdo#100368) -> PASS

    igt@kms_flip@plain-flip-ts-check:
      shard-glk:          FAIL (fdo#100368) -> PASS

    igt@kms_setmode@basic:
      shard-apl:          FAIL (fdo#99912) -> PASS
      shard-kbl:          FAIL (fdo#99912) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105189 https://bugs.freedesktop.org/show_bug.cgi?id=105189
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4538 -> Patchwork_9761

  CI_DRM_4538: 0233efd326b819549668b71be377d405266120ba @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4573: 2884f91dd6d7682ea738ef6f0943cc591643dda2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9761: 0b8dc507cfca99610648a2eaf67da0da0bfdf4be @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9761/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms
  2018-07-25  7:22 [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms Dhinakaran Pandiyan
  2018-07-25  8:20 ` ✓ Fi.CI.BAT: success for " Patchwork
  2018-07-25  9:31 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-07-25 16:12 ` Rodrigo Vivi
  2018-07-25 16:52   ` Dhinakaran Pandiyan
  2 siblings, 1 reply; 6+ messages in thread
From: Rodrigo Vivi @ 2018-07-25 16:12 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, Dhinakaran Pandiyan

On Wed, Jul 25, 2018 at 12:22:28AM -0700, Dhinakaran Pandiyan wrote:
> We have merged several fixes, re-written some tests and improved debug
> capability in the past several months, so this is a good time to give PSR1
> another try. PSR1 has not been tested on HSW and BDW recently, so let's
> enable only on gen9+ now.
> 
> Cc: Rodigo Vivi <rodrigo.vivi@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 4bd5768731ee..942db85da6a1 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -471,10 +471,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	if (!CAN_PSR(dev_priv))
>  		return;
>  
> -	if (!i915_modparams.enable_psr) {
> -		DRM_DEBUG_KMS("PSR disable by flag\n");

Why are you removing the message?
I think it is still useful... and enable_psr == -1 doesn't trigger that.

> +	if (!i915_modparams.enable_psr)
>  		return;
> -	}
>  
>  	/*
>  	 * HSW spec explicitly says PSR is tied to port A.
> @@ -516,7 +514,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  	}
>  
>  	crtc_state->has_psr = true;
> -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> +
> +	/* Enable only PSR 1 by default for now */
> +	crtc_state->has_psr2 = i915_modparams.enable_psr == 1 &&
> +			       intel_psr2_config_valid(intel_dp, crtc_state);
> +

this might get confusing...
-1 - enable psr1
0 - disable
1 - enable psr2

and far from the variable... Well... I want to kill the parameter anyways
so no hard feelings on having this here, but what about some debug messages
at least?

/* Enable only PSR1 by default for now */
if (i915_modparams.enable_psr == -1) {
	DRM_DEBUG_KMS("Avoiding PSR2 by platform default")
	crtc_state->has_psr2 = 0;
} else {
	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
}

>  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ? "2" : "");
>  }
>  
> @@ -956,12 +958,10 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>  	if (!dev_priv->psr.sink_support)
>  		return;
>  
> -	if (i915_modparams.enable_psr == -1) {
> -		i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
> -
> -		/* Per platform default: all disabled. */
> -		i915_modparams.enable_psr = 0;
> -	}
> +	/* Enable PSR 1 default only on gen9+ */
> +	if (i915_modparams.enable_psr == -1)
> +		if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
> +			i915_modparams.enable_psr = 0;

we talked about this in person, but just for the record:
we need to check cnl and icl on CI for psr cases before make this > 9.

>
>  	/* Set link_standby x link_off defaults */
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms
  2018-07-25 16:12 ` [PATCH] " Rodrigo Vivi
@ 2018-07-25 16:52   ` Dhinakaran Pandiyan
  2018-07-25 16:56     ` Rodrigo Vivi
  0 siblings, 1 reply; 6+ messages in thread
From: Dhinakaran Pandiyan @ 2018-07-25 16:52 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

On Wed, 2018-07-25 at 09:12 -0700, Rodrigo Vivi wrote:
> On Wed, Jul 25, 2018 at 12:22:28AM -0700, Dhinakaran Pandiyan wrote:
> > 
> > We have merged several fixes, re-written some tests and improved
> > debug
> > capability in the past several months, so this is a good time to
> > give PSR1
> > another try. PSR1 has not been tested on HSW and BDW recently, so
> > let's
> > enable only on gen9+ now.
> > 
> > Cc: Rodigo Vivi <rodrigo.vivi@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 4bd5768731ee..942db85da6a1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -471,10 +471,8 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  	if (!CAN_PSR(dev_priv))
> >  		return;
> >  
> > -	if (!i915_modparams.enable_psr) {
> > -		DRM_DEBUG_KMS("PSR disable by flag\n");
> Why are you removing the message?
> I think it is still useful... and enable_psr == -1 doesn't trigger
> that.
> 
The text was a bit vague to start with, and is confusing when combined
with this patch. Agreed, it is useful to have a debug message, I'll
replace it.

> > 
> > +	if (!i915_modparams.enable_psr)
> >  		return;
> > -	}
> >  
> >  	/*
> >  	 * HSW spec explicitly says PSR is tied to port A.
> > @@ -516,7 +514,11 @@ void intel_psr_compute_config(struct intel_dp
> > *intel_dp,
> >  	}
> >  
> >  	crtc_state->has_psr = true;
> > -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > crtc_state);
> > +
> > +	/* Enable only PSR 1 by default for now */
> > +	crtc_state->has_psr2 = i915_modparams.enable_psr == 1 &&
> > +			       intel_psr2_config_valid(intel_dp,
> > crtc_state);
> > +
> this might get confusing...
> -1 - enable psr1
> 0 - disable
> 1 - enable psr2
> 
> and far from the variable... Well... I want to kill the parameter
> anyways
> so no hard feelings on having this here, but what about some debug
> messages
> at least?
> 
> /* Enable only PSR1 by default for now */
> if (i915_modparams.enable_psr == -1) {
> 	DRM_DEBUG_KMS("Avoiding PSR2 by platform default")
> 	crtc_state->has_psr2 = 0;
> } else {
> 	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> crtc_state);
> }
> 

The reason I added a check for i915.enable_psr==1 was to enable PSR2
only when the user passes the exact value. Otherwise, we should fall
back to default.

> > 
> >  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ?
> > "2" : "");
> >  }
> >  
> > @@ -956,12 +958,10 @@ void intel_psr_init(struct drm_i915_private
> > *dev_priv)
> >  	if (!dev_priv->psr.sink_support)
> >  		return;
> >  
> > -	if (i915_modparams.enable_psr == -1) {
> > -		i915_modparams.enable_psr = dev_priv-
> > >vbt.psr.enable;
> > -
> > -		/* Per platform default: all disabled. */
> > -		i915_modparams.enable_psr = 0;
> > -	}
> > +	/* Enable PSR 1 default only on gen9+ */
> > +	if (i915_modparams.enable_psr == -1)
> > +		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > >vbt.psr.enable)
> > +			i915_modparams.enable_psr = 0;
> we talked about this in person, but just for the record:
> we need to check cnl and icl on CI for psr cases before make this >
> 9.

The failures on ICL are due to an unrelated debug warning. The CNL ones
are interesting, most likely due to us enabling PSR2 by setting the
module parameter=1 from the IGTs. But, it still should not be failing,
I'll check.



> 
> > 
> > 
> >  	/* Set link_standby x link_off defaults */
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms
  2018-07-25 16:52   ` Dhinakaran Pandiyan
@ 2018-07-25 16:56     ` Rodrigo Vivi
  0 siblings, 0 replies; 6+ messages in thread
From: Rodrigo Vivi @ 2018-07-25 16:56 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx

On Wed, Jul 25, 2018 at 09:52:44AM -0700, Dhinakaran Pandiyan wrote:
> On Wed, 2018-07-25 at 09:12 -0700, Rodrigo Vivi wrote:
> > On Wed, Jul 25, 2018 at 12:22:28AM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > We have merged several fixes, re-written some tests and improved
> > > debug
> > > capability in the past several months, so this is a good time to
> > > give PSR1
> > > another try. PSR1 has not been tested on HSW and BDW recently, so
> > > let's
> > > enable only on gen9+ now.
> > > 
> > > Cc: Rodigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_psr.c | 20 ++++++++++----------
> > >  1 file changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 4bd5768731ee..942db85da6a1 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -471,10 +471,8 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  	if (!CAN_PSR(dev_priv))
> > >  		return;
> > >  
> > > -	if (!i915_modparams.enable_psr) {
> > > -		DRM_DEBUG_KMS("PSR disable by flag\n");
> > Why are you removing the message?
> > I think it is still useful... and enable_psr == -1 doesn't trigger
> > that.
> > 
> The text was a bit vague to start with, and is confusing when combined
> with this patch. Agreed, it is useful to have a debug message, I'll
> replace it.
> 
> > > 
> > > +	if (!i915_modparams.enable_psr)
> > >  		return;
> > > -	}
> > >  
> > >  	/*
> > >  	 * HSW spec explicitly says PSR is tied to port A.
> > > @@ -516,7 +514,11 @@ void intel_psr_compute_config(struct intel_dp
> > > *intel_dp,
> > >  	}
> > >  
> > >  	crtc_state->has_psr = true;
> > > -	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > > crtc_state);
> > > +
> > > +	/* Enable only PSR 1 by default for now */
> > > +	crtc_state->has_psr2 = i915_modparams.enable_psr == 1 &&
> > > +			       intel_psr2_config_valid(intel_dp,
> > > crtc_state);
> > > +
> > this might get confusing...
> > -1 - enable psr1
> > 0 - disable
> > 1 - enable psr2
> > 
> > and far from the variable... Well... I want to kill the parameter
> > anyways
> > so no hard feelings on having this here, but what about some debug
> > messages
> > at least?
> > 
> > /* Enable only PSR1 by default for now */
> > if (i915_modparams.enable_psr == -1) {
> > 	DRM_DEBUG_KMS("Avoiding PSR2 by platform default")
> > 	crtc_state->has_psr2 = 0;
> > } else {
> > 	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp,
> > crtc_state);
> > }
> > 
> 
> The reason I added a check for i915.enable_psr==1 was to enable PSR2
> only when the user passes the exact value. Otherwise, we should fall
> back to default.

well, it could be == 1 check inverting my block here...
but my main point is to have some kind of debug message ;)

> 
> > > 
> > >  	DRM_DEBUG_KMS("Enabling PSR%s\n", crtc_state->has_psr2 ?
> > > "2" : "");
> > >  }
> > >  
> > > @@ -956,12 +958,10 @@ void intel_psr_init(struct drm_i915_private
> > > *dev_priv)
> > >  	if (!dev_priv->psr.sink_support)
> > >  		return;
> > >  
> > > -	if (i915_modparams.enable_psr == -1) {
> > > -		i915_modparams.enable_psr = dev_priv-
> > > >vbt.psr.enable;
> > > -
> > > -		/* Per platform default: all disabled. */
> > > -		i915_modparams.enable_psr = 0;
> > > -	}
> > > +	/* Enable PSR 1 default only on gen9+ */
> > > +	if (i915_modparams.enable_psr == -1)
> > > +		if (INTEL_GEN(dev_priv) < 9 || !dev_priv-
> > > >vbt.psr.enable)
> > > +			i915_modparams.enable_psr = 0;
> > we talked about this in person, but just for the record:
> > we need to check cnl and icl on CI for psr cases before make this >
> > 9.
> 
> The failures on ICL are due to an unrelated debug warning. The CNL ones
> are interesting, most likely due to us enabling PSR2 by setting the
> module parameter=1 from the IGTs. But, it still should not be failing,
> I'll check.
> 
> 
> 
> > 
> > > 
> > > 
> > >  	/* Set link_standby x link_off defaults */
> > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

end of thread, other threads:[~2018-07-25 16:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-25  7:22 [PATCH] drm/i915/psr: Enable PSR1 by default on gen9+ platforms Dhinakaran Pandiyan
2018-07-25  8:20 ` ✓ Fi.CI.BAT: success for " Patchwork
2018-07-25  9:31 ` ✓ Fi.CI.IGT: " Patchwork
2018-07-25 16:12 ` [PATCH] " Rodrigo Vivi
2018-07-25 16:52   ` Dhinakaran Pandiyan
2018-07-25 16:56     ` Rodrigo Vivi

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.