Re: [Intel-gfx] [PATCH v3] drm/i915/psr: Force PSR probe only after full initialization

["Souza, Jose" <jose.souza@intel.com>]

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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx