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

[Mun, Gwan-gyeong]

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 at google.com>
> Reported-by: Ross Zwisler <zwisler at google.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Anshuman Gupta <anshuman.gupta at intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> ---
>  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)"

Except that, looks good to me.

Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>

> +	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)