| Message ID | 20180725072228.2469-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/psr: Enable PSR1 by default on gen9+ platforms | expand |
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 >
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))
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))
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))
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(-)