| Message ID | 20180906235202.29865-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/psr: Enable PSR1 on gen-9+ HW | expand |
On Thu, Sep 06, 2018 at 04:52:02PM -0700, Dhinakaran Pandiyan wrote: > We have new tests and fixes in place since the feature was last > disabled. > > Try again for gen-9+ hardware and enable only PSR1 as a first step. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Jose Roberto de Souza <jose.souza@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > References: commit 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW") > References: commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."") > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index b6838b525502..fc823f93a4dc 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug) > static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > const struct intel_crtc_state *crtc_state) > { > + /* Disable PSR2 by default for all platforms */ > + if (i915_modparams.enable_psr == -1) > + return false; > + > switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > case I915_PSR_DEBUG_FORCE_PSR1: > return false; > @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > * intel_psr_init - Init basic PSR work and mutex. > * @dev_priv: i915 device private > * > - * This function is called only once at driver load to initialize basic > + * This function is called only once at driver load to initialize basic > * PSR stuff. > */ > void intel_psr_init(struct drm_i915_private *dev_priv) > @@ -1065,19 +1069,14 @@ 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; > - } > + 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)) > /* HSW and BDW require workarounds that we don't implement. */ > dev_priv->psr.link_standby = false; > else > - /* For new platforms let's respect VBT back again */ bikeshed: Can we please leave the clean-up for a separated patch? In case we need to revert we don't loose the clean-up part! :$ Also a bikeshed of bikeshed: I think we need to revisit this block entirely anyways. I can't remember why we stopped respecting the bspec here. And probably this was only masking some issues that got fixed during your great journey! ;) Maybe this block even explain the current gap on hsw and bdw?! ;) > dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link; > > INIT_WORK(&dev_priv->psr.work, intel_psr_work); > -- > 2.17.1 >
On Thu, 2018-09-06 at 22:06 -0700, Rodrigo Vivi wrote: > On Thu, Sep 06, 2018 at 04:52:02PM -0700, Dhinakaran Pandiyan wrote: > > We have new tests and fixes in place since the feature was last > > disabled. > > > > Try again for gen-9+ hardware and enable only PSR1 as a first step. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Jose Roberto de Souza <jose.souza@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > References: commit 2ee7dc497e34 ("drm/i915: disable PSR by default > > on HSW/BDW") > > References: commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by > > default on Valleyview and Cherryview."") > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index b6838b525502..fc823f93a4dc 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug) > > static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > > const struct intel_crtc_state > > *crtc_state) > > { > > + /* Disable PSR2 by default for all platforms */ > > + if (i915_modparams.enable_psr == -1) > > + return false; > > + > > switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > > case I915_PSR_DEBUG_FORCE_PSR1: > > return false; > > @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private > > *dev_priv, > > * intel_psr_init - Init basic PSR work and mutex. > > * @dev_priv: i915 device private > > * > > - * This function is called only once at driver load to initialize > > basic > > + * This function is called only once at driver load to initialize > > basic > > * PSR stuff. > > */ > > void intel_psr_init(struct drm_i915_private *dev_priv) > > @@ -1065,19 +1069,14 @@ 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; > > - } > > + 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)) > > /* HSW and BDW require workarounds that we don't > > implement. */ > > dev_priv->psr.link_standby = false; > > else > > - /* For new platforms let's respect VBT back again > > */ > > bikeshed: Can we please leave the clean-up for a separated patch? > In case we need to revert we don't loose the clean-up part! :$ > Okay. > Also a bikeshed of bikeshed: I think we need to revisit this block > entirely > anyways. I can't remember why we stopped respecting the bspec here. > And probably this was only masking some issues that got fixed during > your great journey! ;) > > Maybe this block even explain the current gap on hsw and bdw?! ;) The gap is I haven't had time to investigate :) > > > dev_priv->psr.link_standby = dev_priv- > > >vbt.psr.full_link; > > > > INIT_WORK(&dev_priv->psr.work, intel_psr_work); > > -- > > 2.17.1 > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Sep 06, 2018 at 10:06:09PM -0700, Rodrigo Vivi wrote: > On Thu, Sep 06, 2018 at 04:52:02PM -0700, Dhinakaran Pandiyan wrote: > > We have new tests and fixes in place since the feature was last > > disabled. > > > > Try again for gen-9+ hardware and enable only PSR1 as a first step. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Jose Roberto de Souza <jose.souza@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > References: commit 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW") > > References: commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."") > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index b6838b525502..fc823f93a4dc 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug) > > static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > > const struct intel_crtc_state *crtc_state) > > { > > + /* Disable PSR2 by default for all platforms */ > > + if (i915_modparams.enable_psr == -1) > > + return false; > > + > > switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > > case I915_PSR_DEBUG_FORCE_PSR1: > > return false; > > @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, > > * intel_psr_init - Init basic PSR work and mutex. > > * @dev_priv: i915 device private > > * > > - * This function is called only once at driver load to initialize basic > > + * This function is called only once at driver load to initialize basic > > * PSR stuff. > > */ > > void intel_psr_init(struct drm_i915_private *dev_priv) > > @@ -1065,19 +1069,14 @@ 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; > > - } > > + 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)) > > /* HSW and BDW require workarounds that we don't implement. */ > > dev_priv->psr.link_standby = false; > > else > > - /* For new platforms let's respect VBT back again */ > > bikeshed: Can we please leave the clean-up for a separated patch? > In case we need to revert we don't loose the clean-up part! :$ > > Also a bikeshed of bikeshed: I think we need to revisit this block entirely > anyways. I can't remember why we stopped respecting the bspec here. > And probably this was only masking some issues that got fixed during > your great journey! ;) Another vbt related thing was the aux handshake thing. We tried it here https://patchwork.freedesktop.org/series/8046/ but IIRC it caused some problems that no one had time to diagnose so we never merged that stuff. Not sure if anyone wants to try and figure out what went wrong there. Actually, after a bit more digging I guess the fails were listed here https://lists.freedesktop.org/archives/intel-gfx/2016-June/097379.html Some sink crc issues, but as that was deemed unusable anyway maybe there was nothing wrong after all?
On Tue, 2018-09-11 at 17:04 +0300, Ville Syrjälä wrote: > On Thu, Sep 06, 2018 at 10:06:09PM -0700, Rodrigo Vivi wrote: > > On Thu, Sep 06, 2018 at 04:52:02PM -0700, Dhinakaran Pandiyan > > wrote: > > > We have new tests and fixes in place since the feature was last > > > disabled. > > > > > > Try again for gen-9+ hardware and enable only PSR1 as a first > > > step. > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Jose Roberto de Souza <jose.souza@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > References: commit 2ee7dc497e34 ("drm/i915: disable PSR by > > > default on HSW/BDW") > > > References: commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by > > > default on Valleyview and Cherryview."") > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com > > > > > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++--------- > > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index b6838b525502..fc823f93a4dc 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug) > > > static bool intel_psr2_enabled(struct drm_i915_private > > > *dev_priv, > > > const struct intel_crtc_state > > > *crtc_state) > > > { > > > + /* Disable PSR2 by default for all platforms */ > > > + if (i915_modparams.enable_psr == -1) > > > + return false; > > > + > > > switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) > > > { > > > case I915_PSR_DEBUG_FORCE_PSR1: > > > return false; > > > @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct > > > drm_i915_private *dev_priv, > > > * intel_psr_init - Init basic PSR work and mutex. > > > * @dev_priv: i915 device private > > > * > > > - * This function is called only once at driver load to > > > initialize basic > > > + * This function is called only once at driver load to > > > initialize basic > > > * PSR stuff. > > > */ > > > void intel_psr_init(struct drm_i915_private *dev_priv) > > > @@ -1065,19 +1069,14 @@ 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; > > > - } > > > + 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)) > > > /* HSW and BDW require workarounds that we don't > > > implement. */ > > > dev_priv->psr.link_standby = false; > > > else > > > - /* For new platforms let's respect VBT back > > > again */ > > > > bikeshed: Can we please leave the clean-up for a separated patch? > > In case we need to revert we don't loose the clean-up part! :$ > > > > Also a bikeshed of bikeshed: I think we need to revisit this block > > entirely > > anyways. I can't remember why we stopped respecting the bspec here. > > And probably this was only masking some issues that got fixed > > during > > your great journey! ;) > > Another vbt related thing was the aux handshake thing. We tried it > here > https://patchwork.freedesktop.org/series/8046/ but IIRC it caused > some > problems that no one had time to diagnose so we never merged that > stuff. > Not sure if anyone wants to try and figure out what went wrong there. > I had to check with Art about this; we do need AUX handshake to wake up the sink like the spec says. The current code is right. > Actually, after a bit more digging I guess the fails were listed here > https://lists.freedesktop.org/archives/intel-gfx/2016-June/097379.htm > l > Some sink crc issues, but as that was deemed unusable anyway maybe > there was nothing wrong after all? Sink crc issues should not been seen anymore. >
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b6838b525502..fc823f93a4dc 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug) static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, const struct intel_crtc_state *crtc_state) { + /* Disable PSR2 by default for all platforms */ + if (i915_modparams.enable_psr == -1) + return false; + switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { case I915_PSR_DEBUG_FORCE_PSR1: return false; @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv, * intel_psr_init - Init basic PSR work and mutex. * @dev_priv: i915 device private * - * This function is called only once at driver load to initialize basic + * This function is called only once at driver load to initialize basic * PSR stuff. */ void intel_psr_init(struct drm_i915_private *dev_priv) @@ -1065,19 +1069,14 @@ 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; - } + 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)) /* HSW and BDW require workarounds that we don't implement. */ dev_priv->psr.link_standby = false; else - /* For new platforms let's respect VBT back again */ dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link; INIT_WORK(&dev_priv->psr.work, intel_psr_work);
We have new tests and fixes in place since the feature was last disabled. Try again for gen-9+ hardware and enable only PSR1 as a first step. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Jose Roberto de Souza <jose.souza@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> References: commit 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW") References: commit dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on Valleyview and Cherryview."") Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)