On Fri, 2020-02-21 at 12:11 -0800, Souza, Jose wrote:
> On Fri, 2020-02-21 at 20:04 +0000, Mun, Gwan-gyeong wrote:
> > On Thu, 2020-02-20 at 15:15 -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 the end of i915_driver_modeset_probe() when the display
> > > is
> > > all initialized.
> > >
> > > 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
> > > v4:
> > > - renamed intel_psr_force_mode_changed_set() to
> > > intel_psr_set_force_mode_changed()
> > > - removed the set parameter from
> > > intel_psr_set_force_mode_changed()
> > > - not calling intel_psr_set_force_mode_changed() from
> > > intel_psr_enable/update(), directly setting it after the same
> > > checks
> > > that intel_psr_set_force_mode_changed() does
> > > - moved intel_psr_set_force_mode_changed() arm call to
> > > i915_driver_modeset_probe() as it is a better for a PSR call, all
> > > the
> > > functions calls happening between the old and the new function
> > > call
> > > will cause issue
> > >
> > > 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@xxxxxxxxxx>
> > > Reported-by: Ross Zwisler <zwisler@xxxxxxxxxx>
> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Cc: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_psr.c | 25
> > > ++++++++++++++++++++
> > > ----
> > > 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, 26 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index b4942b6445ae..7e754201f54d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -936,10 +936,12 @@ void intel_psr_enable(struct intel_dp
> > > *intel_dp,
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > >
> > > - if (!crtc_state->has_psr)
> > Enabling of crtc_state->has_psr is handled on
> > intel_psr_compute_config().
> > And the intel_psr_compute_config() checks "CAN_PSR(dev_priv) and
> > intel_dp != dev_priv->psr.dp".
> > therefore if we have this line "if (!crtc_state->has_psr)", we
> > don't
> > need to add "if (!CAN_PSR(dev_priv) || dev_priv->psr.dp !=
> > intel_dp)"
>
> crtc_state->has_psr will only be set to true if the state allows, we
> don't want to keep computing pipe configuration for every following
> page flip if PSR can not be enable(like PSR setup time do not fit in
> vblank).
>
Yes, you are right. we also have to handle the case which can not
support PSR. (I missed those cases.)
> That is why it is unseting force_mode_changed even if crtc_state-
> > has_psr is false.
> > Except that, looks good to me.
> >
> > Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> >
> > > + if (!CAN_PSR(dev_priv) || dev_priv->psr.dp != intel_dp)
> > > return;
> > >
> > > - if (drm_WARN_ON(&dev_priv->drm, !CAN_PSR(dev_priv)))
> > > + dev_priv->psr.force_mode_changed = false;
> > > +
> > > + if (!crtc_state->has_psr)
> > > return;
> > >
> > > drm_WARN_ON(&dev_priv->drm, dev_priv->drrs.dp);
> > > @@ -1099,6 +1101,8 @@ void intel_psr_update(struct intel_dp
> > > *intel_dp,
> > > if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp)
> > > return;
> > >
> > > + dev_priv->psr.force_mode_changed = false;
> > > +
> > > mutex_lock(&dev_priv->psr.lock);
> > >
> > > enable = crtc_state->has_psr && psr_global_enabled(dev_priv);
> > > @@ -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,18 @@ 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_set_force_mode_changed(struct intel_dp *intel_dp)
> > > +{
> > > + struct drm_i915_private *dev_priv;
> > > +
> > > + if (!intel_dp)
> > > + return;
> > > +
> > > + dev_priv = dp_to_i915(intel_dp);
> > > + if (!CAN_PSR(dev_priv) || intel_dp != dev_priv->psr.dp)
> > > + return;
> > > +
> > > + dev_priv->psr.force_mode_changed = true;
> > > }
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> > > b/drivers/gpu/drm/i915/display/intel_psr.h
> > > index c58a1d438808..274fc6bb6221 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_set_force_mode_changed(struct intel_dp
> > > *intel_dp);
> > >
> > > #endif /* __INTEL_PSR_H__ */
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > b/drivers/gpu/drm/i915/i915_drv.c
> > > index 759d333448e1..066934327345 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"
> > >
> > > @@ -264,6 +265,8 @@ static int i915_driver_modeset_probe(struct
> > > drm_i915_private *i915)
> > >
> > > intel_init_ipc(i915);
> > >
> > > + intel_psr_set_force_mode_changed(i915->psr.dp);
> > > +
> > > return 0;
> > >
> > > cleanup_gem:
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4305ccc4c683..2b741b352ce0 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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
