| Message ID | 20191125233826.161899-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/display: Force the state compute phase once to enable PSR | expand |
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)
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@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. 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)
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); + 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); + 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)
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(+)