| Message ID | 20201028210712.66475-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Force initial atomic check in all eDP panels | expand |
On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote: > After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR > atomic check") dig_port was not being used but while fixing it I > realized that would be better to mark all CRTCs that has a eDP > connector as needing to have their state computed. > The principal reason is that in future we will support PSR in > multiple panels. > And this is only forcing the state compute if no register change is > need our atomic handling will just ignore this CRTC + connector > during the atomic commit phase. > > Cc: Imre Deak <imre.deak@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > index 1576c3722d0b..b5441f0b5b58 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector, > struct drm_connector_state *new_state) > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > - struct intel_connector *intel_connector; Arg, compiler didn't warn for this. > - struct intel_digital_port *dig_port; > struct drm_crtc_state *crtc_state; > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > - !dev_priv->psr.force_mode_changed) > - return; > - > - intel_connector = to_intel_connector(connector); > - dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder)); > - if (dev_priv->psr.dp != &dig_port->dp) > + !dev_priv->psr.force_mode_changed || > + connector->connector_type != DRM_MODE_CONNECTOR_eDP) > return; Can't you simplify even more by an is_edp && CAN_PSR check in intel_dp_initial_fastset_check() instead of the psr.force_mode_changed logic? > > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > -- > 2.29.1 >
On Thu, 2020-10-29 at 18:12 +0200, Imre Deak wrote: > On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote: > > After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR > > atomic check") dig_port was not being used but while fixing it I > > realized that would be better to mark all CRTCs that has a eDP > > connector as needing to have their state computed. > > The principal reason is that in future we will support PSR in > > multiple panels. > > And this is only forcing the state compute if no register change is > > need our atomic handling will just ignore this CRTC + connector > > during the atomic commit phase. > > > > Cc: Imre Deak <imre.deak@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_psr.c | 10 ++-------- > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > index 1576c3722d0b..b5441f0b5b58 100644 > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector, > > struct drm_connector_state *new_state) > > { > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > - struct intel_connector *intel_connector; > > Arg, compiler didn't warn for this. > > > - struct intel_digital_port *dig_port; > > struct drm_crtc_state *crtc_state; > > > > > > > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > - !dev_priv->psr.force_mode_changed) > > - return; > > - > > - intel_connector = to_intel_connector(connector); > > - dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder)); > > - if (dev_priv->psr.dp != &dig_port->dp) > > + !dev_priv->psr.force_mode_changed || > > + connector->connector_type != DRM_MODE_CONNECTOR_eDP) > > return; > > Can't you simplify even more by an is_edp && CAN_PSR check in > intel_dp_initial_fastset_check() instead of the psr.force_mode_changed > logic? In the past we ha problems forcing a modeset from the initial commit but with this new callback I guess it was fixed. I sent a test to try-bot, if that work we simply and remove some more code around it. thanks for the headsup. > > > > > > > > > > > crtc_state = drm_atomic_get_new_crtc_state(new_state->state, > > -- > > 2.29.1 > >
On Fri, Oct 30, 2020 at 06:26:06PM +0200, Souza, Jose wrote: > On Thu, 2020-10-29 at 18:12 +0200, Imre Deak wrote: > > On Wed, Oct 28, 2020 at 02:07:12PM -0700, José Roberto de Souza wrote: > > > After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR > > > atomic check") dig_port was not being used but while fixing it I > > > realized that would be better to mark all CRTCs that has a eDP > > > connector as needing to have their state computed. > > > The principal reason is that in future we will support PSR in > > > multiple panels. > > > And this is only forcing the state compute if no register change is > > > need our atomic handling will just ignore this CRTC + connector > > > during the atomic commit phase. > > > > > > Cc: Imre Deak <imre.deak@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_psr.c | 10 ++-------- > > > 1 file changed, 2 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c > > > index 1576c3722d0b..b5441f0b5b58 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > > > @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector, > > > struct drm_connector_state *new_state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > > - struct intel_connector *intel_connector; > > > > Arg, compiler didn't warn for this. > > > > > - struct intel_digital_port *dig_port; > > > struct drm_crtc_state *crtc_state; > > > > > > > > > > > > > > > if (!CAN_PSR(dev_priv) || !new_state->crtc || > > > - !dev_priv->psr.force_mode_changed) > > > - return; > > > - > > > - intel_connector = to_intel_connector(connector); > > > - dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder)); > > > - if (dev_priv->psr.dp != &dig_port->dp) > > > + !dev_priv->psr.force_mode_changed || > > > + connector->connector_type != DRM_MODE_CONNECTOR_eDP) > > > return; > > > > Can't you simplify even more by an is_edp && CAN_PSR check in > > intel_dp_initial_fastset_check() instead of the psr.force_mode_changed > > logic? > > In the past we ha problems forcing a modeset from the initial commit > but with this new callback I guess it was fixed. Ok. Note that now a forced initial modeset can anyway happen due to other reasons (DSC, wrong DP link rate). Also imo the only way to guarantee enabling PSR (the purpose of psr.force_mode_changed afaiu) is to do it in the initial modeset, (no subsequent modeset is guaranteed). > I sent a test to try-bot, if that work we simply and remove some more > code around it. Ok. > thanks for the headsup. The fastset hook was just recently added, so thought it's better to have all similar modeset forcing at one place. Btw, if intel_pipe_config_compare() wouldn't show a mismatch for the PSR state (I suppose it doesn't) then intel_crtc_check_fastset() will reset mode_changed, so probably better to use connectors_changed. --Imre
On Fri, Oct 30, 2020 at 06:48:12PM +0200, Imre Deak wrote: > [...] > > Btw, if intel_pipe_config_compare() wouldn't show a mismatch for the > PSR state (I suppose it doesn't) then intel_crtc_check_fastset() will > reset mode_changed, so probably better to use connectors_changed. Ah, nvm, PSR will get enabled even during a fastset, so you don't want to force a full modeset -> mode_changed is ok. > > --Imre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c index 1576c3722d0b..b5441f0b5b58 100644 --- a/drivers/gpu/drm/i915/display/intel_psr.c +++ b/drivers/gpu/drm/i915/display/intel_psr.c @@ -1875,17 +1875,11 @@ void intel_psr_atomic_check(struct drm_connector *connector, struct drm_connector_state *new_state) { struct drm_i915_private *dev_priv = to_i915(connector->dev); - struct intel_connector *intel_connector; - struct intel_digital_port *dig_port; struct drm_crtc_state *crtc_state; if (!CAN_PSR(dev_priv) || !new_state->crtc || - !dev_priv->psr.force_mode_changed) - return; - - intel_connector = to_intel_connector(connector); - dig_port = enc_to_dig_port(to_intel_encoder(new_state->best_encoder)); - if (dev_priv->psr.dp != &dig_port->dp) + !dev_priv->psr.force_mode_changed || + connector->connector_type != DRM_MODE_CONNECTOR_eDP) return; crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
After commit 00e5deb5c4f5 ("drm/i915: Fix encoder lookup during PSR atomic check") dig_port was not being used but while fixing it I realized that would be better to mark all CRTCs that has a eDP connector as needing to have their state computed. The principal reason is that in future we will support PSR in multiple panels. And this is only forcing the state compute if no register change is need our atomic handling will just ignore this CRTC + connector during the atomic commit phase. Cc: Imre Deak <imre.deak@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/display/intel_psr.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)