| Message ID | 20171213005934.7010-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 12, 2017 at 04:59:34PM -0800, Dhinakaran Pandiyan wrote: > Since commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc > state"), we check whether PSR can be enabled or not in > psr_compute_config(). Given that the psr.source_ok field is supposed to > track this check, set the field in psr_compute_config() as well. NAK. compute_config() isn't allowed to change global state since the modeset can still fail later, or this might in fact be a TEST_ONLY operation. > > Now tests can distinguish between PSR not enabled due to unmet mode > requirements and something going wrong during commit. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index a1ad85fa5c1a..b59a956047ff 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -358,6 +358,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > &crtc_state->base.adjusted_mode; > int psr_setup_time; > > + dev_priv->psr.source_ok = false; > + > if (!HAS_PSR(dev_priv)) > return; > > @@ -420,7 +422,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > * caps during eDP detection. > */ > if (!dev_priv->psr.psr2_support) { > - crtc_state->has_psr = true; > + dev_priv->psr.source_ok = (crtc_state->has_psr = true); > return; > } > > @@ -440,7 +442,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > return; > } > > - crtc_state->has_psr = true; > + dev_priv->psr.source_ok = (crtc_state->has_psr = true); > crtc_state->has_psr2 = true; > } > > @@ -522,8 +524,6 @@ void intel_psr_enable(struct intel_dp *intel_dp, > } > > dev_priv->psr.psr2_support = crtc_state->has_psr2; > - dev_priv->psr.source_ok = true; > - > dev_priv->psr.busy_frontbuffer_bits = 0; > > dev_priv->psr.setup_vsc(intel_dp, crtc_state); > @@ -657,7 +657,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, > /* Disable PSR on Sink */ > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > - dev_priv->psr.enabled = NULL; > + dev_priv->psr.source_ok = (dev_priv->psr.enabled = NULL); > mutex_unlock(&dev_priv->psr.lock); > > cancel_delayed_work_sync(&dev_priv->psr.work); > -- > 2.11.0
On Thu, 2017-12-14 at 17:06 +0200, Ville Syrjälä wrote: > On Tue, Dec 12, 2017 at 04:59:34PM -0800, Dhinakaran Pandiyan wrote: > > Since commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc > > state"), we check whether PSR can be enabled or not in > > psr_compute_config(). Given that the psr.source_ok field is supposed to > > track this check, set the field in psr_compute_config() as well. > > NAK. compute_config() isn't allowed to change global state since the > modeset can still fail later, or this might in fact be a > TEST_ONLY operation. I thought of it, but the only purpose of this flag is in debugfs to indicate PSR requirements were met. It is not checked anywhere in the driver. Setting this flag during commit (in intel_psr_enable) is redundant because psr.enabled, exposed via debugfs, already provides that information. By moving this flag to where PSR conditions are checked, this could give a hint that something went wrong if PSR was not enabled when PSR conditions were met. Basically, I don't see any use for this flag the way it is set now. The other idea I was considering is killing this flag and exposing a no_fbc_reason type string. > > > > > Now tests can distinguish between PSR not enabled due to unmet mode > > requirements and something going wrong during commit. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index a1ad85fa5c1a..b59a956047ff 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -358,6 +358,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > &crtc_state->base.adjusted_mode; > > int psr_setup_time; > > > > + dev_priv->psr.source_ok = false; > > + > > if (!HAS_PSR(dev_priv)) > > return; > > > > @@ -420,7 +422,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > * caps during eDP detection. > > */ > > if (!dev_priv->psr.psr2_support) { > > - crtc_state->has_psr = true; > > + dev_priv->psr.source_ok = (crtc_state->has_psr = true); > > return; > > } > > > > @@ -440,7 +442,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > > return; > > } > > > > - crtc_state->has_psr = true; > > + dev_priv->psr.source_ok = (crtc_state->has_psr = true); > > crtc_state->has_psr2 = true; > > } > > > > @@ -522,8 +524,6 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > } > > > > dev_priv->psr.psr2_support = crtc_state->has_psr2; > > - dev_priv->psr.source_ok = true; > > - > > dev_priv->psr.busy_frontbuffer_bits = 0; > > > > dev_priv->psr.setup_vsc(intel_dp, crtc_state); > > @@ -657,7 +657,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > /* Disable PSR on Sink */ > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > - dev_priv->psr.enabled = NULL; > > + dev_priv->psr.source_ok = (dev_priv->psr.enabled = NULL); > > mutex_unlock(&dev_priv->psr.lock); > > > > cancel_delayed_work_sync(&dev_priv->psr.work); > > -- > > 2.11.0 >
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index a1ad85fa5c1a..b59a956047ff 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -358,6 +358,8 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, &crtc_state->base.adjusted_mode; int psr_setup_time; + dev_priv->psr.source_ok = false; + if (!HAS_PSR(dev_priv)) return; @@ -420,7 +422,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, * caps during eDP detection. */ if (!dev_priv->psr.psr2_support) { - crtc_state->has_psr = true; + dev_priv->psr.source_ok = (crtc_state->has_psr = true); return; } @@ -440,7 +442,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, return; } - crtc_state->has_psr = true; + dev_priv->psr.source_ok = (crtc_state->has_psr = true); crtc_state->has_psr2 = true; } @@ -522,8 +524,6 @@ void intel_psr_enable(struct intel_dp *intel_dp, } dev_priv->psr.psr2_support = crtc_state->has_psr2; - dev_priv->psr.source_ok = true; - dev_priv->psr.busy_frontbuffer_bits = 0; dev_priv->psr.setup_vsc(intel_dp, crtc_state); @@ -657,7 +657,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, /* Disable PSR on Sink */ drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); - dev_priv->psr.enabled = NULL; + dev_priv->psr.source_ok = (dev_priv->psr.enabled = NULL); mutex_unlock(&dev_priv->psr.lock); cancel_delayed_work_sync(&dev_priv->psr.work);
Since commit 4d90f2d507ab ("drm/i915: Start tracking PSR state in crtc state"), we check whether PSR can be enabled or not in psr_compute_config(). Given that the psr.source_ok field is supposed to track this check, set the field in psr_compute_config() as well. Now tests can distinguish between PSR not enabled due to unmet mode requirements and something going wrong during commit. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)