Re: [Intel-gfx] [PATCH] drm/i915/display: Force the state compute phase once to enable PSR

["Mun, Gwan-gyeong" <gwan-gyeong.mun@intel.com>]

Hi,
On Mon, 2019-11-25 at 15:38 -0800, José Roberto de Souza wrote:
> Recent improvements in the state tracking in i915 caused PSR to not
> be
> enabled when reusing firmware/BIOS modeset, this is due to all
> initial
> commits returning ealier in intel_atomic_check() as needs_modeset()
> is always false.
> 
> To fix that here forcing the state compute phase in CRTC that is
> driving the eDP that supports PSR once. Enable or disable PSR do not
> require a fullmodeset, so user will still experience glitch free boot
> process plus the power savings that PSR brings.
> 
> It was tried to set mode_changed in intel_initial_commit() but at
> this point the connectors are not registered causing a crash when
> computing encoder state.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112253
> Reported-by: <s.zharkoff@gmail.com>
> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_atomic.c |  6 ++++
>  drivers/gpu/drm/i915/display/intel_psr.c    | 32
> +++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_psr.h    |  5 ++++
>  drivers/gpu/drm/i915/i915_drv.h             |  1 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> b/drivers/gpu/drm/i915/display/intel_atomic.c
> index fd0026fc3618..6b57488d9238 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> @@ -37,6 +37,7 @@
>  #include "intel_atomic.h"
>  #include "intel_display_types.h"
>  #include "intel_hdcp.h"
> +#include "intel_psr.h"
>  #include "intel_sprite.h"
>  
>  /**
> @@ -127,6 +128,7 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
>  	struct intel_digital_connector_state *old_conn_state =
>  		to_intel_digital_connector_state(old_state);
>  	struct drm_crtc_state *crtc_state;
> +	int ret;
>  
>  	intel_hdcp_atomic_check(conn, old_state, new_state);
>  
> @@ -149,6 +151,10 @@ int intel_digital_connector_atomic_check(struct
> drm_connector *conn,
>  			old_conn_state->base.hdr_output_metadata))
>  		crtc_state->mode_changed = true;
>  
> +	ret = intel_psr_atomic_check(conn, state);
Because intel_psr_atomic_check() returns always 0, IMHO we don't need
to use this checking block.
And can we move calling of this fuction to below of calling
intel_hdcp_atomic_check(conn, old_state, new_state);?
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> b/drivers/gpu/drm/i915/display/intel_psr.c
> index c1d133362b76..a1acae8d72f7 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1444,3 +1444,35 @@ bool intel_psr_enabled(struct intel_dp
> *intel_dp)
>  
>  	return ret;
>  }
> +
> +int
> +intel_psr_atomic_check(struct drm_connector *conn,
> +		       struct drm_atomic_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(conn->dev);
> +	struct drm_connector_state *conn_state;
> +	struct intel_digital_port *dig_port;
> +	struct intel_connector *intel_conn;
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (!CAN_PSR(dev_priv))
> +		return 0;
> +
> +	conn_state = drm_atomic_get_new_connector_state(state, conn);
should we get conn_state again?
> +	if (!conn_state->crtc)
> +		return 0;
> +
> +	intel_conn = to_intel_connector(conn);
> +	dig_port = enc_to_dig_port(&intel_conn->encoder->base);
> +	if (dev_priv->psr.dp != &dig_port->dp)
> +		return 0;
> +
> +	if (dev_priv->psr.initially_probed)
> +		return 0;
> +
> +	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state-
> >crtc);
> +	crtc_state->mode_changed = true;
> +	dev_priv->psr.initially_probed = true;
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.h
> b/drivers/gpu/drm/i915/display/intel_psr.h
> index 46e4de8b8cd5..b3535e5752a6 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.h
> +++ b/drivers/gpu/drm/i915/display/intel_psr.h
> @@ -6,6 +6,9 @@
>  #ifndef __INTEL_PSR_H__
>  #define __INTEL_PSR_H__
>  
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_connector.h>
> +
>  #include "intel_frontbuffer.h"
>  
>  struct drm_i915_private;
> @@ -35,5 +38,7 @@ void intel_psr_short_pulse(struct intel_dp
> *intel_dp);
>  int intel_psr_wait_for_idle(const struct intel_crtc_state
> *new_crtc_state,
>  			    u32 *out_value);
>  bool intel_psr_enabled(struct intel_dp *intel_dp);
> +int intel_psr_atomic_check(struct drm_connector *conn,
> +			   struct drm_atomic_state *state);
>  
>  #endif /* __INTEL_PSR_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index fdae5a919bc8..d834924ba7c3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -508,6 +508,7 @@ struct i915_psr {
>  	bool dc3co_enabled;
>  	u32 dc3co_exit_delay;
>  	struct delayed_work idle_work;
> +	bool initially_probed;
>  };
>  
>  #define QUIRK_LVDS_SSC_DISABLE (1<<1)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx