| Message ID | 20200218203916.58636-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/psr: Force PSR probe only after full initialization | expand |
On Tue, 2020-02-18 at 12:39 -0800, José Roberto de Souza wrote: > Commit 60c6a14b489b ("drm/i915/display: Force the state compute phase > once to enable PSR") was forcing the state compute too earlier > causing errors because not everything was initialized, so here > moving to i915_driver_register() when everything is ready and driver > is registering into the rest of the system. > > Also fixing the place where it disarm the force probe as during the > atomic check phase errors could happen like the ones due locking and > it would cause PSR to never be enabled if that happens. > Leaving the disarm to the atomic commit phase, intel_psr_enable() or > intel_psr_update() will be called even if the current state do not > allow PSR to be enabled. > > v2: Check if intel_dp is null in intel_psr_force_mode_changed_set() > v3: Check intel_dp before get dev_priv > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute phase > once to enable PSR") > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 > Tested-by: Ross Zwisler <zwisler@google.com> > Reported-by: Ross Zwisler <zwisler@google.com> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 22 ++++++++++++++++++++-- > drivers/gpu/drm/i915/display/intel_psr.h | 1 + > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > drivers/gpu/drm/i915/i915_drv.h | 2 +- > 4 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index b4942b6445ae..2a0f7354fba5 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp *intel_dp, > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + intel_psr_force_mode_changed_set(intel_dp, false); > + Hi, intel_psr_enable() and intel_psr_update already have checking routine for CAN_PSR and has_psr. therefore we don't need to check twice here. And if there are no issues that moving "disarming force_mode_changed" to intel_psr_compute_config(), can we move them to intel_psr_compute_config()? > if (!crtc_state->has_psr) > return; > > @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp > *intel_dp, > struct i915_psr *psr = &dev_priv->psr; > bool enable, psr2_enable; > > + intel_psr_force_mode_changed_set(intel_dp, false); > + > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp) > return; > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct > drm_connector *connector, > struct drm_crtc_state *crtc_state; > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > - dev_priv->psr.initially_probed) > + !dev_priv->psr.force_mode_changed) > return; > > intel_connector = to_intel_connector(connector); > @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct > drm_connector *connector, > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > new_state->crtc); > crtc_state->mode_changed = true; > - dev_priv->psr.initially_probed = true; > +} > + > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > bool set) IMHO, it would be better intel_psr_set_force_mode_changed() as a function name. > +{ > + struct drm_i915_private *dev_priv; > + > + if (!intel_dp) > + return; > + > + dev_priv = dp_to_i915(intel_dp); > + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || > + intel_dp != dev_priv->psr.dp) > + return; > + > + dev_priv->psr.force_mode_changed = set; > } > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > b/drivers/gpu/drm/i915/display/intel_psr.h > index c58a1d438808..27a70468e2b9 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.h > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp *intel_dp); > void intel_psr_atomic_check(struct drm_connector *connector, > struct drm_connector_state *old_state, > struct drm_connector_state *new_state); > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > bool set); > > #endif /* __INTEL_PSR_H__ */ > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index f7a1c33697b7..83791c197611 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -58,6 +58,7 @@ > #include "display/intel_hotplug.h" > #include "display/intel_overlay.h" > #include "display/intel_pipe_crc.h" > +#include "display/intel_psr.h" > #include "display/intel_sprite.h" > #include "display/intel_vga.h" > > @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct > drm_i915_private *dev_priv) > > intel_audio_init(dev_priv); > > + intel_psr_force_mode_changed_set(dev_priv->psr.dp, true); > + > /* > * Some ports require correctly set-up hpd registers for > detection to > * work properly (leading to ghost connected connector status), > e.g. VGA > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 3330b538d379..a546655072bd 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -505,7 +505,7 @@ struct i915_psr { > bool dc3co_enabled; > u32 dc3co_exit_delay; > struct delayed_work dc3co_work; > - bool initially_probed; > + bool force_mode_changed; > }; > > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
On Thu, 2020-02-20 at 12:39 +0000, Mun, Gwan-gyeong wrote: > On Tue, 2020-02-18 at 12:39 -0800, José Roberto de Souza wrote: > > Commit 60c6a14b489b ("drm/i915/display: Force the state compute > > phase > > once to enable PSR") was forcing the state compute too earlier > > causing errors because not everything was initialized, so here > > moving to i915_driver_register() when everything is ready and > > driver > > is registering into the rest of the system. > > > > Also fixing the place where it disarm the force probe as during the > > atomic check phase errors could happen like the ones due locking > > and > > it would cause PSR to never be enabled if that happens. > > Leaving the disarm to the atomic commit phase, intel_psr_enable() > > or > > intel_psr_update() will be called even if the current state do not > > allow PSR to be enabled. > > > > v2: Check if intel_dp is null in intel_psr_force_mode_changed_set() > > v3: Check intel_dp before get dev_priv > > > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute > > phase > > once to enable PSR") > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 > > Tested-by: Ross Zwisler <zwisler@google.com> > > Reported-by: Ross Zwisler <zwisler@google.com> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 22 > > ++++++++++++++++++++-- > > drivers/gpu/drm/i915/display/intel_psr.h | 1 + > > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > 4 files changed, 25 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > b/drivers/gpu/drm/i915/display/intel_psr.c > > index b4942b6445ae..2a0f7354fba5 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp > > *intel_dp, > > { > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > + > Hi, > intel_psr_enable() and intel_psr_update already have checking routine > for CAN_PSR and has_psr. > therefore we don't need to check twice here. Minor overhead but if you really want I can remove the function call and just do a dev_priv->psr.force_mode_changed = false; for intel_psr_enable/update > And if there are no issues that moving "disarming force_mode_changed" > to intel_psr_compute_config(), > can we move them to intel_psr_compute_config()? atomic check can fail at any point so we could disarm the mode_changed, fail, retry(because the return was EAGAIN) and then PSR will not be enabled. > > > if (!crtc_state->has_psr) > > return; > > > > @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp > > *intel_dp, > > struct i915_psr *psr = &dev_priv->psr; > > bool enable, psr2_enable; > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > + > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp) > > return; > > > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct > > drm_connector *connector, > > struct drm_crtc_state *crtc_state; > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > - dev_priv->psr.initially_probed) > > + !dev_priv->psr.force_mode_changed) > > return; > > > > intel_connector = to_intel_connector(connector); > > @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct > > drm_connector *connector, > > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > > new_state->crtc); > > crtc_state->mode_changed = true; > > - dev_priv->psr.initially_probed = true; > > +} > > + > > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > > bool set) > IMHO, it would be better intel_psr_set_force_mode_changed() as a > function name. Okay > > +{ > > + struct drm_i915_private *dev_priv; > > + > > + if (!intel_dp) > > + return; > > + > > + dev_priv = dp_to_i915(intel_dp); > > + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || > > + intel_dp != dev_priv->psr.dp) > > + return; > > + > > + dev_priv->psr.force_mode_changed = set; > > } > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > b/drivers/gpu/drm/i915/display/intel_psr.h > > index c58a1d438808..27a70468e2b9 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp > > *intel_dp); > > void intel_psr_atomic_check(struct drm_connector *connector, > > struct drm_connector_state *old_state, > > struct drm_connector_state *new_state); > > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > > bool set); > > > > #endif /* __INTEL_PSR_H__ */ > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index f7a1c33697b7..83791c197611 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -58,6 +58,7 @@ > > #include "display/intel_hotplug.h" > > #include "display/intel_overlay.h" > > #include "display/intel_pipe_crc.h" > > +#include "display/intel_psr.h" > > #include "display/intel_sprite.h" > > #include "display/intel_vga.h" > > > > @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct > > drm_i915_private *dev_priv) > > > > intel_audio_init(dev_priv); > > > > + intel_psr_force_mode_changed_set(dev_priv->psr.dp, true); > > + > > /* > > * Some ports require correctly set-up hpd registers for > > detection to > > * work properly (leading to ghost connected connector status), > > e.g. VGA > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 3330b538d379..a546655072bd 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -505,7 +505,7 @@ struct i915_psr { > > bool dc3co_enabled; > > u32 dc3co_exit_delay; > > struct delayed_work dc3co_work; > > - bool initially_probed; > > + bool force_mode_changed; > > }; > > > > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
On Thu, 2020-02-20 at 12:55 -0800, Souza, Jose wrote: > On Thu, 2020-02-20 at 12:39 +0000, Mun, Gwan-gyeong wrote: > > On Tue, 2020-02-18 at 12:39 -0800, José Roberto de Souza wrote: > > > Commit 60c6a14b489b ("drm/i915/display: Force the state compute > > > phase > > > once to enable PSR") was forcing the state compute too earlier > > > causing errors because not everything was initialized, so here > > > moving to i915_driver_register() when everything is ready and > > > driver > > > is registering into the rest of the system. > > > > > > Also fixing the place where it disarm the force probe as during > > > the > > > atomic check phase errors could happen like the ones due locking > > > and > > > it would cause PSR to never be enabled if that happens. > > > Leaving the disarm to the atomic commit phase, intel_psr_enable() > > > or > > > intel_psr_update() will be called even if the current state do > > > not > > > allow PSR to be enabled. > > > > > > v2: Check if intel_dp is null in > > > intel_psr_force_mode_changed_set() > > > v3: Check intel_dp before get dev_priv > > > > > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute > > > phase > > > once to enable PSR") > > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 > > > Tested-by: Ross Zwisler <zwisler@google.com> > > > Reported-by: Ross Zwisler <zwisler@google.com> > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 22 > > > ++++++++++++++++++++-- > > > drivers/gpu/drm/i915/display/intel_psr.h | 1 + > > > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > 4 files changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > index b4942b6445ae..2a0f7354fba5 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp > > > *intel_dp, > > > { > > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > + > > Hi, > > intel_psr_enable() and intel_psr_update already have checking > > routine > > for CAN_PSR and has_psr. > > therefore we don't need to check twice here. > > Minor overhead but if you really want I can remove the function call > and just do a dev_priv->psr.force_mode_changed = false; for > intel_psr_enable/update > > > And if there are no issues that moving "disarming > > force_mode_changed" > > to intel_psr_compute_config(), > > can we move them to intel_psr_compute_config()? > > atomic check can fail at any point so we could disarm the > mode_changed, > fail, retry(because the return was EAGAIN) and then PSR will not be > enabled. > If disarming the "force_mode_changed" would be handled on intel_psr_compute_config(), (after failing atomic check and)the retry step will set "crtc_state- >mode_changed = true" on intel_digital_connector_atomic_check(). ( because the force_mode_changed is not disabled yet.) The mode_changed will lead "encoder->compute_config" which will call intel_psr_compute_config(). And we can disable "force_mode_changed" on intel_psr_compute_config() which sets "crtc_state->has_psr = true". the "crtc_state->has_psr" enables PSR. > > > if (!crtc_state->has_psr) > > > return; > > > > > > @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp > > > *intel_dp, > > > struct i915_psr *psr = &dev_priv->psr; > > > bool enable, psr2_enable; > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > + > > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp) > > > return; > > > > > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct > > > drm_connector *connector, > > > struct drm_crtc_state *crtc_state; > > > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > > - dev_priv->psr.initially_probed) > > > + !dev_priv->psr.force_mode_changed) > > > return; > > > > > > intel_connector = to_intel_connector(connector); > > > @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct > > > drm_connector *connector, > > > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > > > new_state->crtc); > > > crtc_state->mode_changed = true; > > > - dev_priv->psr.initially_probed = true; > > > +} > > > + > > > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > > > bool set) > > IMHO, it would be better intel_psr_set_force_mode_changed() as a > > function name. > > Okay > > > > +{ > > > + struct drm_i915_private *dev_priv; > > > + > > > + if (!intel_dp) > > > + return; > > > + > > > + dev_priv = dp_to_i915(intel_dp); > > > + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || > > > + intel_dp != dev_priv->psr.dp) > > > + return; > > > + > > > + dev_priv->psr.force_mode_changed = set; > > > } > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > > b/drivers/gpu/drm/i915/display/intel_psr.h > > > index c58a1d438808..27a70468e2b9 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > > @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp > > > *intel_dp); > > > void intel_psr_atomic_check(struct drm_connector *connector, > > > struct drm_connector_state *old_state, > > > struct drm_connector_state *new_state); > > > +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, > > > bool set); > > > > > > #endif /* __INTEL_PSR_H__ */ > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > b/drivers/gpu/drm/i915/i915_drv.c > > > index f7a1c33697b7..83791c197611 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -58,6 +58,7 @@ > > > #include "display/intel_hotplug.h" > > > #include "display/intel_overlay.h" > > > #include "display/intel_pipe_crc.h" > > > +#include "display/intel_psr.h" > > > #include "display/intel_sprite.h" > > > #include "display/intel_vga.h" > > > > > > @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct > > > drm_i915_private *dev_priv) > > > > > > intel_audio_init(dev_priv); > > > > > > + intel_psr_force_mode_changed_set(dev_priv->psr.dp, true); > > > + > > > /* > > > * Some ports require correctly set-up hpd registers for > > > detection to > > > * work properly (leading to ghost connected connector status), > > > e.g. VGA > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 3330b538d379..a546655072bd 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -505,7 +505,7 @@ struct i915_psr { > > > bool dc3co_enabled; > > > u32 dc3co_exit_delay; > > > struct delayed_work dc3co_work; > > > - bool initially_probed; > > > + bool force_mode_changed; > > > }; > > > > > > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
On Fri, 2020-02-21 at 15:46 +0000, Mun, Gwan-gyeong wrote: > On Thu, 2020-02-20 at 12:55 -0800, Souza, Jose wrote: > > On Thu, 2020-02-20 at 12:39 +0000, Mun, Gwan-gyeong wrote: > > > On Tue, 2020-02-18 at 12:39 -0800, José Roberto de Souza wrote: > > > > Commit 60c6a14b489b ("drm/i915/display: Force the state compute > > > > phase > > > > once to enable PSR") was forcing the state compute too earlier > > > > causing errors because not everything was initialized, so here > > > > moving to i915_driver_register() when everything is ready and > > > > driver > > > > is registering into the rest of the system. > > > > > > > > Also fixing the place where it disarm the force probe as during > > > > the > > > > atomic check phase errors could happen like the ones due > > > > locking > > > > and > > > > it would cause PSR to never be enabled if that happens. > > > > Leaving the disarm to the atomic commit phase, > > > > intel_psr_enable() > > > > or > > > > intel_psr_update() will be called even if the current state do > > > > not > > > > allow PSR to be enabled. > > > > > > > > v2: Check if intel_dp is null in > > > > intel_psr_force_mode_changed_set() > > > > v3: Check intel_dp before get dev_priv > > > > > > > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute > > > > phase > > > > once to enable PSR") > > > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 > > > > Tested-by: Ross Zwisler <zwisler@google.com> > > > > Reported-by: Ross Zwisler <zwisler@google.com> > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_psr.c | 22 > > > > ++++++++++++++++++++-- > > > > drivers/gpu/drm/i915/display/intel_psr.h | 1 + > > > > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > 4 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > index b4942b6445ae..2a0f7354fba5 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp > > > > *intel_dp, > > > > { > > > > struct drm_i915_private *dev_priv = > > > > dp_to_i915(intel_dp); > > > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > > + > > > Hi, > > > intel_psr_enable() and intel_psr_update already have checking > > > routine > > > for CAN_PSR and has_psr. > > > therefore we don't need to check twice here. > > > > Minor overhead but if you really want I can remove the function > > call > > and just do a dev_priv->psr.force_mode_changed = false; for > > intel_psr_enable/update > > > > > And if there are no issues that moving "disarming > > > force_mode_changed" > > > to intel_psr_compute_config(), > > > can we move them to intel_psr_compute_config()? > > > > atomic check can fail at any point so we could disarm the > > mode_changed, > > fail, retry(because the return was EAGAIN) and then PSR will not be > > enabled. > > > If disarming the "force_mode_changed" would be handled on > intel_psr_compute_config(), > (after failing atomic check and)the retry step will set "crtc_state- > > mode_changed = true" on > intel_digital_connector_atomic_check(). ( because the > force_mode_changed is not disabled yet.) > > The mode_changed will lead "encoder->compute_config" which will call > intel_psr_compute_config(). > And we can disable "force_mode_changed" on intel_psr_compute_config() > which sets "crtc_state->has_psr = true". > the "crtc_state->has_psr" enables PSR. After call encoder->compute_config()->intel_psr_compute_config() there is a lot of code left to be executed in intel_atomic_check() that can cause the atomic check to fail. The next pipe in this loop can already cause that. > > > > > if (!crtc_state->has_psr) > > > > return; > > > > > > > > @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp > > > > *intel_dp, > > > > struct i915_psr *psr = &dev_priv->psr; > > > > bool enable, psr2_enable; > > > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > > + > > > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != > > > > intel_dp) > > > > return; > > > > > > > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct > > > > drm_connector *connector, > > > > struct drm_crtc_state *crtc_state; > > > > > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > > > - dev_priv->psr.initially_probed) > > > > + !dev_priv->psr.force_mode_changed) > > > > return; > > > > > > > > intel_connector = to_intel_connector(connector); > > > > @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct > > > > drm_connector *connector, > > > > crtc_state = drm_atomic_get_new_crtc_state(new_state- > > > > >state, > > > > new_state- > > > > >crtc); > > > > crtc_state->mode_changed = true; > > > > - dev_priv->psr.initially_probed = true; > > > > +} > > > > + > > > > +void intel_psr_force_mode_changed_set(struct intel_dp > > > > *intel_dp, > > > > bool set) > > > IMHO, it would be better intel_psr_set_force_mode_changed() as a > > > function name. > > > > Okay > > > > > > +{ > > > > + struct drm_i915_private *dev_priv; > > > > + > > > > + if (!intel_dp) > > > > + return; > > > > + > > > > + dev_priv = dp_to_i915(intel_dp); > > > > + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || > > > > + intel_dp != dev_priv->psr.dp) > > > > + return; > > > > + > > > > + dev_priv->psr.force_mode_changed = set; > > > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > > > b/drivers/gpu/drm/i915/display/intel_psr.h > > > > index c58a1d438808..27a70468e2b9 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > > > @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp > > > > *intel_dp); > > > > void intel_psr_atomic_check(struct drm_connector *connector, > > > > struct drm_connector_state > > > > *old_state, > > > > struct drm_connector_state > > > > *new_state); > > > > +void intel_psr_force_mode_changed_set(struct intel_dp > > > > *intel_dp, > > > > bool set); > > > > > > > > #endif /* __INTEL_PSR_H__ */ > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > index f7a1c33697b7..83791c197611 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > @@ -58,6 +58,7 @@ > > > > #include "display/intel_hotplug.h" > > > > #include "display/intel_overlay.h" > > > > #include "display/intel_pipe_crc.h" > > > > +#include "display/intel_psr.h" > > > > #include "display/intel_sprite.h" > > > > #include "display/intel_vga.h" > > > > > > > > @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct > > > > drm_i915_private *dev_priv) > > > > > > > > intel_audio_init(dev_priv); > > > > > > > > + intel_psr_force_mode_changed_set(dev_priv->psr.dp, > > > > true); > > > > + > > > > /* > > > > * Some ports require correctly set-up hpd registers > > > > for > > > > detection to > > > > * work properly (leading to ghost connected connector > > > > status), > > > > e.g. VGA > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 3330b538d379..a546655072bd 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -505,7 +505,7 @@ struct i915_psr { > > > > bool dc3co_enabled; > > > > u32 dc3co_exit_delay; > > > > struct delayed_work dc3co_work; > > > > - bool initially_probed; > > > > + bool force_mode_changed; > > > > }; > > > > > > > > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
On Fri, 2020-02-21 at 10:15 -0800, Souza, Jose wrote: > On Fri, 2020-02-21 at 15:46 +0000, Mun, Gwan-gyeong wrote: > > On Thu, 2020-02-20 at 12:55 -0800, Souza, Jose wrote: > > > On Thu, 2020-02-20 at 12:39 +0000, Mun, Gwan-gyeong wrote: > > > > On Tue, 2020-02-18 at 12:39 -0800, José Roberto de Souza wrote: > > > > > Commit 60c6a14b489b ("drm/i915/display: Force the state > > > > > compute > > > > > phase > > > > > once to enable PSR") was forcing the state compute too > > > > > earlier > > > > > causing errors because not everything was initialized, so > > > > > here > > > > > moving to i915_driver_register() when everything is ready and > > > > > driver > > > > > is registering into the rest of the system. > > > > > > > > > > Also fixing the place where it disarm the force probe as > > > > > during > > > > > the > > > > > atomic check phase errors could happen like the ones due > > > > > locking > > > > > and > > > > > it would cause PSR to never be enabled if that happens. > > > > > Leaving the disarm to the atomic commit phase, > > > > > intel_psr_enable() > > > > > or > > > > > intel_psr_update() will be called even if the current state > > > > > do > > > > > not > > > > > allow PSR to be enabled. > > > > > > > > > > v2: Check if intel_dp is null in > > > > > intel_psr_force_mode_changed_set() > > > > > v3: Check intel_dp before get dev_priv > > > > > > > > > > Fixes: 60c6a14b489b ("drm/i915/display: Force the state > > > > > compute > > > > > phase > > > > > once to enable PSR") > > > > > Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 > > > > > Tested-by: Ross Zwisler <zwisler@google.com> > > > > > Reported-by: Ross Zwisler <zwisler@google.com> > > > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_psr.c | 22 > > > > > ++++++++++++++++++++-- > > > > > drivers/gpu/drm/i915/display/intel_psr.h | 1 + > > > > > drivers/gpu/drm/i915/i915_drv.c | 3 +++ > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > > > > 4 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > index b4942b6445ae..2a0f7354fba5 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > > > @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp > > > > > *intel_dp, > > > > > { > > > > > struct drm_i915_private *dev_priv = > > > > > dp_to_i915(intel_dp); > > > > > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > > > + > > > > Hi, > > > > intel_psr_enable() and intel_psr_update already have checking > > > > routine > > > > for CAN_PSR and has_psr. > > > > therefore we don't need to check twice here. > > > > > > Minor overhead but if you really want I can remove the function > > > call > > > and just do a dev_priv->psr.force_mode_changed = false; for > > > intel_psr_enable/update > > > > > > > And if there are no issues that moving "disarming > > > > force_mode_changed" > > > > to intel_psr_compute_config(), > > > > can we move them to intel_psr_compute_config()? > > > > > > atomic check can fail at any point so we could disarm the > > > mode_changed, > > > fail, retry(because the return was EAGAIN) and then PSR will not > > > be > > > enabled. > > > > > If disarming the "force_mode_changed" would be handled on > > intel_psr_compute_config(), > > (after failing atomic check and)the retry step will set > > "crtc_state- > > > mode_changed = true" on > > intel_digital_connector_atomic_check(). ( because the > > force_mode_changed is not disabled yet.) > > > > The mode_changed will lead "encoder->compute_config" which will > > call > > intel_psr_compute_config(). > > And we can disable "force_mode_changed" on > > intel_psr_compute_config() > > which sets "crtc_state->has_psr = true". > > the "crtc_state->has_psr" enables PSR. > > After call encoder->compute_config()->intel_psr_compute_config() > there > is a lot of code left to be executed in intel_atomic_check() that can > cause the atomic check to fail. > The next pipe in this loop can already cause that. Hi Jose, Thank you for explaining in detail. > > > > > > if (!crtc_state->has_psr) > > > > > return; > > > > > > > > > > @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp > > > > > *intel_dp, > > > > > struct i915_psr *psr = &dev_priv->psr; > > > > > bool enable, psr2_enable; > > > > > > > > > > + intel_psr_force_mode_changed_set(intel_dp, false); > > > > > + > > > > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != > > > > > intel_dp) > > > > > return; > > > > > > > > > > @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct > > > > > drm_connector *connector, > > > > > struct drm_crtc_state *crtc_state; > > > > > > > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > > > > - dev_priv->psr.initially_probed) > > > > > + !dev_priv->psr.force_mode_changed) > > > > > return; > > > > > > > > > > intel_connector = to_intel_connector(connector); > > > > > @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct > > > > > drm_connector *connector, > > > > > crtc_state = drm_atomic_get_new_crtc_state(new_state- > > > > > > state, > > > > > new_state- > > > > > > crtc); > > > > > crtc_state->mode_changed = true; > > > > > - dev_priv->psr.initially_probed = true; > > > > > +} > > > > > + > > > > > +void intel_psr_force_mode_changed_set(struct intel_dp > > > > > *intel_dp, > > > > > bool set) > > > > IMHO, it would be better intel_psr_set_force_mode_changed() as > > > > a > > > > function name. > > > > > > Okay > > > > > > > > +{ > > > > > + struct drm_i915_private *dev_priv; > > > > > + > > > > > + if (!intel_dp) > > > > > + return; > > > > > + > > > > > + dev_priv = dp_to_i915(intel_dp); > > > > > + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || > > > > > + intel_dp != dev_priv->psr.dp) > > > > > + return; > > > > > + > > > > > + dev_priv->psr.force_mode_changed = set; > > > > > } > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h > > > > > b/drivers/gpu/drm/i915/display/intel_psr.h > > > > > index c58a1d438808..27a70468e2b9 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_psr.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.h > > > > > @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp > > > > > *intel_dp); > > > > > void intel_psr_atomic_check(struct drm_connector *connector, > > > > > struct drm_connector_state > > > > > *old_state, > > > > > struct drm_connector_state > > > > > *new_state); > > > > > +void intel_psr_force_mode_changed_set(struct intel_dp > > > > > *intel_dp, > > > > > bool set); > > > > > > > > > > #endif /* __INTEL_PSR_H__ */ > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > > > > b/drivers/gpu/drm/i915/i915_drv.c > > > > > index f7a1c33697b7..83791c197611 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > > > @@ -58,6 +58,7 @@ > > > > > #include "display/intel_hotplug.h" > > > > > #include "display/intel_overlay.h" > > > > > #include "display/intel_pipe_crc.h" > > > > > +#include "display/intel_psr.h" > > > > > #include "display/intel_sprite.h" > > > > > #include "display/intel_vga.h" > > > > > > > > > > @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct > > > > > drm_i915_private *dev_priv) > > > > > > > > > > intel_audio_init(dev_priv); > > > > > > > > > > + intel_psr_force_mode_changed_set(dev_priv->psr.dp, > > > > > true); > > > > > + > > > > > /* > > > > > * Some ports require correctly set-up hpd registers > > > > > for > > > > > detection to > > > > > * work properly (leading to ghost connected connector > > > > > status), > > > > > e.g. VGA > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > > index 3330b538d379..a546655072bd 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > > @@ -505,7 +505,7 @@ struct i915_psr { > > > > > bool dc3co_enabled; > > > > > u32 dc3co_exit_delay; > > > > > struct delayed_work dc3co_work; > > > > > - bool initially_probed; > > > > > + bool force_mode_changed; > > > > > }; > > > > > > > > > > #define QUIRK_LVDS_SSC_DISABLE (1<<1)
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index b4942b6445ae..2a0f7354fba5 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -936,6 +936,8 @@ void intel_psr_enable(struct intel_dp *intel_dp, { struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + intel_psr_force_mode_changed_set(intel_dp, false); + if (!crtc_state->has_psr) return; @@ -1096,6 +1098,8 @@ void intel_psr_update(struct intel_dp *intel_dp, struct i915_psr *psr = &dev_priv->psr; bool enable, psr2_enable; + intel_psr_force_mode_changed_set(intel_dp, false); + if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp) return; @@ -1629,7 +1633,7 @@ void intel_psr_atomic_check(struct drm_connector *connector, struct drm_crtc_state *crtc_state; if (!CAN_PSR(dev_priv) || !new_state->crtc || - dev_priv->psr.initially_probed) + !dev_priv->psr.force_mode_changed) return; intel_connector = to_intel_connector(connector); @@ -1640,5 +1644,19 @@ void intel_psr_atomic_check(struct drm_connector *connector, crtc_state = drm_atomic_get_new_crtc_state(new_state->state, new_state->crtc); crtc_state->mode_changed = true; - dev_priv->psr.initially_probed = true; +} + +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, bool set) +{ + struct drm_i915_private *dev_priv; + + if (!intel_dp) + return; + + dev_priv = dp_to_i915(intel_dp); + if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp) || + intel_dp != dev_priv->psr.dp) + return; + + dev_priv->psr.force_mode_changed = set; } diff --git a/drivers/gpu/drm/i915/display/intel_psr.h b/drivers/gpu/drm/i915/display/intel_psr.h index c58a1d438808..27a70468e2b9 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.h +++ b/drivers/gpu/drm/i915/display/intel_psr.h @@ -40,5 +40,6 @@ bool intel_psr_enabled(struct intel_dp *intel_dp); void intel_psr_atomic_check(struct drm_connector *connector, struct drm_connector_state *old_state, struct drm_connector_state *new_state); +void intel_psr_force_mode_changed_set(struct intel_dp *intel_dp, bool set); #endif /* __INTEL_PSR_H__ */ diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index f7a1c33697b7..83791c197611 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -58,6 +58,7 @@ #include "display/intel_hotplug.h" #include "display/intel_overlay.h" #include "display/intel_pipe_crc.h" +#include "display/intel_psr.h" #include "display/intel_sprite.h" #include "display/intel_vga.h" @@ -1256,6 +1257,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) intel_audio_init(dev_priv); + intel_psr_force_mode_changed_set(dev_priv->psr.dp, true); + /* * Some ports require correctly set-up hpd registers for detection to * work properly (leading to ghost connected connector status), e.g. VGA diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3330b538d379..a546655072bd 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -505,7 +505,7 @@ struct i915_psr { bool dc3co_enabled; u32 dc3co_exit_delay; struct delayed_work dc3co_work; - bool initially_probed; + bool force_mode_changed; }; #define QUIRK_LVDS_SSC_DISABLE (1<<1)
Commit 60c6a14b489b ("drm/i915/display: Force the state compute phase once to enable PSR") was forcing the state compute too earlier causing errors because not everything was initialized, so here moving to i915_driver_register() when everything is ready and driver is registering into the rest of the system. Also fixing the place where it disarm the force probe as during the atomic check phase errors could happen like the ones due locking and it would cause PSR to never be enabled if that happens. Leaving the disarm to the atomic commit phase, intel_psr_enable() or intel_psr_update() will be called even if the current state do not allow PSR to be enabled. v2: Check if intel_dp is null in intel_psr_force_mode_changed_set() v3: Check intel_dp before get dev_priv Fixes: 60c6a14b489b ("drm/i915/display: Force the state compute phase once to enable PSR") Closes: https://gitlab.freedesktop.org/drm/intel/issues/1151 Tested-by: Ross Zwisler <zwisler@google.com> Reported-by: Ross Zwisler <zwisler@google.com> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 22 ++++++++++++++++++++-- drivers/gpu/drm/i915/display/intel_psr.h | 1 + drivers/gpu/drm/i915/i915_drv.c | 3 +++ drivers/gpu/drm/i915/i915_drv.h | 2 +- 4 files changed, 25 insertions(+), 3 deletions(-)