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

[Souza, Jose]

On Thu, 2019-12-05 at 09:17 +0000, Mun, Gwan-gyeong wrote:
> 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 at gmail.com>
> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at 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.

All the DRM atomic check functions that we can implement needs to
return 0 for success or the error number, just keeping it consistent.

Also in future when we support multiple instances of PSR we might need
some locking mechanism that would make us need to return -EAGAIN.

> And can we move calling of this fuction to below of calling
> intel_hdcp_atomic_check(conn, old_state, new_state);?

The order here do not matter much, but if call it after the 'if
(!new_state->crtc)' I can delete 'if (!conn_state->crtc)' from 
intel_psr_atomic_check().

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

Thanks for catching this, yes no need.


> > +	if (!conn_state->crtc)
> > +		return 0;

planning to remove this one too.

Thanks for reviewing.

Please let me know if you have any other comments, will send another
version tomorrow.

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